-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
ast/parser: generate new wildcards for else #5412
ast/parser: generate new wildcards for else #5412
Conversation
if v, ok := rule.Head.Args[i].Value.(Var); ok && v.IsWildcard() { | ||
rule.Head.Args[i].Value = Var(p.genwildcard()) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The copy here had been misleading the "wildcard unmangling" logic in the formatter: https://github.com/open-policy-agent/opa/blob/main/format/format.go#L168
It picked up the else
's invisible args and counted them as a second use that would necessitate the unmangling. It really doesn't, though, and is best sidestepped by introducing fresh wildcard variables for the else args in the first place.
@@ -4830,6 +4854,9 @@ func assertParseRule(t *testing.T, msg string, input string, correct *Rule, opts | |||
if !rule.Head.Ref().Equal(correct.Head.Ref()) { | |||
t.Errorf("Error on test \"%s\": rule heads not equal: ref = %v (parsed), ref = %v (correct)", msg, rule.Head.Ref(), correct.Head.Ref()) | |||
} | |||
if !rule.Head.Equal(correct.Head) { | |||
t.Errorf("Error on test \"%s\": rule heads not equal: %v (parsed), %v (correct)", msg, rule.Head, correct.Head) | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just made the comparison a little more helpful. It's not much, but anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍
The previous behaviour had triggered a check in the formatter for multiple use of wildcard variables: f(_) := true { true } else := false The formatter found `$1`, the `_` argument of f, again in else, and thus changed it into `_1`: f(_1) := true { true } else := false There's no extra meaning to the copied wildcards in `else`, they should not count as second usage. Signed-off-by: Stephan Renatus <[email protected]>
cedd478
to
5a08964
Compare
This is a spiritual follow-up to open-policy-agent#5412. A policy like f(_) := 1 { true } { true } { true } would have been pretty-printed as f(_0) := 1 f(_0) := 1 f(_0) := 1 because of the duplicated wildcards. They now get the same treatment as "else" gets: fresh wildcards. Signed-off-by: Stephan Renatus <[email protected]>
This is a spiritual follow-up to #5412. A policy like f(_) := 1 { true } { true } { true } would have been pretty-printed as f(_0) := 1 f(_0) := 1 f(_0) := 1 because of the duplicated wildcards. They now get the same treatment as "else" gets: fresh wildcards. Signed-off-by: Stephan Renatus <[email protected]>
The previous behaviour had triggered a check in the formatter for multiple use of wildcard variables:
The formatter found
$1
, the_
argument of f, again in else, and thus changed it into_1
:There's no extra meaning to the copied wildcards in
else
, they should not count as second usage.Fixes #5347.
else
defined for a function.