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

Jump to bottom

ast/compile: 'every' rewriting steps #4231

Merged
merged 20 commits into from Jan 29, 2022
Merged

ast/compile: 'every' rewriting steps #4231

merged 20 commits into from Jan 29, 2022

Conversation

srenatus
Copy link
Contributor

@srenatus srenatus commented Jan 14, 2022 •
edited

This PR covers the compiler doing it's job on every:

  • resolve refs
  • rewriting other declared vars in domain
  • rewriting its declared vars and others in its body
  • rewriting dynamics in the body
  • expanding expressions
  • safety body reordering and var checking

@srenatus srenatus force-pushed the sr/ast/every-kw-rewriting branch 7 times, most recently from 0dc372c to 9093e0c Compare January 19, 2022 10:32
@srenatus srenatus marked this pull request as ready for review January 19, 2022 10:50
@srenatus srenatus force-pushed the sr/ast/every-kw-rewriting branch 2 times, most recently from ba17c25 to 3ecaaa5 Compare January 20, 2022 13:42
@srenatus srenatus marked this pull request as draft January 20, 2022 13:44
@srenatus
Copy link
Contributor Author

there's a broken test I've got to take care of (not sure yet if the test is bogus or the code). moving to draft.

tsandall
tsandall reviewed Jan 20, 2022
View reviewed changes
ast/policy.go Outdated Show resolved Hide resolved
ast/compile.go Outdated Show resolved Hide resolved
ast/policy.go Outdated Show resolved Hide resolved
srenatus
srenatus commented Jan 21, 2022
View reviewed changes
ast/compile.go
Comment on lines +2961 to +2968
if ev, ok := x.(*Every); ok {
vis.Walk(ev.Body)
return true
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bit of a trap: WalkClosures checks that its node in question is of type *Every. So, when passing a function, you get an x of type *Every, and to walk its actual closure, the every.Body, it'll have to be done like this.

I wonder if there's a better approach here. Having WalkClosures apply f to ev.Body here would not allow us to differentiate what we've been called on when we need it, here.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is unavoidable due to the scoping rules we've implemented for every. Presumably it'll be the same for any other keywords we add that support closures and local variable declaration. This seems like the best we can do.

@srenatus srenatus marked this pull request as ready for review January 21, 2022 13:36
@tsandall
Copy link
Member

LGTM!

tsandall
tsandall previously approved these changes Jan 29, 2022
View reviewed changes
ast/compile.go Outdated Show resolved Hide resolved
ast/compile.go
Comment on lines +2961 to +2968
if ev, ok := x.(*Every); ok {
vis.Walk(ev.Body)
return true
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this is unavoidable due to the scoping rules we've implemented for every. Presumably it'll be the same for any other keywords we add that support closures and local variable declaration. This seems like the best we can do.

ast/policy.go Outdated Show resolved Hide resolved
@srenatus
ast: prealloc expression
84c2c7c 
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@srenatus
ast/visit: skip 'Every' Body when skipping closures
7f3380c 
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@srenatus
ast/parser_ext: add MustParseModuleWithOpts, MustParseBodyWithOpts
b1ce68b 
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@srenatus
ast/compile: 'every' rewriting (dynamic, declared vars)
4f9e3da 
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@srenatus
ast/compile: simplify every rewriting (key/val are vars)
6f0b910 
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@srenatus
ast/compile: deal with wildcard cases
3227f5d 
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@srenatus
ast/compile: add missing stack.Pop()
22ced5d 
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@srenatus
ast/compile_test: fix indentation
b757da8 
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@srenatus
ast/compile: fix output vars of 'every' body
00e3896 
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@srenatus
ast/compile: safety check vars in every.Body
ddd81d8 
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@srenatus
ast/compile_test: add nested case for every rewriting
8e78ae7 
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@srenatus
ast/compiler: rewrite prints in 'every' bodies
8ac09b4 
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@srenatus
ast/compile_test: add "rewrite dynamics" tests for "every"
0b72c1c 
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@srenatus
ast/compile: expand "every" domain
3954eb3 
There's one broken test that I haven't figured out how to fix yet.

From the perspective of topdown, this change felt right, it will
simplify evaluation.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@srenatus
ast/compile: outputVarsForExpr: don't return vars from "Every" body
0f1bfe0 
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@srenatus
ast/compile: fix safety reordering for every
9289cb5 
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@srenatus
ast: rename (Every).Vars() -> (Every).KeyValueVars()
decbdb1 
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@srenatus
ast/compile: use VarVisitor for KeyValueVars()
5f41d87 
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@srenatus
ast/compile: add ContainsClosures
46478b6 
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@srenatus
ast/compile_test: add unused assigned var in "every" body case
5f5da56 
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@srenatus srenatus merged commit cb867a1 into open-policy-agent:main Jan 29, 2022
@srenatus srenatus deleted the sr/ast/every-kw-rewriting branch January 29, 2022 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsandall tsandall tsandall left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@srenatus @tsandall