diff --git a/docs/TODO b/docs/TODO index 2ab4ca81a..46a1b3136 100644 --- a/docs/TODO +++ b/docs/TODO @@ -34,9 +34,8 @@ 1. **Scoping: skip and filter at the file level** (add to strategy-composition.md or a new page) — Java's `skipATU`, `globalFilter`, `sourceFilePostfixes`, and `sourceFileDirectories` provide coarse-grained scoping before any pattern matching runs. The Python equivalent concept needs documentation. - ### New pages worth adding 1. **Common parser problems** (new page, e.g., `docs/developer/modules/parser-known-limitations.md`) — Java has CommonCdtParsingProblems.md listing concrete CDT/MSVC-extension parsing failures with workarounds. A Python equivalent covering known tree-sitter, libcst, clang binding, and ANTLR limitations would be directly useful. -1. **Related works and context** (new section in index.md or index.md) — The Java UserGuide.md relates the tool to WHARS and ADA tooling. Adding a "related works" section that situates Renaissance-Experiments relative to the Java version, comby, and other code transformation tools would help new contributors understand design choices. \ No newline at end of file +1. **Related works and context** (new section in index.md or index.md) — The Java UserGuide.md relates the tool to WHARS and ADA tooling. Adding a "related works" section that situates Renaissance-Experiments relative to the Java version, comby, and other code transformation tools would help new contributors understand design choices. diff --git a/src/rejuvenation/migration-type-recipes.py b/src/rejuvenation/migration-type-recipes.py index 6837eb9cf..29f1e7bd8 100644 --- a/src/rejuvenation/migration-type-recipes.py +++ b/src/rejuvenation/migration-type-recipes.py @@ -6,13 +6,11 @@ """CLI that modernizes type parameters (TypeVar, ParamSpec, TypeVarTuple) to PEP 695 syntax. -Runs TypeVarCheck on a file or directory, and reports which files were modified and which need +Runs TypeParameterCheck on a file or directory, and reports which files were modified and which need manual review. -Examples: - python src/rejuvenation/migration-type-recipes.py ./some_repo --py 3.12 --report review.md - python src/rejuvenation/migration-type-recipes.py ./some_repo/file.py --py 3.10 - python src/rejuvenation/migration-type-recipes.py ./some_repo --py 3.12 --no-ruff +Example: + python src/rejuvenation/migration-type-recipes.py ./repo --py 3.12 --report review.md --ignore folder/types.py """ @@ -23,6 +21,7 @@ import subprocess import sys import textwrap +from collections import Counter from collections.abc import Sequence from dataclasses import dataclass from pathlib import Path @@ -30,21 +29,51 @@ from termcolor import colored from renaissance.project.project_scanner import PythonScanner -from renaissance.recipes.type_var_check import TypeVarCheck -from renaissance.recipes.type_var_domain import UNSAFE_RULES, UnsafeReason, doc_link +from renaissance.recipes.type_parameter_check import MIN_SUPPORTED_PYTHON, TypeParameterCheck +from renaissance.recipes.type_parameter_rules import DOCS_BASE_URL, UNSAFE_RULES, UnsafeReason from renaissance.utils.import_resolution import collect_project_imported_names _MAJOR_MINOR_PART_COUNT = 2 +_TYPE_PARAM_KINDS = ("TypeVar", "ParamSpec", "TypeVarTuple") +_FIXED_DESCRIPTIONS = { + "converted": "converted to PEP 695 in functions", + "orphaned": "unused declaration removed", + "cross_file": "import replaced by a local declaration", +} +_MAX_LISTED_NAMES = 10 @dataclass class FileReport: - """Outcome of running TypeVarCheck against a single file.""" + """Outcome of running TypeParameterCheck against a single file.""" path: Path result: dict[str, dict[str, str]] | None error: str | None reasons: dict[str, dict[str, UnsafeReason]] | None = None + kinds: dict[str, str] | None = None + ignored: frozenset[str] = frozenset() + + +@dataclass +class KindCount: + """How many type parameters of one kind a run reported, and how many were fixed, need manual review or were ignored.""" + + reported: int = 0 + fixed: int = 0 + needs_review: int = 0 + ignored: int = 0 + + +@dataclass(frozen=True) +class LocalizationSettings: + """Whether imported type parameters are copied into the importing file, and the files they are never copied from.""" + + enabled: bool = True + ignored_files: frozenset[Path] = frozenset() + + +_DEFAULT_LOCALIZATION = LocalizationSettings() def resolve_target_files(target: Path) -> list[Path]: @@ -62,14 +91,19 @@ def _parse_py_version(text: str) -> tuple[int, int]: """Parse a "MAJOR.MINOR" string into a (major, minor) tuple for argparse's type=. Raises: - argparse.ArgumentTypeError: If text is not in MAJOR.MINOR form. + argparse.ArgumentTypeError: If text is not in MAJOR.MINOR form or is below MIN_SUPPORTED_PYTHON. """ parts = text.split(".") if len(parts) != _MAJOR_MINOR_PART_COUNT or not all(part.isdigit() for part in parts): message = f"expected MAJOR.MINOR (e.g. 3.12), got {text!r}" raise argparse.ArgumentTypeError(message) - return (int(parts[0]), int(parts[1])) + version = (int(parts[0]), int(parts[1])) + if version < MIN_SUPPORTED_PYTHON: + minimum = ".".join(map(str, MIN_SUPPORTED_PYTHON)) + message = f"expected {minimum} or higher, since PEP 695 syntax does not exist below it, got {text!r}" + raise argparse.ArgumentTypeError(message) + return version def has_fixed(report: FileReport) -> bool: @@ -80,10 +114,8 @@ def has_fixed(report: FileReport) -> bool: def has_unsafe(report: FileReport) -> bool: - """Return True if any phase of report.result left at least one name unsafe to touch.""" - if report.result is None: - return False - return any("unsafe" in phase.values() for phase in report.result.values()) + """Return True if any phase of report.result gave at least one name a reason to review.""" + return any(report.reasons.values()) if report.reasons else False def is_clean(report: FileReport) -> bool: @@ -99,16 +131,19 @@ def process_file( min_python: tuple[int, int], project_root: Path, project_wide_imported_names: frozenset[str], + localization: LocalizationSettings = _DEFAULT_LOCALIZATION, ) -> FileReport: - """Run TypeVarCheck's three phases on one file and return its FileReport. + """Run TypeParameterCheck's three phases on one file and return its FileReport. Any exception is caught and stored on FileReport.error, so one bad file never aborts a batch run. """ try: - recipe = TypeVarCheck(path) + recipe = TypeParameterCheck(path) recipe.min_python = min_python recipe.project_root = project_root recipe.project_wide_imported_names = project_wide_imported_names + recipe.ignore_localize_origins = localization.ignored_files + recipe.localize_imports = localization.enabled result = recipe.check() reasons = { "cross_file": recipe.cross_file_unsafe_reasons, @@ -117,18 +152,88 @@ def process_file( } except Exception as exc: # noqa: BLE001 - isolate one bad file, never abort the whole batch return FileReport(path=path, result=None, error=f"{type(exc).__name__}: {exc}") - return FileReport(path=path, result=result, error=None, reasons=reasons) + return FileReport( + path=path, + result=result, + error=None, + reasons=reasons, + kinds=recipe.type_param_kinds, + ignored=frozenset(recipe.ignored_names), + ) + + +def _merge_reports(first: FileReport, second: FileReport) -> FileReport: + """Return the report of a file processed twice: second's names replace first's, and first's other names stay. + + A reason from first is dropped when second evaluated the same name again. A second that errored is returned as it is. + """ + if second.error is not None or first.result is None or second.result is None: + return second + first_reasons = first.reasons or {} + second_reasons = second.reasons or {} + return FileReport( + path=first.path, + result={phase: {**names, **second.result.get(phase, {})} for phase, names in first.result.items()}, + error=None, + reasons={ + phase: {name: reason for name, reason in reasons.items() if name not in second.result.get(phase, {})} + | second_reasons.get(phase, {}) + for phase, reasons in first_reasons.items() + }, + kinds={**(first.kinds or {}), **(second.kinds or {})}, + ignored=first.ignored | second.ignored, + ) + + +def _run_second_pass( + reports: list[FileReport], + files: Sequence[Path], + project_root: Path, + min_python: tuple[int, int], + localization: LocalizationSettings, +) -> list[FileReport]: + """Process again the files that kept a declaration because another file imported it, and merge the two reports of each. + + The imported names are collected again, so a declaration whose importers were all localized in the first pass is + no longer protected. Every other report is returned as it was. + """ + candidates = { + report.path + for report in reports + if any(UnsafeReason.IMPORTED_ELSEWHERE_IN_PROJECT in reasons.values() for reasons in (report.reasons or {}).values()) + } + if not candidates: + return reports + imported_names_by_file = collect_project_imported_names(files, project_root) + merged: list[FileReport] = [] + for report in reports: + if report.path not in candidates: + merged.append(report) + continue + second = process_file( + report.path, + min_python=min_python, + project_root=project_root, + project_wide_imported_names=imported_names_by_file.get(report.path, frozenset()), + localization=localization, + ) + merged.append(_merge_reports(report, second)) + print(f"File {report.path} checked again.") + return merged -def _run_ruff_unused_import_cleanup(paths: list[Path]) -> None: - """Run `ruff check --fix --select F401` over every path, dropping any now-unused import. +def _run_ruff_import_cleanup(paths: list[Path]) -> None: + """Run `ruff check --fix --select F401,I001` over every path. + + Drops any now-unused import (F401), then merges and sorts the imports (I001) under the settings ruff + finds for each path. Best-effort: if ruff can't be invoked (e.g. not installed), prints a warning instead of raising. The files are already valid without this cleanup. """ try: subprocess.run( # noqa: S603 - fixed argv list (sys.executable + literals + our own discovered paths), no shell - [sys.executable, "-m", "ruff", "check", "--fix", "--select", "F401", *(str(path) for path in paths)], + [sys.executable, "-m", "ruff", "check", "--fix", "--select", "F401,I001", *(str(path) for path in paths)], capture_output=True, text=True, check=False, @@ -137,25 +242,204 @@ def _run_ruff_unused_import_cleanup(paths: list[Path]) -> None: print(colored(f"warning: could not run ruff for import cleanup: {exc}", "yellow")) -def _format_commit_summary(reports: list[FileReport]) -> str: - """Build the short, copy-pasteable commit-message-style summary.""" - modified = sum(1 for report in reports if has_fixed(report)) +def _files(count: int) -> str: + """Return count with "file" pluralized, e.g. "1 file" or "2 files".""" + return f"{count} file" if count == 1 else f"{count} files" + + +def _format_names(file_counts: Counter[str]) -> str: + """Return the names, most files first then alphabetical, each with its file count if above one. + + Lists at most _MAX_LISTED_NAMES names and summarises the rest as "and N more". + """ + ranked = sorted(file_counts.items(), key=lambda item: (-item[1], item[0])) + listed = ", ".join(name if count == 1 else f"{name} ({_files(count)})" for name, count in ranked[:_MAX_LISTED_NAMES]) + hidden = len(ranked) - _MAX_LISTED_NAMES + return f"{listed} and {hidden} more" if hidden > 0 else listed + + +def _format_commit_summary(reports: Sequence[FileReport], min_python: tuple[int, int]) -> str: + """Build the copy-pasteable commit message: what was done to which names, and what is left for manual review. + + Names are grouped by action with the number of files they were fixed in. The manual review line, + with the files needing it and their reasons by frequency, is left out when no file needs review. + """ + modified = [report for report in reports if has_fixed(report)] + names_by_action = {action: Counter[str]() for action in _FIXED_DESCRIPTIONS.values()} + for report in modified: + for name, actions in _fixed_descriptions(report).items(): + for action in actions: + names_by_action[action][name] += 1 + version = ".".join(map(str, min_python)) + lines = [ + "Modernize TypeVar/ParamSpec/TypeVarTuple usage to PEP 695 syntax", + "", + f"Convert type parameters to PEP 695 syntax in {_files(len(modified))} (Python {version}+).", + "", + *(f"- {action}: {_format_names(file_counts)}" for action, file_counts in names_by_action.items() if file_counts), + ] needs_review = sum(1 for report in reports if has_unsafe(report)) - clean = sum(1 for report in reports if is_clean(report)) - errors = sum(1 for report in reports if report.error is not None) - return ( - "Modernize TypeVar/ParamSpec/TypeVarTuple usage to PEP 695 syntax\n\n" - f"{modified} files modified, {needs_review} need manual review, {clean} clean, " - f"{errors} errors (of {len(reports)} processed)" - ) + if needs_review: + reasons = ", ".join(f"{UNSAFE_RULES[reason].message} ({count})" for reason, count in rank_reasons(reports)) + lines.extend(["", f"{_files(needs_review)} left for manual review: {reasons}."]) + return "\n".join(lines) + + +def _name_statuses(report: FileReport) -> dict[str, str]: + """Return {name: "fixed" | "unsafe"} over all phases of report.result; "unsafe" wins if any phase gave the name a reason.""" + statuses: dict[str, str] = {} + for phase in (report.result or {}).values(): + for name, status in phase.items(): + if statuses.get(name) != "unsafe": + statuses[name] = status + for phase_reasons in (report.reasons or {}).values(): + statuses.update(dict.fromkeys(phase_reasons, "unsafe")) + return statuses -def _format_console_report(reports: list[FileReport], *, ruff_ran: bool) -> str: - """Build the full per-file report: MODIFIED / NEEDS MANUAL REVIEW / ERRORS sections. +def count_by_kind(reports: Sequence[FileReport]) -> dict[str, KindCount]: + """Count the type parameters the reports list, per kind (TypeVar, ParamSpec, TypeVarTuple). - Clean files (no type parameter usage at all) are only counted, not listed, so the report shows - what needs attention. Under MODIFIED, a converted name isn't repeated as orphaned. The ruff - import-cleanup line is only included when ruff_ran is True. + A name counts once per file, as needing review if any phase left it unsafe and as fixed otherwise. A name + the file imports from an ignored file counts as ignored. Files that errored are skipped. + """ + counts = {kind: KindCount() for kind in _TYPE_PARAM_KINDS} + for report in reports: + kinds = report.kinds or {} + for name in report.ignored: + counts[kinds[name]].reported += 1 + counts[kinds[name]].ignored += 1 + for name, status in _name_statuses(report).items(): + count = counts[kinds[name]] + count.reported += 1 + if status == "unsafe": + count.needs_review += 1 + else: + count.fixed += 1 + return counts + + +def rank_reasons(reports: Sequence[FileReport]) -> list[tuple[UnsafeReason, int]]: + """Return each UnsafeReason the reports hit with its count, most common first, ties in UnsafeReason order. + + Every (phase, name) reason counts, so a name unsafe for two reasons counts under both. + """ + counter = Counter(reason for report in reports for phase in (report.reasons or {}).values() for reason in phase.values()) + order = list(UnsafeReason) + return sorted(counter.items(), key=lambda item: (-item[1], order.index(item[0]))) + + +def _code(text: object) -> str: + """Return text as a Markdown inline code span.""" + return f"`{text}`" + + +def _heading(text: str) -> str: + """Return text as a bold Markdown line, used for the report's section headings.""" + return f"**{text}**" + + +def _table_row(cells: Sequence[str], widths: Sequence[int]) -> str: + """Return one Markdown table row with each cell padded to its column width.""" + return "| " + " | ".join(cell.ljust(width) for cell, width in zip(cells, widths, strict=True)) + " |" + + +def _format_table(headers: Sequence[str], rows: Sequence[Sequence[str]]) -> list[str]: + """Return a Markdown table as lines, with every column padded to its widest cell and the dash row to the same width.""" + widths = [max(len(cell) for cell in column) for column in zip(headers, *rows, strict=True)] + return [_table_row(headers, widths), _table_row(["-" * width for width in widths], widths), *(_table_row(row, widths) for row in rows)] + + +def _summary_tables(reports: Sequence[FileReport], *, show_ignored: bool) -> list[str]: + """Return the report's summary tables: type parameters per kind, then the unsafe reasons by frequency. + + The Ignored column is only there when show_ignored is True. The reasons table is left out when no reason was hit. + """ + headers = ["Kind", "Reported", "Fixed", "Needs review", *(["Ignored"] if show_ignored else [])] + kind_rows = [ + [_code(kind), str(c.reported), str(c.fixed), str(c.needs_review), *([str(c.ignored)] if show_ignored else [])] + for kind, c in count_by_kind(reports).items() + ] + lines = ["", _heading("TYPE PARAMETERS BY KIND"), "", *_format_table(headers, kind_rows)] + ranking = rank_reasons(reports) + if ranking: + reason_rows = [[UNSAFE_RULES[reason].message, str(count)] for reason, count in ranking] + lines.extend(["", _heading("UNSAFE REASONS, MOST COMMON FIRST"), "", *_format_table(["Reason", "Count"], reason_rows)]) + return lines + + +def _join_with_blank_lines(entries: list[list[str]]) -> list[str]: + """Flatten entries into one list of lines, with a blank line between consecutive entries.""" + lines: list[str] = [] + for index, entry in enumerate(entries): + if index: + lines.append("") + lines.extend(entry) + return lines + + +def _fixed_descriptions(report: FileReport) -> dict[str, list[str]]: + """Return {name: [what was done to it]} for the names report.result fixed. + + A converted name isn't described as removed, because its declaration is always removed afterwards, + and a removed name isn't described as localized. + """ + result = report.result or {} + converted = {name for name, status in result.get("converted", {}).items() if status == "fixed"} + removed = {name for name, status in result.get("orphaned", {}).items() if status == "fixed"} - converted + descriptions: dict[str, list[str]] = {} + for phase, names in result.items(): + for name, status in names.items(): + if status == "fixed" and not (phase == "orphaned" and name in converted) and not (phase == "cross_file" and name in removed): + descriptions.setdefault(name, []).append(_FIXED_DESCRIPTIONS[phase]) + return descriptions + + +def _modified_entry(report: FileReport, project_root: Path) -> list[str]: + """Return the MODIFIED lines for one file: its path relative to project_root, then what was done to each name.""" + lines = [f" {_code(report.path.relative_to(project_root))}"] + lines.extend(f" {_code(name)}: {', '.join(texts)}" for name, texts in _fixed_descriptions(report).items()) + return lines + + +def _needs_review_groups(reports: Sequence[FileReport], project_root: Path) -> list[list[str]]: + """Return one block of NEEDS MANUAL REVIEW lines per reason, most common reason first. + + A block is the reason as a heading, then each file it applies to with the names it applies to. + """ + names_by_reason: dict[UnsafeReason, dict[Path, list[str]]] = {} + for report in reports: + for phase_reasons in (report.reasons or {}).values(): + for name, reason in phase_reasons.items(): + names = names_by_reason.setdefault(reason, {}).setdefault(report.path, []) + if name not in names: + names.append(name) + groups: list[list[str]] = [] + for reason, _ in rank_reasons(reports): + message = UNSAFE_RULES[reason].message + files = names_by_reason[reason].items() + groups.append( + [ + _heading(message[:1].upper() + message[1:]), + *(f" {_code(path.relative_to(project_root))}: {', '.join(map(_code, names))}" for path, names in files), + ], + ) + return groups + + +def _format_console_report( + reports: list[FileReport], + *, + project_root: Path, + localization: LocalizationSettings, +) -> str: + """Build the full report as Markdown: MODIFIED / NEEDS MANUAL REVIEW / ERRORS sections. + + Files are listed by their path relative to project_root. MODIFIED lists each file with what was done to + its names, separated by a blank line; NEEDS MANUAL REVIEW groups the files and names by reason. Clean + files (no type parameter usage at all) are only counted, not listed, so the report shows what needs + attention. The IGNORED FILES section is only included when localization.ignored_files is not empty, + and the Ignored column when it is not empty or localization is disabled. """ modified = [report for report in reports if has_fixed(report)] needs_review = [report for report in reports if has_unsafe(report)] @@ -163,42 +447,26 @@ def _format_console_report(reports: list[FileReport], *, ruff_ran: bool) -> str: clean_count = sum(1 for report in reports if is_clean(report)) lines = [ - "Renaissance TypeVarCheck migration report", + "Renaissance TypeParameterCheck migration report", ( f"Processed {len(reports)} files: {len(modified)} modified, {len(needs_review)} need " f"manual review, {clean_count} clean, {len(errors)} errors" ), ] - if ruff_ran: - lines.append( - "Unused imports across the modified files above were also cleaned up via `ruff check --fix --select F401`.", - ) - lines.extend(["", f"MODIFIED ({len(modified)})"]) - for report in modified: - lines.append(f" {report.path}") - result = report.result or {} - converted = {name for name, status in result.get("converted", {}).items() if status == "fixed"} - for phase, names in result.items(): - # A converted name's declaration is always removed afterwards; listing it under orphaned too would repeat it. - fixed = [name for name, status in names.items() if status == "fixed" and not (phase == "orphaned" and name in converted)] - if fixed: - lines.append(f" {phase}: {', '.join(fixed)}") - - lines.extend(["", f"NEEDS MANUAL REVIEW ({len(needs_review)})"]) - for report in needs_review: - lines.append(f" {report.path}") - for phase, names in (report.result or {}).items(): - phase_reasons = (report.reasons or {}).get(phase, {}) - unsafe = [name for name, status in names.items() if status == "unsafe"] - if unsafe: - lines.append(f" {phase}: {', '.join(unsafe)}") - for name in unsafe: - reason = phase_reasons.get(name) - if reason is not None: - lines.append(f" {name}: {UNSAFE_RULES[reason].message} -> {doc_link(reason)}") - - lines.extend(["", f"ERRORS ({len(errors)})"]) - lines.extend(f" {report.path}: {report.error}" for report in errors) + if localization.ignored_files: + lines.extend(["", _heading(f"IGNORED FILES ({len(localization.ignored_files)})")]) + lines.extend(f" {_code(ignored.relative_to(project_root))}" for ignored in sorted(localization.ignored_files)) + lines.extend(_summary_tables(reports, show_ignored=bool(localization.ignored_files) or not localization.enabled)) + lines.extend(["", _heading(f"MODIFIED ({len(modified)})")]) + lines.extend(_join_with_blank_lines([_modified_entry(report, project_root) for report in modified])) + + lines.extend(["", _heading(f"NEEDS MANUAL REVIEW ({len(needs_review)})")]) + if needs_review: + lines.extend([f" {DOCS_BASE_URL}", ""]) + lines.extend(_join_with_blank_lines(_needs_review_groups(needs_review, project_root))) + + lines.extend(["", _heading(f"ERRORS ({len(errors)})")]) + lines.extend(f" {_code(report.path.relative_to(project_root))}: {report.error}" for report in errors) return "\n".join(lines) @@ -210,9 +478,7 @@ def build_arg_parser() -> argparse.ArgumentParser: description="Modernize legacy TypeVar/ParamSpec/TypeVarTuple usage to PEP 695 syntax.", epilog=textwrap.dedent("""\ Examples: - python src/rejuvenation/migration-type-recipes.py ./some_repo --py 3.12 --report review.md - python src/rejuvenation/migration-type-recipes.py ./some_repo/file.py --py 3.10 - python src/rejuvenation/migration-type-recipes.py ./some_repo --py 3.12 --no-ruff + python src/rejuvenation/migration-type-recipes.py ./repo --py 3.12 --report review.md --ignore folder/types.py """), formatter_class=argparse.RawDescriptionHelpFormatter, ) @@ -222,22 +488,62 @@ def build_arg_parser() -> argparse.ArgumentParser: type=_parse_py_version, required=True, metavar="MAJOR.MINOR", - help=( - "Minimum Python version the target project supports (not the one running this tool), e.g. 3.12. " - "PEP 695 rewrites need 3.12+, PEP 696 type parameter defaults 3.13+." - ), + help=("Minimum Python version the target project supports is 3.12."), ) parser.add_argument("--report", type=Path, metavar="PATH", help="Also write the full report to this file.") parser.add_argument( "--no-ruff", action="store_true", - help="Skip the final `ruff check --fix --select F401` pass that drops imports made unused.", + help="Skip the final `ruff check --fix --select F401,I001` pass that drops imports made unused and sorts the rest.", + ) + parser.add_argument( + "--ignore", + action="extend", + nargs="+", + type=Path, + default=[], + metavar="FILE", + help=( + "Files, relative to the scanned directory, that are left untouched; their type parameters are not copied " + "into the files importing them, which keep those imports. Give the path argument first, since this option " + "takes every value after it." + ), + ) + parser.add_argument( + "--no-localization", + action="store_true", + help=( + "Keep the imports of project type parameters instead of copying their declarations into the importing file; " + "the functions using them are not converted." + ), + ) + parser.add_argument( + "--second-pass", + action="store_true", + help=( + "Process again the files that kept a declaration only because another file imported it when the run started, " + "and report both passes together." + ), ) return parser +def _localization_settings(parser: argparse.ArgumentParser, args: argparse.Namespace, project_root: Path) -> LocalizationSettings: + """Build the LocalizationSettings from --no-localization and --ignore, the latter resolved against project_root. + + Calls parser.error, which exits with code 2, if an --ignore file does not exist. + """ + for ignored in args.ignore: + if not (project_root / ignored).is_file(): + parser.error(f"--ignore: file does not exist: {ignored}") + return LocalizationSettings( + enabled=not args.no_localization, + ignored_files=frozenset((project_root / ignored).resolve() for ignored in args.ignore), + ) + + def main(argv: Sequence[str] | None = None) -> int: - """Parse arguments, run TypeVarCheck across the target, print/save the report, return an exit code. + """Parse arguments, run TypeParameterCheck across the target, print/save the report, return an exit code. Exit codes: 0 on normal completion (files needing manual review are informational, not a failure), 2 on a usage error (bad path/argument), 3 if any file hit an unhandled exception. @@ -255,35 +561,45 @@ def main(argv: Sequence[str] | None = None) -> int: target = target.resolve() files = resolve_target_files(target) project_root = target if target.is_dir() else target.parent - # TODO(FrancescoPezzella): computed once upfront, so an origin whose importers all get localized this run is only converted - # on a second run. - # https://github.com/TNO/Renaissance.Py/issues/279 imported_names_by_file = collect_project_imported_names(files, project_root) + localization = _localization_settings(parser, args, project_root) + reports: list[FileReport] = [] for path in files: + if path in localization.ignored_files: + continue project_wide_imported_names = imported_names_by_file.get(path, frozenset()) report = process_file( path, min_python=args.py, project_root=project_root, project_wide_imported_names=project_wide_imported_names, + localization=localization, ) reports.append(report) print(f"File {path} checked.") + if args.second_pass: + reports = _run_second_pass(reports, files, project_root, args.py, localization) + modified_paths = [report.path for report in reports if has_fixed(report)] ruff_ran = bool(modified_paths) and not args.no_ruff if ruff_ran: # TODO(FrancescoPezzella): removed statements leave their blank lines behind; ruff's E303 (preview) collapses them, but not # at file start. # https://github.com/TNO/Renaissance.Py/issues/279 - _run_ruff_unused_import_cleanup(modified_paths) + _run_ruff_import_cleanup(modified_paths) - console_report = _format_console_report(reports, ruff_ran=ruff_ran) + console_report = _format_console_report( + reports, + project_root=project_root, + localization=localization, + ) print(console_report) - print() - print(colored(_format_commit_summary(reports), "green", attrs=["bold"])) + if modified_paths: + print() + print(colored(_format_commit_summary(reports, args.py), "green", attrs=["bold"])) if args.report is not None: args.report.write_text(console_report, encoding="utf-8") diff --git a/src/renaissance/integrations/python/ast/cst_node.py b/src/renaissance/integrations/python/ast/cst_node.py index cc240b0d5..12ac353f9 100644 --- a/src/renaissance/integrations/python/ast/cst_node.py +++ b/src/renaissance/integrations/python/ast/cst_node.py @@ -242,7 +242,9 @@ def preceding_sibling(self) -> PythonCstNode | None: @staticmethod def load(file_path: Path) -> PythonCstNode: """AI: Parse the Python source file at file_path into a PythonCstNode tree.""" - with Path(file_path).open() as file: + # TODO(FrancescoPezzella): drop the explicit encoding with Python 3.15, where UTF-8 is the default. + # https://github.com/TNO/Renaissance.Py/issues/188 + with Path(file_path).open(encoding="utf-8") as file: content = file.read() return PythonCstNode.load_from_text(content, str(file_path)) diff --git a/src/renaissance/integrations/python/ast/rst_node.py b/src/renaissance/integrations/python/ast/rst_node.py index 4035ac545..eab7ae4f6 100644 --- a/src/renaissance/integrations/python/ast/rst_node.py +++ b/src/renaissance/integrations/python/ast/rst_node.py @@ -356,7 +356,9 @@ def load( # Keep a uniform loader signature across AST node implementations. # Python's AST parser does not need extra arguments or a working dir. _ = extra_args, working_dir - with Path(file_path).open() as file: + # TODO(FrancescoPezzella): drop the explicit encoding with Python 3.15, where UTF-8 is the default. + # https://github.com/TNO/Renaissance.Py/issues/188 + with Path(file_path).open(encoding="utf-8") as file: content = file.read() return PythonRstNode.load_from_text(content, str(file_path)) diff --git a/src/renaissance/project/project_scanner.py b/src/renaissance/project/project_scanner.py index 5478a6954..e2131e759 100644 --- a/src/renaissance/project/project_scanner.py +++ b/src/renaissance/project/project_scanner.py @@ -55,7 +55,7 @@ def find_sources(self) -> list[str]: class PythonScanner(ProjectScanner): """Scan a directory tree for Python source files.""" - EXCLUDED_DIRS = frozenset({".git", "__pycache__", ".venv", "venv"}) + EXCLUDED_DIRS = frozenset({".git", "__pycache__", ".pixi", ".venv", "venv"}) # TODO: incomplete list, extend this list with more files/directories that should always be ignored def __init__(self, root_dir: str = ".", package_dirs: list[str] | None = None) -> None: diff --git a/src/renaissance/recipes/type_parameter_check.py b/src/renaissance/recipes/type_parameter_check.py new file mode 100644 index 000000000..ceb958bec --- /dev/null +++ b/src/renaissance/recipes/type_parameter_check.py @@ -0,0 +1,371 @@ +# Copyright (C) 2024-2026 TNO +# +# Licensed under the terms specified in the LICENSE file in the root directory +# of this repository, also available at: +# https://github.com/TNO/Renaissance.Py/blob/main/LICENSE + +"""Recipe that modernizes legacy TypeVar/ParamSpec/TypeVarTuple usage to PEP 695 syntax.""" + +from collections.abc import Sequence +from pathlib import Path +from typing import cast + +import libcst as cst + +from renaissance.integrations.python.ast.cst_node import PythonCstNode +from renaissance.recipes.python_refactoring import PythonRefactoring +from renaissance.recipes.step_runner import Step, run_steps +from renaissance.recipes.type_parameter_declarations import ( + declared_default, + find_type_param_declarations, + type_param_constructor_name, + type_param_text, + type_params_bracket, +) +from renaissance.recipes.type_parameter_localization import Localization, plan_localizations +from renaissance.recipes.type_parameter_rules import ( + UnsafeReason, + conversion_refusal, + is_safe_to_remove, + mark_unsafe, +) +from renaissance.recipes.type_parameter_uses import ( + generic_classes, + owning_function, + signature_function, + uses_of, +) +from renaissance.utils.cst_utils import names_read_by, trailing_of_sole_statement +from renaissance.utils.import_resolution import is_import_line + +MIN_SUPPORTED_PYTHON = (3, 12) + + +class TypeParameterCheck(PythonRefactoring[PythonCstNode]): + """Modernize legacy TypeVar/ParamSpec/TypeVarTuple usage in a Python file to PEP 695 syntax. + + Attributes: + min_python: Minimum Python version the target codebase supports (default 3.12, where PEP 695 syntax exists). + project_wide_imported_names: Names other project files import from this file; never removed. + project_root: Root that absolute imports resolve from; the file's own directory if None. + ignore_localize_origins: Absolute paths of files whose declarations are never copied into their importers. + localize_imports: When False, no declaration is copied into the files importing it. + type_param_kinds: Each declared name the phases see, mapped to "TypeVar", "ParamSpec" or "TypeVarTuple". + ignored_names: Type parameter names this file imports from an ignored origin, or from any project file when + localize_imports is False; they keep their import. + cross_file_unsafe_reasons: Why each "unsafe" name of the first phase was refused. + converted_unsafe_reasons: The same for the second phase; also holds "fixed" names a generic class still uses. + orphaned_unsafe_reasons: The same for the third phase. + + """ + + min_python: tuple[int, int] = MIN_SUPPORTED_PYTHON + project_wide_imported_names: frozenset[str] = frozenset() + project_root: Path | None = None + ignore_localize_origins: frozenset[Path] = frozenset() + localize_imports: bool = True + + def __init__(self, file: Path) -> None: + """Prepare the recipe for file, parsed with the CST backend.""" + super().__init__(file, PythonCstNode) + self.type_param_kinds: dict[str, str] = {} + self.ignored_names: set[str] = set() + + def run(self) -> None: + """Run check() and store its result on self.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 check(self) -> dict[str, dict[str, str]]: + """Run the three phases in order, committing each one that fixed something. + + localize_imported_typevars, then convert_declared_typevars, then + remove_orphaned_declarations. Returns {"cross_file": {...}, "converted": {...}, + "orphaned": {...}}, each mapping name -> "fixed" | "unsafe". + """ + return run_steps( + [ + Step("cross_file", self, self.localize_imported_typevars), + Step("converted", self, self.convert_declared_typevars), + Step("orphaned", self, self.remove_orphaned_declarations), + ], + ) + + def convert_declared_typevars(self) -> dict[str, str]: + """Add PEP 695 type parameters to the functions whose signatures use a module-level TypeVar/ParamSpec/TypeVarTuple. + + The declaration stays. Methods of a class generic over the name are left unchanged, and so is a function that + also uses a declaration conversion_refusal rejects, so no function mixes legacy and PEP 695 type parameters. + Bounds, constraints and `default=` are carried over, defaulted ones last. Returns {name: "fixed" | "unsafe"}, + with each UnsafeReason on self.converted_unsafe_reasons; a "fixed" name that a generic class still uses gets + USED_IN_GENERIC_CLASS there too. + """ + root = self._cst_root() + module = root.translation_unit.atu + + results: dict[str, str] = {} + self.converted_unsafe_reasons: dict[str, UnsafeReason] = {} + convertible, generic_classes_by_name, blocked = self._plan_conversions(root, module, results) + + # One self.replace() per function: a function using two converted names would otherwise get two + # rewrites of the same node, which the rewriter rejects as conflicting. + new_params: dict[int, tuple[PythonCstNode, list[tuple[str, bool]]]] = {} + for name, (param, candidates) in convertible.items(): + allowed = [function for function in candidates if id(function) not in blocked] + if not allowed: + mark_unsafe(results, self.converted_unsafe_reasons, name, UnsafeReason.SHARES_FUNCTION_WITH_UNCONVERTIBLE) + continue + for function in allowed: + new_params.setdefault(id(function), (function, []))[1].append(param) + results[name] = "fixed" + + for function, params in new_params.values(): + self._add_type_params(function, params) + + self._report_generic_class_leftovers(results, generic_classes_by_name) + return results + + def _plan_conversions( + self, root: PythonCstNode, module: cst.Module, results: dict[str, str] + ) -> tuple[dict[str, tuple[tuple[str, bool], list[PythonCstNode]]], dict[str, list[PythonCstNode]], set[int]]: + """Return what each declaration that can be converted needs, marking in results the ones that can't. + + Returns {name: ((type parameter text, has default), functions to convert)}, {name: classes generic over it} + and the ids of the functions a refused declaration also uses, which must stay unchanged. + """ + convertible: dict[str, tuple[tuple[str, bool], list[PythonCstNode]]] = {} + generic_classes_by_name: dict[str, list[PythonCstNode]] = {} + blocked: set[int] = set() + for name, decl_stmt in find_type_param_declarations(module).items(): + self.type_param_kinds[name] = type_param_constructor_name(decl_stmt) + uses = uses_of(root, name, decl_stmt) + signature_users = {id(function) for use in uses if (function := signature_function(use)) is not None} + functions = list(dict.fromkeys(function for use in uses if (function := owning_function(use, signature_users)) is not None)) + if not functions: + continue + + classes = generic_classes(uses) + # TODO(FrancescoPezzella): convert a class with the name in a base (`class Box(Base[T])`) to + # `class Box[T](Base[T])`, still refusing `covariant=`. + # https://github.com/TNO/Renaissance.Py/issues/279 + # Maybe ruff's UP046 could take over one day, if it handles bases other than `Generic`: + # https://github.com/astral-sh/ruff/issues/29481 + in_generic_class = [function for function in functions if self._is_inside(function, classes)] + candidates = [function for function in functions if function not in in_generic_class] + if not candidates: + if in_generic_class: + mark_unsafe(results, self.converted_unsafe_reasons, name, UnsafeReason.USED_IN_GENERIC_CLASS) + continue + reason = conversion_refusal(decl_stmt, self.min_python) + if reason is not None: + mark_unsafe(results, self.converted_unsafe_reasons, name, reason) + blocked.update(id(function) for function in candidates) + continue + + convertible[name] = ((type_param_text(decl_stmt, module), declared_default(decl_stmt) is not None), candidates) + generic_classes_by_name[name] = classes + return convertible, generic_classes_by_name, blocked + + def _report_generic_class_leftovers(self, results: dict[str, str], generic_classes_by_name: dict[str, list[PythonCstNode]]) -> None: + """Add USED_IN_GENERIC_CLASS to converted_unsafe_reasons for each "fixed" name a generic class still uses.""" + for name, classes in generic_classes_by_name.items(): + if classes and results.get(name) == "fixed": + self.converted_unsafe_reasons[name] = UnsafeReason.USED_IN_GENERIC_CLASS + + @staticmethod + def _is_inside(node: PythonCstNode, containers: Sequence[PythonCstNode]) -> bool: + """Return True if one of containers encloses node.""" + 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. + + A function without a bracket gets one after its name; an existing bracket is rebuilt with params after its own, + defaulted ones last, which drops its comments and line breaks. + """ + node = cast("cst.FunctionDef", function.node) + unit = function.translation_unit + existing = node.type_parameters + if existing is None: + self.replace( + node.name.value + type_params_bracket(params), unit.wrappers[node.name], include_whitespace=False, include_comments=False + ) + return + # TODO(FrancescoPezzella): rebuilding the existing bracket drops its comments and line breaks. + # https://github.com/TNO/Renaissance.Py/issues/279 + current = [ + (unit.atu.code_for_node(param.with_changes(comma=cst.MaybeSentinel.DEFAULT)), param.default is not None) + for param in existing.params + ] + self.replace(type_params_bracket([*current, *params]), unit.wrappers[existing], include_whitespace=False, include_comments=False) + + def remove_orphaned_declarations(self) -> dict[str, str]: + """Remove every module-level TypeVar/ParamSpec/TypeVarTuple declaration that nothing uses. + + A declaration that is_safe_to_remove rejects is kept and reported "unsafe", with its UnsafeReason on + self.orphaned_unsafe_reasons. Returns {name: "fixed" | "unsafe"}. + """ + root = self._cst_root() + module = root.translation_unit.atu + + results: dict[str, str] = {} + self.orphaned_unsafe_reasons: dict[str, UnsafeReason] = {} + for name, decl_stmt in find_type_param_declarations(module).items(): + self.type_param_kinds[name] = type_param_constructor_name(decl_stmt) + if uses_of(root, name, decl_stmt): + continue + + reason = is_safe_to_remove(module, name, self.project_wide_imported_names) + if reason is not None: + mark_unsafe(results, self.orphaned_unsafe_reasons, name, reason) + continue + + self._remove_declaration(decl_stmt) + results[name] = "fixed" + + return results + + def _remove_declaration(self, decl_stmt: cst.Assign) -> None: + """Remove decl_stmt's statement from the file, with the comment trailing it when it is alone on its line.""" + unit = self._cst_root().translation_unit + targets: list[cst.CSTNode] = [decl_stmt] + trailing = trailing_of_sole_statement(unit.atu, decl_stmt) + if trailing is not None and trailing.comment is not None: + targets.append(trailing.comment) + # TODO(FrancescoPezzella): enable once comment blocks get correctly deleted + # https://github.com/TNO/Renaissance.Py/issues/189 + self.remove([unit.wrappers[target] for target in targets], include_comments=False) + + def localize_imported_typevars(self) -> dict[str, str]: + """Replace imports of TypeVar/ParamSpec/TypeVarTuple names from the target project with local declarations. + + Imports are resolved against project_root. Where safe, the import is dropped and a copy of the origin's + declaration, plus the imports it needs, is added after the file's last import. Names from an + ignore_localize_origins file, or all names when localize_imports is False, keep their import and go to + self.ignored_names. Returns {name: "fixed" | "unsafe"}, with each UnsafeReason on + self.cross_file_unsafe_reasons. + + Raises: + SyntaxError: if an origin file can't be parsed. + + """ + project_root = self.project_root if self.project_root is not None else Path(self.filename).parent + plan = plan_localizations( + self._cst_root().translation_unit.atu, + Path(self.filename), + project_root, + localize_imports=self.localize_imports, + ignore_origins=self.ignore_localize_origins, + ) + self.cross_file_unsafe_reasons: dict[str, UnsafeReason] = plan.unsafe_reasons + self.type_param_kinds.update(plan.kinds) + self.ignored_names |= plan.ignored_names + self._place_localizations(plan.localizations) + return plan.results + + def _place_localizations(self, localizations: list[Localization]) -> None: + """Write the localizations: narrow or drop each import, then add the needed imports and declarations after the last one. + + Each needed import is added once, before the declarations; if every import is dropped, they take the place of the + last one. A localization is written in place of its import instead when that import shares a line with other + statements and would be dropped, when no import remains to go after, or when a statement in between reads one + of its names. + """ + body = self._cst_root().translation_unit.atu.body + wrappers = self._cst_root().translation_unit.wrappers + dropped = {id(item.line) for item in localizations if not item.kept} + anchor, replaces_anchor = self._anchor_index(body, dropped) + if anchor is None: + for item in localizations: + self._write_in_place(item) + return + + imports: list[str] = [] + declarations: list[str] = [] + anchor_raw: cst.ImportFrom | None = None + for item in localizations: + if not self._goes_after_last_import(item, body, anchor): + self._write_in_place(item) + continue + imports.extend(item.imports) + declarations.extend(item.declarations) + if replaces_anchor and item.line is body[anchor]: + anchor_raw = item.raw + elif item.kept: + self._narrow(item) + else: + self._drop(item) + + if not declarations: + return + text = "\n".join([*dict.fromkeys(imports), *declarations]) + if anchor_raw is not None: + self.replace(text, wrappers[anchor_raw], include_whitespace=False, include_comments=False) + else: + self.insert_after(f"\n{text}", wrappers[body[anchor]], include_whitespace=False, include_comments=True) + + def _anchor_index(self, body: Sequence[cst.BaseStatement], dropped: set[int]) -> tuple[int | None, bool]: + """Return the index in body of the statement that localized text goes after, and whether it replaces that statement. + + The anchor is the last import line that stays. If every import is dropped, it is the last dropped line holding + a single statement, whose place the text takes. The index is None when body has neither. + """ + anchor = max( + (index for index, statement in enumerate(body) if is_import_line(statement) and id(statement) not in dropped), + default=None, + ) + if anchor is not None: + return anchor, False + anchor = max( + ( + index + for index, statement in enumerate(body) + if id(statement) in dropped and isinstance(statement, cst.SimpleStatementLine) and len(statement.body) == 1 + ), + default=None, + ) + return anchor, True + + def _write_in_place(self, item: Localization) -> None: + """Replace the names of item where its import is: its needed imports and declarations follow the kept names, if any.""" + wrappers = self._cst_root().translation_unit.wrappers + text = "\n".join([*item.imports, *item.declarations]) + if not item.kept: + self.replace(text, wrappers[item.raw], include_whitespace=False, include_comments=False) + return + self._narrow(item) + self.insert_after(f"\n{text}", wrappers[item.line], include_whitespace=False, include_comments=True) + + def _drop(self, item: Localization) -> None: + """Remove the import of item, with the comment trailing it, from its line.""" + wrappers = self._cst_root().translation_unit.wrappers + targets = [wrappers[item.raw]] + comment = item.line.trailing_whitespace.comment + if comment is not None: + targets.append(wrappers[comment]) + self.remove(targets, include_comments=False) + + def _narrow(self, item: Localization) -> None: + """Replace the import of item with one that only imports its kept names, keeping its comments.""" + module = self._cst_root().translation_unit.atu + narrowed = module.code_for_node(item.raw.with_changes(names=item.kept, semicolon=cst.MaybeSentinel.DEFAULT)) + self.replace(narrowed, self._cst_root().translation_unit.wrappers[item.raw], include_whitespace=False, include_comments=False) + + def _goes_after_last_import(self, item: Localization, body: Sequence[cst.BaseStatement], anchor: int) -> bool: + """Return True if item can be written after body[anchor], the last import that stays. + + It can't if its import would be dropped from a line with other statements, or if a non-import statement between + that import and the anchor reads one of its names. + """ + if not item.kept and len(item.line.body) != 1: + return False + index = next(position for position, statement in enumerate(body) if statement is item.line) + return not any(not is_import_line(statement) and item.names & names_read_by(statement) for statement in body[index + 1 : anchor]) diff --git a/src/renaissance/recipes/type_parameter_declarations.py b/src/renaissance/recipes/type_parameter_declarations.py new file mode 100644 index 000000000..88992444a --- /dev/null +++ b/src/renaissance/recipes/type_parameter_declarations.py @@ -0,0 +1,156 @@ +# 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 + +"""Read and rewrite the declaration of a TypeVar/ParamSpec/TypeVarTuple: `NAME = TypeVar(...)`.""" + +import builtins +from collections.abc import Iterable +from typing import cast + +import libcst as cst + +from renaissance.utils.cst_utils import module_statements, names_read_by, trailing_of_sole_statement + +_CONSTRUCTORS = frozenset({"TypeVar", "ParamSpec", "TypeVarTuple"}) + + +def _is_type_param_call(value: cst.BaseExpression) -> bool: + """Return True if `value` is a call to TypeVar/ParamSpec/TypeVarTuple.""" + return isinstance(value, cst.Call) and isinstance(value.func, cst.Name) and value.func.value in _CONSTRUCTORS + + +def find_type_param_declarations(module: cst.Module) -> dict[str, cst.Assign]: + """Find every module-level "NAME = TypeVar/ParamSpec/TypeVarTuple(...)" declaration.""" + declarations: dict[str, cst.Assign] = {} + for statement in module_statements(module): + if isinstance(statement, cst.Assign) and _is_type_param_call(statement.value): + for target in statement.targets: + if isinstance(target.target, cst.Name): + declarations[target.target.value] = statement + return declarations + + +def declaration_text(module: cst.Module, decl_stmt: cst.Assign) -> str: + """Return decl_stmt as written in module, without its `;`. + + The comment trailing its line follows it when it is the only statement there, since on a shared + line the comment may belong to another statement. + """ + text = module.code_for_node(decl_stmt.with_changes(semicolon=cst.MaybeSentinel.DEFAULT)) + trailing = trailing_of_sole_statement(module, decl_stmt) + if trailing is not None and trailing.comment is not None: + text += module.code_for_node(trailing.whitespace) + module.code_for_node(trailing.comment) + return text + + +def _call(decl_stmt: cst.Assign) -> cst.Call: + """Return the TypeVar/ParamSpec/TypeVarTuple call a declaration assigns.""" + return cast("cst.Call", decl_stmt.value) + + +def _positional_arguments(call: cst.Call) -> list[cst.Arg]: + """Return the call's positional arguments, `*args` included.""" + return [argument for argument in call.args if argument.keyword is None and argument.star != "**"] + + +def _keyword_value(call: cst.Call, keyword: str) -> cst.BaseExpression | None: + """Return the value the call passes as `keyword=`, or None if it passes none.""" + return next((argument.value for argument in call.args if argument.keyword is not None and argument.keyword.value == keyword), None) + + +def type_param_constructor_name(decl_stmt: cst.Assign) -> str: + """Return the name of the call a declaration uses, e.g. "TypeVar" for `T = TypeVar("T")`.""" + return cast("cst.Name", _call(decl_stmt).func).value + + +def declaration_argument_names(decl_stmt: cst.Assign) -> set[str]: + """Return the non-builtin names a declaration's arguments use, e.g. {"Shape"} for `TypeVar("T", bound=Shape)`. + + The first positional argument (the type parameter's own name) is skipped; a string argument is read as a + forward reference (`bound="Shape"`). A string that isn't a valid expression contributes no names. + """ + call = _call(decl_stmt) + keyword_arguments = [argument for argument in call.args if argument.keyword is not None or argument.star == "**"] + names: set[str] = set() + for argument in [*_positional_arguments(call)[1:], *keyword_arguments]: + expression = argument.value + if isinstance(expression, cst.SimpleString | cst.ConcatenatedString) and isinstance(text := expression.evaluated_value, str): + try: + expression = cst.parse_expression(text) + except cst.ParserSyntaxError: + continue + names |= names_read_by(expression) + return names - set(dir(builtins)) + + +# Variance needs no syntax of its own: a PEP 695 type parameter always infers it, and a function's has none. +_CONVERTIBLE_KEYWORDS: dict[str, frozenset[str]] = { + "TypeVar": frozenset({"bound", "default", "infer_variance", "covariant", "contravariant"}), + "ParamSpec": frozenset({"default", "infer_variance", "covariant", "contravariant"}), + # (Py 3.15) TypeVarTuple now accepts bound, covariant, contravariant, and infer_variance keyword arguments, + # matching the interface of TypeVar and ParamSpec. bound semantics remain undefined in the specification. + "TypeVarTuple": frozenset({"default", "infer_variance"}), +} + + +def declared_default(decl_stmt: cst.Assign) -> cst.BaseExpression | None: + """Return the value a legacy declaration passes as `default=` (PEP 696), or None if it passes none.""" + return _keyword_value(_call(decl_stmt), "default") + + +def has_unconvertible_argument(decl_stmt: cst.Assign) -> bool: + """Return True if a legacy declaration passes a keyword type_param_text can't carry over. + + That is any keyword outside the constructor's known set, e.g. `bound=` on a ParamSpec or TypeVarTuple, + a keyword added by a later Python version, or a `**mapping`. + """ + convertible = _CONVERTIBLE_KEYWORDS[type_param_constructor_name(decl_stmt)] + return any( + argument.star == "**" or (argument.keyword is not None and argument.keyword.value not in convertible) + for argument in _call(decl_stmt).args + ) + + +def type_param_text(decl_stmt: cst.Assign, module: cst.Module) -> str: + """Return the PEP 695 type parameter equivalent to a legacy declaration from `module`, in its original formatting. + + E.g. `T = TypeVar("T", bound=int, default=int)` gives `T: int = int`. Bounds, constraints and + `default=` are carried over; `infer_variance=`, `covariant=` and `contravariant=` are dropped. Callers must + first reject declarations for which has_unconvertible_argument() is True. + """ + call = _call(decl_stmt) + constructor = type_param_constructor_name(decl_stmt) + 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 ''})" + + default = declared_default(decl_stmt) + if default is not None: + text += f" = {module.code_for_node(default)}" + return text + + +def type_params_bracket(params: Iterable[tuple[str, bool]]) -> str: + """Return the `[...]` bracket for type parameters given as (text, has_default), with the defaulted ones last. + + The relative order within each group is kept. A non-default type parameter following a defaulted + one is a SyntaxError. + """ + ordered = sorted(params, key=lambda param: param[1]) + return f"[{', '.join(text for text, _ in ordered)}]" diff --git a/src/renaissance/recipes/type_parameter_localization.py b/src/renaissance/recipes/type_parameter_localization.py new file mode 100644 index 000000000..682141bb6 --- /dev/null +++ b/src/renaissance/recipes/type_parameter_localization.py @@ -0,0 +1,281 @@ +# 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 + +"""Decide which TypeVar/ParamSpec/TypeVarTuple names a file imports from its project get a local declaration instead.""" + +from collections.abc import Sequence +from dataclasses import dataclass, field +from pathlib import Path +from typing import cast + +import libcst as cst + +from renaissance.recipes.type_parameter_declarations import ( + declaration_argument_names, + declaration_text, + find_type_param_declarations, + type_param_constructor_name, +) +from renaissance.recipes.type_parameter_rules import UnsafeReason, is_safe_to_localize, mark_unsafe +from renaissance.utils.cst_utils import module_statements +from renaissance.utils.import_resolution import ( + from_import_binding, + from_import_sources, + import_from_module, + import_from_text, + imported_name, + rebase_relative_module, + resolve_project_module, +) + + +@dataclass(frozen=True) +class Localization: + """The type parameter names one import statement brings in that are localized, and the text that replaces them.""" + + line: cst.SimpleStatementLine + raw: cst.ImportFrom + names: frozenset[str] + kept: list[cst.ImportAlias] + imports: list[str] + declarations: list[str] + + +@dataclass +class LocalizationPlan: + """What plan_localizations decides for one file. + + Attributes: + localizations: What to write for each import that has names to localize. + results: Each name seen, mapped to "fixed" or "unsafe". + unsafe_reasons: Why each "unsafe" name was refused. + kinds: Each name seen, mapped to "TypeVar", "ParamSpec" or "TypeVarTuple". + ignored_names: The names kept as imports because their origin is ignored or localization is off. + + """ + + localizations: list[Localization] = field(default_factory=list[Localization]) + results: dict[str, str] = field(default_factory=dict[str, str]) + unsafe_reasons: dict[str, UnsafeReason] = field(default_factory=dict[str, UnsafeReason]) + kinds: dict[str, str] = field(default_factory=dict[str, str]) + ignored_names: set[str] = field(default_factory=set[str]) + + +def plan_localizations( + module: cst.Module, + filename: Path, + project_root: Path, + *, + localize_imports: bool, + ignore_origins: frozenset[Path], +) -> LocalizationPlan: + """Return the plan deciding which type parameters imported by module's `from X import ...` statements to localize. + + X is resolved against project_root from filename; only imports that resolve to a project file are considered. + Names from a file in ignore_origins, or from any file when localize_imports is False, keep their import and go to + the plan's ignored_names. `import *` and aliased names are skipped. + + Raises: + SyntaxError: if an origin file can't be parsed, with its path as the filename. + + """ + return _Planner(module, filename, project_root, localize_imports, ignore_origins).run() + + +@dataclass(frozen=True) +class _Planner: + """Decide the localizations of one file, whose parsed module is `module` and path is `filename`.""" + + module: cst.Module + filename: Path + project_root: Path + localize_imports: bool + ignore_origins: frozenset[Path] + + def run(self) -> LocalizationPlan: + """Return the plan for every project import of the module.""" + plan = LocalizationPlan() + for line in self.module.body: + if not isinstance(line, cst.SimpleStatementLine): + continue + for statement in line.body: + if not isinstance(statement, cst.ImportFrom): + continue + origin_path = resolve_project_module( + self.filename, self.project_root, import_from_module(statement), len(statement.relative) + ) + if origin_path is None: + continue + self._localize_names_from(plan, line, statement, origin_path) + return plan + + def _localize_names_from(self, plan: LocalizationPlan, line: cst.SimpleStatementLine, raw: cst.ImportFrom, origin_path: Path) -> None: + """Queue the localization of every safe type parameter name that raw, on line, imports from the origin_path file. + + Records each name in the plan's results as "fixed" or, via mark_unsafe, as "unsafe", its kind in kinds, and + appends what to write for the "fixed" ones to localizations. + """ + if isinstance(raw.names, cst.ImportStar): + return + origin = _parse_origin(origin_path) + declarations = find_type_param_declarations(origin) + localized: dict[str, cst.Assign] = {} + needed_imports: list[str] = [] + + for alias in raw.names: + name = alias.evaluated_name + if alias.asname is not None or name not in declarations: + continue + + decl_stmt = declarations[name] + plan.kinds[name] = type_param_constructor_name(decl_stmt) + if not self.localize_imports or origin_path in self.ignore_origins: + plan.ignored_names.add(name) + continue + reason = is_safe_to_localize(origin, name) + if reason is not None: + mark_unsafe(plan.results, plan.unsafe_reasons, name, reason) + continue + argument_imports = self._argument_imports(origin, origin_path, raw, decl_stmt) + if argument_imports is None: + mark_unsafe(plan.results, plan.unsafe_reasons, name, UnsafeReason.NAME_NOT_AT_MODULE_LEVEL) + continue + + constructor_import = self._missing_constructor_import(origin, origin_path, raw, decl_stmt) + for needed in [constructor_import, *argument_imports]: + if needed is not None and needed not in needed_imports: + needed_imports.append(needed) + localized[name] = decl_stmt + plan.results[name] = "fixed" + + if not localized: + return + plan.localizations.append(_localization(line, raw, origin, localized, needed_imports)) + + def _missing_constructor_import( + self, + origin: cst.Module, + origin_path: Path, + via: cst.ImportFrom, + decl_stmt: cst.Assign, + ) -> str | None: + """Return the "from module import Ctor" line the localized declaration needs, or None if it isn't needed. + + None when this file already imports the constructor from that module, or the origin doesn't import it. The + module is the one the origin imports the constructor from, as named from this file, which reaches origin_path + through via; if this file can't name it, the origin module itself. + """ + ctor_name = type_param_constructor_name(decl_stmt) + ctor_source = from_import_sources(origin).get(ctor_name) + if ctor_source is None: + return None + + via_module = import_from_module(via) + ctor_module = rebase_relative_module(via_module, len(via.relative), origin_path, ctor_source[0] or None, ctor_source[1]) + ctor_module = ctor_module or import_from_text(via) + + # TODO(FrancescoPezzella): the constructor already imported here from another module (e.g. `typing` vs the origin's + # `typing_extensions`) isn't matched, so a second, shadowing import of the same name is added. + # https://github.com/TNO/Renaissance.Py/issues/279 + for statement in module_statements(self.module): + if ( + isinstance(statement, cst.ImportFrom) + and not isinstance(statement.names, cst.ImportStar) + and 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_imports( + self, + origin: cst.Module, + origin_path: Path, + raw: cst.ImportFrom, + decl_stmt: cst.Assign, + ) -> list[str] | None: + """Return the import lines, sorted by name, for the names decl_stmt's arguments use that this file doesn't import yet. + + A name already imported from the origin module, or from the module the origin gets it from, needs no line. + Returns None if a name isn't available as the same object at module level: the origin doesn't bind it at + runtime (e.g. only under `if TYPE_CHECKING:`), or this file binds it to something else. + """ + origin_sources = from_import_sources(origin) + importing_sources = from_import_sources(self.module) + imports: list[str] = [] + for name in sorted(declaration_argument_names(decl_stmt)): + if name not in origin_sources: + return None + if name not in importing_sources: + imports.append(_argument_import(origin, origin_path, raw, name)) + continue + origin_source = origin_sources[name] + accepted = {(import_from_module(raw) or "", len(raw.relative))} + if origin_source is not None and origin_source[1] == 0: + accepted.add(origin_source) + if importing_sources[name] not in accepted: + return None + return imports + + +def _parse_origin(origin_path: Path) -> cst.Module: + """Return the parsed origin_path file. + + Raises: + SyntaxError: if the file can't be parsed, with origin_path as its filename. + + """ + try: + return cst.parse_module(origin_path.read_text(encoding="utf-8")) + except cst.ParserSyntaxError as error: + raise SyntaxError(error.message, (str(origin_path), error.raw_line, error.raw_column + 1, None)) from error + + +def _argument_import(origin: cst.Module, origin_path: Path, via: cst.ImportFrom, name: str) -> str: + """Return the import line for name, which a localized declaration uses and the origin binds at module level. + + A name the origin imports is imported from the module it gets it from, as named from this file, which + reaches the origin (origin_path) through via. Any other name, or one this file can't name the module + of, is imported from the origin module itself. + """ + binding = from_import_binding(origin, name) + if binding is not None: + statement, alias = binding + module = rebase_relative_module( + import_from_module(via), + len(via.relative), + origin_path, + import_from_module(statement) or None, + len(statement.relative), + ) + if module is not None: + imported = alias.evaluated_name + return f"from {module} import {name if imported == name else f'{imported} as {name}'}" + return f"from {import_from_text(via)} import {name}" + + +def _localization( + line: cst.SimpleStatementLine, + raw: cst.ImportFrom, + origin: cst.Module, + declarations: dict[str, cst.Assign], + needed_imports: list[str], +) -> Localization: + """Return what replaces the names of declarations that the import raw, on line, brings in. + + The declarations keep their original formatting and the comment trailing them. needed_imports are the + imports they need (their constructors, names their arguments use) that this file doesn't have yet. The + kept aliases are the names raw imports that are not localized. + """ + kept = [alias for alias in cast("Sequence[cst.ImportAlias]", raw.names) if imported_name(alias) not in declarations] + if kept and raw.rpar is None: + # Without parentheses, a trailing comma after the last name is a syntax error. + kept[-1] = kept[-1].with_changes(comma=cst.MaybeSentinel.DEFAULT) + # TODO(FrancescoPezzella): the comment lines above a declaration are not copied: the rewriter only handles a one-line comment. + # https://github.com/TNO/Renaissance.Py/issues/189 + texts = [declaration_text(origin, decl) for decl in declarations.values()] + return Localization(line, raw, frozenset(declarations), kept, needed_imports, texts) diff --git a/src/renaissance/recipes/type_parameter_rules.py b/src/renaissance/recipes/type_parameter_rules.py new file mode 100644 index 000000000..066cd21ea --- /dev/null +++ b/src/renaissance/recipes/type_parameter_rules.py @@ -0,0 +1,184 @@ +# 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 + +"""The reasons a TypeVar/ParamSpec/TypeVarTuple is left unconverted, and the rules that decide them.""" + +from collections.abc import Callable, Iterable +from dataclasses import dataclass +from enum import StrEnum + +import libcst as cst +from libcst.codemod import CodemodContext +from libcst.codemod.visitors import GatherExportsVisitor + +from renaissance.recipes.type_parameter_declarations import ( + declared_default, + find_type_param_declarations, + has_unconvertible_argument, + type_param_constructor_name, +) +from renaissance.utils.import_resolution import imports_name_conditionally + +PEP_696_MINIMUM = (3, 13) +DOCS_BASE_URL = "https://tno.github.io/Renaissance.Py/user/features/type-parameter-modernization/" + + +class UnsafeReason(StrEnum): + """Every distinct, permanent reason a TypeVar/ParamSpec/TypeVarTuple candidate is left unconverted. + + Each member has a matching documented rule under DOCS_BASE_URL - see UNSAFE_RULES. + """ + + PEP696_VERSION_GATE = "pep696_version_gate" + NO_PEP695_EQUIVALENT = "no_pep695_equivalent" + DECLARED_TYPEVAR_EXPORTED = "declared_typevar_exported" + USED_IN_GENERIC_CLASS = "used_in_generic_class" + SHARES_FUNCTION_WITH_UNCONVERTIBLE = "shares_function_with_unconvertible" + NAME_NOT_AT_MODULE_LEVEL = "name_not_at_module_level" + IMPORTED_ELSEWHERE_IN_PROJECT = "imported_elsewhere_in_project" + ORIGIN_IMPORTS_CONSTRUCTOR_CONDITIONALLY = "origin_imports_constructor_conditionally" + + +@dataclass(frozen=True) +class UnsafeRule: + """A short human-readable explanation plus the docs anchor slug for one UnsafeReason.""" + + message: str + doc_anchor: str + + +UNSAFE_RULES: dict[UnsafeReason, UnsafeRule] = { + UnsafeReason.PEP696_VERSION_GATE: UnsafeRule( + "declares a default= (PEP 696), which needs the target's minimum Python to be 3.13+", + "feature-type-parameter-modernization-pep696-version-gate", + ), + UnsafeReason.NO_PEP695_EQUIVALENT: UnsafeRule( + "declaration passes an argument with no PEP 695 equivalent, e.g. bound= on a ParamSpec or a **mapping", + "feature-type-parameter-modernization-no-pep695-equivalent", + ), + UnsafeReason.DECLARED_TYPEVAR_EXPORTED: UnsafeRule( + "exported via __all__", + "feature-type-parameter-modernization-declared-type-parameter-exported", + ), + UnsafeReason.USED_IN_GENERIC_CLASS: UnsafeRule( + "only used inside a class that is generic over it", + "feature-type-parameter-modernization-used-in-generic-class", + ), + UnsafeReason.SHARES_FUNCTION_WITH_UNCONVERTIBLE: UnsafeRule( + "every function using it also uses a type variable that can't be converted", + "feature-type-parameter-modernization-shares-function-with-unconvertible", + ), + UnsafeReason.NAME_NOT_AT_MODULE_LEVEL: UnsafeRule( + "declaration uses a name that isn't available at module level as the same object", + "feature-type-parameter-modernization-name-not-at-module-level", + ), + UnsafeReason.IMPORTED_ELSEWHERE_IN_PROJECT: UnsafeRule( + "imported directly by another file in the target project", + "feature-type-parameter-modernization-imported-elsewhere-in-project", + ), + UnsafeReason.ORIGIN_IMPORTS_CONSTRUCTOR_CONDITIONALLY: UnsafeRule( + "origin module imports TypeVar/ParamSpec/TypeVarTuple inside an if/try block, e.g. under TYPE_CHECKING or per Python version", + "feature-type-parameter-modernization-origin-imports-constructor-conditionally", + ), +} + + +@dataclass(frozen=True) +class RefusalRule[**P]: + """One reason to refuse a type parameter, and the check that decides whether it applies. + + Attributes: + reason: What the report says when the check refuses. + refuses: Return True if the reason applies to the arguments the rule's stage passes. + + """ + + reason: UnsafeReason + refuses: Callable[P, bool] + + +def _first_refusal[**P](rules: Iterable[RefusalRule[P]], *args: P.args, **kwargs: P.kwargs) -> UnsafeReason | None: + """Return the reason of the first rule that refuses, or None if none does.""" + return next((rule.reason for rule in rules if rule.refuses(*args, **kwargs)), None) + + +def _has_no_pep695_equivalent(declaration: cst.Assign, _min_python: tuple[int, int]) -> bool: + """Return True if the declaration passes an argument that PEP 695 syntax can't carry over.""" + return has_unconvertible_argument(declaration) + + +def _needs_newer_python(declaration: cst.Assign, min_python: tuple[int, int]) -> bool: + """Return True if the declaration has a `default=` (PEP 696) and min_python is below the 3.13 that introduced it.""" + return declared_default(declaration) is not None and min_python < PEP_696_MINIMUM + + +def _is_exported(module: cst.Module, name: str, _project_wide_imported_names: frozenset[str]) -> bool: + """Return True if the module lists `name` in `__all__`, assigned, annotated or extended with `+=`. + + `__all__.extend(...)` and `__all__.append(...)` are not read. + """ + exports = GatherExportsVisitor(CodemodContext()) + module.visit(exports) + return name in exports.explicit_exported_objects + + +def _is_imported_elsewhere(_module: cst.Module, name: str, project_wide_imported_names: frozenset[str]) -> bool: + """Return True if another file in the target project imports `name` directly, regardless of `__all__`.""" + return name in project_wide_imported_names + + +def _origin_imports_constructor_conditionally(origin: cst.Module, name: str) -> bool: + """Return True if the origin imports the constructor of `name` inside a module-level `if`/`try` block. + + Which implementation the constructor binds then depends on the runtime or the type checker, e.g. + `typing_extensions` below 3.13 or under `TYPE_CHECKING`. + """ + declaration = find_type_param_declarations(origin).get(name) + return declaration is not None and imports_name_conditionally(origin, type_param_constructor_name(declaration)) + + +# Each stage reports the first rule that refuses, so the order is the order of precedence. Commenting out +# a rule switches it off; the rules a pipeline step enforces by its own structure are not listed here +# (USED_IN_GENERIC_CLASS, SHARES_FUNCTION_WITH_UNCONVERTIBLE, NAME_NOT_AT_MODULE_LEVEL). +CONVERSION_RULES: tuple[RefusalRule[[cst.Assign, tuple[int, int]]], ...] = ( + RefusalRule(UnsafeReason.NO_PEP695_EQUIVALENT, _has_no_pep695_equivalent), + RefusalRule(UnsafeReason.PEP696_VERSION_GATE, _needs_newer_python), +) +REMOVAL_RULES: tuple[RefusalRule[[cst.Module, str, frozenset[str]]], ...] = ( + RefusalRule(UnsafeReason.DECLARED_TYPEVAR_EXPORTED, _is_exported), + RefusalRule(UnsafeReason.IMPORTED_ELSEWHERE_IN_PROJECT, _is_imported_elsewhere), +) +LOCALIZATION_RULES: tuple[RefusalRule[[cst.Module, str]], ...] = ( + RefusalRule(UnsafeReason.ORIGIN_IMPORTS_CONSTRUCTOR_CONDITIONALLY, _origin_imports_constructor_conditionally), +) + + +def conversion_refusal(declaration: cst.Assign, min_python: tuple[int, int]) -> UnsafeReason | None: + """Return None if `declaration` can become a PEP 695 type parameter on a target with `min_python`, else why not.""" + return _first_refusal(CONVERSION_RULES, declaration, min_python) + + +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.""" + return _first_refusal(LOCALIZATION_RULES, origin, name) + + +def is_safe_to_remove( + module: cst.Module, + name: str, + project_wide_imported_names: frozenset[str] = frozenset(), +) -> UnsafeReason | None: + """Return None if this file's own binding of `name` may be removed or replaced, else the reason it may not. + + `project_wide_imported_names` holds the names other files of the target project import from this one. + """ + return _first_refusal(REMOVAL_RULES, module, name, project_wide_imported_names) + + +def mark_unsafe(results: dict[str, str], reasons: dict[str, UnsafeReason], name: str, reason: UnsafeReason) -> None: + """Record `name` as unsafe with `reason` in both `results` (status) and `reasons` (why).""" + results[name] = "unsafe" + reasons[name] = reason diff --git a/src/renaissance/recipes/type_parameter_uses.py b/src/renaissance/recipes/type_parameter_uses.py new file mode 100644 index 000000000..7abb26310 --- /dev/null +++ b/src/renaissance/recipes/type_parameter_uses.py @@ -0,0 +1,67 @@ +# Copyright (C) 2024-2026 TNO +# +# Licensed under the terms specified in the LICENSE file in the root directory +# of this repository, also available at: +# https://github.com/TNO/Renaissance.Py/blob/main/LICENSE + +"""Find where a module-level TypeVar/ParamSpec/TypeVarTuple name is used, with libcst's scope analysis.""" + +from collections.abc import Iterable + +import libcst as cst + +from renaissance.integrations.python.ast.cst_node import PythonCstNode + + +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 signature_function(use: PythonCstNode) -> PythonCstNode | None: + """Return the function whose parameter or return annotation contains use, or None if no signature does. + + A use in a function's body, a parameter default or a decorator is in no signature. + """ + in_annotation = False + child, current = use, use.parent + while current is not None: + if isinstance(current.node, cst.Annotation): + in_annotation = True + elif isinstance(current.node, cst.FunctionDef): + return current if in_annotation and child.node is not current.node.body else None + child, current = current, current.parent + return None + + +def owning_function(node: PythonCstNode, signature_users: set[int]) -> PythonCstNode | None: + """Return the outermost function enclosing node whose id is in signature_users, or None if there is none.""" + owner = None + current = node.parent + while current is not None: + if isinstance(current.node, cst.FunctionDef) and id(current) in signature_users: + owner = current + current = current.parent + return owner + + +def generic_classes(uses: Iterable[PythonCstNode]) -> list[PythonCstNode]: + """Return the classes that have one of `uses` in a base (`Generic[T]`, `Protocol[T]`, `Base[int, T]`). + + Such a class is generic over the used name. + """ + classes: list[PythonCstNode] = [] + for use in uses: + child, current = use, use.parent + while current is not None: + if isinstance(current.node, cst.ClassDef) and any(child.node is base for base in current.node.bases): + if current not in classes: + classes.append(current) + break + child, current = current, current.parent + return classes diff --git a/src/renaissance/recipes/type_var_check.py b/src/renaissance/recipes/type_var_check.py deleted file mode 100644 index d322b05e9..000000000 --- a/src/renaissance/recipes/type_var_check.py +++ /dev/null @@ -1,413 +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 - -"""Recipe that modernizes legacy TypeVar/ParamSpec/TypeVarTuple usage to PEP 695 syntax.""" - -from collections.abc import Sequence -from pathlib import Path -from typing import cast - -import libcst as cst - -from renaissance.integrations.python.ast.cst_node import PythonCstNode -from renaissance.recipes.python_refactoring import PythonRefactoring -from renaissance.recipes.step_runner import Step, run_steps -from renaissance.recipes.type_var_domain import ( - UnsafeReason, - declaration_argument_names, - declared_default, - declares_type_param, - find_type_param_declarations, - from_import_sources, - generic_classes, - has_unconvertible_argument, - is_safe_to_localize, - is_safe_to_remove, - module_statements, - outermost_function, - type_param_constructor_name, - type_param_text, - type_params_bracket, - uses_of, -) -from renaissance.utils.import_resolution import import_from_module, imported_name, rebase_relative_module, resolve_project_module - -PEP_695_MINIMUM = (3, 12) -PEP_696_MINIMUM = (3, 13) - - -class TypeVarCheck(PythonRefactoring[PythonCstNode]): - """Modernize legacy TypeVar/ParamSpec/TypeVarTuple usage in a Python file to PEP 695 syntax. - - See check() for the three phases this runs, in order. - """ - - # Minimum Python version the target codebase supports; None means unknown. - min_python: tuple[int, int] | None = None - - # Names other files in the target project import directly from this file; never removed. - project_wide_imported_names: frozenset[str] = frozenset() - - # Root that absolute imports resolve from; None falls back to this file's own directory. - project_root: Path | None = None - - def __init__(self, file: Path) -> None: - """Prepare the recipe for file, parsed with the CST backend.""" - super().__init__(file, PythonCstNode) - - def run(self) -> None: - """Entry point called by PythonRefactoring.process(); stores check()'s result.""" - self.result = self.check() - - def _cst_root(self) -> PythonCstNode: - """Return the root node of the file as it currently is (after the last commit).""" - return cast("PythonCstNode", cast("object", self.root)) - - def _target_supports_pep695(self) -> bool: - """Return True only if min_python is known and is 3.12+, where PEP 695 syntax (`def f[T](...)`) exists.""" - return self.min_python is not None and self.min_python >= PEP_695_MINIMUM - - def _target_supports_pep696(self) -> bool: - """Return True only if min_python is known and is 3.13+, where PEP 696 defaults (`def f[T = int](...)`) exist.""" - return self.min_python is not None and self.min_python >= PEP_696_MINIMUM - - def _conversion_refusal(self, decl_stmt: cst.Assign) -> UnsafeReason | None: - """Return why decl_stmt can't be converted to a PEP 695 type parameter on this target, or None if it can.""" - if not self._target_supports_pep695(): - return UnsafeReason.PEP695_VERSION_GATE - if has_unconvertible_argument(decl_stmt): - return UnsafeReason.NO_PEP695_EQUIVALENT - if declared_default(decl_stmt) is not None and not self._target_supports_pep696(): - return UnsafeReason.PEP696_VERSION_GATE - return None - - def check(self) -> dict[str, dict[str, str]]: - """Run the three phases in order, committing each one that fixed something. - - localize_imported_typevars, then convert_declared_typevars, then - remove_orphaned_declarations. Returns {"cross_file": {...}, "converted": {...}, - "orphaned": {...}}, each mapping name -> "fixed" | "unsafe". - """ - return run_steps( - [ - Step("cross_file", self, self.localize_imported_typevars), - Step("converted", self, self.convert_declared_typevars), - Step("orphaned", self, self.remove_orphaned_declarations), - ], - ) - - def convert_declared_typevars(self) -> dict[str, str]: - """Add a PEP 695 type parameter to every function using a module-level TypeVar/ParamSpec/TypeVarTuple. - - The declaration is left in place. A use is attributed to the outermost function containing it; a `T` - that a same-named PEP 695 type parameter of a function or class declares is not a use. Methods of a - class generic over the name are never converted. Bounds, constraints and `default=` are carried over - in their original formatting, with defaulted type parameters placed last. A name only such methods - use, or whose declaration _conversion_refusal rejects, is reported "unsafe", with its UnsafeReason on - self.converted_unsafe_reasons. Returns {name: "fixed" | "unsafe"}. - """ - root = self._cst_root() - module = root.translation_unit.atu - - results: dict[str, str] = {} - self.converted_unsafe_reasons: dict[str, UnsafeReason] = {} - # One self.replace() per function: a function using two converted names would otherwise get two - # rewrites of the same node, which the rewriter rejects as conflicting. - new_params: dict[int, tuple[PythonCstNode, list[tuple[str, bool]]]] = {} - for name, decl_stmt in find_type_param_declarations(module).items(): - uses = uses_of(root, name, decl_stmt) - functions = list(dict.fromkeys(function for use in uses if (function := outermost_function(use)) is not None)) - if not functions: - continue - - classes = generic_classes(uses) - in_generic_class = [function for function in functions if self._is_inside(function, classes)] - candidates = [ - function - for function in functions - if function not in in_generic_class and not declares_type_param(cast("cst.FunctionDef", function.node), name) - ] - if not candidates: - if in_generic_class: - self._mark_unsafe(results, self.converted_unsafe_reasons, name, UnsafeReason.USED_IN_GENERIC_CLASS) - continue - reason = self._conversion_refusal(decl_stmt) - if reason is not None: - self._mark_unsafe(results, self.converted_unsafe_reasons, name, reason) - continue - - param = (type_param_text(decl_stmt, module), declared_default(decl_stmt) is not None) - for function in candidates: - new_params.setdefault(id(function), (function, []))[1].append(param) - results[name] = "fixed" - - for function, params in new_params.values(): - self._add_type_params(function, params) - - return results - - @staticmethod - def _is_inside(node: PythonCstNode, containers: Sequence[PythonCstNode]) -> bool: - """Return True if one of containers encloses node.""" - current = node.parent - while current is not None: - if any(current is container for container in containers): - return True - current = current.parent - return False - - def _add_type_params(self, function: PythonCstNode, params: list[tuple[str, bool]]) -> None: - """Add params, given as (text, has_default), to function's PEP 695 type parameters, changing nothing else. - - A function without a bracket gets one after its name; an existing bracket is rewritten with its own - type parameters followed by params, the defaulted ones last. - """ - node = cast("cst.FunctionDef", function.node) - unit = function.translation_unit - existing = node.type_parameters - if existing is None: - self.replace( - node.name.value + type_params_bracket(params), unit.wrappers[node.name], include_whitespace=False, include_comments=False - ) - return - # TODO(FrancescoPezzella): rebuilding the existing bracket drops its comments and line breaks. - # https://github.com/TNO/Renaissance.Py/issues/279 - current = [ - (unit.atu.code_for_node(param.with_changes(comma=cst.MaybeSentinel.DEFAULT)), param.default is not None) - for param in existing.params - ] - self.replace(type_params_bracket([*current, *params]), unit.wrappers[existing], include_whitespace=False, include_comments=False) - - def remove_orphaned_declarations(self) -> dict[str, str]: - """Remove every module-level TypeVar/ParamSpec/TypeVarTuple declaration that nothing uses anymore. - - A declaration is orphaned when uses_of finds no use of its name: every `T` left belongs to a PEP 695 - type parameter, or there is none. One that is_safe_to_remove rejects is kept and reported "unsafe", - with its UnsafeReason on self.orphaned_unsafe_reasons. Returns {name: "fixed" | "unsafe"}. - """ - root = self._cst_root() - module = root.translation_unit.atu - - results: dict[str, str] = {} - self.orphaned_unsafe_reasons: dict[str, UnsafeReason] = {} - for name, decl_stmt in find_type_param_declarations(module).items(): - if uses_of(root, name, decl_stmt): - continue - - # TODO(FrancescoPezzella): a public TypeVar no file in the project uses (e.g. redis.typing.AnyKeyT) is removed, - # though other projects may import it. - # https://github.com/TNO/Renaissance.Py/issues/279 - reason = is_safe_to_remove(module, name, self.project_wide_imported_names) - if reason is not None: - self._mark_unsafe(results, self.orphaned_unsafe_reasons, name, reason) - continue - - # TODO(FrancescoPezzella): `T = U = TypeVar("T")` is removed whole when T is unused, so U is no longer defined. - # https://github.com/TNO/Renaissance.Py/issues/279 - self._remove_declaration(decl_stmt) - results[name] = "fixed" - - return results - - def _mark_unsafe(self, results: dict[str, str], reasons: dict[str, UnsafeReason], name: str, reason: UnsafeReason) -> None: - """Record `name` as unsafe with `reason` in both `results` (status) and `reasons` (why).""" - results[name] = "unsafe" - reasons[name] = reason - - def _remove_declaration(self, decl_stmt: cst.Assign) -> None: - """Remove decl_stmt's statement from the file.""" - # TODO(FrancescoPezzella): enable once comment blocks get correctly deleted - # https://github.com/TNO/Renaissance.Py/issues/189 - # TODO(FrancescoPezzella): a declaration that starts a `;` line leaves a leading `;`, a syntax error that commit() - # writes to disk. - # https://github.com/TNO/Renaissance.Py/issues/279 - self.remove(self._cst_root().translation_unit.wrappers[decl_stmt], include_comments=False) - - def localize_imported_typevars(self) -> dict[str, str]: - """Replace imports of TypeVar/ParamSpec/TypeVarTuple names from the target project with local declarations. - - Imports are resolved against project_root. Where safe, the import becomes a copy of the origin's - declaration, plus imports from the origin for the names its arguments use (e.g. `bound=Shape`). - It is unsafe when is_safe_to_localize rejects the origin or _argument_names_to_import returns None. - Returns {name: "fixed" | "unsafe"}, with each UnsafeReason on self.cross_file_unsafe_reasons. - """ - results: dict[str, str] = {} - self.cross_file_unsafe_reasons: dict[str, UnsafeReason] = {} - module = self._cst_root().translation_unit.atu - - project_root = self.project_root if self.project_root is not None else Path(self.filename).parent - for line in module.body: - if not isinstance(line, cst.SimpleStatementLine): - continue - for statement in line.body: - if not isinstance(statement, cst.ImportFrom): - continue - origin_path = resolve_project_module( - Path(self.filename), project_root, import_from_module(statement), len(statement.relative) - ) - if origin_path is None: - continue - self._localize_names_from(line, statement, origin_path, results) - - return results - - def _localize_names_from( - self, - line: cst.SimpleStatementLine, - raw: cst.ImportFrom, - origin_path: Path, - results: dict[str, str], - ) -> None: - """Localize every safe type parameter name that raw, on line, imports from the origin_path file. - - Records each name in results as "fixed" or, via _mark_unsafe, as "unsafe". - - Raises: - SyntaxError: if the origin file can't be parsed, with the origin's path as its filename. - - """ - if isinstance(raw.names, cst.ImportStar): - return - try: - origin = cst.parse_module(origin_path.read_text(encoding="utf-8")) - except cst.ParserSyntaxError as error: - raise SyntaxError(error.message, (str(origin_path), error.raw_line, error.raw_column + 1, None)) from error - declarations = find_type_param_declarations(origin) - localized: dict[str, cst.Assign] = {} - needed_imports: list[str] = [] - needed_argument_names: set[str] = set() - - for alias in raw.names: - # TODO(FrancescoPezzella): `from x import T as U` is not currently being caught, tbd - # https://github.com/TNO/Renaissance.Py/issues/279 - name = alias.evaluated_name - if alias.asname is not None or name not in declarations: - continue - - decl_stmt = declarations[name] - reason = is_safe_to_localize(origin, name) - if reason is not None: - self._mark_unsafe(results, self.cross_file_unsafe_reasons, name, reason) - continue - argument_names = self._argument_names_to_import(origin, raw, decl_stmt) - if argument_names is None: - self._mark_unsafe(results, self.cross_file_unsafe_reasons, name, UnsafeReason.DECLARATION_NAME_UNAVAILABLE) - continue - - constructor_import = self._missing_constructor_import(origin, origin_path, raw, decl_stmt) - if constructor_import is not None and constructor_import not in needed_imports: - needed_imports.append(constructor_import) - needed_argument_names |= argument_names - localized[name] = decl_stmt - results[name] = "fixed" - - if not localized: - return - if needed_argument_names: - needed_imports.append(f"from {self._import_from_text(raw)} import {', '.join(sorted(needed_argument_names))}") - self._localize_import(line, raw, origin, localized, needed_imports) - - @staticmethod - def _import_from_text(raw: cst.ImportFrom) -> str: - """Return what follows `from` in raw: its leading dots and dotted module, e.g. "..pkg.mod".""" - return "." * len(raw.relative) + (import_from_module(raw) or "") - - def _missing_constructor_import( - self, - origin: cst.Module, - origin_path: Path, - via: cst.ImportFrom, - decl_stmt: cst.Assign, - ) -> str | None: - """Return the "from module import Ctor" line the localized declaration needs, or None if this file has it. - - The module is the one the origin imports the constructor from, as named from this file, which - reaches the origin (origin_path) through via. If this file can't name that module, the - constructor is imported from the origin module itself. - """ - ctor_name = type_param_constructor_name(decl_stmt) - ctor_source = from_import_sources(origin).get(ctor_name) - if ctor_source is None: - # TODO(FrancescoPezzella): an origin using `from typing import *` lands here, - # so a file not importing the constructor itself gets a NameError. - # TBD - needs a fix - # https://github.com/TNO/Renaissance.Py/issues/279 - return None - - via_module = import_from_module(via) - ctor_module = rebase_relative_module(via_module, len(via.relative), origin_path, ctor_source[0] or None, ctor_source[1]) - ctor_module = ctor_module or self._import_from_text(via) - - # TODO(FrancescoPezzella): the constructor already imported here from another module (e.g. `typing` vs the origin's - # `typing_extensions`) isn't matched, so a second, shadowing import of the same name is added. - # https://github.com/TNO/Renaissance.Py/issues/279 - for statement in module_statements(self._cst_root().translation_unit.atu): - if ( - isinstance(statement, cst.ImportFrom) - and not isinstance(statement.names, cst.ImportStar) - and self._import_from_text(statement) == ctor_module - and any(imported_name(alias) == ctor_name for alias in statement.names) - ): - return None - - return f"from {ctor_module} import {ctor_name}" - - def _argument_names_to_import(self, origin: cst.Module, raw: cst.ImportFrom, decl_stmt: cst.Assign) -> set[str] | None: - """Return the names decl_stmt's arguments use that this file must import from the origin module. - - A name this file already imports from the origin module (or from the same absolute module the - origin imports it from) is fine and not returned. Returns None if one of those names can't be - imported from the origin as the same object: the origin doesn't bind it at module level at - runtime (e.g. only under `if TYPE_CHECKING:`), or this file binds it to something else. - """ - origin_sources = from_import_sources(origin) - importing_sources = from_import_sources(self._cst_root().translation_unit.atu) - missing: set[str] = set() - for name in declaration_argument_names(decl_stmt): - if name not in origin_sources: - return None - if name not in importing_sources: - missing.add(name) - continue - origin_source = origin_sources[name] - accepted = {(import_from_module(raw) or "", len(raw.relative))} - if origin_source is not None and origin_source[1] == 0: - accepted.add(origin_source) - if importing_sources[name] not in accepted: - return None - return missing - - def _localize_import( - self, - line: cst.SimpleStatementLine, - raw: cst.ImportFrom, - origin: cst.Module, - declarations: dict[str, cst.Assign], - needed_imports: list[str], - ) -> None: - """Replace the import raw, on line, with local copies of declarations from the origin module. - - The declarations keep their original formatting and come after needed_imports: the imports they need - (their constructors, names their arguments use) that this file doesn't have yet. If raw imports other - names too, it is narrowed to those, keeping its comments, and the declarations go after line; - otherwise they replace raw. - """ - no_semicolon = cst.MaybeSentinel.DEFAULT - decl_text = "\n".join( - [*needed_imports, *(origin.code_for_node(decl.with_changes(semicolon=no_semicolon)) for decl in declarations.values())] - ) - wrappers = self._cst_root().translation_unit.wrappers - - kept = [alias for alias in cast("Sequence[cst.ImportAlias]", raw.names) if imported_name(alias) not in declarations] - if not kept: - self.replace(decl_text, wrappers[raw], include_whitespace=False, include_comments=False) - return - if raw.rpar is None: - # Without parentheses, a trailing comma after the last name is a syntax error. - kept[-1] = kept[-1].with_changes(comma=cst.MaybeSentinel.DEFAULT) - narrowed = self._cst_root().translation_unit.atu.code_for_node(raw.with_changes(names=kept, semicolon=no_semicolon)) - self.replace(narrowed, wrappers[raw], include_whitespace=False, include_comments=False) - self.insert_after(f"\n{decl_text}", wrappers[line], include_whitespace=False, include_comments=True) diff --git a/src/renaissance/recipes/type_var_domain.py b/src/renaissance/recipes/type_var_domain.py deleted file mode 100644 index cfc8968fa..000000000 --- a/src/renaissance/recipes/type_var_domain.py +++ /dev/null @@ -1,401 +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 - -"""TypeVar/ParamSpec/TypeVarTuple domain model and safety analysis.""" - -import builtins -from collections.abc import Iterable, Iterator -from dataclasses import dataclass -from enum import StrEnum -from typing import cast - -import libcst as cst -from libcst import matchers - -from renaissance.integrations.python.ast.cst_node import PythonCstNode -from renaissance.utils.import_resolution import import_from_module, imported_name - -DOCS_BASE_URL = "https://tno.github.io/Renaissance.Py/user/features/type-parameter-modernization/" - - -class UnsafeReason(StrEnum): - """Every distinct, permanent reason a TypeVar/ParamSpec/TypeVarTuple candidate is left unconverted. - - Each member has a matching documented rule under DOCS_BASE_URL - see UNSAFE_RULES and doc_link(). - """ - - PEP695_VERSION_GATE = "pep695_version_gate" - PEP696_VERSION_GATE = "pep696_version_gate" - NO_PEP695_EQUIVALENT = "no_pep695_equivalent" - DECLARED_TYPEVAR_EXPORTED = "declared_typevar_exported" - USED_IN_GENERIC_CLASS = "used_in_generic_class" - DECLARATION_NAME_UNAVAILABLE = "declaration_name_unavailable" - IMPORTED_ELSEWHERE_IN_PROJECT = "imported_elsewhere_in_project" - ORIGIN_IMPORTS_CONSTRUCTOR_CONDITIONALLY = "origin_imports_constructor_conditionally" - - -@dataclass(frozen=True) -class UnsafeRule: - """A short human-readable explanation plus the docs anchor slug for one UnsafeReason.""" - - message: str - doc_anchor: str - - -UNSAFE_RULES: dict[UnsafeReason, UnsafeRule] = { - UnsafeReason.PEP695_VERSION_GATE: UnsafeRule( - "target's minimum Python version is unknown or below 3.12", - "feature-type-parameter-modernization-pep695-version-gate", - ), - UnsafeReason.PEP696_VERSION_GATE: UnsafeRule( - "declares a default= (PEP 696), which needs the target's minimum Python to be 3.13+", - "feature-type-parameter-modernization-pep696-version-gate", - ), - UnsafeReason.NO_PEP695_EQUIVALENT: UnsafeRule( - "declaration passes an argument with no PEP 695 equivalent, e.g. covariant=True or bound= on a ParamSpec", - "feature-type-parameter-modernization-no-pep695-equivalent", - ), - UnsafeReason.DECLARED_TYPEVAR_EXPORTED: UnsafeRule( - "exported via __all__", - "feature-type-parameter-modernization-declared-type-parameter-exported", - ), - UnsafeReason.USED_IN_GENERIC_CLASS: UnsafeRule( - "only used inside a class that is generic over it", - "feature-type-parameter-modernization-used-in-generic-class", - ), - UnsafeReason.DECLARATION_NAME_UNAVAILABLE: UnsafeRule( - "a name its declaration uses can't be imported here as the same object", - "feature-type-parameter-modernization-declaration-name-unavailable", - ), - UnsafeReason.IMPORTED_ELSEWHERE_IN_PROJECT: UnsafeRule( - "imported directly by another file in the target project", - "feature-type-parameter-modernization-imported-elsewhere-in-project", - ), - UnsafeReason.ORIGIN_IMPORTS_CONSTRUCTOR_CONDITIONALLY: UnsafeRule( - "origin module imports TypeVar/ParamSpec/TypeVarTuple conditionally, e.g. per Python version", - "feature-type-parameter-modernization-origin-imports-constructor-conditionally", - ), -} - - -def doc_link(reason: UnsafeReason) -> str: - """Return the full URL to the documented rule explaining why `reason` makes a candidate unsafe.""" - return f"{DOCS_BASE_URL}#{UNSAFE_RULES[reason].doc_anchor}" - - -_CONSTRUCTORS = frozenset({"TypeVar", "ParamSpec", "TypeVarTuple"}) - - -def _is_type_param_call(value: cst.BaseExpression) -> bool: - """Return True if `value` is a call to TypeVar/ParamSpec/TypeVarTuple.""" - return isinstance(value, cst.Call) and isinstance(value.func, cst.Name) and value.func.value in _CONSTRUCTORS - - -def module_statements(module: cst.Module) -> Iterator[cst.BaseSmallStatement | cst.BaseCompoundStatement]: - """Yield the module's top-level statements in order, one per small statement of a line like `a = 1; b = 2`.""" - for statement in module.body: - if isinstance(statement, cst.SimpleStatementLine): - yield from statement.body - else: - yield statement - - -def find_type_param_declarations(module: cst.Module) -> dict[str, cst.Assign]: - """Find every module-level "NAME = TypeVar/ParamSpec/TypeVarTuple(...)" declaration.""" - declarations: dict[str, cst.Assign] = {} - for statement in module_statements(module): - if isinstance(statement, cst.Assign) and _is_type_param_call(statement.value): - for target in statement.targets: - if isinstance(target.target, cst.Name): - declarations[target.target.value] = statement - return declarations - - -def _call(decl_stmt: cst.Assign) -> cst.Call: - """Return the TypeVar/ParamSpec/TypeVarTuple call a declaration assigns.""" - return cast("cst.Call", decl_stmt.value) - - -def _positional_arguments(call: cst.Call) -> list[cst.Arg]: - """Return the call's positional arguments, `*args` included.""" - return [argument for argument in call.args if argument.keyword is None and argument.star != "**"] - - -def _keyword_value(call: cst.Call, keyword: str) -> cst.BaseExpression | None: - """Return the value the call passes as `keyword=`, or None if it passes none.""" - return next((argument.value for argument in call.args if argument.keyword is not None and argument.keyword.value == keyword), None) - - -def type_param_constructor_name(decl_stmt: cst.Assign) -> str: - """Return the name of the call a declaration uses, e.g. "TypeVar" for `T = TypeVar("T")`.""" - return cast("cst.Name", _call(decl_stmt).func).value - - -def _string_value(expression: cst.BaseExpression) -> str | None: - """Return the value of a plain or concatenated string literal, or None for any other expression.""" - if isinstance(expression, cst.SimpleString | cst.ConcatenatedString): - value = expression.evaluated_value - return value if isinstance(value, str) else None - return None - - -def _find_dunder_all(module: cst.Module) -> set[str] | None: - """Return the names in this module's plain `__all__ = [...]` assignment, or None if it has none. - - Only a list, tuple or set literal is read; annotated, `+=`, `.extend()` and `.append()` forms are ignored. - """ - for statement in module_statements(module): - if ( - isinstance(statement, cst.Assign) - and any(isinstance(target.target, cst.Name) and target.target.value == "__all__" for target in statement.targets) - and isinstance(statement.value, cst.List | cst.Tuple | cst.Set) - ): - values = (_string_value(element.value) for element in statement.value.elements if isinstance(element, cst.Element)) - return {value for value in values if value is not None} - return None - - -class _NameCollector(cst.CSTVisitor): - """Collect the names an expression reads: every `Name` except an attribute's or a keyword argument's own name.""" - - def __init__(self) -> None: - """Start with no names collected.""" - super().__init__() - self.names: set[str] = set() - - def visit_Name(self, node: cst.Name) -> None: # noqa: N802 - libcst dispatches on this exact name - """Record the name.""" - self.names.add(node.value) - - def visit_Attribute(self, node: cst.Attribute) -> bool: # noqa: N802 - libcst dispatches on this exact name - """Visit only the attribute's object: in `np.integer`, `integer` is not a name read here.""" - node.value.visit(self) - return False - - def visit_Arg(self, node: cst.Arg) -> bool: # noqa: N802 - libcst dispatches on this exact name - """Visit only the argument's value, not the keyword it is passed as.""" - node.value.visit(self) - return False - - -def declaration_argument_names(decl_stmt: cst.Assign) -> set[str]: - """Return the non-builtin names a declaration's arguments use, e.g. {"Shape"} for `TypeVar("T", bound=Shape)`. - - The first positional argument (the type parameter's own name) is skipped; a string argument is read as a - forward reference (`bound="Shape"`). A string that isn't a valid expression contributes no names. - """ - call = _call(decl_stmt) - keyword_arguments = [argument for argument in call.args if argument.keyword is not None or argument.star == "**"] - collector = _NameCollector() - for argument in [*_positional_arguments(call)[1:], *keyword_arguments]: - expression = argument.value - text = _string_value(expression) - if text is not None: - try: - expression = cst.parse_expression(text) - except cst.ParserSyntaxError: - continue - expression.visit(collector) - return collector.names - set(dir(builtins)) - - -def from_import_sources(module: cst.Module) -> dict[str, tuple[str, int] | None]: - """Map every name bound at module level to its `from` import's (module, level), or None if bound otherwise. - - Names bound by a class, function, assignment or plain `import` map to None. - """ - sources: dict[str, tuple[str, int] | None] = {} - for statement in module_statements(module): - if isinstance(statement, cst.ImportFrom): - source = (import_from_module(statement) or "", len(statement.relative)) - if isinstance(statement.names, cst.ImportStar): - sources["*"] = source - else: - for alias in statement.names: - sources[imported_name(alias)] = source - elif isinstance(statement, cst.Import): - for alias in statement.names: - sources[imported_name(alias).split(".")[0]] = None - elif isinstance(statement, cst.ClassDef | cst.FunctionDef): - sources[statement.name.value] = None - elif isinstance(statement, cst.Assign | cst.AnnAssign): - sources.update(dict.fromkeys(_assigned_names(statement))) - return sources - - -def _assigned_names(statement: cst.Assign | cst.AnnAssign) -> set[str]: - """Return every name an assignment's targets use, including inside tuple or list targets.""" - targets = [target.target for target in statement.targets] if isinstance(statement, cst.Assign) else [statement.target] - collector = _NameCollector() - for target in targets: - target.visit(collector) - return collector.names - - -def _imports_name_conditionally(module: cst.Module, name: str) -> bool: - """Return True if an import inside a module-level `if`/`try` block binds `name`.""" - for statement in module.body: - if not isinstance(statement, cst.If | cst.Try | cst.TryStar): - continue - for node in matchers.findall(statement, matchers.Import() | matchers.ImportFrom()): - names = cast("cst.Import | cst.ImportFrom", node).names - if not isinstance(names, cst.ImportStar) and any(imported_name(alias).split(".")[0] == name for alias in names): - return True - return False - - -def is_safe_to_localize(origin: cst.Module, name: str) -> UnsafeReason | None: - """Return None if `name` is safe to duplicate as a local declaration, else the reason it isn't. - - Its constructor (TypeVar/ParamSpec/TypeVarTuple) must not be imported inside a module-level - `if`/`try` block at the origin (ORIGIN_IMPORTS_CONSTRUCTOR_CONDITIONALLY), since which - implementation it binds then depends on the runtime, e.g. `typing_extensions` below 3.13. - """ - declaration = find_type_param_declarations(origin).get(name) - if declaration is not None and _imports_name_conditionally(origin, type_param_constructor_name(declaration)): - return UnsafeReason.ORIGIN_IMPORTS_CONSTRUCTOR_CONDITIONALLY - return None - - -def is_safe_to_remove( - module: cst.Module, - name: str, - project_wide_imported_names: frozenset[str] = frozenset(), -) -> UnsafeReason | None: - """Return None if this file's own binding of `name` may be removed or replaced. - - Otherwise returns the reason it may not: DECLARED_TYPEVAR_EXPORTED if exported via a plain `__all__ = [...]`, - or IMPORTED_ELSEWHERE_IN_PROJECT if `name` is in `project_wide_imported_names` (another file in - the target project imports it directly, regardless of `__all__`). - """ - dunder_all = _find_dunder_all(module) - if dunder_all is not None and name in dunder_all: - return UnsafeReason.DECLARED_TYPEVAR_EXPORTED - if name in project_wide_imported_names: - return UnsafeReason.IMPORTED_ELSEWHERE_IN_PROJECT - return None - - -def uses_of(root: PythonCstNode, name: str, decl_stmt: cst.Assign) -> list[PythonCstNode]: - """Return the nodes that use the module-level `name` that decl_stmt declares, resolved with libcst's scope analysis. - - A use inside a string annotation or `cast("T", x)` counts; a `T` that a PEP 695 type parameter of an - enclosing function or class (`def f[T]`, `class C[T]`) declares does not. - """ - # TODO(FrancescoPezzella): libcst puts a function's decorators and parameter defaults inside its PEP 695 type - # parameter scope, so a `T` read only there (`@dec(T)`, `y=T`) is missed and the declaration it needs is removed. - # https://github.com/TNO/Renaissance.Py/issues/279 - target = next(target.target for target in decl_stmt.targets if isinstance(target.target, cst.Name) and target.target.value == name) - return [reference.node for reference in root.translation_unit.wrappers[target].referenced_by] - - -def outermost_function(node: PythonCstNode) -> PythonCstNode | None: - """Return the outermost function or method enclosing node, or None if node is outside every function.""" - outermost = None - current = node.parent - while current is not None: - if isinstance(current.node, cst.FunctionDef): - outermost = current - current = current.parent - return outermost - - -def generic_classes(uses: Iterable[PythonCstNode]) -> list[PythonCstNode]: - """Return the classes that have one of `uses` in a base (`Generic[T]`, `Protocol[T]`, `Base[int, T]`). - - Such a class is generic over the used name. - """ - classes: list[PythonCstNode] = [] - for use in uses: - child, current = use, use.parent - while current is not None: - if isinstance(current.node, cst.ClassDef) and any(child.node is base for base in current.node.bases): - if current not in classes: - classes.append(current) - break - child, current = current, current.parent - return classes - - -def declares_type_param(function: cst.FunctionDef, name: str) -> bool: - """Return True if the function declares `name` as its own PEP 695 type parameter (`def f[T](...)`).""" - parameters = function.type_parameters - return parameters is not None and any(parameter.param.name.value == name for parameter in parameters.params) - - -# infer_variance needs no syntax of its own: a PEP 695 type parameter always infers its variance. -_CONVERTIBLE_KEYWORDS: dict[str, frozenset[str]] = { - "TypeVar": frozenset({"bound", "default", "infer_variance"}), - "ParamSpec": frozenset({"default", "infer_variance"}), - "TypeVarTuple": frozenset({"default", "infer_variance"}), -} - - -def declared_default(decl_stmt: cst.Assign) -> cst.BaseExpression | None: - """Return the value a legacy declaration passes as `default=` (PEP 696), or None if it passes none.""" - return _keyword_value(_call(decl_stmt), "default") - - -def has_unconvertible_argument(decl_stmt: cst.Assign) -> bool: - """Return True if a legacy declaration passes a keyword type_param_text can't carry over. - - That is any keyword outside the constructor's known set, e.g. `covariant=`/`contravariant=`, - `bound=` on a ParamSpec or TypeVarTuple, a keyword added by a later Python version, or a `**mapping`. - """ - convertible = _CONVERTIBLE_KEYWORDS[type_param_constructor_name(decl_stmt)] - return any( - argument.star == "**" or (argument.keyword is not None and argument.keyword.value not in convertible) - for argument in _call(decl_stmt).args - ) - - -def type_param_text(decl_stmt: cst.Assign, module: cst.Module) -> str: - """Return the PEP 695 type parameter equivalent to a legacy declaration from `module`, in its original formatting. - - E.g. `T = TypeVar("T", bound=int, default=int)` gives `T: int = int`. Bounds, constraints and - `default=` are carried over; `infer_variance=` needs nothing, since a PEP 695 type parameter always - infers its variance. Callers must first reject declarations for which - has_unconvertible_argument() is True. - """ - # TODO(FrancescoPezzella): a default that uses another legacy declaration (`default=T`) still refers to it after conversion. - # https://github.com/TNO/Renaissance.Py/issues/279 - call = _call(decl_stmt) - constructor = type_param_constructor_name(decl_stmt) - positional = _positional_arguments(call) - # TODO(FrancescoPezzella): the type parameter is named after the string argument, not the variable, so - # `T = TypeVar("U")` gives `def f[U](x: T)`; a non-literal name (`TypeVar(name)`, `TypeVar(*args)`) raises - # AttributeError and stops every phase for the file. - # https://github.com/TNO/Renaissance.Py/issues/279 - name = cast("str", cast("cst.SimpleString", positional[0].value).evaluated_value) - - if constructor == "ParamSpec": - text = f"**{name}" - elif constructor == "TypeVarTuple": - text = f"*{name}" - else: - text = name - bound = _keyword_value(call, "bound") - constraints = [f"{argument.star}{module.code_for_node(argument.value)}" for argument in positional[1:]] - if bound is not None: - text += f": {module.code_for_node(bound)}" - elif constraints: - # A single constraint keeps a trailing comma, or the parentheses would make it a bound. - text += f": ({', '.join(constraints)}{',' if len(constraints) == 1 else ''})" - - default = declared_default(decl_stmt) - if default is not None: - text += f" = {module.code_for_node(default)}" - return text - - -def type_params_bracket(params: Iterable[tuple[str, bool]]) -> str: - """Return the `[...]` bracket for type parameters given as (text, has_default), with the defaulted ones last. - - The relative order within each group is kept. A non-default type parameter following a defaulted - one is a SyntaxError. - """ - ordered = sorted(params, key=lambda param: param[1]) - return f"[{', '.join(text for text, _ in ordered)}]" diff --git a/src/renaissance/utils/cst_utils.py b/src/renaissance/utils/cst_utils.py new file mode 100644 index 000000000..4717f254b --- /dev/null +++ b/src/renaissance/utils/cst_utils.py @@ -0,0 +1,67 @@ +# Copyright (C) 2024-2026 TNO +# +# Licensed under the terms specified in the LICENSE file in the root directory +# of this repository, also available at: +# https://github.com/TNO/Renaissance.Py/blob/main/LICENSE + +"""Helpers over the libcst tree of one module: its top-level statements, the names they read or bind and the comment trailing a line.""" + +from collections.abc import Iterator + +import libcst as cst + + +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 trailing_of_sole_statement(module: cst.Module, statement: cst.BaseSmallStatement) -> cst.TrailingWhitespace | None: + """Return the whitespace and comment trailing the line of statement in module, or None if the line holds others.""" + for line in module.body: + if isinstance(line, cst.SimpleStatementLine) and len(line.body) == 1 and line.body[0] is statement: + return line.trailing_whitespace + return None + + +class _NameCollector(cst.CSTVisitor): + """Collect the names an expression reads: every `Name` except an attribute's or a keyword argument's own name.""" + + def __init__(self) -> None: + """Start with no names collected.""" + super().__init__() + self.names: set[str] = set() + + def visit_Name(self, node: cst.Name) -> None: # noqa: N802 - libcst dispatches on this exact name + """Record the name.""" + self.names.add(node.value) + + def visit_Attribute(self, node: cst.Attribute) -> bool: # noqa: N802 - libcst dispatches on this exact name + """Visit only the attribute's object: in `np.integer`, `integer` is not a name read here.""" + node.value.visit(self) + return False + + def visit_Arg(self, node: cst.Arg) -> bool: # noqa: N802 - libcst dispatches on this exact name + """Visit only the argument's value, not the keyword it is passed as.""" + node.value.visit(self) + return False + + +def names_read_by(node: cst.CSTNode) -> set[str]: + """Return every name `node` reads, built-ins included; attribute names and keyword argument names are not reads.""" + collector = _NameCollector() + node.visit(collector) + return collector.names + + +def 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 diff --git a/src/renaissance/utils/import_resolution.py b/src/renaissance/utils/import_resolution.py index e4a19c266..ed7a664d4 100644 --- a/src/renaissance/utils/import_resolution.py +++ b/src/renaissance/utils/import_resolution.py @@ -4,7 +4,7 @@ # of this repository, also available at: # https://github.com/TNO/Renaissance.Py/blob/main/LICENSE -"""Resolve project-internal `from X import Y` statements to the .py file they import from, and re-anchor them on other files.""" +"""Read the import statements of a module, and resolve project-internal `from X import Y` to the .py file they import from.""" from collections.abc import Sequence from pathlib import Path @@ -14,6 +14,8 @@ from libcst import matchers from libcst.helpers import get_full_name_for_node +from renaissance.utils.cst_utils import assigned_names, module_statements + 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`.""" @@ -25,6 +27,65 @@ def imported_name(alias: cst.ImportAlias) -> str: return alias.evaluated_alias or alias.evaluated_name +def import_from_text(statement: cst.ImportFrom) -> str: + """Return what follows `from` in an import-from: its leading dots and dotted module, e.g. "..pkg.mod".""" + return "." * len(statement.relative) + (import_from_module(statement) or "") + + +def is_import_line(statement: cst.BaseStatement) -> bool: + """Return True if statement is a line made only of import statements.""" + return isinstance(statement, cst.SimpleStatementLine) and all( + isinstance(small, cst.Import | cst.ImportFrom) for small in statement.body + ) + + +def from_import_sources(module: cst.Module) -> dict[str, tuple[str, int] | None]: + """Map every name bound at module level to its `from` import's (module, level), or None if bound otherwise. + + Names bound by a class, function, assignment or plain `import` map to None. + """ + sources: dict[str, tuple[str, int] | None] = {} + for statement in module_statements(module): + if isinstance(statement, cst.ImportFrom): + source = (import_from_module(statement) or "", len(statement.relative)) + if isinstance(statement.names, cst.ImportStar): + sources["*"] = source + else: + for alias in statement.names: + sources[imported_name(alias)] = source + elif isinstance(statement, cst.Import): + for alias in statement.names: + sources[imported_name(alias).split(".")[0]] = None + elif isinstance(statement, cst.ClassDef | cst.FunctionDef): + sources[statement.name.value] = None + elif isinstance(statement, cst.Assign | cst.AnnAssign): + sources.update(dict.fromkeys(assigned_names(statement))) + return sources + + +def from_import_binding(module: cst.Module, name: str) -> tuple[cst.ImportFrom, cst.ImportAlias] | None: + """Return the module-level `from` import and the alias that bind name, or None if another statement binds it last.""" + if from_import_sources(module).get(name) is None: + return None + binding: tuple[cst.ImportFrom, cst.ImportAlias] | None = None + for statement in module_statements(module): + if isinstance(statement, cst.ImportFrom) and not isinstance(statement.names, cst.ImportStar): + binding = next(((statement, alias) for alias in statement.names if imported_name(alias) == name), binding) + return binding + + +def imports_name_conditionally(module: cst.Module, name: str) -> bool: + """Return True if an import inside a module-level `if`/`try` block binds `name`.""" + for statement in module.body: + if not isinstance(statement, cst.If | cst.Try | cst.TryStar): + continue + for node in matchers.findall(statement, matchers.Import() | matchers.ImportFrom()): + names = cast("cst.Import | cst.ImportFrom", node).names + if not isinstance(names, cst.ImportStar) and any(imported_name(alias).split(".")[0] == name for alias in names): + return True + return False + + def 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`. @@ -37,9 +98,6 @@ def resolve_project_module(importing_file: Path, project_root: Path, module: str Tries `.py` first, then `/__init__.py`. Returns None if neither exists or if resolution would walk above `project_root`, as for a stdlib or third-party import. The returned path is always absolute. - - # TODO: doesn't follow re-exports through an intermediate __init__.py, or handle namespace - # packages (no __init__.py, PEP 420) - out of scope for now. """ # A relative path can't walk above itself: Path("a.py").parent.parent is still Path("."). importing_file = importing_file.absolute() @@ -131,8 +189,6 @@ def _record_from_import( module_bindings: dict[str, Path], ) -> None: """Record `statement`'s imported names against their origin, and bind any alias that is itself a project module.""" - # TODO(FrancescoPezzella): `from pkg.mod import *` isn't expanded to the names it actually pulls in. - # https://github.com/TNO/Renaissance.Py/issues/279 module = import_from_module(statement) level = len(statement.relative) aliases = [] if isinstance(statement.names, cst.ImportStar) else list(statement.names) diff --git a/test/examples/test_python_examples.py b/test/examples/test_python_examples.py index 214d6936a..30faac15d 100644 --- a/test/examples/test_python_examples.py +++ b/test/examples/test_python_examples.py @@ -12,6 +12,9 @@ from rejuvenation.python_lst_example import python_lst_smoke_test from rejuvenation.python_rst_example import python_rst_smoke_test +# TODO(FrancescoPezzella): `result` is never used and each test compares its own local `result` with itself, so no test +# checks the output of the examples. +# https://github.com/TNO/Renaissance.Py/issues/307 result = ( "\nfrom module import foo, bar, baz, quux\nba(51)\n# changed function f1 to f2\nf2(52\n,123456)\n\n# changed function f1 to f2\n" "f2(53\n,123456)\n\npa(54)\n\n# changed if expr to const\nisAOne=True\nif(isAOne):\n ba()\n\n\npa(54) \n" diff --git a/test/python/ast/test_python_cst_node.py b/test/python/ast/test_python_cst_node.py index 31dd6cdfc..3c42f7578 100644 --- a/test/python/ast/test_python_cst_node.py +++ b/test/python/ast/test_python_cst_node.py @@ -91,6 +91,23 @@ def test_load_file(self) -> None: atu = PythonCstNode.load(Path(targets.__file__).parent / "demo.py") assert_that(atu, is_not(None)) + @pytest.mark.parametrize( + "source", + [ + pytest.param('x = "“quoted”"\n', id="undecodable-in-cp1252"), + pytest.param('x = "café"\n', id="two-byte-character"), + pytest.param('x = "\U0001f600"\n', id="four-byte-character"), + ], + ) + def test_load_file_decodes_utf8(self, tmp_path: Path, source: str) -> None: + """Verify load reads the file as UTF-8, whatever the locale's default encoding is.""" + path = tmp_path / "utf8.py" + path.write_bytes(source.encode("utf-8")) + + atu = PythonCstNode.load(path) + + assert_that(atu.signature, is_(source)) + def test_load_invalid_file(self) -> None: """AI: Verify loading a syntactically invalid Python file raises a ParserSyntaxError.""" with pytest.raises(ParserSyntaxError, match="Syntax Error"): diff --git a/test/python/ast/test_python_rst_node.py b/test/python/ast/test_python_rst_node.py index 6927c2da6..096d58601 100644 --- a/test/python/ast/test_python_rst_node.py +++ b/test/python/ast/test_python_rst_node.py @@ -161,6 +161,23 @@ def test_load_file(self) -> None: atu = PythonRstNode.load(Path(targets.__file__).parent / "demo.py") assert_that(atu.translation_unit.atu.type_ignores, is_(empty())) + @pytest.mark.parametrize( + "source", + [ + pytest.param('x = "“quoted”"\n', id="undecodable-in-cp1252"), + pytest.param('x = "café"\n', id="two-byte-character"), + pytest.param('x = "\U0001f600"\n', id="four-byte-character"), + ], + ) + def test_load_file_decodes_utf8(self, tmp_path: Path, source: str) -> None: + """Verify load reads the file as UTF-8, whatever the locale's default encoding is.""" + path = tmp_path / "utf8.py" + path.write_bytes(source.encode("utf-8")) + + atu = PythonRstNode.load(path) + + assert_that(atu.signature, is_(source)) + def test_load_invalid_file(self) -> None: """AI: Verify loading a syntactically invalid Python file raises an IndentationError.""" with pytest.raises(IndentationError, match="unexpected indent"): diff --git a/test/recipes/conftest.py b/test/recipes/conftest.py index c82bbb08d..997554735 100644 --- a/test/recipes/conftest.py +++ b/test/recipes/conftest.py @@ -4,7 +4,7 @@ # of this repository, also available at: # https://github.com/TNO/Renaissance.Py/blob/main/LICENSE -"""Shared fixture for the TypeVarCheck tests.""" +"""Shared fixture for the TypeParameterCheck tests.""" import textwrap from collections.abc import Callable @@ -14,24 +14,23 @@ from pytest_mock import MockerFixture from renaissance.integrations.python.ast.cst_node import PythonCstNode -from renaissance.recipes.type_var_check import PEP_695_MINIMUM, TypeVarCheck +from renaissance.recipes.type_parameter_check import TypeParameterCheck @pytest.fixture -def create_type_var_check(mocker: MockerFixture) -> Callable[[str], TypeVarCheck]: - """Build a TypeVarCheck for in-memory, dedented source text, with min_python 3.12 so PEP 695 conversion is enabled. +def create_type_parameter_check(mocker: MockerFixture) -> Callable[[str], TypeParameterCheck]: + """Build a TypeParameterCheck for in-memory, dedented source text. `PythonFactory.create` is mocked so nothing touches the filesystem. """ - def _create(text: str) -> TypeVarCheck: + def _create(text: str) -> TypeParameterCheck: mocker.patch( "renaissance.integrations.python.ast.factory.PythonFactory.create", return_value=PythonCstNode.load_from_text(textwrap.dedent(text), "x.py"), ) - subject = TypeVarCheck(Path("x.py")) + subject = TypeParameterCheck(Path("x.py")) subject.in_memory = True - subject.min_python = PEP_695_MINIMUM return subject return _create diff --git a/test/recipes/test_type_var_check.py b/test/recipes/test_type_parameter_check.py similarity index 67% rename from test/recipes/test_type_var_check.py rename to test/recipes/test_type_parameter_check.py index cb33b3e05..851b2706f 100644 --- a/test/recipes/test_type_var_check.py +++ b/test/recipes/test_type_parameter_check.py @@ -4,17 +4,17 @@ # of this repository, also available at: # https://github.com/TNO/Renaissance.Py/blob/main/LICENSE -"""End-to-end tests for TypeVarCheck.check() across its phases.""" +"""End-to-end tests for TypeParameterCheck.check() across its phases.""" from collections.abc import Callable import pytest from hamcrest import assert_that, contains_string, equal_to, not_ -from renaissance.recipes.type_var_check import TypeVarCheck +from renaissance.recipes.type_parameter_check import TypeParameterCheck -class TestTypeVarCheck: +class TestTypeParameterCheck: """See module docstring.""" @pytest.mark.parametrize( @@ -26,12 +26,12 @@ class TestTypeVarCheck: ) def test_check_converts_then_removes_the_declaration_end_to_end( self, - create_type_var_check: Callable[[str], TypeVarCheck], + create_type_parameter_check: Callable[[str], TypeParameterCheck], signature: str, expected_converted: dict[str, str], ) -> None: """Verify check() adds the type parameter where missing, then removes the now-orphaned declaration.""" - subject = create_type_var_check(f""" + subject = create_type_parameter_check(f""" from typing import TypeVar T = TypeVar('T') @@ -45,9 +45,23 @@ def test_check_converts_then_removes_the_declaration_end_to_end( assert_that(output, contains_string("def b[T](x: T) -> T:")) assert_that(output, not_(contains_string("T = TypeVar"))) - def test_check_keeps_declaration_still_used_by_a_generic_class(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + def test_check_records_the_kind_of_every_declaration(self, create_type_parameter_check: Callable[[str], TypeParameterCheck]) -> None: + """Verify check() exposes each declared name's constructor (TypeVar, ParamSpec or TypeVarTuple) on type_param_kinds.""" + subject = create_type_parameter_check(""" + from typing import ParamSpec, TypeVar, TypeVarTuple + T = TypeVar('T') + P = ParamSpec('P') + Ts = TypeVarTuple('Ts') + """) + subject.run() + + assert_that(subject.type_param_kinds, equal_to({"T": "TypeVar", "P": "ParamSpec", "Ts": "TypeVarTuple"})) + + def test_check_keeps_declaration_still_used_by_a_generic_class( + self, create_type_parameter_check: Callable[[str], TypeParameterCheck] + ) -> None: """Verify check() converts a standalone function but keeps the declaration a Generic[...] base still needs.""" - subject = create_type_var_check(""" + subject = create_type_parameter_check(""" from typing import Generic, TypeVar T = TypeVar('T') diff --git a/test/recipes/test_type_var_check_convert.py b/test/recipes/test_type_parameter_check_convert.py similarity index 60% rename from test/recipes/test_type_var_check_convert.py rename to test/recipes/test_type_parameter_check_convert.py index e0e6ce27d..512534bc1 100644 --- a/test/recipes/test_type_var_check_convert.py +++ b/test/recipes/test_type_parameter_check_convert.py @@ -4,7 +4,7 @@ # of this repository, also available at: # https://github.com/TNO/Renaissance.Py/blob/main/LICENSE -"""Tests for TypeVarCheck.convert_declared_typevars.""" +"""Tests for TypeParameterCheck.convert_declared_typevars.""" import ast import textwrap @@ -14,8 +14,8 @@ 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 +from renaissance.recipes.type_parameter_check import MIN_SUPPORTED_PYTHON, TypeParameterCheck +from renaissance.recipes.type_parameter_rules import PEP_696_MINIMUM, UnsafeReason def _type_param_name(param: ast.type_param) -> str: @@ -23,7 +23,7 @@ def _type_param_name(param: ast.type_param) -> str: return cast("ast.TypeVar | ast.ParamSpec | ast.TypeVarTuple", param).name -class TestTypeVarCheckConvert: +class TestTypeParameterCheckConvert: """See module docstring.""" @pytest.mark.parametrize( @@ -42,10 +42,10 @@ class TestTypeVarCheckConvert: ], ) def test_converts_typevar_shared_by_two_functions( - self, create_type_var_check: Callable[[str], TypeVarCheck], functions: str, expected_signatures: list[str] + self, create_type_parameter_check: Callable[[str], TypeParameterCheck], functions: str, expected_signatures: list[str] ) -> None: """Verify a TypeVar shared by two functions, or two methods of one class, gives each its own [T].""" - subject = create_type_var_check(f'from typing import TypeVar\n\n{functions}\nT = TypeVar("T")\n') + subject = create_type_parameter_check(f'from typing import TypeVar\n\n{functions}\nT = TypeVar("T")\n') result = subject.convert_declared_typevars() assert_that(result, equal_to({"T": "fixed"})) @@ -53,6 +53,35 @@ def test_converts_typevar_shared_by_two_functions( for signature in expected_signatures: assert_that(output, contains_string(signature)) + @pytest.mark.parametrize( + ("functions", "converted", "untouched"), + [ + pytest.param( + "def a[T](x: T) -> T:\n return x\ndef b(y: T) -> T:\n return y\n", + "def b[T](y: T) -> T:", + "def a[T](x: T) -> T:", + id="function", + ), + pytest.param( + "class Foo:\n def a[T](self, x: T) -> T:\n return x\n def b(self, y: T) -> T:\n return y\n", + "def b[T](self, y: T) -> T:", + "def a[T](self, x: T) -> T:", + id="method", + ), + ], + ) + def test_leaves_function_with_its_own_type_parameter_alone( + self, create_type_parameter_check: Callable[[str], TypeParameterCheck], functions: str, converted: str, untouched: str + ) -> None: + """Verify a function that already declares its own [T] is not rewritten, while another one using the module T is.""" + subject = create_type_parameter_check(f'from typing import TypeVar\n\n{functions}\nT = TypeVar("T")\n') + result = subject.convert_declared_typevars() + + assert_that(result, equal_to({"T": "fixed"})) + output = subject.apply_to_string() + assert_that(output, contains_string(converted)) + assert_that(output.count(untouched), equal_to(1)) + @pytest.mark.parametrize( "docstring", [ @@ -61,10 +90,12 @@ def test_converts_typevar_shared_by_two_functions( pytest.param('"""Produce a cast.\n\n.. seealso::\n\n :ref:`tutorial_casts`\n"""', id="nested-block"), ], ) - def test_converts_method_keeping_its_docstring(self, create_type_var_check: Callable[[str], TypeVarCheck], docstring: str) -> None: + def test_converts_method_keeping_its_docstring( + self, create_type_parameter_check: Callable[[str], TypeParameterCheck], docstring: str + ) -> None: """Verify converting a method's signature keeps its docstring exactly, including any nested indentation.""" body = textwrap.indent(f"{docstring}\nreturn x\n", " " * 8) - subject = create_type_var_check( + subject = create_type_parameter_check( "from typing import TypeVar\n\nclass Foo:\n" f" def cast(self, x: T) -> T:\n{body}" " def other(self, y: T) -> T:\n return y\n\n" @@ -84,12 +115,12 @@ def test_converts_method_keeping_its_docstring(self, create_type_var_check: Call ) def test_converts_functions_using_a_typevar_whose_declaration_must_stay( self, - create_type_var_check: Callable[[str], TypeVarCheck], + create_type_parameter_check: Callable[[str], TypeParameterCheck], extra_line: str, imported_elsewhere: frozenset[str], ) -> None: """Verify functions get [T] even when T's declaration can't be removed, and the declaration is kept.""" - subject = create_type_var_check(f""" + subject = create_type_parameter_check(f""" from typing import TypeVar {extra_line} @@ -108,9 +139,11 @@ def a(x: T) -> T: assert_that(output, contains_string("def a[T](x: T) -> T:")) assert_that(output, contains_string('T = TypeVar("T")')) - def test_converts_function_preserving_unusual_body_formatting(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + def test_converts_function_preserving_unusual_body_formatting( + self, create_type_parameter_check: Callable[[str], TypeParameterCheck] + ) -> None: """AI: Verify converting a signature never reformats or collapses its body's unusual formatting.""" - subject = create_type_var_check(""" + subject = create_type_parameter_check(""" from typing import TypeVar def b(x: T) -> T: @@ -128,10 +161,12 @@ def b(x: T) -> T: assert_that(output, contains_string("def b[T](x: T) -> T:")) assert_that(output, contains_string("return foo(\n x,\n extra=1,\n )")) - def test_does_not_add_redundant_type_param_to_nested_closure(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + def test_does_not_add_redundant_type_param_to_nested_closure( + self, create_type_parameter_check: Callable[[str], TypeParameterCheck] + ) -> None: """AI: Verify a nested closure referencing an enclosing function's converted type param doesn't get its own copy.""" # PEP 695 type parameters are already visible in nested scopes. - subject = create_type_var_check(""" + subject = create_type_parameter_check(""" from typing import ParamSpec from collections.abc import Callable @@ -152,9 +187,71 @@ def wrapper(*args: P.args, **kwargs: P.kwargs) -> int: assert_that(output, contains_string("def wrapper(*args: P.args, **kwargs: P.kwargs) -> int:")) assert_that(output, not_(contains_string("wrapper[**P]"))) - def test_merges_into_an_existing_type_params_bracket(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + @pytest.mark.parametrize( + ("declarations", "functions", "expected_lines"), + [ + pytest.param( + 'T = TypeVar("T")', + "def handle(scope: int) -> None:\n def wrap(func: Callable[[], T]) -> T:\n return func()\n", + ["def handle(scope: int) -> None:", " def wrap[T](func: Callable[[], T]) -> T:"], + id="only-nested-signature", + ), + pytest.param( + 'K = TypeVar("K")\nF = TypeVar("F")', + 'def keyword(name: "K | None" = None) -> "K | None":\n def decorator(func: F) -> F:\n return func\n', + ['def keyword[K](name: "K | None" = None) -> "K | None":', " def decorator[F](func: F) -> F:"], + id="outer-and-nested-own-one-each", + ), + pytest.param( + 'T = TypeVar("T")', + ( + "def outer() -> None:\n" + " def middle(x: T) -> T:\n" + " def inner(y: T) -> T:\n" + " return y\n" + " return inner(x)\n" + ), + ["def outer() -> None:", " def middle[T](x: T) -> T:", " def inner(y: T) -> T:"], + id="outermost-signature-of-three-levels", + ), + ], + ) + def test_binds_typevar_to_the_outermost_function_whose_signature_uses_it( + self, create_type_parameter_check: Callable[[str], TypeParameterCheck], declarations: str, functions: str, expected_lines: list[str] + ) -> None: + """Verify [T] goes on the outermost function whose own signature uses T, never on an enclosing one that doesn't.""" + subject = create_type_parameter_check( + f"from collections.abc import Callable\nfrom typing import TypeVar\n\n{declarations}\n\n{functions}" + ) + result = subject.convert_declared_typevars() + + names = [declaration.split(" = ", maxsplit=1)[0] for declaration in declarations.splitlines()] + assert_that(result, equal_to(dict.fromkeys(names, "fixed"))) + output = subject.apply_to_string() + for line in expected_lines: + assert_that(output, contains_string(f"{line}\n")) + + @pytest.mark.parametrize( + "body", + [ + pytest.param(" y: list[T] = []\n", id="local-annotation"), + pytest.param(' y = cast("T", x)\n', id="cast"), + ], + ) + def test_leaves_typevar_used_only_in_a_function_body_unattributed( + self, create_type_parameter_check: Callable[[str], TypeParameterCheck], body: str + ) -> None: + """Verify a T that no enclosing signature uses gives no function a [T].""" + source = f'from typing import TypeVar, cast\n\nT = TypeVar("T")\n\n\ndef f(x: int) -> None:\n{body}' + subject = create_type_parameter_check(source) + result = subject.convert_declared_typevars() + + assert_that(result, equal_to({})) + assert_that(subject.apply_to_string(), equal_to(source)) + + def test_merges_into_an_existing_type_params_bracket(self, create_type_parameter_check: Callable[[str], TypeParameterCheck]) -> None: """AI: Verify converting a second TypeVar merges it into an existing PEP 695 bracket instead of adding a new one.""" - subject = create_type_var_check(""" + subject = create_type_parameter_check(""" from typing import TypeVar def f[U](x: U, y: T) -> T: @@ -168,9 +265,9 @@ def f[U](x: U, y: T) -> T: output = subject.apply_to_string() assert_that(output, contains_string("def f[U, T](x: U, y: T) -> T:")) - def test_converts_a_decorated_overload(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + def test_converts_a_decorated_overload(self, create_type_parameter_check: Callable[[str], TypeParameterCheck]) -> None: """AI: Verify converting a decorated @overload signature accounts for its non-zero-column indentation.""" - subject = create_type_var_check(""" + subject = create_type_parameter_check(""" from typing import TypeVar, overload class Config: @@ -188,9 +285,9 @@ def get(self, key: str, default: object = None) -> object: assert_that(output, contains_string("@overload")) assert_that(output, contains_string("def get[T](self, key: str, default: T = ...) -> T: ...")) - def test_converts_function_using_two_new_type_params(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + def test_converts_function_using_two_new_type_params(self, create_type_parameter_check: Callable[[str], TypeParameterCheck]) -> None: """Verify a function using two declared type parameters gets both in one bracket.""" - subject = create_type_var_check(""" + subject = create_type_parameter_check(""" from typing import ParamSpec, TypeVar from collections.abc import Callable @@ -212,18 +309,20 @@ def identity(x: T) -> 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: + def test_orders_type_params_by_declaration(self, create_type_parameter_check: Callable[[str], TypeParameterCheck]) -> None: """Verify the added type parameters follow the order their declarations appear in the file.""" names = ["A", "B", "C", "D", "E", "F"] declarations = "\n".join(f'{name} = TypeVar("{name}")' for name in names) parameters = ", ".join(f"{name.lower()}: {name}" for name in names) - subject = create_type_var_check(f"from typing import TypeVar\n{declarations}\ndef f({parameters}) -> None: ...\n") + subject = create_type_parameter_check(f"from typing import TypeVar\n{declarations}\ndef f({parameters}) -> None: ...\n") subject.convert_declared_typevars() assert_that(subject.apply_to_string(), contains_string(f"def f[{', '.join(names)}]({parameters}) -> None: ...")) - def test_converts_function_preserving_multiline_string_literal(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + def test_converts_function_preserving_multiline_string_literal( + self, create_type_parameter_check: Callable[[str], TypeParameterCheck] + ) -> None: """Verify converting a function keeps a multi-line string literal whose continuation line is at column 0.""" # Built line by line: textwrap.dedent in the fixture would otherwise be blocked by the column-0 line. source_lines = [ @@ -235,7 +334,7 @@ def test_converts_function_preserving_multiline_string_literal(self, create_type ' third"""', " return x", ] - subject = create_type_var_check("\n".join(source_lines) + "\n") + subject = create_type_parameter_check("\n".join(source_lines) + "\n") result = subject.convert_declared_typevars() assert_that(result, has_entry("T", "fixed")) @@ -247,19 +346,22 @@ def test_converts_function_preserving_multiline_string_literal(self, create_type assert_that(ast.literal_eval(text_assign.value), equal_to("first\nsecond\n third")) @pytest.mark.parametrize( - "class_header", + ("class_header", "expected_reasons"), [ - pytest.param("class Box(Generic[T]):", id="generic-base"), - pytest.param("class Box(Protocol[T]):", id="protocol-base"), - pytest.param("class Sub(Base[T]):", id="generic-subclass"), - pytest.param("class Box[T]:", id="pep695-class"), + pytest.param("class Box(Generic[T]):", {"T": UnsafeReason.USED_IN_GENERIC_CLASS}, id="generic-base"), + pytest.param("class Box(Protocol[T]):", {"T": UnsafeReason.USED_IN_GENERIC_CLASS}, id="protocol-base"), + pytest.param("class Sub(Base[T]):", {"T": UnsafeReason.USED_IN_GENERIC_CLASS}, id="generic-subclass"), + pytest.param("class Box[T]:", {}, id="pep695-class"), ], ) def test_converts_function_but_not_methods_of_a_generic_class( - self, create_type_var_check: Callable[[str], TypeVarCheck], class_header: str + self, + create_type_parameter_check: Callable[[str], TypeParameterCheck], + class_header: str, + expected_reasons: dict[str, UnsafeReason], ) -> None: """Verify a standalone function gets [T] while the methods of a class generic over T keep using the class's T.""" - subject = create_type_var_check(f""" + subject = create_type_parameter_check(f""" from typing import Generic, Protocol, TypeVar T = TypeVar('T') @@ -273,13 +375,14 @@ def first(items: list[T]) -> T: result = subject.convert_declared_typevars() assert_that(result, equal_to({"T": "fixed"})) + assert_that(subject.converted_unsafe_reasons, equal_to(expected_reasons)) output = subject.apply_to_string() assert_that(output, contains_string("def first[T](items: list[T]) -> T:")) assert_that(output, contains_string(" def get(self, x: T) -> T:")) - def test_reports_typevar_used_only_in_a_generic_class(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + def test_reports_typevar_used_only_in_a_generic_class(self, create_type_parameter_check: Callable[[str], TypeParameterCheck]) -> 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(""" + subject = create_type_parameter_check(""" from typing import Generic, TypeVar T = TypeVar('T') @@ -293,9 +396,11 @@ 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: + def test_ignores_methods_using_their_class_type_parameter( + self, create_type_parameter_check: Callable[[str], TypeParameterCheck] + ) -> 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(""" + subject = create_type_parameter_check(""" from typing import TypeVar T = TypeVar('T') @@ -318,10 +423,10 @@ def get(self, x: T) -> T: ], ) def test_converts_method_of_a_class_not_generic_over_the_name( - self, create_type_var_check: Callable[[str], TypeVarCheck], class_header: str + self, create_type_parameter_check: Callable[[str], TypeParameterCheck], class_header: str ) -> None: """Verify a method gets [T] when its class isn't generic over T.""" - subject = create_type_var_check(f""" + subject = create_type_parameter_check(f""" from typing import Generic, TypeVar T = TypeVar('T') U = TypeVar('U') @@ -335,11 +440,66 @@ def get(self, x: T) -> T: assert_that(result, has_entry("T", "fixed")) assert_that(subject.apply_to_string(), contains_string(" def get[T](self, x: T) -> T:")) + @pytest.mark.parametrize( + ("functions", "expected_converted", "expected_unchanged"), + [ + pytest.param( + "def f(x: U) -> U:\n return x\ndef g(x: U, y: Bad) -> Bad:\n return y\n", + "def f[U](x: U) -> U:", + "def g(x: U, y: Bad) -> Bad:", + id="functions", + ), + pytest.param( + "class Util:\n def f(self, x: U) -> U:\n return x\n def g(self, x: U, y: Bad) -> Bad:\n return y\n", + " def f[U](self, x: U) -> U:", + " def g(self, x: U, y: Bad) -> Bad:", + id="methods", + ), + ], + ) + def test_converts_only_functions_free_of_unconvertible_typevars( + self, + create_type_parameter_check: Callable[[str], TypeParameterCheck], + functions: str, + expected_converted: str, + expected_unchanged: str, + ) -> None: + """Verify a function that also uses an unconvertible TypeVar is left whole, while the others using U get [U].""" + subject = create_type_parameter_check( + f'from typing import TypeVar\n\nBad = TypeVar("Bad", future_kw=1)\nU = TypeVar("U")\n\n{functions}' + ) + result = subject.convert_declared_typevars() + + assert_that(result, equal_to({"Bad": "unsafe", "U": "fixed"})) + output = subject.apply_to_string() + assert_that(output, contains_string(expected_converted)) + assert_that(output, contains_string(expected_unchanged)) + + @pytest.mark.parametrize( + "class_declaration", + [ + pytest.param('T = TypeVar("T")', id="plain"), + pytest.param('T = TypeVar("T", covariant=True)', id="covariant"), + ], + ) + def test_converts_method_mixing_with_its_class_type_parameter( + self, create_type_parameter_check: Callable[[str], TypeParameterCheck], class_declaration: str + ) -> None: + """Verify a method of a class generic over T gets [U] while keeping the class's legacy T.""" + subject = create_type_parameter_check( + f'from typing import Generic, TypeVar\n\n{class_declaration}\nU = TypeVar("U")\n\n' + "class Box(Generic[T]):\n def pair(self, x: T, y: U) -> tuple[T, U]:\n return x, y\n" + ) + result = subject.convert_declared_typevars() + + assert_that(result, has_entry("U", "fixed")) + assert_that(subject.apply_to_string(), contains_string(" def pair[U](self, x: T, y: U) -> tuple[T, U]:")) + def test_converts_function_when_typevar_is_also_used_outside_functions( - self, create_type_var_check: Callable[[str], TypeVarCheck] + self, create_type_parameter_check: Callable[[str], TypeParameterCheck] ) -> None: """Verify a function using T is converted even though a module-level alias also uses T.""" - subject = create_type_var_check(""" + subject = create_type_parameter_check(""" from typing import TypeVar T = TypeVar('T') @@ -368,7 +528,7 @@ def _source(declarations: str, signature: str) -> str: return f"{_HEADER}{declarations}\n\n\n{signature}\n ...\n" -class TestTypeVarCheckConvertArguments: +class TestTypeParameterCheckConvertArguments: """Every constructor argument is carried over or refused with a reason, never lost silently.""" @pytest.mark.parametrize( @@ -391,6 +551,20 @@ class TestTypeVarCheckConvertArguments: id="typevar-constraints-default", ), pytest.param('T = TypeVar("T", infer_variance=True)', _TYPEVAR_USE, "def a[T](x: T) -> T:", id="typevar-infer-variance"), + pytest.param('T = TypeVar("T", covariant=True)', _TYPEVAR_USE, "def a[T](x: T) -> T:", id="typevar-covariant"), + pytest.param('T = TypeVar("T", contravariant=True)', _TYPEVAR_USE, "def a[T](x: T) -> T:", id="typevar-contravariant"), + pytest.param( + 'T = TypeVar("T", bound=int, covariant=True)', + _TYPEVAR_USE, + "def a[T: int](x: T) -> T:", + id="typevar-bound-covariant", + ), + pytest.param( + 'P = ParamSpec("P", covariant=True)', + _PARAMSPEC_USE, + "def a[**P](f: Callable[P, int]) -> Callable[P, int]:", + id="paramspec-covariant", + ), pytest.param( 'P = ParamSpec("P")', _PARAMSPEC_USE, @@ -414,14 +588,14 @@ class TestTypeVarCheckConvertArguments: ) def test_converts_keeping_every_expressible_argument( self, - create_type_var_check: Callable[[str], TypeVarCheck], + create_type_parameter_check: Callable[[str], TypeParameterCheck], declaration: str, signature: str, expected_signature: str, ) -> None: - """Verify bounds, constraints, defaults and inferred variance are carried over.""" + """Verify bounds, constraints and defaults are carried over, and variance arguments are dropped.""" name = declaration.split(" = ", maxsplit=1)[0] - subject = create_type_var_check(_source(declaration, signature)) + subject = create_type_parameter_check(_source(declaration, signature)) subject.min_python = PEP_696_MINIMUM result = subject.convert_declared_typevars() @@ -434,61 +608,33 @@ def test_converts_keeping_every_expressible_argument( @pytest.mark.parametrize( ("declaration", "signature", "min_python", "expected_reason"), [ - pytest.param( - 'T = TypeVar("T")', - _TYPEVAR_USE, - (3, 11), - UnsafeReason.PEP695_VERSION_GATE, - id="typevar-plain-3.11", - ), pytest.param( 'T = TypeVar("T", default=int)', _TYPEVAR_USE, - PEP_695_MINIMUM, + MIN_SUPPORTED_PYTHON, UnsafeReason.PEP696_VERSION_GATE, id="typevar-default-3.12", ), pytest.param( 'P = ParamSpec("P", default=[int, str])', _PARAMSPEC_USE, - PEP_695_MINIMUM, + MIN_SUPPORTED_PYTHON, UnsafeReason.PEP696_VERSION_GATE, id="paramspec-default-3.12", ), pytest.param( 'Ts = TypeVarTuple("Ts", default=Unpack[tuple[int]])', _TYPEVARTUPLE_USE, - PEP_695_MINIMUM, + MIN_SUPPORTED_PYTHON, UnsafeReason.PEP696_VERSION_GATE, id="typevartuple-default-3.12", ), pytest.param( - 'T = TypeVar("T", covariant=True)', - _TYPEVAR_USE, - PEP_696_MINIMUM, - UnsafeReason.NO_PEP695_EQUIVALENT, - id="typevar-covariant", - ), - pytest.param( - 'T = TypeVar("T", contravariant=True)', - _TYPEVAR_USE, - PEP_696_MINIMUM, - UnsafeReason.NO_PEP695_EQUIVALENT, - id="typevar-contravariant", - ), - pytest.param( - 'P = ParamSpec("P", covariant=True)', - _PARAMSPEC_USE, - PEP_696_MINIMUM, - UnsafeReason.NO_PEP695_EQUIVALENT, - id="paramspec-covariant", - ), - pytest.param( - 'Ts = TypeVarTuple("Ts", bound=int, covariant=True)', + 'Ts = TypeVarTuple("Ts", bound=int)', _TYPEVARTUPLE_USE, PEP_696_MINIMUM, UnsafeReason.NO_PEP695_EQUIVALENT, - id="typevartuple-bound-covariant", + id="typevartuple-bound", ), pytest.param( 'P = ParamSpec("P", bound=int)', @@ -515,7 +661,7 @@ def test_converts_keeping_every_expressible_argument( ) def test_refuses_argument_it_cannot_carry_over( self, - create_type_var_check: Callable[[str], TypeVarCheck], + create_type_parameter_check: Callable[[str], TypeParameterCheck], declaration: str, signature: str, min_python: tuple[int, int], @@ -524,7 +670,7 @@ def test_refuses_argument_it_cannot_carry_over( """Verify an argument the target can't express is reported unsafe with its reason and the file is left unchanged.""" name = declaration.split(" = ", maxsplit=1)[0] source = _source(declaration, signature) - subject = create_type_var_check(source) + subject = create_type_parameter_check(source) subject.min_python = min_python result = subject.convert_declared_typevars() @@ -533,6 +679,51 @@ def test_refuses_argument_it_cannot_carry_over( assert_that(subject.converted_unsafe_reasons, has_entry(name, expected_reason)) assert_that(subject.apply_to_string(), equal_to(source)) + @pytest.mark.parametrize( + ("declarations", "signature", "min_python", "expected_reasons"), + [ + pytest.param( + 'Bad = TypeVar("Bad", future_kw=1)\nTs = TypeVarTuple("Ts")', + "def a(func: Callable[[*Ts], Bad], *args: *Ts) -> Bad:", + PEP_696_MINIMUM, + {"Bad": UnsafeReason.NO_PEP695_EQUIVALENT, "Ts": UnsafeReason.SHARES_FUNCTION_WITH_UNCONVERTIBLE}, + id="unconvertible-typevar-with-typevartuple", + ), + pytest.param( + 'T = TypeVar("T")\nD = TypeVar("D", default=int)', + "def a(x: T, y: D) -> T | D:", + MIN_SUPPORTED_PYTHON, + {"D": UnsafeReason.PEP696_VERSION_GATE, "T": UnsafeReason.SHARES_FUNCTION_WITH_UNCONVERTIBLE}, + id="version-gated-default-declared-last", + ), + pytest.param( + 'P = ParamSpec("P", bound=int)\nT = TypeVar("T")', + "def a(f: Callable[P, T]) -> Callable[P, T]:", + PEP_696_MINIMUM, + {"P": UnsafeReason.NO_PEP695_EQUIVALENT, "T": UnsafeReason.SHARES_FUNCTION_WITH_UNCONVERTIBLE}, + id="bound-paramspec-with-typevar", + ), + ], + ) + def test_leaves_function_using_an_unconvertible_typevar_unchanged( + self, + create_type_parameter_check: Callable[[str], TypeParameterCheck], + declarations: str, + signature: str, + min_python: tuple[int, int], + expected_reasons: dict[str, UnsafeReason], + ) -> None: + """Verify a function using one unconvertible TypeVar gets none of its TypeVars converted, so it never mixes syntaxes.""" + source = _source(declarations, signature) + subject = create_type_parameter_check(source) + subject.min_python = min_python + + result = subject.convert_declared_typevars() + + assert_that(result, equal_to(dict.fromkeys(expected_reasons, "unsafe"))) + assert_that(subject.converted_unsafe_reasons, equal_to(expected_reasons)) + assert_that(subject.apply_to_string(), equal_to(source)) + @pytest.mark.parametrize( ("declarations", "signature", "expected_signature"), [ @@ -558,13 +749,13 @@ def test_refuses_argument_it_cannot_carry_over( ) def test_places_defaulted_type_params_last( self, - create_type_var_check: Callable[[str], TypeVarCheck], + create_type_parameter_check: Callable[[str], TypeParameterCheck], declarations: str, signature: str, expected_signature: str, ) -> None: """Verify type parameters with a default come after those without one, which Python requires.""" - subject = create_type_var_check(_source(declarations, signature)) + subject = create_type_parameter_check(_source(declarations, signature)) subject.min_python = PEP_696_MINIMUM subject.convert_declared_typevars() @@ -572,23 +763,3 @@ def test_places_defaulted_type_params_last( output = subject.apply_to_string() ast.parse(output) assert_that(output, contains_string(expected_signature)) - - @pytest.mark.xfail( - reason="A default referencing another legacy declaration is carried over as is, so the new type " - "parameter's default refers to the module-level TypeVar.", - raises=AssertionError, - strict=True, - ) - def test_does_not_convert_default_referencing_another_declaration( - self, - create_type_var_check: Callable[[str], TypeVarCheck], - ) -> None: - """Verify a declaration whose default uses another legacy type parameter is reported unsafe and left unconverted.""" - subject = create_type_var_check(_source('T = TypeVar("T")\nU = TypeVar("U", default=T)', "def f(x: U) -> U:")) - subject.min_python = PEP_696_MINIMUM - - result = subject.convert_declared_typevars() - - assert_that(result, has_entry("U", "unsafe")) - assert_that(subject.converted_unsafe_reasons, has_entry("U", not_(UnsafeReason.PEP696_VERSION_GATE))) - assert_that(subject.apply_to_string(), contains_string("def f(x: U) -> U:")) diff --git a/test/recipes/test_type_parameter_check_localize.py b/test/recipes/test_type_parameter_check_localize.py new file mode 100644 index 000000000..a10901364 --- /dev/null +++ b/test/recipes/test_type_parameter_check_localize.py @@ -0,0 +1,355 @@ +# 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 how TypeParameterCheck.localize_imported_typevars writes the localizations that plan_localizations decides.""" + +import ast +import textwrap +from pathlib import Path + +import pytest +from hamcrest import all_of, assert_that, contains_string, equal_to, has_entry, has_length, not_ +from pytest_mock import MockerFixture + +from renaissance.integrations.python.ast.cst_node import PythonCstNode +from renaissance.recipes.type_parameter_check import TypeParameterCheck + + +def _imports_after_code(module_text: str) -> list[str]: + """Return the module-level imports of module_text that come after its first non-import statement.""" + body = ast.parse(module_text).body + first_code = next((index for index, node in enumerate(body) if not isinstance(node, ast.Import | ast.ImportFrom)), len(body)) + return [ast.unparse(node) for node in body[first_code:] if isinstance(node, ast.Import | ast.ImportFrom)] + + +class TestTypeParameterCheckLocalize: + """See module docstring.""" + + def _create_cross_file( + self, + mocker: MockerFixture, + tmp_path: Path, + origin_text: str, + importing_text: str, + layout: tuple[str, str] = ("file_1.py", "file_2.py"), + ) -> TypeParameterCheck: + """Write origin_text to the origin file and return an in-memory TypeParameterCheck on the importing file holding importing_text. + + layout gives the (origin, importing) file paths, relative to tmp_path, which is the project root. + """ + origin_rel_path, importing_rel_path = layout + origin_file = tmp_path / origin_rel_path + origin_file.parent.mkdir(parents=True, exist_ok=True) + origin_file.write_text(textwrap.dedent(origin_text)) + + importing_code = textwrap.dedent(importing_text) + importing_file = str(tmp_path / importing_rel_path) + mocker.patch( + "renaissance.integrations.python.ast.factory.PythonFactory.create", + return_value=PythonCstNode.load_from_text(importing_code, importing_file), + ) + subject = TypeParameterCheck(importing_file) + subject.in_memory = True + subject.project_root = tmp_path + return subject + + @pytest.mark.parametrize( + ("dunder_all", "imported_elsewhere"), + [ + pytest.param('__all__ = ["T"]', frozenset[str](), id="re-exported-via-dunder-all"), + pytest.param("", frozenset({"T"}), id="imported-from-here-elsewhere"), + ], + ) + def test_localizes_typevar_this_file_re_exports( + self, + mocker: MockerFixture, + tmp_path: Path, + dunder_all: str, + imported_elsewhere: frozenset[str], + ) -> None: + """Verify an imported TypeVar is localized even when this file passes it on to others.""" + subject = self._create_cross_file( + mocker, + tmp_path, + """ + from typing import TypeVar + T = TypeVar("T") + """, + f""" + from file_1 import T + {dunder_all} + def b(x: T) -> T: + return x + """, + ) + subject.project_wide_imported_names = imported_elsewhere + + result = subject.localize_imported_typevars() + + assert_that(result, equal_to({"T": "fixed"})) + output = subject.apply_to_string() + assert_that(output, contains_string('T = TypeVar("T")')) + assert_that(output, not_(contains_string("from file_1 import T"))) + + @pytest.mark.parametrize( + ("layout", "importing_import", "expected_line"), + [ + pytest.param(("file_1.py", "file_2.py"), "from file_1 import T, helper", "from file_1 import helper", id="absolute"), + pytest.param(("file_1.py", "file_2.py"), "from .file_1 import T, helper", "from .file_1 import helper", id="relative"), + pytest.param( + ("file_1.py", "sub/use.py"), + "from ..file_1 import T, helper", + "from ..file_1 import helper", + id="relative-parent", + ), + pytest.param(("__init__.py", "file_2.py"), "from . import T, helper", "from . import helper", id="bare-package"), + pytest.param( + ("file_1.py", "file_2.py"), + "from file_1 import T, helper as h", + "from file_1 import helper as h", + id="alias-kept", + ), + ], + ) + def test_keeps_other_names_when_localizing_one_of_several_imports( + self, + mocker: MockerFixture, + tmp_path: Path, + layout: tuple[str, str], + importing_import: str, + expected_line: str, + ) -> None: + """Verify localizing one imported name keeps the other names imported from the same module, alias included.""" + subject = self._create_cross_file( + mocker, + tmp_path, + 'from typing import TypeVar\nT = TypeVar("T")\ndef helper() -> None:\n pass\n', + f"{importing_import}\ndef b(x: T) -> T:\n return x\n", + layout, + ) + result = subject.localize_imported_typevars() + + assert_that(result, has_entry("T", "fixed")) + output = subject.apply_to_string() + assert_that(output.splitlines().count(expected_line), equal_to(1)) + assert_that(output, contains_string('T = TypeVar("T")')) + + @pytest.mark.parametrize( + ("importing_import", "comments"), + [ + pytest.param("from file_1 import T, helper # noqa: F401", ["# noqa: F401"], id="trailing-comment"), + pytest.param( + "from file_1 import ( # header\n T, # the typevar\n helper, # the helper\n)", + ["# header", "# the helper"], + id="multi-line", + ), + ], + ) + def test_narrowing_an_import_keeps_its_comments( + self, + mocker: MockerFixture, + tmp_path: Path, + importing_import: str, + comments: list[str], + ) -> None: + """Verify each comment of a narrowed import is kept once, and never on the localized declaration's line.""" + subject = self._create_cross_file( + mocker, + tmp_path, + 'from typing import TypeVar\nT = TypeVar("T")\ndef helper() -> None:\n pass\n', + f"{importing_import}\ndef b(x: T) -> T:\n helper()\n return x\n", + ) + result = subject.localize_imported_typevars() + + assert_that(result, has_entry("T", "fixed")) + lines = subject.apply_to_string().splitlines() + for comment in comments: + holders = [line for line in lines if comment in line] + assert_that(holders, has_length(1), comment) + assert_that(holders[0], not_(contains_string("TypeVar(")), comment) + + @pytest.mark.parametrize( + ("origin_extra", "importing_import", "expected_lines"), + [ + pytest.param( + 'T = TypeVar("T")\nU = TypeVar("U")', + "from file_1 import T, U", + ["from typing import TypeVar", 'T = TypeVar("T")', 'U = TypeVar("U")'], + id="two-names", + ), + pytest.param( + 'T = TypeVar("T")\nU = TypeVar("U")\ndef helper() -> None:\n pass', + "from file_1 import T, helper, U", + ["from file_1 import helper", "from typing import TypeVar", 'T = TypeVar("T")', 'U = TypeVar("U")'], + id="keeps-other-name", + ), + pytest.param( + 'class Shape:\n pass\nclass Circle(Shape):\n pass\nT = TypeVar("T", bound=Shape)\nU = TypeVar("U", bound=Circle)', + "from file_1 import T, U", + [ + "from file_1 import Circle", + "from file_1 import Shape", + "from typing import TypeVar", + 'T = TypeVar("T", bound=Shape)', + 'U = TypeVar("U", bound=Circle)', + ], + id="bounds-each-imported-once", + ), + ], + ) + def test_localizes_two_names_from_one_import_statement( + self, + mocker: MockerFixture, + tmp_path: Path, + origin_extra: str, + importing_import: str, + expected_lines: list[str], + ) -> None: + """Verify one import statement bringing in two localizable names localizes both, adding each needed import once.""" + subject = self._create_cross_file( + mocker, + tmp_path, + f"from typing import TypeVar\n{origin_extra}\n", + f"{importing_import}\ndef b(x: T, y: U) -> T:\n return x\n", + ) + result = subject.localize_imported_typevars() + + assert_that(result, equal_to({"T": "fixed", "U": "fixed"})) + output = subject.apply_to_string() + for line in expected_lines: + assert_that(output.splitlines().count(line), equal_to(1), line) + still_imported = { + alias.name + for node in ast.parse(output).body + if isinstance(node, ast.ImportFrom) and node.module == "file_1" + for alias in node.names + } + assert_that(still_imported & {"T", "U"}, equal_to(set())) + + @pytest.mark.parametrize( + "importing_text", + [ + pytest.param("from file_1 import T\nimport os\nfrom collections import OrderedDict\n", id="import-after-removed"), + pytest.param("import os\nfrom file_1 import T\n", id="removed-is-last-import"), + pytest.param("from __future__ import annotations\nfrom file_1 import T\nimport os\n", id="future-import-present"), + pytest.param("from file_1 import T, helper\nimport os\n", id="narrowed-import"), + pytest.param("from file_1 import T\n", id="only-import"), + ], + ) + def test_localized_declaration_comes_after_every_import(self, mocker: MockerFixture, tmp_path: Path, importing_text: str) -> None: + """Verify the declaration is placed after the import block, so no import is left below code (E402).""" + subject = self._create_cross_file( + mocker, + tmp_path, + 'from typing import TypeVar\nT = TypeVar("T")\ndef helper() -> None:\n pass\n', + f"{importing_text}def b(x: T) -> T:\n return x\n", + ) + result = subject.localize_imported_typevars() + + assert_that(result, equal_to({"T": "fixed"})) + output = subject.apply_to_string() + assert_that(output, contains_string('T = TypeVar("T")')) + assert_that(_imports_after_code(output), equal_to([])) + assert_that(output.count("from typing import TypeVar"), equal_to(1)) + + @pytest.mark.parametrize( + "dropped_import", + [ + pytest.param("from file_1 import T # noqa: F401", id="trailing-comment"), + pytest.param("from file_1 import (\n T,\n) # noqa: F401", id="multi-line-trailing-comment"), + ], + ) + def test_dropping_an_import_drops_the_comment_on_its_line(self, mocker: MockerFixture, tmp_path: Path, dropped_import: str) -> None: + """Verify a comment trailing a fully localized import goes with it instead of staying behind on a line of its own.""" + subject = self._create_cross_file( + mocker, + tmp_path, + 'from typing import TypeVar\nT = TypeVar("T")\n', + f"import os\n{dropped_import}\nimport sys\ndef b(x: T) -> T:\n return x\n", + ) + result = subject.localize_imported_typevars() + + assert_that(result, equal_to({"T": "fixed"})) + output = subject.apply_to_string() + assert_that(output, not_(contains_string("noqa"))) + assert_that(_imports_after_code(output), equal_to([])) + + @pytest.mark.parametrize( + ("origin_extra", "expected_once"), + [ + pytest.param('T = TypeVar("T")\nU = TypeVar("U")', ["from typing import TypeVar"], id="constructor"), + pytest.param( + 'class Shape:\n pass\nT = TypeVar("T", bound=Shape)\nU = TypeVar("U", bound=Shape)', + ["from typing import TypeVar", "from file_1 import Shape"], + id="constructor-and-bound-name", + ), + ], + ) + def test_each_needed_import_is_added_once_per_file( + self, + mocker: MockerFixture, + tmp_path: Path, + origin_extra: str, + expected_once: list[str], + ) -> None: + """Verify names localized from separate import statements share the imports they need, one line each per file.""" + subject = self._create_cross_file( + mocker, + tmp_path, + f"from typing import TypeVar\n{origin_extra}\n", + "from file_1 import T\nfrom file_1 import U\ndef b(x: T, y: U) -> T:\n return x\n", + ) + result = subject.localize_imported_typevars() + + assert_that(result, equal_to({"T": "fixed", "U": "fixed"})) + output = subject.apply_to_string() + for line in expected_once: + assert_that(output.splitlines().count(line), equal_to(1), line) + + def test_declaration_stays_before_a_statement_that_uses_it_between_imports( + self, + mocker: MockerFixture, + tmp_path: Path, + ) -> None: + """Verify a declaration is not moved below a module-level statement that already needs it.""" + subject = self._create_cross_file( + mocker, + tmp_path, + 'from typing import TypeVar\nT = TypeVar("T")\n', + "from typing import Generic\nfrom file_1 import T\nclass Box(Generic[T]):\n pass\nimport os\n", + ) + result = subject.localize_imported_typevars() + + assert_that(result, equal_to({"T": "fixed"})) + output = subject.apply_to_string() + # Running the module is what proves the declaration is bound before `Box` uses it. + exec(compile(output, "importing.py", "exec"), {}) # noqa: S102 + + def test_check_localizes_then_converts(self, mocker: MockerFixture, tmp_path: Path) -> None: + """Verify check() localizes a cross-file TypeVar, then converts it.""" + subject = self._create_cross_file( + mocker, + tmp_path, + """ + from typing import TypeVar + T = TypeVar("T") + def a(x: T) -> T: + return x + """, + """ + from file_1 import T + def b(x: T) -> T: + return x + """, + ) + subject.run() + + assert_that(subject.result["cross_file"], has_entry("T", "fixed")) + assert_that(subject.result["converted"], has_entry("T", "fixed")) + assert_that( + subject.apply_to_string(), + all_of(contains_string("def b[T](x: T) -> T:"), not_(contains_string("T = TypeVar"))), + ) diff --git a/test/recipes/test_type_var_check_orphaned.py b/test/recipes/test_type_parameter_check_orphaned.py similarity index 63% rename from test/recipes/test_type_var_check_orphaned.py rename to test/recipes/test_type_parameter_check_orphaned.py index dc1c13560..b83b62227 100644 --- a/test/recipes/test_type_var_check_orphaned.py +++ b/test/recipes/test_type_parameter_check_orphaned.py @@ -4,7 +4,7 @@ # of this repository, also available at: # https://github.com/TNO/Renaissance.Py/blob/main/LICENSE -"""Tests for TypeVarCheck.remove_orphaned_declarations.""" +"""Tests for TypeParameterCheck.remove_orphaned_declarations.""" import textwrap from collections.abc import Callable @@ -12,8 +12,8 @@ import pytest from hamcrest import assert_that, contains_string, equal_to, has_entry, not_ -from renaissance.recipes.type_var_check import TypeVarCheck -from renaissance.recipes.type_var_domain import UnsafeReason +from renaissance.recipes.type_parameter_check import TypeParameterCheck +from renaissance.recipes.type_parameter_rules import UnsafeReason ALREADY_CONVERTED_SOURCE = """ from typing import TypeVar @@ -60,29 +60,26 @@ def get(self, x: T) -> T: """ -class TestTypeVarCheckOrphaned: +class TestTypeParameterCheckOrphaned: """See module docstring.""" @pytest.mark.parametrize( - ("source", "min_python"), + "source", [ - pytest.param(ALREADY_CONVERTED_SOURCE, (3, 12), id="already-converted"), - pytest.param(ALREADY_CONVERTED_SOURCE, (3, 11), id="already-converted-below-pep695"), - pytest.param(UNREFERENCED_SOURCE, (3, 11), id="unreferenced-below-pep695"), - pytest.param(NESTED_ONLY_CONVERTED_SOURCE, (3, 12), id="only-nested-function-converted"), - pytest.param(CLOSURE_IN_CONVERTED_FUNCTION_SOURCE, (3, 12), id="closure-inside-converted-function"), - pytest.param(CLASS_WITH_OWN_TYPE_PARAMETER_SOURCE, (3, 12), id="class-with-own-type-parameter"), + pytest.param(ALREADY_CONVERTED_SOURCE, id="already-converted"), + pytest.param(UNREFERENCED_SOURCE, id="unreferenced"), + pytest.param(NESTED_ONLY_CONVERTED_SOURCE, id="only-nested-function-converted"), + pytest.param(CLOSURE_IN_CONVERTED_FUNCTION_SOURCE, id="closure-inside-converted-function"), + pytest.param(CLASS_WITH_OWN_TYPE_PARAMETER_SOURCE, id="class-with-own-type-parameter"), ], ) def test_removes_declaration_whose_references_are_all_shadowed( self, - create_type_var_check: Callable[[str], TypeVarCheck], + create_type_parameter_check: Callable[[str], TypeParameterCheck], source: str, - min_python: tuple[int, int], ) -> None: """Verify a declaration whose references are all shadowed by PEP 695 parameters, or absent, is removed.""" - subject = create_type_var_check(source) - subject.min_python = min_python + subject = create_type_parameter_check(source) result = subject.remove_orphaned_declarations() @@ -92,9 +89,30 @@ def test_removes_declaration_whose_references_are_all_shadowed( for def_line in (line.strip() for line in textwrap.dedent(source).splitlines() if line.strip().startswith("def ")): assert_that(output, contains_string(def_line)) - def test_keeps_declaration_still_used_without_pep695(self, create_type_var_check: Callable[[str], TypeVarCheck]) -> None: + @pytest.mark.parametrize( + "declaration", + [ + pytest.param("T = TypeVar('T') # noqa: PLC0105", id="trailing-comment"), + pytest.param("T = TypeVar(\n 'T',\n bound=int,\n) # noqa: PLC0105", id="multi-line-trailing-comment"), + ], + ) + def test_removing_a_declaration_removes_the_comment_trailing_it( + self, + create_type_parameter_check: Callable[[str], TypeParameterCheck], + declaration: str, + ) -> None: + """Verify the comment trailing a removed declaration goes with it instead of staying behind on a line of its own.""" + subject = create_type_parameter_check(f"from typing import TypeVar\n{declaration}\n\ndef b() -> None:\n pass\n") + + result = subject.remove_orphaned_declarations() + + assert_that(result, equal_to({"T": "fixed"})) + output = subject.apply_to_string() + assert_that(output, not_(contains_string("noqa"))) + + def test_keeps_declaration_still_used_without_pep695(self, create_type_parameter_check: Callable[[str], TypeParameterCheck]) -> None: """Verify a declaration still used by a function without its own type parameter is left alone, unreported.""" - subject = create_type_var_check(""" + subject = create_type_parameter_check(""" from typing import TypeVar T = TypeVar('T') @@ -116,10 +134,10 @@ def b(y: T) -> T: ], ) def test_keeps_declaration_used_in_a_string_annotation( - self, create_type_var_check: Callable[[str], TypeVarCheck], annotation: str + self, create_type_parameter_check: Callable[[str], TypeParameterCheck], annotation: str ) -> None: """Verify a declaration referenced only from a string annotation is not removed as orphaned.""" - subject = create_type_var_check(f""" + subject = create_type_parameter_check(f""" from typing import TypeVar T = TypeVar('T') @@ -131,35 +149,24 @@ def b(x: {annotation}) -> None: assert_that(result, equal_to({})) assert_that(subject.apply_to_string(), contains_string("T = TypeVar('T')")) - @pytest.mark.parametrize( - ("imported_elsewhere", "expected_reason"), - [ - pytest.param(frozenset[str](), UnsafeReason.DECLARED_TYPEVAR_EXPORTED, id="exported-via-dunder-all"), - pytest.param(frozenset({"T"}), UnsafeReason.IMPORTED_ELSEWHERE_IN_PROJECT, id="imported-elsewhere"), - ], - ) - def test_keeps_orphaned_declaration_that_is_unsafe_to_remove( - self, - create_type_var_check: Callable[[str], TypeVarCheck], - imported_elsewhere: frozenset[str], - expected_reason: UnsafeReason, + def test_keeps_orphaned_declaration_imported_elsewhere( + self, create_type_parameter_check: Callable[[str], TypeParameterCheck] ) -> None: """Verify an orphaned declaration is kept and reported unsafe when removing it would break an importer.""" - source = ALREADY_CONVERTED_SOURCE if imported_elsewhere else '__all__ = ["T"]\n' + ALREADY_CONVERTED_SOURCE - subject = create_type_var_check(source) - subject.project_wide_imported_names = imported_elsewhere + subject = create_type_parameter_check(ALREADY_CONVERTED_SOURCE) + subject.project_wide_imported_names = frozenset({"T"}) result = subject.remove_orphaned_declarations() assert_that(result, equal_to({"T": "unsafe"})) - assert_that(subject.orphaned_unsafe_reasons, equal_to({"T": expected_reason})) + assert_that(subject.orphaned_unsafe_reasons, equal_to({"T": UnsafeReason.IMPORTED_ELSEWHERE_IN_PROJECT})) assert_that(subject.apply_to_string(), contains_string("T = TypeVar('T')")) def test_removing_orphaned_declaration_keeps_comment_shared_with_next_declaration( - self, create_type_var_check: Callable[[str], TypeVarCheck] + self, create_type_parameter_check: Callable[[str], TypeParameterCheck] ) -> None: """Verify removing an orphaned declaration keeps a leading comment that also documents the next declaration.""" - subject = create_type_var_check(""" + subject = create_type_parameter_check(""" from typing import TypeVar # explains both T and U below diff --git a/test/recipes/test_type_parameter_check_properties.py b/test/recipes/test_type_parameter_check_properties.py new file mode 100644 index 000000000..49632cade --- /dev/null +++ b/test/recipes/test_type_parameter_check_properties.py @@ -0,0 +1,107 @@ +# Copyright (C) 2024-2026 TNO +# +# Licensed under the terms specified in the LICENSE file in the root directory +# of this repository, also available at: +# https://github.com/TNO/Renaissance.Py/blob/main/LICENSE + +"""Property-based tests for the TypeParameterCheck phases on generated modules.""" + +import ast +from pathlib import Path +from typing import TYPE_CHECKING, NamedTuple +from unittest.mock import patch + +import pytest +from hamcrest import assert_that, equal_to +from hypothesis import example, given, settings +from hypothesis import strategies as st + +from renaissance.integrations.python.ast.cst_node import PythonCstNode +from renaissance.recipes.type_parameter_check import TypeParameterCheck + +if TYPE_CHECKING: + from collections.abc import Callable + + +class _Declaration(NamedTuple): + """A module-level type parameter declaration and a parameter that uses it.""" + + constructor: str + statement: str + parameter: str + + +_DECLARATIONS = { + "T": _Declaration("TypeVar", 'T = TypeVar("T")', "x: T"), + "B": _Declaration("TypeVar", 'B = TypeVar("B", bound=int)', "x: B"), + "D": _Declaration("TypeVar", 'D = TypeVar("D", default=int)', "x: D"), + "P": _Declaration("ParamSpec", 'P = ParamSpec("P")', "f: Callable[P, int]"), + "Ts": _Declaration("TypeVarTuple", 'Ts = TypeVarTuple("Ts")', "*args: *Ts"), +} +_PLAIN_PARAMETER = "x: int" + +_PHASES = [ + pytest.param(TypeParameterCheck.check, id="check"), + pytest.param(TypeParameterCheck.convert_declared_typevars, id="convert"), + pytest.param(TypeParameterCheck.remove_orphaned_declarations, id="orphaned"), +] + + +@st.composite +def modules(draw: st.DrawFn, *, with_declarations: bool = True) -> str: + """Draw a module of type parameter declarations, functions using them, and optionally a plain and a generic class.""" + names = draw(st.lists(st.sampled_from(sorted(_DECLARATIONS)), max_size=3, unique=True)) if with_declarations else [] + parameters = [_DECLARATIONS[name].parameter for name in names] or [_PLAIN_PARAMETER] + generic_class = "T" in names and draw(st.booleans()) + + typing_names = {_DECLARATIONS[name].constructor for name in names} | ({"Generic"} if generic_class else set[str]()) + imports = [f"from typing import {', '.join(sorted(typing_names))}"] if typing_names else [] + if "P" in names: + imports.insert(0, "from collections.abc import Callable") + + blocks = ["\n".join(imports)] if imports else [] + if names: + blocks.append("\n".join(_DECLARATIONS[name].statement for name in names)) + blocks.extend( + f"def f{index}({parameter}) -> None:\n pass" + for index, parameter in enumerate(draw(st.lists(st.sampled_from(parameters), max_size=4))) + ) + if draw(st.booleans()): + blocks.append(f"class Plain:\n def method(self, {draw(st.sampled_from(parameters))}) -> None:\n pass") + if generic_class: + blocks.append("class Box(Generic[T]):\n def get(self, x: T) -> T:\n return x") + return "\n\n\n".join(blocks) + "\n" if blocks else "" + + +def _run(source: str, phase: Callable[[TypeParameterCheck], object]) -> str: + """Run one phase of TypeParameterCheck on source in memory and return the rewritten text.""" + with patch( + "renaissance.integrations.python.ast.factory.PythonFactory.create", + return_value=PythonCstNode.load_from_text(source, "x.py"), + ): + subject = TypeParameterCheck(Path("x.py")) + subject.in_memory = True + phase(subject) + return subject.apply_to_string() + + +class TestTypeParameterCheckProperties: + """Properties every phase must have on any module, whatever it converts or removes.""" + + @pytest.mark.parametrize("phase", _PHASES) + @settings(max_examples=50, deadline=None) + @given(source=modules()) + @example(source="") + def test_output_parses_and_a_second_run_changes_nothing(self, phase: Callable[[TypeParameterCheck], object], source: str) -> None: + """The phase leaves valid Python, and running it again on that result changes nothing.""" + once = _run(source, phase) + + ast.parse(once) + assert_that(_run(once, phase), equal_to(once)) + + @pytest.mark.parametrize("phase", _PHASES) + @settings(max_examples=50, deadline=None) + @given(source=modules(with_declarations=False)) + def test_module_without_type_parameters_is_left_unchanged(self, phase: Callable[[TypeParameterCheck], object], source: str) -> None: + """A module that declares no type parameter comes out identical.""" + assert_that(_run(source, phase), equal_to(source)) diff --git a/test/recipes/test_type_parameter_check_variance.py b/test/recipes/test_type_parameter_check_variance.py new file mode 100644 index 000000000..b14169113 --- /dev/null +++ b/test/recipes/test_type_parameter_check_variance.py @@ -0,0 +1,195 @@ +# Copyright (C) 2024-2026 TNO +# +# Licensed under the terms specified in the LICENSE file in the root directory +# of this repository, also available at: +# https://github.com/TNO/Renaissance.Py/blob/main/LICENSE + +"""Tests that dropping `covariant=`/`contravariant=` is safe for functions and unsafe for classes. + +The type checker (pyright) and the interpreter judge the code before and after TypeParameterCheck.convert_declared_typevars. +""" + +import importlib.util +import json +import subprocess +import sys +from typing import TYPE_CHECKING, NamedTuple + +import pytest +from hamcrest import assert_that, contains_string, equal_to, has_item, has_property, not_ + +from renaissance.recipes.type_parameter_rules import UnsafeReason + +# TODO(FrancescoPezzella): Replace with a lazy import in Python 3.15. +# https://github.com/TNO/Renaissance.Py/issues/275 +if TYPE_CHECKING: + from collections.abc import Callable + from pathlib import Path + from types import ModuleType + + from renaissance.recipes.type_parameter_check import TypeParameterCheck + + +class _Diagnostic(NamedTuple): + """One pyright finding, without the file it was found in.""" + + line: int + severity: str + rule: str + message: str + + +def _write(source: str, path: Path) -> Path: + """Write `source` to `path` and return `path`.""" + path.write_text(source, encoding="utf-8") + return path + + +def _type_check(path: Path) -> list[_Diagnostic]: + """Return every diagnostic pyright reports for the file at `path`, `reveal_type` results included.""" + command = [sys.executable, "-m", "pyright", "--pythonpath", sys.executable, "--outputjson", str(path)] + result = subprocess.run(command, capture_output=True, text=True, encoding="utf-8", cwd=path.parent, check=False) # noqa: S603 - fixed argv + report = json.loads(result.stdout[result.stdout.index("{") :]) + return [ + _Diagnostic(item["range"]["start"]["line"] + 1, item["severity"], item.get("rule", ""), item["message"]) + for item in report["generalDiagnostics"] + ] + + +def _import(path: Path) -> ModuleType: + """Import the file at `path` as a module.""" + spec = importlib.util.spec_from_file_location(path.stem, path) + if spec is None or spec.loader is None: + message = f"could not load {path}" + raise RuntimeError(message) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +_FUNCTION_TEMPLATE = """\ +from collections.abc import Callable +from typing import TypeVar, reveal_type + +{declaration} + + +def first(x: T) -> T: + return x + + +reveal_type(first(1)) +reveal_type(first("a")) +reveal_type(first([1])) +accepted: Callable[[bool], int] = first +rejected: Callable[[int], str] = first +""" + +_PROTOCOL = """\ +class Reader(Protocol[T_co]): + def read(self) -> T_co: ... +""" + +_GENERIC_CLASS = """\ +class Reader(Generic[T_co]): + def __init__(self, value: T_co) -> None: + self._value = value + + def read(self) -> T_co: + return self._value +""" + +_CLASS_TEMPLATE = """\ +from typing import Generic, Protocol, TypeVar, reveal_type + +T_co = TypeVar("T_co"{covariant}) + + +{class_definition} + +def read_twice(reader: Reader[T_co]) -> tuple[T_co, T_co]: + return reader.read(), reader.read() + + +def widen(reader: Reader[int]) -> None: + wider: Reader[object] = reader + reveal_type(wider) +""" + + +class TestVarianceOfFunctions: + """A function's TypeVar has no variance, so dropping it must not change anything.""" + + @pytest.mark.parametrize( + "declaration", + [ + pytest.param('T = TypeVar("T", covariant=True)', id="covariant"), + pytest.param('T = TypeVar("T", contravariant=True)', id="contravariant"), + pytest.param('T = TypeVar("T", bound=int, covariant=True)', id="bound-and-covariant"), + ], + ) + def test_function_behaves_the_same_without_the_variance( + self, + create_type_parameter_check: Callable[[str], TypeParameterCheck], + tmp_path: Path, + declaration: str, + ) -> None: + """Verify the type checker accepts, rejects and infers exactly as before, and the function returns the same values.""" + before = _FUNCTION_TEMPLATE.format(declaration=declaration) + subject = create_type_parameter_check(before) + + result = subject.convert_declared_typevars() + + after = subject.apply_to_string() + assert_that(result, equal_to({"T": "fixed"})) + assert_that(after, contains_string("def first[T")) + before_path = _write(before, tmp_path / "before.py") + after_path = _write(after, tmp_path / "after.py") + before_findings = _type_check(before_path) + assert_that(before_findings, has_item(has_property("message", contains_string('Type of "first(1)" is "int"')))) + assert_that(before_findings, has_item(has_property("rule", "reportAssignmentType"))) + assert_that(_type_check(after_path), equal_to(before_findings)) + values = (1, "a", [1]) + assert_that( + [_import(after_path).first(value) for value in values], + equal_to([_import(before_path).first(value) for value in values]), + ) + + +class TestVarianceOfClasses: + """A generic class's variance changes what the type checker accepts, so only functions may lose it.""" + + @pytest.mark.parametrize( + ("class_definition", "class_header"), + [ + pytest.param(_PROTOCOL, "class Reader(Protocol[T_co]):", id="protocol"), + pytest.param(_GENERIC_CLASS, "class Reader(Generic[T_co]):", id="generic-class"), + ], + ) + def test_class_keeps_its_variance_and_dropping_it_changes_the_type_checking( + self, + create_type_parameter_check: Callable[[str], TypeParameterCheck], + tmp_path: Path, + class_definition: str, + class_header: str, + ) -> None: + """Verify the class and its covariant declaration stay, the code still type checks, and dropping the variance does not.""" + covariant = _CLASS_TEMPLATE.format(covariant=", covariant=True", class_definition=class_definition) + subject = create_type_parameter_check(covariant) + + result = subject.convert_declared_typevars() + + converted = subject.apply_to_string() + assert_that(result, equal_to({"T_co": "fixed"})) + assert_that(subject.converted_unsafe_reasons, equal_to({"T_co": UnsafeReason.USED_IN_GENERIC_CLASS})) + assert_that(converted, contains_string("def read_twice[T_co](reader: Reader[T_co])")) + assert_that(converted, contains_string('T_co = TypeVar("T_co", covariant=True)')) + assert_that(converted, contains_string(class_header)) + original_findings = _type_check(_write(covariant, tmp_path / "original.py")) + assert_that(original_findings, has_item(has_property("message", contains_string('Type of "wider" is "Reader[int]"')))) + assert_that(_type_check(_write(converted, tmp_path / "converted.py")), equal_to(original_findings)) + assert_that([finding.rule for finding in original_findings], not_(has_item("reportAssignmentType"))) + + invariant = converted.replace(", covariant=True", "") + invariant_rules = [finding.rule for finding in _type_check(_write(invariant, tmp_path / "invariant.py"))] + assert_that(invariant_rules, has_item("reportAssignmentType")) diff --git a/test/recipes/test_type_parameter_declarations.py b/test/recipes/test_type_parameter_declarations.py new file mode 100644 index 000000000..71fa0c532 --- /dev/null +++ b/test/recipes/test_type_parameter_declarations.py @@ -0,0 +1,38 @@ +# Copyright (C) 2024-2026 TNO +# +# Licensed under the terms specified in the LICENSE file in the root directory +# of this repository, also available at: +# https://github.com/TNO/Renaissance.Py/blob/main/LICENSE + +"""Tests for renaissance.recipes.type_parameter_declarations.""" + +from typing import cast + +import libcst as cst +import pytest +from hamcrest import assert_that, equal_to + +from renaissance.recipes.type_parameter_declarations import declaration_argument_names + + +class TestDeclarationArgumentNames: + """See module docstring.""" + + @pytest.mark.parametrize( + ("declaration", "expected"), + [ + pytest.param('T = TypeVar("T")', set(), id="the-type-parameters-own-name-is-skipped"), + pytest.param('T = TypeVar("T", bound=Shape)', {"Shape"}, id="keyword-argument"), + pytest.param('T = TypeVar("T", Shape, Other)', {"Shape", "Other"}, id="constraints"), + pytest.param('T = TypeVar("T", bound=np.integer)', {"np"}, id="attribute-reads-only-its-root"), + pytest.param('T = TypeVar("T", bound="Shape")', {"Shape"}, id="string-is-a-forward-reference"), + pytest.param('T = TypeVar("T", bound="not valid(")', set(), id="string-that-is-not-an-expression"), + pytest.param('T = TypeVar("T", int, str)', set(), id="built-ins-are-left-out"), + pytest.param('T = TypeVar("T", **options)', {"options"}, id="mapping-argument"), + ], + ) + def test_returns_the_non_builtin_names_the_arguments_use(self, declaration: str, expected: set[str]) -> None: + """Verify the names the arguments read are returned, with the declared name and the built-ins left out.""" + line = cast("cst.SimpleStatementLine", cst.parse_statement(declaration)) + + assert_that(declaration_argument_names(cast("cst.Assign", line.body[0])), equal_to(expected)) diff --git a/test/recipes/test_type_parameter_localization.py b/test/recipes/test_type_parameter_localization.py new file mode 100644 index 000000000..b1320d1a8 --- /dev/null +++ b/test/recipes/test_type_parameter_localization.py @@ -0,0 +1,384 @@ +# Copyright (C) 2024-2026 TNO +# +# Licensed under the terms specified in the LICENSE file in the root directory +# of this repository, also available at: +# https://github.com/TNO/Renaissance.Py/blob/main/LICENSE + +"""Tests for renaissance.recipes.type_parameter_localization.plan_localizations.""" + +import ast +import builtins +import textwrap +from pathlib import Path +from typing import cast + +import libcst as cst +import pytest +from hamcrest import assert_that, equal_to, has_entry, has_item, has_length, not_ + +from renaissance.recipes.type_parameter_localization import LocalizationPlan, plan_localizations +from renaissance.recipes.type_parameter_rules import UnsafeReason + + +def _module_level_names(module: ast.Module) -> set[str]: + """Return the names bound at module level by imports, classes, functions and assignments.""" + names: set[str] = set() + for node in module.body: + if isinstance(node, ast.Import | ast.ImportFrom): + names.update((alias.asname or alias.name).split(".")[0] for alias in node.names) + elif isinstance(node, ast.ClassDef | ast.FunctionDef | ast.AsyncFunctionDef): + names.add(node.name) + elif isinstance(node, ast.Assign): + names.update(target.id for target in node.targets if isinstance(target, ast.Name)) + return names + + +def _names_used_by(call: ast.Call) -> set[str]: + """Return the non-builtin names a TypeVar call's arguments use, including inside string forward references.""" + names: set[str] = set() + for argument in [*call.args[1:], *(keyword.value for keyword in call.keywords)]: + expression = argument + if isinstance(argument, ast.Constant) and isinstance(argument.value, str): + expression = ast.parse(argument.value, mode="eval").body + names.update(node.id for node in ast.walk(expression) if isinstance(node, ast.Name)) + return names - set(dir(builtins)) + + +def _write_origin(tmp_path: Path, origin_text: str, origin_rel_path: str) -> None: + """Write origin_text to origin_rel_path, relative to tmp_path, which is the project root.""" + origin_file = tmp_path / origin_rel_path + origin_file.parent.mkdir(parents=True, exist_ok=True) + origin_file.write_text(textwrap.dedent(origin_text)) + + +def _plan( + tmp_path: Path, + origin_text: str, + importing_text: str, + layout: tuple[str, str] = ("file_1.py", "file_2.py"), +) -> LocalizationPlan: + """Write origin_text to the origin file and return the plan for importing_text, whose file is not written. + + layout gives the (origin, importing) file paths, relative to tmp_path, which is the project root. + """ + origin_rel_path, importing_rel_path = layout + _write_origin(tmp_path, origin_text, origin_rel_path) + module = cst.parse_module(textwrap.dedent(importing_text)) + return plan_localizations(module, tmp_path / importing_rel_path, tmp_path, localize_imports=True, ignore_origins=frozenset()) + + +class TestPlanLocalizations: + """See module docstring.""" + + @pytest.mark.parametrize( + "origin_extra", + [ + pytest.param("def a(x: T) -> T:\n return x", id="plain-origin"), + pytest.param('__all__ = ["T"]', id="origin-exports-it"), + pytest.param("class Box(Generic[T]):\n pass", id="origin-class-is-generic-over-it"), + ], + ) + def test_localizes_typevar_whatever_its_origin_does_with_it(self, tmp_path: Path, origin_extra: str) -> None: + """Verify an imported TypeVar is localized, also when its origin exports it or has a class generic over it.""" + origin = f"from typing import Generic, TypeVar\nT = TypeVar('T')\n{origin_extra}\n" + plan = _plan(tmp_path, origin, "from file_1 import T\ndef b(x: T) -> T:\n return x\n") + + assert_that(plan.results, equal_to({"T": "fixed"})) + assert_that(plan.localizations, has_length(1)) + assert_that(plan.localizations[0].declarations, equal_to(["T = TypeVar('T')"])) + assert_that(plan.localizations[0].kept, equal_to([])) + + @pytest.mark.parametrize( + ("origin_ignored", "localize_imports", "localized"), + [ + pytest.param(True, True, False, id="ignored-origin"), + pytest.param(False, False, False, id="localization-off"), + pytest.param(False, True, True, id="origin-not-ignored"), + ], + ) + def test_names_are_not_localized_from_an_ignored_origin_or_when_localization_is_off( + self, + tmp_path: Path, + *, + origin_ignored: bool, + localize_imports: bool, + localized: bool, + ) -> None: + """Verify an import from an ignored origin, or any import with localize_imports off, is kept and recorded as ignored.""" + _write_origin(tmp_path, "from typing import TypeVar\nT = TypeVar('T')\n", "file_1.py") + plan = plan_localizations( + cst.parse_module("from file_1 import T\ndef b(x: T) -> T:\n return x\n"), + tmp_path / "file_2.py", + tmp_path, + localize_imports=localize_imports, + ignore_origins=frozenset({tmp_path / "file_1.py"}) if origin_ignored else frozenset(), + ) + + assert_that(plan.results, equal_to({"T": "fixed"} if localized else {})) + assert_that(plan.ignored_names, equal_to(set[str]() if localized else {"T"})) + assert_that(plan.localizations, has_length(1 if localized else 0)) + + @pytest.mark.parametrize( + "declaration", + [ + pytest.param('T = TypeVar("T", bound=Shape)', id="invariant-bound"), + pytest.param('T = TypeVar("T", int, Shape)', id="constraints"), + pytest.param('T = TypeVar("T", bound=Shape, covariant=True)', id="covariant"), + pytest.param('T = TypeVar("T", bound=Shape, contravariant=True)', id="contravariant"), + pytest.param('T = TypeVar("T", bound="Shape")', id="string-bound"), + ], + ) + def test_localized_declaration_keeps_its_meaning(self, tmp_path: Path, declaration: str) -> None: + """Verify the local copy has the origin's exact arguments and every name they use is defined by the imports it brings.""" + origin = f"from typing import TypeVar\nclass Shape:\n pass\n{declaration}\n" + plan = _plan(tmp_path, origin, "from file_1 import T\ndef biggest(items: list[T]) -> T:\n return items[0]\n") + + assert_that(plan.results, equal_to({"T": "fixed"})) + written = ast.parse("\n".join([*plan.localizations[0].imports, *plan.localizations[0].declarations])) + local_call = next(node.value for node in written.body if isinstance(node, ast.Assign)) + origin_call = cast("ast.Assign", ast.parse(origin).body[-1]).value + assert_that(ast.dump(local_call), equal_to(ast.dump(origin_call))) + assert_that(_names_used_by(cast("ast.Call", local_call)) - _module_level_names(written), equal_to(set())) + + @pytest.mark.parametrize( + ("origin_imports", "expected_line"), + [ + pytest.param("from typing import Any", "from typing import Any", id="absolute-module"), + pytest.param("from ._shapes import Shape", "from sub._shapes import Shape", id="relative-module-in-origin"), + pytest.param("from shapes import Base as Shape", "from shapes import Base as Shape", id="aliased-in-origin"), + ], + ) + def test_names_a_declaration_uses_are_imported_from_where_the_origin_gets_them( + self, + tmp_path: Path, + origin_imports: str, + expected_line: str, + ) -> None: + """Verify a name the declaration uses is imported from the module the origin imports it from, not from the origin.""" + name = expected_line.rsplit(" ", maxsplit=1)[-1] + plan = _plan( + tmp_path, + f'from typing import TypeVar\n{origin_imports}\nT = TypeVar("T", bound={name})\n', + "from sub.file_1 import T\ndef b(x: T) -> T:\n return x\n", + ("sub/file_1.py", "use.py"), + ) + + assert_that(plan.results, equal_to({"T": "fixed"})) + imports = plan.localizations[0].imports + assert_that(imports.count(expected_line), equal_to(1)) + assert_that(imports, not_(has_item(f"from sub.file_1 import {name}"))) + + @pytest.mark.parametrize( + ("origin", "importing_extra"), + [ + pytest.param( + 'from typing import TypeVar\nclass Shape:\n pass\nT = TypeVar("T", bound=Shape)\n', + "class Shape:\n pass", + id="name-means-something-else-here", + ), + pytest.param( + "from typing import TYPE_CHECKING, TypeVar\n" + "if TYPE_CHECKING:\n" + " from shapes import Shape\n" + 'T = TypeVar("T", bound="Shape")\n', + "", + id="origin-binds-it-only-for-type-checking", + ), + ], + ) + def test_does_not_localize_when_a_name_the_declaration_uses_cant_be_imported_safely( + self, tmp_path: Path, origin: str, importing_extra: str + ) -> None: + """Verify a declaration using a name that isn't available at module level as the same object stays imported.""" + plan = _plan( + tmp_path, + origin, + f"from file_1 import T\n{importing_extra}\ndef biggest(items: list[T]) -> T:\n return items[0]\n", + ) + + assert_that(plan.results, equal_to({"T": "unsafe"})) + assert_that(plan.unsafe_reasons, equal_to({"T": UnsafeReason.NAME_NOT_AT_MODULE_LEVEL})) + assert_that(plan.localizations, has_length(0)) + + @pytest.mark.parametrize( + ("origin_import", "importing_header", "expected_imports"), + [ + pytest.param("from typing import TypeVar", "", ["from typing import TypeVar"], id="missing-here"), + pytest.param("from typing import TypeVar", 'from typing import TypeVar\nU = TypeVar("U")', [], id="already-here"), + # The origin has no from-import of "TypeVar"; only safe because this file imports it. + pytest.param("from typing import *", "from typing import TypeVar", [], id="origin-wildcard-import"), + ], + ) + def test_localized_file_imports_the_constructor_exactly_once( + self, + tmp_path: Path, + origin_import: str, + importing_header: str, + expected_imports: list[str], + ) -> None: + """Verify localizing adds "from typing import TypeVar" when missing and never when the file already has it.""" + plan = _plan( + tmp_path, + f'{origin_import}\nT = TypeVar("T")\ndef a(x: T) -> T:\n return x\n', + f"{importing_header}\nfrom file_1 import T\ndef b(x: T) -> T:\n return x\n", + ) + + assert_that(plan.results, has_entry("T", "fixed")) + assert_that(plan.localizations[0].imports, equal_to(expected_imports)) + + @pytest.mark.parametrize( + ("origin_declaration", "expected_comment_lines"), + [ + pytest.param('T = TypeVar("T") # noqa: PLC0105', ['T = TypeVar("T") # noqa: PLC0105'], id="noqa"), + pytest.param( + 'T = TypeVar(\n "T",\n bound=int,\n) # noqa: PLC0105', + [") # noqa: PLC0105"], + id="multi-line-declaration", + ), + pytest.param('T = TypeVar("T"); U = 1 # unrelated', [], id="shares-line-with-another-statement"), + ], + ) + def test_localized_declaration_keeps_the_comment_trailing_it( + self, + tmp_path: Path, + origin_declaration: str, + expected_comment_lines: list[str], + ) -> None: + """Verify the comment trailing a declaration's line is copied with it, unless the line holds another statement.""" + plan = _plan( + tmp_path, + f"from typing import TypeVar\n{origin_declaration}\n", + "from file_1 import T\ndef b(x: T) -> T:\n return x\n", + ) + + assert_that(plan.results, equal_to({"T": "fixed"})) + written = "\n".join(plan.localizations[0].declarations) + assert_that([line for line in written.splitlines() if "#" in line], equal_to(expected_comment_lines)) + + @pytest.mark.parametrize( + ("layout", "importing_header", "expected_imports"), + [ + pytest.param(("file_1.py", "file_2.py"), "from .file_1 import T", ["from ._compat import TypeVar"], id="same-dir"), + pytest.param( + ("sub/file_1.py", "use.py"), + "from .sub.file_1 import T", + ["from .sub._compat import TypeVar"], + id="origin-in-subpackage", + ), + pytest.param( + ("file_1.py", "sub/use.py"), + "from ..file_1 import T", + ["from .._compat import TypeVar"], + id="importing-in-subpackage", + ), + pytest.param(("sub/file_1.py", "use.py"), "from sub.file_1 import T", ["from sub._compat import TypeVar"], id="absolute-via"), + pytest.param( + ("file_1.py", "file_2.py"), + "from ._compat import TypeVar\nfrom .file_1 import T", + [], + id="already-here", + ), + # A top-level origin's `._compat` names no package from here, so the origin itself provides TypeVar. + pytest.param(("file_1.py", "use.py"), "from file_1 import T", ["from file_1 import TypeVar"], id="not-rebasable"), + ], + ) + def test_localized_file_imports_a_relatively_imported_constructor_from_the_same_module( + self, + tmp_path: Path, + layout: tuple[str, str], + importing_header: str, + expected_imports: list[str], + ) -> None: + """Verify a constructor the origin imports relatively is imported once, from that same module as seen from here.""" + plan = _plan( + tmp_path, + 'from ._compat import TypeVar\nT = TypeVar("T")\n', + f"{importing_header}\ndef b(x: T) -> T:\n return x\n", + layout, + ) + + assert_that(plan.results, has_entry("T", "fixed")) + assert_that(plan.localizations[0].imports, equal_to(expected_imports)) + + @pytest.mark.parametrize( + "origin_imports", + [ + pytest.param( + """ + import sys + if sys.version_info >= (3, 13): + from typing import TypeVar + else: + from typing_extensions import TypeVar + """, + id="python-version", + ), + pytest.param( + """ + from typing import TYPE_CHECKING + if TYPE_CHECKING: + from typing_extensions import TypeVar + else: + from typing import TypeVar + """, + id="type-checking", + ), + ], + ) + def test_does_not_localize_when_origin_imports_constructor_conditionally( + self, + tmp_path: Path, + origin_imports: str, + ) -> None: + """Verify a TypeVar whose origin imports TypeVar inside an `if` block stays imported, marked unsafe.""" + plan = _plan( + tmp_path, + textwrap.dedent(origin_imports) + 'T = TypeVar("T", contravariant=True, default=None)\n', + """ + from typing import TypeVar + from file_1 import T, helper + def b(x: T) -> None: + helper() + """, + ) + + assert_that(plan.results, has_entry("T", "unsafe")) + assert_that(plan.unsafe_reasons, has_entry("T", UnsafeReason.ORIGIN_IMPORTS_CONSTRUCTOR_CONDITIONALLY)) + assert_that(plan.localizations, has_length(0)) + + def test_unparsable_origin_error_names_the_origin_file(self, tmp_path: Path) -> None: + """Verify a syntax error in the origin module is raised with the origin file's path, not ``.""" + with pytest.raises(SyntaxError) as excinfo: + _plan( + tmp_path, + """ + from typing import TypeVar + T = TypeVar("T") + def broken(: + """, + """ + from file_1 import T + def b(x: T) -> T: + return x + """, + ) + + assert_that(excinfo.value.filename, equal_to(str(tmp_path / "file_1.py"))) + + def test_no_typevar_import_found(self, tmp_path: Path) -> None: + """Verify nothing is planned when the importing file has no cross-file TypeVar.""" + plan = _plan( + tmp_path, + """ + def helper() -> None: + pass + """, + """ + from file_1 import helper + def b() -> None: + helper() + """, + ) + + assert_that(plan.results, equal_to({})) + assert_that(plan.localizations, has_length(0)) diff --git a/test/recipes/test_type_var_domain.py b/test/recipes/test_type_parameter_rules.py similarity index 69% rename from test/recipes/test_type_var_domain.py rename to test/recipes/test_type_parameter_rules.py index d336f5338..62cfd8f09 100644 --- a/test/recipes/test_type_var_domain.py +++ b/test/recipes/test_type_parameter_rules.py @@ -4,19 +4,26 @@ # of this repository, also available at: # https://github.com/TNO/Renaissance.Py/blob/main/LICENSE -"""Tests for type_var_domain's safety predicates and their UnsafeReason results.""" +"""Tests for type_parameter_rules's safety predicates and their UnsafeReason results.""" import re import textwrap +from collections import Counter from pathlib import Path +from typing import cast import libcst as cst import pytest from hamcrest import assert_that, equal_to, not_ -from renaissance.recipes.type_var_domain import ( +from renaissance.recipes.type_parameter_rules import ( + CONVERSION_RULES, + LOCALIZATION_RULES, + PEP_696_MINIMUM, + REMOVAL_RULES, UNSAFE_RULES, UnsafeReason, + conversion_refusal, is_safe_to_localize, is_safe_to_remove, ) @@ -139,3 +146,43 @@ def test_anchor_is_set_on_a_heading(self, reason: UnsafeReason) -> None: heading = re.compile(rf"^#+ .+ \{{ #{re.escape(anchor)} \}}$", re.MULTILINE) assert_that(heading.search(_FEATURE_DOC.read_text(encoding="utf-8")), not_(None)) + + +class TestConversionRefusal: + """See module docstring.""" + + @pytest.mark.parametrize( + ("declaration", "min_python", "expected"), + [ + pytest.param('T = TypeVar("T")', (3, 12), None, id="plain-declaration"), + pytest.param('T = TypeVar("T", default=int)', (3, 12), UnsafeReason.PEP696_VERSION_GATE, id="default-below-3.13"), + pytest.param('T = TypeVar("T", default=int)', PEP_696_MINIMUM, None, id="default-from-3.13"), + pytest.param('P = ParamSpec("P", bound=int)', (3, 12), UnsafeReason.NO_PEP695_EQUIVALENT, id="bound-on-a-param-spec"), + pytest.param( + 'T = TypeVar("T", default=int, **options)', + (3, 12), + UnsafeReason.NO_PEP695_EQUIVALENT, + id="the-first-registered-rule-wins", + ), + ], + ) + def test_returns_none_when_convertible_else_the_first_reason( + self, + declaration: str, + min_python: tuple[int, int], + expected: UnsafeReason | None, + ) -> None: + """Verify a declaration is refused only for a reason that applies to it, and the first registered reason wins.""" + statement = cast("cst.SimpleStatementLine", cst.parse_statement(declaration)) + + assert_that(conversion_refusal(cast("cst.Assign", statement.body[0]), min_python), equal_to(expected)) + + +class TestRefusalRuleRegistry: + """See module docstring.""" + + def test_no_reason_is_registered_twice(self) -> None: + """Verify each UnsafeReason is enforced by at most one rule, so a report never has two rules for one reason.""" + registered = [rule.reason for rules in (CONVERSION_RULES, REMOVAL_RULES, LOCALIZATION_RULES) for rule in rules] + + assert_that(max(Counter(registered).values()), equal_to(1)) diff --git a/test/recipes/test_type_var_check_localize.py b/test/recipes/test_type_var_check_localize.py deleted file mode 100644 index 7db16657e..000000000 --- a/test/recipes/test_type_var_check_localize.py +++ /dev/null @@ -1,549 +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 TypeVarCheck.localize_imported_typevars.""" - -import ast -import builtins -import textwrap -from pathlib import Path -from typing import TYPE_CHECKING, cast - -import pytest -from hamcrest import all_of, assert_that, contains_string, equal_to, has_entry, has_length, not_ -from pytest_mock import MockerFixture - -from renaissance.integrations.python.ast.cst_node import PythonCstNode -from renaissance.recipes.type_var_check import PEP_695_MINIMUM, TypeVarCheck -from renaissance.recipes.type_var_domain import UnsafeReason - -# TODO(FrancescoPezzella): Replace with a lazy import in Python 3.15. -# https://github.com/TNO/Renaissance.Py/issues/275 -if TYPE_CHECKING: - from hamcrest.core.matcher import Matcher - - -def _module_level_names(module: ast.Module) -> set[str]: - """Return the names bound at module level by imports, classes, functions and assignments.""" - names: set[str] = set() - for node in module.body: - if isinstance(node, ast.Import | ast.ImportFrom): - names.update((alias.asname or alias.name).split(".")[0] for alias in node.names) - elif isinstance(node, ast.ClassDef | ast.FunctionDef | ast.AsyncFunctionDef): - names.add(node.name) - elif isinstance(node, ast.Assign): - names.update(target.id for target in node.targets if isinstance(target, ast.Name)) - return names - - -def _names_used_by(call: ast.Call) -> set[str]: - """Return the non-builtin names a TypeVar call's arguments use, including inside string forward references.""" - names: set[str] = set() - for argument in [*call.args[1:], *(keyword.value for keyword in call.keywords)]: - expression = argument - if isinstance(argument, ast.Constant) and isinstance(argument.value, str): - expression = ast.parse(argument.value, mode="eval").body - names.update(node.id for node in ast.walk(expression) if isinstance(node, ast.Name)) - return names - set(dir(builtins)) - - -class TestTypeVarCheckLocalize: - """See module docstring.""" - - def _create_cross_file( - self, - mocker: MockerFixture, - tmp_path: Path, - origin_text: str, - importing_text: str, - layout: tuple[str, str] = ("file_1.py", "file_2.py"), - ) -> TypeVarCheck: - """Write origin_text to the origin file and return an in-memory TypeVarCheck on the importing file holding importing_text. - - layout gives the (origin, importing) file paths, relative to tmp_path, which is the project root. - """ - origin_rel_path, importing_rel_path = layout - origin_file = tmp_path / origin_rel_path - origin_file.parent.mkdir(parents=True, exist_ok=True) - origin_file.write_text(textwrap.dedent(origin_text)) - - importing_code = textwrap.dedent(importing_text) - importing_file = str(tmp_path / importing_rel_path) - mocker.patch( - "renaissance.integrations.python.ast.factory.PythonFactory.create", - return_value=PythonCstNode.load_from_text(importing_code, importing_file), - ) - subject = TypeVarCheck(importing_file) - subject.in_memory = True - subject.min_python = PEP_695_MINIMUM - subject.project_root = tmp_path - return subject - - @pytest.mark.parametrize( - "origin_extra", - [ - pytest.param("def a(x: T) -> T:\n return x", id="plain-origin"), - pytest.param('__all__ = ["T"]', id="origin-exports-it"), - pytest.param("class Box(Generic[T]):\n pass", id="origin-class-is-generic-over-it"), - ], - ) - def test_localizes_typevar_whatever_its_origin_does_with_it(self, mocker: MockerFixture, tmp_path: Path, origin_extra: str) -> None: - """Verify an imported TypeVar is localized, also when its origin exports it or has a class generic over it.""" - origin = f"from typing import Generic, TypeVar\nT = TypeVar('T')\n{origin_extra}\n" - subject = self._create_cross_file( - mocker, - tmp_path, - origin, - """ - from file_1 import T - def b(x: T) -> T: - return x - """, - ) - result = subject.localize_imported_typevars() - - assert_that(result, equal_to({"T": "fixed"})) - output = subject.apply_to_string() - assert_that(output, contains_string("T = TypeVar('T')")) - assert_that(output, not_(contains_string("from file_1 import T"))) - - @pytest.mark.parametrize( - "declaration", - [ - pytest.param('T = TypeVar("T", bound=Shape)', id="invariant-bound"), - pytest.param('T = TypeVar("T", int, Shape)', id="constraints"), - pytest.param('T = TypeVar("T", bound=Shape, covariant=True)', id="covariant"), - pytest.param('T = TypeVar("T", bound=Shape, contravariant=True)', id="contravariant"), - pytest.param('T = TypeVar("T", bound="Shape")', id="string-bound"), - ], - ) - def test_localized_declaration_keeps_its_meaning(self, mocker: MockerFixture, tmp_path: Path, declaration: str) -> None: - """Verify the local copy has the origin's exact arguments and every name they use is defined in the importing file.""" - origin = f"from typing import TypeVar\nclass Shape:\n pass\n{declaration}\n" - subject = self._create_cross_file( - mocker, - tmp_path, - origin, - """ - from file_1 import T - def biggest(items: list[T]) -> T: - return items[0] - """, - ) - result = subject.localize_imported_typevars() - - assert_that(result, equal_to({"T": "fixed"})) - module = ast.parse(subject.apply_to_string()) - local_call = next(node.value for node in module.body if isinstance(node, ast.Assign)) - origin_call = cast("ast.Assign", ast.parse(origin).body[-1]).value - assert_that(ast.dump(local_call), equal_to(ast.dump(origin_call))) - assert_that(_names_used_by(cast("ast.Call", local_call)) - _module_level_names(module), equal_to(set())) - - @pytest.mark.parametrize( - ("origin", "importing_extra"), - [ - pytest.param( - 'from typing import TypeVar\nclass Shape:\n pass\nT = TypeVar("T", bound=Shape)\n', - "class Shape:\n pass", - id="name-means-something-else-here", - ), - pytest.param( - "from typing import TYPE_CHECKING, TypeVar\n" - "if TYPE_CHECKING:\n" - " from shapes import Shape\n" - 'T = TypeVar("T", bound="Shape")\n', - "", - id="origin-binds-it-only-for-type-checking", - ), - ], - ) - def test_does_not_localize_when_a_name_the_declaration_uses_cant_be_imported_safely( - self, mocker: MockerFixture, tmp_path: Path, origin: str, importing_extra: str - ) -> None: - """Verify a declaration using a name that can't be imported from the origin as the same object stays imported.""" - subject = self._create_cross_file( - mocker, - tmp_path, - origin, - f"from file_1 import T\n{importing_extra}\ndef biggest(items: list[T]) -> T:\n return items[0]\n", - ) - result = subject.localize_imported_typevars() - - assert_that(result, equal_to({"T": "unsafe"})) - assert_that(subject.cross_file_unsafe_reasons, equal_to({"T": UnsafeReason.DECLARATION_NAME_UNAVAILABLE})) - assert_that(subject.apply_to_string(), contains_string("from file_1 import T")) - - @pytest.mark.parametrize( - ("dunder_all", "imported_elsewhere"), - [ - pytest.param('__all__ = ["T"]', frozenset[str](), id="re-exported-via-dunder-all"), - pytest.param("", frozenset({"T"}), id="imported-from-here-elsewhere"), - ], - ) - def test_localizes_typevar_this_file_re_exports( - self, - mocker: MockerFixture, - tmp_path: Path, - dunder_all: str, - imported_elsewhere: frozenset[str], - ) -> None: - """Verify an imported TypeVar is localized even when this file passes it on to others.""" - subject = self._create_cross_file( - mocker, - tmp_path, - """ - from typing import TypeVar - T = TypeVar("T") - """, - f""" - from file_1 import T - {dunder_all} - def b(x: T) -> T: - return x - """, - ) - subject.project_wide_imported_names = imported_elsewhere - - result = subject.localize_imported_typevars() - - assert_that(result, equal_to({"T": "fixed"})) - output = subject.apply_to_string() - assert_that(output, contains_string('T = TypeVar("T")')) - assert_that(output, not_(contains_string("from file_1 import T"))) - - @pytest.mark.parametrize( - ("layout", "importing_import", "expected_line"), - [ - pytest.param(("file_1.py", "file_2.py"), "from file_1 import T, helper", "from file_1 import helper", id="absolute"), - pytest.param(("file_1.py", "file_2.py"), "from .file_1 import T, helper", "from .file_1 import helper", id="relative"), - pytest.param( - ("file_1.py", "sub/use.py"), - "from ..file_1 import T, helper", - "from ..file_1 import helper", - id="relative-parent", - ), - pytest.param(("__init__.py", "file_2.py"), "from . import T, helper", "from . import helper", id="bare-package"), - pytest.param( - ("file_1.py", "file_2.py"), - "from file_1 import T, helper as h", - "from file_1 import helper as h", - id="alias-kept", - ), - ], - ) - def test_keeps_other_names_when_localizing_one_of_several_imports( - self, - mocker: MockerFixture, - tmp_path: Path, - layout: tuple[str, str], - importing_import: str, - expected_line: str, - ) -> None: - """Verify localizing one imported name keeps the other names imported from the same module, alias included.""" - subject = self._create_cross_file( - mocker, - tmp_path, - 'from typing import TypeVar\nT = TypeVar("T")\ndef helper() -> None:\n pass\n', - f"{importing_import}\ndef b(x: T) -> T:\n return x\n", - layout, - ) - result = subject.localize_imported_typevars() - - assert_that(result, has_entry("T", "fixed")) - output = subject.apply_to_string() - assert_that(output.splitlines().count(expected_line), equal_to(1)) - assert_that(output, contains_string('T = TypeVar("T")')) - - @pytest.mark.parametrize( - ("importing_import", "comments"), - [ - pytest.param("from file_1 import T, helper # noqa: F401", ["# noqa: F401"], id="trailing-comment"), - pytest.param( - "from file_1 import ( # header\n T, # the typevar\n helper, # the helper\n)", - ["# header", "# the helper"], - id="multi-line", - ), - ], - ) - def test_narrowing_an_import_keeps_its_comments( - self, - mocker: MockerFixture, - tmp_path: Path, - importing_import: str, - comments: list[str], - ) -> None: - """Verify each comment of a narrowed import is kept once, and never on the localized declaration's line.""" - subject = self._create_cross_file( - mocker, - tmp_path, - 'from typing import TypeVar\nT = TypeVar("T")\ndef helper() -> None:\n pass\n', - f"{importing_import}\ndef b(x: T) -> T:\n helper()\n return x\n", - ) - result = subject.localize_imported_typevars() - - assert_that(result, has_entry("T", "fixed")) - lines = subject.apply_to_string().splitlines() - for comment in comments: - holders = [line for line in lines if comment in line] - assert_that(holders, has_length(1), comment) - assert_that(holders[0], not_(contains_string("TypeVar(")), comment) - - @pytest.mark.parametrize( - ("origin_extra", "importing_import", "expected_lines"), - [ - pytest.param( - 'T = TypeVar("T")\nU = TypeVar("U")', - "from file_1 import T, U", - ["from typing import TypeVar", 'T = TypeVar("T")', 'U = TypeVar("U")'], - id="two-names", - ), - pytest.param( - 'T = TypeVar("T")\nU = TypeVar("U")\ndef helper() -> None:\n pass', - "from file_1 import T, helper, U", - ["from file_1 import helper", "from typing import TypeVar", 'T = TypeVar("T")', 'U = TypeVar("U")'], - id="keeps-other-name", - ), - pytest.param( - 'class Shape:\n pass\nclass Circle(Shape):\n pass\nT = TypeVar("T", bound=Shape)\nU = TypeVar("U", bound=Circle)', - "from file_1 import T, U", - [ - "from file_1 import Circle, Shape", - "from typing import TypeVar", - 'T = TypeVar("T", bound=Shape)', - 'U = TypeVar("U", bound=Circle)', - ], - id="bounds-share-one-import", - ), - ], - ) - def test_localizes_two_names_from_one_import_statement( - self, - mocker: MockerFixture, - tmp_path: Path, - origin_extra: str, - importing_import: str, - expected_lines: list[str], - ) -> None: - """Verify one import statement bringing in two localizable names localizes both, adding each needed import once.""" - subject = self._create_cross_file( - mocker, - tmp_path, - f"from typing import TypeVar\n{origin_extra}\n", - f"{importing_import}\ndef b(x: T, y: U) -> T:\n return x\n", - ) - result = subject.localize_imported_typevars() - - assert_that(result, equal_to({"T": "fixed", "U": "fixed"})) - output = subject.apply_to_string() - for line in expected_lines: - assert_that(output.splitlines().count(line), equal_to(1), line) - still_imported = { - alias.name - for node in ast.parse(output).body - if isinstance(node, ast.ImportFrom) and node.module == "file_1" - for alias in node.names - } - assert_that(still_imported & {"T", "U"}, equal_to(set())) - - @pytest.mark.parametrize( - ("origin_import", "importing_header"), - [ - pytest.param("from typing import TypeVar", "", id="missing-here"), - pytest.param("from typing import TypeVar", 'from typing import TypeVar\nU = TypeVar("U")', id="already-here"), - # The origin has no from-import of "TypeVar"; only safe because this file imports it. - pytest.param("from typing import *", "from typing import TypeVar", id="origin-wildcard-import"), - ], - ) - def test_localized_file_imports_the_constructor_exactly_once( - self, - mocker: MockerFixture, - tmp_path: Path, - origin_import: str, - importing_header: str, - ) -> None: - """Verify localizing adds "from typing import TypeVar" when missing and never duplicates it.""" - subject = self._create_cross_file( - mocker, - tmp_path, - f'{origin_import}\nT = TypeVar("T")\ndef a(x: T) -> T:\n return x\n', - f"{importing_header}\nfrom file_1 import T\ndef b(x: T) -> T:\n return x\n", - ) - result = subject.localize_imported_typevars() - - assert_that(result, has_entry("T", "fixed")) - assert_that(subject.apply_to_string().count("from typing import TypeVar"), equal_to(1)) - - @pytest.mark.parametrize( - ("layout", "importing_header", "expected_line"), - [ - pytest.param(("file_1.py", "file_2.py"), "from .file_1 import T", "from ._compat import TypeVar", id="same-dir"), - pytest.param( - ("sub/file_1.py", "use.py"), - "from .sub.file_1 import T", - "from .sub._compat import TypeVar", - id="origin-in-subpackage", - ), - pytest.param( - ("file_1.py", "sub/use.py"), - "from ..file_1 import T", - "from .._compat import TypeVar", - id="importing-in-subpackage", - ), - pytest.param(("sub/file_1.py", "use.py"), "from sub.file_1 import T", "from sub._compat import TypeVar", id="absolute-via"), - pytest.param( - ("file_1.py", "file_2.py"), - "from ._compat import TypeVar\nfrom .file_1 import T", - "from ._compat import TypeVar", - id="already-here", - ), - # A top-level origin's `._compat` names no package from here, so the origin itself provides TypeVar. - pytest.param(("file_1.py", "use.py"), "from file_1 import T", "from file_1 import TypeVar", id="not-rebasable"), - ], - ) - def test_localized_file_imports_a_relatively_imported_constructor_from_the_same_module( - self, - mocker: MockerFixture, - tmp_path: Path, - layout: tuple[str, str], - importing_header: str, - expected_line: str, - ) -> None: - """Verify a constructor the origin imports relatively is imported once, from that same module as seen from here.""" - subject = self._create_cross_file( - mocker, - tmp_path, - 'from ._compat import TypeVar\nT = TypeVar("T")\n', - f"{importing_header}\ndef b(x: T) -> T:\n return x\n", - layout, - ) - result = subject.localize_imported_typevars() - - assert_that(result, has_entry("T", "fixed")) - assert_that(subject.apply_to_string().splitlines().count(expected_line), equal_to(1)) - - def test_does_not_localize_when_origin_imports_constructor_conditionally( - self, - mocker: MockerFixture, - tmp_path: Path, - ) -> None: - """A TypeVar whose origin picks TypeVar per Python version stays imported, marked unsafe.""" - subject = self._create_cross_file( - mocker, - tmp_path, - """ - import sys - if sys.version_info >= (3, 13): - from typing import TypeVar - else: - from typing_extensions import TypeVar - T = TypeVar("T", contravariant=True, default=None) - """, - """ - from typing import TypeVar - from file_1 import T, helper - def b(x: T) -> None: - helper() - """, - ) - result = subject.localize_imported_typevars() - - assert_that(result, has_entry("T", "unsafe")) - assert_that( - subject.cross_file_unsafe_reasons, - has_entry("T", UnsafeReason.ORIGIN_IMPORTS_CONSTRUCTOR_CONDITIONALLY), - ) - output = subject.apply_to_string() - assert_that(output, contains_string("from file_1 import T, helper")) - assert_that(output, not_(contains_string("T = TypeVar"))) - - def test_unparsable_origin_error_names_the_origin_file(self, mocker: MockerFixture, tmp_path: Path) -> None: - """Verify a syntax error in the origin module is raised with the origin file's path, not ``.""" - subject = self._create_cross_file( - mocker, - tmp_path, - """ - from typing import TypeVar - T = TypeVar("T") - def broken(: - """, - """ - from file_1 import T - def b(x: T) -> T: - return x - """, - ) - - with pytest.raises(SyntaxError) as excinfo: - subject.localize_imported_typevars() - - assert_that(excinfo.value.filename, equal_to(str(tmp_path / "file_1.py"))) - - def test_no_typevar_import_found(self, mocker: MockerFixture, tmp_path: Path) -> None: - """AI: Verify localize_imported_typevars reports nothing when the importing file has no cross-file TypeVar.""" - subject = self._create_cross_file( - mocker, - tmp_path, - """ - def helper() -> None: - pass - """, - """ - from file_1 import helper - def b() -> None: - helper() - """, - ) - result = subject.localize_imported_typevars() - - assert_that(result, equal_to({})) - - @pytest.mark.parametrize( - ("min_python", "expected_converted", "output_matcher"), - [ - pytest.param( - PEP_695_MINIMUM, - "fixed", - all_of(contains_string("def b[T](x: T) -> T:"), not_(contains_string("T = TypeVar"))), - id="converts-on-3.12", - ), - pytest.param( - (3, 10), - "unsafe", - all_of(contains_string("def b(x: T) -> T:"), contains_string('T = TypeVar("T")')), - id="only-localizes-below-3.12", - ), - ], - ) - def test_check_localizes_then_converts_when_the_target_allows( - self, - mocker: MockerFixture, - tmp_path: Path, - min_python: tuple[int, int], - expected_converted: str, - output_matcher: Matcher[str], - ) -> None: - """Verify check() localizes a cross-file TypeVar, then converts it only if the target supports PEP 695.""" - subject = self._create_cross_file( - mocker, - tmp_path, - """ - from typing import TypeVar - T = TypeVar("T") - def a(x: T) -> T: - return x - """, - """ - from file_1 import T - def b(x: T) -> T: - return x - """, - ) - subject.min_python = min_python - subject.run() - - assert_that(subject.result["cross_file"], has_entry("T", "fixed")) - assert_that(subject.result["converted"], has_entry("T", expected_converted)) - assert_that(subject.apply_to_string(), output_matcher) diff --git a/test/recipes/test_type_var_check_properties.py b/test/recipes/test_type_var_check_properties.py deleted file mode 100644 index 123a51ff2..000000000 --- a/test/recipes/test_type_var_check_properties.py +++ /dev/null @@ -1,37 +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 - -"""Property-based tests for TypeVarCheck.check.""" - -import ast -from unittest.mock import patch - -import hypothesmith -from hypothesis import given, reject, settings - -from renaissance.integrations.python.ast.cst_node import PythonCstNode -from renaissance.recipes.type_var_check import TypeVarCheck - - -class TestTypeVarCheckProperties: - """See module docstring.""" - - @given(source=hypothesmith.from_grammar()) - @settings(max_examples=50, deadline=None) - def test_check_never_crashes(self, source: str) -> None: - """AI: Verify check() never raises on arbitrary hypothesmith-generated valid Python source.""" - try: - ast.parse(source) - except SyntaxError: - reject() - - with patch( - "renaissance.integrations.python.ast.factory.PythonFactory.create", - return_value=PythonCstNode.load_from_text(source), - ): - subject = TypeVarCheck("x.py") - subject.in_memory = True - subject.check() diff --git a/test/rejuvenation/test_migration_type_recipes.py b/test/rejuvenation/test_migration_type_recipes.py index bb61884c5..7172bad7f 100644 --- a/test/rejuvenation/test_migration_type_recipes.py +++ b/test/rejuvenation/test_migration_type_recipes.py @@ -10,6 +10,8 @@ importlib.util.spec_from_file_location instead of a normal import - see _load_script(). """ +import ast +import codecs import importlib.util import textwrap from pathlib import Path @@ -19,7 +21,7 @@ from hamcrest import assert_that, contains_string, equal_to, not_ from hamcrest.core.matcher import Matcher -from renaissance.recipes.type_var_domain import UnsafeReason, doc_link +from renaissance.recipes.type_parameter_rules import DOCS_BASE_URL, UNSAFE_RULES, UnsafeReason _SCRIPT_PATH = Path(__file__).resolve().parents[2] / "src" / "rejuvenation" / "migration-type-recipes.py" @@ -64,25 +66,16 @@ def identity(x: T) -> T: class TestProcessFile: """process_file: writes changes for real, and isolates per-file errors.""" - @pytest.mark.parametrize( - ("source", "imported_elsewhere", "expected_reason"), - [ - pytest.param(UNSAFE_TYPEVAR_SOURCE, frozenset[str](), UnsafeReason.DECLARED_TYPEVAR_EXPORTED, id="exported-via-dunder-all"), - pytest.param(LEGACY_TYPEVAR_SOURCE, frozenset({"T"}), UnsafeReason.IMPORTED_ELSEWHERE_IN_PROJECT, id="imported-elsewhere"), - ], - ) - def test_converts_function_but_keeps_a_declaration_others_rely_on( - self, tmp_path: Path, source: str, imported_elsewhere: frozenset[str], expected_reason: UnsafeReason - ) -> None: + def test_converts_function_but_keeps_a_declaration_imported_elsewhere(self, tmp_path: Path) -> None: """The function is converted on disk, the declaration stays, and the report says why it stayed.""" target = tmp_path / "mod.py" - target.write_text(source, encoding="utf-8") + target.write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") - report = migration.process_file(target, min_python=(3, 12), project_root=tmp_path, project_wide_imported_names=imported_elsewhere) + report = migration.process_file(target, min_python=(3, 12), project_root=tmp_path, project_wide_imported_names=frozenset({"T"})) assert_that(migration.has_fixed(report), equal_to(obj=True)) assert_that(report.reasons, not_(None)) - assert_that((report.reasons or {})["orphaned"], equal_to({"T": expected_reason})) + assert_that((report.reasons or {})["orphaned"], equal_to({"T": UnsafeReason.IMPORTED_ELSEWHERE_IN_PROJECT})) written = target.read_text(encoding="utf-8") assert_that(written, contains_string("def identity[T](x: T) -> T:")) assert_that(written, contains_string('T = TypeVar("T")')) @@ -102,31 +95,19 @@ class TestRuffImportCleanup: """main(): the ruff F401 batch step actually drops now-unused imports end to end.""" @pytest.mark.parametrize( - ("extra_args", "import_matcher", "report_matcher"), + ("extra_args", "import_matcher"), [ - pytest.param( - [], - not_(contains_string("TypeVar")), - contains_string("cleaned up via `ruff"), - id="default-drops-import", - ), - pytest.param( - ["--no-ruff"], - contains_string("from typing import TypeVar"), - not_(contains_string("cleaned up via `ruff")), - id="no-ruff-keeps-import", - ), + pytest.param([], not_(contains_string("TypeVar")), id="default-drops-import"), + pytest.param(["--no-ruff"], contains_string("from typing import TypeVar"), id="no-ruff-keeps-import"), ], ) def test_unused_typevar_import_cleanup( self, tmp_path: Path, - capsys: pytest.CaptureFixture[str], extra_args: list[str], import_matcher: Matcher[str], - report_matcher: Matcher[str], ) -> None: - """The redundant TypeVar import is dropped, and the report says so, unless --no-ruff; conversion runs either way.""" + """The redundant TypeVar import is dropped unless --no-ruff; conversion runs either way.""" target = tmp_path / "mod.py" target.write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") @@ -136,7 +117,38 @@ def test_unused_typevar_import_cleanup( written = target.read_text(encoding="utf-8") assert_that(written, contains_string("def identity[T]")) assert_that(written, import_matcher) - assert_that(capsys.readouterr().out, report_matcher) + + def test_localized_imports_are_merged_with_existing_ones(self, tmp_path: Path) -> None: + """The localized TypeVar's import joins the file's own `typing` import.""" + (tmp_path / "xarray_types.py").write_text( + 'from typing import TypeVar\n\nJoinOptions = str\nAlignable = int\nT = TypeVar("T")\n', + encoding="utf-8", + ) + target = tmp_path / "use.py" + target.write_text( + textwrap.dedent("""\ + from typing import Any + from xarray_types import JoinOptions + from xarray_types import T + from xarray_types import Alignable + + + def pick(x: T, how: JoinOptions, other: Alignable) -> Any: + return x + + + class Items(list[T]): + pass + """), + encoding="utf-8", + ) + + exit_code = migration.main([str(target), "--py", "3.12"]) + + assert_that(exit_code, equal_to(0)) + lines = target.read_text(encoding="utf-8").splitlines() + for import_line in ["from typing import Any, TypeVar", "from xarray_types import Alignable, JoinOptions"]: + assert_that(lines.count(import_line), equal_to(1), import_line) def test_unmodified_sibling_file_is_left_untouched(self, tmp_path: Path) -> None: """A sibling file with no TypeVar usage - and its own genuinely-unused import - survives main() byte-for-byte.""" @@ -157,29 +169,109 @@ def greet() -> str: assert_that(sibling.read_text(encoding="utf-8"), equal_to(sibling_source)) -class TestConsoleReportDocLinks: - """main(): each unsafe name printed under NEEDS MANUAL REVIEW links to its documented rule.""" +class TestConsoleReportNeedsReviewSection: + """main(): what the NEEDS MANUAL REVIEW section lists per reason.""" - def test_needs_manual_review_includes_doc_link_for_the_specific_reason( + def test_names_are_listed_per_file_under_their_reason_and_the_documentation_is_linked_once( self, tmp_path: Path, capsys: pytest.CaptureFixture[str], ) -> None: - """The report links a __all__-exported TypeVar to the DECLARED_TYPEVAR_EXPORTED rule.""" + """Each reason is a heading with its files and names below it; the documentation is linked once, without per-name anchors.""" target = tmp_path / "mod.py" target.write_text(UNSAFE_TYPEVAR_SOURCE, encoding="utf-8") migration.main([str(target), "--py", "3.12"]) output = capsys.readouterr().out - assert_that(output, contains_string(doc_link(UnsafeReason.DECLARED_TYPEVAR_EXPORTED))) + assert_that(output.count(DOCS_BASE_URL), equal_to(1)) + assert_that(output, not_(contains_string("#feature-"))) + assert_that(output, contains_string("**Exported via __all__**\n `mod.py`: `T`\n")) + assert_that(output, not_(contains_string(" orphaned:"))) + + def test_files_and_names_sharing_a_reason_are_listed_under_one_heading( + self, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], + ) -> None: + """A reason is printed once, with every file it applies to, and a file lists all its names on one line.""" + (tmp_path / "one.py").write_text(UNSAFE_TYPEVAR_SOURCE, encoding="utf-8") + (tmp_path / "two.py").write_text( + textwrap.dedent("""\ + from typing import TypeVar + + U = TypeVar("U") + + __all__ = ["U"] + + + def identity(x: U) -> U: + return x + """), + encoding="utf-8", + ) + (tmp_path / "three.py").write_text( + textwrap.dedent("""\ + from typing import TypeVar + + A = TypeVar("A") + B = TypeVar("B") + + __all__ = ["A", "B"] + + + def pair(a: A, b: B) -> tuple[A, B]: + return a, b + """), + encoding="utf-8", + ) + + migration.main([str(tmp_path), "--py", "3.12", "--no-ruff"]) + + output = capsys.readouterr().out + assert_that(output.count("**Exported via __all__**"), equal_to(1)) + for line in [" `one.py`: `T`\n", " `three.py`: `A`, `B`\n", " `two.py`: `U`\n"]: + assert_that(output, contains_string(line)) + + def test_name_converted_in_functions_but_still_used_by_a_generic_class_is_in_both_sections( + self, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], + ) -> None: + """A name converted in some functions that a generic class still uses is listed as modified and as needing review.""" + target = tmp_path / "mod.py" + target.write_text( + textwrap.dedent("""\ + from typing import Generic, TypeVar + + T = TypeVar("T") + + + def first(items: list[T]) -> T: + return items[0] + + + class Box(Generic[T]): + def get(self, x: T) -> T: + return x + """), + encoding="utf-8", + ) + + migration.main([str(target), "--py", "3.12", "--no-ruff"]) + + output = capsys.readouterr().out + modified, needs_review = output.split("**NEEDS MANUAL REVIEW (") + assert_that(output, contains_string("1 modified, 1 need manual review")) + assert_that(modified, contains_string(" `T`: converted to PEP 695 in functions\n")) + assert_that(needs_review, contains_string("**Only used inside a class that is generic over it**\n `mod.py`: `T`\n")) class TestConsoleReportModifiedSection: """main(): what the MODIFIED section lists per file.""" - def test_converted_name_is_not_listed_again_as_orphaned(self, tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None: - """A converted name is listed once; only declarations removed without a conversion appear under orphaned.""" + def test_converted_name_is_not_listed_again_as_removed(self, tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None: + """A converted name is listed once; only declarations removed without a conversion are listed as removed.""" target = tmp_path / "mod.py" target.write_text( textwrap.dedent("""\ @@ -202,8 +294,205 @@ def g[U](y: U) -> U: migration.main([str(target), "--py", "3.12", "--no-ruff"]) output = capsys.readouterr().out - assert_that(output, contains_string(" converted: T\n")) - assert_that(output, contains_string(" orphaned: U\n")) + assert_that(output, contains_string(" `T`: converted to PEP 695 in functions\n")) + assert_that(output, not_(contains_string(" `T`: unused declaration removed"))) + assert_that(output, contains_string(" `U`: unused declaration removed\n")) + + +def _table_rows(output: str, first_header: str) -> list[list[str]]: + """Return the data rows, as cells without padding or backticks, of the printed Markdown table whose header starts with first_header.""" + rows = [[cell.strip().strip("`") for cell in line.strip("|").split("|")] for line in output.splitlines() if line.startswith("|")] + start = next(index for index, row in enumerate(rows) if row[0] == first_header) + table: list[list[str]] = [] + for row in rows[start + 2 :]: + if row[0] in {"Kind", "Reason"}: + break + table.append(row) + return table + + +class TestSummaryTables: + """main(): the summary tables at the top of the printed report.""" + + def test_counts_per_kind_and_reasons_by_frequency(self, tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None: + """A name counts once per file, needs review if it has a reason, errored files are skipped, reasons are ranked.""" + (tmp_path / "fixed.py").write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") + (tmp_path / "exported_1.py").write_text(UNSAFE_TYPEVAR_SOURCE, encoding="utf-8") + (tmp_path / "exported_2.py").write_text(UNSAFE_TYPEVAR_SOURCE, encoding="utf-8") + (tmp_path / "bound_paramspec.py").write_text( + textwrap.dedent("""\ + from collections.abc import Callable + from typing import ParamSpec + + P = ParamSpec("P", bound=int) + + + def wrap(f: Callable[P, int]) -> Callable[P, int]: + return f + """), + encoding="utf-8", + ) + (tmp_path / "broken.py").write_text("def broken(:\n", encoding="utf-8") + + migration.main([str(tmp_path), "--py", "3.12", "--no-ruff"]) + + output = capsys.readouterr().out + assert_that( + _table_rows(output, "Kind"), + equal_to([["TypeVar", "3", "1", "2"], ["ParamSpec", "1", "0", "1"], ["TypeVarTuple", "0", "0", "0"]]), + ) + assert_that( + _table_rows(output, "Reason"), + equal_to( + [ + [UNSAFE_RULES[UnsafeReason.DECLARED_TYPEVAR_EXPORTED].message, "2"], + [UNSAFE_RULES[UnsafeReason.NO_PEP695_EQUIVALENT].message, "1"], + ], + ), + ) + + +_COMMIT_SUBJECT = "Modernize TypeVar/ParamSpec/TypeVarTuple usage to PEP 695 syntax" +_DEAD_TYPEVAR_SOURCE = 'from typing import TypeVar\n\nT_FreqStr = TypeVar("T_FreqStr")\n' +_TWELVE_TYPEVARS_SOURCE = "from typing import TypeVar\n\n" + "".join( + f'T{n:02} = TypeVar("T{n:02}")\n\n\ndef f{n:02}(x: T{n:02}) -> T{n:02}:\n return x\n\n\n' for n in range(1, 13) +) + + +class TestCommitSummary: + """main(): the commit-style message printed after the report.""" + + @pytest.mark.parametrize( + ("files", "expected"), + [ + pytest.param( + {"a.py": LEGACY_TYPEVAR_SOURCE}, + "Convert type parameters to PEP 695 syntax in 1 file (Python 3.12+).\n\n- converted to PEP 695 in functions: T", + id="converted-only", + ), + pytest.param( + {"a.py": LEGACY_TYPEVAR_SOURCE, "b.py": LEGACY_TYPEVAR_SOURCE}, + "Convert type parameters to PEP 695 syntax in 2 files (Python 3.12+).\n\n- converted to PEP 695 in functions: T (2 files)", + id="name-counted-per-file", + ), + pytest.param( + {"a.py": UNSAFE_TYPEVAR_SOURCE, "b.py": _DEAD_TYPEVAR_SOURCE}, + "Convert type parameters to PEP 695 syntax in 2 files (Python 3.12+).\n\n" + "- converted to PEP 695 in functions: T\n" + "- unused declaration removed: T_FreqStr\n\n" + f"1 file left for manual review: {UNSAFE_RULES[UnsafeReason.DECLARED_TYPEVAR_EXPORTED].message} (1).", + id="removed-and-left-for-review", + ), + pytest.param( + {"a.py": _TWELVE_TYPEVARS_SOURCE}, + "Convert type parameters to PEP 695 syntax in 1 file (Python 3.12+).\n\n" + "- converted to PEP 695 in functions: T01, T02, T03, T04, T05, T06, T07, T08, T09, T10 and 2 more", + id="long-list-is-capped", + ), + ], + ) + def test_lists_what_changed_per_action_and_what_is_left_for_review( + self, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], + files: dict[str, str], + expected: str, + ) -> None: + """Names are grouped by what was done to them, with the files they were fixed in; leftover review is summarised by reason.""" + for name, source in files.items(): + (tmp_path / name).write_text(source, encoding="utf-8") + + migration.main([str(tmp_path), "--py", "3.12", "--no-ruff"]) + + summary = capsys.readouterr().out.split(_COMMIT_SUBJECT, 1)[1].strip() + assert_that(summary, equal_to(expected)) + + def test_no_message_when_no_file_was_modified(self, tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None: + """A run that changes nothing prints no commit message.""" + (tmp_path / "a.py").write_text("x = 1\n", encoding="utf-8") + + migration.main([str(tmp_path), "--py", "3.12", "--no-ruff"]) + + assert_that(capsys.readouterr().out, not_(contains_string(_COMMIT_SUBJECT))) + + +class TestIgnore: + """main(): --ignore leaves a shared declaration file and the imports of its names untouched, and reports them.""" + + def test_ignored_file_and_its_importer_are_left_untouched_and_the_report_lists_it( + self, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], + ) -> None: + """Neither file changes, the ignored one is listed but not checked, and its name counts as reported and ignored.""" + shared_source = 'from typing import TypeVar\n\nT = TypeVar("T")\n\n\ndef first(items: list[T]) -> T:\n return items[0]\n' + user_source = "from shared import T\n\n\ndef identity(x: T) -> T:\n return x\n" + (tmp_path / "shared.py").write_text(shared_source, encoding="utf-8") + (tmp_path / "user.py").write_text(user_source, encoding="utf-8") + + migration.main([str(tmp_path), "--py", "3.12", "--no-ruff", "--ignore", "shared.py"]) + + output = capsys.readouterr().out + assert_that((tmp_path / "shared.py").read_text(encoding="utf-8"), equal_to(shared_source)) + assert_that((tmp_path / "user.py").read_text(encoding="utf-8"), equal_to(user_source)) + assert_that(output, contains_string("**IGNORED FILES (1)**\n `shared.py`\n")) + assert_that(output, not_(contains_string("shared.py checked"))) + assert_that( + _table_rows(output, "Kind"), + equal_to([["TypeVar", "1", "0", "0", "1"], ["ParamSpec", "0", "0", "0", "0"], ["TypeVarTuple", "0", "0", "0", "0"]]), + ) + + +class TestSecondPass: + """main(): --second-pass removes a declaration whose importers were all localized, and reports it in the same report.""" + + @pytest.mark.parametrize( + ("extra_args", "declaration_kept", "expected_review"), + [ + pytest.param([], True, "NEEDS MANUAL REVIEW (1)", id="single-pass"), + pytest.param(["--second-pass"], False, "NEEDS MANUAL REVIEW (0)", id="second-pass"), + ], + ) + def test_declaration_left_behind_by_localizing_is_removed_in_the_same_run( + self, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], + extra_args: list[str], + *, + declaration_kept: bool, + expected_review: str, + ) -> None: + """Only the second pass removes the origin's declaration, and the one report counts both files once.""" + (tmp_path / "origin.py").write_text('from typing import TypeVar\n\nT = TypeVar("T")\n', encoding="utf-8") + (tmp_path / "user.py").write_text("from origin import T\n\n\ndef identity(x: T) -> T:\n return x\n", encoding="utf-8") + + migration.main([str(tmp_path), "--py", "3.12", "--no-ruff", *extra_args]) + + output = capsys.readouterr().out + assert_that('T = TypeVar("T")' in (tmp_path / "origin.py").read_text(encoding="utf-8"), equal_to(declaration_kept)) + assert_that(output, contains_string(expected_review)) + assert_that(output, contains_string("Processed 2 files")) + + +class TestNoLocalization: + """main(): --no-localization keeps every project import of a type parameter and reports the names left.""" + + def test_importer_is_left_untouched_and_the_report_counts_the_name_as_ignored( + self, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], + ) -> None: + """The importer is unchanged, no file is listed as ignored, and the imported name counts as reported and ignored.""" + (tmp_path / "shared.py").write_text('from typing import TypeVar\n\nT = TypeVar("T")\n', encoding="utf-8") + user_source = "from shared import T\n\n\ndef identity(x: T) -> T:\n return x\n" + (tmp_path / "user.py").write_text(user_source, encoding="utf-8") + + migration.main([str(tmp_path), "--py", "3.12", "--no-ruff", "--no-localization"]) + + output = capsys.readouterr().out + assert_that((tmp_path / "user.py").read_text(encoding="utf-8"), equal_to(user_source)) + assert_that(output, not_(contains_string("IGNORED FILES"))) + assert_that(_table_rows(output, "Kind")[0], equal_to(["TypeVar", "2", "0", "1", "1"])) class TestPerFileProgressFeedback: @@ -245,31 +534,79 @@ def test_one_bad_file_does_not_abort_the_batch( assert_that(good.read_text(encoding="utf-8"), contains_string("def identity[T]")) -class TestMainProjectWideImportSafety: - """main(): a TypeVar imported by another file in the batch is localized there and kept at its origin.""" +class TestMainFileEncoding: + """main(): a file's encoding is never changed, and a file that is not UTF-8 is reported instead of aborting the run.""" + + @pytest.mark.xfail( + strict=True, + reason="A UTF-8 byte order mark corrupts the rewritten file: https://github.com/TNO/Renaissance.Py/issues/153", + ) + @pytest.mark.parametrize( + "source", + [ + pytest.param(LEGACY_TYPEVAR_SOURCE, id="lf"), + pytest.param(LEGACY_TYPEVAR_SOURCE.replace("\n", "\r\n"), id="crlf"), + ], + ) + def test_modified_file_keeps_its_utf8_bom_and_still_parses(self, tmp_path: Path, source: str) -> None: + """A file that starts with a UTF-8 byte order mark is converted, keeps the mark, and is valid Python afterwards.""" + target = tmp_path / "mod.py" + target.write_bytes(codecs.BOM_UTF8 + source.encode("utf-8")) + + exit_code = migration.main([str(tmp_path), "--py", "3.12", "--no-ruff"]) + assert_that(exit_code, equal_to(0)) + written = target.read_bytes() + assert_that(written.startswith(codecs.BOM_UTF8), equal_to(obj=True)) + text = written.decode("utf-8-sig") + ast.parse(text) + assert_that(text, contains_string("def identity[T](x: T) -> T:")) + + @pytest.mark.xfail( + strict=True, + raises=UnicodeDecodeError, + reason="A file that is not UTF-8 aborts the whole run: https://github.com/TNO/Renaissance.Py/issues/188", + ) @pytest.mark.parametrize( - ("consumer_rel", "import_line"), + "content", [ - pytest.param("pkg/client.py", "from pkg.typing_mod import T", id="absolute-dotted"), - pytest.param("pkg/client.py", "from .typing_mod import T", id="relative"), - pytest.param("other/client.py", "from pkg.typing_mod import T", id="absolute-other-directory"), + pytest.param("# café\nx = 1\n".encode("latin-1"), id="latin-1"), + pytest.param("# café\nx = 1\n".encode("utf-16"), id="utf-16"), ], ) - def test_consumer_is_localized_and_origin_declaration_survives( + def test_file_that_is_not_utf8_is_reported_and_the_rest_of_the_batch_is_converted( self, tmp_path: Path, - consumer_rel: str, - import_line: str, + capsys: pytest.CaptureFixture[str], + content: bytes, ) -> None: + """The undecodable file is listed under ERRORS and left byte-identical, and the other file is converted.""" + good = tmp_path / "good.py" + good.write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") + undecodable = tmp_path / "undecodable.py" + undecodable.write_bytes(content) + + exit_code = migration.main([str(tmp_path), "--py", "3.12", "--no-ruff"]) + + assert_that(exit_code, equal_to(3)) + assert_that(capsys.readouterr().out, contains_string("**ERRORS (1)**\n `undecodable.py`")) + assert_that(undecodable.read_bytes(), equal_to(content)) + assert_that(good.read_text(encoding="utf-8"), contains_string("def identity[T]")) + + +class TestMainProjectWideImportSafety: + """main(): a TypeVar imported by another file in the batch is localized there and kept at its origin.""" + + def test_consumer_is_localized_and_origin_declaration_survives(self, tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None: """The importing file gets a PEP 695 local TypeVar, and the origin declaration is not removed.""" + import_line = "from pkg.typing_mod import T" pkg = tmp_path / "pkg" pkg.mkdir() (pkg / "__init__.py").write_text("", encoding="utf-8") (pkg / "typing_mod.py").write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") - consumer = tmp_path / consumer_rel - consumer.parent.mkdir(exist_ok=True) + consumer = pkg / "client.py" consumer.write_text(f"{import_line}\n\ndef use(x: T) -> T:\n return x\n", encoding="utf-8") + (pkg / "unused.py").write_text(f"{import_line}\n", encoding="utf-8") exit_code = migration.main([str(tmp_path), "--py", "3.12"]) @@ -277,8 +614,39 @@ def test_consumer_is_localized_and_origin_declaration_survives( consumer_text = consumer.read_text(encoding="utf-8") assert_that(consumer_text, contains_string("def use[T](x: T) -> T:")) assert_that(consumer_text, not_(contains_string(import_line))) + assert_that(capsys.readouterr().out, contains_string(" `T`: unused declaration removed\n")) assert_that((pkg / "typing_mod.py").read_text(encoding="utf-8"), contains_string('T = TypeVar("T")')) + @pytest.mark.xfail( + strict=True, + reason="`import *` is not checked when deciding whether a declaration is imported elsewhere in the project: " + "see 'imported by another project file' in docs/user/features/type-parameter-modernization.md", + ) + @pytest.mark.parametrize( + ("package", "import_line"), + [ + pytest.param("", "from origin import *", id="absolute"), + pytest.param("pkg", "from .origin import *", id="relative"), + ], + ) + def test_origin_declaration_survives_a_star_import_of_its_module(self, tmp_path: Path, package: str, import_line: str) -> None: + """A declaration that another file reaches through `import *` is kept, and that file is left untouched.""" + directory = tmp_path / package + directory.mkdir(exist_ok=True) + if package: + (directory / "__init__.py").write_text("", encoding="utf-8") + origin = directory / "origin.py" + origin.write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") + consumer = directory / "consumer.py" + consumer_source = f"{import_line}\n\n\ndef use(x: T) -> T:\n return x\n" + consumer.write_text(consumer_source, encoding="utf-8") + + exit_code = migration.main([str(tmp_path), "--py", "3.12", "--no-ruff"]) + + assert_that(exit_code, equal_to(0)) + assert_that(origin.read_text(encoding="utf-8"), contains_string('T = TypeVar("T")')) + assert_that(consumer.read_text(encoding="utf-8"), equal_to(consumer_source)) + @pytest.mark.parametrize("target_arg", [pytest.param(".", id="dot"), pytest.param("pkg", id="subdirectory")]) def test_origin_declaration_survives_with_relative_target( self, @@ -299,19 +667,6 @@ def test_origin_declaration_survives_with_relative_target( assert_that((pkg / "typing_mod.py").read_text(encoding="utf-8"), contains_string('T = TypeVar("T")')) assert_that((pkg / "client.py").read_text(encoding="utf-8"), contains_string("def use[T](x: T) -> T:")) - def test_origin_declaration_survives_module_attribute_access(self, tmp_path: Path) -> None: - """A TypeVar accessed as a module attribute by another file is kept at its origin.""" - pkg = tmp_path / "pkg" - pkg.mkdir() - (pkg / "__init__.py").write_text("", encoding="utf-8") - (pkg / "typing_mod.py").write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") - (pkg / "client.py").write_text("import pkg.typing_mod\n\ndef use(x: pkg.typing_mod.T) -> None: ...\n", encoding="utf-8") - - exit_code = migration.main([str(tmp_path), "--py", "3.12"]) - - assert_that(exit_code, equal_to(0)) - assert_that((pkg / "typing_mod.py").read_text(encoding="utf-8"), contains_string('T = TypeVar("T")')) - class TestPyVersionFlag: """main(): the required --py flag alone sets the target's minimum Python version.""" @@ -319,14 +674,14 @@ class TestPyVersionFlag: @pytest.mark.parametrize( ("py_version", "expected", "unexpected"), [ - pytest.param("3.11", 'T = TypeVar("T")', "def identity[", id="3.11-gated"), - pytest.param("3.12", "def identity[T]", 'T = TypeVar("T")', id="3.12-converted"), + pytest.param("3.12", 'T = TypeVar("T", default=int)', "def identity[", id="3.12-default-kept"), + pytest.param("3.13", "def identity[T = int]", 'T = TypeVar("T", default=int)', id="3.13-default-converted"), ], ) def test_py_flag_gates_rewrites(self, tmp_path: Path, py_version: str, expected: str, unexpected: str) -> None: - """--py decides whether the PEP 695 rewrite runs.""" + """--py decides whether a type parameter default (PEP 696) is converted.""" target = tmp_path / "mod.py" - target.write_text(LEGACY_TYPEVAR_SOURCE, encoding="utf-8") + target.write_text(LEGACY_TYPEVAR_SOURCE.replace('TypeVar("T")', 'TypeVar("T", default=int)'), encoding="utf-8") exit_code = migration.main([str(target), "--py", py_version]) @@ -339,7 +694,6 @@ def test_py_flag_gates_rewrites(self, tmp_path: Path, py_version: str, expected: ("bad_args", "expected_error"), [ pytest.param([], "the following arguments are required: --py", id="flag-missing"), - pytest.param(["--py", "3"], "expected MAJOR.MINOR (e.g. 3.12), got '3'", id="missing-minor"), pytest.param(["--py", "3.x"], "expected MAJOR.MINOR (e.g. 3.12), got '3.x'", id="non-numeric"), ], ) diff --git a/test/utils/test_cst_utils.py b/test/utils/test_cst_utils.py new file mode 100644 index 000000000..e07cb9cef --- /dev/null +++ b/test/utils/test_cst_utils.py @@ -0,0 +1,71 @@ +# Copyright (C) 2024-2026 TNO +# +# Licensed under the terms specified in the LICENSE file in the root directory +# of this repository, also available at: +# https://github.com/TNO/Renaissance.Py/blob/main/LICENSE + +"""Tests for renaissance.utils.cst_utils.""" + +from typing import cast + +import libcst as cst +import pytest +from hamcrest import assert_that, equal_to + +from renaissance.utils.cst_utils import assigned_names, module_statements, names_read_by + + +class TestNamesReadBy: + """See module docstring.""" + + @pytest.mark.parametrize( + ("expression", "expected"), + [ + pytest.param("x", {"x"}, id="name"), + pytest.param("f(a, key=b)", {"f", "a", "b"}, id="keyword-argument-name-is-not-a-read"), + pytest.param("np.integer", {"np"}, id="attribute-name-is-not-a-read"), + pytest.param("a.b.c(d)", {"a", "d"}, id="attribute-chain-reads-only-its-root"), + pytest.param("int | str", {"int", "str"}, id="built-ins-are-included"), + ], + ) + def test_returns_the_names_an_expression_reads(self, expression: str, expected: set[str]) -> None: + """Verify every name read is returned, but neither an attribute's own name nor a keyword argument's name.""" + assert_that(names_read_by(cst.parse_expression(expression)), equal_to(expected)) + + +class TestAssignedNames: + """See module docstring.""" + + @pytest.mark.parametrize( + ("statement", "expected"), + [ + pytest.param("x = 1", {"x"}, id="plain"), + pytest.param("x = y = 1", {"x", "y"}, id="chained"), + pytest.param("x: int = 1", {"x"}, id="annotated-leaves-the-annotation-out"), + pytest.param("a, b = 1, 2", {"a", "b"}, id="tuple-target"), + pytest.param("[a, (b, c)] = value", {"a", "b", "c"}, id="nested-targets"), + ], + ) + def test_returns_the_names_the_targets_bind(self, statement: str, expected: set[str]) -> None: + """Verify every name in the targets is returned, including inside tuple or list targets.""" + line = cast("cst.SimpleStatementLine", cst.parse_statement(statement)) + + assert_that(assigned_names(cast("cst.Assign | cst.AnnAssign", line.body[0])), equal_to(expected)) + + +class TestModuleStatements: + """See module docstring.""" + + @pytest.mark.parametrize( + ("source", "expected"), + [ + pytest.param("a = 1\nb = 2\n", ["Assign", "Assign"], id="one-per-line"), + pytest.param("a = 1; b = 2\n", ["Assign", "Assign"], id="one-per-small-statement"), + pytest.param("a = 1\nif a:\n b = 2\n", ["Assign", "If"], id="compound-statement-is-one-item"), + ], + ) + def test_yields_the_top_level_statements_in_order(self, source: str, expected: list[str]) -> None: + """Verify a line of several small statements gives one item each and a compound statement gives one in all.""" + statements = [type(statement).__name__ for statement in module_statements(cst.parse_module(source))] + + assert_that(statements, equal_to(expected)) diff --git a/test/utils/test_import_resolution.py b/test/utils/test_import_resolution.py index d264d851f..1e3e68496 100644 --- a/test/utils/test_import_resolution.py +++ b/test/utils/test_import_resolution.py @@ -10,10 +10,19 @@ from pathlib import Path from typing import cast +import libcst as cst import pytest -from hamcrest import assert_that, equal_to, has_item, has_key, not_ +from hamcrest import assert_that, equal_to, has_item, has_key, is_, none, not_, not_none -from renaissance.utils.import_resolution import collect_project_imported_names, rebase_relative_module, resolve_project_module +from renaissance.utils.import_resolution import ( + collect_project_imported_names, + from_import_binding, + from_import_sources, + import_from_module, + imports_name_conditionally, + rebase_relative_module, + resolve_project_module, +) @pytest.fixture @@ -187,3 +196,68 @@ def test_does_not_record_unrelated_attribute_access(self, module_tree: Path, con result = collect_project_imported_names([consumer], module_tree) assert_that(result, not_(has_key(module_tree / "pkg" / "mod.py"))) + + +class TestFromImportSources: + """See module docstring.""" + + @pytest.mark.parametrize( + ("source", "expected"), + [ + pytest.param("from a.b import X\n", {"X": ("a.b", 0)}, id="absolute"), + pytest.param("from ..pkg import X as Y\n", {"Y": ("pkg", 2)}, id="relative-alias-keys-the-bound-name"), + pytest.param("from . import x\n", {"x": ("", 1)}, id="bare-relative"), + pytest.param("import os.path\n", {"os": None}, id="plain-import-binds-the-top-package"), + pytest.param("class C: ...\n", {"C": None}, id="class"), + pytest.param("from a import X\nX = 1\n", {"X": None}, id="later-assignment-rebinds"), + ], + ) + def test_maps_each_bound_name_to_its_from_import(self, source: str, expected: dict[str, tuple[str, int] | None]) -> None: + """Verify a name maps to its `from` import's (module, level), or to None when anything else binds it last.""" + assert_that(from_import_sources(cst.parse_module(source)), equal_to(expected)) + + +class TestFromImportBinding: + """See module docstring.""" + + def test_returns_the_import_and_alias_binding_the_name(self) -> None: + """Verify the alias that binds an aliased name is returned with its import statement.""" + binding = from_import_binding(cst.parse_module("from a import X as Y\n"), "Y") + + assert_that(binding, not_none()) + statement, alias = cast("tuple[cst.ImportFrom, cst.ImportAlias]", binding) + assert_that(alias.evaluated_name, equal_to("X")) + assert_that(import_from_module(statement), equal_to("a")) + + @pytest.mark.parametrize( + "source", + [ + pytest.param("from a import X\nX = 1\n", id="rebound-by-assignment"), + pytest.param("import X\n", id="plain-import"), + pytest.param("A = 1\n", id="not-imported"), + ], + ) + def test_is_none_when_no_from_import_binds_the_name_last(self, source: str) -> None: + """Verify None is returned when another statement binds the name, or nothing does.""" + assert_that(from_import_binding(cst.parse_module(source), "X"), none()) + + +class TestImportsNameConditionally: + """See module docstring.""" + + @pytest.mark.parametrize( + ("source", "expected"), + [ + pytest.param("if TYPE_CHECKING:\n from typing import TypeVar\n", True, id="under-if"), + pytest.param( + "try:\n from typing import TypeVar\nexcept ImportError:\n from typing_extensions import TypeVar\n", + True, + id="under-try", + ), + pytest.param("from typing import TypeVar\n", False, id="unconditional"), + pytest.param("if TYPE_CHECKING:\n from typing import ParamSpec\n", False, id="other-name"), + ], + ) + def test_detects_an_import_of_the_name_inside_a_module_level_if_or_try(self, source: str, *, expected: bool) -> None: + """Verify only an import of the name inside a module-level `if` or `try` counts.""" + assert_that(imports_name_conditionally(cst.parse_module(source), "TypeVar"), is_(expected))