From 71eb556bf2ecde32fa601260d31c0952b90428af Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Tue, 24 Sep 2024 14:52:08 +0200 Subject: [PATCH] Bracketing ref keywords in formatter output Signed-off-by: Johan Fylling --- ast/policy.go | 18 +++- compile/compile_test.go | 2 +- format/format.go | 99 ++++++++++++++----- .../testfiles/v0/test_contains.rego.formatted | 2 +- format/testfiles/v0/test_if.rego.formatted | 2 +- .../testfiles/v0/test_if_else.rego.formatted | 2 +- format/testfiles/v0/test_in.rego | 13 +++ format/testfiles/v0/test_in.rego.formatted | 13 +++ format/testfiles/v0_to_v1/keyword_errors.rego | 13 +++ .../v0_to_v1/keyword_errors.rego.error | 5 + format/testfiles/v0_to_v1/keywords.rego | 12 +-- format/testfiles/v0_to_v1/keywords.rego.error | 5 - .../v0_to_v1/keywords.rego.formatted | 7 ++ format/testfiles/v1/test_contains.rego | 4 +- .../testfiles/v1/test_contains.rego.formatted | 4 +- format/testfiles/v1/test_contains_if.rego | 2 +- .../v1/test_contains_if.rego.formatted | 2 +- format/testfiles/v1/test_if.rego | 2 +- format/testfiles/v1/test_if.rego.formatted | 2 +- format/testfiles/v1/test_if_else.rego | 2 +- .../testfiles/v1/test_if_else.rego.formatted | 2 +- format/testfiles/v1/test_in.rego | 11 +++ format/testfiles/v1/test_in.rego.formatted | 11 +++ internal/future/filter_imports.go | 12 +++ 24 files changed, 193 insertions(+), 54 deletions(-) create mode 100644 format/testfiles/v0/test_in.rego create mode 100644 format/testfiles/v0/test_in.rego.formatted create mode 100644 format/testfiles/v0_to_v1/keyword_errors.rego create mode 100644 format/testfiles/v0_to_v1/keyword_errors.rego.error delete mode 100644 format/testfiles/v0_to_v1/keywords.rego.error create mode 100644 format/testfiles/v0_to_v1/keywords.rego.formatted create mode 100644 format/testfiles/v1/test_in.rego create mode 100644 format/testfiles/v1/test_in.rego.formatted diff --git a/ast/policy.go b/ast/policy.go index 29963a09a4..43e9bba4a3 100644 --- a/ast/policy.go +++ b/ast/policy.go @@ -100,7 +100,7 @@ var Wildcard = &Term{Value: Var("_")} var WildcardPrefix = "$" // Keywords contains strings that map to language keywords. -var Keywords = KeywordsV0 +var Keywords = KeywordsForRegoVersion(DefaultRegoVersion) var KeywordsV0 = [...]string{ "not", @@ -134,9 +134,23 @@ var KeywordsV1 = [...]string{ "every", } +func KeywordsForRegoVersion(v RegoVersion) []string { + switch v { + case RegoV0: + return KeywordsV0[:] + case RegoV1, RegoV0CompatV1: + return KeywordsV1[:] + } + return nil +} + // IsKeyword returns true if s is a language keyword. func IsKeyword(s string) bool { - for _, x := range Keywords { + return IsInKeywords(s, Keywords) +} + +func IsInKeywords(s string, keywords []string) bool { + for _, x := range keywords { if x == s { return true } diff --git a/compile/compile_test.go b/compile/compile_test.go index ef683c7591..be1790ba63 100644 --- a/compile/compile_test.go +++ b/compile/compile_test.go @@ -1741,7 +1741,7 @@ contains := 2 { import rego.v1 p if { - data.foo.contains = input.x + data.foo["contains"] = input.x } `, `package foo diff --git a/format/format.go b/format/format.go index afe4e439b2..75eb74eec1 100644 --- a/format/format.go +++ b/format/format.go @@ -126,7 +126,16 @@ type fmtOpts struct { // than if they don't. refHeads bool - regoV1 bool + regoV1 bool + futureKeywords []string +} + +func (o fmtOpts) keywords() []string { + if o.regoV1 { + return ast.KeywordsV1[:] + } + kws := ast.KeywordsV0[:] + return append(kws, o.futureKeywords...) } func AstWithOpts(x interface{}, opts Opts) ([]byte, error) { @@ -171,6 +180,10 @@ func AstWithOpts(x interface{}, opts Opts) ([]byte, error) { } case *ast.Import: + if kw, ok := future.WhichFutureKeyword(n); ok { + o.futureKeywords = append(o.futureKeywords, kw) + } + switch { case isRegoV1Compatible(n): o.contains = true @@ -200,8 +213,9 @@ func AstWithOpts(x interface{}, opts Opts) ([]byte, error) { }) w := &writer{ - indent: "\t", - errs: make([]*ast.Error, 0), + indent: "\t", + errs: make([]*ast.Error, 0), + fmtOpts: o, } switch x := x.(type) { @@ -219,18 +233,17 @@ func AstWithOpts(x interface{}, opts Opts) ([]byte, error) { x.Imports = ensureFutureKeywordImport(x.Imports, kw) } } - w.writeModule(x, o) + w.writeModule(x) case *ast.Package: w.writePackage(x, nil) case *ast.Import: w.writeImports([]*ast.Import{x}, nil) case *ast.Rule: - w.writeRule(x, false /* isElse */, o, nil) + w.writeRule(x, false /* isElse */, nil) case *ast.Head: w.writeHead(x, false, // isDefault false, // isExpandedConst - o, nil) case ast.Body: w.writeBody(x, nil) @@ -302,9 +315,10 @@ type writer struct { beforeEnd *ast.Comment delay bool errs ast.Errors + fmtOpts fmtOpts } -func (w *writer) writeModule(module *ast.Module, o fmtOpts) { +func (w *writer) writeModule(module *ast.Module) { var pkg *ast.Package var others []interface{} var comments []*ast.Comment @@ -342,7 +356,7 @@ func (w *writer) writeModule(module *ast.Module, o fmtOpts) { imports, others = gatherImports(others) comments = w.writeImports(imports, comments) rules, others = gatherRules(others) - comments = w.writeRules(rules, o, comments) + comments = w.writeRules(rules, comments) } for i, c := range comments { @@ -365,7 +379,15 @@ func (w *writer) writePackage(pkg *ast.Package, comments []*ast.Comment) []*ast. comments = w.insertComments(comments, pkg.Location) w.startLine() - w.write(pkg.String()) + + // Omit head as all packages have the DefaultRootDocument prepended at parse time. + path := make(ast.Ref, len(pkg.Path)-1) + path[0] = ast.VarTerm(string(pkg.Path[1].Value.(ast.String))) + copy(path[1:], pkg.Path[2:]) + + w.write("package ") + w.writeRef(path) + w.blankLine() return comments @@ -380,16 +402,16 @@ func (w *writer) writeComments(comments []*ast.Comment) { } } -func (w *writer) writeRules(rules []*ast.Rule, o fmtOpts, comments []*ast.Comment) []*ast.Comment { +func (w *writer) writeRules(rules []*ast.Rule, comments []*ast.Comment) []*ast.Comment { for _, rule := range rules { comments = w.insertComments(comments, rule.Location) - comments = w.writeRule(rule, false, o, comments) + comments = w.writeRule(rule, false, comments) w.blankLine() } return comments } -func (w *writer) writeRule(rule *ast.Rule, isElse bool, o fmtOpts, comments []*ast.Comment) []*ast.Comment { +func (w *writer) writeRule(rule *ast.Rule, isElse bool, comments []*ast.Comment) []*ast.Comment { if rule == nil { return comments } @@ -408,17 +430,17 @@ func (w *writer) writeRule(rule *ast.Rule, isElse bool, o fmtOpts, comments []*a // 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, o, comments) + comments = w.writeHead(rule.Head, rule.Default, isExpandedConst, comments) // this excludes partial sets UNLESS `contains` is used - partialSetException := o.contains || rule.Head.Value != nil + partialSetException := w.fmtOpts.contains || rule.Head.Value != nil if len(rule.Body) == 0 || isExpandedConst { w.endLine() return comments } - if (o.regoV1 || o.ifs) && partialSetException { + if (w.fmtOpts.regoV1 || w.fmtOpts.ifs) && partialSetException { w.write(" if") if len(rule.Body) == 1 { if rule.Body[0].Location.Row == rule.Head.Location.Row { @@ -426,7 +448,7 @@ func (w *writer) writeRule(rule *ast.Rule, isElse bool, o fmtOpts, comments []*a comments = w.writeExpr(rule.Body[0], comments) w.endLine() if rule.Else != nil { - comments = w.writeElse(rule, o, comments) + comments = w.writeElse(rule, comments) } return comments } @@ -454,12 +476,12 @@ func (w *writer) writeRule(rule *ast.Rule, isElse bool, o fmtOpts, comments []*a w.startLine() w.write("}") if rule.Else != nil { - comments = w.writeElse(rule, o, comments) + comments = w.writeElse(rule, comments) } return comments } -func (w *writer) writeElse(rule *ast.Rule, o fmtOpts, comments []*ast.Comment) []*ast.Comment { +func (w *writer) writeElse(rule *ast.Rule, 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: @@ -521,16 +543,16 @@ func (w *writer) writeElse(rule *ast.Rule, o fmtOpts, comments []*ast.Comment) [ rule.Else.Head.Value.Location = rule.Else.Head.Location } - return w.writeRule(rule.Else, true, o, comments) + return w.writeRule(rule.Else, true, comments) } -func (w *writer) writeHead(head *ast.Head, isDefault, isExpandedConst bool, o fmtOpts, comments []*ast.Comment) []*ast.Comment { +func (w *writer) writeHead(head *ast.Head, isDefault, isExpandedConst bool, comments []*ast.Comment) []*ast.Comment { ref := head.Ref() if head.Key != nil && head.Value == nil && !head.HasDynamicRef() { ref = ref.GroundPrefix() } - if o.refHeads || len(ref) == 1 { - w.write(ref.String()) + if w.fmtOpts.refHeads || len(ref) == 1 { + w.writeRef(ref) } else { w.write(ref[0].String()) w.write("[") @@ -548,7 +570,7 @@ func (w *writer) writeHead(head *ast.Head, isDefault, isExpandedConst bool, o fm w.write(")") } if head.Key != nil { - if o.contains && head.Value == nil { + if w.fmtOpts.contains && head.Value == nil { w.write(" contains ") comments = w.writeTerm(head.Key, comments) } else if head.Value == nil { // no `if` for p[x] notation @@ -566,7 +588,7 @@ func (w *writer) writeHead(head *ast.Head, isDefault, isExpandedConst bool, o fm // * a.b -> a contains "b" // * a.b.c -> a.b.c := true // * a.b.c.d -> a.b.c.d := true - isRegoV1RefConst := o.regoV1 && isExpandedConst && head.Key == nil && len(head.Args) == 0 + isRegoV1RefConst := w.fmtOpts.regoV1 && isExpandedConst && head.Key == nil && len(head.Args) == 0 if head.Location == head.Value.Location && head.Name != "else" && @@ -578,7 +600,7 @@ func (w *writer) writeHead(head *ast.Head, isDefault, isExpandedConst bool, o fm return comments } - if head.Assign || o.regoV1 { + if head.Assign || w.fmtOpts.regoV1 { // preserve assignment operator, and enforce it if formatting for Rego v1 w.write(" := ") } else { @@ -856,7 +878,7 @@ var varRegexp = regexp.MustCompile("^[[:alpha:]_][[:alpha:][:digit:]_]*$") func (w *writer) writeRefStringPath(s ast.String) { str := string(s) - if varRegexp.MatchString(str) && !ast.IsKeyword(str) { + if varRegexp.MatchString(str) && !ast.IsInKeywords(str, w.fmtOpts.keywords()) { w.write("." + str) } else { w.writeBracketed(s.String()) @@ -1067,7 +1089,8 @@ func (w *writer) writeImports(imports []*ast.Import, comments []*ast.Comment) [] }) for _, i := range group { w.startLine() - w.write(i.String()) + //w.write(i.String()) + w.writeImport(i) if c, ok := m[i]; ok { w.write(" " + c.String()) } @@ -1079,6 +1102,28 @@ func (w *writer) writeImports(imports []*ast.Import, comments []*ast.Comment) [] return comments } +func (w *writer) writeImport(imp *ast.Import) { + path := imp.Path.Value.(ast.Ref) + + buf := []string{"import"} + + if _, ok := future.WhichFutureKeyword(imp); ok { + // We don't want to wrap future.keywords imports in parens, so we create a new writer that doesn't + w2 := writer{ + buf: bytes.Buffer{}, + } + w2.writeRef(path) + buf = append(buf, w2.buf.String()) + } else { + buf = append(buf, path.String()) + } + + if len(imp.Alias) > 0 { + buf = append(buf, "as "+imp.Alias.String()) + } + w.write(strings.Join(buf, " ")) +} + type entryWriter func(interface{}, []*ast.Comment) []*ast.Comment func (w *writer) writeIterable(elements []interface{}, last *ast.Location, close *ast.Location, comments []*ast.Comment, fn entryWriter) []*ast.Comment { diff --git a/format/testfiles/v0/test_contains.rego.formatted b/format/testfiles/v0/test_contains.rego.formatted index 7b712fa9db..b372275212 100644 --- a/format/testfiles/v0/test_contains.rego.formatted +++ b/format/testfiles/v0/test_contains.rego.formatted @@ -1,4 +1,4 @@ -package test.contains +package test["contains"] import future.keywords.contains diff --git a/format/testfiles/v0/test_if.rego.formatted b/format/testfiles/v0/test_if.rego.formatted index b657f105fd..cae30b526c 100644 --- a/format/testfiles/v0/test_if.rego.formatted +++ b/format/testfiles/v0/test_if.rego.formatted @@ -1,4 +1,4 @@ -package test.if +package test["if"] import future.keywords.if diff --git a/format/testfiles/v0/test_if_else.rego.formatted b/format/testfiles/v0/test_if_else.rego.formatted index 76aeb5639e..12880d2cfb 100644 --- a/format/testfiles/v0/test_if_else.rego.formatted +++ b/format/testfiles/v0/test_if_else.rego.formatted @@ -1,4 +1,4 @@ -package test.if +package test["if"] import future.keywords.if diff --git a/format/testfiles/v0/test_in.rego b/format/testfiles/v0/test_in.rego new file mode 100644 index 0000000000..63be7918b1 --- /dev/null +++ b/format/testfiles/v0/test_in.rego @@ -0,0 +1,13 @@ +package test.in + +import future.keywords.in + +a["in"] := "foo" + +b.c["in"] := "bar" + +c["in"].d := "baz" + +p { + input["in"] +} diff --git a/format/testfiles/v0/test_in.rego.formatted b/format/testfiles/v0/test_in.rego.formatted new file mode 100644 index 0000000000..8d4dec17f8 --- /dev/null +++ b/format/testfiles/v0/test_in.rego.formatted @@ -0,0 +1,13 @@ +package test["in"] + +import future.keywords.in + +a["in"] := "foo" + +b.c["in"] := "bar" + +c["in"].d := "baz" + +p { + input["in"] +} diff --git a/format/testfiles/v0_to_v1/keyword_errors.rego b/format/testfiles/v0_to_v1/keyword_errors.rego new file mode 100644 index 0000000000..f77a4a4c0a --- /dev/null +++ b/format/testfiles/v0_to_v1/keyword_errors.rego @@ -0,0 +1,13 @@ +package test + +if := 1 + +contains := 2 + +in := 3 + +every := 4 + +p { + data.foo.contains.bar == 42 +} diff --git a/format/testfiles/v0_to_v1/keyword_errors.rego.error b/format/testfiles/v0_to_v1/keyword_errors.rego.error new file mode 100644 index 0000000000..cc44add905 --- /dev/null +++ b/format/testfiles/v0_to_v1/keyword_errors.rego.error @@ -0,0 +1,5 @@ +4 errors occurred: +testfiles/v0_to_v1/keyword_errors.rego:3: rego_parse_error: if keyword cannot be used for rule name +testfiles/v0_to_v1/keyword_errors.rego:5: rego_parse_error: contains keyword cannot be used for rule name +testfiles/v0_to_v1/keyword_errors.rego:7: rego_parse_error: in keyword cannot be used for rule name +testfiles/v0_to_v1/keyword_errors.rego:9: rego_parse_error: every keyword cannot be used for rule name \ No newline at end of file diff --git a/format/testfiles/v0_to_v1/keywords.rego b/format/testfiles/v0_to_v1/keywords.rego index b2b791436e..5ad60d0ee2 100644 --- a/format/testfiles/v0_to_v1/keywords.rego +++ b/format/testfiles/v0_to_v1/keywords.rego @@ -1,9 +1,5 @@ -package test +package test.if.contains.in.every -if := 1 - -contains := 2 - -in := 3 - -every := 4 +p { + data.if.contains.in.every == 42 +} diff --git a/format/testfiles/v0_to_v1/keywords.rego.error b/format/testfiles/v0_to_v1/keywords.rego.error deleted file mode 100644 index 9ec40a4eba..0000000000 --- a/format/testfiles/v0_to_v1/keywords.rego.error +++ /dev/null @@ -1,5 +0,0 @@ -4 errors occurred: -testfiles/v0_to_v1/keywords.rego:3: rego_parse_error: if keyword cannot be used for rule name -testfiles/v0_to_v1/keywords.rego:5: rego_parse_error: contains keyword cannot be used for rule name -testfiles/v0_to_v1/keywords.rego:7: rego_parse_error: in keyword cannot be used for rule name -testfiles/v0_to_v1/keywords.rego:9: rego_parse_error: every keyword cannot be used for rule name \ No newline at end of file diff --git a/format/testfiles/v0_to_v1/keywords.rego.formatted b/format/testfiles/v0_to_v1/keywords.rego.formatted new file mode 100644 index 0000000000..b4bdb7ac1d --- /dev/null +++ b/format/testfiles/v0_to_v1/keywords.rego.formatted @@ -0,0 +1,7 @@ +package test["if"]["contains"]["in"]["every"] + +import rego.v1 + +p if { + data["if"]["contains"]["in"]["every"] == 42 +} diff --git a/format/testfiles/v1/test_contains.rego b/format/testfiles/v1/test_contains.rego index 2af8286e54..b0728edde2 100644 --- a/format/testfiles/v1/test_contains.rego +++ b/format/testfiles/v1/test_contains.rego @@ -1,4 +1,4 @@ -package test.contains2 # FIXME: refs aren't allowed to contains keywords +package test["contains"] p contains "foo" if { true } @@ -9,3 +9,5 @@ deny contains msg if {msg := "bar" } # partial objects unchanged o[k] = v if { k := "ok"; v := "nok" } + +foo["contains"] := 42 diff --git a/format/testfiles/v1/test_contains.rego.formatted b/format/testfiles/v1/test_contains.rego.formatted index 172c99aa75..7f3a09ab3c 100644 --- a/format/testfiles/v1/test_contains.rego.formatted +++ b/format/testfiles/v1/test_contains.rego.formatted @@ -1,4 +1,4 @@ -package test.contains2 # FIXME: refs aren't allowed to contains keywords +package test["contains"] p contains "foo" @@ -13,3 +13,5 @@ o[k] := v if { k := "ok" v := "nok" } + +foo["contains"] := 42 diff --git a/format/testfiles/v1/test_contains_if.rego b/format/testfiles/v1/test_contains_if.rego index bbb9c727df..04ab519107 100644 --- a/format/testfiles/v1/test_contains_if.rego +++ b/format/testfiles/v1/test_contains_if.rego @@ -1,4 +1,4 @@ -package test.if2 # FIXME: refs aren't allowed to contains keywords +package test["if"] q[x] = y if { y := 10 diff --git a/format/testfiles/v1/test_contains_if.rego.formatted b/format/testfiles/v1/test_contains_if.rego.formatted index 268c86b4c7..4375b0d3de 100644 --- a/format/testfiles/v1/test_contains_if.rego.formatted +++ b/format/testfiles/v1/test_contains_if.rego.formatted @@ -1,4 +1,4 @@ -package test.if2 # FIXME: refs aren't allowed to contains keywords +package test["if"] q[x] := y if { y := 10 diff --git a/format/testfiles/v1/test_if.rego b/format/testfiles/v1/test_if.rego index fc71c338d6..02482f9269 100644 --- a/format/testfiles/v1/test_if.rego +++ b/format/testfiles/v1/test_if.rego @@ -1,4 +1,4 @@ -package test.if2 # FIXME: refs aren't allowed to contains keywords +package test["if"] p if 1 > 0 # shorthand diff --git a/format/testfiles/v1/test_if.rego.formatted b/format/testfiles/v1/test_if.rego.formatted index cfeba398a6..90f8b37d1d 100644 --- a/format/testfiles/v1/test_if.rego.formatted +++ b/format/testfiles/v1/test_if.rego.formatted @@ -1,4 +1,4 @@ -package test.if2 # FIXME: refs aren't allowed to contains keywords +package test["if"] p if 1 > 0 # shorthand diff --git a/format/testfiles/v1/test_if_else.rego b/format/testfiles/v1/test_if_else.rego index d32186d979..e894902873 100644 --- a/format/testfiles/v1/test_if_else.rego +++ b/format/testfiles/v1/test_if_else.rego @@ -1,4 +1,4 @@ -package test.if2 # FIXME: refs aren't allowed to contains keywords +package test["if"] p := 1 if { 1 > 0 } else := 2 diff --git a/format/testfiles/v1/test_if_else.rego.formatted b/format/testfiles/v1/test_if_else.rego.formatted index 4183a0845f..361bcd6299 100644 --- a/format/testfiles/v1/test_if_else.rego.formatted +++ b/format/testfiles/v1/test_if_else.rego.formatted @@ -1,4 +1,4 @@ -package test.if2 # FIXME: refs aren't allowed to contains keywords +package test["if"] p := 1 if 1 > 0 diff --git a/format/testfiles/v1/test_in.rego b/format/testfiles/v1/test_in.rego new file mode 100644 index 0000000000..1397087c80 --- /dev/null +++ b/format/testfiles/v1/test_in.rego @@ -0,0 +1,11 @@ +package test["in"] + +a["in"] := "foo" + +b.c["in"] := "bar" + +c["in"].d := "baz" + +p if { + input["in"] +} diff --git a/format/testfiles/v1/test_in.rego.formatted b/format/testfiles/v1/test_in.rego.formatted new file mode 100644 index 0000000000..1397087c80 --- /dev/null +++ b/format/testfiles/v1/test_in.rego.formatted @@ -0,0 +1,11 @@ +package test["in"] + +a["in"] := "foo" + +b.c["in"] := "bar" + +c["in"].d := "baz" + +p if { + input["in"] +} diff --git a/internal/future/filter_imports.go b/internal/future/filter_imports.go index 2863aad4e9..cf5721101a 100644 --- a/internal/future/filter_imports.go +++ b/internal/future/filter_imports.go @@ -35,3 +35,15 @@ func IsFutureKeyword(imp *ast.Import, kw string) bool { path[1].Equal(ast.StringTerm("keywords")) && path[2].Equal(ast.StringTerm(kw)) } + +func WhichFutureKeyword(imp *ast.Import) (string, bool) { + path := imp.Path.Value.(ast.Ref) + if len(path) == 3 && + ast.FutureRootDocument.Equal(path[0]) && + path[1].Equal(ast.StringTerm("keywords")) { + if str, ok := path[2].Value.(ast.String); ok { + return string(str), true + } + } + return "", false +}