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

Rule: prefer-strings-count #948

Merged
merged 1 commit into from
Jul 30, 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
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