Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adapt constant folding to ignore functions returning Unknown #1117

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Adapt constant folding to ignore functions returning Unknown #1117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions cel/folding.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,10 @@ func tryFold(ctx *OptimizerContext, a *ast.AST, expr ast.Expr) error {
if err != nil {
return err
}
// Don't update the expression if it's an unknown value.
if out.Type() == types.UnknownType {
return nil
}
// Update the fold expression to be a literal.
ctx.UpdateExpr(expr, ctx.NewLiteral(out))
return nil
Expand Down
80 changes: 80 additions & 0 deletions cel/folding_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,15 @@ package cel
import (
"reflect"
"sort"
"strings"
"testing"

"google.golang.org/protobuf/encoding/prototext"
"google.golang.org/protobuf/proto"

"github.com/google/cel-go/common/ast"
"github.com/google/cel-go/common/types"
"github.com/google/cel-go/common/types/ref"

proto3pb "github.com/google/cel-go/test/proto3pb"
exprpb "google.golang.org/genproto/googleapis/api/expr/v1alpha1"
Expand Down Expand Up @@ -313,6 +316,83 @@ func TestConstantFoldingOptimizer(t *testing.T) {
}
}

func TestConstantFoldingCallsWithSideEffects(t *testing.T) {
tests := []struct {
expr string
folded string
error string
}{
{
expr: `noSideEffect(3)`,
folded: `3`,
},
{
expr: `withSideEffect(3)`,
folded: `withSideEffect(3)`,
},
{
expr: `noImpl(3)`,
error: `constant-folding evaluation failed: no such overload: noImpl`,
},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To keep things simple, the constant folding optimizer should validate that all functions / overloads referenced in the ast.Expr have bindings before attempting to fold.

}
e, err := NewEnv(
OptionalTypes(),
EnableMacroCallTracking(),
Function("noSideEffect",
Overload("noSideEffect_int_int",
[]*Type{IntType},
IntType, FunctionBinding(func(args ...ref.Val) ref.Val {
return args[0]
}))),
Function("withSideEffect",
Overload("withSideEffect_int_int",
[]*Type{IntType},
IntType, FunctionBinding(func(args ...ref.Val) ref.Val {
return &types.Unknown{}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the side-effect case, we can propose marking a function as volatile or late-bound also skip trying to fold these functions.

cel.Function("withSideEffect", 
    cel.Overload("withSideEffect_int_int", []*cel.Type{cel.IntType, cel.IntType}, cel.IntType),
    cel.LateFunctionBinding())

This marker will let users specify that the function definition will be provided later on the cel.Activation.

// validates that the function has a late binding flag, and packages up context needed to resolve
// the function within the Activation by the interpreter.
lateFn, err := env.LateBoundFunction("withSideEffect", func(rhs, lhs ref.Val) ref.Val {
   // do something interesting
})
input, err := cel.NewActivation(data)
input.AddFunctionBinding(lateFn)

I'd like to do this to support late-bound functions inside of cel-go, so perhaps we can work toward the late-bound function support and solve two issues at once?

}))),
Function("noImpl",
Overload("noImpl_int_int",
[]*Type{IntType},
IntType)),
)
if err != nil {
t.Fatalf("NewEnv() failed: %v", err)
}
for _, tst := range tests {
tc := tst
t.Run(tc.expr, func(t *testing.T) {
checked, iss := e.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 := NewStaticOptimizer(folder)
optimized, iss := opt.Optimize(e, checked)
if tc.error != "" {
if iss.Err() == nil {
t.Errorf("got nil, wanted error containing %q", tc.error)
} else if !strings.Contains(iss.Err().Error(), tc.error) {
t.Errorf("got %q, wanted error containing %q", iss.Err().Error(), tc.error)
}
return
}
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)
}
})
}
}

func TestConstantFoldingOptimizerMacroElimination(t *testing.T) {
tests := []struct {
expr string
Expand Down