Improve logic of find_children (#4161)

Author: ssbarnea

.github/workflows/tox.yml

@@ -72,7 +72,7 @@ jobs:
     env:
       # Number of expected test passes, safety measure for accidental skip of
       # tests. Update value if you add/remove tests.
-      PYTEST_REQPASS: 879
+      PYTEST_REQPASS: 882
     steps:
       - uses: actions/checkout@v4
         with:

examples/playbooks/common-include-1.yml

@@ -8,3 +8,5 @@
     - name: Some include_tasks with file and jinja2
       ansible.builtin.include_tasks:
         file: "{{ 'tasks/included-with-lint.yml' }}"
+    - name: Some include 3
+      ansible.builtin.include_tasks: file=tasks/included-with-lint.yml

examples/playbooks/common-include-wrong-syntax.yml

@@ -0,0 +1,9 @@
+---
+- name: Fixture for test coverage
+  hosts: localhost
+  gather_facts: false
+  tasks:
+    - name: Some include with invalid syntax
+      ansible.builtin.include_tasks: "file="
+    - name: Some include with invalid syntax
+      ansible.builtin.include_tasks: other=tasks/included-with-lint.yml

examples/playbooks/common-include-wrong-syntax2.yml

@@ -0,0 +1,8 @@
+---
+- name: Fixture for test coverage
+  hosts: localhost
+  gather_facts: false
+  tasks:
+    - name: Some include with invalid syntax
+      ansible.builtin.include_tasks:
+        file: null

examples/playbooks/common-include-wrong-syntax3.yml

@@ -0,0 +1,7 @@
+---
+- name: Fixture
+  hosts: localhost
+  tasks:
+    - name: Fixture
+      ansible.builtin.include_role:
+        name: include_wrong_syntax

examples/playbooks/include.yml

@@ -11,6 +11,7 @@
   tasks:
     - ansible.builtin.include_tasks: tasks/x.yml
     - ansible.builtin.include_tasks: tasks/x.yml y=z
+    - ansible.builtin.include_tasks: file=tasks/x.yml
 
   handlers:
     - ansible.builtin.include_tasks: handlers/y.yml

examples/roles/include_wrong_syntax/tasks/main.yml

@@ -0,0 +1,3 @@
+---
+- name: Invalid syntax for import (coverage)
+  ansible.builtin.import_tasks: wrong=imported_tasks.yml

examples/roles/var_naming_pattern/tasks/include_task_with_vars.yml

@@ -1,12 +1,15 @@
 ---
-- name: include_task_with_vars | Foo
+- name: include_task_with_vars | Var1
+  ansible.builtin.include_tasks: file=../tasks/included-task-with-vars.yml
+
+- name: include_task_with_vars | Var2
   ansible.builtin.include_tasks: ../tasks/included-task-with-vars.yml
   vars:
     var_naming_pattern_1: bar
     _var_naming_pattern_2: ... # we allow _ before the prefix
     __var_naming_pattern_3: ... # we allow __ before the prefix
 
-- name: include_task_with_vars | Foo
+- name: include_task_with_vars | Var3
   ansible.builtin.include_role:
     name: bobbins
   vars:

playbook.yml

@@ -1,6 +1,7 @@
 ---
 - name: Example
   hosts: localhost
+  gather_facts: false
   tasks:
     - name: include extra tasks
       ansible.builtin.include_tasks:

src/ansiblelint/rules/role_name.py

@@ -164,7 +164,7 @@ def _infer_role_name(meta: Path, default: str) -> str:
             if meta_data:
                 try:
                     return str(meta_data["galaxy_info"]["role_name"])
-                except KeyError:
+                except (KeyError, TypeError):
                     pass
         return default
 

src/ansiblelint/rules/syntax_check.py

@@ -27,6 +27,13 @@ class KnownError:
             re.MULTILINE | re.S | re.DOTALL,
         ),
     ),
+    KnownError(
+        tag="no-file",
+        regex=re.compile(
+            r"^ERROR! (?P<title>No file specified for [^\n]*)",
+            re.MULTILINE | re.S | re.DOTALL,
+        ),
+    ),
     KnownError(
         tag="empty-playbook",
         regex=re.compile(

src/ansiblelint/runner.py

@@ -455,6 +455,9 @@ def _emit_matches(self, files: list[Lintable]) -> Generator[MatchError, None, No
         visited: set[Lintable] = set()
         while visited != self.lintables:
             for lintable in self.lintables - visited:
+                visited.add(lintable)
+                if not lintable.path.exists():
+                    continue
                 try:
                     children = self.find_children(lintable)
                     for child in children:
@@ -468,8 +471,10 @@ def _emit_matches(self, files: list[Lintable]) -> Generator[MatchError, None, No
                     exc.rule = self.rules["load-failure"]
                     yield exc
                 except AttributeError:
-                    yield MatchError(lintable=lintable, rule=self.rules["load-failure"])
-                visited.add(lintable)
+                    yield MatchError(
+                        lintable=lintable,
+                        rule=self.rules["load-failure"],
+                    )
 
     def find_children(self, lintable: Lintable) -> list[Lintable]:
         """Traverse children of a single file or folder."""
@@ -490,7 +495,6 @@ def find_children(self, lintable: Lintable) -> list[Lintable]:
             except AnsibleError as exc:
                 msg = f"Loading {lintable.filename} caused an {type(exc).__name__} exception: {exc}, file was ignored."
                 logging.exception(msg)
-                # raise SystemExit(exc) from exc
                 return []
         results = []
         # playbook_ds can be an AnsibleUnicode string, which we consider invalid

src/ansiblelint/utils.py

@@ -28,7 +28,7 @@
 import logging
 import os
 import re
-from collections.abc import ItemsView, Iterator, Mapping, Sequence
+from collections.abc import ItemsView, Iterable, Iterator, Mapping, Sequence
 from dataclasses import _MISSING_TYPE, dataclass, field
 from functools import cache, lru_cache
 from pathlib import Path
@@ -290,7 +290,7 @@ class HandleChildren:
     rules: RulesCollection = field(init=True, repr=False)
     app: App
 
-    def include_children(
+    def include_children(  # pylint: disable=too-many-return-statements
         self,
         lintable: Lintable,
         k: str,
@@ -325,14 +325,21 @@ def include_children(
             return []
 
         # handle include: filename.yml tags=blah
-        (args, _) = tokenize(v)
+        (args, kwargs) = tokenize(v)
 
-        result = path_dwim(basedir, args[0])
+        if args:
+            file = args[0]
+        elif "file" in kwargs:
+            file = kwargs["file"]
+        else:
+            return []
+
+        result = path_dwim(basedir, file)
         while basedir not in ["", "/"]:
             if os.path.exists(result):
                 break
             basedir = os.path.dirname(basedir)
-            result = path_dwim(basedir, args[0])
+            result = path_dwim(basedir, file)
 
         return [Lintable(result, kind=parent_type)]
 
@@ -430,7 +437,7 @@ def roles_children(
         # pylint: disable=unused-argument # parent_type)
         basedir = str(lintable.path.parent)
         results: list[Lintable] = []
-        if not v:
+        if not v or not isinstance(v, Iterable):
             # typing does not prevent junk from being passed in
             return results
         for role in v:
@@ -467,10 +474,24 @@ def _get_task_handler_children_for_tasks_or_playbooks(
             if not task_handler or isinstance(task_handler, str):  # pragma: no branch
                 continue
 
-            file_name = task_handler[task_handler_key]
-            if isinstance(file_name, Mapping) and file_name.get("file", None):
-                file_name = file_name["file"]
+            file_name = ""
+            action_args = task_handler[task_handler_key]
+            if isinstance(action_args, str):
+                (args, kwargs) = tokenize(action_args)
+                if len(args) == 1:
+                    file_name = args[0]
+                elif kwargs.get("file", None):
+                    file_name = kwargs["file"]
+                else:
+                    # ignore invalid data (syntax check will outside the scope)
+                    continue
+
+            if isinstance(action_args, Mapping) and action_args.get("file", None):
+                file_name = action_args["file"]
 
+            if not file_name:
+                # ignore invalid data (syntax check will outside the scope)
+                continue
             f = path_dwim(basedir, file_name)
             while basedir not in ["", "/"]:
                 if os.path.exists(f):

test/test_runner.py

@@ -178,6 +178,52 @@ def test_files_not_scanned_twice(default_rules_collection: RulesCollection) -> N
     assert len(run2) == 0
 
 
+@pytest.mark.parametrize(
+    ("filename", "failures", "checked_files_no"),
+    (
+        pytest.param(
+            "examples/playbooks/common-include-wrong-syntax.yml",
+            1,
+            1,
+            id="1",
+        ),
+        pytest.param(
+            "examples/playbooks/common-include-wrong-syntax2.yml",
+            1,
+            1,
+            id="2",
+        ),
+        pytest.param(
+            "examples/playbooks/common-include-wrong-syntax3.yml",
+            0,
+            2,
+            id="3",
+        ),
+    ),
+)
+def test_include_wrong_syntax(
+    filename: str,
+    failures: int,
+    checked_files_no: int,
+    default_rules_collection: RulesCollection,
+) -> None:
+    """Ensure that lintables aren't double-checked."""
+    checked_files: set[Lintable] = set()
+
+    path = Path(filename).resolve()
+    runner = Runner(
+        path,
+        rules=default_rules_collection,
+        verbosity=0,
+        checked_files=checked_files,
+    )
+    result = runner.run()
+    assert len(runner.checked_files) == checked_files_no
+    assert len(result) == failures, result
+    for item in result:
+        assert item.tag == "syntax-check[no-file]"
+
+
 def test_runner_not_found(default_rules_collection: RulesCollection) -> None:
     """Ensure that lintables aren't double-checked."""
     checked_files: set[Lintable] = set()

test/test_yaml_utils.py

@@ -768,12 +768,12 @@ def test_get_path_to_play(
         pytest.param(
             "examples/playbooks/include.yml",
             14,
-            [0, "tasks", 1],
+            [0, "tasks", 2],
             id="playbook-multi_tasks_blocks-tasks_last_task_before_handlers",
         ),
         pytest.param(
             "examples/playbooks/include.yml",
-            16,
+            17,
             [0, "handlers", 0],
             id="playbook-multi_tasks_blocks-handlers_task",
         ),