diff --git a/README.md b/README.md index f7990582..4dc07000 100644 --- a/README.md +++ b/README.md @@ -197,92 +197,93 @@ The following rules are currently available: -| Category | Title | Description | -|-------------|-------------------------------------------------------------------------------------------------------|-----------------------------------------------------------| -| bugs | [annotation-without-metadata](https://docs.styra.com/regal/rules/bugs/annotation-without-metadata) | Annotation without metadata | -| bugs | [argument-always-wildcard](https://docs.styra.com/regal/rules/bugs/argument-always-wildcard) | Argument is always a wildcard | -| bugs | [constant-condition](https://docs.styra.com/regal/rules/bugs/constant-condition) | Constant condition | -| 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 | [if-object-literal](https://docs.styra.com/regal/rules/bugs/if-object-literal) | Object literal 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 | [internal-entrypoint](https://docs.styra.com/regal/rules/bugs/internal-entrypoint) | Entrypoint can't be marked internal | -| bugs | [invalid-metadata-attribute](https://docs.styra.com/regal/rules/bugs/invalid-metadata-attribute) | Invalid attribute in metadata annotation | -| bugs | [leaked-internal-reference](https://docs.styra.com/regal/rules/bugs/leaked-internal-reference) | Outside reference to internal rule or function | -| bugs | [not-equals-in-loop](https://docs.styra.com/regal/rules/bugs/not-equals-in-loop) | Use of != in loop | -| bugs | [redundant-existence-check](https://docs.styra.com/regal/rules/bugs/redundant-existence-check) | Redundant existence check | -| bugs | [rule-named-if](https://docs.styra.com/regal/rules/bugs/rule-named-if) | Rule named "if" | -| bugs | [rule-shadows-builtin](https://docs.styra.com/regal/rules/bugs/rule-shadows-builtin) | Rule name shadows built-in | -| bugs | [sprintf-arguments-mismatch](https://docs.styra.com/regal/rules/bugs/sprintf-arguments-mismatch) | Mismatch in `sprintf` arguments count | -| bugs | [top-level-iteration](https://docs.styra.com/regal/rules/bugs/top-level-iteration) | Iteration in top-level assignment | -| bugs | [unassigned-return-value](https://docs.styra.com/regal/rules/bugs/unassigned-return-value) | Non-boolean return value unassigned | -| bugs | [unused-output-variable](https://docs.styra.com/regal/rules/bugs/unused-output-variable) | Unused output variable | -| bugs | [var-shadows-builtin](https://docs.styra.com/regal/rules/bugs/var-shadows-builtin) | Variable name shadows built-in | -| bugs | [zero-arity-function](https://docs.styra.com/regal/rules/bugs/zero-arity-function) | Avoid functions without args | -| custom | [forbidden-function-call](https://docs.styra.com/regal/rules/custom/forbidden-function-call) | Forbidden function call | -| custom | [naming-convention](https://docs.styra.com/regal/rules/custom/naming-convention) | Naming convention violation | -| custom | [one-liner-rule](https://docs.styra.com/regal/rules/custom/one-liner-rule) | Rule body could be made a one-liner | -| custom | [prefer-value-in-head](https://docs.styra.com/regal/rules/custom/prefer-value-in-head) | Prefer value in rule head | -| idiomatic | [ambiguous-scope](https://docs.styra.com/regal/rules/idiomatic/ambiguous-scope) | Ambiguous metadata scope | -| idiomatic | [boolean-assignment](https://docs.styra.com/regal/rules/idiomatic/boolean-assignment) | Prefer `if` over boolean assignment | -| idiomatic | [custom-has-key-construct](https://docs.styra.com/regal/rules/idiomatic/custom-has-key-construct) | Custom function may be replaced by `in` and `object.keys` | -| idiomatic | [custom-in-construct](https://docs.styra.com/regal/rules/idiomatic/custom-in-construct) | Custom function may be replaced by `in` keyword | -| idiomatic | [directory-package-mismatch](https://docs.styra.com/regal/rules/idiomatic/directory-package-mismatch) | Directory structure should mirror package | -| idiomatic | [equals-pattern-matching](https://docs.styra.com/regal/rules/idiomatic/equals-pattern-matching) | Prefer pattern matching in function arguments | -| idiomatic | [no-defined-entrypoint](https://docs.styra.com/regal/rules/idiomatic/no-defined-entrypoint) | Missing entrypoint annotation | -| idiomatic | [non-raw-regex-pattern](https://docs.styra.com/regal/rules/idiomatic/non-raw-regex-pattern) | Use raw strings for regex patterns | -| idiomatic | [prefer-set-or-object-rule](https://docs.styra.com/regal/rules/idiomatic/prefer-set-or-object-rule) | Prefer set or object rule over comprehension | -| idiomatic | [use-contains](https://docs.styra.com/regal/rules/idiomatic/use-contains) | Use the `contains` keyword | -| idiomatic | [use-if](https://docs.styra.com/regal/rules/idiomatic/use-if) | Use the `if` keyword | -| idiomatic | [use-in-operator](https://docs.styra.com/regal/rules/idiomatic/use-in-operator) | Use in to check for membership | -| idiomatic | [use-some-for-output-vars](https://docs.styra.com/regal/rules/idiomatic/use-some-for-output-vars) | Use `some` to declare output variables | -| idiomatic | [use-strings-count](https://docs.styra.com/regal/rules/idiomatic/use-strings-count) | Use `strings.count` where possible | -| imports | [avoid-importing-input](https://docs.styra.com/regal/rules/imports/avoid-importing-input) | Avoid importing input | -| imports | [circular-import](https://docs.styra.com/regal/rules/imports/circular-import) | Circular import | -| imports | [ignored-import](https://docs.styra.com/regal/rules/imports/ignored-import) | Reference ignores import | -| imports | [implicit-future-keywords](https://docs.styra.com/regal/rules/imports/implicit-future-keywords) | Use explicit future keyword imports | -| imports | [import-after-rule](https://docs.styra.com/regal/rules/imports/import-after-rule) | Import declared after rule | -| imports | [import-shadows-builtin](https://docs.styra.com/regal/rules/imports/import-shadows-builtin) | Import shadows built-in namespace | -| imports | [import-shadows-import](https://docs.styra.com/regal/rules/imports/import-shadows-import) | Import shadows another import | -| imports | [prefer-package-imports](https://docs.styra.com/regal/rules/imports/prefer-package-imports) | Prefer importing packages over rules | -| imports | [redundant-alias](https://docs.styra.com/regal/rules/imports/redundant-alias) | Redundant alias | -| imports | [redundant-data-import](https://docs.styra.com/regal/rules/imports/redundant-data-import) | Redundant import of data | -| imports | [unresolved-import](https://docs.styra.com/regal/rules/imports/unresolved-import) | Unresolved import | -| imports | [use-rego-v1](https://docs.styra.com/regal/rules/imports/use-rego-v1) | Use `import rego.v1` | -| performance | [with-outside-test-context](https://docs.styra.com/regal/rules/performance/with-outside-test-context) | `with` used outside test context | -| style | [avoid-get-and-list-prefix](https://docs.styra.com/regal/rules/style/avoid-get-and-list-prefix) | Avoid `get_` and `list_` prefix for rules and functions | -| style | [chained-rule-body](https://docs.styra.com/regal/rules/style/chained-rule-body) | Avoid chaining rule bodies | -| style | [default-over-else](https://docs.styra.com/regal/rules/style/default-over-else) | Prefer default assignment over fallback else | -| style | [default-over-not](https://docs.styra.com/regal/rules/style/default-over-not) | Prefer default assignment over negated condition | -| style | [detached-metadata](https://docs.styra.com/regal/rules/style/detached-metadata) | Detached metadata annotation | -| style | [double-negative](https://docs.styra.com/regal/rules/style/double-negative) | Avoid double negatives | -| style | [external-reference](https://docs.styra.com/regal/rules/style/external-reference) | External reference in function | -| style | [file-length](https://docs.styra.com/regal/rules/style/file-length) | Max file length exceeded | -| style | [function-arg-return](https://docs.styra.com/regal/rules/style/function-arg-return) | Function argument used for return value | -| style | [line-length](https://docs.styra.com/regal/rules/style/line-length) | Line too long | -| style | [messy-rule](https://docs.styra.com/regal/rules/style/messy-rule) | Messy incremental rule | -| style | [no-whitespace-comment](https://docs.styra.com/regal/rules/style/no-whitespace-comment) | Comment should start with whitespace | -| style | [opa-fmt](https://docs.styra.com/regal/rules/style/opa-fmt) | File should be formatted with `opa fmt` | -| style | [pointless-reassignment](https://docs.styra.com/regal/rules/style/pointless-reassignment) | Pointless reassignment of variable | -| style | [prefer-snake-case](https://docs.styra.com/regal/rules/style/prefer-snake-case) | Prefer snake_case for names | -| style | [prefer-some-in-iteration](https://docs.styra.com/regal/rules/style/prefer-some-in-iteration) | Prefer `some .. in` for iteration | -| style | [rule-length](https://docs.styra.com/regal/rules/style/rule-length) | Max rule length exceeded | -| style | [rule-name-repeats-package](https://docs.styra.com/regal/rules/style/rule-name-repeats-package) | Rule name repeats package | -| style | [todo-comment](https://docs.styra.com/regal/rules/style/todo-comment) | Avoid TODO comments | -| style | [trailing-default-rule](https://docs.styra.com/regal/rules/style/trailing-default-rule) | Default rule should be declared first | -| style | [unconditional-assignment](https://docs.styra.com/regal/rules/style/unconditional-assignment) | Unconditional assignment in rule body | -| style | [unnecessary-some](https://docs.styra.com/regal/rules/style/unnecessary-some) | Unnecessary use of `some` | -| style | [use-assignment-operator](https://docs.styra.com/regal/rules/style/use-assignment-operator) | Prefer := over = for assignment | -| style | [yoda-condition](https://docs.styra.com/regal/rules/style/yoda-condition) | Yoda condition | -| testing | [dubious-print-sprintf](https://docs.styra.com/regal/rules/testing/dubious-print-sprintf) | Dubious use of print and sprintf | -| testing | [file-missing-test-suffix](https://docs.styra.com/regal/rules/testing/file-missing-test-suffix) | Files containing tests should have a _test.rego suffix | -| testing | [identically-named-tests](https://docs.styra.com/regal/rules/testing/identically-named-tests) | Multiple tests with same name | -| testing | [metasyntactic-variable](https://docs.styra.com/regal/rules/testing/metasyntactic-variable) | Metasyntactic variable name | -| testing | [print-or-trace-call](https://docs.styra.com/regal/rules/testing/print-or-trace-call) | Call to print or trace function | -| testing | [test-outside-test-package](https://docs.styra.com/regal/rules/testing/test-outside-test-package) | Test outside of test package | -| testing | [todo-test](https://docs.styra.com/regal/rules/testing/todo-test) | TODO test encountered | +| Category | Title | Description | +|-------------|---------------------------------------------------------------------------------------------------------|-----------------------------------------------------------| +| bugs | [annotation-without-metadata](https://docs.styra.com/regal/rules/bugs/annotation-without-metadata) | Annotation without metadata | +| bugs | [argument-always-wildcard](https://docs.styra.com/regal/rules/bugs/argument-always-wildcard) | Argument is always a wildcard | +| bugs | [constant-condition](https://docs.styra.com/regal/rules/bugs/constant-condition) | Constant condition | +| 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 | [if-object-literal](https://docs.styra.com/regal/rules/bugs/if-object-literal) | Object literal 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 | [internal-entrypoint](https://docs.styra.com/regal/rules/bugs/internal-entrypoint) | Entrypoint can't be marked internal | +| bugs | [invalid-metadata-attribute](https://docs.styra.com/regal/rules/bugs/invalid-metadata-attribute) | Invalid attribute in metadata annotation | +| bugs | [leaked-internal-reference](https://docs.styra.com/regal/rules/bugs/leaked-internal-reference) | Outside reference to internal rule or function | +| bugs | [not-equals-in-loop](https://docs.styra.com/regal/rules/bugs/not-equals-in-loop) | Use of != in loop | +| bugs | [redundant-existence-check](https://docs.styra.com/regal/rules/bugs/redundant-existence-check) | Redundant existence check | +| bugs | [rule-named-if](https://docs.styra.com/regal/rules/bugs/rule-named-if) | Rule named "if" | +| bugs | [rule-shadows-builtin](https://docs.styra.com/regal/rules/bugs/rule-shadows-builtin) | Rule name shadows built-in | +| bugs | [sprintf-arguments-mismatch](https://docs.styra.com/regal/rules/bugs/sprintf-arguments-mismatch) | Mismatch in `sprintf` arguments count | +| bugs | [top-level-iteration](https://docs.styra.com/regal/rules/bugs/top-level-iteration) | Iteration in top-level assignment | +| bugs | [unassigned-return-value](https://docs.styra.com/regal/rules/bugs/unassigned-return-value) | Non-boolean return value unassigned | +| bugs | [unused-output-variable](https://docs.styra.com/regal/rules/bugs/unused-output-variable) | Unused output variable | +| bugs | [var-shadows-builtin](https://docs.styra.com/regal/rules/bugs/var-shadows-builtin) | Variable name shadows built-in | +| bugs | [zero-arity-function](https://docs.styra.com/regal/rules/bugs/zero-arity-function) | Avoid functions without args | +| custom | [forbidden-function-call](https://docs.styra.com/regal/rules/custom/forbidden-function-call) | Forbidden function call | +| custom | [naming-convention](https://docs.styra.com/regal/rules/custom/naming-convention) | Naming convention violation | +| custom | [one-liner-rule](https://docs.styra.com/regal/rules/custom/one-liner-rule) | Rule body could be made a one-liner | +| custom | [prefer-value-in-head](https://docs.styra.com/regal/rules/custom/prefer-value-in-head) | Prefer value in rule head | +| idiomatic | [ambiguous-scope](https://docs.styra.com/regal/rules/idiomatic/ambiguous-scope) | Ambiguous metadata scope | +| idiomatic | [boolean-assignment](https://docs.styra.com/regal/rules/idiomatic/boolean-assignment) | Prefer `if` over boolean assignment | +| idiomatic | [custom-has-key-construct](https://docs.styra.com/regal/rules/idiomatic/custom-has-key-construct) | Custom function may be replaced by `in` and `object.keys` | +| idiomatic | [custom-in-construct](https://docs.styra.com/regal/rules/idiomatic/custom-in-construct) | Custom function may be replaced by `in` keyword | +| idiomatic | [directory-package-mismatch](https://docs.styra.com/regal/rules/idiomatic/directory-package-mismatch) | Directory structure should mirror package | +| idiomatic | [equals-pattern-matching](https://docs.styra.com/regal/rules/idiomatic/equals-pattern-matching) | Prefer pattern matching in function arguments | +| idiomatic | [no-defined-entrypoint](https://docs.styra.com/regal/rules/idiomatic/no-defined-entrypoint) | Missing entrypoint annotation | +| idiomatic | [non-raw-regex-pattern](https://docs.styra.com/regal/rules/idiomatic/non-raw-regex-pattern) | Use raw strings for regex patterns | +| idiomatic | [prefer-set-or-object-rule](https://docs.styra.com/regal/rules/idiomatic/prefer-set-or-object-rule) | Prefer set or object rule over comprehension | +| idiomatic | [use-contains](https://docs.styra.com/regal/rules/idiomatic/use-contains) | Use the `contains` keyword | +| idiomatic | [use-if](https://docs.styra.com/regal/rules/idiomatic/use-if) | Use the `if` keyword | +| idiomatic | [use-in-operator](https://docs.styra.com/regal/rules/idiomatic/use-in-operator) | Use in to check for membership | +| idiomatic | [use-some-for-output-vars](https://docs.styra.com/regal/rules/idiomatic/use-some-for-output-vars) | Use `some` to declare output variables | +| idiomatic | [use-strings-count](https://docs.styra.com/regal/rules/idiomatic/use-strings-count) | Use `strings.count` where possible | +| imports | [avoid-importing-input](https://docs.styra.com/regal/rules/imports/avoid-importing-input) | Avoid importing input | +| imports | [circular-import](https://docs.styra.com/regal/rules/imports/circular-import) | Circular import | +| imports | [ignored-import](https://docs.styra.com/regal/rules/imports/ignored-import) | Reference ignores import | +| imports | [implicit-future-keywords](https://docs.styra.com/regal/rules/imports/implicit-future-keywords) | Use explicit future keyword imports | +| imports | [import-after-rule](https://docs.styra.com/regal/rules/imports/import-after-rule) | Import declared after rule | +| imports | [import-shadows-builtin](https://docs.styra.com/regal/rules/imports/import-shadows-builtin) | Import shadows built-in namespace | +| imports | [import-shadows-import](https://docs.styra.com/regal/rules/imports/import-shadows-import) | Import shadows another import | +| imports | [prefer-package-imports](https://docs.styra.com/regal/rules/imports/prefer-package-imports) | Prefer importing packages over rules | +| imports | [redundant-alias](https://docs.styra.com/regal/rules/imports/redundant-alias) | Redundant alias | +| imports | [redundant-data-import](https://docs.styra.com/regal/rules/imports/redundant-data-import) | Redundant import of data | +| imports | [unresolved-import](https://docs.styra.com/regal/rules/imports/unresolved-import) | Unresolved import | +| imports | [use-rego-v1](https://docs.styra.com/regal/rules/imports/use-rego-v1) | Use `import rego.v1` | +| performance | [with-outside-test-context](https://docs.styra.com/regal/rules/performance/with-outside-test-context) | `with` used outside test context | +| style | [avoid-get-and-list-prefix](https://docs.styra.com/regal/rules/style/avoid-get-and-list-prefix) | Avoid `get_` and `list_` prefix for rules and functions | +| style | [chained-rule-body](https://docs.styra.com/regal/rules/style/chained-rule-body) | Avoid chaining rule bodies | +| style | [comprehension-term-assignment](https://docs.styra.com/regal/rules/style/comprehension-term-assignment) | Assignment can be moved to comprehension term | +| style | [default-over-else](https://docs.styra.com/regal/rules/style/default-over-else) | Prefer default assignment over fallback else | +| style | [default-over-not](https://docs.styra.com/regal/rules/style/default-over-not) | Prefer default assignment over negated condition | +| style | [detached-metadata](https://docs.styra.com/regal/rules/style/detached-metadata) | Detached metadata annotation | +| style | [double-negative](https://docs.styra.com/regal/rules/style/double-negative) | Avoid double negatives | +| style | [external-reference](https://docs.styra.com/regal/rules/style/external-reference) | External reference in function | +| style | [file-length](https://docs.styra.com/regal/rules/style/file-length) | Max file length exceeded | +| style | [function-arg-return](https://docs.styra.com/regal/rules/style/function-arg-return) | Function argument used for return value | +| style | [line-length](https://docs.styra.com/regal/rules/style/line-length) | Line too long | +| style | [messy-rule](https://docs.styra.com/regal/rules/style/messy-rule) | Messy incremental rule | +| style | [no-whitespace-comment](https://docs.styra.com/regal/rules/style/no-whitespace-comment) | Comment should start with whitespace | +| style | [opa-fmt](https://docs.styra.com/regal/rules/style/opa-fmt) | File should be formatted with `opa fmt` | +| style | [pointless-reassignment](https://docs.styra.com/regal/rules/style/pointless-reassignment) | Pointless reassignment of variable | +| style | [prefer-snake-case](https://docs.styra.com/regal/rules/style/prefer-snake-case) | Prefer snake_case for names | +| style | [prefer-some-in-iteration](https://docs.styra.com/regal/rules/style/prefer-some-in-iteration) | Prefer `some .. in` for iteration | +| style | [rule-length](https://docs.styra.com/regal/rules/style/rule-length) | Max rule length exceeded | +| style | [rule-name-repeats-package](https://docs.styra.com/regal/rules/style/rule-name-repeats-package) | Rule name repeats package | +| style | [todo-comment](https://docs.styra.com/regal/rules/style/todo-comment) | Avoid TODO comments | +| style | [trailing-default-rule](https://docs.styra.com/regal/rules/style/trailing-default-rule) | Default rule should be declared first | +| style | [unconditional-assignment](https://docs.styra.com/regal/rules/style/unconditional-assignment) | Unconditional assignment in rule body | +| style | [unnecessary-some](https://docs.styra.com/regal/rules/style/unnecessary-some) | Unnecessary use of `some` | +| style | [use-assignment-operator](https://docs.styra.com/regal/rules/style/use-assignment-operator) | Prefer := over = for assignment | +| style | [yoda-condition](https://docs.styra.com/regal/rules/style/yoda-condition) | Yoda condition | +| testing | [dubious-print-sprintf](https://docs.styra.com/regal/rules/testing/dubious-print-sprintf) | Dubious use of print and sprintf | +| testing | [file-missing-test-suffix](https://docs.styra.com/regal/rules/testing/file-missing-test-suffix) | Files containing tests should have a _test.rego suffix | +| testing | [identically-named-tests](https://docs.styra.com/regal/rules/testing/identically-named-tests) | Multiple tests with same name | +| testing | [metasyntactic-variable](https://docs.styra.com/regal/rules/testing/metasyntactic-variable) | Metasyntactic variable name | +| testing | [print-or-trace-call](https://docs.styra.com/regal/rules/testing/print-or-trace-call) | Call to print or trace function | +| testing | [test-outside-test-package](https://docs.styra.com/regal/rules/testing/test-outside-test-package) | Test outside of test package | +| testing | [todo-test](https://docs.styra.com/regal/rules/testing/todo-test) | TODO test encountered | diff --git a/bundle/regal/ast/ast.rego b/bundle/regal/ast/ast.rego index adf669e8..cb4bbe4d 100644 --- a/bundle/regal/ast/ast.rego +++ b/bundle/regal/ast/ast.rego @@ -296,3 +296,9 @@ is_chained_rule_body(rule, lines) if { startswith(col_text, "{") } + +assignment_terms(expr) := [expr.terms[1], expr.terms[2]] if { + expr.terms[0].type == "ref" + expr.terms[0].value[0].type == "var" + expr.terms[0].value[0].value == "assign" +} diff --git a/bundle/regal/ast/ast_test.rego b/bundle/regal/ast/ast_test.rego index 2c5af297..2d2938b2 100644 --- a/bundle/regal/ast/ast_test.rego +++ b/bundle/regal/ast/ast_test.rego @@ -47,10 +47,9 @@ test_find_vars if { vars := ast.find_vars(module.rules) - names := {name | + names := {var.value | some var in vars var.type == "var" - name := var.value } names == {"a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", "n", "o", "p", "q", "r", "s", "t", "u"} @@ -73,10 +72,9 @@ test_find_vars_comprehension_lhs if { vars := ast.find_vars(module.rules) - names := {name | + names := {var.value | some var in vars var.type == "var" - name := var.value } names == {"a", "b", "c", "d", "e", "f", "g"} diff --git a/bundle/regal/ast/search.rego b/bundle/regal/ast/search.rego index ffcf7715..175bd347 100644 --- a/bundle/regal/ast/search.rego +++ b/bundle/regal/ast/search.rego @@ -22,9 +22,9 @@ _find_assign_vars(value) := var if { # [a, b, c] := [1, 2, 3] # or # {a: b} := {"foo": "bar"} -_find_assign_vars(value) := var if { +_find_assign_vars(value) := vars if { value[1].type in {"array", "object"} - var := _find_nested_vars(value[1]) + vars := _find_nested_vars(value[1]) } # var declared via `some`, i.e. `some x` or `some x, y` @@ -34,19 +34,19 @@ _find_some_decl_vars(value) := [v | ] # single var declared via `some in`, i.e. `some x in y` -_find_some_in_decl_vars(value) := var if { +_find_some_in_decl_vars(value) := vars if { arr := value[0].value count(arr) == 3 - var := _find_nested_vars(arr[1]) + vars := _find_nested_vars(arr[1]) } # two vars declared via `some in`, i.e. `some x, y in z` -_find_some_in_decl_vars(value) := var if { +_find_some_in_decl_vars(value) := vars if { arr := value[0].value count(arr) == 4 - var := [v | + vars := [v | some i in [1, 2] some v in _find_nested_vars(arr[i]) ] @@ -64,11 +64,17 @@ find_ref_vars(value) := [var | # one or two vars declared via `every`, i.e. `every x in y {}` # or `every`, i.e. `every x, y in y {}` -_find_every_vars(value) := var if { - key_var := [v | v := value.key; v.type == "var"; indexof(v.value, "$") == -1] - val_var := [v | v := value.value; v.type == "var"; indexof(v.value, "$") == -1] +_find_every_vars(value) := vars if { + key_var := [value.key | + value.key.type == "var" + indexof(value.key.value, "$") == -1 + ] + val_var := [value.value | + value.value.type == "var" + indexof(value.value.value, "$") == -1 + ] - var := array.concat(key_var, val_var) + vars := array.concat(key_var, val_var) } # METADATA @@ -215,6 +221,17 @@ found.symbols[rule_index] contains value.symbols if { walk(rule, [_, value]) } +found.comprehensions[rule_index] contains value if { + some i, rule in _rules + + # converting to string until https://github.com/open-policy-agent/opa/issues/6736 is fixed + rule_index := sprintf("%d", [i]) + + walk(rule, [_, value]) + + value.type in {"arraycomprehension", "objectcomprehension", "setcomprehension"} +} + # METADATA # description: | # finds all vars declared in `rule` *before* the `location` provided diff --git a/bundle/regal/config/provided/data.yaml b/bundle/regal/config/provided/data.yaml index dd81b388..b7813757 100644 --- a/bundle/regal/config/provided/data.yaml +++ b/bundle/regal/config/provided/data.yaml @@ -6,7 +6,7 @@ rules: annotation-without-metadata: level: error argument-always-wildcard: - except-function-name-pattern: "^mock_" + except-function-name-pattern: ^mock_ level: error constant-condition: level: error @@ -123,6 +123,8 @@ rules: level: error chained-rule-body: level: error + comprehension-term-assignment: + level: error default-over-else: level: error prefer-default-functions: false diff --git a/bundle/regal/rules/bugs/impossible-not/impossible_not.rego b/bundle/regal/rules/bugs/impossible-not/impossible_not.rego index edcb66f2..b98bbd30 100644 --- a/bundle/regal/rules/bugs/impossible-not/impossible_not.rego +++ b/bundle/regal/rules/bugs/impossible-not/impossible_not.rego @@ -108,10 +108,7 @@ var_to_ref(terms) := [terms] if terms.type == "var" var_to_ref(terms) := terms.value if terms.type == "ref" -to_string(ref) := concat(".", [path | - some part in ref - path := part.value -]) +to_string(ref) := concat(".", [part.value | some part in ref]) resolve(ref, _, _) := to_string(ref) if ref[0].value == "data" diff --git a/bundle/regal/rules/bugs/invalid-metadata-attribute/invalid_metadata_attribute.rego b/bundle/regal/rules/bugs/invalid-metadata-attribute/invalid_metadata_attribute.rego index 2e5f3a72..e669add3 100644 --- a/bundle/regal/rules/bugs/invalid-metadata-attribute/invalid_metadata_attribute.rego +++ b/bundle/regal/rules/bugs/invalid-metadata-attribute/invalid_metadata_attribute.rego @@ -24,10 +24,9 @@ report contains violation if { ) } -_block_to_string(block) := concat("\n", [line | +_block_to_string(block) := concat("\n", [entry.text | some i, entry in block i > 0 - line := entry.text ]) _find_line(block, attribute) := [line | diff --git a/bundle/regal/rules/idiomatic/prefer-set-or-object-rule/prefer_set_or_object_rule.rego b/bundle/regal/rules/idiomatic/prefer-set-or-object-rule/prefer_set_or_object_rule.rego index bfc7a970..b76347e7 100644 --- a/bundle/regal/rules/idiomatic/prefer-set-or-object-rule/prefer_set_or_object_rule.rego +++ b/bundle/regal/rules/idiomatic/prefer-set-or-object-rule/prefer_set_or_object_rule.rego @@ -56,19 +56,17 @@ is_array_conversion(value) if { count(body) == 1 - body[0].terms[0].type == "ref" - body[0].terms[0].value[0].type == "var" - body[0].terms[0].value[0].value == "assign" + [lhs, rhs] := ast.assignment_terms(body[0]) # Assignment to comprehension variable - body[0].terms[1].type == "var" - body[0].terms[1].value == var + lhs.type == "var" + lhs.value == var # On the right hand side a ref with at least one wildcard - body[0].terms[2].type == "ref" - body[0].terms[2].value[0].type == "var" + rhs.type == "ref" + rhs.value[0].type == "var" - some ref_val in body[0].terms[2].value + some ref_val in rhs.value ref_val.type == "var" startswith(ref_val.value, "$") diff --git a/bundle/regal/rules/imports/prefer-package-imports/prefer_package_imports.rego b/bundle/regal/rules/imports/prefer-package-imports/prefer_package_imports.rego index 9ee44679..af25fab6 100644 --- a/bundle/regal/rules/imports/prefer-package-imports/prefer_package_imports.rego +++ b/bundle/regal/rules/imports/prefer-package-imports/prefer_package_imports.rego @@ -42,10 +42,7 @@ custom_regal_package_and_import(pkg_path, path) if { # schemas: # - input: schema.regal.aggregate aggregate_report contains violation if { - all_package_paths := {pkg | - some entry in input.aggregate - pkg := entry.aggregate_data.package_path - } + all_package_paths := {entry.aggregate_data.package_path | some entry in input.aggregate} all_imports := {imp | some entry in input.aggregate diff --git a/bundle/regal/rules/style/comprehension-term-assignment/comprehension_term_assignment.rego b/bundle/regal/rules/style/comprehension-term-assignment/comprehension_term_assignment.rego new file mode 100644 index 00000000..3937cec9 --- /dev/null +++ b/bundle/regal/rules/style/comprehension-term-assignment/comprehension_term_assignment.rego @@ -0,0 +1,86 @@ +# METADATA +# description: Assignment can be moved to comprehension term +package regal.rules.style["comprehension-term-assignment"] + +import rego.v1 + +import data.regal.ast +import data.regal.result +import data.regal.util + +# METADATA +# description: | +# find comprehensions where assignment in the body may be replaced by +# using the value in the comprehension term (or key/value) directly +# scope: document + +# METADATA +# description: find in set and array comprehensions +report contains violation if { + comp := ast.found.comprehensions[_][_].value + + # a single expression can't be moved to term position + count(comp.body) > 1 + + # limit to simple vars, not term vars in nested + # composite structures, function calls or whatever + comp.term.type == "var" + + some expr in comp.body + + [lhs, rhs] := ast.assignment_terms(expr) + + lhs.type == comp.term.type + lhs.value == comp.term.value + + # using any of these at the term position may be OK when the value + # is simple, like [{"first_name": name.first} | some name in names] + # but almost certainly hard to understand when more complex composite values + # or call chains are involved.. + # trying to determine "complexity" is... hard / undesirable + # so let's just allow these assignnments and focus on what we do know + rhs.type in {"var", "ref"} + not _dynamic_ref(rhs) + + violation := result.fail(rego.metadata.chain(), result.ranged_location_from_text(expr)) +} + +# METADATA +# description: find in object comprehensions (both keys and values) +report contains violation if { + comp := ast.found.comprehensions[_][_].value + + # a single expression can't be moved to term position + count(comp.body) > 1 + + # only true for object comprehension + comp.key + + some expr in comp.body + + [lhs, rhs] := ast.assignment_terms(expr) + + some kind in ["key", "value"] + + lhs.type == comp[kind].type + lhs.value == comp[kind].value + + rhs.type in {"var", "ref"} + not _dynamic_ref(rhs) + + violation := result.fail(rego.metadata.chain(), result.ranged_location_from_text(expr)) +} + +_dynamic_ref(value) if { + value.type == "ref" + + _call_or_non_static(value) +} + +_call_or_non_static(ref) if ref.value[0].type == "call" + +_call_or_non_static(ref) if not _static_ref(ref) + +_static_ref(ref) if every part in util.rest(ref.value) { + part.type == "string" +} diff --git a/bundle/regal/rules/style/comprehension-term-assignment/comprehension_term_assignment_test.rego b/bundle/regal/rules/style/comprehension-term-assignment/comprehension_term_assignment_test.rego new file mode 100644 index 00000000..3519f1a3 --- /dev/null +++ b/bundle/regal/rules/style/comprehension-term-assignment/comprehension_term_assignment_test.rego @@ -0,0 +1,187 @@ +package regal.rules.style["comprehension-term-assignment_test"] + +import rego.v1 + +import data.regal.ast +import data.regal.config + +import data.regal.rules.style["comprehension-term-assignment"] as rule + +test_fail_comprehension_term_assignment_last_expr if { + module := ast.with_rego_v1(`comp := [x | + some y in input + x := y + ]`) + + r := rule.report with input as module + r == {{ + "category": "style", + "description": "Assignment can be moved to comprehension term", + "level": "error", + "location": {"col": 3, "end": {"col": 9, "row": 7}, "file": "policy.rego", "row": 7, "text": "\t\tx := y"}, + "related_resources": [{ + "description": "documentation", + "ref": config.docs.resolve_url("$baseUrl/$category/comprehension-term-assignment", "style"), + }], + "title": "comprehension-term-assignment", + }} +} + +test_fail_comprehension_term_assignment_not_last_expr if { + module := ast.with_rego_v1(`comp := [x | + some y in input + x := y + x == 1 + ]`) + + r := rule.report with input as module + r == {{ + "category": "style", + "description": "Assignment can be moved to comprehension term", + "level": "error", + "location": {"col": 3, "end": {"col": 9, "row": 7}, "file": "policy.rego", "row": 7, "text": "\t\tx := y"}, + "related_resources": [{ + "description": "documentation", + "ref": config.docs.resolve_url("$baseUrl/$category/comprehension-term-assignment", "style"), + }], + "title": "comprehension-term-assignment", + }} +} + +test_fail_comprehension_term_assignment_static_ref if { + module := ast.with_rego_v1(`comp := [x | + some y in input + x := y.attribute + ]`) + + r := rule.report with input as module + r == {{ + "category": "style", + "description": "Assignment can be moved to comprehension term", + "level": "error", + "location": { + "col": 3, + "end": {"col": 19, "row": 7}, + "file": "policy.rego", + "row": 7, + "text": "\t\tx := y.attribute", + }, + "related_resources": [{ + "description": "documentation", + "ref": config.docs.resolve_url("$baseUrl/$category/comprehension-term-assignment", "style"), + }], + "title": "comprehension-term-assignment", + }} +} + +test_fail_object_comprehension_key_assignment_static_ref if { + module := ast.with_rego_v1(`comp := {k: v | + some y, v in input + k := y.attribute + }`) + + r := rule.report with input as module + r == {{ + "category": "style", + "description": "Assignment can be moved to comprehension term", + "level": "error", + "location": { + "col": 3, + "end": {"col": 19, "row": 7}, + "file": "policy.rego", + "row": 7, "text": "\t\tk := y.attribute", + }, + "related_resources": [{ + "description": "documentation", + "ref": config.docs.resolve_url("$baseUrl/$category/comprehension-term-assignment", "style"), + }], + "title": "comprehension-term-assignment", + }} +} + +test_fail_object_comprehension_value_assignment_static_ref if { + module := ast.with_rego_v1(`comp := {k: v | + some k, y in input + v := y.attribute + }`) + + r := rule.report with input as module + r == {{ + "category": "style", + "description": "Assignment can be moved to comprehension term", + "level": "error", + "location": { + "col": 3, + "end": {"col": 19, "row": 7}, + "file": "policy.rego", + "row": 7, "text": "\t\tv := y.attribute", + }, + "related_resources": [{ + "description": "documentation", + "ref": config.docs.resolve_url("$baseUrl/$category/comprehension-term-assignment", "style"), + }], + "title": "comprehension-term-assignment", + }} +} + +test_success_not_flagging_function_call if { + module := ast.with_rego_v1(`comp := [x | + some y in input + x := http.send({"method": "get", "url": sprintf("https://example.org/%s", [y])}) + ]`) + + r := rule.report with input as module + r == set() +} + +test_success_not_flagging_composite_values if { + module := ast.with_rego_v1(`comp := [x | + some y in input + x := { + "foo": "bar", + "baz": y, + } + ]`) + + r := rule.report with input as module + r == set() +} + +test_success_not_flagging_single_expression if { + module := ast.with_rego_v1(`comp := [x | x := input.foo[_].bar]`) + + r := rule.report with input as module + r == set() +} + +test_success_not_flagging_dynamic_ref if { + module := ast.with_rego_v1(`f(x) := [1, x, 3] + + find_vars(node) := [x | + some var in node + x := f(var)[_] + ]`) + + r := rule.report with input as module + r == set() +} + +test_success_not_flagging_custom_function_call if { + module := ast.with_rego_v1(`rows := [row | + some comment in comments + row := util.to_location_object(comment.location).row + ]`) + + r := rule.report with input as module + r == set() +} + +test_success_not_flagging_assigned_comprehension if { + module := ast.with_rego_v1(`comp := [x | + some var in input + x := [y | some y in var] + ]`) + + r := rule.report with input as module + r == set() +} diff --git a/bundle/regal/rules/style/pointless-reassignment/pointless_reassignment.rego b/bundle/regal/rules/style/pointless-reassignment/pointless_reassignment.rego index 033d6a43..181a6c72 100644 --- a/bundle/regal/rules/style/pointless-reassignment/pointless_reassignment.rego +++ b/bundle/regal/rules/style/pointless-reassignment/pointless_reassignment.rego @@ -26,9 +26,7 @@ report contains violation if { not expr["with"] - expr.terms[0].value[0].type == "var" - expr.terms[0].value[0].value == "assign" - expr.terms[2].type == "var" + ast.assignment_terms(expr)[1].type == "var" violation := result.fail(rego.metadata.chain(), result.location(expr.terms)) } diff --git a/cmd/new.go b/cmd/new.go index 8034a6ed..7dff15fc 100644 --- a/cmd/new.go +++ b/cmd/new.go @@ -269,7 +269,7 @@ func scaffoldCustomRule(params newRuleCommandParams) error { } func scaffoldBuiltinRule(params newRuleCommandParams) error { - rulesDir := filepath.Join(params.output, "bundle", "regal", "rules", params.category) + rulesDir := filepath.Join(params.output, "bundle", "regal", "rules", params.category, params.name) if err := os.MkdirAll(rulesDir, 0o770); err != nil { return err diff --git a/docs/rules/style/comprehension-term-assignment.md b/docs/rules/style/comprehension-term-assignment.md new file mode 100644 index 00000000..2364c33d --- /dev/null +++ b/docs/rules/style/comprehension-term-assignment.md @@ -0,0 +1,86 @@ +# comprehension-term-assignment + +**Summary**: Assignment can be moved to comprehension term + +**Category**: Style + +**Avoid** +```rego +package policy + +import rego.v1 + +names := [name | + some user in input.users + name := user.name # redundant assignment +] +``` + +**Prefer** +```rego +package policy + +import rego.v1 + +names := [user.name | + some user in input.users +] + +# which in this case can be made a one-liner +names := [user.name | some user in input.users] +``` + +## Rationale + +Adding an intermediate assignment in a comprehension body to a variable used as the comprehension term (i.e. the value +to the left side of `|` in a comprehension) is redundant, as the value can be used directly as the comprehension term. +Making code as compact as possible should never be a goal in itself, but the same is true for making code needlessly +verbose. And in cases like `names := [user.name | some user in input.users]`, adding an intermediate assignment does +nothing to improve readability. + +## Exceptions + +This rule will only flag simple assignments where the value could be moved directly into the comprehension term. More +complex assignments involving dynamic references or function calls, will not be considered as violations. + +Example: + +```rego +first_names := [first_name | + some user in input.users + first_name := capitalize(user.name.split(" ")[0]) +] +``` + +While it's possible to move the value of the `first_name` assignment directly to the comprehension term, it's arguably +less readable, as it makes it harder to see that the form is a comprehension to begin with. + +```rego +# Not recommended +first_names := [capitalize(user.name.split(" ")[0]) | + some user in input.users +] +``` + +## Configuration Options + +This linter rule provides the following configuration options: + +```yaml +rules: + style: + comprehension-tail-assignment: + # one of "error", "warning", "ignore" + level: error +``` + +## Related Resources + +- OPA Docs: [Comprehensions](https://www.openpolicyagent.org/docs/latest/policy-language/#comprehensions) +- Regal Docs: [pointless-reassignment](https://docs.styra.com/regal/rules/style/pointless-reassignment) + +## 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 1efe1f06..c86598d2 100644 --- a/e2e/testdata/violations/most_violations.rego +++ b/e2e/testdata/violations/most_violations.rego @@ -232,6 +232,11 @@ yoda_condition if { "foo" == input.bar } +comprehension_term_assignment := [x | + some y in input + x := y.x +] + pointless_reassignment := yoda_condition ### Testing ###