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

planner: fix with replacement with local-var values #6996

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

srenatus
Copy link
Contributor

@srenatus srenatus commented Sep 5, 2024

Every function down the call stacks of a with-replacement that does this will now get extra variables added to its args to carry along the local variables that will eventually be needed to replace the with-mocked func call.

TODOs:

  • think hard about variable name clashes (do we need a factory func for this? OTOH I think local vars are safe...)
  • more test cases

Fixes #5311.

Every function down the call stacks of a with-replacement that does this
will now get extra variables added to its args to carry along the local
variables that will eventually be needed to replace the with-mocked func call.

TODOs:
- [ ] think hard about variable name clashes (do we need a func for this?)
- [ ] more test cases

Fixes open-policy-agent#5311.

Signed-off-by: Stephan Renatus <[email protected]>
@@ -160,7 +160,7 @@ func (p *Planner) buildFunctrie() error {
return nil
}

func (p *Planner) planRules(rules []*ast.Rule) (string, error) {
func (p *Planner) planRules(rules []*ast.Rule, extras ...*ast.Term) (string, error) {
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 shortcut -- I didn't want to adjust all call sites, but only the one where I'd like to have extra args injected...

@srenatus
Copy link
Contributor Author

srenatus commented Sep 5, 2024

TBC. this approach works well with "proper" functions, but not with rules... their eval is driven by planRefData and planRules is all over the place there. It's less approachable to add extra arguments 💭

Copy link

stale bot commented Oct 5, 2024

This pull request has been automatically marked as stale because it has not had any activity in the last 30 days.

@stale stale bot added the inactive label Oct 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

plan/wasm: with replacement using vars in indirect calls fails
1 participant