From f6744496c7ba0c0a7013b7f5e8008e1732f3a9c9 Mon Sep 17 00:00:00 2001 From: Anders Eknert Date: Wed, 8 May 2024 11:33:30 +0200 Subject: [PATCH] Rule: `impossible-not` (#698) See docs in PR (and issue) for further details. Fixes #603 Signed-off-by: Anders Eknert --- README.md | 1 + bundle/regal/config/provided/data.yaml | 2 + bundle/regal/rules/bugs/impossible_not.rego | 136 +++++++++++++++++ .../regal/rules/bugs/impossible_not_test.rego | 138 ++++++++++++++++++ .../rules/imports/unresolved_import.rego | 1 - docs/rules/bugs/impossible-not.md | 85 +++++++++++ e2e/testdata/violations/most_violations.rego | 6 + 7 files changed, 368 insertions(+), 1 deletion(-) create mode 100644 bundle/regal/rules/bugs/impossible_not.rego create mode 100644 bundle/regal/rules/bugs/impossible_not_test.rego create mode 100644 docs/rules/bugs/impossible-not.md diff --git a/README.md b/README.md index c1dfe8c2..00272a3b 100644 --- a/README.md +++ b/README.md @@ -198,6 +198,7 @@ The following rules are currently available: | bugs | [deprecated-builtin](https://docs.styra.com/regal/rules/bugs/deprecated-builtin) | Avoid using deprecated built-in functions | | bugs | [duplicate-rule](https://docs.styra.com/regal/rules/bugs/duplicate-rule) | Duplicate rule | | bugs | [if-empty-object](https://docs.styra.com/regal/rules/bugs/if-empty-object) | Empty object following `if` | +| bugs | [impossible-not](https://docs.styra.com/regal/rules/bugs/impossible-not) | Impossible `not` condition | | bugs | [inconsistent-args](https://docs.styra.com/regal/rules/bugs/inconsistent-args) | Inconsistently named function arguments | | bugs | [invalid-metadata-attribute](https://docs.styra.com/regal/rules/bugs/invalid-metadata-attribute) | Invalid attribute in metadata annotation | | bugs | [not-equals-in-loop](https://docs.styra.com/regal/rules/bugs/not-equals-in-loop) | Use of != in loop | diff --git a/bundle/regal/config/provided/data.yaml b/bundle/regal/config/provided/data.yaml index 07169ee1..8bb0d552 100644 --- a/bundle/regal/config/provided/data.yaml +++ b/bundle/regal/config/provided/data.yaml @@ -8,6 +8,8 @@ rules: level: error if-empty-object: level: error + impossible-not: + level: error inconsistent-args: level: error invalid-metadata-attribute: diff --git a/bundle/regal/rules/bugs/impossible_not.rego b/bundle/regal/rules/bugs/impossible_not.rego new file mode 100644 index 00000000..c20d23fd --- /dev/null +++ b/bundle/regal/rules/bugs/impossible_not.rego @@ -0,0 +1,136 @@ +# METADATA +# description: Impossible `not` condition +package regal.rules.bugs["impossible-not"] + +import rego.v1 + +import data.regal.ast +import data.regal.result + +# regal ignore:rule-length +aggregate contains entry if { + package_path := [part.value | some part in input["package"].path] + + imported_symbols := {symbol: path | + some _import in input.imports + + _import.path.value[0].value == "data" + count(_import.path.value) > 1 + + symbol := imported_symbol(_import) + path := [part.value | some part in _import.path.value] + } + + multivalue_rules := {path | + some rule in ast.rules + + rule.head.key + not rule.head.value + + # ignore general ref head rules for now + every path in array.slice(rule.head.ref, 1, count(rule.head.ref)) { + path.type == "string" + } + + path := concat(".", array.concat(package_path, [p | + some ref in rule.head.ref + p := ref.value + ])) + } + + negated_refs := [negated_ref | + some i, rule in input.rules + + walk(rule, [_, value]) + + value.negated + + # if terms is an array, it's a function call, and most likely not "impossible" + is_object(value.terms) + value.terms.type in {"ref", "var"} + + ref := var_to_ref(value.terms) + + # for now, ignore ref if it has variable components + every path in array.slice(ref, 1, count(ref)) { + path.type == "string" + } + + # ignore negated local vars + not ref[0].value in ast.function_arg_names(rule) + not ref[0].value in {var.value | some var in ast.find_vars_in_local_scope(rule, value.location)} + + negated_ref := { + "ref": ref, + "resolved_path": resolve(ref, package_path, imported_symbols), + } + ] + + entry := result.aggregate(rego.metadata.chain(), { + "imported_symbols": imported_symbols, + "multivalue_rules": multivalue_rules, + "negated_refs": negated_refs, + }) +} + +# METADATA +# schemas: +# - input: schema.regal.aggregate +aggregate_report contains violation if { + all_multivalue_refs := {path | + some entry in input.aggregate + some path in entry.aggregate_data.multivalue_rules + } + + some entry in input.aggregate + some negated in entry.aggregate_data.negated_refs + + negated.resolved_path in all_multivalue_refs + + loc := object.union(result.location(negated.ref), {"location": { + "file": entry.aggregate_source.file, + # note that the "not" isn't present in the AST, so we'll add it manually to the text + # in the location to try and make it clear where the issue is (as opposed to just + # printing the ref) + "text": sprintf("not %s", [to_string(negated.ref)]), + }}) + + violation := result.fail(rego.metadata.chain(), loc) +} + +var_to_ref(terms) := [terms] if terms.type == "var" + +var_to_ref(terms) := terms.value if terms.type == "ref" + +imported_symbol(imp) := imp.alias + +imported_symbol(imp) := regal.last(imp.path.value).value if not imp.alias + +to_string(ref) := concat(".", [path | + some part in ref + path := part.value +]) + +resolve(ref, _, _) := to_string(ref) if ref[0].value == "data" + +# imported symbol +resolve(ref, _, imported_symbols) := concat(".", resolved) if { + ref[0].value != "data" + + resolved := array.concat( + imported_symbols[ref[0].value], + [part.value | some part in array.slice(ref, 1, count(ref))], + ) +} + +# not imported — must be local or package +resolve(ref, pkg_path, imported_symbols) := concat(".", resolved) if { + ref[0].value != "data" + + not imported_symbols[ref[0].value] + + resolved := array.concat( + pkg_path, + [part.value | some part in ref], + ) +} diff --git a/bundle/regal/rules/bugs/impossible_not_test.rego b/bundle/regal/rules/bugs/impossible_not_test.rego new file mode 100644 index 00000000..088c328e --- /dev/null +++ b/bundle/regal/rules/bugs/impossible_not_test.rego @@ -0,0 +1,138 @@ +package regal.rules.bugs["impossible-not_test"] + +import rego.v1 + +import data.regal.rules.bugs["impossible-not"] as rule + +test_fail_multivalue_not_reference_same_package if { + agg1 := rule.aggregate with input as regal.parse_module("p1.rego", `package foo + + import rego.v1 + + partial contains "foo" + `) + + agg2 := rule.aggregate with input as regal.parse_module("p2.rego", `package foo + + import rego.v1 + + test_foo if { + not partial + } + `) + + r := rule.aggregate_report with input as {"aggregate": (agg1 | agg2)} + r == expected_with_location({"col": 7, "file": "p2.rego", "row": 6, "text": "not partial"}) +} + +test_fail_multivalue_not_reference_different_package_using_direct_reference if { + agg1 := rule.aggregate with input as regal.parse_module("p1.rego", `package foo + + import rego.v1 + + partial contains "foo" + `) + + agg2 := rule.aggregate with input as regal.parse_module("p2.rego", `package bar + + import rego.v1 + + test_foo if { + not data.foo.partial + } + `) + + r := rule.aggregate_report with input as {"aggregate": (agg1 | agg2)} + r == expected_with_location({"col": 7, "file": "p2.rego", "row": 6, "text": "not data.foo.partial"}) +} + +test_fail_multivalue_not_reference_different_package_using_import if { + agg1 := rule.aggregate with input as regal.parse_module("p1.rego", `package foo + + import rego.v1 + + partial contains "foo" + + another contains "bar" + `) + + agg2 := rule.aggregate with input as regal.parse_module("p2.rego", `package bar + + import rego.v1 + + import data.foo + + test_foo if { + not foo.partial + } + `) + + r := rule.aggregate_report with input as {"aggregate": (agg1 | agg2)} + r == expected_with_location({"col": 7, "file": "p2.rego", "row": 8, "text": "not foo.partial"}) +} + +test_success_multivalue_not_reference_invalidated_by_local_var if { + agg1 := rule.aggregate with input as regal.parse_module("p1.rego", `package foo + + import rego.v1 + + partial contains "foo" + `) + + agg2 := rule.aggregate with input as regal.parse_module("p2.rego", `package bar + + import rego.v1 + + import data.foo + + test_foo if { + foo := input.bar + not foo.partial + } + `) + + r := rule.aggregate_report with input as {"aggregate": (agg1 | agg2)} + r == set() +} + +test_success_multivalue_not_reference_invalidated_by_function_argument if { + agg1 := rule.aggregate with input as regal.parse_module("p1.rego", `package foo + + import rego.v1 + + partial contains "foo" + `) + + agg2 := rule.aggregate with input as regal.parse_module("p2.rego", `package bar + + import rego.v1 + + import data.foo + + my_function(foo) if { + not foo.partial + } + `) + + r := rule.aggregate_report with input as {"aggregate": (agg1 | agg2)} + r == set() +} + +expected := { + "category": "bugs", + "description": "Impossible `not` condition", + "level": "error", + "related_resources": [{ + "description": "documentation", + "ref": "https://docs.styra.com/regal/rules/bugs/impossible-not", + }], + "title": "impossible-not", +} + +expected_with_location(location) := {object.union(expected, {"location": location})} if is_object(location) + +expected_with_location(location) := {object.union(expected, {"location": loc}) | + some loc in location +} if { + is_set(location) +} diff --git a/bundle/regal/rules/imports/unresolved_import.rego b/bundle/regal/rules/imports/unresolved_import.rego index 972978bc..ce7f649b 100644 --- a/bundle/regal/rules/imports/unresolved_import.rego +++ b/bundle/regal/rules/imports/unresolved_import.rego @@ -46,7 +46,6 @@ aggregate contains entry if { aggregate_report contains violation if { all_known_refs := {path | some entry in input.aggregate - some path in entry.aggregate_data.exported_refs } diff --git a/docs/rules/bugs/impossible-not.md b/docs/rules/bugs/impossible-not.md new file mode 100644 index 00000000..0a67c788 --- /dev/null +++ b/docs/rules/bugs/impossible-not.md @@ -0,0 +1,85 @@ +# impossible-not + +**Summary**: Impossible `not` condition + +**Category**: Bugs + +**Type**: Aggregate - only runs when more than one file is provided for linting + +**Avoid** +```rego +package policy + +import rego.v1 + +report contains violation if { + # ... some conditions +} +``` + +```rego +package policy_test + +import rego.v1 + +import data.policy + +test_report_is_empty { + # evaluation will stop here, as even an empty set is "true" + not policy.report +} +``` + +**Prefer** +```rego +package policy + +import rego.v1 + +report contains violation if { + # ... some conditions +} +``` + +```rego +package policy_test + +import rego.v1 + +import data.policy + +test_report_is_empty { + count(policy.report) == 0 +} +``` + +## Rationale + +The `not` keyword negates the expression that follows it. A common mistake, especially in tests, is to use `not` +to test the result of evaluating a partial (i.e. multi-value) rule. However, as even an empty set is considered +"truthy", the `not` will in that case always evaluate to `false`. There are more cases where `not` is impossible, +or a [constant condition](https://docs.styra.com/regal/rules/bugs/constant-condition), but references to partial +rules are by far the most common. For tests where you want to assert the set is empty or has a specific number of +items, use the built-in `count` function instead. + +## Configuration Options + +This linter rule provides the following configuration options: + +```yaml +rules: + bugs: + impossible-not: + # one of "error", "warning", "ignore" + level: error +``` + +## Related Resources + +- Regal Docs: [constant-condition](https://docs.styra.com/regal/rules/bugs/constant-condition) + +## Community + +If you think you've found a problem with this rule or its documentation, would like to suggest improvements, new rules, +or just talk about Regal in general, please join us in the `#regal` channel in the Styra Community +[Slack](https://communityinviter.com/apps/styracommunity/signup)! diff --git a/e2e/testdata/violations/most_violations.rego b/e2e/testdata/violations/most_violations.rego index 9757963f..bebf5871 100644 --- a/e2e/testdata/violations/most_violations.rego +++ b/e2e/testdata/violations/most_violations.rego @@ -83,6 +83,12 @@ redundant_existence_check if { deprecated_builtin := all([true]) +partial contains "foo" + +impossible_not if { + not partial +} + ### Idiomatic ### custom_has_key_construct(map, key) if {