Skip to content

Commit e68184d

Browse files
authored
Fix false positivie in prefer-equals-comparison (#1828)
We'd previously fail to account for comprehension term vars in this rule. I'm pretty sure the logic here needs to extend into other locations in Regal, but this at least fixes the immediate issue as reported. Fixes #1826 Signed-off-by: Anders Eknert <anders.eknert@apple.com>
1 parent 54cdcb6 commit e68184d

4 files changed

Lines changed: 87 additions & 2 deletions

File tree

bundle/regal/rules/idiomatic/prefer-equals-comparison/prefer_equals_comparison.rego

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ package regal.rules.idiomatic["prefer-equals-comparison"]
44

55
import data.regal.ast
66
import data.regal.result
7+
import data.regal.util
78

89
report contains violation if {
910
some rule_index, expr
@@ -26,5 +27,33 @@ _unassignable(term, _) if {
2627

2728
_unassignable(term, rule_index) if {
2829
term.type == "var"
29-
not ast.is_output_var(input.rules[to_number(rule_index)], term)
30+
ri := to_number(rule_index)
31+
not ast.is_output_var(input.rules[ri], term)
32+
not _is_declared_comp_term(term, rule_index)
33+
}
34+
35+
_is_declared_comp_term(term, rule_index) if {
36+
some comp_term in _comprehension_term_vars_in_scope(rule_index, term.location)
37+
comp_term.type == "var"
38+
comp_term.value == term.value
39+
}
40+
41+
_comprehension_term_vars_in_scope(rule_index, location) := {node |
42+
loc := util.to_location_object(location)
43+
44+
some comp in ast.found.comprehensions[rule_index]
45+
util.contains_location(util.to_location_object(comp.location), loc)
46+
47+
fields := {
48+
"arraycomprehension": ["term"],
49+
"objectcomprehension": ["key", "value"],
50+
"setcomprehension": ["term"],
51+
}[comp.type]
52+
53+
some field in fields
54+
term := comp.value[field]
55+
56+
walk(term, [_, node])
57+
58+
node.type == "var"
3059
}

bundle/regal/rules/idiomatic/prefer-equals-comparison/prefer_equals_comparison_test.rego

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,3 +117,39 @@ test_success_not_impossible_assignment_output_var_equals_static_ref if {
117117

118118
r == set()
119119
}
120+
121+
test_success_not_impossible_assignment_set_comprehension_term if {
122+
r := rule.report with input as ast.policy(`r := {x | x = y[_].z}`)
123+
124+
r == set()
125+
}
126+
127+
test_success_not_impossible_assignment_set_comprehension_term_complex if {
128+
r := rule.report with input as ast.policy(`r := {{x} | x = y[_].z}`)
129+
130+
r == set()
131+
}
132+
133+
test_success_not_impossible_assignment_array_comprehension_term if {
134+
r := rule.report with input as ast.policy(`r := [x | x = y[_].z]`)
135+
136+
r == set()
137+
}
138+
139+
test_success_not_impossible_assignment_array_comprehension_term_complex if {
140+
r := rule.report with input as ast.policy(`r := [[x] | x = y[_].z]`)
141+
142+
r == set()
143+
}
144+
145+
test_success_not_impossible_assignment_object_comprehension_term if {
146+
r := rule.report with input as ast.policy(`r := {k: v | k = y[v].k}`)
147+
148+
r == set()
149+
}
150+
151+
test_success_not_impossible_assignment_object_comprehension_term_complex if {
152+
r := rule.report with input as ast.policy(`r := {{k}: {v} | k = y[v].k}`)
153+
154+
r == set()
155+
}

bundle/regal/rules/imports/unresolved-reference/unresolved_reference.rego

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,5 +143,5 @@ _to_location_object(loc, text, file) := {"location": {
143143
from_col := substring(text, col - 1, -1)
144144
ref_text := substring(from_col, 0, indexof(from_col, " "))
145145

146-
end_col := to_number(vals[1]) + count(ref_text)
146+
end_col := col + count(ref_text)
147147
}

bundle/regal/util/util.rego

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -228,3 +228,23 @@ parse_bool("FALSE") := false
228228
# METADATA
229229
# description: creates a string where s is repeated n times
230230
repeat(s, n) := replace(sprintf("%-*s", [n, " "]), " ", s)
231+
232+
# METADATA
233+
# description: true if location 'sub' is fully contained within location 'sup'
234+
contains_location(sup, sub) if {
235+
sup.row < sub.row
236+
sup.end.row > sub.end.row
237+
} else if {
238+
sup.row == sub.row
239+
sup.col <= sub.col
240+
sup.end.row > sub.end.row
241+
} else if {
242+
sup.row < sub.row
243+
sup.end.row == sub.end.row
244+
sup.end.col >= sub.end.col
245+
} else if {
246+
sup.row == sub.row
247+
sup.col <= sub.col
248+
sup.end.row == sub.end.row
249+
sup.end.col >= sub.end.col
250+
}

0 commit comments

Comments
 (0)