Skip to content

Commit

Permalink
Rule: prefer-strings-count (StyraInc#948)
Browse files Browse the repository at this point in the history
Fixes StyraInc#940

Signed-off-by: Anders Eknert <anders@styra.com>
  • Loading branch information
anderseknert authored and srenatus committed Oct 1, 2024
1 parent a090667 commit 7697995
Show file tree
Hide file tree
Showing 8 changed files with 139 additions and 3 deletions.
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ The following rules are currently available:
| 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 |
Expand Down
2 changes: 2 additions & 0 deletions bundle/regal/capabilities.rego
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ provided := data.internal.capabilities

has_object_keys if "object.keys" in object.keys(config.capabilities.builtins)

has_strings_count if "strings.count" in object.keys(config.capabilities.builtins)

# if if if!
has_if if "if" in config.capabilities.future_keywords

Expand Down
4 changes: 3 additions & 1 deletion bundle/regal/config/provided/data.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ rules:
annotation-without-metadata:
level: error
argument-always-wildcard:
except-function-name-pattern: '^mock_'
level: error
except-function-name-pattern: "^mock_"
constant-condition:
level: error
deprecated-builtin:
Expand Down Expand Up @@ -83,6 +83,8 @@ rules:
level: error
use-some-for-output-vars:
level: error
use-strings-count:
level: error
imports:
avoid-importing-input:
level: error
Expand Down
35 changes: 35 additions & 0 deletions bundle/regal/rules/idiomatic/use_strings_count.rego
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
# METADATA
# description: Use `strings.count` where possible
package regal.rules.idiomatic["use-strings-count"]

import rego.v1

import data.regal.ast
import data.regal.capabilities
import data.regal.result

# METADATA
# description: Missing capability for built-in function `strings.count`
# custom:
# severity: warning
notices contains result.notice(rego.metadata.chain()) if not capabilities.has_object_keys

# METADATA
# description: flag calls to `count` where the first argument is a call to `indexof_n`
report contains violation if {
some rule in input.rules

ref := ast.refs[_][_]

ref[0].value[0].type == "var"
ref[0].value[0].value == "count"

ref[1].type == "call"
ref[1].value[0].value[0].type == "var"
ref[1].value[0].value[0].value == "indexof_n"

loc1 := result.location(ref[0])
loc2 := result.ranged_location_from_text(ref[1])

violation := result.fail(rego.metadata.chain(), object.union(loc1, {"location": {"end": loc2.location.end}}))
}
42 changes: 42 additions & 0 deletions bundle/regal/rules/idiomatic/use_strings_count_test.rego
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
package regal.rules.idiomatic["use-strings-count_test"]

import rego.v1

import data.regal.ast
import data.regal.config

import data.regal.rules.idiomatic["use-strings-count"] as rule

test_fail_can_use_strings_count if {
module := ast.with_rego_v1(`x := count(indexof_n("foo", "o"))`)

r := rule.report with input as module
r == {{
"category": "idiomatic",
"description": "Use `strings.count` where possible",
"level": "error",
"location": {
"col": 6,
"file": "policy.rego",
"row": 5,
"text": `x := count(indexof_n("foo", "o"))`,
"end": {"col": 34, "row": 5},
},
"related_resources": [{
"description": "documentation",
"ref": config.docs.resolve_url("$baseUrl/$category/use-strings-count", "idiomatic"),
}],
"title": "use-strings-count",
}}
}

test_has_notice_if_unmet_capability if {
r := rule.notices with config.capabilities as {}
r == {{
"category": "idiomatic",
"description": "Missing capability for built-in function `strings.count`",
"level": "notice",
"severity": "warning",
"title": "use-strings-count",
}}
}
50 changes: 50 additions & 0 deletions docs/rules/idiomatic/use-strings-count.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
# use-strings-count

**Summary**: Use `strings.count` where possible

**Category**: Idiomatic

**Avoid**
```rego
package policy
import rego.v1
num_as := count(indexof_n("foobarbaz", "a"))
```

**Prefer**
```rego
package policy
import rego.v1
num_as := strings.count("foobarbaz", "a")
```

## Rationale

The `strings.count` function added in [OPA v0.67.0](https://github.com/open-policy-agent/opa/releases/tag/v0.67.0)
is both more readable and efficient compared to using `count(indexof_n(...))` and should therefore be preferred.

## Configuration Options

This linter rule provides the following configuration options:

```yaml
rules:
idiomatic:
use-strings-count:
# one of "error", "warning", "ignore"
level: error
```
## Related Resources
- OPA Docs: [strings.count](https://www.openpolicyagent.org/docs/latest/policy-reference/#builtin-strings-stringscount)
## 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)!
6 changes: 4 additions & 2 deletions e2e/cli_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -710,8 +710,9 @@ func TestLintWithCustomCapabilitiesAndUnmetRequirement(t *testing.T) {
// This is only an informative warning — command should not fail
expectExitCode(t, err, 0, &stdout, &stderr)

expectOut := "1 file linted. No violations found. 2 rules skipped:\n" +
expectOut := "1 file linted. No violations found. 3 rules skipped:\n" +
"- custom-has-key-construct: Missing capability for built-in function `object.keys`\n" +
"- use-strings-count: Missing capability for built-in function `strings.count`\n" +
"- use-rego-v1: Missing capability for `import rego.v1`\n\n"

if stdout.String() != expectOut {
Expand All @@ -736,8 +737,9 @@ func TestLintWithCustomCapabilitiesAndUnmetRequirementMultipleFiles(t *testing.T
// This is only an informative warning — command should not fail
expectExitCode(t, err, 0, &stdout, &stderr)

expectOut := "2 files linted. No violations found. 2 rules skipped:\n" +
expectOut := "2 files linted. No violations found. 3 rules skipped:\n" +
"- custom-has-key-construct: Missing capability for built-in function `object.keys`\n" +
"- use-strings-count: Missing capability for built-in function `strings.count`\n" +
"- use-rego-v1: Missing capability for `import rego.v1`\n\n"

if stdout.String() != expectOut {
Expand Down
2 changes: 2 additions & 0 deletions e2e/testdata/violations/most_violations.rego
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,8 @@ incremental if input.x

incremental if input.y

prefer_strings_count := count(indexof_n("foobarbaz", "a"))

### Style ###

# avoid-get-and-list-prefix
Expand Down

0 comments on commit 7697995

Please sign in to comment.