Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support ignore directives in aggregate rules #809

Merged
merged 2 commits into from
Jun 10, 2024
Merged
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
28 changes: 28 additions & 0 deletions bundle/regal/ast/comments.rego
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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
Expand Down
16 changes: 10 additions & 6 deletions bundle/regal/ast/imports.rego
Original file line number Diff line number Diff line change
Expand Up @@ -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"}
Expand All @@ -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

Expand All @@ -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"], _)
Expand Down
40 changes: 21 additions & 19 deletions bundle/regal/main.rego
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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 {
Expand All @@ -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, ",")
}
73 changes: 59 additions & 14 deletions bundle/regal/main_test.rego
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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"
Expand All @@ -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"
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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"
Expand All @@ -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"
Expand All @@ -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"
Expand Down
7 changes: 7 additions & 0 deletions bundle/regal/util/util.rego
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
3 changes: 2 additions & 1 deletion docs/CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Loading