Allow heterogeneous int/uint/double comparisons (fixes #114) - #192
Open
benja0rtzzz wants to merge 1 commit into
Open
Allow heterogeneous int/uint/double comparisons (fixes #114)#192benja0rtzzz wants to merge 1 commit into
benja0rtzzz wants to merge 1 commit into
Conversation
…n#114) The CEL spec requires the six comparison operators to work across int, uint, and double in any operand order; arithmetic stays homogeneous. IntType guarded all six with type_matched, while UintType and DoubleType guarded only __eq__/__ne__, so the result depended on which operand came first. type_matched now compares the unwrapped Python values when both operands are numeric CEL types. It must not cast int to float: Python's mixed comparison is exact, while a float() cast would silently break integers above 2**53. BoolType and every other type still raise. Also removes the @wip tag, and its tools/tags.toml mirror, from the 94 conformance scenarios this makes pass, and corrects json_query.feature's `==` scenario, which recorded the old raising behavior as intended CEL semantics.
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Allow heterogeneous int/uint/double comparisons (fixes #114)
Problem
The six comparison operators fail on mixed numeric types, and whether they
fail depends on which operand you write first:
The CEL spec requires
==,!=,<,<=,>,>=to work acrossint,uint, anddoublein any combination and any operand order — seeNumeric Values
in the language definition, and
compareIntDouble/compareUintDoubleincel-go's
common/types. Arithmetic across those types stays homogeneous;only comparison is relaxed.
Cause.
IntTypedefines all six comparison dunders behind thetype_matchedguard.UintTypeandDoubleTypeguard only__eq__/__ne__and never override the four relational operators at all, so those inherit
Python's native
int/floatmethods, which already accept mixed operands.Python tries the left operand's method first, so:
4.0 < 10reaches plainfloat.__lt__— celpy's type system is bypassedentirely, and it works.
10 < 4.0reachesIntType.__lt__, which raises. Raising is not returningNotImplemented, so Python never gets to tryDoubleType's reflectedmethod — the answer was available and unreachable.
==/!=are guarded on all three types, so they fail in both orders.Fix
type_matchedgains one branch: when both operands are some combination ofIntType,UintType, andDoubleTypebut aren't otherwise type-matched, itcompares the unwrapped Python values with the matching
operator.{eq,ne,lt,le,gt,ge}instead of raising. Any other mismatch raisesexactly as before, with the same message.
Applying an operator to the wrapped
values re-enters the guard. Unwrapping reaches the native comparison
underneath.
The unwrap must not cast
inttofloat.IntType/UintTypeunwrap toint,DoubleTypetofloat, each to its natural type, and Python's mixedint/floatcomparison, which is exact by language definition. A pair offloat()casts would look equivalent andsilently break every integer above 2**53, where a
doublecan no longerrepresent consecutive integers. NaN, ±infinity and
-0.0come along for freefrom IEEE 754.
The exception is deliberately narrow.
BoolTypesubclassesintbut is adistinct CEL type, so it stays rejected; an
isinstance(other, int)checkwould have let it through.
Consequences of the
__eq__half,list membership (
1 in [1.0, 2.0]), list/map equality ({1: 1} == {1.0: 1.0}),and cross-type map-key lookup (
{1: 'a'}[1u]) now work. All three delegate to==. The map lookup was never a "key not found" — Python already guaranteesequal
int/floatvalues hash alike, so it found the right bucket and thenraised during the collision check.
Not changed: arithmetic (
IntType(1) + DoubleType(2.0)still raises);comparison strictness for strings, bytes, timestamps and durations;
MapType,ListType, and every__hash__, all byte-for-byte identical.IntType(10) < DoubleType(4.0)TypeErrorFalseIntType(2) == DoubleType(2.0)TypeErrorTrueUintType(1) < DoubleType(1.5)TypeErrorTrueIntType(2**53+1) == DoubleType(float(2**53))TypeErrorFalse— exactDoubleType(nan) == IntType(5)TypeErrorFalseDoubleType(-0.0) == IntType(0)TypeErrorTrueIntType(1) + DoubleType(2.0)TypeErrorTypeError— unchangedIntType(1) == BoolType(True)TypeErrorTypeError— unchangedTests
Ten new unit tests in
tests/test_celtypes.pycover the full int/uint/doublematrix in both operand orders, the issue's repro, and the 2**53 / NaN /
infinity /
-0.0edge cases. Two new scenarios infeatures/json_query.featurerun the issue's literal CLI repro end to end.
pytestbehave(native)behave(compiled)Verified on the whole envlist —
py310,py311,py312,py313,py314and
toolsall pass.mypy --strictandruff format --diffare clean. Notracked test changed status or regressed.
ruff checkfails, but it already fails onmain— see Notes.94
@wiptags removed across 6 feature files —comparisons.feature(80),lists.feature(6),fields.feature(4),proto2.feature(2),proto3.feature(2) — each with its mirror entry intools/tags.toml, in theorder that file already used. Per your request on the issue thread, so a
future CEL-spec extraction doesn't re-tag them. No other entry in those
sections was touched.
One expected output changed, in
json_query.feature'sJQ Conditionals and Comparisons: ==. It evaluates_ == 1against1,1.0,"1","banana". The1.0case printednullbecause the comparisonraised — and the scenario's own comment recorded that as intended CEL
semantics ("CEL does not do the required type coercions"). It wasn't; the spec
says
1 == 1.0is true. Expected output is nowtrue\ntrue\nnull\nnull\nandthe comment distinguishes the two claims: no string coercion (correct), but
heterogeneous numeric comparison per spec. The string cases are unaffected.
Reinforcement proof.
celtypes.pyalone was reverted to upstream with the scenarios left untagged, and the tracked suite re-run:1274 passed, 91 failed, 4 error. That's 95 tracked failures — exactly the 94
newly-untagged scenarios plus the
json_queryone.The fix was then restored and the suite re-confirmed green.
Left in place, disclosed
16 scenarios kept
@wip— pre-existing compiled-runner gap. They werealready failing before this change and still fail after; no status changed and
this PR claims none of them. They now fail differently, under the compiled
runner only, with
int() argument must be ... not 'list'. All 16 involvedyn()over a constructed wrapper-message literal (e.g.Int32Value{value: 34} == dyn(UInt64Value{value: 34u})). I haven't chased itdown, but the trigger seems to sit upstream of the comparison: something
reaches the unwrap as a numeric CEL type while carrying a list as its payload.
If that's right, the old guard was rejecting it for an unrelated reason and
never inspecting it, so this change surfaced it rather than caused it — which
would point at
dyn()over constructed messages rather thanceltypes.py.You'd know better than me; happy to open a separate issue if it's useful.
1 scenario kept
@wip— a question rather than a fix.lt_literal/not_lt_dyn_int_big_lossy_doubleisdyn(9223372036854775807) < 9223372036854775808.0, i.e.int64::MAX < 2**63.This returns
true, the exact answer; the fixture expectsfalse. That lookslike a cel-go artifact — its clamping compares against
float64(math.MaxInt64), and 2**63−1 isn't representable as adouble, soit rounds up to 2**63 and the comparison collapses to
2**63 < 2**63.So the open question is whether celpy should match cel-go here or stay
mathematically exact. Reproducing the artifact would mean reintroducing the
imprecision this patch otherwise avoids, so I left the scenario
@wipratherthan decide it.
Arithmetic asymmetry, out of scope.
DoubleType(2.0) + IntType(1)alreadyreturns a bare, unwrapped
floatrather than raising — the sameleft-operand-first asymmetry, on the arithmetic side, escaping the CEL type
system. Untouched here since arithmetic is meant to stay homogeneous.
Notes
tox -e lintfails onmaintoday, independently of this PR.ruffisunpinned (
ruff>=0.15.13), and 0.16.0 — released 2026-07-23, after the lastCI run on
mainon 2026-07-17 — widened the default rule set. Under thelocked 0.15.13 everything is clean. Under 0.16.1,
ruff check src toolsreports 476 errors on untouched
mainand 479 on this branch. The 3extra are
UP006/UP007on theDict/Unionannotations I added, matchingthe style
celtypes.pyalready uses throughout — I left them consistent withthe file rather than modernizing 3 lines out of ~476. Since
Testshasneeds: Lint, the matrix here will be skipped until this is settled. Happy topin ruff, change my 3, or leave it to a separate PR — whichever you prefer.
tools/tags.tomlwas hand-edited to mirror the.featurechanges; itround-trips through
tomllibandpytest toolspasses. Coverage stays at100%.