From 353033c65cfedb2ba95c8c5283de2031ecd27729 Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Wed, 3 Jun 2026 12:37:20 +0200 Subject: [PATCH] ast: Apply location to inner `ast.Not` expressions (#8727) Also fixing locations of and/or expressions. --------- Signed-off-by: Johan Fylling --- v1/ast/compile_test.go | 181 ++++++++++++++++------- v1/ast/location/location.go | 3 + v1/ast/marshal_test.go | 11 +- v1/ast/parser.go | 122 ++++++++++------ v1/ast/parser_logical_test.go | 268 ++++++++++++++++++++++++++++++++++ v1/ast/parser_test.go | 40 +++++ 6 files changed, 526 insertions(+), 99 deletions(-) diff --git a/v1/ast/compile_test.go b/v1/ast/compile_test.go index a0ba3e1226..30350381da 100644 --- a/v1/ast/compile_test.go +++ b/v1/ast/compile_test.go @@ -13290,15 +13290,16 @@ func TestCompilerNotImport(t *testing.T) { note: "negated call with vars, inside comprehension, unsafe assignment", module: `package negation import future.keywords.not - + p := [x | not x := "foo" ] - + f(_) := true `, expErrs: Errors{ &Error{ - Code: CompileErr, - Message: "var x is unsafe", + Code: CompileErr, + Message: "var x is unsafe", + Location: &Location{File: "mod.rego", Row: 4, Col: 15, Text: []byte(`not x := "foo"`)}, }, }, }, @@ -13333,15 +13334,16 @@ func TestCompilerNotImport(t *testing.T) { note: "negated assignment", module: `package negation import future.keywords.not - + p if { not a := 1 } `, expErrs: Errors{ &Error{ - Code: CompileErr, - Message: "var a is unsafe", // FIXME: Use more specific error msg: "cannot assign vars inside negated expression" + Code: CompileErr, + Message: "var a is unsafe", // FIXME: Use more specific error msg: "cannot assign vars inside negated expression" + Location: &Location{File: "mod.rego", Row: 5, Col: 6, Text: []byte("not a := 1")}, }, }, }, @@ -13364,15 +13366,16 @@ func TestCompilerNotImport(t *testing.T) { note: "negated assignment, call", module: `package negation import future.keywords.not - + p if { not a := 1 + 2 } `, expErrs: Errors{ &Error{ - Code: CompileErr, - Message: "var a is unsafe", + Code: CompileErr, + Message: "var a is unsafe", + Location: &Location{File: "mod.rego", Row: 5, Col: 6, Text: []byte("not a := 1 + 2")}, }, }, }, @@ -13398,15 +13401,16 @@ func TestCompilerNotImport(t *testing.T) { note: "negated assignment, call, output var", module: `package negation import future.keywords.not - + p if { not plus(1, 2, a) } `, expErrs: Errors{ &Error{ - Code: CompileErr, - Message: "var a is unsafe", + Code: CompileErr, + Message: "var a is unsafe", + Location: &Location{File: "mod.rego", Row: 5, Col: 6, Text: []byte("not plus(1, 2, a)")}, }, }, }, @@ -13431,15 +13435,16 @@ func TestCompilerNotImport(t *testing.T) { note: "negated equality, unsafe var", module: `package negation import future.keywords.not - + p if { not a == 1 } `, expErrs: Errors{ &Error{ - Code: CompileErr, - Message: "var a is unsafe", + Code: CompileErr, + Message: "var a is unsafe", + Location: &Location{File: "mod.rego", Row: 5, Col: 6, Text: []byte("not a == 1")}, }, }, }, @@ -13447,15 +13452,16 @@ func TestCompilerNotImport(t *testing.T) { note: "negated equality, unsafe var, explicit not-body", module: `package negation import future.keywords.not - + p if { not { a == 1 } } `, expErrs: Errors{ &Error{ - Code: CompileErr, - Message: "var a is unsafe", + Code: CompileErr, + Message: "var a is unsafe", + Location: &Location{File: "mod.rego", Row: 5, Col: 12, Text: []byte("a == 1")}, }, }, }, @@ -13463,15 +13469,16 @@ func TestCompilerNotImport(t *testing.T) { note: "negated unification, unsafe var", module: `package negation import future.keywords.not - + p if { not a = 1 } `, expErrs: Errors{ &Error{ - Code: CompileErr, - Message: "var a is unsafe", + Code: CompileErr, + Message: "var a is unsafe", + Location: &Location{File: "mod.rego", Row: 5, Col: 6, Text: []byte("not a = 1")}, }, }, }, @@ -13494,15 +13501,16 @@ func TestCompilerNotImport(t *testing.T) { note: "negated unification, unsafe var, call", module: `package negation import future.keywords.not - + p if { not a = 1 + 2 } `, expErrs: Errors{ &Error{ - Code: CompileErr, - Message: "var a is unsafe", + Code: CompileErr, + Message: "var a is unsafe", + Location: &Location{File: "mod.rego", Row: 5, Col: 6, Text: []byte("not a = 1 + 2")}, }, }, }, @@ -13528,15 +13536,16 @@ func TestCompilerNotImport(t *testing.T) { note: "negated enumeration, unsafe var (wildcard)", module: `package negation import future.keywords.not - + p if { not input.a[_] } `, expErrs: Errors{ &Error{ - Code: CompileErr, - Message: "var _ is unsafe", + Code: CompileErr, + Message: "var _ is unsafe", + Location: &Location{File: "mod.rego", Row: 5, Col: 6, Text: []byte("not input.a[_]")}, }, }, }, @@ -13639,15 +13648,16 @@ func TestCompilerNotImport(t *testing.T) { note: "negated unsafe var", module: `package negation import future.keywords.not - + p if { not a + 2 = 3 } `, expErrs: Errors{ &Error{ - Code: CompileErr, - Message: "var a is unsafe", + Code: CompileErr, + Message: "var a is unsafe", + Location: &Location{File: "mod.rego", Row: 5, Col: 6, Text: []byte("not a + 2 = 3")}, }, }, }, @@ -13655,15 +13665,16 @@ func TestCompilerNotImport(t *testing.T) { note: "negated unsafe var, explicit not-body", module: `package negation import future.keywords.not - + p if { not { a + 2 = 3 } } `, expErrs: Errors{ &Error{ - Code: CompileErr, - Message: "var a is unsafe", + Code: CompileErr, + Message: "var a is unsafe", + Location: &Location{File: "mod.rego", Row: 5, Col: 12, Text: []byte("a + 2")}, }, }, }, @@ -13930,7 +13941,7 @@ func TestCompilerNotImport(t *testing.T) { note: "nested negation (comprehension), unsafe var reference", module: `package negation import future.keywords.not - + p if { not [42 | not v = 2 @@ -13938,8 +13949,9 @@ func TestCompilerNotImport(t *testing.T) { }`, expErrs: Errors{ &Error{ - Code: CompileErr, - Message: "var v is unsafe", + Code: CompileErr, + Message: "var v is unsafe", + Location: &Location{File: "mod.rego", Row: 6, Col: 7, Text: []byte("not v = 2")}, }, }, }, @@ -14140,7 +14152,7 @@ func TestCompilerNotImport(t *testing.T) { note: "explicit not-body, var indirection, unsafe var", module: `package negation import future.keywords.not - + p if { not { a = x @@ -14151,16 +14163,19 @@ func TestCompilerNotImport(t *testing.T) { `, expErrs: Errors{ &Error{ - Code: CompileErr, - Message: "var a is unsafe", + Code: CompileErr, + Message: "var a is unsafe", + Location: &Location{File: "mod.rego", Row: 6, Col: 7, Text: []byte("a = x")}, }, &Error{ - Code: CompileErr, - Message: "var x is unsafe", + Code: CompileErr, + Message: "var x is unsafe", + Location: &Location{File: "mod.rego", Row: 6, Col: 7, Text: []byte("a = x")}, }, &Error{ - Code: CompileErr, - Message: "var b is unsafe", + Code: CompileErr, + Message: "var b is unsafe", + Location: &Location{File: "mod.rego", Row: 7, Col: 7, Text: []byte("b = a")}, }, }, }, @@ -14271,18 +14286,72 @@ func TestCompilerNotImport(t *testing.T) { } `, popts), }, + + { + note: "no import, negated undefined function (regression: GH#8717)", + module: `package negation + + p if { + not f(1) + } + `, + expErrs: Errors{ + &Error{ + Code: TypeErr, + Message: "undefined function f", + Location: &Location{File: "mod.rego", Row: 4, Col: 7, Text: []byte("not f(1)")}, + }, + }, + }, + { + note: "implicit not-body, negated undefined function (regression: GH#8717)", + module: `package negation + import future.keywords.not + + p if { + not f(1) + } + `, + expErrs: Errors{ + &Error{ + Code: TypeErr, + Message: "undefined function f", + Location: &Location{File: "mod.rego", Row: 5, Col: 7, Text: []byte("not f(1)")}, + }, + }, + }, + { + note: "explicit not-body, negated undefined function (regression: GH#8717)", + module: `package negation + import future.keywords.not + + p if { + not { f(1) } + } + `, + expErrs: Errors{ + &Error{ + Code: TypeErr, + Message: "undefined function f", + Location: &Location{File: "mod.rego", Row: 5, Col: 13, Text: []byte("f(1)")}, + }, + }, + }, } for _, tc := range tests { t.Run(tc.note, func(t *testing.T) { + mod, err := ParseModuleWithOpts("mod.rego", tc.module, ParserOptions{ + Capabilities: CapabilitiesForThisVersion(), + }) + if err != nil { + t.Fatalf("unexpected parse error: %v", err) + } c := NewCompiler() - c.Compile(map[string]*Module{"mod.rego": MustParseModuleWithOpts(tc.module, - ParserOptions{ - Capabilities: CapabilitiesForThisVersion(), - })}) + c.Compile(map[string]*Module{"mod.rego": mod}) if len(tc.expErrs) > 0 { - assertErrors(t, c.Errors, tc.expErrs, false) + assertErrors(t, c.Errors, tc.expErrs, true) } else if len(c.Errors) > 0 { if c.Failed() { t.Fatalf("unexpected compile errors: %v", c.Errors) @@ -14293,6 +14362,20 @@ func TestCompilerNotImport(t *testing.T) { if diff := cmp.Diff(tc.expMod, c.Modules["mod.rego"]); diff != "" { t.Errorf("unexpected module (-want, +got):\n%s", diff) } + + // Regression guard for the future.keywords.not location bug (GH#8717) + WalkExprs(c.Modules["mod.rego"], func(expr *Expr) bool { + not, ok := expr.Terms.(*Not) + if !ok { + return false + } + for i, inner := range not.Body { + if inner.Location == nil { + t.Errorf("Not.Body[%d] missing Location: %v", i, inner) + } + } + return false + }) } for n, m := range c.Modules { diff --git a/v1/ast/location/location.go b/v1/ast/location/location.go index 6431b02ce9..e056c9ca13 100644 --- a/v1/ast/location/location.go +++ b/v1/ast/location/location.go @@ -29,6 +29,9 @@ func NewLocation(text []byte, file string, row int, col int) *Location { // Equal checks if two locations are equal to each other. func (loc *Location) Equal(other *Location) bool { + if loc == nil || other == nil { + return loc == other + } return loc.File == other.File && loc.Row == other.Row && loc.Col == other.Col && diff --git a/v1/ast/marshal_test.go b/v1/ast/marshal_test.go index a2b23cbda7..563c4589ad 100644 --- a/v1/ast/marshal_test.go +++ b/v1/ast/marshal_test.go @@ -5,6 +5,7 @@ import ( "strings" "testing" + "github.com/google/go-cmp/cmp" astJSON "github.com/open-policy-agent/opa/v1/ast/json" "github.com/open-policy-agent/opa/v1/util" ) @@ -1193,17 +1194,17 @@ func TestNot_MarshalJSON(t *testing.T) { "implicit body: location included": { Not: module.Rules[0].Body[0].Terms.(*Not), Options: astJSON.Options{MarshalOptions: astJSON.MarshalOptions{IncludeLocation: astJSON.NodeToggle{Not: true}}}, - ExpectedJSON: `{"body":[{"index":0,"terms":[{"type":"ref","value":[{"type":"var","value":"equal"}]},{"type":"call","value":[{"type":"ref","value":[{"type":"var","value":"plus"}]},{"type":"ref","value":[{"type":"var","value":"input"},{"type":"string","value":"x"}]},{"type":"number","value":2}]},{"type":"number","value":42}]}],"explicit_body":false,"location":{"file":"example.rego","row":8,"col":3},"type":"not"}`, + ExpectedJSON: `{"body":[{"index":0,"terms":[{"type":"ref","value":[{"type":"var","value":"equal"}]},{"type":"call","value":[{"type":"ref","value":[{"type":"var","value":"plus"}]},{"type":"ref","value":[{"type":"var","value":"input"},{"type":"string","value":"x"}]},{"type":"number","value":2}]},{"type":"number","value":42}]}],"explicit_body":false,"location":{"file":"example.rego","row":7,"col":4},"type":"not"}`, }, "explicit body: location included": { Not: module.Rules[1].Body[0].Terms.(*Not), Options: astJSON.Options{MarshalOptions: astJSON.MarshalOptions{IncludeLocation: astJSON.NodeToggle{Not: true}}}, - ExpectedJSON: `{"body":[{"index":0,"terms":[{"type":"ref","value":[{"type":"var","value":"assign"}]},{"type":"var","value":"x"},{"type":"ref","value":[{"type":"var","value":"input"},{"type":"string","value":"x"}]}]},{"index":1,"terms":[{"type":"ref","value":[{"type":"var","value":"assign"}]},{"type":"var","value":"y"},{"type":"number","value":2}]},{"index":2,"terms":[{"type":"ref","value":[{"type":"var","value":"assign"}]},{"type":"var","value":"z"},{"type":"call","value":[{"type":"ref","value":[{"type":"var","value":"plus"}]},{"type":"var","value":"x"},{"type":"var","value":"y"}]}]},{"index":3,"terms":[{"type":"ref","value":[{"type":"var","value":"equal"}]},{"type":"var","value":"z"},{"type":"number","value":42}]}],"explicit_body":true,"location":{"file":"example.rego","row":11,"col":8},"type":"not"}`, + ExpectedJSON: `{"body":[{"index":0,"terms":[{"type":"ref","value":[{"type":"var","value":"assign"}]},{"type":"var","value":"x"},{"type":"ref","value":[{"type":"var","value":"input"},{"type":"string","value":"x"}]}]},{"index":1,"terms":[{"type":"ref","value":[{"type":"var","value":"assign"}]},{"type":"var","value":"y"},{"type":"number","value":2}]},{"index":2,"terms":[{"type":"ref","value":[{"type":"var","value":"assign"}]},{"type":"var","value":"z"},{"type":"call","value":[{"type":"ref","value":[{"type":"var","value":"plus"}]},{"type":"var","value":"x"},{"type":"var","value":"y"}]}]},{"index":3,"terms":[{"type":"ref","value":[{"type":"var","value":"equal"}]},{"type":"var","value":"z"},{"type":"number","value":42}]}],"explicit_body":true,"location":{"file":"example.rego","row":11,"col":4},"type":"not"}`, }, "explicit body: location included, also for nested expressions": { Not: module.Rules[1].Body[0].Terms.(*Not), Options: astJSON.Options{MarshalOptions: astJSON.MarshalOptions{IncludeLocation: astJSON.NodeToggle{Not: true, Expr: true}}}, - ExpectedJSON: `{"body":[{"index":0,"location":{"file":"example.rego","row":12,"col":5},"terms":[{"type":"ref","value":[{"type":"var","value":"assign"}]},{"type":"var","value":"x"},{"type":"ref","value":[{"type":"var","value":"input"},{"type":"string","value":"x"}]}]},{"index":1,"location":{"file":"example.rego","row":13,"col":5},"terms":[{"type":"ref","value":[{"type":"var","value":"assign"}]},{"type":"var","value":"y"},{"type":"number","value":2}]},{"index":2,"location":{"file":"example.rego","row":14,"col":5},"terms":[{"type":"ref","value":[{"type":"var","value":"assign"}]},{"type":"var","value":"z"},{"type":"call","value":[{"type":"ref","value":[{"type":"var","value":"plus"}]},{"type":"var","value":"x"},{"type":"var","value":"y"}]}]},{"index":3,"location":{"file":"example.rego","row":15,"col":5},"terms":[{"type":"ref","value":[{"type":"var","value":"equal"}]},{"type":"var","value":"z"},{"type":"number","value":42}]}],"explicit_body":true,"location":{"file":"example.rego","row":11,"col":8},"type":"not"}`, + ExpectedJSON: `{"body":[{"index":0,"location":{"file":"example.rego","row":12,"col":5},"terms":[{"type":"ref","value":[{"type":"var","value":"assign"}]},{"type":"var","value":"x"},{"type":"ref","value":[{"type":"var","value":"input"},{"type":"string","value":"x"}]}]},{"index":1,"location":{"file":"example.rego","row":13,"col":5},"terms":[{"type":"ref","value":[{"type":"var","value":"assign"}]},{"type":"var","value":"y"},{"type":"number","value":2}]},{"index":2,"location":{"file":"example.rego","row":14,"col":5},"terms":[{"type":"ref","value":[{"type":"var","value":"assign"}]},{"type":"var","value":"z"},{"type":"call","value":[{"type":"ref","value":[{"type":"var","value":"plus"}]},{"type":"var","value":"x"},{"type":"var","value":"y"}]}]},{"index":3,"location":{"file":"example.rego","row":15,"col":5},"terms":[{"type":"ref","value":[{"type":"var","value":"equal"}]},{"type":"var","value":"z"},{"type":"number","value":42}]}],"explicit_body":true,"location":{"file":"example.rego","row":11,"col":4},"type":"not"}`, }, } @@ -1216,8 +1217,8 @@ func TestNot_MarshalJSON(t *testing.T) { got := string(bs) exp := data.ExpectedJSON - if got != exp { - t.Fatalf("expected:\n%s got\n%s", exp, got) + if diff := cmp.Diff(exp, got); diff != "" { + t.Errorf("unexpected json: (-want, +got):\n%s", diff) } }) } diff --git a/v1/ast/parser.go b/v1/ast/parser.go index a164cea9ff..8af16314b9 100644 --- a/v1/ast/parser.go +++ b/v1/ast/parser.go @@ -1221,6 +1221,13 @@ func (p *Parser) parseLiteral() (expr *Expr) { if expr != nil { loc.Text = p.s.Text(offset, p.s.lastEnd) expr.SetLoc(loc) + // For implicit not-body wrapping (future.keywords.not), propagate + // the outer `not ` span to the inner expression. + if not, ok := expr.Terms.(*Not); ok && !not.ExplicitBody { + for _, inner := range not.Body { + inner.SetLoc(loc) + } + } } }() @@ -1260,14 +1267,19 @@ func (p *Parser) parseLiteral() (expr *Expr) { if tok == tokens.Dot || tok == tokens.LBrack { p.s.tok = tokens.Ident - return p.parseLiteralExpr(false) + return p.parseLiteralExpr(false, nil) } } - negated := isNegatedExpression(p) + var notLoc *Location + negated := isNegated(p) + if negated { + notLoc = p.s.Loc() + p.scan() + } if negated && p.notBodies && p.s.tok == tokens.LBrace { - nb := p.parseNotBody() + nb := p.parseNotBody(notLoc) if nb != nil && p.s.tok == tokens.With { if nb.With = p.parseWith(); nb.With == nil { @@ -1292,7 +1304,7 @@ func (p *Parser) parseLiteral() (expr *Expr) { } return p.parseEvery() default: - return p.parseLiteralExpr(negated) + return p.parseLiteralExpr(negated, notLoc) } } @@ -1308,7 +1320,7 @@ func (p *Parser) isAllowedRefKeywordStr(s string) bool { return false } -func (p *Parser) parseLiteralExpr(negated bool) *Expr { +func (p *Parser) parseLiteralExpr(negated bool, notLoc *Location) *Expr { startOffset := p.s.loc.Offset startLoc := p.s.Loc() s := p.save() @@ -1343,7 +1355,14 @@ func (p *Parser) parseLiteralExpr(negated bool) *Expr { // Move 'with' statement to outer not expr w := expr.With expr.With = nil - expr = NewExpr(&Not{Body: NewBody(expr), Location: p.s.Loc()}) + + var spanned *Location + if notLoc != nil { + // Extend the location to also include the 'not ' prefix + spanned = p.extendLoc(notLoc) + } + + expr = NewExpr(&Not{Body: NewBody(expr), Location: spanned}).SetLocation(spanned) expr.With = w } else { expr.Negated = negated @@ -1506,18 +1525,19 @@ func (p *Parser) parseSome() *Expr { return NewExpr(decl).SetLocation(decl.Location) } -func (p *Parser) parseNotBody() *Expr { - loc := p.s.Loc() - p.scan() +func (p *Parser) parseNotBody(notLoc *Location) *Expr { + p.scan() // consume `{` body := p.parseBody(tokens.RBrace) if body == nil { return nil } - p.scan() + p.scan() // consume `}` - not := &Not{Body: body, ExplicitBody: true, Location: loc} - return NewExpr(not).SetLocation(loc) + // Extend the location to also include the 'not ' prefix + spanned := p.extendLoc(notLoc) + not := &Not{Body: body, ExplicitBody: true, Location: spanned} + return NewExpr(not).SetLocation(spanned) } // logicalKeywordsActive reports whether the scanner currently treats `and` or @@ -1549,19 +1569,19 @@ func (p *Parser) parseLogicalOrChain(lhsBody Body, lhsExplicit bool, lhsLoc *Loc } lhsBody = NewBody(andExpr) lhsExplicit = false + lhsLoc = andExpr.Location } for p.s.tok == tokens.LogicalOr { p.scan() - rhsBody, rhsExplicit := p.parseLogicalOperand() + rhsBody, rhsExplicit, rhsLoc := p.parseLogicalOperand() if rhsBody == nil { return nil } // RHS may extend into a higher-precedence `and`-chain. if p.s.tok == tokens.LogicalAnd { - rhsLoc := rhsBody[0].Location andExpr := p.parseLogicalAndChain(rhsBody, rhsExplicit, rhsLoc) if andExpr == nil { return nil @@ -1570,16 +1590,18 @@ func (p *Parser) parseLogicalOrChain(lhsBody Body, lhsExplicit bool, lhsLoc *Loc rhsExplicit = false } + exprLoc := p.extendLoc(lhsLoc) node := &LogicalOr{ Lhs: lhsBody, Rhs: rhsBody, ExplicitLhs: lhsExplicit, ExplicitRhs: rhsExplicit, - Location: lhsLoc, + Location: exprLoc, } - wrapper := NewExpr(node).SetLocation(lhsLoc) + wrapper := NewExpr(node).SetLocation(exprLoc) lhsBody = NewBody(wrapper) lhsExplicit = false + lhsLoc = exprLoc } return lhsBody[0] @@ -1600,72 +1622,81 @@ func (p *Parser) parseLogicalAndChain(lhsBody Body, lhsExplicit bool, lhsLoc *Lo for p.s.tok == tokens.LogicalAnd { p.scan() - rhsBody, rhsExplicit := p.parseLogicalOperand() + rhsBody, rhsExplicit, _ := p.parseLogicalOperand() if rhsBody == nil { return nil } + exprLoc := p.extendLoc(lhsLoc) node := &LogicalAnd{ Lhs: lhsBody, Rhs: rhsBody, ExplicitLhs: lhsExplicit, ExplicitRhs: rhsExplicit, - Location: lhsLoc, + Location: exprLoc, } - wrapper := NewExpr(node).SetLocation(lhsLoc) + wrapper := NewExpr(node).SetLocation(exprLoc) lhsBody = NewBody(wrapper) lhsExplicit = false + lhsLoc = exprLoc } return lhsBody[0] } -func isNegatedExpression(p *Parser) bool { - if p.s.tok == tokens.Not { - // Distinguish the `not` keyword from a ref like `not.x`. - s := p.save() - p.scanWS() - tok := p.s.tok - p.restore(s) - if tok != tokens.Dot && tok != tokens.LBrack { - p.scan() - return true - } +// extendLoc returns a copy of start with Text re-spanned from start.Offset +// to the scanner's current lastEnd. +func (p *Parser) extendLoc(start *Location) *Location { + cpy := *start + cpy.Text = p.s.Text(start.Offset, p.s.lastEnd) + return &cpy +} + +func isNegated(p *Parser) bool { + if p.s.tok != tokens.Not { + return false } - return false + // Distinguish the `not` keyword from a ref like `not.x`. + s := p.save() + p.scanWS() + tok := p.s.tok + p.restore(s) + return tok != tokens.Dot && tok != tokens.LBrack } // parseLogicalOperand parses a single operand of an `and`/`or` expression. -// Returns the operand body and whether it was parsed from an explicit `{...}` -// body. Returns (nil, false) on parse error. -func (p *Parser) parseLogicalOperand() (Body, bool) { +func (p *Parser) parseLogicalOperand() (Body, bool, *Location) { if p.s.tok == tokens.LBrace { loc := p.s.Loc() p.scan() body := p.parseBody(tokens.RBrace) if body == nil { - return nil, false + return nil, false, nil } p.scan() - _ = loc - return body, true + return body, true, loc } - negated := isNegatedExpression(p) + var notLoc *Location + negated := isNegated(p) + if negated { + notLoc = p.s.Loc() + p.scan() + } if negated && p.notBodies && p.s.tok == tokens.LBrace { - nb := p.parseNotBody() + nb := p.parseNotBody(notLoc) if nb == nil { - return nil, false + return nil, false, nil } - return NewBody(nb), false + return NewBody(nb), false, nb.Location } startOffset := p.s.loc.Offset startLoc := p.s.Loc() expr := p.parseExpr() if expr == nil { - return nil, false + return nil, false, nil } if expr.Location == nil { @@ -1675,13 +1706,14 @@ func (p *Parser) parseLogicalOperand() (Body, bool) { if negated && p.notBodies { // Don't attach any existing 'with' statements, they belong to the and/or, not the negated expression. - notNode := &Not{Body: NewBody(expr), Location: expr.Location} - expr = NewExpr(notNode).SetLocation(expr.Location) + spanned := p.extendLoc(notLoc) + notNode := &Not{Body: NewBody(expr), Location: spanned} + expr = NewExpr(notNode).SetLocation(spanned) } else if negated { expr.Negated = true } - return NewBody(expr), false + return NewBody(expr), false, expr.Location } func (p *Parser) parseEvery() *Expr { diff --git a/v1/ast/parser_logical_test.go b/v1/ast/parser_logical_test.go index cb847e3512..c28c411f17 100644 --- a/v1/ast/parser_logical_test.go +++ b/v1/ast/parser_logical_test.go @@ -1,6 +1,7 @@ package ast import ( + "bytes" "strings" "testing" ) @@ -601,3 +602,270 @@ func TestParseLogical_RoundTripString(t *testing.T) { }) } } + +func TestParseLogical_ChainLocations(t *testing.T) { + // `not` enabled so the `not { … }` explicit not-body RHS case parses. + opts := logicalParserOpts("not") + + type chainLoc struct { + col int + text string + } + + tests := []struct { + note string + input string + chains []chainLoc // expected (Col, Text) on every chain wrapper, in walk order + }{ + { + note: "simple and", + input: "x and y", + chains: []chainLoc{{col: 1, text: "x and y"}}, + }, + { + note: "simple or", + input: "x or y", + chains: []chainLoc{{col: 1, text: "x or y"}}, + }, + { + note: "and-chain left-associative", + input: "x and y and z", + chains: []chainLoc{ + {col: 1, text: "x and y and z"}, + {col: 1, text: "x and y"}, + }, + }, + { + note: "or-chain left-associative", + input: "x or y or z", + chains: []chainLoc{ + {col: 1, text: "x or y or z"}, + {col: 1, text: "x or y"}, + }, + }, + { + note: "mixed precedence", + input: "x or y and z", + chains: []chainLoc{ + {col: 1, text: "x or y and z"}, + {col: 6, text: "y and z"}, + }, + }, + { + note: "mixed precedence — and tighter than or", + input: "x and y or z", + chains: []chainLoc{ + {col: 1, text: "x and y or z"}, + {col: 1, text: "x and y"}, + }, + }, + { + note: "mixed precedence — and-chains on both sides of or", + input: "x and y or z and w", + chains: []chainLoc{ + {col: 1, text: "x and y or z and w"}, + {col: 1, text: "x and y"}, + {col: 12, text: "z and w"}, + }, + }, + { + note: "LHS explicit body, and", + input: "{ x; y } and z", + chains: []chainLoc{{col: 1, text: "{ x; y } and z"}}, + }, + { + note: "LHS explicit body, or", + input: "{ x; y } or z", + chains: []chainLoc{{col: 1, text: "{ x; y } or z"}}, + }, + { + note: "RHS explicit body, and", + input: "x and { y; z }", + chains: []chainLoc{{col: 1, text: "x and { y; z }"}}, + }, + { + note: "RHS explicit body extending into and-chain — anchors at `{`", + input: "a or { x; y } and z", + chains: []chainLoc{ + {col: 1, text: "a or { x; y } and z"}, + {col: 6, text: "{ x; y } and z"}, + }, + }, + { + // Branch coverage for the `negated && notBodies && LBrace` arm + // of parseLogicalOperand. The constructed *Not's Location + // anchors at the `not` keyword and spans the full `not { ... }` + // text, so the surrounding chain inherits that span. + note: "RHS negated explicit not-body extending into and-chain", + input: "a or not { x; y } and z", + chains: []chainLoc{ + {col: 1, text: "a or not { x; y } and z"}, + {col: 6, text: "not { x; y } and z"}, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.note, func(t *testing.T) { + body, err := ParseBodyWithOpts(tc.input, opts) + if err != nil { + t.Fatalf("parse error: %v", err) + } + if len(body) != 1 { + t.Fatalf("expected 1 expr in body, got %d", len(body)) + } + + var got []chainLoc + collectChainLocs(t, body[0], func(col int, text string) { + got = append(got, chainLoc{col: col, text: text}) + }) + + if len(got) != len(tc.chains) { + t.Fatalf("expected %d chain wrappers, got %d: %+v", len(tc.chains), len(got), got) + } + + for i, want := range tc.chains { + if got[i].col != want.col { + t.Errorf("chain[%d] Col = %d, want %d", i, got[i].col, want.col) + } + if got[i].text != want.text { + t.Errorf("chain[%d] Text = %q, want %q", i, got[i].text, want.text) + } + } + }) + } +} + +func collectChainLocs(t *testing.T, e *Expr, emit func(col int, text string)) { + t.Helper() + if e == nil { + return + } + var nodeLoc *Location + var lhs, rhs Body + switch terms := e.Terms.(type) { + case *LogicalAnd: + nodeLoc = terms.Location + lhs, rhs = terms.Lhs, terms.Rhs + case *LogicalOr: + nodeLoc = terms.Location + lhs, rhs = terms.Lhs, terms.Rhs + default: + return + } + + if e.Location == nil { + t.Fatalf("chain wrapper Expr.Location is nil") + } + if nodeLoc == nil { + t.Fatalf("chain node.Location is nil") + } + if e.Location.Col != nodeLoc.Col || e.Location.Row != nodeLoc.Row || !bytes.Equal(e.Location.Text, nodeLoc.Text) { + t.Errorf("chain wrapper Expr.Location %+v != node.Location %+v", e.Location, nodeLoc) + } + + emit(e.Location.Col, string(e.Location.Text)) + for _, x := range lhs { + collectChainLocs(t, x, emit) + } + for _, x := range rhs { + collectChainLocs(t, x, emit) + } +} + +func TestParseLogical_InnerExprHasLocation(t *testing.T) { + module := `package test + import future.keywords.and + import future.keywords.or + + p if { + f(1) and f(2) + f(3) or f(4) + } + ` + + popts := ParserOptions{ + Capabilities: CapabilitiesForThisVersion(CapabilitiesExperimentalKeywords(true)), + } + + mod, err := ParseModuleWithOpts("test.rego", module, popts) + if err != nil { + t.Fatalf("unexpected parse error: %v", err) + } + + outer := mod.Rules[0].Body[0] + if outer.Location == nil { + t.Fatalf("outer and Expr has nil Location") + } + + and := outer.Terms.(*LogicalAnd) + if and.Location == nil { + t.Fatalf("LogicalAnd.Location is nil") + } + + inner := and.Lhs[0] + if inner.Location == nil { + t.Fatalf("inner Expr inside LogicalAnd.Lhs has nil Location") + } + if inner.Location.Col != 4 { + t.Errorf("Expected column to be 4 but got: %v", inner.Location.Col) + } + if inner.Location.Row != 6 { + t.Errorf("Expected row to be 6 but got: %v", inner.Location.Row) + } + if inner.Location.File != "test.rego" { + t.Errorf("Expected file to be test.rego but got: %v", inner.Location.File) + } + + inner = and.Rhs[0] + if inner.Location == nil { + t.Fatalf("inner Expr inside LogicalAnd.Rhs has nil Location") + } + if inner.Location.Col != 13 { + t.Errorf("Expected column to be 4 but got: %v", inner.Location.Col) + } + if inner.Location.Row != 6 { + t.Errorf("Expected row to be 6 but got: %v", inner.Location.Row) + } + if inner.Location.File != "test.rego" { + t.Errorf("Expected file to be test.rego but got: %v", inner.Location.File) + } + + outer = mod.Rules[0].Body[1] + if outer.Location == nil { + t.Fatalf("outer or Expr has nil Location") + } + + or := outer.Terms.(*LogicalOr) + if and.Location == nil { + t.Fatalf("LogicalOr.Location is nil") + } + + inner = or.Lhs[0] + if inner.Location == nil { + t.Fatalf("inner Expr inside LogicalOr.Lhs has nil Location") + } + if inner.Location.Col != 4 { + t.Errorf("Expected column to be 4 but got: %v", inner.Location.Col) + } + if inner.Location.Row != 7 { + t.Errorf("Expected row to be 7 but got: %v", inner.Location.Row) + } + if inner.Location.File != "test.rego" { + t.Errorf("Expected file to be test.rego but got: %v", inner.Location.File) + } + + inner = or.Rhs[0] + if inner.Location == nil { + t.Fatalf("inner Expr inside LogicalOr.Lhs has nil Location") + } + if inner.Location.Col != 12 { + t.Errorf("Expected column to be 12 but got: %v", inner.Location.Col) + } + if inner.Location.Row != 7 { + t.Errorf("Expected row to be 7 but got: %v", inner.Location.Row) + } + if inner.Location.File != "test.rego" { + t.Errorf("Expected file to be test.rego but got: %v", inner.Location.File) + } +} diff --git a/v1/ast/parser_test.go b/v1/ast/parser_test.go index 6597e6c3ce..6a29fab12c 100644 --- a/v1/ast/parser_test.go +++ b/v1/ast/parser_test.go @@ -9458,3 +9458,43 @@ func TestNotImport(t *testing.T) { }) } } + +// TestParseNotBody_InnerExprHasLocation regression test for: GH#8717 +func TestParseNotBody_InnerExprHasLocation(t *testing.T) { + module := `package test + import future.keywords.not + + p if { + not f(1) + } + ` + + mod, err := ParseModule("test.rego", module) + if err != nil { + t.Fatalf("unexpected parse error: %v", err) + } + + outer := mod.Rules[0].Body[0] + if outer.Location == nil { + t.Fatalf("outer Expr has nil Location") + } + + not := outer.Terms.(*Not) + if not.Location == nil { + t.Fatalf("Not.Location is nil") + } + + inner := not.Body[0] + if inner.Location == nil { + t.Fatalf("inner Expr inside Not.Body has nil Location") + } + if inner.Location.Col != 4 { + t.Errorf("Expected column to be 4 but got: %v", inner.Location.Col) + } + if inner.Location.Row != 5 { + t.Errorf("Expected row to be 5 but got: %v", inner.Location.Row) + } + if inner.Location.File != "test.rego" { + t.Errorf("Expected file to be test.rego but got: %v", inner.Location.File) + } +}