From 5a68041a7991c139b9b0d43ceeef96e22a7d75f7 Mon Sep 17 00:00:00 2001 From: Francesco Pezzella Date: Fri, 16 Oct 2026 14:33:10 +0200 Subject: [PATCH 1/3] Port TypeVarCheck to libcst Analysis and rewriting now use libcst only, so uses in string annotations and names shadowed by a class's own type parameters are handled, multi-line strings and import comments are kept, and unparse_utils is removed. --- docs/developer/modules/recipes.md | 6 + src/renaissance/recipes/python_refactoring.py | 15 - src/renaissance/recipes/step_runner.py | 4 +- src/renaissance/recipes/type_var_check.py | 283 +++++++------ src/renaissance/recipes/type_var_domain.py | 372 ++++++++++-------- src/renaissance/utils/import_resolution.py | 84 ++-- src/renaissance/utils/unparse_utils.py | 124 ------ test/recipes/conftest.py | 33 +- test/recipes/test_python_refactoring.py | 25 -- test/recipes/test_type_var_check_convert.py | 49 ++- test/recipes/test_type_var_check_localize.py | 24 +- test/recipes/test_type_var_check_orphaned.py | 17 +- .../recipes/test_type_var_check_properties.py | 4 +- test/recipes/test_type_var_domain.py | 6 +- test/utils/test_unparse_utils.py | 113 ------ 15 files changed, 495 insertions(+), 664 deletions(-) delete mode 100644 src/renaissance/utils/unparse_utils.py delete mode 100644 test/utils/test_unparse_utils.py diff --git a/docs/developer/modules/recipes.md b/docs/developer/modules/recipes.md index fbcc6386..bfc7d738 100644 --- a/docs/developer/modules/recipes.md +++ b/docs/developer/modules/recipes.md @@ -25,6 +25,9 @@ safely cover. This page covers `TypeVarCheck`, the recipe behind - `src/renaissance/recipes/step_runner.py`: `Step`/`run_steps`, runs fix actions in order and commits only the ones that fixed something. - `src/renaissance/recipes/python_refactoring.py`: the base class. +- `src/renaissance/utils/import_resolution.py`: resolves `from X import Y` to a file in the project, re-anchors one + file's relative import on another file, and collects the names each project file imports from the others. +- `src/renaissance/recipes/python_refactoring.py`: the base class. - `src/renaissance/utils/import_resolution.py`: resolves `from X import Y` to a file in the project, re-anchors one file's relative import on another file, and collects the names each project file imports from the others. @@ -119,11 +122,14 @@ See *Verified by test modules* in [Type parameter modernization](../../user/feat - A new recipe is a `PythonRefactoring` subclass in its own `snake_case`-named module in `src/renaissance/recipes/`; no registration is needed. - A new type-parameter-declaring construct: extend `_is_type_param_call` and `type_param_text` together. +- A new type-parameter-declaring construct: extend `_is_type_param_call` and `type_param_text` together. - A keyword added by a later Python, or future PEP 695 syntax for variance or a bound on `**P`/`*Ts`: add it to `_CONVERTIBLE_KEYWORDS` and carry it over in `type_param_text`. + `_CONVERTIBLE_KEYWORDS` and carry it over in `type_param_text`. - A new version gate uses the syntax's own minimum Python from its PEP, not a value copied from another gate. - Re-exports through `__init__.py` or namespace packages (PEP 420): extend `resolve_project_module`. - `Step`/`run_steps` is generic and can be reused by any recipe. +- `Step`/`run_steps` is generic and can be reused by any recipe. ## Non-goals diff --git a/src/renaissance/recipes/python_refactoring.py b/src/renaissance/recipes/python_refactoring.py index 238d634d..8cee2b4a 100644 --- a/src/renaissance/recipes/python_refactoring.py +++ b/src/renaissance/recipes/python_refactoring.py @@ -6,7 +6,6 @@ """AI: Base processor for Python-specific source refactoring recipes.""" -import ast import importlib from collections.abc import Sequence from pathlib import Path @@ -121,19 +120,5 @@ def body(self) -> Sequence[NodeT]: return cast("Sequence[NodeT]", [wrappers[statement] for statement in root.translation_unit.atu.body]) return cast("Sequence[NodeT]", root.body) - def find_rst_node(self, target: ast.AST) -> PythonRstNode: - """Return the PythonRstNode wrapping the raw ast node `target`, e.g. to pass to self.replace().""" - # TODO(FrancescoPezzella): Drop once recipes can navigate wrapper nodes via the unified node protocol? - # decided during 24-09 discussion over future Node Protocol implementation - # https://github.com/TNO/Renaissance.Py/issues/269 - found: list[PythonRstNode] = [] - - def visit(node: PythonRstNode) -> None: - if node.node is target: - found.append(node) - - cast("PythonRstNode", cast("object", self.root)).process(visit) - return found[0] - def run(self) -> None: """Perform this recipe's refactoring; a no-op unless a subclass overrides it.""" diff --git a/src/renaissance/recipes/step_runner.py b/src/renaissance/recipes/step_runner.py index 12f2ec46..ce7a6d5a 100644 --- a/src/renaissance/recipes/step_runner.py +++ b/src/renaissance/recipes/step_runner.py @@ -9,7 +9,7 @@ from collections.abc import Callable, Sequence from dataclasses import dataclass -from renaissance.recipes.python_refactoring import PythonRefactoring +from renaissance.syntax_tree import ASTProcessor @dataclass(frozen=True) @@ -17,7 +17,7 @@ class Step: """One independently-runnable, independently-committable fix action.""" label: str - recipe: PythonRefactoring + recipe: ASTProcessor action: Callable[[], dict[str, str]] diff --git a/src/renaissance/recipes/type_var_check.py b/src/renaissance/recipes/type_var_check.py index 9cd1dcb9..4809d8ab 100644 --- a/src/renaissance/recipes/type_var_check.py +++ b/src/renaissance/recipes/type_var_check.py @@ -6,38 +6,42 @@ """Recipe that modernizes legacy TypeVar/ParamSpec/TypeVarTuple usage to PEP 695 syntax.""" -import ast +from collections.abc import Sequence from pathlib import Path from typing import cast -from renaissance.integrations.python.ast.rst_node import PythonRstNode +import libcst as cst + +from renaissance.integrations.python.ast.cst_node import PythonCstNode from renaissance.recipes.python_refactoring import PythonRefactoring from renaissance.recipes.step_runner import Step, run_steps from renaissance.recipes.type_var_domain import ( UnsafeReason, - all_refs_shadowed_by_pep695, - build_type_param, declaration_argument_names, declared_default, + declares_type_param, find_type_param_declarations, from_import_sources, - functions_in_generic_classes, - functions_using_nodes, + generic_classes, has_unconvertible_argument, + import_from_module, + imported_name, is_safe_to_localize, is_safe_to_remove, + module_statements, + outermost_function, type_param_constructor_name, - type_param_name, - type_params_defaults_last, + type_param_text, + type_params_bracket, + uses_of, ) from renaissance.utils.import_resolution import rebase_relative_module, resolve_project_module -from renaissance.utils.unparse_utils import unparse_signature_only PEP_695_MINIMUM = (3, 12) PEP_696_MINIMUM = (3, 13) -class TypeVarCheck(PythonRefactoring): +class TypeVarCheck(PythonRefactoring[PythonCstNode]): """Modernize legacy TypeVar/ParamSpec/TypeVarTuple usage in a Python file to PEP 695 syntax. See check() for the three phases this runs, in order. @@ -52,10 +56,18 @@ class TypeVarCheck(PythonRefactoring): # Root that absolute imports resolve from; None falls back to this file's own directory. project_root: Path | None = None + def __init__(self, file: Path) -> None: + """Prepare the recipe for file, parsed with the CST backend.""" + super().__init__(file, PythonCstNode) + def run(self) -> None: """Entry point called by PythonRefactoring.process(); stores check()'s result.""" self.result = self.check() + def _cst_root(self) -> PythonCstNode: + """Return the root node of the file as it currently is (after the last commit).""" + return cast("PythonCstNode", cast("object", self.root)) + def _target_supports_pep695(self) -> bool: """Return True only if min_python is known and is 3.12+, where PEP 695 syntax (`def f[T](...)`) exists.""" return self.min_python is not None and self.min_python >= PEP_695_MINIMUM @@ -64,7 +76,7 @@ def _target_supports_pep696(self) -> bool: """Return True only if min_python is known and is 3.13+, where PEP 696 defaults (`def f[T = int](...)`) exist.""" return self.min_python is not None and self.min_python >= PEP_696_MINIMUM - def _conversion_refusal(self, decl_stmt: ast.Assign) -> UnsafeReason | None: + 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 @@ -92,36 +104,36 @@ 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. Names whose references are all shadowed by a same-named PEP 695 - type parameter are skipped, and methods of a class generic over the name are never converted. - Bounds, constraints and `default=` are carried over, with defaulted type parameters placed last. - A name only such methods use, or whose declaration _conversion_refusal rejects, is reported - "unsafe", with its UnsafeReason on self.converted_unsafe_reasons. Returns {name: "fixed" | "unsafe"}. + The declaration is left in place. A use is attributed to the outermost function containing it; a `T` + that a same-named PEP 695 type parameter of a function or class declares is not a use. Methods of a + class generic over the name are never converted. Bounds, constraints and `default=` are carried over + in their original formatting, with defaulted type parameters placed last. A name only such methods + use, or whose declaration _conversion_refusal rejects, is reported "unsafe", with its UnsafeReason on + self.converted_unsafe_reasons. Returns {name: "fixed" | "unsafe"}. """ - root = cast("PythonRstNode", cast("object", self.root)) - tree = cast("ast.Module", root.node) - declarations = find_type_param_declarations(tree) - usage = functions_using_nodes(tree, declarations.keys()) + root = self._cst_root() + module = root.translation_unit.atu results: dict[str, str] = {} self.converted_unsafe_reasons: dict[str, UnsafeReason] = {} # One self.replace() per function: a function using two converted names would otherwise get two # rewrites of the same node, which the rewriter rejects as conflicting. - touched_functions: dict[int, ast.FunctionDef | ast.AsyncFunctionDef] = {} - for name, functions in usage.items(): - decl_stmt = declarations[name] - if all_refs_shadowed_by_pep695(tree, name, decl_stmt): + new_params: dict[int, tuple[PythonCstNode, list[tuple[str, bool]]]] = {} + for name, decl_stmt in find_type_param_declarations(module).items(): + uses = uses_of(root, name, decl_stmt) + functions = list(dict.fromkeys(function for use in uses if (function := outermost_function(use)) is not None)) + if not functions: continue - generic_class_method_ids = {id(method) for method in functions_in_generic_classes(tree, name)} + classes = generic_classes(uses) + in_generic_class = [function for function in functions if self._is_inside(function, classes)] candidates = [ function for function in functions - if id(function) not in generic_class_method_ids - and not any(type_param_name(existing) == name for existing in function.type_params) + if function not in in_generic_class and not declares_type_param(cast("cst.FunctionDef", function.node), name) ] if not candidates: - if any(id(function) in generic_class_method_ids for function in functions): + if in_generic_class: self._mark_unsafe(results, self.converted_unsafe_reasons, name, UnsafeReason.USED_IN_GENERIC_CLASS) continue reason = self._conversion_refusal(decl_stmt) @@ -129,40 +141,66 @@ def convert_declared_typevars(self) -> dict[str, str]: self._mark_unsafe(results, self.converted_unsafe_reasons, name, reason) continue - type_param = build_type_param(decl_stmt) + param = (type_param_text(decl_stmt, module), declared_default(decl_stmt) is not None) for function in candidates: - function.type_params = [*function.type_params, type_param] - touched_functions[id(function)] = function + new_params.setdefault(id(function), (function, []))[1].append(param) results[name] = "fixed" - for function in touched_functions.values(): - function.type_params = type_params_defaults_last(function.type_params) - rst_node = self.find_rst_node(function) - self.replace(unparse_signature_only(function, rst_node.text), rst_node, include_whitespace=False, include_comments=False) + for function, params in new_params.values(): + self._add_type_params(function, params) return results + @staticmethod + def _is_inside(node: PythonCstNode, containers: Sequence[PythonCstNode]) -> bool: + """Return True if one of containers encloses node.""" + current = node.parent + while current is not None: + if any(current is container for container in containers): + return True + current = current.parent + return False + + def _add_type_params(self, function: PythonCstNode, params: list[tuple[str, bool]]) -> None: + """Add params, given as (text, has_default), to function's PEP 695 type parameters, changing nothing else. + + A function without a bracket gets one after its name; an existing bracket is rewritten with its own + type parameters followed by params, the defaulted ones last. + """ + node = cast("cst.FunctionDef", function.node) + unit = function.translation_unit + existing = node.type_parameters + if existing is None: + self.replace( + node.name.value + type_params_bracket(params), unit.wrappers[node.name], include_whitespace=False, include_comments=False + ) + return + current = [ + (unit.atu.code_for_node(param.with_changes(comma=cst.MaybeSentinel.DEFAULT)), param.default is not None) + for param in existing.params + ] + self.replace(type_params_bracket([*current, *params]), unit.wrappers[existing], include_whitespace=False, include_comments=False) + def remove_orphaned_declarations(self) -> dict[str, str]: """Remove every module-level TypeVar/ParamSpec/TypeVarTuple declaration that nothing uses anymore. - A declaration is orphaned when no reference to its name remains outside a same-named PEP 695 type - parameter (see all_refs_shadowed_by_pep695). One that is_safe_to_remove rejects is kept and - reported "unsafe", with its UnsafeReason on self.orphaned_unsafe_reasons. Returns - {name: "fixed" | "unsafe"}. + A declaration is orphaned when uses_of finds no use of its name: every `T` left belongs to a PEP 695 + type parameter, or there is none. One that is_safe_to_remove rejects is kept and reported "unsafe", + with its UnsafeReason on self.orphaned_unsafe_reasons. Returns {name: "fixed" | "unsafe"}. """ - root = cast("PythonRstNode", cast("object", self.root)) - tree = cast("ast.Module", root.node) + root = self._cst_root() + module = root.translation_unit.atu results: dict[str, str] = {} self.orphaned_unsafe_reasons: dict[str, UnsafeReason] = {} - for name, decl_stmt in find_type_param_declarations(tree).items(): - if not all_refs_shadowed_by_pep695(tree, name, decl_stmt): + for name, decl_stmt in find_type_param_declarations(module).items(): + if uses_of(root, name, decl_stmt): continue # TODO(FrancescoPezzella): a public TypeVar no file in the project uses (e.g. redis.typing.AnyKeyT) is removed, # though other projects may import it. # https://github.com/TNO/Renaissance.Py/issues/279 - reason = is_safe_to_remove(tree, name, self.project_wide_imported_names) + 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 @@ -177,14 +215,11 @@ def _mark_unsafe(self, results: dict[str, str], reasons: dict[str, UnsafeReason] results[name] = "unsafe" reasons[name] = reason - def _remove_declaration(self, decl_stmt: ast.Assign) -> None: + def _remove_declaration(self, decl_stmt: cst.Assign) -> None: """Remove decl_stmt's statement from the file.""" - for stmt_node in self.body: - if stmt_node.node is decl_stmt: - # TODO(FrancescoPezzella): enable once comment blocks get correctly deleted - # https://github.com/TNO/Renaissance.Py/issues/189 - self.remove(stmt_node, include_comments=False) - break + # 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) def localize_imported_typevars(self) -> dict[str, str]: """Replace imports of TypeVar/ParamSpec/TypeVarTuple names from the target project with local declarations. @@ -196,75 +231,91 @@ def localize_imported_typevars(self) -> dict[str, str]: """ results: dict[str, str] = {} self.cross_file_unsafe_reasons: dict[str, UnsafeReason] = {} - importing_tree = cast("ast.Module", cast("PythonRstNode", cast("object", self.root)).node) + module = self._cst_root().translation_unit.atu project_root = self.project_root if self.project_root is not None else Path(self.filename).parent - for import_node in self.body: - raw = import_node.node - if not isinstance(raw, ast.ImportFrom): + for line in module.body: + if not isinstance(line, cst.SimpleStatementLine): continue - - origin_path = resolve_project_module(Path(self.filename), project_root, raw.module, raw.level) - if origin_path is None: - continue - - self._localize_names_from(import_node, raw, importing_tree, origin_path, results) + for statement in line.body: + if not isinstance(statement, cst.ImportFrom): + continue + origin_path = resolve_project_module( + Path(self.filename), project_root, import_from_module(statement), len(statement.relative) + ) + if origin_path is None: + continue + self._localize_names_from(line, statement, origin_path, results) return results def _localize_names_from( self, - import_node: PythonRstNode, - raw: ast.ImportFrom, - importing_tree: ast.Module, + line: cst.SimpleStatementLine, + raw: cst.ImportFrom, origin_path: Path, results: dict[str, str], ) -> None: - """Localize every safe type parameter name that raw imports from the origin_path file, in one edit of import_node. + """Localize every safe type parameter name that raw, on line, imports from the origin_path file. Records each name in results as "fixed" or, via _mark_unsafe, as "unsafe". + + Raises: + SyntaxError: if the origin file can't be parsed, with the origin's path as its filename. + """ - origin_tree = ast.parse(origin_path.read_text(encoding="utf-8"), str(origin_path)) - declarations = find_type_param_declarations(origin_tree) - localized: dict[str, ast.Assign] = {} + if isinstance(raw.names, cst.ImportStar): + return + try: + origin = cst.parse_module(origin_path.read_text(encoding="utf-8")) + except cst.ParserSyntaxError as error: + raise SyntaxError(error.message, (str(origin_path), error.raw_line, error.raw_column + 1, None)) from error + declarations = find_type_param_declarations(origin) + localized: dict[str, cst.Assign] = {} needed_imports: list[str] = [] needed_argument_names: set[str] = set() for alias in raw.names: # TODO(FrancescoPezzella): `from x import T as U` is not currently being caught, tbd # https://github.com/TNO/Renaissance.Py/issues/279 - if alias.asname is not None or alias.name not in declarations: + name = alias.evaluated_name + if alias.asname is not None or name not in declarations: continue - decl_stmt = declarations[alias.name] - reason = is_safe_to_localize(origin_tree, alias.name) + decl_stmt = declarations[name] + reason = is_safe_to_localize(origin, name) if reason is not None: - self._mark_unsafe(results, self.cross_file_unsafe_reasons, alias.name, reason) + self._mark_unsafe(results, self.cross_file_unsafe_reasons, name, reason) continue - argument_names = self._argument_names_to_import(importing_tree, origin_tree, raw, decl_stmt) + argument_names = self._argument_names_to_import(origin, raw, decl_stmt) if argument_names is None: - self._mark_unsafe(results, self.cross_file_unsafe_reasons, alias.name, UnsafeReason.DECLARATION_NAME_UNAVAILABLE) + self._mark_unsafe(results, self.cross_file_unsafe_reasons, name, UnsafeReason.DECLARATION_NAME_UNAVAILABLE) continue - constructor_import = self._missing_constructor_import(origin_tree, origin_path, raw, decl_stmt) + constructor_import = self._missing_constructor_import(origin, origin_path, raw, decl_stmt) if constructor_import is not None and constructor_import not in needed_imports: needed_imports.append(constructor_import) needed_argument_names |= argument_names - localized[alias.name] = decl_stmt - results[alias.name] = "fixed" + localized[name] = decl_stmt + results[name] = "fixed" if not localized: return if needed_argument_names: - needed_imports.append(f"from {'.' * raw.level}{raw.module or ''} import {', '.join(sorted(needed_argument_names))}") - self._localize_import(import_node, raw, localized, needed_imports) + needed_imports.append(f"from {self._import_from_text(raw)} import {', '.join(sorted(needed_argument_names))}") + self._localize_import(line, raw, origin, localized, needed_imports) + + @staticmethod + def _import_from_text(raw: cst.ImportFrom) -> str: + """Return what follows `from` in raw: its leading dots and dotted module, e.g. "..pkg.mod".""" + return "." * len(raw.relative) + (import_from_module(raw) or "") def _missing_constructor_import( self, - origin_tree: ast.Module, + origin: cst.Module, origin_path: Path, - via: ast.ImportFrom, - decl_stmt: ast.Assign, + via: cst.ImportFrom, + decl_stmt: cst.Assign, ) -> str | None: """Return the "from module import Ctor" line the localized declaration needs, or None if this file has it. @@ -273,7 +324,7 @@ def _missing_constructor_import( constructor is imported from the origin module itself. """ ctor_name = type_param_constructor_name(decl_stmt) - ctor_source = from_import_sources(origin_tree).get(ctor_name) + 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. @@ -281,26 +332,25 @@ def _missing_constructor_import( # https://github.com/TNO/Renaissance.Py/issues/279 return None - via_module = "." * via.level + (via.module or "") - ctor_module = rebase_relative_module(via.module, via.level, origin_path, ctor_source[0] or None, ctor_source[1]) or via_module + via_module = import_from_module(via) + ctor_module = rebase_relative_module(via_module, len(via.relative), origin_path, ctor_source[0] or None, ctor_source[1]) + ctor_module = ctor_module or self._import_from_text(via) # TODO(FrancescoPezzella): the constructor already imported here from another module (e.g. `typing` vs the origin's # `typing_extensions`) isn't matched, so a second, shadowing import of the same name is added. # https://github.com/TNO/Renaissance.Py/issues/279 - for import_node in self.body: - raw = import_node.node + for statement in module_statements(self._cst_root().translation_unit.atu): if ( - isinstance(raw, ast.ImportFrom) - and "." * raw.level + (raw.module or "") == ctor_module - and any((alias.asname or alias.name) == ctor_name for alias in raw.names) + isinstance(statement, cst.ImportFrom) + and not isinstance(statement.names, cst.ImportStar) + and self._import_from_text(statement) == ctor_module + and any(imported_name(alias) == ctor_name for alias in statement.names) ): return None return f"from {ctor_module} import {ctor_name}" - def _argument_names_to_import( - self, importing_tree: ast.Module, origin_tree: ast.Module, raw: ast.ImportFrom, decl_stmt: ast.Assign - ) -> set[str] | None: + def _argument_names_to_import(self, origin: cst.Module, raw: cst.ImportFrom, decl_stmt: cst.Assign) -> set[str] | None: """Return the names decl_stmt's arguments use that this file must import from the origin module. A name this file already imports from the origin module (or from the same absolute module the @@ -308,8 +358,8 @@ def _argument_names_to_import( imported from the origin as the same object: the origin doesn't bind it at module level at runtime (e.g. only under `if TYPE_CHECKING:`), or this file binds it to something else. """ - origin_sources = from_import_sources(origin_tree) - importing_sources = from_import_sources(importing_tree) + origin_sources = from_import_sources(origin) + importing_sources = from_import_sources(self._cst_root().translation_unit.atu) missing: set[str] = set() for name in declaration_argument_names(decl_stmt): if name not in origin_sources: @@ -318,7 +368,7 @@ def _argument_names_to_import( missing.add(name) continue origin_source = origin_sources[name] - accepted = {(raw.module or "", raw.level)} + accepted = {(import_from_module(raw) or "", len(raw.relative))} if origin_source is not None and origin_source[1] == 0: accepted.add(origin_source) if importing_sources[name] not in accepted: @@ -327,25 +377,32 @@ def _argument_names_to_import( def _localize_import( self, - import_node: PythonRstNode, - raw: ast.ImportFrom, - declarations: dict[str, ast.Assign], + line: cst.SimpleStatementLine, + raw: cst.ImportFrom, + origin: cst.Module, + declarations: dict[str, cst.Assign], needed_imports: list[str], ) -> None: - """Replace import_node with the text of declarations' statements as local declarations, in one edit. + """Replace the import raw, on line, with local copies of declarations from the origin module. - Narrows or removes the original import for declarations' names, and prepends needed_imports: the - imports the declarations need (their constructors, names their arguments use) that this file - doesn't have yet. + 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. """ - decl_text = "\n".join([*needed_imports, *(ast.unparse(decl_stmt) for decl_stmt in declarations.values())]) + no_semicolon = cst.MaybeSentinel.DEFAULT + decl_text = "\n".join( + [*needed_imports, *(origin.code_for_node(decl.with_changes(semicolon=no_semicolon)) for decl in declarations.values())] + ) + wrappers = self._cst_root().translation_unit.wrappers - # TODO(FrancescoPezzella): the narrowed import is rebuilt with ast.unparse, so comments inside it are lost and a trailing - # comment ends up on the last declaration's line. - # https://github.com/TNO/Renaissance.Py/issues/279 - kept: list[ast.alias] = [alias for alias in raw.names if (alias.asname or alias.name) not in declarations] - if kept: - narrowed = ast.unparse(ast.ImportFrom(module=raw.module, names=kept, level=raw.level)) - self.replace(f"{narrowed}\n{decl_text}", import_node, include_whitespace=False, include_comments=False) - else: - self.replace(decl_text, import_node, include_whitespace=False, include_comments=False) + kept = [alias for alias in cast("Sequence[cst.ImportAlias]", raw.names) if imported_name(alias) not in declarations] + if not kept: + self.replace(decl_text, wrappers[raw], include_whitespace=False, include_comments=False) + return + if raw.rpar is None: + # Without parentheses, a trailing comma after the last name is a syntax error. + kept[-1] = kept[-1].with_changes(comma=cst.MaybeSentinel.DEFAULT) + narrowed = self._cst_root().translation_unit.atu.code_for_node(raw.with_changes(names=kept, semicolon=no_semicolon)) + self.replace(narrowed, wrappers[raw], include_whitespace=False, include_comments=False) + self.insert_after(f"\n{decl_text}", wrappers[line], include_whitespace=False, include_comments=True) diff --git a/src/renaissance/recipes/type_var_domain.py b/src/renaissance/recipes/type_var_domain.py index 1b005ccb..5fb1a66d 100644 --- a/src/renaissance/recipes/type_var_domain.py +++ b/src/renaissance/recipes/type_var_domain.py @@ -6,13 +6,18 @@ """TypeVar/ParamSpec/TypeVarTuple domain model and safety analysis.""" -import ast import builtins -from collections.abc import Iterable +from collections.abc import Iterable, Iterator from dataclasses import dataclass from enum import StrEnum from typing import cast +import libcst as cst +from libcst import matchers +from libcst.helpers import get_full_name_for_node + +from renaissance.integrations.python.ast.cst_node import PythonCstNode + DOCS_BASE_URL = "https://tno.github.io/Renaissance.Py/user/features/type-parameter-modernization/" @@ -81,173 +86,192 @@ def doc_link(reason: UnsafeReason) -> str: return f"{DOCS_BASE_URL}#{UNSAFE_RULES[reason].doc_anchor}" -def _is_type_param_call(value: ast.expr) -> bool: +_CONSTRUCTORS = frozenset({"TypeVar", "ParamSpec", "TypeVarTuple"}) + + +def _is_type_param_call(value: cst.BaseExpression) -> bool: """Return True if `value` is a call to TypeVar/ParamSpec/TypeVarTuple.""" - return isinstance(value, ast.Call) and isinstance(value.func, ast.Name) and value.func.id in ("TypeVar", "ParamSpec", "TypeVarTuple") + return isinstance(value, cst.Call) and isinstance(value.func, cst.Name) and value.func.value in _CONSTRUCTORS + + +def module_statements(module: cst.Module) -> Iterator[cst.BaseSmallStatement | cst.BaseCompoundStatement]: + """Yield the module's top-level statements in order, one per small statement of a line like `a = 1; b = 2`.""" + for statement in module.body: + if isinstance(statement, cst.SimpleStatementLine): + yield from statement.body + else: + yield statement -def find_type_param_declarations(tree: ast.Module) -> dict[str, ast.Assign]: +def find_type_param_declarations(module: cst.Module) -> dict[str, cst.Assign]: """Find every module-level "NAME = TypeVar/ParamSpec/TypeVarTuple(...)" declaration.""" - declarations: dict[str, ast.Assign] = {} - for stmt in tree.body: - if isinstance(stmt, ast.Assign) and _is_type_param_call(stmt.value): - for target in stmt.targets: - if isinstance(target, ast.Name): - declarations[target.id] = stmt + declarations: dict[str, cst.Assign] = {} + for statement in module_statements(module): + if isinstance(statement, cst.Assign) and _is_type_param_call(statement.value): + for target in statement.targets: + if isinstance(target.target, cst.Name): + declarations[target.target.value] = statement return declarations -def type_param_name(param: ast.type_param) -> str: - """Return a PEP 695 type parameter's name, narrowing to the subclasses whose stubs declare `.name`.""" - return cast("ast.TypeVar | ast.ParamSpec | ast.TypeVarTuple", param).name +def _call(decl_stmt: cst.Assign) -> cst.Call: + """Return the TypeVar/ParamSpec/TypeVarTuple call a declaration assigns.""" + return cast("cst.Call", decl_stmt.value) -def type_param_constructor_name(decl_stmt: ast.Assign) -> str: +def _positional_arguments(call: cst.Call) -> list[cst.Arg]: + """Return the call's positional arguments, `*args` included.""" + return [argument for argument in call.args if argument.keyword is None and argument.star != "**"] + + +def _keyword_value(call: cst.Call, keyword: str) -> cst.BaseExpression | None: + """Return the value the call passes as `keyword=`, or None if it passes none.""" + return next((argument.value for argument in call.args if argument.keyword is not None and argument.keyword.value == keyword), None) + + +def type_param_constructor_name(decl_stmt: cst.Assign) -> str: """Return the name of the call a declaration uses, e.g. "TypeVar" for `T = TypeVar("T")`.""" - call = cast("ast.Call", decl_stmt.value) - return cast("ast.Name", call.func).id + return cast("cst.Name", _call(decl_stmt).func).value + + +def _string_value(expression: cst.BaseExpression) -> str | None: + """Return the value of a plain or concatenated string literal, or None for any other expression.""" + if isinstance(expression, cst.SimpleString | cst.ConcatenatedString): + value = expression.evaluated_value + return value if isinstance(value, str) else None + return None -def _find_dunder_all(tree: ast.Module) -> set[str] | None: +def _find_dunder_all(module: cst.Module) -> set[str] | None: """Return the names in this module's plain `__all__ = [...]` assignment, or None if it has none. Only a list, tuple or set literal is read; annotated, `+=`, `.extend()` and `.append()` forms are ignored. """ - for stmt in tree.body: + for statement in module_statements(module): if ( - isinstance(stmt, ast.Assign) - and any(isinstance(t, ast.Name) and t.id == "__all__" for t in stmt.targets) - and isinstance(stmt.value, ast.List | ast.Tuple | ast.Set) + isinstance(statement, cst.Assign) + and any(isinstance(target.target, cst.Name) and target.target.value == "__all__" for target in statement.targets) + and isinstance(statement.value, cst.List | cst.Tuple | cst.Set) ): - return {elt.value for elt in stmt.value.elts if isinstance(elt, ast.Constant) and isinstance(elt.value, str)} + values = (_string_value(element.value) for element in statement.value.elements if isinstance(element, cst.Element)) + return {value for value in values if value is not None} return None -def _declares_type_param(class_node: ast.ClassDef, name: str) -> bool: - """Return True if the class declares `name` as its own PEP 695 type parameter (`class Box[T]:`).""" - return any(type_param_name(param) == name for param in class_node.type_params) +class _NameCollector(cst.CSTVisitor): + """Collect the names an expression reads: every `Name` except an attribute's or a keyword argument's own name.""" + def __init__(self) -> None: + """Start with no names collected.""" + super().__init__() + self.names: set[str] = set() -def _inherits_type_param(class_node: ast.ClassDef, name: str) -> bool: - """Return True if the class is generic over the module-level `name` through one of its bases. + def visit_Name(self, node: cst.Name) -> None: # noqa: N802 - libcst dispatches on this exact name + """Record the name.""" + self.names.add(node.value) - That is, a base mentions `name` (`Generic[T]`, `Protocol[T]`, `Mapping[T]`, `Base[int, T]`) and the - class doesn't declare its own `name`. - """ - return not _declares_type_param(class_node, name) and any( - isinstance(child, ast.Name) and child.id == name for base in class_node.bases for child in ast.walk(base) - ) + def visit_Attribute(self, node: cst.Attribute) -> bool: # noqa: N802 - libcst dispatches on this exact name + """Visit only the attribute's object: in `np.integer`, `integer` is not a name read here.""" + node.value.visit(self) + return False + + def visit_Arg(self, node: cst.Arg) -> bool: # noqa: N802 - libcst dispatches on this exact name + """Visit only the argument's value, not the keyword it is passed as.""" + node.value.visit(self) + return False -def declaration_argument_names(decl_stmt: ast.Assign) -> set[str]: +def declaration_argument_names(decl_stmt: cst.Assign) -> set[str]: """Return the non-builtin names a declaration's arguments use, e.g. {"Shape"} for `TypeVar("T", bound=Shape)`. The first positional argument (the type parameter's own name) is skipped; a string argument is read as a forward reference (`bound="Shape"`). A string that isn't a valid expression contributes no names. """ - call = cast("ast.Call", decl_stmt.value) - names: set[str] = set() - for argument in [*call.args[1:], *(keyword.value for keyword in call.keywords)]: - expression = argument - if isinstance(argument, ast.Constant) and isinstance(argument.value, str): + call = _call(decl_stmt) + keyword_arguments = [argument for argument in call.args if argument.keyword is not None or argument.star == "**"] + collector = _NameCollector() + for argument in [*_positional_arguments(call)[1:], *keyword_arguments]: + expression = argument.value + text = _string_value(expression) + if text is not None: try: - expression = ast.parse(argument.value, mode="eval").body - except SyntaxError: + expression = cst.parse_expression(text) + except cst.ParserSyntaxError: continue - names.update(node.id for node in ast.walk(expression) if isinstance(node, ast.Name)) - return names - set(dir(builtins)) + expression.visit(collector) + return collector.names - set(dir(builtins)) + +def import_from_module(statement: cst.ImportFrom) -> str | None: + """Return the dotted module an import-from names after its leading dots, or None for a bare `from . import x`.""" + return get_full_name_for_node(statement.module) if statement.module is not None else None -def from_import_sources(tree: ast.Module) -> dict[str, tuple[str, int] | None]: + +def imported_name(alias: cst.ImportAlias) -> str: + """Return the name an import alias binds: its `as` name if it has one, else the imported name.""" + return alias.evaluated_alias or alias.evaluated_name + + +def from_import_sources(module: cst.Module) -> dict[str, tuple[str, int] | None]: """Map every name bound at module level to its `from` import's (module, level), or None if bound otherwise. Names bound by a class, function, assignment or plain `import` map to None. """ sources: dict[str, tuple[str, int] | None] = {} - for stmt in tree.body: - if isinstance(stmt, ast.ImportFrom): - for alias in stmt.names: - sources[alias.asname or alias.name] = (stmt.module or "", stmt.level) - elif isinstance(stmt, ast.Import): - for alias in stmt.names: - sources[(alias.asname or alias.name).split(".")[0]] = None - elif isinstance(stmt, ast.ClassDef | ast.FunctionDef | ast.AsyncFunctionDef): - sources[stmt.name] = None - elif isinstance(stmt, ast.Assign | ast.AnnAssign): - sources.update(dict.fromkeys(_assigned_names(stmt))) + for statement in module_statements(module): + if isinstance(statement, cst.ImportFrom): + source = (import_from_module(statement) or "", len(statement.relative)) + if isinstance(statement.names, cst.ImportStar): + sources["*"] = source + else: + for alias in statement.names: + sources[imported_name(alias)] = source + elif isinstance(statement, cst.Import): + for alias in statement.names: + sources[imported_name(alias).split(".")[0]] = None + elif isinstance(statement, cst.ClassDef | cst.FunctionDef): + sources[statement.name.value] = None + elif isinstance(statement, cst.Assign | cst.AnnAssign): + sources.update(dict.fromkeys(_assigned_names(statement))) return sources -def _assigned_names(stmt: ast.Assign | ast.AnnAssign) -> list[str]: - """Return every name an assignment binds, including inside tuple or list targets.""" - targets = stmt.targets if isinstance(stmt, ast.Assign) else [stmt.target] - return [node.id for target in targets for node in ast.walk(target) if isinstance(node, ast.Name)] +def _assigned_names(statement: cst.Assign | cst.AnnAssign) -> set[str]: + """Return every name an assignment's targets use, including inside tuple or list targets.""" + targets = [target.target for target in statement.targets] if isinstance(statement, cst.Assign) else [statement.target] + collector = _NameCollector() + for target in targets: + target.visit(collector) + return collector.names -def _imports_name_conditionally(tree: ast.Module, name: str) -> bool: +def _imports_name_conditionally(module: cst.Module, name: str) -> bool: """Return True if an import inside a module-level `if`/`try` block binds `name`.""" - for stmt in tree.body: - if not isinstance(stmt, ast.If | ast.Try | ast.TryStar): + for statement in module.body: + if not isinstance(statement, cst.If | cst.Try | cst.TryStar): continue - for node in ast.walk(stmt): - if isinstance(node, ast.Import | ast.ImportFrom) and any( - (alias.asname or alias.name.split(".")[0]) == name for alias in node.names - ): + for node in matchers.findall(statement, matchers.Import() | matchers.ImportFrom()): + names = cast("cst.Import | cst.ImportFrom", node).names + if not isinstance(names, cst.ImportStar) and any(imported_name(alias).split(".")[0] == name for alias in names): return True return False -def is_safe_to_localize(origin_tree: ast.Module, name: str) -> UnsafeReason | None: +def is_safe_to_localize(origin: cst.Module, name: str) -> UnsafeReason | None: """Return None if `name` is safe to duplicate as a local declaration, else the reason it isn't. Its constructor (TypeVar/ParamSpec/TypeVarTuple) must not be imported inside a module-level `if`/`try` block at the origin (ORIGIN_IMPORTS_CONSTRUCTOR_CONDITIONALLY), since which implementation it binds then depends on the runtime, e.g. `typing_extensions` below 3.13. """ - declaration = find_type_param_declarations(origin_tree).get(name) - if declaration is not None and _imports_name_conditionally(origin_tree, type_param_constructor_name(declaration)): + declaration = find_type_param_declarations(origin).get(name) + if declaration is not None and _imports_name_conditionally(origin, type_param_constructor_name(declaration)): return UnsafeReason.ORIGIN_IMPORTS_CONSTRUCTOR_CONDITIONALLY return None -def functions_using_nodes(tree: ast.Module, names: Iterable[str]) -> dict[str, list[ast.FunctionDef | ast.AsyncFunctionDef]]: - """Map each of `names`, in the given order, to the outermost function/method node whose signature or body references it. - - A name used in a nested function (a closure) is attributed to the outermost one, never the nested - one: a PEP 695 type parameter on the enclosing function is already visible inside its closures. - """ - usage: dict[str, list[ast.FunctionDef | ast.AsyncFunctionDef]] = {name: [] for name in names} - - def visit(node: ast.AST, enclosing: ast.FunctionDef | ast.AsyncFunctionDef | None) -> None: - current = enclosing - if isinstance(node, ast.FunctionDef | ast.AsyncFunctionDef) and enclosing is None: - current = node - if isinstance(node, ast.Name) and current is not None and node.id in usage and current not in usage[node.id]: - usage[node.id].append(current) - for child in ast.iter_child_nodes(node): - visit(child, current) - - visit(tree, None) - return usage - - -def functions_in_generic_classes(tree: ast.Module, name: str) -> list[ast.FunctionDef | ast.AsyncFunctionDef]: - """Return every function or method defined inside a class that is generic over `name`. - - A class is generic over `name` when it declares it as a PEP 695 type parameter (`class Box[T]:`) - or inherits it through one of its bases (see _inherits_type_param). - """ - return [ - function - for node in ast.walk(tree) - if isinstance(node, ast.ClassDef) and (_declares_type_param(node, name) or _inherits_type_param(node, name)) - for function in ast.walk(node) - if isinstance(function, ast.FunctionDef | ast.AsyncFunctionDef) - ] - - def is_safe_to_remove( - tree: ast.Module, + module: cst.Module, name: str, project_wide_imported_names: frozenset[str] = frozenset(), ) -> UnsafeReason | None: @@ -257,7 +281,7 @@ def is_safe_to_remove( or IMPORTED_ELSEWHERE_IN_PROJECT if `name` is in `project_wide_imported_names` (another file in the target project imports it directly, regardless of `__all__`). """ - dunder_all = _find_dunder_all(tree) + dunder_all = _find_dunder_all(module) if dunder_all is not None and name in dunder_all: return UnsafeReason.DECLARED_TYPEVAR_EXPORTED if name in project_wide_imported_names: @@ -265,31 +289,48 @@ def is_safe_to_remove( return None -def all_refs_shadowed_by_pep695(tree: ast.Module, name: str, decl_stmt: ast.Assign) -> bool: - """Return True if every remaining reference to `name` is shadowed by a PEP 695 type parameter. +def uses_of(root: PythonCstNode, name: str, decl_stmt: cst.Assign) -> list[PythonCstNode]: + """Return the nodes that use the module-level `name` that decl_stmt declares, resolved with libcst's scope analysis. + + A use inside a string annotation or `cast("T", x)` counts; a `T` that a PEP 695 type parameter of an + enclosing function or class (`def f[T]`, `class C[T]`) declares does not. + """ + target = next(target.target for target in decl_stmt.targets if isinstance(target.target, cst.Name) and target.target.value == name) + return [reference.node for reference in root.translation_unit.wrappers[target].referenced_by] + + +def outermost_function(node: PythonCstNode) -> PythonCstNode | None: + """Return the outermost function or method enclosing node, or None if node is outside every function.""" + outermost = None + current = node.parent + while current is not None: + if isinstance(current.node, cst.FunctionDef): + outermost = current + current = current.parent + return outermost + + +def generic_classes(uses: Iterable[PythonCstNode]) -> list[PythonCstNode]: + """Return the classes that have one of `uses` in a base (`Generic[T]`, `Protocol[T]`, `Base[int, T]`). - E.g. `def b[T](x: T) -> T:`, where `T` resolves to the function's own parameter rather than - the module-level declaration, making it dead. Also true (vacuously) if `name` isn't - referenced anywhere at all. + Such a class is generic over the used name. """ - found_live_use = False - - def visit(node: ast.AST, *, shadowed: bool) -> None: - nonlocal found_live_use - if node is decl_stmt or found_live_use: - return - if isinstance(node, ast.Name) and node.id == name: - if not shadowed: - found_live_use = True - return - current = shadowed - if isinstance(node, ast.FunctionDef | ast.AsyncFunctionDef): - current = shadowed or any(type_param_name(param) == name for param in node.type_params) - for child in ast.iter_child_nodes(node): - visit(child, shadowed=current) - - visit(tree, shadowed=False) - return not found_live_use + classes: list[PythonCstNode] = [] + for use in uses: + child, current = use, use.parent + while current is not None: + if isinstance(current.node, cst.ClassDef) and any(child.node is base for base in current.node.bases): + if current not in classes: + classes.append(current) + break + child, current = current, current.parent + return classes + + +def declares_type_param(function: cst.FunctionDef, name: str) -> bool: + """Return True if the function declares `name` as its own PEP 695 type parameter (`def f[T](...)`).""" + parameters = function.type_parameters + return parameters is not None and any(parameter.param.name.value == name for parameter in parameters.params) # infer_variance needs no syntax of its own: a PEP 695 type parameter always infers its variance. @@ -300,59 +341,64 @@ def visit(node: ast.AST, *, shadowed: bool) -> None: } -def declared_default(decl_stmt: ast.Assign) -> ast.expr | None: +def declared_default(decl_stmt: cst.Assign) -> cst.BaseExpression | None: """Return the value a legacy declaration passes as `default=` (PEP 696), or None if it passes none.""" - call = cast("ast.Call", decl_stmt.value) - return next((keyword.value for keyword in call.keywords if keyword.arg == "default"), None) + return _keyword_value(_call(decl_stmt), "default") -def has_unconvertible_argument(decl_stmt: ast.Assign) -> bool: - """Return True if a legacy declaration passes a keyword build_type_param can't carry over. +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`. """ - call = cast("ast.Call", decl_stmt.value) convertible = _CONVERTIBLE_KEYWORDS[type_param_constructor_name(decl_stmt)] - return any(keyword.arg is None or keyword.arg not in convertible for keyword in call.keywords) + return any( + argument.star == "**" or (argument.keyword is not None and argument.keyword.value not in convertible) + for argument in _call(decl_stmt).args + ) -def build_type_param(decl_stmt: ast.Assign) -> ast.type_param: - """Translate a legacy declaration into the equivalent PEP 695 type_param node. +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)` becomes `T: int = int`. Bounds, constraints and + E.g. `T = TypeVar("T", bound=int, default=int)` gives `T: int = int`. Bounds, constraints and `default=` are carried over; `infer_variance=` needs nothing, since a PEP 695 type parameter always infers its variance. Callers must first reject declarations for which has_unconvertible_argument() is True. """ # TODO(FrancescoPezzella): a default that uses another legacy declaration (`default=T`) still refers to it after conversion. # https://github.com/TNO/Renaissance.Py/issues/279 - call = cast("ast.Call", decl_stmt.value) - ctor = type_param_constructor_name(decl_stmt) - name = cast("str", cast("ast.Constant", call.args[0]).value) - default = declared_default(decl_stmt) - - if ctor == "ParamSpec": - return ast.ParamSpec(name=name, default_value=default) - if ctor == "TypeVarTuple": - return ast.TypeVarTuple(name=name, default_value=default) + call = _call(decl_stmt) + constructor = type_param_constructor_name(decl_stmt) + positional = _positional_arguments(call) + name = cast("str", cast("cst.SimpleString", positional[0].value).evaluated_value) + + if constructor == "ParamSpec": + text = f"**{name}" + elif constructor == "TypeVarTuple": + text = f"*{name}" + else: + text = name + bound = _keyword_value(call, "bound") + constraints = [f"{argument.star}{module.code_for_node(argument.value)}" for argument in positional[1:]] + if bound is not None: + text += f": {module.code_for_node(bound)}" + elif constraints: + # A single constraint keeps a trailing comma, or the parentheses would make it a bound. + text += f": ({', '.join(constraints)}{',' if len(constraints) == 1 else ''})" - bound = next((kw.value for kw in call.keywords if kw.arg == "bound"), None) - constraints = call.args[1:] - if bound is None and constraints: - bound = ast.Tuple(elts=list(constraints), ctx=ast.Load()) - return ast.TypeVar(name=name, bound=bound, default_value=default) + default = declared_default(decl_stmt) + if default is not None: + text += f" = {module.code_for_node(default)}" + return text -def type_params_defaults_last(params: list[ast.type_param]) -> list[ast.type_param]: - """Return `params` with every type parameter that has a default moved after those without one. +def type_params_bracket(params: Iterable[tuple[str, bool]]) -> str: + """Return the `[...]` bracket for type parameters given as (text, has_default), with the defaulted ones last. The relative order within each group is kept. A non-default type parameter following a defaulted one is a SyntaxError. """ - - def has_default(param: ast.type_param) -> bool: - """Return True if `param` declares a default (`T = int`).""" - return isinstance(param, ast.TypeVar | ast.ParamSpec | ast.TypeVarTuple) and param.default_value is not None - - return sorted(params, key=has_default) + ordered = sorted(params, key=lambda param: param[1]) + return f"[{', '.join(text for text, _ in ordered)}]" diff --git a/src/renaissance/utils/import_resolution.py b/src/renaissance/utils/import_resolution.py index 2b6fc664..19380d74 100644 --- a/src/renaissance/utils/import_resolution.py +++ b/src/renaissance/utils/import_resolution.py @@ -6,13 +6,17 @@ """Resolve project-internal `from X import Y` statements to the .py file they import from, and re-anchor them on other files.""" -import ast from collections.abc import Sequence from pathlib import Path +from typing import cast + +import libcst as cst +from libcst import matchers +from libcst.helpers import get_full_name_for_node def resolve_project_module(importing_file: Path, project_root: Path, module: str | None, level: int) -> Path | None: - """Resolve one `ast.ImportFrom`'s `(module, level)` to a concrete .py file under `project_root`. + """Resolve one import-from's `(module, level)` to a concrete .py file under `project_root`. `level == 0` is an absolute import (`module` is dotted from `project_root`, e.g. "redis.typing"). `level >= 1` is relative (PEP 328): anchor at `importing_file`'s own @@ -96,49 +100,52 @@ def collect_project_imported_names(files: Sequence[Path], project_root: Path) -> imported: dict[Path, set[str]] = {} for file in files: try: - tree = ast.parse(file.read_text(encoding="utf-8")) - except OSError, SyntaxError: + module = cst.parse_module(file.read_text(encoding="utf-8")) + except OSError, cst.ParserSyntaxError: continue module_bindings: dict[str, Path] = {} - for stmt in ast.walk(tree): - if isinstance(stmt, ast.ImportFrom): - _record_from_import(file, project_root, stmt, imported, module_bindings) - elif isinstance(stmt, ast.Import): - _bind_imported_modules(file, project_root, stmt, module_bindings) - _record_module_attribute_reads(tree, module_bindings, imported) + for statement in matchers.findall(module, matchers.ImportFrom() | matchers.Import()): + if isinstance(statement, cst.ImportFrom): + _record_from_import(file, project_root, statement, imported, module_bindings) + else: + _bind_imported_modules(file, project_root, cast("cst.Import", statement), module_bindings) + _record_module_attribute_reads(module, module_bindings, imported) return {path: frozenset(names) for path, names in imported.items()} def _record_from_import( file: Path, project_root: Path, - stmt: ast.ImportFrom, + statement: cst.ImportFrom, imported: dict[Path, set[str]], module_bindings: dict[str, Path], ) -> None: - """Record `stmt`'s imported names against their origin, and bind any alias that is itself a project module.""" + """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 - origin = resolve_project_module(file, project_root, stmt.module, stmt.level) + module = get_full_name_for_node(statement.module) if statement.module is not None else None + level = len(statement.relative) + aliases = [] if isinstance(statement.names, cst.ImportStar) else list(statement.names) + origin = resolve_project_module(file, project_root, module, level) if origin is not None: - imported.setdefault(origin, set()).update(alias.name for alias in stmt.names) - for alias in stmt.names: - submodule = f"{stmt.module}.{alias.name}" if stmt.module is not None else alias.name - submodule_origin = resolve_project_module(file, project_root, submodule, stmt.level) + imported.setdefault(origin, set()).update([alias.evaluated_name for alias in aliases] or ["*"]) + for alias in aliases: + submodule = f"{module}.{alias.evaluated_name}" if module is not None else alias.evaluated_name + submodule_origin = resolve_project_module(file, project_root, submodule, level) if submodule_origin is not None: - module_bindings[alias.asname or alias.name] = submodule_origin + module_bindings[alias.evaluated_alias or alias.evaluated_name] = submodule_origin -def _bind_imported_modules(file: Path, project_root: Path, stmt: ast.Import, module_bindings: dict[str, Path]) -> None: - """Bind the dotted name(s) `stmt` makes available to the project module file each one refers to.""" - for alias in stmt.names: - if alias.asname is not None: - origin = resolve_project_module(file, project_root, alias.name, 0) +def _bind_imported_modules(file: Path, project_root: Path, statement: cst.Import, module_bindings: dict[str, Path]) -> None: + """Bind the dotted name(s) `statement` makes available to the project module file each one refers to.""" + for alias in statement.names: + if alias.evaluated_alias is not None: + origin = resolve_project_module(file, project_root, alias.evaluated_name, 0) if origin is not None: - module_bindings[alias.asname] = origin + module_bindings[alias.evaluated_alias] = origin continue # `import a.b.c` binds `a`, and makes `a.b` and `a.b.c` reachable through it. - parts = alias.name.split(".") + parts = alias.evaluated_name.split(".") for end in range(1, len(parts) + 1): prefix = ".".join(parts[:end]) origin = resolve_project_module(file, project_root, prefix, 0) @@ -146,24 +153,23 @@ def _bind_imported_modules(file: Path, project_root: Path, stmt: ast.Import, mod module_bindings[prefix] = origin -def _record_module_attribute_reads(tree: ast.Module, module_bindings: dict[str, Path], imported: dict[Path, set[str]]) -> None: - """Record every `.` in `tree` as a dependency on `attr` in that module's file.""" +def _record_module_attribute_reads(module: cst.Module, module_bindings: dict[str, Path], imported: dict[Path, set[str]]) -> None: + """Record every `.` in `module` as a dependency on `attr` in that module's file.""" if not module_bindings: return - for node in ast.walk(tree): - if not isinstance(node, ast.Attribute): - continue - dotted = _dotted_name(node.value) + for node in matchers.findall(module, matchers.Attribute()): + attribute = cast("cst.Attribute", node) + dotted = _dotted_name(attribute.value) origin = module_bindings.get(dotted) if dotted is not None else None if origin is not None: - imported.setdefault(origin, set()).add(node.attr) + imported.setdefault(origin, set()).add(attribute.attr.value) -def _dotted_name(expr: ast.expr) -> str | None: - """Return `expr` as a dotted name (`a.b.c`) if it is a plain name/attribute chain, else None.""" - if isinstance(expr, ast.Name): - return expr.id - if isinstance(expr, ast.Attribute): - base = _dotted_name(expr.value) - return f"{base}.{expr.attr}" if base is not None else None +def _dotted_name(expression: cst.BaseExpression) -> str | None: + """Return `expression` as a dotted name (`a.b.c`) if it is a plain name/attribute chain, else None.""" + if isinstance(expression, cst.Name): + return expression.value + if isinstance(expression, cst.Attribute): + base = _dotted_name(expression.value) + return f"{base}.{expression.attr.value}" if base is not None else None return None diff --git a/src/renaissance/utils/unparse_utils.py b/src/renaissance/utils/unparse_utils.py deleted file mode 100644 index 6e55eb23..00000000 --- a/src/renaissance/utils/unparse_utils.py +++ /dev/null @@ -1,124 +0,0 @@ -# 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 - -"""Splice a PEP 695 type-params bracket into a function's original source, changing nothing else. - -The function's formatting and comments are kept. -""" - -import ast -import io -import re -import tokenize - - -def _name_end_offset(source: str, name: str) -> int: - """Return the character offset right after `def name`/`async def name` in `source`. - - The `def` line may be indented, e.g. in a decorated function's source. - """ - match = re.search(rf"^[ \t]*(async\s+)?def\s+{re.escape(name)}\b", source, re.MULTILINE) - if match is None: - message = f"no 'def {name}' header found" - raise ValueError(message) - return match.end() - - -def _bracket_end_offset(source: str, open_offset: int) -> int: - """Return the offset right after the `]` matching the `[` at `open_offset` in `source`. - - Tracks bracket depth, so a nested `list[int]` doesn't close it early. A `[` or `]` inside a string - literal is not accounted for. - """ - depth = 0 - for offset in range(open_offset, len(source)): - if source[offset] == "[": - depth += 1 - elif source[offset] == "]": - depth -= 1 - if depth == 0: - return offset + 1 - message = "no closing ']' found" - raise ValueError(message) - - -def _type_params_bracket(node: ast.FunctionDef | ast.AsyncFunctionDef) -> str: - """Return the PEP 695 `[...]` bracket text for node's current type_params, or "" if none. - - E.g. `"[T]"`, `"[T: int, **P]"` - whatever `ast.unparse(node)` would produce. - """ - if not node.type_params: - return "" - unparsed = ast.unparse(node) - start = _name_end_offset(unparsed, node.name) - end = _bracket_end_offset(unparsed, start) - return unparsed[start:end] - - -def _header_end_line(source: str) -> int: - """Return the 1-indexed line where a def header's terminating ':' sits in `source`. - - Tracks `([{`/`)]}` bracket depth (via `tokenize`) so a colon inside a string default, a - lambda default, or an annotation - anything not at the header's own top level - isn't - mistaken for the real one. - """ - depth = 0 - for tok in tokenize.generate_tokens(io.StringIO(source).readline): - if tok.type == tokenize.OP and tok.string in "([{": - depth += 1 - elif tok.type == tokenize.OP and tok.string in ")]}": - depth -= 1 - elif tok.type == tokenize.OP and tok.string == ":" and depth == 0: - return tok.end[0] - message = "no header-terminating ':' found" - raise ValueError(message) - - -def unparse_signature_only(node: ast.FunctionDef | ast.AsyncFunctionDef, original_text: str) -> str: - """Insert node's PEP 695 type-params bracket into `original_text`, changing nothing else. - - `original_text` is node's source from before `node.type_params` was changed. An existing bracket - after the function name (`def f[U](...)`) is replaced by the new one, which already holds both the - old and new type parameters; otherwise a bracket is inserted. - - Everything else is kept, but lines after the first are re-indented relative to a column-0 `def` - (see `_renormalize_indent`), since the rewriter adds the target's own indentation to them again. - """ - new_bracket = _type_params_bracket(node) - insert_at = _name_end_offset(original_text, node.name) - - end = insert_at - if original_text[insert_at : insert_at + 1] == "[": - end = _bracket_end_offset(original_text, insert_at) - - spliced = original_text[:insert_at] + new_bracket + original_text[end:] - lines = spliced.split("\n") - if len(lines) == 1: - return spliced - - header_end_line = _header_end_line(spliced) - header_tail = lines[1:header_end_line] # a multi-line signature's own continuation lines - body = lines[header_end_line:] - # the header's own continuation lines (e.g. a closing ") -> T:") sit at the def's own column, - # so they renormalize to 0; the body sits one Python indentation level deeper, so 4. - return "\n".join([lines[0], *_renormalize_indent(header_tail, 0), *_renormalize_indent(body, 4)]) - - -def _renormalize_indent(lines: list[str], target_indent: int) -> list[str]: - """Shift `lines` so their common leading indent becomes `target_indent`.""" - non_blank = [line for line in lines if line.strip()] - if not non_blank: - return lines - # TODO(FrancescoPezzella): this minimum also counts lines inside multi-line string literals, which can change a literal's value. - # https://github.com/TNO/Renaissance.Py/issues/279 - common = min(len(line) - len(line.lstrip(" ")) for line in non_blank) - shift = common - target_indent - if shift > 0: - return [line[shift:] if line.strip() else line for line in lines] - if shift < 0: - pad = " " * -shift - return [pad + line if line.strip() else line for line in lines] - return lines diff --git a/test/recipes/conftest.py b/test/recipes/conftest.py index 65f0d2ca..c82bbb08 100644 --- a/test/recipes/conftest.py +++ b/test/recipes/conftest.py @@ -4,48 +4,33 @@ # of this repository, also available at: # https://github.com/TNO/Renaissance.Py/blob/main/LICENSE -"""Shared fixtures for the refactoring recipe test suite.""" +"""Shared fixture for the TypeVarCheck tests.""" import textwrap from collections.abc import Callable -from typing import cast +from pathlib import Path import pytest from pytest_mock import MockerFixture -from renaissance.integrations.python.ast.rst_node import PythonRstNode -from renaissance.recipes.python_refactoring import PythonRefactoring +from renaissance.integrations.python.ast.cst_node import PythonCstNode from renaissance.recipes.type_var_check import PEP_695_MINIMUM, TypeVarCheck @pytest.fixture -def make_recipe(mocker: MockerFixture) -> Callable[[type[PythonRefactoring], str], PythonRefactoring]: - """Build a `recipe_cls` instance against in-memory, dedented source text. +def create_type_var_check(mocker: MockerFixture) -> Callable[[str], TypeVarCheck]: + """Build a TypeVarCheck for in-memory, dedented source text, with min_python 3.12 so PEP 695 conversion is enabled. - `PythonFactory.create` is mocked so nothing touches the filesystem. Shared by every - refactoring recipe's tests instead of each reimplementing this setup - cast the result - to the concrete recipe type if you need attributes/methods beyond PythonRefactoring's own. + `PythonFactory.create` is mocked so nothing touches the filesystem. """ - def _make(recipe_cls: type[PythonRefactoring], text: str, filename: str = "x.py") -> PythonRefactoring: - code = textwrap.dedent(text) + def _create(text: str) -> TypeVarCheck: mocker.patch( "renaissance.integrations.python.ast.factory.PythonFactory.create", - return_value=PythonRstNode.load_from_text(code), + return_value=PythonCstNode.load_from_text(textwrap.dedent(text), "x.py"), ) - subject = recipe_cls(filename) + subject = TypeVarCheck(Path("x.py")) subject.in_memory = True - return subject - - return _make - - -@pytest.fixture -def create_type_var_check(make_recipe: Callable[[type[PythonRefactoring], str], PythonRefactoring]) -> Callable[[str], TypeVarCheck]: - """Like `make_recipe`, but with min_python set to 3.12 so PEP 695 conversion is enabled.""" - - def _create(text: str) -> TypeVarCheck: - subject = cast("TypeVarCheck", make_recipe(TypeVarCheck, text)) subject.min_python = PEP_695_MINIMUM return subject diff --git a/test/recipes/test_python_refactoring.py b/test/recipes/test_python_refactoring.py index 7b18f388..cb6e84d9 100644 --- a/test/recipes/test_python_refactoring.py +++ b/test/recipes/test_python_refactoring.py @@ -6,7 +6,6 @@ """Tests for the PythonRefactoring recipe base class.""" -import ast import keyword import textwrap from pathlib import Path @@ -318,27 +317,3 @@ def test_recipe_rewrites_with_the_chosen_backend(self, mocker: MockerFixture, no assert_that(cast("PythonFactory", subject.factory).clazz is node_class, is_(True)) assert_that(subject.apply_to_string(), is_("x = 3\ny = 2\n")) - - # ------------------------------------------------------------------ - # find_rst_node - # ------------------------------------------------------------------ - - def test_find_rst_node_returns_wrapper_for_raw_ast_node(self, mocker: MockerFixture) -> None: - """AI: Verify find_rst_node locates the PythonRstNode wrapping a given raw ast.FunctionDef.""" - self._patch_factory( - mocker, - """ - def foo(): - pass - """, - "test_foo.py", - ) - - subject = UnitToPytest("test_foo.py") - root = cast("PythonRstNode", cast("object", subject.root)) - module = cast("ast.Module", root.node) - target = next(node for node in ast.walk(module) if isinstance(node, ast.FunctionDef)) - - found = subject.find_rst_node(target) - - assert_that(found.node, is_(target)) diff --git a/test/recipes/test_type_var_check_convert.py b/test/recipes/test_type_var_check_convert.py index 4461d4e5..e0e6ce27 100644 --- a/test/recipes/test_type_var_check_convert.py +++ b/test/recipes/test_type_var_check_convert.py @@ -9,12 +9,18 @@ import ast import textwrap from collections.abc import Callable +from typing import cast import pytest from hamcrest import assert_that, contains_string, equal_to, has_entry, not_ from renaissance.recipes.type_var_check import PEP_695_MINIMUM, PEP_696_MINIMUM, TypeVarCheck -from renaissance.recipes.type_var_domain import UnsafeReason, type_param_name +from renaissance.recipes.type_var_domain import UnsafeReason + + +def _type_param_name(param: ast.type_param) -> str: + """Return a parsed PEP 695 type parameter's name.""" + return cast("ast.TypeVar | ast.ParamSpec | ast.TypeVarTuple", param).name class TestTypeVarCheckConvert: @@ -203,8 +209,8 @@ def identity(x: T) -> T: assert_that(result["converted"], has_entry("T", "fixed")) assert_that(result["orphaned"], equal_to({"P": "fixed", "T": "fixed"})) functions = {node.name: node for node in ast.parse(subject.apply_to_string()).body if isinstance(node, ast.FunctionDef)} - assert_that([type_param_name(param) for param in functions["run_in_threadpool"].type_params], equal_to(["P", "T"])) - assert_that([type_param_name(param) for param in functions["identity"].type_params], equal_to(["T"])) + assert_that([_type_param_name(param) for param in functions["run_in_threadpool"].type_params], equal_to(["P", "T"])) + assert_that([_type_param_name(param) for param in functions["identity"].type_params], equal_to(["T"])) def test_orders_type_params_by_declaration(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: """Verify the added type parameters follow the order their declarations appear in the file.""" @@ -217,12 +223,6 @@ def test_orders_type_params_by_declaration(self, create_type_var_check: Callable assert_that(subject.apply_to_string(), contains_string(f"def f[{', '.join(names)}]({parameters}) -> None: ...")) - @pytest.mark.xfail( - reason="_renormalize_indent takes the body's minimum indent over every line, including the lines " - "inside a multi-line string literal, so the literal's value changes.", - raises=AssertionError, - strict=True, - ) def test_converts_function_preserving_multiline_string_literal(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: """Verify converting a function keeps a multi-line string literal whose continuation line is at column 0.""" # Built line by line: textwrap.dedent in the fixture would otherwise be blocked by the column-0 line. @@ -277,22 +277,13 @@ def first(items: list[T]) -> T: assert_that(output, contains_string("def first[T](items: list[T]) -> T:")) assert_that(output, contains_string(" def get(self, x: T) -> T:")) - @pytest.mark.parametrize( - "class_header", - [ - pytest.param("class Box(Generic[T]):", id="generic-base"), - pytest.param("class Box[T]:", id="pep695-class"), - ], - ) - def test_reports_typevar_used_only_in_a_generic_class( - self, create_type_var_check: Callable[[str], TypeVarCheck], class_header: str - ) -> None: + def test_reports_typevar_used_only_in_a_generic_class(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: """Verify a TypeVar only a generic class uses is reported unsafe with its reason and left as it is.""" - subject = create_type_var_check(f""" + subject = create_type_var_check(""" from typing import Generic, TypeVar T = TypeVar('T') - {class_header} + class Box(Generic[T]): def get(self, x: T) -> T: return x """) @@ -302,6 +293,22 @@ def get(self, x: T) -> T: assert_that(subject.converted_unsafe_reasons, equal_to({"T": UnsafeReason.USED_IN_GENERIC_CLASS})) assert_that(subject.apply_to_string(), contains_string(" def get(self, x: T) -> T:")) + def test_ignores_methods_using_their_class_type_parameter(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + """Verify a T that is the class's own type parameter (`class Box[T]`) is not a use of the module-level T.""" + subject = create_type_var_check(""" + from typing import TypeVar + T = TypeVar('T') + + class Box[T]: + def get(self, x: T) -> T: + return x + """) + result = subject.convert_declared_typevars() + + assert_that(result, equal_to({})) + assert_that(subject.converted_unsafe_reasons, equal_to({})) + assert_that(subject.apply_to_string(), contains_string(" def get(self, x: T) -> T:")) + @pytest.mark.parametrize( "class_header", [ diff --git a/test/recipes/test_type_var_check_localize.py b/test/recipes/test_type_var_check_localize.py index 7d92bf2d..7db16657 100644 --- a/test/recipes/test_type_var_check_localize.py +++ b/test/recipes/test_type_var_check_localize.py @@ -16,7 +16,7 @@ from hamcrest import all_of, assert_that, contains_string, equal_to, has_entry, has_length, not_ from pytest_mock import MockerFixture -from renaissance.integrations.python.ast.rst_node import PythonRstNode +from renaissance.integrations.python.ast.cst_node import PythonCstNode from renaissance.recipes.type_var_check import PEP_695_MINIMUM, TypeVarCheck from renaissance.recipes.type_var_domain import UnsafeReason @@ -74,7 +74,7 @@ def _create_cross_file( importing_file = str(tmp_path / importing_rel_path) mocker.patch( "renaissance.integrations.python.ast.factory.PythonFactory.create", - return_value=PythonRstNode.load_from_text(importing_code, importing_file), + return_value=PythonCstNode.load_from_text(importing_code, importing_file), ) subject = TypeVarCheck(importing_file) subject.in_memory = True @@ -211,7 +211,7 @@ def b(x: T) -> T: assert_that(result, equal_to({"T": "fixed"})) output = subject.apply_to_string() - assert_that(output, contains_string("T = TypeVar('T')")) + assert_that(output, contains_string('T = TypeVar("T")')) assert_that(output, not_(contains_string("from file_1 import T"))) @pytest.mark.parametrize( @@ -255,7 +255,7 @@ def test_keeps_other_names_when_localizing_one_of_several_imports( assert_that(result, has_entry("T", "fixed")) output = subject.apply_to_string() assert_that(output.splitlines().count(expected_line), equal_to(1)) - assert_that(output, contains_string("T = TypeVar('T')")) + assert_that(output, contains_string('T = TypeVar("T")')) @pytest.mark.parametrize( ("importing_import", "comments"), @@ -268,12 +268,6 @@ def test_keeps_other_names_when_localizing_one_of_several_imports( ), ], ) - @pytest.mark.xfail( - reason="The narrowed import is rebuilt with ast.unparse, which has no comments: comments inside the import are " - "lost and a trailing comment stays after the replacement's last line, the localized declaration.", - raises=AssertionError, - strict=True, - ) def test_narrowing_an_import_keeps_its_comments( self, mocker: MockerFixture, @@ -303,13 +297,13 @@ def test_narrowing_an_import_keeps_its_comments( pytest.param( 'T = TypeVar("T")\nU = TypeVar("U")', "from file_1 import T, U", - ["from typing import TypeVar", "T = TypeVar('T')", "U = TypeVar('U')"], + ["from typing import TypeVar", 'T = TypeVar("T")', 'U = TypeVar("U")'], id="two-names", ), pytest.param( 'T = TypeVar("T")\nU = TypeVar("U")\ndef helper() -> None:\n pass', "from file_1 import T, helper, U", - ["from file_1 import helper", "from typing import TypeVar", "T = TypeVar('T')", "U = TypeVar('U')"], + ["from file_1 import helper", "from typing import TypeVar", 'T = TypeVar("T")', 'U = TypeVar("U")'], id="keeps-other-name", ), pytest.param( @@ -318,8 +312,8 @@ def test_narrowing_an_import_keeps_its_comments( [ "from file_1 import Circle, Shape", "from typing import TypeVar", - "T = TypeVar('T', bound=Shape)", - "U = TypeVar('U', bound=Circle)", + 'T = TypeVar("T", bound=Shape)', + 'U = TypeVar("U", bound=Circle)', ], id="bounds-share-one-import", ), @@ -518,7 +512,7 @@ def b() -> None: pytest.param( (3, 10), "unsafe", - all_of(contains_string("def b(x: T) -> T:"), contains_string("T = TypeVar('T')")), + all_of(contains_string("def b(x: T) -> T:"), contains_string('T = TypeVar("T")')), id="only-localizes-below-3.12", ), ], diff --git a/test/recipes/test_type_var_check_orphaned.py b/test/recipes/test_type_var_check_orphaned.py index b363a9f0..dc1c1356 100644 --- a/test/recipes/test_type_var_check_orphaned.py +++ b/test/recipes/test_type_var_check_orphaned.py @@ -8,12 +8,10 @@ import textwrap from collections.abc import Callable -from typing import cast import pytest from hamcrest import assert_that, contains_string, equal_to, has_entry, not_ -from renaissance.recipes.python_refactoring import PythonRefactoring from renaissance.recipes.type_var_check import TypeVarCheck from renaissance.recipes.type_var_domain import UnsafeReason @@ -52,6 +50,15 @@ def inner(y: T) -> T: return inner(x) """ +CLASS_WITH_OWN_TYPE_PARAMETER_SOURCE = """ +from typing import TypeVar +T = TypeVar('T') + +class Box[T]: + def get(self, x: T) -> T: + return x +""" + class TestTypeVarCheckOrphaned: """See module docstring.""" @@ -64,16 +71,17 @@ class TestTypeVarCheckOrphaned: pytest.param(UNREFERENCED_SOURCE, (3, 11), id="unreferenced-below-pep695"), pytest.param(NESTED_ONLY_CONVERTED_SOURCE, (3, 12), id="only-nested-function-converted"), pytest.param(CLOSURE_IN_CONVERTED_FUNCTION_SOURCE, (3, 12), id="closure-inside-converted-function"), + pytest.param(CLASS_WITH_OWN_TYPE_PARAMETER_SOURCE, (3, 12), id="class-with-own-type-parameter"), ], ) def test_removes_declaration_whose_references_are_all_shadowed( self, - make_recipe: Callable[[type[PythonRefactoring], str], PythonRefactoring], + 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 = cast("TypeVarCheck", make_recipe(TypeVarCheck, source)) + subject = create_type_var_check(source) subject.min_python = min_python result = subject.remove_orphaned_declarations() @@ -107,7 +115,6 @@ def b(y: T) -> T: pytest.param('"list[T]"', id="inside-annotation"), ], ) - @pytest.mark.xfail(reason="names inside string annotations aren't seen as references.", strict=True) def test_keeps_declaration_used_in_a_string_annotation( self, create_type_var_check: Callable[[str], TypeVarCheck], annotation: str ) -> None: diff --git a/test/recipes/test_type_var_check_properties.py b/test/recipes/test_type_var_check_properties.py index 98821925..123a51ff 100644 --- a/test/recipes/test_type_var_check_properties.py +++ b/test/recipes/test_type_var_check_properties.py @@ -12,7 +12,7 @@ import hypothesmith from hypothesis import given, reject, settings -from renaissance.integrations.python.ast.rst_node import PythonRstNode +from renaissance.integrations.python.ast.cst_node import PythonCstNode from renaissance.recipes.type_var_check import TypeVarCheck @@ -30,7 +30,7 @@ def test_check_never_crashes(self, source: str) -> None: with patch( "renaissance.integrations.python.ast.factory.PythonFactory.create", - return_value=PythonRstNode.load_from_text(source), + return_value=PythonCstNode.load_from_text(source), ): subject = TypeVarCheck("x.py") subject.in_memory = True diff --git a/test/recipes/test_type_var_domain.py b/test/recipes/test_type_var_domain.py index 3a25adc8..d336f533 100644 --- a/test/recipes/test_type_var_domain.py +++ b/test/recipes/test_type_var_domain.py @@ -6,11 +6,11 @@ """Tests for type_var_domain's safety predicates and their UnsafeReason results.""" -import ast import re import textwrap from pathlib import Path +import libcst as cst import pytest from hamcrest import assert_that, equal_to, not_ @@ -22,9 +22,9 @@ ) -def _parse(source: str) -> ast.Module: +def _parse(source: str) -> cst.Module: """Parse source after removing its common indentation.""" - return ast.parse(textwrap.dedent(source)) + return cst.parse_module(textwrap.dedent(source)) class TestIsSafeToRemove: diff --git a/test/utils/test_unparse_utils.py b/test/utils/test_unparse_utils.py deleted file mode 100644 index bd1b953d..00000000 --- a/test/utils/test_unparse_utils.py +++ /dev/null @@ -1,113 +0,0 @@ -# Copyright (C) 2024-2026 TNO -# -# Licensed under the terms specified in the LICENSE file in the root directory -# of this repository, also available at: -# https://github.com/TNO/Renaissance.Py/blob/main/LICENSE - -"""Tests for the signature-only PEP 695 bracket-splice helpers.""" - -import ast -import textwrap -from typing import cast - -import pytest -from hamcrest import assert_that, equal_to - -from renaissance.utils.unparse_utils import ( - _bracket_end_offset, # pyright: ignore[reportPrivateUsage] - _header_end_line, # pyright: ignore[reportPrivateUsage] - _name_end_offset, # pyright: ignore[reportPrivateUsage] - unparse_signature_only, -) - - -class TestNameEndOffset: - """See module docstring.""" - - @pytest.mark.parametrize( - ("source", "name", "expected"), - [ - pytest.param("def f(x: int) -> int:\n return x\n", "f", 5, id="plain-def"), - pytest.param("async def g(x: int) -> int:\n return x\n", "g", 11, id="async-def"), - pytest.param("@overload\n def __call__(self, x: int) -> int: ...\n", "__call__", 26, id="indented-after-decorator"), - ], - ) - def test_finds_the_offset_after_the_name(self, source: str, name: str, expected: int) -> None: - """Verify the offset right after the function name in its "def" line.""" - assert_that(_name_end_offset(source, name), equal_to(expected)) - - def test_raises_when_name_not_found(self) -> None: - """Verify a ValueError is raised when the function name doesn't appear in the source.""" - with pytest.raises(ValueError, match="no 'def f' header found"): - _name_end_offset("x = 1\n", "f") - - -class TestBracketEndOffset: - """See module docstring.""" - - @pytest.mark.parametrize( - ("source", "expected"), - [ - pytest.param("def f[T](x: T) -> T:\n return x\n", 8, id="simple"), - pytest.param("def f[T: list[int]](x: T) -> T:\n return x\n", 19, id="nested-bracket-in-bound"), - ], - ) - def test_finds_the_matching_closing_bracket(self, source: str, expected: int) -> None: - """Verify the offset right after the "]" matching the type-param bracket, across nested brackets.""" - assert_that(_bracket_end_offset(source, 5), equal_to(expected)) - - -class TestHeaderEndLine: - """See module docstring.""" - - @pytest.mark.parametrize( - ("source", "expected"), - [ - pytest.param("def f(x: int) -> int:\n return x\n", 1, id="one-line-signature"), - pytest.param("def f(\n a: int,\n b: str,\n) -> None:\n pass\n", 4, id="multi-line-signature"), - pytest.param('def f(\n b: str = "x:y",\n) -> None:\n pass\n', 3, id="colon-in-string-default"), - pytest.param("def f(cb=lambda: 1) -> int:\n return cb()\n", 1, id="colon-in-lambda-default"), - ], - ) - def test_finds_the_line_of_the_header_terminating_colon(self, source: str, expected: int) -> None: - """Verify the header ends on the line of its own ":", not one inside a default value.""" - assert_that(_header_end_line(source), equal_to(expected)) - - def test_raises_when_no_header_terminating_colon(self) -> None: - """Verify a ValueError is raised when the source has no header-terminating colon at all.""" - with pytest.raises(ValueError, match="no header-terminating ':' found"): - _header_end_line("x = 1\n") - - -class TestUnparseSignatureOnly: - """See module docstring.""" - - @pytest.mark.parametrize( - ("original", "expected"), - [ - pytest.param( - textwrap.dedent("""\ - def f(x): - # explains something - return x - """), - "def f[T](x):\n # explains something\n return x\n", - id="body-comment", - ), - # 8 spaces: one level for the class, one for the method body. - pytest.param("def f(x):\n return x", "def f[T](x):\n return x", id="method-body-absolute-indent"), - pytest.param("def f(x): ...\n", "def f[T](x): ...\n", id="inline-body"), - pytest.param( - "def f(\n x: int,\n y: int = 1,\n) -> int:\n return x\n", - "def f[T](\n x: int,\n y: int = 1,\n) -> int:\n return x\n", - id="multi-line-signature", - ), - pytest.param("def f[U](x: U, y):\n return x\n", "def f[U, T](x: U, y):\n return x\n", id="existing-bracket"), - ], - ) - def test_adds_the_bracket_and_keeps_everything_else(self, original: str, expected: str) -> None: - """Verify adding T inserts or extends the type-param bracket and keeps the rest of the source.""" - node = cast("ast.FunctionDef", ast.parse(original).body[0]) - node.type_params = [*node.type_params, ast.TypeVar(name="T")] - - assert_that(unparse_signature_only(node, original), equal_to(expected)) From f38626cc92a56235f5c79d02ecc4a43ef2b71aa9 Mon Sep 17 00:00:00 2001 From: Francesco Pezzella Date: Tue, 13 Oct 2026 13:34:15 +0200 Subject: [PATCH 2/3] Track the multi-line string re-indentation and PatternMatch placeholder bugs in TODOs Link the rewriter, TextUtils and UnitToPytest spots to #283, and the is_ancestor_of crash to #284. --- src/renaissance/recipes/unit_to_pytest.py | 2 ++ src/renaissance/syntax_tree/ast_rewriter.py | 7 +++++++ src/renaissance/utils/text_utils.py | 6 +++++- 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/renaissance/recipes/unit_to_pytest.py b/src/renaissance/recipes/unit_to_pytest.py index db99a597..ed6b988f 100644 --- a/src/renaissance/recipes/unit_to_pytest.py +++ b/src/renaissance/recipes/unit_to_pytest.py @@ -218,6 +218,8 @@ def restructure_module(self) -> None: # file does not contain any test class, create a new class and add function in class cls = f"class {self.convert_file_to_test_class()}:\n" for fun in funs: + # TODO(FrancescoPezzella): textwrap.indent also indents the lines inside a multi-line string, changing its value. + # https://github.com/TNO/Renaissance.Py/issues/283 cls += textwrap.indent(convert_function(fun), " ") self.remove([fun]) self.insert_before(cls, funs[0]) diff --git a/src/renaissance/syntax_tree/ast_rewriter.py b/src/renaissance/syntax_tree/ast_rewriter.py index c3576622..45de22a6 100644 --- a/src/renaissance/syntax_tree/ast_rewriter.py +++ b/src/renaissance/syntax_tree/ast_rewriter.py @@ -451,6 +451,8 @@ def __insert( else spaces ) # indent the new content except the first line + # TODO(FrancescoPezzella): unlike __replace, this ignores correct_indent, so the re-indentation can't be turned off. + # https://github.com/TNO/Renaissance.Py/issues/283 new_content = TextUtils.shift_right(new_content, indent, start_line=1) if before: @@ -499,6 +501,8 @@ def __compose_replacement(self, replacement: str, matches: Sequence[PatternMatch if regex_match: raw_signature = "".join(regex_match.groups()) place_holder_length = end_index - index + 1 + # TODO(FrancescoPezzella): this also indents the lines inside a multi-line string in the captured text. + # https://github.com/TNO/Renaissance.Py/issues/283 indent_replacement = raw_signature.replace("\n", "\n" + spaces) if ( placeholder.startswith("$$") @@ -535,6 +539,9 @@ def __get_text(self, node: Rewritable) -> str: # the descendants may need to be rewritten as well # rewrites = [rewrite for rewrite in self.rewrites if any(node.is_ancestor_of(rewrite_node) # for rewrite_node in rewrite.nodes)] + # TODO(FrancescoPezzella): is_ancestor_of only exists on ASTNode, so a PatternMatch replacement with placeholders raises + # AttributeError on Python RST and CST nodes. + # https://github.com/TNO/Renaissance.Py/issues/284 rewrites = [rewrite for rewrite in self.rewrites if any(node.is_ancestor_of(rewrite_node) for rewrite_node in rewrite.nodes)] if rewrites: rewriter = _RewriteActions(node, self.encoding, self.correct_indent, rewrites) diff --git a/src/renaissance/utils/text_utils.py b/src/renaissance/utils/text_utils.py index ec1774a4..93ed6e7b 100644 --- a/src/renaissance/utils/text_utils.py +++ b/src/renaissance/utils/text_utils.py @@ -23,6 +23,8 @@ class TextUtils: @staticmethod def shift_left(text: str, shift: int, start_line: int = 0) -> str: """Shifts each line of the given text to the left by the specified number of spaces. Only spaces are shifted.""" + # TODO(FrancescoPezzella): every line is shifted, also inside a multi-line string literal, changing its value. + # https://github.com/TNO/Renaissance.Py/issues/283 if shift == 0: return text pattern = re.compile(r"\s{0," + str(shift) + "}(.*)") @@ -57,7 +59,9 @@ def strip_indent(text: str, start_line: int = 0) -> str: @staticmethod def shift_right(text: str, shift: int, start_line: int = 0) -> str: - """Shifts each line of the given text to the left by the specified number of spaces. Only spaces are shifted.""" + """Shifts each line of the given text to the right by the specified number of spaces.""" + # TODO(FrancescoPezzella): every line is shifted, also inside a multi-line string literal, changing its value. + # https://github.com/TNO/Renaissance.Py/issues/283 if shift == 0: return text lines = text.split("\n") From 9c057886dfe16593e24d0048d31138e2e8807bd7 Mon Sep 17 00:00:00 2001 From: Francesco Pezzella Date: Tue, 13 Oct 2026 14:18:13 +0200 Subject: [PATCH 3/3] Track the TypeVarCheck bugs found in review in TODOs, and explain why __is_ancestor_in_nodes can't just drop its `and False` (#286). Move import_from_module and imported_name to import_resolution, so the import-from logic exists once. --- src/renaissance/recipes/type_var_check.py | 11 ++++++++--- src/renaissance/recipes/type_var_domain.py | 19 ++++++++----------- src/renaissance/syntax_tree/ast_rewriter.py | 5 +++++ src/renaissance/utils/import_resolution.py | 14 ++++++++++++-- 4 files changed, 33 insertions(+), 16 deletions(-) diff --git a/src/renaissance/recipes/type_var_check.py b/src/renaissance/recipes/type_var_check.py index 4809d8ab..d322b05e 100644 --- a/src/renaissance/recipes/type_var_check.py +++ b/src/renaissance/recipes/type_var_check.py @@ -24,8 +24,6 @@ from_import_sources, generic_classes, has_unconvertible_argument, - import_from_module, - imported_name, is_safe_to_localize, is_safe_to_remove, module_statements, @@ -35,7 +33,7 @@ type_params_bracket, uses_of, ) -from renaissance.utils.import_resolution import rebase_relative_module, resolve_project_module +from renaissance.utils.import_resolution import import_from_module, imported_name, rebase_relative_module, resolve_project_module PEP_695_MINIMUM = (3, 12) PEP_696_MINIMUM = (3, 13) @@ -175,6 +173,8 @@ def _add_type_params(self, function: PythonCstNode, params: list[tuple[str, bool node.name.value + type_params_bracket(params), unit.wrappers[node.name], include_whitespace=False, include_comments=False ) return + # TODO(FrancescoPezzella): rebuilding the existing bracket drops its comments and line breaks. + # https://github.com/TNO/Renaissance.Py/issues/279 current = [ (unit.atu.code_for_node(param.with_changes(comma=cst.MaybeSentinel.DEFAULT)), param.default is not None) for param in existing.params @@ -205,6 +205,8 @@ def remove_orphaned_declarations(self) -> dict[str, str]: 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" @@ -219,6 +221,9 @@ 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]: diff --git a/src/renaissance/recipes/type_var_domain.py b/src/renaissance/recipes/type_var_domain.py index 5fb1a66d..cfc8968f 100644 --- a/src/renaissance/recipes/type_var_domain.py +++ b/src/renaissance/recipes/type_var_domain.py @@ -14,9 +14,9 @@ import libcst as cst from libcst import matchers -from libcst.helpers import get_full_name_for_node from renaissance.integrations.python.ast.cst_node import PythonCstNode +from renaissance.utils.import_resolution import import_from_module, imported_name DOCS_BASE_URL = "https://tno.github.io/Renaissance.Py/user/features/type-parameter-modernization/" @@ -202,16 +202,6 @@ def declaration_argument_names(decl_stmt: cst.Assign) -> set[str]: return collector.names - set(dir(builtins)) -def import_from_module(statement: cst.ImportFrom) -> str | None: - """Return the dotted module an import-from names after its leading dots, or None for a bare `from . import x`.""" - return get_full_name_for_node(statement.module) if statement.module is not None else None - - -def imported_name(alias: cst.ImportAlias) -> str: - """Return the name an import alias binds: its `as` name if it has one, else the imported name.""" - return alias.evaluated_alias or alias.evaluated_name - - def 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. @@ -295,6 +285,9 @@ 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] @@ -372,6 +365,10 @@ def type_param_text(decl_stmt: cst.Assign, module: cst.Module) -> str: 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/syntax_tree/ast_rewriter.py b/src/renaissance/syntax_tree/ast_rewriter.py index 45de22a6..b9e70799 100644 --- a/src/renaissance/syntax_tree/ast_rewriter.py +++ b/src/renaissance/syntax_tree/ast_rewriter.py @@ -337,6 +337,11 @@ def no_conflict(node1, rew) -> bool: result = any(no_conflict(node, rew) for rew in rewrite_nodes) + # TODO(FrancescoPezzella): an outer rewrite should suppress the rewrites nested inside it, but both are applied. + # Dropping `and False` is not enough: no_conflict also matches each node with its own rewrite, so result is almost + # always True and apply() would skip nearly every rewrite. A fix has to leave out the node's own rewrite and use a + # true ancestor check such as __is_nested. + # https://github.com/TNO/Renaissance.Py/issues/286 return result and False # TODO: Why `and False` def __replace( diff --git a/src/renaissance/utils/import_resolution.py b/src/renaissance/utils/import_resolution.py index 19380d74..e4a19c26 100644 --- a/src/renaissance/utils/import_resolution.py +++ b/src/renaissance/utils/import_resolution.py @@ -15,6 +15,16 @@ from libcst.helpers import get_full_name_for_node +def import_from_module(statement: cst.ImportFrom) -> str | None: + """Return the dotted module an import-from names after its leading dots, or None for a bare `from . import x`.""" + return get_full_name_for_node(statement.module) if statement.module is not None else None + + +def imported_name(alias: cst.ImportAlias) -> str: + """Return the name an import alias binds: its `as` name if it has one, else the imported name.""" + return alias.evaluated_alias or alias.evaluated_name + + def resolve_project_module(importing_file: Path, project_root: Path, module: str | None, level: int) -> Path | None: """Resolve one import-from's `(module, level)` to a concrete .py file under `project_root`. @@ -123,7 +133,7 @@ def _record_from_import( """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 = get_full_name_for_node(statement.module) if statement.module is not None else None + module = import_from_module(statement) level = len(statement.relative) aliases = [] if isinstance(statement.names, cst.ImportStar) else list(statement.names) origin = resolve_project_module(file, project_root, module, level) @@ -133,7 +143,7 @@ def _record_from_import( submodule = f"{module}.{alias.evaluated_name}" if module is not None else alias.evaluated_name submodule_origin = resolve_project_module(file, project_root, submodule, level) if submodule_origin is not None: - module_bindings[alias.evaluated_alias or alias.evaluated_name] = submodule_origin + module_bindings[imported_name(alias)] = submodule_origin def _bind_imported_modules(file: Path, project_root: Path, statement: cst.Import, module_bindings: dict[str, Path]) -> None: