ast/compile: reorder body for safety differently #4801
Conversation
Our previous approach, ordering for closures first, and taking the growing
set of output variables of the reordered body into account in a second step,
did not work out for some examples:
object.get(input.subject.roles[_], comp, [""], output)
comp = [ 1 | true ]
every y in [2] {
y in output
}
Here, the closure of `every` would have not checkout out because we've never
registered the output variable `output` -- since the first call to object.get
is unsafe without `comp`, too.
Now, the two stages have been merged. It's got surprisingly little fallout,
one test case had to be adjusted, which I believe to be a minor case, too.
Fixes the second part of open-policy-agent#4766.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
srenatus
force-pushed
the
sr/ast/reordering-for-closures-and-safety-in-one
branch
from
c7225ca
to
7823c8d
Compare
|
|
||
| reordered := Body{} | ||
| bodyVars := body.Vars(SafetyCheckVisitorParams) | ||
| reordered := make(Body, 0, len(body)) |
There was a problem hiding this comment.
micro-optimizations of little consequence:
- the bodyVars never change, so we can collect them outside of the loop (compared to reorderBodyForClosures)
- we know how large reordered could become, and usually would, if there are no unsafe variables; so allocate upfront.
| @@ -3160,21 +3157,35 @@ func reorderBodyForSafety(builtins map[string]*Builtin, arity func(Ref) int, glo | |||
| continue | |||
| } | |||
|
|
|||
| safe.Update(outputVarsForExpr(e, arity, safe)) | |||
| ovs := outputVarsForExpr(e, arity, safe) | |||
There was a problem hiding this comment.
These are the ones we've been missing in reorderBodyForClosures: because there, we kept feeding globals as third parameter, instead of the accumulated safe vars, we'd never see output variables for function calls whose inputs only become safe after reordering.
| safe.Update(outputVarsForExpr(e, arity, safe)) | ||
| ovs := outputVarsForExpr(e, arity, safe) | ||
|
|
||
| // check closures: is this expression closing over variables that |
There was a problem hiding this comment.
This is basically the inlining of reorderBodyForClosures.
| uv := cv.Diff(outputVarsForBody(reordered, arity, safe)) | ||
|
|
||
| if len(uv) > 0 { | ||
| if uv.Equal(ovs) { // special case "closure-self" |
There was a problem hiding this comment.
This one is ugly, but it's what makes x = [ x | x = 1 ] fail without affecting the other test cases 🙈
| `package compr | ||
| opts := ParserOptions{AllFutureKeywords: true, unreleasedKeywords: true} | ||
|
|
||
| tests := []struct { |
There was a problem hiding this comment.
"modernized" the test case structure here, adding the one we care about
| object.get(input.subject.roles[_], comp, [""], output) | ||
| comp = [ 1 | true ] | ||
| every y in [2] { | ||
| y in output |
There was a problem hiding this comment.
This is the case that broke PE for #4766.
| @@ -797,7 +832,7 @@ func TestCompilerCheckSafetyBodyErrors(t *testing.T) { | |||
| {"array-compr-mixed", `p { _ = [x | y = [a | a = z[i]]] }`, `{a, x, z, i}`}, | |||
| {"array-compr-builtin", `p { [true | eq != 2] }`, `{eq,}`}, | |||
| {"closure-self", `p { x = [x | x = 1] }`, `{x,}`}, | |||
| {"closure-transitive", `p { x = y; x = [y | y = 1] }`, `{y,}`}, | |||
| {"closure-transitive", `p { x = y; x = [y | y = 1] }`, `{x,y}`}, | |||
There was a problem hiding this comment.
Can we accept that? I hope so 😅
There was a problem hiding this comment.
Nice! Before the closure reordering would not see the result of the normal expression reordering. Now, closures are handled in the same pass regular expressions so queries like x = y; y = [1 | true]; [1 | x] no longer get rejected w/ an unsafe error (x in this example).
Our previous approach, ordering for closures first, and taking the growing
set of output variables of the reordered body into account in a second step,
did not work out for some examples:
Here, the closure of
everywould have not checkout out because we've neverregistered the output variable
output-- since the first call to object.getis unsafe without
comp, too.Now, the two stages have been merged. It's got surprisingly little fallout,
one test case had to be adjusted, which I believe to be a minor case, too.
Fixes the second part of #4766.