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

topdown: use function arity to determine captured output (partial eval) #3683

Merged

Conversation

srenatus
Copy link
Contributor

@srenatus srenatus commented Jul 27, 2021

Before, we had been passing len(e.terms) to saveCall, thereby
sidestepping the checks that would get the output variable into the
save set.

In the problematic policy,

y = f(1)
count(y)

with

f(x) = [] { _ = input }

the call to count(y) would be mishandled: it would be evaluated,
since its arguments aren't in the saveset. Eval would fail because
its argument was a variable (see also #3680)

Fixes #3681.

@srenatus srenatus marked this pull request as ready for review July 27, 2021 12:42
@srenatus srenatus marked this pull request as draft July 28, 2021 07:15
@srenatus srenatus marked this pull request as ready for review July 30, 2021 09:38
@srenatus
Copy link
Contributor Author

Ehh, misclicked. Sorry for the noise.

@srenatus srenatus marked this pull request as draft July 30, 2021 09:39
Before, we had been passing `len(e.terms)` to `saveCall`, thereby
sidestepping the checks that would get the output variable into the
save set.

In the problematic policy,

    y = f(1)
    count(y)

with

    f(x) = [] { _ = input }

the call to `count(y)` would be mishandled: it would be evaluated,
since its arguments aren't in the saveset. Eval would fail because
its argument was a variable (see also
open-policy-agent#3680)

Fixes open-policy-agent#3681.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@srenatus srenatus force-pushed the sr/issue-3681/shallow-pw-and-function branch from 72d3a54 to 228afa8 Compare July 30, 2021 10:00
@srenatus srenatus changed the title topdown: save call to builtin if args aren't ground topdown: use function arity to determine captured output Jul 30, 2021
@srenatus srenatus changed the title topdown: use function arity to determine captured output topdown: use function arity to determine captured output (partial eval) Jul 30, 2021
@srenatus srenatus marked this pull request as ready for review July 30, 2021 10:05
Copy link
Member

@tsandall tsandall left a comment

Choose a reason for hiding this comment

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

LGTM. A nice and easy fix!

@srenatus srenatus merged commit 9e890c4 into open-policy-agent:main Aug 12, 2021
@srenatus srenatus deleted the sr/issue-3681/shallow-pw-and-function branch August 12, 2021 21:00
jgchn pushed a commit to jgchn/opa that referenced this pull request Aug 20, 2021
…-agent#3683)

Before, we had been passing `len(e.terms)` to `saveCall`, thereby
sidestepping the checks that would get the output variable into the
save set.

In the problematic policy,

    y = f(1)
    count(y)

with

    f(x) = [] { _ = input }

the call to `count(y)` would be mishandled: it would be evaluated,
since its arguments aren't in the saveset. Eval would fail because
its argument was a variable (see also
open-policy-agent#3680)

Fixes open-policy-agent#3681.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
dolevf pushed a commit to dolevf/opa that referenced this pull request Nov 4, 2021
…-agent#3683)

Before, we had been passing `len(e.terms)` to `saveCall`, thereby
sidestepping the checks that would get the output variable into the
save set.

In the problematic policy,

    y = f(1)
    count(y)

with

    f(x) = [] { _ = input }

the call to `count(y)` would be mishandled: it would be evaluated,
since its arguments aren't in the saveset. Eval would fail because
its argument was a variable (see also
open-policy-agent#3680)

Fixes open-policy-agent#3681.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
Signed-off-by: Dolev Farhi <farhi.dolev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PE with shallow inlining producing incorrect result for some function calls
2 participants