Rule: rule-assigns-default (#1243)

Flags rules where the default value is used as the assigned value
in any other incremental definition.

Fixes #1018

(unrelated changes mostly adding some ignore directives in Go code
is from me having update golangci-lint which led to a few new issues
getting reported)

Signed-off-by: Anders Eknert <[email protected]>

Author: anderseknert

README.md

@@ -214,6 +214,7 @@ The following rules are currently available:
 | 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-assigns-default](https://docs.styra.com/regal/rules/bugs/rule-assigns-default)                    | Rule assigned its default value                           |
 | 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                     |

bundle/regal/config/provided/data.yaml

@@ -32,6 +32,8 @@ rules:
       level: error
     redundant-existence-check:
       level: error
+    rule-assigns-default:
+      level: error
     rule-named-if:
       level: error
     rule-shadows-builtin:

bundle/regal/rules/bugs/rule-assigns-default/rule_assigns_default.rego

@@ -0,0 +1,28 @@
+# regal eval:use-as-input
+# METADATA
+# description: Rule assigned its default value
+package regal.rules.bugs["rule-assigns-default"]
+
+import rego.v1
+
+import data.regal.ast
+import data.regal.result
+
+report contains violation if {
+	some rule in input.rules
+
+	not rule["default"]
+
+	ref := ast.ref_to_string(rule.head.ref)
+
+	_default_rule_values[ref] == rule.head.value.value
+
+	violation := result.fail(rego.metadata.chain(), result.location(rule.head.value))
+}
+
+_default_rule_values[ref] := rule.head.value.value if {
+	some rule in input.rules
+	rule["default"]
+
+	ref := ast.ref_to_string(rule.head.ref)
+}

bundle/regal/rules/bugs/rule-assigns-default/rule_assigns_default_test.rego

@@ -0,0 +1,55 @@
+package regal.rules.bugs["rule-assigns-default_test"]
+
+import rego.v1
+
+import data.regal.ast
+import data.regal.config
+
+import data.regal.rules.bugs["rule-assigns-default"] as rule
+
+test_fail_rule_assigned_default_value if {
+	module := ast.with_rego_v1(`
+
+	default allow := false
+
+	allow := false if {
+		some conditions in policy
+	}
+	`)
+	r := rule.report with input as module
+
+	r == {{
+		"category": "bugs",
+		"description": "Rule assigned its default value",
+		"level": "error",
+		"location": {
+			"col": 11,
+			"end": {
+				"col": 16,
+				"row": 9,
+			},
+			"file": "policy.rego",
+			"row": 9,
+			"text": "\tallow := false if {",
+		},
+		"related_resources": [{
+			"description": "documentation",
+			"ref": config.docs.resolve_url("$baseUrl/$category/rule-assigns-default", "bugs"),
+		}],
+		"title": "rule-assigns-default",
+	}}
+}
+
+test_success_rule_not_assigned_default_value if {
+	module := ast.with_rego_v1(`
+
+	default allow := false
+
+	allow := true if {
+		some conditions in policy
+	}
+	`)
+	r := rule.report with input as module
+
+	r == set()
+}

docs/rules/bugs/rule-assigns-default.md

@@ -0,0 +1,61 @@
+# rule-assigns-default
+
+**Summary**: Rule assigned its default value
+
+**Category**: Bugs
+
+**Avoid**
+```rego
+package policy
+
+import rego.v1
+
+default allow := false
+
+# this rule assigns the same value as the default
+# and the policy would work the same without it
+allow := false if {
+    not "admin" in input.user.roles
+}
+```
+
+**Prefer**
+```rego
+package policy
+
+import rego.v1
+
+default allow := false
+
+# or just `allow if {` as `true` is implicit
+allow := true if {
+    "admin" in input.user.roles
+}
+```
+
+## Rationale
+
+When a default value is used for a rule, assigning the same value anywhere else to that rule is pointless, as the rule
+would evaluate to the same value with or without the assignment.
+
+## Configuration Options
+
+This linter rule provides the following configuration options:
+
+```yaml
+rules:
+  bugs:
+    rule-assigns-default:
+      # one of "error", "warning", "ignore"
+      level: error
+```
+
+## Related Resources
+
+- GitHub: [Source Code](https://github.com/StyraInc/regal/blob/main/bundle/regal/rules/bugs/rule-assigns-default/rule_assigns_default.rego)
+
+## 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)!

e2e/cli_test.go

@@ -456,6 +456,10 @@ func TestLintAggregateIgnoreDirective(t *testing.T) {
 		t.Errorf("expected 2 violations, got %d", rep.Summary.NumViolations)
 	}
 
+	if rep.Summary.NumViolations == 0 {
+		t.Fatal("expected violations, got none")
+	}
+
 	if rep.Violations[0].Title != "no-defined-entrypoint" {
 		t.Errorf("expected violation 'no-defined-entrypoint', got %q", rep.Violations[0].Title)
 	}

e2e/testdata/violations/most_violations.rego

@@ -68,7 +68,7 @@ top_level_iteration := input[_]
 
 unassigned_return_value if indexof("foo", "o")
 
-zero_arity_function() := true
+zero_arity_function := true
 
 inconsistent_args(a, b) if {
 	a == b
@@ -93,9 +93,9 @@ impossible_not if {
 	not partial
 }
 
-argument_always_wildcard(_) if true
+argument_always_wildcard(_) = true
 
-argument_always_wildcard(_) if true
+argument_always_wildcard(_) = true
 
 # title: annotation without metadata
 some_rule := true
@@ -107,6 +107,12 @@ unused_output_variable if {
 	input[x]
 }
 
+default rule_assigns_default := false
+
+rule_assigns_default := false if {
+	input.yes
+}
+
 ### Idiomatic ###
 
 custom_has_key_construct(map, key) if {
@@ -164,7 +170,7 @@ line_length := "should be no longer than 120 characters but this line is really
 
 #no-whitespace-comment
 
- opa_fmt := "fail"
+opa_fmt := "fail"
 
 preferSnakeCase := "fail"
 
@@ -178,7 +184,9 @@ use_assignment = "oparator"
 
 chained_rule_body if {
 	input.x
-} {
+}
+
+chained_rule_body if {
 	input.y
 }
 
@@ -243,6 +251,7 @@ pointless_reassignment := yoda_condition
 
 # this will also trigger the test-outside-test-package rule
 test_identically_named_tests := true
+
 test_identically_named_tests := false
 
 todo_test_bad if {
@@ -253,7 +262,7 @@ print_or_trace_call if {
 	print("forbidden!")
 }
 
-abc if true
+abc = true
 
 # metasyntactic variable
 foo := "bar"
@@ -271,6 +280,7 @@ y if {
 
 # double negation
 not_fine := true
+
 fine if not not_fine
 
 # rule name repeats package name

internal/io/io.go

@@ -100,7 +100,7 @@ func FindInput(file string, workspacePath string) (string, io.Reader) {
 	relative := strings.TrimPrefix(file, workspacePath)
 	components := strings.Split(filepath.Dir(relative), string(filepath.Separator))
 
-	for i := range len(components) {
+	for i := range components {
 		inputPath := filepath.Join(workspacePath, filepath.Join(components[:len(components)-i]...), "input.json")
 
 		f, err := os.Open(inputPath)

internal/lsp/diff.go

@@ -37,6 +37,7 @@ type operation struct {
 
 // operations returns the list of operations to convert a into b, consolidating
 // operations for multiple lines and not including equal lines.
+// nolint:gosec
 func operations(a, b []string) []*operation {
 	if len(a) == 0 && len(b) == 0 {
 		return nil

internal/lsp/documentsymbol.go

@@ -24,6 +24,7 @@ func documentSymbols(
 
 	lines := strings.Split(contents, "\n")
 
+	// nolint:gosec
 	pkgRange := types.Range{
 		Start: types.Position{Line: 0, Character: 0},
 		End:   types.Position{Line: uint(len(lines) - 1), Character: uint(len(lines[len(lines)-1]))},
@@ -128,6 +129,7 @@ func documentSymbols(
 	return docSymbols
 }
 
+// nolint:gosec
 func locationToRange(location *ast.Location) types.Range {
 	lines := bytes.Split(location.Text, []byte("\n"))
 

internal/lsp/foldingrange.go

@@ -22,6 +22,7 @@ func (s stack) Pop() (stack, scanner.Position) {
 	return s[:l-1], s[l-1]
 }
 
+// nolint:gosec
 func TokenFoldingRanges(policy string) []types.FoldingRange {
 	scn, err := scanner.New(strings.NewReader(policy))
 	if err != nil {
@@ -98,6 +99,7 @@ func TokenFoldingRanges(policy string) []types.FoldingRange {
 	return foldingRanges
 }
 
+// nolint:gosec
 func findFoldingRanges(text string, module *ast.Module) []types.FoldingRange {
 	uintZero := uint(0)
 

internal/lsp/hover/hover.go

@@ -143,6 +143,7 @@ func UpdateBuiltinPositions(cache *cache.Cache, uri string, builtins map[string]
 
 	builtinsOnLine := map[uint][]types2.BuiltinPosition{}
 
+	// nolint:gosec
 	for _, call := range rego.AllBuiltinCalls(module, builtins) {
 		line := uint(call.Location.Row)
 

internal/lsp/lint.go

@@ -117,6 +117,7 @@ func updateParse(
 			link = "https://docs.styra.com/opa/errors/" + hints[0]
 		}
 
+		// nolint:gosec
 		diags = append(diags, types.Diagnostic{
 			Severity: 1, // parse errors are the only error Diagnostic the server sends
 			Range: types.Range{
@@ -322,6 +323,7 @@ type astError struct {
 	Message  string        `json:"message"`
 }
 
+// nolint:gosec
 func getRangeForViolation(item report.Violation) types.Range {
 	start := types.Position{
 		Line:      uint(max(item.Location.Row-1, 0)),

internal/lsp/rego/rego.go

@@ -38,6 +38,7 @@ type KeywordUseLocation struct {
 }
 
 func PositionFromLocation(loc *ast.Location) types.Position {
+	// nolint:gosec
 	return types.Position{
 		Line:      uint(loc.Row - 1),
 		Character: uint(loc.Col - 1),

internal/lsp/server.go

@@ -1855,6 +1855,7 @@ func (l *LanguageServer) handleTextDocumentDefinition(
 		return nil, nil
 	}
 
+	// nolint:gosec
 	loc := types.Location{
 		URI: uri.FromPath(l.clientIdentifier, definition.Result.File),
 		Range: types.Range{