Begin to Include end location where applicable (#917)

This PR attempts to establish end location as an optional attribute of location data returned in violations, and updates ~10 rules in the bugs category to use this convention. If no end location is provided, the default is as it used to be — end location will be set to the end of the line. For some rules, this is the right thing to do anyway, and therefore, not all rules will need to be updated.. That obviously doesn't "fix" #654, but it's a start, and as far as I've seen so far, works quite well. Importantly this change is non-breaking method for API users! Ideally, we'd put the current "row" and "col" into a "start" object too, but that *would* be a breaking change, and one that would impact all API users, so let's consider that for our first stable release instead. NOTE that this does not change anything in the output of `regal lint` using the default "pretty"-reporter, but is only included in the more detailed JSON and SARIF formats. First and foremost this change helps LSP clients display diagnostics more accurately, and that is really where this is most useful. Signed-off-by: Anders Eknert <[email protected]>
StyraInc · Jul 16, 2024 · 9ed8ec1 · 9ed8ec1
1 parent 988d558
commit 9ed8ec1
Show file tree

Hide file tree

Showing 21 changed files with 227 additions and 126 deletions.
diff --git a/bundle/regal/result.rego b/bundle/regal/result.rego
@@ -56,7 +56,15 @@ _category_title_from_path(path) := [category, title] if ["regal", "rules", categ
 
 _category_title_from_path(path) := [category, title] if ["custom", "regal", "rules", category, title] = path
 
-# Provided rules, i.e. regal.rules.category.title
+# METADATA
+# description: |
+#   helper function to call when building the "return value" for the `report` in any linter rule —
+#   recommendation being that both built-in rules and custom rules use this in favor of building the
+#   result by hand
+# scope: document
+
+# METADATA
+# description: provided rules, i.e. regal.rules.category.title
 fail(metadata, details) := violation if {
 	is_array(metadata) # from rego.metadata.chain()
 
@@ -76,7 +84,8 @@ fail(metadata, details) := violation if {
 	violation := _fail_annotated(annotation, details)
 }
 
-# Custom rules, i.e. custom.regal.rules.category.title
+# METADATA
+# description: custom rules, i.e. custom.regal.rules.category.title
 fail(metadata, details) := violation if {
 	is_array(metadata) # from rego.metadata.chain()
 
@@ -94,8 +103,15 @@ fail(metadata, details) := violation if {
 	violation := _fail_annotated_custom(annotation, details)
 }
 
+# METADATA
+# description: fallback case
 fail(metadata, details) := _fail_annotated(metadata, details)
 
+# METADATA
+# description: |
+#   creates a notice object, i.e. one used to inform users of things like a rule getting
+#   ignored because the set capabilities does not include a dependency (like a buiöt-in function)
+#   needed by the rule
 notice(metadata) := result if {
 	is_array(metadata)
 	rule_meta := metadata[0]
@@ -159,7 +175,7 @@ resource_urls(related_resources, category) := [r |
 	r := object.union(object.remove(item, ["ref"]), {"ref": config.docs.resolve_url(item.ref, category)})
 ]
 
-with_text(location) := {"location": object.union(
+_with_text(location) := {"location": object.union(
 	location,
 	{
 		"file": input.regal.file.name, # regal ignore:external-reference
@@ -169,17 +185,17 @@ with_text(location) := {"location": object.union(
 	location.row
 } else := {"location": location}
 
-location(x) := with_text(x.location) if x.location
+location(x) := _with_text(x.location) if x.location
 
-location(x) := with_text(x[0].location) if is_array(x)
+location(x) := _with_text(x[0].location) if is_array(x)
 
 # Special case for comments, where this typo sadly is hard to change at this point
-location(x) := with_text(x.Location) if x.Location
+location(x) := _with_text(x.Location) if x.Location
 
 # Special case for rule refs, where location is currently only assigned to the value
 # In this case, we'll just assume that the column is 1, as that's the most likely place
 # See: https://github.com/open-policy-agent/opa/issues/5790
-location(x) := with_text(location) if {
+location(x) := _with_text(location) if {
 	not x.location
 	count(x.ref) == 1
 	x.ref[0].type == "var"
@@ -191,3 +207,21 @@ location(x) := {} if {
 	not x.Location
 	count(x.ref) != 1
 }
+
+# METADATA
+# description: |
+#   similar to `location` but includes an `end` attribute where `end.row` is the same
+#   value as x.location.row, and `end.col` is the start col + the length of the text
+#   original attribute value base64 decoded from x.location
+ranged_location_from_text(x) := end if {
+	otext := base64.decode(x.location.text)
+	lines := split(otext, "\n")
+
+	loc := location(x)
+	end := object.union(loc, {"location": {"end": {
+		# note the use of the _original_ location text here, as loc.location.text
+		# will be the entire line where the violation occured
+		"row": (loc.location.row + count(lines)) - 1,
+		"col": loc.location.col + count(regal.last(lines)),
+	}}})
+}
diff --git a/bundle/regal/rules/bugs/argument_always_wildcard.rego b/bundle/regal/rules/bugs/argument_always_wildcard.rego
@@ -7,16 +7,10 @@ import rego.v1
 import data.regal.ast
 import data.regal.result
 
-function_groups[name] contains fn if {
-	some fn in ast.functions
-
-	name := ast.ref_to_string(fn.head.ref)
-}
-
 report contains violation if {
-	some functions in function_groups
+	some functions in _function_groups
 
-	fn := any_member(functions)
+	fn := _any_member(functions)
 
 	some pos in numbers.range(0, count(fn.head.args) - 1)
 
@@ -25,7 +19,13 @@ report contains violation if {
 		startswith(function.head.args[pos].value, "$")
 	}
 
-	violation := result.fail(rego.metadata.chain(), result.location(fn.head.args[pos]))
+	violation := result.fail(rego.metadata.chain(), result.ranged_location_from_text(fn.head.args[pos]))
+}
+
+_function_groups[name] contains fn if {
+	some fn in ast.functions
+
+	name := ast.ref_to_string(fn.head.ref)
 }
 
-any_member(s) := [x | some x in s][0]
+_any_member(s) := [x | some x in s][0]
diff --git a/bundle/regal/rules/bugs/argument_always_wildcard_test.rego b/bundle/regal/rules/bugs/argument_always_wildcard_test.rego
@@ -17,7 +17,7 @@ test_fail_single_function_single_argument_always_a_wildcard if {
 		"category": "bugs",
 		"description": "Argument is always a wildcard",
 		"level": "error",
-		"location": {"col": 4, "file": "policy.rego", "row": 6, "text": "\tf(_) := 1"},
+		"location": {"col": 4, "file": "policy.rego", "row": 6, "text": "\tf(_) := 1", "end": {"col": 5, "row": 6}},
 		"related_resources": [{
 			"description": "documentation",
 			"ref": config.docs.resolve_url("$baseUrl/$category/argument-always-wildcard", "bugs"),
@@ -37,7 +37,7 @@ test_fail_single_argument_always_a_wildcard if {
 		"category": "bugs",
 		"description": "Argument is always a wildcard",
 		"level": "error",
-		"location": {"col": 4, "file": "policy.rego", "row": 6, "text": "\tf(_) := 1"},
+		"location": {"col": 4, "file": "policy.rego", "row": 6, "text": "\tf(_) := 1", "end": {"col": 5, "row": 6}},
 		"related_resources": [{
 			"description": "documentation",
 			"ref": config.docs.resolve_url("$baseUrl/$category/argument-always-wildcard", "bugs"),
@@ -57,7 +57,13 @@ test_fail_single_argument_always_a_wildcard_default_function if {
 		"category": "bugs",
 		"description": "Argument is always a wildcard",
 		"level": "error",
-		"location": {"col": 12, "file": "policy.rego", "row": 6, "text": "\tdefault f(_) := 1"},
+		"location": {
+			"col": 12,
+			"file": "policy.rego",
+			"row": 6,
+			"text": "\tdefault f(_) := 1",
+			"end": {"col": 13, "row": 6},
+		},
 		"related_resources": [{
 			"description": "documentation",
 			"ref": config.docs.resolve_url("$baseUrl/$category/argument-always-wildcard", "bugs"),
@@ -77,7 +83,7 @@ test_fail_multiple_argument_always_a_wildcard if {
 		"category": "bugs",
 		"description": "Argument is always a wildcard",
 		"level": "error",
-		"location": {"col": 7, "file": "policy.rego", "row": 6, "text": "\tf(x, _) := x + 1"},
+		"location": {"col": 7, "file": "policy.rego", "row": 6, "text": "\tf(x, _) := x + 1", "end": {"col": 8, "row": 6}},
 		"related_resources": [{
 			"description": "documentation",
 			"ref": config.docs.resolve_url("$baseUrl/$category/argument-always-wildcard", "bugs"),

diff --git a/bundle/regal/rules/bugs/constant_condition.rego b/bundle/regal/rules/bugs/constant_condition.rego
@@ -18,6 +18,9 @@ _rules_with_bodies := [rule |
 	not ast.generated_body(rule)
 ]
 
+# METADATA
+# description: single scalar value, like a lone `true` inside a rule body
+# scope: rule
 report contains violation if {
 	expr := _rules_with_bodies[_].body[_]
 
@@ -27,9 +30,12 @@ report contains violation if {
 	# however meaningless it may be. Maybe consider for another rule?
 	expr.terms.type in ast.scalar_types
 
-	violation := result.fail(rego.metadata.chain(), result.location(expr))
+	violation := result.fail(rego.metadata.chain(), result.ranged_location_from_text(expr.terms))
 }
 
+# METADATA
+# description: two scalar values with a "boolean operator" between, like 1 == 1, or 2 > 1
+# scope: rule
 report contains violation if {
 	expr := _rules_with_bodies[_].body[_]
 
@@ -39,5 +45,5 @@ report contains violation if {
 	expr.terms[1].type in ast.scalar_types
 	expr.terms[2].type in ast.scalar_types
 
-	violation := result.fail(rego.metadata.chain(), result.location(expr))
+	violation := result.fail(rego.metadata.chain(), result.ranged_location_from_text(expr))
 }
diff --git a/bundle/regal/rules/bugs/constant_condition_test.rego b/bundle/regal/rules/bugs/constant_condition_test.rego
@@ -14,7 +14,7 @@ test_fail_simple_constant_condition if {
 	r == {{
 		"category": "bugs",
 		"description": "Constant condition",
-		"location": {"col": 2, "file": "policy.rego", "row": 4, "text": "\t1"},
+		"location": {"col": 2, "file": "policy.rego", "row": 4, "text": "\t1", "end": {"row": 4, "col": 3}},
 		"related_resources": [{
 			"description": "documentation",
 			"ref": config.docs.resolve_url("$baseUrl/$category/constant-condition", "bugs"),
@@ -34,7 +34,13 @@ test_fail_rule_with_body_looking_generated if {
 	r == {{
 		"category": "bugs",
 		"description": "Constant condition",
-		"location": {"col": 9, "file": "policy.rego", "row": 3, "text": "allow { true }"},
+		"location": {
+			"file": "policy.rego",
+			"col": 9,
+			"row": 3,
+			"end": {"row": 3, "col": 13},
+			"text": "allow { true }",
+		},
 		"related_resources": [{
 			"description": "documentation",
 			"ref": config.docs.resolve_url("$baseUrl/$category/constant-condition", "bugs"),
@@ -51,7 +57,7 @@ test_fail_operator_constant_condition if {
 	r == {{
 		"category": "bugs",
 		"description": "Constant condition",
-		"location": {"col": 2, "file": "policy.rego", "row": 4, "text": "\t1 == 1"},
+		"location": {"col": 2, "file": "policy.rego", "row": 4, "text": "\t1 == 1", "end": {"col": 8, "row": 4}},
 		"related_resources": [{
 			"description": "documentation",
 			"ref": config.docs.resolve_url("$baseUrl/$category/constant-condition", "bugs"),

diff --git a/bundle/regal/rules/bugs/deprecated_builtin.rego b/bundle/regal/rules/bugs/deprecated_builtin.rego
@@ -20,17 +20,11 @@ report contains violation if {
 
 	some ref in ast.all_refs
 
-	ref[0].value[0].type == "var"
-	not ref[0].value[0].value in {"input", "data"}
+	call := ref[0]
 
-	name := concat(".", [value |
-		some part in ref[0].value
-		value := part.value
-	])
+	ast.ref_to_string(call.value) in deprecated_builtins
 
-	name in deprecated_builtins
-
-	violation := result.fail(rego.metadata.chain(), result.location(ref))
+	violation := result.fail(rego.metadata.chain(), result.ranged_location_from_text(call))
 }
 
 any_deprecated_builtin(caps_builtins, deprecated_builtins) if {

diff --git a/bundle/regal/rules/bugs/deprecated_builtin_test.rego b/bundle/regal/rules/bugs/deprecated_builtin_test.rego
@@ -19,7 +19,13 @@ test_fail_call_to_deprecated_builtin_function if {
 		"category": "bugs",
 		"description": "Avoid using deprecated built-in functions",
 		"level": "error",
-		"location": {"col": 3, "file": "policy.rego", "row": 7, "text": "\t\tany([true, false])"},
+		"location": {
+			"col": 3,
+			"file": "policy.rego",
+			"row": 7,
+			"text": "\t\tany([true, false])",
+			"end": {"col": 6, "row": 7},
+		},
 		"related_resources": [{
 			"description": "documentation",
 			"ref": config.docs.resolve_url("$baseUrl/$category/deprecated-builtin", "bugs"),

diff --git a/bundle/regal/rules/bugs/duplicate_rule.rego b/bundle/regal/rules/bugs/duplicate_rule.rego
@@ -19,7 +19,7 @@ report contains violation if {
 	]
 
 	violation := result.fail(rego.metadata.chain(), object.union(
-		result.location(input.rules[first]),
+		result.ranged_location_from_text(input.rules[first]),
 		{"description": message(dup_locations)},
 	))
 }

diff --git a/bundle/regal/rules/bugs/duplicate_rule_test.rego b/bundle/regal/rules/bugs/duplicate_rule_test.rego
@@ -19,12 +19,11 @@ test_fail_simple_duplicate_rule if {
 	`)
 
 	r := rule.report with input as module
-
 	r == {{
 		"category": "bugs",
 		"description": "Duplicate rule found at line 10",
 		"level": "error",
-		"location": {"col": 2, "file": "policy.rego", "row": 6, "text": "\tallow if {"},
+		"location": {"col": 2, "file": "policy.rego", "row": 6, "text": "\tallow if {", "end": {"col": 4, "row": 8}},
 		"related_resources": [{
 			"description": "documentation",
 			"ref": config.docs.resolve_url("$baseUrl/$category/duplicate-rule", "bugs"),
@@ -68,7 +67,7 @@ test_fail_multiple_duplicate_rules if {
 		"category": "bugs",
 		"description": "Duplicate rules found at lines 14, 18",
 		"level": "error",
-		"location": {"col": 2, "file": "policy.rego", "row": 10, "text": "\tallow if {"},
+		"location": {"col": 2, "file": "policy.rego", "row": 10, "text": "\tallow if {", "end": {"col": 4, "row": 12}},
 		"related_resources": [{
 			"description": "documentation",
 			"ref": config.docs.resolve_url("$baseUrl/$category/duplicate-rule", "bugs"),

diff --git a/bundle/regal/rules/bugs/if_empty_object.rego b/bundle/regal/rules/bugs/if_empty_object.rego
@@ -2,10 +2,6 @@
 # description: Empty object following `if`
 package regal.rules.bugs["if-empty-object"]
 
-# NOTE: this rule has been deprecated and is no longer enabled by default
-# Use the `if-object-literal` rule instead, which checks for any object,
-# non-empty or not
-
 import rego.v1
 
 import data.regal.capabilities
@@ -17,6 +13,11 @@ import data.regal.result
 #   severity: warning
 notices contains result.notice(rego.metadata.chain()) if not capabilities.has_if
 
+# METADATA
+# description: |
+#   NOTE: this rule has been deprecated and is no longer enabled by default
+#   Use the `if-object-literal` rule instead, which checks for any object,
+#   non-empty or not
 report contains violation if {
 	some rule in input.rules
 

diff --git a/bundle/regal/rules/bugs/if_object_literal.rego b/bundle/regal/rules/bugs/if_object_literal.rego
@@ -17,8 +17,7 @@ report contains violation if {
 	some rule in input.rules
 
 	count(rule.body) == 1
-
 	rule.body[0].terms.type == "object"
 
-	violation := result.fail(rego.metadata.chain(), result.location(rule))
+	violation := result.fail(rego.metadata.chain(), result.ranged_location_from_text(rule.body[0].terms))
 }
diff --git a/bundle/regal/rules/bugs/if_object_literal_test.rego b/bundle/regal/rules/bugs/if_object_literal_test.rego
@@ -14,7 +14,7 @@ test_fail_if_empty_object if {
 		"category": "bugs",
 		"description": "Object literal following `if`",
 		"level": "error",
-		"location": {"col": 1, "file": "policy.rego", "row": 5, "text": "rule if {}"},
+		"location": {"col": 9, "file": "policy.rego", "row": 5, "text": "rule if {}", "end": {"col": 11, "row": 5}},
 		"related_resources": [{
 			"description": "documentation",
 			"ref": config.docs.resolve_url("$baseUrl/$category/if-object-literal", "bugs"),
@@ -30,7 +30,13 @@ test_fail_non_empty_object if {
 		"category": "bugs",
 		"description": "Object literal following `if`",
 		"level": "error",
-		"location": {"col": 1, "file": "policy.rego", "row": 5, "text": `rule if {"x": input.x}`},
+		"location": {
+			"col": 9,
+			"file": "policy.rego",
+			"row": 5,
+			"text": `rule if {"x": input.x}`,
+			"end": {"col": 23, "row": 5},
+		},
 		"related_resources": [{
 			"description": "documentation",
 			"ref": config.docs.resolve_url("$baseUrl/$category/if-object-literal", "bugs"),

diff --git a/bundle/regal/rules/bugs/inconsistent_args.rego b/bundle/regal/rules/bugs/inconsistent_args.rego
@@ -35,7 +35,25 @@ report contains violation if {
 
 	inconsistent_args(position)
 
-	violation := result.fail(rego.metadata.chain(), result.location(find_function_by_name(name)))
+	violation := result.fail(rego.metadata.chain(), _args_location(find_function_by_name(name)))
+}
+
+_args_location(fn) := loc if {
+	# mostly to get the `text` attribute
+	oloc := result.location(fn)
+
+	farg := fn.head.args[0].location
+	larg := regal.last(fn.head.args).location
+
+	# use the location of the first and last arg for highlighting
+	loc := object.union(oloc, {"location": {
+		"row": farg.row,
+		"col": farg.col,
+		"end": {
+			"row": larg.row,
+			"col": larg.col + count(base64.decode(larg.text)),
+		},
+	}})
 }
 
 inconsistent_args(position) if {