diff --git a/.gitignore b/.gitignore index 931aaa55..2ed6c9d1 100644 --- a/.gitignore +++ b/.gitignore @@ -234,3 +234,6 @@ __marimo__/ **/*.dll **/.*.so **/*.dot + +# Folder containing relevant presentations made with Marp +.marp_presentations \ No newline at end of file diff --git a/docs/TODO b/docs/TODO index 7d9b4fa1..1437f612 100644 --- a/docs/TODO +++ b/docs/TODO @@ -1,4 +1,3 @@ - ## Documentation gaps: action points for docs ### Stub pages that need content @@ -35,9 +34,12 @@ 15. **Scoping: skip and filter at the file level** (add to strategy-composition.md or a new page) — Java's `skipATU`, `globalFilter`, `sourceFilePostfixes`, and `sourceFileDirectories` provide coarse-grained scoping before any pattern matching runs. The Python equivalent concept needs documentation. - ### New pages worth adding 19. **Common parser problems** (new page, e.g., `docs/developer/modules/parser-known-limitations.md`) — Java has CommonCdtParsingProblems.md listing concrete CDT/MSVC-extension parsing failures with workarounds. A Python equivalent covering known tree-sitter, libcst, clang binding, and ANTLR limitations would be directly useful. -20. **Related works and context** (new section in index.md or index.md) — The Java UserGuide.md relates the tool to WHARS and ADA tooling. Adding a "related works" section that situates Renaissance-Experiments relative to the Java version, comby, and other code transformation tools would help new contributors understand design choices. \ No newline at end of file +20. **Related works and context** (new section in index.md or index.md) — The Java UserGuide.md relates the tool to WHARS and ADA tooling. Adding a "related works" section that situates Renaissance-Experiments relative to the Java version, comby, and other code transformation tools would help new contributors understand design choices. + +### Naming + +21. **Type parameter modernization: rename the recipe and update its code references** — The page and rule links use `type-parameter-modernization`, but the recipe is still `TypeVarCheck` (`type_var_check.py`, `type_var_domain.py`, `test_type_var_*`). Rename the recipe to `TypeParameterCheck` and update its implementation and tests in one change (ADR 15 resolves recipe names with `snake_case()`), then update the docs that name the recipe. diff --git a/docs/developer/architecture/position-consistency.md b/docs/developer/architecture/position-consistency.md index d93a4999..bf934ef6 100644 --- a/docs/developer/architecture/position-consistency.md +++ b/docs/developer/architecture/position-consistency.md @@ -159,9 +159,10 @@ the two sides can silently disagree, producing incorrect or corrupted output. * `src/renaissance/syntax_tree/ast_rewriter.py` - derives the buffer a rewrite batch is applied to. * `src/renaissance/common/rewriter.py` - splices replacement text into that buffer, in bytes. * `src/renaissance/syntax_tree/ast_node.py` - `binary_file_content`, the root-owned buffer accessor. -* `src/renaissance/integrations/python/ast/rst_node.py`, `src/renaissance/integrations/python/ast/cst_node.py` - and `src/renaissance/integrations/python/ast/util.py` - the Python adapters and the line/column to - offset conversion they share. +* `src/renaissance/integrations/python/ast/rst_node.py` and `src/renaissance/integrations/python/ast/util.py` - + the RST adapter and its line/column to offset conversion. +* `src/renaissance/integrations/python/ast/cst_node.py` - the CST adapter, which converts libcst's line/column + positions to byte offsets itself. * `src/renaissance/integrations/tree_sitter/lst.py` - the tree-sitter adapter. * `src/renaissance/integrations/clang/clang_ast_node.py` and `src/renaissance/integrations/clang/clang_json_ast_node.py` - the clang adapters. diff --git a/docs/developer/feature-test-map/core.md b/docs/developer/feature-test-map/core.md index da499e34..be229dbb 100644 --- a/docs/developer/feature-test-map/core.md +++ b/docs/developer/feature-test-map/core.md @@ -11,7 +11,16 @@ https://github.com/TNO/Renaissance.Py/blob/main/LICENSE ## 1. Pattern matching - **Feature:** [Pattern matching](../../user/features/pattern-matching.md) -- **Suggested test modules:** `tests/matching/` +- **Test file(s):** + - `test/syntax_tree/test_match_finder.py` + - `test/syntax_tree/test_match_finder_multi_assignments.py` + - `test/syntax_tree/test_match_tree.py` + - `test/syntax_tree/test_match_dict.py` + - `test/syntax_tree/test_pattern_match.py` + - `test/syntax_tree/test_pattern_kind.py` + - `test/python/ast/test_python_matcher.py` + - `test/c_cpp/test_c_match_finder.py` +- **Code file(s):** `src/renaissance/syntax_tree/match_finder.py`, `src/renaissance/syntax_tree/pattern_kind.py` ## 2. Rewrite semantics @@ -19,5 +28,30 @@ https://github.com/TNO/Renaissance.Py/blob/main/LICENSE - **Concepts:** [Rewrite semantics](../../user/concepts/rewrite-semantics.md) - **Test modules:** [Rewrite semantics test module](../../developer/modules/rewrite-semantics.md) - **BDD feature file:** `features/rewrite-semantics.feature` -- **BDD steps:** `features/steps/test-rewrite-semantics.py` +- **BDD steps:** `features/steps/test_rewrite_semantics.py` +- **Test file(s):** + - `test/common/test_rewriter.py` + - `test/syntax_tree/test_ast_rewriter.py` + - `test/syntax_tree/test_rewrite_semantics_properties.py` - **Code files:** `src/renaissance/common/rewriter.py`, `src/renaissance/syntax_tree/ast_rewriter.py` + +## 3. Type parameter modernization + +- **Feature:** [Type parameter modernization](../../user/features/type-parameter-modernization.md) +- **Concepts:** [Rewrite semantics](../../user/concepts/rewrite-semantics.md) +- **Code modules:** [Refactoring recipes](../../developer/modules/recipes.md) +- **Test file(s):** + - `test/recipes/test_type_var_check.py` + - `test/recipes/test_type_var_check_convert.py` + - `test/recipes/test_type_var_check_localize.py` + - `test/recipes/test_type_var_check_orphaned.py` + - `test/recipes/test_type_var_check_properties.py` + - `test/recipes/test_type_var_domain.py` + - `test/recipes/test_step_runner.py` + - `test/recipes/test_python_refactoring.py` + - `test/utils/test_import_resolution.py` + - `test/rejuvenation/test_migration_type_recipes.py` +- **Code file(s):** `src/renaissance/recipes/type_var_check.py`, `src/renaissance/recipes/type_var_domain.py`, + `src/renaissance/recipes/step_runner.py`, + `src/renaissance/recipes/python_refactoring.py`, + `src/renaissance/utils/import_resolution.py`, `src/rejuvenation/migration-type-recipes.py` diff --git a/docs/developer/modules/index.md b/docs/developer/modules/index.md index ffafab81..e661a459 100644 --- a/docs/developer/modules/index.md +++ b/docs/developer/modules/index.md @@ -15,3 +15,6 @@ https://github.com/TNO/Renaissance.Py/blob/main/LICENSE 5. [Transformation modules](transformation.md) 6. [Observability modules](observability.md) 7. [Strategy modules](strategy.md) +8. [Rewrite semantics module](rewrite-semantics.md) +9. [Refactoring recipes](recipes.md) +10. [Rejected recipes](rejected-recipes.md) diff --git a/docs/developer/modules/parser-and-ast.md b/docs/developer/modules/parser-and-ast.md index ed9d1d61..af0318e4 100644 --- a/docs/developer/modules/parser-and-ast.md +++ b/docs/developer/modules/parser-and-ast.md @@ -107,11 +107,16 @@ preserves comments and whitespace, or reparses source. Each adapter supplies those behaviors individually. Reference and data-flow collections (`references`, `referenced_by`) are also parser-specific because not every parser can provide the same analysis. Keep the original parser node available -when callers need parser-specific data. +when callers need parser-specific data. The Python CST adapter resolves them +with libcst's scope analysis, which also finds names used inside string +annotations. An annotation or type hint written above a definition in the +same scope still refers to it, since it is evaluated lazily. The native Python `ast.AST` adapter is a lightweight structural adapter and does not preserve source trivia in the same way as the lossless Python RST/CST adapters. Use the lossless adapter when source-preserving rewriting is needed. +`PythonRefactoring` parses with the RST adapter by default; pass +`node_class=PythonCstNode` to use the CST adapter instead. ## Classify nodes diff --git a/docs/developer/modules/recipes.md b/docs/developer/modules/recipes.md new file mode 100644 index 00000000..bfc7d738 --- /dev/null +++ b/docs/developer/modules/recipes.md @@ -0,0 +1,339 @@ + + +# Refactoring recipes + +{ #codemod-recipes } + +**Stable ID:** `CODEMOD-RECIPES` + +## Responsibility + +Recipes are `PythonRefactoring` subclasses that rewrite one Python file at a time, for gaps `ruff` does not fully or +safely cover. This page covers `TypeVarCheck`, the recipe behind +[Type parameter modernization](../../user/features/type-parameter-modernization.md). + +## Location + +- `src/renaissance/recipes/type_var_check.py`: the `TypeVarCheck` pipeline (orchestration only). +- `src/renaissance/recipes/type_var_domain.py`: domain model and safety analysis. +- `src/renaissance/recipes/step_runner.py`: `Step`/`run_steps`, runs fix actions in order and commits only the ones + that fixed something. +- `src/renaissance/recipes/python_refactoring.py`: the base class. +- `src/renaissance/utils/import_resolution.py`: resolves `from X import Y` to a file in the project, re-anchors one + file's relative import on another file, and collects the names each project file imports from the others. +- `src/renaissance/recipes/python_refactoring.py`: the base class. +- `src/renaissance/utils/import_resolution.py`: resolves `from X import Y` to a file in the project, re-anchors one + file's relative import on another file, and collects the names each project file imports from the others. + +## Public entry points + +- `TypeVarCheck.check()` (and `run()`): runs the three phases through `run_steps`, committing to disk between them. +- The phases, each returning `{name: "fixed" | "unsafe"}` and recording the `UnsafeReason` of each unsafe name: + + | Phase | Reasons attribute | + | ------------------------------------------------------------ | --------------------------- | + | `localize_imported_typevars()` | `cross_file_unsafe_reasons` | + | `convert_declared_typevars()` (only adds type parameters) | `converted_unsafe_reasons` | + | `remove_orphaned_declarations()` (only removes declarations) | `orphaned_unsafe_reasons` | + +- Attributes set by the CLI (`migration-type-recipes.py`): `min_python`, `project_root` (falls back to the file's + folder) and `project_wide_imported_names` (from `collect_project_imported_names`). +- `PythonRefactoring.process("TypeVarCheck", file)`, used by `cli.py refactor`, never sets those attributes, so every + version-gated rewrite is reported `"unsafe"` there. + +## Internal structure + +- **libcst throughout.** The recipe runs on the CST backend (`PythonRefactoring[PythonCstNode]`) and analyses the + same libcst tree it rewrites; the cross-file phase parses the origin with `libcst.parse_module`. Each edit + replaces only the node it changes, so formatting and comments elsewhere are kept. + +- **Unsafe reasons.** `is_safe_to_remove`/`is_safe_to_localize` and `_conversion_refusal` return an `UnsafeReason` + or `None`. Each of the 9 reasons has an `UnsafeRule` (message + docs anchor) in `UNSAFE_RULES`; `doc_link()` turns + it into the URL that the CLI report prints. + +- **Version gates.** `_conversion_refusal` checks `min_python` against `PEP_695_MINIMUM = (3, 12)` and, for a + `default=`, `PEP_696_MINIMUM = (3, 13)`. `None` never passes. A keyword outside `_CONVERTIBLE_KEYWORDS` is refused + (`NO_PEP695_EQUIVALENT`) instead of dropped. + +- **Uses.** `uses_of` takes a declaration's uses from libcst's scope analysis (`referenced_by`). A use inside a + string annotation or `cast("T", x)` counts; a `T` that a PEP 695 type parameter of an enclosing function or class + declares (`def f[T]`, `class C[T]`) does not. + +- **Signature edit.** `_add_type_params` replaces only the function's name (`f` → `f[T]`) or, if it has one, its + type-parameter bracket; the body is never touched. `type_param_text` writes each parameter (`T: int`, `**P`, + `*Ts`, `= default`) from the declaration's own text. + +- **One edit per function.** All type parameters of a function go into one `replace()`, in declaration order, with + the defaulted ones last (`type_params_bracket`). Two edits on one node would be rejected as conflicting rewrites. + +- **Owning function.** A legacy type parameter belongs to the function whose signature uses it. `signature_function` + finds the functions whose parameter or return annotations use the name, and `owning_function` gives each use to the + outermost of them that encloses it. A nested function whose enclosing function's signature also uses `T` gets no + `[T]` of its own, since it already sees the outer one. A use outside every such signature, e.g. `y: list[T] = []` in + a function whose signature doesn't use `T`, is given to no function. + +- **Generic classes.** Methods of a class with the name in a base (`generic_classes`) are skipped. A generic class at + the origin does not block localization. + +- **No mixed syntax.** A function using a declaration that `_conversion_refusal` rejects gets none of its type + parameters, since PEP 695 forbids mixing them with legacy ones. A name whose every function is blocked is + `SHARES_FUNCTION_WITH_UNCONVERTIBLE`. This needs two passes: all refusals are known before any parameter is added. + +- **Orphans.** A declaration with no uses left (`uses_of` is empty) is orphaned. Conversion skips such names too. + +- **Localization.** The origin's declaration is copied as written, with the comment trailing it when it is alone on + its line (`declaration_text`). `DECLARATION_NAME_UNAVAILABLE` if the origin only binds a name its arguments use + (`declaration_argument_names`) under `if TYPE_CHECKING:` or this file binds it to something else + (`from_import_sources`). All names localized from one import statement are handled together. + + A name the origin imports, the constructor or one an argument uses, is imported here from the module the origin + gets it from, with its alias (`from_import_binding`). A relative import of the origin is re-anchored on this file + by `rebase_relative_module`. A name the origin defines itself, or one whose module this file can't name, is + imported from the origin. + +- **Imports.** An import whose names are now declared locally is narrowed, keeping its comments, or dropped with its + trailing comment when no name remains. The needed imports, each once per file, and then the declarations go after + the file's last import, so no import is left below code (E402). A localization is written where its import was + when that import shares its line with other statements and is dropped, when no import of the file remains, or when + a statement between it and the last import reads one of its names. Merging and sorting the added imports, and + removing imports that became unused, is left to the CLI's `ruff check --fix --select F401,I001` pass. + +## Related features + +- [Type parameter modernization](../../user/features/type-parameter-modernization.md) + +## Related concepts + +- [Rewrite semantics](../../user/concepts/rewrite-semantics.md) +- [Python version gates](../../user/features/type-parameter-modernization.md#feature-type-parameter-modernization-version-gates) + +## Validated by test modules + +See *Verified by test modules* in [Type parameter modernization](../../user/features/type-parameter-modernization.md). + +## Extension points + +- A new recipe is a `PythonRefactoring` subclass in its own `snake_case`-named module in `src/renaissance/recipes/`; + no registration is needed. +- A new type-parameter-declaring construct: extend `_is_type_param_call` and `type_param_text` together. +- A new type-parameter-declaring construct: extend `_is_type_param_call` and `type_param_text` together. +- A keyword added by a later Python, or future PEP 695 syntax for variance or a bound on `**P`/`*Ts`: add it to + `_CONVERTIBLE_KEYWORDS` and carry it over in `type_param_text`. + `_CONVERTIBLE_KEYWORDS` and carry it over in `type_param_text`. +- A new version gate uses the syntax's own minimum Python from its PEP, not a value copied from another gate. +- Re-exports through `__init__.py` or namespace packages (PEP 420): extend `resolve_project_module`. +- `Step`/`run_steps` is generic and can be reused by any recipe. +- `Step`/`run_steps` is generic and can be reused by any recipe. + +## Non-goals + +- Make the tool more efficient. Currently the tool does not make use of any filtering or similar features, making it + exponentially slower than other tools (Ruff can run on milliseconds on big projects, while it may take upwards of 30 + minutes per project with this tool). This is a limitation of it, and a low priority fix due to time constraints. + +- The target's minimum Python is not detected (e.g. from `requires-python`); it must be passed with `--py`. A + project with: + + ```toml + [project] + requires-python = ">=3.12" + ``` + + run with `--py 3.11` is treated as below 3.12, so nothing is converted. + +- Re-exports, namespace packages, `import *` and renamed imports are not followed. In: + + ```python + # origin.py + from typing import TypeVar + + T = TypeVar("T") + + # main.py + from origin import T as U + + def first(items: list[U]) -> U: ... + ``` + + `main.py` is left unchanged. + +- Only declarations in the file itself can block a function. A legacy type parameter that stays imported, such as one + from another package, does not. In: + + ```python + from other_package import K + + V = TypeVar("V") + + def get(mapping: dict[K, V], key: K) -> V: ... + ``` + + `get` becomes `def get[V](mapping: dict[K, V], key: K) -> V: ...`. `K` is still the imported legacy type + parameter, so `get` mixes both syntaxes, which + [PEP 695](https://peps.python.org/pep-0695/#compatibility-with-traditional-typevars) forbids. Type checkers + report an error. + +- Imported names are collected once before the run, so removing a declaration whose importers were all localized + needs a second run. With `origin.py` as above and: + + ```python + # main.py + from origin import T + + def first(items: list[T]) -> T: ... + ``` + + the first run turns `main.py` into `def first[T](items: list[T]) -> T: ...`, but keeps `T` in `origin.py`, since + `main.py` imported it when the run started. A second run removes it. + +- Only a plain `__all__ = [...]` is read to detect an exported declaration. In: + + ```python + __all__: list[str] = ["T", "first"] + + T = TypeVar("T") + + def first(items: list[T]) -> T: ... + ``` + + `first` is converted and `T = TypeVar("T")` is removed, although `__all__` exports it. + +- A declaration is only treated as public API when `__all__` lists it. One that nothing in the project uses is + removed, even if other projects import it. In: + + ```python + # mylib/typing.py + from typing import TypeVar + + KeyT = TypeVar("KeyT", bytes, str) + ``` + + `KeyT = TypeVar("KeyT", bytes, str)` is removed, so `from mylib.typing import KeyT` fails elsewhere. With + `__all__ = ["KeyT"]` it is kept. + +- A name used only in the string value of a `TypeAlias` is not seen as a use, so its declaration can be removed as + orphaned. In: + + ```python + T = TypeVar("T") + Pairs: TypeAlias = "list[tuple[T, T]]" + ``` + + `T = TypeVar("T")` is removed, and `Pairs` refers to an undefined `T`. + +- `Unpack[Ts]` → `*Ts` is left to `ruff`'s `UP044`, see [Rejected recipes](rejected-recipes.md). In: + + ```python + Ts = TypeVarTuple("Ts") + + def pack(*args: Unpack[Ts]) -> tuple[Unpack[Ts]]: ... + ``` + + only `[*Ts]` is added: `def pack[*Ts](*args: Unpack[Ts]) -> tuple[Unpack[Ts]]: ...`. + +- An import that follows another statement on the same line is written back wrongly. This is rare, and `ruff`'s + `E702` flags such lines. In: + + ```python + import os; from origin import T + ``` + + the lines that replace the import get an extra indent, a syntax error. + +- Without the CLI's `ruff` pass (`--no-ruff`, or using the recipe as a library) the added imports are not merged + into the file's existing `from` imports and not sorted. They are still placed with the other imports. + +- A declaration still read by a decorator or a parameter default of a converted function can be removed. This is + rare. In: + + ```python + T = TypeVar("T") + + def first(items: list[T], default=T) -> T: ... + ``` + + `first` becomes `def first[T](items: list[T], default=T) -> T: ...`. The annotations now use the new type + parameter, but `default=T` is evaluated outside it and still reads the module-level `T`. libcst places it inside + the type parameter's scope, so the declaration looks unused and is removed: importing the module raises + `NameError`. + +- A declaration followed by `;` on the same line is removed without its `;`. This is rare, and `ruff`'s `E702` flags + such lines. In: + + ```python + T = TypeVar("T"); U = 1 + + def first(items: list[T]) -> T: ... + ``` + + the file is written as `; U = 1`, a syntax error, and reported under errors. + +- Declarations that break the naming rule of [PEP 484](https://peps.python.org/pep-0484/) (a single variable named + after the string argument) are not handled. This is rare, and `ruff`'s `PLC0132` flags a mismatched name: + + ```python + T = U = TypeVar("T") # if T is unused, the statement is removed and U is no longer defined + V = TypeVar("W") # def f(x: V) -> V becomes def f[W](x: V) -> V + X = TypeVar(name) # AttributeError: no phase is applied to the file + ``` + +- A declaration whose constructor comes from `from typing import *` is localized without importing it. This is rare, + and `ruff`'s `F403` flags the star import. In: + + ```python + # origin.py + from typing import * + + T = TypeVar("T") + + # main.py + from origin import T + + class Pair(dict[str, T]): ... + ``` + + `main.py` gets `T = TypeVar("T")` without an import of `TypeVar`, so it raises `NameError`. + +- A `default=` that uses another declaration is carried over as is. This is rare. With `--py 3.13`: + + ```python + T = TypeVar("T") + U = TypeVar("U", default=T) + + def first(items: list[U]) -> U: ... + ``` + + becomes `def first[U = T](items: list[U]) -> U: ...`, whose default still refers to the module-level `T`. + +## Need to fix + +Errors that should be fixed, not accepted. + +- The comment lines above a localized declaration are not copied. Only the comment trailing its line is, because + the rewriter handles a comment as one line + ([issue 189](https://github.com/TNO/Renaissance.Py/issues/189)). In: + + ```python + # origin.py + from typing import TypeVar + + # Placeholder until the array API types are narrower. + T = TypeVar("T") # noqa: PLC0105 + + # main.py + from origin import T + ``` + + `main.py` gets `T = TypeVar("T") # noqa: PLC0105` without the comment above it. The origin keeps its comment + when it drops the declaration. + +- Line endings are not preserved: a modified CRLF file is written back with LF. This causes no problem when the + repository stores LF and git converts on checkout, the usual case, since git commits the same content. A + repository that stores CRLF gets a diff on every line of a modified file, so a pull request may be rejected + ([issue 279](https://github.com/TNO/Renaissance.Py/issues/279)). diff --git a/docs/developer/modules/rejected-recipes.md b/docs/developer/modules/rejected-recipes.md new file mode 100644 index 00000000..c09511d7 --- /dev/null +++ b/docs/developer/modules/rejected-recipes.md @@ -0,0 +1,46 @@ + + +# Rejected recipes + +{ #codemod-rejected-recipes } + +**Stable ID:** `CODEMOD-REJECTED-RECIPES` + +## Responsibility + +Recipes that were built and later removed because an existing tool already covers them. Check that the reason still +holds before rebuilding one. + +## TypeVarTupleCheck (`Unpack[T]` → `*T`) + +{ #codemod-rejected-recipes-typevartuplecheck } + +**What it did:** rewrote `Unpack[Ts]` to `*Ts` ([PEP 646](https://peps.python.org/pep-0646/), Python 3.11+) for a +`TypeVarTuple` declared in the same file. + +**Why it was removed:** + +- `ruff`'s `UP044` covers more cases (`typing.Unpack`, imported `TypeVarTuple`s, `Unpack[tuple[...]]`) and skips + places where `*` is invalid, where the recipe wrote a `SyntaxError`. +- Its only advantage was not needing `--unsafe-fixes`. +- Legacy `Unpack[...]` is rare in real projects. + +**What to use instead**, after [Type parameter modernization](../../user/features/type-parameter-modernization.md) (which converts +the `TypeVarTuple` declaration but leaves `Unpack[Ts]` as it is): + +```shell +ruff check --select UP044 --unsafe-fixes --target-version py311 --fix +``` + +Use `py311` or higher, and review any `Unpack[tuple[...]]` rewrite: unlike `Unpack[Ts]`, it changes the runtime +object. + +## Related features + +- [Type parameter modernization](../../user/features/type-parameter-modernization.md) diff --git a/docs/glossary.md b/docs/glossary.md index fc12a965..4625d6a5 100644 --- a/docs/glossary.md +++ b/docs/glossary.md @@ -36,6 +36,10 @@ proper ancestor. An [insertion](#insertion) that adds text immediately after the end location of an AST node. See [Rewrite semantics](user/concepts/rewrite-semantics.md). +### Around + +See [Surround](#surround). + ### AST (Abstract Syntax Tree) A hierarchical representation of source code produced by a compiler front-end, @@ -264,3 +268,21 @@ See [Transformation](user/concepts/transformation.md). ### Trivia White spaces and comments. + +### Type hint + +An annotation that states the expected type of a variable, parameter or return value, such as `x: list[int]`. +Type checkers read it; Python does not enforce it at runtime. +See [Type parameters](user/concepts/type-parameters.md). + +### Type parameter + +A name in a type hint that stands for a type chosen later, declared on a generic function, class or type alias. +Three kinds exist: [type variable](#type-variable), type variable tuple (`*Ts`) and parameter specification (`**P`). +See [Type parameters](user/concepts/type-parameters.md). + +### Type variable + +The kind of [type parameter](#type-parameter) that stands for a single type. +Also called *TypeVar*, after the legacy class that declares it (`T = TypeVar("T")`). +See [Type parameters](user/concepts/type-parameters.md). diff --git a/docs/user/concepts/index.md b/docs/user/concepts/index.md index a5707565..53b9cd78 100644 --- a/docs/user/concepts/index.md +++ b/docs/user/concepts/index.md @@ -34,3 +34,7 @@ This section introduces the conceptual model of the repository. ## 5. Reusable libraries 1. [Standard analyses and transformations](standard-libraries.md) + +## 6. Python typing terms + +1. [Type parameters](type-parameters.md) diff --git a/docs/user/concepts/type-parameters.md b/docs/user/concepts/type-parameters.md new file mode 100644 index 00000000..1644902d --- /dev/null +++ b/docs/user/concepts/type-parameters.md @@ -0,0 +1,72 @@ + + +# Type parameters + +{ #concept-type-parameters } + +**Stable ID:** `CONCEPT-TYPE-PARAMETERS` + +## Purpose + +Fixes the meaning of three terms that are easy to confuse: *type hint*, *type parameter* and *type variable*. + +## Scope + +The terms as used by [Type parameter modernization](../features/type-parameter-modernization.md). This page does not describe +how the tool rewrites them. + +## Definition + +A **type hint** states the type of a value. A **type parameter** is a type left open in a generic function, class or +type alias. A **type variable** is the kind of type parameter that stands for one type. + +```python +def first[T](items: list[T]) -> T: ... +``` + +| Part of the code | Term | Role | +| --------------------- | -------------- | ----------------------------------- | +| ``[T]`` | Type parameter | Declares ``T``, a type left open | +| ``items: list[T]`` | Type hint | States the type of ``items`` | +| ``T`` after the arrow | Type hint | States the type of the return value | + +Here `T` is a type variable. + +Before Python 3.12, `T` had to be declared separately. The legacy form still works on every version: + +```python +T = TypeVar("T") + +def first(items: list[T]) -> T: ... +``` + +Three kinds of type parameter exist: + +| Kind of type parameter | Written as | Legacy declaration | +| ----------------------- | ---------- | ------------------ | +| Type variable | ``T`` | ``TypeVar`` | +| Type variable tuple | ``*Ts`` | ``TypeVarTuple`` | +| Parameter specification | ``**P`` | ``ParamSpec`` | + +## Invariants / guarantees + +1. A type hint is never a type parameter: `list[T]` is a type hint, `T` is the type parameter it uses. +2. The legacy and the PEP 695 declaration define the same kind of type parameter. Only the place where it is declared + differs, and the inline form needs Python 3.12. + +## Related features + +- [Type parameter modernization](../features/type-parameter-modernization.md) + +## Notes + +- A PEP 695 type parameter is also a `TypeVar`, `TypeVarTuple` or `ParamSpec` object at runtime (see + `__type_params__`). This is why "TypeVar" is often used for all three kinds. +- The documentation says *type parameter* for the general case and *type variable* only for the single-type kind. +- The recipe class is still named `TypeVarCheck`. diff --git a/docs/user/features/index.md b/docs/user/features/index.md index 4fd36548..5925ff0d 100644 --- a/docs/user/features/index.md +++ b/docs/user/features/index.md @@ -29,3 +29,7 @@ This section documents the main user-visible features of the repository. ## 4. Reporting 1. [Observability and reporting](observability-and-reporting.md) + +## 5. Code modernization + +1. [Type parameter modernization](type-parameter-modernization.md) diff --git a/docs/user/features/rewrite-semantics.md b/docs/user/features/rewrite-semantics.md index 5abf1d4e..fca28281 100644 --- a/docs/user/features/rewrite-semantics.md +++ b/docs/user/features/rewrite-semantics.md @@ -26,7 +26,7 @@ which produce errors, so that transformation authors can reason about the outcom * [Rewrite semantics test module](../../developer/modules/rewrite-semantics.md) * BDD scenarios: `features/rewrite-semantics.feature` -* BDD steps: `features/steps/test-rewrite-semantics.py` +* BDD steps: `features/steps/test_rewrite_semantics.py` ## Corner case: Dominated, overlapping replacements diff --git a/docs/user/features/type-parameter-modernization.md b/docs/user/features/type-parameter-modernization.md new file mode 100644 index 00000000..39f99aff --- /dev/null +++ b/docs/user/features/type-parameter-modernization.md @@ -0,0 +1,365 @@ + + +# Type parameter modernization { #feature-type-parameter-modernization } + +**Stable ID:** `FEATURE-TYPE-PARAMETER-MODERNIZATION` + +## User-facing summary + +Rewrites legacy `TypeVar`, `ParamSpec` and `TypeVarTuple` declarations to [PEP 695](https://peps.python.org/pep-0695/) +syntax in one command. `ruff`'s `UP047` only does part of this, as an unsafe fix: it skips type parameters imported +from another module, `default=` and nested functions, and never removes the old declaration. + +```python +# before +T = TypeVar("T") + +def first(items: list[T]) -> T: ... + +# after +def first[T](items: list[T]) -> T: ... +``` + +The tool runs three phases: + +1. **Localize cross-file imports.** A type parameter imported from another file in the project + (`from .shapes import T`) is replaced by an exact local copy of its declaration, placed after the file's imports. +2. **Convert to PEP 695.** Every function whose signature uses a declared type parameter gets its own `[T]`, with + bounds, constraints and `default=` carried over (`def f[T: str = str](...)`). A nested function gets none when an + enclosing function's signature uses `T` too. If `T` is also used outside functions (a `Generic[T]` base, an alias), + the declaration is kept. A function that also uses a type parameter that can't be + converted is left unchanged. +3. **Remove orphaned declarations.** A module-level `T = TypeVar("T")` that nothing uses is deleted, including one left + behind by `ruff`'s `UP047` or by a manual rewrite. + +## Inputs + +A Python file or directory, and the target project's minimum supported Python version (`--py`). Supported +declarations: `TypeVar` (with `bound=`, constraints or `default=`), `ParamSpec` and `TypeVarTuple` (with `default=`). + +Imports are resolved against the directory passed to the CLI (the file's own folder for a single file). These are +skipped without a report: + +- imports from outside that directory (stdlib, third-party); +- re-exports through an intermediate `__init__.py`, and namespace packages; +- a renamed import (`from .shapes import T as U`); a file whose only type parameters arrive that way counts as clean. + +### Generic classes { #feature-type-parameter-modernization-generic-class } + +A class is *generic over* `T` when it declares `T` itself (`class Box[T]:`) or has `T` in a base (`Generic[T]`, +`Protocol[T]`, `Base[int, T]`). Inside such a class, `T` belongs to the class, so the tool never adds `[T]` to its +methods. With `class Box[T]:` the methods don't use the module-level `T`, so a declaration nothing else uses is removed: + +```python +# before +T = TypeVar("T") + +class Box[T]: + def get(self) -> T: ... + +# after +class Box[T]: + def get(self) -> T: ... +``` + +## Outputs / effects + +- Every safe change is written to the file in place. +- Every candidate left unchanged for one of the reasons under *Constraints* is listed in the report as **needs manual + review**, with a link to the section that explains it. +- A file listed under `ERRORS` may already be changed: each phase is written before the next one runs, so a failure + keeps the earlier phases. Check it with `git diff`. + +### Known limitations + +None of these is a *Constraint*: the tool applies the change. Review the result with `git diff`. Most of them have an +example under *Non-goals* or *Need to fix* in [Refactoring recipes](../../developer/modules/recipes.md#non-goals). + +**Can leave code that fails or changes meaning:** + +- A decorator or a parameter default that reads `T` (`def f(x: list[T], default=T)`) is not counted as a use, so the + declaration can be removed and importing the module raises `NameError`. +- A declaration constructed from `from typing import *` is localized without importing `TypeVar`: `NameError`. +- A declaration followed by `;` on the same line is removed without its `;`, leaving invalid syntax. The file is + reported under `ERRORS`. +- A declaration that breaks the naming rule of PEP 484 (`T = U = TypeVar("T")`, `V = TypeVar("W")`, + `TypeVar(name)`) gets a wrong rewrite or is skipped with an error. +- `U = TypeVar("U", default=T)` becomes `def f[U = T]`, where `T` still means the module-level one. Fix it by hand as + `def f[T, U = T]`. +- Only `__all__` marks a declaration as public. One that nothing in the project uses is removed, even if other + projects import it. +- Only a plain `__all__ = [...]` is read. Annotated (`__all__: list[str] = ...`), `+=`, `.extend()` and `.append()` + forms are not. +- A `T` used only in the string value of a `TypeAlias` (`A: TypeAlias = "list[T]"`) is seen as unused, so its + declaration is removed. +- A function that uses a legacy type parameter that stays imported (e.g. from another package) still gets `[...]` + for the file's own declarations. It then mixes both syntaxes, which type checkers reject. +- A localized `T` is a new object: code comparing type parameters by identity (`is`) sees two different objects. + A file importing `T` from a localized file gets that copy too. + +**Cosmetic or operational:** + +- A declaration whose importers are all localized in a run is only removed by a second run. +- Removing a line can leave extra blank lines. Run your formatter afterwards. +- With `--no-ruff`, the imports added by localizing are not merged into the file's existing `from` imports and not + sorted. +- A modified CRLF file is written back with LF. +- The comment lines above a localized declaration are not copied. The comment trailing its line, such as + `# noqa: PLC0105`, is. +- `Unpack[Ts]` is left as it is. Run `ruff`'s `UP044` to rewrite it to `*Ts` (see + [Rejected recipes](../../developer/modules/rejected-recipes.md)). +- There is no filtering of files, so a large project can take tens of minutes. + +## Constraints + +Each case below stops one change and is reported as **unsafe** under *needs manual review*, with a link to its section. + +### Python version gates { #feature-type-parameter-modernization-version-gates } + +PEP 695 syntax is a `SyntaxError` on older Pythons, so the tool only writes it when `--py` (the oldest version the +target supports, not the one running the tool) is new enough. The version is never guessed: unknown counts as too +old. Removing a declaration that is already unused needs no new syntax, so it never depends on `--py`. + +| Syntax | PEP | Minimum Python | +| --------------------- | -------------------------------------------- | -------------- | +| `def f[T](...)` | [PEP 695](https://peps.python.org/pep-0695/) | 3.12 | +| `def f[T = int](...)` | [PEP 696](https://peps.python.org/pep-0696/) | 3.13 | + +#### PEP 695 version gate { #feature-type-parameter-modernization-pep695-version-gate } + +Below 3.12 nothing is converted. Localization and orphan removal still run. + +```python +T = TypeVar("T") + +def first(items: list[T]) -> T: ... +``` + +With `--py 3.11` the file is unchanged. With `--py 3.12` it becomes `def first[T](items: list[T]) -> T: ...` and the +declaration is removed. + +**To fix this yourself:** if the project supports 3.12+, run again with `--py 3.12`. Otherwise keep the old syntax. + +#### PEP 696 version gate { #feature-type-parameter-modernization-pep696-version-gate } + +Below 3.13 a declaration with `default=` is not converted, since dropping the default would change what type +checkers infer. Other declarations in the file are still converted, except in functions that also use it (see +*A function also uses a type variable that can't be converted* below). + +```python +T = TypeVar("T", default=int) + +def first(items: list[T]) -> T: ... +``` + +With `--py 3.12` the file is unchanged. With `--py 3.13` it becomes `def first[T = int](items: list[T]) -> T: ...`. + +**To fix this yourself:** run again with `--py 3.13` if the project supports it. Otherwise keep the declaration. + +### A declaration passes an argument with no PEP 695 equivalent { #feature-type-parameter-modernization-no-pep695-equivalent } + +The `[...]` syntax cannot write `covariant=`/`contravariant=`, a `bound=` on a `ParamSpec` or `TypeVarTuple`, a +`**mapping` of options, or a keyword added by a later Python version. (`infer_variance=True` is fine: a PEP 695 +type parameter always infers its variance.) + +```python +T_co = TypeVar("T_co", covariant=True) + +def first(items: Sequence[T_co]) -> T_co: ... +``` + +The file is unchanged. + +**To fix this yourself:** variance only matters for generic classes. If only functions use the type parameter, +remove `covariant=`/`contravariant=` and run again. + +### A function also uses a type variable that can't be converted { #feature-type-parameter-modernization-shares-function-with-unconvertible } + +[PEP 695](https://peps.python.org/pep-0695/#compatibility-with-traditional-typevars) forbids a function from mixing +`[...]` type parameters with legacy ones of its own. A function that uses a +declaration refused for a version gate or an argument with no PEP 695 equivalent keeps all its type parameters +legacy. A legacy type parameter of an enclosing generic class is not the method's own, so it blocks nothing. + +```python +T_co = TypeVar("T_co", covariant=True) +Ts = TypeVarTuple("Ts") + +def run(func: Callable[[*Ts], T_co], *args: *Ts) -> T_co: ... +``` + +The file is unchanged. `T_co` is reported for its own reason. `Ts` is reported for this one, because every function +using it is left unchanged. Functions that use `Ts` without `T_co` are still converted. + +**To fix this yourself:** fix the refused declaration as its section describes, then run again. + +### A declared type parameter is exported via `__all__` { #feature-type-parameter-modernization-declared-type-parameter-exported } + +`T` is in its file's plain `__all__ = [...]`, so it is public API. The functions are converted, the declaration is kept. + +```python +__all__ = ["T", "first"] + +T = TypeVar("T") + +def first(items: list[T]) -> T: ... +``` + +becomes: + +```python +__all__ = ["T", "first"] + +T = TypeVar("T") + +def first[T](items: list[T]) -> T: ... +``` + +**To remove it yourself:** remove `T` from `__all__` (a breaking change for importers), then delete the declaration. + +### A declared type parameter is only used inside a generic class { #feature-type-parameter-modernization-used-in-generic-class } + +Only methods of a class with `T` in a base (see *Generic classes* above) use `T`, so there is nothing to +convert. Standalone functions that also use `T` are still converted. + +```python +T = TypeVar("T") + +class Box(Generic[T]): + def get(self) -> T: ... +``` + +The file is unchanged. + +**To fix this yourself:** `ruff`'s `UP046` rewrites `Generic[T]` to `class Box[T]:`. Run the tool again afterwards: +it removes the now unused `T = TypeVar("T")`. + +### A name the declaration uses can't be imported here { #feature-type-parameter-modernization-declaration-name-unavailable } + +Localizing copies the declaration and imports the names it uses (`bound=Shape`) from the module the origin gets them +from, or from the origin when it defines them. That fails when the origin only imports `Shape` under +`if TYPE_CHECKING:`, or when this file already has a different `Shape`. + +```python +# shapes.py +from typing import TYPE_CHECKING, TypeVar + +if TYPE_CHECKING: + from geometry import Shape + +T = TypeVar("T", bound="Shape") + +# main.py +from shapes import T + +def biggest(items: list[T]) -> T: ... +``` + +`main.py` is unchanged and keeps importing `T` from `shapes.py`. + +**To fix this yourself:** import the name the same way the origin does, or rename one of the two, then copy the +declaration by hand. + +### The origin imports `TypeVar` conditionally { #feature-type-parameter-modernization-origin-imports-constructor-conditionally } + +The origin imports `TypeVar` inside an `if` or `try` (for example `typing` or `typing_extensions` per Python +version). A copy would use whatever `TypeVar` this file has, which may fail on some versions. + +```python +# shapes.py +try: + from typing import TypeVar +except ImportError: + from typing_extensions import TypeVar + +T = TypeVar("T") + +# main.py +from shapes import T + +def first(items: list[T]) -> T: ... +``` + +`main.py` is unchanged and keeps importing `T` from `shapes.py`. + +**To fix this yourself:** add the same conditional import to this file, then copy the declaration by hand. + +### A declared type parameter is imported by another project file { #feature-type-parameter-modernization-imported-elsewhere-in-project } + +Another file in the target directory imports `T` from this module (`from mod import T` or `mod.T`). The functions +are converted, the declaration is kept. Only checked when a directory is passed; `import *` is not checked. + +```python +# shapes.py +from typing import TypeVar + +T = TypeVar("T") + +def first(items: list[T]) -> T: ... + +# other.py +import shapes + +def second(items: list[shapes.T]) -> shapes.T: ... +``` + +`first` becomes `def first[T](items: list[T]) -> T: ...`. `T = TypeVar("T")` stays, because `other.py` reads +`shapes.T`. + +**To remove it yourself:** find that import (`from import ` or `.`), remove it, and +run the tool again. + +## Related concepts + +- [Rewrite semantics](../concepts/rewrite-semantics.md) +- [Type parameters](../concepts/type-parameters.md) + +## Verified by test modules + +- `test/recipes/test_type_var_check.py` +- `test/recipes/test_type_var_check_convert.py` +- `test/recipes/test_type_var_check_localize.py` +- `test/recipes/test_type_var_check_orphaned.py` +- `test/recipes/test_type_var_check_properties.py` +- `test/recipes/test_type_var_domain.py` +- `test/recipes/test_step_runner.py` +- `test/recipes/test_python_refactoring.py` +- `test/utils/test_import_resolution.py` +- `test/rejuvenation/test_migration_type_recipes.py` + +## Implemented by code modules + +- [Refactoring recipes](../../developer/modules/recipes.md) +- `src/renaissance/recipes/type_var_check.py`, `type_var_domain.py`, `step_runner.py` and `python_refactoring.py`. + The recipe class is still named `TypeVarCheck`. +- `src/renaissance/utils/import_resolution.py` +- `src/rejuvenation/migration-type-recipes.py` (the CLI) + +## API entry points + +```shell +python src/rejuvenation/migration-type-recipes.py --py MAJOR.MINOR [--report PATH] [--no-ruff] +``` + +- ``: a `.py` file or a directory, scanned recursively (`.git`, `__pycache__`, `.venv` and `venv` skipped). +- `--py` (required): the oldest Python version the target project supports. +- `--report`: also write the report to a file. +- `--no-ruff`: skip the final `ruff` pass. + +After the recipe, the CLI runs `ruff check --fix --select F401,I001` on the changed files. This removes **every** +unused import in them, including ones that were unused before (such as `from typing import TypeVar` after the +declaration is gone). It also merges and sorts **every** import block in them, including imports that were unsorted +before, using the `ruff` settings of the project. `ruff` must be installed in the same environment; if it cannot run, +the report still says the pass ran. Files are changed directly, so run it on a git checkout. + +The report has a summary line and the sections `MODIFIED`, `NEEDS MANUAL REVIEW` and `ERRORS`. Exit codes: 0 when +the run completes (manual review is not a failure), 2 for a usage error, 3 if any file raised an exception. + +## Change considerations + +See *Extension points* in [Refactoring recipes](../../developer/modules/recipes.md). diff --git a/mkdocs.yml b/mkdocs.yml index 0da4c06f..da211cfb 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -125,6 +125,7 @@ nav: - 8. Transformation: user/concepts/transformation.md - 9. Composition: user/concepts/composition.md - 10. Standard analyses and transformations: user/concepts/standard-libraries.md + - 11. Type parameters: user/concepts/type-parameters.md - Features: - Overview: user/features/index.md @@ -140,6 +141,7 @@ nav: - 9. Find: user/features/find.md - 10. Modify: user/features/modify.md - 11. Rewrite semantics: user/features/rewrite-semantics.md + - 12. Type parameter modernization: user/features/type-parameter-modernization.md - Workflows: - Overview: user/workflows/index.md @@ -205,6 +207,8 @@ nav: - Observability modules: developer/modules/observability.md - Strategy modules: developer/modules/strategy.md - Rewrite semantics module: developer/modules/rewrite-semantics.md + - Refactoring recipes: developer/modules/recipes.md + - Rejected recipes: developer/modules/rejected-recipes.md - API reference: - Overview: developer/api/index.md @@ -242,4 +246,4 @@ nav: - Linking policy enforcement: documentation-system/enforcement/linking-policy-enforcement.md - CI overview: documentation-system/enforcement/ci-overview.md - - Shared images: assets/images/README.md \ No newline at end of file + - Shared images: assets/images/README.md diff --git a/pyproject.toml b/pyproject.toml index 9717829e..93f00623 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -37,6 +37,7 @@ dependencies = [ "hypothesmith>=0.3.3", "autopep8>=2.0", "flake8>=7.0", + "packaging>=24.0", ] [dependency-groups] @@ -120,6 +121,7 @@ select = [ "ANN002", "ANN003", "ANN204", "ANN205", "ANN401", ] #select = ["ALL"] # goal of issue + ignore = [ "D203", # conflicts with D211 (no blank line before class) — keep D211 "D213", # conflicts with D212 (summary on the first line) — keep D212 @@ -147,6 +149,7 @@ exclude = ["features/targets", "**/node_modules", "**/__pycache__", "**/.*", ".v extraPaths = ["src", "test", "features"] [tool.pymarkdown] +plugins.md013.heading_line_length = 140 plugins.md013.line_length = 140 plugins.md013.tables = false plugins.md013.code_blocks = false @@ -154,4 +157,3 @@ plugins.md013.code_blocks = false plugins.md007.enabled = false plugins.pml101.enabled = true extensions.markdown-tables.enabled = true - diff --git a/src/rejuvenation/migration-type-recipes.py b/src/rejuvenation/migration-type-recipes.py new file mode 100644 index 00000000..6837eb9c --- /dev/null +++ b/src/rejuvenation/migration-type-recipes.py @@ -0,0 +1,295 @@ +# Copyright (C) 2024-2026 TNO +# +# Licensed under the terms specified in the LICENSE file in the root directory +# of this repository, also available at: +# https://github.com/TNO/Renaissance.Py/blob/main/LICENSE + +"""CLI that modernizes type parameters (TypeVar, ParamSpec, TypeVarTuple) to PEP 695 syntax. + +Runs TypeVarCheck on a file or directory, and reports which files were modified and which need +manual review. + +Examples: + python src/rejuvenation/migration-type-recipes.py ./some_repo --py 3.12 --report review.md + python src/rejuvenation/migration-type-recipes.py ./some_repo/file.py --py 3.10 + python src/rejuvenation/migration-type-recipes.py ./some_repo --py 3.12 --no-ruff + +""" + +# Printing the report is this CLI's purpose. +# ruff: noqa: T201 + +import argparse +import subprocess +import sys +import textwrap +from collections.abc import Sequence +from dataclasses import dataclass +from pathlib import Path + +from termcolor import colored + +from renaissance.project.project_scanner import PythonScanner +from renaissance.recipes.type_var_check import TypeVarCheck +from renaissance.recipes.type_var_domain import UNSAFE_RULES, UnsafeReason, doc_link +from renaissance.utils.import_resolution import collect_project_imported_names + +_MAJOR_MINOR_PART_COUNT = 2 + + +@dataclass +class FileReport: + """Outcome of running TypeVarCheck against a single file.""" + + path: Path + result: dict[str, dict[str, str]] | None + error: str | None + reasons: dict[str, dict[str, UnsafeReason]] | None = None + + +def resolve_target_files(target: Path) -> list[Path]: + """Return the .py files to process for `target`. + + A single .py file is returned as-is; a directory is scanned recursively with PythonScanner over + the whole tree (no package_dirs allowlist), so any project layout works. + """ + if target.is_file(): + return [target] + return [Path(path) for path in PythonScanner(str(target)).find_sources()] + + +def _parse_py_version(text: str) -> tuple[int, int]: + """Parse a "MAJOR.MINOR" string into a (major, minor) tuple for argparse's type=. + + Raises: + argparse.ArgumentTypeError: If text is not in MAJOR.MINOR form. + + """ + parts = text.split(".") + if len(parts) != _MAJOR_MINOR_PART_COUNT or not all(part.isdigit() for part in parts): + message = f"expected MAJOR.MINOR (e.g. 3.12), got {text!r}" + raise argparse.ArgumentTypeError(message) + return (int(parts[0]), int(parts[1])) + + +def has_fixed(report: FileReport) -> bool: + """Return True if any phase of report.result fixed at least one name.""" + if report.result is None: + return False + return any("fixed" in phase.values() for phase in report.result.values()) + + +def has_unsafe(report: FileReport) -> bool: + """Return True if any phase of report.result left at least one name unsafe to touch.""" + if report.result is None: + return False + return any("unsafe" in phase.values() for phase in report.result.values()) + + +def is_clean(report: FileReport) -> bool: + """Return True if report.result found no type parameter usage at all.""" + if report.result is None: + return False + return not any(phase for phase in report.result.values()) + + +def process_file( + path: Path, + *, + min_python: tuple[int, int], + project_root: Path, + project_wide_imported_names: frozenset[str], +) -> FileReport: + """Run TypeVarCheck's three phases on one file and return its FileReport. + + Any exception is caught and stored on FileReport.error, so one bad file never aborts a batch run. + """ + try: + recipe = TypeVarCheck(path) + recipe.min_python = min_python + recipe.project_root = project_root + recipe.project_wide_imported_names = project_wide_imported_names + result = recipe.check() + reasons = { + "cross_file": recipe.cross_file_unsafe_reasons, + "converted": recipe.converted_unsafe_reasons, + "orphaned": recipe.orphaned_unsafe_reasons, + } + except Exception as exc: # noqa: BLE001 - isolate one bad file, never abort the whole batch + return FileReport(path=path, result=None, error=f"{type(exc).__name__}: {exc}") + return FileReport(path=path, result=result, error=None, reasons=reasons) + + +def _run_ruff_unused_import_cleanup(paths: list[Path]) -> None: + """Run `ruff check --fix --select F401` over every path, dropping any now-unused import. + + Best-effort: if ruff can't be invoked (e.g. not installed), prints a warning instead of raising. + The files are already valid without this cleanup. + """ + try: + subprocess.run( # noqa: S603 - fixed argv list (sys.executable + literals + our own discovered paths), no shell + [sys.executable, "-m", "ruff", "check", "--fix", "--select", "F401", *(str(path) for path in paths)], + capture_output=True, + text=True, + check=False, + ) + except OSError as exc: + print(colored(f"warning: could not run ruff for import cleanup: {exc}", "yellow")) + + +def _format_commit_summary(reports: list[FileReport]) -> str: + """Build the short, copy-pasteable commit-message-style summary.""" + modified = sum(1 for report in reports if has_fixed(report)) + needs_review = sum(1 for report in reports if has_unsafe(report)) + clean = sum(1 for report in reports if is_clean(report)) + errors = sum(1 for report in reports if report.error is not None) + return ( + "Modernize TypeVar/ParamSpec/TypeVarTuple usage to PEP 695 syntax\n\n" + f"{modified} files modified, {needs_review} need manual review, {clean} clean, " + f"{errors} errors (of {len(reports)} processed)" + ) + + +def _format_console_report(reports: list[FileReport], *, ruff_ran: bool) -> str: + """Build the full per-file report: MODIFIED / NEEDS MANUAL REVIEW / ERRORS sections. + + Clean files (no type parameter usage at all) are only counted, not listed, so the report shows + what needs attention. Under MODIFIED, a converted name isn't repeated as orphaned. The ruff + import-cleanup line is only included when ruff_ran is True. + """ + modified = [report for report in reports if has_fixed(report)] + needs_review = [report for report in reports if has_unsafe(report)] + errors = [report for report in reports if report.error is not None] + clean_count = sum(1 for report in reports if is_clean(report)) + + lines = [ + "Renaissance TypeVarCheck migration report", + ( + f"Processed {len(reports)} files: {len(modified)} modified, {len(needs_review)} need " + f"manual review, {clean_count} clean, {len(errors)} errors" + ), + ] + if ruff_ran: + lines.append( + "Unused imports across the modified files above were also cleaned up via `ruff check --fix --select F401`.", + ) + lines.extend(["", f"MODIFIED ({len(modified)})"]) + for report in modified: + lines.append(f" {report.path}") + result = report.result or {} + converted = {name for name, status in result.get("converted", {}).items() if status == "fixed"} + for phase, names in result.items(): + # A converted name's declaration is always removed afterwards; listing it under orphaned too would repeat it. + fixed = [name for name, status in names.items() if status == "fixed" and not (phase == "orphaned" and name in converted)] + if fixed: + lines.append(f" {phase}: {', '.join(fixed)}") + + lines.extend(["", f"NEEDS MANUAL REVIEW ({len(needs_review)})"]) + for report in needs_review: + lines.append(f" {report.path}") + for phase, names in (report.result or {}).items(): + phase_reasons = (report.reasons or {}).get(phase, {}) + unsafe = [name for name, status in names.items() if status == "unsafe"] + if unsafe: + lines.append(f" {phase}: {', '.join(unsafe)}") + for name in unsafe: + reason = phase_reasons.get(name) + if reason is not None: + lines.append(f" {name}: {UNSAFE_RULES[reason].message} -> {doc_link(reason)}") + + lines.extend(["", f"ERRORS ({len(errors)})"]) + lines.extend(f" {report.path}: {report.error}" for report in errors) + + return "\n".join(lines) + + +def build_arg_parser() -> argparse.ArgumentParser: + """Build the argument parser for this CLI's --help/usage text and flags.""" + parser = argparse.ArgumentParser( + prog="migration-type-recipes.py", + description="Modernize legacy TypeVar/ParamSpec/TypeVarTuple usage to PEP 695 syntax.", + epilog=textwrap.dedent("""\ + Examples: + python src/rejuvenation/migration-type-recipes.py ./some_repo --py 3.12 --report review.md + python src/rejuvenation/migration-type-recipes.py ./some_repo/file.py --py 3.10 + python src/rejuvenation/migration-type-recipes.py ./some_repo --py 3.12 --no-ruff + """), + formatter_class=argparse.RawDescriptionHelpFormatter, + ) + parser.add_argument("path", type=Path, help="A .py file or a directory to scan.") + parser.add_argument( + "--py", + type=_parse_py_version, + required=True, + metavar="MAJOR.MINOR", + help=( + "Minimum Python version the target project supports (not the one running this tool), e.g. 3.12. " + "PEP 695 rewrites need 3.12+, PEP 696 type parameter defaults 3.13+." + ), + ) + parser.add_argument("--report", type=Path, metavar="PATH", help="Also write the full report to this file.") + parser.add_argument( + "--no-ruff", + action="store_true", + help="Skip the final `ruff check --fix --select F401` pass that drops imports made unused.", + ) + return parser + + +def main(argv: Sequence[str] | None = None) -> int: + """Parse arguments, run TypeVarCheck across the target, print/save the report, return an exit code. + + Exit codes: 0 on normal completion (files needing manual review are informational, not a + failure), 2 on a usage error (bad path/argument), 3 if any file hit an unhandled exception. + """ + parser = build_arg_parser() + args = parser.parse_args(argv) + + target: Path = args.path + if not target.exists(): + parser.error(f"path does not exist: {target}") + if target.is_file() and target.suffix != ".py": + parser.error(f"not a Python file: {target}") + + # Absolute and normalized, so file paths match the keys collect_project_imported_names returns. + target = target.resolve() + files = resolve_target_files(target) + project_root = target if target.is_dir() else target.parent + # TODO(FrancescoPezzella): computed once upfront, so an origin whose importers all get localized this run is only converted + # on a second run. + # https://github.com/TNO/Renaissance.Py/issues/279 + imported_names_by_file = collect_project_imported_names(files, project_root) + + reports: list[FileReport] = [] + for path in files: + project_wide_imported_names = imported_names_by_file.get(path, frozenset()) + report = process_file( + path, + min_python=args.py, + project_root=project_root, + project_wide_imported_names=project_wide_imported_names, + ) + reports.append(report) + print(f"File {path} checked.") + + modified_paths = [report.path for report in reports if has_fixed(report)] + ruff_ran = bool(modified_paths) and not args.no_ruff + if ruff_ran: + # TODO(FrancescoPezzella): removed statements leave their blank lines behind; ruff's E303 (preview) collapses them, but not + # at file start. + # https://github.com/TNO/Renaissance.Py/issues/279 + _run_ruff_unused_import_cleanup(modified_paths) + + console_report = _format_console_report(reports, ruff_ran=ruff_ran) + print(console_report) + print() + print(colored(_format_commit_summary(reports), "green", attrs=["bold"])) + + if args.report is not None: + args.report.write_text(console_report, encoding="utf-8") + + return 3 if any(report.error is not None for report in reports) else 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/src/renaissance/integrations/clang/clang_ast_node.py b/src/renaissance/integrations/clang/clang_ast_node.py index a93650a2..4f6e49a8 100644 --- a/src/renaissance/integrations/clang/clang_ast_node.py +++ b/src/renaissance/integrations/clang/clang_ast_node.py @@ -376,7 +376,7 @@ def is_statement(self) -> bool: @override @property - def referenced_by(self) -> Sequence[ASTReference[Cursor, ClangTranslationUnit]]: + def referenced_by(self) -> Sequence[ASTReference[ClangASTNode]]: self.translation_unit.lazy_create_references(self) node_id = self.node.hash ref_by = self.translation_unit._referenced_by.get(node_id, EMPTY_LIST) @@ -428,7 +428,7 @@ def is_match(node): @override @property - def references(self) -> Sequence[ASTReference[Cursor, ClangTranslationUnit]]: + def references(self) -> Sequence[ASTReference[ClangASTNode]]: self.translation_unit.lazy_create_references(self) return list( ASTReference( diff --git a/src/renaissance/integrations/clang/clang_json_ast_node.py b/src/renaissance/integrations/clang/clang_json_ast_node.py index 8ffdca85..0d2cf4b0 100644 --- a/src/renaissance/integrations/clang/clang_json_ast_node.py +++ b/src/renaissance/integrations/clang/clang_json_ast_node.py @@ -390,7 +390,7 @@ def properties(self) -> dict[str, Any]: @override @property - def referenced_by(self) -> Sequence[ASTReference[dict[str, Any], ClangJsonTranslationUnit]]: + def referenced_by(self) -> Sequence[ASTReference[ClangJsonASTNode]]: if self.inserted: return [] self.translation_unit.lazy_create_references(self) @@ -414,7 +414,7 @@ def _get_function_definition(self): @override @property - def references(self) -> list[ASTReference[dict[str, Any], ClangJsonTranslationUnit]]: + def references(self) -> list[ASTReference[ClangJsonASTNode]]: if self.inserted: return [] self.translation_unit.lazy_create_references(self) diff --git a/src/renaissance/integrations/python/ast/cst_node.py b/src/renaissance/integrations/python/ast/cst_node.py index b41e93c0..cc240b0d 100644 --- a/src/renaissance/integrations/python/ast/cst_node.py +++ b/src/renaissance/integrations/python/ast/cst_node.py @@ -6,42 +6,127 @@ """AI: ASTNode implementation backed by libcst's concrete syntax tree.""" +import io +import itertools +import sys +from collections import defaultdict from pathlib import Path -from typing import Self import libcst from libcst import BaseCompoundStatement, BaseSmallStatement, ClassDef, CSTNode, FunctionDef, MetadataWrapper -from libcst.metadata import WhitespaceInclusivePositionProvider +from libcst.metadata import Access, Assignment, CodePosition, PositionProvider, ScopeProvider from renaissance.integrations.python.ast.kinds import PYTHON_KIND_MAP -from renaissance.integrations.python.ast.util import convert +from renaissance.syntax_tree.ast_node import ASTReference from renaissance.syntax_tree.semantic_kind import SemanticKind from renaissance.utils.ast_utils import next_sibling, preceding_sibling +type _Link = tuple[CSTNode, str, Access] + class PythonCstTranslationUnit: """AI: Parse Python source into a libcst tree with position lookups for AST-node wrapping.""" def __init__(self, content, file_name: str) -> None: """AI: Parse Python source into a libcst tree with position lookups for AST-node wrapping.""" - # TODO: see docs/developer/architecture/position-consistency.md - exposes no buffer, so the rewriter derives its own. + # TODO(pjljvandelaar): see docs/developer/architecture/position-consistency.md - exposes no buffer, so the rewriter + # derives its own. + # https://github.com/TNO/Renaissance.Py/issues/153 + # TODO(FrancescoPezzella): content is a str while offset/end_offset count bytes of self.encoding, so never slice + # content with them. self.encoding is the filesystem encoding, not the file's (#153, #188). + # https://github.com/TNO/Renaissance.Py/issues/188 self.content = content - self.lines = content.splitlines() + self.encoding = sys.getfilesystemencoding() + # newline="" splits on \n, \r\n and \r only, like libcst, and keeps each line ending. + self._lines = io.StringIO(content, newline="").readlines() + self._line_starts = list(itertools.accumulate((len(line.encode(self.encoding)) for line in self._lines), initial=0)) self.file_name = file_name - self.references_initialized = False self.wrapper = MetadataWrapper(libcst.parse_module(content)) self.atu = self.wrapper.module - self.spans = self.wrapper.resolve(WhitespaceInclusivePositionProvider) + self.spans = self.wrapper.resolve(PositionProvider) + self.wrappers: dict[CSTNode, PythonCstNode] = {} + self._uses_of: dict[CSTNode, list[_Link]] | None = None + self._definitions_of: dict[CSTNode, list[_Link]] = {} + + def _index_references(self) -> dict[CSTNode, list[_Link]]: + """Resolve libcst's scopes once, linking every definition to its uses; return the definition-to-uses map. + + Also fills self._definitions_of, the use-to-definitions map. Builtins, which have no defining node, are skipped. + An annotation or type hint above a definition in the same scope is linked to it too, since it is evaluated + lazily (PEP 563, PEP 649); a value read there at once is not. + """ + if self._uses_of is not None: + return self._uses_of + uses_of: defaultdict[CSTNode, list[_Link]] = defaultdict(list) + definitions_of: defaultdict[CSTNode, list[_Link]] = defaultdict(list) + scopes = {scope for scope in self.wrapper.resolve(ScopeProvider).values() if scope is not None} + for scope in scopes: + for assignment in scope.assignments: + if not isinstance(assignment, Assignment): + continue + lazy_accesses_above = { + access + for access in scope.accesses[assignment.name] + if not access.referents and (access.is_annotation or access.is_type_hint) + } + # libcst only links an access to the definitions above it in the same scope. + for access in [*assignment.references, *lazy_accesses_above]: + uses_of[assignment.node].append((access.node, assignment.name, access)) + definitions_of[access.node].append((assignment.node, assignment.name, access)) + self._uses_of = dict(uses_of) + self._definitions_of = dict(definitions_of) + return self._uses_of + + def _to_references(self, links: list[_Link], ref_kind: str) -> list[ASTReference[PythonCstNode]]: + """Wrap links as references, ordered by the position of the node each points at. + + Each reference's properties hold the bound "name" and whether the use sits in an annotation ("is_annotation") + or another type hint such as `cast("T", x)` ("is_type_hint"). + """ + references = [ + ASTReference( + self.wrappers[node], + ref_kind, + {"name": name, "is_annotation": access.is_annotation, "is_type_hint": access.is_type_hint}, + ) + for node, name, access in links + if node in self.wrappers + ] + return sorted(references, key=lambda reference: reference.node.offset) + + def uses_of(self, node: CSTNode) -> list[ASTReference[PythonCstNode]]: + """Return the uses of the name(s) that node defines; empty if node defines nothing.""" + return self._to_references(self._index_references().get(node, []), "use") + + def definitions_of(self, node: CSTNode) -> list[ASTReference[PythonCstNode]]: + """Return the definitions that node, a use of a name, resolves to; empty if node is not a resolved use.""" + self._index_references() + return self._to_references(self._definitions_of.get(node, []), "definition") + + def _byte_offset(self, position: CodePosition) -> int: + """Return the offset, in bytes of self.encoding, of a libcst position whose column counts characters. + + A position on the line after the last one (the end of a node that ends with the file) maps to the file's length. + """ + line_index = position.line - 1 + if line_index >= len(self._lines): + return self._line_starts[-1] + return self._line_starts[line_index] + len(self._lines[line_index][: position.column].encode(self.encoding)) def start_of(self, node: CSTNode) -> int: - """AI: Return the character offset where node begins in the source text.""" + """Return the byte offset where node begins in the source text, or 0 if it has no position. + + A decorated function or class begins at its first decorator. + """ + if isinstance(node, (ClassDef, FunctionDef)) and node.decorators: + node = node.decorators[0] span = self.spans.get(node) - return convert(self.lines, span.start.line, span.start.column) if span else 0 + return self._byte_offset(span.start) if span else 0 def end_of(self, node: CSTNode) -> int: - """AI: Return the character offset where node ends in the source text.""" + """Return the byte offset where node's code ends in the source text, or 0 if it has no position.""" span = self.spans.get(node) - return convert(self.lines, span.end.line, span.end.column) if span else 0 + return self._byte_offset(span.end) if span else 0 def signature_of(self, node: CSTNode) -> str: """AI: Return the source code text corresponding to node, or an empty string on failure.""" @@ -54,7 +139,7 @@ def signature_of(self, node: CSTNode) -> str: class PythonCstNode: """AI: ASTNode implementation backed by libcst's concrete syntax tree.""" - def __init__(self, node: CSTNode, translation_unit: PythonCstTranslationUnit, parent=None) -> None: + def __init__(self, node: CSTNode, translation_unit: PythonCstTranslationUnit, parent: PythonCstNode | None = None) -> None: """AI: Wrap a libcst node as an AST node within the given translation unit.""" self.parent = parent if parent and parent.root: @@ -63,12 +148,13 @@ def __init__(self, node: CSTNode, translation_unit: PythonCstTranslationUnit, pa self.root = self self.translation_unit = translation_unit self.node = node + translation_unit.wrappers[node] = self self.is_statement = isinstance(self.node, (BaseSmallStatement, BaseCompoundStatement)) self.parser_kind = type(node).__name__ self.semantic_kind = PYTHON_KIND_MAP.get(self.parser_kind, SemanticKind.NODE) - self.children: list[Self] = [PythonCstNode(node, translation_unit, self) for node in node.children] + self.children: list[PythonCstNode] = [PythonCstNode(node, translation_unit, self) for node in node.children] self.properties = {} # for shower @@ -94,21 +180,30 @@ def signature(self): @property def offset(self): """AI: Return the character offset where this node begins in the source text.""" + # TODO(FrancescoPezzella): the docstring says characters, but is this really counting characters? Since the + # UTF-8 offsets fix it counts bytes of translation_unit.encoding, so shouldn't the docstring say bytes? + # https://github.com/TNO/Renaissance.Py/issues/153 return self.translation_unit.start_of(self.node) @property def length(self): """AI: Return the length in characters of this node's source text.""" + # TODO(FrancescoPezzella): the docstring says characters, but is this really counting characters? Since the + # UTF-8 offsets fix the length is in bytes of translation_unit.encoding, so shouldn't the docstring say bytes? + # https://github.com/TNO/Renaissance.Py/issues/153 return self.end_offset - self.offset @property def end_offset(self): """AI: Return the character offset where this node ends in the source text.""" + # TODO(FrancescoPezzella): the docstring says characters, but is this really counting characters? Since the + # UTF-8 offsets fix it counts bytes of translation_unit.encoding, so shouldn't the docstring say bytes? + # https://github.com/TNO/Renaissance.Py/issues/153 return self.translation_unit.end_of(self.node) @property def extended_end_offset(self) -> int: - """AI: Return the end offset; libcst attaches trailing trivia to the node itself, so there is nothing to extend.""" + """Return the end offset; trailing comments are found by the rewriter, as for the RST backend.""" return self.end_offset @property @@ -125,12 +220,22 @@ def name(self): self.name = "" # self._derive_name() @property - def next_sibling(self) -> Self | None: + def references(self) -> list[ASTReference[PythonCstNode]]: + """Return the definitions this node uses, resolved with libcst's scope analysis (empty if it uses none).""" + return self.translation_unit.definitions_of(self.node) + + @property + def referenced_by(self) -> list[ASTReference[PythonCstNode]]: + """Return the uses of the name(s) this node defines, resolved with libcst's scope analysis (empty if none).""" + return self.translation_unit.uses_of(self.node) + + @property + def next_sibling(self) -> PythonCstNode | None: """AI: Return the sibling node immediately following this one, or None.""" return next_sibling(self) @property - def preceding_sibling(self) -> Self | None: + def preceding_sibling(self) -> PythonCstNode | None: """AI: Return the sibling node immediately preceding this one, or None.""" return preceding_sibling(self) diff --git a/src/renaissance/project/project_scanner.py b/src/renaissance/project/project_scanner.py index 215001fd..5478a695 100644 --- a/src/renaissance/project/project_scanner.py +++ b/src/renaissance/project/project_scanner.py @@ -69,7 +69,7 @@ def __init__(self, root_dir: str = ".", package_dirs: list[str] | None = None) - self.package_dirs = package_dirs def find_sources(self) -> list[str]: - """Return every .py file under root_dir (or package_dirs, if given), sorted, excluding EXCLUDED_DIRS.""" + """Return every .py file under root_dir (or package_dirs, if given), sorted, skipping EXCLUDED_DIRS subdirectories.""" path = Path(self.root_dir) if not path.exists(): message = f"root_dir does not exist: {self.root_dir}" @@ -78,13 +78,16 @@ def find_sources(self) -> list[str]: message = f"root_dir is not a directory: {self.root_dir}" raise NotADirectoryError(message) - roots = [Path(self.root_dir) / d for d in self.package_dirs] if self.package_dirs else [Path(self.root_dir)] + roots = [path / d for d in self.package_dirs] if self.package_dirs else [path] files: list[Path] = [] for root in roots: if not root.exists(): continue - files.extend(path for path in root.rglob("*.py") if not any(part in self.EXCLUDED_DIRS for part in path.parts)) - return sorted(str(path) for path in files) + for dirpath, dirnames, filenames in root.walk(): + # Pruning in place stops walk() from descending into excluded directories at all. + dirnames[:] = [name for name in dirnames if name not in self.EXCLUDED_DIRS] + files.extend(dirpath / name for name in filenames if (dirpath / name).match("*.py")) + return sorted(str(file) for file in files) class BearCppScanner(CppScanner): diff --git a/src/renaissance/recipes/python_refactoring.py b/src/renaissance/recipes/python_refactoring.py index 32189d9f..8cee2b4a 100644 --- a/src/renaissance/recipes/python_refactoring.py +++ b/src/renaissance/recipes/python_refactoring.py @@ -13,6 +13,7 @@ from termcolor import colored +from renaissance.integrations.python.ast.cst_node import PythonCstNode from renaissance.integrations.python.ast.factory import PythonFactory, PythonPatternFactory from renaissance.integrations.python.ast.rst_node import PythonRstNode from renaissance.integrations.python.ast.util import to_str @@ -22,12 +23,12 @@ from renaissance.utils.text_utils import snake_case -class PythonRefactoring(ASTProcessor): +class PythonRefactoring[NodeT: PythonRstNode | PythonCstNode = PythonRstNode](ASTProcessor): """AI: Base processor for Python-specific source refactoring recipes.""" - def __init__(self, file) -> None: - """AI: Prepare a Python-specific refactoring processor for the given source file.""" - factory = PythonFactory(PythonRstNode) + def __init__(self, file: Path, node_class: type[NodeT] = PythonRstNode) -> None: + """Prepare a refactoring processor for file, parsed with node_class (the RST backend by default).""" + factory = PythonFactory(node_class) atu = factory.create(file) super().__init__(atu, factory, False) self.pattern_factory = PythonPatternFactory(self.factory) @@ -111,9 +112,13 @@ def class_base_arguments(self, class_node: PythonRstNode) -> list[str]: return [child.signature for child in bases_implicit.children] @property - def body(self) -> Sequence[PythonRstNode]: - """AI: Return the root node's body statements.""" - return cast("PythonRstNode", cast("object", self.root)).body + def body(self) -> Sequence[NodeT]: + """Return the module's top-level statements; with the CST backend, a simple statement comes as its whole line.""" + root = cast("NodeT", cast("object", self.root)) + if isinstance(root, PythonCstNode): + wrappers = root.translation_unit.wrappers + return cast("Sequence[NodeT]", [wrappers[statement] for statement in root.translation_unit.atu.body]) + return cast("Sequence[NodeT]", root.body) def run(self) -> None: - """AI: Run this refactoring recipe. Subclasses override this to perform the refactoring.""" + """Perform this recipe's refactoring; a no-op unless a subclass overrides it.""" diff --git a/src/renaissance/recipes/step_runner.py b/src/renaissance/recipes/step_runner.py new file mode 100644 index 00000000..ce7a6d5a --- /dev/null +++ b/src/renaissance/recipes/step_runner.py @@ -0,0 +1,35 @@ +# Copyright (C) 2024-2026 TNO +# +# Licensed under the terms specified in the LICENSE file in the root directory +# of this repository, also available at: +# https://github.com/TNO/Renaissance.Py/blob/main/LICENSE + +"""A small, generic way to sequence independently-committable fix actions across one or more recipes.""" + +from collections.abc import Callable, Sequence +from dataclasses import dataclass + +from renaissance.syntax_tree import ASTProcessor + + +@dataclass(frozen=True) +class Step: + """One independently-runnable, independently-committable fix action.""" + + label: str + recipe: ASTProcessor + action: Callable[[], dict[str, str]] + + +def run_steps(steps: Sequence[Step]) -> dict[str, dict[str, str]]: + """Run each step in order, committing its recipe if the step fixed anything. + + Returns {step.label: {name: "fixed" | "unsafe"}}, one entry per step, in step order. + """ + result: dict[str, dict[str, str]] = {} + for step in steps: + outcome = step.action() + if "fixed" in outcome.values(): + step.recipe.commit() + result[step.label] = outcome + return result diff --git a/src/renaissance/recipes/type_var_check.py b/src/renaissance/recipes/type_var_check.py new file mode 100644 index 00000000..d322b05e --- /dev/null +++ b/src/renaissance/recipes/type_var_check.py @@ -0,0 +1,413 @@ +# Copyright (C) 2024-2026 TNO +# +# Licensed under the terms specified in the LICENSE file in the root directory +# of this repository, also available at: +# https://github.com/TNO/Renaissance.Py/blob/main/LICENSE + +"""Recipe that modernizes legacy TypeVar/ParamSpec/TypeVarTuple usage to PEP 695 syntax.""" + +from collections.abc import Sequence +from pathlib import Path +from typing import cast + +import libcst as cst + +from renaissance.integrations.python.ast.cst_node import PythonCstNode +from renaissance.recipes.python_refactoring import PythonRefactoring +from renaissance.recipes.step_runner import Step, run_steps +from renaissance.recipes.type_var_domain import ( + UnsafeReason, + declaration_argument_names, + declared_default, + declares_type_param, + find_type_param_declarations, + from_import_sources, + generic_classes, + has_unconvertible_argument, + is_safe_to_localize, + is_safe_to_remove, + module_statements, + outermost_function, + type_param_constructor_name, + type_param_text, + type_params_bracket, + uses_of, +) +from renaissance.utils.import_resolution import import_from_module, imported_name, rebase_relative_module, resolve_project_module + +PEP_695_MINIMUM = (3, 12) +PEP_696_MINIMUM = (3, 13) + + +class TypeVarCheck(PythonRefactoring[PythonCstNode]): + """Modernize legacy TypeVar/ParamSpec/TypeVarTuple usage in a Python file to PEP 695 syntax. + + See check() for the three phases this runs, in order. + """ + + # Minimum Python version the target codebase supports; None means unknown. + min_python: tuple[int, int] | None = None + + # Names other files in the target project import directly from this file; never removed. + project_wide_imported_names: frozenset[str] = frozenset() + + # Root that absolute imports resolve from; None falls back to this file's own directory. + project_root: Path | None = None + + def __init__(self, file: Path) -> None: + """Prepare the recipe for file, parsed with the CST backend.""" + super().__init__(file, PythonCstNode) + + def run(self) -> None: + """Entry point called by PythonRefactoring.process(); stores check()'s result.""" + self.result = self.check() + + def _cst_root(self) -> PythonCstNode: + """Return the root node of the file as it currently is (after the last commit).""" + return cast("PythonCstNode", cast("object", self.root)) + + def _target_supports_pep695(self) -> bool: + """Return True only if min_python is known and is 3.12+, where PEP 695 syntax (`def f[T](...)`) exists.""" + return self.min_python is not None and self.min_python >= PEP_695_MINIMUM + + def _target_supports_pep696(self) -> bool: + """Return True only if min_python is known and is 3.13+, where PEP 696 defaults (`def f[T = int](...)`) exist.""" + return self.min_python is not None and self.min_python >= PEP_696_MINIMUM + + def _conversion_refusal(self, decl_stmt: cst.Assign) -> UnsafeReason | None: + """Return why decl_stmt can't be converted to a PEP 695 type parameter on this target, or None if it can.""" + if not self._target_supports_pep695(): + return UnsafeReason.PEP695_VERSION_GATE + if has_unconvertible_argument(decl_stmt): + return UnsafeReason.NO_PEP695_EQUIVALENT + if declared_default(decl_stmt) is not None and not self._target_supports_pep696(): + return UnsafeReason.PEP696_VERSION_GATE + return None + + def check(self) -> dict[str, dict[str, str]]: + """Run the three phases in order, committing each one that fixed something. + + localize_imported_typevars, then convert_declared_typevars, then + remove_orphaned_declarations. Returns {"cross_file": {...}, "converted": {...}, + "orphaned": {...}}, each mapping name -> "fixed" | "unsafe". + """ + return run_steps( + [ + Step("cross_file", self, self.localize_imported_typevars), + Step("converted", self, self.convert_declared_typevars), + Step("orphaned", self, self.remove_orphaned_declarations), + ], + ) + + def convert_declared_typevars(self) -> dict[str, str]: + """Add a PEP 695 type parameter to every function using a module-level TypeVar/ParamSpec/TypeVarTuple. + + The declaration is left in place. A use is attributed to the outermost function containing it; a `T` + that a same-named PEP 695 type parameter of a function or class declares is not a use. Methods of a + class generic over the name are never converted. Bounds, constraints and `default=` are carried over + in their original formatting, with defaulted type parameters placed last. A name only such methods + use, or whose declaration _conversion_refusal rejects, is reported "unsafe", with its UnsafeReason on + self.converted_unsafe_reasons. Returns {name: "fixed" | "unsafe"}. + """ + root = self._cst_root() + module = root.translation_unit.atu + + results: dict[str, str] = {} + self.converted_unsafe_reasons: dict[str, UnsafeReason] = {} + # One self.replace() per function: a function using two converted names would otherwise get two + # rewrites of the same node, which the rewriter rejects as conflicting. + new_params: dict[int, tuple[PythonCstNode, list[tuple[str, bool]]]] = {} + for name, decl_stmt in find_type_param_declarations(module).items(): + uses = uses_of(root, name, decl_stmt) + functions = list(dict.fromkeys(function for use in uses if (function := outermost_function(use)) is not None)) + if not functions: + continue + + classes = generic_classes(uses) + in_generic_class = [function for function in functions if self._is_inside(function, classes)] + candidates = [ + function + for function in functions + if function not in in_generic_class and not declares_type_param(cast("cst.FunctionDef", function.node), name) + ] + if not candidates: + if in_generic_class: + self._mark_unsafe(results, self.converted_unsafe_reasons, name, UnsafeReason.USED_IN_GENERIC_CLASS) + continue + reason = self._conversion_refusal(decl_stmt) + if reason is not None: + self._mark_unsafe(results, self.converted_unsafe_reasons, name, reason) + continue + + param = (type_param_text(decl_stmt, module), declared_default(decl_stmt) is not None) + for function in candidates: + new_params.setdefault(id(function), (function, []))[1].append(param) + results[name] = "fixed" + + for function, params in new_params.values(): + self._add_type_params(function, params) + + return results + + @staticmethod + def _is_inside(node: PythonCstNode, containers: Sequence[PythonCstNode]) -> bool: + """Return True if one of containers encloses node.""" + current = node.parent + while current is not None: + if any(current is container for container in containers): + return True + current = current.parent + return False + + def _add_type_params(self, function: PythonCstNode, params: list[tuple[str, bool]]) -> None: + """Add params, given as (text, has_default), to function's PEP 695 type parameters, changing nothing else. + + A function without a bracket gets one after its name; an existing bracket is rewritten with its own + type parameters followed by params, the defaulted ones last. + """ + node = cast("cst.FunctionDef", function.node) + unit = function.translation_unit + existing = node.type_parameters + if existing is None: + self.replace( + node.name.value + type_params_bracket(params), unit.wrappers[node.name], include_whitespace=False, include_comments=False + ) + return + # TODO(FrancescoPezzella): rebuilding the existing bracket drops its comments and line breaks. + # https://github.com/TNO/Renaissance.Py/issues/279 + current = [ + (unit.atu.code_for_node(param.with_changes(comma=cst.MaybeSentinel.DEFAULT)), param.default is not None) + for param in existing.params + ] + self.replace(type_params_bracket([*current, *params]), unit.wrappers[existing], include_whitespace=False, include_comments=False) + + def remove_orphaned_declarations(self) -> dict[str, str]: + """Remove every module-level TypeVar/ParamSpec/TypeVarTuple declaration that nothing uses anymore. + + A declaration is orphaned when uses_of finds no use of its name: every `T` left belongs to a PEP 695 + type parameter, or there is none. One that is_safe_to_remove rejects is kept and reported "unsafe", + with its UnsafeReason on self.orphaned_unsafe_reasons. Returns {name: "fixed" | "unsafe"}. + """ + root = self._cst_root() + module = root.translation_unit.atu + + results: dict[str, str] = {} + self.orphaned_unsafe_reasons: dict[str, UnsafeReason] = {} + for name, decl_stmt in find_type_param_declarations(module).items(): + if uses_of(root, name, decl_stmt): + continue + + # TODO(FrancescoPezzella): a public TypeVar no file in the project uses (e.g. redis.typing.AnyKeyT) is removed, + # though other projects may import it. + # https://github.com/TNO/Renaissance.Py/issues/279 + reason = is_safe_to_remove(module, name, self.project_wide_imported_names) + if reason is not None: + self._mark_unsafe(results, self.orphaned_unsafe_reasons, name, reason) + continue + + # TODO(FrancescoPezzella): `T = U = TypeVar("T")` is removed whole when T is unused, so U is no longer defined. + # https://github.com/TNO/Renaissance.Py/issues/279 + self._remove_declaration(decl_stmt) + results[name] = "fixed" + + return results + + def _mark_unsafe(self, results: dict[str, str], reasons: dict[str, UnsafeReason], name: str, reason: UnsafeReason) -> None: + """Record `name` as unsafe with `reason` in both `results` (status) and `reasons` (why).""" + results[name] = "unsafe" + reasons[name] = reason + + def _remove_declaration(self, decl_stmt: cst.Assign) -> None: + """Remove decl_stmt's statement from the file.""" + # TODO(FrancescoPezzella): enable once comment blocks get correctly deleted + # https://github.com/TNO/Renaissance.Py/issues/189 + # TODO(FrancescoPezzella): a declaration that starts a `;` line leaves a leading `;`, a syntax error that commit() + # writes to disk. + # https://github.com/TNO/Renaissance.Py/issues/279 + self.remove(self._cst_root().translation_unit.wrappers[decl_stmt], include_comments=False) + + def localize_imported_typevars(self) -> dict[str, str]: + """Replace imports of TypeVar/ParamSpec/TypeVarTuple names from the target project with local declarations. + + Imports are resolved against project_root. Where safe, the import becomes a copy of the origin's + declaration, plus imports from the origin for the names its arguments use (e.g. `bound=Shape`). + It is unsafe when is_safe_to_localize rejects the origin or _argument_names_to_import returns None. + Returns {name: "fixed" | "unsafe"}, with each UnsafeReason on self.cross_file_unsafe_reasons. + """ + results: dict[str, str] = {} + self.cross_file_unsafe_reasons: dict[str, UnsafeReason] = {} + module = self._cst_root().translation_unit.atu + + project_root = self.project_root if self.project_root is not None else Path(self.filename).parent + for line in module.body: + if not isinstance(line, cst.SimpleStatementLine): + continue + for statement in line.body: + if not isinstance(statement, cst.ImportFrom): + continue + origin_path = resolve_project_module( + Path(self.filename), project_root, import_from_module(statement), len(statement.relative) + ) + if origin_path is None: + continue + self._localize_names_from(line, statement, origin_path, results) + + return results + + def _localize_names_from( + self, + line: cst.SimpleStatementLine, + raw: cst.ImportFrom, + origin_path: Path, + results: dict[str, str], + ) -> None: + """Localize every safe type parameter name that raw, on line, imports from the origin_path file. + + Records each name in results as "fixed" or, via _mark_unsafe, as "unsafe". + + Raises: + SyntaxError: if the origin file can't be parsed, with the origin's path as its filename. + + """ + if isinstance(raw.names, cst.ImportStar): + return + try: + origin = cst.parse_module(origin_path.read_text(encoding="utf-8")) + except cst.ParserSyntaxError as error: + raise SyntaxError(error.message, (str(origin_path), error.raw_line, error.raw_column + 1, None)) from error + declarations = find_type_param_declarations(origin) + localized: dict[str, cst.Assign] = {} + needed_imports: list[str] = [] + needed_argument_names: set[str] = set() + + for alias in raw.names: + # TODO(FrancescoPezzella): `from x import T as U` is not currently being caught, tbd + # https://github.com/TNO/Renaissance.Py/issues/279 + name = alias.evaluated_name + if alias.asname is not None or name not in declarations: + continue + + decl_stmt = declarations[name] + reason = is_safe_to_localize(origin, name) + if reason is not None: + self._mark_unsafe(results, self.cross_file_unsafe_reasons, name, reason) + continue + argument_names = self._argument_names_to_import(origin, raw, decl_stmt) + if argument_names is None: + self._mark_unsafe(results, self.cross_file_unsafe_reasons, name, UnsafeReason.DECLARATION_NAME_UNAVAILABLE) + continue + + constructor_import = self._missing_constructor_import(origin, origin_path, raw, decl_stmt) + if constructor_import is not None and constructor_import not in needed_imports: + needed_imports.append(constructor_import) + needed_argument_names |= argument_names + localized[name] = decl_stmt + results[name] = "fixed" + + if not localized: + return + if needed_argument_names: + needed_imports.append(f"from {self._import_from_text(raw)} import {', '.join(sorted(needed_argument_names))}") + self._localize_import(line, raw, origin, localized, needed_imports) + + @staticmethod + def _import_from_text(raw: cst.ImportFrom) -> str: + """Return what follows `from` in raw: its leading dots and dotted module, e.g. "..pkg.mod".""" + return "." * len(raw.relative) + (import_from_module(raw) or "") + + def _missing_constructor_import( + self, + origin: cst.Module, + origin_path: Path, + via: cst.ImportFrom, + decl_stmt: cst.Assign, + ) -> str | None: + """Return the "from module import Ctor" line the localized declaration needs, or None if this file has it. + + The module is the one the origin imports the constructor from, as named from this file, which + reaches the origin (origin_path) through via. If this file can't name that module, the + constructor is imported from the origin module itself. + """ + ctor_name = type_param_constructor_name(decl_stmt) + ctor_source = from_import_sources(origin).get(ctor_name) + if ctor_source is None: + # TODO(FrancescoPezzella): an origin using `from typing import *` lands here, + # so a file not importing the constructor itself gets a NameError. + # TBD - needs a fix + # https://github.com/TNO/Renaissance.Py/issues/279 + return None + + via_module = import_from_module(via) + ctor_module = rebase_relative_module(via_module, len(via.relative), origin_path, ctor_source[0] or None, ctor_source[1]) + ctor_module = ctor_module or self._import_from_text(via) + + # TODO(FrancescoPezzella): the constructor already imported here from another module (e.g. `typing` vs the origin's + # `typing_extensions`) isn't matched, so a second, shadowing import of the same name is added. + # https://github.com/TNO/Renaissance.Py/issues/279 + for statement in module_statements(self._cst_root().translation_unit.atu): + if ( + isinstance(statement, cst.ImportFrom) + and not isinstance(statement.names, cst.ImportStar) + and self._import_from_text(statement) == ctor_module + and any(imported_name(alias) == ctor_name for alias in statement.names) + ): + return None + + return f"from {ctor_module} import {ctor_name}" + + def _argument_names_to_import(self, origin: cst.Module, raw: cst.ImportFrom, decl_stmt: cst.Assign) -> set[str] | None: + """Return the names decl_stmt's arguments use that this file must import from the origin module. + + A name this file already imports from the origin module (or from the same absolute module the + origin imports it from) is fine and not returned. Returns None if one of those names can't be + imported from the origin as the same object: the origin doesn't bind it at module level at + runtime (e.g. only under `if TYPE_CHECKING:`), or this file binds it to something else. + """ + origin_sources = from_import_sources(origin) + importing_sources = from_import_sources(self._cst_root().translation_unit.atu) + missing: set[str] = set() + for name in declaration_argument_names(decl_stmt): + if name not in origin_sources: + return None + if name not in importing_sources: + missing.add(name) + continue + origin_source = origin_sources[name] + accepted = {(import_from_module(raw) or "", len(raw.relative))} + if origin_source is not None and origin_source[1] == 0: + accepted.add(origin_source) + if importing_sources[name] not in accepted: + return None + return missing + + def _localize_import( + self, + line: cst.SimpleStatementLine, + raw: cst.ImportFrom, + origin: cst.Module, + declarations: dict[str, cst.Assign], + needed_imports: list[str], + ) -> None: + """Replace the import raw, on line, with local copies of declarations from the origin module. + + The declarations keep their original formatting and come after needed_imports: the imports they need + (their constructors, names their arguments use) that this file doesn't have yet. If raw imports other + names too, it is narrowed to those, keeping its comments, and the declarations go after line; + otherwise they replace raw. + """ + no_semicolon = cst.MaybeSentinel.DEFAULT + decl_text = "\n".join( + [*needed_imports, *(origin.code_for_node(decl.with_changes(semicolon=no_semicolon)) for decl in declarations.values())] + ) + wrappers = self._cst_root().translation_unit.wrappers + + kept = [alias for alias in cast("Sequence[cst.ImportAlias]", raw.names) if imported_name(alias) not in declarations] + if not kept: + self.replace(decl_text, wrappers[raw], include_whitespace=False, include_comments=False) + return + if raw.rpar is None: + # Without parentheses, a trailing comma after the last name is a syntax error. + kept[-1] = kept[-1].with_changes(comma=cst.MaybeSentinel.DEFAULT) + narrowed = self._cst_root().translation_unit.atu.code_for_node(raw.with_changes(names=kept, semicolon=no_semicolon)) + self.replace(narrowed, wrappers[raw], include_whitespace=False, include_comments=False) + self.insert_after(f"\n{decl_text}", wrappers[line], include_whitespace=False, include_comments=True) diff --git a/src/renaissance/recipes/type_var_domain.py b/src/renaissance/recipes/type_var_domain.py new file mode 100644 index 00000000..cfc8968f --- /dev/null +++ b/src/renaissance/recipes/type_var_domain.py @@ -0,0 +1,401 @@ +# Copyright (C) 2024-2026 TNO +# +# Licensed under the terms specified in the LICENSE file in the root directory +# of this repository, also available at: +# https://github.com/TNO/Renaissance.Py/blob/main/LICENSE + +"""TypeVar/ParamSpec/TypeVarTuple domain model and safety analysis.""" + +import builtins +from collections.abc import Iterable, Iterator +from dataclasses import dataclass +from enum import StrEnum +from typing import cast + +import libcst as cst +from libcst import matchers + +from renaissance.integrations.python.ast.cst_node import PythonCstNode +from renaissance.utils.import_resolution import import_from_module, imported_name + +DOCS_BASE_URL = "https://tno.github.io/Renaissance.Py/user/features/type-parameter-modernization/" + + +class UnsafeReason(StrEnum): + """Every distinct, permanent reason a TypeVar/ParamSpec/TypeVarTuple candidate is left unconverted. + + Each member has a matching documented rule under DOCS_BASE_URL - see UNSAFE_RULES and doc_link(). + """ + + PEP695_VERSION_GATE = "pep695_version_gate" + PEP696_VERSION_GATE = "pep696_version_gate" + NO_PEP695_EQUIVALENT = "no_pep695_equivalent" + DECLARED_TYPEVAR_EXPORTED = "declared_typevar_exported" + USED_IN_GENERIC_CLASS = "used_in_generic_class" + DECLARATION_NAME_UNAVAILABLE = "declaration_name_unavailable" + IMPORTED_ELSEWHERE_IN_PROJECT = "imported_elsewhere_in_project" + ORIGIN_IMPORTS_CONSTRUCTOR_CONDITIONALLY = "origin_imports_constructor_conditionally" + + +@dataclass(frozen=True) +class UnsafeRule: + """A short human-readable explanation plus the docs anchor slug for one UnsafeReason.""" + + message: str + doc_anchor: str + + +UNSAFE_RULES: dict[UnsafeReason, UnsafeRule] = { + UnsafeReason.PEP695_VERSION_GATE: UnsafeRule( + "target's minimum Python version is unknown or below 3.12", + "feature-type-parameter-modernization-pep695-version-gate", + ), + UnsafeReason.PEP696_VERSION_GATE: UnsafeRule( + "declares a default= (PEP 696), which needs the target's minimum Python to be 3.13+", + "feature-type-parameter-modernization-pep696-version-gate", + ), + UnsafeReason.NO_PEP695_EQUIVALENT: UnsafeRule( + "declaration passes an argument with no PEP 695 equivalent, e.g. covariant=True or bound= on a ParamSpec", + "feature-type-parameter-modernization-no-pep695-equivalent", + ), + UnsafeReason.DECLARED_TYPEVAR_EXPORTED: UnsafeRule( + "exported via __all__", + "feature-type-parameter-modernization-declared-type-parameter-exported", + ), + UnsafeReason.USED_IN_GENERIC_CLASS: UnsafeRule( + "only used inside a class that is generic over it", + "feature-type-parameter-modernization-used-in-generic-class", + ), + UnsafeReason.DECLARATION_NAME_UNAVAILABLE: UnsafeRule( + "a name its declaration uses can't be imported here as the same object", + "feature-type-parameter-modernization-declaration-name-unavailable", + ), + UnsafeReason.IMPORTED_ELSEWHERE_IN_PROJECT: UnsafeRule( + "imported directly by another file in the target project", + "feature-type-parameter-modernization-imported-elsewhere-in-project", + ), + UnsafeReason.ORIGIN_IMPORTS_CONSTRUCTOR_CONDITIONALLY: UnsafeRule( + "origin module imports TypeVar/ParamSpec/TypeVarTuple conditionally, e.g. per Python version", + "feature-type-parameter-modernization-origin-imports-constructor-conditionally", + ), +} + + +def doc_link(reason: UnsafeReason) -> str: + """Return the full URL to the documented rule explaining why `reason` makes a candidate unsafe.""" + return f"{DOCS_BASE_URL}#{UNSAFE_RULES[reason].doc_anchor}" + + +_CONSTRUCTORS = frozenset({"TypeVar", "ParamSpec", "TypeVarTuple"}) + + +def _is_type_param_call(value: cst.BaseExpression) -> bool: + """Return True if `value` is a call to TypeVar/ParamSpec/TypeVarTuple.""" + return isinstance(value, cst.Call) and isinstance(value.func, cst.Name) and value.func.value in _CONSTRUCTORS + + +def module_statements(module: cst.Module) -> Iterator[cst.BaseSmallStatement | cst.BaseCompoundStatement]: + """Yield the module's top-level statements in order, one per small statement of a line like `a = 1; b = 2`.""" + for statement in module.body: + if isinstance(statement, cst.SimpleStatementLine): + yield from statement.body + else: + yield statement + + +def find_type_param_declarations(module: cst.Module) -> dict[str, cst.Assign]: + """Find every module-level "NAME = TypeVar/ParamSpec/TypeVarTuple(...)" declaration.""" + declarations: dict[str, cst.Assign] = {} + for statement in module_statements(module): + if isinstance(statement, cst.Assign) and _is_type_param_call(statement.value): + for target in statement.targets: + if isinstance(target.target, cst.Name): + declarations[target.target.value] = statement + return declarations + + +def _call(decl_stmt: cst.Assign) -> cst.Call: + """Return the TypeVar/ParamSpec/TypeVarTuple call a declaration assigns.""" + return cast("cst.Call", decl_stmt.value) + + +def _positional_arguments(call: cst.Call) -> list[cst.Arg]: + """Return the call's positional arguments, `*args` included.""" + return [argument for argument in call.args if argument.keyword is None and argument.star != "**"] + + +def _keyword_value(call: cst.Call, keyword: str) -> cst.BaseExpression | None: + """Return the value the call passes as `keyword=`, or None if it passes none.""" + return next((argument.value for argument in call.args if argument.keyword is not None and argument.keyword.value == keyword), None) + + +def type_param_constructor_name(decl_stmt: cst.Assign) -> str: + """Return the name of the call a declaration uses, e.g. "TypeVar" for `T = TypeVar("T")`.""" + return cast("cst.Name", _call(decl_stmt).func).value + + +def _string_value(expression: cst.BaseExpression) -> str | None: + """Return the value of a plain or concatenated string literal, or None for any other expression.""" + if isinstance(expression, cst.SimpleString | cst.ConcatenatedString): + value = expression.evaluated_value + return value if isinstance(value, str) else None + return None + + +def _find_dunder_all(module: cst.Module) -> set[str] | None: + """Return the names in this module's plain `__all__ = [...]` assignment, or None if it has none. + + Only a list, tuple or set literal is read; annotated, `+=`, `.extend()` and `.append()` forms are ignored. + """ + for statement in module_statements(module): + if ( + isinstance(statement, cst.Assign) + and any(isinstance(target.target, cst.Name) and target.target.value == "__all__" for target in statement.targets) + and isinstance(statement.value, cst.List | cst.Tuple | cst.Set) + ): + values = (_string_value(element.value) for element in statement.value.elements if isinstance(element, cst.Element)) + return {value for value in values if value is not None} + return None + + +class _NameCollector(cst.CSTVisitor): + """Collect the names an expression reads: every `Name` except an attribute's or a keyword argument's own name.""" + + def __init__(self) -> None: + """Start with no names collected.""" + super().__init__() + self.names: set[str] = set() + + def visit_Name(self, node: cst.Name) -> None: # noqa: N802 - libcst dispatches on this exact name + """Record the name.""" + self.names.add(node.value) + + def visit_Attribute(self, node: cst.Attribute) -> bool: # noqa: N802 - libcst dispatches on this exact name + """Visit only the attribute's object: in `np.integer`, `integer` is not a name read here.""" + node.value.visit(self) + return False + + def visit_Arg(self, node: cst.Arg) -> bool: # noqa: N802 - libcst dispatches on this exact name + """Visit only the argument's value, not the keyword it is passed as.""" + node.value.visit(self) + return False + + +def declaration_argument_names(decl_stmt: cst.Assign) -> set[str]: + """Return the non-builtin names a declaration's arguments use, e.g. {"Shape"} for `TypeVar("T", bound=Shape)`. + + The first positional argument (the type parameter's own name) is skipped; a string argument is read as a + forward reference (`bound="Shape"`). A string that isn't a valid expression contributes no names. + """ + call = _call(decl_stmt) + keyword_arguments = [argument for argument in call.args if argument.keyword is not None or argument.star == "**"] + collector = _NameCollector() + for argument in [*_positional_arguments(call)[1:], *keyword_arguments]: + expression = argument.value + text = _string_value(expression) + if text is not None: + try: + expression = cst.parse_expression(text) + except cst.ParserSyntaxError: + continue + expression.visit(collector) + return collector.names - set(dir(builtins)) + + +def from_import_sources(module: cst.Module) -> dict[str, tuple[str, int] | None]: + """Map every name bound at module level to its `from` import's (module, level), or None if bound otherwise. + + Names bound by a class, function, assignment or plain `import` map to None. + """ + sources: dict[str, tuple[str, int] | None] = {} + for statement in module_statements(module): + if isinstance(statement, cst.ImportFrom): + source = (import_from_module(statement) or "", len(statement.relative)) + if isinstance(statement.names, cst.ImportStar): + sources["*"] = source + else: + for alias in statement.names: + sources[imported_name(alias)] = source + elif isinstance(statement, cst.Import): + for alias in statement.names: + sources[imported_name(alias).split(".")[0]] = None + elif isinstance(statement, cst.ClassDef | cst.FunctionDef): + sources[statement.name.value] = None + elif isinstance(statement, cst.Assign | cst.AnnAssign): + sources.update(dict.fromkeys(_assigned_names(statement))) + return sources + + +def _assigned_names(statement: cst.Assign | cst.AnnAssign) -> set[str]: + """Return every name an assignment's targets use, including inside tuple or list targets.""" + targets = [target.target for target in statement.targets] if isinstance(statement, cst.Assign) else [statement.target] + collector = _NameCollector() + for target in targets: + target.visit(collector) + return collector.names + + +def _imports_name_conditionally(module: cst.Module, name: str) -> bool: + """Return True if an import inside a module-level `if`/`try` block binds `name`.""" + for statement in module.body: + if not isinstance(statement, cst.If | cst.Try | cst.TryStar): + continue + for node in matchers.findall(statement, matchers.Import() | matchers.ImportFrom()): + names = cast("cst.Import | cst.ImportFrom", node).names + if not isinstance(names, cst.ImportStar) and any(imported_name(alias).split(".")[0] == name for alias in names): + return True + return False + + +def is_safe_to_localize(origin: cst.Module, name: str) -> UnsafeReason | None: + """Return None if `name` is safe to duplicate as a local declaration, else the reason it isn't. + + Its constructor (TypeVar/ParamSpec/TypeVarTuple) must not be imported inside a module-level + `if`/`try` block at the origin (ORIGIN_IMPORTS_CONSTRUCTOR_CONDITIONALLY), since which + implementation it binds then depends on the runtime, e.g. `typing_extensions` below 3.13. + """ + declaration = find_type_param_declarations(origin).get(name) + if declaration is not None and _imports_name_conditionally(origin, type_param_constructor_name(declaration)): + return UnsafeReason.ORIGIN_IMPORTS_CONSTRUCTOR_CONDITIONALLY + return None + + +def is_safe_to_remove( + module: cst.Module, + name: str, + project_wide_imported_names: frozenset[str] = frozenset(), +) -> UnsafeReason | None: + """Return None if this file's own binding of `name` may be removed or replaced. + + Otherwise returns the reason it may not: DECLARED_TYPEVAR_EXPORTED if exported via a plain `__all__ = [...]`, + or IMPORTED_ELSEWHERE_IN_PROJECT if `name` is in `project_wide_imported_names` (another file in + the target project imports it directly, regardless of `__all__`). + """ + dunder_all = _find_dunder_all(module) + if dunder_all is not None and name in dunder_all: + return UnsafeReason.DECLARED_TYPEVAR_EXPORTED + if name in project_wide_imported_names: + return UnsafeReason.IMPORTED_ELSEWHERE_IN_PROJECT + return None + + +def uses_of(root: PythonCstNode, name: str, decl_stmt: cst.Assign) -> list[PythonCstNode]: + """Return the nodes that use the module-level `name` that decl_stmt declares, resolved with libcst's scope analysis. + + A use inside a string annotation or `cast("T", x)` counts; a `T` that a PEP 695 type parameter of an + enclosing function or class (`def f[T]`, `class C[T]`) declares does not. + """ + # TODO(FrancescoPezzella): libcst puts a function's decorators and parameter defaults inside its PEP 695 type + # parameter scope, so a `T` read only there (`@dec(T)`, `y=T`) is missed and the declaration it needs is removed. + # https://github.com/TNO/Renaissance.Py/issues/279 + target = next(target.target for target in decl_stmt.targets if isinstance(target.target, cst.Name) and target.target.value == name) + return [reference.node for reference in root.translation_unit.wrappers[target].referenced_by] + + +def outermost_function(node: PythonCstNode) -> PythonCstNode | None: + """Return the outermost function or method enclosing node, or None if node is outside every function.""" + outermost = None + current = node.parent + while current is not None: + if isinstance(current.node, cst.FunctionDef): + outermost = current + current = current.parent + return outermost + + +def generic_classes(uses: Iterable[PythonCstNode]) -> list[PythonCstNode]: + """Return the classes that have one of `uses` in a base (`Generic[T]`, `Protocol[T]`, `Base[int, T]`). + + Such a class is generic over the used name. + """ + classes: list[PythonCstNode] = [] + for use in uses: + child, current = use, use.parent + while current is not None: + if isinstance(current.node, cst.ClassDef) and any(child.node is base for base in current.node.bases): + if current not in classes: + classes.append(current) + break + child, current = current, current.parent + return classes + + +def declares_type_param(function: cst.FunctionDef, name: str) -> bool: + """Return True if the function declares `name` as its own PEP 695 type parameter (`def f[T](...)`).""" + parameters = function.type_parameters + return parameters is not None and any(parameter.param.name.value == name for parameter in parameters.params) + + +# infer_variance needs no syntax of its own: a PEP 695 type parameter always infers its variance. +_CONVERTIBLE_KEYWORDS: dict[str, frozenset[str]] = { + "TypeVar": frozenset({"bound", "default", "infer_variance"}), + "ParamSpec": frozenset({"default", "infer_variance"}), + "TypeVarTuple": frozenset({"default", "infer_variance"}), +} + + +def declared_default(decl_stmt: cst.Assign) -> cst.BaseExpression | None: + """Return the value a legacy declaration passes as `default=` (PEP 696), or None if it passes none.""" + return _keyword_value(_call(decl_stmt), "default") + + +def has_unconvertible_argument(decl_stmt: cst.Assign) -> bool: + """Return True if a legacy declaration passes a keyword type_param_text can't carry over. + + That is any keyword outside the constructor's known set, e.g. `covariant=`/`contravariant=`, + `bound=` on a ParamSpec or TypeVarTuple, a keyword added by a later Python version, or a `**mapping`. + """ + convertible = _CONVERTIBLE_KEYWORDS[type_param_constructor_name(decl_stmt)] + return any( + argument.star == "**" or (argument.keyword is not None and argument.keyword.value not in convertible) + for argument in _call(decl_stmt).args + ) + + +def type_param_text(decl_stmt: cst.Assign, module: cst.Module) -> str: + """Return the PEP 695 type parameter equivalent to a legacy declaration from `module`, in its original formatting. + + E.g. `T = TypeVar("T", bound=int, default=int)` gives `T: int = int`. Bounds, constraints and + `default=` are carried over; `infer_variance=` needs nothing, since a PEP 695 type parameter always + infers its variance. Callers must first reject declarations for which + has_unconvertible_argument() is True. + """ + # TODO(FrancescoPezzella): a default that uses another legacy declaration (`default=T`) still refers to it after conversion. + # https://github.com/TNO/Renaissance.Py/issues/279 + call = _call(decl_stmt) + constructor = type_param_constructor_name(decl_stmt) + positional = _positional_arguments(call) + # TODO(FrancescoPezzella): the type parameter is named after the string argument, not the variable, so + # `T = TypeVar("U")` gives `def f[U](x: T)`; a non-literal name (`TypeVar(name)`, `TypeVar(*args)`) raises + # AttributeError and stops every phase for the file. + # https://github.com/TNO/Renaissance.Py/issues/279 + name = cast("str", cast("cst.SimpleString", positional[0].value).evaluated_value) + + if constructor == "ParamSpec": + text = f"**{name}" + elif constructor == "TypeVarTuple": + text = f"*{name}" + else: + text = name + bound = _keyword_value(call, "bound") + constraints = [f"{argument.star}{module.code_for_node(argument.value)}" for argument in positional[1:]] + if bound is not None: + text += f": {module.code_for_node(bound)}" + elif constraints: + # A single constraint keeps a trailing comma, or the parentheses would make it a bound. + text += f": ({', '.join(constraints)}{',' if len(constraints) == 1 else ''})" + + default = declared_default(decl_stmt) + if default is not None: + text += f" = {module.code_for_node(default)}" + return text + + +def type_params_bracket(params: Iterable[tuple[str, bool]]) -> str: + """Return the `[...]` bracket for type parameters given as (text, has_default), with the defaulted ones last. + + The relative order within each group is kept. A non-default type parameter following a defaulted + one is a SyntaxError. + """ + ordered = sorted(params, key=lambda param: param[1]) + return f"[{', '.join(text for text, _ in ordered)}]" diff --git a/src/renaissance/recipes/unit_to_pytest.py b/src/renaissance/recipes/unit_to_pytest.py index db99a597..ed6b988f 100644 --- a/src/renaissance/recipes/unit_to_pytest.py +++ b/src/renaissance/recipes/unit_to_pytest.py @@ -218,6 +218,8 @@ def restructure_module(self) -> None: # file does not contain any test class, create a new class and add function in class cls = f"class {self.convert_file_to_test_class()}:\n" for fun in funs: + # TODO(FrancescoPezzella): textwrap.indent also indents the lines inside a multi-line string, changing its value. + # https://github.com/TNO/Renaissance.Py/issues/283 cls += textwrap.indent(convert_function(fun), " ") self.remove([fun]) self.insert_before(cls, funs[0]) diff --git a/src/renaissance/syntax_tree/ast_factory.py b/src/renaissance/syntax_tree/ast_factory.py index a19bafb7..a9fa27a7 100644 --- a/src/renaissance/syntax_tree/ast_factory.py +++ b/src/renaissance/syntax_tree/ast_factory.py @@ -29,6 +29,9 @@ def __init__( working_dir: Path | None = None, ) -> None: """AI: Configure a factory that creates AST nodes of the given node-implementation type.""" + # TODO(UNKNOWN): Make ASTFactory generic over the node class (clazz: type[NodeT], returning NodeT) so callers get the + # concrete node type instead of ASTNode[Unknown, Unknown]. The bound must also accept PythonRstNode, which is not an ASTNode. + # https://github.com/TNO/Renaissance.Py/issues/149 self.clazz = clazz self.extra_args: Sequence[str] = extra_args if isinstance(extra_args, Sequence) else [] # TODO: Why not diff --git a/src/renaissance/syntax_tree/ast_node.py b/src/renaissance/syntax_tree/ast_node.py index 50badad5..437d6ece 100644 --- a/src/renaissance/syntax_tree/ast_node.py +++ b/src/renaissance/syntax_tree/ast_node.py @@ -16,6 +16,8 @@ from pathlib import Path from typing import Any, Self +from renaissance.syntax_tree.node_protocol import NodeProtocol +from renaissance.syntax_tree.semantic_kind import SemanticKind from renaissance.utils.ast_utils import format_node, next_sibling, preceding_sibling, process_node from renaissance.utils.text_utils import TextUtils @@ -29,18 +31,25 @@ class VisitorResult(Enum): SKIP = 2 -class ASTReference[NodeT, TranslationUnitT]: - """AI: Represent a reference from one AST node to another, along with the kind of reference.""" +class ASTReference[NodeProtocolT: NodeProtocol]: + """Reference between two AST nodes, with its kind and additional properties.""" - def __init__(self, ast_node: ASTNode[NodeT, TranslationUnitT], ref_kind: str, properties: dict[str, Any]) -> None: - """AI: Represent a reference from one AST node to another, along with the kind of reference.""" + def __init__(self, ast_node: NodeProtocolT, ref_kind: str, properties: dict[str, Any]) -> None: + """Initialise the reference. + + Args: + ast_node: Node at the other end of the reference. + ref_kind: Kind of the reference. + properties: Additional properties of the reference. + + """ self._node = ast_node self._ref_kind = ref_kind self._properties = properties @property - def node(self) -> ASTNode[NodeT, TranslationUnitT]: - """AI: Return the AST node this reference points from.""" + def node(self) -> NodeProtocolT: + """Return the node at the other end of the reference.""" return self._node @property @@ -62,6 +71,8 @@ class ASTNode[NodeT, TranslationUnitT](ABC): """ cache: dict[str, bytes] = {} + parser_kind: str + semantic_kind: SemanticKind def __init__(self, root: Self) -> None: """AI: Initialize a new AST node rooted at the given translation-unit-level node.""" @@ -135,12 +146,12 @@ def next_sibling(self) -> Self | None: @property @abstractmethod - def references(self) -> list[ASTReference[NodeT, TranslationUnitT]]: + def references(self) -> Sequence[ASTReference[ASTNode[NodeT, TranslationUnitT]]]: """AI: Return the references that this node points to.""" @property @abstractmethod - def referenced_by(self) -> list[ASTReference[NodeT, TranslationUnitT]]: + def referenced_by(self) -> Sequence[ASTReference[ASTNode[NodeT, TranslationUnitT]]]: """AI: Return the references that point to this node.""" def get_ancestor(self, kind: str | re.Pattern[str]) -> Self | None: diff --git a/src/renaissance/syntax_tree/ast_processor.py b/src/renaissance/syntax_tree/ast_processor.py index 5059f9de..5c3f4e70 100644 --- a/src/renaissance/syntax_tree/ast_processor.py +++ b/src/renaissance/syntax_tree/ast_processor.py @@ -14,7 +14,7 @@ from renaissance.syntax_tree import ASTNode from renaissance.syntax_tree.ast_factory import ASTFactory from renaissance.syntax_tree.ast_finder import ASTFinder, find_semantic_kind -from renaissance.syntax_tree.ast_rewriter import ASTRewriter +from renaissance.syntax_tree.ast_rewriter import ASTRewriter, Rewritable from renaissance.syntax_tree.match_finder import PatternMatch from renaissance.syntax_tree.node_protocol import NodeProtocol from renaissance.syntax_tree.semantic_kind import SemanticKind @@ -59,7 +59,7 @@ def root(self) -> ASTNode: def replace( self, new_content: str, - target: ASTNode | Sequence[ASTNode] | PatternMatch | Sequence[PatternMatch], + target: Rewritable | Sequence[Rewritable] | PatternMatch | Sequence[PatternMatch], include_whitespace: bool = True, include_comments: bool = True, ) -> None: @@ -68,7 +68,7 @@ def replace( def remove( self, - target: ASTNode | Sequence[ASTNode] | PatternMatch | Sequence[PatternMatch], + target: Rewritable | Sequence[Rewritable] | PatternMatch | Sequence[PatternMatch], include_whitespace: bool = True, include_comments: bool = True, ) -> None: @@ -78,7 +78,7 @@ def remove( def insert_before( self, new_content: str, - target: ASTNode | Sequence[ASTNode] | PatternMatch | Sequence[PatternMatch], + target: Rewritable | Sequence[Rewritable] | PatternMatch | Sequence[PatternMatch], include_whitespace: bool = True, include_comments: bool = True, ) -> None: @@ -88,7 +88,7 @@ def insert_before( def insert_after( self, new_content: str, - target: ASTNode | Sequence[ASTNode] | PatternMatch | Sequence[PatternMatch], + target: Rewritable | Sequence[Rewritable] | PatternMatch | Sequence[PatternMatch], include_whitespace: bool = True, include_comments: bool = True, ) -> None: diff --git a/src/renaissance/syntax_tree/ast_refactor_actions.py b/src/renaissance/syntax_tree/ast_refactor_actions.py index f8ee9adf..298e1449 100644 --- a/src/renaissance/syntax_tree/ast_refactor_actions.py +++ b/src/renaissance/syntax_tree/ast_refactor_actions.py @@ -16,6 +16,8 @@ from .match_finder import MatchFinder, PatternMatch from .semantic_kind import SemanticKind +# TODO(FrancescoPezzella): Replace with a lazy import in Python 3.15. +# https://github.com/TNO/Renaissance.Py/issues/275 if TYPE_CHECKING: from renaissance.integrations.clang.c_pattern_factory import CPPPatternFactory diff --git a/src/renaissance/syntax_tree/ast_rewriter.py b/src/renaissance/syntax_tree/ast_rewriter.py index 3f939ec7..b9e70799 100644 --- a/src/renaissance/syntax_tree/ast_rewriter.py +++ b/src/renaissance/syntax_tree/ast_rewriter.py @@ -305,10 +305,7 @@ def __check_for_conflicting_rewrites(self) -> None: @staticmethod def __is_nested(node: Rewritable, maybe_ancestor: Rewritable) -> bool: - """Return True if maybe_ancestor is an ancestor of node, walking .parent. - - Not node.is_ancestor_of() - not every Rewritable implements it (e.g. PythonRstNode). - """ + """Return True if maybe_ancestor is an ancestor of node, walking .parent (works for every Rewritable).""" parent = node.parent while parent: if parent is maybe_ancestor: @@ -340,6 +337,11 @@ def no_conflict(node1, rew) -> bool: result = any(no_conflict(node, rew) for rew in rewrite_nodes) + # TODO(FrancescoPezzella): an outer rewrite should suppress the rewrites nested inside it, but both are applied. + # Dropping `and False` is not enough: no_conflict also matches each node with its own rewrite, so result is almost + # always True and apply() would skip nearly every rewrite. A fix has to leave out the node's own rewrite and use a + # true ancestor check such as __is_nested. + # https://github.com/TNO/Renaissance.Py/issues/286 return result and False # TODO: Why `and False` def __replace( @@ -454,6 +456,8 @@ def __insert( else spaces ) # indent the new content except the first line + # TODO(FrancescoPezzella): unlike __replace, this ignores correct_indent, so the re-indentation can't be turned off. + # https://github.com/TNO/Renaissance.Py/issues/283 new_content = TextUtils.shift_right(new_content, indent, start_line=1) if before: @@ -502,6 +506,8 @@ def __compose_replacement(self, replacement: str, matches: Sequence[PatternMatch if regex_match: raw_signature = "".join(regex_match.groups()) place_holder_length = end_index - index + 1 + # TODO(FrancescoPezzella): this also indents the lines inside a multi-line string in the captured text. + # https://github.com/TNO/Renaissance.Py/issues/283 indent_replacement = raw_signature.replace("\n", "\n" + spaces) if ( placeholder.startswith("$$") @@ -538,6 +544,9 @@ def __get_text(self, node: Rewritable) -> str: # the descendants may need to be rewritten as well # rewrites = [rewrite for rewrite in self.rewrites if any(node.is_ancestor_of(rewrite_node) # for rewrite_node in rewrite.nodes)] + # TODO(FrancescoPezzella): is_ancestor_of only exists on ASTNode, so a PatternMatch replacement with placeholders raises + # AttributeError on Python RST and CST nodes. + # https://github.com/TNO/Renaissance.Py/issues/284 rewrites = [rewrite for rewrite in self.rewrites if any(node.is_ancestor_of(rewrite_node) for rewrite_node in rewrite.nodes)] if rewrites: rewriter = _RewriteActions(node, self.encoding, self.correct_indent, rewrites) diff --git a/src/renaissance/utils/import_resolution.py b/src/renaissance/utils/import_resolution.py new file mode 100644 index 00000000..e4a19c26 --- /dev/null +++ b/src/renaissance/utils/import_resolution.py @@ -0,0 +1,185 @@ +# Copyright (C) 2024-2026 TNO +# +# Licensed under the terms specified in the LICENSE file in the root directory +# of this repository, also available at: +# https://github.com/TNO/Renaissance.Py/blob/main/LICENSE + +"""Resolve project-internal `from X import Y` statements to the .py file they import from, and re-anchor them on other files.""" + +from collections.abc import Sequence +from pathlib import Path +from typing import cast + +import libcst as cst +from libcst import matchers +from libcst.helpers import get_full_name_for_node + + +def import_from_module(statement: cst.ImportFrom) -> str | None: + """Return the dotted module an import-from names after its leading dots, or None for a bare `from . import x`.""" + return get_full_name_for_node(statement.module) if statement.module is not None else None + + +def imported_name(alias: cst.ImportAlias) -> str: + """Return the name an import alias binds: its `as` name if it has one, else the imported name.""" + return alias.evaluated_alias or alias.evaluated_name + + +def resolve_project_module(importing_file: Path, project_root: Path, module: str | None, level: int) -> Path | None: + """Resolve one import-from's `(module, level)` to a concrete .py file under `project_root`. + + `level == 0` is an absolute import (`module` is dotted from `project_root`, e.g. + "redis.typing"). `level >= 1` is relative (PEP 328): anchor at `importing_file`'s own + directory for level 1, walking up `level - 1` further parent directories for each extra dot + (`from ..module import x`); `module` is None for a bare `from . import x`, which resolves to + the anchor directory's own `__init__.py`. + + Tries `.py` first, then `/__init__.py`. Returns None if neither exists or if + resolution would walk above `project_root`, as for a stdlib or third-party import. The returned + path is always absolute. + + # TODO: doesn't follow re-exports through an intermediate __init__.py, or handle namespace + # packages (no __init__.py, PEP 420) - out of scope for now. + """ + # A relative path can't walk above itself: Path("a.py").parent.parent is still Path("."). + importing_file = importing_file.absolute() + project_root = project_root.absolute() + if level == 0: + anchor = project_root + else: + anchor = importing_file.parent + for _ in range(level - 1): + anchor = anchor.parent + if project_root not in (anchor, *anchor.parents): + return None + + candidate = anchor / (module.replace(".", "/") + ".py") if module is not None else anchor / "__init__.py" + if candidate.is_file(): + return candidate + if module is not None: + candidate_package = anchor / module.replace(".", "/") / "__init__.py" + if candidate_package.is_file(): + return candidate_package + return None + + +def rebase_relative_module(via_module: str | None, via_level: int, origin_file: Path, module: str | None, level: int) -> str | None: + """Rewrite an import target of the origin module so that the importing file names the same module. + + The importing file reaches the origin (`origin_file`) with `from import ...`, where `via` is + `via_level` dots followed by `via_module`. The origin's own `from import ...` with `level` + dots is re-anchored on that same path, e.g. via ".sub.file_1" and "._compat" give ".sub._compat". + An absolute import (`level == 0`) is returned unchanged. + + Returns the text that follows `from`, or None if the importing file can't name the module: an + absolute `via` to a top-level origin leaves no package for the relative import to start from. + """ + if level == 0: + return module + parts = via_module.split(".") if via_module is not None else [] + if origin_file.name != "__init__.py": + parts = parts[:-1] + dots = via_level + for _ in range(level - 1): + if parts: + parts.pop() + elif dots > 0: + dots += 1 + else: + return None + if dots == 0 and not parts: + return None + if module is not None: + parts.append(module) + return "." * dots + ".".join(parts) + + +def collect_project_imported_names(files: Sequence[Path], project_root: Path) -> dict[Path, frozenset[str]]: + """Map each project file to the names any file in `files` imports or reads from it. + + Two kinds of dependency are recorded against the resolved origin file: + + - `from module import name`: `alias.name` (the name as declared in the origin module, not + `alias.asname`) - an aliased import still depends on the original name existing. + - An attribute read through an imported project module (`import pkg.mod` then `pkg.mod.T`, + `import pkg.mod as m` then `m.T`, `from pkg import mod` then `mod.T`): the attribute name. + + Imports that don't resolve inside `project_root` (stdlib/third-party) are skipped, as is any + file that can't be read or parsed. + """ + imported: dict[Path, set[str]] = {} + for file in files: + try: + module = cst.parse_module(file.read_text(encoding="utf-8")) + except OSError, cst.ParserSyntaxError: + continue + module_bindings: dict[str, Path] = {} + for statement in matchers.findall(module, matchers.ImportFrom() | matchers.Import()): + if isinstance(statement, cst.ImportFrom): + _record_from_import(file, project_root, statement, imported, module_bindings) + else: + _bind_imported_modules(file, project_root, cast("cst.Import", statement), module_bindings) + _record_module_attribute_reads(module, module_bindings, imported) + return {path: frozenset(names) for path, names in imported.items()} + + +def _record_from_import( + file: Path, + project_root: Path, + statement: cst.ImportFrom, + imported: dict[Path, set[str]], + module_bindings: dict[str, Path], +) -> None: + """Record `statement`'s imported names against their origin, and bind any alias that is itself a project module.""" + # TODO(FrancescoPezzella): `from pkg.mod import *` isn't expanded to the names it actually pulls in. + # https://github.com/TNO/Renaissance.Py/issues/279 + module = import_from_module(statement) + level = len(statement.relative) + aliases = [] if isinstance(statement.names, cst.ImportStar) else list(statement.names) + origin = resolve_project_module(file, project_root, module, level) + if origin is not None: + imported.setdefault(origin, set()).update([alias.evaluated_name for alias in aliases] or ["*"]) + for alias in aliases: + submodule = f"{module}.{alias.evaluated_name}" if module is not None else alias.evaluated_name + submodule_origin = resolve_project_module(file, project_root, submodule, level) + if submodule_origin is not None: + module_bindings[imported_name(alias)] = submodule_origin + + +def _bind_imported_modules(file: Path, project_root: Path, statement: cst.Import, module_bindings: dict[str, Path]) -> None: + """Bind the dotted name(s) `statement` makes available to the project module file each one refers to.""" + for alias in statement.names: + if alias.evaluated_alias is not None: + origin = resolve_project_module(file, project_root, alias.evaluated_name, 0) + if origin is not None: + module_bindings[alias.evaluated_alias] = origin + continue + # `import a.b.c` binds `a`, and makes `a.b` and `a.b.c` reachable through it. + parts = alias.evaluated_name.split(".") + for end in range(1, len(parts) + 1): + prefix = ".".join(parts[:end]) + origin = resolve_project_module(file, project_root, prefix, 0) + if origin is not None: + module_bindings[prefix] = origin + + +def _record_module_attribute_reads(module: cst.Module, module_bindings: dict[str, Path], imported: dict[Path, set[str]]) -> None: + """Record every `.` in `module` as a dependency on `attr` in that module's file.""" + if not module_bindings: + return + for node in matchers.findall(module, matchers.Attribute()): + attribute = cast("cst.Attribute", node) + dotted = _dotted_name(attribute.value) + origin = module_bindings.get(dotted) if dotted is not None else None + if origin is not None: + imported.setdefault(origin, set()).add(attribute.attr.value) + + +def _dotted_name(expression: cst.BaseExpression) -> str | None: + """Return `expression` as a dotted name (`a.b.c`) if it is a plain name/attribute chain, else None.""" + if isinstance(expression, cst.Name): + return expression.value + if isinstance(expression, cst.Attribute): + base = _dotted_name(expression.value) + return f"{base}.{expression.attr.value}" if base is not None else None + return None diff --git a/src/renaissance/utils/text_utils.py b/src/renaissance/utils/text_utils.py index ec1774a4..93ed6e7b 100644 --- a/src/renaissance/utils/text_utils.py +++ b/src/renaissance/utils/text_utils.py @@ -23,6 +23,8 @@ class TextUtils: @staticmethod def shift_left(text: str, shift: int, start_line: int = 0) -> str: """Shifts each line of the given text to the left by the specified number of spaces. Only spaces are shifted.""" + # TODO(FrancescoPezzella): every line is shifted, also inside a multi-line string literal, changing its value. + # https://github.com/TNO/Renaissance.Py/issues/283 if shift == 0: return text pattern = re.compile(r"\s{0," + str(shift) + "}(.*)") @@ -57,7 +59,9 @@ def strip_indent(text: str, start_line: int = 0) -> str: @staticmethod def shift_right(text: str, shift: int, start_line: int = 0) -> str: - """Shifts each line of the given text to the left by the specified number of spaces. Only spaces are shifted.""" + """Shifts each line of the given text to the right by the specified number of spaces.""" + # TODO(FrancescoPezzella): every line is shifted, also inside a multi-line string literal, changing its value. + # https://github.com/TNO/Renaissance.Py/issues/283 if shift == 0: return text lines = text.split("\n") diff --git a/test/c_cpp/test_clang_ast_node.py b/test/c_cpp/test_clang_ast_node.py index bc01a007..6afa558b 100644 --- a/test/c_cpp/test_clang_ast_node.py +++ b/test/c_cpp/test_clang_ast_node.py @@ -8,6 +8,7 @@ import pytest from hamcrest import assert_that, has_length, has_string, is_ +from pytest_mock import MockerFixture from renaissance.integrations.clang import ClangASTNode, CPatternFactory from renaissance.syntax_tree import ASTFactory @@ -68,7 +69,7 @@ def test_var_decl_include_semi_column_and_keep_space(self) -> None: "so slicing the (differently-sized) cached byte array with clang's offsets returns " "corrupted text.", ) - def test_signature_after_multibyte_char_when_filesystem_encoding_is_not_utf8(self, mocker) -> None: + def test_signature_after_multibyte_char_when_filesystem_encoding_is_not_utf8(self, mocker: MockerFixture) -> None: """AI: Verify a node's signature offsets remain correct after a multi-byte character when the filesystem encoding isn't UTF-8.""" # 'é' encodes as 1 byte in latin-1 but 2 bytes in UTF-8. libclang parses/reports # offsets against a UTF-8 encoding of the source regardless of the platform's diff --git a/test/extractors/test_code_graph_extractors.py b/test/extractors/test_code_graph_extractors.py index 187dda40..67a5ed19 100644 --- a/test/extractors/test_code_graph_extractors.py +++ b/test/extractors/test_code_graph_extractors.py @@ -12,6 +12,7 @@ import pytest import tree_sitter_python from hamcrest import assert_that, has_item, instance_of, is_, not_ +from pytest_mock import MockerFixture from renaissance.integrations.tree_sitter.extractor import ( BaseCodeGraphExtractor, @@ -46,7 +47,7 @@ def test_is_abstract(self) -> None: with pytest.raises(NotImplementedError): extractor._process_file("file.py", MagicMock()) - def test_extract_calls_process_file_for_each_file(self, mocker, tmp_path) -> None: + def test_extract_calls_process_file_for_each_file(self, mocker: MockerFixture, tmp_path) -> None: """AI: Verify extract() calls _process_file once per input file.""" f1 = tmp_path / "a.py" f1.write_text("x = 1") @@ -75,7 +76,7 @@ def test_extract_skips_file_on_error(self, tmp_path) -> None: # Should not raise extractor.extract([str(tmp_path / "nonexistent.py")]) - def test_save_graph_writes_file(self, tmp_path, mocker) -> None: + def test_save_graph_writes_file(self, tmp_path, mocker: MockerFixture) -> None: """AI: Verify save_graph writes the graph to a graphml file.""" with patch("renaissance.integrations.tree_sitter.adapter.TreeSitterAdapter"): extractor = PythonCodeGraphExtractor("python", tree_sitter_python) diff --git a/test/project/test_project_scanner.py b/test/project/test_project_scanner.py index ba2b3706..cb356755 100644 --- a/test/project/test_project_scanner.py +++ b/test/project/test_project_scanner.py @@ -12,6 +12,7 @@ import pytest from hamcrest import assert_that, calling, contains_inanyorder, empty, equal_to, is_, raises +from pytest_mock import MockerFixture from renaissance.project.project_scanner import ( BearCppScanner, @@ -267,6 +268,20 @@ def test_default_package_dirs_scans_whole_root_dir(self, tmp_path) -> None: assert_that(result, equal_to([str(redis_like / "client.py")])) + @pytest.mark.parametrize("excluded_dir", sorted(PythonScanner.EXCLUDED_DIRS)) + def test_root_dir_inside_excluded_name_is_still_scanned(self, tmp_path: Path, excluded_dir: str) -> None: + """Assert PythonScanner.find_sources only applies EXCLUDED_DIRS below root_dir, never to its ancestors.""" + root = tmp_path / excluded_dir / "project" + noise_dir = root / excluded_dir + noise_dir.mkdir(parents=True) + (noise_dir / "ignored.py").write_text("") + (root / "kept.py").write_text("") + + scanner = PythonScanner(str(root)) + result = [Path(p).name for p in scanner.find_sources()] + + assert_that(result, equal_to(["kept.py"])) + # --------------------------------------------------------------------------- # BearCppScanner @@ -276,7 +291,7 @@ def test_default_package_dirs_scans_whole_root_dir(self, tmp_path) -> None: class TestBearCppScanner: """AI: Tests generating a compilation database via Bear before discovering sources.""" - def test_find_sources_calls_run_bear_when_compile_commands_missing(self, tmp_path, mocker) -> None: + def test_find_sources_calls_run_bear_when_compile_commands_missing(self, tmp_path, mocker: MockerFixture) -> None: """AI: Assert BearCppScanner.find_sources runs Bear when the compile database is missing.""" scanner = BearCppScanner( build_dir=str(tmp_path), @@ -291,7 +306,7 @@ def test_find_sources_calls_run_bear_when_compile_commands_missing(self, tmp_pat assert_that(mock_bear.call_count, is_(1)) - def test_find_sources_does_not_call_run_bear_when_compile_commands_exists(self, tmp_path, mocker) -> None: + def test_find_sources_does_not_call_run_bear_when_compile_commands_exists(self, tmp_path, mocker: MockerFixture) -> None: """AI: Assert BearCppScanner.find_sources skips running Bear when the compile database already exists.""" commands = [{"file": "/src/main.cpp"}] compile_commands = tmp_path / "compile_commands.json" @@ -308,14 +323,14 @@ def test_find_sources_does_not_call_run_bear_when_compile_commands_exists(self, mock_bear.assert_not_called() assert_that(result, equal_to(["/src/main.cpp"])) - def test_run_bear_raises_on_nonzero_exit(self, mocker) -> None: + def test_run_bear_raises_on_nonzero_exit(self, mocker: MockerFixture) -> None: """AI: Assert BearCppScanner.run_bear raises RuntimeError when the Bear subprocess exits non-zero.""" scanner = BearCppScanner() mocker.patch("renaissance.project.project_scanner.subprocess.run", return_value=Mock(returncode=1)) assert_that(calling(scanner.run_bear), raises(RuntimeError)) - def test_run_bear_succeeds_on_zero_exit(self, mocker) -> None: + def test_run_bear_succeeds_on_zero_exit(self, mocker: MockerFixture) -> None: """AI: Assert BearCppScanner.run_bear does not raise when the Bear subprocess exits zero.""" scanner = BearCppScanner() mocker.patch("renaissance.project.project_scanner.subprocess.run", return_value=Mock(returncode=0)) diff --git a/test/python/ast/test_offset_encoding.py b/test/python/ast/test_offset_encoding.py index 269c0a01..83894e37 100644 --- a/test/python/ast/test_offset_encoding.py +++ b/test/python/ast/test_offset_encoding.py @@ -29,15 +29,6 @@ # Each 'e-acute' is one character but two UTF-8 bytes, so character and byte offsets diverge after it. NON_ASCII_SOURCE = 'x = "\u00e9\u00e9\u00e9"\ny = f(1)\n' -CST_ENCODING_REASON = ( - "PythonCstTranslationUnit keeps `content` as str and splits str lines, so convert() returns " - "character offsets, while _RewriteActions slices node.text.encode(...), i.e. UTF-8 bytes. The " - "two units drift apart by one per extra byte, here cutting the slice short mid-codepoint." -) -CST_SPAN_REASON = ( - "WhitespaceInclusivePositionProvider resolves no span for the trailing SimpleStatementLine, so " - "PythonCstTranslationUnit.end_of falls back to 0 and the node reports an end before its start." -) LST_ENCODING_REASON = ( "LSTNode.offset is a byte offset from tree-sitter, but length is len(signature) in characters " "and the signature itself is cut out of the source with byte offsets applied to str, so both " @@ -87,7 +78,7 @@ def test_offsets_address_bytes_for_ascii_source(load: Callable[[str], PythonNode "load", [ pytest.param(_rst_root, id="rst"), - pytest.param(_cst_root, id="cst", marks=pytest.mark.xfail(reason=CST_ENCODING_REASON, strict=True)), + pytest.param(_cst_root, id="cst"), pytest.param(_lst_root, id="lst", marks=pytest.mark.xfail(reason=LST_ENCODING_REASON, strict=True)), ], ) @@ -100,7 +91,7 @@ def test_offsets_address_bytes_for_non_ascii_source(load: Callable[[str], Python "load", [ pytest.param(_rst_root, id="rst"), - pytest.param(_cst_root, id="cst", marks=pytest.mark.xfail(reason=CST_SPAN_REASON, strict=True)), + pytest.param(_cst_root, id="cst"), pytest.param(_lst_root, id="lst"), ], ) @@ -115,7 +106,7 @@ def test_trailing_statement_ends_after_it_starts(load: Callable[[str], PythonNod "load", [ pytest.param(_rst_root, id="rst"), - pytest.param(_cst_root, id="cst", marks=pytest.mark.xfail(reason=CST_ENCODING_REASON, strict=True)), + pytest.param(_cst_root, id="cst"), pytest.param(_lst_root, id="lst", marks=pytest.mark.xfail(reason=LST_ENCODING_REASON, strict=True)), ], ) diff --git a/test/python/ast/test_python_cst_node.py b/test/python/ast/test_python_cst_node.py index 500e76be..31dd6cdf 100644 --- a/test/python/ast/test_python_cst_node.py +++ b/test/python/ast/test_python_cst_node.py @@ -77,7 +77,7 @@ def next_me(): assert_that(me.name, is_("mememe")) assert_that(me.preceding_sibling.name, is_("previous_me")) assert_that(me.next_sibling.name, is_("next_me")) - assert_that(me.parent.parent.name, is_("Parent")) + assert_that(me.parent and me.parent.parent and me.parent.parent.name, is_("Parent")) # all children are mashed together assert_that(me.children, has_length(8)) diff --git a/test/python/ast/test_python_cst_references.py b/test/python/ast/test_python_cst_references.py new file mode 100644 index 00000000..2342d2f2 --- /dev/null +++ b/test/python/ast/test_python_cst_references.py @@ -0,0 +1,120 @@ +# Copyright (C) 2024-2026 TNO +# +# Licensed under the terms specified in the LICENSE file in the root directory +# of this repository, also available at: +# https://github.com/TNO/Renaissance.Py/blob/main/LICENSE + +"""Tests for PythonCstNode.references and referenced_by, resolved with libcst's scope analysis.""" + +from typing import TYPE_CHECKING + +import pytest +from hamcrest import assert_that, contains_inanyorder, equal_to + +from renaissance.integrations.python.ast.cst_node import PythonCstNode + +# TODO(FrancescoPezzella): Replace with a lazy import in Python 3.15. +# https://github.com/TNO/Renaissance.Py/issues/275 +if TYPE_CHECKING: + from collections.abc import Iterator + +HEADER = 'from typing import Generic, Literal, TypeAlias, TypeVar, cast\nT = TypeVar("T")\n' + + +def _load(body: str) -> PythonCstNode: + """Parse HEADER followed by body into a PythonCstNode tree.""" + return PythonCstNode.load_from_text(HEADER + body, "refs.py") + + +def traverse(node: PythonCstNode) -> Iterator[PythonCstNode]: + """Yield node and all its descendants, depth first.""" + yield node + for child in node.children: + yield from traverse(child) + + +def _declared_t(root: PythonCstNode) -> PythonCstNode: + """Return the Name node that `T = TypeVar("T")` assigns.""" + return next( + node + for node in traverse(root) + if node.parser_kind == "Name" and node.signature == "T" and node.parent is not None and node.parent.parser_kind == "AssignTarget" + ) + + +class TestPythonCstReferences: + """References between a module-level name and its uses.""" + + @pytest.mark.parametrize( + ("body", "expected_uses"), + [ + pytest.param("def b(x: T) -> T: ...\n", ["T", "T"], id="plain-names"), + pytest.param('def b(x: "T") -> None: ...\n', ['"T"'], id="whole-string-annotation"), + pytest.param('def b(x: "list[T]") -> None: ...\n', ['"list[T]"'], id="string-inside-annotation"), + pytest.param('def b(x: list["T"]) -> None: ...\n', ['"T"'], id="nested-string"), + pytest.param('def b(x: object) -> None:\n cast("T", x)\n', ['"T"'], id="cast-in-body"), + pytest.param("class Box(Generic[T]): ...\n", ["T"], id="generic-class-base"), + pytest.param( + 'A: TypeAlias = "list[T]"\n', + ['"list[T]"'], + id="type-alias-string", + marks=pytest.mark.xfail(reason="libcst's ScopeProvider doesn't read the string value of a TypeAlias.", strict=True), + ), + ], + ) + def test_definition_and_uses_point_at_each_other(self, body: str, expected_uses: list[str]) -> None: + """Every use of T is in T's referenced_by, and each of those uses references T's definition.""" + declared = _declared_t(_load(body)) + + uses = [reference.node for reference in declared.referenced_by] + + assert_that([use.signature for use in uses], contains_inanyorder(*expected_uses)) + for use in uses: + assert_that([reference.node for reference in use.references], equal_to([declared])) + + @pytest.mark.parametrize( + "body", + [ + pytest.param("def b[T](x: T) -> T: ...\n", id="pep695-parameter"), + pytest.param('def b[T](x: "T") -> "list[T]": ...\n', id="pep695-parameter-in-strings"), + pytest.param("def outer[T](x: T) -> T:\n def inner(y: T) -> T:\n return y\n return inner(x)\n", id="closure"), + pytest.param("def outer() -> None:\n def inner[T](x: T) -> T:\n return x\n", id="nested-pep695-function"), + pytest.param("def b() -> None:\n T = 1\n print(T)\n", id="local-variable"), + pytest.param('d = {}\nd["T"]\n', id="dict-key-string"), + pytest.param('def b(x: Literal["T"]) -> None: ...\n', id="literal-string"), + ], + ) + def test_uses_bound_to_another_name_are_not_references(self, body: str) -> None: + """A T that resolves to a type parameter, a local variable or a plain string is not a use of the module-level T.""" + assert_that(_declared_t(_load(body)).referenced_by, equal_to([])) + + @pytest.mark.parametrize( + ("body", "expected_uses"), + [ + pytest.param("def b(x: T) -> T: ...\n", ["T", "T"], id="annotations"), + pytest.param('def b(x: "list[T]") -> None: ...\n', ['"list[T]"'], id="string-annotation"), + pytest.param('y = cast("T", 1)\n', ['"T"'], id="cast"), + pytest.param("w = T\n", [], id="evaluated-at-once"), + ], + ) + def test_lazily_evaluated_use_above_the_declaration_is_a_reference(self, body: str, expected_uses: list[str]) -> None: + """An annotation or type hint above `T = TypeVar("T")` is a use of it; a value read there at once is not.""" + root = PythonCstNode.load_from_text( + f'from __future__ import annotations\nfrom typing import TypeVar, cast\n{body}T = TypeVar("T")\n', "refs.py" + ) + declared = _declared_t(root) + + uses = [reference.node for reference in declared.referenced_by] + + assert_that([use.signature for use in uses], contains_inanyorder(*expected_uses)) + for use in uses: + assert_that([reference.node for reference in use.references], equal_to([declared])) + + def test_import_reports_which_name_each_use_refers_to(self) -> None: + """An import binding several names lists the uses of each, with the imported name in the reference properties.""" + root = _load('def b(x: object) -> None:\n cast("int", x)\n') + import_from = next(node for node in traverse(root) if node.parser_kind == "ImportFrom") + + names = [(str(reference.properties["name"]), reference.node.signature) for reference in import_from.referenced_by] + + assert_that(sorted(names), equal_to([("TypeVar", "TypeVar"), ("cast", "cast")])) diff --git a/test/recipes/conftest.py b/test/recipes/conftest.py new file mode 100644 index 00000000..c82bbb08 --- /dev/null +++ b/test/recipes/conftest.py @@ -0,0 +1,37 @@ +# Copyright (C) 2024-2026 TNO +# +# Licensed under the terms specified in the LICENSE file in the root directory +# of this repository, also available at: +# https://github.com/TNO/Renaissance.Py/blob/main/LICENSE + +"""Shared fixture for the TypeVarCheck tests.""" + +import textwrap +from collections.abc import Callable +from pathlib import Path + +import pytest +from pytest_mock import MockerFixture + +from renaissance.integrations.python.ast.cst_node import PythonCstNode +from renaissance.recipes.type_var_check import PEP_695_MINIMUM, TypeVarCheck + + +@pytest.fixture +def create_type_var_check(mocker: MockerFixture) -> Callable[[str], TypeVarCheck]: + """Build a TypeVarCheck for in-memory, dedented source text, with min_python 3.12 so PEP 695 conversion is enabled. + + `PythonFactory.create` is mocked so nothing touches the filesystem. + """ + + def _create(text: str) -> TypeVarCheck: + mocker.patch( + "renaissance.integrations.python.ast.factory.PythonFactory.create", + return_value=PythonCstNode.load_from_text(textwrap.dedent(text), "x.py"), + ) + subject = TypeVarCheck(Path("x.py")) + subject.in_memory = True + subject.min_python = PEP_695_MINIMUM + return subject + + return _create diff --git a/test/recipes/test_python_refactoring.py b/test/recipes/test_python_refactoring.py index 129556d5..cb6e84d9 100644 --- a/test/recipes/test_python_refactoring.py +++ b/test/recipes/test_python_refactoring.py @@ -8,12 +8,18 @@ import keyword import textwrap +from pathlib import Path +from typing import cast from unittest.mock import patch -from hamcrest import assert_that, contains_string, is_ +import pytest +from hamcrest import assert_that, contains_string, equal_to, is_ from hypothesis import given, settings from hypothesis import strategies as st +from pytest_mock import MockerFixture +from renaissance.integrations.python.ast.cst_node import PythonCstNode +from renaissance.integrations.python.ast.factory import PythonFactory from renaissance.integrations.python.ast.rst_node import PythonRstNode from renaissance.recipes.python_refactoring import PythonRefactoring from renaissance.recipes.unit_to_pytest import UnitToPytest @@ -42,18 +48,25 @@ class TestPythonRefactoring: .filter(lambda name: not keyword.iskeyword(name)) ) - def _patch_factory(self, mocker, text="pass", filename="test_foo.py") -> None: + def _patch_factory( + self, + mocker: MockerFixture, + text: str = "pass", + filename: str = "test_foo.py", + node_class: type[PythonRstNode | PythonCstNode] = PythonRstNode, + ) -> None: + """Make PythonFactory.create return text parsed by node_class instead of reading filename from disk.""" code = textwrap.dedent(text) mocker.patch( "renaissance.integrations.python.ast.factory.PythonFactory.create", - return_value=PythonRstNode.load_from_text(code, filename), + return_value=node_class.load_from_text(code, filename), ) # ------------------------------------------------------------------ # __init__ / replace_stmt # ------------------------------------------------------------------ - def test_init_sets_default_list_patterns(self, mocker) -> None: + def test_init_sets_default_list_patterns(self, mocker: MockerFixture) -> None: """AI: Verify a subclass's __init__ sets its black/white list patterns from subclass defaults.""" self._patch_factory(mocker) subject = UnitToPytest("test_foo.py") @@ -61,7 +74,7 @@ def test_init_sets_default_list_patterns(self, mocker) -> None: assert_that(subject.black_list_pattern, is_("utils_for_test")) assert_that(subject.white_list_pattern, is_("test")) - def test_replace_stmt_rewrites_matching_pattern(self, mocker) -> None: + def test_replace_stmt_rewrites_matching_pattern(self, mocker: MockerFixture) -> None: """AI: Verify replace_stmt rewrites a statement matching an exact pattern.""" self._patch_factory( mocker, @@ -76,7 +89,7 @@ def test_replace_stmt_rewrites_matching_pattern(self, mocker) -> None: assert_that(subject.apply_to_string(), contains_string("import pytest")) assert_that(subject.apply_to_string(), contains_string("from hamcrest import *")) - def test_replace_stmt_expands_variadic_captures(self, mocker) -> None: + def test_replace_stmt_expands_variadic_captures(self, mocker: MockerFixture) -> None: """AI: Verify replace_stmt rewrites a statement matched via a variadic ($$symbols) capture.""" self._patch_factory( mocker, @@ -97,7 +110,7 @@ def test_replace_stmt_expands_variadic_captures(self, mocker) -> None: # extract_call_arguments() # ------------------------------------------------------------------ - def test_extract_call_arguments_positional_only(self, mocker) -> None: + def test_extract_call_arguments_positional_only(self, mocker: MockerFixture) -> None: """Verify extract_call_arguments returns only positional values for positional-only calls.""" self._patch_factory(mocker, "fun(1, 'x')") subject = UnitToPytest("test_foo.py") @@ -108,7 +121,7 @@ def test_extract_call_arguments_positional_only(self, mocker) -> None: assert_that(positional, is_(["1", "'x'"])) assert_that(keyword, is_({})) - def test_extract_call_arguments_keyword_only(self, mocker) -> None: + def test_extract_call_arguments_keyword_only(self, mocker: MockerFixture) -> None: """Verify extract_call_arguments returns only keyword values for keyword-only calls.""" self._patch_factory(mocker, "fun(a=1, b='x')") subject = UnitToPytest("test_foo.py") @@ -119,7 +132,7 @@ def test_extract_call_arguments_keyword_only(self, mocker) -> None: assert_that(positional, is_([])) assert_that(keyword, is_({"a": "1", "b": "'x'"})) - def test_extract_call_arguments_mixed(self, mocker) -> None: + def test_extract_call_arguments_mixed(self, mocker: MockerFixture) -> None: """Verify extract_call_arguments splits mixed positional and keyword arguments correctly.""" self._patch_factory(mocker, "fun(1, 2, b='x', c=other)") subject = UnitToPytest("test_foo.py") @@ -130,7 +143,7 @@ def test_extract_call_arguments_mixed(self, mocker) -> None: assert_that(positional, is_(["1", "2"])) assert_that(keyword, is_({"b": "'x'", "c": "other"})) - def test_extract_call_arguments_accepts_node_inside_call(self, mocker) -> None: + def test_extract_call_arguments_accepts_node_inside_call(self, mocker: MockerFixture) -> None: """Verify extract_call_arguments works when given a child node nested inside a call.""" self._patch_factory(mocker, "fun(1, k='v')") subject = UnitToPytest("test_foo.py") @@ -142,7 +155,7 @@ def test_extract_call_arguments_accepts_node_inside_call(self, mocker) -> None: assert_that(positional, is_(["1"])) assert_that(keyword, is_({"k": "'v'"})) - def test_extract_call_arguments_walks_parent_chain_to_first_call(self, mocker) -> None: + def test_extract_call_arguments_walks_parent_chain_to_first_call(self, mocker: MockerFixture) -> None: """Verify extract_call_arguments climbs ancestors and uses the first enclosing call node.""" self._patch_factory(mocker, "outer(inner(1), k=2)") subject = UnitToPytest("test_foo.py") @@ -156,7 +169,7 @@ def test_extract_call_arguments_walks_parent_chain_to_first_call(self, mocker) - assert_that(positional, is_(["1"])) assert_that(keyword, is_({})) - def test_extract_call_arguments_returns_empty_for_non_call_node(self, mocker) -> None: + def test_extract_call_arguments_returns_empty_for_non_call_node(self, mocker: MockerFixture) -> None: """Verify extract_call_arguments returns empty positional/keyword results for non-call nodes.""" self._patch_factory(mocker, "x = 1") subject = UnitToPytest("test_foo.py") @@ -194,7 +207,7 @@ def test_extract_call_arguments_hypothesis_roundtrip(self, positional_args, keyw # class_declares_base() / class_base_arguments() # ------------------------------------------------------------------ - def test_class_base_arguments_returns_declared_bases(self, mocker) -> None: + def test_class_base_arguments_returns_declared_bases(self, mocker: MockerFixture) -> None: """Verify class_base_arguments returns all explicitly declared base classes.""" self._patch_factory(mocker, "class Child(Base1, Base2):\n pass") subject = UnitToPytest("test_foo.py") @@ -204,7 +217,7 @@ def test_class_base_arguments_returns_declared_bases(self, mocker) -> None: assert_that(bases, is_(["Base1", "Base2"])) - def test_class_base_arguments_returns_empty_without_bases(self, mocker) -> None: + def test_class_base_arguments_returns_empty_without_bases(self, mocker: MockerFixture) -> None: """Verify class_base_arguments returns an empty list for classes without base classes.""" self._patch_factory(mocker, "class Child:\n pass") subject = UnitToPytest("test_foo.py") @@ -214,7 +227,7 @@ def test_class_base_arguments_returns_empty_without_bases(self, mocker) -> None: assert_that(bases, is_([])) - def test_class_declares_base_checks_base_membership(self, mocker) -> None: + def test_class_declares_base_checks_base_membership(self, mocker: MockerFixture) -> None: """Verify class_declares_base reports whether a requested base class is present.""" self._patch_factory(mocker, "class Child(Base):\n pass") subject = UnitToPytest("test_foo.py") @@ -223,7 +236,7 @@ def test_class_declares_base_checks_base_membership(self, mocker) -> None: assert_that(subject.class_declares_base(class_node, "Base"), is_(True)) assert_that(subject.class_declares_base(class_node, "Other"), is_(False)) - def test_class_declares_base_does_not_follow_transitive_inheritance(self, mocker) -> None: + def test_class_declares_base_does_not_follow_transitive_inheritance(self, mocker: MockerFixture) -> None: """Verify class_declares_base checks only direct bases, not transitive ancestors.""" self._patch_factory( mocker, @@ -249,7 +262,7 @@ class Bottom(Middle): # process() — skip branch # ------------------------------------------------------------------ - def test_process_skips_file_matching_black_list(self, mocker, capsys) -> None: + def test_process_skips_file_matching_black_list(self, mocker: MockerFixture, capsys) -> None: """AI: Verify process() skips and does not run the refactor when the filename matches the black list.""" self._patch_factory(mocker, "pass", "utils_for_test_foo.py") run_spy = mocker.patch("renaissance.recipes.unit_to_pytest.UnitToPytest.run") @@ -258,7 +271,7 @@ def test_process_skips_file_matching_black_list(self, mocker, capsys) -> None: assert_that(captured.out, contains_string("skipping")) assert_that(run_spy.call_count, is_(0)) - def test_process_skips_file_not_matching_white_list(self, mocker, capsys) -> None: + def test_process_skips_file_not_matching_white_list(self, mocker: MockerFixture, capsys) -> None: """AI: Verify process() skips and does not run the refactor when the filename doesn't match the white list.""" self._patch_factory(mocker, "pass", "my_module.py") run_spy = mocker.patch("renaissance.recipes.unit_to_pytest.UnitToPytest.run") @@ -271,7 +284,7 @@ def test_process_skips_file_not_matching_white_list(self, mocker, capsys) -> Non # process() — run branch # ------------------------------------------------------------------ - def test_process_runs_refactor_on_matching_file(self, mocker, capsys) -> None: + def test_process_runs_refactor_on_matching_file(self, mocker: MockerFixture, capsys) -> None: """AI: Verify process() runs the refactor when the filename matches both white and black list patterns.""" self._patch_factory(mocker, "pass", "test_foo.py") run_spy = mocker.patch("renaissance.recipes.unit_to_pytest.UnitToPytest.run") @@ -284,15 +297,23 @@ def test_process_runs_refactor_on_matching_file(self, mocker, capsys) -> None: # body property # ------------------------------------------------------------------ - def test_body_returns_module_level_statements(self, mocker) -> None: - """AI: Verify the body property returns the module's top-level statements.""" - self._patch_factory( - mocker, - """ - x = 1 - y = 2 - """, - "test_foo.py", - ) - subject = UnitToPytest("test_foo.py") - assert_that(len(subject.body), is_(2)) + @pytest.mark.parametrize("node_class", [PythonRstNode, PythonCstNode], ids=["rst", "cst"]) + def test_body_returns_module_level_statements(self, mocker: MockerFixture, node_class: type[PythonRstNode | PythonCstNode]) -> None: + """Verify the body property returns the module's top-level statements, whichever backend parsed the file.""" + self._patch_factory(mocker, "import os\nx = 1\ndef f():\n return x\n", "test_foo.py", node_class) + subject = PythonRefactoring(Path("test_foo.py"), node_class=node_class) + + statements: list[str] = [str(statement.signature).strip() for statement in subject.body] + + assert_that(statements, equal_to(["import os", "x = 1", "def f():\n return x"])) + + @pytest.mark.parametrize("node_class", [PythonRstNode, PythonCstNode], ids=["rst", "cst"]) + def test_recipe_rewrites_with_the_chosen_backend(self, mocker: MockerFixture, node_class: type[PythonRstNode | PythonCstNode]) -> None: + """Verify a recipe parses with the node class it is given and rewrites through that backend's nodes.""" + self._patch_factory(mocker, "x = 1\ny = 2\n", "test_foo.py", node_class) + subject = PythonRefactoring(Path("test_foo.py"), node_class=node_class) + + subject.replace("x = 3", subject.body[0], include_whitespace=False, include_comments=False) + + assert_that(cast("PythonFactory", subject.factory).clazz is node_class, is_(True)) + assert_that(subject.apply_to_string(), is_("x = 3\ny = 2\n")) diff --git a/test/recipes/test_refactor_with_rewrite.py b/test/recipes/test_refactor_with_rewrite.py index 66810a86..2475d004 100644 --- a/test/recipes/test_refactor_with_rewrite.py +++ b/test/recipes/test_refactor_with_rewrite.py @@ -10,6 +10,7 @@ import pytest from hamcrest import assert_that, is_ +from pytest_mock import MockerFixture from renaissance.integrations.python.ast.rst_node import PythonRstNode from renaissance.recipes.python_refactoring import PythonRefactoring @@ -18,7 +19,7 @@ class TestRefactorWithRewrite: """AI: Tests for PythonRefactoring's rewrite-based refactoring support.""" - def _create(self, mocker, text) -> PythonRefactoring: + def _create(self, mocker: MockerFixture, text) -> PythonRefactoring: code = textwrap.dedent(text) mocker.patch( "renaissance.integrations.python.ast.factory.PythonFactory.create", @@ -29,7 +30,7 @@ def _create(self, mocker, text) -> PythonRefactoring: return subject @pytest.mark.skip("comment are not correctly calculated") - def test_refactor_with_comment_and_spaces(self, mocker) -> None: + def test_refactor_with_comment_and_spaces(self, mocker: MockerFixture) -> None: """AI: Verify rewrite-based refactoring preserves kept comments while removing others (comment placement TBD).""" refactoring = self._create( mocker, @@ -77,7 +78,7 @@ def test_functions(self): # end comments to keep"""), ) - def test_refactor_replace_multi_placeholder(self, mocker) -> None: + def test_refactor_replace_multi_placeholder(self, mocker: MockerFixture) -> None: """Test case showing a replacement of a multi placeholder that matches a non-empty list of AST nodes in the code.""" refactoring = self._create(mocker, "def f(a):\n f(2, 0)") function_call = refactoring.pattern_factory.create_expression("f($$params, 0)") @@ -88,7 +89,7 @@ def test_refactor_replace_multi_placeholder(self, mocker) -> None: assert_that(refactoring.apply_to_string(), is_("def f(a):\n f(1, 0)")) @pytest.mark.skip("empty array can't be detected") - def test_refactor_replace_multi_placeholder_empty(self, mocker) -> None: + def test_refactor_replace_multi_placeholder_empty(self, mocker: MockerFixture) -> None: """Test case showing a replacement of a multi placeholder that matches an empty list of AST nodes in the code.""" # TODO: is this the behaviour we want? # Can $$params be empty and a comma absent, while present in the pattern. diff --git a/test/recipes/test_simplify_renaissance.py b/test/recipes/test_simplify_renaissance.py index 2523b80e..c445d983 100644 --- a/test/recipes/test_simplify_renaissance.py +++ b/test/recipes/test_simplify_renaissance.py @@ -9,6 +9,7 @@ import textwrap from hamcrest import assert_that, contains_string, is_, not_ +from pytest_mock import MockerFixture from renaissance.integrations.python.ast.rst_node import PythonRstNode from renaissance.recipes.simplify_renaissance import SimplifyRenaissance @@ -17,7 +18,7 @@ class TestSimplifyRenaissance: """AI: Tests for the SimplifyRenaissance recipe.""" - def _create(self, mocker, text) -> SimplifyRenaissance: + def _create(self, mocker: MockerFixture, text) -> SimplifyRenaissance: code = textwrap.dedent(text) mocker.patch( "renaissance.integrations.python.ast.factory.PythonFactory.create", @@ -27,13 +28,13 @@ def _create(self, mocker, text) -> SimplifyRenaissance: subject.in_memory = True return subject - def test_init_sets_white_and_black_list(self, mocker) -> None: + def test_init_sets_white_and_black_list(self, mocker: MockerFixture) -> None: """AI: Verify __init__ sets the white and black list patterns from the recipe's own defaults.""" subject = self._create(mocker, "pass") assert_that(subject.white_list_pattern, is_("unit_to_pytest")) assert_that(subject.black_list_pattern, is_("SimplifyRenaissance")) - def test_run_skips_file_matching_black_list(self, mocker, capsys) -> None: + def test_run_skips_file_matching_black_list(self, mocker: MockerFixture, capsys) -> None: """AI: Verify run() skips a file whose name matches the black list pattern.""" mocker.patch( "renaissance.integrations.python.ast.factory.PythonFactory.create", @@ -45,7 +46,7 @@ def test_run_skips_file_matching_black_list(self, mocker, capsys) -> None: captured = capsys.readouterr() assert_that(captured.out, contains_string("skipping")) - def test_run_skips_file_not_matching_white_list(self, mocker, capsys) -> None: + def test_run_skips_file_not_matching_white_list(self, mocker: MockerFixture, capsys) -> None: """AI: Verify run() skips a file whose name doesn't match the white list pattern.""" mocker.patch( "renaissance.integrations.python.ast.factory.PythonFactory.create", @@ -57,7 +58,7 @@ def test_run_skips_file_not_matching_white_list(self, mocker, capsys) -> None: captured = capsys.readouterr() assert_that(captured.out, contains_string("skipping")) - def test_run_rewrites_expansion_signature_access(self, mocker) -> None: + def test_run_rewrites_expansion_signature_access(self, mocker: MockerFixture) -> None: """AI: Verify a manual replace_stmt call rewrites .expansions[...] signature access to bracket indexing.""" subject = self._create( mocker, @@ -70,7 +71,7 @@ def foo(): assert_that(subject.apply_to_string(), contains_string('val= match["$key"]')) assert_that(subject.apply_to_string(), not_(contains_string(".expansions"))) - def test_run_rewrites_factory_create_from_text(self, mocker) -> None: + def test_run_rewrites_factory_create_from_text(self, mocker: MockerFixture) -> None: """AI: Verify run() rewrites ASTFactory.create_from_text calls to the simplified load_from_text form.""" subject = self._create( mocker, @@ -84,7 +85,7 @@ def foo(): assert_that(subject.apply_to_string(), contains_string("PythonASTNode.load_from_text(code, name)")) assert_that(subject.apply_to_string(), not_(contains_string("ASTFactory"))) - def test_run_processes_matching_file(self, mocker, capsys) -> None: + def test_run_processes_matching_file(self, mocker: MockerFixture, capsys) -> None: """AI: Verify run() processes a matching file and reports it via stdout.""" subject = self._create(mocker, "pass") subject.run() diff --git a/test/recipes/test_step_runner.py b/test/recipes/test_step_runner.py new file mode 100644 index 00000000..5079b417 --- /dev/null +++ b/test/recipes/test_step_runner.py @@ -0,0 +1,56 @@ +# Copyright (C) 2024-2026 TNO +# +# Licensed under the terms specified in the LICENSE file in the root directory +# of this repository, also available at: +# https://github.com/TNO/Renaissance.Py/blob/main/LICENSE + +"""Tests for Step/run_steps.""" + +from collections.abc import Callable + +import pytest +from hamcrest import assert_that, equal_to +from pytest_mock import MockerFixture + +from renaissance.recipes.python_refactoring import PythonRefactoring +from renaissance.recipes.step_runner import Step, run_steps + + +class TestRunSteps: + """See module docstring.""" + + def test_collects_each_steps_result_under_its_own_label_in_order(self, mocker: MockerFixture) -> None: + """AI: Verify run_steps returns each step's result keyed by its label, preserving step order.""" + recipe = mocker.Mock(spec=PythonRefactoring) + steps = [ + Step("first", recipe, lambda: {"A": "fixed"}), + Step("second", recipe, lambda: {"B": "unsafe"}), + ] + + result = run_steps(steps) + + assert_that(result, equal_to({"first": {"A": "fixed"}, "second": {"B": "unsafe"}})) + assert_that(list(result.keys()), equal_to(["first", "second"])) + + @pytest.mark.parametrize( + ("action_result", "expect_commit"), + [ + ({"A": "fixed"}, True), + ({"A": "unsafe"}, False), + ({}, False), + ({"A": "fixed", "B": "unsafe"}, True), + ], + ) + def test_commits_only_when_a_step_fixed_something( + self, + mocker: MockerFixture, + action_result: dict[str, str], + expect_commit: bool, # noqa: FBT001 + ) -> None: + """AI: Verify a step's recipe is committed only when its action reports at least one "fixed" result.""" + recipe = mocker.Mock(spec=PythonRefactoring) + action: Callable[[], dict[str, str]] = lambda: action_result # noqa: E731 + + run_steps([Step("only", recipe, action)]) + + assert_that(recipe.commit.called, equal_to(expect_commit)) diff --git a/test/recipes/test_taut_to_python_unittest.py b/test/recipes/test_taut_to_python_unittest.py index 3b06316d..b9e3b2e1 100644 --- a/test/recipes/test_taut_to_python_unittest.py +++ b/test/recipes/test_taut_to_python_unittest.py @@ -11,6 +11,7 @@ import pytest from hamcrest import assert_that, ends_with, is_ +from pytest_mock import MockerFixture import targets import test_data.test_class as tst_class @@ -31,7 +32,7 @@ def test_init(self) -> None: subject = TautToPythonUnittest(Path(targets.__file__).parent / "taut/taut_test.py") assert_that(subject.filename, ends_with("taut_test.py")) - def _create(self, mocker, text) -> TautToPythonUnittest: + def _create(self, mocker: MockerFixture, text) -> TautToPythonUnittest: code = textwrap.dedent(text) mocker.patch( "renaissance.integrations.python.ast.factory.PythonFactory.create", @@ -50,7 +51,7 @@ def _create(self, mocker, text) -> TautToPythonUnittest: ), ], ) - def test_remove_import(self, input_code, expected_code, mocker) -> None: + def test_remove_import(self, input_code, expected_code, mocker: MockerFixture) -> None: """AI: Verify remove_taut_import removes the TAUT import while keeping other imports.""" subject = self._create(mocker, input_code) subject.remove_taut_import() @@ -70,7 +71,7 @@ def test_remove_import(self, input_code, expected_code, mocker) -> None: ), ], ) - def test_replace_taut(self, input_code, expected_code, mocker) -> None: + def test_replace_taut(self, input_code, expected_code, mocker: MockerFixture) -> None: """AI: Verify replace_taut rewrites TAUT.TestCase base classes to unittest.TestCase.""" subject = self._create(mocker, input_code) subject.replace_taut() @@ -86,7 +87,7 @@ def test_replace_taut(self, input_code, expected_code, mocker) -> None: ), ], ) - def test_replace_skip(self, input_code, expected_code, mocker) -> None: + def test_replace_skip(self, input_code, expected_code, mocker: MockerFixture) -> None: """AI: Verify replace_taut_skip rewrites @TAUT.skip_test to @unittest.skip.""" subject = self._create(mocker, input_code) subject.replace_taut_skip() @@ -100,7 +101,7 @@ def test_replace_skip(self, input_code, expected_code, mocker) -> None: (tst_testdoubles.test_indent_fun, tst_testdoubles.test_indent_fun_new, " "), ], ) - def test_indentation(self, input_code, expected_code, indent, mocker) -> None: + def test_indentation(self, input_code, expected_code, indent, mocker: MockerFixture) -> None: """AI: Verify move_indent adjusts a statement's indentation to the given prefix.""" subject = self._create(mocker, input_code) subject.move_indent(indent) @@ -116,7 +117,7 @@ def test_indentation(self, input_code, expected_code, indent, mocker) -> None: ), ], ) - def test_replace_import(self, input_code, expected_code, mocker) -> None: + def test_replace_import(self, input_code, expected_code, mocker: MockerFixture) -> None: """AI: Verify replace_taut_import rewrites a mock/TAUT import to the unittest.mock/mock fallback try-import.""" subject = self._create(mocker, input_code) subject.replace_taut_import() @@ -137,7 +138,7 @@ def test_replace_import(self, input_code, expected_code, mocker) -> None: # ('EMRWxREAD.emrwxread.set_retval(0)', 'self.emrwxread.set_retval(0)') ], ) - def test_add_self(self, input_code, expected_code, mocker) -> None: + def test_add_self(self, input_code, expected_code, mocker: MockerFixture) -> None: """AI: Verify add_self prefixes bare test-double variable names with self.""" subject = self._create(mocker, input_code) subject.add_self() @@ -153,7 +154,7 @@ def test_add_self(self, input_code, expected_code, mocker) -> None: ), ], ) - def test_remove_decorator(self, input_code, expected_code, mocker) -> None: + def test_remove_decorator(self, input_code, expected_code, mocker: MockerFixture) -> None: """AI: Verify remove_decorator strips the @TAUT.log_stub decorator from a function.""" subject = self._create(mocker, input_code) subject.remove_decorator() @@ -168,7 +169,7 @@ def test_remove_decorator(self, input_code, expected_code, mocker) -> None: ("self.assert_true(len(listA), 5)", "self.assertTrue(len(listA), 5)"), ], ) - def test_convert_assert(self, input_code, expected_code, mocker) -> None: + def test_convert_assert(self, input_code, expected_code, mocker: MockerFixture) -> None: """AI: Verify convert_assert rewrites snake_case assert_* calls to unittest's assertEqual/assertFalse/assertTrue.""" subject = self._create(mocker, input_code) subject.convert_assert() @@ -176,7 +177,7 @@ def test_convert_assert(self, input_code, expected_code, mocker) -> None: assert_that(result, is_(expected_code)) @pytest.mark.parametrize("input_code, expected_code", [(tst_code.taut_code, tst_code.result_code)]) - def test_log_abcdxtl(self, input_code, expected_code, mocker) -> None: + def test_log_abcdxtl(self, input_code, expected_code, mocker: MockerFixture) -> None: """AI: Verify replace_log_compxtl rewrites the ABCDxTL log-component call in the sample TAUT code.""" subject = self._create(mocker, input_code) subject.in_memory = True @@ -185,7 +186,7 @@ def test_log_abcdxtl(self, input_code, expected_code, mocker) -> None: assert_that(result, is_(expected_code)) @pytest.mark.parametrize("input_code, insert_code", [(tst_insert.input_code, tst_insert.insert_code)]) - def test_insert_class(self, input_code, insert_code, mocker) -> None: + def test_insert_class(self, input_code, insert_code, mocker: MockerFixture) -> None: """AI: Verify insert_class appends the generated class boilerplate after the existing code.""" subject = self._create(mocker, input_code) subject.insert_class() @@ -193,14 +194,14 @@ def test_insert_class(self, input_code, insert_code, mocker) -> None: assert_that(result, is_(input_code + insert_code)) @pytest.mark.parametrize("input_code, expected_code", [(tst_class.set_up, tst_class.new_set_up)]) - def test_setup(self, input_code, expected_code, mocker) -> None: + def test_setup(self, input_code, expected_code, mocker: MockerFixture) -> None: """AI: Verify convert_setup rewrites the sample set_up method to unittest's setUp form.""" subject = self._create(mocker, input_code) subject.convert_setup() result = subject.apply_to_string() assert result == expected_code - def test_teardown(self, mocker) -> None: + def test_teardown(self, mocker: MockerFixture) -> None: """AI: Verify convert_teardown rewrites a simple tear_down method to unittest's tearDown form.""" subject = self._create(mocker, tst_class.tear_down_simple) subject.convert_teardown() @@ -208,7 +209,7 @@ def test_teardown(self, mocker) -> None: assert result == tst_class.tear_down_simple_new @pytest.mark.parametrize("input_code, expected_code", [(tst_class.tear_down, tst_class.new_tear_down)]) - def test_teardown_refactor(self, input_code, expected_code, mocker) -> None: + def test_teardown_refactor(self, input_code, expected_code, mocker: MockerFixture) -> None: """AI: Verify refactor_teardown rewrites the sample tear_down method body into its expected form.""" subject = self._create(mocker, input_code) subject.refactor_teardown() @@ -216,7 +217,7 @@ def test_teardown_refactor(self, input_code, expected_code, mocker) -> None: assert_that(result, is_(expected_code)) @pytest.mark.parametrize("input_code, expected_code", [(tst_testdoubles.test_doubles_fun, tst_testdoubles.test_doubles_fun_new)]) - def test_testdoubles_fun(self, input_code, expected_code, mocker) -> None: + def test_testdoubles_fun(self, input_code, expected_code, mocker: MockerFixture) -> None: """AI: Verify refactor_testdoubles_fun rewrites the sample test-doubles function-level usage.""" subject = self._create(mocker, input_code) subject.refactor_testdoubles_fun() @@ -224,7 +225,7 @@ def test_testdoubles_fun(self, input_code, expected_code, mocker) -> None: assert_that(result, is_(expected_code)) @pytest.mark.parametrize("input_code, expected_code", [(tst_testdoubles.test_doubles_class, tst_testdoubles.test_doubles_class_new)]) - def test_testdoubles_class(self, input_code, expected_code, mocker) -> None: + def test_testdoubles_class(self, input_code, expected_code, mocker: MockerFixture) -> None: """AI: Verify refactor_testdoubles_class rewrites the sample test-doubles class-level usage.""" subject = self._create(mocker, input_code) subject.refactor_testdoubles_class() @@ -238,14 +239,14 @@ def test_testdoubles_class(self, input_code, expected_code, mocker) -> None: ("a = mock.patch(arg)", "a = mock.patch(arg)"), ], ) - def test_remove_mock(self, input_code, expected_code, mocker) -> None: + def test_remove_mock(self, input_code, expected_code, mocker: MockerFixture) -> None: """AI: Verify replace_mock strips the mock. prefix from mock.patch calls/decorators.""" subject = self._create(mocker, input_code) subject.replace_mock() result = subject.apply_to_string() assert_that(result, is_(expected_code)) - def test_remove_stubserver(self, mocker) -> None: + def test_remove_stubserver(self, mocker: MockerFixture) -> None: """AI: Verify remove_stubserver strips the @TAUT.StubServer decorator from a function.""" subject = self._create(mocker, "@TAUT.StubServer\ndef test():\n pass\n") expected_code = "\ndef test():\n pass\n" @@ -260,7 +261,7 @@ def test_remove_stubserver(self, mocker) -> None: ("self.tds.append(TestDoubles(a=ImprovedStub(b)))", "self.a = ImprovedStub(b)"), ], ) - def test_convert_tds(self, input_code, expected_code, mocker) -> None: + def test_convert_tds(self, input_code, expected_code, mocker: MockerFixture) -> None: """AI: Verify convert_tds rewrites TestDoubles/ImprovedStub append calls into add_patcher/attribute assignment.""" subject = self._create(mocker, input_code) subject.convert_tds() @@ -274,7 +275,7 @@ def test_convert_tds(self, input_code, expected_code, mocker) -> None: ("def a():\n assert_double_equal(l.x, 0.0)", "def a():\n self.assert_double_equal(l.x, 0.0)"), ], ) - def test_assert_doubles(self, input_code, expected_code, mocker) -> None: + def test_assert_doubles(self, input_code, expected_code, mocker: MockerFixture) -> None: """AI: Verify manually replacing assert_double_equal name references prefixes them with self.""" subject = self._create(mocker, input_code) [ @@ -285,7 +286,7 @@ def test_assert_doubles(self, input_code, expected_code, mocker) -> None: result = subject.apply_to_string() assert_that(result, is_(expected_code)) - def test_import_verify(self, mocker) -> None: + def test_import_verify(self, mocker: MockerFixture) -> None: """AI: Verify convert_import_verify rewrites import_and_verify_module calls into an import plus assertIsNotNone.""" subject = self._create(mocker, "def test_import(self):\n self.import_and_verify_module('ABCDxTL')") expected_code = "def test_import(self):\n import ABCDxTL\n self.assertIsNotNone(ABCDxTL)" @@ -293,7 +294,7 @@ def test_import_verify(self, mocker) -> None: result = subject.apply_to_string() assert_that(result, is_(expected_code)) - def test_insert_asserter(self, mocker) -> None: + def test_insert_asserter(self, mocker: MockerFixture) -> None: """AI: Verify insert_asserter plus remove_assert_func inserts the asserter helper and removes the original function.""" subject = self._create(mocker, "def assert_double_equal(a, br=c):\n pass") expected_code = tst_insert.insert_code @@ -302,7 +303,7 @@ def test_insert_asserter(self, mocker) -> None: result = subject.apply_to_string() assert_that(result, is_(expected_code)) - def test_replace_unittest_asserter(self, mocker) -> None: + def test_replace_unittest_asserter(self, mocker: MockerFixture) -> None: """AI: Verify replace_unittest_with_asserter rewrites TAUT.TestCase base class to Asserter.""" subject = self._create(mocker, "class A(TAUT.TestCase):\n def b(self):\n self.assert_raises(a, b=c)") expected_code = "class A(Asserter):\n def b(self):\n self.assert_raises(a, b=c)" @@ -317,49 +318,49 @@ def test_replace_unittest_asserter(self, mocker) -> None: ("assert_double_equal", "self.assert_double_equal"), ], ) - def test_assert_func(self, mocker, input_code, expected_code) -> None: + def test_assert_func(self, mocker: MockerFixture, input_code, expected_code) -> None: """AI: Verify assert_func prefixes bare assert_raises/assert_double_equal calls with self.""" subject = self._create(mocker, input_code) subject.assert_func() result = subject.apply_to_string() assert_that(result, is_(expected_code)) - def test_convert_testdoubles_func(self, mocker) -> None: + def test_convert_testdoubles_func(self, mocker: MockerFixture) -> None: """AI: Verify convert_testdoubles_fun rewrites the multi-line sample test-doubles class usage.""" subject = self._create(mocker, tst_testdoubles.test_taut_doubles_class) subject.convert_testdoubles_fun() result = subject.apply_to_string() assert_that(result, is_(tst_testdoubles.test_taut_doubles_class_new)) - def test_convert_testdoubles_func_single_line(self, mocker) -> None: + def test_convert_testdoubles_func_single_line(self, mocker: MockerFixture) -> None: """AI: Verify convert_testdoubles_fun rewrites the single-line sample test-doubles class usage.""" subject = self._create(mocker, tst_testdoubles.test_taut_doubles_class_single_line) subject.convert_testdoubles_fun() result = subject.apply_to_string() assert_that(result, is_(tst_testdoubles.test_taut_doubles_class_single_line_new)) - def test_setup_common(self, mocker) -> None: + def test_setup_common(self, mocker: MockerFixture) -> None: """AI: Verify convert_setup_common rewrites the shared set_up_common method into its expected form.""" subject = self._create(mocker, tst_class.set_up_common) subject.convert_setup_common() result = subject.apply_to_string() assert_that(result, is_(tst_class.set_up_common_new)) - def test_teardown_common(self, mocker) -> None: + def test_teardown_common(self, mocker: MockerFixture) -> None: """AI: Verify convert_teardown_common rewrites the shared tear_down_common method into its expected form.""" subject = self._create(mocker, tst_class.tear_down_common) subject.convert_teardown_common() result = subject.apply_to_string() assert_that(result, is_(tst_class.tear_down_common_new)) - def test_add_patcher(self, mocker) -> None: + def test_add_patcher(self, mocker: MockerFixture) -> None: """AI: Verify convert_add_patcher appends the add_patcher helper method to the class.""" subject = self._create(mocker, tst_class.tear_down_common_new) subject.convert_add_patcher() result = subject.apply_to_string() assert_that(result, is_(tst_class.tear_down_common_new + tst_class.insert_add_patcher + "\n")) - def test_shared_setup(self, mocker) -> None: + def test_shared_setup(self, mocker: MockerFixture) -> None: """AI: Verify shared_setup renames sharedSetUp to setUp.""" subject = self._create(mocker, "class A():\n def sharedSetUp(self):\n pass") expected_code = "class A():\n def setUp(self):\n pass" @@ -367,7 +368,7 @@ def test_shared_setup(self, mocker) -> None: result = subject.apply_to_string() assert_that(result, is_(expected_code)) - def test_with_testdoubles(self, mocker) -> None: + def test_with_testdoubles(self, mocker: MockerFixture) -> None: """AI: Verify with_testdoubles rewrites a TAUT.TestDoubles with-statement into patch.object usage.""" subject = self._create(mocker, "with TAUT.TestDoubles(module=mod, b=c):\n pass") expected_code = "with patch.object(mod, 'b', new=c):\n pass" @@ -375,7 +376,7 @@ def test_with_testdoubles(self, mocker) -> None: result = subject.apply_to_string() assert_that(result, is_(expected_code)) - def test_insert_patch_import(self, mocker) -> None: + def test_insert_patch_import(self, mocker: MockerFixture) -> None: """AI: Verify insert_patch_import inserts the unittest.mock/mock fallback try-import after the unittest import.""" subject = self._create(mocker, "import unittest\nself.patches = []") expected_code = ( diff --git a/test/recipes/test_type_var_check.py b/test/recipes/test_type_var_check.py new file mode 100644 index 00000000..cb33b3e0 --- /dev/null +++ b/test/recipes/test_type_var_check.py @@ -0,0 +1,67 @@ +# Copyright (C) 2024-2026 TNO +# +# Licensed under the terms specified in the LICENSE file in the root directory +# of this repository, also available at: +# https://github.com/TNO/Renaissance.Py/blob/main/LICENSE + +"""End-to-end tests for TypeVarCheck.check() across its phases.""" + +from collections.abc import Callable + +import pytest +from hamcrest import assert_that, contains_string, equal_to, not_ + +from renaissance.recipes.type_var_check import TypeVarCheck + + +class TestTypeVarCheck: + """See module docstring.""" + + @pytest.mark.parametrize( + ("signature", "expected_converted"), + [ + pytest.param("def b(x: T) -> T:", {"T": "fixed"}, id="legacy-signature"), + pytest.param("def b[T](x: T) -> T:", {}, id="ruff-style-leftover"), + ], + ) + def test_check_converts_then_removes_the_declaration_end_to_end( + self, + create_type_var_check: Callable[[str], TypeVarCheck], + signature: str, + expected_converted: dict[str, str], + ) -> None: + """Verify check() adds the type parameter where missing, then removes the now-orphaned declaration.""" + subject = create_type_var_check(f""" + from typing import TypeVar + T = TypeVar('T') + + {signature} + return x + """) + subject.run() + + assert_that(subject.result, equal_to({"cross_file": {}, "converted": expected_converted, "orphaned": {"T": "fixed"}})) + output = subject.apply_to_string() + assert_that(output, contains_string("def b[T](x: T) -> T:")) + assert_that(output, not_(contains_string("T = TypeVar"))) + + def test_check_keeps_declaration_still_used_by_a_generic_class(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """Verify check() converts a standalone function but keeps the declaration a Generic[...] base still needs.""" + subject = create_type_var_check(""" + from typing import Generic, TypeVar + T = TypeVar('T') + + class Box(Generic[T]): + def get(self, x: T) -> T: + return x + + def first(items: list[T]) -> T: + return items[0] + """) + subject.run() + + assert_that(subject.result, equal_to({"cross_file": {}, "converted": {"T": "fixed"}, "orphaned": {}})) + output = subject.apply_to_string() + assert_that(output, contains_string("def first[T](items: list[T]) -> T:")) + assert_that(output, contains_string("T = TypeVar('T')")) + assert_that(output, contains_string(" def get(self, x: T) -> T:")) diff --git a/test/recipes/test_type_var_check_convert.py b/test/recipes/test_type_var_check_convert.py new file mode 100644 index 00000000..e0e6ce27 --- /dev/null +++ b/test/recipes/test_type_var_check_convert.py @@ -0,0 +1,594 @@ +# Copyright (C) 2024-2026 TNO +# +# Licensed under the terms specified in the LICENSE file in the root directory +# of this repository, also available at: +# https://github.com/TNO/Renaissance.Py/blob/main/LICENSE + +"""Tests for TypeVarCheck.convert_declared_typevars.""" + +import ast +import textwrap +from collections.abc import Callable +from typing import cast + +import pytest +from hamcrest import assert_that, contains_string, equal_to, has_entry, not_ + +from renaissance.recipes.type_var_check import PEP_695_MINIMUM, PEP_696_MINIMUM, TypeVarCheck +from renaissance.recipes.type_var_domain import UnsafeReason + + +def _type_param_name(param: ast.type_param) -> str: + """Return a parsed PEP 695 type parameter's name.""" + return cast("ast.TypeVar | ast.ParamSpec | ast.TypeVarTuple", param).name + + +class TestTypeVarCheckConvert: + """See module docstring.""" + + @pytest.mark.parametrize( + ("functions", "expected_signatures"), + [ + pytest.param( + "def a(x: T) -> T:\n return x\ndef b(y: T) -> T:\n return y\n", + ["def a[T](x: T) -> T:", "def b[T](y: T) -> T:"], + id="functions", + ), + pytest.param( + "class Foo:\n def a(self, x: T) -> T:\n return x\n def b(self, y: T) -> T:\n return y\n", + ["def a[T](self, x: T) -> T:", "def b[T](self, y: T) -> T:"], + id="methods", + ), + ], + ) + def test_converts_typevar_shared_by_two_functions( + self, create_type_var_check: Callable[[str], TypeVarCheck], functions: str, expected_signatures: list[str] + ) -> None: + """Verify a TypeVar shared by two functions, or two methods of one class, gives each its own [T].""" + subject = create_type_var_check(f'from typing import TypeVar\n\n{functions}\nT = TypeVar("T")\n') + result = subject.convert_declared_typevars() + + assert_that(result, equal_to({"T": "fixed"})) + output = subject.apply_to_string() + for signature in expected_signatures: + assert_that(output, contains_string(signature)) + + @pytest.mark.parametrize( + "docstring", + [ + pytest.param('"""One liner."""', id="single-line"), + pytest.param('"""First line.\n\nSecond line already indented.\nThird line too.\n"""', id="multi-line"), + pytest.param('"""Produce a cast.\n\n.. seealso::\n\n :ref:`tutorial_casts`\n"""', id="nested-block"), + ], + ) + def test_converts_method_keeping_its_docstring(self, create_type_var_check: Callable[[str], TypeVarCheck], docstring: str) -> None: + """Verify converting a method's signature keeps its docstring exactly, including any nested indentation.""" + body = textwrap.indent(f"{docstring}\nreturn x\n", " " * 8) + subject = create_type_var_check( + "from typing import TypeVar\n\nclass Foo:\n" + f" def cast(self, x: T) -> T:\n{body}" + " def other(self, y: T) -> T:\n return y\n\n" + 'T = TypeVar("T")\n' + ) + result = subject.convert_declared_typevars() + + assert_that(result, has_entry("T", "fixed")) + assert_that(subject.apply_to_string(), contains_string(f" def cast[T](self, x: T) -> T:\n{body}")) + + @pytest.mark.parametrize( + ("extra_line", "imported_elsewhere"), + [ + pytest.param('__all__ = ["T"]', frozenset[str](), id="exported-via-dunder-all"), + pytest.param("", frozenset({"T"}), id="imported-elsewhere"), + ], + ) + def test_converts_functions_using_a_typevar_whose_declaration_must_stay( + self, + create_type_var_check: Callable[[str], TypeVarCheck], + extra_line: str, + imported_elsewhere: frozenset[str], + ) -> None: + """Verify functions get [T] even when T's declaration can't be removed, and the declaration is kept.""" + subject = create_type_var_check(f""" + from typing import TypeVar + + {extra_line} + + def a(x: T) -> T: + return x + + T = TypeVar("T") + """) + subject.project_wide_imported_names = imported_elsewhere + + result = subject.convert_declared_typevars() + + assert_that(result, equal_to({"T": "fixed"})) + output = subject.apply_to_string() + assert_that(output, contains_string("def a[T](x: T) -> T:")) + assert_that(output, contains_string('T = TypeVar("T")')) + + def test_converts_function_preserving_unusual_body_formatting(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """AI: Verify converting a signature never reformats or collapses its body's unusual formatting.""" + subject = create_type_var_check(""" + from typing import TypeVar + + def b(x: T) -> T: + return foo( + x, + extra=1, + ) + + T = TypeVar("T") + """) + result = subject.convert_declared_typevars() + + assert_that(result, has_entry("T", "fixed")) + output = subject.apply_to_string() + assert_that(output, contains_string("def b[T](x: T) -> T:")) + assert_that(output, contains_string("return foo(\n x,\n extra=1,\n )")) + + def test_does_not_add_redundant_type_param_to_nested_closure(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """AI: Verify a nested closure referencing an enclosing function's converted type param doesn't get its own copy.""" + # PEP 695 type parameters are already visible in nested scopes. + subject = create_type_var_check(""" + from typing import ParamSpec + from collections.abc import Callable + + P = ParamSpec("P") + + def requires(func: Callable[P, int]) -> Callable[P, int]: + def wrapper(*args: P.args, **kwargs: P.kwargs) -> int: + return func(*args, **kwargs) + + return wrapper + """) + result = subject.convert_declared_typevars() + + assert_that(result, has_entry("P", "fixed")) + output = subject.apply_to_string() + ast.parse(output) # raises SyntaxError if the nested closure's edit corrupted the output + assert_that(output, contains_string("def requires[**P](func: Callable[P, int]) -> Callable[P, int]:")) + assert_that(output, contains_string("def wrapper(*args: P.args, **kwargs: P.kwargs) -> int:")) + assert_that(output, not_(contains_string("wrapper[**P]"))) + + def test_merges_into_an_existing_type_params_bracket(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """AI: Verify converting a second TypeVar merges it into an existing PEP 695 bracket instead of adding a new one.""" + subject = create_type_var_check(""" + from typing import TypeVar + + def f[U](x: U, y: T) -> T: + return y + + T = TypeVar("T") + """) + result = subject.convert_declared_typevars() + + assert_that(result, has_entry("T", "fixed")) + output = subject.apply_to_string() + assert_that(output, contains_string("def f[U, T](x: U, y: T) -> T:")) + + def test_converts_a_decorated_overload(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """AI: Verify converting a decorated @overload signature accounts for its non-zero-column indentation.""" + subject = create_type_var_check(""" + from typing import TypeVar, overload + + class Config: + @overload + def get(self, key: str, default: T = ...) -> T: ... + def get(self, key: str, default: object = None) -> object: + return default + + T = TypeVar("T") + """) + result = subject.convert_declared_typevars() + + assert_that(result, has_entry("T", "fixed")) + output = subject.apply_to_string() + assert_that(output, contains_string("@overload")) + assert_that(output, contains_string("def get[T](self, key: str, default: T = ...) -> T: ...")) + + def test_converts_function_using_two_new_type_params(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """Verify a function using two declared type parameters gets both in one bracket.""" + subject = create_type_var_check(""" + from typing import ParamSpec, TypeVar + from collections.abc import Callable + + P = ParamSpec("P") + T = TypeVar("T") + + def run_in_threadpool(func: Callable[P, T]) -> T: + return func() + + def identity(x: T) -> T: + return x + """) + result = subject.check() + + assert_that(result["converted"], has_entry("P", "fixed")) + assert_that(result["converted"], has_entry("T", "fixed")) + assert_that(result["orphaned"], equal_to({"P": "fixed", "T": "fixed"})) + functions = {node.name: node for node in ast.parse(subject.apply_to_string()).body if isinstance(node, ast.FunctionDef)} + assert_that([_type_param_name(param) for param in functions["run_in_threadpool"].type_params], equal_to(["P", "T"])) + assert_that([_type_param_name(param) for param in functions["identity"].type_params], equal_to(["T"])) + + def test_orders_type_params_by_declaration(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """Verify the added type parameters follow the order their declarations appear in the file.""" + names = ["A", "B", "C", "D", "E", "F"] + declarations = "\n".join(f'{name} = TypeVar("{name}")' for name in names) + parameters = ", ".join(f"{name.lower()}: {name}" for name in names) + subject = create_type_var_check(f"from typing import TypeVar\n{declarations}\ndef f({parameters}) -> None: ...\n") + + subject.convert_declared_typevars() + + assert_that(subject.apply_to_string(), contains_string(f"def f[{', '.join(names)}]({parameters}) -> None: ...")) + + def test_converts_function_preserving_multiline_string_literal(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """Verify converting a function keeps a multi-line string literal whose continuation line is at column 0.""" + # Built line by line: textwrap.dedent in the fixture would otherwise be blocked by the column-0 line. + source_lines = [ + "from typing import TypeVar", + 'T = TypeVar("T")', + "def f(x: T) -> T:", + ' text = """first', + "second", + ' third"""', + " return x", + ] + subject = create_type_var_check("\n".join(source_lines) + "\n") + result = subject.convert_declared_typevars() + + assert_that(result, has_entry("T", "fixed")) + output = subject.apply_to_string() + assert_that(output, contains_string("def f[T](x: T) -> T:")) + text_assign = next( + node for node in ast.walk(ast.parse(output)) if isinstance(node, ast.Assign) and ast.unparse(node.targets[0]) == "text" + ) + assert_that(ast.literal_eval(text_assign.value), equal_to("first\nsecond\n third")) + + @pytest.mark.parametrize( + "class_header", + [ + pytest.param("class Box(Generic[T]):", id="generic-base"), + pytest.param("class Box(Protocol[T]):", id="protocol-base"), + pytest.param("class Sub(Base[T]):", id="generic-subclass"), + pytest.param("class Box[T]:", id="pep695-class"), + ], + ) + def test_converts_function_but_not_methods_of_a_generic_class( + self, create_type_var_check: Callable[[str], TypeVarCheck], class_header: str + ) -> None: + """Verify a standalone function gets [T] while the methods of a class generic over T keep using the class's T.""" + subject = create_type_var_check(f""" + from typing import Generic, Protocol, TypeVar + T = TypeVar('T') + + {class_header} + def get(self, x: T) -> T: + return x + + def first(items: list[T]) -> T: + return items[0] + """) + result = subject.convert_declared_typevars() + + assert_that(result, equal_to({"T": "fixed"})) + output = subject.apply_to_string() + assert_that(output, contains_string("def first[T](items: list[T]) -> T:")) + assert_that(output, contains_string(" def get(self, x: T) -> T:")) + + def test_reports_typevar_used_only_in_a_generic_class(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """Verify a TypeVar only a generic class uses is reported unsafe with its reason and left as it is.""" + subject = create_type_var_check(""" + from typing import Generic, TypeVar + T = TypeVar('T') + + class Box(Generic[T]): + def get(self, x: T) -> T: + return x + """) + result = subject.convert_declared_typevars() + + assert_that(result, equal_to({"T": "unsafe"})) + assert_that(subject.converted_unsafe_reasons, equal_to({"T": UnsafeReason.USED_IN_GENERIC_CLASS})) + assert_that(subject.apply_to_string(), contains_string(" def get(self, x: T) -> T:")) + + def test_ignores_methods_using_their_class_type_parameter(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """Verify a T that is the class's own type parameter (`class Box[T]`) is not a use of the module-level T.""" + subject = create_type_var_check(""" + from typing import TypeVar + T = TypeVar('T') + + class Box[T]: + def get(self, x: T) -> T: + return x + """) + result = subject.convert_declared_typevars() + + assert_that(result, equal_to({})) + assert_that(subject.converted_unsafe_reasons, equal_to({})) + assert_that(subject.apply_to_string(), contains_string(" def get(self, x: T) -> T:")) + + @pytest.mark.parametrize( + "class_header", + [ + pytest.param("class Util:", id="plain-class"), + pytest.param("class Box(Generic[U]):", id="generic-over-another-name"), + pytest.param("class Box[U]:", id="pep695-class-over-another-name"), + ], + ) + def test_converts_method_of_a_class_not_generic_over_the_name( + self, create_type_var_check: Callable[[str], TypeVarCheck], class_header: str + ) -> None: + """Verify a method gets [T] when its class isn't generic over T.""" + subject = create_type_var_check(f""" + from typing import Generic, TypeVar + T = TypeVar('T') + U = TypeVar('U') + + {class_header} + def get(self, x: T) -> T: + return x + """) + result = subject.convert_declared_typevars() + + assert_that(result, has_entry("T", "fixed")) + assert_that(subject.apply_to_string(), contains_string(" def get[T](self, x: T) -> T:")) + + def test_converts_function_when_typevar_is_also_used_outside_functions( + self, create_type_var_check: Callable[[str], TypeVarCheck] + ) -> None: + """Verify a function using T is converted even though a module-level alias also uses T.""" + subject = create_type_var_check(""" + from typing import TypeVar + T = TypeVar('T') + + Pair = tuple[T, T] + + def first(pair: Pair) -> T: + return pair[0] + + def same(x: T) -> T: + return x + """) + result = subject.convert_declared_typevars() + + assert_that(result, equal_to({"T": "fixed"})) + assert_that(subject.apply_to_string(), contains_string("def same[T](x: T) -> T:")) + + +_HEADER = "from collections.abc import Callable\nfrom typing import ParamSpec, TypeVar, TypeVarTuple, Unpack\n\n" +_TYPEVAR_USE = "def a(x: T) -> T:" +_PARAMSPEC_USE = "def a(f: Callable[P, int]) -> Callable[P, int]:" +_TYPEVARTUPLE_USE = "def a(*args: *Ts) -> tuple[*Ts]:" + + +def _source(declarations: str, signature: str) -> str: + """Build a module with the given legacy declarations and a single function using them.""" + return f"{_HEADER}{declarations}\n\n\n{signature}\n ...\n" + + +class TestTypeVarCheckConvertArguments: + """Every constructor argument is carried over or refused with a reason, never lost silently.""" + + @pytest.mark.parametrize( + ("declaration", "signature", "expected_signature"), + [ + pytest.param('T = TypeVar("T")', _TYPEVAR_USE, "def a[T](x: T) -> T:", id="typevar-plain"), + pytest.param('T = TypeVar("T", int, str)', _TYPEVAR_USE, "def a[T: (int, str)](x: T) -> T:", id="typevar-constraints"), + pytest.param('T = TypeVar("T", bound=int)', _TYPEVAR_USE, "def a[T: int](x: T) -> T:", id="typevar-bound"), + pytest.param('T = TypeVar("T", default=int)', _TYPEVAR_USE, "def a[T = int](x: T) -> T:", id="typevar-default"), + pytest.param( + 'T = TypeVar("T", bound=str, default=str)', + _TYPEVAR_USE, + "def a[T: str = str](x: T) -> T:", + id="typevar-bound-default", + ), + pytest.param( + 'T = TypeVar("T", int, str, default=int)', + _TYPEVAR_USE, + "def a[T: (int, str) = int](x: T) -> T:", + id="typevar-constraints-default", + ), + pytest.param('T = TypeVar("T", infer_variance=True)', _TYPEVAR_USE, "def a[T](x: T) -> T:", id="typevar-infer-variance"), + pytest.param( + 'P = ParamSpec("P")', + _PARAMSPEC_USE, + "def a[**P](f: Callable[P, int]) -> Callable[P, int]:", + id="paramspec-plain", + ), + pytest.param( + 'P = ParamSpec("P", default=[int, str])', + _PARAMSPEC_USE, + "def a[**P = [int, str]](f: Callable[P, int]) -> Callable[P, int]:", + id="paramspec-default", + ), + pytest.param('Ts = TypeVarTuple("Ts")', _TYPEVARTUPLE_USE, "def a[*Ts](*args: *Ts) -> tuple[*Ts]:", id="typevartuple-plain"), + pytest.param( + 'Ts = TypeVarTuple("Ts", default=Unpack[tuple[int]])', + _TYPEVARTUPLE_USE, + "def a[*Ts = Unpack[tuple[int]]](*args: *Ts) -> tuple[*Ts]:", + id="typevartuple-default", + ), + ], + ) + def test_converts_keeping_every_expressible_argument( + self, + create_type_var_check: Callable[[str], TypeVarCheck], + declaration: str, + signature: str, + expected_signature: str, + ) -> None: + """Verify bounds, constraints, defaults and inferred variance are carried over.""" + name = declaration.split(" = ", maxsplit=1)[0] + subject = create_type_var_check(_source(declaration, signature)) + subject.min_python = PEP_696_MINIMUM + + result = subject.convert_declared_typevars() + + assert_that(result, has_entry(name, "fixed")) + output = subject.apply_to_string() + ast.parse(output) + assert_that(output, contains_string(expected_signature)) + + @pytest.mark.parametrize( + ("declaration", "signature", "min_python", "expected_reason"), + [ + pytest.param( + 'T = TypeVar("T")', + _TYPEVAR_USE, + (3, 11), + UnsafeReason.PEP695_VERSION_GATE, + id="typevar-plain-3.11", + ), + pytest.param( + 'T = TypeVar("T", default=int)', + _TYPEVAR_USE, + PEP_695_MINIMUM, + UnsafeReason.PEP696_VERSION_GATE, + id="typevar-default-3.12", + ), + pytest.param( + 'P = ParamSpec("P", default=[int, str])', + _PARAMSPEC_USE, + PEP_695_MINIMUM, + UnsafeReason.PEP696_VERSION_GATE, + id="paramspec-default-3.12", + ), + pytest.param( + 'Ts = TypeVarTuple("Ts", default=Unpack[tuple[int]])', + _TYPEVARTUPLE_USE, + PEP_695_MINIMUM, + UnsafeReason.PEP696_VERSION_GATE, + id="typevartuple-default-3.12", + ), + pytest.param( + 'T = TypeVar("T", covariant=True)', + _TYPEVAR_USE, + PEP_696_MINIMUM, + UnsafeReason.NO_PEP695_EQUIVALENT, + id="typevar-covariant", + ), + pytest.param( + 'T = TypeVar("T", contravariant=True)', + _TYPEVAR_USE, + PEP_696_MINIMUM, + UnsafeReason.NO_PEP695_EQUIVALENT, + id="typevar-contravariant", + ), + pytest.param( + 'P = ParamSpec("P", covariant=True)', + _PARAMSPEC_USE, + PEP_696_MINIMUM, + UnsafeReason.NO_PEP695_EQUIVALENT, + id="paramspec-covariant", + ), + pytest.param( + 'Ts = TypeVarTuple("Ts", bound=int, covariant=True)', + _TYPEVARTUPLE_USE, + PEP_696_MINIMUM, + UnsafeReason.NO_PEP695_EQUIVALENT, + id="typevartuple-bound-covariant", + ), + pytest.param( + 'P = ParamSpec("P", bound=int)', + _PARAMSPEC_USE, + PEP_696_MINIMUM, + UnsafeReason.NO_PEP695_EQUIVALENT, + id="paramspec-bound", + ), + pytest.param( + 'T = TypeVar("T", future_kw=1)', + _TYPEVAR_USE, + PEP_696_MINIMUM, + UnsafeReason.NO_PEP695_EQUIVALENT, + id="unknown-keyword", + ), + pytest.param( + 'T = TypeVar("T", **options)', + _TYPEVAR_USE, + PEP_696_MINIMUM, + UnsafeReason.NO_PEP695_EQUIVALENT, + id="double-star-keywords", + ), + ], + ) + def test_refuses_argument_it_cannot_carry_over( + self, + create_type_var_check: Callable[[str], TypeVarCheck], + declaration: str, + signature: str, + min_python: tuple[int, int], + expected_reason: UnsafeReason, + ) -> None: + """Verify an argument the target can't express is reported unsafe with its reason and the file is left unchanged.""" + name = declaration.split(" = ", maxsplit=1)[0] + source = _source(declaration, signature) + subject = create_type_var_check(source) + subject.min_python = min_python + + result = subject.convert_declared_typevars() + + assert_that(result, has_entry(name, "unsafe")) + assert_that(subject.converted_unsafe_reasons, has_entry(name, expected_reason)) + assert_that(subject.apply_to_string(), equal_to(source)) + + @pytest.mark.parametrize( + ("declarations", "signature", "expected_signature"), + [ + pytest.param( + 'T = TypeVar("T", default=int)\nU = TypeVar("U")', + "def f(x: T, y: U) -> T:", + "def f[U, T = int](x: T, y: U) -> T:", + id="defaulted-declared-first", + ), + pytest.param( + 'U = TypeVar("U")', + "def f[T = int](x: T, y: U) -> T:", + "def f[U, T = int](x: T, y: U) -> T:", + id="existing-defaulted-param", + ), + pytest.param( + 'T = TypeVar("T", default=int)\nTs = TypeVarTuple("Ts")', + "def f(x: T, *args: *Ts) -> T:", + "def f[*Ts, T = int](x: T, *args: *Ts) -> T:", + id="typevartuple-after-defaulted", + ), + ], + ) + def test_places_defaulted_type_params_last( + self, + create_type_var_check: Callable[[str], TypeVarCheck], + declarations: str, + signature: str, + expected_signature: str, + ) -> None: + """Verify type parameters with a default come after those without one, which Python requires.""" + subject = create_type_var_check(_source(declarations, signature)) + subject.min_python = PEP_696_MINIMUM + + subject.convert_declared_typevars() + + output = subject.apply_to_string() + ast.parse(output) + assert_that(output, contains_string(expected_signature)) + + @pytest.mark.xfail( + reason="A default referencing another legacy declaration is carried over as is, so the new type " + "parameter's default refers to the module-level TypeVar.", + raises=AssertionError, + strict=True, + ) + def test_does_not_convert_default_referencing_another_declaration( + self, + create_type_var_check: Callable[[str], TypeVarCheck], + ) -> None: + """Verify a declaration whose default uses another legacy type parameter is reported unsafe and left unconverted.""" + subject = create_type_var_check(_source('T = TypeVar("T")\nU = TypeVar("U", default=T)', "def f(x: U) -> U:")) + subject.min_python = PEP_696_MINIMUM + + result = subject.convert_declared_typevars() + + assert_that(result, has_entry("U", "unsafe")) + assert_that(subject.converted_unsafe_reasons, has_entry("U", not_(UnsafeReason.PEP696_VERSION_GATE))) + assert_that(subject.apply_to_string(), contains_string("def f(x: U) -> U:")) diff --git a/test/recipes/test_type_var_check_localize.py b/test/recipes/test_type_var_check_localize.py new file mode 100644 index 00000000..7db16657 --- /dev/null +++ b/test/recipes/test_type_var_check_localize.py @@ -0,0 +1,549 @@ +# Copyright (C) 2024-2026 TNO +# +# Licensed under the terms specified in the LICENSE file in the root directory +# of this repository, also available at: +# https://github.com/TNO/Renaissance.Py/blob/main/LICENSE + +"""Tests for TypeVarCheck.localize_imported_typevars.""" + +import ast +import builtins +import textwrap +from pathlib import Path +from typing import TYPE_CHECKING, cast + +import pytest +from hamcrest import all_of, assert_that, contains_string, equal_to, has_entry, has_length, not_ +from pytest_mock import MockerFixture + +from renaissance.integrations.python.ast.cst_node import PythonCstNode +from renaissance.recipes.type_var_check import PEP_695_MINIMUM, TypeVarCheck +from renaissance.recipes.type_var_domain import UnsafeReason + +# TODO(FrancescoPezzella): Replace with a lazy import in Python 3.15. +# https://github.com/TNO/Renaissance.Py/issues/275 +if TYPE_CHECKING: + from hamcrest.core.matcher import Matcher + + +def _module_level_names(module: ast.Module) -> set[str]: + """Return the names bound at module level by imports, classes, functions and assignments.""" + names: set[str] = set() + for node in module.body: + if isinstance(node, ast.Import | ast.ImportFrom): + names.update((alias.asname or alias.name).split(".")[0] for alias in node.names) + elif isinstance(node, ast.ClassDef | ast.FunctionDef | ast.AsyncFunctionDef): + names.add(node.name) + elif isinstance(node, ast.Assign): + names.update(target.id for target in node.targets if isinstance(target, ast.Name)) + return names + + +def _names_used_by(call: ast.Call) -> set[str]: + """Return the non-builtin names a TypeVar call's arguments use, including inside string forward references.""" + names: set[str] = set() + for argument in [*call.args[1:], *(keyword.value for keyword in call.keywords)]: + expression = argument + if isinstance(argument, ast.Constant) and isinstance(argument.value, str): + expression = ast.parse(argument.value, mode="eval").body + names.update(node.id for node in ast.walk(expression) if isinstance(node, ast.Name)) + return names - set(dir(builtins)) + + +class TestTypeVarCheckLocalize: + """See module docstring.""" + + def _create_cross_file( + self, + mocker: MockerFixture, + tmp_path: Path, + origin_text: str, + importing_text: str, + layout: tuple[str, str] = ("file_1.py", "file_2.py"), + ) -> TypeVarCheck: + """Write origin_text to the origin file and return an in-memory TypeVarCheck on the importing file holding importing_text. + + layout gives the (origin, importing) file paths, relative to tmp_path, which is the project root. + """ + origin_rel_path, importing_rel_path = layout + origin_file = tmp_path / origin_rel_path + origin_file.parent.mkdir(parents=True, exist_ok=True) + origin_file.write_text(textwrap.dedent(origin_text)) + + importing_code = textwrap.dedent(importing_text) + importing_file = str(tmp_path / importing_rel_path) + mocker.patch( + "renaissance.integrations.python.ast.factory.PythonFactory.create", + return_value=PythonCstNode.load_from_text(importing_code, importing_file), + ) + subject = TypeVarCheck(importing_file) + subject.in_memory = True + subject.min_python = PEP_695_MINIMUM + subject.project_root = tmp_path + return subject + + @pytest.mark.parametrize( + "origin_extra", + [ + pytest.param("def a(x: T) -> T:\n return x", id="plain-origin"), + pytest.param('__all__ = ["T"]', id="origin-exports-it"), + pytest.param("class Box(Generic[T]):\n pass", id="origin-class-is-generic-over-it"), + ], + ) + def test_localizes_typevar_whatever_its_origin_does_with_it(self, mocker: MockerFixture, tmp_path: Path, origin_extra: str) -> None: + """Verify an imported TypeVar is localized, also when its origin exports it or has a class generic over it.""" + origin = f"from typing import Generic, TypeVar\nT = TypeVar('T')\n{origin_extra}\n" + subject = self._create_cross_file( + mocker, + tmp_path, + origin, + """ + from file_1 import T + def b(x: T) -> T: + return x + """, + ) + result = subject.localize_imported_typevars() + + assert_that(result, equal_to({"T": "fixed"})) + output = subject.apply_to_string() + assert_that(output, contains_string("T = TypeVar('T')")) + assert_that(output, not_(contains_string("from file_1 import T"))) + + @pytest.mark.parametrize( + "declaration", + [ + pytest.param('T = TypeVar("T", bound=Shape)', id="invariant-bound"), + pytest.param('T = TypeVar("T", int, Shape)', id="constraints"), + pytest.param('T = TypeVar("T", bound=Shape, covariant=True)', id="covariant"), + pytest.param('T = TypeVar("T", bound=Shape, contravariant=True)', id="contravariant"), + pytest.param('T = TypeVar("T", bound="Shape")', id="string-bound"), + ], + ) + def test_localized_declaration_keeps_its_meaning(self, mocker: MockerFixture, tmp_path: Path, declaration: str) -> None: + """Verify the local copy has the origin's exact arguments and every name they use is defined in the importing file.""" + origin = f"from typing import TypeVar\nclass Shape:\n pass\n{declaration}\n" + subject = self._create_cross_file( + mocker, + tmp_path, + origin, + """ + from file_1 import T + def biggest(items: list[T]) -> T: + return items[0] + """, + ) + result = subject.localize_imported_typevars() + + assert_that(result, equal_to({"T": "fixed"})) + module = ast.parse(subject.apply_to_string()) + local_call = next(node.value for node in module.body if isinstance(node, ast.Assign)) + origin_call = cast("ast.Assign", ast.parse(origin).body[-1]).value + assert_that(ast.dump(local_call), equal_to(ast.dump(origin_call))) + assert_that(_names_used_by(cast("ast.Call", local_call)) - _module_level_names(module), equal_to(set())) + + @pytest.mark.parametrize( + ("origin", "importing_extra"), + [ + pytest.param( + 'from typing import TypeVar\nclass Shape:\n pass\nT = TypeVar("T", bound=Shape)\n', + "class Shape:\n pass", + id="name-means-something-else-here", + ), + pytest.param( + "from typing import TYPE_CHECKING, TypeVar\n" + "if TYPE_CHECKING:\n" + " from shapes import Shape\n" + 'T = TypeVar("T", bound="Shape")\n', + "", + id="origin-binds-it-only-for-type-checking", + ), + ], + ) + def test_does_not_localize_when_a_name_the_declaration_uses_cant_be_imported_safely( + self, mocker: MockerFixture, tmp_path: Path, origin: str, importing_extra: str + ) -> None: + """Verify a declaration using a name that can't be imported from the origin as the same object stays imported.""" + subject = self._create_cross_file( + mocker, + tmp_path, + origin, + f"from file_1 import T\n{importing_extra}\ndef biggest(items: list[T]) -> T:\n return items[0]\n", + ) + result = subject.localize_imported_typevars() + + assert_that(result, equal_to({"T": "unsafe"})) + assert_that(subject.cross_file_unsafe_reasons, equal_to({"T": UnsafeReason.DECLARATION_NAME_UNAVAILABLE})) + assert_that(subject.apply_to_string(), contains_string("from file_1 import T")) + + @pytest.mark.parametrize( + ("dunder_all", "imported_elsewhere"), + [ + pytest.param('__all__ = ["T"]', frozenset[str](), id="re-exported-via-dunder-all"), + pytest.param("", frozenset({"T"}), id="imported-from-here-elsewhere"), + ], + ) + def test_localizes_typevar_this_file_re_exports( + self, + mocker: MockerFixture, + tmp_path: Path, + dunder_all: str, + imported_elsewhere: frozenset[str], + ) -> None: + """Verify an imported TypeVar is localized even when this file passes it on to others.""" + subject = self._create_cross_file( + mocker, + tmp_path, + """ + from typing import TypeVar + T = TypeVar("T") + """, + f""" + from file_1 import T + {dunder_all} + def b(x: T) -> T: + return x + """, + ) + subject.project_wide_imported_names = imported_elsewhere + + result = subject.localize_imported_typevars() + + assert_that(result, equal_to({"T": "fixed"})) + output = subject.apply_to_string() + assert_that(output, contains_string('T = TypeVar("T")')) + assert_that(output, not_(contains_string("from file_1 import T"))) + + @pytest.mark.parametrize( + ("layout", "importing_import", "expected_line"), + [ + pytest.param(("file_1.py", "file_2.py"), "from file_1 import T, helper", "from file_1 import helper", id="absolute"), + pytest.param(("file_1.py", "file_2.py"), "from .file_1 import T, helper", "from .file_1 import helper", id="relative"), + pytest.param( + ("file_1.py", "sub/use.py"), + "from ..file_1 import T, helper", + "from ..file_1 import helper", + id="relative-parent", + ), + pytest.param(("__init__.py", "file_2.py"), "from . import T, helper", "from . import helper", id="bare-package"), + pytest.param( + ("file_1.py", "file_2.py"), + "from file_1 import T, helper as h", + "from file_1 import helper as h", + id="alias-kept", + ), + ], + ) + def test_keeps_other_names_when_localizing_one_of_several_imports( + self, + mocker: MockerFixture, + tmp_path: Path, + layout: tuple[str, str], + importing_import: str, + expected_line: str, + ) -> None: + """Verify localizing one imported name keeps the other names imported from the same module, alias included.""" + subject = self._create_cross_file( + mocker, + tmp_path, + 'from typing import TypeVar\nT = TypeVar("T")\ndef helper() -> None:\n pass\n', + f"{importing_import}\ndef b(x: T) -> T:\n return x\n", + layout, + ) + result = subject.localize_imported_typevars() + + assert_that(result, has_entry("T", "fixed")) + output = subject.apply_to_string() + assert_that(output.splitlines().count(expected_line), equal_to(1)) + assert_that(output, contains_string('T = TypeVar("T")')) + + @pytest.mark.parametrize( + ("importing_import", "comments"), + [ + pytest.param("from file_1 import T, helper # noqa: F401", ["# noqa: F401"], id="trailing-comment"), + pytest.param( + "from file_1 import ( # header\n T, # the typevar\n helper, # the helper\n)", + ["# header", "# the helper"], + id="multi-line", + ), + ], + ) + def test_narrowing_an_import_keeps_its_comments( + self, + mocker: MockerFixture, + tmp_path: Path, + importing_import: str, + comments: list[str], + ) -> None: + """Verify each comment of a narrowed import is kept once, and never on the localized declaration's line.""" + subject = self._create_cross_file( + mocker, + tmp_path, + 'from typing import TypeVar\nT = TypeVar("T")\ndef helper() -> None:\n pass\n', + f"{importing_import}\ndef b(x: T) -> T:\n helper()\n return x\n", + ) + result = subject.localize_imported_typevars() + + assert_that(result, has_entry("T", "fixed")) + lines = subject.apply_to_string().splitlines() + for comment in comments: + holders = [line for line in lines if comment in line] + assert_that(holders, has_length(1), comment) + assert_that(holders[0], not_(contains_string("TypeVar(")), comment) + + @pytest.mark.parametrize( + ("origin_extra", "importing_import", "expected_lines"), + [ + pytest.param( + 'T = TypeVar("T")\nU = TypeVar("U")', + "from file_1 import T, U", + ["from typing import TypeVar", 'T = TypeVar("T")', 'U = TypeVar("U")'], + id="two-names", + ), + pytest.param( + 'T = TypeVar("T")\nU = TypeVar("U")\ndef helper() -> None:\n pass', + "from file_1 import T, helper, U", + ["from file_1 import helper", "from typing import TypeVar", 'T = TypeVar("T")', 'U = TypeVar("U")'], + id="keeps-other-name", + ), + pytest.param( + 'class Shape:\n pass\nclass Circle(Shape):\n pass\nT = TypeVar("T", bound=Shape)\nU = TypeVar("U", bound=Circle)', + "from file_1 import T, U", + [ + "from file_1 import Circle, Shape", + "from typing import TypeVar", + 'T = TypeVar("T", bound=Shape)', + 'U = TypeVar("U", bound=Circle)', + ], + id="bounds-share-one-import", + ), + ], + ) + def test_localizes_two_names_from_one_import_statement( + self, + mocker: MockerFixture, + tmp_path: Path, + origin_extra: str, + importing_import: str, + expected_lines: list[str], + ) -> None: + """Verify one import statement bringing in two localizable names localizes both, adding each needed import once.""" + subject = self._create_cross_file( + mocker, + tmp_path, + f"from typing import TypeVar\n{origin_extra}\n", + f"{importing_import}\ndef b(x: T, y: U) -> T:\n return x\n", + ) + result = subject.localize_imported_typevars() + + assert_that(result, equal_to({"T": "fixed", "U": "fixed"})) + output = subject.apply_to_string() + for line in expected_lines: + assert_that(output.splitlines().count(line), equal_to(1), line) + still_imported = { + alias.name + for node in ast.parse(output).body + if isinstance(node, ast.ImportFrom) and node.module == "file_1" + for alias in node.names + } + assert_that(still_imported & {"T", "U"}, equal_to(set())) + + @pytest.mark.parametrize( + ("origin_import", "importing_header"), + [ + pytest.param("from typing import TypeVar", "", id="missing-here"), + pytest.param("from typing import TypeVar", 'from typing import TypeVar\nU = TypeVar("U")', id="already-here"), + # The origin has no from-import of "TypeVar"; only safe because this file imports it. + pytest.param("from typing import *", "from typing import TypeVar", id="origin-wildcard-import"), + ], + ) + def test_localized_file_imports_the_constructor_exactly_once( + self, + mocker: MockerFixture, + tmp_path: Path, + origin_import: str, + importing_header: str, + ) -> None: + """Verify localizing adds "from typing import TypeVar" when missing and never duplicates it.""" + subject = self._create_cross_file( + mocker, + tmp_path, + f'{origin_import}\nT = TypeVar("T")\ndef a(x: T) -> T:\n return x\n', + f"{importing_header}\nfrom file_1 import T\ndef b(x: T) -> T:\n return x\n", + ) + result = subject.localize_imported_typevars() + + assert_that(result, has_entry("T", "fixed")) + assert_that(subject.apply_to_string().count("from typing import TypeVar"), equal_to(1)) + + @pytest.mark.parametrize( + ("layout", "importing_header", "expected_line"), + [ + pytest.param(("file_1.py", "file_2.py"), "from .file_1 import T", "from ._compat import TypeVar", id="same-dir"), + pytest.param( + ("sub/file_1.py", "use.py"), + "from .sub.file_1 import T", + "from .sub._compat import TypeVar", + id="origin-in-subpackage", + ), + pytest.param( + ("file_1.py", "sub/use.py"), + "from ..file_1 import T", + "from .._compat import TypeVar", + id="importing-in-subpackage", + ), + pytest.param(("sub/file_1.py", "use.py"), "from sub.file_1 import T", "from sub._compat import TypeVar", id="absolute-via"), + pytest.param( + ("file_1.py", "file_2.py"), + "from ._compat import TypeVar\nfrom .file_1 import T", + "from ._compat import TypeVar", + id="already-here", + ), + # A top-level origin's `._compat` names no package from here, so the origin itself provides TypeVar. + pytest.param(("file_1.py", "use.py"), "from file_1 import T", "from file_1 import TypeVar", id="not-rebasable"), + ], + ) + def test_localized_file_imports_a_relatively_imported_constructor_from_the_same_module( + self, + mocker: MockerFixture, + tmp_path: Path, + layout: tuple[str, str], + importing_header: str, + expected_line: str, + ) -> None: + """Verify a constructor the origin imports relatively is imported once, from that same module as seen from here.""" + subject = self._create_cross_file( + mocker, + tmp_path, + 'from ._compat import TypeVar\nT = TypeVar("T")\n', + f"{importing_header}\ndef b(x: T) -> T:\n return x\n", + layout, + ) + result = subject.localize_imported_typevars() + + assert_that(result, has_entry("T", "fixed")) + assert_that(subject.apply_to_string().splitlines().count(expected_line), equal_to(1)) + + def test_does_not_localize_when_origin_imports_constructor_conditionally( + self, + mocker: MockerFixture, + tmp_path: Path, + ) -> None: + """A TypeVar whose origin picks TypeVar per Python version stays imported, marked unsafe.""" + subject = self._create_cross_file( + mocker, + tmp_path, + """ + import sys + if sys.version_info >= (3, 13): + from typing import TypeVar + else: + from typing_extensions import TypeVar + T = TypeVar("T", contravariant=True, default=None) + """, + """ + from typing import TypeVar + from file_1 import T, helper + def b(x: T) -> None: + helper() + """, + ) + result = subject.localize_imported_typevars() + + assert_that(result, has_entry("T", "unsafe")) + assert_that( + subject.cross_file_unsafe_reasons, + has_entry("T", UnsafeReason.ORIGIN_IMPORTS_CONSTRUCTOR_CONDITIONALLY), + ) + output = subject.apply_to_string() + assert_that(output, contains_string("from file_1 import T, helper")) + assert_that(output, not_(contains_string("T = TypeVar"))) + + def test_unparsable_origin_error_names_the_origin_file(self, mocker: MockerFixture, tmp_path: Path) -> None: + """Verify a syntax error in the origin module is raised with the origin file's path, not ``.""" + subject = self._create_cross_file( + mocker, + tmp_path, + """ + from typing import TypeVar + T = TypeVar("T") + def broken(: + """, + """ + from file_1 import T + def b(x: T) -> T: + return x + """, + ) + + with pytest.raises(SyntaxError) as excinfo: + subject.localize_imported_typevars() + + assert_that(excinfo.value.filename, equal_to(str(tmp_path / "file_1.py"))) + + def test_no_typevar_import_found(self, mocker: MockerFixture, tmp_path: Path) -> None: + """AI: Verify localize_imported_typevars reports nothing when the importing file has no cross-file TypeVar.""" + subject = self._create_cross_file( + mocker, + tmp_path, + """ + def helper() -> None: + pass + """, + """ + from file_1 import helper + def b() -> None: + helper() + """, + ) + result = subject.localize_imported_typevars() + + assert_that(result, equal_to({})) + + @pytest.mark.parametrize( + ("min_python", "expected_converted", "output_matcher"), + [ + pytest.param( + PEP_695_MINIMUM, + "fixed", + all_of(contains_string("def b[T](x: T) -> T:"), not_(contains_string("T = TypeVar"))), + id="converts-on-3.12", + ), + pytest.param( + (3, 10), + "unsafe", + all_of(contains_string("def b(x: T) -> T:"), contains_string('T = TypeVar("T")')), + id="only-localizes-below-3.12", + ), + ], + ) + def test_check_localizes_then_converts_when_the_target_allows( + self, + mocker: MockerFixture, + tmp_path: Path, + min_python: tuple[int, int], + expected_converted: str, + output_matcher: Matcher[str], + ) -> None: + """Verify check() localizes a cross-file TypeVar, then converts it only if the target supports PEP 695.""" + subject = self._create_cross_file( + mocker, + tmp_path, + """ + from typing import TypeVar + T = TypeVar("T") + def a(x: T) -> T: + return x + """, + """ + from file_1 import T + def b(x: T) -> T: + return x + """, + ) + subject.min_python = min_python + subject.run() + + assert_that(subject.result["cross_file"], has_entry("T", "fixed")) + assert_that(subject.result["converted"], has_entry("T", expected_converted)) + assert_that(subject.apply_to_string(), output_matcher) diff --git a/test/recipes/test_type_var_check_orphaned.py b/test/recipes/test_type_var_check_orphaned.py new file mode 100644 index 00000000..dc1c1356 --- /dev/null +++ b/test/recipes/test_type_var_check_orphaned.py @@ -0,0 +1,179 @@ +# Copyright (C) 2024-2026 TNO +# +# Licensed under the terms specified in the LICENSE file in the root directory +# of this repository, also available at: +# https://github.com/TNO/Renaissance.Py/blob/main/LICENSE + +"""Tests for TypeVarCheck.remove_orphaned_declarations.""" + +import textwrap +from collections.abc import Callable + +import pytest +from hamcrest import assert_that, contains_string, equal_to, has_entry, not_ + +from renaissance.recipes.type_var_check import TypeVarCheck +from renaissance.recipes.type_var_domain import UnsafeReason + +ALREADY_CONVERTED_SOURCE = """ +from typing import TypeVar +T = TypeVar('T') + +def b[T](x: T) -> T: + return x +""" + +UNREFERENCED_SOURCE = """ +from typing import TypeVar +T = TypeVar('T') + +def b() -> None: + pass +""" + +NESTED_ONLY_CONVERTED_SOURCE = """ +from typing import TypeVar +T = TypeVar('T') + +def outer() -> None: + def inner[T](x: T) -> T: + return x +""" + +CLOSURE_IN_CONVERTED_FUNCTION_SOURCE = """ +from typing import TypeVar +T = TypeVar('T') + +def outer[T](x: T) -> T: + def inner(y: T) -> T: + return y + return inner(x) +""" + +CLASS_WITH_OWN_TYPE_PARAMETER_SOURCE = """ +from typing import TypeVar +T = TypeVar('T') + +class Box[T]: + def get(self, x: T) -> T: + return x +""" + + +class TestTypeVarCheckOrphaned: + """See module docstring.""" + + @pytest.mark.parametrize( + ("source", "min_python"), + [ + pytest.param(ALREADY_CONVERTED_SOURCE, (3, 12), id="already-converted"), + pytest.param(ALREADY_CONVERTED_SOURCE, (3, 11), id="already-converted-below-pep695"), + pytest.param(UNREFERENCED_SOURCE, (3, 11), id="unreferenced-below-pep695"), + pytest.param(NESTED_ONLY_CONVERTED_SOURCE, (3, 12), id="only-nested-function-converted"), + pytest.param(CLOSURE_IN_CONVERTED_FUNCTION_SOURCE, (3, 12), id="closure-inside-converted-function"), + pytest.param(CLASS_WITH_OWN_TYPE_PARAMETER_SOURCE, (3, 12), id="class-with-own-type-parameter"), + ], + ) + def test_removes_declaration_whose_references_are_all_shadowed( + self, + create_type_var_check: Callable[[str], TypeVarCheck], + source: str, + min_python: tuple[int, int], + ) -> None: + """Verify a declaration whose references are all shadowed by PEP 695 parameters, or absent, is removed.""" + subject = create_type_var_check(source) + subject.min_python = min_python + + result = subject.remove_orphaned_declarations() + + assert_that(result, equal_to({"T": "fixed"})) + output = subject.apply_to_string() + assert_that(output, not_(contains_string("T = TypeVar"))) + for def_line in (line.strip() for line in textwrap.dedent(source).splitlines() if line.strip().startswith("def ")): + assert_that(output, contains_string(def_line)) + + def test_keeps_declaration_still_used_without_pep695(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """Verify a declaration still used by a function without its own type parameter is left alone, unreported.""" + subject = create_type_var_check(""" + from typing import TypeVar + T = TypeVar('T') + + def a[T](x: T) -> T: + return x + def b(y: T) -> T: + return y + """) + result = subject.remove_orphaned_declarations() + + assert_that(result, equal_to({})) + assert_that(subject.apply_to_string(), contains_string("T = TypeVar('T')")) + + @pytest.mark.parametrize( + "annotation", + [ + pytest.param('"T"', id="whole-annotation"), + pytest.param('"list[T]"', id="inside-annotation"), + ], + ) + def test_keeps_declaration_used_in_a_string_annotation( + self, create_type_var_check: Callable[[str], TypeVarCheck], annotation: str + ) -> None: + """Verify a declaration referenced only from a string annotation is not removed as orphaned.""" + subject = create_type_var_check(f""" + from typing import TypeVar + T = TypeVar('T') + + def b(x: {annotation}) -> None: + pass + """) + result = subject.remove_orphaned_declarations() + + assert_that(result, equal_to({})) + assert_that(subject.apply_to_string(), contains_string("T = TypeVar('T')")) + + @pytest.mark.parametrize( + ("imported_elsewhere", "expected_reason"), + [ + pytest.param(frozenset[str](), UnsafeReason.DECLARED_TYPEVAR_EXPORTED, id="exported-via-dunder-all"), + pytest.param(frozenset({"T"}), UnsafeReason.IMPORTED_ELSEWHERE_IN_PROJECT, id="imported-elsewhere"), + ], + ) + def test_keeps_orphaned_declaration_that_is_unsafe_to_remove( + self, + create_type_var_check: Callable[[str], TypeVarCheck], + imported_elsewhere: frozenset[str], + expected_reason: UnsafeReason, + ) -> None: + """Verify an orphaned declaration is kept and reported unsafe when removing it would break an importer.""" + source = ALREADY_CONVERTED_SOURCE if imported_elsewhere else '__all__ = ["T"]\n' + ALREADY_CONVERTED_SOURCE + subject = create_type_var_check(source) + subject.project_wide_imported_names = imported_elsewhere + + result = subject.remove_orphaned_declarations() + + assert_that(result, equal_to({"T": "unsafe"})) + assert_that(subject.orphaned_unsafe_reasons, equal_to({"T": expected_reason})) + assert_that(subject.apply_to_string(), contains_string("T = TypeVar('T')")) + + def test_removing_orphaned_declaration_keeps_comment_shared_with_next_declaration( + self, create_type_var_check: Callable[[str], TypeVarCheck] + ) -> None: + """Verify removing an orphaned declaration keeps a leading comment that also documents the next declaration.""" + subject = create_type_var_check(""" + from typing import TypeVar + + # explains both T and U below + T = TypeVar('T') + U = TypeVar('U') + + Pair = tuple[U, U] + + def b[T](x: T) -> T: + return x + """) + result = subject.remove_orphaned_declarations() + + assert_that(result, has_entry("T", "fixed")) + output = subject.apply_to_string() + assert_that(output, contains_string("# explains both T and U below")) + assert_that(output, contains_string("U = TypeVar('U')")) diff --git a/test/recipes/test_type_var_check_properties.py b/test/recipes/test_type_var_check_properties.py new file mode 100644 index 00000000..123a51ff --- /dev/null +++ b/test/recipes/test_type_var_check_properties.py @@ -0,0 +1,37 @@ +# Copyright (C) 2024-2026 TNO +# +# Licensed under the terms specified in the LICENSE file in the root directory +# of this repository, also available at: +# https://github.com/TNO/Renaissance.Py/blob/main/LICENSE + +"""Property-based tests for TypeVarCheck.check.""" + +import ast +from unittest.mock import patch + +import hypothesmith +from hypothesis import given, reject, settings + +from renaissance.integrations.python.ast.cst_node import PythonCstNode +from renaissance.recipes.type_var_check import TypeVarCheck + + +class TestTypeVarCheckProperties: + """See module docstring.""" + + @given(source=hypothesmith.from_grammar()) + @settings(max_examples=50, deadline=None) + def test_check_never_crashes(self, source: str) -> None: + """AI: Verify check() never raises on arbitrary hypothesmith-generated valid Python source.""" + try: + ast.parse(source) + except SyntaxError: + reject() + + with patch( + "renaissance.integrations.python.ast.factory.PythonFactory.create", + return_value=PythonCstNode.load_from_text(source), + ): + subject = TypeVarCheck("x.py") + subject.in_memory = True + subject.check() diff --git a/test/recipes/test_type_var_domain.py b/test/recipes/test_type_var_domain.py new file mode 100644 index 00000000..d336f533 --- /dev/null +++ b/test/recipes/test_type_var_domain.py @@ -0,0 +1,141 @@ +# Copyright (C) 2024-2026 TNO +# +# Licensed under the terms specified in the LICENSE file in the root directory +# of this repository, also available at: +# https://github.com/TNO/Renaissance.Py/blob/main/LICENSE + +"""Tests for type_var_domain's safety predicates and their UnsafeReason results.""" + +import re +import textwrap +from pathlib import Path + +import libcst as cst +import pytest +from hamcrest import assert_that, equal_to, not_ + +from renaissance.recipes.type_var_domain import ( + UNSAFE_RULES, + UnsafeReason, + is_safe_to_localize, + is_safe_to_remove, +) + + +def _parse(source: str) -> cst.Module: + """Parse source after removing its common indentation.""" + return cst.parse_module(textwrap.dedent(source)) + + +class TestIsSafeToRemove: + """is_safe_to_remove: None when safe, the specific UnsafeReason otherwise.""" + + def test_returns_none_when_safe(self) -> None: + """A TypeVar not exported and not imported by another project file is safe to remove, whatever else is.""" + tree = _parse(""" + from typing import TypeVar + + def a(x: T) -> T: + return x + + T = TypeVar("T") + """) + + assert_that(is_safe_to_remove(tree, "T", frozenset({"U"})), equal_to(None)) + + @pytest.mark.parametrize( + ("dunder_all", "imported_elsewhere", "expected_reason"), + [ + pytest.param('__all__ = ["T"]', frozenset[str](), UnsafeReason.DECLARED_TYPEVAR_EXPORTED, id="exported-via-dunder-all"), + pytest.param("", frozenset({"T"}), UnsafeReason.IMPORTED_ELSEWHERE_IN_PROJECT, id="imported-elsewhere"), + ], + ) + def test_returns_the_specific_reason_when_unsafe( + self, dunder_all: str, imported_elsewhere: frozenset[str], expected_reason: UnsafeReason + ) -> None: + """Each unsafe condition is distinguishable, not collapsed into one generic reason.""" + tree = _parse(f""" + from typing import TypeVar + + {dunder_all} + + def a(x: T) -> T: + return x + + T = TypeVar("T") + """) + + assert_that(is_safe_to_remove(tree, "T", imported_elsewhere), equal_to(expected_reason)) + + +class TestIsSafeToLocalize: + """is_safe_to_localize: None when safe, the specific UnsafeReason otherwise.""" + + @pytest.mark.parametrize( + "source", + [ + pytest.param('from typing import TypeVar\nT = TypeVar("T")\n', id="unconditional-import"), + pytest.param( + "from typing import TYPE_CHECKING, TypeVar\n" + "if TYPE_CHECKING:\n" + " from collections.abc import Sequence\n" + 'T = TypeVar("T")\n', + id="conditional-import-of-another-name", + ), + ], + ) + def test_returns_none_when_safe(self, source: str) -> None: + """A TypeVar whose constructor its origin imports unconditionally is safe to localize, whatever else is conditional.""" + assert_that(is_safe_to_localize(_parse(source), "T"), equal_to(None)) + + @pytest.mark.parametrize( + ("source", "expected_reason"), + [ + pytest.param( + """ + import sys + + if sys.version_info >= (3, 13): + from typing import TypeVar + else: + from typing_extensions import TypeVar + + T = TypeVar("T", default=None) + """, + UnsafeReason.ORIGIN_IMPORTS_CONSTRUCTOR_CONDITIONALLY, + id="if-version-check", + ), + pytest.param( + """ + try: + from typing import ParamSpec + except ImportError: + from typing_extensions import ParamSpec + + T = ParamSpec("T") + """, + UnsafeReason.ORIGIN_IMPORTS_CONSTRUCTOR_CONDITIONALLY, + id="try-except-import", + ), + ], + ) + def test_returns_the_specific_reason_when_unsafe(self, source: str, expected_reason: UnsafeReason) -> None: + """Each unsafe condition is distinguishable, not collapsed into one generic reason.""" + tree = _parse(source) + + assert_that(is_safe_to_localize(tree, "T"), equal_to(expected_reason)) + + +_FEATURE_DOC = Path(__file__).resolve().parents[2] / "docs" / "user" / "features" / "type-parameter-modernization.md" + + +class TestUnsafeRuleDocAnchors: + """Every UnsafeRule's documentation anchor exists where the CLI's report links to it.""" + + @pytest.mark.parametrize("reason", list(UnsafeReason)) + def test_anchor_is_set_on_a_heading(self, reason: UnsafeReason) -> None: + """The rule's anchor is attached to a heading line, the only place attr_list turns it into a link target.""" + anchor = UNSAFE_RULES[reason].doc_anchor + heading = re.compile(rf"^#+ .+ \{{ #{re.escape(anchor)} \}}$", re.MULTILINE) + + assert_that(heading.search(_FEATURE_DOC.read_text(encoding="utf-8")), not_(None)) diff --git a/test/recipes/test_unit_to_pytest.py b/test/recipes/test_unit_to_pytest.py index 6246c24d..d1384912 100644 --- a/test/recipes/test_unit_to_pytest.py +++ b/test/recipes/test_unit_to_pytest.py @@ -10,6 +10,7 @@ from pathlib import Path from hamcrest import assert_that, contains_string, ends_with, is_, not_ +from pytest_mock import MockerFixture import targets from renaissance.integrations.python.ast.rst_node import PythonRstNode @@ -24,7 +25,7 @@ def test_init(self) -> None: subject = UnitToPytest(Path(targets.__file__).parent / "demo.py") assert_that(subject.filename, ends_with("demo.py")) - def test_commit_does_nothing_when_not_changed(self, mocker) -> None: + def test_commit_does_nothing_when_not_changed(self, mocker: MockerFixture) -> None: """AI: Verify has_changed() returns False when the code hasn't been modified.""" subject = self._create( mocker, @@ -34,7 +35,7 @@ def test_commit_does_nothing_when_not_changed(self, mocker) -> None: ) assert_that(subject.has_changed(), is_(False)) - def test_convert_test_class_updates_only_testcase_bases(self, mocker) -> None: + def test_convert_test_class_updates_only_testcase_bases(self, mocker: MockerFixture) -> None: """AI: Verify convert_test_class rewrites only classes that extend TestCase, and normalizes their names.""" subject = self._create( mocker, @@ -50,7 +51,7 @@ class Class2Test(unittest.TestCase): assert_that(subject.apply_to_string(), contains_string("class TestClass1:")) assert_that(subject.apply_to_string(), contains_string("class TestClass2:")) - def _create(self, mocker, text) -> UnitToPytest: + def _create(self, mocker: MockerFixture, text) -> UnitToPytest: code = textwrap.dedent(text) mocker.patch( "renaissance.integrations.python.ast.factory.PythonFactory.create", @@ -60,7 +61,7 @@ def _create(self, mocker, text) -> UnitToPytest: subject.in_memory = True return subject - def test_convert_plain_assert_same_length_rewrites_to_has_length(self, mocker) -> None: + def test_convert_plain_assert_same_length_rewrites_to_has_length(self, mocker: MockerFixture) -> None: """AI: Verify convert_plain_assert_same_length rewrites a length-equality assert into assert_that/has_length.""" expected = textwrap.dedent(""" def test_asert(): @@ -80,7 +81,7 @@ def test_asert(): subject.convert_plain_assert_same_length() assert_that(subject.apply_to_string(), is_(expected)) - def test_restructure_module_injects_methods_when_class_exists(self, mocker) -> None: + def test_restructure_module_injects_methods_when_class_exists(self, mocker: MockerFixture) -> None: """AI: Verify restructure_module moves a module-level function into the existing test class as a method.""" subject = self._create( mocker, @@ -99,7 +100,7 @@ def parse(a): assert_that(subject.apply_to_string(), contains_string("def parse(self,a):")) - def test_convert(self, mocker) -> None: + def test_convert(self, mocker: MockerFixture) -> None: """AI: Verify run() invokes convert_test_class, convert_test_setup, and replace_stmt the expected number of times.""" sut = self._create( mocker, @@ -119,7 +120,7 @@ def test_fun(self): assert_that(spy2.call_count, is_(1)) assert_that(spy3.call_count, is_(26)) - def test_convert_assert(self, mocker) -> None: + def test_convert_assert(self, mocker: MockerFixture) -> None: """AI: Verify convert_assert rewrites assertEqual calls into assert_that/is_, regardless of argument order.""" sut = self._create( mocker, @@ -134,7 +135,7 @@ def test_fun(self): assert_that(sut.apply_to_string(), contains_string("assert_that(call()")) assert_that(sut.apply_to_string(), not_(contains_string("assert_that(1"))) - def test_to_assertthat(self, mocker) -> None: + def test_to_assertthat(self, mocker: MockerFixture) -> None: """AI: Verify a manual replace_stmt call rewrites a plain assert into assert_that(..., is_(True), ...).""" sut = self._create( mocker, @@ -148,7 +149,7 @@ def test_fun(): assert_that(sut.apply_to_string(), contains_string("assert_that(call()")) assert_that(sut.apply_to_string(), not_(contains_string("assert_that(1"))) - def test_convert_test_class_renames_class_ending_with_test(self, mocker) -> None: + def test_convert_test_class_renames_class_ending_with_test(self, mocker: MockerFixture) -> None: """AI: Verify convert_test_class renames a FooTest class to TestFoo.""" subject = self._create( mocker, @@ -160,7 +161,7 @@ class FooTest(TestCase): subject.convert_test_class() assert_that(subject.apply_to_string(), contains_string("class TestFoo:")) - def test_convert_parameterized_test_at_top_level(self, mocker) -> None: + def test_convert_parameterized_test_at_top_level(self, mocker: MockerFixture) -> None: """AI: Verify convert_parameterized_test rewrites a top-level @parameterized.expand into @pytest.mark.parametrize.""" subject = self._create( mocker, @@ -174,7 +175,7 @@ def test_fun(self, val): subject.convert_parameterized_test() assert_that(subject.apply_to_string(), contains_string("@pytest.mark.parametrize")) - def test_convert_parameterized_test_inside_class(self, mocker) -> None: + def test_convert_parameterized_test_inside_class(self, mocker: MockerFixture) -> None: """AI: Verify convert_parameterized_test rewrites a class-nested @parameterized.expand into @pytest.mark.parametrize.""" subject = self._create( mocker, @@ -189,7 +190,7 @@ def test_fun(self, val): subject.convert_parameterized_test() assert_that(subject.apply_to_string(), contains_string("@pytest.mark.parametrize")) - def test_remove_print_removes_entire_function_when_only_statement(self, mocker) -> None: + def test_remove_print_removes_entire_function_when_only_statement(self, mocker: MockerFixture) -> None: """AI: Verify remove_print removes the entire function when print() is its only statement.""" subject = self._create( mocker, @@ -201,7 +202,7 @@ def test_foo(self): subject.remove_print() assert_that(subject.apply_to_string(), not_(contains_string("test_foo"))) - def test_remove_print_removes_only_print_when_other_statements_exist(self, mocker) -> None: + def test_remove_print_removes_only_print_when_other_statements_exist(self, mocker: MockerFixture) -> None: """AI: Verify remove_print removes only the print() call, keeping other statements in the function.""" subject = self._create( mocker, @@ -215,7 +216,7 @@ def test_foo(self): assert_that(subject.apply_to_string(), not_(contains_string("print"))) assert_that(subject.apply_to_string(), contains_string("assert 1 == 1")) - def test_convert_plain_assert_same_length_when_not_swapped(self, mocker) -> None: + def test_convert_plain_assert_same_length_when_not_swapped(self, mocker: MockerFixture) -> None: """AI: Verify convert_plain_assert_same_length still rewrites to has_length when operands aren't swapped.""" subject = self._create( mocker, @@ -229,7 +230,7 @@ def test_foo(): subject.convert_plain_assert_same_length() assert_that(subject.apply_to_string(), contains_string("has_length")) - def test_convert_skip_test_replaces_unittest_skip(self, mocker) -> None: + def test_convert_skip_test_replaces_unittest_skip(self, mocker: MockerFixture) -> None: """AI: Verify convert_skip_test rewrites @unittest.skip to @pytest.mark.skip.""" subject = self._create( mocker, @@ -243,7 +244,7 @@ def test_foo(self): assert_that(subject.apply_to_string(), contains_string("pytest.mark.skip")) assert_that(subject.apply_to_string(), not_(contains_string("unittest.skip"))) - def test_swap_expected_and_actual_swaps_when_literal_is_expected(self, mocker) -> None: + def test_swap_expected_and_actual_swaps_when_literal_is_expected(self, mocker: MockerFixture) -> None: """AI: Verify swap_expected_and_actual reorders assert_that args so the literal becomes the expected value.""" subject = self._create( mocker, @@ -255,7 +256,7 @@ def test_foo(self): subject.swap_expected_and_actual() assert_that(subject.apply_to_string(), contains_string("assert_that(call(), is_(1))")) - def test_restructure_module_moves_functions_into_existing_test_class(self, mocker) -> None: + def test_restructure_module_moves_functions_into_existing_test_class(self, mocker: MockerFixture) -> None: """AI: Verify restructure_module moves a standalone helper function into the existing TestFoo class.""" subject = self._create( mocker, @@ -272,7 +273,7 @@ def helper(a): subject.commit() assert_that(subject.apply_to_string(), contains_string("def helper(self,a):")) - def test_remove_duplicate_import_removes_middle_duplicates(self, mocker) -> None: + def test_remove_duplicate_import_removes_middle_duplicates(self, mocker: MockerFixture) -> None: """AI: Verify remove_duplicate_import collapses repeated identical import statements down to the first and last.""" subject = self._create( mocker, @@ -291,7 +292,7 @@ def test_foo(): result = subject.apply_to_string() assert_that(result.count("import pytest"), is_(2)) - def test_convert_test_setup_adds_pytest_fixture(self, mocker) -> None: + def test_convert_test_setup_adds_pytest_fixture(self, mocker: MockerFixture) -> None: """AI: Verify convert_test_setup rewrites setUp into a pytest autouse fixture named setup.""" subject = self._create( mocker, @@ -307,7 +308,7 @@ def test_foo(self): assert_that(subject.apply_to_string(), contains_string("@pytest.fixture(autouse=True)")) assert_that(subject.apply_to_string(), contains_string("def setup(self)")) - def test_convert_parameterized_test_with_vargs(self, mocker) -> None: + def test_convert_parameterized_test_with_vargs(self, mocker: MockerFixture) -> None: """AI: Verify convert_parameterized_test preserves *rest varargs when rewriting to @pytest.mark.parametrize.""" subject = self._create( mocker, @@ -322,7 +323,7 @@ def test_fun(self, val, *rest): assert_that(subject.apply_to_string(), contains_string("@pytest.mark.parametrize")) assert_that(subject.apply_to_string(), contains_string("*rest")) - def test_restructure_module_rewrites_call_sites_in_existing_class(self, mocker) -> None: + def test_restructure_module_rewrites_call_sites_in_existing_class(self, mocker: MockerFixture) -> None: """AI: Verify restructure_module rewrites call sites of a moved helper function to use self.""" subject = self._create( mocker, @@ -339,13 +340,13 @@ def helper(a): subject.commit() assert_that(subject.apply_to_string(), contains_string("self.helper(1)")) - def test_convert_file_to_test_class_strips_trailing_test(self, mocker) -> None: + def test_convert_file_to_test_class_strips_trailing_test(self, mocker: MockerFixture) -> None: """AI: Verify convert_file_to_test_class strips a trailing '_test' suffix when deriving the class name.""" subject = self._create(mocker, "pass") mocker.patch.object(type(subject), "filename", new_callable=lambda: property(lambda self: "my_module_test.py")) assert_that(subject.convert_file_to_test_class(), is_("TestMyModule")) - def test_convert_file_to_test_class_keeps_test_prefix(self, mocker) -> None: + def test_convert_file_to_test_class_keeps_test_prefix(self, mocker: MockerFixture) -> None: """AI: Verify convert_file_to_test_class keeps a leading 'test_' prefix when deriving the class name.""" subject = self._create(mocker, "pass") mocker.patch.object(type(subject), "filename", new_callable=lambda: property(lambda self: "test_my_module.py")) diff --git a/test/rejuvenation/test_migration_type_recipes.py b/test/rejuvenation/test_migration_type_recipes.py new file mode 100644 index 00000000..bb61884c --- /dev/null +++ b/test/rejuvenation/test_migration_type_recipes.py @@ -0,0 +1,361 @@ +# Copyright (C) 2024-2026 TNO +# +# Licensed under the terms specified in the LICENSE file in the root directory +# of this repository, also available at: +# https://github.com/TNO/Renaissance.Py/blob/main/LICENSE + +"""Tests for the migration-type-recipes.py CLI script (src/rejuvenation). + +The script's filename is hyphenated (not a legal dotted module path), so it's loaded via +importlib.util.spec_from_file_location instead of a normal import - see _load_script(). +""" + +import importlib.util +import textwrap +from pathlib import Path +from types import ModuleType + +import pytest +from hamcrest import assert_that, contains_string, equal_to, not_ +from hamcrest.core.matcher import Matcher + +from renaissance.recipes.type_var_domain import UnsafeReason, doc_link + +_SCRIPT_PATH = Path(__file__).resolve().parents[2] / "src" / "rejuvenation" / "migration-type-recipes.py" + + +def _load_script() -> ModuleType: + """Import migration-type-recipes.py as a module despite its hyphenated filename.""" + spec = importlib.util.spec_from_file_location("migration_type_recipes", _SCRIPT_PATH) + if spec is None or spec.loader is None: + message = f"could not load {_SCRIPT_PATH} as a module" + raise RuntimeError(message) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +migration = _load_script() + + +LEGACY_TYPEVAR_SOURCE = textwrap.dedent("""\ + from typing import TypeVar + + T = TypeVar("T") + + + def identity(x: T) -> T: + return x + """) + +UNSAFE_TYPEVAR_SOURCE = textwrap.dedent("""\ + from typing import TypeVar + + T = TypeVar("T") + + __all__ = ["T"] + + + def identity(x: T) -> T: + return x + """) + + +class TestProcessFile: + """process_file: writes changes for real, and isolates per-file errors.""" + + @pytest.mark.parametrize( + ("source", "imported_elsewhere", "expected_reason"), + [ + pytest.param(UNSAFE_TYPEVAR_SOURCE, frozenset[str](), UnsafeReason.DECLARED_TYPEVAR_EXPORTED, id="exported-via-dunder-all"), + pytest.param(LEGACY_TYPEVAR_SOURCE, frozenset({"T"}), UnsafeReason.IMPORTED_ELSEWHERE_IN_PROJECT, id="imported-elsewhere"), + ], + ) + def test_converts_function_but_keeps_a_declaration_others_rely_on( + self, tmp_path: Path, source: str, imported_elsewhere: frozenset[str], expected_reason: UnsafeReason + ) -> None: + """The function is converted on disk, the declaration stays, and the report says why it stayed.""" + target = tmp_path / "mod.py" + target.write_text(source, encoding="utf-8") + + report = migration.process_file(target, min_python=(3, 12), project_root=tmp_path, project_wide_imported_names=imported_elsewhere) + + assert_that(migration.has_fixed(report), equal_to(obj=True)) + assert_that(report.reasons, not_(None)) + assert_that((report.reasons or {})["orphaned"], equal_to({"T": expected_reason})) + written = target.read_text(encoding="utf-8") + assert_that(written, contains_string("def identity[T](x: T) -> T:")) + assert_that(written, contains_string('T = TypeVar("T")')) + + def test_syntax_error_reported_as_error_not_raised(self, tmp_path: Path) -> None: + """A file that fails to parse is reported on FileReport.error, not raised.""" + target = tmp_path / "broken.py" + target.write_text("def broken(:\n", encoding="utf-8") + + report = migration.process_file(target, min_python=(3, 12), project_root=tmp_path, project_wide_imported_names=frozenset[str]()) + + assert_that(report.error, not_(None)) + assert_that(report.result, equal_to(None)) + + +class TestRuffImportCleanup: + """main(): the ruff F401 batch step actually drops now-unused imports end to end.""" + + @pytest.mark.parametrize( + ("extra_args", "import_matcher", "report_matcher"), + [ + pytest.param( + [], + not_(contains_string("TypeVar")), + contains_string("cleaned up via `ruff"), + id="default-drops-import", + ), + pytest.param( + ["--no-ruff"], + contains_string("from typing import TypeVar"), + not_(contains_string("cleaned up via `ruff")), + id="no-ruff-keeps-import", + ), + ], + ) + def test_unused_typevar_import_cleanup( + self, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], + extra_args: list[str], + import_matcher: Matcher[str], + report_matcher: Matcher[str], + ) -> None: + """The redundant TypeVar import is dropped, and the report says so, unless --no-ruff; conversion runs either way.""" + target = tmp_path / "mod.py" + target.write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") + + exit_code = migration.main([str(target), "--py", "3.12", *extra_args]) + + assert_that(exit_code, equal_to(0)) + written = target.read_text(encoding="utf-8") + assert_that(written, contains_string("def identity[T]")) + assert_that(written, import_matcher) + assert_that(capsys.readouterr().out, report_matcher) + + def test_unmodified_sibling_file_is_left_untouched(self, tmp_path: Path) -> None: + """A sibling file with no TypeVar usage - and its own genuinely-unused import - survives main() byte-for-byte.""" + (tmp_path / "mod.py").write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") + sibling = tmp_path / "sibling.py" + sibling_source = textwrap.dedent("""\ + import os + + + def greet() -> str: + return "hi" + """) + sibling.write_text(sibling_source, encoding="utf-8") + + exit_code = migration.main([str(tmp_path), "--py", "3.12"]) + + assert_that(exit_code, equal_to(0)) + assert_that(sibling.read_text(encoding="utf-8"), equal_to(sibling_source)) + + +class TestConsoleReportDocLinks: + """main(): each unsafe name printed under NEEDS MANUAL REVIEW links to its documented rule.""" + + def test_needs_manual_review_includes_doc_link_for_the_specific_reason( + self, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], + ) -> None: + """The report links a __all__-exported TypeVar to the DECLARED_TYPEVAR_EXPORTED rule.""" + target = tmp_path / "mod.py" + target.write_text(UNSAFE_TYPEVAR_SOURCE, encoding="utf-8") + + migration.main([str(target), "--py", "3.12"]) + + output = capsys.readouterr().out + assert_that(output, contains_string(doc_link(UnsafeReason.DECLARED_TYPEVAR_EXPORTED))) + + +class TestConsoleReportModifiedSection: + """main(): what the MODIFIED section lists per file.""" + + def test_converted_name_is_not_listed_again_as_orphaned(self, tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None: + """A converted name is listed once; only declarations removed without a conversion appear under orphaned.""" + target = tmp_path / "mod.py" + target.write_text( + textwrap.dedent("""\ + from typing import TypeVar + + T = TypeVar("T") + U = TypeVar("U") + + + def f(x: T) -> T: + return x + + + def g[U](y: U) -> U: + return y + """), + encoding="utf-8", + ) + + migration.main([str(target), "--py", "3.12", "--no-ruff"]) + + output = capsys.readouterr().out + assert_that(output, contains_string(" converted: T\n")) + assert_that(output, contains_string(" orphaned: U\n")) + + +class TestPerFileProgressFeedback: + """main(): prints a per-file progress line as each file is checked.""" + + def test_progress_line_path_has_no_parent_segments(self, tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None: + """A target containing `..` is normalized before its files are reported.""" + good = tmp_path / "good.py" + good.write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") + (tmp_path / "sub").mkdir() + + migration.main([str(tmp_path / "sub" / ".."), "--py", "3.12"]) + + output = capsys.readouterr().out + assert_that(output, contains_string(f"File {good} checked.")) + assert_that(output, not_(contains_string(".."))) + + +class TestMainBatchErrorIsolation: + """main(): one bad file in a batch must not abort processing of the rest.""" + + def test_one_bad_file_does_not_abort_the_batch( + self, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], + ) -> None: + """A batch with one broken file still checks and converts the good file, and exits with code 3.""" + good = tmp_path / "good.py" + good.write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") + broken = tmp_path / "broken.py" + broken.write_text("def broken(:\n", encoding="utf-8") + + exit_code = migration.main([str(tmp_path), "--py", "3.12"]) + + assert_that(exit_code, equal_to(3)) + output = capsys.readouterr().out + assert_that(output, contains_string(f"File {good} checked.")) + assert_that(output, contains_string(f"File {broken} checked.")) + assert_that(good.read_text(encoding="utf-8"), contains_string("def identity[T]")) + + +class TestMainProjectWideImportSafety: + """main(): a TypeVar imported by another file in the batch is localized there and kept at its origin.""" + + @pytest.mark.parametrize( + ("consumer_rel", "import_line"), + [ + pytest.param("pkg/client.py", "from pkg.typing_mod import T", id="absolute-dotted"), + pytest.param("pkg/client.py", "from .typing_mod import T", id="relative"), + pytest.param("other/client.py", "from pkg.typing_mod import T", id="absolute-other-directory"), + ], + ) + def test_consumer_is_localized_and_origin_declaration_survives( + self, + tmp_path: Path, + consumer_rel: str, + import_line: str, + ) -> None: + """The importing file gets a PEP 695 local TypeVar, and the origin declaration is not removed.""" + pkg = tmp_path / "pkg" + pkg.mkdir() + (pkg / "__init__.py").write_text("", encoding="utf-8") + (pkg / "typing_mod.py").write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") + consumer = tmp_path / consumer_rel + consumer.parent.mkdir(exist_ok=True) + consumer.write_text(f"{import_line}\n\ndef use(x: T) -> T:\n return x\n", encoding="utf-8") + + exit_code = migration.main([str(tmp_path), "--py", "3.12"]) + + assert_that(exit_code, equal_to(0)) + consumer_text = consumer.read_text(encoding="utf-8") + assert_that(consumer_text, contains_string("def use[T](x: T) -> T:")) + assert_that(consumer_text, not_(contains_string(import_line))) + assert_that((pkg / "typing_mod.py").read_text(encoding="utf-8"), contains_string('T = TypeVar("T")')) + + @pytest.mark.parametrize("target_arg", [pytest.param(".", id="dot"), pytest.param("pkg", id="subdirectory")]) + def test_origin_declaration_survives_with_relative_target( + self, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + target_arg: str, + ) -> None: + """A relative target path still protects a declaration imported by another file in the batch.""" + pkg = tmp_path / "pkg" + pkg.mkdir() + (pkg / "typing_mod.py").write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") + (pkg / "client.py").write_text("from .typing_mod import T\n\ndef use(x: T) -> T:\n return x\n", encoding="utf-8") + monkeypatch.chdir(tmp_path) + + exit_code = migration.main([target_arg, "--py", "3.12"]) + + assert_that(exit_code, equal_to(0)) + assert_that((pkg / "typing_mod.py").read_text(encoding="utf-8"), contains_string('T = TypeVar("T")')) + assert_that((pkg / "client.py").read_text(encoding="utf-8"), contains_string("def use[T](x: T) -> T:")) + + def test_origin_declaration_survives_module_attribute_access(self, tmp_path: Path) -> None: + """A TypeVar accessed as a module attribute by another file is kept at its origin.""" + pkg = tmp_path / "pkg" + pkg.mkdir() + (pkg / "__init__.py").write_text("", encoding="utf-8") + (pkg / "typing_mod.py").write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") + (pkg / "client.py").write_text("import pkg.typing_mod\n\ndef use(x: pkg.typing_mod.T) -> None: ...\n", encoding="utf-8") + + exit_code = migration.main([str(tmp_path), "--py", "3.12"]) + + assert_that(exit_code, equal_to(0)) + assert_that((pkg / "typing_mod.py").read_text(encoding="utf-8"), contains_string('T = TypeVar("T")')) + + +class TestPyVersionFlag: + """main(): the required --py flag alone sets the target's minimum Python version.""" + + @pytest.mark.parametrize( + ("py_version", "expected", "unexpected"), + [ + pytest.param("3.11", 'T = TypeVar("T")', "def identity[", id="3.11-gated"), + pytest.param("3.12", "def identity[T]", 'T = TypeVar("T")', id="3.12-converted"), + ], + ) + def test_py_flag_gates_rewrites(self, tmp_path: Path, py_version: str, expected: str, unexpected: str) -> None: + """--py decides whether the PEP 695 rewrite runs.""" + target = tmp_path / "mod.py" + target.write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") + + exit_code = migration.main([str(target), "--py", py_version]) + + assert_that(exit_code, equal_to(0)) + written = target.read_text(encoding="utf-8") + assert_that(written, contains_string(expected)) + assert_that(written, not_(contains_string(unexpected))) + + @pytest.mark.parametrize( + ("bad_args", "expected_error"), + [ + pytest.param([], "the following arguments are required: --py", id="flag-missing"), + pytest.param(["--py", "3"], "expected MAJOR.MINOR (e.g. 3.12), got '3'", id="missing-minor"), + pytest.param(["--py", "3.x"], "expected MAJOR.MINOR (e.g. 3.12), got '3.x'", id="non-numeric"), + ], + ) + def test_bad_version_arguments_are_usage_errors( + self, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], + bad_args: list[str], + expected_error: str, + ) -> None: + """A missing or malformed --py flag exits with code 2 and an error saying what is wrong with it.""" + target = tmp_path / "mod.py" + target.write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") + + with pytest.raises(SystemExit) as excinfo: + migration.main([str(target), *bad_args]) + + assert_that(excinfo.value.code, equal_to(2)) + assert_that(capsys.readouterr().err, contains_string(expected_error)) diff --git a/test/syntax_tree/test_ast_processor.py b/test/syntax_tree/test_ast_processor.py index d4285937..fb2ff74d 100644 --- a/test/syntax_tree/test_ast_processor.py +++ b/test/syntax_tree/test_ast_processor.py @@ -7,6 +7,7 @@ """Tests for the ASTProcessor pattern-match/find functionality.""" from hamcrest import assert_that, is_ +from pytest_mock import MockerFixture from renaissance.integrations.clang import ClangASTNode from renaissance.syntax_tree import ASTFactory, ASTProcessor, PatternMatch @@ -15,7 +16,7 @@ class TestAstProcessor: """AI: Tests for the ASTProcessor pattern-match/find functionality.""" - def test_find_match(self, mocker) -> None: + def test_find_match(self, mocker: MockerFixture) -> None: """AI: Verify find_match delegates to the match_finder.find_all function.""" node = mocker.Mock() pattern_match = PatternMatch([node, node, node], {}, []) diff --git a/test/syntax_tree/test_ast_refactor_actions.py b/test/syntax_tree/test_ast_refactor_actions.py index 26ff5189..29549b01 100644 --- a/test/syntax_tree/test_ast_refactor_actions.py +++ b/test/syntax_tree/test_ast_refactor_actions.py @@ -7,6 +7,7 @@ """Tests for the ASTRefactorActions helper.""" from hamcrest import assert_that, is_ +from pytest_mock import MockerFixture from renaissance.syntax_tree import ASTRefactorActions from renaissance.syntax_tree.semantic_kind import SemanticKind @@ -15,14 +16,14 @@ class TestASTRefactorActions: """AI: Tests for the ASTRefactorActions helper.""" - def test_it_can_be_created(self, mocker) -> None: + def test_it_can_be_created(self, mocker: MockerFixture) -> None: """AI: Verify ASTRefactorActions can be constructed from a processor and factory.""" proc = mocker.Mock() factory = mocker.Mock() refactor_actions = ASTRefactorActions(proc, factory) assert_that(refactor_actions, not is_(None)) - def test_replace_expr(self, mocker) -> None: + def test_replace_expr(self, mocker: MockerFixture) -> None: """AI: Verify replace_expr delegates to the processor's find_all method.""" proc = mocker.Mock() proc.find_all.return_value = [] @@ -31,7 +32,7 @@ def test_replace_expr(self, mocker) -> None: refactor_actions.replace_expr("name", "my_awsome_name", SemanticKind.NAME) assert_that(proc.find_all.called) - def test_replace_name(self, mocker) -> None: + def test_replace_name(self, mocker: MockerFixture) -> None: """AI: Verify replace_name finds a matching name node and calls the processor's replace method.""" node = mocker.Mock() node.offset = 1 @@ -46,7 +47,7 @@ def test_replace_name(self, mocker) -> None: assert_that(proc.replace.called) - def test_replace_text(self, mocker) -> None: + def test_replace_text(self, mocker: MockerFixture) -> None: """AI: Verify replace_text finds matching literal nodes and calls the processor's replace method.""" node = mocker.Mock() node.semantic_kind = SemanticKind.LITERAL @@ -61,7 +62,7 @@ def test_replace_text(self, mocker) -> None: assert_that(proc.replace.called) - def test_replace_declaration(self, mocker) -> None: + def test_replace_declaration(self, mocker: MockerFixture) -> None: """AI: Verify replace_declaration finds the declaration and calls the processor's replace method.""" node = mocker.Mock() proc = mocker.Mock() @@ -73,7 +74,7 @@ def test_replace_declaration(self, mocker) -> None: assert_that(proc.replace.called) - def test_replace_patterns(self, mocker) -> None: + def test_replace_patterns(self, mocker: MockerFixture) -> None: """AI: Verify _replace_patterns matches the pattern and calls the processor's replace method.""" node = mocker.Mock() proc = mocker.Mock() @@ -86,7 +87,7 @@ def test_replace_patterns(self, mocker) -> None: assert_that(proc.replace.called) assert_that(is_match_mock.called) - def test_find_declaration(self, mocker) -> None: + def test_find_declaration(self, mocker: MockerFixture) -> None: """AI: Verify find_declaration delegates to the processor's find_match method.""" proc = mocker.Mock() factory = mocker.Mock() @@ -94,7 +95,7 @@ def test_find_declaration(self, mocker) -> None: refactor_actions.find_declaration("decl_pattern") assert_that(proc.find_match.called) - def test_collect(self, mocker) -> None: + def test_collect(self, mocker: MockerFixture) -> None: """AI: Verify collect delegates to the processor's find_match method.""" proc = mocker.Mock() proc.find_match.return_value = [] diff --git a/test/syntax_tree/test_ast_rewriter.py b/test/syntax_tree/test_ast_rewriter.py index 3b328dc2..ab9b3659 100644 --- a/test/syntax_tree/test_ast_rewriter.py +++ b/test/syntax_tree/test_ast_rewriter.py @@ -11,6 +11,7 @@ import pytest from hamcrest import assert_that, has_length, is_ +from pytest_mock import MockerFixture from c_cpp.factories import Factories from renaissance.integrations.clang import ClangASTNode, CPatternFactory @@ -1016,7 +1017,7 @@ def test_args( actual = rewriter.apply_to_string() assert_that(compress(expected), is_(compress(actual))) - def test_get_node_in_match_pattern(self, mocker) -> None: + def test_get_node_in_match_pattern(self, mocker: MockerFixture) -> None: """AI: Verify _RewriteAction._get_nodes resolves a node's referenced_by chain back to the original node.""" node = mocker.Mock() reference = mocker.Mock() @@ -1033,7 +1034,7 @@ def test_get_node_in_match_pattern_on_empty_pattern(self) -> None: text = it._RewriteActions__get_texts([]) assert_that(text, is_("node")) - def test_get_text_from_rewrite(self, mocker) -> None: + def test_get_text_from_rewrite(self, mocker: MockerFixture) -> None: """AI: Verify __get_texts extracts the node's text slice from its binary file content.""" node = mocker.Mock() node.root = node diff --git a/test/syntax_tree/test_batch_ast_processor.py b/test/syntax_tree/test_batch_ast_processor.py index 43cde1d3..c0d65610 100644 --- a/test/syntax_tree/test_batch_ast_processor.py +++ b/test/syntax_tree/test_batch_ast_processor.py @@ -7,6 +7,7 @@ """Tests for the BatchASTProcessor.""" from hamcrest import assert_that, has_length, is_ +from pytest_mock import MockerFixture from renaissance.syntax_tree import BatchASTProcessor from renaissance.syntax_tree import batch_ast_processor as bap @@ -21,7 +22,7 @@ def test_it(self) -> None: assert_that(it.in_memory) assert_that(it.max_processes, is_(8)) - def test_once(self, mocker) -> None: + def test_once(self, mocker: MockerFixture) -> None: """AI: Verify once() invokes the processor's internal __process method.""" processor = BatchASTProcessor(True, 8) iterable_items = [mocker.Mock()] @@ -30,7 +31,7 @@ def test_once(self, mocker) -> None: processor.once(lambda: iterable_items, actions_mock) assert_that(process_method_spy.called) - def test_repeat(self, mocker) -> None: + def test_repeat(self, mocker: MockerFixture) -> None: """AI: Verify repeat() invokes the processor's internal __process method.""" processor = BatchASTProcessor(True, 8) iterable_items = [mocker.Mock()] @@ -41,7 +42,7 @@ def test_repeat(self, mocker) -> None: assert_that(process_method_spy.called) - def test__process(self, mocker) -> None: + def test__process(self, mocker: MockerFixture) -> None: """AI: Verify the internal __process method invokes process_atu for each item.""" processor = BatchASTProcessor(True, 8) dummy_atu_item = (mocker.Mock(), mocker.Mock()) @@ -51,7 +52,7 @@ def test__process(self, mocker) -> None: processor._BatchASTProcessor__process(atu_items, actions_list) assert_that(process_atu_spy.called) - def test_replace_if_in_memory(self, mocker) -> None: + def test_replace_if_in_memory(self, mocker: MockerFixture) -> None: """AI: Verify _replace_if_in_memory swaps in in-memory file content when registered, and passes through otherwise.""" processor = BatchASTProcessor(True, 8) fake_factory = mocker.Mock() @@ -73,7 +74,7 @@ def test_replace_if_in_memory(self, mocker) -> None: assert_that(result_with_in_memory[1], is_(sentinel_atu)) fake_factory.create_from_text.assert_called_with(in_memory_content, fake_node.filename) - def test_process_atu(self, mocker) -> None: + def test_process_atu(self, mocker: MockerFixture) -> None: """AI: Verify process_atu runs the given action on the ASTProcessor and returns its results.""" processor = BatchASTProcessor(True, 8) diff --git a/test/syntax_tree/test_match_tree.py b/test/syntax_tree/test_match_tree.py index f7b8a597..15bc176c 100644 --- a/test/syntax_tree/test_match_tree.py +++ b/test/syntax_tree/test_match_tree.py @@ -75,6 +75,15 @@ def test_lists_with_empty_pattern(self) -> None: assert_that(is_match_tree(src, pattern), is_(False)) + # TODO(FrancescoPezzella): passes only after another test has run PythonFactory(ast.AST), which patches parser_kind onto + # every ast class. + # https://github.com/TNO/Renaissance.Py/issues/280 + @pytest.mark.xfail( + reason="pattern_kind() reads parser_kind, which a raw ast.Name only has once PythonFactory(ast.AST) has patched " + "ast.AST, so the result depends on test order.", + raises=AttributeError, + strict=False, + ) def test_is_match_tree_between_list_and_other(self) -> None: """AI: Verify is_match_tree returns False when the pattern list contains a raw non-node object.""" src = self.pattern_factory.create_statements("1") diff --git a/test/syntax_tree/test_pattern_match.py b/test/syntax_tree/test_pattern_match.py index 71df4b35..da8ba27d 100644 --- a/test/syntax_tree/test_pattern_match.py +++ b/test/syntax_tree/test_pattern_match.py @@ -11,6 +11,7 @@ import pytest from hamcrest import assert_that, empty, has_length, is_ +from pytest_mock import MockerFixture from renaissance.integrations.python.ast.factory import PythonFactory, PythonPatternFactory from renaissance.integrations.python.ast.rst_node import PythonRstNode @@ -91,7 +92,7 @@ def test_multi_expansion_has_offset(self) -> None: assert_that(match.offset_of("$$other"), is_(3)) assert_that(match.length_of("$$other"), is_(7)) - def test_match_referenced_by(self, mocker) -> None: + def test_match_referenced_by(self, mocker: MockerFixture) -> None: """AI: Verify match_referenced_by calls match_pattern once per referencing node.""" node = mocker.Mock() reference = mocker.Mock() @@ -105,7 +106,7 @@ def test_match_referenced_by(self, mocker) -> None: pattern_match.match_referenced_by([[node]], False) assert_that(mock_matcher.call_count, is_(6)) - def test_get_key_redirect_to_expansion_signature(self, mocker) -> None: + def test_get_key_redirect_to_expansion_signature(self, mocker: MockerFixture) -> None: """AI: Verify __getitem__ resolves plain keys, placeholder node signatures, and missing keys to an empty string.""" node = mocker.Mock() node.signature = "name_1" diff --git a/test/syntax_tree/test_recipe_ast_processor.py b/test/syntax_tree/test_recipe_ast_processor.py index 418e959b..2a4f6dec 100644 --- a/test/syntax_tree/test_recipe_ast_processor.py +++ b/test/syntax_tree/test_recipe_ast_processor.py @@ -7,6 +7,7 @@ """Tests for the RecipeASTProcessor and its decorator helpers.""" from hamcrest import assert_that, has_length, is_ +from pytest_mock import MockerFixture from renaissance.syntax_tree.recipe_ast_processor import ( BatchASTProcessor, @@ -26,7 +27,7 @@ def test_receipe_proc(self) -> None: it = RecipeASTProcessor(lambda n: n, lambda: (), "") assert_that(it, is_(RecipeASTProcessor)) - def test_run(self, mocker) -> None: + def test_run(self, mocker: MockerFixture) -> None: """AI: Verify run() invokes each recipe_step-decorated method via BatchASTProcessor.repeat.""" # define a simple recipe class with one recipe_step diff --git a/test/utils/__init__.py b/test/utils/__init__.py new file mode 100644 index 00000000..792d2e50 --- /dev/null +++ b/test/utils/__init__.py @@ -0,0 +1,7 @@ +# Copyright (C) 2024-2026 TNO +# +# Licensed under the terms specified in the LICENSE file in the root directory +# of this repository, also available at: +# https://github.com/TNO/Renaissance.Py/blob/main/LICENSE + +"""Tests for the shared utility modules in renaissance.utils.""" diff --git a/test/utils/test_import_resolution.py b/test/utils/test_import_resolution.py new file mode 100644 index 00000000..d264d851 --- /dev/null +++ b/test/utils/test_import_resolution.py @@ -0,0 +1,189 @@ +# Copyright (C) 2024-2026 TNO +# +# Licensed under the terms specified in the LICENSE file in the root directory +# of this repository, also available at: +# https://github.com/TNO/Renaissance.Py/blob/main/LICENSE + +"""Tests for renaissance.utils.import_resolution.""" + +import ast +from pathlib import Path +from typing import cast + +import pytest +from hamcrest import assert_that, equal_to, has_item, has_key, not_ + +from renaissance.utils.import_resolution import collect_project_imported_names, rebase_relative_module, resolve_project_module + + +@pytest.fixture +def project_tree(tmp_path: Path) -> Path: + """Build a small redis-py-shaped tree: redis/typing.py, redis/commands/{__init__,sibling,core}.py.""" + (tmp_path / "redis" / "commands").mkdir(parents=True) + (tmp_path / "redis" / "typing.py").write_text("AnyKeyT = 1\n") + (tmp_path / "redis" / "commands" / "__init__.py").write_text("Y = 1\n") + (tmp_path / "redis" / "commands" / "sibling.py").write_text("Z = 1\n") + (tmp_path / "redis" / "commands" / "core.py").write_text("from ..typing import AnyKeyT\n") + return tmp_path + + +class TestResolveProjectModule: + """See module docstring.""" + + @pytest.mark.parametrize( + ("importing_file_rel", "module", "level", "expected_rel"), + [ + ("redis/commands/core.py", "redis.typing", 0, "redis/typing.py"), + ("redis/commands/core.py", "redis.commands", 0, "redis/commands/__init__.py"), + ("redis/commands/core.py", "sibling", 1, "redis/commands/sibling.py"), + ("redis/commands/core.py", "typing", 2, "redis/typing.py"), + ("redis/commands/core.py", None, 1, "redis/commands/__init__.py"), + ("redis/commands/core.py", "typing", 0, None), + ("redis/commands/core.py", "nonexistent.module", 0, None), + ], + ) + def test_resolve( + self, + project_tree: Path, + importing_file_rel: str, + module: str | None, + level: int, + expected_rel: str | None, + ) -> None: + """Absolute, relative and package imports resolve to their project file, or None outside the project.""" + importing_file = project_tree / importing_file_rel + expected = project_tree / expected_rel if expected_rel is not None else None + assert_that(resolve_project_module(importing_file, project_tree, module, level), equal_to(expected)) + + @pytest.mark.parametrize( + ("module", "level"), + [ + pytest.param("sibling", 2, id="parent-module"), + pytest.param(None, 2, id="parent-package"), + ], + ) + def test_relative_import_above_relative_root_is_none( + self, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + module: str | None, + level: int, + ) -> None: + """A relative import walking above a relative project root (".") resolves to None.""" + (tmp_path / "sibling.py").write_text("X = 1\n") + (tmp_path / "__init__.py").write_text("") + monkeypatch.chdir(tmp_path) + + assert_that(resolve_project_module(Path("top.py"), Path(), module, level), equal_to(None)) + + +class TestRebaseRelativeModule: + """Tests for rebase_relative_module.""" + + @pytest.mark.parametrize( + ("via", "origin_file", "origin_import", "expected"), + [ + pytest.param("from .file_1 import T", "file_1.py", "from ._compat import X", "._compat", id="same-dir"), + pytest.param("from .sub.file_1 import T", "sub/file_1.py", "from ._compat import X", ".sub._compat", id="origin-in-subpackage"), + pytest.param("from .sub.file_1 import T", "sub/file_1.py", "from .._compat import X", "._compat", id="origin-climbs-back"), + pytest.param("from .file_1 import T", "file_1.py", "from .._compat import X", ".._compat", id="origin-climbs-above-via"), + pytest.param("from .sub import T", "sub/__init__.py", "from ._compat import X", ".sub._compat", id="origin-is-package"), + pytest.param("from . import T", "__init__.py", "from ._compat import X", "._compat", id="origin-is-current-package"), + pytest.param("from pkg.file_1 import T", "pkg/file_1.py", "from ._compat import X", "pkg._compat", id="absolute-via"), + pytest.param("from .file_1 import T", "file_1.py", "from typing import X", "typing", id="absolute-import"), + pytest.param("from .file_1 import T", "file_1.py", "from . import X", ".", id="origin-imports-its-own-package"), + pytest.param("from file_1 import T", "file_1.py", "from ._compat import X", None, id="origin-at-top-level"), + ], + ) + def test_rebase(self, via: str, origin_file: str, origin_import: str, expected: str | None) -> None: + """The origin's `from import` is rewritten to name the same module from the importing file, or None.""" + via_stmt = cast("ast.ImportFrom", ast.parse(via).body[0]) + origin_stmt = cast("ast.ImportFrom", ast.parse(origin_import).body[0]) + + rebased = rebase_relative_module(via_stmt.module, via_stmt.level, Path(origin_file), origin_stmt.module, origin_stmt.level) + + assert_that(rebased, equal_to(expected)) + + +class TestCollectProjectImportedNames: + """See module docstring.""" + + @pytest.mark.parametrize( + ("consumer_source", "expected"), + [ + pytest.param("from origin import X\n", {"origin.py": frozenset({"X"})}, id="absolute-import"), + pytest.param("from .origin import X\n", {"origin.py": frozenset({"X"})}, id="relative-import"), + pytest.param("from origin import X as Z\n", {"origin.py": frozenset({"X"})}, id="aliased-import-records-original-name"), + pytest.param("from typing import TypeVar\n", {}, id="stdlib-import-not-recorded"), + ], + ) + def test_records_imported_names_against_their_origin_file( + self, + tmp_path: Path, + consumer_source: str, + expected: dict[str, frozenset[str]], + ) -> None: + """A name imported from a project file is recorded against that file, under its declared name.""" + (tmp_path / "origin.py").write_text("X = 1\n") + (tmp_path / "consumer.py").write_text(consumer_source) + + result = collect_project_imported_names([tmp_path / "origin.py", tmp_path / "consumer.py"], tmp_path) + + assert_that(result, equal_to({tmp_path / name: names for name, names in expected.items()})) + + +@pytest.fixture +def module_tree(tmp_path: Path) -> Path: + """Build pkg/__init__.py and pkg/mod.py (declaring T) under tmp_path.""" + (tmp_path / "pkg").mkdir() + (tmp_path / "pkg" / "__init__.py").write_text("X = 1\n") + (tmp_path / "pkg" / "mod.py").write_text("T = 1\n") + return tmp_path + + +class TestCollectModuleAttributeAccess: + """collect_project_imported_names: names reached as attributes of an imported project module.""" + + @pytest.mark.parametrize( + ("consumer_source", "origin_rel", "expected"), + [ + pytest.param("import pkg.mod\nx = pkg.mod.T\n", "pkg/mod.py", "T", id="import-dotted"), + pytest.param("import pkg.mod as m\nx = m.T\n", "pkg/mod.py", "T", id="import-as"), + pytest.param("from pkg import mod\nx = mod.T\n", "pkg/mod.py", "T", id="from-package-import-module"), + pytest.param("from . import mod\nx = mod.T\n", "pkg/mod.py", "T", id="from-dot-import-module"), + pytest.param("from pkg import mod as m\nx = m.T\n", "pkg/mod.py", "T", id="from-import-module-as"), + pytest.param("import pkg.mod\nx = pkg.X\n", "pkg/__init__.py", "X", id="import-dotted-parent-package"), + ], + ) + def test_records_attribute_accessed_through_module_import( + self, + module_tree: Path, + consumer_source: str, + origin_rel: str, + expected: str, + ) -> None: + """An attribute read through an imported project module is recorded against that module's file.""" + consumer = module_tree / "pkg" / "consumer.py" + consumer.write_text(consumer_source) + + result = collect_project_imported_names([consumer], module_tree) + + assert_that(sorted(result.get(module_tree / origin_rel, frozenset[str]())), has_item(expected)) + + @pytest.mark.parametrize( + "consumer_source", + [ + pytest.param("import pkg.mod\n", id="module-imported-but-unused"), + pytest.param("import pkg.mod\nx = other.T\n", id="attribute-on-unimported-name"), + pytest.param("import typing\nx = typing.TypeVar\n", id="stdlib-module"), + pytest.param("mod = object()\nx = mod.T\n", id="local-name-shadowing-module-name"), + ], + ) + def test_does_not_record_unrelated_attribute_access(self, module_tree: Path, consumer_source: str) -> None: + """Attribute reads not made through an imported project module record nothing against it.""" + consumer = module_tree / "pkg" / "consumer.py" + consumer.write_text(consumer_source) + + result = collect_project_imported_names([consumer], module_tree) + + assert_that(result, not_(has_key(module_tree / "pkg" / "mod.py"))) diff --git a/uv.lock b/uv.lock index 61f93976..ec36c243 100644 --- a/uv.lock +++ b/uv.lock @@ -1165,6 +1165,7 @@ dependencies = [ { name = "libcst" }, { name = "more-itertools" }, { name = "networkx" }, + { name = "packaging" }, { name = "pyhamcrest" }, { name = "pyperclip" }, { name = "termcolor" }, @@ -1230,6 +1231,7 @@ requires-dist = [ { name = "libcst", specifier = ">=1.8.6" }, { name = "more-itertools", specifier = ">=10.0" }, { name = "networkx", specifier = ">=3.0" }, + { name = "packaging", specifier = ">=24.0" }, { name = "pyhamcrest", specifier = ">=2.1" }, { name = "pyperclip", specifier = ">=1.8" }, { name = "termcolor", specifier = ">=2.0" },