Align heterogeneous equality with CEL spec#903
Conversation
Runtime equality converted strings to numeric and boolean values in several scalar implementations, so expressions such as dyn(1) == dyn('1') could evaluate true and Val.equals could disagree with hash-based map key lookup.
Preserve heterogeneous numeric equality, but make non-numeric concrete type mismatches evaluate false. Update scalar equality tests and add interpreter regressions for dynamic string/numeric comparisons and mixed string/numeric map keys.
| .expr("{1u: 1.0, 2: 2.0, 3u: 3.0}[3.1]") | ||
| .err("no such key: double{3.1}"), | ||
| new TestCase(InterpreterTestCase.map_key_string_and_int_are_distinct) | ||
| .expr("{1: 'int', '1': 'string'}['1']") |
There was a problem hiding this comment.
do we have a test that covers trying to use two equal keys in the same map literal?
did this used to fail or had "last one wins" semantics?
There was a problem hiding this comment.
do we have a test that covers trying to use two equal keys in the same map literal?
Yes, for equal heterogeneous numeric keys (somewhere below .expr("{0: 1, 0u: 2}[0.0]"). For exact duplicate same-type keys, no.
did this used to fail or had "last one wins" semantics?
Honestly, I don't know.
| assertThat(uintOf(0).equal(doubleOf(0))).isSameAs(True); | ||
| assertThat(uintOf(0).equal(doubleOf(1))).isSameAs(False); | ||
| assertThat(uintOf(0).equal(stringOf("0"))).isSameAs(True); | ||
| assertThat(uintOf(0).equal(stringOf("0"))).isSameAs(False); |
There was a problem hiding this comment.
isnt it surprising that conformance tests did not cover cases like this?
(i am assuming all are passing, or does this change reduce the number of failing conformance tests?)
There was a problem hiding this comment.
Not really. I suspect, the conformance tests still use the "old" CEL-spec, where this was allowed.
| @Test | ||
| void doubleEqual() { | ||
| assertThat(doubleOf(0).equal(False)).matches(Err::isError); | ||
| assertThat(doubleOf(0).equal(False)).isSameAs(False); |
There was a problem hiding this comment.
i am no expert on the CEL spec, so just wondering what you think about:
Type-checking asserts that arguments to equality operators must be the same type. If the argument types differ, the type-checker will raise an error. However, if at least one argument is dynamically typed, the type-checker considers all arguments dynamic and defers type-agreement checks to the interpreter.
https://github.com/cel-expr/cel-spec/blob/master/doc/langdef.md#equality
would it be preferable to throw an error for equal on incompatible types?
or is this the "dynamically typed" scenario?
There was a problem hiding this comment.
The CEL-spec just say "those are not equal". Mean, that's the same as Java "foo".equals(1) == false
2 participants
Runtime equality converted strings to numeric and boolean values in several scalar implementations, so expressions such as dyn(1) == dyn('1') could evaluate true and Val.equals could disagree with hash-based map key lookup.
Preserve heterogeneous numeric equality, but make non-numeric concrete type mismatches evaluate false. Update scalar equality tests and add interpreter regressions for dynamic string/numeric comparisons and mixed string/numeric map keys.