ewilliams28/releases
1
0
Fork 0
mirror of https://github.com/open-policy-agent/opa.git synced 2026-08-12 19:32:48 -06:00
Code Issues Packages Projects Releases Wiki Activity

format: only use ref heads for all rule heads if necessary (#5450)

Browse Source 
Before, we'd end up formatting

    ps["foo"] = "bar" { true }

as

    ps.foo = "bar" { true }

and older OPA version know how to parse the former, but not
the latter.

Fixes #5449.

Also includes:
* format: pass internal options via struct; because adding a third (in some cases
   fifth) boolean argument just didn't seem right.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
This commit is contained in:
Stephan Renatus
2022-12-07 13:20:26 +01:00
committed by GitHub
parent b7fb9d2cc3
commit c0866e8fce
8 changed files with 104 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files
format/format.go
+56 -33
View File
@@ -61,6 +61,24 @@ func Ast(x interface{}) ([]byte, error) {
return AstWithOpts(x, Opts{})
}
type fmtOpts struct {
// When the future keyword "contains" is imported, all the pretty-printed
// modules will use that format for partial sets.
// NOTE(sr): For ref-head rules, this will be the default behaviour, since
// we need "contains" to disambiguate complete rules from partial sets.
contains bool
// Same logic applies as for "contains": if `future.keywords.if` (or all
// future keywords) is imported, we'll render rules that can use `if` with
// `if`.
ifs bool
// We check all rule ref heads to see if any of them _requires_ support
// for ref heads -- if they do, we'll print all of them in a different way
// than if they don't.
refHeads bool
}
func AstWithOpts(x interface{}, opts Opts) ([]byte, error) {
// The node has to be deep copied because it may be mutated below. Alternatively,
// we could avoid the copy by checking if mutation will occur first. For now,
@@ -75,16 +93,7 @@ func AstWithOpts(x interface{}, opts Opts) ([]byte, error) {
// present.
extraFutureKeywordImports := map[string]struct{}{}
// When the future keyword "contains" is imported, all the pretty-printed
// modules will use that format for partial sets.
// NOTE(sr): For ref-head rules, this will be the default behaviour, since
// we need "contains" to disambiguate complete rules from partial sets.
useContainsKW := false
// Same logic applies as for "contains": if `future.keywords.if` (or all
// future keywords) is imported, we'll render rules that can use `if` with
// `if`.
useIf := false
o := fmtOpts{}
// Preprocess the AST. Set any required defaults and calculate
// values required for printing the formatted output.
@@ -108,12 +117,20 @@ func AstWithOpts(x interface{}, opts Opts) ([]byte, error) {
case *ast.Import:
switch {
case future.IsAllFutureKeywords(n):
useContainsKW = true
useIf = true
o.contains = true
o.ifs = true
case future.IsFutureKeyword(n, "contains"):
useContainsKW = true
o.contains = true
case future.IsFutureKeyword(n, "if"):
useIf = true
o.ifs = true
}
case *ast.Rule:
if len(n.Head.Ref()) > 2 {
o.refHeads = true
}
if len(n.Head.Ref()) == 2 && n.Head.Key != nil && n.Head.Value == nil { // p.q contains "x"
o.refHeads = true
}
}
@@ -132,19 +149,18 @@ func AstWithOpts(x interface{}, opts Opts) ([]byte, error) {
for kw := range extraFutureKeywordImports {
x.Imports = ensureFutureKeywordImport(x.Imports, kw)
}
w.writeModule(x, useContainsKW, useIf)
w.writeModule(x, o)
case *ast.Package:
w.writePackage(x, nil)
case *ast.Import:
w.writeImports([]*ast.Import{x}, nil)
case *ast.Rule:
w.writeRule(x, false /* isElse */, useContainsKW, useIf, nil)
w.writeRule(x, false /* isElse */, o, nil)
case *ast.Head:
w.writeHead(x,
false, // isDefault
false, // isExpandedConst
useContainsKW,
useIf,
o,
nil)
case ast.Body:
w.writeBody(x, nil)
@@ -214,7 +230,7 @@ type writer struct {
delay bool
}
func (w *writer) writeModule(module *ast.Module, useContainsKW, useIf bool) {
func (w *writer) writeModule(module *ast.Module, o fmtOpts) {
var pkg *ast.Package
var others []interface{}
var comments []*ast.Comment
@@ -253,7 +269,7 @@ func (w *writer) writeModule(module *ast.Module, useContainsKW, useIf bool) {
imports, others = gatherImports(others)
comments = w.writeImports(imports, comments)
rules, others = gatherRules(others)
comments = w.writeRules(rules, useContainsKW, useIf, comments)
comments = w.writeRules(rules, o, comments)
}
for i, c := range comments {
@@ -283,16 +299,16 @@ func (w *writer) writeComments(comments []*ast.Comment) {
}
}
func (w *writer) writeRules(rules []*ast.Rule, useContainsKW, useIf bool, comments []*ast.Comment) []*ast.Comment {
func (w *writer) writeRules(rules []*ast.Rule, o fmtOpts, comments []*ast.Comment) []*ast.Comment {
for _, rule := range rules {
comments = w.insertComments(comments, rule.Location)
comments = w.writeRule(rule, false, useContainsKW, useIf, comments)
comments = w.writeRule(rule, false, o, comments)
w.blankLine()
}
return comments
}
func (w *writer) writeRule(rule *ast.Rule, isElse, useContainsKW, useIf bool, comments []*ast.Comment) []*ast.Comment {
func (w *writer) writeRule(rule *ast.Rule, isElse bool, o fmtOpts, comments []*ast.Comment) []*ast.Comment {
if rule == nil {
return comments
}
@@ -311,17 +327,17 @@ func (w *writer) writeRule(rule *ast.Rule, isElse, useContainsKW, useIf bool, co
// pretend that the rule has no body in this case.
isExpandedConst := rule.Body.Equal(ast.NewBody(ast.NewExpr(ast.BooleanTerm(true)))) && rule.Else == nil
comments = w.writeHead(rule.Head, rule.Default, isExpandedConst, useContainsKW, useIf, comments)
comments = w.writeHead(rule.Head, rule.Default, isExpandedConst, o, comments)
// this excludes partial sets UNLESS `contains` is used
partialSetException := useContainsKW || rule.Head.Value != nil
partialSetException := o.contains || rule.Head.Value != nil
if len(rule.Body) == 0 || isExpandedConst {
w.endLine()
return comments
}
if useIf && partialSetException {
if o.ifs && partialSetException {
w.write(" if")
if len(rule.Body) == 1 {
if rule.Body[0].Location.Row == rule.Head.Location.Row {
@@ -329,7 +345,7 @@ func (w *writer) writeRule(rule *ast.Rule, isElse, useContainsKW, useIf bool, co
comments = w.writeExpr(rule.Body[0], comments)
w.endLine()
if rule.Else != nil {
comments = w.writeElse(rule, useContainsKW, useIf, comments)
comments = w.writeElse(rule, o, comments)
}
return comments
}
@@ -357,12 +373,12 @@ func (w *writer) writeRule(rule *ast.Rule, isElse, useContainsKW, useIf bool, co
w.startLine()
w.write("}")
if rule.Else != nil {
comments = w.writeElse(rule, useContainsKW, useIf, comments)
comments = w.writeElse(rule, o, comments)
}
return comments
}
func (w *writer) writeElse(rule *ast.Rule, useContainsKW, useIf bool, comments []*ast.Comment) []*ast.Comment {
func (w *writer) writeElse(rule *ast.Rule, o fmtOpts, comments []*ast.Comment) []*ast.Comment {
// If there was nothing else on the line before the "else" starts
// then preserve this style of else block, otherwise it will be
// started as an "inline" else eg:
@@ -424,15 +440,22 @@ func (w *writer) writeElse(rule *ast.Rule, useContainsKW, useIf bool, comments [
rule.Else.Head.Value.Location = rule.Else.Head.Location
}
return w.writeRule(rule.Else, true, useContainsKW, useIf, comments)
return w.writeRule(rule.Else, true, o, comments)
}
func (w *writer) writeHead(head *ast.Head, isDefault, isExpandedConst, useContainsKW, useIf bool, comments []*ast.Comment) []*ast.Comment {
func (w *writer) writeHead(head *ast.Head, isDefault, isExpandedConst bool, o fmtOpts, comments []*ast.Comment) []*ast.Comment {
ref := head.Ref()
if head.Key != nil && head.Value == nil {
ref = ref.GroundPrefix()
}
w.write(ref.String())
if o.refHeads || len(ref) == 1 {
w.write(ref.String())
} else {
w.write(ref[0].String())
w.write("[")
w.write(ref[1].String())
w.write("]")
}
if len(head.Args) > 0 {
w.write("(")
@@ -444,7 +467,7 @@ func (w *writer) writeHead(head *ast.Head, isDefault, isExpandedConst, useContai
w.write(")")
}
if head.Key != nil {
if useContainsKW && head.Value == nil {
if o.contains && head.Value == nil {
w.write(" contains ")
comments = w.writeTerm(head.Key, comments)
} else if head.Value == nil { // no `if` for p[x] notation
format/testfiles/test.rego.formatted
+4 -4
View File
@@ -24,11 +24,11 @@ globals = {
"fizz": "buzz",
}
partial_obj.x = 1
partial_obj["x"] = 1
partial_obj.y = 2
partial_obj["y"] = 2
partial_obj.z = 3
partial_obj["z"] = 3
partial_set["x"]
@@ -198,7 +198,7 @@ nested_infix {
expanded_const = true
partial_obj.why = true {
partial_obj["why"] = true {
false
}
format/testfiles/test_issue_5449.rego
+3
View File
@@ -0,0 +1,3 @@
package demo
foo["bar"] = "baz" { input }
format/testfiles/test_issue_5449.rego.formatted
+5
View File
@@ -0,0 +1,5 @@
package demo
foo["bar"] = "baz" {
input
}
format/testfiles/test_issue_5449_with_contains_ref_rule.rego
+9
View File
@@ -0,0 +1,9 @@
# This is the same as test_issue_5449.rego, but with another rule
# that gives the formatter the assurance that using ref rules is OK
package demo
import future.keywords.contains
foo["bar"] = "baz" { input }
a.deep contains "ref"
format/testfiles/test_issue_5449_with_contains_ref_rule.rego.formatted
+11
View File
@@ -0,0 +1,11 @@
# This is the same as test_issue_5449.rego, but with another rule
# that gives the formatter the assurance that using ref rules is OK
package demo
import future.keywords.contains
foo.bar = "baz" {
input
}
a.deep contains "ref"
format/testfiles/test_issue_5449_with_ref_rule.rego
+7
View File
@@ -0,0 +1,7 @@
# This is the same as test_issue_5449.rego, but with a rule that gives
# the formatter the assurance that using ref rules is OK
package demo
foo["bar"] = "baz" { input }
a.deep.ref := true
format/testfiles/test_issue_5449_with_ref_rule.rego.formatted
+9
View File
@@ -0,0 +1,9 @@
# This is the same as test_issue_5449.rego, but with a rule that gives
# the formatter the assurance that using ref rules is OK
package demo
foo.bar = "baz" {
input
}
a.deep.ref := true