Fix false positive in one-liner-rule (#1534)

Also, enable the rule! We almost always did this anyway.

Finally, update tests to not mock config for error level,
as that's no longer needed.

Fixes #1527

Signed-off-by: Anders Eknert <anders@styra.com>

Author: anderseknert

.regal/config.yaml

@@ -19,6 +19,8 @@ rules:
         - cfg
         - metadata
         - rule
+    one-liner-rule:
+      level: error
   style:
     line-length:
       level: error

bundle/regal/ast/ast_test.rego

@@ -93,9 +93,7 @@ test_function_calls if {
 	{"assign", "f"} == {call.name | some call in calls}
 }
 
-test_implicit_boolean_assignment if {
-	ast.implicit_boolean_assignment(ast.with_rego_v1(`a.b if true`).rules[0])
-}
+test_implicit_boolean_assignment if ast.implicit_boolean_assignment(ast.with_rego_v1(`a.b if true`).rules[0])
 
 test_ref_to_string if {
 	ast.ref_to_string([{"type": "var", "value": "data"}]) == `data`

bundle/regal/config/exclusion_test.rego

@@ -76,9 +76,7 @@ test_excluded_file_with_ignore if {
 		with config.merged_config as object.union(rules_config_error, rules_config_ignore_delta)
 }
 
-test_ignored_globally if {
-	config.ignored_globally("p.rego") with config.merged_config as config_ignore
-}
+test_ignored_globally if config.ignored_globally("p.rego") with config.merged_config as config_ignore
 
 test_excluded_file_cli_flag if {
 	config.ignored_globally("p.rego") with data.eval.params as params({"ignore_files": ["p.rego"]})

bundle/regal/lsp/codelens/codelens.rego

@@ -33,9 +33,7 @@ _eval_lenses contains {
 	},
 }
 
-_eval_lenses contains _rule_lens(input.regal.file.name, rule, "regal.eval", "Evaluate") if {
-	some rule in ast.rules
-}
+_eval_lenses contains _rule_lens(input.regal.file.name, rule, "regal.eval", "Evaluate") if some rule in ast.rules
 
 _debug_lenses contains {
 	"range": location.to_range(result.location(input["package"]).location),

bundle/regal/lsp/completion/providers/packagename/packagename_test.rego

@@ -137,16 +137,7 @@ test_package_name_completion_on_typing_multiple_suggestions_when_invoked if {
 
 test_build_suggestions if {
 	provider._suggestions("foo.bar.baz", "foo") == ["foo.bar.baz"]
-
 	provider._suggestions("foo.bar.baz", "bar") == ["bar.baz"]
-
 	provider._suggestions("foo.bar.baz", "ba") == ["bar.baz", "baz"]
-}
-
-test_build_suggestions_invoked if {
-	provider._suggestions("foo.bar.baz", "") == [
-		"foo.bar.baz",
-		"bar.baz",
-		"baz",
-	]
+	provider._suggestions("foo.bar.baz", "") == ["foo.bar.baz", "bar.baz", "baz"]
 }

bundle/regal/lsp/completion/ref_names.rego

@@ -23,6 +23,4 @@ ref_names contains name if {
 # if they have imported data.foo as bar, then bar should be suggested.
 # this also has the benefit of skipping future.* and rego.v1 as
 # imported_identifiers will only match data.* and input.*
-ref_names contains name if {
-	some name in ast.imported_identifiers
-}
+ref_names contains name if some name in ast.imported_identifiers

bundle/regal/main/main.rego

@@ -14,33 +14,23 @@ import data.regal.util
 
 # METADATA
 # description: set of all notices returned from linter rules
-lint.notices := _notices if {
-	"lint" in input.regal.operations
-}
+lint.notices := _notices if "lint" in input.regal.operations
 
 # METADATA
 # description: map of all ignore directives encountered when linting
-lint.ignore_directives[input.regal.file.name] := ast.ignore_directives if {
-	"lint" in input.regal.operations
-}
+lint.ignore_directives[input.regal.file.name] := ast.ignore_directives if "lint" in input.regal.operations
 
 # METADATA
 # description: all violations from non-aggregate rules
-lint.violations := report if {
-	"lint" in input.regal.operations
-}
+lint.violations := report if "lint" in input.regal.operations
 
 # METADATA
 # description: map of all aggregated data from aggregate rules, keyed by category/title
-lint.aggregates := aggregate if {
-	"collect" in input.regal.operations
-}
+lint.aggregates := aggregate if "collect" in input.regal.operations
 
 # METADATA
 # description: all violations from aggregate rules
-lint.aggregate.violations := aggregate_report if {
-	"aggregate" in input.regal.operations
-}
+lint.aggregate.violations := aggregate_report if "aggregate" in input.regal.operations
 
 _file_name_relative_to_root(filename, "/") := trim_prefix(filename, "/")
 _file_name_relative_to_root(filename, root) := trim_prefix(filename, concat("", [root, "/"])) if root != "/"

bundle/regal/rules/custom/one-liner-rule/one_liner_rule.rego

@@ -14,8 +14,6 @@ import data.regal.util
 notices contains result.notice(rego.metadata.chain()) if not capabilities.has_if
 
 report contains violation if {
-	max_line_length := object.get(config.rules, ["custom", "one-liner-rule", "max-line-length"], 120)
-
 	some rule in input.rules
 
 	# Bail out of rules with else for now. It is possible that they can be made
@@ -35,22 +33,25 @@ report contains violation if {
 		line := trim_space(s)
 	]
 
-	# Technically, the `if` could be on another line, but who would do that?
 	regex.match(`\s+if`, lines[0])
 	_rule_body_brackets(lines)
 
 	# ideally we'd take style preference into account but for now assume tab == 4 spaces
 	# then just add the sum of the line counts minus the removed '{' character
 	# redundant parens added by `opa fmt` :/
-	((4 + count(lines[0])) + count(lines[1])) - 1 < max_line_length
+	((4 + count(lines[0])) + count(lines[1])) - 1 < _max_line_length
 
 	not _comment_in_body(rule_location.row, object.get(input, "comments", []), lines)
 
 	violation := result.fail(rego.metadata.chain(), result.location(rule.head))
 }
 
+default _max_line_length := 120
+
+_max_line_length := config.rules.custom["one-liner-rule"]["max-line-length"]
+
 # K&R style
-_rule_body_brackets(lines) if endswith(lines[0], "{")
+_rule_body_brackets(lines) if regex.match(`.*if\s*{$`, lines[0])
 
 # Allman style
 _rule_body_brackets(lines) if {

bundle/regal/rules/custom/one-liner-rule/one_liner_rule_test.rego

@@ -12,7 +12,7 @@ test_fail_could_be_one_liner if {
 		input.yes
 	}
 	`)
-	r := rule.report with input as module with config.rules as {"custom": {"one-liner-rule": {"level": "error"}}}
+	r := rule.report with input as module
 
 	r == expected_with_location({
 		"col": 2,
@@ -32,7 +32,7 @@ test_fail_could_be_one_liner_all_keywords if {
 		input.yes
 	}
 	`)
-	r := rule.report with input as module with config.rules as {"custom": {"one-liner-rule": {"level": "error"}}}
+	r := rule.report with input as module
 
 	r == expected_with_location({
 		"col": 2,
@@ -54,8 +54,8 @@ test_fail_could_be_one_liner_allman_style if {
 		input.yes
 	}
 	`)
+	r := rule.report with input as module
 
-	r := rule.report with input as module with config.rules as {"custom": {"one-liner-rule": {"level": "error"}}}
 	r == expected_with_location({
 		"col": 2,
 		"row": 5,
@@ -73,8 +73,8 @@ test_success_too_long_for_a_one_liner if {
 		some_really_long_rule_name_in_fact_53_characters_long == another_long_rule_but_only_45_characters_long
 	}
 	`)
+	r := rule.report with input as module
 
-	r := rule.report with input as module with config.rules as {"custom": {"one-liner-rule": {"level": "error"}}}
 	r == set()
 }
 
@@ -84,8 +84,7 @@ test_success_too_long_for_a_one_liner_configured_line_length if {
 		some_really_long_rule_name_in_fact_53_characters_long
 	}
 	`)
-	r := rule.report with input as module
-		with config.rules as {"custom": {"one-liner-rule": {"level": "error", "max-line-length": 50}}}
+	r := rule.report with input as module with config.rules as {"custom": {"one-liner-rule": {"max-line-length": 50}}}
 
 	r == set()
 }
@@ -97,7 +96,7 @@ test_success_no_one_liner_comment_in_rule_body if {
 		1 == 1
 	}
 	`)
-	r := rule.report with input as module with config.rules as {"custom": {"one-liner-rule": {"level": "error"}}}
+	r := rule.report with input as module
 
 	r == set()
 }
@@ -108,7 +107,7 @@ test_success_no_one_liner_comment_in_rule_body_same_line if {
 		1 == 1 # Surely one equals one
 	}
 	`)
-	r := rule.report with input as module with config.rules as {"custom": {"one-liner-rule": {"level": "error"}}}
+	r := rule.report with input as module
 
 	r == set()
 }
@@ -120,7 +119,7 @@ test_success_no_one_liner_comment_in_rule_body_line_below if {
 		# Surely one equals one
 	}
 	`)
-	r := rule.report with input as module with config.rules as {"custom": {"one-liner-rule": {"level": "error"}}}
+	r := rule.report with input as module
 
 	r == set()
 }
@@ -131,14 +130,13 @@ test_success_does_not_use_if_v0 if {
 		1 == 1
 	}
 	`)
-	r := rule.report with input as module with config.rules as {"custom": {"one-liner-rule": {"level": "error"}}}
+	r := rule.report with input as module
 
 	r == set()
 }
 
 test_success_already_a_one_liner if {
 	r := rule.report with input as ast.with_rego_v1(`allow if 1 == 1`)
-		with config.rules as {"custom": {"one-liner-rule": {"level": "error"}}}
 
 	r == set()
 }
@@ -155,6 +153,20 @@ test_has_notice_if_unmet_capability if {
 	}}
 }
 
+# verify fix for https://github.com/StyraInc/regal/issues/1527
+test_fail_single_expression_spanning_multiple_lines_already_a_one_liner if {
+	module := ast.policy(`
+
+	foo := bar if baz in {
+		"foo",
+		"bar",
+	}
+	`)
+	r := rule.report with input as module
+
+	r == set()
+}
+
 expected := {
 	"category": "custom",
 	"description": "Rule body could be made a one-liner",

bundle/regal/util/util_test.rego

@@ -4,6 +4,7 @@ import data.regal.util
 
 test_find_duplicates if {
 	util.find_duplicates([1, 1, 2, 3, 3, 3]) == {{0, 1}, {3, 4, 5}}
+	util.find_duplicates([1, 2, 3]) == set()
 }
 
 test_json_pretty if {