Closed jitsedesmet closed 1 year ago
Interesting point, and a good test case (it should definitely be added, potentially disabled for now ;) )
It could be handled in the overloadTree, but I'm not sure. I don't think all SPARQL functions throw errors on non-lexicals, e.g. those on terms. Probably something is possible.
Fixing the wrong errors is the most important part, the codestyle is not that important IMO.
Well, we could add a check at the end of search. When no implementation is found we could check whether a non lexical argument was provided. If this is the case we don't throw the implementation_not_found error but throw the invalid_lexical_form instead? Does that sound correct to you?
and a good test case
Yes, I found this while buffing up some tests.
It's not entirely correct no. It can be the case that an implementation is not found but it doesn't matter that it's an invalid lexical form. E.g. if you pass an invalid integer to a function only accepting strings.
The only cases you can throw the error if there is an implementation found matching the given types, but one off the arguments is non-lexical.
So a bit more would have to change, but I think it's pretty doable.
Oh well, that means the way we handle invalid lexicals right now is also wrong. We should look into this :)
In a recent PR I stumbled upon this again, and we had a conversation about this. I reference this here too: https://github.com/comunica/sparqlee/pull/163#issuecomment-1442381246
I think being spec compliant will come at an efficiency cost. What the spec wants:
What we do:
The implementation we have is not correct. We could make it correct by either:
Alright, I just wrote all this text for nothing, since one if is not bad. And We should definitely do this. I'll just post this because writing it took me a while, woeps.
@wschella I was wondering whether we can handle the invalidLexical form error from with the overloadTree? Right now, the Builder enables this behavior from within its functions. This creates some tedious code, since every function added should provide all implementations of InvalidLexicalForm? This behavior makes some functions not function correctly, I think. For example,
"apple"^^xsd:integer + "0"^^xsd:integer
doesn't throw the right error. It throws implementation not found.