Improve jinja error line number identification (#3044)

Author: ssbarnea

examples/playbooks/rule-jinja-invalid.yml

@@ -2,8 +2,11 @@
 - name: Fixture
   hosts: localhost
   tasks:
-    - name: Foo # <-- this is valid jinja
-      ansible.builtin.debug:
-        msg: "{{ 'a' b }}" # <-- jinja2[invalid]
+    - name: A block used to check that we do not identify error at correct level
+      block:
+        - name: Foo # <-- this is valid jinja2
+          ansible.builtin.debug:
+            foo: "{{ 1  }}" # <-- jinja2[spacing]
+            msg: "{{ 'a' b }}" # <-- jinja2[invalid]
 # It should be noted that even ansible --syntax-check fails to spot the jinja
 # error above, but ansible will throw a runtime error when running

src/ansiblelint/rules/jinja.py

@@ -64,12 +64,16 @@ def _msg(self, tag: str, value: str, reformatted: str) -> str:
         """Generate error message."""
         return self._tag2msg[tag].format(value=value, reformatted=reformatted)
 
-    # pylint: disable=too-many-branches
+    # pylint: disable=too-many-branches,too-many-locals
     def matchtask(  # noqa: C901
         self, task: dict[str, Any], file: Lintable | None = None
-    ) -> bool | str | MatchError:
+    ) -> list[MatchError]:
+        result = []
         try:
-            for key, v, _ in nested_items_path(task):
+            for key, v, path in nested_items_path(
+                task,
+                ignored_keys=("block", "ansible.builtin.block", "ansible.legacy.block"),
+            ):
                 if isinstance(v, str):
                     try:
                         template(
@@ -122,29 +126,34 @@ def matchtask(  # noqa: C901
                         # AnsibleError: template error while templating string: expected token ':', got '}'. String: {{ {{ '1' }} }}
                         # AnsibleError: template error while templating string: unable to locate collection ansible.netcommon. String: Foo {{ buildset_registry.host | ipwrap }}
                         if not bypass:
-                            return self.create_matcherror(
-                                message=str(exc),
-                                linenumber=task[LINE_NUMBER_KEY],
-                                filename=file,
-                                tag=f"{self.id}[invalid]",
+                            result.append(
+                                self.create_matcherror(
+                                    message=str(exc),
+                                    linenumber=_get_error_line(task, path),
+                                    filename=file,
+                                    tag=f"{self.id}[invalid]",
+                                )
                             )
+                            continue
                     reformatted, details, tag = self.check_whitespace(
                         v, key=key, lintable=file
                     )
                     if reformatted != v:
-                        return self.create_matcherror(
-                            message=self._msg(
-                                tag=tag, value=v, reformatted=reformatted
-                            ),
-                            linenumber=task[LINE_NUMBER_KEY],
-                            details=details,
-                            filename=file,
-                            tag=f"{self.id}[{tag}]",
+                        result.append(
+                            self.create_matcherror(
+                                message=self._msg(
+                                    tag=tag, value=v, reformatted=reformatted
+                                ),
+                                linenumber=_get_error_line(task, path),
+                                details=details,
+                                filename=file,
+                                tag=f"{self.id}[{tag}]",
+                            )
                         )
         except Exception as exc:
             _logger.info("Exception in JinjaRule.matchtask: %s", exc)
             raise
-        return False
+        return result
 
     def matchyaml(self, file: Lintable) -> list[MatchError]:
         """Return matches for variables defined in vars files."""
@@ -344,7 +353,7 @@ def blacken(text: str) -> str:
     @pytest.fixture(name="error_expected_lines")
     def fixture_error_expected_lines() -> list[int]:
         """Return list of expected error lines."""
-        return [31, 34, 37, 40, 43, 46, 72]
+        return [33, 36, 39, 42, 45, 48, 74]
 
     # 21 68
     @pytest.fixture(name="lint_error_lines")
@@ -636,9 +645,13 @@ def test_jinja_invalid() -> None:
         collection.register(JinjaRule())
         success = "examples/playbooks/rule-jinja-invalid.yml"
         errs = Runner(success, rules=collection).run()
-        assert len(errs) == 1
-        assert errs[0].tag == "jinja[invalid]"
+        assert len(errs) == 2
+        assert errs[0].tag == "jinja[spacing]"
         assert errs[0].rule.id == "jinja"
+        assert errs[0].linenumber == 9
+        assert errs[1].tag == "jinja[invalid]"
+        assert errs[1].rule.id == "jinja"
+        assert errs[1].linenumber == 9
 
     def test_jinja_valid() -> None:
         """Tests our ability to parse jinja, even when variables may not be defined."""
@@ -647,3 +660,16 @@ def test_jinja_valid() -> None:
         success = "examples/playbooks/rule-jinja-valid.yml"
         errs = Runner(success, rules=collection).run()
         assert len(errs) == 0
+
+
+def _get_error_line(task: dict[str, Any], path: list[str | int]) -> int:
+    """Return error line number."""
+    line = task[LINE_NUMBER_KEY]
+    ctx = task
+    for _ in path:
+        ctx = ctx[_]
+        if LINE_NUMBER_KEY in ctx:
+            line = ctx[LINE_NUMBER_KEY]
+    if not isinstance(line, int):
+        raise RuntimeError("Line number is not an integer")
+    return line

src/ansiblelint/yaml_utils.py

@@ -14,6 +14,7 @@
     Dict,
     Iterator,
     Pattern,
+    Sequence,
     Tuple,
     Union,
     cast,
@@ -170,6 +171,7 @@ def iter_tasks_in_file(
 
 def nested_items_path(
     data_collection: dict[Any, Any] | list[Any],
+    ignored_keys: Sequence[str] = (),
 ) -> Iterator[tuple[Any, Any, list[str | int]]]:
     """Iterate a nested data structure, yielding key/index, value, and parent_path.
 
@@ -230,12 +232,15 @@ def nested_items_path(
     # valid data, we better ignore NoneType
     if data_collection is None:
         return
-    yield from _nested_items_path(data_collection=data_collection, parent_path=[])
+    yield from _nested_items_path(
+        data_collection=data_collection, parent_path=[], ignored_keys=ignored_keys
+    )
 
 
 def _nested_items_path(
     data_collection: dict[Any, Any] | list[Any],
     parent_path: list[str | int],
+    ignored_keys: Sequence[str] = (),
 ) -> Iterator[tuple[Any, Any, list[str | int]]]:
     """Iterate through data_collection (internal implementation of nested_items_path).
 
@@ -260,7 +265,7 @@ def _nested_items_path(
             f"of type '{type(data_collection)}'"
         )
     for key, value in convert_data_collection_to_tuples():
-        if key in (SKIPPED_RULES_KEY, "__file__", "__line__"):
+        if key in (SKIPPED_RULES_KEY, "__file__", "__line__", *ignored_keys):
             continue
         yield key, value, parent_path
         if isinstance(value, (dict, list)):