Support ignore directives in aggregate rules (StyraInc#809)

* Support ignore directives in aggregate rules Also some light refactoring, but should hopefully not be too distracting. Fixes StyraInc#803 Signed-off-by: Anders Eknert <anders@styra.com> * Return `null` in textDocument/diagnostic handler (StyraInc#812) While the specification isn't clear here, Neovim apparently had issues dealing with the empty object response. And since all the other editors seem to handle a null response too, let's just go with that instead. Fixes StyraInc#810 Signed-off-by: Anders Eknert <anders@styra.com> --------- Signed-off-by: Anders Eknert <anders@styra.com>
srenatus · Oct 1, 2024 · 08ed836 · 08ed836
1 parent 70364a0
commit 08ed836
Show file tree

Hide file tree

Showing 14 changed files with 202 additions and 52 deletions.
diff --git a/bundle/regal/ast/comments.rego b/bundle/regal/ast/comments.rego
@@ -2,13 +2,20 @@ package regal.ast
 
 import rego.v1
 
+# METADATA
+# description: all comments in the input AST with their "Text" attribute base64 decoded
 comments_decoded := [decoded |
 	some comment in input.comments
 	decoded := object.union(comment, {"Text": base64.decode(comment.Text)})
 ]
 
+# METADATA
+# description: |
+#   an array of partitions, i.e. arrays containing all comments grouped by their "blocks"
 comments["blocks"] := comment_blocks(comments_decoded)
 
+# METADATA
+# description: set of all the standard metadata attribute names, as provided by OPA
 comments["metadata_attributes"] := {
 	"scope",
 	"title",
@@ -21,6 +28,27 @@ comments["metadata_attributes"] := {
 	"custom",
 }
 
+# METADATA
+# description: |
+#   map of all ignore directive comments, like ("# regal ignore:line-length")
+#   found in input AST, indexed by the row they're at
+ignore_directives[row] := rules if {
+	some comment in comments_decoded
+	text := trim_space(comment.Text)
+
+	i := indexof(text, "regal ignore:")
+	i != -1
+
+	list := regex.replace(substring(text, i + 13, -1), `\s`, "")
+
+	row := comment.Location.row + 1
+	rules := split(list, ",")
+}
+
+# METADATA
+# description: |
+#   returns an array of partitions, i.e. arrays containing all comments
+#   grouped by their "blocks".
 comment_blocks(comments) := [partition |
 	rows := [row |
 		some comment in comments

diff --git a/bundle/regal/ast/imports.rego b/bundle/regal/ast/imports.rego
@@ -15,7 +15,7 @@ imports := input.imports
 # description: |
 #   set of all names imported in the input module, meaning commonly the last part of any
 #   imported ref, like "bar" in "data.foo.bar", or an alias like "baz" in "data.foo.bar as baz".
-imported_identifiers contains imported_identifier(imp) if {
+imported_identifiers contains _imported_identifier(imp) if {
 	some imp in imports
 
 	imp.path.value[0].value in {"input", "data"}
@@ -30,14 +30,14 @@ resolved_imports[identifier] := path if {
 	_import.path.value[0].value == "data"
 	count(_import.path.value) > 1
 
-	identifier := imported_identifier(_import)
+	identifier := _imported_identifier(_import)
 	path := [part.value | some part in _import.path.value]
 }
 
-imported_identifier(imp) := imp.alias
-
-imported_identifier(imp) := regal.last(imp.path.value).value if not imp.alias
-
+# METADATA
+# description: |
+#   returns true if provided path (like ["data", "foo", "bar"]) is in the
+#   list of imports (which is commonly ast.imports)
 imports_has_path(imports, path) if {
 	some imp in imports
 
@@ -54,6 +54,10 @@ imports_keyword(imports, keyword) if {
 	_has_keyword(_arr(imp), keyword)
 }
 
+_imported_identifier(imp) := imp.alias
+
+_imported_identifier(imp) := regal.last(imp.path.value).value if not imp.alias
+
 _arr(xs) := [y.value | some y in xs.path.value]
 
 _has_keyword(["future", "keywords"], _)

diff --git a/bundle/regal/main.rego b/bundle/regal/main.rego
@@ -4,15 +4,18 @@ import rego.v1
 
 import data.regal.ast
 import data.regal.config
+import data.regal.util
 
 lint.notices := notices
 
 lint.aggregates := aggregate
 
-lint_aggregate.violations := aggregate_report
+lint.ignore_directives[input.regal.file.name] := ast.ignore_directives
 
 lint.violations := report
 
+lint_aggregate.violations := aggregate_report
+
 rules_to_run[category][title] if {
 	some category, title
 	config.merged_config.rules[category][title]
@@ -34,7 +37,12 @@ grouped_notices[category][title] contains notice if {
 }
 
 # METADATA
-# description: Runs all rules against an input AST and produces a report
+# title: report
+# description: |
+#   This is the main entrypoint for linting, The report rule runs all rules against an input AST and produces a report
+# scope: document
+
+# METADATA
 # entrypoint: true
 report contains violation if {
 	not is_object(input)
@@ -65,7 +73,7 @@ report contains violation if {
 
 	some violation in data.regal.rules[category][title].report
 
-	not ignored(violation, ignore_directives)
+	not ignored(violation, ast.ignore_directives)
 }
 
 # Check custom rules
@@ -77,7 +85,7 @@ report contains violation if {
 	config.for_rule(category, title).level != "ignore"
 	not config.excluded_file(category, title, input.regal.file.name)
 
-	not ignored(violation, ignore_directives)
+	not ignored(violation, ast.ignore_directives)
 }
 
 # Collect aggregates in bundled rules
@@ -119,7 +127,9 @@ aggregate_report contains violation if {
 	# regal ignore:with-outside-test-context
 	some violation in data.regal.rules[category][title].aggregate_report with input as input_for_rule
 
-	not ignored(violation, ignore_directives)
+	ignore_directives := object.get(input.ignore_directives, violation.location.file, {})
+
+	not ignored(violation, util.keys_to_numbers(ignore_directives))
 }
 
 # METADATA
@@ -141,7 +151,12 @@ aggregate_report contains violation if {
 	# regal ignore:with-outside-test-context
 	some violation in data.custom.regal.rules[category][title].aggregate_report with input as input_for_rule
 
-	not ignored(violation, ignore_directives)
+	# for custom rules, we can't assume that the author included
+	# a location in the violation, although they _really_ should
+	file := object.get(violation, ["location", "file"], "")
+	ignore_directives := object.get(input.ignore_directives, file, {})
+
+	not ignored(violation, util.keys_to_numbers(ignore_directives))
 }
 
 ignored(violation, directives) if {
@@ -153,16 +168,3 @@ ignored(violation, directives) if {
 	ignored_rules := directives[violation.location.row + 1]
 	violation.title in ignored_rules
 }
-
-ignore_directives[row] := rules if {
-	some comment in ast.comments_decoded
-	text := trim_space(comment.Text)
-
-	i := indexof(text, "regal ignore:")
-	i != -1
-
-	list := regex.replace(substring(text, i + 13, -1), `\s`, "")
-
-	row := comment.Location.row + 1
-	rules := split(list, ",")
-}
diff --git a/bundle/regal/main_test.rego b/bundle/regal/main_test.rego
@@ -5,12 +5,12 @@ import rego.v1
 import data.regal.config
 import data.regal.main
 
-test_main_basic_input_success if {
+test_basic_input_success if {
 	report := main.report with input as regal.parse_module("p.rego", `package p`)
 	report == set()
 }
 
-test_main_multiple_failures if {
+test_multiple_failures if {
 	policy := `package p
 
 	# both camel case and unification operator
@@ -25,7 +25,7 @@ test_main_multiple_failures if {
 	count(report) == 2
 }
 
-test_main_expect_failure if {
+test_expect_failure if {
 	policy := `package p
 
 	camelCase := "yes"
@@ -36,7 +36,7 @@ test_main_expect_failure if {
 	count(report) == 1
 }
 
-test_main_ignore_rule_config if {
+test_ignore_rule_config if {
 	policy := `package p
 
 	camelCase := "yes"
@@ -47,7 +47,7 @@ test_main_ignore_rule_config if {
 	count(report) == 0
 }
 
-test_main_ignore_directive_failure if {
+test_ignore_directive_failure if {
 	policy := `package p
 
 	# regal ignore:todo-comment
@@ -59,7 +59,7 @@ test_main_ignore_directive_failure if {
 	count(report) == 1
 }
 
-test_main_ignore_directive_success if {
+test_ignore_directive_success if {
 	policy := `package p
 
 	# regal ignore:prefer-snake-case
@@ -71,7 +71,7 @@ test_main_ignore_directive_success if {
 	count(report) == 0
 }
 
-test_main_ignore_directive_success_same_line if {
+test_ignore_directive_success_same_line if {
 	policy := `package p
 
 	camelCase := "yes" # regal ignore:prefer-snake-case
@@ -82,7 +82,7 @@ test_main_ignore_directive_success_same_line if {
 	count(report) == 0
 }
 
-test_main_ignore_directive_success_same_line_trailing_directive if {
+test_ignore_directive_success_same_line_trailing_directive if {
 	policy := `package p
 
 	camelCase := "yes" # camelCase is nice! # regal ignore:prefer-snake-case
@@ -93,7 +93,7 @@ test_main_ignore_directive_success_same_line_trailing_directive if {
 	count(report) == 0
 }
 
-test_main_ignore_directive_success_same_line_todo_comment if {
+test_ignore_directive_success_same_line_todo_comment if {
 	policy := `package p
 
 	camelCase := "yes" # TODO! camelCase isn't nice! # regal ignore:todo-comment
@@ -104,7 +104,7 @@ test_main_ignore_directive_success_same_line_todo_comment if {
 	count(report) == 0
 }
 
-test_main_ignore_directive_multiple_success if {
+test_ignore_directive_multiple_success if {
 	policy := `package p
 
 	# regal ignore:prefer-snake-case,use-assignment-operator
@@ -119,7 +119,7 @@ test_main_ignore_directive_multiple_success if {
 	count(report) == 0
 }
 
-test_main_ignore_directive_multiple_mixed_success if {
+test_ignore_directive_multiple_mixed_success if {
 	policy := `package p
 
 	# regal ignore:prefer-snake-case,todo-comment
@@ -134,7 +134,52 @@ test_main_ignore_directive_multiple_mixed_success if {
 	count(report) == 1
 }
 
-test_main_exclude_files_rule_config if {
+test_ignore_directive_collected_in_aggregate_rule if {
+	module := regal.parse_module("p.rego", `package p
+
+	import rego.v1
+
+	# regal ignore:unresolved-import
+	import data.unresolved
+	`)
+	lint := main.lint with input as module
+
+	lint.ignore_directives == {"p.rego": {6: ["unresolved-import"]}}
+}
+
+test_ignore_directive_enforced_in_aggregate_rule if {
+	report_without_ignore_directives := main.aggregate_report with input as {
+		"aggregates_internal": {"imports/unresolved-import": []},
+		"regal": {"file": {"name": "p.rego"}},
+		"ignore_directives": {},
+	}
+		with config.merged_config as {"rules": {"imports": {"unresolved-import": {"level": "error"}}}}
+		with data.regal.rules.imports["unresolved-import"].aggregate_report as {{
+			"category": "imports",
+			"level": "error",
+			"location": {"col": 1, "file": "p.rego", "row": 6, "text": "import data.provider.parameters"},
+			"title": "unresolved-import",
+		}}
+
+	count(report_without_ignore_directives) == 1
+
+	report_with_ignore_directives := main.aggregate_report with input as {
+		"aggregates_internal": {"imports/unresolved-import": []},
+		"regal": {"file": {"name": "p.rego"}},
+		"ignore_directives": {"p.rego": {"6": ["unresolved-import"]}},
+	}
+		with config.merged_config as {"rules": {"imports": {"unresolved-import": {"level": "error"}}}}
+		with data.regal.rules.imports["unresolved-import"].aggregate_report as {{
+			"category": "imports",
+			"level": "error",
+			"location": {"col": 1, "file": "p.rego", "row": 6, "text": "import data.provider.parameters"},
+			"title": "unresolved-import",
+		}}
+
+	count(report_with_ignore_directives) == 0
+}
+
+test_exclude_files_rule_config if {
 	policy := `package p
 
 	camelCase := "yes"
@@ -145,7 +190,7 @@ test_main_exclude_files_rule_config if {
 	count(report) == 0
 }
 
-test_main_force_exclude_file_eval_param if {
+test_force_exclude_file_eval_param if {
 	policy := `package p
 
 	camelCase := "yes"
@@ -157,7 +202,7 @@ test_main_force_exclude_file_eval_param if {
 	count(report) == 0
 }
 
-test_main_force_exclude_file_config if {
+test_force_exclude_file_config if {
 	policy := `package p
 
 	camelCase := "yes"

diff --git a/bundle/regal/util/util.rego b/bundle/regal/util/util.rego
@@ -35,3 +35,10 @@ has_duplicates(array, item) if count([x |
 #   returns an array of arrays built from all parts of the provided path array,
 #   so e.g. [1, 2, 3] would return [[1], [1, 2], [1, 2, 3]]
 all_paths(path) := [array.slice(path, 0, len) | some len in numbers.range(1, count(path))]
+
+# METADATA
+# description: attempts to turn any key in provided object into numeric form
+keys_to_numbers(obj) := {num: v |
+	some k, v in obj
+	num := to_number(k)
+}
diff --git a/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md
@@ -19,7 +19,8 @@ Recommended, but not required:
 
 - The [rq](https://git.sr.ht/~charles/rq) tool. This is used for automating and simplifying many of the tasks outlined
   in this document, and is (ab)used as a Rego-based replacement for Make in this project. Check out the
-  [do.rq](https://github.com/StyraInc/regal/blob/main/build/do.rq) file to see what that looks like, and for documentation on the available tasks.
+  [do.rq](https://github.com/StyraInc/regal/blob/main/build/do.rq) file to see what that looks like, and for
+  documentation on the available tasks.
 
 ## Contributing New Rules