fix: respect variable scoping when folding comprehensions#1314
Conversation
Iteration ranges and accumulator initializers are evaluated in the enclosing scope, so the comprehension's own iteration variable must not be treated as in-scope there. Previously a free variable in the iteration range that shared a name with the iteration variable caused the optimizer to attempt a fold that fails at evaluation time.
| // constantScopedExpr reports whether an expression references only the given in-scope variables | ||
| // and contains no late-bound function calls, recursing into nested comprehensions with their | ||
| // own iteration and accumulator variables. | ||
| func (opt *constantFoldingOptimizer) constantScopedExpr(ctx *OptimizerContext, a *ast.AST, e ast.Expr, vars map[string]bool) bool { |
There was a problem hiding this comment.
I think there's probably a simpler way to achieve this check which the cel-java implementation uses:
Thanks for looking into this! Consider using the NavigableExpr methods to mirror how Java checks for this case.
|
Tried the cel-java parent-walk approach locally but it conservatively rejects folds like [1,2,3].exists(e, e == 'foo'): the ident e in the loop step walks up to the comprehension that declares iter_var=e, so it gets marked unfoldable even though the whole comprehension evaluates to a literal. The current split (iter_range/accu_init evaluated in the outer scope, loop step/condition/result in the inner scope) keeps those folds working while still rejecting the shadowed-x case from #1296. Happy to go with the cel-java approach if you prefer the trade-off though. |
|
Hi @SAY-5, the java implementation seems to handle those cases fine, so I think this suggests that maybe there was a step missed in the translation from java to go. For example, these cases work out fine: @TestParameters("{source: '[1 + 1, 1 + 2].exists(i, i < 10)', expected: 'true'}")If you wouldn't mind mirroring the approach from Java, that would be ideal for maintainability going forward. |
|
Got it, I'll re-do the port mirroring the Java implementation exactly rather than the shortcut I tried, so the maintainability story stays consistent. Will push the rework once it's matching the Java behavior on the parameterized cases you cited. |
|
@SAY-5 , thank you so much! |
2 participants
The constant folding optimizer treated a comprehension's iteration variable as in-scope while inspecting the iteration range and accumulator initializer, so a free variable in the iteration range that happened to share the iteration variable's name was misjudged foldable and the fold failed at evaluation time. The matcher now walks comprehensions with proper scoping. Closes #1296.