100% test coverage

Like with previous PR aiming to increase our test coverage (#1058),
doing so this time revealed a few places where code was unused, or
could be rewritten to be better optimized, removed, or made easier
to read.

One find that is yet to be addressed here is how we copy the whole
`ast.found.refs` object into `ast.all_refs`, which surely comes with
a cost that seems unnecessary, given we can use `ast.found.refs`
directly instead. But since there are a few consumers, and they are
not **exactly** identical (`all_refs` contains import refs too),
I'll follow up on that in a later PR.

I'm also adding the codecov badge to the README to make it easy to
see when our coverage drops. But I'm not as convinced we should
fail PRs when coverage isn't 100% as I was before, as it might make
for a worse contributor experience.

Fixes #918

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

README.md

@@ -2,6 +2,7 @@
 
 [![Build Status](https://github.com/styrainc/regal/workflows/Build/badge.svg?branch=main)](https://github.com/styrainc/regal/actions)
 ![OPA v0.68.0](https://openpolicyagent.org/badge/v0.68.0)
+[![codecov](https://codecov.io/github/StyraInc/regal/graph/badge.svg?token=EQK01YF3X3)](https://codecov.io/github/StyraInc/regal)
 
 Regal is a linter and language server for [Rego](https://www.openpolicyagent.org/docs/latest/policy-language/), making
 your Rego magnificent, and you the ruler of rules!

build/simplecov/simplecov.rego

@@ -42,3 +42,12 @@ to_value(cm, ncm, line) := null if {
 	not cm[line]
 	not ncm[line]
 }
+
+# utility rule to evaluate when only the
+# lines not covered are of interest
+# invoke like:
+#   regal test --coverage bundle \
+#   | opa eval -f pretty -I -d build/simplecov/simplecov.rego 'data.build.simplecov.not_covered'
+not_covered[file] := info.not_covered if {
+	some file, info in input.files
+}

bundle/regal/ast/ast.rego

@@ -44,12 +44,18 @@ builtin_namespaces contains namespace if {
 
 # METADATA
 # description: |
-#   provide the package name / path as originally declared in the
-#   input policy, so "package foo.bar" would return "foo.bar"
-package_name := concat(".", [path.value |
+#   provides the package path values (strings) as an array starting _from_ "data":
+#   package foo.bar -> ["foo", "bar"]
+package_path := [path.value |
 	some i, path in input["package"].path
 	i > 0
-])
+]
+
+# METADATA
+# description: |
+#   provide the package name / path as originally declared in the
+#   input policy, so "package foo.bar" would return "foo.bar"
+package_name := concat(".", package_path)
 
 named_refs(refs) := [ref |
 	some i, ref in refs
@@ -133,8 +139,6 @@ is_output_var(rule, var) if {
 # METADATA
 # description: as the name implies, answers whether provided value is a ref
 # scope: document
-default is_ref(_) := false
-
 is_ref(value) if value.type == "ref"
 
 is_ref(value) if value[0].type == "ref"
@@ -177,12 +181,12 @@ _exclude_arg(_, _, arg) if arg.type == "call"
 # ignore here, as it's covered elsewhere
 _exclude_arg("assign", 0, _)
 
-all_rules_refs contains found.refs[_][_]
-
 # METADATA
-# description: set containing all references found in the input AST
+# description: |
+#   set containing all references found in the input AST
+#   NOTE: likely to be deprecated — prefer to use `ast.found.refs` over this
 # scope: document
-all_refs contains value if some value in all_rules_refs
+all_refs contains found.refs[_][_]
 
 all_refs contains imported.path if some imported in input.imports
 
@@ -217,15 +221,8 @@ static_ref(ref) if every t in array.slice(ref.value, 1, count(ref.value)) {
 # METADATA
 # description: provides a set of names of all built-in functions called in the input policy
 builtin_functions_called contains name if {
-	some value in all_refs
-
-	value[0].value[0].type == "var"
-	not value[0].value[0].value in {"input", "data"}
-
-	name := concat(".", [value |
-		some part in value[0].value
-		value := part.value
-	])
+	name := function_calls[_][_].name
+	name in builtin_names
 }
 
 # METADATA

bundle/regal/ast/imports.rego

@@ -71,6 +71,8 @@ _arr(xs) := [y.value | some y in xs.path.value]
 
 _has_keyword(["future", "keywords"], _)
 
+_has_keyword(["future", "keywords", "every"], "in")
+
 _has_keyword(["future", "keywords", keyword], keyword)
 
 _has_keyword(["rego", "v1"], _)

bundle/regal/ast/imports_test.rego

@@ -0,0 +1,43 @@
+package regal.ast_test
+
+import rego.v1
+
+import data.regal.ast
+
+test_imports_keyword_rego_v1 if {
+	module := ast.policy("import rego.v1")
+
+	ast.imports_keyword(module.imports, "in")
+	ast.imports_keyword(module.imports, "if")
+	ast.imports_keyword(module.imports, "every")
+	ast.imports_keyword(module.imports, "contains")
+}
+
+test_imports_keyword_future_keywords_all if {
+	module := ast.policy("import future.keywords")
+
+	ast.imports_keyword(module.imports, "in")
+	ast.imports_keyword(module.imports, "if")
+	ast.imports_keyword(module.imports, "every")
+	ast.imports_keyword(module.imports, "contains")
+}
+
+test_imports_keyword_future_keywords_single if {
+	module := ast.policy("import future.keywords.contains")
+
+	ast.imports_keyword(module.imports, "contains")
+
+	not ast.imports_keyword(module.imports, "in")
+	not ast.imports_keyword(module.imports, "if")
+	not ast.imports_keyword(module.imports, "every")
+}
+
+test_imports_keyword_future_keywords_every if {
+	module := ast.policy("import future.keywords.every")
+
+	ast.imports_keyword(module.imports, "every")
+	ast.imports_keyword(module.imports, "in")
+
+	not ast.imports_keyword(module.imports, "if")
+	not ast.imports_keyword(module.imports, "contains")
+}

bundle/regal/config/config_test.rego

@@ -178,3 +178,7 @@ test_all_configured_rules_exist if {
 
 	count(missing_rules - go_rules) == 0
 }
+
+test_default_level_is_error if {
+	config.rule_level("unknown") == "error"
+}

bundle/regal/config/exclusion_test.rego

@@ -4,7 +4,6 @@ import rego.v1
 
 import data.regal.config
 
-# map[pattern: string]map[filename: string]excluded: bool
 cases := {
 	"p.rego": {
 		"p.rego": true,
@@ -47,11 +46,10 @@ cases := {
 test_all_cases_are_as_expected if {
 	not_exp := {pattern: res |
 		some pattern, subcases in cases
-		res := {file: res1 |
+		res := {file |
 			some file, exp in subcases
 			act := config.exclude(pattern, file)
 			exp != act
-			res1 := {"exp": exp, "act": act}
 		}
 		count(res) > 0
 	}
@@ -97,3 +95,9 @@ test_excluded_file_cli_overrides_config if {
 		with data.eval.params as object.union(params, {"ignore_files": [""]})
 	e == false
 }
+
+test_trailing_slash if {
+	config.trailing_slash("foo/**/bar") == {"foo/**/bar", "foo/**/bar/**"}
+	config.trailing_slash("foo") == {"foo", "foo/**"}
+	config.trailing_slash("foo/**") == {"foo/**"}
+}

bundle/regal/lsp/completion/kind/kind_test.rego

@@ -0,0 +1,10 @@
+package regal.lsp.completion.kind_test
+
+import rego.v1
+
+import data.regal.lsp.completion.kind
+
+test_kind_for_coverage if {
+	kind_values := [value | some value in kind]
+	sort(kind_values) == numbers.range(1, 25)
+}

bundle/regal/lsp/completion/main.rego

@@ -10,9 +10,7 @@ import rego.v1
 # METADATA
 # description: main entry point for completion suggestions
 # entrypoint: true
-items contains item if {
-	some provider
-	completion := data.regal.lsp.completion.providers[provider].items[_]
-
-	item := object.union(completion, {"_regal": {"provider": provider}})
+items contains object.union(completion, {"_regal": {"provider": provider}}) if {
+	some provider, completion
+	data.regal.lsp.completion.providers[provider].items[completion]
 }

bundle/regal/lsp/completion/main_test.rego

@@ -0,0 +1,11 @@
+package regal.lsp.completion_test
+
+import rego.v1
+
+import data.regal.lsp.completion
+
+test_completion_entrypoint if {
+	items := completion.items with completion.providers as {"test": {"items": {{"foo": "bar"}}}}
+
+	items == {{"_regal": {"provider": "test"}, "foo": "bar"}}
+}

bundle/regal/lsp/completion/providers/booleans/booleans.rego

@@ -5,8 +5,6 @@ import rego.v1
 import data.regal.lsp.completion.kind
 import data.regal.lsp.completion.location
 
-parsed_current_file := data.workspace.parsed[input.regal.file.uri]
-
 items contains item if {
 	position := location.to_position(input.regal.context.location)
 

bundle/regal/lsp/completion/providers/locals/locals.rego

@@ -12,11 +12,12 @@ items contains item if {
 
 	line := input.regal.file.lines[position.line]
 	line != ""
+
 	location.in_rule_body(line)
 
-	word := location.word_at(line, input.regal.context.location.col)
+	not _excluded(line, position)
 
-	not excluded(line, position)
+	word := location.word_at(line, input.regal.context.location.col)
 
 	some local in location.find_locals(
 		parsed_current_file.rules,
@@ -38,7 +39,7 @@ items contains item if {
 
 # exclude local suggestions in function args definition,
 # as those would recursively contribute to themselves
-excluded(line, position) if _function_args_position(substring(line, 0, position.character))
+_excluded(line, position) if _function_args_position(substring(line, 0, position.character))
 
 _function_args_position(text) if {
 	text == trim_left(text, " \t")

bundle/regal/lsp/completion/providers/locals/locals_test.rego

@@ -122,3 +122,29 @@ function(bar) := f if {
 	count(items) == 1
 	utils.expect_item(items, "foo", {"end": {"character": 18, "line": 4}, "start": {"character": 17, "line": 4}})
 }
+
+test_no_locals_in_completion_items_function_args if {
+	workspace := {"file:///p.rego": `package policy
+
+import rego.v1
+
+function() if {
+	foo := 1
+}
+`}
+
+	regal_module := {"regal": {
+		"file": {
+			"name": "p.rego",
+			"uri": "file:///p.rego",
+			"lines": split(workspace["file:///p.rego"], "\n"),
+		},
+		"context": {"location": {
+			"row": 5,
+			"col": 10,
+		}},
+	}}
+	items := provider.items with input as regal_module with data.workspace.parsed as utils.parsed_modules(workspace)
+
+	count(items) == 0
+}

bundle/regal/lsp/completion/providers/rulerefs/rulerefs.rego

@@ -9,10 +9,6 @@ import data.regal.lsp.completion.location
 
 ref_is_internal(ref) if contains(ref, "._")
 
-default determine_ref_prefix(_) := ""
-
-determine_ref_prefix(word) := word if word != ":="
-
 position := location.to_position(input.regal.context.location)
 
 line := input.regal.file.lines[position.line]
@@ -92,11 +88,10 @@ matching_rule_ref_suggestions contains ref if {
 
 	# \W is used here to match ( in the case of func() := ..., as well as the space in the case of rule := ...
 	first_word := regex.split(`\W+`, trim_space(line))[0]
-	prefix := determine_ref_prefix(word.text)
 
 	some ref in rule_ref_suggestions
 
-	startswith(ref, prefix)
+	startswith(ref, word.text)
 
 	# this is to avoid suggesting a recursive rule, e.g. rule := rule, or func() := func()
 	ref != first_word

bundle/regal/lsp/completion/ref_names.rego

@@ -6,15 +6,10 @@ import data.regal.ast
 
 # ref_names returns a list of ref names that are used in the module.
 # built-in functions are not included as they are provided by another completions provider.
-# imports are not included as we need to use the imported_identifier instead
-# (i.e. maybe an alias).
 ref_names contains name if {
-	some ref in ast.all_refs
+	name := ast.ref_to_string(ast.found.refs[_][_].value)
 
-	name := ast.ref_to_string(ref.value)
-
-	not name in ast.builtin_functions_called
-	not name in imports
+	not name in ast.builtin_names
 }
 
 # if a user has imported data.foo, then foo should be suggested.
@@ -24,10 +19,3 @@ ref_names contains name if {
 ref_names contains name if {
 	some name in ast.imported_identifiers
 }
-
-# imports are not shown as we need to show the imported alias instead
-imports contains ref if {
-	some imp in ast.imports
-
-	ref := ast.ref_to_string(imp.path.value)
-}

bundle/regal/lsp/completion/ref_names_test.rego

@@ -0,0 +1,37 @@
+package regal.lsp.completion_test
+
+import rego.v1
+
+import data.regal.ast
+import data.regal.capabilities
+
+import data.regal.lsp.completion
+
+test_ref_names if {
+	module := ast.with_rego_v1(`
+	import data.imp
+	import data.foo.bar as bb
+
+	x := 1
+
+	allow if {
+		some x
+		input.foo[x] == data.bar[x]
+		startswith("hey", "h")
+
+		imp.foo == data.x
+	}
+	`)
+
+	ref_names := completion.ref_names with input as module
+		with data.internal.combined_config as {"capabilities": capabilities.provided}
+
+	ref_names == {
+		"imp",
+		"bb",
+		"input.foo.$x",
+		"data.bar.$x",
+		"imp.foo",
+		"data.x",
+	}
+}

bundle/regal/main/main.rego

@@ -16,7 +16,7 @@ lint_aggregate.violations := aggregate_report
 
 lint.violations := report
 
-rules_to_run[category][title] if {
+rules_to_run[category] contains title if {
 	some category, title
 	config.merged_config.rules[category][title]
 
@@ -78,7 +78,6 @@ report contains violation if {
 
 	config.for_rule(category, title).level != "ignore"
 	not config.excluded_file(category, title, input.regal.file.name)
-
 	not ignored(violation, ast.ignore_directives)
 }
 
@@ -148,7 +147,7 @@ aggregate_report contains violation if {
 	# for custom rules, we can't assume that the author included
 	# a location in the violation, although they _really_ should
 	file := object.get(violation, ["location", "file"], "")
-	ignore_directives := object.get(input.ignore_directives, file, {})
+	ignore_directives := object.get(input, ["ignore_directives", file], {})
 
 	not ignored(violation, util.keys_to_numbers(ignore_directives))
 }