From bdd4646352844ceb2fcf29c734ac700305747898 Mon Sep 17 00:00:00 2001 From: Shuvam Pal <64604251+unichronic@users.noreply.github.com> Date: Tue, 7 Jul 2026 01:34:38 +0530 Subject: [PATCH] ast: improve rule conflict error (#8802) fixes #6391 ### Why the changes in this PR are needed? 1. the rewritten var `__local0__` is output instead of s for rule p.r[s]. 2. no location is given for the conflicting refs data.play.p.q and data.play.p.r[`__local0__`]. ### What are the changes in this PR? Tracking rule location as well along with ref now. Also added formatting to keep the different conflicts multiline. Signed-off-by: unichronic --- v1/ast/compile.go | 80 ++++++++++++++++++++---------------------- v1/ast/compile_test.go | 38 ++++++++++++-------- 2 files changed, 61 insertions(+), 57 deletions(-) diff --git a/v1/ast/compile.go b/v1/ast/compile.go index f1453dfcc1..6e3b36ec0f 100644 --- a/v1/ast/compile.go +++ b/v1/ast/compile.go @@ -1346,7 +1346,7 @@ func (c *Compiler) checkRuleConflicts() { partialRules := 0 arities := make(map[int]struct{}, len(rules)) name := "" - var conflicts []Ref + var conflicts []ruleRef defaultRules := make([]*Rule, 0) for _, rule := range rules { @@ -1422,7 +1422,7 @@ func (c *Compiler) checkRuleConflicts() { switch { case conflicts != nil: - return !c.err(NewError(TypeErr, rules[0].Loc(), "rule %v conflicts with %v", name, conflicts)) + return !c.err(NewError(TypeErr, rules[0].Loc(), "rule %v conflicts with%v", name, formatConflict(conflicts, rw))) case len(kinds) > 1 || len(arities) > 1 || (completeRules >= 1 && partialRules >= 1): return !c.err(NewError(TypeErr, rules[0].Loc(), "conflicting rules %v found", name)) @@ -4482,30 +4482,50 @@ func attachValueToNode(node *TreeNode, ref Ref, val any) { } } +type ruleRef struct { + ref Ref + loc *Location +} + // flattenChildren flattens all children's rule refs into a sorted array. -func (n *TreeNode) flattenChildren() []Ref { +func (n *TreeNode) flattenChildren() []ruleRef { return n.flattenMatchingChildren(func(_ *Rule) bool { return true }) } // flattenChildFunctions is like flattenChildren but only collects functions (rules with args). -func (n *TreeNode) flattenChildFunctions() []Ref { +func (n *TreeNode) flattenChildFunctions() []ruleRef { return n.flattenMatchingChildren(func(r *Rule) bool { return r.isFunction() }) } -func (n *TreeNode) flattenMatchingChildren(f func(*Rule) bool) []Ref { - ret := newRefSet() +func (n *TreeNode) flattenMatchingChildren(f func(*Rule) bool) []ruleRef { + var ret ruleRefSet for _, sub := range n.Children { // we only want the children, so don't use n.DepthFirst() right away sub.DepthFirst(func(x *TreeNode) bool { for _, rule := range x.Values { if f(rule) { - ret.AddPrefix(rule.Ref()) + ret.AddPrefix(ruleRef{ref: rule.Ref(), loc: rule.Loc()}) } } return false }) } - return util.SortedFunc(ret.s, RefCompare) + return util.SortedFunc(ret.s, func(a, b ruleRef) int { + return RefCompare(a.ref, b.ref) + }) +} + +func formatConflict(conflicts []ruleRef, rw varRewriter) string { + s := strings.Builder{} + s.WriteString(":\n") + for _, conflict := range conflicts { + s.WriteString(" rule ") + s.WriteString(rw(conflict.ref.Copy()).String()) + s.WriteString(" at ") + s.WriteString(conflict.loc.String()) + s.WriteString("\n") + } + return strings.TrimSuffix(s.String(), "\n") } // Copy creates a shallow copy of the TreeNode suitable for augmentation. @@ -7234,48 +7254,24 @@ func rewriteVarsInRef(vars ...map[Var]Var) varRewriter { } } -// NOTE(sr): This is duplicated with compile/compile.go; but moving it into another location -// would cause a circular dependency -- the refSet definition needs ast.Ref. If we make it -// public in the ast package, the compile package could take it from there, but it would also -// increase our public interface. Let's reconsider if we need it in a third place. -type refSet struct { - s []Ref -} - -func newRefSet(x ...Ref) *refSet { - result := &refSet{} - for i := range x { - result.AddPrefix(x[i]) - } - return result -} - -// ContainsPrefix returns true if r is prefixed by any of the existing refs in the set. -func (rs *refSet) ContainsPrefix(r Ref) bool { - return slices.ContainsFunc(rs.s, r.HasPrefix) +type ruleRefSet struct { + s []ruleRef } // AddPrefix inserts r into the set if r is not prefixed by any existing // refs in the set. If any existing refs are prefixed by r, those existing // refs are removed. -func (rs *refSet) AddPrefix(r Ref) { - if rs.ContainsPrefix(r) { - return - } - cpy := []Ref{r} +func (rs *ruleRefSet) AddPrefix(r ruleRef) { for i := range rs.s { - if !rs.s[i].HasPrefix(r) { + if r.ref.HasPrefix(rs.s[i].ref) { + return + } + } + cpy := []ruleRef{r} + for i := range rs.s { + if !rs.s[i].ref.HasPrefix(r.ref) { cpy = append(cpy, rs.s[i]) } } rs.s = cpy } - -// Sorted returns a sorted slice of terms for refs in the set. -func (rs *refSet) Sorted() []*Term { - terms := make([]*Term, len(rs.s)) - for i := range rs.s { - terms[i] = NewTerm(rs.s[i]) - } - return util.SortedFunc(terms, TermValueCompare) -} diff --git a/v1/ast/compile_test.go b/v1/ast/compile_test.go index 30350381da..aa0cffd09b 100644 --- a/v1/ast/compile_test.go +++ b/v1/ast/compile_test.go @@ -2064,8 +2064,8 @@ p[r] := 2 if { r := "foo" }`, "rego_type_error: package badrules.s conflicts with rule s defined at mod1.rego:10", "rego_type_error: package badrules.s conflicts with rule s defined at mod1.rego:9", "rego_type_error: package badrules.t conflicts with rule t defined at mod1.rego:11", - "rego_type_error: rule data.badrules.s conflicts with [data.badrules.s.q]", - "rego_type_error: rule data.badrules.t conflicts with [data.badrules.t.q]", + "rego_type_error: rule data.badrules.s conflicts with:\n rule data.badrules.s.q at mod2b.rego:3", + "rego_type_error: rule data.badrules.t conflicts with:\n rule data.badrules.t.q at mod2c.rego:3", } assertCompilerErrorStrings(t, c, expected) @@ -2225,7 +2225,7 @@ func TestCompilerCheckRuleConflictsDotsInRuleHeads(t *testing.T) { p.q.r if { true }`, `package pkg p.q.r.s if { true }`), - err: "rego_type_error: rule data.pkg.p.q.r conflicts with [data.pkg.p.q.r.s]", + err: "rego_type_error: rule data.pkg.p.q.r conflicts with:\n rule data.pkg.p.q.r.s at mod1.rego:2", }, { note: "single-value with other rule overlap", @@ -2234,7 +2234,15 @@ func TestCompilerCheckRuleConflictsDotsInRuleHeads(t *testing.T) { p.q.r if { true } p.q.r.s if { true } p.q.r.t if { true }`), - err: "rego_type_error: rule data.pkg.p.q.r conflicts with [data.pkg.p.q.r.s data.pkg.p.q.r.t]", + err: "rego_type_error: rule data.pkg.p.q.r conflicts with:\n rule data.pkg.p.q.r.s at mod0.rego:3\n rule data.pkg.p.q.r.t at mod0.rego:4", + }, + { + note: "single-value with other rule overlap, unknown key", + modules: modules( + `package pkg + p := x if { x := 1 } + p.r[s] := 2 if { s := "foo" }`), + err: "rego_type_error: rule data.pkg.p conflicts with:\n rule data.pkg.p.r[s] at mod0.rego:3", }, { note: "single-value with other partial object (same ref) overlap", @@ -2309,7 +2317,7 @@ func TestCompilerCheckRuleConflictsDotsInRuleHeads(t *testing.T) { p contains v if { v := ["a", "b"][_] } p.q := 42 `), - err: "rego_type_error: rule data.pkg.p conflicts with [data.pkg.p.q]", + err: "rego_type_error: rule data.pkg.p conflicts with:\n rule data.pkg.p.q at mod0.rego:3", }, { note: "multi-value rule with other rule (ref) overlap", @@ -2318,7 +2326,7 @@ func TestCompilerCheckRuleConflictsDotsInRuleHeads(t *testing.T) { p contains v if { v := ["a", "b"][_] } p.q.r if { true } `), - err: "rego_type_error: rule data.pkg.p conflicts with [data.pkg.p.q.r]", + err: "rego_type_error: rule data.pkg.p conflicts with:\n rule data.pkg.p.q.r at mod0.rego:3", }, { note: "multi-value rule (dots in head) with other rule (ref) overlap", @@ -2328,7 +2336,7 @@ func TestCompilerCheckRuleConflictsDotsInRuleHeads(t *testing.T) { p.q contains v if { v := ["a", "b"][_] } p.q.r if { true } `), - err: "rule data.pkg.p.q conflicts with [data.pkg.p.q.r]", + err: "rule data.pkg.p.q conflicts with:\n rule data.pkg.p.q.r at mod0.rego:4", }, { note: "multi-value rule (dots and var in head) with other rule (ref) overlap", @@ -2346,7 +2354,7 @@ func TestCompilerCheckRuleConflictsDotsInRuleHeads(t *testing.T) { p(x) := x p.q.r if { true } `), - err: "rego_type_error: rule data.pkg.p conflicts with [data.pkg.p.q.r]", + err: "rego_type_error: rule data.pkg.p conflicts with:\n rule data.pkg.p.q.r at mod0.rego:3", }, { note: "function with other rule (ref) overlap", @@ -2355,7 +2363,7 @@ func TestCompilerCheckRuleConflictsDotsInRuleHeads(t *testing.T) { p(x) := x p.q.r if { true } `), - err: "rego_type_error: rule data.pkg.p conflicts with [data.pkg.p.q.r]", + err: "rego_type_error: rule data.pkg.p conflicts with:\n rule data.pkg.p.q.r at mod0.rego:3", }, { note: "function (ref) with other rule (ref) overlap", @@ -2364,7 +2372,7 @@ func TestCompilerCheckRuleConflictsDotsInRuleHeads(t *testing.T) { p.q(x) := x p.q.r if { true } `), - err: "rego_type_error: rule data.pkg.p.q conflicts with [data.pkg.p.q.r]", + err: "rego_type_error: rule data.pkg.p.q conflicts with:\n rule data.pkg.p.q.r at mod0.rego:3", }, { note: "function within dynamic extent of rule", @@ -2373,7 +2381,7 @@ func TestCompilerCheckRuleConflictsDotsInRuleHeads(t *testing.T) { a[x].c := i if some i, x in ["one", "two"] a.b.d(x) := x `), - err: "rego_type_error: rule data.pkg.a[x].c conflicts with [data.pkg.a.b.d]", + err: "rego_type_error: rule data.pkg.a[x].c conflicts with:\n rule data.pkg.a.b.d at mod0.rego:3", }, { note: "function within dynamic extent of rule (different packages)", @@ -2383,7 +2391,7 @@ func TestCompilerCheckRuleConflictsDotsInRuleHeads(t *testing.T) { `package pkg.a.b d(x) := x `), - err: "rego_type_error: rule data.pkg.a[x].c conflicts with [data.pkg.a.b.d]", + err: "rego_type_error: rule data.pkg.a[x].c conflicts with:\n rule data.pkg.a.b.d at mod1.rego:2", }, { note: "function within dynamic extent, deeper nesting", @@ -2392,7 +2400,7 @@ func TestCompilerCheckRuleConflictsDotsInRuleHeads(t *testing.T) { p[x].q if x := "a" p.r.s.t(x) := x `), - err: "rego_type_error: rule data.pkg.p[x].q conflicts with [data.pkg.p.r.s.t]", + err: "rego_type_error: rule data.pkg.p[x].q conflicts with:\n rule data.pkg.p.r.s.t at mod0.rego:3", }, { note: "non-function rule within dynamic extent (no conflict)", @@ -2452,7 +2460,7 @@ func TestCompilerCheckRulePkgConflicts(t *testing.T) { q := 1`), err: []string{ "rego_type_error: package test.p conflicts with rule p defined at mod0.rego:2", - "rego_type_error: rule data.test.p conflicts with [data.test.p.q]", + "rego_type_error: rule data.test.p conflicts with:\n rule data.test.p.q at mod1.rego:2", }, }, { @@ -2464,7 +2472,7 @@ func TestCompilerCheckRulePkgConflicts(t *testing.T) { q := 1`), err: []string{ "rego_type_error: package test.p conflicts with rule p defined at mod0.rego:2", - "rego_type_error: rule data.test.p conflicts with [data.test.p.q]", + "rego_type_error: rule data.test.p conflicts with:\n rule data.test.p.q at mod1.rego:2", }, }, }