diff --git a/.gitignore b/.gitignore index f4ec0c21..aaaf3255 100644 --- a/.gitignore +++ b/.gitignore @@ -228,3 +228,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..7dc7ab3e 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,8 @@ 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. +19. **Common parser problems** (new page, e.g., `docs/developer/modules/common-parser-problems.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 (the Python `ast` limitations are already covered by `python-ast-known-limitations.md`). -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. diff --git a/docs/developer/feature-test-map/core.md b/docs/developer/feature-test-map/core.md index 90fd6062..4ac69fa4 100644 --- a/docs/developer/feature-test-map/core.md +++ b/docs/developer/feature-test-map/core.md @@ -3,7 +3,16 @@ ## 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 @@ -11,5 +20,32 @@ - **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. TypeVar modernization + +- **Feature:** [TypeVar modernization](../../user/features/typevar-modernization.md) +- **Concepts:** [Python version gates](../../user/concepts/python-version-gates.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/recipes/conftest.py` + - `test/utils/test_unparse_utils.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/unparse_utils.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 30ec915f..2bbda380 100644 --- a/docs/developer/modules/index.md +++ b/docs/developer/modules/index.md @@ -7,3 +7,7 @@ 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. [Python AST known limitations](python-ast-known-limitations.md) +11. [Rejected recipes](rejected-recipes.md) diff --git a/docs/developer/modules/python-ast-known-limitations.md b/docs/developer/modules/python-ast-known-limitations.md new file mode 100644 index 00000000..e69cefbc --- /dev/null +++ b/docs/developer/modules/python-ast-known-limitations.md @@ -0,0 +1,71 @@ +# Python AST known limitations + +{ #codemod-python-ast-known-limitations } + +**Stable ID:** `CODEMOD-PYTHON_AST_KNOWN_LIMITATIONS` + +Concrete limitations found in the Python AST/RST layer (`renaissance.integrations.python.ast`) and the rewrite mechanism it +feeds (`renaissance.syntax_tree.ast_rewriter`, `renaissance.utils.text_utils`) while building recipes +(`TypeVarCheck`). A recipe still has to work around both items below. The string-literal part of +item 1 is also tracked by an `xfail` test (see below). Item 2 is also tracked by a `# TODO` at the `return` in `ast_rewriter.py`, by `xfail` +scenarios in `features/steps/test_rewrite_semantics.py` and `test/syntax_tree/test_ast_rewriter.py`, and by tests +skipped with `TODO: fix impl.` in the latter; this page records why the obvious fix does not work. + +## 1. `ast.unparse()`/`shift_right` lose comments and indentation + +`TextUtils.shift_right`/`shift_left` (`renaissance/utils/text_utils.py`) are pure text operations with no notion of +Python syntax - they shift every line in a range unconditionally, blind to whether a line sits inside a string +literal. `ast.unparse()` already reproduces a docstring's continuation lines verbatim (it's the only multi-line +string constant it emits as a real multi-line literal), so a whole-function/class/module replacement built from +it shifts those already-correctly-indented lines a second time. Separately, regenerating a function's entire body +from the AST also reformats it to `ast.unparse()`'s own style regardless of the original formatting, and - +permanently, since Python's `ast` module never records comments at all - **deletes every comment inside the +body**; there is nothing for `ast.unparse()` to reproduce, and no future fix to this framework can change that +without Python itself changing. Both are real for any recipe that regenerates a whole node's source via +`ast.unparse()` and replaces the original text with it wholesale. + +**`TypeVarCheck` avoids this, it doesn't fix it** - see [Refactoring recipes](../../developer/modules/recipes.md) +for how `unparse_signature_only` splices only the new `[T]`/`[**P]`/`[*Ts]` bracket into the function's original +text instead of regenerating anything via `ast.unparse()`. + +The avoidance is not complete. `unparse_signature_only` still re-indents the function's lines (`_renormalize_indent`), +taking the *smallest* indentation among the body's non-blank lines as the body's own indentation, and that is +blind to string literals too. A multi-line string literal in the body with a continuation line indented less than +the body (for example at column 0) lowers that minimum: the function is re-indented from the wrong baseline and +the literal's contents change silently (`'first\nsecond'` becomes `'first\n second'`). This reproduces with +`TypeVarCheck.convert_declared_typevars` on both a module-level function and a method; a literal whose +continuation lines are indented like the body is not affected. Tracked by the `xfail` test +`test_converts_function_preserving_multiline_string_literal` in `test/recipes/test_type_var_check_convert.py`. + +A future recipe that genuinely needs to regenerate a whole body from the AST - not just a signature - still hits +both issues above and has to work around them itself; neither `ast.unparse()`'s comment blindness nor +`shift_right`/`shift_left`'s string-literal blindness was touched here. + +## 2. `__is_ancestor_in_nodes` can't just drop its `and False` + +`_RewriteActions.__is_ancestor_in_nodes` (`renaissance/syntax_tree/ast_rewriter.py`) is meant to detect when a +queued rewrite is nested inside another queued rewrite's node, so `apply()` can skip the redundant nested one and +let the outer (ancestor) rewrite silently dominate it - but it ends with `return result and False`, which is +always `False` regardless of `result`. The dominance/suppression check never fires: an ancestor replacement and a +nested descendant edit queued in the same batch both get applied instead of the descendant being suppressed. The +one-line in-code `# TODO` at that `return` doesn't capture why this isn't a one-line fix, so it's spelled out here +instead. + +**Why the obvious one-line fix doesn't work:** simply changing `return result and False` to `return result` +does not enable the suppression correctly. `no_conflict(node, rew)` returns `True` for `node is rew` (a node +trivially "overlaps" itself), and `rewrite_nodes` is built by flattening every rewrite in `self.rewrites` - the +same collection `apply()` draws `n` from when it calls `__is_ancestor_in_nodes(n)`. So `result` is a near-total +tautology: `True` for almost any node, since it always includes a self-comparison. Dropping `and False` would +make `__is_ancestor_in_nodes` return `True` for nearly every queued node - including nodes that have no real +ancestor/descendant relationship to anything else - so `apply()`'s `continue` would skip most rewrites, not +just the dominated ones, breaking the majority of currently-passing scenarios rather than fixing the dominance +scenarios that are marked `xfail` (for example `test_dominated_change_not_applied` in +`features/steps/test_rewrite_semantics.py`). A real fix needs to exclude a node's own rewrite from the comparison set and use a genuine +ancestor/descendant check - e.g. reusing `__is_nested` (already used by `__check_for_conflicting_rewrites`, the +sibling check that turns a *different* kind of overlapping-rewrite bug into a clear `ValueError` instead of +corrupting output) - instead of repairing `no_conflict`'s offset-overlap test. + +`TypeVarCheck` avoids triggering this gap by construction: `functions_using_nodes` attributes a name to the +outermost function using it, so a nested closure never gets an edit of its own alongside its parent's. See +[Refactoring recipes](recipes.md), which also describes how `convert_declared_typevars` queues exactly one edit per +function, never a second rewrite on the same node. diff --git a/docs/developer/modules/recipes.md b/docs/developer/modules/recipes.md new file mode 100644 index 00000000..f0673c23 --- /dev/null +++ b/docs/developer/modules/recipes.md @@ -0,0 +1,189 @@ +# Refactoring recipes + +{ #codemod-recipes } + +**Stable ID:** `CODEMOD-RECIPES` + +## Responsibility + +Recipes are `PythonRefactoring` subclasses that inspect and rewrite one Python source file at a time, targeting +gaps that `ruff` either does not detect, only offers as a separate unsafe fix, or never finishes cleaning up. This +page covers `TypeVarCheck`, the recipe built for +[TypeVar modernization](../../user/features/typevar-modernization.md). Recipes that were built and later +removed are listed in [Rejected recipes](rejected-recipes.md). + +## Location + +- `src/renaissance/recipes/type_var_check.py` - the `TypeVarCheck` pipeline itself (orchestration only). +- `src/renaissance/recipes/type_var_domain.py` - TypeVar/ParamSpec/TypeVarTuple domain model and safety + analysis. +- `src/renaissance/recipes/step_runner.py` - `Step`/`run_steps`, the generic "run these independent fix actions + in order, committing each one's owning recipe only if it fixed something" primitive that `TypeVarCheck.check()` + and the CLI use. +- Base class: `src/renaissance/recipes/python_refactoring.py` - also owns two generic, cross-recipe helpers: + `find_rst_node` (used by `TypeVarCheck`'s conversion) and the module-level `narrowed_import_text` (used by `TypeVarCheck`'s + import localization). +- Shared utilities: `src/renaissance/utils/unparse_utils.py` (splices the PEP 695 type-parameter bracket into a + function's original source text), + `src/renaissance/utils/import_resolution.py` (resolves `from X import Y` project-wide to the file it + imports from - not TypeVar-specific, kept out of `type_var_domain.py` on purpose). + +## Public entry points + +- `TypeVarCheck.run()` / `TypeVarCheck.check()` — localizes cross-file type parameter imports, converts every + declared type parameter to PEP 695 syntax (whether one function uses it or several), then removes any declaration left orphaned + by outside means (e.g. a signature converted by hand or by `ruff`'s own `UP047` fix beforehand); commits changes + to disk between phases (via `renaissance.recipes.step_runner.run_steps`, see below). +- `TypeVarCheck.localize_imported_typevars()`, `TypeVarCheck.convert_declared_typevars()`, and + `TypeVarCheck.remove_orphaned_declarations()` — the three phases individually, each returning + `{name: "fixed" | "unsafe"}`. +- Dispatched by name via `PythonRefactoring.process(class_name, file)`, which resolves `"TypeVarCheck"` to + `renaissance.recipes.type_var_check` using `snake_case()`. Only the `refactor` subcommand of + `src/rejuvenation/cli.py` uses this path, and it never sets `min_python`, `project_root` or + `project_wide_imported_names`, so the version-gated rewrites are always reported `"unsafe"` there. The supported + entry point is `migration-type-recipes.py`, which builds the recipes directly. +- `step_runner.run_steps(steps)` - `TypeVarCheck.check()` calls this internally with its own three phases; + `migration-type-recipes.py` calls it once per file with the same three phases. See the CLI's own docs. + +## Internal structure + +The recipe operates on the plain `ast` module directly (`ast.walk`, `ast.iter_child_nodes`, `ast.unparse`) rather +than Renaissance's RstNode-tree traversal, because the cross-file phase already has to parse a second file from +disk with `ast.parse()`. Shared domain helpers (`find_type_param_declarations`, `type_param_constructor_name`, +plus the safety-analysis functions `is_safe_to_convert`/`is_safe_to_localize`) live in `type_var_domain.py`, +kept out of `type_var_check.py` so domain modelling doesn't mix with pipeline orchestration. + +`is_safe_to_convert`/`is_safe_to_localize` return `UnsafeReason | None` (`None` meaning safe), not a bare +`bool` - each of the seven `UnsafeReason` members (the Python-version gate plus the six `__all__`/scope/ +cross-project conditions across both functions) has a matching `UnsafeRule` (a short message plus a docs anchor +slug) in `UNSAFE_RULES`, and `doc_link(reason)` resolves one to the full URL under +[TypeVar modernization](../../user/features/typevar-modernization.md)'s Constraints section. `is_safe_to_convert` +additionally takes `project_wide_imported_names` (a `frozenset[str]`, defaulting to empty) - set on +`TypeVarCheck.project_wide_imported_names` by the CLI, via `renaissance.utils.import_resolution. +collect_project_imported_names` over every file it was given, before either `TypeVarCheck` phase that can +remove a declaration runs. `TypeVarCheck` records the reason behind each `"unsafe"` name on one instance +attribute per phase (see its own docs), and `migration-type-recipes.py`'s +report prints `UNSAFE_RULES[reason].message` and `doc_link(reason)` +next to each one - this is what makes a specific "unsafe" occurrence traceable to the exact documented rule that +caused it, rather than a generic status string. `TypeVarCheck.project_root` (a `Path | None`, falling back to the +file's own directory) is the directory absolute imports resolve from; the CLI sets it to the target directory, or +to the parent folder when given a single file. + +`self.body` (top-level statements only) is not enough to rewrite a method nested in a class; `convert_declared_typevars` +locates the owning `PythonRstNode` for a nested function via `self.find_rst_node(function)` - a generic +`PythonRefactoring` base-class method (matching by node identity against the raw `ast.FunctionDef`/ +`ast.AsyncFunctionDef` node), available to any future recipe needing the same lookup, not just this one. It skips +a function that already declares a matching PEP 695 `type_param` (rather than adding a duplicate) - the same +check that lets phase 2 absorb the "signature already converted, declaration left behind" case directly, without +needing phase 3 for it. + +`convert_declared_typevars` calls `unparse_signature_only(function, original_text)` (from +`renaissance.utils.unparse_utils`) rather than `self.replace(unparse_node(function), ...)`: it splices only the +new `[T]`/`[**P]`/`[*Ts]` bracket into `function`'s *original* source text, right after its name, and leaves +everything else - parameter list, defaults, line breaks, return type, docstring, body, comments - untouched +(indentation aside: the header's continuation lines and the body are re-indented relative to `def`, see +`_renormalize_indent`), rather than regenerating anything from the AST, which used to reformat whatever it touched +(including collapsing a multi-line parameter list onto one line) and, since Python's `ast` module never records +comments at all, silently delete any comments inside the body. See +[Python AST known limitations](python-ast-known-limitations.md) item 1 for the full mechanism. It lives in a +shared utils module rather than in `type_var_check.py` itself, since any future recipe adding a type-params +bracket the same way needs it too. + +`convert_declared_typevars` collects the functions it touches and queues exactly one `self.replace()` per +function, even when several type parameters apply to it: queuing one per name would target the same node twice +before a commit, which the rewriter rejects as a conflicting rewrite. + +`functions_using_nodes` (`type_var_domain.py`) attributes a name's usage to the *outermost* function in a nesting +chain, never a nested closure that merely references it - a PEP 695 type parameter declared on an enclosing +function is already visible inside its nested closures the same way any other name in an enclosing scope is, so a +nested closure must never be treated as an independent user needing its own (shadowing) type parameter. Getting +this wrong used to queue a redundant edit for the nested closure alongside the outer function's edit - which, +combined with the rewrite dominance/suppression gap in +[Python AST known limitations](python-ast-known-limitations.md) item 2, corrupted the output outright. Confirmed +live against `starlette/starlette/authentication.py`'s `requires()` and its nested `*_wrapper` closures. + +The recipe doesn't remove a now-unused import itself (e.g. `from typing import TypeVar` once nothing calls it) - +that used to be hand-rolled (`TypeVarCheck._remove_unused_constructor_imports`), duplicating exactly what +`ruff`'s `F401` rule already detects generically. `migration-type-recipes.py` now runs +`ruff check --fix --select F401` over every file it modified, once, after the recipe has finished, unless +`--no-ruff` is passed - see its own docs. `ruff` runs on the whole of +each modified file, so it removes every unused import there, not only the ones these recipes made unused. +`_localize_import` is a separate, still-hand-rolled concern that survives this: narrowing an import because a +name moved from *imported* to *locally declared* isn't "is this unused," so it isn't something `ruff` can do - +it still uses `narrowed_import_text` directly. + +`remove_orphaned_declarations` detects a dead declaration without counting references: `all_refs_shadowed_by_pep695` +(in `type_var_domain.py`) walks the tree tracking whether the current position is "shadowed" (inside a function +whose `type_params` already declares the same name) and only reports a live use for a `Name` node reached while +*not* shadowed. This is what lets it recognize the state `ruff`'s `UP047` leaves behind — a signature already +rewritten to `def f[T](...)`, with the old `T = TypeVar("T")` still sitting in the module, which `ruff` documents +it will never remove itself. + +Before rewriting anything, `convert_declared_typevars` calls `TypeVarCheck._target_supports_pep695()`, which +compares the recipe's `min_python` class attribute against `PEP_695_MINIMUM = (3, 12)`; `None` (unknown) never +passes. The tool doesn't detect the target's version: `migration-type-recipes.py` sets `min_python` from its +required `--py` flag, and tests set it after construction - the same pattern `in_memory` already uses on the base +class. `remove_orphaned_declarations` has no such check: removing a declaration that is already dead adds no +syntax. + +## Related features + +- [TypeVar modernization](../../user/features/typevar-modernization.md) + +## Related concepts + +- [Python version gates](../../user/concepts/python-version-gates.md) + +## Validated by test modules + +- `test/recipes/test_type_var_check.py` - the end-to-end `run()`/`check()` path and the Python-version gate. +- `test/recipes/test_type_var_check_localize.py` +- `test/recipes/test_type_var_check_convert.py` +- `test/recipes/test_type_var_check_orphaned.py` +- `test/recipes/test_type_var_check_properties.py` - Hypothesis/hypothesmith crash-safety fuzzing of `check()` + against arbitrary generated source (see [ADR 09](../architecture/adr/09_property_based_tests.md)). +- `test/recipes/test_type_var_domain.py` - `is_safe_to_convert`/`is_safe_to_localize` in isolation, confirming + that five of the `UnsafeReason` members (the `__all__`, outside-use, origin-export, generic-base and + conditional-constructor conditions) are returned by their specific unsafe condition. The version gate and + `IMPORTED_ELSEWHERE_IN_PROJECT` are covered through the recipe and the CLI instead. +- `test/recipes/test_step_runner.py` - `Step`/`run_steps`: commit only when a step fixed something, results in step + order. +- `test/recipes/test_python_refactoring.py` - `PythonRefactoring.find_rst_node` and `narrowed_import_text`. +- `test/recipes/conftest.py` - shared fixtures (`make_recipe`, `create_type_var_check`) used by the TypeVar test + files above. +- `test/rejuvenation/test_migration_type_recipes.py` - the CLI: `--py` gating, the report and its documentation + links, the `ruff` `F401` pass and exit codes. +- `test/utils/test_unparse_utils.py` - the bracket-splice mechanism itself (`unparse_signature_only` and its + helpers), independent of the recipe. +- `test/utils/test_import_resolution.py` - `resolve_project_module`/`collect_project_imported_names` in + isolation (absolute/relative import resolution, package `__init__.py` fallback, names read as attributes of + an imported project module, stdlib imports correctly excluded). + +## Extension points + +- A new recipe is added as a new `PythonRefactoring` subclass in its own `snake_case`-named module under + `src/renaissance/recipes/`; the CLI dispatch requires no separate registration. +- `build_type_param` (in `type_var_domain.py`) is the place to extend if a future PEP adds a new kind of + type-parameter declaration. +- `PythonRefactoring.find_rst_node` and `renaissance.utils.unparse_utils.unparse_signature_only` are available to + any new recipe that needs the same lookups - a future recipe doing signature-only `ast.unparse()` replacement + doesn't need to reimplement it. +- `step_runner.Step`/`run_steps` are available to any new recipe (or CLI) that needs to sequence more than one + independently-committable fix action. + +## Non-goals + +- `resolve_project_module` doesn't follow re-exports through an intermediate `__init__.py` or handle namespace + packages (PEP 420); such imports are skipped by both the localization phase and the removal-safety check. +- A `from pkg.mod import *` is not expanded, so a name it pulls in is not detected by the removal-safety check. +- The names other files import from a module are computed once, before the first file is processed, so an origin + whose importers all get localized in the same run is only converted by a second run. +- `localize_imported_typevars` queues one rewrite per localized name, so a single import statement that brings in + two or more localizable names (`from .origin import T, U`) makes the commit fail with a conflicting-rewrite + error. The CLI reports that file under `ERRORS` and leaves it unchanged; one import per name avoids it. + Tracked by the `xfail` test `test_localizes_two_names_from_one_import_statement` in + `test/recipes/test_type_var_check_localize.py`. +- The recipe doesn't detect the target's minimum Python version (e.g. from `requires-python`); it has to be given + explicitly via `--py`. +- Rewriting legacy `Unpack[Ts]` usages to native `*Ts` syntax is left to `ruff`'s `UP044` rule - see + [Rejected recipes](rejected-recipes.md). diff --git a/docs/developer/modules/rejected-recipes.md b/docs/developer/modules/rejected-recipes.md new file mode 100644 index 00000000..efdb3af5 --- /dev/null +++ b/docs/developer/modules/rejected-recipes.md @@ -0,0 +1,47 @@ +# Rejected recipes + +{ #codemod-rejected-recipes } + +**Stable ID:** `CODEMOD-REJECTED-RECIPES` + +## Responsibility + +Records recipes that were built and later removed because an existing tool already covers them, and what to use +instead. A recipe listed here should not be rebuilt without first checking that the reason for removing it no +longer holds. + +## TypeVarTupleCheck (`Unpack[T]` → `*T`) + +{ #codemod-rejected-recipes-typevartuplecheck } + +**What it did:** rewrote a legacy `Unpack[Ts]` usage to native `*Ts` syntax +([PEP 646](https://peps.python.org/pep-0646/), Python 3.11+), for a `TypeVarTuple` declared at module level in +the same file. + +**Why it was removed:** + +- `ruff`'s `UP044` rule (`non-pep646-unpack`) covers every case the recipe handled, and more: `typing.Unpack[...]`, + a `TypeVarTuple` imported from another module, and `Unpack[tuple[...]]`. It also leaves `Unpack` in place + where `*` is not valid syntax (a plain parameter, `**kwargs: Unpack[SomeTypedDict]`), whereas the recipe + rewrote `x: Unpack[Ts]` into a `SyntaxError`. +- The only thing the recipe added was not needing `--unsafe-fixes`. `ruff` marks the fix unsafe because + `Unpack[tuple[...]]` and `*tuple[...]` are different objects at runtime; for a `TypeVarTuple` both forms are + identical. +- Legacy `Unpack[...]` usage is rare in real projects, so the recipe added little value to the CLI. + +**What to use instead:** + +```shell +ruff check --select UP044 --unsafe-fixes --target-version py311 --fix +``` + +`--target-version` must be `py311` or higher. Review the diff for any `Unpack[tuple[...]]` rewrite before +keeping it. + +**Interaction with [TypeVar modernization](../../user/features/typevar-modernization.md):** `TypeVarCheck` still +converts a `TypeVarTuple`'s declaration, including one imported from another file in the target project, to +`def f[*Ts](...)`, and leaves its `Unpack[Ts]` usages as they are. Run `UP044` afterwards to rewrite those. + +## Related features + +- [TypeVar modernization](../../user/features/typevar-modernization.md) diff --git a/docs/glossary.md b/docs/glossary.md index e15c402c..432a85cf 100644 --- a/docs/glossary.md +++ b/docs/glossary.md @@ -28,6 +28,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, @@ -166,6 +170,12 @@ See [Observability](user/concepts/observability.md). An [insertion](#insertion) that adds text immediately before the start location of an AST node. See [Rewrite semantics](user/concepts/rewrite-semantics.md). +### Python version gate + +A check that lets a recipe apply a rewrite only when the target codebase's minimum supported Python version, as +given by the user, meets the threshold of the syntax the rewrite introduces. An unknown minimum never passes. +See [Python version gates](user/concepts/python-version-gates.md). + --- ## R diff --git a/docs/user/concepts/index.md b/docs/user/concepts/index.md index e9a9d6a7..4dba3eab 100644 --- a/docs/user/concepts/index.md +++ b/docs/user/concepts/index.md @@ -23,6 +23,10 @@ This section introduces the conceptual model of the repository. 1. [Transformation](transformation.md) 2. [Composition](composition.md) -## 5. Reusable libraries +## 5. Code modernization + +1. [Python version gates](python-version-gates.md) + +## 6. Reusable libraries 1. [Standard analyses and transformations](standard-libraries.md) diff --git a/docs/user/concepts/python-version-gates.md b/docs/user/concepts/python-version-gates.md new file mode 100644 index 00000000..503e0c31 --- /dev/null +++ b/docs/user/concepts/python-version-gates.md @@ -0,0 +1,60 @@ +# Python version gates + +{ #concept-python-version-gates } + +**Stable ID:** `CONCEPT-PYTHON-VERSION-GATES` + +## Purpose + +Explains the mechanism every version-gated recipe shares for deciding whether a rewrite is safe to apply: take +the target codebase's minimum supported Python version as given by the user, and only rewrite when that minimum +meets the specific syntax feature's own threshold - never a guess. + +## Scope + +Applies to any recipe whose rewrite introduces syntax that doesn't exist on every supported Python version. One +recipe uses this today: + +| Feature | PEP | Minimum Python | Recipe | +| --- | --- | --- | --- | +| Generic type-parameter syntax (`def f[T](...)`) | [PEP 695](https://peps.python.org/pep-0695/) | 3.12 | [TypeVar modernization](../features/typevar-modernization.md) (`TypeVarCheck`) | + +## Definition + +Each version-gated recipe has a `min_python` attribute (`tuple[int, int] | None`, default `None`) holding the +target codebase's minimum supported Python version. The tool never detects it: `migration-type-recipes.py` +requires it through its `--py MAJOR.MINOR` flag and sets it on every recipe it runs; tests set it directly. + +A gate (`TypeVarCheck._target_supports_pep695()`) returns `True` only if `min_python` is set *and* meets the +feature's own threshold (`PEP_695_MINIMUM = (3, 12)`). + +## Invariants / guarantees + +- **Conservative by design.** An unknown `min_python` (a recipe run without it being set) and a minimum below + the threshold both produce the same result: `False`. An unknown minimum is never treated as safe - the syntax + a gate protects is a hard `SyntaxError` on an older interpreter, so guessing wrong isn't a cosmetic mistake, + it's a codebase the recipe would break outright. For example, with `--py 3.11` the CLI reports the PEP 695 + conversion `"unsafe"`; with `--py 3.12` it converts. See + [TypeVar modernization](../features/typevar-modernization.md)'s Constraints section. +- The gate protects rewrites that *introduce* new syntax. Removing a declaration that is already dead, because + every use of it is shadowed by a PEP 695 type parameter, adds no syntax and does not depend on `min_python`. + +## Related features + +- [TypeVar modernization](../features/typevar-modernization.md) + +## Related tests + +- `test/recipes/test_type_var_check.py` (`test_pep695_gate_threshold`) +- `test/rejuvenation/test_migration_type_recipes.py` (`test_py_flag_gates_rewrites`) + +## Related code + +- `rejuvenation/migration-type-recipes.py` (the `--py` flag) +- `renaissance/recipes/type_var_check.py` (`min_python`, `_target_supports_pep695`, `PEP_695_MINIMUM`) + +## Notes + +The threshold a recipe picks is the *syntax feature's own* true minimum, not an arbitrary stricter value chosen +to match another recipe for consistency. A future version-gated recipe should find the PEP's real minimum and +gate there, rather than defaulting to whatever an existing recipe already uses. diff --git a/docs/user/features/index.md b/docs/user/features/index.md index 4f56fb72..32da0374 100644 --- a/docs/user/features/index.md +++ b/docs/user/features/index.md @@ -21,3 +21,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. [TypeVar modernization](typevar-modernization.md) diff --git a/docs/user/features/rewrite-semantics.md b/docs/user/features/rewrite-semantics.md index 92593a67..ae427978 100644 --- a/docs/user/features/rewrite-semantics.md +++ b/docs/user/features/rewrite-semantics.md @@ -18,7 +18,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/typevar-modernization.md b/docs/user/features/typevar-modernization.md new file mode 100644 index 00000000..3015f680 --- /dev/null +++ b/docs/user/features/typevar-modernization.md @@ -0,0 +1,244 @@ +# TypeVar modernization + +{ #feature-typevar-modernization } + +**Stable ID:** `FEATURE-TYPEVAR-MODERNIZATION` + +## User-facing summary + +Modernizes legacy `TypeVar`/`ParamSpec`/`TypeVarTuple` usage in a Python file end to end, in one command — +covering both what `ruff`'s `UP047` rule only offers as a separate, unsafe fix and a gap it doesn't detect or +clean up at all: + +1. **Cross-file import localization.** A type parameter imported from another module in the target project + (`from pkg.other_module import T`, `from .other_module import T`) is invisible to `ruff`'s `UP047` rule, which + only looks at declarations in the + same file. Where safe, the recipe rewrites the import into an equivalent local declaration. +2. **Conversion to PEP 695 syntax.** Every declared `TypeVar`/`ParamSpec`/`TypeVarTuple` is rewritten to + [PEP 695](https://peps.python.org/pep-0695/) generic syntax (`def f[T](...)`) across every function that uses + it — whether it's used by one function (the same rewrite `ruff` offers, but only via `--unsafe-fixes`) or + shared across several (`ruff` can't safely do this at all, since converting one function at a time never lets it + confirm every use site is covered). The now-redundant module-level declaration is removed as part of the same + pass. +3. **Orphaned declaration cleanup.** A defensive final pass for declarations left dead by outside means — e.g. a + signature already converted to PEP 695 syntax by hand, or by running `ruff` before this recipe. `ruff`'s + `UP047`, by its own documentation, never removes the module-level `T = TypeVar("T")` it makes redundant, in + any case. Once every remaining reference to a declared name is shadowed by a same-named PEP 695 type parameter + (or there's no reference left at all), the recipe removes the declaration. + +The recipe doesn't drop the import it just made redundant (`TypeVar`, ...) itself - that's `ruff`'s `F401` +rule's job, already solved there rather than duplicated; see API entry points below for where that cleanup +actually runs. + +A converted `TypeVarTuple` keeps its legacy `Unpack[Ts]` usages (`def f[*Ts](*args: Unpack[Ts])`), which is +valid as it is. To rewrite them to native `*Ts` syntax, run `ruff`'s `UP044` rule afterwards; see +[Rejected recipes](../../developer/modules/rejected-recipes.md) for why the tool doesn't do this itself. + +## Inputs + +A Python file or directory, and the target project's minimum supported Python version (`--py`). + +Supports `TypeVar` (including `bound=` and constraint forms), `ParamSpec`, and `TypeVarTuple` declarations. + +The cross-file phase resolves absolute and relative imports against the target directory passed to the CLI (the +file's own folder when a single file is passed). Imports that don't resolve to a file inside it (stdlib, +third-party, re-exports through an intermediate `__init__.py`, namespace packages) are silently out of scope, not +reported unsafe. + +## Outputs / effects + +- The file is rewritten in place for every change classified as safe. +- `TypeVarCheck` returns `{"cross_file": {...}, "converted": {...}, "orphaned": {...}}`, each mapping + `name -> "fixed" | "unsafe"`. +- The recipe doesn't remove the `from typing import ...` (or equivalent) name it makes redundant - see the + User-facing summary above and the CLI's own `ruff check --fix --select F401` pass in API entry points below. +- Alongside each phase's `"unsafe"` status, `TypeVarCheck` also records *why* on a matching instance attribute - + `cross_file_unsafe_reasons`, `converted_unsafe_reasons`, `orphaned_unsafe_reasons` - each mapping + `name -> UnsafeReason` (see Constraints below for the specific reasons). The CLI collects these into + `FileReport.reasons` and prints the matching documented rule and link next to each unsafe name - see API entry + points below. + +## Constraints + +Every case below is a distinct, permanent reason a candidate is reported `"unsafe"` and left untouched - each has +its own anchor so the CLI's report (printed on every run, and also saved to a file by `--report`) can link a +specific occurrence straight to the rule that explains it, rather than a generic "couldn't convert" message. + +### PEP 695 version gate + +{ #feature-typevar-modernization-pep695-version-gate } + +[PEP 695](https://peps.python.org/pep-0695/) generic syntax (`def f[T](...)`) did not exist before Python 3.12 +(released October 2023). If the minimum Python version passed with `--py` is below 3.12, every candidate is +reported `"unsafe"` by the conversion phase (phase 2) and left untouched. Cross-file localization (phase 1) and +the orphaned-declaration cleanup (phase 3) still run, since neither introduces PEP 695 syntax. + +**To fix this yourself:** if the project actually supports 3.12+, re-run with `--py 3.12` (or higher). If it +has to keep supporting older Pythons, there's no manual PEP 695 rewrite either, since the syntax doesn't exist +before 3.12. + +### A declared TypeVar is exported via `__all__` + +{ #feature-typevar-modernization-declared-typevar-exported } + +A module-level `T = TypeVar(...)` (or `ParamSpec`/`TypeVarTuple`) listed in its own file's `__all__` is public +API - removing its declaration to convert it to PEP 695 syntax would break any importer still doing +`from this_module import T`. Left unconverted, `"unsafe"`. + +**To convert this yourself:** you have to accept the same trade-off the tool won't make automatically - remove +`T` from `__all__` (usually a breaking change for anything still importing it), move it into a PEP 695 signature +at every function that uses it, and delete the old `T = TypeVar(...)` line once every use site is converted. If +`T` can't be dropped from `__all__`, the declaration has to stay as it is. + +### A declared TypeVar is used outside a function body + +{ #feature-typevar-modernization-used-outside-function } + +A module-level declaration referenced anywhere other than inside the function(s) being converted - for example +as a class's `Generic[T]` base, or in a module-level type alias - can't have its declaration removed: a PEP 695 +type parameter only exists inside the function signature it's declared on, so that other use site would be left +referencing a name that no longer exists. Left unconverted, `"unsafe"`. + +**To convert this yourself:** check every other reference first (a `Generic[T]` base, a module-level type alias, +and so on). If those other use sites can be rewritten or removed, the function signatures can then be converted +by hand and the module-level declaration deleted; otherwise it has to stay module-level. + +### An imported TypeVar's origin module exports it via `__all__` + +{ #feature-typevar-modernization-origin-module-exports-name } + +Cross-file localization (phase 1) turns `from other_module import T` into a local `T = TypeVar(...)` +declaration. If `other_module` lists `T` in its own `__all__`, it's advertised as that module's public API - +localizing the import would leave two independent declarations of the same logical type parameter (the +original, still-exported one, and the new local copy), which silently breaks identity-based uses (e.g. +`isinstance` checks or generic subclassing across the two copies). Left as an import, `"unsafe"`. + +**To fix this yourself:** localizing the import means also removing `T` from the *origin* module's `__all__` +(same public-API trade-off as the previous case, on the other file) - otherwise the two files end up with two +independent `T` objects, silently breaking anything relying on both referring to the same one. + +### An imported TypeVar is used in a `Generic[...]` base at its origin + +{ #feature-typevar-modernization-used-in-exported-generic-base } + +If the origin module uses the imported name as a class's `Generic[T]` base, that class's own generic identity is +tied to this specific `T` object - localizing the import would create a second, unrelated `T`, breaking +subclassing or type-checking that depends on the two modules sharing the same type parameter. Left as an +import, `"unsafe"`. Any class in the origin module counts, exported or not, but only a bare `Generic[...]` base +is detected: `typing.Generic[...]` and other generic bases such as `Protocol[T]` are not. + +**To fix this yourself:** the origin module's class is generic over this exact `T` object, so localizing the +import safely means converting that class - and anything downstream that depends on it - in the same +coordinated change, or the two modules end up with different, incompatible `T`s. + +### An imported TypeVar's origin module imports its constructor conditionally + +{ #feature-typevar-modernization-origin-imports-constructor-conditionally } + +A localized declaration is a copy of the origin's `T = TypeVar(...)` call, so it runs with whichever `TypeVar` +the *importing* file has in scope. If the origin imports `TypeVar` (or `ParamSpec`/`TypeVarTuple`) inside a +module-level `if` or `try` block - typically `typing` on newer Pythons and `typing_extensions` below, because +the declaration uses an argument such as `default=` that older `typing` versions reject - the copy may bind a +different implementation than the origin intended and fail at import time on some supported versions. Left as +an import, `"unsafe"`. + +**To fix this yourself:** give the importing file the same conditional import as the origin before copying the +declaration across, or leave the import as it is. + +### A declared TypeVar is imported directly by another file in the target project + +{ #feature-typevar-modernization-imported-elsewhere-in-project } + +A module without `__all__` is still Python-legal to import any of its top-level names from directly - +`__all__` only governs `from module import *`, never `from module import specific_name`. So a declaration with +no `__all__` isn't automatically "unused elsewhere": before converting or removing it, the CLI (see API entry +points below) scans every file it was given for `from this_module import this_name`-shaped imports (absolute +or relative, resolved to the actual file - see `renaissance.utils.import_resolution`), and for the name read as +an attribute of the imported module (`import pkg.this_module` then `pkg.this_module.this_name`, or +`from pkg import this_module` then `this_module.this_name`). Any hit is treated as `"unsafe"`, +`IMPORTED_ELSEWHERE_IN_PROJECT`, regardless of `__all__`. A `from this_module import *` is not expanded, so a +name it pulls in is not detected. Running the recipe on a single file in +isolation (not via the CLI, or via the CLI on a lone file with no other files passed) has nothing to check +against, so this constraint can only fire when the target is a directory scanned alongside the files that +import from it. + +**To convert this yourself:** the report only names the candidate, not the importing file - grep the project +for `from import ` (absolute or relative) and `.` to find it. Once found, either update that +importer in the same change to get `name` from wherever it ends up after conversion, or leave the module-level +declaration as it is if the importer can't be updated alongside it - the same public-API trade-off as the +`__all__` case above, just surfaced by a direct import instead of an explicit `__all__` entry. + +A declaration imported by other files in the target project is always kept at its origin during a run, even +when every importer gets localized in that same run. A second CLI run converts it, once no file imports it anymore. + +A single import statement that brings in two or more localizable type parameters (`from .origin import T, U`) +currently makes the localization phase fail for that file: it is reported under `ERRORS` and left unchanged, +because two rewrites are queued on the same statement. Splitting it into one import per name +(`from .origin import T` and `from .origin import U`) lets the next run localize both. + +A function whose body contains a multi-line string literal with a continuation line indented less than the body +(for example at column 0) is converted with the contents of that literal changed and the body re-indented; the +tool does not report it. Review modified files with `git diff`, or convert such functions by hand. See +[Python AST known limitations](../../developer/modules/python-ast-known-limitations.md). + +Removing a declaration or an unused import leaves its surrounding blank lines behind, so a modified file can +start with, or contain, extra blank lines. Run your formatter afterwards to tidy them up. + +## Related concepts + +- [Python version gates](../concepts/python-version-gates.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` - the step runner the CLI uses for the three phases +- `test/recipes/test_python_refactoring.py` - `find_rst_node` and `narrowed_import_text`, used by the recipe +- `test/recipes/conftest.py` - the fixtures that build the recipe in the tests above +- `test/utils/test_unparse_utils.py` - the bracket splice that adds the PEP 695 type parameters to a signature +- `test/utils/test_import_resolution.py` - the project-wide import resolution the CLI uses for the + `IMPORTED_ELSEWHERE_IN_PROJECT` constraint above +- `test/rejuvenation/test_migration_type_recipes.py` (the CLI wrapper above) + +## 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` +- `src/renaissance/utils/import_resolution.py` and `unparse_utils.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`/`venv` excluded). +- `--py` (required): the minimum Python version the target project supports, not the one running the tool. + PEP 695 rewrites need 3.12+. +- `--report`: also write the report to a file. The same report is always printed to the console. +- `--no-ruff`: skip the final `ruff` pass, leaving every unused import in the modified files in place, including + the ones the recipe made unused. + +Runs `TypeVarCheck` on every file, then `ruff check --fix --select F401` on the files it changed (unless +`--no-ruff` is passed). That pass removes every import `ruff` reports as unused in those files, including +imports that were already unused before the run. `ruff` is invoked as `python -m ruff`, so it has to be +installed in the same environment as the tool; if it can't run, the files keep the recipe's output and +the report line still says the pass ran. Changes are written directly, so run it on a git checkout and review +with `git diff`. + +The report has a summary line, then the `MODIFIED`, `NEEDS MANUAL REVIEW` (each unsafe name with its documented +rule and link) and `ERRORS` sections; files with no type-parameter usage are only counted. The exit code is 0 on +normal completion (files needing manual review are not a failure), 2 for a usage error such as a missing path or +a malformed `--py`, and 3 if any file raised an unhandled exception, which is listed under `ERRORS`. + +## Change considerations + +- Supporting a future type-parameter-declaring construct means extending `_is_type_param_call` and + `build_type_param` in `type_var_domain.py` together. +- Following re-exports through an intermediate `__init__.py`, or supporting namespace packages (PEP 420), means + extending `resolve_project_module` in `renaissance/utils/import_resolution.py`. diff --git a/mkdocs.yml b/mkdocs.yml index d836a63f..5d02d555 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -118,7 +118,8 @@ nav: - 7. Analysis: user/concepts/analysis.md - 8. Transformation: user/concepts/transformation.md - 9. Composition: user/concepts/composition.md - - 10. Standard analyses and transformations: user/concepts/standard-libraries.md + - 10. Python version gates: user/concepts/python-version-gates.md + - 11. Standard analyses and transformations: user/concepts/standard-libraries.md - Features: - Overview: user/features/index.md @@ -134,6 +135,7 @@ nav: - 9. Find: user/features/find.md - 10. Modify: user/features/modify.md - 11. Rewrite semantics: user/features/rewrite-semantics.md + - 12. TypeVar modernization: user/features/typevar-modernization.md - Workflows: - Overview: user/workflows/index.md @@ -192,6 +194,9 @@ 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 + - Python AST known limitations: developer/modules/python-ast-known-limitations.md + - Rejected recipes: developer/modules/rejected-recipes.md - API reference: - Overview: developer/api/index.md diff --git a/pyproject.toml b/pyproject.toml index bcd63f3d..e61f7aa3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -31,6 +31,7 @@ dependencies = [ "hypothesmith>=0.3.3", "autopep8>=2.0", "flake8>=7.0", + "packaging>=24.0", ] [dependency-groups] @@ -111,6 +112,7 @@ select = [ "Q", "RSE", "SIM", "SLOT", "T10", "TID", "UP", "W", "YTT", ] #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 diff --git a/src/rejuvenation/migration-type-recipes.py b/src/rejuvenation/migration-type-recipes.py new file mode 100644 index 00000000..8d919f2d --- /dev/null +++ b/src/rejuvenation/migration-type-recipes.py @@ -0,0 +1,288 @@ +"""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.step_runner import Step, run_steps +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 via PythonScanner + (whole-tree, no package_dirs allowlist, so arbitrary third-party layouts are supported). + """ + 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 against a single file, returning one FileReport. + + Any failure is caught and reported on FileReport.error instead of propagating, since one bad + file must never abort 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 = run_steps( + [ + Step("cross_file", recipe, recipe.localize_imported_typevars), + Step("converted", recipe, recipe.convert_declared_typevars), + Step("orphaned", recipe, recipe.remove_orphaned_declarations), + ], + ) + 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: prints a warning and returns normally if ruff can't be invoked (e.g. not + installed), rather than raising - the files' own content is already correct at this point + regardless of whether this cleanup succeeds. + """ + 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 TypeVar usage found at all) are folded into the top-line count only, never + listed individually - the report's job is to surface what needs attention. 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}") + for phase, names in (report.result or {}).items(): + fixed = [name for name, status in names.items() if status == "fixed"] + 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+.", + ) + 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: computed once upfront, so an origin whose importers all get localized this run is only converted on a second run. + 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: removed statements leave their blank lines behind; ruff's E303 (preview) collapses them, but not at file start. + _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/project/project_scanner.py b/src/renaissance/project/project_scanner.py index 87728eaa..14fd315b 100644 --- a/src/renaissance/project/project_scanner.py +++ b/src/renaissance/project/project_scanner.py @@ -63,7 +63,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}" @@ -72,13 +72,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 6eeeeac2..3819b85c 100644 --- a/src/renaissance/recipes/python_refactoring.py +++ b/src/renaissance/recipes/python_refactoring.py @@ -1,9 +1,10 @@ """AI: Base processor for Python-specific source refactoring recipes.""" +import ast import importlib from collections.abc import Sequence from pathlib import Path -from typing import cast +from typing import Any, cast from termcolor import colored @@ -16,6 +17,24 @@ from renaissance.utils.text_utils import snake_case +def narrowed_import_text(raw: ast.ImportFrom, names: str | set[str]) -> str | None: + """Build the "from module import ..." text for `raw` with `names`' aliases dropped. + + A relative import keeps its leading dots, e.g. "from ..pkg import x" or "from . import x". + + Returns None if nothing would remain (meaning the whole import statement should be removed + instead). + """ + targets = {names} if isinstance(names, str) else names + remaining = [ + alias.name if alias.asname is None else f"{alias.name} as {alias.asname}" + for alias in raw.names + if (alias.asname or alias.name) not in targets + ] + module = "." * raw.level + (raw.module or "") + return f"from {module} import {', '.join(remaining)}" if remaining else None + + class PythonRefactoring(ASTProcessor): """AI: Base processor for Python-specific source refactoring recipes.""" @@ -77,7 +96,7 @@ def extract_call_arguments(self, node: PythonRstNode) -> tuple[list[str], dict[s positional_args = [arg_node.signature for arg_node in (args_implicit.children if args_implicit else [])] keyword_args: dict[str, str] = {} - for kw_node in (keywords_implicit.children if keywords_implicit else []): + for kw_node in keywords_implicit.children if keywords_implicit else []: kw_name = kw_node.node.arg if kw_name: value_node = kw_node.children[0] if kw_node.children else kw_node @@ -109,5 +128,26 @@ def body(self) -> Sequence[PythonRstNode]: """AI: Return the root node's body statements.""" return cast("PythonRstNode", cast("object", self.root)).body + def find_rst_node(self, target: ast.AST) -> Any: + """Locate the PythonRstNode wrapping a raw ast node. + + E.g. after mutating an ast.FunctionDef in place, this finds the RST node to pass to + self.replace(). + """ + # TODO: Drop once recipes can navigate wrapper nodes via the unified node protocol? + # decided during 24-09 discussion over future Node Protocol implementation + found: list[Any] = [] + + def visit(node: Any) -> None: + if node.node is target: + found.append(node) + + cast("PythonRstNode", cast("object", self.root)).process(visit) + return found[0] + def run(self): - """AI: Run this refactoring recipe. Subclasses override this to perform the refactoring.""" + """Perform this recipe's refactoring. + + Overridden by every concrete subclass; the base no-op lets process() call it uniformly + even for a recipe that hasn't overridden it. + """ diff --git a/src/renaissance/recipes/step_runner.py b/src/renaissance/recipes/step_runner.py new file mode 100644 index 00000000..91d4a577 --- /dev/null +++ b/src/renaissance/recipes/step_runner.py @@ -0,0 +1,29 @@ +"""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.recipes.python_refactoring import PythonRefactoring + + +@dataclass(frozen=True) +class Step: + """One independently-runnable, independently-committable fix action.""" + + label: str + recipe: PythonRefactoring + 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..741d96f0 --- /dev/null +++ b/src/renaissance/recipes/type_var_check.py @@ -0,0 +1,243 @@ +"""Recipe that modernizes legacy TypeVar/ParamSpec/TypeVarTuple usage to PEP 695 syntax.""" + +import ast +from pathlib import Path +from typing import Any, cast + +from renaissance.integrations.python.ast.rst_node import PythonRstNode +from renaissance.recipes.python_refactoring import PythonRefactoring, narrowed_import_text +from renaissance.recipes.step_runner import Step, run_steps +from renaissance.recipes.type_var_domain import ( + UnsafeReason, + all_refs_shadowed_by_pep695, + build_type_param, + find_import_source, + find_type_param_declarations, + functions_using_nodes, + is_safe_to_convert, + is_safe_to_localize, + type_param_constructor_name, + type_param_name, +) +from renaissance.utils.import_resolution import resolve_project_module +from renaissance.utils.unparse_utils import unparse_signature_only + +PEP_695_MINIMUM = (3, 12) + + +class TypeVarCheck(PythonRefactoring): + """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 run(self) -> None: + """Entry point called by PythonRefactoring.process(); stores check()'s result.""" + self.result = self.check() + + def _target_supports_pep695(self) -> bool: + """Return True only if min_python is known and is 3.12+. + + An unknown minimum returns False: PEP 695 syntax (`def f[T](...)`) is a hard SyntaxError + before Python 3.12. + """ + return self.min_python is not None and self.min_python >= PEP_695_MINIMUM + + def check(self) -> dict[str, dict[str, str]]: + """Check this file's TypeVar/ParamSpec/TypeVarTuple usage end to end. + + Runs three phases in order - localize_imported_typevars, then convert_declared_typevars, + then remove_orphaned_declarations (see each method's own docstring for what it does and + why). 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]: + """Rewrite every function using a module-level TypeVar/ParamSpec/TypeVarTuple to PEP 695 syntax. + + Whether it's used by one function or shared across several, then remove the + now-redundant module-level declaration - see is_safe_to_convert and the check() + docstring. Returns {name: "fixed" | "unsafe"}. + + PEP 695 syntax requires Python 3.12+ on the target codebase; if min_python doesn't + guarantee that, every candidate is reported "unsafe" and the file is left untouched + by this phase - localize_imported_typevars still runs regardless, since it never + introduces PEP 695 syntax. The specific UnsafeReason behind each "unsafe" entry is + recorded on self.converted_unsafe_reasons. + """ + root = cast("PythonRstNode", cast("object", self.root)) + tree = cast(ast.Module, root.node) + declarations = find_type_param_declarations(tree) + usage = functions_using_nodes(tree, set(declarations.keys())) + + if not self._target_supports_pep695(): + self.converted_unsafe_reasons = dict.fromkeys(usage, UnsafeReason.PEP695_VERSION_GATE) + return dict.fromkeys(usage, "unsafe") + + results: dict[str, str] = {} + self.converted_unsafe_reasons: dict[str, UnsafeReason] = {} + # Collected here instead of replaced immediately: a function using 2+ converted type + # params (e.g. TypeVar and ParamSpec) must get exactly one self.replace() covering all + # of them - queuing one per name would target the same function node twice before a + # commit, which the rewriter rejects as conflicting. + touched_functions: dict[int, ast.FunctionDef | ast.AsyncFunctionDef] = {} + for name, functions in usage.items(): + decl_stmt = declarations[name] + reason = is_safe_to_convert(tree, name, decl_stmt, self.project_wide_imported_names) + if reason is not None: + self._mark_unsafe(results, self.converted_unsafe_reasons, name, reason) + continue + + type_param = build_type_param(decl_stmt) + for function in functions: + if any(type_param_name(existing) == name for existing in function.type_params): + continue # already PEP 695 syntax (handled by Ruff) + function.type_params = [*function.type_params, type_param] + touched_functions[id(function)] = function + + self._remove_declaration(decl_stmt) + results[name] = "fixed" + + for function in touched_functions.values(): + rst_node = self.find_rst_node(function) + self.replace(unparse_signature_only(function, rst_node.text), rst_node, False, False) + + return results + + def remove_orphaned_declarations(self) -> dict[str, str]: + """Remove a module-level TypeVar/ParamSpec/TypeVarTuple declaration once it's orphaned. + + Every remaining reference to it is shadowed by a same-named PEP 695 type parameter on + the function(s) using it (see all_refs_shadowed_by_pep695) - the state ruff's UP047 + leaves behind after converting a signature, since that rule documents that it never + removes the declaration it makes redundant. Returns {name: "fixed" | "unsafe"}; the + specific UnsafeReason behind each "unsafe" entry is recorded on + self.orphaned_unsafe_reasons. + """ + root = cast("PythonRstNode", cast("object", self.root)) + tree = cast(ast.Module, root.node) + declarations = find_type_param_declarations(tree) + + results: dict[str, str] = {} + self.orphaned_unsafe_reasons: dict[str, UnsafeReason] = {} + for name, decl_stmt in declarations.items(): + if not all_refs_shadowed_by_pep695(tree, name, decl_stmt): + continue + + reason = is_safe_to_convert(tree, name, decl_stmt, self.project_wide_imported_names) + if reason is not None: + self._mark_unsafe(results, self.orphaned_unsafe_reasons, name, reason) + continue + + 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: ast.Assign) -> None: + """Remove decl_stmt's statement from the file.""" + for stmt_node in self.body: + if stmt_node.node is decl_stmt: + # TODO - enable once comment blocks get correctly deleted + self.remove(stmt_node, include_comments=False) + break + + def localize_imported_typevars(self) -> dict[str, str]: + """Find TypeVar/ParamSpec/TypeVarTuple names imported from anywhere in the target project. + + Absolute and relative imports are resolved against project_root. Where safe (see + is_safe_to_localize), rewrites the import into an equivalent local + declaration. Returns {name: "fixed" | "unsafe"} for every candidate found; the specific + UnsafeReason behind each "unsafe" entry is recorded on self.cross_file_unsafe_reasons. + """ + results: dict[str, str] = {} + self.cross_file_unsafe_reasons: dict[str, UnsafeReason] = {} + + project_root = self.project_root if self.project_root is not None else Path(self.filename).parent + for import_node in self.body: + raw = import_node.node + if not isinstance(raw, ast.ImportFrom): + continue + + origin_path = resolve_project_module(Path(self.filename), project_root, raw.module, raw.level) + if origin_path is None: + continue + + origin_tree = ast.parse(origin_path.read_text(encoding="utf-8")) + declarations = find_type_param_declarations(origin_tree) + + for alias in raw.names: + if alias.asname is not None or alias.name not in declarations: + continue + + reason = is_safe_to_localize(origin_tree, alias.name) + if reason is not None: + self._mark_unsafe(results, self.cross_file_unsafe_reasons, alias.name, reason) + continue + + decl_stmt = declarations[alias.name] + needed_import = self._missing_constructor_import(origin_tree, decl_stmt) + # TODO: queues one replace per alias on the same import node, which conflicts when a statement localizes 2+ names. + self._localize_import(import_node, raw, alias.name, decl_stmt, needed_import) + results[alias.name] = "fixed" + + return results + + def _missing_constructor_import(self, origin_tree: ast.Module, decl_stmt: ast.Assign) -> str | None: + """Build the "from module import Ctor" text so the localized declaration's constructor is importable. + + Prepend this to the declaration if the constructor call (TypeVar/ParamSpec/TypeVarTuple) + isn't already imported here; returns None if it already is. + """ + ctor_name = type_param_constructor_name(decl_stmt) + ctor_module = find_import_source(origin_tree, ctor_name) + if ctor_module is None: + return None + + for import_node in self.body: + raw = import_node.node + if ( + isinstance(raw, ast.ImportFrom) + and raw.module == ctor_module + and any((alias.asname or alias.name) == ctor_name for alias in raw.names) + ): + return None + + return f"from {ctor_module} import {ctor_name}" + + def _localize_import(self, import_node: Any, raw: ast.ImportFrom, name: str, decl_stmt: ast.Assign, needed_import: str | None) -> None: + """Replace import_node with decl_stmt's text as a local declaration. + + Narrows or removes the original import for name, and prepends needed_import if the + declaration's constructor isn't already imported here. + """ + decl_text = ast.unparse(decl_stmt) + if needed_import is not None: + decl_text = f"{needed_import}\n{decl_text}" + + new_import = narrowed_import_text(raw, name) + if new_import is not None: + self.replace(f"{new_import}\n{decl_text}", import_node, False, False) + else: + self.replace(decl_text, import_node, False, False) diff --git a/src/renaissance/recipes/type_var_domain.py b/src/renaissance/recipes/type_var_domain.py new file mode 100644 index 00000000..20a3917f --- /dev/null +++ b/src/renaissance/recipes/type_var_domain.py @@ -0,0 +1,288 @@ +"""TypeVar/ParamSpec/TypeVarTuple domain model and safety analysis.""" + +import ast +from dataclasses import dataclass +from enum import StrEnum +from typing import cast + +DOCS_BASE_URL = "https://tno.github.io/Renaissance.Py/user/features/typevar-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" + DECLARED_TYPEVAR_EXPORTED = "declared_typevar_exported" + USED_OUTSIDE_FUNCTION = "used_outside_function" + ORIGIN_MODULE_EXPORTS_NAME = "origin_module_exports_name" + USED_IN_EXPORTED_GENERIC_BASE = "used_in_exported_generic_base" + 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-typevar-modernization-pep695-version-gate", + ), + UnsafeReason.DECLARED_TYPEVAR_EXPORTED: UnsafeRule( + "exported via __all__", "feature-typevar-modernization-declared-typevar-exported", + ), + UnsafeReason.USED_OUTSIDE_FUNCTION: UnsafeRule( + "used outside a function body, e.g. a Generic[...] base", "feature-typevar-modernization-used-outside-function", + ), + UnsafeReason.ORIGIN_MODULE_EXPORTS_NAME: UnsafeRule( + "origin module exports it via __all__", "feature-typevar-modernization-origin-module-exports-name", + ), + UnsafeReason.USED_IN_EXPORTED_GENERIC_BASE: UnsafeRule( + "used in a Generic[...] base at its origin module", "feature-typevar-modernization-used-in-exported-generic-base", + ), + UnsafeReason.IMPORTED_ELSEWHERE_IN_PROJECT: UnsafeRule( + "imported directly by another file in the target project", "feature-typevar-modernization-imported-elsewhere-in-project", + ), + UnsafeReason.ORIGIN_IMPORTS_CONSTRUCTOR_CONDITIONALLY: UnsafeRule( + "origin module imports TypeVar/ParamSpec/TypeVarTuple conditionally, e.g. per Python version", + "feature-typevar-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}" + + +def _is_type_param_call(value: ast.expr) -> bool: + """Return True if `value` is a call to TypeVar/ParamSpec/TypeVarTuple.""" + return ( + isinstance(value, ast.Call) + and isinstance(value.func, ast.Name) + and value.func.id in ("TypeVar", "ParamSpec", "TypeVarTuple") + ) + + +def find_type_param_declarations(tree: ast.Module) -> dict[str, ast.Assign]: + """Find every module-level "NAME = TypeVar/ParamSpec/TypeVarTuple(...)" declaration.""" + declarations: dict[str, ast.Assign] = {} + for stmt in tree.body: + if isinstance(stmt, ast.Assign) and _is_type_param_call(stmt.value): + for target in stmt.targets: + if isinstance(target, ast.Name): + declarations[target.id] = stmt + return declarations + + +def type_param_name(param: ast.type_param) -> str: + """Return a PEP 695 type parameter's name. + + `ast.type_param`'s own stub doesn't declare `.name` - only its three concrete subclasses + (`ast.TypeVar`/`ast.ParamSpec`/`ast.TypeVarTuple`) do, and every real type_param is one of + them, so this narrows to get at it. + """ + assert isinstance(param, ast.TypeVar | ast.ParamSpec | ast.TypeVarTuple) + return param.name + + +def type_param_constructor_name(decl_stmt: ast.Assign) -> str: + """Return the name of the call a declaration uses, e.g. "TypeVar" for `T = TypeVar("T")`.""" + call = cast(ast.Call, decl_stmt.value) + return cast(ast.Name, call.func).id + + +def _find_dunder_all(tree: ast.Module) -> set[str] | None: + """Return the names listed in this module's `__all__`, or None if it doesn't declare one.""" + for stmt in tree.body: + if ( + isinstance(stmt, ast.Assign) + and any(isinstance(t, ast.Name) and t.id == "__all__" for t in stmt.targets) + and isinstance(stmt.value, ast.List | ast.Tuple | ast.Set) + ): + return { + elt.value + for elt in stmt.value.elts + if isinstance(elt, ast.Constant) and isinstance(elt.value, str) + } + return None + + +def _used_in_exported_generic_base(tree: ast.Module, name: str) -> bool: + """Return True if `name` appears inside a `Generic[...]` base of any class in this module.""" + for node in ast.walk(tree): + if not isinstance(node, ast.ClassDef): + continue + for base in node.bases: + if not isinstance(base, ast.Subscript): + continue + if not (isinstance(base.value, ast.Name) and base.value.id == "Generic"): + continue + for inner in ast.walk(base.slice): + if isinstance(inner, ast.Name) and inner.id == name: + return True + return False + + +def _imports_name_conditionally(tree: ast.Module, name: str) -> bool: + """Return True if an import inside a module-level `if`/`try` block binds `name`.""" + for stmt in tree.body: + if not isinstance(stmt, ast.If | ast.Try | ast.TryStar): + continue + for node in ast.walk(stmt): + if isinstance(node, ast.Import | ast.ImportFrom) and any( + (alias.asname or alias.name.split(".")[0]) == name for alias in node.names + ): + return True + return False + + +def is_safe_to_localize(origin_tree: ast.Module, name: str) -> UnsafeReason | None: + """Return None if `name` is safe to duplicate as a local declaration, else the reason it isn't. + + The origin module must not advertise it as public API, whether via `__all__` + (ORIGIN_MODULE_EXPORTS_NAME) or as a class-level `Generic[...]` parameter + (USED_IN_EXPORTED_GENERIC_BASE, where identity crossing files can matter for subclassing). + Its constructor (TypeVar/ParamSpec/TypeVarTuple) must also not be imported inside a + module-level `if`/`try` block (ORIGIN_IMPORTS_CONSTRUCTOR_CONDITIONALLY), since which + implementation it binds then depends on the runtime, e.g. `typing_extensions` below 3.13. + """ + dunder_all = _find_dunder_all(origin_tree) + if dunder_all is not None and name in dunder_all: + return UnsafeReason.ORIGIN_MODULE_EXPORTS_NAME + if _used_in_exported_generic_base(origin_tree, name): + return UnsafeReason.USED_IN_EXPORTED_GENERIC_BASE + declaration = find_type_param_declarations(origin_tree).get(name) + if declaration is not None and _imports_name_conditionally(origin_tree, type_param_constructor_name(declaration)): + return UnsafeReason.ORIGIN_IMPORTS_CONSTRUCTOR_CONDITIONALLY + return None + + +def find_import_source(tree: ast.Module, name: str) -> str | None: + """Which module a bare name (e.g. "TypeVar") was imported from in this file, e.g. "typing".""" + for stmt in tree.body: + if isinstance(stmt, ast.ImportFrom) and stmt.module is not None: + for alias in stmt.names: + if (alias.asname or alias.name) == name: + return stmt.module + return None + + +def functions_using_nodes( + tree: ast.Module, names: set[str] +) -> dict[str, list[ast.FunctionDef | ast.AsyncFunctionDef]]: + """Map each of `names` to the outermost function/method node whose signature or body references it. + + A name referenced inside a nested function (a closure) is attributed to the *outermost* + function in its nesting chain, never the nested one: a PEP 695 type parameter declared on the + enclosing function is already visible inside its closures. + """ + usage: dict[str, list[ast.FunctionDef | ast.AsyncFunctionDef]] = {name: [] for name in names} + + def visit(node: ast.AST, enclosing: ast.FunctionDef | ast.AsyncFunctionDef | None) -> None: + current = enclosing + if isinstance(node, ast.FunctionDef | ast.AsyncFunctionDef) and enclosing is None: + current = node + if isinstance(node, ast.Name) and current is not None and node.id in usage and current not in usage[node.id]: + usage[node.id].append(current) + for child in ast.iter_child_nodes(node): + visit(child, current) + + visit(tree, None) + return usage + + +def _used_outside_functions(tree: ast.Module, name: str, decl_stmt: ast.Assign) -> bool: + """Return True if `name` is referenced anywhere outside a function/method body. + + Other than its own declaration - e.g. a class's `Generic[...]` base or a module-level type + alias. + """ + + def visit(node: ast.AST, in_function: bool) -> bool: + if node is decl_stmt: + return False + if isinstance(node, ast.Name) and node.id == name and not in_function: + return True + current = in_function or isinstance(node, ast.FunctionDef | ast.AsyncFunctionDef) + return any(visit(child, current) for child in ast.iter_child_nodes(node)) + + return visit(tree, False) + + +def is_safe_to_convert( + tree: ast.Module, + name: str, + decl_stmt: ast.Assign, + project_wide_imported_names: frozenset[str] = frozenset(), +) -> UnsafeReason | None: + """Return None if `name` is safe to convert to PEP 695 syntax and its declaration removed. + + Otherwise returns the reason it isn't: DECLARED_TYPEVAR_EXPORTED if exported via `__all__`, + IMPORTED_ELSEWHERE_IN_PROJECT if `name` is in `project_wide_imported_names` (another file in + the target project imports it directly, regardless of `__all__`), or + USED_OUTSIDE_FUNCTION if referenced anywhere outside the functions using it. + """ + dunder_all = _find_dunder_all(tree) + 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 + if _used_outside_functions(tree, name, decl_stmt): + return UnsafeReason.USED_OUTSIDE_FUNCTION + return None + + +def all_refs_shadowed_by_pep695(tree: ast.Module, name: str, decl_stmt: ast.Assign) -> bool: + """Return True if every remaining reference to `name` is shadowed by a PEP 695 type parameter. + + E.g. `def b[T](x: T) -> T:`, where `T` resolves to the function's own parameter rather than + the module-level declaration, making it dead. Also true (vacuously) if `name` isn't + referenced anywhere at all. + """ + found_live_use = False + + def visit(node: ast.AST, shadowed: bool) -> None: + nonlocal found_live_use + if node is decl_stmt or found_live_use: + return + if isinstance(node, ast.Name) and node.id == name: + if not shadowed: + found_live_use = True + return + current = shadowed + if isinstance(node, ast.FunctionDef | ast.AsyncFunctionDef): + current = any(type_param_name(param) == name for param in node.type_params) + for child in ast.iter_child_nodes(node): + visit(child, current) + + visit(tree, False) + return not found_live_use + + +def build_type_param(decl_stmt: ast.Assign) -> ast.type_param: + """Translate a legacy declaration into the equivalent PEP 695 type_param node. + + E.g. `T = TypeVar("T", bound=int)` becomes an `ast.TypeVar`/`ast.ParamSpec`/`ast.TypeVarTuple`. + """ + call = cast(ast.Call, decl_stmt.value) + ctor = cast(ast.Name, call.func).id + name = cast(str, cast(ast.Constant, call.args[0]).value) + + if ctor == "ParamSpec": + return ast.ParamSpec(name=name) + if ctor == "TypeVarTuple": + return ast.TypeVarTuple(name=name) + + bound = next((kw.value for kw in call.keywords if kw.arg == "bound"), None) + constraints = call.args[1:] + if bound is None and constraints: + bound = ast.Tuple(elts=list(constraints), ctx=ast.Load()) + return ast.TypeVar(name=name, bound=bound) diff --git a/src/renaissance/utils/import_resolution.py b/src/renaissance/utils/import_resolution.py new file mode 100644 index 00000000..07b84524 --- /dev/null +++ b/src/renaissance/utils/import_resolution.py @@ -0,0 +1,137 @@ +"""Resolve project-internal `from X import Y` statements to the .py file they import from. + +Used across an entire target codebase to check whether a declaration is still depended on by +another file before it's removed/rewritten - unlike `__all__`, an explicit `from module import +name` works regardless of whether the origin module declares `__all__`. +""" + +import ast +from collections.abc import Sequence +from pathlib import Path + + +def resolve_project_module(importing_file: Path, project_root: Path, module: str | None, level: int) -> Path | None: + """Resolve one `ast.ImportFrom`'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` for a package-style import. Returns None + if neither exists, or if resolution would walk above `project_root` - the common case for a + stdlib/third-party import, which is exactly the signal used to exclude those as noise. + Relative input paths are made absolute first, so 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 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: + tree = ast.parse(file.read_text(encoding="utf-8")) + except OSError, SyntaxError: + continue + module_bindings: dict[str, Path] = {} + for stmt in ast.walk(tree): + if isinstance(stmt, ast.ImportFrom): + _record_from_import(file, project_root, stmt, imported, module_bindings) + elif isinstance(stmt, ast.Import): + _bind_imported_modules(file, project_root, stmt, module_bindings) + _record_module_attribute_reads(tree, module_bindings, imported) + return {path: frozenset(names) for path, names in imported.items()} + + +def _record_from_import( + file: Path, + project_root: Path, + stmt: ast.ImportFrom, + imported: dict[Path, set[str]], + module_bindings: dict[str, Path], +) -> None: + """Record `stmt`'s imported names against their origin, and bind any alias that is itself a project module.""" + # TODO: `from pkg.mod import *` isn't expanded to the names it actually pulls in. + origin = resolve_project_module(file, project_root, stmt.module, stmt.level) + if origin is not None: + imported.setdefault(origin, set()).update(alias.name for alias in stmt.names) + for alias in stmt.names: + submodule = f"{stmt.module}.{alias.name}" if stmt.module is not None else alias.name + submodule_origin = resolve_project_module(file, project_root, submodule, stmt.level) + if submodule_origin is not None: + module_bindings[alias.asname or alias.name] = submodule_origin + + +def _bind_imported_modules(file: Path, project_root: Path, stmt: ast.Import, module_bindings: dict[str, Path]) -> None: + """Bind the dotted name(s) `stmt` makes available to the project module file each one refers to.""" + for alias in stmt.names: + if alias.asname is not None: + origin = resolve_project_module(file, project_root, alias.name, 0) + if origin is not None: + module_bindings[alias.asname] = origin + continue + # `import a.b.c` binds `a`, and makes `a.b` and `a.b.c` reachable through it. + parts = alias.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(tree: ast.Module, module_bindings: dict[str, Path], imported: dict[Path, set[str]]) -> None: + """Record every `.` in `tree` as a dependency on `attr` in that module's file.""" + if not module_bindings: + return + for node in ast.walk(tree): + if not isinstance(node, ast.Attribute): + continue + dotted = _dotted_name(node.value) + origin = module_bindings.get(dotted) if dotted is not None else None + if origin is not None: + imported.setdefault(origin, set()).add(node.attr) + + +def _dotted_name(expr: ast.expr) -> str | None: + """Return `expr` as a dotted name (`a.b.c`) if it is a plain name/attribute chain, else None.""" + if isinstance(expr, ast.Name): + return expr.id + if isinstance(expr, ast.Attribute): + base = _dotted_name(expr.value) + return f"{base}.{expr.attr}" if base is not None else None + return None diff --git a/src/renaissance/utils/unparse_utils.py b/src/renaissance/utils/unparse_utils.py new file mode 100644 index 00000000..6e4b94d3 --- /dev/null +++ b/src/renaissance/utils/unparse_utils.py @@ -0,0 +1,136 @@ +"""Splice a PEP 695 type-params bracket into a function's original source, changing nothing else. + +`ast.unparse()` can only regenerate a *whole* node's source, and does so in its own style - +reformatting whatever it touches regardless of the original formatting (collapsing a multi-line +parameter list onto one line, among other things), and dropping anything the `ast` module never +records in the first place (comments, most notably). Convert-to-PEP-695 only ever adds a +`[T]`/`[**P]`/`[*Ts]` bracket right after a function's name - splicing just that bracket into the +function's untouched original text avoids regenerating (and so reformatting) anything else. +""" + +import ast +import io +import re +import tokenize + + +def _name_end_offset(source: str, name: str) -> int: + """Return the character offset right after `def name`/`async def name` in `source`. + + Allows leading whitespace before `def`/`async def` - a decorated function's source has the + decorator on line 1, so the `def` line itself is a continuation line carrying its own real + indentation, not necessarily flush at column 0. + """ + match = re.search(rf"^[ \t]*(async\s+)?def\s+{re.escape(name)}\b", source, re.MULTILINE) + if match is None: + raise ValueError(f"no 'def {name}' header found") + return match.end() + + +def _bracket_end_offset(source: str, open_offset: int) -> int: + """Return the offset right after the `]` matching the `[` at `open_offset` in `source`. + + Tracks bracket depth so a bound like `list[int]` nesting inside the type-params bracket + itself doesn't close it early. Doesn't account for `[`/`]` inside a string literal (e.g. an + unbalanced bracket in a forward-reference bound) - the type-params bracket is always a single, + short, compact expression in practice, so that edge case is accepted rather than solved. + """ + depth = 0 + for offset in range(open_offset, len(source)): + if source[offset] == "[": + depth += 1 + elif source[offset] == "]": + depth -= 1 + if depth == 0: + return offset + 1 + raise ValueError("no closing ']' found") + + +def _type_params_bracket(node: ast.FunctionDef | ast.AsyncFunctionDef) -> str: + """Return the PEP 695 `[...]` bracket text for node's current type_params, or "" if none. + + E.g. `"[T]"`, `"[T: int, **P]"` - whatever `ast.unparse(node)` would produce. + """ + if not node.type_params: + return "" + unparsed = ast.unparse(node) + start = _name_end_offset(unparsed, node.name) + end = _bracket_end_offset(unparsed, start) + return unparsed[start:end] + + +def _header_end_line(source: str) -> int: + """Return the 1-indexed line where a def header's terminating ':' sits in `source`. + + Tracks `([{`/`)]}` bracket depth (via `tokenize`) so a colon inside a string default, a + lambda default, or an annotation - anything not at the header's own top level - isn't + mistaken for the real one. + """ + depth = 0 + for tok in tokenize.generate_tokens(io.StringIO(source).readline): + if tok.type == tokenize.OP and tok.string in "([{": + depth += 1 + elif tok.type == tokenize.OP and tok.string in ")]}": + depth -= 1 + elif tok.type == tokenize.OP and tok.string == ":" and depth == 0: + return tok.end[0] + raise ValueError("no header-terminating ':' found") + + +def unparse_signature_only(node: ast.FunctionDef | ast.AsyncFunctionDef, original_text: str) -> str: + """Insert node's PEP 695 type-params bracket into `original_text`, changing nothing else. + + `original_text` is node's own source text from before `node.type_params` was mutated (e.g. + `self.find_rst_node(node).text`, captured before appending to it). If `original_text` already + has a bracket right after the function name - the function already declared *other* type + params before this pass touched it, e.g. `def f[U](x: U, y: T) -> T:` when only `T` is being + converted - that whole bracket is replaced with the new one (which already includes both the + old and new params, since `node.type_params` does by the time this runs). Otherwise a fresh + bracket is inserted. + + Every other byte of `original_text` - parameter list, defaults, line breaks, return type, + docstring, body, comments - is preserved, though lines after the first are re-indented + relative to a column-0 `def` (see `_renormalize_indent`): `original_text` carries each line's + real, absolute indentation from the source file, but the rewrite pipeline + (`ast_rewriter.py`) re-adds the target's real indentation to every line but the first before + inserting, so returning absolute indentation here would shift everything twice. + """ + new_bracket = _type_params_bracket(node) + insert_at = _name_end_offset(original_text, node.name) + + end = insert_at + if original_text[insert_at : insert_at + 1] == "[": + end = _bracket_end_offset(original_text, insert_at) + + spliced = original_text[:insert_at] + new_bracket + original_text[end:] + lines = spliced.split("\n") + if len(lines) == 1: + return spliced + + header_end_line = _header_end_line(spliced) + header_tail = lines[1:header_end_line] # a multi-line signature's own continuation lines + body = lines[header_end_line:] + # the header's own continuation lines (e.g. a closing ") -> T:") sit at the def's own column, + # so they renormalize to 0; the body sits one Python indentation level deeper, so 4. + return "\n".join([lines[0], *_renormalize_indent(header_tail, 0), *_renormalize_indent(body, 4)]) + + +def _renormalize_indent(lines: list[str], target_indent: int) -> list[str]: + """Shift `lines` so their common leading indent becomes `target_indent`. + + See `unparse_signature_only`'s docstring for why: `original_text` carries each line's real, + absolute indentation, but the rewrite pipeline re-adds the target's own real indentation on + top of whatever this returns, so it must be expressed relative to a column-0 `def` first. + """ + non_blank = [line for line in lines if line.strip()] + if not non_blank: + return lines + # TODO: this minimum also counts lines inside multi-line string literals, which can change a literal's value. + common = min(len(line) - len(line.lstrip(" ")) for line in non_blank) + shift = common - target_indent + if shift > 0: + return [line[shift:] if line.strip() else line for line in lines] + if shift < 0: + pad = " " * -shift + return [pad + line if line.strip() else line for line in lines] + return lines diff --git a/test/project/test_project_scanner.py b/test/project/test_project_scanner.py index 7b2387b1..fd79eb44 100644 --- a/test/project/test_project_scanner.py +++ b/test/project/test_project_scanner.py @@ -261,6 +261,20 @@ def test_default_package_dirs_scans_whole_root_dir(self, tmp_path): 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 diff --git a/test/recipes/conftest.py b/test/recipes/conftest.py new file mode 100644 index 00000000..e5e7f449 --- /dev/null +++ b/test/recipes/conftest.py @@ -0,0 +1,46 @@ +"""Shared fixtures for the refactoring recipe test suite.""" + +import textwrap +from collections.abc import Callable +from typing import cast + +import pytest +from pytest_mock import MockerFixture + +from renaissance.integrations.python.ast.rst_node import PythonRstNode +from renaissance.recipes.python_refactoring import PythonRefactoring +from renaissance.recipes.type_var_check import PEP_695_MINIMUM, TypeVarCheck + + +@pytest.fixture +def make_recipe(mocker: MockerFixture) -> Callable[[type[PythonRefactoring], str], PythonRefactoring]: + """Build a `recipe_cls` instance against in-memory, dedented source text. + + `PythonFactory.create` is mocked so nothing touches the filesystem. Shared by every + refactoring recipe's tests instead of each reimplementing this setup - cast the result + to the concrete recipe type if you need attributes/methods beyond PythonRefactoring's own. + """ + + def _make(recipe_cls: type[PythonRefactoring], text: str, filename: str = "x.py") -> PythonRefactoring: + code = textwrap.dedent(text) + mocker.patch( + "renaissance.integrations.python.ast.factory.PythonFactory.create", + return_value=PythonRstNode.load_from_text(code), + ) + subject = recipe_cls(filename) + subject.in_memory = True + return subject + + return _make + + +@pytest.fixture +def create_type_var_check(make_recipe: Callable[[type[PythonRefactoring], str], PythonRefactoring]) -> Callable[[str], TypeVarCheck]: + """Like `make_recipe`, but with min_python set to 3.12 so PEP 695 conversion is enabled.""" + + def _create(text: str) -> TypeVarCheck: + subject = cast("TypeVarCheck", make_recipe(TypeVarCheck, text)) + 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 e6df8c88..54a1ec64 100644 --- a/test/recipes/test_python_refactoring.py +++ b/test/recipes/test_python_refactoring.py @@ -1,15 +1,19 @@ """Tests for the PythonRefactoring recipe base class.""" +import ast import keyword import textwrap +from typing import cast from unittest.mock import patch +import pytest from hamcrest import assert_that, contains_string, is_ from hypothesis import given, settings from hypothesis import strategies as st +from pytest_mock import MockerFixture from renaissance.integrations.python.ast.rst_node import PythonRstNode -from renaissance.recipes.python_refactoring import PythonRefactoring +from renaissance.recipes.python_refactoring import PythonRefactoring, narrowed_import_text from renaissance.recipes.unit_to_pytest import UnitToPytest from renaissance.syntax_tree.semantic_kind import SemanticKind @@ -288,5 +292,57 @@ def test_body_returns_module_level_statements(self, mocker): """, "test_foo.py", ) + subject = UnitToPytest("test_foo.py") assert_that(len(subject.body), is_(2)) + + # ------------------------------------------------------------------ + # find_rst_node + # ------------------------------------------------------------------ + + def test_find_rst_node_returns_wrapper_for_raw_ast_node(self, mocker: MockerFixture) -> None: + """AI: Verify find_rst_node locates the PythonRstNode wrapping a given raw ast.FunctionDef.""" + self._patch_factory( + mocker, + """ + def foo(): + pass + """, + "test_foo.py", + ) + + subject = UnitToPytest("test_foo.py") + root = cast("PythonRstNode", cast("object", subject.root)) + module = cast(ast.Module, root.node) + target = next(node for node in ast.walk(module) if isinstance(node, ast.FunctionDef)) + + found = subject.find_rst_node(target) + + assert_that(found.node, is_(target)) + + +class TestNarrowedImportText: + """Tests for narrowed_import_text.""" + + @pytest.mark.parametrize( + ("source", "expected"), + [ + ("from pkg.mod import T, x", "from pkg.mod import x"), + ("from .mod import T, x", "from .mod import x"), + ("from ..pkg.mod import T, x", "from ..pkg.mod import x"), + ("from . import T, x", "from . import x"), + ("from .. import T, x as y", "from .. import x as y"), + ], + ) + def test_keeps_module_and_relative_level(self, source: str, expected: str) -> None: + """Dropping a name keeps the module path, including a relative import's leading dots.""" + raw = cast("ast.ImportFrom", ast.parse(source).body[0]) + + assert_that(narrowed_import_text(raw, "T"), is_(expected)) + + @pytest.mark.parametrize("source", ["from pkg.mod import T", "from ..pkg.mod import T", "from . import T"]) + def test_returns_none_when_nothing_remains(self, source: str) -> None: + """Removing the only imported name returns None instead of an empty import.""" + raw = cast("ast.ImportFrom", ast.parse(source).body[0]) + + assert_that(narrowed_import_text(raw, "T"), is_(None)) diff --git a/test/recipes/test_step_runner.py b/test/recipes/test_step_runner.py new file mode 100644 index 00000000..a81da55a --- /dev/null +++ b/test/recipes/test_step_runner.py @@ -0,0 +1,65 @@ +"""Tests for Step/run_steps.""" + +from collections.abc import Callable + +import pytest +from hamcrest import assert_that, equal_to, is_ +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, is_(expect_commit)) + + def test_each_steps_recipe_commits_independently(self, mocker: MockerFixture) -> None: + """AI: Verify each step commits its own recipe independently, based only on its own result.""" + fixing_recipe = mocker.Mock(spec=PythonRefactoring) + unsafe_recipe = mocker.Mock(spec=PythonRefactoring) + + run_steps( + [ + Step("fixes", fixing_recipe, lambda: {"A": "fixed"}), + Step("unsafe", unsafe_recipe, lambda: {"B": "unsafe"}), + ] + ) + + assert_that(fixing_recipe.commit.called, is_(True)) + assert_that(unsafe_recipe.commit.called, is_(False)) diff --git a/test/recipes/test_type_var_check.py b/test/recipes/test_type_var_check.py new file mode 100644 index 00000000..978d2e0d --- /dev/null +++ b/test/recipes/test_type_var_check.py @@ -0,0 +1,150 @@ +"""Whole-class TypeVarCheck concerns not owned by a single phase. + +End-to-end check(), and the PEP 695 version gate. +""" + +import textwrap +from collections.abc import Callable +from pathlib import Path + +import pytest +from hamcrest import assert_that, contains_string, has_entry, is_, not_ +from pytest_mock import MockerFixture + +from renaissance.integrations.python.ast.rst_node import PythonRstNode +from renaissance.recipes.type_var_check import TypeVarCheck + + +class TestTypeVarCheck: + """See module docstring.""" + + def test_check_cleans_up_ruff_style_leftover_end_to_end(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """AI: Verify check() converts a PEP-695-ready TypeVar and leaves its ruff-style leftover for F401.""" + # "orphaned" (phase 3) stays empty here: phase 2 already drops the redundant declaration + # once it sees the function is pre-converted. + subject = create_type_var_check(""" + from typing import TypeVar + T = TypeVar('T') + + def b[T](x: T) -> T: + return x + """) + subject.run() + + assert_that(subject.result["converted"], has_entry("T", "fixed")) + assert_that(subject.result["orphaned"], is_({})) + output = subject.apply_to_string() + assert_that(output, contains_string("def b[T](x: T) -> T:")) + assert_that(output, not_(contains_string("T = TypeVar"))) + # The now-redundant `TypeVar` import itself is left for ruff's F401 to clean up - the + # recipe only owns removing the declaration, not general unused-import detection. + assert_that(output, contains_string("from typing import TypeVar")) + + def _create_versioned(self, mocker: MockerFixture, tmp_path: Path, min_python: tuple[int, int], code: str) -> TypeVarCheck: + """Build an in-memory TypeVarCheck over `code` with the given min_python.""" + file_path = str(tmp_path / "subject.py") + mocker.patch( + "renaissance.integrations.python.ast.factory.PythonFactory.create", + return_value=PythonRstNode.load_from_text(textwrap.dedent(code), file_path), + ) + subject = TypeVarCheck(file_path) + subject.min_python = min_python + subject.in_memory = True + return subject + + @pytest.mark.parametrize( + ("min_python", "expected"), + [ + pytest.param(None, False, id="unknown"), + pytest.param((3, 11), False, id="3.11"), + pytest.param((3, 12), True, id="3.12"), + pytest.param((3, 13), True, id="3.13"), + ], + ) + def test_pep695_gate_threshold( + self, + create_type_var_check: Callable[[str], TypeVarCheck], + min_python: tuple[int, int] | None, + *, + expected: bool, + ) -> None: + """The PEP 695 gate opens only for a known min_python of 3.12 or later.""" + subject = create_type_var_check("x = 1") + subject.min_python = min_python + + assert_that(subject._target_supports_pep695(), is_(expected)) # noqa: SLF001 + + def test_convert_declared_typevars_reports_unsafe_when_target_too_old(self, mocker: MockerFixture, tmp_path: Path) -> None: + """AI: Verify convert_declared_typevars reports "unsafe" and leaves the TypeVar untouched below 3.12.""" + subject = self._create_versioned( + mocker, + tmp_path, + (3, 10), + """ + from typing import TypeVar + + def a(x: T) -> T: + return x + def b(y: T) -> T: + return y + + T = TypeVar("T") + """, + ) + result = subject.convert_declared_typevars() + + assert_that(result, has_entry("T", "unsafe")) + assert_that(subject.apply_to_string(), contains_string('T = TypeVar("T")')) + + def test_convert_declared_typevars_still_fixes_when_target_new_enough(self, mocker: MockerFixture, tmp_path: Path) -> None: + """AI: Verify convert_declared_typevars still converts the TypeVar when the target is 3.12+.""" + subject = self._create_versioned( + mocker, + tmp_path, + (3, 12), + """ + from typing import TypeVar + + def a(x: T) -> T: + return x + + T = TypeVar("T") + """, + ) + result = subject.convert_declared_typevars() + + assert_that(result, has_entry("T", "fixed")) + assert_that(subject.apply_to_string(), contains_string("def a[T](x: T) -> T:")) + + def test_check_still_localizes_when_target_too_old(self, mocker: MockerFixture, tmp_path: Path) -> None: + """AI: Verify cross-file localization still runs when the target is too old for the PEP 695 conversion.""" + (tmp_path / "file_1.py").write_text( + textwrap.dedent(""" + from typing import TypeVar + T = TypeVar("T") + def a(x: T) -> T: + return x + """) + ) + importing_file = str(tmp_path / "file_2.py") + mocker.patch( + "renaissance.integrations.python.ast.factory.PythonFactory.create", + return_value=PythonRstNode.load_from_text( + textwrap.dedent(""" + from file_1 import T + def b(x: T) -> T: + return x + """), + importing_file, + ), + ) + subject = TypeVarCheck(importing_file) + subject.min_python = (3, 10) + subject.in_memory = True + subject.run() + + assert_that(subject.result["cross_file"], has_entry("T", "fixed")) + assert_that(subject.result["converted"], has_entry("T", "unsafe")) + output = subject.apply_to_string() + assert_that(output, contains_string("T = TypeVar('T')")) + assert_that(output, not_(contains_string("def b[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..9125f88a --- /dev/null +++ b/test/recipes/test_type_var_check_convert.py @@ -0,0 +1,523 @@ +"""Tests for TypeVarCheck.convert_declared_typevars.""" + +import ast +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.python_refactoring import PythonRefactoring +from renaissance.recipes.type_var_check import TypeVarCheck +from renaissance.recipes.type_var_domain import UnsafeReason + + +class TestTypeVarCheckConvert: + """See module docstring.""" + + def test_converts_typevar_shared_across_functions_to_pep695(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """AI: Verify a TypeVar shared across two functions converts both to PEP 695 syntax.""" + subject = create_type_var_check(""" + from typing import TypeVar + + def a(x: T) -> T: + return x + def b(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 a[T](x: T) -> T:")) + assert_that(output, contains_string("def b[T](y: T) -> T:")) + assert_that(output, not_(contains_string("T = TypeVar"))) + assert_that(output, contains_string("from typing import TypeVar")) + + def test_converts_typevar_shared_across_methods_to_pep695(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """AI: Verify a TypeVar shared across two methods of the same class converts both to PEP 695 syntax.""" + subject = create_type_var_check(""" + from typing import TypeVar + + class Foo: + def a(self, x: T) -> T: + return x + def b(self, 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 a[T](self, x: T) -> T:")) + assert_that(output, contains_string("def b[T](self, y: T) -> T:")) + + def test_converts_function_with_multiline_docstring_without_double_indenting( + self, create_type_var_check: Callable[[str], TypeVarCheck] + ) -> None: + """AI: Verify converting a signature doesn't double-indent its function's multi-line docstring.""" + # A multi-line docstring's continuation lines must not get double-indented. + subject = create_type_var_check(""" + from typing import TypeVar + + class Foo: + def cast(self, x: T) -> T: + \"\"\"First line. + + Second line already indented. + Third line too. + \"\"\" + return x + def other(self, 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 cast[T](self, x: T) -> T:")) + assert_that(output, contains_string(' """First line.')) + assert_that(output, contains_string(" Second line already indented.")) + assert_that(output, contains_string(" Third line too.")) + assert_that(output, contains_string(' """\n return x')) + # would appear if the continuation lines got shifted twice + assert_that(output, not_(contains_string(" Second line already indented."))) + + def test_converts_function_with_nested_docstring_indentation(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """AI: Verify converting a signature preserves a docstring's internal nested block's relative indentation.""" + # A docstring with an internal nested block (e.g. Sphinx's ".. seealso::") must keep + # that block's *relative* extra indentation, not get flattened to one uniform level. + subject = create_type_var_check(""" + from typing import TypeVar + + class Foo: + def cast(self, x: T) -> T: + \"\"\"Produce a cast. + + .. seealso:: + + :ref:`tutorial_casts` + \"\"\" + return x + def other(self, 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(" .. seealso::")) + assert_that(output, contains_string(" :ref:`tutorial_casts`")) + + def test_converts_function_with_single_line_docstring(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """AI: Verify converting a signature leaves a single-line docstring untouched.""" + subject = create_type_var_check(""" + from typing import TypeVar + + class Foo: + def cast(self, x: T) -> T: + \"\"\"One liner.\"\"\" + return x + def other(self, 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 cast[T](self, x: T) -> T:")) + assert_that(output, contains_string(' """One liner."""')) + + def test_converts_bound_typevar(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """AI: Verify a bound TypeVar converts to a PEP 695 type param carrying the same bound.""" + subject = create_type_var_check(""" + from typing import TypeVar + + def a(x: T) -> T: + return x + def b(y: T) -> T: + return y + + T = TypeVar("T", bound=int) + """) + result = subject.convert_declared_typevars() + + assert_that(result, has_entry("T", "fixed")) + assert_that(subject.apply_to_string(), contains_string("def a[T: int](x: T) -> T:")) + + def test_converts_constrained_typevar(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """AI: Verify a constrained TypeVar converts to a PEP 695 type param carrying the same constraints.""" + subject = create_type_var_check(""" + from typing import TypeVar + + def a(x: T) -> T: + return x + def b(y: T) -> T: + return y + + T = TypeVar("T", int, str) + """) + result = subject.convert_declared_typevars() + + assert_that(result, has_entry("T", "fixed")) + assert_that(subject.apply_to_string(), contains_string("def a[T: (int, str)](x: T) -> T:")) + + def test_converts_paramspec(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """AI: Verify a ParamSpec shared across two functions converts both to PEP 695 `**P` syntax.""" + subject = create_type_var_check(""" + from typing import ParamSpec + + def a(f: Callable[P, int]) -> Callable[P, int]: + return f + def b(f: Callable[P, str]) -> Callable[P, str]: + return f + + P = ParamSpec("P") + """) + result = subject.convert_declared_typevars() + + assert_that(result, has_entry("P", "fixed")) + assert_that(subject.apply_to_string(), contains_string("def a[**P]")) + assert_that(subject.apply_to_string(), contains_string("def b[**P]")) + + def test_converts_typevartuple(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """AI: Verify a TypeVarTuple converts to PEP 695 `*Ts` syntax.""" + subject = create_type_var_check(""" + from typing import TypeVarTuple + + def a(*args: *Ts) -> tuple[*Ts]: + return args + def b(*args: *Ts) -> tuple[*Ts]: + return args + + Ts = TypeVarTuple("Ts") + """) + result = subject.convert_declared_typevars() + + assert_that(result, has_entry("Ts", "fixed")) + assert_that(subject.apply_to_string(), contains_string("def a[*Ts]")) + + def test_does_not_convert_typevar_used_in_generic_base(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """AI: Verify a TypeVar also used in a class's Generic[...] base is left unconverted, marked unsafe.""" + subject = create_type_var_check(""" + from typing import TypeVar, Generic + + def a(x: T) -> T: + return x + def b(y: T) -> T: + return y + + class Box(Generic[T]): + pass + + T = TypeVar("T") + """) + result = subject.convert_declared_typevars() + + assert_that(result, has_entry("T", "unsafe")) + assert_that(subject.converted_unsafe_reasons, has_entry("T", UnsafeReason.USED_OUTSIDE_FUNCTION)) + assert_that(subject.apply_to_string(), contains_string('T = TypeVar("T")')) + + def test_does_not_convert_typevar_in_dunder_all(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """AI: Verify a TypeVar exported via __all__ is left unconverted, marked unsafe.""" + subject = create_type_var_check(""" + from typing import TypeVar + + __all__ = ["T"] + + def a(x: T) -> T: + return x + def b(y: T) -> T: + return y + + T = TypeVar("T") + """) + result = subject.convert_declared_typevars() + + assert_that(result, has_entry("T", "unsafe")) + assert_that(subject.converted_unsafe_reasons, has_entry("T", UnsafeReason.DECLARED_TYPEVAR_EXPORTED)) + assert_that(subject.apply_to_string(), contains_string('T = TypeVar("T")')) + + def test_does_not_convert_typevar_imported_elsewhere_in_project(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """A TypeVar imported directly by another project file is reported unsafe and not converted, even without __all__.""" + subject = create_type_var_check(""" + from typing import TypeVar + + def a(x: T) -> T: + return x + def b(y: T) -> T: + return y + + T = TypeVar("T") + """) + subject.project_wide_imported_names = frozenset({"T"}) + result = subject.convert_declared_typevars() + + assert_that(result, has_entry("T", "unsafe")) + assert_that(subject.converted_unsafe_reasons, has_entry("T", UnsafeReason.IMPORTED_ELSEWHERE_IN_PROJECT)) + assert_that(subject.apply_to_string(), contains_string('T = TypeVar("T")')) + + def test_removes_declaration_but_keeps_import_used_by_other_typevar(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """AI: Verify removing one converted TypeVar's declaration keeps the shared import alive for an unsafe sibling.""" + # T is multi-scope and safe to convert; U is left alone (used in a Generic[...] base), + # so the shared "from typing import TypeVar" import must survive for U's sake. + subject = create_type_var_check(""" + from typing import TypeVar, Generic + + def a(x: T) -> T: + return x + def b(y: T) -> T: + return y + + class Box(Generic[U]): + pass + + T = TypeVar("T") + U = TypeVar("U") + """) + result = subject.convert_declared_typevars() + + assert_that(result, has_entry("T", "fixed")) + assert_that(result, has_entry("U", "unsafe")) + output = subject.apply_to_string() + assert_that(output, contains_string("from typing import TypeVar")) + assert_that(output, contains_string('U = TypeVar("U")')) + assert_that(output, not_(contains_string("T = TypeVar"))) + + def test_converts_single_scope_typevar_without_ruff(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """AI: Verify a TypeVar used by a single function still converts even without a ruff-style leftover.""" + subject = create_type_var_check(""" + from typing import TypeVar + + T = TypeVar('T') + + def b(x: T) -> T: + return x + """) + 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:")) + + def test_converts_function_preserving_internal_comments(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """AI: Verify converting a signature never touches or drops a comment inside its body.""" + # Converting a function's signature must never touch or drop a comment in its body. + subject = create_type_var_check(""" + from typing import TypeVar + + def b(x: T) -> T: + # this explains something non-obvious + return x + + 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("# this explains something non-obvious")) + + 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.""" + # Converting a function's signature must never reformat or collapse its body. + 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.""" + # A nested closure merely referencing an enclosing function's type param must not get + # its own shadowing type param - PEP 695 params 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_preserves_multiline_signature_formatting(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """AI: Verify converting a multi-line signature doesn't collapse it onto one line.""" + # Converting a multi-line signature must not collapse it onto one line. + subject = create_type_var_check(""" + from typing import TypeVar + + def b( + x: T, + y: int = 1, + *, + z: str | None = None, + ) -> T: + return x + + 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](\n")) + assert_that(output, contains_string(" x: T,\n")) + assert_that(output, contains_string(" y: int = 1,\n")) + assert_that(output, contains_string(" *,\n")) + assert_that(output, contains_string(" z: str | None = None,\n")) + # would appear if the signature got collapsed onto one line, like ast.unparse() does by default + assert_that(output, not_(contains_string("def b[T](x: T"))) + + 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.""" + # Regression test: a function that already declares one PEP 695 type parameter must gain + # the new one inside the same bracket, not a second bracket next to it. + 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.""" + # A decorated function's "def" line isn't flush at column 0 like an undecorated one's - + # it's a continuation line carrying its own real 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_two_type_params_sharing_one_import_without_corrupting_it( + self, create_type_var_check: Callable[[str], TypeVarCheck] + ) -> None: + """AI: Verify converting two names sharing one import leaves that import line untouched.""" + # Converting two names sharing one import must leave that import line untouched - the + # recipe never edits it itself (ruff's F401 owns that). + 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.convert_declared_typevars() + + assert_that(result, has_entry("P", "fixed")) + assert_that(result, has_entry("T", "fixed")) + output = subject.apply_to_string() + ast.parse(output) # raises SyntaxError if the shared import got corrupted + assert_that(output, contains_string("from typing import ParamSpec, TypeVar")) + assert_that(output, not_(contains_string("P = ParamSpec"))) + assert_that(output, not_(contains_string("T = TypeVar"))) + + def test_version_gate_below_pep695_reports_unsafe_with_reason( + self, make_recipe: Callable[[type[PythonRefactoring], str], PythonRefactoring] + ) -> None: + """AI: Verify a target below the PEP 695 floor reports unsafe with the version-gate reason.""" + code = """ + from typing import TypeVar + + def a(x: T) -> T: + return x + + T = TypeVar("T") + """ + subject = cast(TypeVarCheck, make_recipe(TypeVarCheck, code)) + subject.min_python = (3, 10) + + result = subject.convert_declared_typevars() + + assert_that(result, has_entry("T", "unsafe")) + assert_that(subject.converted_unsafe_reasons, has_entry("T", UnsafeReason.PEP695_VERSION_GATE)) + assert_that(subject.apply_to_string(), contains_string('T = TypeVar("T")')) + + @pytest.mark.xfail( + reason="_renormalize_indent takes the body's minimum indent over every line, including the lines " + "inside a multi-line string literal, so the literal's value changes.", + raises=AssertionError, + strict=True, + ) + 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")) 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..3ca37c5f --- /dev/null +++ b/test/recipes/test_type_var_check_localize.py @@ -0,0 +1,349 @@ +"""Tests for TypeVarCheck.localize_imported_typevars.""" + +import textwrap +from pathlib import Path + +import pytest +from hamcrest import assert_that, contains_string, has_entry, is_, not_ +from pytest_mock import MockerFixture + +from renaissance.integrations.python.ast.rst_node import PythonRstNode +from renaissance.recipes.type_var_check import PEP_695_MINIMUM, TypeVarCheck +from renaissance.recipes.type_var_domain import UnsafeReason + + +class TestTypeVarCheckLocalize: + """See module docstring.""" + + def _create_cross_file(self, mocker: MockerFixture, tmp_path: Path, origin_text: str, importing_text: str) -> TypeVarCheck: + """Write origin_text to file_1.py and return an in-memory TypeVarCheck on file_2.py holding importing_text.""" + (tmp_path / "file_1.py").write_text(textwrap.dedent(origin_text)) + + importing_code = textwrap.dedent(importing_text) + importing_file = str(tmp_path / "file_2.py") + mocker.patch( + "renaissance.integrations.python.ast.factory.PythonFactory.create", + return_value=PythonRstNode.load_from_text(importing_code, importing_file), + ) + subject = TypeVarCheck(importing_file) + subject.in_memory = True + subject.min_python = PEP_695_MINIMUM + return subject + + def test_localizes_plain_function_generic_typevar(self, mocker: MockerFixture, tmp_path: Path) -> None: + """AI: Verify an imported TypeVar used only inside a plain function localizes into the importing file.""" + 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 + """, + ) + result = subject.localize_imported_typevars() + + assert_that(result, has_entry("T", "fixed")) + assert_that(subject.apply_to_string(), contains_string("T = TypeVar('T')")) + assert_that(subject.apply_to_string(), not_(contains_string("from file_1 import T"))) + + def test_does_not_localize_typevar_in_dunder_all(self, mocker: MockerFixture, tmp_path: Path) -> None: + """AI: Verify an origin-file TypeVar exported via __all__ is left cross-file unlocalized, marked unsafe.""" + subject = self._create_cross_file( + mocker, + tmp_path, + """ + from typing import TypeVar + __all__ = ["T"] + T = TypeVar("T") + def a(x: T) -> T: + return x + """, + """ + from file_1 import T + def b(x: T) -> T: + return x + """, + ) + result = subject.localize_imported_typevars() + + assert_that(result, has_entry("T", "unsafe")) + assert_that(subject.cross_file_unsafe_reasons, has_entry("T", UnsafeReason.ORIGIN_MODULE_EXPORTS_NAME)) + assert_that(subject.apply_to_string(), contains_string("from file_1 import T")) + + def test_does_not_localize_typevar_used_in_exported_generic_base(self, mocker: MockerFixture, tmp_path: Path) -> None: + """AI: Verify an origin-file TypeVar used in an exported Generic[...] base is left cross-file unlocalized.""" + subject = self._create_cross_file( + mocker, + tmp_path, + """ + from typing import TypeVar, Generic + T = TypeVar("T") + class Box(Generic[T]): + pass + """, + """ + from file_1 import T + def b(x: T) -> T: + return x + """, + ) + result = subject.localize_imported_typevars() + + assert_that(result, has_entry("T", "unsafe")) + assert_that(subject.cross_file_unsafe_reasons, has_entry("T", UnsafeReason.USED_IN_EXPORTED_GENERIC_BASE)) + assert_that(subject.apply_to_string(), contains_string("from file_1 import T")) + + def test_keeps_other_names_when_localizing_one_of_several_imports(self, mocker: MockerFixture, tmp_path: Path) -> None: + """AI: Verify localizing one imported name from a multi-name import statement keeps the other names imported.""" + subject = self._create_cross_file( + mocker, + tmp_path, + """ + from typing import TypeVar + T = TypeVar("T") + def helper() -> None: + pass + """, + """ + from file_1 import T, helper + def b(x: T) -> T: + helper() + return x + """, + ) + result = subject.localize_imported_typevars() + + assert_that(result, has_entry("T", "fixed")) + output = subject.apply_to_string() + assert_that(output, contains_string("from file_1 import helper")) + assert_that(output, contains_string("T = TypeVar('T')")) + + @pytest.mark.xfail( + reason="localize_imported_typevars queues one replace per localized name on the same import " + "statement, which the rewriter rejects as conflicting rewrites.", + raises=ValueError, + strict=True, + ) + def test_localizes_two_names_from_one_import_statement(self, mocker: MockerFixture, tmp_path: Path) -> None: + """Verify one import statement bringing in two localizable names localizes both.""" + subject = self._create_cross_file( + mocker, + tmp_path, + """ + from typing import TypeVar + T = TypeVar("T") + U = TypeVar("U") + """, + """ + from file_1 import T, U + def b(x: T, y: U) -> T: + return x + """, + ) + result = subject.localize_imported_typevars() + + assert_that(result, has_entry("T", "fixed")) + assert_that(result, has_entry("U", "fixed")) + output = subject.apply_to_string() + assert_that(output, contains_string("T = TypeVar('T')")) + assert_that(output, contains_string("U = TypeVar('U')")) + assert_that(output, not_(contains_string("from file_1 import"))) + + def test_adds_missing_typevar_import_when_localizing(self, mocker: MockerFixture, tmp_path: Path) -> None: + """AI: Verify localizing a TypeVar adds the "from typing import TypeVar" import if missing.""" + 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 + """, + ) + result = subject.localize_imported_typevars() + + assert_that(result, has_entry("T", "fixed")) + assert_that(subject.apply_to_string(), contains_string("from typing import TypeVar")) + + def test_does_not_duplicate_already_present_typevar_import(self, mocker: MockerFixture, tmp_path: Path) -> None: + """AI: Verify localizing a TypeVar doesn't add a duplicate "from typing import TypeVar" when one already exists.""" + subject = self._create_cross_file( + mocker, + tmp_path, + """ + from typing import TypeVar + T = TypeVar("T") + def a(x: T) -> T: + return x + """, + """ + from typing import TypeVar + from file_1 import T + U = TypeVar("U") + def b(x: T) -> T: + return x + """, + ) + result = subject.localize_imported_typevars() + + assert_that(result, has_entry("T", "fixed")) + output = subject.apply_to_string() + assert_that(output.count("from typing import TypeVar"), is_(1)) + + def test_localizes_when_origin_brings_typevar_into_scope_via_wildcard_import( + self, + mocker: MockerFixture, + tmp_path: Path, + ) -> None: + """AI: Verify localizing still succeeds when the origin brings TypeVar into scope via a wildcard import.""" + # find_import_source can't locate "TypeVar" here - safe only because the importing file + # already imports it itself. + subject = self._create_cross_file( + mocker, + tmp_path, + """ + from typing import * + T = TypeVar("T") + def a(x: T) -> T: + return x + """, + """ + from typing import TypeVar + from file_1 import T + def b(x: T) -> T: + return x + """, + ) + result = subject.localize_imported_typevars() + + assert_that(result, has_entry("T", "fixed")) + output = subject.apply_to_string() + assert_that(output.count("from typing import TypeVar"), is_(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_localizes_project_wide_import_from_different_directory(self, mocker: MockerFixture, tmp_path: Path) -> None: + """A TypeVar imported from a module in a parent directory is localized when project_root is set.""" + (tmp_path / "file_1.py").write_text( + textwrap.dedent(""" + from typing import TypeVar + T = TypeVar("T") + def a(x: T) -> T: + return x + """), + ) + sub = tmp_path / "sub" + sub.mkdir() + importing_code = textwrap.dedent(""" + from file_1 import T + def b(x: T) -> T: + return x + """) + importing_file = str(sub / "file_2.py") + mocker.patch( + "renaissance.integrations.python.ast.factory.PythonFactory.create", + return_value=PythonRstNode.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 + + result = subject.localize_imported_typevars() + + assert_that(result, has_entry("T", "fixed")) + assert_that(subject.apply_to_string(), contains_string("T = TypeVar('T')")) + assert_that(subject.apply_to_string(), not_(contains_string("from file_1 import T"))) + + 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, is_({})) + + def test_check_localizes_and_converts_in_one_pass(self, mocker: MockerFixture, tmp_path: Path) -> None: + """AI: Verify check() localizes a cross-file TypeVar and converts it to PEP 695 in the same run.""" + # Whole-pipeline integration, grouped here since cross-file localization is what + # sets this case apart from the plain-conversion tests in test_type_var_check_convert.py. + 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.run() + + assert_that(subject.result["cross_file"], has_entry("T", "fixed")) + assert_that(subject.result["converted"], has_entry("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"))) + assert_that(output, contains_string("from typing import TypeVar")) 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..319be3d1 --- /dev/null +++ b/test/recipes/test_type_var_check_orphaned.py @@ -0,0 +1,141 @@ +"""Tests for TypeVarCheck.remove_orphaned_declarations.""" + +from collections.abc import Callable + +from hamcrest import assert_that, contains_string, has_entry, has_key, is_not, not_ + +from renaissance.recipes.type_var_check import TypeVarCheck +from renaissance.recipes.type_var_domain import UnsafeReason + + +class TestTypeVarCheckOrphaned: + """See module docstring.""" + + def test_removes_orphaned_declaration_after_manual_or_ruff_pep695_conversion( + self, create_type_var_check: Callable[[str], TypeVarCheck] + ) -> None: + """AI: Verify a TypeVar declaration left orphaned by a manual/ruff PEP 695 conversion is removed.""" + subject = create_type_var_check(""" + from typing import TypeVar + T = TypeVar('T') + + 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("def b[T](x: T) -> T:")) + assert_that(output, not_(contains_string("T = TypeVar"))) + assert_that(output, contains_string("from typing import TypeVar")) + + def test_removes_fully_unused_declaration(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """AI: Verify a TypeVar declaration with no references anywhere is removed.""" + subject = create_type_var_check(""" + from typing import TypeVar + T = TypeVar('T') + + def b() -> None: + pass + """) + result = subject.remove_orphaned_declarations() + + assert_that(result, has_entry("T", "fixed")) + output = subject.apply_to_string() + assert_that(output, not_(contains_string("T = TypeVar"))) + assert_that(output, contains_string("from typing import TypeVar")) + + def test_does_not_touch_declaration_still_live_outside_shadow(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """AI: Verify a TypeVar declaration still live in an un-shadowed function is left untouched, unflagged.""" + 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, is_not(has_key("T"))) + assert_that(subject.apply_to_string(), contains_string("T = TypeVar('T')")) + + def test_does_not_remove_declaration_used_in_generic_base(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """AI: Verify a TypeVar declaration also used in a Generic[...] base is left untouched, unflagged.""" + # The Generic[T] base is a real, non-shadowed use, so this is never even flagged - + # same as any other still-live declaration. + subject = create_type_var_check(""" + from typing import TypeVar, Generic + T = TypeVar('T') + + class Box(Generic[T]): + pass + + def b[T](x: T) -> T: + return x + """) + result = subject.remove_orphaned_declarations() + + assert_that(result, is_not(has_key("T"))) + 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: + """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') + + def b[T](x: T, y: U) -> 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')")) + + def test_does_not_remove_orphaned_declaration_imported_elsewhere_in_project( + self, create_type_var_check: Callable[[str], TypeVarCheck], + ) -> None: + """An orphaned declaration imported directly by another project file is reported unsafe and kept.""" + subject = create_type_var_check(""" + from typing import TypeVar + T = TypeVar('T') + + def b[T](x: T) -> T: + return x + """) + subject.project_wide_imported_names = frozenset({"T"}) + result = subject.remove_orphaned_declarations() + + assert_that(result, has_entry("T", "unsafe")) + assert_that(subject.orphaned_unsafe_reasons, has_entry("T", UnsafeReason.IMPORTED_ELSEWHERE_IN_PROJECT)) + assert_that(subject.apply_to_string(), contains_string("T = TypeVar('T')")) + + def test_does_not_remove_orphaned_declaration_in_dunder_all(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """AI: Verify an orphaned but __all__-exported TypeVar declaration is reported unsafe, not removed.""" + # Every reference is shadowed, but T is still exported public API via __all__, so + # removing the declaration would break importers - flagged "unsafe", not silently fixed. + subject = create_type_var_check(""" + from typing import TypeVar + + __all__ = ["T"] + + T = TypeVar('T') + + def b[T](x: T) -> T: + return x + """) + result = subject.remove_orphaned_declarations() + + assert_that(result, has_entry("T", "unsafe")) + assert_that(subject.orphaned_unsafe_reasons, has_entry("T", UnsafeReason.DECLARED_TYPEVAR_EXPORTED)) + assert_that(subject.apply_to_string(), contains_string("T = TypeVar('T')")) 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..c3d1b299 --- /dev/null +++ b/test/recipes/test_type_var_check_properties.py @@ -0,0 +1,31 @@ +"""Property-based tests for TypeVarCheck.check.""" + +import ast +from unittest.mock import patch + +import hypothesmith +from hypothesis import assume, given, settings + +from renaissance.integrations.python.ast.rst_node import PythonRstNode +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: + assume(False) + + with patch( + "renaissance.integrations.python.ast.factory.PythonFactory.create", + return_value=PythonRstNode.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..c16c2609 --- /dev/null +++ b/test/recipes/test_type_var_domain.py @@ -0,0 +1,178 @@ +"""Tests for type_var_domain's safety predicates and their UnsafeReason results.""" + +import ast +import textwrap + +import pytest +from hamcrest import assert_that, is_ + +from renaissance.recipes.type_var_domain import ( + UnsafeReason, + find_type_param_declarations, + is_safe_to_convert, + is_safe_to_localize, +) + + +def _parse(source: str) -> ast.Module: + return ast.parse(textwrap.dedent(source)) + + +class TestIsSafeToConvert: + """is_safe_to_convert: None when safe, the specific UnsafeReason otherwise.""" + + def test_returns_none_when_safe(self) -> None: + """A TypeVar used only inside functions, not exported, is safe to convert.""" + tree = _parse(""" + from typing import TypeVar + + def a(x: T) -> T: + return x + + T = TypeVar("T") + """) + decl_stmt = find_type_param_declarations(tree)["T"] + + assert_that(is_safe_to_convert(tree, "T", decl_stmt), is_(None)) + + @pytest.mark.parametrize( + ("source", "expected_reason"), + [ + ( + """ + from typing import TypeVar + + __all__ = ["T"] + + def a(x: T) -> T: + return x + + T = TypeVar("T") + """, + UnsafeReason.DECLARED_TYPEVAR_EXPORTED, + ), + ( + """ + from typing import TypeVar, Generic + + def a(x: T) -> T: + return x + + class Box(Generic[T]): + pass + + T = TypeVar("T") + """, + UnsafeReason.USED_OUTSIDE_FUNCTION, + ), + ], + ) + 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) + decl_stmt = find_type_param_declarations(tree)["T"] + + assert_that(is_safe_to_convert(tree, "T", decl_stmt), is_(expected_reason)) + + +class TestIsSafeToLocalize: + """is_safe_to_localize: None when safe, the specific UnsafeReason otherwise.""" + + def test_returns_none_when_safe(self) -> None: + """A TypeVar not exported and not used in a Generic[...] base is safe to localize.""" + tree = _parse(""" + from typing import TypeVar + + T = TypeVar("T") + + def a(x: T) -> T: + return x + """) + + assert_that(is_safe_to_localize(tree, "T"), is_(None)) + + def test_ignores_non_generic_subscripted_base_and_plain_base(self) -> None: + """AI: Verify a TypeVar used only as a subscript of a non-Generic base stays safe to localize.""" + tree = _parse(""" + from typing import TypeVar + from collections.abc import Mapping + + T = TypeVar("T") + + class Plain(object): + pass + + class Box(Mapping[T]): + pass + """) + + assert_that(is_safe_to_localize(tree, "T"), is_(None)) + + @pytest.mark.parametrize( + ("source", "expected_reason"), + [ + ( + """ + from typing import TypeVar + + __all__ = ["T"] + + T = TypeVar("T") + """, + UnsafeReason.ORIGIN_MODULE_EXPORTS_NAME, + ), + ( + """ + from typing import TypeVar, Generic + + T = TypeVar("T") + + class Box(Generic[T]): + pass + """, + UnsafeReason.USED_IN_EXPORTED_GENERIC_BASE, + ), + ( + """ + 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, + ), + ( + """ + try: + from typing import ParamSpec + except ImportError: + from typing_extensions import ParamSpec + + T = ParamSpec("T") + """, + UnsafeReason.ORIGIN_IMPORTS_CONSTRUCTOR_CONDITIONALLY, + ), + ], + ) + 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"), is_(expected_reason)) + + def test_ignores_conditional_imports_of_other_names(self) -> None: + """A conditional import of an unrelated name doesn't make an unconditionally imported constructor unsafe.""" + tree = _parse(""" + from typing import TYPE_CHECKING, TypeVar + + if TYPE_CHECKING: + from collections.abc import Sequence + + T = TypeVar("T") + """) + + assert_that(is_safe_to_localize(tree, "T"), is_(None)) diff --git a/test/rejuvenation/test_migration_type_recipes.py b/test/rejuvenation/test_migration_type_recipes.py new file mode 100644 index 00000000..dacf4885 --- /dev/null +++ b/test/rejuvenation/test_migration_type_recipes.py @@ -0,0 +1,444 @@ +"""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, has_entry, is_, is_not +from hamcrest.core.matcher import Matcher + +from renaissance.project.project_scanner import PythonScanner +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 TestResolveTargetFiles: + """resolve_target_files: single-file shortcut, otherwise delegates to PythonScanner.""" + + def test_single_file_returned_as_is(self, tmp_path: Path) -> None: + """A single .py file path (not a directory) is returned as a one-item list.""" + target = tmp_path / "solo.py" + target.write_text("x = 1\n") + + result = migration.resolve_target_files(target) + + assert_that(result, equal_to([target])) + + def test_directory_target_delegates_to_python_scanner(self, tmp_path: Path) -> None: + """A directory target is scanned via PythonScanner, wrapping each result back into a Path.""" + (tmp_path / "pkg").mkdir() + (tmp_path / "pkg" / "a.py").write_text("x = 1\n") + (tmp_path / "pkg" / "b.py").write_text("y = 2\n") + + result = migration.resolve_target_files(tmp_path) + + assert_that(result, equal_to([Path(p) for p in PythonScanner(str(tmp_path)).find_sources()])) + assert_that(all(isinstance(path, Path) for path in result), is_(True)) + + +class TestClassification: + """has_fixed/has_unsafe/is_clean: classification predicates over a FileReport.""" + + @pytest.mark.parametrize( + ("result", "expected"), + [ + ({"cross_file": {}, "converted": {"T": "fixed"}, "orphaned": {}}, (True, False, False)), + ({"cross_file": {}, "converted": {"T": "unsafe"}, "orphaned": {}}, (False, True, False)), + ( + {"cross_file": {}, "converted": {"T": "fixed", "U": "unsafe"}, "orphaned": {}}, + (True, True, False), + ), + ({"cross_file": {}, "converted": {}, "orphaned": {}}, (False, False, True)), + ], + ) + def test_predicates( + self, + result: dict[str, dict[str, str]], + expected: tuple[bool, bool, bool], + ) -> None: + """Each predicate matches the expected (fixed, unsafe, clean) reading of `result`.""" + expected_fixed, expected_unsafe, expected_clean = expected + report = migration.FileReport(path=Path("x.py"), result=result, error=None) + + assert_that(migration.has_fixed(report), is_(expected_fixed)) + assert_that(migration.has_unsafe(report), is_(expected_unsafe)) + assert_that(migration.is_clean(report), is_(expected_clean)) + + def test_error_report_is_neither_fixed_unsafe_nor_clean(self) -> None: + """A report with no result (an error occurred) is False for every predicate.""" + report = migration.FileReport(path=Path("x.py"), result=None, error="boom") + + assert_that(migration.has_fixed(report), is_(False)) + assert_that(migration.has_unsafe(report), is_(False)) + assert_that(migration.is_clean(report), is_(False)) + + +class TestProcessFile: + """process_file: writes changes for real, and isolates per-file errors.""" + + def test_writes_migrated_content_to_disk(self, tmp_path: Path) -> None: + """process_file() actually writes the PEP 695-converted content to disk.""" + target = tmp_path / "mod.py" + target.write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") + + report = migration.process_file(target, min_python=(3, 12), project_root=tmp_path, project_wide_imported_names=frozenset()) + + assert_that(migration.has_fixed(report), is_(True)) + assert_that(target.read_text(encoding="utf-8"), contains_string("def identity[T]")) + + def test_unsafe_typevar_reported_but_not_written(self, tmp_path: Path) -> None: + """A TypeVar exported via __all__ is reported unsafe and the file is left untouched.""" + target = tmp_path / "mod.py" + target.write_text(UNSAFE_TYPEVAR_SOURCE, encoding="utf-8") + original = target.read_text(encoding="utf-8") + + report = migration.process_file(target, min_python=(3, 12), project_root=tmp_path, project_wide_imported_names=frozenset()) + + assert_that(migration.has_unsafe(report), is_(True)) + assert_that(target.read_text(encoding="utf-8"), equal_to(original)) + + def test_unsafe_typevar_reason_is_recorded(self, tmp_path: Path) -> None: + """The specific UnsafeReason (not just the "unsafe" status) is recorded per name.""" + target = tmp_path / "mod.py" + target.write_text(UNSAFE_TYPEVAR_SOURCE, encoding="utf-8") + + report = migration.process_file(target, min_python=(3, 12), project_root=tmp_path, project_wide_imported_names=frozenset()) + + assert_that(report.reasons, is_not(None)) + assert_that(report.reasons["converted"], has_entry("T", UnsafeReason.DECLARED_TYPEVAR_EXPORTED)) + + def test_project_wide_imported_name_is_reported_unsafe_even_without_dunder_all(self, tmp_path: Path) -> None: + """A name imported directly by another passed-in file is left alone, __all__ or not.""" + target = tmp_path / "mod.py" + target.write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") + + report = migration.process_file(target, min_python=(3, 12), project_root=tmp_path, project_wide_imported_names=frozenset({"T"})) + + assert_that(migration.has_unsafe(report), is_(True)) + assert_that(report.reasons["converted"], has_entry("T", UnsafeReason.IMPORTED_ELSEWHERE_IN_PROJECT)) + assert_that(target.read_text(encoding="utf-8"), 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()) + + assert_that(report.error, is_not(None)) + assert_that(report.result, is_(None)) + + +class TestRuffImportCleanup: + """main(): the ruff F401 batch step actually drops now-unused imports end to end.""" + + @pytest.mark.parametrize( + ("extra_args", "import_matcher"), + [ + pytest.param([], is_not(contains_string("TypeVar")), id="default-drops-import"), + pytest.param(["--no-ruff"], contains_string("from typing import TypeVar"), id="no-ruff-keeps-import"), + ], + ) + def test_unused_typevar_import_cleanup( + self, + tmp_path: Path, + extra_args: list[str], + import_matcher: Matcher[str], + ) -> None: + """The redundant TypeVar import is dropped by default and kept with --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) + + @pytest.mark.parametrize( + ("extra_args", "report_matcher"), + [ + pytest.param([], contains_string("cleaned up via `ruff"), id="default-mentions-ruff"), + pytest.param(["--no-ruff"], is_not(contains_string("cleaned up via `ruff")), id="no-ruff-omits-mention"), + ], + ) + def test_report_mentions_ruff_cleanup_only_when_it_ran( + self, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], + extra_args: list[str], + report_matcher: Matcher[str], + ) -> None: + """The report's ruff cleanup line appears only when the ruff pass actually ran.""" + target = tmp_path / "mod.py" + target.write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") + + migration.main([str(target), "--py", "3.12", *extra_args]) + + 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))) + + def test_no_link_printed_for_modified_files_section(self, tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None: + """A fixed name (no reason attached) never gets a doc link line.""" + target = tmp_path / "mod.py" + target.write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") + + migration.main([str(target), "--py", "3.12"]) + + output = capsys.readouterr().out + assert_that(output, is_not(contains_string("tno.github.io"))) + + +class TestPerFileProgressFeedback: + """main(): prints a per-file progress line as each file is checked.""" + + def test_each_file_gets_a_checked_line(self, tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None: + """Every discovered file - modified, clean, or errored - gets its own 'checked' line.""" + 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") + + migration.main([str(tmp_path), "--py", "3.12"]) + + output = capsys.readouterr().out + assert_that(output, contains_string(f"File {good} checked.")) + assert_that(output, contains_string(f"File {broken} 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, is_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 reports the good file, and exits with code 3.""" + (tmp_path / "good.py").write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") + (tmp_path / "broken.py").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("good.py")) + assert_that(output, contains_string("broken.py")) + + +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, is_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:")) + + @pytest.mark.parametrize( + "consumer_source", + [ + pytest.param("import pkg.typing_mod\n\ndef use(x: pkg.typing_mod.T) -> None: ...\n", id="import-dotted"), + pytest.param("import pkg.typing_mod as tm\n\ndef use(x: tm.T) -> None: ...\n", id="import-as"), + pytest.param("from pkg import typing_mod\n\ndef use(x: typing_mod.T) -> None: ...\n", id="from-package"), + pytest.param("from . import typing_mod\n\ndef use(x: typing_mod.T) -> None: ...\n", id="from-relative"), + ], + ) + def test_origin_declaration_survives_module_attribute_access(self, tmp_path: Path, consumer_source: str) -> 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(consumer_source, 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, is_not(contains_string(unexpected))) + + @pytest.mark.parametrize( + "bad_args", + [ + pytest.param([], id="flag-missing"), + pytest.param(["--py", "3"], id="missing-minor"), + pytest.param(["--py", "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], + ) -> None: + """A missing or malformed --py flag exits with code 2 and a usage line naming --py.""" + target = tmp_path / "mod.py" + target.write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") + original = target.read_text(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("--py MAJOR.MINOR")) + assert_that(target.read_text(encoding="utf-8"), equal_to(original)) diff --git a/test/utils/test_import_resolution.py b/test/utils/test_import_resolution.py new file mode 100644 index 00000000..b5712ca6 --- /dev/null +++ b/test/utils/test_import_resolution.py @@ -0,0 +1,159 @@ +"""Tests for renaissance.utils.import_resolution.""" + +from pathlib import Path + +import pytest +from hamcrest import assert_that, has_entry, has_item, has_key, is_, is_not + +from renaissance.utils.import_resolution import collect_project_imported_names, 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), is_(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), is_(None)) + + +class TestCollectProjectImportedNames: + """See module docstring.""" + + def test_maps_absolute_import_to_origin_file(self, project_tree: Path) -> None: + """A name imported by one file is recorded against the file it is imported from.""" + files = [project_tree / "redis" / "typing.py", project_tree / "redis" / "commands" / "core.py"] + result = collect_project_imported_names(files, project_tree) + assert_that(result, has_entry(project_tree / "redis" / "typing.py", frozenset({"AnyKeyT"}))) + + def test_records_original_name_not_alias(self, tmp_path: Path) -> None: + """An aliased import is recorded under the name declared in the origin module.""" + (tmp_path / "origin.py").write_text("X = 1\n") + (tmp_path / "consumer.py").write_text("from origin import X as Z\n") + files = [tmp_path / "origin.py", tmp_path / "consumer.py"] + result = collect_project_imported_names(files, tmp_path) + assert_that(result, has_entry(tmp_path / "origin.py", frozenset({"X"}))) + + def test_does_not_record_stdlib_import(self, tmp_path: Path) -> None: + """An import that doesn't resolve inside the project is not recorded.""" + (tmp_path / "consumer.py").write_text("from typing import TypeVar\n") + files = [tmp_path / "consumer.py"] + result = collect_project_imported_names(files, tmp_path) + assert_that(result, is_({})) + + def test_unrelated_same_name_in_two_files_does_not_collide(self, tmp_path: Path) -> None: + """Two unrelated files declaring the same name, with no imports between them, record nothing.""" + (tmp_path / "a.py").write_text("T = 1\n") + (tmp_path / "b.py").write_text("T = 2\n") + files = [tmp_path / "a.py", tmp_path / "b.py"] + result = collect_project_imported_names(files, tmp_path) + assert_that(result, is_({})) + + +@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(result, has_entry(module_tree / origin_rel, 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, is_not(has_key(module_tree / "pkg" / "mod.py"))) diff --git a/test/utils/test_unparse_utils.py b/test/utils/test_unparse_utils.py new file mode 100644 index 00000000..f08ca4bc --- /dev/null +++ b/test/utils/test_unparse_utils.py @@ -0,0 +1,175 @@ +"""Tests for the signature-only PEP 695 bracket-splice helpers.""" + +import ast +import textwrap +from typing import cast + +from hamcrest import assert_that, contains_string, is_ + +from renaissance.utils.unparse_utils import ( + _bracket_end_offset, # pyright: ignore[reportPrivateUsage] + _header_end_line, # pyright: ignore[reportPrivateUsage] + _name_end_offset, # pyright: ignore[reportPrivateUsage] + _type_params_bracket, # pyright: ignore[reportPrivateUsage] + unparse_signature_only, +) + + +class TestNameEndOffset: + """See module docstring.""" + + def test_finds_a_plain_def(self) -> None: + """AI: Verify the offset right after the function name in an undecorated "def" line.""" + assert_that(_name_end_offset("def f(x: int) -> int:\n return x\n", "f"), is_(5)) + + def test_finds_an_async_def(self) -> None: + """AI: Verify the offset right after the function name in an "async def" line.""" + source = "async def g(x: int) -> int:\n return x\n" + assert_that(_name_end_offset(source, "g"), is_(11)) + + def test_finds_a_def_indented_after_a_decorator(self) -> None: + """AI: Verify the offset is found correctly when the "def" line is indented after a decorator.""" + # A decorated method's .text includes the decorator on line 1 - the "def" line itself + # is a continuation line carrying its own real indentation, not flush at column 0. + source = "@overload\n def __call__(self, x: int) -> int: ...\n" + assert_that(_name_end_offset(source, "__call__"), is_(26)) + + def test_raises_when_name_not_found(self) -> None: + """AI: Verify a ValueError is raised when the function name doesn't appear in the source.""" + try: + _name_end_offset("x = 1\n", "f") + except ValueError: + return + raise AssertionError("expected ValueError") + + +class TestBracketEndOffset: + """See module docstring.""" + + def test_finds_a_simple_bracket(self) -> None: + """AI: Verify the closing bracket offset for a simple, unnested type-param bracket.""" + source = "def f[T](x: T) -> T:\n return x\n" + assert_that(_bracket_end_offset(source, 5), is_(8)) + + def test_tracks_a_nested_bracket_in_a_bound(self) -> None: + """AI: Verify the closing bracket offset tracks nesting depth correctly across a bound's own brackets.""" + source = "def f[T: list[int]](x: T) -> T:\n return x\n" + assert_that(_bracket_end_offset(source, 5), is_(19)) + + +class TestTypeParamsBracket: + """See module docstring.""" + + def test_no_type_params_returns_empty(self) -> None: + """AI: Verify a function with no type params produces an empty bracket string.""" + node = cast(ast.FunctionDef, ast.parse("def f(x): pass").body[0]) + assert_that(_type_params_bracket(node), is_("")) + + def test_one_type_param(self) -> None: + """AI: Verify a function with one type param produces a single-name bracket.""" + node = cast(ast.FunctionDef, ast.parse("def f(x): pass").body[0]) + node.type_params = [ast.TypeVar(name="T")] + assert_that(_type_params_bracket(node), is_("[T]")) + + def test_two_type_params(self) -> None: + """AI: Verify a function with two type params produces a comma-separated bracket, in order.""" + node = cast(ast.FunctionDef, ast.parse("def f(x): pass").body[0]) + node.type_params = [ast.TypeVar(name="U"), ast.TypeVar(name="T")] + assert_that(_type_params_bracket(node), is_("[U, T]")) + + +class TestHeaderEndLine: + """See module docstring.""" + + def test_one_line_signature(self) -> None: + """AI: Verify a one-line signature's header ends on line 1.""" + assert_that(_header_end_line("def f(x: int) -> int:\n return x\n"), is_(1)) + + def test_multi_line_signature(self) -> None: + """AI: Verify a multi-line signature's header ends on the line with the terminating colon.""" + source = "def f(\n a: int,\n b: str,\n) -> None:\n pass\n" + assert_that(_header_end_line(source), is_(4)) + + def test_ignores_colon_inside_a_string_default(self) -> None: + """AI: Verify a colon inside a string default value isn't mistaken for the header-terminating colon.""" + source = 'def f(\n b: str = "x:y",\n) -> None:\n pass\n' + assert_that(_header_end_line(source), is_(3)) + + def test_ignores_colon_inside_a_lambda_default(self) -> None: + """AI: Verify a lambda default value's colon isn't mistaken for the header-terminating colon.""" + source = "def f(cb=lambda: 1) -> int:\n return cb()\n" + assert_that(_header_end_line(source), is_(1)) + + def test_raises_when_no_header_terminating_colon(self) -> None: + """AI: Verify a ValueError is raised when the source has no header-terminating colon at all.""" + try: + _header_end_line("x = 1\n") + except ValueError: + return + raise AssertionError("expected ValueError") + + +class TestUnparseSignatureOnly: + """See module docstring.""" + + def test_preserves_a_body_comment(self) -> None: + """AI: Verify splicing a new type-param bracket into the header preserves a comment in the body.""" + original = textwrap.dedent("""\ + def f(x): + # explains something + return x + """) + node = cast(ast.FunctionDef, ast.parse(original).body[0]) + node.type_params = [ast.TypeVar(name="T")] + + result = unparse_signature_only(node, original) + + assert_that(result, contains_string("def f[T](x):")) + assert_that(result, contains_string("# explains something")) + + def test_renormalizes_a_method_bodys_absolute_indent_to_four_spaces(self) -> None: + """AI: Verify a method's real 8-space absolute body indent is renormalized to the 4-space baseline.""" + # A method's .text carries the file's real (absolute) indentation - here 8 spaces, one + # level of class plus one level of method body - not the 4-space-relative-to-zero + # baseline the rewrite pipeline's shift expects. + original = "def f(x):\n return x" + node = cast(ast.FunctionDef, ast.parse(original).body[0]) + node.type_params = [ast.TypeVar(name="T")] + + result = unparse_signature_only(node, original) + + assert_that(result, is_("def f[T](x):\n return x")) + + def test_preserves_an_inline_single_line_body(self) -> None: + """AI: Verify an inline "def f(x): ..." body stays on the header's own line after splicing.""" + # "def f(x): ..." keeps its body on the header's own line - there's no separate block + # to renormalize, and the original inline style should survive as-is. + original = "def f(x): ...\n" + node = cast(ast.FunctionDef, ast.parse(original).body[0]) + node.type_params = [ast.TypeVar(name="T")] + + result = unparse_signature_only(node, original) + + assert_that(result, is_("def f[T](x): ...\n")) + + def test_preserves_a_multiline_signature(self) -> None: + """AI: Verify splicing a type-param bracket doesn't collapse a multi-line parameter list onto one line.""" + # Regression test: unparse_signature_only used to regenerate the whole header via + # ast.unparse(), collapsing a multi-line parameter list onto one line. + original = "def f(\n x: int,\n y: int = 1,\n) -> int:\n return x\n" + node = cast(ast.FunctionDef, ast.parse(original).body[0]) + node.type_params = [ast.TypeVar(name="T")] + + result = unparse_signature_only(node, original) + + assert_that(result, is_("def f[T](\n x: int,\n y: int = 1,\n) -> int:\n return x\n")) + + def test_merges_into_an_existing_bracket(self) -> None: + """AI: Verify splicing a new type param into a header that already has one merges into the same bracket.""" + original = "def f[U](x: U, y):\n return x\n" + node = cast(ast.FunctionDef, ast.parse(original).body[0]) + node.type_params = [*node.type_params, ast.TypeVar(name="T")] + + result = unparse_signature_only(node, original) + + assert_that(result, is_("def f[U, T](x: U, y):\n return x\n")) diff --git a/uv.lock b/uv.lock index 084a2b41..aac11354 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" },