Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion v1/ast/compile.go
Original file line number Diff line number Diff line change
Expand Up @@ -1596,11 +1596,16 @@ func (c *Compiler) checkSafetyRuleHeads() {
if vars.DiffCount(vis.vars) > 0 {
unsafe := vars.Diff(vis.vars)
for v := range unsafe {
// Capture the location before v is rewritten back to its
// original name below: vars is keyed by the (possibly
// rewritten) head var, so looking up vars[v] after the
// reassignment would miss the location. See #8719.
loc := vars[v].Location

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could consider the root cause of the issue to be where the rewrite is happening. Could there be a slight risk of a regression here if the location isn't always attached to the rewritten var?

Maybe we could make this handle a wider variety of upstream mishaps by simply using the location from v or w; whichever is non-nil.

if w, ok := c.RewrittenVars[v]; ok {
v = w
}
if !v.IsGenerated() {
if !c.err(NewError(UnsafeVarErr, vars[v].Location, "var %v is unsafe", v)) {
if !c.err(NewError(UnsafeVarErr, loc, "var %v is unsafe", v)) {
return true
}
}
Expand Down
20 changes: 20 additions & 0 deletions v1/ast/compile_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1899,6 +1899,26 @@ obj := {
},
},
},
{
// Regression test for #8719: an unsafe variable that appears in a
// rule head and is rewritten (e.g. declared with `some`) must still
// report its location.
module: `package test

r[x] := y if {
some x
y := [_]
}`,
expectedErrs: []struct {
message string
location int
}{
{
"var x is unsafe",
3,
},
},
},
}

for _, tc := range tests {
Expand Down
Loading