From 73577f74cff4a73a6b5d41b220828a79f70c7f1e Mon Sep 17 00:00:00 2001 From: Pranit More Date: Tue, 21 Jul 2026 12:44:02 +0530 Subject: [PATCH 1/2] Fix unsound constant folding of x in [x] for NaN values Matching an identifier needle against a list element by name assumes the value is equal to itself, which is not true of a NaN double, so the optimizer rewrote expressions that evaluate to false into true. Limit the identifier match to the scalar types that cannot hold a NaN. --- cel/folding.go | 32 ++++++++++++++++++++++++++++++-- cel/folding_test.go | 45 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 2 deletions(-) diff --git a/cel/folding.go b/cel/folding.go index 97b0915d..26e2f108 100644 --- a/cel/folding.go +++ b/cel/folding.go @@ -222,7 +222,7 @@ func maybePruneBranches(ctx *OptimizerContext, expr ast.NavigableExpr) bool { return true } needle := args[0] - if (needle.Kind() == ast.LiteralKind || needle.Kind() == ast.IdentKind) && haystack.Kind() == ast.ListKind { + if (needle.Kind() == ast.LiteralKind || isSelfEqualIdent(needle)) && haystack.Kind() == ast.ListKind { needleIsLit := needle.Kind() == ast.LiteralKind needleLitVal := needle.AsLiteral() needleIdentVal := needle.AsIdent() @@ -595,7 +595,7 @@ func constantCallMatcher(e ast.NavigableExpr) bool { return true } needle := children[0] - if (needle.Kind() == ast.LiteralKind || needle.Kind() == ast.IdentKind) && haystack.Kind() == ast.ListKind { + if (needle.Kind() == ast.LiteralKind || isSelfEqualIdent(needle)) && haystack.Kind() == ast.ListKind { needleIsLit := needle.Kind() == ast.LiteralKind needleLitVal := needle.AsLiteral() needleIdentVal := needle.AsIdent() @@ -619,6 +619,34 @@ func constantCallMatcher(e ast.NavigableExpr) bool { return true } +// isSelfEqualIdent indicates whether the expression is an identifier whose static type +// guarantees that its runtime value is equal to itself. +// +// Matching an identifier against a list element by name only proves list membership when the +// value the name resolves to is self-equal. A double may be NaN, which is not equal to itself, +// and the dyn, aggregate, and abstract types may all hold a NaN at runtime, so the check is +// limited to the scalar types which cannot. +func isSelfEqualIdent(e ast.Expr) bool { + if e.Kind() != ast.IdentKind { + return false + } + nav, ok := e.(ast.NavigableExpr) + if !ok { + return false + } + t := nav.Type() + if t == nil { + return false + } + switch t.Kind() { + case types.BoolKind, types.BytesKind, types.DurationKind, types.IntKind, + types.NullTypeKind, types.StringKind, types.TimestampKind, types.UintKind: + return true + default: + return false + } +} + func isExprConstantOfKind(e ast.Expr, t *types.Type) bool { return e.Kind() == ast.LiteralKind && e.AsLiteral().Type() == t } diff --git a/cel/folding_test.go b/cel/folding_test.go index 896fead1..559b4086 100644 --- a/cel/folding_test.go +++ b/cel/folding_test.go @@ -16,6 +16,7 @@ package cel import ( "errors" + "math" "reflect" "sort" "testing" @@ -378,6 +379,10 @@ func TestConstantFoldingOptimizer(t *testing.T) { }, { expr: `x in [1, 2, x]`, + folded: `x in [1, 2, x]`, + }, + { + expr: `i in [1, 2, i]`, folded: `true`, }, { @@ -496,6 +501,7 @@ func TestConstantFoldingOptimizer(t *testing.T) { Types(&proto3pb.TestAllTypes{}), Variable("x", DynType), Variable("y", DynType), + Variable("i", IntType), // work around different package convention in piper vs github. // google.expr.proto3.test.ImportedGlobalEnum.IMPORT_BAZ Constant("c", IntType, types.Int(2)), @@ -549,6 +555,45 @@ func TestConstantFoldingOptimizer(t *testing.T) { } } +func TestConstantFoldingInListNaN(t *testing.T) { + env, err := NewEnv(Variable("x", DynType), Variable("d", DoubleType)) + if err != nil { + t.Fatalf("NewEnv() failed: %v", err) + } + folder, err := NewConstantFoldingOptimizer() + if err != nil { + t.Fatalf("NewConstantFoldingOptimizer() failed: %v", err) + } + opt, err := NewStaticOptimizer(folder) + if err != nil { + t.Fatalf("NewStaticOptimizer() failed: %v", err) + } + vars := map[string]any{"x": types.Double(math.NaN()), "d": math.NaN()} + for _, expr := range []string{`x in [1, 2, x]`, `d in [d]`} { + t.Run(expr, func(t *testing.T) { + checked, iss := env.Compile(expr) + if iss.Err() != nil { + t.Fatalf("Compile() failed: %v", iss.Err()) + } + optimized, iss := opt.Optimize(env, checked) + if iss.Err() != nil { + t.Fatalf("Optimize() generated an invalid AST: %v", iss.Err()) + } + prg, err := env.Program(optimized) + if err != nil { + t.Fatalf("Program() failed: %v", err) + } + out, _, err := prg.Eval(vars) + if err != nil { + t.Fatalf("Eval() failed: %v", err) + } + if out != types.False { + t.Errorf("got %v, wanted false since NaN is not equal to itself", out) + } + }) + } +} + func TestConstantFoldingCallsWithSideEffects(t *testing.T) { tests := []struct { expr string From 1544dde6df212bd14d09c3859c3b4873b41053d1 Mon Sep 17 00:00:00 2001 From: Pranit More Date: Wed, 22 Jul 2026 11:09:41 +0530 Subject: [PATCH 2/2] Allow type and self-equal aggregates in the in-list ident match --- cel/folding.go | 23 ++++++++-- cel/folding_test.go | 109 ++++++++++++++++++++++++++++++++++++-------- 2 files changed, 108 insertions(+), 24 deletions(-) diff --git a/cel/folding.go b/cel/folding.go index 26e2f108..820f1af8 100644 --- a/cel/folding.go +++ b/cel/folding.go @@ -624,8 +624,9 @@ func constantCallMatcher(e ast.NavigableExpr) bool { // // Matching an identifier against a list element by name only proves list membership when the // value the name resolves to is self-equal. A double may be NaN, which is not equal to itself, -// and the dyn, aggregate, and abstract types may all hold a NaN at runtime, so the check is -// limited to the scalar types which cannot. +// and dyn, abstract, and struct types may all hold a NaN at runtime, so the check is limited +// to the scalar types which cannot, and to the aggregate types whose type parameters are +// themselves self-equal. func isSelfEqualIdent(e ast.Expr) bool { if e.Kind() != ast.IdentKind { return false @@ -634,13 +635,27 @@ func isSelfEqualIdent(e ast.Expr) bool { if !ok { return false } - t := nav.Type() + return isSelfEqualType(nav.Type()) +} + +// isSelfEqualType indicates whether all runtime values of the given type are equal to themselves. +func isSelfEqualType(t *types.Type) bool { if t == nil { return false } switch t.Kind() { case types.BoolKind, types.BytesKind, types.DurationKind, types.IntKind, - types.NullTypeKind, types.StringKind, types.TimestampKind, types.UintKind: + types.NullTypeKind, types.StringKind, types.TimestampKind, types.TypeKind, + types.UintKind: + return true + case types.ListKind, types.MapKind: + // Aggregates compare element-wise, so they are self-equal exactly when their type + // parameters are. A list(dyn) or map(string, double) may still contain a NaN. + for _, p := range t.Parameters() { + if !isSelfEqualType(p) { + return false + } + } return true default: return false diff --git a/cel/folding_test.go b/cel/folding_test.go index 559b4086..f556c40e 100644 --- a/cel/folding_test.go +++ b/cel/folding_test.go @@ -381,10 +381,6 @@ func TestConstantFoldingOptimizer(t *testing.T) { expr: `x in [1, 2, x]`, folded: `x in [1, 2, x]`, }, - { - expr: `i in [1, 2, i]`, - folded: `true`, - }, { expr: `[1, 2].filter(x, x in [1, 2])`, folded: `[1, 2]`, @@ -501,7 +497,6 @@ func TestConstantFoldingOptimizer(t *testing.T) { Types(&proto3pb.TestAllTypes{}), Variable("x", DynType), Variable("y", DynType), - Variable("i", IntType), // work around different package convention in piper vs github. // google.expr.proto3.test.ImportedGlobalEnum.IMPORT_BAZ Constant("c", IntType, types.Int(2)), @@ -555,35 +550,109 @@ func TestConstantFoldingOptimizer(t *testing.T) { } } -func TestConstantFoldingInListNaN(t *testing.T) { - env, err := NewEnv(Variable("x", DynType), Variable("d", DoubleType)) - if err != nil { - t.Fatalf("NewEnv() failed: %v", err) - } - folder, err := NewConstantFoldingOptimizer() - if err != nil { - t.Fatalf("NewConstantFoldingOptimizer() failed: %v", err) +// TestConstantFoldingInListIdent checks which identifier needles may be matched against a list +// element by name. +// +// The rewrite is only sound when the identifier's static type guarantees that its runtime value +// is equal to itself. A NaN double is not, so any type which can carry a NaN must be left alone: +// the vars case of each such entry evaluates the optimized program with a NaN binding and expects +// the same result the unoptimized expression produces. +func TestConstantFoldingInListIdent(t *testing.T) { + nan := math.NaN() + tests := []struct { + expr string + folded string + // vars, when set, is evaluated against the optimized AST and must yield false. + vars map[string]any + }{ + // Scalar types which cannot hold a NaN. + {expr: `b in [b]`, folded: `true`}, + {expr: `by in [by]`, folded: `true`}, + {expr: `du in [du]`, folded: `true`}, + {expr: `i in [1, 2, i]`, folded: `true`}, + {expr: `n in [n]`, folded: `true`}, + {expr: `s in [s]`, folded: `true`}, + {expr: `ts in [ts]`, folded: `true`}, + {expr: `ty in [ty]`, folded: `true`}, + {expr: `u in [u]`, folded: `true`}, + // Aggregate types compare element-wise, so they are self-equal exactly when their type + // parameters are. + {expr: `li in [li]`, folded: `true`}, + {expr: `lli in [lli]`, folded: `true`}, + {expr: `msi in [msi]`, folded: `true`}, + {expr: `ld in [ld]`, folded: `ld in [ld]`, vars: map[string]any{"ld": []float64{nan}}}, + {expr: `lx in [lx]`, folded: `lx in [lx]`, vars: map[string]any{"lx": []any{nan}}}, + {expr: `msd in [msd]`, folded: `msd in [msd]`, vars: map[string]any{"msd": map[string]float64{"a": nan}}}, + // Doubles and dyn may be a NaN directly. + {expr: `d in [d]`, folded: `d in [d]`, vars: map[string]any{"d": nan}}, + {expr: `x in [1, 2, x]`, folded: `x in [1, 2, x]`, vars: map[string]any{"x": types.Double(nan)}}, + // Abstract and struct types are left alone as their contents are not inspected. + {expr: `oi in [oi]`, folded: `oi in [oi]`}, + {expr: `o in [o]`, folded: `o in [o]`}, + // Literal needles use CEL equality rather than a name match, so they are unaffected. + {expr: `1 in [1, 2]`, folded: `true`}, + {expr: `1.0 in [d, 1.0]`, folded: `true`}, } - opt, err := NewStaticOptimizer(folder) + env, err := NewEnv( + OptionalTypes(), + Types(&proto3pb.TestAllTypes{}), + Variable("b", BoolType), + Variable("by", BytesType), + Variable("du", DurationType), + Variable("i", IntType), + Variable("n", NullType), + Variable("s", StringType), + Variable("ts", TimestampType), + Variable("ty", TypeType), + Variable("u", UintType), + Variable("li", ListType(IntType)), + Variable("lli", ListType(ListType(IntType))), + Variable("msi", MapType(StringType, IntType)), + Variable("ld", ListType(DoubleType)), + Variable("lx", ListType(DynType)), + Variable("msd", MapType(StringType, DoubleType)), + Variable("d", DoubleType), + Variable("x", DynType), + Variable("oi", OptionalType(IntType)), + Variable("o", ObjectType("google.expr.proto3.test.TestAllTypes")), + ) if err != nil { - t.Fatalf("NewStaticOptimizer() failed: %v", err) + t.Fatalf("NewEnv() failed: %v", err) } - vars := map[string]any{"x": types.Double(math.NaN()), "d": math.NaN()} - for _, expr := range []string{`x in [1, 2, x]`, `d in [d]`} { - t.Run(expr, func(t *testing.T) { - checked, iss := env.Compile(expr) + for _, tst := range tests { + tc := tst + t.Run(tc.expr, func(t *testing.T) { + checked, iss := env.Compile(tc.expr) if iss.Err() != nil { t.Fatalf("Compile() failed: %v", iss.Err()) } + folder, err := NewConstantFoldingOptimizer() + if err != nil { + t.Fatalf("NewConstantFoldingOptimizer() failed: %v", err) + } + opt, err := NewStaticOptimizer(folder) + if err != nil { + t.Fatalf("NewStaticOptimizer() failed: %v", err) + } optimized, iss := opt.Optimize(env, checked) if iss.Err() != nil { t.Fatalf("Optimize() generated an invalid AST: %v", iss.Err()) } + folded, err := AstToString(optimized) + if err != nil { + t.Fatalf("AstToString() failed: %v", err) + } + if folded != tc.folded { + t.Errorf("got %q, wanted %q", folded, tc.folded) + } + if tc.vars == nil { + return + } prg, err := env.Program(optimized) if err != nil { t.Fatalf("Program() failed: %v", err) } - out, _, err := prg.Eval(vars) + out, _, err := prg.Eval(tc.vars) if err != nil { t.Fatalf("Eval() failed: %v", err) }