Refactor no-loop-var-prefix rule (#2470)

* docs lint rule no-loop-var

* chore: auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Refactor no-loop-var-prefix

- rename rule to loop-var-prefix and keep alias for old name
- make rule produce more detailed messages
- add documentation to the rule

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Sorin Sbarnea <[email protected]>

Author: 3 people

examples/roles/fail_loop_var_prefix/tasks/main.yml

@@ -1,6 +1,6 @@
 ---
-# 5 expected no-loop-var-prefix failures at 3, 9, 19, 26, 33
-- name: That should trigger no-loop-var-prefix
+# 5 expected loop-var-prefix failures at 3, 9, 19, 26, 33
+- name: That should trigger loop-var-prefix
   ansible.builtin.debug:
     var: item
   loop:

examples/roles/loop_var_prefix/tasks/fail.yml

@@ -1,5 +1,5 @@
 ---
-# 0 expected no-loop-var-prefix failures
+# 0 expected loop-var-prefix failures
 - name: That should pass
   ansible.builtin.debug:
     var: loop_var_prefix_item

examples/roles/loop_var_prefix/tasks/pass.yml

@@ -102,6 +102,7 @@ def main():
     "git-latest": "latest[git]",
     "hg-latest": "latest[hg]",
     "no-jinja-nesting": "jinja[invalid]",
+    "no-loop-var-prefix": "loop-var-prefix",
 }
 
 PLAYBOOK_TASK_KEYWORDS = [

src/ansiblelint/constants.py

@@ -0,0 +1,58 @@
+# loop-var-prefix
+
+This rule avoids conflicts with nested looping tasks by configuring a variable prefix with `loop_var`.
+Ansible sets `item` as the loop variable.
+You can use `loop_var` to specify a prefix for loop variables and ensure they are unique to each task.
+
+This rule can produce the following messages:
+
+- `[loop-var-prefix[missing]` - Replace unsafe implicit `item` loop variable by adding `loop_var: <loop_var_prefix>...`.
+- `[loop-var-prefix[wrong]` - Loop variable should start with <loop_var_prefix>
+
+This is an opt-in rule.
+You must enable it in your Ansible-lint configuration as follows:
+
+```yaml
+enable_list:
+  - loop-var-prefix
+```
+
+## Problematic Code
+
+```yaml
+---
+- name: Example playbook
+  hosts: localhost
+  tasks:
+    - name: Does not set a prefix for loop variables.
+      ansible.builtin.debug:
+        var: item
+      loop:
+        - foo
+        - bar # <- These items do not have a unique prefix.
+    - name: Sets
+      ansible.builtin.debug:
+        var: zz_item
+      loop:
+        - foo
+        - bar
+      loop_control:
+        loop_var: zz_item # <- This prefix is not unique.
+```
+
+## Correct Code
+
+```yaml
+---
+- name: Example playbook
+  hosts: localhost
+  tasks:
+    - name: Sets a unique prefix for loop variables.
+      ansible.builtin.debug:
+        var: zz_item
+      loop:
+        - foo
+        - bar
+      loop_control:
+        loop_var: my_prefix # <- Specifies a unique prefix for loop variables.
+```

src/ansiblelint/rules/loop_var_prefix.md

@@ -5,6 +5,7 @@
 from typing import TYPE_CHECKING, Any
 
 from ansiblelint.config import options
+from ansiblelint.errors import MatchError
 from ansiblelint.rules import AnsibleLintRule
 from ansiblelint.text import toidentifier
 
@@ -15,7 +16,7 @@
 class RoleLoopVarPrefix(AnsibleLintRule):
     """Role loop_var should use configured prefix."""
 
-    id = "no-loop-var-prefix"
+    id = "loop-var-prefix"
     link = (
         "https://docs.ansible.com/ansible/latest/user_guide/"
         "playbooks_loops.html#defining-inner-and-outer-variable-names-with-loop-var"
@@ -30,12 +31,13 @@ class RoleLoopVarPrefix(AnsibleLintRule):
 
     def matchtask(
         self, task: dict[str, Any], file: Lintable | None = None
-    ) -> bool | str:
+    ) -> list[MatchError]:
         """Return matches for a task."""
         if not file or not file.role or not options.loop_var_prefix:
-            return False
+            return []
 
         self.prefix = options.loop_var_prefix.format(role=toidentifier(file.role))
+        # self.prefix becomes `loop_var_prefix_` because role name is loop_var_prefix
 
         has_loop = "loop" in task
         for key in task.keys():
@@ -46,10 +48,25 @@ def matchtask(
             loop_control = task.get("loop_control", {})
             loop_var = loop_control.get("loop_var", "")
 
-            if not loop_var or not loop_var.startswith(self.prefix):
-                return True
-
-        return False
+            if loop_var:
+                if not loop_var.startswith(self.prefix):
+                    return [
+                        self.create_matcherror(
+                            message=f"Loop variable should start with {self.prefix}",
+                            filename=file,
+                            tag="loop-var-prefix[wrong]",
+                        )
+                    ]
+            else:
+                return [
+                    self.create_matcherror(
+                        message=f"Replace unsafe implicit `item` loop variable by adding `loop_var: {self.prefix}...`.",
+                        filename=file,
+                        tag="loop-var-prefix[missing]",
+                    )
+                ]
+
+        return []
 
 
 # testing code to be loaded only with pytest or when executed the rule file
@@ -71,13 +88,13 @@ def matchtask(
             ),
         ),
     )
-    def test_no_loop_var_prefix(
+    def test_loop_var_prefix(
         default_rules_collection: RulesCollection, test_file: str, failures: int
     ) -> None:
         """Test rule matches."""
         # Enable checking of loop variable prefixes in roles
         options.loop_var_prefix = "{role}_"
         results = Runner(test_file, rules=default_rules_collection).run()
-        assert len(results) == failures
         for result in results:
-            assert result.message == RoleLoopVarPrefix().shortdesc
+            assert result.rule.id == RoleLoopVarPrefix().id
+        assert len(results) == failures

src/ansiblelint/rules/no_loop_var_prefix.py renamed to src/ansiblelint/rules/loop_var_prefix.py

@@ -112,7 +112,7 @@
           "no-handler",
           "no-jinja-when",
           "no-log-password",
-          "no-loop-var-prefix",
+          "loop-var-prefix",
           "no-prompting",
           "no-relative-paths",
           "no-same-owner",