topdown/eval: fix 'every' term plugging on save #4775
Conversation
srenatus
force-pushed
the
sr/topdown/fix-every-pe
branch
from
1d22b5c
to
9cd21f7
Compare
ghost
mentioned this pull request
ast/compile_test.go
Outdated
| every y in [2] { | ||
| y in output | ||
| } | ||
| }`), |
There was a problem hiding this comment.
☝️ I think we should be able to reorder this properly: If comp = [ 1 | true ] is would come after object.get(...), we'd be fine.
Right now, when checking the output vars of the call, we get nothing because the call is unsafe. However, we also fly through the checks required to add the object.get call to the reordered body, so we'd be getting the same ordering back. However, with the every call, we'll get an error: output is unsafe, because it's never removed from the unsafe set, since it's never part of the output vars of the reordered body.
Previously missing plugging could cause unsafe variables in the PE output. Now, all terms in the 'every' body should be plugged properly. The approach taken here is to plug them all, and then fix the key and val var names of the copied every expression. Those vars are fresh after the compiler is done with the expression, so plugging them should never have any effect outside of the rename. Signed-off-by: Stephan Renatus <[email protected]>
srenatus
force-pushed
the
sr/topdown/fix-every-pe
branch
from
49b8960
to
91cde1a
Compare
| for i := range every.Body { | ||
| switch t := every.Body[i].Terms.(type) { | ||
| case *ast.Term: | ||
| every.Body[i].Terms = e.e.bindings.Plug(t) | ||
| every.Body[i].Terms = e.e.bindings.PlugNamespaced(t, e.e.caller.bindings) | ||
| case []*ast.Term: | ||
| for j := range t { | ||
| t[j] = e.e.bindings.Plug(t[j]) | ||
| for j := 1; j < len(t); j++ { // don't plug operator, t[0] | ||
| t[j] = e.e.bindings.PlugNamespaced(t[j], e.e.caller.bindings) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Unrelated to these changes, but it looks like we're only dealing with single terms and calls here. What happens if the body contains an ast.Every? Is there some rewriting that I'm not aware of?
There was a problem hiding this comment.
Good find. Probably an oversight. I'll look into it later.
Previously missing plugging could cause unsafe variables in the
PE output.
Now, all terms in the 'every' body should be plugged properly.
The approach taken here is to plug them all, and then fix the
key and val var names of the copied every expression. Those vars
are fresh after the compiler is done with the expression, so
plugging them should never have any effect outside of the rename.
Fixes the first part of #4766