adobe / json-formula

Query language for JSON documents
http://opensource.adobe.com/json-formula/
Apache License 2.0
19 stars 8 forks source link

Comparison operator type coercion is not up to spec #119

Closed Eswcvlad closed 6 months ago

Eswcvlad commented 6 months ago

In the spec, the following is mentioned on type coercion with comparison operators:

The left-hand operand of ordering comparison operators (>, >=, <, <=) must be a string or number. Any other type shall be coerced to a number.

If the operands of an ordering comparison are different, they shall both be coerced to a number

Coercion is not always possible, and if so, an TypeError error shall be emitted.

But in the implementation, the following expressions just return false with a debug message, even though right hand expression cannot be converted to number:

This is covered in the tests as well.

Similar situation with strings. An invalid string should be coerced to 0, but both expressions return false (this time without a message):

JohnBrinkman commented 6 months ago

On this issue, I'm leaning toward updating the spec to state that if coercion for purposes of comparison fails, the comparison will return false. I worry that comparisons in the context of predicate filtering will fail unexpectedly.

Eswcvlad commented 6 months ago

Agreed. Makes more sense to treat it as NaN than having a hard error.

Though this doesn't cover the 3 vs. "A" case. By spec, it should correctly coerce "A" to 0 and compare them, but it fails. I've seen this behavior in some other functions as well...

I think this is related to the differences between the number coercion rules in the table and toNumber function behavior. toNumber in case of non-number strings returns null, while table rules suggest it being 0.

Based on this part of the spec, I am assuming the intent is for the default toNumber and coercion rules to be in sync. And then just use toNumber everywhere, so that if there is a custom one provided, then everything still works as expected.

At the moment I am writing a big test file with all the type coercion corner cases in function arguments looking at the spec only. Should help us catch such inconsistencies easier.

JohnBrinkman commented 6 months ago

We need the 3 > "A" to be treated as a failed coercion. I've updated the spec wording to:

If a comparison requires coercion, and the coercion is not possible (including the case where a non-numeric string is coerced to zero), the comparison will return false. e.g., {a: 12} < 12 and "12a" < 13 will each return false.