Allow naming-convention rule to take list of names (not only a pattern) (#1911)

While the title of this PR is what we'll want in the release notes, this is 95% an
internal change meant to improve Regal for ourselves. That a custom rule got better
is mostly a happy coincidence :)

Since we started working on Regal, we have learnt a lot about both OPA, the AST and
of course our own domains — static analysis, linting and language servers. The
terminology used in the project has grown organically along with that, and with time
we've found a shared vocabulary for the project. As a mature project, there's however
been no lack of code written before we knew better, and as this PR highlights there
have been many places where names have been picked rather arbitrarly.

Additionally, the lack of a documented vocabulary meant it wasn't accessible outside
of our own internal know-how.

This change aims to fix that by:

1. Documenting the terminlogy and vocabulary of Regal and our domain
2. Provide the means to help check how well we follow our stated naming conventions
3. Update our Rego code to comply with these naming conventions

While having this goes a long way to unify our terminology, there are things
that won't be caught by a simple "allow list" like this. As an example, our use
of the name `ref` sometimes mean the "outer" type (the ref term), and sometimes
it refers to only the the term slice of the ref (which should rather be called
`terms`). That's out of scope for this PR, but could be something to look into
later.

As noted in the change, we will not enforce anything related to naming conventions,
but occasionally check this and make fixes accordingly (and without ever getting in
the way of anyone contributing to the project).

Fixes #1493

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

Author: anderseknert

.regal/config.yaml

@@ -16,6 +16,101 @@ rules:
       ignore:
         files:
           - "*_test.rego"
+    naming-convention:
+      # Regal naming conventions configuration. Note that this is disabled by default, and
+      # is meant only to be run every once in a while to ensure we stick to our conventions.
+      # These conventions are not static, and the correct "fix" to a naming violation may
+      # very well be to update this configuration rather than the code.
+      #
+      # To check compliance:
+      # regal lint --enable naming-convention bundle
+      #
+      # See naming.md for rationale and examples of these conventions.
+      # NOTE: that this check is not automated, but intended to be used occasionally.
+      level: ignore
+      conventions:
+        - pattern: '\b[a-z_]{5,}\b'
+          names:
+            # indices
+            - i
+            - j
+            - k
+            # count / length
+            - n
+            # rego types
+            - arr
+            - obj
+            - set
+            - str
+            - num
+            # imaginary 'types'
+            - coll
+            - seq
+            - item
+            # ast types
+            - pkg
+            - imp
+            - ref
+            - refs
+            - var
+            - vars
+            - fun
+            - arg
+            - args
+            - term
+            - terms
+            - expr
+            - exprs
+            - rule
+            - head
+            - body
+            - call
+            - comp
+            - node
+            # ast attributes and related
+            - key
+            - keys
+            - val
+            - vals
+            - type
+            # common
+            - file
+            - text
+            - line
+            - rows
+            - cols
+            - loc
+            - end
+            - col
+            - row
+            - agg
+            - aggs
+            - cfg
+            - word
+            - start
+            - link
+            - len
+            - kind
+            - rest
+            - name
+            - pos
+            - sub
+            - sup
+            - last
+            - next
+            - lhs
+            - rhs
+            - path
+            - url
+            - uri
+            - other
+            - diff
+            - dir
+          targets:
+            - var
+      ignore:
+        files:
+          - "*_test.rego"
     narrow-argument:
       level: error
       exclude-args:

bundle/regal/ast/ast.rego

@@ -102,9 +102,9 @@ rules := [rule |
 # description: all the test rules in the input AST
 tests := [rule |
 	some rule in rules
+	some term in rule.head.ref
 
-	some part in rule.head.ref
-	startswith(part.value, "test_")
+	startswith(term.value, "test_")
 ]
 
 # METADATA
@@ -212,59 +212,59 @@ function_calls[rule_index] contains call if {
 
 # METADATA
 # description: |
-#   true if both ref values (or "paths") are equal in type
+#   true if both ref values (terms) are equal in type
 #   and value for each path component, ignoring locations
-ref_value_equal(v1, v2) if {
-	count(v1) == count(v2)
+ref_value_equal(terms, other) if {
+	count(terms) == count(other)
 
-	every i, term in v1 {
-		term.type == v2[i].type
-		term.value == v2[i].value
+	every i, term in terms {
+		term.type == other[i].type
+		term.value == other[i].value
 	}
 }
 
 # METADATA
 # description: |
-#   returns a new ref value made by extending terms1 with terms2,
-#   where the first term of terms2 transformed to string
-extend_ref_terms(terms1, terms2) := array.flatten([
-	terms1,
-	object.union(terms2[0], {"type": "string"}),
-	array.slice(terms2, 1, 100),
+#   returns a new terms array made by extending terms with other,
+#   where the first term of other transformed to string
+extend_ref_terms(terms, other) := array.flatten([
+	terms,
+	object.union(other[0], {"type": "string"}),
+	array.slice(other, 1, 100),
 ])
 
 # METADATA
 # description: |
-#   true if all terms in `terms1` are also present in `terms2`
+#   true if all terms in `terms` are also present in `other`
 #   regardless of length, and ignoring locations
-is_terms_subset(terms1, terms2) if {
-	count(terms1) <= count(terms2)
+is_terms_subset(terms, other) if {
+	count(terms) <= count(other)
 
-	every i, term in terms1 {
-		term.type == terms2[i].type
-		term.value == terms2[i].value
+	every i, term in terms {
+		term.type == other[i].type
+		term.value == other[i].value
 	}
 }
 
 # METADATA
 # description: returns the "path" string of any given ref value
-ref_to_string(ref) := concat("", array.flatten([ref[0].value, [_format_part(part) |
-	some part in array.slice(ref, 1, 100)
+ref_to_string(ref) := concat("", array.flatten([ref[0].value, [_format_term(term) |
+	some term in array.slice(ref, 1, 100)
 
-	not part.type in {"call", "ref", "templatestring"}
+	not term.type in {"call", "ref", "templatestring"}
 ]]))
 
-_format_part(part) := concat("", [".", part.value]) if {
-	part.type == "string"
-	regex.match(`^[a-zA-Z_][a-zA-Z1-9_]*$`, part.value)
+_format_term(term) := concat("", [".", term.value]) if {
+	term.type == "string"
+	regex.match(`^[a-zA-Z_][a-zA-Z1-9_]*$`, term.value)
 } else := sprintf(
 	{
 		"string": `["%v"]`,
 		"var": `[%v]`,
 		"number": `[%d]`,
 		"boolean": `[%v]`,
-	}[part.type],
-	[part.value],
+	}[term.type],
+	[term.value],
 )
 
 # METADATA
@@ -275,9 +275,9 @@ _format_part(part) := concat("", [".", part.value]) if {
 #   foo.bar[baz] -> foo.bar
 ref_static_to_string(ref) := ref_to_string(array.slice(ref, 0, first_non_static)) if {
 	first_non_static := [i |
-		some i, part in ref
+		some i, term in ref
 		i > 0
-		part.type in {"call", "var", "ref", "templatestring"}
+		term.type in {"call", "var", "ref", "templatestring"}
 	][0]
 } else := ref_to_string(ref)
 
@@ -369,11 +369,11 @@ is_chained_rule_body(rule, lines) if {
 # description: answers whether variable of `name` is found anywhere in provided rule `head`
 # scope: document
 var_in_head(head, name) if {
-	some part in ["key", "value"]
-	has_named_var(head[part], name)
+	some type in ["key", "value"]
+	has_named_var(head[type], name)
 } else if {
-	some part in array.slice(head.ref, 1, 100)
-	has_named_var(part, name)
+	some term in array.slice(head.ref, 1, 100)
+	has_named_var(term, name)
 }
 
 # METADATA

bundle/regal/ast/comments.rego

@@ -80,23 +80,23 @@ comment_blocks(comments_decoded) := blocks if {
 			some comment in row_partition
 
 			col := comment.location.col # regal ignore:comprehension-term-assignment
-			partition := [c |
-				some c in row_partition
-				c.location.col == col
+			partition := [partition_comment |
+				some partition_comment in row_partition
+				partition_comment.location.col == col
 			]
 		}
 	]
 }
 
-_splits(xs) := array.flatten([
+_splits(rows) := array.flatten([
 	# -1 ++ [ all indices where there's a step larger than one ] ++ length of xs
 	# the -1 is because we're adding +1 in array.slice
 	-1,
 	[i |
 		some i in numbers.range(0, n - 1)
-		xs[i + 1] != xs[i] + 1
+		rows[i + 1] != rows[i] + 1
 	],
 	n,
 ]) if {
-	n := count(xs)
+	n := count(rows)
 }

bundle/regal/ast/imports.rego

@@ -46,12 +46,13 @@ resolved_imports[identifier] := paths[0] if {
 #   returns true if provided path (like ["data", "foo", "bar"]) is in the
 #   list of imports (which is commonly ast.imports)
 imports_has_path(imports, path) if {
-	pv := imports[_].path.value
+	n := count(path)
+	terms := imports[_].path.value
 
-	count(pv) == count(path)
+	count(terms) == n
 
-	every i, part in path {
-		part == pv[i].value
+	every i, term in path {
+		term == terms[i].value
 	}
 }
 
@@ -62,11 +63,7 @@ imports_has_path(imports, path) if {
 imports_keyword(imports, keyword) if {
 	capabilities.is_opa_v1
 	input.regal.file.rego_version != "v0"
-} else if {
-	pv := imports[_].path.value
-
-	_has_keyword([p.value | some p in pv], keyword)
-}
+} else if _has_keyword([term.value | term := imports[_].path.value[_]], keyword)
 
 _imported_identifier(imp) := imp.alias
 _imported_identifier(imp) := regal.last(imp.path.value).value if not imp.alias

bundle/regal/ast/keywords.rego

@@ -9,19 +9,19 @@ import data.regal.util
 # METADATA
 # description: collects the `if` keyword. this isn't present in the AST, so we'll simply scan the input lines
 keywords[row] contains keyword if {
-	some idx, line in input.regal.file.lines
+	some i, line in input.regal.file.lines
 
 	col := indexof(line, " if ")
 	col > 0
 
-	row := idx + 1
+	row := i + 1
 
 	not row in _comment_row_index
 
 	keyword := {
 		"name": "if",
 		"location": {
-			"row": idx + 1,
+			"row": i + 1,
 			"col": col + 2,
 		},
 	}

bundle/regal/ast/search.rego

@@ -56,8 +56,8 @@ has_term_var(terms) if {
 
 # converting to string until https://github.com/open-policy-agent/opa/issues/6736 is fixed
 _rule_index(rule) := rule_index_strings[i] if {
-	some i, r in _rules
-	r == rule
+	some i
+	rule == _rules[i]
 }
 
 # hack to work around the different input models of linting vs. the lsp package.. we
@@ -119,9 +119,9 @@ found.vars[rule_index].ref contains term if {
 	some rule_index, ref
 	found.refs[rule_index][ref]
 
-	some x, term in ref.value
+	some i, term in ref.value
 
-	x > 0
+	i > 0
 	term.type == "var"
 }
 
@@ -165,9 +165,9 @@ found.vars[rule_index].somein contains var if {
 
 	arr := value[0].value
 
-	some var in array.flatten([_find_nested_vars(arr[1]), [v |
+	some var in array.flatten([_find_nested_vars(arr[1]), [var |
 		count(arr) == 4
-		some v in _find_nested_vars(arr[2])
+		some var in _find_nested_vars(arr[2])
 	]])
 }
 

bundle/regal/config/exclusion.rego

@@ -16,10 +16,10 @@ excluded_file(category, title, file) if {
 # description: |
 #   pattern_compiler transforms a glob pattern into a set of glob
 #   patterns to make the combined set behave as .gitignore
-patterns_compiler(patterns) := {pat |
+patterns_compiler(patterns) := {compiled |
 	some pattern in patterns
-	some p in _leading_doublestar_pattern(_internal_slashes(pattern))
-	some pat in _trailing_slash(p)
+	some processed in _leading_doublestar_pattern(_internal_slashes(pattern))
+	some compiled in _trailing_slash(processed)
 } if {
 	patterns != []
 }