From 69a23d904abe75b11d388e045ccc3c6d777bd31f Mon Sep 17 00:00:00 2001 From: jduraniglesias <89482726+jduraniglesias@users.noreply.github.com> Date: Fri, 7 Jun 2024 18:37:18 +0000 Subject: [PATCH] Fixed partial variables extended bug and split PartialVarsEnvExtended test into two (#955) Fixed partial vars bug and split tests --------- Co-authored-by: jduraniglesias --- cel/cel_test.go | 73 ++++++++++++++++++++++++++++++++++ cel/env.go | 8 +--- repl/evaluator.go | 43 ++++++++++++++++---- repl/evaluator_test.go | 76 +++++++++++++++++++++++++++++++++++- repl/main/README.md | 4 ++ test/bench/bench.go | 84 ++++++++++++++++++++++++++++++++++++++++ test/bench/bench_test.go | 4 ++ 7 files changed, 278 insertions(+), 14 deletions(-) diff --git a/cel/cel_test.go b/cel/cel_test.go index 261f97be..e80a3288 100644 --- a/cel/cel_test.go +++ b/cel/cel_test.go @@ -1733,6 +1733,79 @@ func TestResidualAstAttributeQualifiers(t *testing.T) { } } +func TestPartialVarsEnv(t *testing.T) { + env := testEnv(t, + Variable("x", IntType), + Variable("y", IntType), + ) + + // Use env to make sure internals are all initialized. + ast, iss := env.Compile("x == y") + + if iss.Err() != nil { + t.Fatalf("env.Compile() failed: %v", iss.Err()) + } + prg, err := env.Program(ast, EvalOptions(OptPartialEval)) + if err != nil { + t.Fatalf("env.Program() failed: %v", err) + } + + act, err := env.PartialVars(map[string]any{"x": 1, "y": 1}) + + if err != nil { + t.Fatalf("env.PartialVars failed: %v", err) + } + val, _, err := prg.Eval(act) + if err != nil { + t.Fatalf("Eval failed: %v", err) + } + + if val != types.True { + t.Fatalf("want: %v, got: %v", types.True, val) + } +} + +func TestPartialVarsExtendedEnv(t *testing.T) { + env := testEnv(t, + Variable("x", IntType), + Variable("y", IntType), + ) + + env.Compile("x == y") + // Now test that a sub environment is correctly copied. + env2, err := env.Extend(Variable("z", IntType)) + if err != nil { + t.Fatalf("env.Extend failed: %v", err) + } + + ast, iss := env2.Compile("x == y && y == z") + + if iss.Err() != nil { + t.Fatalf("env.Compile() failed: %v", iss.Err()) + } + prg, err := env2.Program(ast, EvalOptions(OptPartialEval)) + if err != nil { + t.Fatalf("env.Program() failed: %v", err) + } + + act, err := env2.PartialVars(map[string]any{"z": 1, "y": 1}) + + if err != nil { + t.Fatalf("env.PartialVars failed: %v", err) + } + val, _, err := prg.Eval(act) + if err != nil { + t.Fatalf("Eval failed: %v", err) + } + if !types.IsUnknown(val) { + t.Fatalf("Wanted unknown, got %v", val) + } + + if !reflect.DeepEqual(val, types.NewUnknown(1, types.NewAttributeTrail("x"))) { + t.Fatalf("Wanted Unknown(x (1)), got: %v", val) + } +} + func TestResidualAstModified(t *testing.T) { env := testEnv(t, Variable("x", MapType(StringType, IntType)), diff --git a/cel/env.go b/cel/env.go index d20c98f0..504e5d3d 100644 --- a/cel/env.go +++ b/cel/env.go @@ -309,17 +309,13 @@ func (e *Env) Extend(opts ...EnvOption) (*Env, error) { copy(chkOptsCopy, e.chkOpts) // Copy the declarations if needed. - varsCopy := []*decls.VariableDecl{} if chk != nil { // If the type-checker has already been instantiated, then the e.declarations have been // validated within the chk instance. chkOptsCopy = append(chkOptsCopy, checker.ValidatedDeclarations(chk)) - } else { - // If the type-checker has not been instantiated, ensure the unvalidated declarations are - // provided to the extended Env instance. - varsCopy = make([]*decls.VariableDecl, len(e.variables)) - copy(varsCopy, e.variables) } + varsCopy := make([]*decls.VariableDecl, len(e.variables)) + copy(varsCopy, e.variables) // Copy macros and program options macsCopy := make([]parser.Macro, len(e.macros)) diff --git a/repl/evaluator.go b/repl/evaluator.go index 4e1aeb9d..4a9cc416 100644 --- a/repl/evaluator.go +++ b/repl/evaluator.go @@ -266,9 +266,10 @@ type Optioner interface { // EvaluationContext context for the repl. // Handles maintaining state for multiple let expressions. type EvaluationContext struct { - letVars []letVariable - letFns []letFunction - options []Optioner + letVars []letVariable + letFns []letFunction + options []Optioner + enablePartialEval bool } func (ctx *EvaluationContext) indexLetVar(name string) int { @@ -311,6 +312,7 @@ func (ctx *EvaluationContext) copy() *EvaluationContext { copy(cpy.letVars, ctx.letVars) cpy.letFns = make([]letFunction, len(ctx.letFns)) copy(cpy.letFns, ctx.letFns) + cpy.enablePartialEval = ctx.enablePartialEval return &cpy } @@ -419,12 +421,16 @@ func (ctx *EvaluationContext) addOption(opt Optioner) { // programOptions generates the program options for planning. // Assumes context has been planned. -func (ctx *EvaluationContext) programOptions() cel.ProgramOption { +func (ctx *EvaluationContext) programOptions() []cel.ProgramOption { var fns = make([]*functions.Overload, len(ctx.letFns)) for i, fn := range ctx.letFns { fns[i] = fn.generateFunction() } - return cel.Functions(fns...) + result := []cel.ProgramOption{cel.Functions(fns...)} + if ctx.enablePartialEval { + result = append(result, cel.EvalOptions(cel.OptPartialEval)) + } + return result } // Evaluator provides basic environment for evaluating an expression with @@ -489,7 +495,7 @@ func updateContextPlans(ctx *EvaluationContext, env *cel.Env) error { el.ast = ast el.resultType = ast.ResultType() - plan, err := env.Program(ast, ctx.programOptions()) + plan, err := env.Program(ast, ctx.programOptions()...) if err != nil { return err } @@ -579,6 +585,18 @@ func (e *Evaluator) AddOption(opt Optioner) error { return nil } +// EnablePartialEval enables the option to allow partial evaluations. +func (e *Evaluator) EnablePartialEval() error { + cpy := e.ctx.copy() + cpy.enablePartialEval = true + err := updateContextPlans(cpy, e.env) + if err != nil { + return err + } + e.ctx = *cpy + return nil +} + // DelLetVar removes a variable from the evaluation context. // If deleting the variable breaks a later expression, this function will return an error without modifying the context. func (e *Evaluator) DelLetVar(name string) error { @@ -747,6 +765,11 @@ func (e *Evaluator) setOption(args []string) error { if err != nil { issues = append(issues, fmt.Sprintf("extension: %v", err)) } + case "--enable_partial_eval": + err := e.EnablePartialEval() + if err != nil { + issues = append(issues, fmt.Sprintf("enable_partial_eval: %v", err)) + } default: issues = append(issues, fmt.Sprintf("unsupported option '%s'", arg)) } @@ -967,7 +990,12 @@ func (e *Evaluator) Process(cmd Cmder) (string, bool, error) { } if val != nil { t := UnparseType(resultT) + unknown, ok := val.Value().(*types.Unknown) + if ok { + return fmt.Sprintf("Unknown %v", unknown), false, nil + } v, err := ext.FormatString(val, "") + if err != nil { // Default format if type is unsupported by ext.Strings formatter. return fmt.Sprintf("%v : %s", val.Value(), t), false, nil @@ -1039,11 +1067,12 @@ func (e *Evaluator) Evaluate(expr string) (ref.Val, *exprpb.Type, error) { return nil, nil, iss.Err() } - p, err := env.Program(ast, e.ctx.programOptions()) + p, err := env.Program(ast, e.ctx.programOptions()...) if err != nil { return nil, nil, err } + act, _ = env.PartialVars(act) val, _, err := p.Eval(act) // expression can be well-formed and result in an error return val, ast.ResultType(), err diff --git a/repl/evaluator_test.go b/repl/evaluator_test.go index 4d58536c..4e49603d 100644 --- a/repl/evaluator_test.go +++ b/repl/evaluator_test.go @@ -716,7 +716,81 @@ func TestProcess(t *testing.T) { wantExit: false, wantError: false, }, - + { + name: "OptionEnablePartialEval", + commands: []Cmder{ + &simpleCmd{ + cmd: "option", + args: []string{ + "--enable_partial_eval", + }, + }, + &letVarCmd{ + identifier: "x", + typeHint: mustParseType(t, "int"), + src: "", + }, + &letVarCmd{ + identifier: "y", + typeHint: mustParseType(t, "int"), + src: "10", + }, + &evalCmd{ + expr: "x + y > 10 || y > 10", + }, + }, + wantText: "Unknown x (1)", + wantExit: false, + wantError: false, + }, + { + name: "PartialEvalDisabled", + commands: []Cmder{ + &letVarCmd{ + identifier: "x", + typeHint: mustParseType(t, "int"), + src: "", + }, + &letVarCmd{ + identifier: "y", + typeHint: mustParseType(t, "int"), + src: "10", + }, + &evalCmd{ + expr: "x + y > 10 || y > 10", + }, + }, + wantText: "", + wantExit: false, + wantError: true, + }, + { + name: "PartialEvalFiltered", + commands: []Cmder{ + &simpleCmd{ + cmd: "option", + args: []string{ + "--enable_partial_eval", + }, + }, + &letVarCmd{ + identifier: "x", + typeHint: mustParseType(t, "int"), + src: "", + }, + &letVarCmd{ + identifier: "y", + typeHint: mustParseType(t, "int"), + src: "11", + }, + &evalCmd{ + expr: "x + y > 10 || y > 10", + }, + }, + wantText: "true : bool", + wantExit: false, + wantError: false, + }, { name: "LoadDescriptorsError", commands: []Cmder{ diff --git a/repl/main/README.md b/repl/main/README.md index 8063e2f4..6ad875d2 100644 --- a/repl/main/README.md +++ b/repl/main/README.md @@ -161,6 +161,8 @@ may take string arguments. `--extension ` enables CEL extensions. Valid options are: `strings`, `protos`, `math`, `encoders`, `optional`, `bindings`, and `all`. +`--enable_partial_eval` enables partial evaluations + example: `%option --container 'google.protobuf'` @@ -169,6 +171,8 @@ example: `%option --extension 'all'` (Loads all extensions) +`%option --enable_partial_eval` + #### reset `%reset` drops all options and let expressions, returning the evaluator to a diff --git a/test/bench/bench.go b/test/bench/bench.go index d8245852..10b14dac 100644 --- a/test/bench/bench.go +++ b/test/bench/bench.go @@ -40,6 +40,21 @@ type Case struct { Out ref.Val } +// DynamicEnvCase represents an expression compiled in a dynamic environment. +type DynamicEnvCase struct { + // Expr is a human-readable expression which is expected to compile. + Expr string + + // Options indicate additional pieces of configuration such as CEL libraries, variables, and functions. + Options func(b *testing.B) *cel.Env + + // In is expected to be a map[string]any or interpreter.Activation instance representing the input to the expression. + In any + + // Out is the expected CEL valued output. + Out ref.Val +} + var ( // ReferenceCases represent canonical CEL expressions for common use cases. ReferenceCases = []*Case{ @@ -176,6 +191,52 @@ var ( Out: types.String(`formatted list: ["abc", "cde"], size: 2`), }, } + // ReferenceDynamicEnvCases represent CEL expressions compiled in an extended environment. + ReferenceDynamicEnvCases = []*DynamicEnvCase{ + // Base case without extended environment. + { + Expr: `a+b`, + Options: func(b *testing.B) *cel.Env { + stdenv, err := cel.NewEnv(cel.Variable("a", cel.IntType), cel.Variable("b", cel.IntType)) + if err != nil { + b.Fatalf("cel.NewEnv() failed: %v", err) + } + return stdenv + }, + }, + { + Expr: `x == y && y == z`, + Options: func(b *testing.B) *cel.Env { + + stdenv, err := cel.NewEnv(cel.Variable("x", cel.IntType), cel.Variable("y", cel.IntType)) + if err != nil { + b.Fatalf("cel.NewEnv() failed: %v", err) + } + stdenv.Compile("x == y") + stdenv, err = stdenv.Extend(cel.Variable("z", cel.IntType)) + if err != nil { + b.Fatalf("cel.Extend() failed: %v", err) + } + return stdenv + }, + }, + { + Expr: `x + y + z`, + Options: func(b *testing.B) *cel.Env { + + stdenv, err := cel.NewEnv(cel.Variable("x", cel.IntType), cel.Variable("y", cel.IntType)) + if err != nil { + b.Fatalf("cel.NewEnv() failed: %v", err) + } + stdenv.Compile("x + y") + stdenv, err = stdenv.Extend(cel.Variable("z", cel.IntType)) + if err != nil { + b.Fatalf("cel.Extend() failed: %v", err) + } + return stdenv + }, + }, + } ) // RunReferenceCases evaluates the set of ReferenceCases against a custom CEL environment. @@ -188,6 +249,14 @@ func RunReferenceCases(b *testing.B, env *cel.Env) { } } +// RunReferenceDynamicEnvCases evaluates the set of ReferenceDynamicEnvCases. +func RunReferenceDynamicEnvCases(b *testing.B) { + b.Helper() + for _, rc := range ReferenceDynamicEnvCases { + RunDynamicEnvCase(b, rc) + } +} + // RunCase evaluates a single test case against a custom environment, running three different // variants of the expression: optimized, unoptimized, and trace. // @@ -241,3 +310,18 @@ func RunCase(b *testing.B, env *cel.Env, bc *Case) { }) } } + +// RunDynamicEnvCase runs a singular DynamicEnvCase. +func RunDynamicEnvCase(b *testing.B, bc *DynamicEnvCase) { + b.Helper() + b.Run(bc.Expr, func(b *testing.B) { + env := bc.Options(b) + b.ResetTimer() + for i := 0; i < b.N; i++ { + _, err := env.Compile(bc.Expr) + if err != nil { + b.Fatalf("env.Compile(%v) failed: %v", bc.Expr, err) + } + } + }) +} diff --git a/test/bench/bench_test.go b/test/bench/bench_test.go index 78482467..4cfc1e85 100644 --- a/test/bench/bench_test.go +++ b/test/bench/bench_test.go @@ -27,3 +27,7 @@ func BenchmarkReferenceCases(b *testing.B) { } RunReferenceCases(b, stdenv) } + +func BenchmarkReferenceCheckerCases(b *testing.B) { + RunReferenceDynamicEnvCases(b) +}