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.

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

Conversation

zeitgeist87
Copy link
Contributor

Some functions are intended to have side effects and therefore should not be folded even if all parameters are constants. Currently the constant folding optimizer throws an error if such a function does not provide an implementation. This change allows functions with side effects to return Unknown.

Some functions are intended to have side effects and therefore should
not be folded even if all parameters are constants. Currently the
constant folding optimizer throws an error if such a function does
not provide an implementation. This change allows functions with
side effects to return Unknown.
@TristonianJones TristonianJones self-requested a review January 30, 2025 23:02
@TristonianJones
Copy link
Collaborator

/gcbrun

{
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.

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants