ast: improve rule conflict error (#8802)

fixes #6391 
<!--

Thanks for submitting a PR to OPA!

Before pressing 'Create pull request' please read the checklist below.

* All code changes should be accompanied with tests. If you are not
modifying any tests, just provide a short explanation of why updates
to tests are not necessary. In addition to helping catch bugs, tests
are extremely helpful in providing _context_ that explains how your
changes can be used.

* All changes to public APIs **must** be accompanied with
docs. Examples of public APIs include built-in functions,
config fields, and of course, exported Go types/functions/constants/etc.

* Commit messages should explain _why_ you made the changes, not what
you changed. Use active voice. Keep the subject line under 50
characters or so.

* All commits must be signed off by the author. If you are not
familiar with signing off, see our contributor guide below.

* You have read the project's
[guidelines on AI tool
use](https://www.openpolicyagent.org/docs/contrib-code#ai-guidelines).

For more information on contributing to OPA see our
[Contributing Guide](https://www.openpolicyagent.org/docs/contributing/)
for high-level contributing guidelines and development setup.
(See the [Developer Certificate of
Origin](https://www.openpolicyagent.org/docs/contrib-code/#developer-certificate-of-origin)
section for specifics on signing off a commit.)

-->

### 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 <ishuvam.pal@gmail.com>

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)
-}

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",
 			},
 		},
 	}