From a179f2e775a35798e76fafdf5cecd9c4252fa1bc Mon Sep 17 00:00:00 2001 From: Francesco Pezzella Date: Tue, 13 Oct 2026 15:21:33 +0200 Subject: [PATCH 01/25] Document six more TypeVarCheck non-goals and drop their TODOs and xfail test --- src/rejuvenation/migration-type-recipes.py | 3 --- src/renaissance/recipes/type_var_check.py | 14 -------------- src/renaissance/recipes/type_var_domain.py | 9 --------- src/renaissance/utils/import_resolution.py | 5 ----- test/recipes/test_type_var_check_convert.py | 20 -------------------- 5 files changed, 51 deletions(-) diff --git a/src/rejuvenation/migration-type-recipes.py b/src/rejuvenation/migration-type-recipes.py index 6837eb9c..98e43a0c 100644 --- a/src/rejuvenation/migration-type-recipes.py +++ b/src/rejuvenation/migration-type-recipes.py @@ -255,9 +255,6 @@ def main(argv: Sequence[str] | None = None) -> int: target = target.resolve() files = resolve_target_files(target) project_root = target if target.is_dir() else target.parent - # TODO(FrancescoPezzella): computed once upfront, so an origin whose importers all get localized this run is only converted - # on a second run. - # https://github.com/TNO/Renaissance.Py/issues/279 imported_names_by_file = collect_project_imported_names(files, project_root) reports: list[FileReport] = [] diff --git a/src/renaissance/recipes/type_var_check.py b/src/renaissance/recipes/type_var_check.py index d322b05e..8ed9a3c6 100644 --- a/src/renaissance/recipes/type_var_check.py +++ b/src/renaissance/recipes/type_var_check.py @@ -197,16 +197,11 @@ def remove_orphaned_declarations(self) -> dict[str, str]: if uses_of(root, name, decl_stmt): continue - # TODO(FrancescoPezzella): a public TypeVar no file in the project uses (e.g. redis.typing.AnyKeyT) is removed, - # though other projects may import it. - # https://github.com/TNO/Renaissance.Py/issues/279 reason = is_safe_to_remove(module, name, self.project_wide_imported_names) if reason is not None: self._mark_unsafe(results, self.orphaned_unsafe_reasons, name, reason) continue - # TODO(FrancescoPezzella): `T = U = TypeVar("T")` is removed whole when T is unused, so U is no longer defined. - # https://github.com/TNO/Renaissance.Py/issues/279 self._remove_declaration(decl_stmt) results[name] = "fixed" @@ -221,9 +216,6 @@ def _remove_declaration(self, decl_stmt: cst.Assign) -> None: """Remove decl_stmt's statement from the file.""" # TODO(FrancescoPezzella): enable once comment blocks get correctly deleted # https://github.com/TNO/Renaissance.Py/issues/189 - # TODO(FrancescoPezzella): a declaration that starts a `;` line leaves a leading `;`, a syntax error that commit() - # writes to disk. - # https://github.com/TNO/Renaissance.Py/issues/279 self.remove(self._cst_root().translation_unit.wrappers[decl_stmt], include_comments=False) def localize_imported_typevars(self) -> dict[str, str]: @@ -281,8 +273,6 @@ def _localize_names_from( needed_argument_names: set[str] = set() for alias in raw.names: - # TODO(FrancescoPezzella): `from x import T as U` is not currently being caught, tbd - # https://github.com/TNO/Renaissance.Py/issues/279 name = alias.evaluated_name if alias.asname is not None or name not in declarations: continue @@ -331,10 +321,6 @@ def _missing_constructor_import( ctor_name = type_param_constructor_name(decl_stmt) ctor_source = from_import_sources(origin).get(ctor_name) if ctor_source is None: - # TODO(FrancescoPezzella): an origin using `from typing import *` lands here, - # so a file not importing the constructor itself gets a NameError. - # TBD - needs a fix - # https://github.com/TNO/Renaissance.Py/issues/279 return None via_module = import_from_module(via) diff --git a/src/renaissance/recipes/type_var_domain.py b/src/renaissance/recipes/type_var_domain.py index cfc8968f..66e94c12 100644 --- a/src/renaissance/recipes/type_var_domain.py +++ b/src/renaissance/recipes/type_var_domain.py @@ -285,9 +285,6 @@ def uses_of(root: PythonCstNode, name: str, decl_stmt: cst.Assign) -> list[Pytho A use inside a string annotation or `cast("T", x)` counts; a `T` that a PEP 695 type parameter of an enclosing function or class (`def f[T]`, `class C[T]`) declares does not. """ - # TODO(FrancescoPezzella): libcst puts a function's decorators and parameter defaults inside its PEP 695 type - # parameter scope, so a `T` read only there (`@dec(T)`, `y=T`) is missed and the declaration it needs is removed. - # https://github.com/TNO/Renaissance.Py/issues/279 target = next(target.target for target in decl_stmt.targets if isinstance(target.target, cst.Name) and target.target.value == name) return [reference.node for reference in root.translation_unit.wrappers[target].referenced_by] @@ -360,15 +357,9 @@ def type_param_text(decl_stmt: cst.Assign, module: cst.Module) -> str: infers its variance. Callers must first reject declarations for which has_unconvertible_argument() is True. """ - # TODO(FrancescoPezzella): a default that uses another legacy declaration (`default=T`) still refers to it after conversion. - # https://github.com/TNO/Renaissance.Py/issues/279 call = _call(decl_stmt) constructor = type_param_constructor_name(decl_stmt) positional = _positional_arguments(call) - # TODO(FrancescoPezzella): the type parameter is named after the string argument, not the variable, so - # `T = TypeVar("U")` gives `def f[U](x: T)`; a non-literal name (`TypeVar(name)`, `TypeVar(*args)`) raises - # AttributeError and stops every phase for the file. - # https://github.com/TNO/Renaissance.Py/issues/279 name = cast("str", cast("cst.SimpleString", positional[0].value).evaluated_value) if constructor == "ParamSpec": diff --git a/src/renaissance/utils/import_resolution.py b/src/renaissance/utils/import_resolution.py index e4a19c26..8c413058 100644 --- a/src/renaissance/utils/import_resolution.py +++ b/src/renaissance/utils/import_resolution.py @@ -37,9 +37,6 @@ def resolve_project_module(importing_file: Path, project_root: Path, module: str Tries `.py` first, then `/__init__.py`. Returns None if neither exists or if resolution would walk above `project_root`, as for a stdlib or third-party import. The returned path is always absolute. - - # TODO: doesn't follow re-exports through an intermediate __init__.py, or handle namespace - # packages (no __init__.py, PEP 420) - out of scope for now. """ # A relative path can't walk above itself: Path("a.py").parent.parent is still Path("."). importing_file = importing_file.absolute() @@ -131,8 +128,6 @@ def _record_from_import( module_bindings: dict[str, Path], ) -> None: """Record `statement`'s imported names against their origin, and bind any alias that is itself a project module.""" - # TODO(FrancescoPezzella): `from pkg.mod import *` isn't expanded to the names it actually pulls in. - # https://github.com/TNO/Renaissance.Py/issues/279 module = import_from_module(statement) level = len(statement.relative) aliases = [] if isinstance(statement.names, cst.ImportStar) else list(statement.names) diff --git a/test/recipes/test_type_var_check_convert.py b/test/recipes/test_type_var_check_convert.py index e0e6ce27..dd37fe8a 100644 --- a/test/recipes/test_type_var_check_convert.py +++ b/test/recipes/test_type_var_check_convert.py @@ -572,23 +572,3 @@ def test_places_defaulted_type_params_last( output = subject.apply_to_string() ast.parse(output) assert_that(output, contains_string(expected_signature)) - - @pytest.mark.xfail( - reason="A default referencing another legacy declaration is carried over as is, so the new type " - "parameter's default refers to the module-level TypeVar.", - raises=AssertionError, - strict=True, - ) - def test_does_not_convert_default_referencing_another_declaration( - self, - create_type_var_check: Callable[[str], TypeVarCheck], - ) -> None: - """Verify a declaration whose default uses another legacy type parameter is reported unsafe and left unconverted.""" - subject = create_type_var_check(_source('T = TypeVar("T")\nU = TypeVar("U", default=T)', "def f(x: U) -> U:")) - subject.min_python = PEP_696_MINIMUM - - result = subject.convert_declared_typevars() - - assert_that(result, has_entry("U", "unsafe")) - assert_that(subject.converted_unsafe_reasons, has_entry("U", not_(UnsafeReason.PEP696_VERSION_GATE))) - assert_that(subject.apply_to_string(), contains_string("def f(x: U) -> U:")) From 21f5f647039fa4f7855dc453833145295da03417 Mon Sep 17 00:00:00 2001 From: Francesco Pezzella Date: Tue, 13 Oct 2026 16:20:29 +0200 Subject: [PATCH 02/25] Temporary UTF-8 fix; TODO drops the explicit encoding with Python 3.15 (#188) --- .../integrations/python/ast/cst_node.py | 4 +++- .../integrations/python/ast/rst_node.py | 4 +++- test/python/ast/test_python_cst_node.py | 17 +++++++++++++++++ test/python/ast/test_python_rst_node.py | 17 +++++++++++++++++ 4 files changed, 40 insertions(+), 2 deletions(-) diff --git a/src/renaissance/integrations/python/ast/cst_node.py b/src/renaissance/integrations/python/ast/cst_node.py index cc240b0d..12ac353f 100644 --- a/src/renaissance/integrations/python/ast/cst_node.py +++ b/src/renaissance/integrations/python/ast/cst_node.py @@ -242,7 +242,9 @@ def preceding_sibling(self) -> PythonCstNode | None: @staticmethod def load(file_path: Path) -> PythonCstNode: """AI: Parse the Python source file at file_path into a PythonCstNode tree.""" - with Path(file_path).open() as file: + # TODO(FrancescoPezzella): drop the explicit encoding with Python 3.15, where UTF-8 is the default. + # https://github.com/TNO/Renaissance.Py/issues/188 + with Path(file_path).open(encoding="utf-8") as file: content = file.read() return PythonCstNode.load_from_text(content, str(file_path)) diff --git a/src/renaissance/integrations/python/ast/rst_node.py b/src/renaissance/integrations/python/ast/rst_node.py index 4035ac54..eab7ae4f 100644 --- a/src/renaissance/integrations/python/ast/rst_node.py +++ b/src/renaissance/integrations/python/ast/rst_node.py @@ -356,7 +356,9 @@ def load( # Keep a uniform loader signature across AST node implementations. # Python's AST parser does not need extra arguments or a working dir. _ = extra_args, working_dir - with Path(file_path).open() as file: + # TODO(FrancescoPezzella): drop the explicit encoding with Python 3.15, where UTF-8 is the default. + # https://github.com/TNO/Renaissance.Py/issues/188 + with Path(file_path).open(encoding="utf-8") as file: content = file.read() return PythonRstNode.load_from_text(content, str(file_path)) diff --git a/test/python/ast/test_python_cst_node.py b/test/python/ast/test_python_cst_node.py index 31dd6cdf..3c42f757 100644 --- a/test/python/ast/test_python_cst_node.py +++ b/test/python/ast/test_python_cst_node.py @@ -91,6 +91,23 @@ def test_load_file(self) -> None: atu = PythonCstNode.load(Path(targets.__file__).parent / "demo.py") assert_that(atu, is_not(None)) + @pytest.mark.parametrize( + "source", + [ + pytest.param('x = "“quoted”"\n', id="undecodable-in-cp1252"), + pytest.param('x = "café"\n', id="two-byte-character"), + pytest.param('x = "\U0001f600"\n', id="four-byte-character"), + ], + ) + def test_load_file_decodes_utf8(self, tmp_path: Path, source: str) -> None: + """Verify load reads the file as UTF-8, whatever the locale's default encoding is.""" + path = tmp_path / "utf8.py" + path.write_bytes(source.encode("utf-8")) + + atu = PythonCstNode.load(path) + + assert_that(atu.signature, is_(source)) + def test_load_invalid_file(self) -> None: """AI: Verify loading a syntactically invalid Python file raises a ParserSyntaxError.""" with pytest.raises(ParserSyntaxError, match="Syntax Error"): diff --git a/test/python/ast/test_python_rst_node.py b/test/python/ast/test_python_rst_node.py index 6927c2da..096d5860 100644 --- a/test/python/ast/test_python_rst_node.py +++ b/test/python/ast/test_python_rst_node.py @@ -161,6 +161,23 @@ def test_load_file(self) -> None: atu = PythonRstNode.load(Path(targets.__file__).parent / "demo.py") assert_that(atu.translation_unit.atu.type_ignores, is_(empty())) + @pytest.mark.parametrize( + "source", + [ + pytest.param('x = "“quoted”"\n', id="undecodable-in-cp1252"), + pytest.param('x = "café"\n', id="two-byte-character"), + pytest.param('x = "\U0001f600"\n', id="four-byte-character"), + ], + ) + def test_load_file_decodes_utf8(self, tmp_path: Path, source: str) -> None: + """Verify load reads the file as UTF-8, whatever the locale's default encoding is.""" + path = tmp_path / "utf8.py" + path.write_bytes(source.encode("utf-8")) + + atu = PythonRstNode.load(path) + + assert_that(atu.signature, is_(source)) + def test_load_invalid_file(self) -> None: """AI: Verify loading a syntactically invalid Python file raises an IndentationError.""" with pytest.raises(IndentationError, match="unexpected indent"): From ab187f1ad137cf165ef0f788a36429ab683f73ae Mon Sep 17 00:00:00 2001 From: Francesco Pezzella Date: Tue, 13 Oct 2026 17:18:45 +0200 Subject: [PATCH 03/25] Skip functions that would mix legacy and PEP 695 type parameters. A function using a TypeVar that can't be converted is now left whole and reported unsafe. --- src/renaissance/recipes/type_var_check.py | 27 ++++-- src/renaissance/recipes/type_var_domain.py | 5 ++ test/recipes/test_type_var_check_convert.py | 96 +++++++++++++++++++++ 3 files changed, 120 insertions(+), 8 deletions(-) diff --git a/src/renaissance/recipes/type_var_check.py b/src/renaissance/recipes/type_var_check.py index 8ed9a3c6..71c4a16d 100644 --- a/src/renaissance/recipes/type_var_check.py +++ b/src/renaissance/recipes/type_var_check.py @@ -104,9 +104,11 @@ def convert_declared_typevars(self) -> dict[str, str]: The declaration is left in place. A use is attributed to the outermost function containing it; a `T` that a same-named PEP 695 type parameter of a function or class declares is not a use. Methods of a - class generic over the name are never converted. Bounds, constraints and `default=` are carried over - in their original formatting, with defaulted type parameters placed last. A name only such methods - use, or whose declaration _conversion_refusal rejects, is reported "unsafe", with its UnsafeReason on + class generic over the name are never converted. A function that also uses a name whose declaration + _conversion_refusal rejects is left unchanged, so no function mixes legacy and PEP 695 type parameters. + Bounds, constraints and `default=` are carried over in their original formatting, with defaulted type + parameters placed last. A name only methods of such a class use, whose declaration is rejected, or whose + every function is left unchanged is reported "unsafe", with its UnsafeReason on self.converted_unsafe_reasons. Returns {name: "fixed" | "unsafe"}. """ root = self._cst_root() @@ -114,9 +116,8 @@ class generic over the name are never converted. Bounds, constraints and `defaul results: dict[str, str] = {} self.converted_unsafe_reasons: dict[str, UnsafeReason] = {} - # One self.replace() per function: a function using two converted names would otherwise get two - # rewrites of the same node, which the rewriter rejects as conflicting. - new_params: dict[int, tuple[PythonCstNode, list[tuple[str, bool]]]] = {} + convertible: dict[str, tuple[tuple[str, bool], list[PythonCstNode]]] = {} + blocked: set[int] = set() for name, decl_stmt in find_type_param_declarations(module).items(): uses = uses_of(root, name, decl_stmt) functions = list(dict.fromkeys(function for use in uses if (function := outermost_function(use)) is not None)) @@ -137,10 +138,20 @@ class generic over the name are never converted. Bounds, constraints and `defaul reason = self._conversion_refusal(decl_stmt) if reason is not None: self._mark_unsafe(results, self.converted_unsafe_reasons, name, reason) + blocked.update(id(function) for function in candidates) continue - param = (type_param_text(decl_stmt, module), declared_default(decl_stmt) is not None) - for function in candidates: + convertible[name] = ((type_param_text(decl_stmt, module), declared_default(decl_stmt) is not None), candidates) + + # One self.replace() per function: a function using two converted names would otherwise get two + # rewrites of the same node, which the rewriter rejects as conflicting. + new_params: dict[int, tuple[PythonCstNode, list[tuple[str, bool]]]] = {} + for name, (param, candidates) in convertible.items(): + allowed = [function for function in candidates if id(function) not in blocked] + if not allowed: + self._mark_unsafe(results, self.converted_unsafe_reasons, name, UnsafeReason.SHARES_FUNCTION_WITH_UNCONVERTIBLE) + continue + for function in allowed: new_params.setdefault(id(function), (function, []))[1].append(param) results[name] = "fixed" diff --git a/src/renaissance/recipes/type_var_domain.py b/src/renaissance/recipes/type_var_domain.py index 66e94c12..23947345 100644 --- a/src/renaissance/recipes/type_var_domain.py +++ b/src/renaissance/recipes/type_var_domain.py @@ -32,6 +32,7 @@ class UnsafeReason(StrEnum): NO_PEP695_EQUIVALENT = "no_pep695_equivalent" DECLARED_TYPEVAR_EXPORTED = "declared_typevar_exported" USED_IN_GENERIC_CLASS = "used_in_generic_class" + SHARES_FUNCTION_WITH_UNCONVERTIBLE = "shares_function_with_unconvertible" DECLARATION_NAME_UNAVAILABLE = "declaration_name_unavailable" IMPORTED_ELSEWHERE_IN_PROJECT = "imported_elsewhere_in_project" ORIGIN_IMPORTS_CONSTRUCTOR_CONDITIONALLY = "origin_imports_constructor_conditionally" @@ -66,6 +67,10 @@ class UnsafeRule: "only used inside a class that is generic over it", "feature-type-parameter-modernization-used-in-generic-class", ), + UnsafeReason.SHARES_FUNCTION_WITH_UNCONVERTIBLE: UnsafeRule( + "every function using it also uses a type variable that can't be converted", + "feature-type-parameter-modernization-shares-function-with-unconvertible", + ), UnsafeReason.DECLARATION_NAME_UNAVAILABLE: UnsafeRule( "a name its declaration uses can't be imported here as the same object", "feature-type-parameter-modernization-declaration-name-unavailable", diff --git a/test/recipes/test_type_var_check_convert.py b/test/recipes/test_type_var_check_convert.py index dd37fe8a..6b09ba5e 100644 --- a/test/recipes/test_type_var_check_convert.py +++ b/test/recipes/test_type_var_check_convert.py @@ -335,6 +335,57 @@ def get(self, x: T) -> T: assert_that(result, has_entry("T", "fixed")) assert_that(subject.apply_to_string(), contains_string(" def get[T](self, x: T) -> T:")) + @pytest.mark.parametrize( + ("functions", "expected_converted", "expected_unchanged"), + [ + pytest.param( + "def f(x: U) -> U:\n return x\ndef g(x: U, y: T_co) -> T_co:\n return y\n", + "def f[U](x: U) -> U:", + "def g(x: U, y: T_co) -> T_co:", + id="functions", + ), + pytest.param( + "class Util:\n def f(self, x: U) -> U:\n return x\n def g(self, x: U, y: T_co) -> T_co:\n return y\n", + " def f[U](self, x: U) -> U:", + " def g(self, x: U, y: T_co) -> T_co:", + id="methods", + ), + ], + ) + def test_converts_only_functions_free_of_unconvertible_typevars( + self, create_type_var_check: Callable[[str], TypeVarCheck], functions: str, expected_converted: str, expected_unchanged: str + ) -> None: + """Verify a function that also uses an unconvertible TypeVar is left whole, while the others using U get [U].""" + subject = create_type_var_check( + f'from typing import TypeVar\n\nT_co = TypeVar("T_co", covariant=True)\nU = TypeVar("U")\n\n{functions}' + ) + result = subject.convert_declared_typevars() + + assert_that(result, equal_to({"T_co": "unsafe", "U": "fixed"})) + output = subject.apply_to_string() + assert_that(output, contains_string(expected_converted)) + assert_that(output, contains_string(expected_unchanged)) + + @pytest.mark.parametrize( + "class_declaration", + [ + pytest.param('T = TypeVar("T")', id="plain"), + pytest.param('T = TypeVar("T", covariant=True)', id="covariant"), + ], + ) + def test_converts_method_mixing_with_its_class_type_parameter( + self, create_type_var_check: Callable[[str], TypeVarCheck], class_declaration: str + ) -> None: + """Verify a method of a class generic over T gets [U] while keeping the class's legacy T.""" + subject = create_type_var_check( + f'from typing import Generic, TypeVar\n\n{class_declaration}\nU = TypeVar("U")\n\n' + "class Box(Generic[T]):\n def pair(self, x: T, y: U) -> tuple[T, U]:\n return x, y\n" + ) + result = subject.convert_declared_typevars() + + assert_that(result, has_entry("U", "fixed")) + assert_that(subject.apply_to_string(), contains_string(" def pair[U](self, x: T, y: U) -> tuple[T, U]:")) + def test_converts_function_when_typevar_is_also_used_outside_functions( self, create_type_var_check: Callable[[str], TypeVarCheck] ) -> None: @@ -533,6 +584,51 @@ def test_refuses_argument_it_cannot_carry_over( assert_that(subject.converted_unsafe_reasons, has_entry(name, expected_reason)) assert_that(subject.apply_to_string(), equal_to(source)) + @pytest.mark.parametrize( + ("declarations", "signature", "min_python", "expected_reasons"), + [ + pytest.param( + 'T_co = TypeVar("T_co", covariant=True)\nTs = TypeVarTuple("Ts")', + "def a(func: Callable[[*Ts], T_co], *args: *Ts) -> T_co:", + PEP_696_MINIMUM, + {"T_co": UnsafeReason.NO_PEP695_EQUIVALENT, "Ts": UnsafeReason.SHARES_FUNCTION_WITH_UNCONVERTIBLE}, + id="covariant-typevar-with-typevartuple", + ), + pytest.param( + 'T = TypeVar("T")\nD = TypeVar("D", default=int)', + "def a(x: T, y: D) -> T | D:", + PEP_695_MINIMUM, + {"D": UnsafeReason.PEP696_VERSION_GATE, "T": UnsafeReason.SHARES_FUNCTION_WITH_UNCONVERTIBLE}, + id="version-gated-default-declared-last", + ), + pytest.param( + 'P = ParamSpec("P", bound=int)\nT = TypeVar("T")', + "def a(f: Callable[P, T]) -> Callable[P, T]:", + PEP_696_MINIMUM, + {"P": UnsafeReason.NO_PEP695_EQUIVALENT, "T": UnsafeReason.SHARES_FUNCTION_WITH_UNCONVERTIBLE}, + id="bound-paramspec-with-typevar", + ), + ], + ) + def test_leaves_function_using_an_unconvertible_typevar_unchanged( + self, + create_type_var_check: Callable[[str], TypeVarCheck], + declarations: str, + signature: str, + min_python: tuple[int, int], + expected_reasons: dict[str, UnsafeReason], + ) -> None: + """Verify a function using one unconvertible TypeVar gets none of its TypeVars converted, so it never mixes syntaxes.""" + source = _source(declarations, signature) + subject = create_type_var_check(source) + subject.min_python = min_python + + result = subject.convert_declared_typevars() + + assert_that(result, equal_to(dict.fromkeys(expected_reasons, "unsafe"))) + assert_that(subject.converted_unsafe_reasons, equal_to(expected_reasons)) + assert_that(subject.apply_to_string(), equal_to(source)) + @pytest.mark.parametrize( ("declarations", "signature", "expected_signature"), [ From b72ac733e1f10ec0d0b39325334444856f483fa3 Mon Sep 17 00:00:00 2001 From: Francesco Pezzella Date: Tue, 13 Oct 2026 17:29:38 +0200 Subject: [PATCH 04/25] Bind each TypeVar to the function whose signature uses it. A TypeVar used only by a nested function now gets its [T] there instead of on the enclosing function. --- src/renaissance/recipes/type_var_check.py | 22 ++++---- src/renaissance/recipes/type_var_domain.py | 28 ++++++++-- test/recipes/test_type_var_check_convert.py | 62 +++++++++++++++++++++ 3 files changed, 96 insertions(+), 16 deletions(-) diff --git a/src/renaissance/recipes/type_var_check.py b/src/renaissance/recipes/type_var_check.py index 71c4a16d..e543b42d 100644 --- a/src/renaissance/recipes/type_var_check.py +++ b/src/renaissance/recipes/type_var_check.py @@ -27,7 +27,8 @@ is_safe_to_localize, is_safe_to_remove, module_statements, - outermost_function, + owning_function, + signature_function, type_param_constructor_name, type_param_text, type_params_bracket, @@ -102,14 +103,14 @@ def check(self) -> dict[str, dict[str, str]]: def convert_declared_typevars(self) -> dict[str, str]: """Add a PEP 695 type parameter to every function using a module-level TypeVar/ParamSpec/TypeVarTuple. - The declaration is left in place. A use is attributed to the outermost function containing it; a `T` - that a same-named PEP 695 type parameter of a function or class declares is not a use. Methods of a - class generic over the name are never converted. A function that also uses a name whose declaration - _conversion_refusal rejects is left unchanged, so no function mixes legacy and PEP 695 type parameters. - Bounds, constraints and `default=` are carried over in their original formatting, with defaulted type - parameters placed last. A name only methods of such a class use, whose declaration is rejected, or whose - every function is left unchanged is reported "unsafe", with its UnsafeReason on - self.converted_unsafe_reasons. Returns {name: "fixed" | "unsafe"}. + The declaration is left in place. A use is attributed to the outermost enclosing function whose signature + uses the name; a use outside every such signature is not attributed. A `T` that a same-named PEP 695 type + parameter of a function or class declares is not a use. Methods of a class generic over the name are never + converted. A function that also uses a name whose declaration _conversion_refusal rejects is left unchanged, + so no function mixes legacy and PEP 695 type parameters. Bounds, constraints and `default=` are carried over + in their original formatting, with defaulted type parameters placed last. A name only methods of such a class + use, whose declaration is rejected, or whose every function is left unchanged is reported "unsafe", with its + UnsafeReason on self.converted_unsafe_reasons. Returns {name: "fixed" | "unsafe"}. """ root = self._cst_root() module = root.translation_unit.atu @@ -120,7 +121,8 @@ class generic over the name are never converted. A function that also uses a nam blocked: set[int] = set() for name, decl_stmt in find_type_param_declarations(module).items(): uses = uses_of(root, name, decl_stmt) - functions = list(dict.fromkeys(function for use in uses if (function := outermost_function(use)) is not None)) + signature_users = {id(function) for use in uses if (function := signature_function(use)) is not None} + functions = list(dict.fromkeys(function for use in uses if (function := owning_function(use, signature_users)) is not None)) if not functions: continue diff --git a/src/renaissance/recipes/type_var_domain.py b/src/renaissance/recipes/type_var_domain.py index 23947345..61649832 100644 --- a/src/renaissance/recipes/type_var_domain.py +++ b/src/renaissance/recipes/type_var_domain.py @@ -294,15 +294,31 @@ def uses_of(root: PythonCstNode, name: str, decl_stmt: cst.Assign) -> list[Pytho return [reference.node for reference in root.translation_unit.wrappers[target].referenced_by] -def outermost_function(node: PythonCstNode) -> PythonCstNode | None: - """Return the outermost function or method enclosing node, or None if node is outside every function.""" - outermost = None +def signature_function(use: PythonCstNode) -> PythonCstNode | None: + """Return the function whose parameter or return annotation contains use, or None if no signature does. + + A use in a function's body, a parameter default or a decorator is in no signature. + """ + in_annotation = False + child, current = use, use.parent + while current is not None: + if isinstance(current.node, cst.Annotation): + in_annotation = True + elif isinstance(current.node, cst.FunctionDef): + return current if in_annotation and child.node is not current.node.body else None + child, current = current, current.parent + return None + + +def owning_function(node: PythonCstNode, signature_users: set[int]) -> PythonCstNode | None: + """Return the outermost function enclosing node whose id is in signature_users, or None if there is none.""" + owner = None current = node.parent while current is not None: - if isinstance(current.node, cst.FunctionDef): - outermost = current + if isinstance(current.node, cst.FunctionDef) and id(current) in signature_users: + owner = current current = current.parent - return outermost + return owner def generic_classes(uses: Iterable[PythonCstNode]) -> list[PythonCstNode]: diff --git a/test/recipes/test_type_var_check_convert.py b/test/recipes/test_type_var_check_convert.py index 6b09ba5e..7e75d82b 100644 --- a/test/recipes/test_type_var_check_convert.py +++ b/test/recipes/test_type_var_check_convert.py @@ -152,6 +152,68 @@ def wrapper(*args: P.args, **kwargs: P.kwargs) -> int: assert_that(output, contains_string("def wrapper(*args: P.args, **kwargs: P.kwargs) -> int:")) assert_that(output, not_(contains_string("wrapper[**P]"))) + @pytest.mark.parametrize( + ("declarations", "functions", "expected_lines"), + [ + pytest.param( + 'T = TypeVar("T")', + "def handle(scope: int) -> None:\n def wrap(func: Callable[[], T]) -> T:\n return func()\n", + ["def handle(scope: int) -> None:", " def wrap[T](func: Callable[[], T]) -> T:"], + id="only-nested-signature", + ), + pytest.param( + 'K = TypeVar("K")\nF = TypeVar("F")', + 'def keyword(name: "K | None" = None) -> "K | None":\n def decorator(func: F) -> F:\n return func\n', + ['def keyword[K](name: "K | None" = None) -> "K | None":', " def decorator[F](func: F) -> F:"], + id="outer-and-nested-own-one-each", + ), + pytest.param( + 'T = TypeVar("T")', + ( + "def outer() -> None:\n" + " def middle(x: T) -> T:\n" + " def inner(y: T) -> T:\n" + " return y\n" + " return inner(x)\n" + ), + ["def outer() -> None:", " def middle[T](x: T) -> T:", " def inner(y: T) -> T:"], + id="outermost-signature-of-three-levels", + ), + ], + ) + def test_binds_typevar_to_the_outermost_function_whose_signature_uses_it( + self, create_type_var_check: Callable[[str], TypeVarCheck], declarations: str, functions: str, expected_lines: list[str] + ) -> None: + """Verify [T] goes on the outermost function whose own signature uses T, never on an enclosing one that doesn't.""" + subject = create_type_var_check( + f"from collections.abc import Callable\nfrom typing import TypeVar\n\n{declarations}\n\n{functions}" + ) + result = subject.convert_declared_typevars() + + names = [declaration.split(" = ", maxsplit=1)[0] for declaration in declarations.splitlines()] + assert_that(result, equal_to(dict.fromkeys(names, "fixed"))) + output = subject.apply_to_string() + for line in expected_lines: + assert_that(output, contains_string(f"{line}\n")) + + @pytest.mark.parametrize( + "body", + [ + pytest.param(" y: list[T] = []\n", id="local-annotation"), + pytest.param(' y = cast("T", x)\n', id="cast"), + ], + ) + def test_leaves_typevar_used_only_in_a_function_body_unattributed( + self, create_type_var_check: Callable[[str], TypeVarCheck], body: str + ) -> None: + """Verify a T that no enclosing signature uses gives no function a [T].""" + source = f'from typing import TypeVar, cast\n\nT = TypeVar("T")\n\n\ndef f(x: int) -> None:\n{body}' + subject = create_type_var_check(source) + result = subject.convert_declared_typevars() + + assert_that(result, equal_to({})) + assert_that(subject.apply_to_string(), equal_to(source)) + def test_merges_into_an_existing_type_params_bracket(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: """AI: Verify converting a second TypeVar merges it into an existing PEP 695 bracket instead of adding a new one.""" subject = create_type_var_check(""" From 05ac6a7818566c0a6ed4bc630b3858a1c9ace93b Mon Sep 17 00:00:00 2001 From: Francesco Pezzella Date: Wed, 14 Oct 2026 15:54:36 +0200 Subject: [PATCH 05/25] Place localized TypeVar declarations after the import block and run ruff I001 in the CLI Needed imports are added once per file after the last import, and the CLI merges and sorts them with ruff. --- src/rejuvenation/migration-type-recipes.py | 16 +- src/renaissance/recipes/type_var_check.py | 160 +++++++++++++++--- src/renaissance/recipes/type_var_domain.py | 7 + test/recipes/test_type_var_check_localize.py | 106 ++++++++++++ .../test_migration_type_recipes.py | 49 ++++++ 5 files changed, 307 insertions(+), 31 deletions(-) diff --git a/src/rejuvenation/migration-type-recipes.py b/src/rejuvenation/migration-type-recipes.py index 98e43a0c..0d12470c 100644 --- a/src/rejuvenation/migration-type-recipes.py +++ b/src/rejuvenation/migration-type-recipes.py @@ -120,15 +120,18 @@ def process_file( 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. +def _run_ruff_import_cleanup(paths: list[Path]) -> None: + """Run `ruff check --fix --select F401,I001` over every path. + + Drops any now-unused import (F401), then merges and sorts the imports (I001) under the settings ruff + finds for each path. Best-effort: if ruff can't be invoked (e.g. not installed), prints a warning instead of raising. The files are already valid without this cleanup. """ try: subprocess.run( # noqa: S603 - fixed argv list (sys.executable + literals + our own discovered paths), no shell - [sys.executable, "-m", "ruff", "check", "--fix", "--select", "F401", *(str(path) for path in paths)], + [sys.executable, "-m", "ruff", "check", "--fix", "--select", "F401,I001", *(str(path) for path in paths)], capture_output=True, text=True, check=False, @@ -171,7 +174,8 @@ def _format_console_report(reports: list[FileReport], *, ruff_ran: bool) -> str: ] if ruff_ran: lines.append( - "Unused imports across the modified files above were also cleaned up via `ruff check --fix --select F401`.", + "Imports across the modified files above were also cleaned up via `ruff check --fix --select F401,I001`: " + "unused ones dropped, the rest merged and sorted.", ) lines.extend(["", f"MODIFIED ({len(modified)})"]) for report in modified: @@ -231,7 +235,7 @@ def build_arg_parser() -> argparse.ArgumentParser: parser.add_argument( "--no-ruff", action="store_true", - help="Skip the final `ruff check --fix --select F401` pass that drops imports made unused.", + help="Skip the final `ruff check --fix --select F401,I001` pass that drops imports made unused and sorts the rest.", ) return parser @@ -275,7 +279,7 @@ def main(argv: Sequence[str] | None = None) -> int: # TODO(FrancescoPezzella): removed statements leave their blank lines behind; ruff's E303 (preview) collapses them, but not # at file start. # https://github.com/TNO/Renaissance.Py/issues/279 - _run_ruff_unused_import_cleanup(modified_paths) + _run_ruff_import_cleanup(modified_paths) console_report = _format_console_report(reports, ruff_ran=ruff_ran) print(console_report) diff --git a/src/renaissance/recipes/type_var_check.py b/src/renaissance/recipes/type_var_check.py index e543b42d..74859319 100644 --- a/src/renaissance/recipes/type_var_check.py +++ b/src/renaissance/recipes/type_var_check.py @@ -7,6 +7,7 @@ """Recipe that modernizes legacy TypeVar/ParamSpec/TypeVarTuple usage to PEP 695 syntax.""" from collections.abc import Sequence +from dataclasses import dataclass from pathlib import Path from typing import cast @@ -27,6 +28,7 @@ is_safe_to_localize, is_safe_to_remove, module_statements, + names_read_by, owning_function, signature_function, type_param_constructor_name, @@ -40,6 +42,18 @@ PEP_696_MINIMUM = (3, 13) +@dataclass(frozen=True) +class _Localization: + """The type parameter names one import statement brings in that are localized, and the text that replaces them.""" + + line: cst.SimpleStatementLine + raw: cst.ImportFrom + names: frozenset[str] + kept: list[cst.ImportAlias] + imports: list[str] + declarations: list[str] + + class TypeVarCheck(PythonRefactoring[PythonCstNode]): """Modernize legacy TypeVar/ParamSpec/TypeVarTuple usage in a Python file to PEP 695 syntax. @@ -234,8 +248,10 @@ def _remove_declaration(self, decl_stmt: cst.Assign) -> None: def localize_imported_typevars(self) -> dict[str, str]: """Replace imports of TypeVar/ParamSpec/TypeVarTuple names from the target project with local declarations. - Imports are resolved against project_root. Where safe, the import becomes a copy of the origin's - declaration, plus imports from the origin for the names its arguments use (e.g. `bound=Shape`). + Imports are resolved against project_root. Where safe, the import is dropped and a copy of the origin's + declaration is added, plus imports from the origin for the names its arguments use (e.g. `bound=Shape`). + The added imports (each once per file) and declarations go after the file's last import, so no + import is left below code. A declaration stays where its import was when a statement in between uses it. It is unsafe when is_safe_to_localize rejects the origin or _argument_names_to_import returns None. Returns {name: "fixed" | "unsafe"}, with each UnsafeReason on self.cross_file_unsafe_reasons. """ @@ -244,6 +260,7 @@ def localize_imported_typevars(self) -> dict[str, str]: module = self._cst_root().translation_unit.atu project_root = self.project_root if self.project_root is not None else Path(self.filename).parent + localizations: list[_Localization] = [] for line in module.body: if not isinstance(line, cst.SimpleStatementLine): continue @@ -255,8 +272,9 @@ def localize_imported_typevars(self) -> dict[str, str]: ) if origin_path is None: continue - self._localize_names_from(line, statement, origin_path, results) + self._localize_names_from(line, statement, origin_path, results, localizations) + self._place_localizations(localizations) return results def _localize_names_from( @@ -265,10 +283,12 @@ def _localize_names_from( raw: cst.ImportFrom, origin_path: Path, results: dict[str, str], + localizations: list[_Localization], ) -> None: - """Localize every safe type parameter name that raw, on line, imports from the origin_path file. + """Queue the localization of every safe type parameter name that raw, on line, imports from the origin_path file. - Records each name in results as "fixed" or, via _mark_unsafe, as "unsafe". + Records each name in results as "fixed" or, via _mark_unsafe, as "unsafe", and appends what to + write for the "fixed" ones to localizations. Raises: SyntaxError: if the origin file can't be parsed, with the origin's path as its filename. @@ -311,7 +331,7 @@ def _localize_names_from( return if needed_argument_names: needed_imports.append(f"from {self._import_from_text(raw)} import {', '.join(sorted(needed_argument_names))}") - self._localize_import(line, raw, origin, localized, needed_imports) + localizations.append(self._localization(line, raw, origin, localized, needed_imports)) @staticmethod def _import_from_text(raw: cst.ImportFrom) -> str: @@ -379,34 +399,124 @@ def _argument_names_to_import(self, origin: cst.Module, raw: cst.ImportFrom, dec return None return missing - def _localize_import( - self, + @staticmethod + def _localization( line: cst.SimpleStatementLine, raw: cst.ImportFrom, origin: cst.Module, declarations: dict[str, cst.Assign], needed_imports: list[str], - ) -> None: - """Replace the import raw, on line, with local copies of declarations from the origin module. + ) -> _Localization: + """Return what replaces the names of declarations that the import raw, on line, brings in. - The declarations keep their original formatting and come after needed_imports: the imports they need - (their constructors, names their arguments use) that this file doesn't have yet. If raw imports other - names too, it is narrowed to those, keeping its comments, and the declarations go after line; - otherwise they replace raw. + The declarations keep their original formatting. needed_imports are the imports they need (their + constructors, names their arguments use) that this file doesn't have yet. The kept aliases are the + names raw imports that are not localized. """ no_semicolon = cst.MaybeSentinel.DEFAULT - decl_text = "\n".join( - [*needed_imports, *(origin.code_for_node(decl.with_changes(semicolon=no_semicolon)) for decl in declarations.values())] + kept = [alias for alias in cast("Sequence[cst.ImportAlias]", raw.names) if imported_name(alias) not in declarations] + if kept and raw.rpar is None: + # Without parentheses, a trailing comma after the last name is a syntax error. + kept[-1] = kept[-1].with_changes(comma=cst.MaybeSentinel.DEFAULT) + texts = [origin.code_for_node(decl.with_changes(semicolon=no_semicolon)) for decl in declarations.values()] + return _Localization(line, raw, frozenset(declarations), kept, needed_imports, texts) + + @staticmethod + def _is_import_line(statement: cst.BaseStatement) -> bool: + """Return True if statement is a line made only of import statements.""" + return isinstance(statement, cst.SimpleStatementLine) and all( + isinstance(small, cst.Import | cst.ImportFrom) for small in statement.body ) + + def _place_localizations(self, localizations: list[_Localization]) -> None: + """Write each localization: its import is narrowed or dropped, and what replaces its names goes after the last import. + + The needed imports come first, each once, then the declarations. If every import of the file is dropped, they + take the place of the last one. A localization is written where its import is when that import shares its + line with other statements and would be dropped, when no import remains to go after, or when a statement + between it and the last import reads one of its names. + """ + body = self._cst_root().translation_unit.atu.body wrappers = self._cst_root().translation_unit.wrappers + dropped = {id(item.line) for item in localizations if not item.kept} + anchor = max( + (index for index, statement in enumerate(body) if self._is_import_line(statement) and id(statement) not in dropped), + default=None, + ) + replaces_anchor = anchor is None + if anchor is None: + anchor = max( + ( + index + for index, statement in enumerate(body) + if id(statement) in dropped and isinstance(statement, cst.SimpleStatementLine) and len(statement.body) == 1 + ), + default=None, + ) + if anchor is None: + for item in localizations: + self._write_in_place(item) + return - kept = [alias for alias in cast("Sequence[cst.ImportAlias]", raw.names) if imported_name(alias) not in declarations] - if not kept: - self.replace(decl_text, wrappers[raw], include_whitespace=False, include_comments=False) + imports: list[str] = [] + declarations: list[str] = [] + anchor_raw: cst.ImportFrom | None = None + for item in localizations: + if not self._goes_after_last_import(item, body, anchor): + self._write_in_place(item) + continue + imports.extend(item.imports) + declarations.extend(item.declarations) + if replaces_anchor and item.line is body[anchor]: + anchor_raw = item.raw + elif item.kept: + self._narrow(item) + else: + self._drop(item) + + if not declarations: return - if raw.rpar is None: - # Without parentheses, a trailing comma after the last name is a syntax error. - kept[-1] = kept[-1].with_changes(comma=cst.MaybeSentinel.DEFAULT) - narrowed = self._cst_root().translation_unit.atu.code_for_node(raw.with_changes(names=kept, semicolon=no_semicolon)) - self.replace(narrowed, wrappers[raw], include_whitespace=False, include_comments=False) - self.insert_after(f"\n{decl_text}", wrappers[line], include_whitespace=False, include_comments=True) + text = "\n".join([*dict.fromkeys(imports), *declarations]) + if anchor_raw is not None: + self.replace(text, wrappers[anchor_raw], include_whitespace=False, include_comments=False) + else: + self.insert_after(f"\n{text}", wrappers[body[anchor]], include_whitespace=False, include_comments=True) + + def _write_in_place(self, item: _Localization) -> None: + """Replace the names of item where its import is: its needed imports and declarations follow the kept names, if any.""" + wrappers = self._cst_root().translation_unit.wrappers + text = "\n".join([*item.imports, *item.declarations]) + if not item.kept: + self.replace(text, wrappers[item.raw], include_whitespace=False, include_comments=False) + return + self._narrow(item) + self.insert_after(f"\n{text}", wrappers[item.line], include_whitespace=False, include_comments=True) + + def _drop(self, item: _Localization) -> None: + """Remove the import of item, with the comment trailing it, from its line.""" + wrappers = self._cst_root().translation_unit.wrappers + targets = [wrappers[item.raw]] + comment = item.line.trailing_whitespace.comment + if comment is not None: + targets.append(wrappers[comment]) + self.remove(targets, include_comments=False) + + def _narrow(self, item: _Localization) -> None: + """Replace the import of item with one that only imports its kept names, keeping its comments.""" + module = self._cst_root().translation_unit.atu + narrowed = module.code_for_node(item.raw.with_changes(names=item.kept, semicolon=cst.MaybeSentinel.DEFAULT)) + self.replace(narrowed, self._cst_root().translation_unit.wrappers[item.raw], include_whitespace=False, include_comments=False) + + def _goes_after_last_import(self, item: _Localization, body: Sequence[cst.BaseStatement], anchor: int) -> bool: + """Return True if item can be written after body[anchor], the last import that stays. + + It can't when its import would be dropped from a line with other statements, or when a statement + between it and the anchor that isn't an import reads one of its names: moving the declaration + down would define it after that use. + """ + if not item.kept and len(item.line.body) != 1: + return False + index = next(position for position, statement in enumerate(body) if statement is item.line) + return not any( + not self._is_import_line(statement) and item.names & names_read_by(statement) for statement in body[index + 1 : anchor] + ) diff --git a/src/renaissance/recipes/type_var_domain.py b/src/renaissance/recipes/type_var_domain.py index 61649832..1e7ecdf4 100644 --- a/src/renaissance/recipes/type_var_domain.py +++ b/src/renaissance/recipes/type_var_domain.py @@ -207,6 +207,13 @@ def declaration_argument_names(decl_stmt: cst.Assign) -> set[str]: return collector.names - set(dir(builtins)) +def names_read_by(node: cst.CSTNode) -> set[str]: + """Return every name `node` reads, built-ins included; attribute names and keyword argument names are not reads.""" + collector = _NameCollector() + node.visit(collector) + return collector.names + + def from_import_sources(module: cst.Module) -> dict[str, tuple[str, int] | None]: """Map every name bound at module level to its `from` import's (module, level), or None if bound otherwise. diff --git a/test/recipes/test_type_var_check_localize.py b/test/recipes/test_type_var_check_localize.py index 7db16657..80900546 100644 --- a/test/recipes/test_type_var_check_localize.py +++ b/test/recipes/test_type_var_check_localize.py @@ -50,6 +50,13 @@ def _names_used_by(call: ast.Call) -> set[str]: return names - set(dir(builtins)) +def _imports_after_code(module_text: str) -> list[str]: + """Return the module-level imports of module_text that come after its first non-import statement.""" + body = ast.parse(module_text).body + first_code = next((index for index, node in enumerate(body) if not isinstance(node, ast.Import | ast.ImportFrom)), len(body)) + return [ast.unparse(node) for node in body[first_code:] if isinstance(node, ast.Import | ast.ImportFrom)] + + class TestTypeVarCheckLocalize: """See module docstring.""" @@ -376,6 +383,105 @@ def test_localized_file_imports_the_constructor_exactly_once( assert_that(result, has_entry("T", "fixed")) assert_that(subject.apply_to_string().count("from typing import TypeVar"), equal_to(1)) + @pytest.mark.parametrize( + "importing_text", + [ + pytest.param("from file_1 import T\nimport os\nfrom collections import OrderedDict\n", id="import-after-removed"), + pytest.param("import os\nfrom file_1 import T\n", id="removed-is-last-import"), + pytest.param("from __future__ import annotations\nfrom file_1 import T\nimport os\n", id="future-import-present"), + pytest.param("from file_1 import T, helper\nimport os\n", id="narrowed-import"), + pytest.param("from file_1 import T\n", id="only-import"), + ], + ) + def test_localized_declaration_comes_after_every_import(self, mocker: MockerFixture, tmp_path: Path, importing_text: str) -> None: + """Verify the declaration is placed after the import block, so no import is left below code (E402).""" + subject = self._create_cross_file( + mocker, + tmp_path, + 'from typing import TypeVar\nT = TypeVar("T")\ndef helper() -> None:\n pass\n', + f"{importing_text}def b(x: T) -> T:\n return x\n", + ) + result = subject.localize_imported_typevars() + + assert_that(result, equal_to({"T": "fixed"})) + output = subject.apply_to_string() + assert_that(output, contains_string('T = TypeVar("T")')) + assert_that(_imports_after_code(output), equal_to([])) + assert_that(output.count("from typing import TypeVar"), equal_to(1)) + + @pytest.mark.parametrize( + "dropped_import", + [ + pytest.param("from file_1 import T # noqa: F401", id="trailing-comment"), + pytest.param("from file_1 import (\n T,\n) # noqa: F401", id="multi-line-trailing-comment"), + ], + ) + def test_dropping_an_import_drops_the_comment_on_its_line(self, mocker: MockerFixture, tmp_path: Path, dropped_import: str) -> None: + """Verify a comment trailing a fully localized import goes with it instead of staying behind on a line of its own.""" + subject = self._create_cross_file( + mocker, + tmp_path, + 'from typing import TypeVar\nT = TypeVar("T")\n', + f"import os\n{dropped_import}\nimport sys\ndef b(x: T) -> T:\n return x\n", + ) + result = subject.localize_imported_typevars() + + assert_that(result, equal_to({"T": "fixed"})) + output = subject.apply_to_string() + assert_that(output, not_(contains_string("noqa"))) + assert_that(_imports_after_code(output), equal_to([])) + + @pytest.mark.parametrize( + ("origin_extra", "expected_once"), + [ + pytest.param('T = TypeVar("T")\nU = TypeVar("U")', ["from typing import TypeVar"], id="constructor"), + pytest.param( + 'class Shape:\n pass\nT = TypeVar("T", bound=Shape)\nU = TypeVar("U", bound=Shape)', + ["from typing import TypeVar", "from file_1 import Shape"], + id="constructor-and-bound-name", + ), + ], + ) + def test_each_needed_import_is_added_once_per_file( + self, + mocker: MockerFixture, + tmp_path: Path, + origin_extra: str, + expected_once: list[str], + ) -> None: + """Verify names localized from separate import statements share the imports they need, one line each per file.""" + subject = self._create_cross_file( + mocker, + tmp_path, + f"from typing import TypeVar\n{origin_extra}\n", + "from file_1 import T\nfrom file_1 import U\ndef b(x: T, y: U) -> T:\n return x\n", + ) + result = subject.localize_imported_typevars() + + assert_that(result, equal_to({"T": "fixed", "U": "fixed"})) + output = subject.apply_to_string() + for line in expected_once: + assert_that(output.splitlines().count(line), equal_to(1), line) + + def test_declaration_stays_before_a_statement_that_uses_it_between_imports( + self, + mocker: MockerFixture, + tmp_path: Path, + ) -> None: + """Verify a declaration is not moved below a module-level statement that already needs it.""" + subject = self._create_cross_file( + mocker, + tmp_path, + 'from typing import TypeVar\nT = TypeVar("T")\n', + "from typing import Generic\nfrom file_1 import T\nclass Box(Generic[T]):\n pass\nimport os\n", + ) + result = subject.localize_imported_typevars() + + assert_that(result, equal_to({"T": "fixed"})) + output = subject.apply_to_string() + # Running the module is what proves the declaration is bound before `Box` uses it. + exec(compile(output, "importing.py", "exec"), {}) # noqa: S102 + @pytest.mark.parametrize( ("layout", "importing_header", "expected_line"), [ diff --git a/test/rejuvenation/test_migration_type_recipes.py b/test/rejuvenation/test_migration_type_recipes.py index bb61884c..72db1c16 100644 --- a/test/rejuvenation/test_migration_type_recipes.py +++ b/test/rejuvenation/test_migration_type_recipes.py @@ -138,6 +138,55 @@ def test_unused_typevar_import_cleanup( assert_that(written, import_matcher) assert_that(capsys.readouterr().out, report_matcher) + @pytest.mark.parametrize( + ("extra_args", "import_lines"), + [ + pytest.param([], ["from typing import Any, TypeVar", "from xarray_types import Alignable, JoinOptions"], id="default-merges"), + pytest.param( + ["--no-ruff"], + [ + "from typing import Any", + "from typing import TypeVar", + "from xarray_types import JoinOptions", + "from xarray_types import Alignable", + ], + id="no-ruff-keeps-them-apart", + ), + ], + ) + def test_localized_imports_are_merged_with_existing_ones( + self, + tmp_path: Path, + extra_args: list[str], + import_lines: list[str], + ) -> None: + """The localized TypeVar's import joins the file's own `typing` import, unless --no-ruff; Python < 3.12 only localizes.""" + (tmp_path / "xarray_types.py").write_text( + 'from typing import TypeVar\n\nJoinOptions = str\nAlignable = int\nT = TypeVar("T")\n', + encoding="utf-8", + ) + target = tmp_path / "use.py" + target.write_text( + textwrap.dedent("""\ + from typing import Any + from xarray_types import JoinOptions + from xarray_types import T + from xarray_types import Alignable + + + def pick(x: T, how: JoinOptions, other: Alignable) -> Any: + return x + """), + encoding="utf-8", + ) + + exit_code = migration.main([str(target), "--py", "3.11", *extra_args]) + + assert_that(exit_code, equal_to(0)) + lines = target.read_text(encoding="utf-8").splitlines() + for import_line in import_lines: + assert_that(lines.count(import_line), equal_to(1), import_line) + 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") From a0f4b86ba58716d20de187bd380dfb1be9516265 Mon Sep 17 00:00:00 2001 From: Francesco Pezzella Date: Wed, 14 Oct 2026 16:34:41 +0200 Subject: [PATCH 06/25] Copy the trailing comment of a localized TypeVar declaration Keeps `# noqa` and similar comments on the declaration; comment lines above it are tracked under "Need to fix". --- src/renaissance/recipes/type_var_check.py | 12 +++++--- src/renaissance/recipes/type_var_domain.py | 16 ++++++++++ test/recipes/test_type_var_check_localize.py | 32 ++++++++++++++++++++ 3 files changed, 55 insertions(+), 5 deletions(-) diff --git a/src/renaissance/recipes/type_var_check.py b/src/renaissance/recipes/type_var_check.py index 74859319..b27d18a6 100644 --- a/src/renaissance/recipes/type_var_check.py +++ b/src/renaissance/recipes/type_var_check.py @@ -19,6 +19,7 @@ from renaissance.recipes.type_var_domain import ( UnsafeReason, declaration_argument_names, + declaration_text, declared_default, declares_type_param, find_type_param_declarations, @@ -409,16 +410,17 @@ def _localization( ) -> _Localization: """Return what replaces the names of declarations that the import raw, on line, brings in. - The declarations keep their original formatting. needed_imports are the imports they need (their - constructors, names their arguments use) that this file doesn't have yet. The kept aliases are the - names raw imports that are not localized. + The declarations keep their original formatting and the comment trailing them. needed_imports are the + imports they need (their constructors, names their arguments use) that this file doesn't have yet. The + kept aliases are the names raw imports that are not localized. """ - no_semicolon = cst.MaybeSentinel.DEFAULT kept = [alias for alias in cast("Sequence[cst.ImportAlias]", raw.names) if imported_name(alias) not in declarations] if kept and raw.rpar is None: # Without parentheses, a trailing comma after the last name is a syntax error. kept[-1] = kept[-1].with_changes(comma=cst.MaybeSentinel.DEFAULT) - texts = [origin.code_for_node(decl.with_changes(semicolon=no_semicolon)) for decl in declarations.values()] + # TODO(FrancescoPezzella): the comment lines above a declaration are not copied: the rewriter only handles a one-line comment. + # https://github.com/TNO/Renaissance.Py/issues/189 + texts = [declaration_text(origin, decl) for decl in declarations.values()] return _Localization(line, raw, frozenset(declarations), kept, needed_imports, texts) @staticmethod diff --git a/src/renaissance/recipes/type_var_domain.py b/src/renaissance/recipes/type_var_domain.py index 1e7ecdf4..fd52745d 100644 --- a/src/renaissance/recipes/type_var_domain.py +++ b/src/renaissance/recipes/type_var_domain.py @@ -119,6 +119,22 @@ def find_type_param_declarations(module: cst.Module) -> dict[str, cst.Assign]: return declarations +def declaration_text(module: cst.Module, decl_stmt: cst.Assign) -> str: + """Return decl_stmt as written in module, without its `;`. + + The comment trailing its line follows it when it is the only statement there, since on a shared + line the comment may belong to another statement. + """ + text = module.code_for_node(decl_stmt.with_changes(semicolon=cst.MaybeSentinel.DEFAULT)) + for line in module.body: + if isinstance(line, cst.SimpleStatementLine) and len(line.body) == 1 and line.body[0] is decl_stmt: + trailing = line.trailing_whitespace + if trailing.comment is not None: + text += module.code_for_node(trailing.whitespace) + module.code_for_node(trailing.comment) + break + return text + + def _call(decl_stmt: cst.Assign) -> cst.Call: """Return the TypeVar/ParamSpec/TypeVarTuple call a declaration assigns.""" return cast("cst.Call", decl_stmt.value) diff --git a/test/recipes/test_type_var_check_localize.py b/test/recipes/test_type_var_check_localize.py index 80900546..01c8f732 100644 --- a/test/recipes/test_type_var_check_localize.py +++ b/test/recipes/test_type_var_check_localize.py @@ -482,6 +482,38 @@ def test_declaration_stays_before_a_statement_that_uses_it_between_imports( # Running the module is what proves the declaration is bound before `Box` uses it. exec(compile(output, "importing.py", "exec"), {}) # noqa: S102 + @pytest.mark.parametrize( + ("origin_declaration", "expected_comment_lines"), + [ + pytest.param('T = TypeVar("T") # noqa: PLC0105', ['T = TypeVar("T") # noqa: PLC0105'], id="noqa"), + pytest.param( + 'T = TypeVar(\n "T",\n bound=int,\n) # noqa: PLC0105', + [") # noqa: PLC0105"], + id="multi-line-declaration", + ), + pytest.param('T = TypeVar("T"); U = 1 # unrelated', [], id="shares-line-with-another-statement"), + ], + ) + def test_localized_declaration_keeps_the_comment_trailing_it( + self, + mocker: MockerFixture, + tmp_path: Path, + origin_declaration: str, + expected_comment_lines: list[str], + ) -> None: + """Verify the comment trailing a declaration's line is copied with it, unless the line holds another statement.""" + subject = self._create_cross_file( + mocker, + tmp_path, + f"from typing import TypeVar\n{origin_declaration}\n", + "from file_1 import T\ndef b(x: T) -> T:\n return x\n", + ) + result = subject.localize_imported_typevars() + + assert_that(result, equal_to({"T": "fixed"})) + output = subject.apply_to_string() + assert_that([line for line in output.splitlines() if "#" in line], equal_to(expected_comment_lines)) + @pytest.mark.parametrize( ("layout", "importing_header", "expected_line"), [ From bb636449ad784c3ab3d967702a0d54654b1d7466 Mon Sep 17 00:00:00 2001 From: Francesco Pezzella Date: Wed, 14 Oct 2026 16:52:13 +0200 Subject: [PATCH 07/25] Import the names a localized TypeVar uses from where the origin gets them and drop its trailing comment on removal Fixes `from import Any` for names the origin imports, and the stray `# noqa` left by orphan removal. --- src/renaissance/recipes/type_var_check.py | 79 ++++++++++++++------ src/renaissance/recipes/type_var_domain.py | 28 +++++-- test/recipes/test_type_var_check_localize.py | 38 +++++++++- test/recipes/test_type_var_check_orphaned.py | 21 ++++++ 4 files changed, 134 insertions(+), 32 deletions(-) diff --git a/src/renaissance/recipes/type_var_check.py b/src/renaissance/recipes/type_var_check.py index b27d18a6..3d376e18 100644 --- a/src/renaissance/recipes/type_var_check.py +++ b/src/renaissance/recipes/type_var_check.py @@ -23,6 +23,7 @@ declared_default, declares_type_param, find_type_param_declarations, + from_import_binding, from_import_sources, generic_classes, has_unconvertible_argument, @@ -32,6 +33,7 @@ names_read_by, owning_function, signature_function, + trailing_of_sole_statement, type_param_constructor_name, type_param_text, type_params_bracket, @@ -241,19 +243,25 @@ def _mark_unsafe(self, results: dict[str, str], reasons: dict[str, UnsafeReason] reasons[name] = reason def _remove_declaration(self, decl_stmt: cst.Assign) -> None: - """Remove decl_stmt's statement from the file.""" + """Remove decl_stmt's statement from the file, with the comment trailing it when it is alone on its line.""" + unit = self._cst_root().translation_unit + targets: list[cst.CSTNode] = [decl_stmt] + trailing = trailing_of_sole_statement(unit.atu, decl_stmt) + if trailing is not None and trailing.comment is not None: + targets.append(trailing.comment) # TODO(FrancescoPezzella): enable once comment blocks get correctly deleted # https://github.com/TNO/Renaissance.Py/issues/189 - self.remove(self._cst_root().translation_unit.wrappers[decl_stmt], include_comments=False) + self.remove([unit.wrappers[target] for target in targets], include_comments=False) def localize_imported_typevars(self) -> dict[str, str]: """Replace imports of TypeVar/ParamSpec/TypeVarTuple names from the target project with local declarations. Imports are resolved against project_root. Where safe, the import is dropped and a copy of the origin's - declaration is added, plus imports from the origin for the names its arguments use (e.g. `bound=Shape`). - The added imports (each once per file) and declarations go after the file's last import, so no - import is left below code. A declaration stays where its import was when a statement in between uses it. - It is unsafe when is_safe_to_localize rejects the origin or _argument_names_to_import returns None. + declaration is added, plus imports for the names its arguments use (e.g. `bound=Shape`), from the module + the origin gets each one from. The added imports (each once per file) and declarations go after the + file's last import, so no import is left below code. A declaration stays where its import was when a + statement in between uses it. + It is unsafe when is_safe_to_localize rejects the origin or _argument_imports returns None. Returns {name: "fixed" | "unsafe"}, with each UnsafeReason on self.cross_file_unsafe_reasons. """ results: dict[str, str] = {} @@ -304,7 +312,6 @@ def _localize_names_from( declarations = find_type_param_declarations(origin) localized: dict[str, cst.Assign] = {} needed_imports: list[str] = [] - needed_argument_names: set[str] = set() for alias in raw.names: name = alias.evaluated_name @@ -316,22 +323,20 @@ def _localize_names_from( if reason is not None: self._mark_unsafe(results, self.cross_file_unsafe_reasons, name, reason) continue - argument_names = self._argument_names_to_import(origin, raw, decl_stmt) - if argument_names is None: + argument_imports = self._argument_imports(origin, origin_path, raw, decl_stmt) + if argument_imports is None: self._mark_unsafe(results, self.cross_file_unsafe_reasons, name, UnsafeReason.DECLARATION_NAME_UNAVAILABLE) continue constructor_import = self._missing_constructor_import(origin, origin_path, raw, decl_stmt) - if constructor_import is not None and constructor_import not in needed_imports: - needed_imports.append(constructor_import) - needed_argument_names |= argument_names + for needed in [constructor_import, *argument_imports]: + if needed is not None and needed not in needed_imports: + needed_imports.append(needed) localized[name] = decl_stmt results[name] = "fixed" if not localized: return - if needed_argument_names: - needed_imports.append(f"from {self._import_from_text(raw)} import {', '.join(sorted(needed_argument_names))}") localizations.append(self._localization(line, raw, origin, localized, needed_imports)) @staticmethod @@ -375,22 +380,28 @@ def _missing_constructor_import( return f"from {ctor_module} import {ctor_name}" - def _argument_names_to_import(self, origin: cst.Module, raw: cst.ImportFrom, decl_stmt: cst.Assign) -> set[str] | None: - """Return the names decl_stmt's arguments use that this file must import from the origin module. + def _argument_imports( + self, + origin: cst.Module, + origin_path: Path, + raw: cst.ImportFrom, + decl_stmt: cst.Assign, + ) -> list[str] | None: + """Return the import lines for the names decl_stmt's arguments use that this file doesn't have, sorted by name. A name this file already imports from the origin module (or from the same absolute module the - origin imports it from) is fine and not returned. Returns None if one of those names can't be - imported from the origin as the same object: the origin doesn't bind it at module level at - runtime (e.g. only under `if TYPE_CHECKING:`), or this file binds it to something else. + origin imports it from) is fine and not returned. Returns None if one of the names can't be + imported as the same object: the origin doesn't bind it at module level at runtime (e.g. only + under `if TYPE_CHECKING:`), or this file binds it to something else. """ origin_sources = from_import_sources(origin) importing_sources = from_import_sources(self._cst_root().translation_unit.atu) - missing: set[str] = set() - for name in declaration_argument_names(decl_stmt): + imports: list[str] = [] + for name in sorted(declaration_argument_names(decl_stmt)): if name not in origin_sources: return None if name not in importing_sources: - missing.add(name) + imports.append(self._argument_import(origin, origin_path, raw, name)) continue origin_source = origin_sources[name] accepted = {(import_from_module(raw) or "", len(raw.relative))} @@ -398,7 +409,29 @@ def _argument_names_to_import(self, origin: cst.Module, raw: cst.ImportFrom, dec accepted.add(origin_source) if importing_sources[name] not in accepted: return None - return missing + return imports + + def _argument_import(self, origin: cst.Module, origin_path: Path, via: cst.ImportFrom, name: str) -> str: + """Return the import line for name, which a localized declaration uses and the origin binds at module level. + + A name the origin imports is imported from the module it gets it from, as named from this file, which + reaches the origin (origin_path) through via. Any other name, or one this file can't name the module + of, is imported from the origin module itself. + """ + binding = from_import_binding(origin, name) + if binding is not None: + statement, alias = binding + module = rebase_relative_module( + import_from_module(via), + len(via.relative), + origin_path, + import_from_module(statement) or None, + len(statement.relative), + ) + if module is not None: + imported = alias.evaluated_name + return f"from {module} import {name if imported == name else f'{imported} as {name}'}" + return f"from {self._import_from_text(via)} import {name}" @staticmethod def _localization( diff --git a/src/renaissance/recipes/type_var_domain.py b/src/renaissance/recipes/type_var_domain.py index fd52745d..43e361d5 100644 --- a/src/renaissance/recipes/type_var_domain.py +++ b/src/renaissance/recipes/type_var_domain.py @@ -126,15 +126,20 @@ def declaration_text(module: cst.Module, decl_stmt: cst.Assign) -> str: line the comment may belong to another statement. """ text = module.code_for_node(decl_stmt.with_changes(semicolon=cst.MaybeSentinel.DEFAULT)) - for line in module.body: - if isinstance(line, cst.SimpleStatementLine) and len(line.body) == 1 and line.body[0] is decl_stmt: - trailing = line.trailing_whitespace - if trailing.comment is not None: - text += module.code_for_node(trailing.whitespace) + module.code_for_node(trailing.comment) - break + trailing = trailing_of_sole_statement(module, decl_stmt) + if trailing is not None and trailing.comment is not None: + text += module.code_for_node(trailing.whitespace) + module.code_for_node(trailing.comment) return text +def trailing_of_sole_statement(module: cst.Module, statement: cst.BaseSmallStatement) -> cst.TrailingWhitespace | None: + """Return the whitespace and comment trailing the line of statement in module, or None if the line holds others.""" + for line in module.body: + if isinstance(line, cst.SimpleStatementLine) and len(line.body) == 1 and line.body[0] is statement: + return line.trailing_whitespace + return None + + def _call(decl_stmt: cst.Assign) -> cst.Call: """Return the TypeVar/ParamSpec/TypeVarTuple call a declaration assigns.""" return cast("cst.Call", decl_stmt.value) @@ -254,6 +259,17 @@ def from_import_sources(module: cst.Module) -> dict[str, tuple[str, int] | None] return sources +def from_import_binding(module: cst.Module, name: str) -> tuple[cst.ImportFrom, cst.ImportAlias] | None: + """Return the module-level `from` import and the alias that bind name, or None if another statement binds it last.""" + if from_import_sources(module).get(name) is None: + return None + binding: tuple[cst.ImportFrom, cst.ImportAlias] | None = None + for statement in module_statements(module): + if isinstance(statement, cst.ImportFrom) and not isinstance(statement.names, cst.ImportStar): + binding = next(((statement, alias) for alias in statement.names if imported_name(alias) == name), binding) + return binding + + def _assigned_names(statement: cst.Assign | cst.AnnAssign) -> set[str]: """Return every name an assignment's targets use, including inside tuple or list targets.""" targets = [target.target for target in statement.targets] if isinstance(statement, cst.Assign) else [statement.target] diff --git a/test/recipes/test_type_var_check_localize.py b/test/recipes/test_type_var_check_localize.py index 01c8f732..a189e723 100644 --- a/test/recipes/test_type_var_check_localize.py +++ b/test/recipes/test_type_var_check_localize.py @@ -13,7 +13,7 @@ from typing import TYPE_CHECKING, cast import pytest -from hamcrest import all_of, assert_that, contains_string, equal_to, has_entry, has_length, not_ +from hamcrest import all_of, assert_that, contains_string, equal_to, has_entry, has_item, has_length, not_ from pytest_mock import MockerFixture from renaissance.integrations.python.ast.cst_node import PythonCstNode @@ -149,6 +149,37 @@ def biggest(items: list[T]) -> T: assert_that(ast.dump(local_call), equal_to(ast.dump(origin_call))) assert_that(_names_used_by(cast("ast.Call", local_call)) - _module_level_names(module), equal_to(set())) + @pytest.mark.parametrize( + ("origin_imports", "expected_line"), + [ + pytest.param("from typing import Any", "from typing import Any", id="absolute-module"), + pytest.param("from ._shapes import Shape", "from sub._shapes import Shape", id="relative-module-in-origin"), + pytest.param("from shapes import Base as Shape", "from shapes import Base as Shape", id="aliased-in-origin"), + ], + ) + def test_names_a_declaration_uses_are_imported_from_where_the_origin_gets_them( + self, + mocker: MockerFixture, + tmp_path: Path, + origin_imports: str, + expected_line: str, + ) -> None: + """Verify a name the declaration uses is imported from the module the origin imports it from, not from the origin.""" + name = expected_line.rsplit(" ", maxsplit=1)[-1] + subject = self._create_cross_file( + mocker, + tmp_path, + f'from typing import TypeVar\n{origin_imports}\nT = TypeVar("T", bound={name})\n', + "from sub.file_1 import T\ndef b(x: T) -> T:\n return x\n", + ("sub/file_1.py", "use.py"), + ) + result = subject.localize_imported_typevars() + + assert_that(result, equal_to({"T": "fixed"})) + lines = subject.apply_to_string().splitlines() + assert_that(lines.count(expected_line), equal_to(1)) + assert_that(lines, not_(has_item(f"from sub.file_1 import {name}"))) + @pytest.mark.parametrize( ("origin", "importing_extra"), [ @@ -317,12 +348,13 @@ def test_narrowing_an_import_keeps_its_comments( 'class Shape:\n pass\nclass Circle(Shape):\n pass\nT = TypeVar("T", bound=Shape)\nU = TypeVar("U", bound=Circle)', "from file_1 import T, U", [ - "from file_1 import Circle, Shape", + "from file_1 import Circle", + "from file_1 import Shape", "from typing import TypeVar", 'T = TypeVar("T", bound=Shape)', 'U = TypeVar("U", bound=Circle)', ], - id="bounds-share-one-import", + id="bounds-each-imported-once", ), ], ) diff --git a/test/recipes/test_type_var_check_orphaned.py b/test/recipes/test_type_var_check_orphaned.py index dc1c1356..ccce2238 100644 --- a/test/recipes/test_type_var_check_orphaned.py +++ b/test/recipes/test_type_var_check_orphaned.py @@ -92,6 +92,27 @@ def test_removes_declaration_whose_references_are_all_shadowed( for def_line in (line.strip() for line in textwrap.dedent(source).splitlines() if line.strip().startswith("def ")): assert_that(output, contains_string(def_line)) + @pytest.mark.parametrize( + "declaration", + [ + pytest.param("T = TypeVar('T') # noqa: PLC0105", id="trailing-comment"), + pytest.param("T = TypeVar(\n 'T',\n bound=int,\n) # noqa: PLC0105", id="multi-line-trailing-comment"), + ], + ) + def test_removing_a_declaration_removes_the_comment_trailing_it( + self, + create_type_var_check: Callable[[str], TypeVarCheck], + declaration: str, + ) -> None: + """Verify the comment trailing a removed declaration goes with it instead of staying behind on a line of its own.""" + subject = create_type_var_check(f"from typing import TypeVar\n{declaration}\n\ndef b() -> None:\n pass\n") + + result = subject.remove_orphaned_declarations() + + assert_that(result, equal_to({"T": "fixed"})) + output = subject.apply_to_string() + assert_that(output, not_(contains_string("noqa"))) + def test_keeps_declaration_still_used_without_pep695(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: """Verify a declaration still used by a function without its own type parameter is left alone, unreported.""" subject = create_type_var_check(""" From cd910b3d9a6cb5261340ce4b94e915c722816eef Mon Sep 17 00:00:00 2001 From: Francesco Pezzella Date: Mon, 19 Oct 2026 09:49:46 +0200 Subject: [PATCH 08/25] Decided that CLRF -> LF is a good thing, and not something that needs a fix. Document that rewritten files are written with LF line endings as intended behaviour instead of listing it under Need to fix. --- docs/developer/modules/recipes.md | 10 +++++----- docs/user/features/type-parameter-modernization.md | 3 ++- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/docs/developer/modules/recipes.md b/docs/developer/modules/recipes.md index bfc7d738..6924fa85 100644 --- a/docs/developer/modules/recipes.md +++ b/docs/developer/modules/recipes.md @@ -104,6 +104,11 @@ safely cover. This page covers `TypeVarCheck`, the recipe behind a statement between it and the last import reads one of its names. Merging and sorting the added imports, and removing imports that became unused, is left to the CLI's `ruff check --fix --select F401,I001` pass. +- **Line endings.** Files are read with universal newlines, so the tree and every edit use LF, and a modified file is + written back with LF, also on the lines the recipe did not change. A file the recipe does not modify is not + rewritten. LF is what git and the Python ecosystem normalise to. A repository that stores CRLF shows every line of a + modified file as changed in `git diff`. + ## Related features - [Type parameter modernization](../../user/features/type-parameter-modernization.md) @@ -332,8 +337,3 @@ Errors that should be fixed, not accepted. `main.py` gets `T = TypeVar("T") # noqa: PLC0105` without the comment above it. The origin keeps its comment when it drops the declaration. - -- Line endings are not preserved: a modified CRLF file is written back with LF. This causes no problem when the - repository stores LF and git converts on checkout, the usual case, since git commits the same content. A - repository that stores CRLF gets a diff on every line of a modified file, so a pull request may be rejected - ([issue 279](https://github.com/TNO/Renaissance.Py/issues/279)). diff --git a/docs/user/features/type-parameter-modernization.md b/docs/user/features/type-parameter-modernization.md index 39f99aff..9f19ec72 100644 --- a/docs/user/features/type-parameter-modernization.md +++ b/docs/user/features/type-parameter-modernization.md @@ -109,7 +109,8 @@ example under *Non-goals* or *Need to fix* in [Refactoring recipes](../../develo - Removing a line can leave extra blank lines. Run your formatter afterwards. - With `--no-ruff`, the imports added by localizing are not merged into the file's existing `from` imports and not sorted. -- A modified CRLF file is written back with LF. +- A modified CRLF file is written back with LF. In a repository that stores CRLF, `git diff` then shows all its lines as + changed. - The comment lines above a localized declaration are not copied. The comment trailing its line, such as `# noqa: PLC0105`, is. - `Unpack[Ts]` is left as it is. Run `ruff`'s `UP044` to rewrite it to `*Ts` (see From f85aed5c6b40bece9a5d0b202bc5d5cd1c9e759d Mon Sep 17 00:00:00 2001 From: Francesco Pezzella Date: Mon, 19 Oct 2026 10:38:27 +0200 Subject: [PATCH 09/25] List files in the TypeVar migration report with a blank line between entries. --- src/rejuvenation/migration-type-recipes.py | 78 ++++++++++++++-------- 1 file changed, 52 insertions(+), 26 deletions(-) diff --git a/src/rejuvenation/migration-type-recipes.py b/src/rejuvenation/migration-type-recipes.py index 0d12470c..c2864d2d 100644 --- a/src/rejuvenation/migration-type-recipes.py +++ b/src/rejuvenation/migration-type-recipes.py @@ -153,12 +153,56 @@ def _format_commit_summary(reports: list[FileReport]) -> str: ) -def _format_console_report(reports: list[FileReport], *, ruff_ran: bool) -> str: +def _join_with_blank_lines(entries: list[list[str]]) -> list[str]: + """Flatten entries into one list of lines, with a blank line between consecutive entries.""" + lines: list[str] = [] + for index, entry in enumerate(entries): + if index: + lines.append("") + lines.extend(entry) + return lines + + +def _modified_entry(report: FileReport, project_root: Path) -> list[str]: + """Return the MODIFIED lines for one file: its path relative to project_root, then the names fixed per phase. + + A converted name isn't repeated under orphaned, because its declaration is always removed afterwards. + """ + lines = [f" {report.path.relative_to(project_root)}"] + result = report.result or {} + converted = {name for name, status in result.get("converted", {}).items() if status == "fixed"} + for phase, names in result.items(): + fixed = [name for name, status in names.items() if status == "fixed" and not (phase == "orphaned" and name in converted)] + if fixed: + lines.append(f" {phase}: {', '.join(fixed)}") + return lines + + +def _needs_review_entry(report: FileReport, project_root: Path) -> list[str]: + """Return the NEEDS MANUAL REVIEW lines for one file. + + The path relative to project_root comes first, then each unsafe name per phase with its reason. + """ + lines = [f" {report.path.relative_to(project_root)}"] + 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)}") + return lines + + +def _format_console_report(reports: list[FileReport], *, ruff_ran: bool, project_root: Path) -> str: """Build the full per-file report: MODIFIED / NEEDS MANUAL REVIEW / ERRORS sections. - Clean files (no type parameter usage at all) are only counted, not listed, so the report shows - what needs attention. Under MODIFIED, a converted name isn't repeated as orphaned. The ruff - import-cleanup line is only included when ruff_ran is True. + Files are listed by their path relative to project_root, separated by a blank line in the MODIFIED + and NEEDS MANUAL REVIEW sections. Clean files (no type parameter usage at all) are only counted, + not listed, so the report shows 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)] @@ -178,31 +222,13 @@ def _format_console_report(reports: list[FileReport], *, ruff_ran: bool) -> str: "unused ones dropped, the rest merged and sorted.", ) lines.extend(["", f"MODIFIED ({len(modified)})"]) - for report in modified: - lines.append(f" {report.path}") - result = report.result or {} - converted = {name for name, status in result.get("converted", {}).items() if status == "fixed"} - for phase, names in result.items(): - # A converted name's declaration is always removed afterwards; listing it under orphaned too would repeat it. - fixed = [name for name, status in names.items() if status == "fixed" and not (phase == "orphaned" and name in converted)] - if fixed: - lines.append(f" {phase}: {', '.join(fixed)}") + lines.extend(_join_with_blank_lines([_modified_entry(report, project_root) for report in modified])) 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(_join_with_blank_lines([_needs_review_entry(report, project_root) for report in needs_review])) lines.extend(["", f"ERRORS ({len(errors)})"]) - lines.extend(f" {report.path}: {report.error}" for report in errors) + lines.extend(f" {report.path.relative_to(project_root)}: {report.error}" for report in errors) return "\n".join(lines) @@ -281,7 +307,7 @@ def main(argv: Sequence[str] | None = None) -> int: # https://github.com/TNO/Renaissance.Py/issues/279 _run_ruff_import_cleanup(modified_paths) - console_report = _format_console_report(reports, ruff_ran=ruff_ran) + console_report = _format_console_report(reports, ruff_ran=ruff_ran, project_root=project_root) print(console_report) print() print(colored(_format_commit_summary(reports), "green", attrs=["bold"])) From d4cadac4246822ffd468aad7e68002923bb3505d Mon Sep 17 00:00:00 2001 From: Francesco Pezzella Date: Mon, 19 Oct 2026 11:15:44 +0200 Subject: [PATCH 10/25] Document the qualified TypeVar constructor and src-layout absolute imports as TypeVarCheck non-goals. --- docs/developer/modules/recipes.md | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/docs/developer/modules/recipes.md b/docs/developer/modules/recipes.md index 6924fa85..1195884d 100644 --- a/docs/developer/modules/recipes.md +++ b/docs/developer/modules/recipes.md @@ -316,6 +316,35 @@ See *Verified by test modules* in [Type parameter modernization](../../user/feat becomes `def first[U = T](items: list[U]) -> U: ...`, whose default still refers to the module-level `T`. +- A declaration written with a qualified constructor, such as `typing.TypeVar`, is not recognised. This is rare. In: + + ```python + import typing + + T = typing.TypeVar("T") + + def first(items: list[T]) -> T: ... + ``` + + `first` is left unchanged and the file is reported as clean. + +- Absolute imports are resolved from the path given to the tool. In a `src/` layout run on the repository root, + `from pkg.typing import T` is not matched to `src/pkg/typing.py`. This is rare. In: + + ```python + # src/pkg/typing.py + from typing import TypeVar + + T = TypeVar("T") + + # src/pkg/other.py + from pkg.typing import T + + def first(items: list[T]) -> T: ... + ``` + + `T = TypeVar("T")` is removed from `typing.py` and `other.py` is left importing it: `ImportError`. + ## Need to fix Errors that should be fixed, not accepted. From e9ba6359b59dadeac9435b9be21d5ba2c0ffa5d9 Mon Sep 17 00:00:00 2001 From: Francesco Pezzella Date: Mon, 19 Oct 2026 11:17:46 +0200 Subject: [PATCH 11/25] Shorten the line-ending and new non-goal documentation to the essential behaviour and one example each. --- docs/developer/modules/recipes.md | 20 +++++++------------ .../features/type-parameter-modernization.md | 3 +-- 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/docs/developer/modules/recipes.md b/docs/developer/modules/recipes.md index 1195884d..e92495e2 100644 --- a/docs/developer/modules/recipes.md +++ b/docs/developer/modules/recipes.md @@ -104,10 +104,8 @@ safely cover. This page covers `TypeVarCheck`, the recipe behind a statement between it and the last import reads one of its names. Merging and sorting the added imports, and removing imports that became unused, is left to the CLI's `ruff check --fix --select F401,I001` pass. -- **Line endings.** Files are read with universal newlines, so the tree and every edit use LF, and a modified file is - written back with LF, also on the lines the recipe did not change. A file the recipe does not modify is not - rewritten. LF is what git and the Python ecosystem normalise to. A repository that stores CRLF shows every line of a - modified file as changed in `git diff`. +- **Line endings.** Files are read with universal newlines and a modified file is written back with LF, also on the + lines the recipe did not change. A file the recipe does not modify is not rewritten. ## Related features @@ -316,25 +314,21 @@ See *Verified by test modules* in [Type parameter modernization](../../user/feat becomes `def first[U = T](items: list[U]) -> U: ...`, whose default still refers to the module-level `T`. -- A declaration written with a qualified constructor, such as `typing.TypeVar`, is not recognised. This is rare. In: +- A constructor written with its module, such as `typing.TypeVar`, is not recognised. This is rare. In: ```python - import typing - T = typing.TypeVar("T") def first(items: list[T]) -> T: ... ``` - `first` is left unchanged and the file is reported as clean. + `first` is left unchanged and the file is reported clean. -- Absolute imports are resolved from the path given to the tool. In a `src/` layout run on the repository root, - `from pkg.typing import T` is not matched to `src/pkg/typing.py`. This is rare. In: +- Absolute imports are resolved from the given path, so in a `src/` layout run on the repository root, + `from pkg.typing import T` does not find `src/pkg/typing.py`. This is rare. In: ```python # src/pkg/typing.py - from typing import TypeVar - T = TypeVar("T") # src/pkg/other.py @@ -343,7 +337,7 @@ See *Verified by test modules* in [Type parameter modernization](../../user/feat def first(items: list[T]) -> T: ... ``` - `T = TypeVar("T")` is removed from `typing.py` and `other.py` is left importing it: `ImportError`. + `T = TypeVar("T")` is removed and `other.py` fails with `ImportError`. ## Need to fix diff --git a/docs/user/features/type-parameter-modernization.md b/docs/user/features/type-parameter-modernization.md index 9f19ec72..0e49192c 100644 --- a/docs/user/features/type-parameter-modernization.md +++ b/docs/user/features/type-parameter-modernization.md @@ -109,8 +109,7 @@ example under *Non-goals* or *Need to fix* in [Refactoring recipes](../../develo - Removing a line can leave extra blank lines. Run your formatter afterwards. - With `--no-ruff`, the imports added by localizing are not merged into the file's existing `from` imports and not sorted. -- A modified CRLF file is written back with LF. In a repository that stores CRLF, `git diff` then shows all its lines as - changed. +- A modified CRLF file is written back with LF, so `git diff` shows all its lines changed in a repository that stores CRLF. - The comment lines above a localized declaration are not copied. The comment trailing its line, such as `# noqa: PLC0105`, is. - `Unpack[Ts]` is left as it is. Run `ruff`'s `UP044` to rewrite it to `*Ts` (see From 8d276d531403aaf9fd1177129797cecd02cbacad Mon Sep 17 00:00:00 2001 From: Francesco Pezzella Date: Mon, 19 Oct 2026 11:37:41 +0200 Subject: [PATCH 12/25] Add per-kind and ranked unsafe-reason summary tables to the TypeVar migration report. Expose each declaration's kind from TypeVarCheck so the CLI can count them. --- docs/developer/modules/recipes.md | 1 + .../features/type-parameter-modernization.md | 7 +- src/rejuvenation/migration-type-recipes.py | 79 ++++++++++++++++++- src/renaissance/recipes/type_var_check.py | 19 +++-- test/recipes/test_type_var_check.py | 12 +++ .../test_migration_type_recipes.py | 72 +++++++++++++++++ 6 files changed, 182 insertions(+), 8 deletions(-) diff --git a/docs/developer/modules/recipes.md b/docs/developer/modules/recipes.md index e92495e2..3fa9b8a5 100644 --- a/docs/developer/modules/recipes.md +++ b/docs/developer/modules/recipes.md @@ -42,6 +42,7 @@ safely cover. This page covers `TypeVarCheck`, the recipe behind | `convert_declared_typevars()` (only adds type parameters) | `converted_unsafe_reasons` | | `remove_orphaned_declarations()` (only removes declarations) | `orphaned_unsafe_reasons` | +- `type_param_kinds`: maps each type parameter name the phases see to `"TypeVar"`, `"ParamSpec"` or `"TypeVarTuple"`. - Attributes set by the CLI (`migration-type-recipes.py`): `min_python`, `project_root` (falls back to the file's folder) and `project_wide_imported_names` (from `collect_project_imported_names`). - `PythonRefactoring.process("TypeVarCheck", file)`, used by `cli.py refactor`, never sets those attributes, so every diff --git a/docs/user/features/type-parameter-modernization.md b/docs/user/features/type-parameter-modernization.md index 0e49192c..a860606e 100644 --- a/docs/user/features/type-parameter-modernization.md +++ b/docs/user/features/type-parameter-modernization.md @@ -357,8 +357,11 @@ declaration is gone). It also merges and sorts **every** import block in them, i before, using the `ruff` settings of the project. `ruff` must be installed in the same environment; if it cannot run, the report still says the pass ran. Files are changed directly, so run it on a git checkout. -The report has a summary line and the sections `MODIFIED`, `NEEDS MANUAL REVIEW` and `ERRORS`. Exit codes: 0 when -the run completes (manual review is not a failure), 2 for a usage error, 3 if any file raised an exception. +The report has a summary line, two summary tables and the sections `MODIFIED`, `NEEDS MANUAL REVIEW` and `ERRORS`. +The tables count the type parameters per kind (`TypeVar`, `ParamSpec`, `TypeVarTuple`) and rank the reasons for manual +review from most to least common. A name counts once per file, as needing review if any phase left it unsafe. Files are +listed relative to ``. Exit codes: 0 when the run completes (manual review is not a failure), 2 for a usage error, +3 if any file raised an exception. ## Change considerations diff --git a/src/rejuvenation/migration-type-recipes.py b/src/rejuvenation/migration-type-recipes.py index c2864d2d..025503e6 100644 --- a/src/rejuvenation/migration-type-recipes.py +++ b/src/rejuvenation/migration-type-recipes.py @@ -23,6 +23,7 @@ import subprocess import sys import textwrap +from collections import Counter from collections.abc import Sequence from dataclasses import dataclass from pathlib import Path @@ -35,6 +36,7 @@ from renaissance.utils.import_resolution import collect_project_imported_names _MAJOR_MINOR_PART_COUNT = 2 +_TYPE_PARAM_KINDS = ("TypeVar", "ParamSpec", "TypeVarTuple") @dataclass @@ -45,6 +47,16 @@ class FileReport: result: dict[str, dict[str, str]] | None error: str | None reasons: dict[str, dict[str, UnsafeReason]] | None = None + kinds: dict[str, str] | None = None + + +@dataclass +class KindCount: + """How many type parameters of one kind a run reported, and how many were fixed or need manual review.""" + + reported: int = 0 + fixed: int = 0 + needs_review: int = 0 def resolve_target_files(target: Path) -> list[Path]: @@ -117,7 +129,7 @@ def process_file( } 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) + return FileReport(path=path, result=result, error=None, reasons=reasons, kinds=recipe.type_param_kinds) def _run_ruff_import_cleanup(paths: list[Path]) -> None: @@ -153,6 +165,70 @@ def _format_commit_summary(reports: list[FileReport]) -> str: ) +def _name_statuses(report: FileReport) -> dict[str, str]: + """Return {name: "fixed" | "unsafe"} over all phases of report.result; "unsafe" wins if any phase left the name unsafe.""" + statuses: dict[str, str] = {} + for phase in (report.result or {}).values(): + for name, status in phase.items(): + if statuses.get(name) != "unsafe": + statuses[name] = status + return statuses + + +def count_by_kind(reports: Sequence[FileReport]) -> dict[str, KindCount]: + """Count the type parameters the reports list, per kind (TypeVar, ParamSpec, TypeVarTuple). + + A name counts once per file, as needing review if any phase left it unsafe and as fixed otherwise. + Files that errored are skipped. + """ + counts = {kind: KindCount() for kind in _TYPE_PARAM_KINDS} + for report in reports: + kinds = report.kinds or {} + for name, status in _name_statuses(report).items(): + count = counts[kinds[name]] + count.reported += 1 + if status == "unsafe": + count.needs_review += 1 + else: + count.fixed += 1 + return counts + + +def rank_reasons(reports: Sequence[FileReport]) -> list[tuple[UnsafeReason, int]]: + """Return each UnsafeReason the reports hit with its count, most common first, ties in UnsafeReason order. + + Every (phase, name) reason counts, so a name unsafe for two reasons counts under both. + """ + counter = Counter(reason for report in reports for phase in (report.reasons or {}).values() for reason in phase.values()) + order = list(UnsafeReason) + return sorted(counter.items(), key=lambda item: (-item[1], order.index(item[0]))) + + +def _table_row(cells: Sequence[str], widths: Sequence[int]) -> str: + """Return one Markdown table row with each cell padded to its column width.""" + return "| " + " | ".join(cell.ljust(width) for cell, width in zip(cells, widths, strict=True)) + " |" + + +def _format_table(headers: Sequence[str], rows: Sequence[Sequence[str]]) -> list[str]: + """Return a Markdown table as lines, with every column padded to its widest cell and the dash row to the same width.""" + widths = [max(len(cell) for cell in column) for column in zip(headers, *rows, strict=True)] + return [_table_row(headers, widths), _table_row(["-" * width for width in widths], widths), *(_table_row(row, widths) for row in rows)] + + +def _summary_tables(reports: Sequence[FileReport]) -> list[str]: + """Return the report's summary tables: type parameters per kind, then the unsafe reasons by frequency. + + The reasons table is left out when no reason was hit. + """ + kind_rows = [[kind, str(c.reported), str(c.fixed), str(c.needs_review)] for kind, c in count_by_kind(reports).items()] + lines = ["", "TYPE PARAMETERS BY KIND", "", *_format_table(["Kind", "Reported", "Fixed", "Needs review"], kind_rows)] + ranking = rank_reasons(reports) + if ranking: + reason_rows = [[UNSAFE_RULES[reason].message, str(count)] for reason, count in ranking] + lines.extend(["", "UNSAFE REASONS, MOST COMMON FIRST", "", *_format_table(["Reason", "Count"], reason_rows)]) + return lines + + def _join_with_blank_lines(entries: list[list[str]]) -> list[str]: """Flatten entries into one list of lines, with a blank line between consecutive entries.""" lines: list[str] = [] @@ -221,6 +297,7 @@ def _format_console_report(reports: list[FileReport], *, ruff_ran: bool, project "Imports across the modified files above were also cleaned up via `ruff check --fix --select F401,I001`: " "unused ones dropped, the rest merged and sorted.", ) + lines.extend(_summary_tables(reports)) lines.extend(["", f"MODIFIED ({len(modified)})"]) lines.extend(_join_with_blank_lines([_modified_entry(report, project_root) for report in modified])) diff --git a/src/renaissance/recipes/type_var_check.py b/src/renaissance/recipes/type_var_check.py index 3d376e18..54484885 100644 --- a/src/renaissance/recipes/type_var_check.py +++ b/src/renaissance/recipes/type_var_check.py @@ -73,8 +73,12 @@ class TypeVarCheck(PythonRefactoring[PythonCstNode]): project_root: Path | None = None def __init__(self, file: Path) -> None: - """Prepare the recipe for file, parsed with the CST backend.""" + """Prepare the recipe for file, parsed with the CST backend. + + type_param_kinds maps each type parameter name the phases see to "TypeVar", "ParamSpec" or "TypeVarTuple". + """ super().__init__(file, PythonCstNode) + self.type_param_kinds: dict[str, str] = {} def run(self) -> None: """Entry point called by PythonRefactoring.process(); stores check()'s result.""" @@ -127,7 +131,8 @@ def convert_declared_typevars(self) -> dict[str, str]: so no function mixes legacy and PEP 695 type parameters. Bounds, constraints and `default=` are carried over in their original formatting, with defaulted type parameters placed last. A name only methods of such a class use, whose declaration is rejected, or whose every function is left unchanged is reported "unsafe", with its - UnsafeReason on self.converted_unsafe_reasons. Returns {name: "fixed" | "unsafe"}. + UnsafeReason on self.converted_unsafe_reasons. Records each declaration's kind on self.type_param_kinds. + Returns {name: "fixed" | "unsafe"}. """ root = self._cst_root() module = root.translation_unit.atu @@ -137,6 +142,7 @@ def convert_declared_typevars(self) -> dict[str, str]: convertible: dict[str, tuple[tuple[str, bool], list[PythonCstNode]]] = {} blocked: set[int] = set() for name, decl_stmt in find_type_param_declarations(module).items(): + self.type_param_kinds[name] = type_param_constructor_name(decl_stmt) uses = uses_of(root, name, decl_stmt) signature_users = {id(function) for use in uses if (function := signature_function(use)) is not None} functions = list(dict.fromkeys(function for use in uses if (function := owning_function(use, signature_users)) is not None)) @@ -216,7 +222,8 @@ def remove_orphaned_declarations(self) -> dict[str, str]: A declaration is orphaned when uses_of finds no use of its name: every `T` left belongs to a PEP 695 type parameter, or there is none. One that is_safe_to_remove rejects is kept and reported "unsafe", - with its UnsafeReason on self.orphaned_unsafe_reasons. Returns {name: "fixed" | "unsafe"}. + with its UnsafeReason on self.orphaned_unsafe_reasons. Records each declaration's kind on + self.type_param_kinds. Returns {name: "fixed" | "unsafe"}. """ root = self._cst_root() module = root.translation_unit.atu @@ -224,6 +231,7 @@ def remove_orphaned_declarations(self) -> dict[str, str]: results: dict[str, str] = {} self.orphaned_unsafe_reasons: dict[str, UnsafeReason] = {} for name, decl_stmt in find_type_param_declarations(module).items(): + self.type_param_kinds[name] = type_param_constructor_name(decl_stmt) if uses_of(root, name, decl_stmt): continue @@ -296,8 +304,8 @@ def _localize_names_from( ) -> None: """Queue the localization of every safe type parameter name that raw, on line, imports from the origin_path file. - Records each name in results as "fixed" or, via _mark_unsafe, as "unsafe", and appends what to - write for the "fixed" ones to localizations. + Records each name in results as "fixed" or, via _mark_unsafe, as "unsafe", its kind on + self.type_param_kinds, and appends what to write for the "fixed" ones to localizations. Raises: SyntaxError: if the origin file can't be parsed, with the origin's path as its filename. @@ -319,6 +327,7 @@ def _localize_names_from( continue decl_stmt = declarations[name] + self.type_param_kinds[name] = type_param_constructor_name(decl_stmt) reason = is_safe_to_localize(origin, name) if reason is not None: self._mark_unsafe(results, self.cross_file_unsafe_reasons, name, reason) diff --git a/test/recipes/test_type_var_check.py b/test/recipes/test_type_var_check.py index cb33b3e0..b07f1fc8 100644 --- a/test/recipes/test_type_var_check.py +++ b/test/recipes/test_type_var_check.py @@ -45,6 +45,18 @@ def test_check_converts_then_removes_the_declaration_end_to_end( assert_that(output, contains_string("def b[T](x: T) -> T:")) assert_that(output, not_(contains_string("T = TypeVar"))) + def test_check_records_the_kind_of_every_declaration(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """Verify check() exposes each declared name's constructor (TypeVar, ParamSpec or TypeVarTuple) on type_param_kinds.""" + subject = create_type_var_check(""" + from typing import ParamSpec, TypeVar, TypeVarTuple + T = TypeVar('T') + P = ParamSpec('P') + Ts = TypeVarTuple('Ts') + """) + subject.run() + + assert_that(subject.type_param_kinds, equal_to({"T": "TypeVar", "P": "ParamSpec", "Ts": "TypeVarTuple"})) + def test_check_keeps_declaration_still_used_by_a_generic_class(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: """Verify check() converts a standalone function but keeps the declaration a Generic[...] base still needs.""" subject = create_type_var_check(""" diff --git a/test/rejuvenation/test_migration_type_recipes.py b/test/rejuvenation/test_migration_type_recipes.py index 72db1c16..e9e30ef8 100644 --- a/test/rejuvenation/test_migration_type_recipes.py +++ b/test/rejuvenation/test_migration_type_recipes.py @@ -255,6 +255,78 @@ def g[U](y: U) -> U: assert_that(output, contains_string(" orphaned: U\n")) +class TestSummaryTables: + """count_by_kind and rank_reasons: the numbers behind the report's summary tables.""" + + @pytest.mark.parametrize( + ("reports", "expected"), + [ + pytest.param( + [ + migration.FileReport( + path=Path("a.py"), + result={"cross_file": {}, "converted": {"T": "fixed", "P": "fixed"}, "orphaned": {"T": "unsafe", "Ts": "unsafe"}}, + error=None, + kinds={"T": "TypeVar", "P": "ParamSpec", "Ts": "TypeVarTuple"}, + ), + ], + {"TypeVar": (1, 0, 1), "ParamSpec": (1, 1, 0), "TypeVarTuple": (1, 0, 1)}, + id="name-unsafe-in-any-phase-needs-review", + ), + pytest.param( + [ + migration.FileReport(path=Path("a.py"), result={"converted": {"T": "fixed"}}, error=None, kinds={"T": "TypeVar"}), + migration.FileReport(path=Path("b.py"), result={"converted": {"T": "fixed"}}, error=None, kinds={"T": "TypeVar"}), + migration.FileReport(path=Path("c.py"), result=None, error="SyntaxError: x"), + ], + {"TypeVar": (2, 2, 0), "ParamSpec": (0, 0, 0), "TypeVarTuple": (0, 0, 0)}, + id="same-name-in-two-files-counts-twice-and-errors-are-skipped", + ), + ], + ) + def test_count_by_kind(self, reports: list[object], expected: dict[str, tuple[int, int, int]]) -> None: + """Each name counts once per file under its kind: fixed unless any phase left it unsafe.""" + counts = migration.count_by_kind(reports) + + assert_that({kind: (c.reported, c.fixed, c.needs_review) for kind, c in counts.items()}, equal_to(expected)) + + def test_rank_reasons_orders_by_count_then_by_declaration_order(self) -> None: + """Every (phase, name) reason counts; the most common comes first, a tie keeps the UnsafeReason declaration order.""" + reports = [ + migration.FileReport( + path=Path("a.py"), + result={}, + error=None, + reasons={"converted": {"T": UnsafeReason.USED_IN_GENERIC_CLASS}, "orphaned": {"T": UnsafeReason.DECLARED_TYPEVAR_EXPORTED}}, + ), + migration.FileReport( + path=Path("b.py"), + result={}, + error=None, + reasons={ + "orphaned": { + "U": UnsafeReason.DECLARED_TYPEVAR_EXPORTED, + "V": UnsafeReason.USED_IN_GENERIC_CLASS, + "W": UnsafeReason.PEP695_VERSION_GATE, + }, + }, + ), + ] + + ranking = migration.rank_reasons(reports) + + assert_that( + ranking, + equal_to( + [ + (UnsafeReason.DECLARED_TYPEVAR_EXPORTED, 2), + (UnsafeReason.USED_IN_GENERIC_CLASS, 2), + (UnsafeReason.PEP695_VERSION_GATE, 1), + ], + ), + ) + + class TestPerFileProgressFeedback: """main(): prints a per-file progress line as each file is checked.""" From c69166293d1c4200358c46df6327401f11bc123f Mon Sep 17 00:00:00 2001 From: Francesco Pezzella Date: Mon, 19 Oct 2026 15:11:41 +0200 Subject: [PATCH 13/25] Report a type parameter still used by a generic class even when its functions were converted. Simplify the migration report: plain-language descriptions, one shared documentation link, and fewer, end-to-end CLI tests. --- docs/developer/modules/recipes.md | 4 +- .../features/type-parameter-modernization.md | 16 +- src/rejuvenation/migration-type-recipes.py | 47 ++-- src/renaissance/recipes/type_var_check.py | 9 + src/renaissance/recipes/type_var_domain.py | 7 +- test/recipes/test_type_var_check_convert.py | 16 +- .../test_migration_type_recipes.py | 219 ++++++++---------- 7 files changed, 149 insertions(+), 169 deletions(-) diff --git a/docs/developer/modules/recipes.md b/docs/developer/modules/recipes.md index 3fa9b8a5..3f71e624 100644 --- a/docs/developer/modules/recipes.md +++ b/docs/developer/modules/recipes.md @@ -55,8 +55,8 @@ safely cover. This page covers `TypeVarCheck`, the recipe behind replaces only the node it changes, so formatting and comments elsewhere are kept. - **Unsafe reasons.** `is_safe_to_remove`/`is_safe_to_localize` and `_conversion_refusal` return an `UnsafeReason` - or `None`. Each of the 9 reasons has an `UnsafeRule` (message + docs anchor) in `UNSAFE_RULES`; `doc_link()` turns - it into the URL that the CLI report prints. + or `None`. Each of the 9 reasons has an `UnsafeRule` (message + docs anchor) in `UNSAFE_RULES`. The CLI report prints + each message and links `DOCS_BASE_URL` once. - **Version gates.** `_conversion_refusal` checks `min_python` against `PEP_695_MINIMUM = (3, 12)` and, for a `default=`, `PEP_696_MINIMUM = (3, 13)`. `None` never passes. A keyword outside `_CONVERTIBLE_KEYWORDS` is refused diff --git a/docs/user/features/type-parameter-modernization.md b/docs/user/features/type-parameter-modernization.md index a860606e..d9084837 100644 --- a/docs/user/features/type-parameter-modernization.md +++ b/docs/user/features/type-parameter-modernization.md @@ -72,7 +72,7 @@ class Box[T]: - Every safe change is written to the file in place. - Every candidate left unchanged for one of the reasons under *Constraints* is listed in the report as **needs manual - review**, with a link to the section that explains it. + review**, with its reason. The report links this page once, at the top of that section. - A file listed under `ERRORS` may already be changed: each phase is written before the next one runs, so a failure keeps the earlier phases. Check it with `git diff`. @@ -118,7 +118,7 @@ example under *Non-goals* or *Need to fix* in [Refactoring recipes](../../develo ## Constraints -Each case below stops one change and is reported as **unsafe** under *needs manual review*, with a link to its section. +Each case below stops one change and is reported under *needs manual review*. ### Python version gates { #feature-type-parameter-modernization-version-gates } @@ -225,7 +225,8 @@ def first[T](items: list[T]) -> T: ... ### A declared type parameter is only used inside a generic class { #feature-type-parameter-modernization-used-in-generic-class } Only methods of a class with `T` in a base (see *Generic classes* above) use `T`, so there is nothing to -convert. Standalone functions that also use `T` are still converted. +convert. Standalone functions that also use `T` are still converted, and `T` is then listed both as modified and as +needing review, because its declaration stays. ```python T = TypeVar("T") @@ -236,8 +237,9 @@ class Box(Generic[T]): The file is unchanged. -**To fix this yourself:** `ruff`'s `UP046` rewrites `Generic[T]` to `class Box[T]:`. Run the tool again afterwards: -it removes the now unused `T = TypeVar("T")`. +**To fix this yourself:** `ruff`'s `UP046` rewrites `Generic[T]` and `Protocol[T]` to `class Box[T]:`. Run the tool +again afterwards: it removes the now unused `T = TypeVar("T")`. `UP046` does not touch a `T` in any other base, so +write `class Box[T](MutableSet[T]):` by hand. ### A name the declaration uses can't be imported here { #feature-type-parameter-modernization-declaration-name-unavailable } @@ -359,7 +361,9 @@ the report still says the pass ran. Files are changed directly, so run it on a g The report has a summary line, two summary tables and the sections `MODIFIED`, `NEEDS MANUAL REVIEW` and `ERRORS`. The tables count the type parameters per kind (`TypeVar`, `ParamSpec`, `TypeVarTuple`) and rank the reasons for manual -review from most to least common. A name counts once per file, as needing review if any phase left it unsafe. Files are +review from most to least common. A name counts once per file, as needing review if it has a reason to review. `MODIFIED` +says what was done to each name: converted in functions, import replaced by a local declaration, or unused declaration +removed. `NEEDS MANUAL REVIEW` gives each name its reason, and a name can be in both sections. Files are listed relative to ``. Exit codes: 0 when the run completes (manual review is not a failure), 2 for a usage error, 3 if any file raised an exception. diff --git a/src/rejuvenation/migration-type-recipes.py b/src/rejuvenation/migration-type-recipes.py index 025503e6..fa6ac6fc 100644 --- a/src/rejuvenation/migration-type-recipes.py +++ b/src/rejuvenation/migration-type-recipes.py @@ -32,11 +32,16 @@ from renaissance.project.project_scanner import PythonScanner from renaissance.recipes.type_var_check import TypeVarCheck -from renaissance.recipes.type_var_domain import UNSAFE_RULES, UnsafeReason, doc_link +from renaissance.recipes.type_var_domain import DOCS_BASE_URL, UNSAFE_RULES, UnsafeReason from renaissance.utils.import_resolution import collect_project_imported_names _MAJOR_MINOR_PART_COUNT = 2 _TYPE_PARAM_KINDS = ("TypeVar", "ParamSpec", "TypeVarTuple") +_FIXED_DESCRIPTIONS = { + "cross_file": "import replaced by a local declaration", + "converted": "converted to PEP 695 in functions", + "orphaned": "unused declaration removed", +} @dataclass @@ -92,10 +97,8 @@ def has_fixed(report: FileReport) -> bool: 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()) + """Return True if any phase of report.result gave at least one name a reason to review.""" + return any(report.reasons.values()) if report.reasons else False def is_clean(report: FileReport) -> bool: @@ -166,12 +169,14 @@ def _format_commit_summary(reports: list[FileReport]) -> str: def _name_statuses(report: FileReport) -> dict[str, str]: - """Return {name: "fixed" | "unsafe"} over all phases of report.result; "unsafe" wins if any phase left the name unsafe.""" + """Return {name: "fixed" | "unsafe"} over all phases of report.result; "unsafe" wins if any phase gave the name a reason.""" statuses: dict[str, str] = {} for phase in (report.result or {}).values(): for name, status in phase.items(): if statuses.get(name) != "unsafe": statuses[name] = status + for phase_reasons in (report.reasons or {}).values(): + statuses.update(dict.fromkeys(phase_reasons, "unsafe")) return statuses @@ -240,35 +245,27 @@ def _join_with_blank_lines(entries: list[list[str]]) -> list[str]: def _modified_entry(report: FileReport, project_root: Path) -> list[str]: - """Return the MODIFIED lines for one file: its path relative to project_root, then the names fixed per phase. + """Return the MODIFIED lines for one file: its path relative to project_root, then what was done to each name. - A converted name isn't repeated under orphaned, because its declaration is always removed afterwards. + A converted name isn't described as removed, because its declaration is always removed afterwards. """ lines = [f" {report.path.relative_to(project_root)}"] result = report.result or {} converted = {name for name, status in result.get("converted", {}).items() if status == "fixed"} + descriptions: dict[str, list[str]] = {} for phase, names in result.items(): - fixed = [name for name, status in names.items() if status == "fixed" and not (phase == "orphaned" and name in converted)] - if fixed: - lines.append(f" {phase}: {', '.join(fixed)}") + for name, status in names.items(): + if status == "fixed" and not (phase == "orphaned" and name in converted): + descriptions.setdefault(name, []).append(_FIXED_DESCRIPTIONS[phase]) + lines.extend(f" {name}: {', '.join(texts)}" for name, texts in descriptions.items()) return lines def _needs_review_entry(report: FileReport, project_root: Path) -> list[str]: - """Return the NEEDS MANUAL REVIEW lines for one file. - - The path relative to project_root comes first, then each unsafe name per phase with its reason. - """ + """Return the NEEDS MANUAL REVIEW lines for one file: its path relative to project_root, then each name with its reason.""" lines = [f" {report.path.relative_to(project_root)}"] - 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)}") + for phase_reasons in (report.reasons or {}).values(): + lines.extend(f" {name}: {UNSAFE_RULES[reason].message}" for name, reason in phase_reasons.items()) return lines @@ -302,6 +299,8 @@ def _format_console_report(reports: list[FileReport], *, ruff_ran: bool, project lines.extend(_join_with_blank_lines([_modified_entry(report, project_root) for report in modified])) lines.extend(["", f"NEEDS MANUAL REVIEW ({len(needs_review)})"]) + if needs_review: + lines.extend([f" {DOCS_BASE_URL}", ""]) lines.extend(_join_with_blank_lines([_needs_review_entry(report, project_root) for report in needs_review])) lines.extend(["", f"ERRORS ({len(errors)})"]) diff --git a/src/renaissance/recipes/type_var_check.py b/src/renaissance/recipes/type_var_check.py index 54484885..4d1874c2 100644 --- a/src/renaissance/recipes/type_var_check.py +++ b/src/renaissance/recipes/type_var_check.py @@ -140,6 +140,7 @@ def convert_declared_typevars(self) -> dict[str, str]: results: dict[str, str] = {} self.converted_unsafe_reasons: dict[str, UnsafeReason] = {} convertible: dict[str, tuple[tuple[str, bool], list[PythonCstNode]]] = {} + generic_classes_by_name: dict[str, list[PythonCstNode]] = {} blocked: set[int] = set() for name, decl_stmt in find_type_param_declarations(module).items(): self.type_param_kinds[name] = type_param_constructor_name(decl_stmt) @@ -167,6 +168,7 @@ def convert_declared_typevars(self) -> dict[str, str]: continue convertible[name] = ((type_param_text(decl_stmt, module), declared_default(decl_stmt) is not None), candidates) + generic_classes_by_name[name] = classes # One self.replace() per function: a function using two converted names would otherwise get two # rewrites of the same node, which the rewriter rejects as conflicting. @@ -183,8 +185,15 @@ def convert_declared_typevars(self) -> dict[str, str]: for function, params in new_params.values(): self._add_type_params(function, params) + self._report_generic_class_leftovers(results, generic_classes_by_name) return results + def _report_generic_class_leftovers(self, results: dict[str, str], generic_classes_by_name: dict[str, list[PythonCstNode]]) -> None: + """Add USED_IN_GENERIC_CLASS to converted_unsafe_reasons for each "fixed" name a generic class still uses.""" + for name, classes in generic_classes_by_name.items(): + if classes and results.get(name) == "fixed": + self.converted_unsafe_reasons[name] = UnsafeReason.USED_IN_GENERIC_CLASS + @staticmethod def _is_inside(node: PythonCstNode, containers: Sequence[PythonCstNode]) -> bool: """Return True if one of containers encloses node.""" diff --git a/src/renaissance/recipes/type_var_domain.py b/src/renaissance/recipes/type_var_domain.py index 43e361d5..101c838d 100644 --- a/src/renaissance/recipes/type_var_domain.py +++ b/src/renaissance/recipes/type_var_domain.py @@ -24,7 +24,7 @@ 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(). + Each member has a matching documented rule under DOCS_BASE_URL - see UNSAFE_RULES. """ PEP695_VERSION_GATE = "pep695_version_gate" @@ -86,11 +86,6 @@ class UnsafeRule: } -def doc_link(reason: UnsafeReason) -> str: - """Return the full URL to the documented rule explaining why `reason` makes a candidate unsafe.""" - return f"{DOCS_BASE_URL}#{UNSAFE_RULES[reason].doc_anchor}" - - _CONSTRUCTORS = frozenset({"TypeVar", "ParamSpec", "TypeVarTuple"}) diff --git a/test/recipes/test_type_var_check_convert.py b/test/recipes/test_type_var_check_convert.py index 7e75d82b..2955df12 100644 --- a/test/recipes/test_type_var_check_convert.py +++ b/test/recipes/test_type_var_check_convert.py @@ -309,16 +309,19 @@ def test_converts_function_preserving_multiline_string_literal(self, create_type assert_that(ast.literal_eval(text_assign.value), equal_to("first\nsecond\n third")) @pytest.mark.parametrize( - "class_header", + ("class_header", "expected_reasons"), [ - pytest.param("class Box(Generic[T]):", id="generic-base"), - pytest.param("class Box(Protocol[T]):", id="protocol-base"), - pytest.param("class Sub(Base[T]):", id="generic-subclass"), - pytest.param("class Box[T]:", id="pep695-class"), + pytest.param("class Box(Generic[T]):", {"T": UnsafeReason.USED_IN_GENERIC_CLASS}, id="generic-base"), + pytest.param("class Box(Protocol[T]):", {"T": UnsafeReason.USED_IN_GENERIC_CLASS}, id="protocol-base"), + pytest.param("class Sub(Base[T]):", {"T": UnsafeReason.USED_IN_GENERIC_CLASS}, id="generic-subclass"), + pytest.param("class Box[T]:", {}, id="pep695-class"), ], ) def test_converts_function_but_not_methods_of_a_generic_class( - self, create_type_var_check: Callable[[str], TypeVarCheck], class_header: str + self, + create_type_var_check: Callable[[str], TypeVarCheck], + class_header: str, + expected_reasons: dict[str, UnsafeReason], ) -> None: """Verify a standalone function gets [T] while the methods of a class generic over T keep using the class's T.""" subject = create_type_var_check(f""" @@ -335,6 +338,7 @@ def first(items: list[T]) -> T: result = subject.convert_declared_typevars() assert_that(result, equal_to({"T": "fixed"})) + assert_that(subject.converted_unsafe_reasons, equal_to(expected_reasons)) output = subject.apply_to_string() assert_that(output, contains_string("def first[T](items: list[T]) -> T:")) assert_that(output, contains_string(" def get(self, x: T) -> T:")) diff --git a/test/rejuvenation/test_migration_type_recipes.py b/test/rejuvenation/test_migration_type_recipes.py index e9e30ef8..af9d9e8d 100644 --- a/test/rejuvenation/test_migration_type_recipes.py +++ b/test/rejuvenation/test_migration_type_recipes.py @@ -19,7 +19,7 @@ from hamcrest import assert_that, contains_string, equal_to, not_ from hamcrest.core.matcher import Matcher -from renaissance.recipes.type_var_domain import UnsafeReason, doc_link +from renaissance.recipes.type_var_domain import DOCS_BASE_URL, UNSAFE_RULES, UnsafeReason _SCRIPT_PATH = Path(__file__).resolve().parents[2] / "src" / "rejuvenation" / "migration-type-recipes.py" @@ -138,29 +138,8 @@ def test_unused_typevar_import_cleanup( assert_that(written, import_matcher) assert_that(capsys.readouterr().out, report_matcher) - @pytest.mark.parametrize( - ("extra_args", "import_lines"), - [ - pytest.param([], ["from typing import Any, TypeVar", "from xarray_types import Alignable, JoinOptions"], id="default-merges"), - pytest.param( - ["--no-ruff"], - [ - "from typing import Any", - "from typing import TypeVar", - "from xarray_types import JoinOptions", - "from xarray_types import Alignable", - ], - id="no-ruff-keeps-them-apart", - ), - ], - ) - def test_localized_imports_are_merged_with_existing_ones( - self, - tmp_path: Path, - extra_args: list[str], - import_lines: list[str], - ) -> None: - """The localized TypeVar's import joins the file's own `typing` import, unless --no-ruff; Python < 3.12 only localizes.""" + def test_localized_imports_are_merged_with_existing_ones(self, tmp_path: Path) -> None: + """The localized TypeVar's import joins the file's own `typing` import; Python < 3.12 only localizes.""" (tmp_path / "xarray_types.py").write_text( 'from typing import TypeVar\n\nJoinOptions = str\nAlignable = int\nT = TypeVar("T")\n', encoding="utf-8", @@ -180,11 +159,11 @@ def pick(x: T, how: JoinOptions, other: Alignable) -> Any: encoding="utf-8", ) - exit_code = migration.main([str(target), "--py", "3.11", *extra_args]) + exit_code = migration.main([str(target), "--py", "3.11"]) assert_that(exit_code, equal_to(0)) lines = target.read_text(encoding="utf-8").splitlines() - for import_line in import_lines: + for import_line in ["from typing import Any, TypeVar", "from xarray_types import Alignable, JoinOptions"]: assert_that(lines.count(import_line), equal_to(1), import_line) def test_unmodified_sibling_file_is_left_untouched(self, tmp_path: Path) -> None: @@ -206,29 +185,65 @@ def greet() -> str: 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.""" +class TestConsoleReportNeedsReviewSection: + """main(): what the NEEDS MANUAL REVIEW section lists per file.""" - def test_needs_manual_review_includes_doc_link_for_the_specific_reason( + def test_names_come_with_their_reason_under_one_shared_documentation_link( self, tmp_path: Path, capsys: pytest.CaptureFixture[str], ) -> None: - """The report links a __all__-exported TypeVar to the DECLARED_TYPEVAR_EXPORTED rule.""" + """Each name is listed with its reason in words; the documentation is linked once, without per-name anchors.""" 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))) + assert_that(output.count(DOCS_BASE_URL), equal_to(1)) + assert_that(output, not_(contains_string("#feature-"))) + assert_that(output, contains_string(" T: exported via __all__\n")) + assert_that(output, not_(contains_string(" orphaned:"))) + + def test_name_converted_in_functions_but_still_used_by_a_generic_class_is_in_both_sections( + self, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], + ) -> None: + """A name converted in some functions that a generic class still uses is listed as modified and as needing review.""" + target = tmp_path / "mod.py" + target.write_text( + textwrap.dedent("""\ + from typing import Generic, TypeVar + + T = TypeVar("T") + + + def first(items: list[T]) -> T: + return items[0] + + + class Box(Generic[T]): + def get(self, x: T) -> T: + return x + """), + encoding="utf-8", + ) + + migration.main([str(target), "--py", "3.12", "--no-ruff"]) + + output = capsys.readouterr().out + modified, needs_review = output.split("NEEDS MANUAL REVIEW (") + assert_that(output, contains_string("1 modified, 1 need manual review")) + assert_that(modified, contains_string(" T: converted to PEP 695 in functions\n")) + assert_that(needs_review, contains_string(" T: only used inside a class that is generic over it\n")) class TestConsoleReportModifiedSection: """main(): what the MODIFIED section lists per file.""" - def test_converted_name_is_not_listed_again_as_orphaned(self, tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None: - """A converted name is listed once; only declarations removed without a conversion appear under orphaned.""" + def test_converted_name_is_not_listed_again_as_removed(self, tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None: + """A converted name is listed once; only declarations removed without a conversion are listed as removed.""" target = tmp_path / "mod.py" target.write_text( textwrap.dedent("""\ @@ -251,77 +266,58 @@ def g[U](y: U) -> U: migration.main([str(target), "--py", "3.12", "--no-ruff"]) output = capsys.readouterr().out - assert_that(output, contains_string(" converted: T\n")) - assert_that(output, contains_string(" orphaned: U\n")) + assert_that(output, contains_string(" T: converted to PEP 695 in functions\n")) + assert_that(output, not_(contains_string(" T: unused declaration removed"))) + assert_that(output, contains_string(" U: unused declaration removed\n")) + + +def _table_rows(output: str, first_header: str) -> list[list[str]]: + """Return the data rows, as stripped cells, of the printed Markdown table whose header starts with first_header.""" + rows = [[cell.strip() for cell in line.strip("|").split("|")] for line in output.splitlines() if line.startswith("|")] + start = next(index for index, row in enumerate(rows) if row[0] == first_header) + table: list[list[str]] = [] + for row in rows[start + 2 :]: + if row[0] in {"Kind", "Reason"}: + break + table.append(row) + return table class TestSummaryTables: - """count_by_kind and rank_reasons: the numbers behind the report's summary tables.""" + """main(): the summary tables at the top of the printed report.""" + + def test_counts_per_kind_and_reasons_by_frequency(self, tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None: + """A name counts once per file, needs review if it has a reason, errored files are skipped, reasons are ranked.""" + (tmp_path / "fixed.py").write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") + (tmp_path / "exported_1.py").write_text(UNSAFE_TYPEVAR_SOURCE, encoding="utf-8") + (tmp_path / "exported_2.py").write_text(UNSAFE_TYPEVAR_SOURCE, encoding="utf-8") + (tmp_path / "covariant.py").write_text( + textwrap.dedent("""\ + from typing import TypeVar - @pytest.mark.parametrize( - ("reports", "expected"), - [ - pytest.param( - [ - migration.FileReport( - path=Path("a.py"), - result={"cross_file": {}, "converted": {"T": "fixed", "P": "fixed"}, "orphaned": {"T": "unsafe", "Ts": "unsafe"}}, - error=None, - kinds={"T": "TypeVar", "P": "ParamSpec", "Ts": "TypeVarTuple"}, - ), - ], - {"TypeVar": (1, 0, 1), "ParamSpec": (1, 1, 0), "TypeVarTuple": (1, 0, 1)}, - id="name-unsafe-in-any-phase-needs-review", - ), - pytest.param( - [ - migration.FileReport(path=Path("a.py"), result={"converted": {"T": "fixed"}}, error=None, kinds={"T": "TypeVar"}), - migration.FileReport(path=Path("b.py"), result={"converted": {"T": "fixed"}}, error=None, kinds={"T": "TypeVar"}), - migration.FileReport(path=Path("c.py"), result=None, error="SyntaxError: x"), - ], - {"TypeVar": (2, 2, 0), "ParamSpec": (0, 0, 0), "TypeVarTuple": (0, 0, 0)}, - id="same-name-in-two-files-counts-twice-and-errors-are-skipped", - ), - ], - ) - def test_count_by_kind(self, reports: list[object], expected: dict[str, tuple[int, int, int]]) -> None: - """Each name counts once per file under its kind: fixed unless any phase left it unsafe.""" - counts = migration.count_by_kind(reports) - - assert_that({kind: (c.reported, c.fixed, c.needs_review) for kind, c in counts.items()}, equal_to(expected)) - - def test_rank_reasons_orders_by_count_then_by_declaration_order(self) -> None: - """Every (phase, name) reason counts; the most common comes first, a tie keeps the UnsafeReason declaration order.""" - reports = [ - migration.FileReport( - path=Path("a.py"), - result={}, - error=None, - reasons={"converted": {"T": UnsafeReason.USED_IN_GENERIC_CLASS}, "orphaned": {"T": UnsafeReason.DECLARED_TYPEVAR_EXPORTED}}, - ), - migration.FileReport( - path=Path("b.py"), - result={}, - error=None, - reasons={ - "orphaned": { - "U": UnsafeReason.DECLARED_TYPEVAR_EXPORTED, - "V": UnsafeReason.USED_IN_GENERIC_CLASS, - "W": UnsafeReason.PEP695_VERSION_GATE, - }, - }, - ), - ] + T_co = TypeVar("T_co", covariant=True) + + + def first(x: T_co) -> T_co: + return x + """), + encoding="utf-8", + ) + (tmp_path / "broken.py").write_text("def broken(:\n", encoding="utf-8") - ranking = migration.rank_reasons(reports) + migration.main([str(tmp_path), "--py", "3.12", "--no-ruff"]) + output = capsys.readouterr().out assert_that( - ranking, + _table_rows(output, "Kind"), + equal_to([["TypeVar", "4", "1", "3"], ["ParamSpec", "0", "0", "0"], ["TypeVarTuple", "0", "0", "0"]]), + ) + assert_that( + _table_rows(output, "Reason"), equal_to( [ - (UnsafeReason.DECLARED_TYPEVAR_EXPORTED, 2), - (UnsafeReason.USED_IN_GENERIC_CLASS, 2), - (UnsafeReason.PEP695_VERSION_GATE, 1), + [UNSAFE_RULES[UnsafeReason.DECLARED_TYPEVAR_EXPORTED].message, "2"], + [UNSAFE_RULES[UnsafeReason.NO_PEP695_EQUIVALENT].message, "1"], ], ), ) @@ -369,27 +365,14 @@ def test_one_bad_file_does_not_abort_the_batch( 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: + def test_consumer_is_localized_and_origin_declaration_survives(self, tmp_path: Path) -> None: """The importing file gets a PEP 695 local TypeVar, and the origin declaration is not removed.""" + import_line = "from pkg.typing_mod import T" 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 = pkg / "client.py" 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"]) @@ -420,19 +403,6 @@ def test_origin_declaration_survives_with_relative_target( assert_that((pkg / "typing_mod.py").read_text(encoding="utf-8"), contains_string('T = TypeVar("T")')) assert_that((pkg / "client.py").read_text(encoding="utf-8"), contains_string("def use[T](x: T) -> T:")) - def test_origin_declaration_survives_module_attribute_access(self, tmp_path: Path) -> None: - """A TypeVar accessed as a module attribute by another file is kept at its origin.""" - pkg = tmp_path / "pkg" - pkg.mkdir() - (pkg / "__init__.py").write_text("", encoding="utf-8") - (pkg / "typing_mod.py").write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") - (pkg / "client.py").write_text("import pkg.typing_mod\n\ndef use(x: pkg.typing_mod.T) -> None: ...\n", encoding="utf-8") - - exit_code = migration.main([str(tmp_path), "--py", "3.12"]) - - assert_that(exit_code, equal_to(0)) - assert_that((pkg / "typing_mod.py").read_text(encoding="utf-8"), contains_string('T = TypeVar("T")')) - class TestPyVersionFlag: """main(): the required --py flag alone sets the target's minimum Python version.""" @@ -460,7 +430,6 @@ def test_py_flag_gates_rewrites(self, tmp_path: Path, py_version: str, expected: ("bad_args", "expected_error"), [ pytest.param([], "the following arguments are required: --py", id="flag-missing"), - pytest.param(["--py", "3"], "expected MAJOR.MINOR (e.g. 3.12), got '3'", id="missing-minor"), pytest.param(["--py", "3.x"], "expected MAJOR.MINOR (e.g. 3.12), got '3.x'", id="non-numeric"), ], ) From f0beca3abd5c1c48dc222dae8f8df468bf052528 Mon Sep 17 00:00:00 2001 From: Francesco Pezzella Date: Mon, 19 Oct 2026 15:22:11 +0200 Subject: [PATCH 14/25] Type-Parameter-Modernization doc fix regarding Ruff change --- docs/user/features/type-parameter-modernization.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/user/features/type-parameter-modernization.md b/docs/user/features/type-parameter-modernization.md index d9084837..fec0b034 100644 --- a/docs/user/features/type-parameter-modernization.md +++ b/docs/user/features/type-parameter-modernization.md @@ -237,8 +237,8 @@ class Box(Generic[T]): The file is unchanged. -**To fix this yourself:** `ruff`'s `UP046` rewrites `Generic[T]` and `Protocol[T]` to `class Box[T]:`. Run the tool -again afterwards: it removes the now unused `T = TypeVar("T")`. `UP046` does not touch a `T` in any other base, so +**To fix this yourself:** `ruff`'s `UP046` rewrites `Generic[T]` to `class Box[T]:`. Run the tool again afterwards: +it removes the now unused `T = TypeVar("T")`. `UP046` does not touch `Protocol[T]` or a `T` in any other base, so write `class Box[T](MutableSet[T]):` by hand. ### A name the declaration uses can't be imported here { #feature-type-parameter-modernization-declaration-name-unavailable } From f02aa5b522fbfaaa7d0d41a2653050301d6dc295 Mon Sep 17 00:00:00 2001 From: Francesco Pezzella Date: Mon, 19 Oct 2026 16:59:20 +0200 Subject: [PATCH 15/25] Convert TypeVars declared with covariant=/contravariant= when only functions use them, since variance applies to generic classes only. --- docs/developer/feature-test-map/core.md | 1 + docs/developer/modules/recipes.md | 3 +- .../features/type-parameter-modernization.md | 25 +-- src/renaissance/recipes/type_var_check.py | 1 + src/renaissance/recipes/type_var_domain.py | 17 +- test/recipes/test_type_var_check_convert.py | 63 +++--- test/recipes/test_type_var_check_variance.py | 195 ++++++++++++++++++ .../test_migration_type_recipes.py | 13 +- 8 files changed, 254 insertions(+), 64 deletions(-) create mode 100644 test/recipes/test_type_var_check_variance.py diff --git a/docs/developer/feature-test-map/core.md b/docs/developer/feature-test-map/core.md index be229dbb..5569859f 100644 --- a/docs/developer/feature-test-map/core.md +++ b/docs/developer/feature-test-map/core.md @@ -46,6 +46,7 @@ https://github.com/TNO/Renaissance.Py/blob/main/LICENSE - `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_check_variance.py` - `test/recipes/test_type_var_domain.py` - `test/recipes/test_step_runner.py` - `test/recipes/test_python_refactoring.py` diff --git a/docs/developer/modules/recipes.md b/docs/developer/modules/recipes.md index 3f71e624..50b2f2e9 100644 --- a/docs/developer/modules/recipes.md +++ b/docs/developer/modules/recipes.md @@ -60,7 +60,8 @@ safely cover. This page covers `TypeVarCheck`, the recipe behind - **Version gates.** `_conversion_refusal` checks `min_python` against `PEP_695_MINIMUM = (3, 12)` and, for a `default=`, `PEP_696_MINIMUM = (3, 13)`. `None` never passes. A keyword outside `_CONVERTIBLE_KEYWORDS` is refused - (`NO_PEP695_EQUIVALENT`) instead of dropped. + (`NO_PEP695_EQUIVALENT`) instead of dropped. `covariant` and `contravariant` are in the set and are dropped, because + a function's type parameter has no variance; converting classes would have to refuse them. - **Uses.** `uses_of` takes a declaration's uses from libcst's scope analysis (`referenced_by`). A use inside a string annotation or `cast("T", x)` counts; a `T` that a PEP 695 type parameter of an enclosing function or class diff --git a/docs/user/features/type-parameter-modernization.md b/docs/user/features/type-parameter-modernization.md index fec0b034..24db0423 100644 --- a/docs/user/features/type-parameter-modernization.md +++ b/docs/user/features/type-parameter-modernization.md @@ -31,7 +31,8 @@ The tool runs three phases: 1. **Localize cross-file imports.** A type parameter imported from another file in the project (`from .shapes import T`) is replaced by an exact local copy of its declaration, placed after the file's imports. 2. **Convert to PEP 695.** Every function whose signature uses a declared type parameter gets its own `[T]`, with - bounds, constraints and `default=` carried over (`def f[T: str = str](...)`). A nested function gets none when an + bounds, constraints and `default=` carried over (`def f[T: str = str](...)`). `covariant=` and `contravariant=` are + dropped, since only generic classes have a variance. A nested function gets none when an enclosing function's signature uses `T` too. If `T` is also used outside functions (a `Generic[T]` base, an alias), the declaration is kept. A function that also uses a type parameter that can't be converted is left unchanged. @@ -164,20 +165,19 @@ With `--py 3.12` the file is unchanged. With `--py 3.13` it becomes `def first[T ### A declaration passes an argument with no PEP 695 equivalent { #feature-type-parameter-modernization-no-pep695-equivalent } -The `[...]` syntax cannot write `covariant=`/`contravariant=`, a `bound=` on a `ParamSpec` or `TypeVarTuple`, a -`**mapping` of options, or a keyword added by a later Python version. (`infer_variance=True` is fine: a PEP 695 -type parameter always infers its variance.) +The `[...]` syntax cannot write a `bound=` on a `ParamSpec` or `TypeVarTuple`, a `**mapping` of options, or a keyword +added by a later Python version. (`covariant=`, `contravariant=` and `infer_variance=` are fine: a function's type +parameter has no variance.) ```python -T_co = TypeVar("T_co", covariant=True) +P = ParamSpec("P", bound=int) -def first(items: Sequence[T_co]) -> T_co: ... +def run(func: Callable[P, int]) -> Callable[P, int]: ... ``` The file is unchanged. -**To fix this yourself:** variance only matters for generic classes. If only functions use the type parameter, -remove `covariant=`/`contravariant=` and run again. +**To fix this yourself:** keep the declaration. ### A function also uses a type variable that can't be converted { #feature-type-parameter-modernization-shares-function-with-unconvertible } @@ -187,14 +187,14 @@ declaration refused for a version gate or an argument with no PEP 695 equivalent legacy. A legacy type parameter of an enclosing generic class is not the method's own, so it blocks nothing. ```python -T_co = TypeVar("T_co", covariant=True) +P = ParamSpec("P", bound=int) Ts = TypeVarTuple("Ts") -def run(func: Callable[[*Ts], T_co], *args: *Ts) -> T_co: ... +def run(func: Callable[P, tuple[*Ts]]) -> Callable[P, tuple[*Ts]]: ... ``` -The file is unchanged. `T_co` is reported for its own reason. `Ts` is reported for this one, because every function -using it is left unchanged. Functions that use `Ts` without `T_co` are still converted. +The file is unchanged. `P` is reported for its own reason. `Ts` is reported for this one, because every function +using it is left unchanged. Functions that use `Ts` without `P` are still converted. **To fix this yourself:** fix the refused declaration as its section describes, then run again. @@ -328,6 +328,7 @@ run the tool again. - `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_check_variance.py` - `test/recipes/test_type_var_domain.py` - `test/recipes/test_step_runner.py` - `test/recipes/test_python_refactoring.py` diff --git a/src/renaissance/recipes/type_var_check.py b/src/renaissance/recipes/type_var_check.py index 4d1874c2..41303573 100644 --- a/src/renaissance/recipes/type_var_check.py +++ b/src/renaissance/recipes/type_var_check.py @@ -151,6 +151,7 @@ def convert_declared_typevars(self) -> dict[str, str]: continue classes = generic_classes(uses) + # TODO: convert a class with the name in a base (`class Box(Base[T])`) to `class Box[T](Base[T])`, still refusing `covariant=`. in_generic_class = [function for function in functions if self._is_inside(function, classes)] candidates = [ function diff --git a/src/renaissance/recipes/type_var_domain.py b/src/renaissance/recipes/type_var_domain.py index 101c838d..40806f5f 100644 --- a/src/renaissance/recipes/type_var_domain.py +++ b/src/renaissance/recipes/type_var_domain.py @@ -56,7 +56,7 @@ class UnsafeRule: "feature-type-parameter-modernization-pep696-version-gate", ), UnsafeReason.NO_PEP695_EQUIVALENT: UnsafeRule( - "declaration passes an argument with no PEP 695 equivalent, e.g. covariant=True or bound= on a ParamSpec", + "declaration passes an argument with no PEP 695 equivalent, e.g. bound= on a ParamSpec or a **mapping", "feature-type-parameter-modernization-no-pep695-equivalent", ), UnsafeReason.DECLARED_TYPEVAR_EXPORTED: UnsafeRule( @@ -378,10 +378,10 @@ def declares_type_param(function: cst.FunctionDef, name: str) -> bool: return parameters is not None and any(parameter.param.name.value == name for parameter in parameters.params) -# infer_variance needs no syntax of its own: a PEP 695 type parameter always infers its variance. +# Variance needs no syntax of its own: a PEP 695 type parameter always infers it, and a function's has none. _CONVERTIBLE_KEYWORDS: dict[str, frozenset[str]] = { - "TypeVar": frozenset({"bound", "default", "infer_variance"}), - "ParamSpec": frozenset({"default", "infer_variance"}), + "TypeVar": frozenset({"bound", "default", "infer_variance", "covariant", "contravariant"}), + "ParamSpec": frozenset({"default", "infer_variance", "covariant", "contravariant"}), "TypeVarTuple": frozenset({"default", "infer_variance"}), } @@ -394,8 +394,8 @@ def declared_default(decl_stmt: cst.Assign) -> cst.BaseExpression | None: def has_unconvertible_argument(decl_stmt: cst.Assign) -> bool: """Return True if a legacy declaration passes a keyword type_param_text can't carry over. - That is any keyword outside the constructor's known set, e.g. `covariant=`/`contravariant=`, - `bound=` on a ParamSpec or TypeVarTuple, a keyword added by a later Python version, or a `**mapping`. + That is any keyword outside the constructor's known set, e.g. `bound=` on a ParamSpec or TypeVarTuple, + a keyword added by a later Python version, or a `**mapping`. """ convertible = _CONVERTIBLE_KEYWORDS[type_param_constructor_name(decl_stmt)] return any( @@ -408,9 +408,8 @@ def type_param_text(decl_stmt: cst.Assign, module: cst.Module) -> str: """Return the PEP 695 type parameter equivalent to a legacy declaration from `module`, in its original formatting. E.g. `T = TypeVar("T", bound=int, default=int)` gives `T: int = int`. Bounds, constraints and - `default=` are carried over; `infer_variance=` needs nothing, since a PEP 695 type parameter always - infers its variance. Callers must first reject declarations for which - has_unconvertible_argument() is True. + `default=` are carried over; `infer_variance=`, `covariant=` and `contravariant=` are dropped. Callers must + first reject declarations for which has_unconvertible_argument() is True. """ call = _call(decl_stmt) constructor = type_param_constructor_name(decl_stmt) diff --git a/test/recipes/test_type_var_check_convert.py b/test/recipes/test_type_var_check_convert.py index 2955df12..2f685361 100644 --- a/test/recipes/test_type_var_check_convert.py +++ b/test/recipes/test_type_var_check_convert.py @@ -405,15 +405,15 @@ def get(self, x: T) -> T: ("functions", "expected_converted", "expected_unchanged"), [ pytest.param( - "def f(x: U) -> U:\n return x\ndef g(x: U, y: T_co) -> T_co:\n return y\n", + "def f(x: U) -> U:\n return x\ndef g(x: U, y: Bad) -> Bad:\n return y\n", "def f[U](x: U) -> U:", - "def g(x: U, y: T_co) -> T_co:", + "def g(x: U, y: Bad) -> Bad:", id="functions", ), pytest.param( - "class Util:\n def f(self, x: U) -> U:\n return x\n def g(self, x: U, y: T_co) -> T_co:\n return y\n", + "class Util:\n def f(self, x: U) -> U:\n return x\n def g(self, x: U, y: Bad) -> Bad:\n return y\n", " def f[U](self, x: U) -> U:", - " def g(self, x: U, y: T_co) -> T_co:", + " def g(self, x: U, y: Bad) -> Bad:", id="methods", ), ], @@ -422,12 +422,10 @@ def test_converts_only_functions_free_of_unconvertible_typevars( self, create_type_var_check: Callable[[str], TypeVarCheck], functions: str, expected_converted: str, expected_unchanged: str ) -> None: """Verify a function that also uses an unconvertible TypeVar is left whole, while the others using U get [U].""" - subject = create_type_var_check( - f'from typing import TypeVar\n\nT_co = TypeVar("T_co", covariant=True)\nU = TypeVar("U")\n\n{functions}' - ) + subject = create_type_var_check(f'from typing import TypeVar\n\nBad = TypeVar("Bad", future_kw=1)\nU = TypeVar("U")\n\n{functions}') result = subject.convert_declared_typevars() - assert_that(result, equal_to({"T_co": "unsafe", "U": "fixed"})) + assert_that(result, equal_to({"Bad": "unsafe", "U": "fixed"})) output = subject.apply_to_string() assert_that(output, contains_string(expected_converted)) assert_that(output, contains_string(expected_unchanged)) @@ -508,6 +506,20 @@ class TestTypeVarCheckConvertArguments: id="typevar-constraints-default", ), pytest.param('T = TypeVar("T", infer_variance=True)', _TYPEVAR_USE, "def a[T](x: T) -> T:", id="typevar-infer-variance"), + pytest.param('T = TypeVar("T", covariant=True)', _TYPEVAR_USE, "def a[T](x: T) -> T:", id="typevar-covariant"), + pytest.param('T = TypeVar("T", contravariant=True)', _TYPEVAR_USE, "def a[T](x: T) -> T:", id="typevar-contravariant"), + pytest.param( + 'T = TypeVar("T", bound=int, covariant=True)', + _TYPEVAR_USE, + "def a[T: int](x: T) -> T:", + id="typevar-bound-covariant", + ), + pytest.param( + 'P = ParamSpec("P", covariant=True)', + _PARAMSPEC_USE, + "def a[**P](f: Callable[P, int]) -> Callable[P, int]:", + id="paramspec-covariant", + ), pytest.param( 'P = ParamSpec("P")', _PARAMSPEC_USE, @@ -536,7 +548,7 @@ def test_converts_keeping_every_expressible_argument( signature: str, expected_signature: str, ) -> None: - """Verify bounds, constraints, defaults and inferred variance are carried over.""" + """Verify bounds, constraints and defaults are carried over, and variance arguments are dropped.""" name = declaration.split(" = ", maxsplit=1)[0] subject = create_type_var_check(_source(declaration, signature)) subject.min_python = PEP_696_MINIMUM @@ -580,32 +592,11 @@ def test_converts_keeping_every_expressible_argument( id="typevartuple-default-3.12", ), pytest.param( - 'T = TypeVar("T", covariant=True)', - _TYPEVAR_USE, - PEP_696_MINIMUM, - UnsafeReason.NO_PEP695_EQUIVALENT, - id="typevar-covariant", - ), - pytest.param( - 'T = TypeVar("T", contravariant=True)', - _TYPEVAR_USE, - PEP_696_MINIMUM, - UnsafeReason.NO_PEP695_EQUIVALENT, - id="typevar-contravariant", - ), - pytest.param( - 'P = ParamSpec("P", covariant=True)', - _PARAMSPEC_USE, - PEP_696_MINIMUM, - UnsafeReason.NO_PEP695_EQUIVALENT, - id="paramspec-covariant", - ), - pytest.param( - 'Ts = TypeVarTuple("Ts", bound=int, covariant=True)', + 'Ts = TypeVarTuple("Ts", bound=int)', _TYPEVARTUPLE_USE, PEP_696_MINIMUM, UnsafeReason.NO_PEP695_EQUIVALENT, - id="typevartuple-bound-covariant", + id="typevartuple-bound", ), pytest.param( 'P = ParamSpec("P", bound=int)', @@ -654,11 +645,11 @@ def test_refuses_argument_it_cannot_carry_over( ("declarations", "signature", "min_python", "expected_reasons"), [ pytest.param( - 'T_co = TypeVar("T_co", covariant=True)\nTs = TypeVarTuple("Ts")', - "def a(func: Callable[[*Ts], T_co], *args: *Ts) -> T_co:", + 'Bad = TypeVar("Bad", future_kw=1)\nTs = TypeVarTuple("Ts")', + "def a(func: Callable[[*Ts], Bad], *args: *Ts) -> Bad:", PEP_696_MINIMUM, - {"T_co": UnsafeReason.NO_PEP695_EQUIVALENT, "Ts": UnsafeReason.SHARES_FUNCTION_WITH_UNCONVERTIBLE}, - id="covariant-typevar-with-typevartuple", + {"Bad": UnsafeReason.NO_PEP695_EQUIVALENT, "Ts": UnsafeReason.SHARES_FUNCTION_WITH_UNCONVERTIBLE}, + id="unconvertible-typevar-with-typevartuple", ), pytest.param( 'T = TypeVar("T")\nD = TypeVar("D", default=int)', diff --git a/test/recipes/test_type_var_check_variance.py b/test/recipes/test_type_var_check_variance.py new file mode 100644 index 00000000..72e0c160 --- /dev/null +++ b/test/recipes/test_type_var_check_variance.py @@ -0,0 +1,195 @@ +# Copyright (C) 2024-2026 TNO +# +# Licensed under the terms specified in the LICENSE file in the root directory +# of this repository, also available at: +# https://github.com/TNO/Renaissance.Py/blob/main/LICENSE + +"""Tests that dropping `covariant=`/`contravariant=` is safe for functions and unsafe for classes. + +The type checker (pyright) and the interpreter judge the code before and after TypeVarCheck.convert_declared_typevars. +""" + +import importlib.util +import json +import subprocess +import sys +from typing import TYPE_CHECKING, NamedTuple + +import pytest +from hamcrest import assert_that, contains_string, equal_to, has_item, has_property, not_ + +from renaissance.recipes.type_var_domain import UnsafeReason + +# TODO(FrancescoPezzella): Replace with a lazy import in Python 3.15. +# https://github.com/TNO/Renaissance.Py/issues/275 +if TYPE_CHECKING: + from collections.abc import Callable + from pathlib import Path + from types import ModuleType + + from renaissance.recipes.type_var_check import TypeVarCheck + + +class _Diagnostic(NamedTuple): + """One pyright finding, without the file it was found in.""" + + line: int + severity: str + rule: str + message: str + + +def _write(source: str, path: Path) -> Path: + """Write `source` to `path` and return `path`.""" + path.write_text(source, encoding="utf-8") + return path + + +def _type_check(path: Path) -> list[_Diagnostic]: + """Return every diagnostic pyright reports for the file at `path`, `reveal_type` results included.""" + command = [sys.executable, "-m", "pyright", "--pythonpath", sys.executable, "--outputjson", str(path)] + result = subprocess.run(command, capture_output=True, text=True, encoding="utf-8", cwd=path.parent, check=False) # noqa: S603 - fixed argv + report = json.loads(result.stdout[result.stdout.index("{") :]) + return [ + _Diagnostic(item["range"]["start"]["line"] + 1, item["severity"], item.get("rule", ""), item["message"]) + for item in report["generalDiagnostics"] + ] + + +def _import(path: Path) -> ModuleType: + """Import the file at `path` as a module.""" + spec = importlib.util.spec_from_file_location(path.stem, path) + if spec is None or spec.loader is None: + message = f"could not load {path}" + raise RuntimeError(message) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +_FUNCTION_TEMPLATE = """\ +from collections.abc import Callable +from typing import TypeVar, reveal_type + +{declaration} + + +def first(x: T) -> T: + return x + + +reveal_type(first(1)) +reveal_type(first("a")) +reveal_type(first([1])) +accepted: Callable[[bool], int] = first +rejected: Callable[[int], str] = first +""" + +_PROTOCOL = """\ +class Reader(Protocol[T_co]): + def read(self) -> T_co: ... +""" + +_GENERIC_CLASS = """\ +class Reader(Generic[T_co]): + def __init__(self, value: T_co) -> None: + self._value = value + + def read(self) -> T_co: + return self._value +""" + +_CLASS_TEMPLATE = """\ +from typing import Generic, Protocol, TypeVar, reveal_type + +T_co = TypeVar("T_co"{covariant}) + + +{class_definition} + +def read_twice(reader: Reader[T_co]) -> tuple[T_co, T_co]: + return reader.read(), reader.read() + + +def widen(reader: Reader[int]) -> None: + wider: Reader[object] = reader + reveal_type(wider) +""" + + +class TestVarianceOfFunctions: + """A function's TypeVar has no variance, so dropping it must not change anything.""" + + @pytest.mark.parametrize( + "declaration", + [ + pytest.param('T = TypeVar("T", covariant=True)', id="covariant"), + pytest.param('T = TypeVar("T", contravariant=True)', id="contravariant"), + pytest.param('T = TypeVar("T", bound=int, covariant=True)', id="bound-and-covariant"), + ], + ) + def test_function_behaves_the_same_without_the_variance( + self, + create_type_var_check: Callable[[str], TypeVarCheck], + tmp_path: Path, + declaration: str, + ) -> None: + """Verify the type checker accepts, rejects and infers exactly as before, and the function returns the same values.""" + before = _FUNCTION_TEMPLATE.format(declaration=declaration) + subject = create_type_var_check(before) + + result = subject.convert_declared_typevars() + + after = subject.apply_to_string() + assert_that(result, equal_to({"T": "fixed"})) + assert_that(after, contains_string("def first[T")) + before_path = _write(before, tmp_path / "before.py") + after_path = _write(after, tmp_path / "after.py") + before_findings = _type_check(before_path) + assert_that(before_findings, has_item(has_property("message", contains_string('Type of "first(1)" is "int"')))) + assert_that(before_findings, has_item(has_property("rule", "reportAssignmentType"))) + assert_that(_type_check(after_path), equal_to(before_findings)) + values = (1, "a", [1]) + assert_that( + [_import(after_path).first(value) for value in values], + equal_to([_import(before_path).first(value) for value in values]), + ) + + +class TestVarianceOfClasses: + """A generic class's variance changes what the type checker accepts, so only functions may lose it.""" + + @pytest.mark.parametrize( + ("class_definition", "class_header"), + [ + pytest.param(_PROTOCOL, "class Reader(Protocol[T_co]):", id="protocol"), + pytest.param(_GENERIC_CLASS, "class Reader(Generic[T_co]):", id="generic-class"), + ], + ) + def test_class_keeps_its_variance_and_dropping_it_changes_the_type_checking( + self, + create_type_var_check: Callable[[str], TypeVarCheck], + tmp_path: Path, + class_definition: str, + class_header: str, + ) -> None: + """Verify the class and its covariant declaration stay, the code still type checks, and dropping the variance does not.""" + covariant = _CLASS_TEMPLATE.format(covariant=", covariant=True", class_definition=class_definition) + subject = create_type_var_check(covariant) + + result = subject.convert_declared_typevars() + + converted = subject.apply_to_string() + assert_that(result, equal_to({"T_co": "fixed"})) + assert_that(subject.converted_unsafe_reasons, equal_to({"T_co": UnsafeReason.USED_IN_GENERIC_CLASS})) + assert_that(converted, contains_string("def read_twice[T_co](reader: Reader[T_co])")) + assert_that(converted, contains_string('T_co = TypeVar("T_co", covariant=True)')) + assert_that(converted, contains_string(class_header)) + original_findings = _type_check(_write(covariant, tmp_path / "original.py")) + assert_that(original_findings, has_item(has_property("message", contains_string('Type of "wider" is "Reader[int]"')))) + assert_that(_type_check(_write(converted, tmp_path / "converted.py")), equal_to(original_findings)) + assert_that([finding.rule for finding in original_findings], not_(has_item("reportAssignmentType"))) + + invariant = converted.replace(", covariant=True", "") + invariant_rules = [finding.rule for finding in _type_check(_write(invariant, tmp_path / "invariant.py"))] + assert_that(invariant_rules, has_item("reportAssignmentType")) diff --git a/test/rejuvenation/test_migration_type_recipes.py b/test/rejuvenation/test_migration_type_recipes.py index af9d9e8d..8d5a1c8e 100644 --- a/test/rejuvenation/test_migration_type_recipes.py +++ b/test/rejuvenation/test_migration_type_recipes.py @@ -291,15 +291,16 @@ def test_counts_per_kind_and_reasons_by_frequency(self, tmp_path: Path, capsys: (tmp_path / "fixed.py").write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") (tmp_path / "exported_1.py").write_text(UNSAFE_TYPEVAR_SOURCE, encoding="utf-8") (tmp_path / "exported_2.py").write_text(UNSAFE_TYPEVAR_SOURCE, encoding="utf-8") - (tmp_path / "covariant.py").write_text( + (tmp_path / "bound_paramspec.py").write_text( textwrap.dedent("""\ - from typing import TypeVar + from collections.abc import Callable + from typing import ParamSpec - T_co = TypeVar("T_co", covariant=True) + P = ParamSpec("P", bound=int) - def first(x: T_co) -> T_co: - return x + def wrap(f: Callable[P, int]) -> Callable[P, int]: + return f """), encoding="utf-8", ) @@ -310,7 +311,7 @@ def first(x: T_co) -> T_co: output = capsys.readouterr().out assert_that( _table_rows(output, "Kind"), - equal_to([["TypeVar", "4", "1", "3"], ["ParamSpec", "0", "0", "0"], ["TypeVarTuple", "0", "0", "0"]]), + equal_to([["TypeVar", "3", "1", "2"], ["ParamSpec", "1", "0", "1"], ["TypeVarTuple", "0", "0", "0"]]), ) assert_that( _table_rows(output, "Reason"), From d28bdd56a11cce5833a9a18abd73226cf3a62904 Mon Sep 17 00:00:00 2001 From: Francesco Pezzella Date: Mon, 19 Oct 2026 17:19:27 +0200 Subject: [PATCH 16/25] List a type parameter that is localized and then removed only as an unused declaration removed in the migration report. --- docs/user/features/type-parameter-modernization.md | 3 ++- src/rejuvenation/migration-type-recipes.py | 6 ++++-- test/rejuvenation/test_migration_type_recipes.py | 4 +++- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/docs/user/features/type-parameter-modernization.md b/docs/user/features/type-parameter-modernization.md index 24db0423..29c35be1 100644 --- a/docs/user/features/type-parameter-modernization.md +++ b/docs/user/features/type-parameter-modernization.md @@ -364,7 +364,8 @@ The report has a summary line, two summary tables and the sections `MODIFIED`, ` The tables count the type parameters per kind (`TypeVar`, `ParamSpec`, `TypeVarTuple`) and rank the reasons for manual review from most to least common. A name counts once per file, as needing review if it has a reason to review. `MODIFIED` says what was done to each name: converted in functions, import replaced by a local declaration, or unused declaration -removed. `NEEDS MANUAL REVIEW` gives each name its reason, and a name can be in both sections. Files are +removed; a name localized and then removed is listed only as removed. `NEEDS MANUAL REVIEW` gives each name its +reason, and a name can be in both sections. Files are listed relative to ``. Exit codes: 0 when the run completes (manual review is not a failure), 2 for a usage error, 3 if any file raised an exception. diff --git a/src/rejuvenation/migration-type-recipes.py b/src/rejuvenation/migration-type-recipes.py index fa6ac6fc..8a93d9fb 100644 --- a/src/rejuvenation/migration-type-recipes.py +++ b/src/rejuvenation/migration-type-recipes.py @@ -247,15 +247,17 @@ def _join_with_blank_lines(entries: list[list[str]]) -> list[str]: def _modified_entry(report: FileReport, project_root: Path) -> list[str]: """Return the MODIFIED lines for one file: its path relative to project_root, then what was done to each name. - A converted name isn't described as removed, because its declaration is always removed afterwards. + A converted name isn't described as removed, because its declaration is always removed afterwards, + and a removed name isn't described as localized. """ lines = [f" {report.path.relative_to(project_root)}"] result = report.result or {} converted = {name for name, status in result.get("converted", {}).items() if status == "fixed"} + removed = {name for name, status in result.get("orphaned", {}).items() if status == "fixed"} - converted descriptions: dict[str, list[str]] = {} for phase, names in result.items(): for name, status in names.items(): - if status == "fixed" and not (phase == "orphaned" and name in converted): + if status == "fixed" and not (phase == "orphaned" and name in converted) and not (phase == "cross_file" and name in removed): descriptions.setdefault(name, []).append(_FIXED_DESCRIPTIONS[phase]) lines.extend(f" {name}: {', '.join(texts)}" for name, texts in descriptions.items()) return lines diff --git a/test/rejuvenation/test_migration_type_recipes.py b/test/rejuvenation/test_migration_type_recipes.py index 8d5a1c8e..4d552b0e 100644 --- a/test/rejuvenation/test_migration_type_recipes.py +++ b/test/rejuvenation/test_migration_type_recipes.py @@ -366,7 +366,7 @@ def test_one_bad_file_does_not_abort_the_batch( class TestMainProjectWideImportSafety: """main(): a TypeVar imported by another file in the batch is localized there and kept at its origin.""" - def test_consumer_is_localized_and_origin_declaration_survives(self, tmp_path: Path) -> None: + def test_consumer_is_localized_and_origin_declaration_survives(self, tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None: """The importing file gets a PEP 695 local TypeVar, and the origin declaration is not removed.""" import_line = "from pkg.typing_mod import T" pkg = tmp_path / "pkg" @@ -375,6 +375,7 @@ def test_consumer_is_localized_and_origin_declaration_survives(self, tmp_path: P (pkg / "typing_mod.py").write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") consumer = pkg / "client.py" consumer.write_text(f"{import_line}\n\ndef use(x: T) -> T:\n return x\n", encoding="utf-8") + (pkg / "unused.py").write_text(f"{import_line}\n", encoding="utf-8") exit_code = migration.main([str(tmp_path), "--py", "3.12"]) @@ -382,6 +383,7 @@ def test_consumer_is_localized_and_origin_declaration_survives(self, tmp_path: P consumer_text = consumer.read_text(encoding="utf-8") assert_that(consumer_text, contains_string("def use[T](x: T) -> T:")) assert_that(consumer_text, not_(contains_string(import_line))) + assert_that(capsys.readouterr().out, contains_string(" T: unused declaration removed\n")) 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")]) From 24901765348c6c518c53f1f24f21d708de66450c Mon Sep 17 00:00:00 2001 From: Francesco Pezzella Date: Mon, 19 Oct 2026 18:16:16 +0200 Subject: [PATCH 17/25] Rename the declaration-name-unavailable reason to name-not-at-module-level and explain its two causes. --- docs/developer/modules/recipes.md | 2 +- .../user/features/type-parameter-modernization.md | 15 +++++++++------ src/renaissance/recipes/type_var_check.py | 8 ++++---- src/renaissance/recipes/type_var_domain.py | 8 ++++---- test/recipes/test_type_var_check_localize.py | 4 ++-- 5 files changed, 20 insertions(+), 17 deletions(-) diff --git a/docs/developer/modules/recipes.md b/docs/developer/modules/recipes.md index 50b2f2e9..c6aeabc4 100644 --- a/docs/developer/modules/recipes.md +++ b/docs/developer/modules/recipes.md @@ -90,7 +90,7 @@ safely cover. This page covers `TypeVarCheck`, the recipe behind - **Orphans.** A declaration with no uses left (`uses_of` is empty) is orphaned. Conversion skips such names too. - **Localization.** The origin's declaration is copied as written, with the comment trailing it when it is alone on - its line (`declaration_text`). `DECLARATION_NAME_UNAVAILABLE` if the origin only binds a name its arguments use + its line (`declaration_text`). `NAME_NOT_AT_MODULE_LEVEL` if the origin only binds a name its arguments use (`declaration_argument_names`) under `if TYPE_CHECKING:` or this file binds it to something else (`from_import_sources`). All names localized from one import statement are handled together. diff --git a/docs/user/features/type-parameter-modernization.md b/docs/user/features/type-parameter-modernization.md index 29c35be1..06c99b5f 100644 --- a/docs/user/features/type-parameter-modernization.md +++ b/docs/user/features/type-parameter-modernization.md @@ -241,11 +241,14 @@ The file is unchanged. it removes the now unused `T = TypeVar("T")`. `UP046` does not touch `Protocol[T]` or a `T` in any other base, so write `class Box[T](MutableSet[T]):` by hand. -### A name the declaration uses can't be imported here { #feature-type-parameter-modernization-declaration-name-unavailable } +### The declaration uses a name that isn't available at module level { #feature-type-parameter-modernization-name-not-at-module-level } -Localizing copies the declaration and imports the names it uses (`bound=Shape`) from the module the origin gets them -from, or from the origin when it defines them. That fails when the origin only imports `Shape` under -`if TYPE_CHECKING:`, or when this file already has a different `Shape`. +Localizing copies the declaration into this file, so every name it uses (`bound=Shape`) must be available at module +level, as the same object, in both files. The file is left unchanged when: + +- **The origin binds the name only under `if TYPE_CHECKING:`**, or inside a function or class. The copy could not + import it at runtime. +- **This file binds the name to something else.** It already has a different `Shape`. ```python # shapes.py @@ -264,8 +267,8 @@ def biggest(items: list[T]) -> T: ... `main.py` is unchanged and keeps importing `T` from `shapes.py`. -**To fix this yourself:** import the name the same way the origin does, or rename one of the two, then copy the -declaration by hand. +**To fix this yourself:** import the name at module level the same way the origin does, or rename one of the two, +then copy the declaration by hand. ### The origin imports `TypeVar` conditionally { #feature-type-parameter-modernization-origin-imports-constructor-conditionally } diff --git a/src/renaissance/recipes/type_var_check.py b/src/renaissance/recipes/type_var_check.py index 41303573..1d7fc0b1 100644 --- a/src/renaissance/recipes/type_var_check.py +++ b/src/renaissance/recipes/type_var_check.py @@ -344,7 +344,7 @@ def _localize_names_from( continue argument_imports = self._argument_imports(origin, origin_path, raw, decl_stmt) if argument_imports is None: - self._mark_unsafe(results, self.cross_file_unsafe_reasons, name, UnsafeReason.DECLARATION_NAME_UNAVAILABLE) + self._mark_unsafe(results, self.cross_file_unsafe_reasons, name, UnsafeReason.NAME_NOT_AT_MODULE_LEVEL) continue constructor_import = self._missing_constructor_import(origin, origin_path, raw, decl_stmt) @@ -409,9 +409,9 @@ def _argument_imports( """Return the import lines for the names decl_stmt's arguments use that this file doesn't have, sorted by name. A name this file already imports from the origin module (or from the same absolute module the - origin imports it from) is fine and not returned. Returns None if one of the names can't be - imported as the same object: the origin doesn't bind it at module level at runtime (e.g. only - under `if TYPE_CHECKING:`), or this file binds it to something else. + origin imports it from) is fine and not returned. Returns None if one of the names isn't available + as the same object at module level: the origin doesn't bind it at runtime (e.g. only under + `if TYPE_CHECKING:`), or this file binds it to something else. """ origin_sources = from_import_sources(origin) importing_sources = from_import_sources(self._cst_root().translation_unit.atu) diff --git a/src/renaissance/recipes/type_var_domain.py b/src/renaissance/recipes/type_var_domain.py index 40806f5f..4a7da85c 100644 --- a/src/renaissance/recipes/type_var_domain.py +++ b/src/renaissance/recipes/type_var_domain.py @@ -33,7 +33,7 @@ class UnsafeReason(StrEnum): DECLARED_TYPEVAR_EXPORTED = "declared_typevar_exported" USED_IN_GENERIC_CLASS = "used_in_generic_class" SHARES_FUNCTION_WITH_UNCONVERTIBLE = "shares_function_with_unconvertible" - DECLARATION_NAME_UNAVAILABLE = "declaration_name_unavailable" + NAME_NOT_AT_MODULE_LEVEL = "name_not_at_module_level" IMPORTED_ELSEWHERE_IN_PROJECT = "imported_elsewhere_in_project" ORIGIN_IMPORTS_CONSTRUCTOR_CONDITIONALLY = "origin_imports_constructor_conditionally" @@ -71,9 +71,9 @@ class UnsafeRule: "every function using it also uses a type variable that can't be converted", "feature-type-parameter-modernization-shares-function-with-unconvertible", ), - UnsafeReason.DECLARATION_NAME_UNAVAILABLE: UnsafeRule( - "a name its declaration uses can't be imported here as the same object", - "feature-type-parameter-modernization-declaration-name-unavailable", + UnsafeReason.NAME_NOT_AT_MODULE_LEVEL: UnsafeRule( + "declaration uses a name that isn't available at module level as the same object", + "feature-type-parameter-modernization-name-not-at-module-level", ), UnsafeReason.IMPORTED_ELSEWHERE_IN_PROJECT: UnsafeRule( "imported directly by another file in the target project", diff --git a/test/recipes/test_type_var_check_localize.py b/test/recipes/test_type_var_check_localize.py index a189e723..7dc0971b 100644 --- a/test/recipes/test_type_var_check_localize.py +++ b/test/recipes/test_type_var_check_localize.py @@ -201,7 +201,7 @@ def test_names_a_declaration_uses_are_imported_from_where_the_origin_gets_them( def test_does_not_localize_when_a_name_the_declaration_uses_cant_be_imported_safely( self, mocker: MockerFixture, tmp_path: Path, origin: str, importing_extra: str ) -> None: - """Verify a declaration using a name that can't be imported from the origin as the same object stays imported.""" + """Verify a declaration using a name that isn't available at module level as the same object stays imported.""" subject = self._create_cross_file( mocker, tmp_path, @@ -211,7 +211,7 @@ def test_does_not_localize_when_a_name_the_declaration_uses_cant_be_imported_saf result = subject.localize_imported_typevars() assert_that(result, equal_to({"T": "unsafe"})) - assert_that(subject.cross_file_unsafe_reasons, equal_to({"T": UnsafeReason.DECLARATION_NAME_UNAVAILABLE})) + assert_that(subject.cross_file_unsafe_reasons, equal_to({"T": UnsafeReason.NAME_NOT_AT_MODULE_LEVEL})) assert_that(subject.apply_to_string(), contains_string("from file_1 import T")) @pytest.mark.parametrize( From 68f7adf17d57cf1750e0dacb58a12ce9e3210482 Mon Sep 17 00:00:00 2001 From: Francesco Pezzella Date: Tue, 20 Oct 2026 11:31:36 +0200 Subject: [PATCH 18/25] Reject --py below 3.12 and remove the PEP 695 version gate. Minimum supported version for the tool is 3.12. --- docs/developer/modules/recipes.md | 15 +++--- .../features/type-parameter-modernization.md | 24 ++-------- src/rejuvenation/migration-type-recipes.py | 20 ++++---- src/renaissance/recipes/type_var_check.py | 16 ++----- src/renaissance/recipes/type_var_domain.py | 5 -- test/recipes/conftest.py | 5 +- test/recipes/test_type_var_check_convert.py | 17 ++----- test/recipes/test_type_var_check_localize.py | 46 ++++--------------- test/recipes/test_type_var_check_orphaned.py | 15 +++--- .../test_migration_type_recipes.py | 16 ++++--- 10 files changed, 60 insertions(+), 119 deletions(-) diff --git a/docs/developer/modules/recipes.md b/docs/developer/modules/recipes.md index c6aeabc4..590d0dce 100644 --- a/docs/developer/modules/recipes.md +++ b/docs/developer/modules/recipes.md @@ -45,8 +45,8 @@ safely cover. This page covers `TypeVarCheck`, the recipe behind - `type_param_kinds`: maps each type parameter name the phases see to `"TypeVar"`, `"ParamSpec"` or `"TypeVarTuple"`. - Attributes set by the CLI (`migration-type-recipes.py`): `min_python`, `project_root` (falls back to the file's folder) and `project_wide_imported_names` (from `collect_project_imported_names`). -- `PythonRefactoring.process("TypeVarCheck", file)`, used by `cli.py refactor`, never sets those attributes, so every - version-gated rewrite is reported `"unsafe"` there. +- `PythonRefactoring.process("TypeVarCheck", file)`, used by `cli.py refactor`, never sets those attributes, so + `min_python` stays `MIN_SUPPORTED_PYTHON` there and a `default=` declaration is reported `"unsafe"`. ## Internal structure @@ -55,11 +55,12 @@ safely cover. This page covers `TypeVarCheck`, the recipe behind replaces only the node it changes, so formatting and comments elsewhere are kept. - **Unsafe reasons.** `is_safe_to_remove`/`is_safe_to_localize` and `_conversion_refusal` return an `UnsafeReason` - or `None`. Each of the 9 reasons has an `UnsafeRule` (message + docs anchor) in `UNSAFE_RULES`. The CLI report prints + or `None`. Each of the 8 reasons has an `UnsafeRule` (message + docs anchor) in `UNSAFE_RULES`. The CLI report prints each message and links `DOCS_BASE_URL` once. -- **Version gates.** `_conversion_refusal` checks `min_python` against `PEP_695_MINIMUM = (3, 12)` and, for a - `default=`, `PEP_696_MINIMUM = (3, 13)`. `None` never passes. A keyword outside `_CONVERTIBLE_KEYWORDS` is refused +- **Version gates.** `min_python` is never below `MIN_SUPPORTED_PYTHON = (3, 12)`, which the CLI enforces, so PEP 695 + syntax is always available. `_conversion_refusal` checks it against `PEP_696_MINIMUM = (3, 13)` for a + `default=`. A keyword outside `_CONVERTIBLE_KEYWORDS` is refused (`NO_PEP695_EQUIVALENT`) instead of dropped. `covariant` and `contravariant` are in the set and are dropped, because a function's type parameter has no variance; converting classes would have to refuse them. @@ -147,10 +148,10 @@ See *Verified by test modules* in [Type parameter modernization](../../user/feat ```toml [project] - requires-python = ">=3.12" + requires-python = ">=3.13" ``` - run with `--py 3.11` is treated as below 3.12, so nothing is converted. + run with `--py 3.12` is treated as below 3.13, so a `default=` declaration is not converted. - Re-exports, namespace packages, `import *` and renamed imports are not followed. In: diff --git a/docs/user/features/type-parameter-modernization.md b/docs/user/features/type-parameter-modernization.md index 06c99b5f..e126fb8d 100644 --- a/docs/user/features/type-parameter-modernization.md +++ b/docs/user/features/type-parameter-modernization.md @@ -123,30 +123,14 @@ Each case below stops one change and is reported under *needs manual review*. ### Python version gates { #feature-type-parameter-modernization-version-gates } -PEP 695 syntax is a `SyntaxError` on older Pythons, so the tool only writes it when `--py` (the oldest version the -target supports, not the one running the tool) is new enough. The version is never guessed: unknown counts as too -old. Removing a declaration that is already unused needs no new syntax, so it never depends on `--py`. +PEP 695 syntax (`def f[T](...)`) is a `SyntaxError` before Python 3.12, so `--py` (the oldest version the target +supports, not the one running the tool) must be 3.12 or higher. A lower value is a usage error and nothing is +changed. The version is never guessed. Newer syntax is written only when `--py` is new enough: | Syntax | PEP | Minimum Python | | --------------------- | -------------------------------------------- | -------------- | -| `def f[T](...)` | [PEP 695](https://peps.python.org/pep-0695/) | 3.12 | | `def f[T = int](...)` | [PEP 696](https://peps.python.org/pep-0696/) | 3.13 | -#### PEP 695 version gate { #feature-type-parameter-modernization-pep695-version-gate } - -Below 3.12 nothing is converted. Localization and orphan removal still run. - -```python -T = TypeVar("T") - -def first(items: list[T]) -> T: ... -``` - -With `--py 3.11` the file is unchanged. With `--py 3.12` it becomes `def first[T](items: list[T]) -> T: ...` and the -declaration is removed. - -**To fix this yourself:** if the project supports 3.12+, run again with `--py 3.12`. Otherwise keep the old syntax. - #### PEP 696 version gate { #feature-type-parameter-modernization-pep696-version-gate } Below 3.13 a declaration with `default=` is not converted, since dropping the default would change what type @@ -353,7 +337,7 @@ python src/rejuvenation/migration-type-recipes.py --py MAJOR.MINOR [--rep ``` - ``: a `.py` file or a directory, scanned recursively (`.git`, `__pycache__`, `.venv` and `venv` skipped). -- `--py` (required): the oldest Python version the target project supports. +- `--py` (required): the oldest Python version the target project supports, 3.12 or higher. - `--report`: also write the report to a file. - `--no-ruff`: skip the final `ruff` pass. diff --git a/src/rejuvenation/migration-type-recipes.py b/src/rejuvenation/migration-type-recipes.py index 8a93d9fb..03872af9 100644 --- a/src/rejuvenation/migration-type-recipes.py +++ b/src/rejuvenation/migration-type-recipes.py @@ -11,7 +11,7 @@ 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/file.py --py 3.13 python src/rejuvenation/migration-type-recipes.py ./some_repo --py 3.12 --no-ruff """ @@ -31,7 +31,7 @@ from termcolor import colored from renaissance.project.project_scanner import PythonScanner -from renaissance.recipes.type_var_check import TypeVarCheck +from renaissance.recipes.type_var_check import MIN_SUPPORTED_PYTHON, TypeVarCheck from renaissance.recipes.type_var_domain import DOCS_BASE_URL, UNSAFE_RULES, UnsafeReason from renaissance.utils.import_resolution import collect_project_imported_names @@ -79,14 +79,19 @@ 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. + argparse.ArgumentTypeError: If text is not in MAJOR.MINOR form or is below MIN_SUPPORTED_PYTHON. """ 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])) + version = (int(parts[0]), int(parts[1])) + if version < MIN_SUPPORTED_PYTHON: + minimum = ".".join(map(str, MIN_SUPPORTED_PYTHON)) + message = f"expected {minimum} or higher, since PEP 695 syntax does not exist below it, got {text!r}" + raise argparse.ArgumentTypeError(message) + return version def has_fixed(report: FileReport) -> bool: @@ -319,7 +324,7 @@ def build_arg_parser() -> argparse.ArgumentParser: 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/file.py --py 3.13 python src/rejuvenation/migration-type-recipes.py ./some_repo --py 3.12 --no-ruff """), formatter_class=argparse.RawDescriptionHelpFormatter, @@ -330,10 +335,7 @@ def build_arg_parser() -> argparse.ArgumentParser: type=_parse_py_version, required=True, metavar="MAJOR.MINOR", - help=( - "Minimum Python version the target project supports (not the one running this tool), e.g. 3.12. " - "PEP 695 rewrites need 3.12+, PEP 696 type parameter defaults 3.13+." - ), + help=("Minimum Python version the target project supports is 3.12."), ) parser.add_argument("--report", type=Path, metavar="PATH", help="Also write the full report to this file.") parser.add_argument( diff --git a/src/renaissance/recipes/type_var_check.py b/src/renaissance/recipes/type_var_check.py index 1d7fc0b1..2dc8c253 100644 --- a/src/renaissance/recipes/type_var_check.py +++ b/src/renaissance/recipes/type_var_check.py @@ -41,7 +41,7 @@ ) from renaissance.utils.import_resolution import import_from_module, imported_name, rebase_relative_module, resolve_project_module -PEP_695_MINIMUM = (3, 12) +MIN_SUPPORTED_PYTHON = (3, 12) PEP_696_MINIMUM = (3, 13) @@ -63,8 +63,8 @@ class TypeVarCheck(PythonRefactoring[PythonCstNode]): 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 + # Minimum Python version the target codebase supports, where PEP 695 syntax exists. + min_python: tuple[int, int] = MIN_SUPPORTED_PYTHON # Names other files in the target project import directly from this file; never removed. project_wide_imported_names: frozenset[str] = frozenset() @@ -88,18 +88,12 @@ def _cst_root(self) -> PythonCstNode: """Return the root node of the file as it currently is (after the last commit).""" return cast("PythonCstNode", cast("object", self.root)) - def _target_supports_pep695(self) -> bool: - """Return True only if min_python is known and is 3.12+, where PEP 695 syntax (`def f[T](...)`) exists.""" - return self.min_python is not None and self.min_python >= PEP_695_MINIMUM - def _target_supports_pep696(self) -> bool: - """Return True only if min_python is known and is 3.13+, where PEP 696 defaults (`def f[T = int](...)`) exist.""" - return self.min_python is not None and self.min_python >= PEP_696_MINIMUM + """Return True only if min_python is 3.13+, where PEP 696 defaults (`def f[T = int](...)`) exist.""" + return self.min_python >= PEP_696_MINIMUM def _conversion_refusal(self, decl_stmt: cst.Assign) -> UnsafeReason | None: """Return why decl_stmt can't be converted to a PEP 695 type parameter on this target, or None if it can.""" - if not self._target_supports_pep695(): - return UnsafeReason.PEP695_VERSION_GATE if has_unconvertible_argument(decl_stmt): return UnsafeReason.NO_PEP695_EQUIVALENT if declared_default(decl_stmt) is not None and not self._target_supports_pep696(): diff --git a/src/renaissance/recipes/type_var_domain.py b/src/renaissance/recipes/type_var_domain.py index 4a7da85c..e42a4b34 100644 --- a/src/renaissance/recipes/type_var_domain.py +++ b/src/renaissance/recipes/type_var_domain.py @@ -27,7 +27,6 @@ class UnsafeReason(StrEnum): Each member has a matching documented rule under DOCS_BASE_URL - see UNSAFE_RULES. """ - PEP695_VERSION_GATE = "pep695_version_gate" PEP696_VERSION_GATE = "pep696_version_gate" NO_PEP695_EQUIVALENT = "no_pep695_equivalent" DECLARED_TYPEVAR_EXPORTED = "declared_typevar_exported" @@ -47,10 +46,6 @@ class UnsafeRule: UNSAFE_RULES: dict[UnsafeReason, UnsafeRule] = { - UnsafeReason.PEP695_VERSION_GATE: UnsafeRule( - "target's minimum Python version is unknown or below 3.12", - "feature-type-parameter-modernization-pep695-version-gate", - ), UnsafeReason.PEP696_VERSION_GATE: UnsafeRule( "declares a default= (PEP 696), which needs the target's minimum Python to be 3.13+", "feature-type-parameter-modernization-pep696-version-gate", diff --git a/test/recipes/conftest.py b/test/recipes/conftest.py index c82bbb08..7e12d33a 100644 --- a/test/recipes/conftest.py +++ b/test/recipes/conftest.py @@ -14,12 +14,12 @@ from pytest_mock import MockerFixture from renaissance.integrations.python.ast.cst_node import PythonCstNode -from renaissance.recipes.type_var_check import PEP_695_MINIMUM, TypeVarCheck +from renaissance.recipes.type_var_check import TypeVarCheck @pytest.fixture def create_type_var_check(mocker: MockerFixture) -> Callable[[str], TypeVarCheck]: - """Build a TypeVarCheck for in-memory, dedented source text, with min_python 3.12 so PEP 695 conversion is enabled. + """Build a TypeVarCheck for in-memory, dedented source text. `PythonFactory.create` is mocked so nothing touches the filesystem. """ @@ -31,7 +31,6 @@ def _create(text: str) -> TypeVarCheck: ) subject = TypeVarCheck(Path("x.py")) subject.in_memory = True - subject.min_python = PEP_695_MINIMUM return subject return _create diff --git a/test/recipes/test_type_var_check_convert.py b/test/recipes/test_type_var_check_convert.py index 2f685361..eb2b5467 100644 --- a/test/recipes/test_type_var_check_convert.py +++ b/test/recipes/test_type_var_check_convert.py @@ -14,7 +14,7 @@ import pytest from hamcrest import assert_that, contains_string, equal_to, has_entry, not_ -from renaissance.recipes.type_var_check import PEP_695_MINIMUM, PEP_696_MINIMUM, TypeVarCheck +from renaissance.recipes.type_var_check import MIN_SUPPORTED_PYTHON, PEP_696_MINIMUM, TypeVarCheck from renaissance.recipes.type_var_domain import UnsafeReason @@ -563,31 +563,24 @@ def test_converts_keeping_every_expressible_argument( @pytest.mark.parametrize( ("declaration", "signature", "min_python", "expected_reason"), [ - pytest.param( - 'T = TypeVar("T")', - _TYPEVAR_USE, - (3, 11), - UnsafeReason.PEP695_VERSION_GATE, - id="typevar-plain-3.11", - ), pytest.param( 'T = TypeVar("T", default=int)', _TYPEVAR_USE, - PEP_695_MINIMUM, + MIN_SUPPORTED_PYTHON, UnsafeReason.PEP696_VERSION_GATE, id="typevar-default-3.12", ), pytest.param( 'P = ParamSpec("P", default=[int, str])', _PARAMSPEC_USE, - PEP_695_MINIMUM, + MIN_SUPPORTED_PYTHON, UnsafeReason.PEP696_VERSION_GATE, id="paramspec-default-3.12", ), pytest.param( 'Ts = TypeVarTuple("Ts", default=Unpack[tuple[int]])', _TYPEVARTUPLE_USE, - PEP_695_MINIMUM, + MIN_SUPPORTED_PYTHON, UnsafeReason.PEP696_VERSION_GATE, id="typevartuple-default-3.12", ), @@ -654,7 +647,7 @@ def test_refuses_argument_it_cannot_carry_over( pytest.param( 'T = TypeVar("T")\nD = TypeVar("D", default=int)', "def a(x: T, y: D) -> T | D:", - PEP_695_MINIMUM, + MIN_SUPPORTED_PYTHON, {"D": UnsafeReason.PEP696_VERSION_GATE, "T": UnsafeReason.SHARES_FUNCTION_WITH_UNCONVERTIBLE}, id="version-gated-default-declared-last", ), diff --git a/test/recipes/test_type_var_check_localize.py b/test/recipes/test_type_var_check_localize.py index 7dc0971b..97494345 100644 --- a/test/recipes/test_type_var_check_localize.py +++ b/test/recipes/test_type_var_check_localize.py @@ -10,21 +10,16 @@ import builtins import textwrap from pathlib import Path -from typing import TYPE_CHECKING, cast +from typing import cast import pytest from hamcrest import all_of, assert_that, contains_string, equal_to, has_entry, has_item, has_length, not_ from pytest_mock import MockerFixture from renaissance.integrations.python.ast.cst_node import PythonCstNode -from renaissance.recipes.type_var_check import PEP_695_MINIMUM, TypeVarCheck +from renaissance.recipes.type_var_check import TypeVarCheck from renaissance.recipes.type_var_domain import UnsafeReason -# TODO(FrancescoPezzella): Replace with a lazy import in Python 3.15. -# https://github.com/TNO/Renaissance.Py/issues/275 -if TYPE_CHECKING: - from hamcrest.core.matcher import Matcher - def _module_level_names(module: ast.Module) -> set[str]: """Return the names bound at module level by imports, classes, functions and assignments.""" @@ -85,7 +80,6 @@ def _create_cross_file( ) subject = TypeVarCheck(importing_file) subject.in_memory = True - subject.min_python = PEP_695_MINIMUM subject.project_root = tmp_path return subject @@ -670,32 +664,8 @@ def b() -> None: assert_that(result, equal_to({})) - @pytest.mark.parametrize( - ("min_python", "expected_converted", "output_matcher"), - [ - pytest.param( - PEP_695_MINIMUM, - "fixed", - all_of(contains_string("def b[T](x: T) -> T:"), not_(contains_string("T = TypeVar"))), - id="converts-on-3.12", - ), - pytest.param( - (3, 10), - "unsafe", - all_of(contains_string("def b(x: T) -> T:"), contains_string('T = TypeVar("T")')), - id="only-localizes-below-3.12", - ), - ], - ) - def test_check_localizes_then_converts_when_the_target_allows( - self, - mocker: MockerFixture, - tmp_path: Path, - min_python: tuple[int, int], - expected_converted: str, - output_matcher: Matcher[str], - ) -> None: - """Verify check() localizes a cross-file TypeVar, then converts it only if the target supports PEP 695.""" + def test_check_localizes_then_converts(self, mocker: MockerFixture, tmp_path: Path) -> None: + """Verify check() localizes a cross-file TypeVar, then converts it.""" subject = self._create_cross_file( mocker, tmp_path, @@ -711,9 +681,11 @@ def b(x: T) -> T: return x """, ) - subject.min_python = min_python subject.run() assert_that(subject.result["cross_file"], has_entry("T", "fixed")) - assert_that(subject.result["converted"], has_entry("T", expected_converted)) - assert_that(subject.apply_to_string(), output_matcher) + assert_that(subject.result["converted"], has_entry("T", "fixed")) + assert_that( + subject.apply_to_string(), + all_of(contains_string("def b[T](x: T) -> T:"), not_(contains_string("T = TypeVar"))), + ) diff --git a/test/recipes/test_type_var_check_orphaned.py b/test/recipes/test_type_var_check_orphaned.py index ccce2238..7736da14 100644 --- a/test/recipes/test_type_var_check_orphaned.py +++ b/test/recipes/test_type_var_check_orphaned.py @@ -64,25 +64,22 @@ class TestTypeVarCheckOrphaned: """See module docstring.""" @pytest.mark.parametrize( - ("source", "min_python"), + "source", [ - pytest.param(ALREADY_CONVERTED_SOURCE, (3, 12), id="already-converted"), - pytest.param(ALREADY_CONVERTED_SOURCE, (3, 11), id="already-converted-below-pep695"), - pytest.param(UNREFERENCED_SOURCE, (3, 11), id="unreferenced-below-pep695"), - pytest.param(NESTED_ONLY_CONVERTED_SOURCE, (3, 12), id="only-nested-function-converted"), - pytest.param(CLOSURE_IN_CONVERTED_FUNCTION_SOURCE, (3, 12), id="closure-inside-converted-function"), - pytest.param(CLASS_WITH_OWN_TYPE_PARAMETER_SOURCE, (3, 12), id="class-with-own-type-parameter"), + pytest.param(ALREADY_CONVERTED_SOURCE, id="already-converted"), + pytest.param(UNREFERENCED_SOURCE, id="unreferenced"), + pytest.param(NESTED_ONLY_CONVERTED_SOURCE, id="only-nested-function-converted"), + pytest.param(CLOSURE_IN_CONVERTED_FUNCTION_SOURCE, id="closure-inside-converted-function"), + pytest.param(CLASS_WITH_OWN_TYPE_PARAMETER_SOURCE, id="class-with-own-type-parameter"), ], ) def test_removes_declaration_whose_references_are_all_shadowed( self, create_type_var_check: Callable[[str], TypeVarCheck], source: str, - min_python: tuple[int, int], ) -> None: """Verify a declaration whose references are all shadowed by PEP 695 parameters, or absent, is removed.""" subject = create_type_var_check(source) - subject.min_python = min_python result = subject.remove_orphaned_declarations() diff --git a/test/rejuvenation/test_migration_type_recipes.py b/test/rejuvenation/test_migration_type_recipes.py index 4d552b0e..0d459758 100644 --- a/test/rejuvenation/test_migration_type_recipes.py +++ b/test/rejuvenation/test_migration_type_recipes.py @@ -139,7 +139,7 @@ def test_unused_typevar_import_cleanup( assert_that(capsys.readouterr().out, report_matcher) def test_localized_imports_are_merged_with_existing_ones(self, tmp_path: Path) -> None: - """The localized TypeVar's import joins the file's own `typing` import; Python < 3.12 only localizes.""" + """The localized TypeVar's import joins the file's own `typing` import.""" (tmp_path / "xarray_types.py").write_text( 'from typing import TypeVar\n\nJoinOptions = str\nAlignable = int\nT = TypeVar("T")\n', encoding="utf-8", @@ -155,11 +155,15 @@ def test_localized_imports_are_merged_with_existing_ones(self, tmp_path: Path) - def pick(x: T, how: JoinOptions, other: Alignable) -> Any: return x + + + class Items(list[T]): + pass """), encoding="utf-8", ) - exit_code = migration.main([str(target), "--py", "3.11"]) + exit_code = migration.main([str(target), "--py", "3.12"]) assert_that(exit_code, equal_to(0)) lines = target.read_text(encoding="utf-8").splitlines() @@ -413,14 +417,14 @@ class TestPyVersionFlag: @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"), + pytest.param("3.12", 'T = TypeVar("T", default=int)', "def identity[", id="3.12-default-kept"), + pytest.param("3.13", "def identity[T = int]", 'T = TypeVar("T", default=int)', id="3.13-default-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.""" + """--py decides whether a type parameter default (PEP 696) is converted.""" target = tmp_path / "mod.py" - target.write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") + target.write_text(LEGACY_TYPEVAR_SOURCE.replace('TypeVar("T")', 'TypeVar("T", default=int)'), encoding="utf-8") exit_code = migration.main([str(target), "--py", py_version]) From 091a72a6a02353bb9ced3b1bb6fec11bd645720b Mon Sep 17 00:00:00 2001 From: Francesco Pezzella Date: Tue, 20 Oct 2026 13:13:28 +0200 Subject: [PATCH 19/25] Add --ignore-localize to keep the imports of shared type parameter files, and report them with an Ignored column. --- docs/developer/modules/recipes.md | 3 +- .../features/type-parameter-modernization.md | 29 +++++-- src/rejuvenation/migration-type-recipes.py | 87 ++++++++++++++----- src/renaissance/recipes/type_var_check.py | 31 +++++-- test/recipes/test_type_var_check_localize.py | 36 ++++++++ .../test_migration_type_recipes.py | 20 +++++ 6 files changed, 172 insertions(+), 34 deletions(-) diff --git a/docs/developer/modules/recipes.md b/docs/developer/modules/recipes.md index 590d0dce..6f4917ac 100644 --- a/docs/developer/modules/recipes.md +++ b/docs/developer/modules/recipes.md @@ -44,7 +44,8 @@ safely cover. This page covers `TypeVarCheck`, the recipe behind - `type_param_kinds`: maps each type parameter name the phases see to `"TypeVar"`, `"ParamSpec"` or `"TypeVarTuple"`. - Attributes set by the CLI (`migration-type-recipes.py`): `min_python`, `project_root` (falls back to the file's - folder) and `project_wide_imported_names` (from `collect_project_imported_names`). + folder), `project_wide_imported_names` (from `collect_project_imported_names`) and `ignore_localize_origins` + (absolute paths of the files `--ignore-localize` names). `ignored_names` holds the names this file imports from them. - `PythonRefactoring.process("TypeVarCheck", file)`, used by `cli.py refactor`, never sets those attributes, so `min_python` stays `MIN_SUPPORTED_PYTHON` there and a `default=` declaration is reported `"unsafe"`. diff --git a/docs/user/features/type-parameter-modernization.md b/docs/user/features/type-parameter-modernization.md index e126fb8d..263dec58 100644 --- a/docs/user/features/type-parameter-modernization.md +++ b/docs/user/features/type-parameter-modernization.md @@ -29,7 +29,8 @@ def first[T](items: list[T]) -> T: ... The tool runs three phases: 1. **Localize cross-file imports.** A type parameter imported from another file in the project - (`from .shapes import T`) is replaced by an exact local copy of its declaration, placed after the file's imports. + (`from .shapes import T`) is replaced by an exact local copy of its declaration, placed after the file's imports, + unless the origin is named by `--ignore-localize` (see *Shared declaration files* below). 2. **Convert to PEP 695.** Every function whose signature uses a declared type parameter gets its own `[T]`, with bounds, constraints and `default=` carried over (`def f[T: str = str](...)`). `covariant=` and `contravariant=` are dropped, since only generic classes have a variance. A nested function gets none when an @@ -51,6 +52,21 @@ skipped without a report: - re-exports through an intermediate `__init__.py`, and namespace packages; - a renamed import (`from .shapes import T as U`); a file whose only type parameters arrive that way counts as clean. +### Shared declaration files { #feature-type-parameter-modernization-ignore-localize } + +`--ignore-localize FILE...` names files, relative to `` as the report lists them, whose type parameters are +never copied into the files importing them. Those files keep their imports, so the functions using the imported names +are not converted. Use it for a file that is shared on purpose, such as a `types.py`. + +```shell +python src/rejuvenation/migration-type-recipes.py ./some_repo --py 3.12 --ignore-localize pkg/types.py pkg/aliases.py +``` + +The named files are processed as usual. Give `` first, since the option takes every value after it. A file that +does not exist is a usage error. The report lists the files under `NOT LOCALIZED FROM` and adds an `Ignored` column to +the table, counting each ignored name once per importing file. A file whose only type parameters are ignored counts +as clean. + ### Generic classes { #feature-type-parameter-modernization-generic-class } A class is *generic over* `T` when it declares `T` itself (`class Box[T]:`) or has `T` in a base (`Generic[T]`, @@ -333,13 +349,16 @@ run the tool again. ## API entry points ```shell -python src/rejuvenation/migration-type-recipes.py --py MAJOR.MINOR [--report PATH] [--no-ruff] +python src/rejuvenation/migration-type-recipes.py --py MAJOR.MINOR \ + [--report PATH] [--no-ruff] [--ignore-localize FILE...] ``` - ``: a `.py` file or a directory, scanned recursively (`.git`, `__pycache__`, `.venv` and `venv` skipped). - `--py` (required): the oldest Python version the target project supports, 3.12 or higher. - `--report`: also write the report to a file. - `--no-ruff`: skip the final `ruff` pass. +- `--ignore-localize`: files whose type parameters are never copied into the files importing them (see + *Shared declaration files*). After the recipe, the CLI runs `ruff check --fix --select F401,I001` on the changed files. This removes **every** unused import in them, including ones that were unused before (such as `from typing import TypeVar` after the @@ -352,9 +371,9 @@ The tables count the type parameters per kind (`TypeVar`, `ParamSpec`, `TypeVarT review from most to least common. A name counts once per file, as needing review if it has a reason to review. `MODIFIED` says what was done to each name: converted in functions, import replaced by a local declaration, or unused declaration removed; a name localized and then removed is listed only as removed. `NEEDS MANUAL REVIEW` gives each name its -reason, and a name can be in both sections. Files are -listed relative to ``. Exit codes: 0 when the run completes (manual review is not a failure), 2 for a usage error, -3 if any file raised an exception. +reason, and a name can be in both sections. With `--ignore-localize` the report also has the `NOT LOCALIZED FROM` +section and the `Ignored` column. Files are listed relative to ``. Exit codes: 0 when the run completes (manual +review is not a failure), 2 for a usage error, 3 if any file raised an exception. ## Change considerations diff --git a/src/rejuvenation/migration-type-recipes.py b/src/rejuvenation/migration-type-recipes.py index 03872af9..9ed640b0 100644 --- a/src/rejuvenation/migration-type-recipes.py +++ b/src/rejuvenation/migration-type-recipes.py @@ -9,10 +9,8 @@ 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.13 - python src/rejuvenation/migration-type-recipes.py ./some_repo --py 3.12 --no-ruff +Example: + python src/rejuvenation/migration-type-recipes.py ./repo --py 3.12 --report review.md --ignore-localize repo/folder/types.py """ @@ -24,7 +22,7 @@ import sys import textwrap from collections import Counter -from collections.abc import Sequence +from collections.abc import Collection, Sequence from dataclasses import dataclass from pathlib import Path @@ -53,15 +51,17 @@ class FileReport: error: str | None reasons: dict[str, dict[str, UnsafeReason]] | None = None kinds: dict[str, str] | None = None + ignored: frozenset[str] = frozenset() @dataclass class KindCount: - """How many type parameters of one kind a run reported, and how many were fixed or need manual review.""" + """How many type parameters of one kind a run reported, and how many were fixed, need manual review or were ignored.""" reported: int = 0 fixed: int = 0 needs_review: int = 0 + ignored: int = 0 def resolve_target_files(target: Path) -> list[Path]: @@ -119,6 +119,7 @@ def process_file( min_python: tuple[int, int], project_root: Path, project_wide_imported_names: frozenset[str], + ignore_localize_origins: frozenset[Path] = frozenset(), ) -> FileReport: """Run TypeVarCheck's three phases on one file and return its FileReport. @@ -129,6 +130,7 @@ def process_file( recipe.min_python = min_python recipe.project_root = project_root recipe.project_wide_imported_names = project_wide_imported_names + recipe.ignore_localize_origins = ignore_localize_origins result = recipe.check() reasons = { "cross_file": recipe.cross_file_unsafe_reasons, @@ -137,7 +139,14 @@ def process_file( } 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, kinds=recipe.type_param_kinds) + return FileReport( + path=path, + result=result, + error=None, + reasons=reasons, + kinds=recipe.type_param_kinds, + ignored=frozenset(recipe.ignored_names), + ) def _run_ruff_import_cleanup(paths: list[Path]) -> None: @@ -188,12 +197,15 @@ def _name_statuses(report: FileReport) -> dict[str, str]: def count_by_kind(reports: Sequence[FileReport]) -> dict[str, KindCount]: """Count the type parameters the reports list, per kind (TypeVar, ParamSpec, TypeVarTuple). - A name counts once per file, as needing review if any phase left it unsafe and as fixed otherwise. - Files that errored are skipped. + A name counts once per file, as needing review if any phase left it unsafe and as fixed otherwise. A name + the file imports from an ignored file counts as ignored. Files that errored are skipped. """ counts = {kind: KindCount() for kind in _TYPE_PARAM_KINDS} for report in reports: kinds = report.kinds or {} + for name in report.ignored: + counts[kinds[name]].reported += 1 + counts[kinds[name]].ignored += 1 for name, status in _name_statuses(report).items(): count = counts[kinds[name]] count.reported += 1 @@ -225,13 +237,17 @@ def _format_table(headers: Sequence[str], rows: Sequence[Sequence[str]]) -> list return [_table_row(headers, widths), _table_row(["-" * width for width in widths], widths), *(_table_row(row, widths) for row in rows)] -def _summary_tables(reports: Sequence[FileReport]) -> list[str]: +def _summary_tables(reports: Sequence[FileReport], *, show_ignored: bool) -> list[str]: """Return the report's summary tables: type parameters per kind, then the unsafe reasons by frequency. - The reasons table is left out when no reason was hit. + The Ignored column is only there when show_ignored is True. The reasons table is left out when no reason was hit. """ - kind_rows = [[kind, str(c.reported), str(c.fixed), str(c.needs_review)] for kind, c in count_by_kind(reports).items()] - lines = ["", "TYPE PARAMETERS BY KIND", "", *_format_table(["Kind", "Reported", "Fixed", "Needs review"], kind_rows)] + headers = ["Kind", "Reported", "Fixed", "Needs review", *(["Ignored"] if show_ignored else [])] + kind_rows = [ + [kind, str(c.reported), str(c.fixed), str(c.needs_review), *([str(c.ignored)] if show_ignored else [])] + for kind, c in count_by_kind(reports).items() + ] + lines = ["", "TYPE PARAMETERS BY KIND", "", *_format_table(headers, kind_rows)] ranking = rank_reasons(reports) if ranking: reason_rows = [[UNSAFE_RULES[reason].message, str(count)] for reason, count in ranking] @@ -276,13 +292,20 @@ def _needs_review_entry(report: FileReport, project_root: Path) -> list[str]: return lines -def _format_console_report(reports: list[FileReport], *, ruff_ran: bool, project_root: Path) -> str: +def _format_console_report( + reports: list[FileReport], + *, + ruff_ran: bool, + project_root: Path, + ignore_localize_origins: Collection[Path], +) -> str: """Build the full per-file report: MODIFIED / NEEDS MANUAL REVIEW / ERRORS sections. Files are listed by their path relative to project_root, separated by a blank line in the MODIFIED and NEEDS MANUAL REVIEW sections. Clean files (no type parameter usage at all) are only counted, not listed, so the report shows what needs attention. The ruff import-cleanup line is only - included when ruff_ran is True. + included when ruff_ran is True. The NOT LOCALIZED FROM section and the Ignored column are only + included when ignore_localize_origins is not empty. """ modified = [report for report in reports if has_fixed(report)] needs_review = [report for report in reports if has_unsafe(report)] @@ -301,7 +324,10 @@ def _format_console_report(reports: list[FileReport], *, ruff_ran: bool, project "Imports across the modified files above were also cleaned up via `ruff check --fix --select F401,I001`: " "unused ones dropped, the rest merged and sorted.", ) - lines.extend(_summary_tables(reports)) + if ignore_localize_origins: + lines.extend(["", f"NOT LOCALIZED FROM ({len(ignore_localize_origins)})"]) + lines.extend(f" {origin.relative_to(project_root)}" for origin in sorted(ignore_localize_origins)) + lines.extend(_summary_tables(reports, show_ignored=bool(ignore_localize_origins))) lines.extend(["", f"MODIFIED ({len(modified)})"]) lines.extend(_join_with_blank_lines([_modified_entry(report, project_root) for report in modified])) @@ -323,9 +349,7 @@ def build_arg_parser() -> argparse.ArgumentParser: 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.13 - python src/rejuvenation/migration-type-recipes.py ./some_repo --py 3.12 --no-ruff + python src/rejuvenation/migration-type-recipes.py ./repo --py 3.12 --report review.md --ignore-localize repo/folder/types.py """), formatter_class=argparse.RawDescriptionHelpFormatter, ) @@ -343,6 +367,18 @@ def build_arg_parser() -> argparse.ArgumentParser: action="store_true", help="Skip the final `ruff check --fix --select F401,I001` pass that drops imports made unused and sorts the rest.", ) + parser.add_argument( + "--ignore-localize", + action="extend", + nargs="+", + type=Path, + default=[], + metavar="FILE", + help=( + "Files, relative to the scanned directory, whose type parameters are never copied into the files importing " + "them; those imports stay. Give the path argument first, since this option takes every value after it." + ), + ) return parser @@ -367,6 +403,11 @@ def main(argv: Sequence[str] | None = None) -> int: project_root = target if target.is_dir() else target.parent imported_names_by_file = collect_project_imported_names(files, project_root) + for ignored in args.ignore_localize: + if not (project_root / ignored).is_file(): + parser.error(f"--ignore-localize: file does not exist: {ignored}") + ignore_localize_origins = frozenset((project_root / ignored).resolve() for ignored in args.ignore_localize) + reports: list[FileReport] = [] for path in files: project_wide_imported_names = imported_names_by_file.get(path, frozenset()) @@ -375,6 +416,7 @@ def main(argv: Sequence[str] | None = None) -> int: min_python=args.py, project_root=project_root, project_wide_imported_names=project_wide_imported_names, + ignore_localize_origins=ignore_localize_origins, ) reports.append(report) print(f"File {path} checked.") @@ -387,7 +429,12 @@ def main(argv: Sequence[str] | None = None) -> int: # https://github.com/TNO/Renaissance.Py/issues/279 _run_ruff_import_cleanup(modified_paths) - console_report = _format_console_report(reports, ruff_ran=ruff_ran, project_root=project_root) + console_report = _format_console_report( + reports, + ruff_ran=ruff_ran, + project_root=project_root, + ignore_localize_origins=ignore_localize_origins, + ) print(console_report) print() print(colored(_format_commit_summary(reports), "green", attrs=["bold"])) diff --git a/src/renaissance/recipes/type_var_check.py b/src/renaissance/recipes/type_var_check.py index 2dc8c253..488e9559 100644 --- a/src/renaissance/recipes/type_var_check.py +++ b/src/renaissance/recipes/type_var_check.py @@ -72,13 +72,18 @@ class TypeVarCheck(PythonRefactoring[PythonCstNode]): # Root that absolute imports resolve from; None falls back to this file's own directory. project_root: Path | None = None + # Absolute paths of the files whose declarations are never copied into the files importing them. + ignore_localize_origins: frozenset[Path] = frozenset() + def __init__(self, file: Path) -> None: """Prepare the recipe for file, parsed with the CST backend. type_param_kinds maps each type parameter name the phases see to "TypeVar", "ParamSpec" or "TypeVarTuple". + ignored_names holds the names this file imports from an ignore_localize_origins file. """ super().__init__(file, PythonCstNode) self.type_param_kinds: dict[str, str] = {} + self.ignored_names: set[str] = set() def run(self) -> None: """Entry point called by PythonRefactoring.process(); stores check()'s result.""" @@ -274,6 +279,7 @@ def localize_imported_typevars(self) -> dict[str, str]: file's last import, so no import is left below code. A declaration stays where its import was when a statement in between uses it. It is unsafe when is_safe_to_localize rejects the origin or _argument_imports returns None. + Names from an ignore_localize_origins file keep their import and go to self.ignored_names instead. Returns {name: "fixed" | "unsafe"}, with each UnsafeReason on self.cross_file_unsafe_reasons. """ results: dict[str, str] = {} @@ -310,17 +316,10 @@ def _localize_names_from( Records each name in results as "fixed" or, via _mark_unsafe, as "unsafe", its kind on self.type_param_kinds, and appends what to write for the "fixed" ones to localizations. - - Raises: - SyntaxError: if the origin file can't be parsed, with the origin's path as its filename. - """ if isinstance(raw.names, cst.ImportStar): return - try: - origin = cst.parse_module(origin_path.read_text(encoding="utf-8")) - except cst.ParserSyntaxError as error: - raise SyntaxError(error.message, (str(origin_path), error.raw_line, error.raw_column + 1, None)) from error + origin = self._parse_origin(origin_path) declarations = find_type_param_declarations(origin) localized: dict[str, cst.Assign] = {} needed_imports: list[str] = [] @@ -332,6 +331,9 @@ def _localize_names_from( decl_stmt = declarations[name] self.type_param_kinds[name] = type_param_constructor_name(decl_stmt) + if origin_path in self.ignore_localize_origins: + self.ignored_names.add(name) + continue reason = is_safe_to_localize(origin, name) if reason is not None: self._mark_unsafe(results, self.cross_file_unsafe_reasons, name, reason) @@ -352,6 +354,19 @@ def _localize_names_from( return localizations.append(self._localization(line, raw, origin, localized, needed_imports)) + @staticmethod + def _parse_origin(origin_path: Path) -> cst.Module: + """Return the parsed origin_path file. + + Raises: + SyntaxError: if the file can't be parsed, with origin_path as its filename. + + """ + try: + return cst.parse_module(origin_path.read_text(encoding="utf-8")) + except cst.ParserSyntaxError as error: + raise SyntaxError(error.message, (str(origin_path), error.raw_line, error.raw_column + 1, None)) from error + @staticmethod def _import_from_text(raw: cst.ImportFrom) -> str: """Return what follows `from` in raw: its leading dots and dotted module, e.g. "..pkg.mod".""" diff --git a/test/recipes/test_type_var_check_localize.py b/test/recipes/test_type_var_check_localize.py index 97494345..5b087d89 100644 --- a/test/recipes/test_type_var_check_localize.py +++ b/test/recipes/test_type_var_check_localize.py @@ -111,6 +111,42 @@ def b(x: T) -> T: assert_that(output, contains_string("T = TypeVar('T')")) assert_that(output, not_(contains_string("from file_1 import T"))) + @pytest.mark.parametrize( + ("origin_ignored", "expected_result", "expected_ignored_names"), + [ + pytest.param(True, {}, {"T"}, id="ignored-origin"), + pytest.param(False, {"T": "fixed"}, set[str](), id="origin-not-ignored"), + ], + ) + def test_names_from_an_ignored_origin_are_not_localized( + self, + mocker: MockerFixture, + tmp_path: Path, + *, + origin_ignored: bool, + expected_result: dict[str, str], + expected_ignored_names: set[str], + ) -> None: + """Verify an import from an origin in ignore_localize_origins is kept and its names are recorded as ignored.""" + subject = self._create_cross_file( + mocker, + tmp_path, + "from typing import TypeVar\nT = TypeVar('T')\n", + """ + from file_1 import T + def b(x: T) -> T: + return x + """, + ) + if origin_ignored: + subject.ignore_localize_origins = frozenset({tmp_path / "file_1.py"}) + + result = subject.localize_imported_typevars() + + assert_that(result, equal_to(expected_result)) + assert_that(subject.ignored_names, equal_to(expected_ignored_names)) + assert_that("from file_1 import T" in subject.apply_to_string(), equal_to(origin_ignored)) + @pytest.mark.parametrize( "declaration", [ diff --git a/test/rejuvenation/test_migration_type_recipes.py b/test/rejuvenation/test_migration_type_recipes.py index 0d459758..4bb8dd9d 100644 --- a/test/rejuvenation/test_migration_type_recipes.py +++ b/test/rejuvenation/test_migration_type_recipes.py @@ -328,6 +328,26 @@ def wrap(f: Callable[P, int]) -> Callable[P, int]: ) +class TestIgnoreLocalize: + """main(): --ignore-localize keeps the imports of a shared declaration file and reports them.""" + + def test_importer_is_left_untouched_and_the_report_lists_it(self, tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None: + """The importer is unchanged, the ignored file is listed, and its name counts as reported and ignored, not for review.""" + (tmp_path / "shared.py").write_text('from typing import TypeVar\n\nT = TypeVar("T")\n', encoding="utf-8") + user_source = "from shared import T\n\n\ndef identity(x: T) -> T:\n return x\n" + (tmp_path / "user.py").write_text(user_source, encoding="utf-8") + + migration.main([str(tmp_path), "--py", "3.12", "--no-ruff", "--ignore-localize", "shared.py"]) + + output = capsys.readouterr().out + assert_that((tmp_path / "user.py").read_text(encoding="utf-8"), equal_to(user_source)) + assert_that(output, contains_string("NOT LOCALIZED FROM (1)\n shared.py\n")) + assert_that( + _table_rows(output, "Kind"), + equal_to([["TypeVar", "2", "0", "1", "1"], ["ParamSpec", "0", "0", "0", "0"], ["TypeVarTuple", "0", "0", "0", "0"]]), + ) + + class TestPerFileProgressFeedback: """main(): prints a per-file progress line as each file is checked.""" From 6777d9e48062ed7631e882ce78c63be0623d12bb Mon Sep 17 00:00:00 2001 From: Francesco Pezzella Date: Tue, 20 Oct 2026 13:28:24 +0200 Subject: [PATCH 20/25] Reword the conditional-origin-import reason to cover TYPE_CHECKING blocks, not only Python version checks. --- .../features/type-parameter-modernization.md | 11 ++++-- src/renaissance/recipes/type_var_domain.py | 5 ++- test/recipes/test_type_var_check_localize.py | 37 ++++++++++++++----- 3 files changed, 38 insertions(+), 15 deletions(-) diff --git a/docs/user/features/type-parameter-modernization.md b/docs/user/features/type-parameter-modernization.md index 263dec58..f32af6cd 100644 --- a/docs/user/features/type-parameter-modernization.md +++ b/docs/user/features/type-parameter-modernization.md @@ -273,14 +273,17 @@ then copy the declaration by hand. ### The origin imports `TypeVar` conditionally { #feature-type-parameter-modernization-origin-imports-constructor-conditionally } The origin imports `TypeVar` inside an `if` or `try` (for example `typing` or `typing_extensions` per Python -version). A copy would use whatever `TypeVar` this file has, which may fail on some versions. +version, or `typing_extensions` under `TYPE_CHECKING` and `typing` otherwise). A copy would use whatever `TypeVar` +this file has, which may fail on some versions or differ for the type checker. ```python # shapes.py -try: - from typing import TypeVar -except ImportError: +from typing import TYPE_CHECKING + +if TYPE_CHECKING: from typing_extensions import TypeVar +else: + from typing import TypeVar T = TypeVar("T") diff --git a/src/renaissance/recipes/type_var_domain.py b/src/renaissance/recipes/type_var_domain.py index e42a4b34..7d10b0c2 100644 --- a/src/renaissance/recipes/type_var_domain.py +++ b/src/renaissance/recipes/type_var_domain.py @@ -75,7 +75,7 @@ class UnsafeRule: "feature-type-parameter-modernization-imported-elsewhere-in-project", ), UnsafeReason.ORIGIN_IMPORTS_CONSTRUCTOR_CONDITIONALLY: UnsafeRule( - "origin module imports TypeVar/ParamSpec/TypeVarTuple conditionally, e.g. per Python version", + "origin module imports TypeVar/ParamSpec/TypeVarTuple inside an if/try block, e.g. under TYPE_CHECKING or per Python version", "feature-type-parameter-modernization-origin-imports-constructor-conditionally", ), } @@ -286,7 +286,8 @@ def is_safe_to_localize(origin: cst.Module, name: str) -> UnsafeReason | None: Its constructor (TypeVar/ParamSpec/TypeVarTuple) must not be imported inside a module-level `if`/`try` block at the origin (ORIGIN_IMPORTS_CONSTRUCTOR_CONDITIONALLY), since which - implementation it binds then depends on the runtime, e.g. `typing_extensions` below 3.13. + implementation it binds then depends on the runtime or the type checker, e.g. `typing_extensions` + below 3.13 or under `TYPE_CHECKING`. """ declaration = find_type_param_declarations(origin).get(name) if declaration is not None and _imports_name_conditionally(origin, type_param_constructor_name(declaration)): diff --git a/test/recipes/test_type_var_check_localize.py b/test/recipes/test_type_var_check_localize.py index 5b087d89..a75882d3 100644 --- a/test/recipes/test_type_var_check_localize.py +++ b/test/recipes/test_type_var_check_localize.py @@ -624,23 +624,42 @@ def test_localized_file_imports_a_relatively_imported_constructor_from_the_same_ assert_that(result, has_entry("T", "fixed")) assert_that(subject.apply_to_string().splitlines().count(expected_line), equal_to(1)) + @pytest.mark.parametrize( + "origin_imports", + [ + pytest.param( + """ + import sys + if sys.version_info >= (3, 13): + from typing import TypeVar + else: + from typing_extensions import TypeVar + """, + id="python-version", + ), + pytest.param( + """ + from typing import TYPE_CHECKING + if TYPE_CHECKING: + from typing_extensions import TypeVar + else: + from typing import TypeVar + """, + id="type-checking", + ), + ], + ) def test_does_not_localize_when_origin_imports_constructor_conditionally( self, mocker: MockerFixture, tmp_path: Path, + origin_imports: str, ) -> None: - """A TypeVar whose origin picks TypeVar per Python version stays imported, marked unsafe.""" + """Verify a TypeVar whose origin imports TypeVar inside an `if` block 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) - """, + textwrap.dedent(origin_imports) + 'T = TypeVar("T", contravariant=True, default=None)\n', """ from typing import TypeVar from file_1 import T, helper From 421d5c8a74a616ee08c2d8bbb416b7ae80e70c3b Mon Sep 17 00:00:00 2001 From: Francesco Pezzella Date: Tue, 20 Oct 2026 14:04:44 +0200 Subject: [PATCH 21/25] Make the type-parameter CLI's closing commit message list what was converted, removed or localized per name, and the files left for manual review by reason. --- .../features/type-parameter-modernization.md | 4 +- src/rejuvenation/migration-type-recipes.py | 71 ++++++++++++++----- .../test_migration_type_recipes.py | 64 +++++++++++++++++ 3 files changed, 121 insertions(+), 18 deletions(-) diff --git a/docs/user/features/type-parameter-modernization.md b/docs/user/features/type-parameter-modernization.md index f32af6cd..d8cd3aec 100644 --- a/docs/user/features/type-parameter-modernization.md +++ b/docs/user/features/type-parameter-modernization.md @@ -375,7 +375,9 @@ review from most to least common. A name counts once per file, as needing review says what was done to each name: converted in functions, import replaced by a local declaration, or unused declaration removed; a name localized and then removed is listed only as removed. `NEEDS MANUAL REVIEW` gives each name its reason, and a name can be in both sections. With `--ignore-localize` the report also has the `NOT LOCALIZED FROM` -section and the `Ignored` column. Files are listed relative to ``. Exit codes: 0 when the run completes (manual +section and the `Ignored` column. Files are listed relative to ``. When files were modified, the run ends with a +commit message: the names grouped by what was done to them (with the number of files if above one, at most ten per +group), then the files left for manual review by reason. Exit codes: 0 when the run completes (manual review is not a failure), 2 for a usage error, 3 if any file raised an exception. ## Change considerations diff --git a/src/rejuvenation/migration-type-recipes.py b/src/rejuvenation/migration-type-recipes.py index 9ed640b0..7bdc2bed 100644 --- a/src/rejuvenation/migration-type-recipes.py +++ b/src/rejuvenation/migration-type-recipes.py @@ -36,10 +36,11 @@ _MAJOR_MINOR_PART_COUNT = 2 _TYPE_PARAM_KINDS = ("TypeVar", "ParamSpec", "TypeVarTuple") _FIXED_DESCRIPTIONS = { - "cross_file": "import replaced by a local declaration", "converted": "converted to PEP 695 in functions", "orphaned": "unused declaration removed", + "cross_file": "import replaced by a local declaration", } +_MAX_LISTED_NAMES = 10 @dataclass @@ -169,17 +170,47 @@ def _run_ruff_import_cleanup(paths: list[Path]) -> None: 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)) +def _files(count: int) -> str: + """Return count with "file" pluralized, e.g. "1 file" or "2 files".""" + return f"{count} file" if count == 1 else f"{count} files" + + +def _format_names(file_counts: Counter[str]) -> str: + """Return the names, most files first then alphabetical, each with its file count if above one. + + Lists at most _MAX_LISTED_NAMES names and summarises the rest as "and N more". + """ + ranked = sorted(file_counts.items(), key=lambda item: (-item[1], item[0])) + listed = ", ".join(name if count == 1 else f"{name} ({_files(count)})" for name, count in ranked[:_MAX_LISTED_NAMES]) + hidden = len(ranked) - _MAX_LISTED_NAMES + return f"{listed} and {hidden} more" if hidden > 0 else listed + + +def _format_commit_summary(reports: Sequence[FileReport], min_python: tuple[int, int]) -> str: + """Build the copy-pasteable commit message: what was done to which names, and what is left for manual review. + + Names are grouped by action with the number of files they were fixed in. The manual review line, + with the files needing it and their reasons by frequency, is left out when no file needs review. + """ + modified = [report for report in reports if has_fixed(report)] + names_by_action = {action: Counter[str]() for action in _FIXED_DESCRIPTIONS.values()} + for report in modified: + for name, actions in _fixed_descriptions(report).items(): + for action in actions: + names_by_action[action][name] += 1 + version = ".".join(map(str, min_python)) + lines = [ + "Modernize TypeVar/ParamSpec/TypeVarTuple usage to PEP 695 syntax", + "", + f"Convert type parameters to PEP 695 syntax in {_files(len(modified))} (Python {version}+).", + "", + *(f"- {action}: {_format_names(file_counts)}" for action, file_counts in names_by_action.items() if file_counts), + ] 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)" - ) + if needs_review: + reasons = ", ".join(f"{UNSAFE_RULES[reason].message} ({count})" for reason, count in rank_reasons(reports)) + lines.extend(["", f"{_files(needs_review)} left for manual review: {reasons}."]) + return "\n".join(lines) def _name_statuses(report: FileReport) -> dict[str, str]: @@ -265,13 +296,12 @@ def _join_with_blank_lines(entries: list[list[str]]) -> list[str]: return lines -def _modified_entry(report: FileReport, project_root: Path) -> list[str]: - """Return the MODIFIED lines for one file: its path relative to project_root, then what was done to each name. +def _fixed_descriptions(report: FileReport) -> dict[str, list[str]]: + """Return {name: [what was done to it]} for the names report.result fixed. A converted name isn't described as removed, because its declaration is always removed afterwards, and a removed name isn't described as localized. """ - lines = [f" {report.path.relative_to(project_root)}"] result = report.result or {} converted = {name for name, status in result.get("converted", {}).items() if status == "fixed"} removed = {name for name, status in result.get("orphaned", {}).items() if status == "fixed"} - converted @@ -280,7 +310,13 @@ def _modified_entry(report: FileReport, project_root: Path) -> list[str]: for name, status in names.items(): if status == "fixed" and not (phase == "orphaned" and name in converted) and not (phase == "cross_file" and name in removed): descriptions.setdefault(name, []).append(_FIXED_DESCRIPTIONS[phase]) - lines.extend(f" {name}: {', '.join(texts)}" for name, texts in descriptions.items()) + return descriptions + + +def _modified_entry(report: FileReport, project_root: Path) -> list[str]: + """Return the MODIFIED lines for one file: its path relative to project_root, then what was done to each name.""" + lines = [f" {report.path.relative_to(project_root)}"] + lines.extend(f" {name}: {', '.join(texts)}" for name, texts in _fixed_descriptions(report).items()) return lines @@ -436,8 +472,9 @@ def main(argv: Sequence[str] | None = None) -> int: ignore_localize_origins=ignore_localize_origins, ) print(console_report) - print() - print(colored(_format_commit_summary(reports), "green", attrs=["bold"])) + if modified_paths: + print() + print(colored(_format_commit_summary(reports, args.py), "green", attrs=["bold"])) if args.report is not None: args.report.write_text(console_report, encoding="utf-8") diff --git a/test/rejuvenation/test_migration_type_recipes.py b/test/rejuvenation/test_migration_type_recipes.py index 4bb8dd9d..7483ab44 100644 --- a/test/rejuvenation/test_migration_type_recipes.py +++ b/test/rejuvenation/test_migration_type_recipes.py @@ -328,6 +328,70 @@ def wrap(f: Callable[P, int]) -> Callable[P, int]: ) +_COMMIT_SUBJECT = "Modernize TypeVar/ParamSpec/TypeVarTuple usage to PEP 695 syntax" +_DEAD_TYPEVAR_SOURCE = 'from typing import TypeVar\n\nT_FreqStr = TypeVar("T_FreqStr")\n' +_TWELVE_TYPEVARS_SOURCE = "from typing import TypeVar\n\n" + "".join( + f'T{n:02} = TypeVar("T{n:02}")\n\n\ndef f{n:02}(x: T{n:02}) -> T{n:02}:\n return x\n\n\n' for n in range(1, 13) +) + + +class TestCommitSummary: + """main(): the commit-style message printed after the report.""" + + @pytest.mark.parametrize( + ("files", "expected"), + [ + pytest.param( + {"a.py": LEGACY_TYPEVAR_SOURCE}, + "Convert type parameters to PEP 695 syntax in 1 file (Python 3.12+).\n\n- converted to PEP 695 in functions: T", + id="converted-only", + ), + pytest.param( + {"a.py": LEGACY_TYPEVAR_SOURCE, "b.py": LEGACY_TYPEVAR_SOURCE}, + "Convert type parameters to PEP 695 syntax in 2 files (Python 3.12+).\n\n- converted to PEP 695 in functions: T (2 files)", + id="name-counted-per-file", + ), + pytest.param( + {"a.py": UNSAFE_TYPEVAR_SOURCE, "b.py": _DEAD_TYPEVAR_SOURCE}, + "Convert type parameters to PEP 695 syntax in 2 files (Python 3.12+).\n\n" + "- converted to PEP 695 in functions: T\n" + "- unused declaration removed: T_FreqStr\n\n" + f"1 file left for manual review: {UNSAFE_RULES[UnsafeReason.DECLARED_TYPEVAR_EXPORTED].message} (1).", + id="removed-and-left-for-review", + ), + pytest.param( + {"a.py": _TWELVE_TYPEVARS_SOURCE}, + "Convert type parameters to PEP 695 syntax in 1 file (Python 3.12+).\n\n" + "- converted to PEP 695 in functions: T01, T02, T03, T04, T05, T06, T07, T08, T09, T10 and 2 more", + id="long-list-is-capped", + ), + ], + ) + def test_lists_what_changed_per_action_and_what_is_left_for_review( + self, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], + files: dict[str, str], + expected: str, + ) -> None: + """Names are grouped by what was done to them, with the files they were fixed in; leftover review is summarised by reason.""" + for name, source in files.items(): + (tmp_path / name).write_text(source, encoding="utf-8") + + migration.main([str(tmp_path), "--py", "3.12", "--no-ruff"]) + + summary = capsys.readouterr().out.split(_COMMIT_SUBJECT, 1)[1].strip() + assert_that(summary, equal_to(expected)) + + def test_no_message_when_no_file_was_modified(self, tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None: + """A run that changes nothing prints no commit message.""" + (tmp_path / "a.py").write_text("x = 1\n", encoding="utf-8") + + migration.main([str(tmp_path), "--py", "3.12", "--no-ruff"]) + + assert_that(capsys.readouterr().out, not_(contains_string(_COMMIT_SUBJECT))) + + class TestIgnoreLocalize: """main(): --ignore-localize keeps the imports of a shared declaration file and reports them.""" From c6eabf6fb02c13def83f6e0669029b331a633e6c Mon Sep 17 00:00:00 2001 From: Francesco Pezzella Date: Tue, 20 Oct 2026 14:15:35 +0200 Subject: [PATCH 22/25] Replace --ignore-localize with --ignore, which leaves the named files untouched and keeps their imports in other files. Document that the tool only sees the scanned project, so unused type parameters in a shared module are removed unless it is passed to --ignore. --- docs/developer/modules/recipes.md | 7 ++-- .../features/type-parameter-modernization.md | 31 +++++++------- src/rejuvenation/migration-type-recipes.py | 41 ++++++++++--------- .../test_migration_type_recipes.py | 23 +++++++---- 4 files changed, 57 insertions(+), 45 deletions(-) diff --git a/docs/developer/modules/recipes.md b/docs/developer/modules/recipes.md index 6f4917ac..276eddc9 100644 --- a/docs/developer/modules/recipes.md +++ b/docs/developer/modules/recipes.md @@ -45,7 +45,7 @@ safely cover. This page covers `TypeVarCheck`, the recipe behind - `type_param_kinds`: maps each type parameter name the phases see to `"TypeVar"`, `"ParamSpec"` or `"TypeVarTuple"`. - Attributes set by the CLI (`migration-type-recipes.py`): `min_python`, `project_root` (falls back to the file's folder), `project_wide_imported_names` (from `collect_project_imported_names`) and `ignore_localize_origins` - (absolute paths of the files `--ignore-localize` names). `ignored_names` holds the names this file imports from them. + (absolute paths of the files `--ignore` names). `ignored_names` holds the names this file imports from them. - `PythonRefactoring.process("TypeVarCheck", file)`, used by `cli.py refactor`, never sets those attributes, so `min_python` stays `MIN_SUPPORTED_PYTHON` there and a `default=` declaration is reported `"unsafe"`. @@ -211,8 +211,9 @@ See *Verified by test modules* in [Type parameter modernization](../../user/feat `first` is converted and `T = TypeVar("T")` is removed, although `__all__` exports it. -- A declaration is only treated as public API when `__all__` lists it. One that nothing in the project uses is - removed, even if other projects import it. In: +- A declaration is only treated as public API when `__all__` lists it, since the tool sees nothing outside the scanned + project. One that nothing in the project uses is removed, even if other projects import it. `--ignore` leaves + such a file untouched. In: ```python # mylib/typing.py diff --git a/docs/user/features/type-parameter-modernization.md b/docs/user/features/type-parameter-modernization.md index d8cd3aec..4d3391d0 100644 --- a/docs/user/features/type-parameter-modernization.md +++ b/docs/user/features/type-parameter-modernization.md @@ -30,7 +30,7 @@ The tool runs three phases: 1. **Localize cross-file imports.** A type parameter imported from another file in the project (`from .shapes import T`) is replaced by an exact local copy of its declaration, placed after the file's imports, - unless the origin is named by `--ignore-localize` (see *Shared declaration files* below). + unless the origin is named by `--ignore` (see *Shared declaration files* below). 2. **Convert to PEP 695.** Every function whose signature uses a declared type parameter gets its own `[T]`, with bounds, constraints and `default=` carried over (`def f[T: str = str](...)`). `covariant=` and `contravariant=` are dropped, since only generic classes have a variance. A nested function gets none when an @@ -52,20 +52,20 @@ skipped without a report: - re-exports through an intermediate `__init__.py`, and namespace packages; - a renamed import (`from .shapes import T as U`); a file whose only type parameters arrive that way counts as clean. -### Shared declaration files { #feature-type-parameter-modernization-ignore-localize } +### Shared declaration files { #feature-type-parameter-modernization-ignore } -`--ignore-localize FILE...` names files, relative to `` as the report lists them, whose type parameters are -never copied into the files importing them. Those files keep their imports, so the functions using the imported names -are not converted. Use it for a file that is shared on purpose, such as a `types.py`. +`--ignore FILE...` names files, relative to `` as the report lists them, that are left untouched: no phase runs +on them, and their type parameters are never copied into the files importing them. Those files keep their imports, so +the functions using the imported names are not converted. Use it for a file that other projects may import, such as a +shared `types.py`. ```shell -python src/rejuvenation/migration-type-recipes.py ./some_repo --py 3.12 --ignore-localize pkg/types.py pkg/aliases.py +python src/rejuvenation/migration-type-recipes.py ./some_repo --py 3.12 --ignore pkg/types.py pkg/aliases.py ``` -The named files are processed as usual. Give `` first, since the option takes every value after it. A file that -does not exist is a usage error. The report lists the files under `NOT LOCALIZED FROM` and adds an `Ignored` column to -the table, counting each ignored name once per importing file. A file whose only type parameters are ignored counts -as clean. +Give `` first, since the option takes every value after it. A file that does not exist is a usage error. The +report lists the files under `IGNORED FILES` and adds an `Ignored` column to the table, counting each name imported +from an ignored file once per importing file. The ignored files are not in the processed count. ### Generic classes { #feature-type-parameter-modernization-generic-class } @@ -109,8 +109,9 @@ example under *Non-goals* or *Need to fix* in [Refactoring recipes](../../develo `TypeVar(name)`) gets a wrong rewrite or is skipped with an error. - `U = TypeVar("U", default=T)` becomes `def f[U = T]`, where `T` still means the module-level one. Fix it by hand as `def f[T, U = T]`. -- Only `__all__` marks a declaration as public. One that nothing in the project uses is removed, even if other - projects import it. +- Only `__all__` marks a declaration as public, since the tool sees nothing outside the scanned project. One that + nothing in the project uses is removed, even if other projects import it, as with a shared `types.py`. Name such a + file with `--ignore` (see *Shared declaration files*). - Only a plain `__all__ = [...]` is read. Annotated (`__all__: list[str] = ...`), `+=`, `.extend()` and `.append()` forms are not. - A `T` used only in the string value of a `TypeAlias` (`A: TypeAlias = "list[T]"`) is seen as unused, so its @@ -353,14 +354,14 @@ run the tool again. ```shell python src/rejuvenation/migration-type-recipes.py --py MAJOR.MINOR \ - [--report PATH] [--no-ruff] [--ignore-localize FILE...] + [--report PATH] [--no-ruff] [--ignore FILE...] ``` - ``: a `.py` file or a directory, scanned recursively (`.git`, `__pycache__`, `.venv` and `venv` skipped). - `--py` (required): the oldest Python version the target project supports, 3.12 or higher. - `--report`: also write the report to a file. - `--no-ruff`: skip the final `ruff` pass. -- `--ignore-localize`: files whose type parameters are never copied into the files importing them (see +- `--ignore`: files left untouched, whose type parameters are never copied into the files importing them (see *Shared declaration files*). After the recipe, the CLI runs `ruff check --fix --select F401,I001` on the changed files. This removes **every** @@ -374,7 +375,7 @@ The tables count the type parameters per kind (`TypeVar`, `ParamSpec`, `TypeVarT review from most to least common. A name counts once per file, as needing review if it has a reason to review. `MODIFIED` says what was done to each name: converted in functions, import replaced by a local declaration, or unused declaration removed; a name localized and then removed is listed only as removed. `NEEDS MANUAL REVIEW` gives each name its -reason, and a name can be in both sections. With `--ignore-localize` the report also has the `NOT LOCALIZED FROM` +reason, and a name can be in both sections. With `--ignore` the report also has the `IGNORED FILES` section and the `Ignored` column. Files are listed relative to ``. When files were modified, the run ends with a commit message: the names grouped by what was done to them (with the number of files if above one, at most ten per group), then the files left for manual review by reason. Exit codes: 0 when the run completes (manual diff --git a/src/rejuvenation/migration-type-recipes.py b/src/rejuvenation/migration-type-recipes.py index 7bdc2bed..ca3cef23 100644 --- a/src/rejuvenation/migration-type-recipes.py +++ b/src/rejuvenation/migration-type-recipes.py @@ -10,7 +10,7 @@ manual review. Example: - python src/rejuvenation/migration-type-recipes.py ./repo --py 3.12 --report review.md --ignore-localize repo/folder/types.py + python src/rejuvenation/migration-type-recipes.py ./repo --py 3.12 --report review.md --ignore folder/types.py """ @@ -120,7 +120,7 @@ def process_file( min_python: tuple[int, int], project_root: Path, project_wide_imported_names: frozenset[str], - ignore_localize_origins: frozenset[Path] = frozenset(), + ignored_files: frozenset[Path] = frozenset(), ) -> FileReport: """Run TypeVarCheck's three phases on one file and return its FileReport. @@ -131,7 +131,7 @@ def process_file( recipe.min_python = min_python recipe.project_root = project_root recipe.project_wide_imported_names = project_wide_imported_names - recipe.ignore_localize_origins = ignore_localize_origins + recipe.ignore_localize_origins = ignored_files result = recipe.check() reasons = { "cross_file": recipe.cross_file_unsafe_reasons, @@ -333,15 +333,15 @@ def _format_console_report( *, ruff_ran: bool, project_root: Path, - ignore_localize_origins: Collection[Path], + ignored_files: Collection[Path], ) -> str: """Build the full per-file report: MODIFIED / NEEDS MANUAL REVIEW / ERRORS sections. Files are listed by their path relative to project_root, separated by a blank line in the MODIFIED and NEEDS MANUAL REVIEW sections. Clean files (no type parameter usage at all) are only counted, not listed, so the report shows what needs attention. The ruff import-cleanup line is only - included when ruff_ran is True. The NOT LOCALIZED FROM section and the Ignored column are only - included when ignore_localize_origins is not empty. + included when ruff_ran is True. The IGNORED FILES section and the Ignored column are only + included when ignored_files is not empty. """ modified = [report for report in reports if has_fixed(report)] needs_review = [report for report in reports if has_unsafe(report)] @@ -360,10 +360,10 @@ def _format_console_report( "Imports across the modified files above were also cleaned up via `ruff check --fix --select F401,I001`: " "unused ones dropped, the rest merged and sorted.", ) - if ignore_localize_origins: - lines.extend(["", f"NOT LOCALIZED FROM ({len(ignore_localize_origins)})"]) - lines.extend(f" {origin.relative_to(project_root)}" for origin in sorted(ignore_localize_origins)) - lines.extend(_summary_tables(reports, show_ignored=bool(ignore_localize_origins))) + if ignored_files: + lines.extend(["", f"IGNORED FILES ({len(ignored_files)})"]) + lines.extend(f" {ignored.relative_to(project_root)}" for ignored in sorted(ignored_files)) + lines.extend(_summary_tables(reports, show_ignored=bool(ignored_files))) lines.extend(["", f"MODIFIED ({len(modified)})"]) lines.extend(_join_with_blank_lines([_modified_entry(report, project_root) for report in modified])) @@ -385,7 +385,7 @@ def build_arg_parser() -> argparse.ArgumentParser: description="Modernize legacy TypeVar/ParamSpec/TypeVarTuple usage to PEP 695 syntax.", epilog=textwrap.dedent("""\ Examples: - python src/rejuvenation/migration-type-recipes.py ./repo --py 3.12 --report review.md --ignore-localize repo/folder/types.py + python src/rejuvenation/migration-type-recipes.py ./repo --py 3.12 --report review.md --ignore folder/types.py """), formatter_class=argparse.RawDescriptionHelpFormatter, ) @@ -404,15 +404,16 @@ def build_arg_parser() -> argparse.ArgumentParser: help="Skip the final `ruff check --fix --select F401,I001` pass that drops imports made unused and sorts the rest.", ) parser.add_argument( - "--ignore-localize", + "--ignore", action="extend", nargs="+", type=Path, default=[], metavar="FILE", help=( - "Files, relative to the scanned directory, whose type parameters are never copied into the files importing " - "them; those imports stay. Give the path argument first, since this option takes every value after it." + "Files, relative to the scanned directory, that are left untouched; their type parameters are not copied " + "into the files importing them, which keep those imports. Give the path argument first, since this option " + "takes every value after it." ), ) return parser @@ -439,20 +440,22 @@ def main(argv: Sequence[str] | None = None) -> int: project_root = target if target.is_dir() else target.parent imported_names_by_file = collect_project_imported_names(files, project_root) - for ignored in args.ignore_localize: + for ignored in args.ignore: if not (project_root / ignored).is_file(): - parser.error(f"--ignore-localize: file does not exist: {ignored}") - ignore_localize_origins = frozenset((project_root / ignored).resolve() for ignored in args.ignore_localize) + parser.error(f"--ignore: file does not exist: {ignored}") + ignored_files = frozenset((project_root / ignored).resolve() for ignored in args.ignore) reports: list[FileReport] = [] for path in files: + if path in ignored_files: + continue 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, - ignore_localize_origins=ignore_localize_origins, + ignored_files=ignored_files, ) reports.append(report) print(f"File {path} checked.") @@ -469,7 +472,7 @@ def main(argv: Sequence[str] | None = None) -> int: reports, ruff_ran=ruff_ran, project_root=project_root, - ignore_localize_origins=ignore_localize_origins, + ignored_files=ignored_files, ) print(console_report) if modified_paths: diff --git a/test/rejuvenation/test_migration_type_recipes.py b/test/rejuvenation/test_migration_type_recipes.py index 7483ab44..fe1f9377 100644 --- a/test/rejuvenation/test_migration_type_recipes.py +++ b/test/rejuvenation/test_migration_type_recipes.py @@ -392,23 +392,30 @@ def test_no_message_when_no_file_was_modified(self, tmp_path: Path, capsys: pyte assert_that(capsys.readouterr().out, not_(contains_string(_COMMIT_SUBJECT))) -class TestIgnoreLocalize: - """main(): --ignore-localize keeps the imports of a shared declaration file and reports them.""" +class TestIgnore: + """main(): --ignore leaves a shared declaration file and the imports of its names untouched, and reports them.""" - def test_importer_is_left_untouched_and_the_report_lists_it(self, tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None: - """The importer is unchanged, the ignored file is listed, and its name counts as reported and ignored, not for review.""" - (tmp_path / "shared.py").write_text('from typing import TypeVar\n\nT = TypeVar("T")\n', encoding="utf-8") + def test_ignored_file_and_its_importer_are_left_untouched_and_the_report_lists_it( + self, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], + ) -> None: + """Neither file changes, the ignored one is listed but not checked, and its name counts as reported and ignored.""" + shared_source = 'from typing import TypeVar\n\nT = TypeVar("T")\n\n\ndef first(items: list[T]) -> T:\n return items[0]\n' user_source = "from shared import T\n\n\ndef identity(x: T) -> T:\n return x\n" + (tmp_path / "shared.py").write_text(shared_source, encoding="utf-8") (tmp_path / "user.py").write_text(user_source, encoding="utf-8") - migration.main([str(tmp_path), "--py", "3.12", "--no-ruff", "--ignore-localize", "shared.py"]) + migration.main([str(tmp_path), "--py", "3.12", "--no-ruff", "--ignore", "shared.py"]) output = capsys.readouterr().out + assert_that((tmp_path / "shared.py").read_text(encoding="utf-8"), equal_to(shared_source)) assert_that((tmp_path / "user.py").read_text(encoding="utf-8"), equal_to(user_source)) - assert_that(output, contains_string("NOT LOCALIZED FROM (1)\n shared.py\n")) + assert_that(output, contains_string("IGNORED FILES (1)\n shared.py\n")) + assert_that(output, not_(contains_string("shared.py checked"))) assert_that( _table_rows(output, "Kind"), - equal_to([["TypeVar", "2", "0", "1", "1"], ["ParamSpec", "0", "0", "0", "0"], ["TypeVarTuple", "0", "0", "0", "0"]]), + equal_to([["TypeVar", "1", "0", "0", "1"], ["ParamSpec", "0", "0", "0", "0"], ["TypeVarTuple", "0", "0", "0", "0"]]), ) From 7ec994233ef24ca5efe39f99ab52524dc9ba3ef7 Mon Sep 17 00:00:00 2001 From: Francesco Pezzella Date: Tue, 20 Oct 2026 16:05:29 +0200 Subject: [PATCH 23/25] Add --no-localization to keep the imports of project type parameters instead of copying their declarations into the importing file. --- docs/developer/modules/recipes.md | 5 +- .../features/type-parameter-modernization.md | 27 +++++++--- src/rejuvenation/migration-type-recipes.py | 51 ++++++++++++++----- src/renaissance/recipes/type_var_check.py | 11 ++-- test/recipes/test_type_var_check_localize.py | 22 ++++---- .../test_migration_type_recipes.py | 21 ++++++++ 6 files changed, 102 insertions(+), 35 deletions(-) diff --git a/docs/developer/modules/recipes.md b/docs/developer/modules/recipes.md index 276eddc9..bc10fec6 100644 --- a/docs/developer/modules/recipes.md +++ b/docs/developer/modules/recipes.md @@ -44,8 +44,9 @@ safely cover. This page covers `TypeVarCheck`, the recipe behind - `type_param_kinds`: maps each type parameter name the phases see to `"TypeVar"`, `"ParamSpec"` or `"TypeVarTuple"`. - Attributes set by the CLI (`migration-type-recipes.py`): `min_python`, `project_root` (falls back to the file's - folder), `project_wide_imported_names` (from `collect_project_imported_names`) and `ignore_localize_origins` - (absolute paths of the files `--ignore` names). `ignored_names` holds the names this file imports from them. + folder), `project_wide_imported_names` (from `collect_project_imported_names`), `ignore_localize_origins` (absolute + paths of the files `--ignore` names) and `localize_imports` (`False` with `--no-localization`). `ignored_names` holds + the names this file imports from those files, or all its imported names when `localize_imports` is `False`. - `PythonRefactoring.process("TypeVarCheck", file)`, used by `cli.py refactor`, never sets those attributes, so `min_python` stays `MIN_SUPPORTED_PYTHON` there and a `default=` declaration is reported `"unsafe"`. diff --git a/docs/user/features/type-parameter-modernization.md b/docs/user/features/type-parameter-modernization.md index 4d3391d0..e2b86aac 100644 --- a/docs/user/features/type-parameter-modernization.md +++ b/docs/user/features/type-parameter-modernization.md @@ -30,7 +30,7 @@ The tool runs three phases: 1. **Localize cross-file imports.** A type parameter imported from another file in the project (`from .shapes import T`) is replaced by an exact local copy of its declaration, placed after the file's imports, - unless the origin is named by `--ignore` (see *Shared declaration files* below). + unless the origin is named by `--ignore` (see *Shared declaration files* below) or `--no-localization` is given. 2. **Convert to PEP 695.** Every function whose signature uses a declared type parameter gets its own `[T]`, with bounds, constraints and `default=` carried over (`def f[T: str = str](...)`). `covariant=` and `contravariant=` are dropped, since only generic classes have a variance. A nested function gets none when an @@ -67,6 +67,20 @@ Give `` first, since the option takes every value after it. A file that do report lists the files under `IGNORED FILES` and adds an `Ignored` column to the table, counting each name imported from an ignored file once per importing file. The ignored files are not in the processed count. +### Disabling localization { #feature-type-parameter-modernization-no-localization } + +Converting a function and removing an unused declaration only improve the file. Localizing does not always: the copy is a +second declaration, and a localized `T` is a different object from the original. `--no-localization` turns the first +phase off for the whole project. Every project import of a type parameter stays, so the functions using the imported +names are not converted. The other phases run as usual, on every file. + +```shell +python src/rejuvenation/migration-type-recipes.py ./some_repo --py 3.12 --no-localization +``` + +The report adds the `Ignored` column, counting each imported name once per importing file. `--ignore` differs in that +it leaves chosen files unprocessed, so their declarations are never removed. + ### Generic classes { #feature-type-parameter-modernization-generic-class } A class is *generic over* `T` when it declares `T` itself (`class Box[T]:`) or has `T` in a base (`Generic[T]`, @@ -354,7 +368,7 @@ run the tool again. ```shell python src/rejuvenation/migration-type-recipes.py --py MAJOR.MINOR \ - [--report PATH] [--no-ruff] [--ignore FILE...] + [--report PATH] [--no-ruff] [--ignore FILE...] [--no-localization] ``` - ``: a `.py` file or a directory, scanned recursively (`.git`, `__pycache__`, `.venv` and `venv` skipped). @@ -363,6 +377,7 @@ python src/rejuvenation/migration-type-recipes.py --py MAJOR.MINOR \ - `--no-ruff`: skip the final `ruff` pass. - `--ignore`: files left untouched, whose type parameters are never copied into the files importing them (see *Shared declaration files*). +- `--no-localization`: keep every project import of a type parameter (see *Disabling localization*). After the recipe, the CLI runs `ruff check --fix --select F401,I001` on the changed files. This removes **every** unused import in them, including ones that were unused before (such as `from typing import TypeVar` after the @@ -375,10 +390,10 @@ The tables count the type parameters per kind (`TypeVar`, `ParamSpec`, `TypeVarT review from most to least common. A name counts once per file, as needing review if it has a reason to review. `MODIFIED` says what was done to each name: converted in functions, import replaced by a local declaration, or unused declaration removed; a name localized and then removed is listed only as removed. `NEEDS MANUAL REVIEW` gives each name its -reason, and a name can be in both sections. With `--ignore` the report also has the `IGNORED FILES` -section and the `Ignored` column. Files are listed relative to ``. When files were modified, the run ends with a -commit message: the names grouped by what was done to them (with the number of files if above one, at most ten per -group), then the files left for manual review by reason. Exit codes: 0 when the run completes (manual +reason, and a name can be in both sections. With `--ignore` the report also has the `IGNORED FILES` section, and with +`--ignore` or `--no-localization` the `Ignored` column. Files are listed relative to ``. When files were modified, +the run ends with a commit message: the names grouped by what was done to them (with the number of files if above one, +at most ten per group), then the files left for manual review by reason. Exit codes: 0 when the run completes (manual review is not a failure), 2 for a usage error, 3 if any file raised an exception. ## Change considerations diff --git a/src/rejuvenation/migration-type-recipes.py b/src/rejuvenation/migration-type-recipes.py index ca3cef23..0e63eb52 100644 --- a/src/rejuvenation/migration-type-recipes.py +++ b/src/rejuvenation/migration-type-recipes.py @@ -22,7 +22,7 @@ import sys import textwrap from collections import Counter -from collections.abc import Collection, Sequence +from collections.abc import Sequence from dataclasses import dataclass from pathlib import Path @@ -65,6 +65,17 @@ class KindCount: ignored: int = 0 +@dataclass(frozen=True) +class LocalizationSettings: + """Whether imported type parameters are copied into the importing file, and the files they are never copied from.""" + + enabled: bool = True + ignored_files: frozenset[Path] = frozenset() + + +_DEFAULT_LOCALIZATION = LocalizationSettings() + + def resolve_target_files(target: Path) -> list[Path]: """Return the .py files to process for `target`. @@ -120,7 +131,7 @@ def process_file( min_python: tuple[int, int], project_root: Path, project_wide_imported_names: frozenset[str], - ignored_files: frozenset[Path] = frozenset(), + localization: LocalizationSettings = _DEFAULT_LOCALIZATION, ) -> FileReport: """Run TypeVarCheck's three phases on one file and return its FileReport. @@ -131,7 +142,8 @@ def process_file( recipe.min_python = min_python recipe.project_root = project_root recipe.project_wide_imported_names = project_wide_imported_names - recipe.ignore_localize_origins = ignored_files + recipe.ignore_localize_origins = localization.ignored_files + recipe.localize_imports = localization.enabled result = recipe.check() reasons = { "cross_file": recipe.cross_file_unsafe_reasons, @@ -333,15 +345,15 @@ def _format_console_report( *, ruff_ran: bool, project_root: Path, - ignored_files: Collection[Path], + localization: LocalizationSettings, ) -> str: """Build the full per-file report: MODIFIED / NEEDS MANUAL REVIEW / ERRORS sections. Files are listed by their path relative to project_root, separated by a blank line in the MODIFIED and NEEDS MANUAL REVIEW sections. Clean files (no type parameter usage at all) are only counted, not listed, so the report shows what needs attention. The ruff import-cleanup line is only - included when ruff_ran is True. The IGNORED FILES section and the Ignored column are only - included when ignored_files is not empty. + included when ruff_ran is True. The IGNORED FILES section is only included when + localization.ignored_files is not empty, and the Ignored column when it is not empty or localization is disabled. """ modified = [report for report in reports if has_fixed(report)] needs_review = [report for report in reports if has_unsafe(report)] @@ -360,10 +372,10 @@ def _format_console_report( "Imports across the modified files above were also cleaned up via `ruff check --fix --select F401,I001`: " "unused ones dropped, the rest merged and sorted.", ) - if ignored_files: - lines.extend(["", f"IGNORED FILES ({len(ignored_files)})"]) - lines.extend(f" {ignored.relative_to(project_root)}" for ignored in sorted(ignored_files)) - lines.extend(_summary_tables(reports, show_ignored=bool(ignored_files))) + if localization.ignored_files: + lines.extend(["", f"IGNORED FILES ({len(localization.ignored_files)})"]) + lines.extend(f" {ignored.relative_to(project_root)}" for ignored in sorted(localization.ignored_files)) + lines.extend(_summary_tables(reports, show_ignored=bool(localization.ignored_files) or not localization.enabled)) lines.extend(["", f"MODIFIED ({len(modified)})"]) lines.extend(_join_with_blank_lines([_modified_entry(report, project_root) for report in modified])) @@ -416,6 +428,14 @@ def build_arg_parser() -> argparse.ArgumentParser: "takes every value after it." ), ) + parser.add_argument( + "--no-localization", + action="store_true", + help=( + "Keep the imports of project type parameters instead of copying their declarations into the importing file; " + "the functions using them are not converted." + ), + ) return parser @@ -443,11 +463,14 @@ def main(argv: Sequence[str] | None = None) -> int: for ignored in args.ignore: if not (project_root / ignored).is_file(): parser.error(f"--ignore: file does not exist: {ignored}") - ignored_files = frozenset((project_root / ignored).resolve() for ignored in args.ignore) + localization = LocalizationSettings( + enabled=not args.no_localization, + ignored_files=frozenset((project_root / ignored).resolve() for ignored in args.ignore), + ) reports: list[FileReport] = [] for path in files: - if path in ignored_files: + if path in localization.ignored_files: continue project_wide_imported_names = imported_names_by_file.get(path, frozenset()) report = process_file( @@ -455,7 +478,7 @@ def main(argv: Sequence[str] | None = None) -> int: min_python=args.py, project_root=project_root, project_wide_imported_names=project_wide_imported_names, - ignored_files=ignored_files, + localization=localization, ) reports.append(report) print(f"File {path} checked.") @@ -472,7 +495,7 @@ def main(argv: Sequence[str] | None = None) -> int: reports, ruff_ran=ruff_ran, project_root=project_root, - ignored_files=ignored_files, + localization=localization, ) print(console_report) if modified_paths: diff --git a/src/renaissance/recipes/type_var_check.py b/src/renaissance/recipes/type_var_check.py index 488e9559..68926859 100644 --- a/src/renaissance/recipes/type_var_check.py +++ b/src/renaissance/recipes/type_var_check.py @@ -75,11 +75,15 @@ class TypeVarCheck(PythonRefactoring[PythonCstNode]): # Absolute paths of the files whose declarations are never copied into the files importing them. ignore_localize_origins: frozenset[Path] = frozenset() + # When False, no declaration is copied into the files importing it. + localize_imports: bool = True + def __init__(self, file: Path) -> None: """Prepare the recipe for file, parsed with the CST backend. type_param_kinds maps each type parameter name the phases see to "TypeVar", "ParamSpec" or "TypeVarTuple". - ignored_names holds the names this file imports from an ignore_localize_origins file. + ignored_names holds the names this file imports from an ignore_localize_origins file, or all of its + imported names when localize_imports is False. """ super().__init__(file, PythonCstNode) self.type_param_kinds: dict[str, str] = {} @@ -279,7 +283,8 @@ def localize_imported_typevars(self) -> dict[str, str]: file's last import, so no import is left below code. A declaration stays where its import was when a statement in between uses it. It is unsafe when is_safe_to_localize rejects the origin or _argument_imports returns None. - Names from an ignore_localize_origins file keep their import and go to self.ignored_names instead. + Names from an ignore_localize_origins file, or all names when localize_imports is False, keep their import + and go to self.ignored_names instead. Returns {name: "fixed" | "unsafe"}, with each UnsafeReason on self.cross_file_unsafe_reasons. """ results: dict[str, str] = {} @@ -331,7 +336,7 @@ def _localize_names_from( decl_stmt = declarations[name] self.type_param_kinds[name] = type_param_constructor_name(decl_stmt) - if origin_path in self.ignore_localize_origins: + if not self.localize_imports or origin_path in self.ignore_localize_origins: self.ignored_names.add(name) continue reason = is_safe_to_localize(origin, name) diff --git a/test/recipes/test_type_var_check_localize.py b/test/recipes/test_type_var_check_localize.py index a75882d3..4b433c99 100644 --- a/test/recipes/test_type_var_check_localize.py +++ b/test/recipes/test_type_var_check_localize.py @@ -112,22 +112,23 @@ def b(x: T) -> T: assert_that(output, not_(contains_string("from file_1 import T"))) @pytest.mark.parametrize( - ("origin_ignored", "expected_result", "expected_ignored_names"), + ("origin_ignored", "localize_imports", "localized"), [ - pytest.param(True, {}, {"T"}, id="ignored-origin"), - pytest.param(False, {"T": "fixed"}, set[str](), id="origin-not-ignored"), + pytest.param(True, True, False, id="ignored-origin"), + pytest.param(False, False, False, id="localization-off"), + pytest.param(False, True, True, id="origin-not-ignored"), ], ) - def test_names_from_an_ignored_origin_are_not_localized( + def test_names_are_not_localized_from_an_ignored_origin_or_when_localization_is_off( self, mocker: MockerFixture, tmp_path: Path, *, origin_ignored: bool, - expected_result: dict[str, str], - expected_ignored_names: set[str], + localize_imports: bool, + localized: bool, ) -> None: - """Verify an import from an origin in ignore_localize_origins is kept and its names are recorded as ignored.""" + """Verify an import from an ignored origin, or any import with localize_imports off, is kept and recorded as ignored.""" subject = self._create_cross_file( mocker, tmp_path, @@ -138,14 +139,15 @@ def b(x: T) -> T: return x """, ) + subject.localize_imports = localize_imports if origin_ignored: subject.ignore_localize_origins = frozenset({tmp_path / "file_1.py"}) result = subject.localize_imported_typevars() - assert_that(result, equal_to(expected_result)) - assert_that(subject.ignored_names, equal_to(expected_ignored_names)) - assert_that("from file_1 import T" in subject.apply_to_string(), equal_to(origin_ignored)) + assert_that(result, equal_to({"T": "fixed"} if localized else {})) + assert_that(subject.ignored_names, equal_to(set[str]() if localized else {"T"})) + assert_that("from file_1 import T" in subject.apply_to_string(), equal_to(not localized)) @pytest.mark.parametrize( "declaration", diff --git a/test/rejuvenation/test_migration_type_recipes.py b/test/rejuvenation/test_migration_type_recipes.py index fe1f9377..bc599097 100644 --- a/test/rejuvenation/test_migration_type_recipes.py +++ b/test/rejuvenation/test_migration_type_recipes.py @@ -419,6 +419,27 @@ def test_ignored_file_and_its_importer_are_left_untouched_and_the_report_lists_i ) +class TestNoLocalization: + """main(): --no-localization keeps every project import of a type parameter and reports the names left.""" + + def test_importer_is_left_untouched_and_the_report_counts_the_name_as_ignored( + self, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], + ) -> None: + """The importer is unchanged, no file is listed as ignored, and the imported name counts as reported and ignored.""" + (tmp_path / "shared.py").write_text('from typing import TypeVar\n\nT = TypeVar("T")\n', encoding="utf-8") + user_source = "from shared import T\n\n\ndef identity(x: T) -> T:\n return x\n" + (tmp_path / "user.py").write_text(user_source, encoding="utf-8") + + migration.main([str(tmp_path), "--py", "3.12", "--no-ruff", "--no-localization"]) + + output = capsys.readouterr().out + assert_that((tmp_path / "user.py").read_text(encoding="utf-8"), equal_to(user_source)) + assert_that(output, not_(contains_string("IGNORED FILES"))) + assert_that(_table_rows(output, "Kind")[0], equal_to(["TypeVar", "2", "0", "1", "1"])) + + class TestPerFileProgressFeedback: """main(): prints a per-file progress line as each file is checked.""" From 626586edd1c0c1fa6e2996055737facbd1734bc6 Mon Sep 17 00:00:00 2001 From: Francesco Pezzella Date: Tue, 20 Oct 2026 16:15:52 +0200 Subject: [PATCH 24/25] Group the manual-review section of the migration report by reason and style its headings, paths and names as Markdown. Drop the redundant ruff import-cleanup sentence. --- .../features/type-parameter-modernization.md | 26 +++--- src/rejuvenation/migration-type-recipes.py | 82 +++++++++------- .../test_migration_type_recipes.py | 93 ++++++++++++------- 3 files changed, 126 insertions(+), 75 deletions(-) diff --git a/docs/user/features/type-parameter-modernization.md b/docs/user/features/type-parameter-modernization.md index e2b86aac..0c1b86c4 100644 --- a/docs/user/features/type-parameter-modernization.md +++ b/docs/user/features/type-parameter-modernization.md @@ -382,19 +382,19 @@ python src/rejuvenation/migration-type-recipes.py --py MAJOR.MINOR \ After the recipe, the CLI runs `ruff check --fix --select F401,I001` on the changed files. This removes **every** unused import in them, including ones that were unused before (such as `from typing import TypeVar` after the declaration is gone). It also merges and sorts **every** import block in them, including imports that were unsorted -before, using the `ruff` settings of the project. `ruff` must be installed in the same environment; if it cannot run, -the report still says the pass ran. Files are changed directly, so run it on a git checkout. - -The report has a summary line, two summary tables and the sections `MODIFIED`, `NEEDS MANUAL REVIEW` and `ERRORS`. -The tables count the type parameters per kind (`TypeVar`, `ParamSpec`, `TypeVarTuple`) and rank the reasons for manual -review from most to least common. A name counts once per file, as needing review if it has a reason to review. `MODIFIED` -says what was done to each name: converted in functions, import replaced by a local declaration, or unused declaration -removed; a name localized and then removed is listed only as removed. `NEEDS MANUAL REVIEW` gives each name its -reason, and a name can be in both sections. With `--ignore` the report also has the `IGNORED FILES` section, and with -`--ignore` or `--no-localization` the `Ignored` column. Files are listed relative to ``. When files were modified, -the run ends with a commit message: the names grouped by what was done to them (with the number of files if above one, -at most ten per group), then the files left for manual review by reason. Exit codes: 0 when the run completes (manual -review is not a failure), 2 for a usage error, 3 if any file raised an exception. +before, using the `ruff` settings of the project. `ruff` must be installed in the same environment. Files are changed +directly, so run it on a git checkout. + +The report is Markdown, with a summary line, two summary tables and the sections `MODIFIED`, `NEEDS MANUAL REVIEW` and +`ERRORS`. The tables count the type parameters per kind (`TypeVar`, `ParamSpec`, `TypeVarTuple`) and rank the reasons for +manual review from most to least common. A name counts once per file, as needing review if it has a reason to review. +`MODIFIED` says what was done to each name: converted in functions, import replaced by a local declaration, or unused +declaration removed; a name localized and then removed is listed only as removed. `NEEDS MANUAL REVIEW` groups the files +and names by reason, most common reason first, and a name can be in both sections. With `--ignore` the report also has +the `IGNORED FILES` section, and with `--ignore` or `--no-localization` the `Ignored` column. Files are listed relative +to ``. When files were modified, the run ends with a commit message: the names grouped by what was done to them +(with the number of files if above one, at most ten per group), then the files left for manual review by reason. Exit +codes: 0 when the run completes (manual review is not a failure), 2 for a usage error, 3 if any file raised an exception. ## Change considerations diff --git a/src/rejuvenation/migration-type-recipes.py b/src/rejuvenation/migration-type-recipes.py index 0e63eb52..7fcec318 100644 --- a/src/rejuvenation/migration-type-recipes.py +++ b/src/rejuvenation/migration-type-recipes.py @@ -269,6 +269,16 @@ def rank_reasons(reports: Sequence[FileReport]) -> list[tuple[UnsafeReason, int] return sorted(counter.items(), key=lambda item: (-item[1], order.index(item[0]))) +def _code(text: object) -> str: + """Return text as a Markdown inline code span.""" + return f"`{text}`" + + +def _heading(text: str) -> str: + """Return text as a bold Markdown line, used for the report's section headings.""" + return f"**{text}**" + + def _table_row(cells: Sequence[str], widths: Sequence[int]) -> str: """Return one Markdown table row with each cell padded to its column width.""" return "| " + " | ".join(cell.ljust(width) for cell, width in zip(cells, widths, strict=True)) + " |" @@ -287,14 +297,14 @@ def _summary_tables(reports: Sequence[FileReport], *, show_ignored: bool) -> lis """ headers = ["Kind", "Reported", "Fixed", "Needs review", *(["Ignored"] if show_ignored else [])] kind_rows = [ - [kind, str(c.reported), str(c.fixed), str(c.needs_review), *([str(c.ignored)] if show_ignored else [])] + [_code(kind), str(c.reported), str(c.fixed), str(c.needs_review), *([str(c.ignored)] if show_ignored else [])] for kind, c in count_by_kind(reports).items() ] - lines = ["", "TYPE PARAMETERS BY KIND", "", *_format_table(headers, kind_rows)] + lines = ["", _heading("TYPE PARAMETERS BY KIND"), "", *_format_table(headers, kind_rows)] ranking = rank_reasons(reports) if ranking: reason_rows = [[UNSAFE_RULES[reason].message, str(count)] for reason, count in ranking] - lines.extend(["", "UNSAFE REASONS, MOST COMMON FIRST", "", *_format_table(["Reason", "Count"], reason_rows)]) + lines.extend(["", _heading("UNSAFE REASONS, MOST COMMON FIRST"), "", *_format_table(["Reason", "Count"], reason_rows)]) return lines @@ -327,33 +337,49 @@ def _fixed_descriptions(report: FileReport) -> dict[str, list[str]]: def _modified_entry(report: FileReport, project_root: Path) -> list[str]: """Return the MODIFIED lines for one file: its path relative to project_root, then what was done to each name.""" - lines = [f" {report.path.relative_to(project_root)}"] - lines.extend(f" {name}: {', '.join(texts)}" for name, texts in _fixed_descriptions(report).items()) + lines = [f" {_code(report.path.relative_to(project_root))}"] + lines.extend(f" {_code(name)}: {', '.join(texts)}" for name, texts in _fixed_descriptions(report).items()) return lines -def _needs_review_entry(report: FileReport, project_root: Path) -> list[str]: - """Return the NEEDS MANUAL REVIEW lines for one file: its path relative to project_root, then each name with its reason.""" - lines = [f" {report.path.relative_to(project_root)}"] - for phase_reasons in (report.reasons or {}).values(): - lines.extend(f" {name}: {UNSAFE_RULES[reason].message}" for name, reason in phase_reasons.items()) - return lines +def _needs_review_groups(reports: Sequence[FileReport], project_root: Path) -> list[list[str]]: + """Return one block of NEEDS MANUAL REVIEW lines per reason, most common reason first. + + A block is the reason as a heading, then each file it applies to with the names it applies to. + """ + names_by_reason: dict[UnsafeReason, dict[Path, list[str]]] = {} + for report in reports: + for phase_reasons in (report.reasons or {}).values(): + for name, reason in phase_reasons.items(): + names = names_by_reason.setdefault(reason, {}).setdefault(report.path, []) + if name not in names: + names.append(name) + groups: list[list[str]] = [] + for reason, _ in rank_reasons(reports): + message = UNSAFE_RULES[reason].message + files = names_by_reason[reason].items() + groups.append( + [ + _heading(message[:1].upper() + message[1:]), + *(f" {_code(path.relative_to(project_root))}: {', '.join(map(_code, names))}" for path, names in files), + ], + ) + return groups def _format_console_report( reports: list[FileReport], *, - ruff_ran: bool, project_root: Path, localization: LocalizationSettings, ) -> str: - """Build the full per-file report: MODIFIED / NEEDS MANUAL REVIEW / ERRORS sections. + """Build the full report as Markdown: MODIFIED / NEEDS MANUAL REVIEW / ERRORS sections. - Files are listed by their path relative to project_root, separated by a blank line in the MODIFIED - and NEEDS MANUAL REVIEW sections. Clean files (no type parameter usage at all) are only counted, - not listed, so the report shows what needs attention. The ruff import-cleanup line is only - included when ruff_ran is True. The IGNORED FILES section is only included when - localization.ignored_files is not empty, and the Ignored column when it is not empty or localization is disabled. + Files are listed by their path relative to project_root. MODIFIED lists each file with what was done to + its names, separated by a blank line; NEEDS MANUAL REVIEW groups the files and names by reason. Clean + files (no type parameter usage at all) are only counted, not listed, so the report shows what needs + attention. The IGNORED FILES section is only included when localization.ignored_files is not empty, + and the Ignored column when it is not empty or localization is disabled. """ modified = [report for report in reports if has_fixed(report)] needs_review = [report for report in reports if has_unsafe(report)] @@ -367,25 +393,20 @@ def _format_console_report( f"manual review, {clean_count} clean, {len(errors)} errors" ), ] - if ruff_ran: - lines.append( - "Imports across the modified files above were also cleaned up via `ruff check --fix --select F401,I001`: " - "unused ones dropped, the rest merged and sorted.", - ) if localization.ignored_files: - lines.extend(["", f"IGNORED FILES ({len(localization.ignored_files)})"]) - lines.extend(f" {ignored.relative_to(project_root)}" for ignored in sorted(localization.ignored_files)) + lines.extend(["", _heading(f"IGNORED FILES ({len(localization.ignored_files)})")]) + lines.extend(f" {_code(ignored.relative_to(project_root))}" for ignored in sorted(localization.ignored_files)) lines.extend(_summary_tables(reports, show_ignored=bool(localization.ignored_files) or not localization.enabled)) - lines.extend(["", f"MODIFIED ({len(modified)})"]) + lines.extend(["", _heading(f"MODIFIED ({len(modified)})")]) lines.extend(_join_with_blank_lines([_modified_entry(report, project_root) for report in modified])) - lines.extend(["", f"NEEDS MANUAL REVIEW ({len(needs_review)})"]) + lines.extend(["", _heading(f"NEEDS MANUAL REVIEW ({len(needs_review)})")]) if needs_review: lines.extend([f" {DOCS_BASE_URL}", ""]) - lines.extend(_join_with_blank_lines([_needs_review_entry(report, project_root) for report in needs_review])) + lines.extend(_join_with_blank_lines(_needs_review_groups(needs_review, project_root))) - lines.extend(["", f"ERRORS ({len(errors)})"]) - lines.extend(f" {report.path.relative_to(project_root)}: {report.error}" for report in errors) + lines.extend(["", _heading(f"ERRORS ({len(errors)})")]) + lines.extend(f" {_code(report.path.relative_to(project_root))}: {report.error}" for report in errors) return "\n".join(lines) @@ -493,7 +514,6 @@ def main(argv: Sequence[str] | None = None) -> int: console_report = _format_console_report( reports, - ruff_ran=ruff_ran, project_root=project_root, localization=localization, ) diff --git a/test/rejuvenation/test_migration_type_recipes.py b/test/rejuvenation/test_migration_type_recipes.py index bc599097..f9b231c8 100644 --- a/test/rejuvenation/test_migration_type_recipes.py +++ b/test/rejuvenation/test_migration_type_recipes.py @@ -102,31 +102,19 @@ class TestRuffImportCleanup: """main(): the ruff F401 batch step actually drops now-unused imports end to end.""" @pytest.mark.parametrize( - ("extra_args", "import_matcher", "report_matcher"), + ("extra_args", "import_matcher"), [ - pytest.param( - [], - not_(contains_string("TypeVar")), - contains_string("cleaned up via `ruff"), - id="default-drops-import", - ), - pytest.param( - ["--no-ruff"], - contains_string("from typing import TypeVar"), - not_(contains_string("cleaned up via `ruff")), - id="no-ruff-keeps-import", - ), + pytest.param([], 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, - capsys: pytest.CaptureFixture[str], extra_args: list[str], import_matcher: Matcher[str], - report_matcher: Matcher[str], ) -> None: - """The redundant TypeVar import is dropped, and the report says so, unless --no-ruff; conversion runs either way.""" + """The redundant TypeVar import is dropped unless --no-ruff; conversion runs either way.""" target = tmp_path / "mod.py" target.write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") @@ -136,7 +124,6 @@ def test_unused_typevar_import_cleanup( written = target.read_text(encoding="utf-8") assert_that(written, contains_string("def identity[T]")) assert_that(written, import_matcher) - assert_that(capsys.readouterr().out, report_matcher) def test_localized_imports_are_merged_with_existing_ones(self, tmp_path: Path) -> None: """The localized TypeVar's import joins the file's own `typing` import.""" @@ -190,14 +177,14 @@ def greet() -> str: class TestConsoleReportNeedsReviewSection: - """main(): what the NEEDS MANUAL REVIEW section lists per file.""" + """main(): what the NEEDS MANUAL REVIEW section lists per reason.""" - def test_names_come_with_their_reason_under_one_shared_documentation_link( + def test_names_are_listed_per_file_under_their_reason_and_the_documentation_is_linked_once( self, tmp_path: Path, capsys: pytest.CaptureFixture[str], ) -> None: - """Each name is listed with its reason in words; the documentation is linked once, without per-name anchors.""" + """Each reason is a heading with its files and names below it; the documentation is linked once, without per-name anchors.""" target = tmp_path / "mod.py" target.write_text(UNSAFE_TYPEVAR_SOURCE, encoding="utf-8") @@ -206,9 +193,53 @@ def test_names_come_with_their_reason_under_one_shared_documentation_link( output = capsys.readouterr().out assert_that(output.count(DOCS_BASE_URL), equal_to(1)) assert_that(output, not_(contains_string("#feature-"))) - assert_that(output, contains_string(" T: exported via __all__\n")) + assert_that(output, contains_string("**Exported via __all__**\n `mod.py`: `T`\n")) assert_that(output, not_(contains_string(" orphaned:"))) + def test_files_and_names_sharing_a_reason_are_listed_under_one_heading( + self, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], + ) -> None: + """A reason is printed once, with every file it applies to, and a file lists all its names on one line.""" + (tmp_path / "one.py").write_text(UNSAFE_TYPEVAR_SOURCE, encoding="utf-8") + (tmp_path / "two.py").write_text( + textwrap.dedent("""\ + from typing import TypeVar + + U = TypeVar("U") + + __all__ = ["U"] + + + def identity(x: U) -> U: + return x + """), + encoding="utf-8", + ) + (tmp_path / "three.py").write_text( + textwrap.dedent("""\ + from typing import TypeVar + + A = TypeVar("A") + B = TypeVar("B") + + __all__ = ["A", "B"] + + + def pair(a: A, b: B) -> tuple[A, B]: + return a, b + """), + encoding="utf-8", + ) + + migration.main([str(tmp_path), "--py", "3.12", "--no-ruff"]) + + output = capsys.readouterr().out + assert_that(output.count("**Exported via __all__**"), equal_to(1)) + for line in [" `one.py`: `T`\n", " `three.py`: `A`, `B`\n", " `two.py`: `U`\n"]: + assert_that(output, contains_string(line)) + def test_name_converted_in_functions_but_still_used_by_a_generic_class_is_in_both_sections( self, tmp_path: Path, @@ -237,10 +268,10 @@ def get(self, x: T) -> T: migration.main([str(target), "--py", "3.12", "--no-ruff"]) output = capsys.readouterr().out - modified, needs_review = output.split("NEEDS MANUAL REVIEW (") + modified, needs_review = output.split("**NEEDS MANUAL REVIEW (") assert_that(output, contains_string("1 modified, 1 need manual review")) - assert_that(modified, contains_string(" T: converted to PEP 695 in functions\n")) - assert_that(needs_review, contains_string(" T: only used inside a class that is generic over it\n")) + assert_that(modified, contains_string(" `T`: converted to PEP 695 in functions\n")) + assert_that(needs_review, contains_string("**Only used inside a class that is generic over it**\n `mod.py`: `T`\n")) class TestConsoleReportModifiedSection: @@ -270,14 +301,14 @@ def g[U](y: U) -> U: migration.main([str(target), "--py", "3.12", "--no-ruff"]) output = capsys.readouterr().out - assert_that(output, contains_string(" T: converted to PEP 695 in functions\n")) - assert_that(output, not_(contains_string(" T: unused declaration removed"))) - assert_that(output, contains_string(" U: unused declaration removed\n")) + assert_that(output, contains_string(" `T`: converted to PEP 695 in functions\n")) + assert_that(output, not_(contains_string(" `T`: unused declaration removed"))) + assert_that(output, contains_string(" `U`: unused declaration removed\n")) def _table_rows(output: str, first_header: str) -> list[list[str]]: - """Return the data rows, as stripped cells, of the printed Markdown table whose header starts with first_header.""" - rows = [[cell.strip() for cell in line.strip("|").split("|")] for line in output.splitlines() if line.startswith("|")] + """Return the data rows, as cells without padding or backticks, of the printed Markdown table whose header starts with first_header.""" + rows = [[cell.strip().strip("`") for cell in line.strip("|").split("|")] for line in output.splitlines() if line.startswith("|")] start = next(index for index, row in enumerate(rows) if row[0] == first_header) table: list[list[str]] = [] for row in rows[start + 2 :]: @@ -411,7 +442,7 @@ def test_ignored_file_and_its_importer_are_left_untouched_and_the_report_lists_i output = capsys.readouterr().out assert_that((tmp_path / "shared.py").read_text(encoding="utf-8"), equal_to(shared_source)) assert_that((tmp_path / "user.py").read_text(encoding="utf-8"), equal_to(user_source)) - assert_that(output, contains_string("IGNORED FILES (1)\n shared.py\n")) + assert_that(output, contains_string("**IGNORED FILES (1)**\n `shared.py`\n")) assert_that(output, not_(contains_string("shared.py checked"))) assert_that( _table_rows(output, "Kind"), @@ -499,7 +530,7 @@ def test_consumer_is_localized_and_origin_declaration_survives(self, tmp_path: P consumer_text = consumer.read_text(encoding="utf-8") assert_that(consumer_text, contains_string("def use[T](x: T) -> T:")) assert_that(consumer_text, not_(contains_string(import_line))) - assert_that(capsys.readouterr().out, contains_string(" T: unused declaration removed\n")) + assert_that(capsys.readouterr().out, contains_string(" `T`: unused declaration removed\n")) 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")]) From 2631a90df3cf962e0017a3af1f94c31396841412 Mon Sep 17 00:00:00 2001 From: Francesco Pezzella Date: Tue, 20 Oct 2026 16:23:17 +0200 Subject: [PATCH 25/25] Add --second-pass to reprocess files that kept a declaration only because of imports later localized, merging both passes into one report. --- docs/developer/modules/recipes.md | 2 +- .../features/type-parameter-modernization.md | 7 +- src/rejuvenation/migration-type-recipes.py | 93 +++++++++++++++++-- .../test_migration_type_recipes.py | 31 +++++++ 4 files changed, 123 insertions(+), 10 deletions(-) diff --git a/docs/developer/modules/recipes.md b/docs/developer/modules/recipes.md index bc10fec6..e2fe8c5f 100644 --- a/docs/developer/modules/recipes.md +++ b/docs/developer/modules/recipes.md @@ -188,7 +188,7 @@ See *Verified by test modules* in [Type parameter modernization](../../user/feat report an error. - Imported names are collected once before the run, so removing a declaration whose importers were all localized - needs a second run. With `origin.py` as above and: + needs a second run, which `--second-pass` does for the files concerned. With `origin.py` as above and: ```python # main.py diff --git a/docs/user/features/type-parameter-modernization.md b/docs/user/features/type-parameter-modernization.md index 0c1b86c4..fe21dbc4 100644 --- a/docs/user/features/type-parameter-modernization.md +++ b/docs/user/features/type-parameter-modernization.md @@ -137,7 +137,8 @@ example under *Non-goals* or *Need to fix* in [Refactoring recipes](../../develo **Cosmetic or operational:** -- A declaration whose importers are all localized in a run is only removed by a second run. +- A declaration whose importers are all localized in a run is only removed by a second run, or by `--second-pass`. + A chain of three files, each importing from the previous one, can still need another run. - Removing a line can leave extra blank lines. Run your formatter afterwards. - With `--no-ruff`, the imports added by localizing are not merged into the file's existing `from` imports and not sorted. @@ -368,7 +369,7 @@ run the tool again. ```shell python src/rejuvenation/migration-type-recipes.py --py MAJOR.MINOR \ - [--report PATH] [--no-ruff] [--ignore FILE...] [--no-localization] + [--report PATH] [--no-ruff] [--ignore FILE...] [--no-localization] [--second-pass] ``` - ``: a `.py` file or a directory, scanned recursively (`.git`, `__pycache__`, `.venv` and `venv` skipped). @@ -378,6 +379,8 @@ python src/rejuvenation/migration-type-recipes.py --py MAJOR.MINOR \ - `--ignore`: files left untouched, whose type parameters are never copied into the files importing them (see *Shared declaration files*). - `--no-localization`: keep every project import of a type parameter (see *Disabling localization*). +- `--second-pass`: process again only the files that kept a declaration because another file imported it when the run + started, and merge the result into the same report. After the recipe, the CLI runs `ruff check --fix --select F401,I001` on the changed files. This removes **every** unused import in them, including ones that were unused before (such as `from typing import TypeVar` after the diff --git a/src/rejuvenation/migration-type-recipes.py b/src/rejuvenation/migration-type-recipes.py index 7fcec318..af9b457f 100644 --- a/src/rejuvenation/migration-type-recipes.py +++ b/src/rejuvenation/migration-type-recipes.py @@ -162,6 +162,66 @@ def process_file( ) +def _merge_reports(first: FileReport, second: FileReport) -> FileReport: + """Return the report of a file processed twice: second's names replace first's, and first's other names stay. + + A reason from first is dropped when second evaluated the same name again. A second that errored is returned as it is. + """ + if second.error is not None or first.result is None or second.result is None: + return second + first_reasons = first.reasons or {} + second_reasons = second.reasons or {} + return FileReport( + path=first.path, + result={phase: {**names, **second.result.get(phase, {})} for phase, names in first.result.items()}, + error=None, + reasons={ + phase: {name: reason for name, reason in reasons.items() if name not in second.result.get(phase, {})} + | second_reasons.get(phase, {}) + for phase, reasons in first_reasons.items() + }, + kinds={**(first.kinds or {}), **(second.kinds or {})}, + ignored=first.ignored | second.ignored, + ) + + +def _run_second_pass( + reports: list[FileReport], + files: Sequence[Path], + project_root: Path, + min_python: tuple[int, int], + localization: LocalizationSettings, +) -> list[FileReport]: + """Process again the files that kept a declaration because another file imported it, and merge the two reports of each. + + The imported names are collected again, so a declaration whose importers were all localized in the first pass is + no longer protected. Every other report is returned as it was. + """ + candidates = { + report.path + for report in reports + if any(UnsafeReason.IMPORTED_ELSEWHERE_IN_PROJECT in reasons.values() for reasons in (report.reasons or {}).values()) + } + if not candidates: + return reports + imported_names_by_file = collect_project_imported_names(files, project_root) + merged: list[FileReport] = [] + for report in reports: + if report.path not in candidates: + merged.append(report) + continue + second = process_file( + report.path, + min_python=min_python, + project_root=project_root, + project_wide_imported_names=imported_names_by_file.get(report.path, frozenset()), + localization=localization, + ) + merged.append(_merge_reports(report, second)) + print(f"File {report.path} checked again.") + return merged + + def _run_ruff_import_cleanup(paths: list[Path]) -> None: """Run `ruff check --fix --select F401,I001` over every path. @@ -457,9 +517,31 @@ def build_arg_parser() -> argparse.ArgumentParser: "the functions using them are not converted." ), ) + parser.add_argument( + "--second-pass", + action="store_true", + help=( + "Process again the files that kept a declaration only because another file imported it when the run started, " + "and report both passes together." + ), + ) return parser +def _localization_settings(parser: argparse.ArgumentParser, args: argparse.Namespace, project_root: Path) -> LocalizationSettings: + """Build the LocalizationSettings from --no-localization and --ignore, the latter resolved against project_root. + + Calls parser.error, which exits with code 2, if an --ignore file does not exist. + """ + for ignored in args.ignore: + if not (project_root / ignored).is_file(): + parser.error(f"--ignore: file does not exist: {ignored}") + return LocalizationSettings( + enabled=not args.no_localization, + ignored_files=frozenset((project_root / ignored).resolve() for ignored in args.ignore), + ) + + def main(argv: Sequence[str] | None = None) -> int: """Parse arguments, run TypeVarCheck across the target, print/save the report, return an exit code. @@ -481,13 +563,7 @@ def main(argv: Sequence[str] | None = None) -> int: project_root = target if target.is_dir() else target.parent imported_names_by_file = collect_project_imported_names(files, project_root) - for ignored in args.ignore: - if not (project_root / ignored).is_file(): - parser.error(f"--ignore: file does not exist: {ignored}") - localization = LocalizationSettings( - enabled=not args.no_localization, - ignored_files=frozenset((project_root / ignored).resolve() for ignored in args.ignore), - ) + localization = _localization_settings(parser, args, project_root) reports: list[FileReport] = [] for path in files: @@ -504,6 +580,9 @@ def main(argv: Sequence[str] | None = None) -> int: reports.append(report) print(f"File {path} checked.") + if args.second_pass: + reports = _run_second_pass(reports, files, project_root, args.py, localization) + 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: diff --git a/test/rejuvenation/test_migration_type_recipes.py b/test/rejuvenation/test_migration_type_recipes.py index f9b231c8..378e3ea7 100644 --- a/test/rejuvenation/test_migration_type_recipes.py +++ b/test/rejuvenation/test_migration_type_recipes.py @@ -450,6 +450,37 @@ def test_ignored_file_and_its_importer_are_left_untouched_and_the_report_lists_i ) +class TestSecondPass: + """main(): --second-pass removes a declaration whose importers were all localized, and reports it in the same report.""" + + @pytest.mark.parametrize( + ("extra_args", "declaration_kept", "expected_review"), + [ + pytest.param([], True, "NEEDS MANUAL REVIEW (1)", id="single-pass"), + pytest.param(["--second-pass"], False, "NEEDS MANUAL REVIEW (0)", id="second-pass"), + ], + ) + def test_declaration_left_behind_by_localizing_is_removed_in_the_same_run( + self, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], + extra_args: list[str], + *, + declaration_kept: bool, + expected_review: str, + ) -> None: + """Only the second pass removes the origin's declaration, and the one report counts both files once.""" + (tmp_path / "origin.py").write_text('from typing import TypeVar\n\nT = TypeVar("T")\n', encoding="utf-8") + (tmp_path / "user.py").write_text("from origin import T\n\n\ndef identity(x: T) -> T:\n return x\n", encoding="utf-8") + + migration.main([str(tmp_path), "--py", "3.12", "--no-ruff", *extra_args]) + + output = capsys.readouterr().out + assert_that('T = TypeVar("T")' in (tmp_path / "origin.py").read_text(encoding="utf-8"), equal_to(declaration_kept)) + assert_that(output, contains_string(expected_review)) + assert_that(output, contains_string("Processed 2 files")) + + class TestNoLocalization: """main(): --no-localization keeps every project import of a type parameter and reports the names left."""