wschella / Sparqlee

Simple SPARQL expression evaluator library
0 stars 0 forks source link

Type info dissapearing at runtime allows invalid combination of operand types #4

Closed wschella closed 6 years ago

wschella commented 6 years ago

We should have some runtime check for operators, so that we can't compare operands of different types.

This could be done in the operator wrapping:

applyBin(left: Term, right: Term): Term {
        return new BooleanLiteral(left.gt(right));
}

or in the term's operator imlementation:

gt(other: NumericLiteral): boolean {
    return this.value > other.value;
}

Note: in the above example the NumericLiteral type dissapears at runtime, allowing any term to be passed to this function, instead of left.gt being using the default gt(other: Term) implementation that would error correctly.

We could allow these heterogeneous types, but it is not in the spec.

joachimvh commented 6 years ago

Only way I can see this happen is if Term/StringLiteral classes and you use instanceof checks to make sure the input is correct. Not sure how much of an extra delay this would cause though.

wschella commented 6 years ago

Thinking further about #7 made me realize we probably want to keep the original lexical value, and make the parsed value (such as the number) optional (we should at least be able to handle it being undefined). This would maybe incorporate nicely with having the specific Literal classes (such as NumericLiteral) have specifically named properties (such as numericValue: number). This would allow us to do:

gt(other: NumericLiteral): boolean {
    return this.numericValue > other.numericValue;
}

and then handle undefined's in the applyBin method. I'll experiment with this next week.

wschella commented 6 years ago

What we really want here is a way to determine to the lowest common ancestor/parent class. Couple of problems:

Since the class tree is quite small, and only some parts are relevant, we could manually check with some branches ('if instaceof x') and the likes, but this will be very ugly and might introduce severe overhead. The important part is that we need to be able to refer to specific implementations of functions, as well as inherited ones, so I think we will need distinct function names to make this work.

Note: I think it can be done elegantly (and efficient), but it'll require some more thinking.

wschella commented 6 years ago

Advantage is we can build this tree offline (since only limited amount of types are relevant), and thus create a map of (type, type) pairs to the relevant implementation, and add runtime we would only need to fetch the correct implementation by fetching it from this map with the terms types as key. I'm pretty confident this will work, and I'll try to implement this.

We might need different tables for different operators to keep things clean.

wschella commented 6 years ago

Should be fixed in #12, but needs to be tested decently. That's another issue though (#13).