fix: validate top module first

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]
+    # Marshalled plain source tree, set once the module is validated.
+    tree: dict | None = None
+
+
 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]],
+    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.
+    Records each module in ``required`` the first time it is reached (deepest ancestors first) 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,7 @@ def process_required_modules(
                 template_dirs,
                 all_required_modules,
                 modules_trace + [module_name],
-                chain,
-                module_exports,
+                required,
             )
 
         own_exported_definitions = list[mistletoe.block_token.token]()
@@ -770,20 +781,32 @@ def process_required_modules(
                             exported_concept, plain_file_parse_result.plain_source, renderer
                         )
                     )
-        module_exports[module_name] = 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
+        required[module_name] = RequiredModule(
+            plain_file_parse_result, ancestor_exported_definitions, code_variables, own_exported_definitions
         )
-        if module_name not in (chain_module_name for chain_module_name, _ in chain):
-            chain.append((module_name, marshalled_plain_source))
+        exported_definitions.extend(own_exported_definitions)
 
         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]
+) -> None:
+    """Validate recorded required modules top-down, in listed order, each module once."""
+    for module_name in required_modules:
+        module = required[module_name]
+        if module.tree is not None:
+            continue
+
+        validate_module_structure(module.parse_result, module_name)
+        module.tree = validate_and_build_module_tree(
+            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)
+
+
 def process_exported_definitions(plain_source: dict, exported_definitions: list[mistletoe.block_token.token]) -> None:
     if len(exported_definitions) == 0:
         return
@@ -806,14 +829,8 @@ def process_exported_definitions(plain_source: dict, exported_definitions: list[
                 plain_source[plain_spec.DEFINITIONS].children.append(exported_definition)
 
 
-def validate_and_marshall_module(
-    plain_file_parse_result: PlainFileParseResult,
-    module_name: str,
-    exported_definitions: list[mistletoe.block_token.token],
-    code_variables: dict,
-    template_dirs: list[str],
-) -> dict:
-    """Every check a module must pass after parsing, and its marshalled plain source tree."""
+def validate_module_structure(plain_file_parse_result: PlainFileParseResult, module_name: str) -> None:
+    """Required concepts defined, functional specs present and non-empty, implementation reqs present."""
     if len(plain_file_parse_result.required_concepts) > 0:
         missing_required_concepts_msg = "Missing required concepts: "
         missing_required_concepts_msg += ", ".join(plain_file_parse_result.required_concepts)
@@ -841,6 +858,16 @@ def validate_and_marshall_module(
         has_requires=bool(plain_file_parse_result.required_modules),
     )
 
+
+def validate_and_build_module_tree(
+    plain_file_parse_result: PlainFileParseResult,
+    module_name: str,
+    exported_definitions: list[mistletoe.block_token.token],
+    code_variables: dict,
+    template_dirs: list[str],
+) -> dict:
+    """Merge exported definitions, validate acceptance tests, linked resources and concepts, and return the
+    marshalled plain source tree."""
     process_exported_definitions(plain_file_parse_result.plain_source, exported_definitions)
 
     process_acceptance_tests(plain_file_parse_result.plain_source)
@@ -890,20 +917,27 @@ def _parse_module(
         modules_trace=[],
     )
 
-    chain: list[tuple[str, dict]] = []
+    # Check the top module's structure, walk its requires, finish the top module's checks, then validate
+    # every required module top-down.
+    validate_module_structure(plain_file_parse_result, module_name)
+
+    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={},
+        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
     )
 
+    validate_required_modules(plain_file_parse_result.required_modules, required, template_dirs)
+    # The walk records modules deepest first, which is the chain order.
+    chain = [(chain_module_name, module.tree) for chain_module_name, module in required.items()]
+
     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):
@@ -61,6 +61,7 @@ def test_plain_file_parser_validates_required_modules(get_test_data_path):
 
 
 def write_module(directory, module_name, exported_concepts, required_modules, definitions, functional_spec):
+    # functional_spec=None writes a module without a ***functional specs*** section.
     exports = "".join(f"  - {concept}\n" for concept in exported_concepts)
     requires = "".join(f"  - {required_module}\n" for required_module in required_modules)
     (directory / f"{module_name}.plain").write_text(
@@ -70,8 +71,8 @@ def write_module(directory, module_name, exported_concepts, required_modules, de
         + "---\n\n"
         "***definitions***\n\n"
         + "".join(f"- {definition}\n\n" for definition in definitions)
-        + "***implementation reqs***\n\n- An implementation requirement.\n\n"
-        f"***functional specs***\n\n- {functional_spec}\n"
+        + "***implementation reqs***\n\n- An implementation requirement.\n"
+        + (f"\n***functional specs***\n\n- {functional_spec}\n" if functional_spec is not None else "")
     )
 
 
@@ -154,6 +155,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 +276,95 @@ 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: each test pins which error is reported first.
+def test_top_module_shape_is_checked_before_its_requires_are_read(tmp_path):
+    write_module(tmp_path, "Top", [], ["Missing"], [], None)
+
+    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:.")
+    write_module(tmp_path, "Top", [], ["Base"], [], None)
+
+    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)
+
+
+def test_required_module_without_functional_specs_is_rejected(tmp_path):
+    write_module(tmp_path, "Base", [], [], [], None)
+    write_module(tmp_path, "Top", [], ["Base"], [], "Top works.")
+
+    with pytest.raises(MissingFunctionalitiesError, match="Module 'Base'"):
+        plain_file.plain_file_parser("Top.plain", [str(tmp_path)])
+
+
+def test_top_module_concept_errors_come_before_required_module_structure_errors(tmp_path):
+    write_module(tmp_path, "Base", [], [], [], None)
+    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_each_required_module_is_validated_once_top_down(tmp_path, monkeypatch):
+    write_modules_using_required_exports(tmp_path, {"Top": ["Base", "Middle"], "Middle": ["Base"], "Base": []})
+    # A second validation pass over a module would be idempotent, so only the call count can observe it.
+    built = []
+    build_tree = plain_file.validate_and_build_module_tree
+
+    def record_and_build(parse_result, module_name, *args):
+        built.append(module_name)
+        return build_tree(parse_result, module_name, *args)
+
+    monkeypatch.setattr(plain_file, "validate_and_build_module_tree", record_and_build)
+
+    plain_file.parse_module_chain("Top.plain", [str(tmp_path)])
+
+    assert built == ["Top", "Base", "Middle"]