fix: report a chain's errors top-down instead of deepest first

Check the top module's structure before the requires walk, record
required modules during the walk, then validate them top-down after the
top module. This is the order codeplain reports errors in, so a broken
chain now gives the same first error from `plain-parser check` and from
codeplain's `--dry-run`. Each module is still read once.

Author: bananaplain

src/plain_parser/plain_file.py

@@ -709,19 +709,31 @@ def parse_plain_file(
     )
 
 
+@dataclass
+class RequiredModule:
+    """A required module as parsed by the requires walk; validated once the top module is complete."""
+
+    parse_result: PlainFileParseResult
+    # Definitions exported to this module by the modules it requires.
+    ancestor_exported_definitions: list[mistletoe.block_token.token]
+    code_variables: dict
+    # Definitions this module exports to the modules that require it.
+    exported_definitions: list[mistletoe.block_token.token]
+
+
 def process_required_modules(
     required_modules: list[str],
     template_dirs: list[str],
     all_required_modules: list[str],
     modules_trace: list[str],
-    chain: list[tuple[str, dict]],
-    module_exports: dict[str, list[mistletoe.block_token.token]],
+    walk_order: list[str],
+    required: dict[str, RequiredModule],
 ) -> list[mistletoe.block_token.token]:
-    """Parse and fully validate every required module, deepest first.
+    """Parse every required module, deepest first, without validating it.
 
-    Appends ``(module name, marshalled plain source tree)`` to ``chain`` for each module not already in it,
-    records each module's exported definitions in ``module_exports``, and returns the exported definitions
-    of the directly required modules.
+    Appends each module name to ``walk_order`` the first time it is reached (deepest ancestors first),
+    records its parse result in ``required``, and returns the exported definitions of the directly
+    required modules.
     """
     exported_definitions = list[mistletoe.block_token.token]()
     for module_name in required_modules:
@@ -730,7 +742,7 @@ def process_required_modules(
 
         if len(all_required_modules) > 0 and module_name == all_required_modules[-1]:
             # Already processed as an earlier entry; its exports are still visible to this module.
-            exported_definitions.extend(module_exports[module_name])
+            exported_definitions.extend(required[module_name].exported_definitions)
             continue
 
         code_variables: dict = {}
@@ -753,8 +765,8 @@ def process_required_modules(
                 template_dirs,
                 all_required_modules,
                 modules_trace + [module_name],
-                chain,
-                module_exports,
+                walk_order,
+                required,
             )
 
         own_exported_definitions = list[mistletoe.block_token.token]()
@@ -770,20 +782,37 @@ def process_required_modules(
                             exported_concept, plain_file_parse_result.plain_source, renderer
                         )
                     )
-        module_exports[module_name] = own_exported_definitions
+        required[module_name] = RequiredModule(
+            plain_file_parse_result, ancestor_exported_definitions, code_variables, own_exported_definitions
+        )
         exported_definitions.extend(own_exported_definitions)
 
-        marshalled_plain_source = validate_and_marshall_module(
-            plain_file_parse_result, module_name, ancestor_exported_definitions, code_variables, template_dirs
-        )
-        if module_name not in (chain_module_name for chain_module_name, _ in chain):
-            chain.append((module_name, marshalled_plain_source))
+        if module_name not in walk_order:
+            walk_order.append(module_name)
 
         all_required_modules.append(module_name)
 
     return exported_definitions
 
 
+def validate_required_modules(
+    required_modules: list[str],
+    required: dict[str, RequiredModule],
+    template_dirs: list[str],
+    trees: dict[str, dict],
+) -> None:
+    """Validate recorded required modules as codeplain does: top-down, in listed order, each module once."""
+    for module_name in required_modules:
+        if module_name in trees:
+            continue
+
+        module = required[module_name]
+        trees[module_name] = validate_and_marshall_module(
+            module.parse_result, module_name, module.ancestor_exported_definitions, module.code_variables, template_dirs
+        )
+        validate_required_modules(module.parse_result.required_modules, required, template_dirs, trees)
+
+
 def process_exported_definitions(plain_source: dict, exported_definitions: list[mistletoe.block_token.token]) -> None:
     if len(exported_definitions) == 0:
         return
@@ -908,20 +937,29 @@ def _parse_module(
         modules_trace=[],
     )
 
-    chain: list[tuple[str, dict]] = []
+    # The order below is codeplain's: the top module's structure, the requires walk, the top module's
+    # remaining checks, then every required module top-down.
+    validate_module_structure(plain_file_parse_result, module_name)
+
+    walk_order: list[str] = []
+    required: dict[str, RequiredModule] = {}
     exported_definitions = process_required_modules(
         plain_file_parse_result.required_modules,
         template_dirs=template_dirs,
         all_required_modules=[],
         modules_trace=[],
-        chain=chain,
-        module_exports={},
+        walk_order=walk_order,
+        required=required,
     )
 
-    marshalled_plain_source = validate_and_marshall_module(
+    marshalled_plain_source = validate_and_build_module_tree(
         plain_file_parse_result, module_name, exported_definitions, code_variables, template_dirs
     )
 
+    trees: dict[str, dict] = {}
+    validate_required_modules(plain_file_parse_result.required_modules, required, template_dirs, trees)
+    chain = [(chain_module_name, trees[chain_module_name]) for chain_module_name in walk_order]
+
     return module_name, marshalled_plain_source, plain_file_parse_result.required_modules, chain
 
 

tests/test_requires.py

@@ -1,7 +1,7 @@
 import pytest
 
 from plain_parser import plain_file
-from plain_parser.exceptions import ModuleDoesNotExistError, PlainSyntaxError
+from plain_parser.exceptions import MissingFunctionalitiesError, ModuleDoesNotExistError, PlainSyntaxError
 
 
 def test_non_existent_require(get_test_data_path):
@@ -154,6 +154,14 @@ def write_modules_using_required_exports(directory, requires_by_module):
         marks=pytest.mark.xfail(strict=True, reason="an ancestor must be listed before its dependent"),
     ),
     #   Top
+    #    ||     Top lists Base twice
+    #   Base
+    pytest.param(
+        {"Top": ["Base", "Base"], "Base": []},
+        ["Base", "Top"],
+        id="top-lists-base-twice",
+    ),
+    #   Top
     #    |
     #   Upper    every module also requires every module below it
     #    |
@@ -267,3 +275,62 @@ def test_requires_independent_branches_are_rejected(tmp_path, requires_by_module
 
     with pytest.raises(PlainSyntaxError, match="There must be a fixed order how required modules are dependent"):
         plain_file.parse_module_chain("Top.plain", [str(tmp_path)])
+
+
+# Two broken modules in one chain: the first error reported is the one codeplain reports.
+def test_top_module_shape_is_checked_before_its_requires_are_read(tmp_path):
+    (tmp_path / "Top.plain").write_text("---\nrequires:\n  - Missing\n---\n\n***implementation reqs***\n\n- Req.\n")
+
+    with pytest.raises(MissingFunctionalitiesError):
+        plain_file.plain_file_parser("Top.plain", [str(tmp_path)])
+
+
+def test_top_module_shape_is_checked_before_required_modules_are_validated(tmp_path):
+    write_module(tmp_path, "Base", [], [], [], "Base uses :BaseMissing:.")
+    (tmp_path / "Top.plain").write_text("---\nrequires:\n  - Base\n---\n\n***implementation reqs***\n\n- Req.\n")
+
+    with pytest.raises(MissingFunctionalitiesError):
+        plain_file.plain_file_parser("Top.plain", [str(tmp_path)])
+
+
+def test_parse_errors_of_required_modules_come_before_top_module_concept_errors(tmp_path):
+    (tmp_path / "Base.plain").write_text(
+        "***implementation reqs***\n\n- {% if %}\n\n***functional specs***\n\n- Base works.\n"
+    )
+    write_module(tmp_path, "Top", [], ["Base"], [], "Top uses :TopMissing:.")
+
+    with pytest.raises(PlainSyntaxError, match="Invalid Liquid"):
+        plain_file.plain_file_parser("Top.plain", [str(tmp_path)])
+
+
+def test_top_module_concept_errors_come_before_required_module_concept_errors(tmp_path):
+    write_module(tmp_path, "Base", [], [], [], "Base uses :BaseMissing:.")
+    write_module(tmp_path, "Top", [], ["Base"], [], "Top uses :TopMissing:.")
+
+    with pytest.raises(PlainSyntaxError, match="Concept :TopMissing: is not defined"):
+        plain_file.plain_file_parser("Top.plain", [str(tmp_path)])
+
+
+def test_required_modules_are_validated_top_down(tmp_path):
+    write_module(tmp_path, "Base", [], [], [], "Base uses :BaseMissing:.")
+    write_module(tmp_path, "Middle", [], ["Base"], [], "Middle uses :MiddleMissing:.")
+    write_module(tmp_path, "Top", [], ["Middle"], [], "Top works.")
+
+    with pytest.raises(PlainSyntaxError, match="Concept :MiddleMissing: is not defined"):
+        plain_file.plain_file_parser("Top.plain", [str(tmp_path)])
+
+
+def test_module_listed_twice_around_its_dependent_is_rejected(tmp_path):
+    write_modules_using_required_exports(tmp_path, {"Top": ["Base", "Middle", "Base"], "Middle": ["Base"], "Base": []})
+
+    with pytest.raises(PlainSyntaxError, match="There must be a fixed order how required modules are dependent"):
+        plain_file.parse_module_chain("Top.plain", [str(tmp_path)])
+
+
+def test_chain_has_a_tree_for_every_module_reached_by_the_walk(tmp_path):
+    write_modules_using_required_exports(tmp_path, {"Top": ["Base", "Middle"], "Middle": ["Base"], "Base": []})
+
+    chain = plain_file.parse_module_chain("Top.plain", [str(tmp_path)])
+
+    assert [module_name for module_name, _ in chain] == ["Base", "Middle", "Top"]
+    assert all("functional specs" in tree for _, tree in chain)