diff --git a/.github/workflows/code-quality.yml b/.github/workflows/code-quality.yml index bba99454..bb2e0c3b 100644 --- a/.github/workflows/code-quality.yml +++ b/.github/workflows/code-quality.yml @@ -37,6 +37,8 @@ jobs: run: uv run pyright ./src ./test ./tools ./features - name: Check ruff and pyright issue budgets run: uv run python tools/lint_budget.py --check + - name: Check layer boundaries + run: uv run python tools/check_layering.py --check - name: Check spelling with codespell run: uv run codespell ./src ./test ./tools ./features --ignore-words-list=NotIn,ANC - name: Test with pytest and collect coverage diff --git a/IMPROVEMENT-PLAN.md b/IMPROVEMENT-PLAN.md new file mode 100644 index 00000000..8f394b25 --- /dev/null +++ b/IMPROVEMENT-PLAN.md @@ -0,0 +1,190 @@ +# Renaissance.Py improvement plan + +A dependency-ordered roadmap for making Renaissance.Py the best platform for multi-language analysis and transformation of software. + +This document is a proposal for discussion. It reorders the TODO list in [README.md](README.md) by dependency rather than by appetite, +and relates it to [issue 110](https://github.com/TNO/Renaissance.Py/issues/110) and [issue 187](https://github.com/TNO/Renaissance.Py/issues/187). + +## The core observation + +Most items on the TODO list are not independent. They all bottom out in three model defects. + +1. **A node has no identity as a piece of text.** + `TextSegment` exists in `src/renaissance/syntax_tree/text_segment.py`, but `NodeProtocol` does not extend it, + and `ASTNode` carries its own mutable `_offset` and `_filename`. + This is why the rewriter and the parser have different representations of the code, + why character-based rewriting is bolted on, and why `reportUnknown*` dominates the lint budget + (roughly 4000 of the tracked Pyright issues). +2. **A pattern is not a type.** + A pattern is an `ASTNode` whose `$`-names are discovered by regular expression in `src/renaissance/utils/ast_utils.py`. + This is why placeholders have no semantic kind, why binding consistency is an ad-hoc check inside `_resolve_match_one`, + and why the matcher cannot *report* ambiguity. +3. **A recipe is not a value.** + `RecipeASTProcessor` uses decorators (`@recipe_step`, `@after_step`) on a class. + This is why there is no `RepeatUntilFixedPoint(recipe)`, no recursive find-and-replace, + and no recipe-composes-recipe as required by issue 187. + +Fixing these three turns roughly 70% of the TODO list into consequences rather than separate projects. + +## Dependency graph + +```mermaid +graph TD + P0["P0: Guardrails
import-layering tool + per-package budgets + type-hint ADR"] + P1["P1: Structure
core / parser_bindings / recipes"] + P2["P2: ASTNode IS-A TextSegment
read-only NodeProtocol"] + P3["P3: Pattern as a first-class type
typed placeholders + semantic kind"] + P4["P4: Matcher correctness
binding consistency + ambiguity reporting"] + P5["P5: Unified rewriter
AST + character, one representation"] + P6["P6: Recipe algebra
composition + fixpoint"] + P7["P7: Semantic services
variables_read/written, has_side_effects"] + P0 --> P1 --> P2 + P2 --> P3 --> P4 + P2 --> P5 + P4 --> P6 + P5 --> P6 + P3 --> P7 + P6 --> P7 +``` + +## Phase 0 - Make quality enforceable before moving anything + +Small, pure wins. These come first because every later phase is validated by them. + +- **Import-layering guard rail**, the "core is independent of all other code" tool. + Write it now, against the current layout, as a test in `test/tools`. + It immediately flags the two real violations: the late `cpp_utils` import in `src/renaissance/syntax_tree/__init__.py` + and the `CPPPatternFactory` `TYPE_CHECKING` import in `src/renaissance/syntax_tree/ast_refactor_actions.py`. + Those two violations become the acceptance criterion for phase 1. +- **Split `lint-budget.json` per package.** + A single global budget means new `core/` code inherits a 9200-issue allowance. + Per-package budgets allow declaring `core/` strict-clean with budget 0, while `parser_bindings/` burns down slowly. + This is the highest-leverage change for the Ruff and Pyright goal; without it the counts will not converge. +- **Settle the `X | Sequence[X]` question as an ADR, not as a code change.** + Recommendation: accept only `Sequence[X]` in core APIs. + The union doubles the type checker's work at every call site, which is a large slice of the `reportUnknownArgumentType` count, + and ergonomics are recoverable with a single `as_sequence()` normalizer at the boundary. + Decide this before phase 2, because phase 2 touches every signature. + +## Phase 1 - Structure, as pure moves + +Adopt the issue 187 layout exactly, but mechanically, with zero semantic changes, so that reviewers can verify it is a rename. + +```text +src/renaissance/core/ # was syntax_tree/ + common/ + utils/ +src/renaissance/parser_bindings/ # was integrations/ +src/renaissance/recipes/ +test/unit/ test/feature/ test/e2e/ # kind at top level, src structure beneath +``` + +Two judgement calls on the open questions in issue 110. + +- **`features/` moves under `test/`.** + On the "kind-first versus structure-first" question: kind-first, as preferred in the issue. + End-to-end tests genuinely have no `src` counterpart, and this keeps `pytest test/unit` fast. +- **`src/rejuvenation/` is not a package, it is a demo gallery.** + Ten `*_example.py` files shipping inside a library is a liability. + Move the examples to `examples/`, excluded from the distribution, keep only `src/rejuvenation/cli.py` + and promote it to `src/renaissance/cli.py`. + +Use deprecating re-export shims for one release so that downstream users are not broken. + +## Phase 2 - ASTNode is a TextSegment, and the protocol becomes read-only + +This is the keystone phase. Concretely: + +- `NodeProtocol` extends `TextSegment`, so every node answers `full_text`, `start_offset`, `end_offset`, `start_line` and so on. +- All `NodeProtocol` members become read-only properties. Mutation becomes the rewriter's job, + which is exactly the separation that issue 187 asks for. +- `ASTNode._offset`, `_filename`, `_parent` and `_children` stop being public mutable state. + +Execute protocol-first: tighten `NodeProtocol`, let Pyright in strict mode report which of the six implementations break, +then fix them one parser binding per pull request. +Expect this phase to delete a large fraction of the `reportUnknownMemberType` debt as a side effect. +It is not a separate workstream. + +## Phase 3 - Pattern as a first-class type + +```python +class Pattern[N: NodeProtocol]: + tree: N + semantic_kind: SemanticKind # what this pattern may match + placeholders: Mapping[str, Placeholder] + + +class Placeholder: + name: str + pattern_kind: PatternKind # MATCH_ONE | MATCH_ALL + semantic_kind: SemanticKind # fixes PatternFactory.create_statement("$placeholder") +``` + +Placeholders are discovered once, at pattern construction, inside the `PatternFactory`, +instead of being re-parsed by regular expression during every match attempt in `match_finder`. +This removes the `create_statement("$placeholder")` over-matching problem by construction: +the placeholder carries `STATEMENT`, so `node_kinds_match` rejects an expression. + +## Phase 4 - Matcher correctness + +With `Pattern` in place, `Variant.exp` becomes a typed `Bindings` object that owns the consistency rule. +The ambiguity requirement then becomes a feature of that object rather than a check scattered through the matcher. + +- A re-binding of `$x` to a structurally equal but distinct node is recorded as an alternative binding, not silently accepted. +- `PatternMatch` gains `ambiguous_placeholders`, and `find_match` can be asked to fail loudly. + +The identical-if-branches corner case described in the README then becomes a reported diagnostic +instead of something caught by an assertion downstream. + +## Phase 5 - One representation for parser and rewriter + +Phase 2 makes this almost mechanical. +If every node is a `TextSegment` over the same `full_text` bytes, then `Rewriter` in `src/renaissance/common/rewriter.py` +and `ASTRewriter` in `src/renaissance/syntax_tree/ast_rewriter.py` can collapse into a single rewriter +that accepts either a `TextSegment` range or a node. +That is precisely the "both AST-based and character-based rewriting" item. +The `Rewritable` protocol disappears, subsumed by `TextSegment`. + +Drive this phase with the existing `@xfail_*` scenarios in `features/rewrite-semantics.feature` as the acceptance list. + +## Phase 6 - Recipe algebra + +Turn recipes into values. + +```python +class Recipe(Protocol): + def apply(self, unit: TranslationUnit) -> RewriteResult: ... + + +Sequence(r1, r2) # composition +RepeatUntilFixedPoint(r) # recursive find and replace falls out, per issue 187 +``` + +Keep the decorator API as a thin adapter over this, so that existing recipes keep working. + +Recursive find-and-replace is not a separate item. +It is `RepeatUntilFixedPoint(FindAndReplace(pattern, replacement))`. +Port the Renaissance-Ada convergence and iteration-cap semantics here; +the current `max_repeat=5` in `BatchASTProcessor` is the thing being generalized. + +## Phase 7 - Semantic services + +Introduce `variables_read`, `variables_written` and `has_side_effects` as core interfaces, implemented per parser binding. + +This comes last because the interface shape depends on the node identity from phase 2 and the typed patterns from phase 3. +Ship the Python `ast` and Clang implementations; let tree-sitter raise `NotImplementedError`, +so that a recipe needing semantics fails loudly rather than silently mis-transforming. + +## Recommended push-back + +- **Do not treat "remove all Ruff and Pyright issues" as a phase.** + At roughly 9200 issues it will never be scheduled, and done standalone it produces annotation churn with no design improvement. + Phases 2 to 5 eliminate most of it structurally, and the per-package ratchet from phase 0 captures the gains. + Only the residue deserves dedicated effort. +- **Do not combine phase 1 and phase 2.** + A move-plus-redesign pull request is unreviewable, and it destroys the ability to distinguish a regression from a relocation. +- **The GDI tracing idea in issue 187** is a validation tool for the structure, not a driver of it. + The import-layering checker in phase 0 gives most of that signal statically, today, for a fraction of the effort. + +## Suggested first three pull requests + +1. `lint-budget.json` split per package, with `core` declared strict at budget 0 once those two violations are fixed. +1. Two ADRs: "Sequence over union in core APIs" and "ASTNode is a TextSegment". diff --git a/README.md b/README.md index 9adf21b1..67e971c2 100644 --- a/README.md +++ b/README.md @@ -128,13 +128,57 @@ By following these steps, you will have configured, installed, and verified the An incomplete list of todo's: -- The get_properties methods of both `ClangASTNode` and `ClangJsonASTNode` are not complete yet. This might cause mismatches in the `Match_Finder` -- C++ constructs have not been tested yet -- An example of how to use includes in a `Pattern` must be added -- Tests need to be added for macro handling -- The methods `get_references` and `referred_by` must be added to `ASTNode` and implemented in the concrete classes -- Test cases for multiple match patterns need to be added. Currently, there is only one working case in the examples -- Comments in Clang appear incorrectly in the `ASTShower`. This seems to be a Clang issue, which is surprising +- Improve the structure of the archive. The structure is reflected in both code and documentation. + The last three items are absent in the user's documentation / only present in the developer's documentation as + they are implementation details. + See [110](https://github.com/TNO/Renaissance.Py/issues/110) and + [187](https://github.com/TNO/Renaissance.Py/issues/187) for background information. + - The renaissance core, the language agnostic part, containing among others AST-based pattern matching. + - Recipes that analyse and/or transform code. Recipes can use other recipes - for higher efficiency and quality. + - Parser bindings to connect different parsers to the core. + - Tools that are needed in the development: They safe guard the renaissance development. + - Tests including unit and feature tests, which internal structure reflects the structure of the whole archive. +- Improve the nodeprotocol + - The nodeprotocol currently has mutable attributes, while most parser have readonly attributes - The protocol must get readonly attributes. + - The nodeprotocol doesn't link an AST node to a TextSegment of the code - This link has to be added + (probably through inheritance as an ASTNode is a TextSegment). +- Remove all issues detected by Ruff and Pyright + - Do we want a union, e.g., X | Sequence[X], or just a Sequence[X] as type hint - + as a single element of X can always be turned into a sequence of X elements? +- Introduce the pattern class. Currently a pattern is just an ASTNode, yet a pattern is NOT an ASTNode. + A pattern has placeholders. + - A pattern should also have a semantic kind to ensure that exotic patterns, like created with PatternFactory.create_statement("$placeholder"), + will only match a statement node and not every AST node. +- Improve rewriter to support both AST-based and character-based code rewritting + (see [user documentation for details.](https://tno.github.io/Renaissance.Py/user/concepts/rewrite-semantics/)) +- Improve the rewriter such that the rewriter and the parser have exactly the same (internal) representation of the code, + as difference in representations can result in subtle errors. +- Improve pattern matcher to report multiple assignments to placeholders. + This aspect is crucial for quality. For example, when looking for instances of + + ```python + if $cond: + $f($$before, $trueArg, $$after) + else: + $f($$before, $falseArg, $$after) + ``` + + we didn't considered the corner cases where both branches were identical. + Fortunately, we assert that we expected only one match, and hence were pointed to these unexpected corner cases. +- Make a recursive find and replace. + - Repeat until convergence and recursive find and replace can yield different results. + - See the implementation in Renaissance-Ada as example. +- Make interface and implementation in Python for variable read and written by a code snippet (sequence of AST Node) +- Add tools for guard railing + - A tool should check that the code in the renaissance core is independent of all other code. +- Minor + - The get_properties methods of both `ClangASTNode` and `ClangJsonASTNode` are not complete yet. This might cause mismatches in the `Match_Finder` + - C++ constructs have not been tested yet + - An example of how to use includes in a `Pattern` must be added + - Tests need to be added for macro handling + - The methods `get_references` and `referred_by` must be added to `ASTNode` and implemented in the concrete classes + - Test cases for multiple match patterns need to be added. Currently, there is only one working case in the examples + - Comments in Clang appear incorrectly in the `ASTShower`. This seems to be a Clang issue, which is surprising ## Usage diff --git a/docs/developer/index.md b/docs/developer/index.md index 1851b048..73420849 100644 --- a/docs/developer/index.md +++ b/docs/developer/index.md @@ -5,8 +5,9 @@ The developer documentation describes the implementation-oriented view of the re ## Sections 1. [Architecture](architecture/index.md) -2. [Coding conventions](coding-conventions.md) -3. [Feature ↔ test mapping](feature-test-map/index.md) -4. [Code modules](modules/index.md) -5. [API reference](api/index.md) -6. [Coverage report](https://tno.github.io/Renaissance.Py/coverage/) (generated from the latest `main` build) +1. [Coding conventions](coding-conventions.md) +1. [Guard rail tools](tools/index.md) +1. [Feature ↔ test mapping](feature-test-map/index.md) +1. [Code modules](modules/index.md) +1. [API reference](api/index.md) +1. [Coverage report](https://tno.github.io/Renaissance.Py/coverage/) (generated from the latest `main` build) diff --git a/docs/developer/tools/check-layering.md b/docs/developer/tools/check-layering.md new file mode 100644 index 00000000..b6bb653c --- /dev/null +++ b/docs/developer/tools/check-layering.md @@ -0,0 +1,82 @@ +# Layering check + +`tools/check_layering.py` fails the build when a module imports another module that it is not allowed to depend on. + +## The rule it enforces + +The source tree is divided into layers, and a layer may only import the layers below it. + +| Layer | Packages | May import | +| --- | --- | --- | +| `core` | `renaissance.syntax_tree`, `renaissance.common`, `renaissance.utils` | nothing outside the core | +| `parser_bindings` | `renaissance.integrations` | `core` | +| `recipes` | `renaissance.recipes`, `rejuvenation` | `core`, `parser_bindings` | + +Imports within a layer are always allowed, and imports of the standard library or of third-party packages are ignored. + +The direction matters because the core carries the language- and parser-agnostic machinery: the AST abstraction, pattern matching, +rewriting and recipe execution. A parser binding connects one concrete parser to that machinery, and a recipe expresses an analysis +or a transformation in terms of it. As long as the core never reaches back into a binding, a binding can be added, replaced or +removed without touching the core, and a recipe can run against any parser that provides the abstractions the core defines. One +import in the wrong direction silently removes that property: the core then only works when that particular parser is installed +and importable. + +## Running it + +```powershell +python tools/check_layering.py +``` + +`--check` is accepted as well, for symmetry with the other guard rails that CI runs; this check never writes anything, so both +invocations behave identically. The script only parses the sources, it never imports them, so a parser binding whose external +dependency is missing cannot make the check fail or pass by accident. + +Exit codes: + +| Code | Meaning | +| --- | --- | +| 0 | every violation found is covered by the allowlist | +| 1 | a violation is not allowlisted, or an allowlist entry no longer matches a violation | +| 2 | the sources could not be analysed, for example because a file does not parse | + +## Reading its output + +Every violation is reported on one line, naming the file and the line number so that terminals and CI logs can link to it: + +```text +src\renaissance\utils\ast_utils.py:7: renaissance.utils.ast_utils (core) imports renaissance.integrations (parser_bindings) +``` + +Violations are reported wherever they are written: at the top of a module, inside a function body, inside a `try` block, and inside +an `if TYPE_CHECKING:` block. The last kind is marked `[type-checking only]`. Such an import does not exist at run time, but it is +still a design dependency — the core cannot be type-checked, documented or understood without the binding — so it is reported like +any other. + +Relative imports are resolved against the package of the importing module before they are judged, so `from ..integrations import x` +is reported as an import of `renaissance.integrations`. + +## The allowlist + +Violations that are known and accepted for now are listed in the `ALLOWLIST` of the script, each with the module, the imported +module and the reason it still exists. An allowlisted violation is printed on every run, under a heading that says it is to be +resolved, but it does not fail the check. This keeps the build green while the problem stays visible. + +Two properties keep the allowlist from becoming a dumping ground: + +1. An entry names one importing module and one imported module exactly. Wildcards are not supported, so an entry can never cover a + violation that was added later. +1. An entry that no longer matches a violation fails the check. Once an import is removed, the next run tells you to delete its + entry, so the allowlist cannot outlive the problem it describes. + +Add an entry only when the fix is a design change that does not belong in the pull request at hand, and record *why* in the reason. +The preferred fix is almost always to move the abstraction the core needs into the core, and to let the binding implement it. + +## When the packages are renamed + +The layer definitions are the `LAYERS` and `ALLOWED_DEPENDENCIES` mappings at the top of the script. `LAYERS` maps a layer name to +the packages it owns, and `ALLOWED_DEPENDENCIES` maps a layer name to the other layers it may import. Renaming or moving a package +is a one-line edit to `LAYERS`; introducing a layer is one entry in each mapping. Nothing else in the script knows the package +names, and the tests run against synthetic package trees rather than against `src`, so they keep working across a restructuring. + +After a rename, run the check: the modules whose allowlist entries still carry the old names will be reported as stale, which is +the signal to update or delete them. diff --git a/docs/developer/tools/index.md b/docs/developer/tools/index.md new file mode 100644 index 00000000..373e2fd1 --- /dev/null +++ b/docs/developer/tools/index.md @@ -0,0 +1,22 @@ +# Guard rail tools + +Guard rail tools safeguard the development of Renaissance.Py. +Each of them encodes a rule that the repository has agreed on, checks it automatically, and fails the build when the rule is broken, +so that a rule stays true instead of slowly eroding. + +They are kept in the `tools` directory, can all be run locally before a pull request is opened, and are all run again in CI. + +## Available tools + +1. [Layering check](check-layering.md) (`tools/check_layering.py`): + forbids imports that cross a layer boundary, which keeps the core independent of the parser bindings and the recipes. +1. [Lint and type-check budget](../coding-conventions.md#lint-and-type-check-budgets) (`tools/lint_budget.py`): + counts the `ruff` and `pyright` issues per kind and fails when a change introduces a new one. +1. [Image policy check](../../documentation-system/enforcement/image-policy-enforcement.md) (`tools/check_doc_images_policy.py`): + enforces the naming and the location of documentation image assets. + +## Adding a guard rail + +A new guard rail is worth adding when a rule is stated in the documentation, is easy to break by accident, and can be checked +mechanically. Give it a script in `tools`, tests in `test/tools`, a page in this section, an entry in the list above, and a step in +the appropriate workflow in `.github/workflows`. diff --git a/lint-budget.json b/lint-budget.json index 896872bd..bbde5f2c 100644 --- a/lint-budget.json +++ b/lint-budget.json @@ -47,7 +47,7 @@ "BLE001": 24, "C400": 6, "C901": 9, - "CPY001": 188, + "CPY001": 190, "EM101": 4, "EM102": 5, "ERA001": 195, @@ -55,7 +55,7 @@ "FBT002": 38, "FBT003": 135, "FIX002": 46, - "INP001": 36, + "INP001": 37, "PLR0206": 1, "PLR0911": 1, "PLR0912": 4, diff --git a/mkdocs.yml b/mkdocs.yml index d836a63f..c624be10 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -145,6 +145,10 @@ nav: - Overview: developer/index.md - Coding conventions: developer/coding-conventions.md + - Guard rail tools: + - Overview: developer/tools/index.md + - Layering check: developer/tools/check-layering.md + - Architecture: - Overview: developer/architecture/index.md - Code architecture: developer/architecture/code-architecture.md diff --git a/test/tools/test_check_layering.py b/test/tools/test_check_layering.py new file mode 100644 index 00000000..b8bbb3ed --- /dev/null +++ b/test/tools/test_check_layering.py @@ -0,0 +1,128 @@ +"""Tests for the layer-boundary guard rail in tools/check_layering.py. + +The `tools` directory is not on the pytest `pythonpath`, so the script is loaded from its path. +The unit tests run against synthetic package trees, so they stay valid when `src/` is restructured. +""" + +import importlib.util +from pathlib import Path +from types import ModuleType + +import pytest + +ROOT = Path(__file__).resolve().parents[2] + + +def _load_check_layering() -> ModuleType: + """Import tools/check_layering.py as a module.""" + path = ROOT / "tools" / "check_layering.py" + spec = importlib.util.spec_from_file_location("check_layering", path) + if spec is None or spec.loader is None: + message = f"cannot load {path}" + raise RuntimeError(message) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +check_layering = _load_check_layering() + + +@pytest.fixture +def source_root(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + """Return an empty source tree with two layers: `bindings` may import `core`, but not the other way around.""" + monkeypatch.setattr(check_layering, "LAYERS", {"core": ("core",), "bindings": ("bindings",)}) + monkeypatch.setattr(check_layering, "ALLOWED_DEPENDENCIES", {"core": frozenset[str](), "bindings": frozenset({"core"})}) + monkeypatch.setattr(check_layering, "ALLOWLIST", ()) + for package in ("core", "bindings"): + (tmp_path / package).mkdir() + (tmp_path / package / "__init__.py").write_text("", encoding="utf-8") + return tmp_path + + +def _write(source_root: Path, relative: str, source: str) -> None: + """Write `source` to `relative` inside `source_root`, creating the packages on the way.""" + path = source_root / relative + path.parent.mkdir(parents=True, exist_ok=True) + for package in path.parents: + if package == source_root: + break + (package / "__init__.py").touch() + path.write_text(source, encoding="utf-8") + + +def _pairs(source_root: Path) -> list[tuple[str, str]]: + """Return the (module, imported module) pair of every violation found below `source_root`.""" + return [(violation.module, violation.imported) for violation in check_layering.find_violations(source_root)] + + +def test_import_of_a_lower_layer_is_allowed(source_root: Path) -> None: + """Assert a parser binding may import the core.""" + _write(source_root, "bindings/parser.py", "from core.node import Node\n") + assert _pairs(source_root) == [] # noqa: S101 (pytest assertion; new tests must not raise the S101 budget) + + +def test_import_of_a_higher_layer_is_reported(source_root: Path) -> None: + """Assert the core may not import a parser binding.""" + _write(source_root, "core/node.py", "from bindings.parser import Parser\n") + assert _pairs(source_root) == [("core.node", "bindings.parser")] # noqa: S101 (pytest assertion; new tests must not raise the S101 budget) + + +def test_relative_import_is_resolved_before_it_is_judged(source_root: Path) -> None: + """Assert a relative import that leaves the layer is resolved to its absolute module and reported.""" + _write(source_root, "core/deep/node.py", "from ...bindings.parser import Parser\n") + assert _pairs(source_root) == [("core.deep.node", "bindings.parser")] # noqa: S101 (pytest assertion; new tests must not raise the S101 budget) + + +def test_import_inside_a_function_is_reported(source_root: Path) -> None: + """Assert a deferred import does not escape the check.""" + _write(source_root, "core/node.py", "def build():\n from bindings.parser import Parser\n return Parser\n") + assert _pairs(source_root) == [("core.node", "bindings.parser")] # noqa: S101 (pytest assertion; new tests must not raise the S101 budget) + + +def test_type_checking_import_is_reported_and_flagged(source_root: Path) -> None: + """Assert an import that only exists for the type checker is reported, and marked as such.""" + _write(source_root, "core/node.py", "from typing import TYPE_CHECKING\n\nif TYPE_CHECKING:\n from bindings.parser import Parser\n") + violations = check_layering.find_violations(source_root) + flags = [(violation.imported, violation.type_checking_only) for violation in violations] + assert flags == [("bindings.parser", True)] # noqa: S101 (pytest assertion; new tests must not raise the S101 budget) + + +def test_same_layer_and_external_imports_are_ignored(source_root: Path) -> None: + """Assert imports within a layer, and imports of packages no layer owns, are not violations.""" + _write(source_root, "core/node.py", "import json\n\nfrom core.text import Text\nfrom clang.cindex import Cursor\n") + assert _pairs(source_root) == [] # noqa: S101 (pytest assertion; new tests must not raise the S101 budget) + + +def test_allowlisted_violation_does_not_fail_the_check(source_root: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """Assert a violation listed with its reason is reported but accepted.""" + _write(source_root, "core/node.py", "from bindings.parser import Parser\n") + exemption = check_layering.Exemption(module="core.node", imported="bindings.parser", reason="accepted for this test") + monkeypatch.setattr(check_layering, "ALLOWLIST", (exemption,)) + monkeypatch.setattr(check_layering, "SOURCE_ROOT", source_root) + monkeypatch.setattr(check_layering, "ROOT", source_root) + assert check_layering.main([]) == check_layering.EXIT_OK # noqa: S101 (pytest assertion; new tests must not raise the S101 budget) + + +def test_stale_allowlist_entry_fails_the_check(source_root: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """Assert an allowlist entry that no longer matches a violation fails, so the allowlist cannot rot.""" + exemption = check_layering.Exemption(module="core.node", imported="bindings.parser", reason="already resolved") + monkeypatch.setattr(check_layering, "ALLOWLIST", (exemption,)) + monkeypatch.setattr(check_layering, "SOURCE_ROOT", source_root) + monkeypatch.setattr(check_layering, "ROOT", source_root) + assert check_layering.main([]) == check_layering.EXIT_VIOLATIONS # noqa: S101 (pytest assertion; new tests must not raise the S101 budget) + + +def test_violation_outside_the_allowlist_fails_the_check(source_root: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """Assert an unknown violation fails the check.""" + _write(source_root, "core/node.py", "from bindings.parser import Parser\n") + monkeypatch.setattr(check_layering, "SOURCE_ROOT", source_root) + monkeypatch.setattr(check_layering, "ROOT", source_root) + assert check_layering.main(["--check"]) == check_layering.EXIT_VIOLATIONS # noqa: S101 (pytest assertion; new tests must not raise the S101 budget) + + +def test_the_known_violations_of_the_source_tree_are_found() -> None: + """Assert the core-to-binding imports that the allowlist describes are the ones the tool reports.""" + found = {(violation.module, violation.imported) for violation in check_layering.find_violations(ROOT / "src")} + known = {(exemption.module, exemption.imported) for exemption in check_layering.ALLOWLIST} + assert known <= found # noqa: S101 (pytest assertion; new tests must not raise the S101 budget) diff --git a/tools/check_layering.py b/tools/check_layering.py new file mode 100644 index 00000000..eb0a0646 --- /dev/null +++ b/tools/check_layering.py @@ -0,0 +1,239 @@ +"""Guard rail that forbids imports crossing a layer boundary. + +The source tree is divided into layers. Each layer may only import the layers below it: +the core is language- and parser-agnostic and may not depend on anything else, parser +bindings and recipes build on the core, and the application code may use all of them. +Keeping the core free of such dependencies is what makes a parser binding replaceable. + +This script parses every module under `src/` and reports each import that crosses a +boundary in the wrong direction. Violations that are known and accepted for now are listed +in `ALLOWLIST`, together with the reason; an allowlist entry that no longer matches a real +violation fails the check as well, so the allowlist cannot outlive the problem it describes. + +Run `python tools/check_layering.py` locally, or `python tools/check_layering.py --check` +as CI does; both report and neither writes. +""" + +import argparse +import ast +import sys +from collections.abc import Iterable, Iterator +from dataclasses import dataclass +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +SOURCE_ROOT = ROOT / "src" + +# The packages each layer owns. Renaming a package is a one-line edit here. +LAYERS: dict[str, tuple[str, ...]] = { + "core": ("renaissance.syntax_tree", "renaissance.common", "renaissance.utils"), + "parser_bindings": ("renaissance.integrations",), + "recipes": ("renaissance.recipes", "rejuvenation"), +} + +# The other layers each layer may import; importing within a layer is always allowed. +ALLOWED_DEPENDENCIES: dict[str, frozenset[str]] = { + "core": frozenset(), + "parser_bindings": frozenset({"core"}), + "recipes": frozenset({"core", "parser_bindings"}), +} + +EXIT_OK = 0 +EXIT_VIOLATIONS = 1 +EXIT_TOOL_FAILURE = 2 + + +@dataclass(frozen=True) +class Exemption: + """A known violation that is reported but does not fail the check yet.""" + + module: str + imported: str + reason: str + + +ALLOWLIST: tuple[Exemption, ...] = ( + Exemption( + module="renaissance.syntax_tree", + imported="renaissance.integrations.clang.cpp_utils", + reason="re-export of CPPUtils; the import is deliberately late to break a circular import (issue 110)", + ), + Exemption( + module="renaissance.syntax_tree.ast_refactor_actions", + imported="renaissance.integrations.clang.c_pattern_factory", + reason="CPPPatternFactory is used in a type annotation only; the core needs a parser-agnostic pattern type (issue 110)", + ), + Exemption( + module="renaissance.utils.ast_utils", + imported="renaissance.integrations", + reason="MATCH_ALL and MATCH_ONE are placeholder markers that belong in the core, not in the bindings (issue 110)", + ), +) + + +@dataclass(frozen=True) +class ImportSite: + """An import statement found in a module.""" + + module: str + line: int + type_checking_only: bool + + +@dataclass(frozen=True) +class Violation: + """An import that crosses a layer boundary in a direction that is not allowed.""" + + module: str + imported: str + source_layer: str + target_layer: str + path: Path + line: int + type_checking_only: bool + + def describe(self, relative_to: Path) -> str: + """Return a one-line report naming the file and line, so terminals can link to it.""" + suffix = " [type-checking only]" if self.type_checking_only else "" + location = self.path.relative_to(relative_to) if self.path.is_relative_to(relative_to) else self.path + return f"{location}:{self.line}: {self.module} ({self.source_layer}) imports {self.imported} ({self.target_layer}){suffix}" + + +def module_name(path: Path, source_root: Path) -> str: + """Return the dotted module name of `path`, which lies inside `source_root`.""" + parts = path.relative_to(source_root).with_suffix("").parts + if parts and parts[-1] == "__init__": + parts = parts[:-1] + return ".".join(parts) + + +def layer_of(module: str) -> str | None: + """Return the layer that owns `module`, or None when no layer claims it.""" + for layer, packages in LAYERS.items(): + if any(module == package or module.startswith(f"{package}.") for package in packages): + return layer + return None + + +def resolve_import(name: str | None, package: str, level: int) -> str: + """Return the absolute module name of an import of `name` written inside `package`.""" + if not level: + return name or "" + parts = package.split(".") if package else [] + prefix = ".".join(parts[: max(len(parts) - level + 1, 0)]) + if not prefix: + return name or "" + return f"{prefix}.{name}" if name else prefix + + +def _is_type_checking_test(test: ast.expr) -> bool: + """Return whether `test` is the `TYPE_CHECKING` condition of a type-checking-only block.""" + if isinstance(test, ast.Name): + return test.id == "TYPE_CHECKING" + return isinstance(test, ast.Attribute) and test.attr == "TYPE_CHECKING" + + +def iter_import_sites(nodes: Iterable[ast.AST], package: str, *, type_checking: bool) -> Iterator[ImportSite]: + """Yield every import below `nodes`, including those nested in functions and in `if TYPE_CHECKING` blocks.""" + for node in nodes: + if isinstance(node, ast.Import): + for alias in node.names: + yield ImportSite(alias.name, node.lineno, type_checking) + elif isinstance(node, ast.ImportFrom): + yield ImportSite(resolve_import(node.module, package, node.level), node.lineno, type_checking) + elif isinstance(node, ast.If) and _is_type_checking_test(node.test): + yield from iter_import_sites(node.body, package, type_checking=True) + yield from iter_import_sites(node.orelse, package, type_checking=type_checking) + else: + yield from iter_import_sites(ast.iter_child_nodes(node), package, type_checking=type_checking) + + +def violations_in(path: Path, source_root: Path) -> list[Violation]: + """Return the layering violations of the single module at `path`.""" + module = module_name(path, source_root) + source_layer = layer_of(module) + if source_layer is None: + return [] + package = module if path.name == "__init__.py" else module.rpartition(".")[0] + allowed = ALLOWED_DEPENDENCIES.get(source_layer, frozenset()) + tree = ast.parse(path.read_text(encoding="utf-8"), filename=str(path)) + violations: list[Violation] = [] + for site in iter_import_sites(tree.body, package, type_checking=False): + target_layer = layer_of(site.module) + if target_layer is None or target_layer == source_layer or target_layer in allowed: + continue + violations.append( + Violation( + module=module, + imported=site.module, + source_layer=source_layer, + target_layer=target_layer, + path=path, + line=site.line, + type_checking_only=site.type_checking_only, + ) + ) + return violations + + +def find_violations(source_root: Path) -> list[Violation]: + """Return every layering violation below `source_root`, ordered by file and line.""" + violations: list[Violation] = [] + for path in sorted(source_root.rglob("*.py")): + if "__pycache__" in path.parts: + continue + violations += violations_in(path, source_root) + return sorted(violations, key=lambda violation: (str(violation.path), violation.line)) + + +def exemption_for(violation: Violation) -> Exemption | None: + """Return the allowlist entry that covers `violation`, or None when it is not exempted.""" + for exemption in ALLOWLIST: + if exemption.module == violation.module and exemption.imported == violation.imported: + return exemption + return None + + +def stale_exemptions(violations: Iterable[Violation]) -> list[Exemption]: + """Return the allowlist entries that no longer match a violation, so they can be deleted.""" + matched = {(violation.module, violation.imported) for violation in violations} + return [exemption for exemption in ALLOWLIST if (exemption.module, exemption.imported) not in matched] + + +def main(argv: list[str] | None = None) -> int: + """Report the layering violations under `src/` and fail on any that is not allowlisted.""" + parser = argparse.ArgumentParser(description="Check that the imports in src/ respect the layer boundaries.") + # --check mirrors tools/lint_budget.py; this check never writes, so it only documents the CI invocation. + parser.add_argument("--check", action="store_true", help="only report, never write (used by CI; this check never writes)") + parser.parse_args(argv) + + try: + violations = find_violations(SOURCE_ROOT) + except (OSError, SyntaxError) as error: + print(f"Could not analyse {SOURCE_ROOT}: {error}", file=sys.stderr) # noqa: T201 (a script reports on stdout and stderr) + return EXIT_TOOL_FAILURE + + forbidden = [violation for violation in violations if exemption_for(violation) is None] + exempted = [violation for violation in violations if exemption_for(violation) is not None] + stale = stale_exemptions(violations) + + report: list[str] = [] + if exempted: + report.append("Allowlisted layering violations (to be resolved, not to be extended):") + report += [f" {violation.describe(ROOT)}" for violation in exempted] + if forbidden: + report.append("\nThese imports cross a layer boundary that is not allowed:") + report += [f" {violation.describe(ROOT)}" for violation in forbidden] + report.append("\nImport through an abstraction of the layer below instead, or ask for an allowlist entry.") + if stale: + report.append("\nThese allowlist entries no longer match a violation and must be deleted from tools/check_layering.py:") + report += [f" {exemption.module} -> {exemption.imported}" for exemption in stale] + if not forbidden and not stale: + report.append(f"Layering check passed: {len(exempted)} allowlisted violation(s), no new ones.") + print("\n".join(report)) # noqa: T201 (a script reports on stdout and stderr) + + return EXIT_VIOLATIONS if forbidden or stale else EXIT_OK + + +if __name__ == "__main__": + raise SystemExit(main())