Use ast.unparse for generating patches with lib_updater.py - #6142
Conversation
Sorry, something went wrong.
WalkthroughRewrites patch generation in scripts/lib_updater.py from string-based formatting to AST-driven decorator construction, adds UtMethod.has_args, refactors reason extraction logic to AST/surrounding-source paths, introduces new constants, updates JSON load/show handling, and standardizes indentation and unittest aliasing. Changes
Sequence Diagram(s)sequenceDiagram
actor Dev as Developer/CLI
participant LU as LibUpdater
participant AST as Python AST
participant SF as Source File
participant JSON as Patches JSON
rect rgb(235, 245, 255)
note right of LU: Phase 1/2 Patch Emission (AST-driven)
Dev->>LU: run update
LU->>AST: build decorator nodes (PatchSpec.as_ast_node)
AST-->>LU: ast.Attribute / ast.Call
LU->>LU: render as_decorator() and indent with DEFAULT_INDENT
LU->>SF: write decorators and wrappers
end
rect rgb(245, 235, 255)
note right of LU: Patch Extraction (reason/cond)
LU->>AST: parse source for UT methods
alt UT method has_args()
LU->>AST: walk dec_node for COMMENT string and optional cond
AST-->>LU: reason, cond
else No args
LU->>SF: scan surrounding lines for COMMENT
SF-->>LU: reason line
end
LU->>LU: normalize reason via removeprefix(COMMENT)
end
rect rgb(235, 255, 240)
note right of LU: JSON I/O
LU->>JSON: load patches
JSON-->>LU: dicts
LU->>LU: convert ut_method -> UtMethod via _replace
LU-->>Dev: show-patches via spec._asdict()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (1 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
|
|
||
| # TODO: RUSTPYTHON; called `Result::unwrap()` on an `Err` value: StackUnderflow | ||
| ''' | ||
| @unittest.expectedFailure # TODO: RUSTPYTHON; TypeError: class pattern defines no positional sub-patterns (__match_args__ missing) |
There was a problem hiding this comment.
Test now no longer breaks file
Sorry, something went wrong.
|
|
||
| # TODO: RUSTPYTHON | ||
| @unittest.skip(reason="RUSTPYTHON hangs") | ||
| @unittest.skip("TODO: RUSTPYTHON; hangs") |
There was a problem hiding this comment.
Was added manually
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)scripts/lib_updater.py (2)📜 Review details149-149: Broaden detection of unittest alias (optional).
Matching only attr_node.value.id == "unittest" misses import unittest as ut. Consider collecting aliases from imports and/or allow a small alias set.
Example minimal tweak (constant-only):
- or getattr(attr_node.value, "id", None) != UT + or getattr(attr_node.value, "id", None) not in {UT, "ut", "UT"}Or, for correctness, scan imports to discover the actual alias and check against that alias. I can draft that if you want.
328-331: Slightly simplify JSON→PatchSpec load path (optional).
Avoid constructing then _replace; also tolerate missing keys.
Apply this diff:
- test_name: [ - PatchSpec(**spec)._replace(ut_method=UtMethod(spec["ut_method"])) - for spec in specs - ] + test_name: [ + PatchSpec( + ut_method=UtMethod(spec["ut_method"]), + cond=spec.get("cond"), + reason=spec.get("reason", ""), + ) + for spec in specs + ]
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 7044d43 and 1966c3e.
⛔ Files ignored due to path filters (6)- Lib/test/test_ast/test_ast.py is excluded by !Lib/**
- Lib/test/test_csv.py is excluded by !Lib/**
- Lib/test/test_logging.py is excluded by !Lib/**
- Lib/test/test_os.py is excluded by !Lib/**
- Lib/test/test_str.py is excluded by !Lib/**
- Lib/test/test_xml_etree.py is excluded by !Lib/**
- scripts/lib_updater.py (8 hunks)
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Follow PEP 8 style for custom Python code
Use ruff for linting Python code
Files:
- scripts/lib_updater.py
Learnt from: ShaharNaveh PR: RustPython/RustPython#6089 File: scripts/lib_updater.py:198-202 Timestamp: 2025-09-07T05:38:31.644Z Learning: In scripts/lib_updater.py, the iter_patches function intentionally does not handle SyntaxError from ast.parse(contents). The author confirmed this behavior is fine and intended - the tool should fail fast on unparseable files rather than silently skip processing.
[error] 41-41: SyntaxError: invalid syntax
(E999)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run tests under miri
- GitHub Check: Check Rust code with rustfmt and clippy
scripts/lib_updater.py (2)243-245: Decorator emission and phase-2 wrapper formatting look good.
Switch to textwrap.indent and centralized DEFAULT_INDENT improves consistency and readability.
Also applies to: 253-260
35-35: Nice cleanup.
Importing textwrap, introducing DEFAULT_INDENT, centralizing UT, and adding _reason/has_args/as_decorator streamline formatting and reduce stringly-typed code.
Also applies to: 43-46, 57-59, 87-90, 106-113
Sorry, something went wrong.
| if typing.TYPE_CHECKING: | ||
| from collections.abc import Iterator | ||
|
|
||
| type Patches = dict[str, dict[str, list["PatchSpec"]]] |
There was a problem hiding this comment.
⚠️ Potential issue
Fix: PEP 695 type alias breaks under Python ≤3.11 (E999).
Flake8 E999 points here. Replace the type statement with a TypeAlias so the script runs on 3.11 (CI baseline).
Apply this diff:
-type Patches = dict[str, dict[str, list["PatchSpec"]]]
+Patches: typing.TypeAlias = dict[str, dict[str, list["PatchSpec"]]]‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type Patches = dict[str, dict[str, list["PatchSpec"]]] | |
| Patches: typing.TypeAlias = dict[str, dict[str, list["PatchSpec"]]] |
[error] 41-41: SyntaxError: invalid syntax
(E999)
🤖 Prompt for AI AgentsIn scripts/lib_updater.py around line 41, the use of the PEP 695 "type" alias causes Flake8 E999 on Python ≤3.11; replace it with typing.TypeAlias: add "from typing import TypeAlias" to the imports and change the declaration to use the TypeAlias annotation, e.g. "Patches: TypeAlias = dict[str, dict[str, list['PatchSpec']]]", preserving the inner forward reference string for PatchSpec.
Sorry, something went wrong.
| @property | ||
| def _attr_node(self) -> ast.Attribute: | ||
| return ast.Attribute(value=ast.Name(id=UT), attr=self.ut_method) | ||
|
|
||
| def as_ast_node(self) -> ast.Attribute | ast.Call: | ||
| if not self.ut_method.has_args(): | ||
| return self._attr_node | ||
|
|
||
| args = [] | ||
| if self.cond: | ||
| args.append(ast.parse(self.cond).body[0].value) | ||
| args.append(ast.Constant(value=self._reason)) | ||
|
|
||
| return ast.Call(func=self._attr_node, args=args, keywords=[]) | ||
|
|
There was a problem hiding this comment.
⚠️ Potential issue
Required: add ctx=ast.Load() to constructed AST nodes to make ast.unparse reliable.
ast.Attribute and ast.Name require ctx; missing ctx can cause unparse failures or incorrect trees.
Apply this diff:
- def _attr_node(self) -> ast.Attribute:
- return ast.Attribute(value=ast.Name(id=UT), attr=self.ut_method)
+ def _attr_node(self) -> ast.Attribute:
+ return ast.Attribute(
+ value=ast.Name(id=UT, ctx=ast.Load()),
+ attr=str(self.ut_method),
+ ctx=ast.Load(),
+ )
- def as_ast_node(self) -> ast.Attribute | ast.Call:
+ def as_ast_node(self) -> ast.Attribute | ast.Call:
if not self.ut_method.has_args():
return self._attr_node
@@
- return ast.Call(func=self._attr_node, args=args, keywords=[])
+ return ast.Call(func=self._attr_node, args=args, keywords=[])‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @property | |
| def _attr_node(self) -> ast.Attribute: | |
| return ast.Attribute(value=ast.Name(id=UT), attr=self.ut_method) | |
| def as_ast_node(self) -> ast.Attribute | ast.Call: | |
| if not self.ut_method.has_args(): | |
| return self._attr_node | |
| args = [] | |
| if self.cond: | |
| args.append(ast.parse(self.cond).body[0].value) | |
| args.append(ast.Constant(value=self._reason)) | |
| return ast.Call(func=self._attr_node, args=args, keywords=[]) | |
| @property | |
| def _attr_node(self) -> ast.Attribute: | |
| return ast.Attribute( | |
| value=ast.Name(id=UT, ctx=ast.Load()), | |
| attr=str(self.ut_method), | |
| ctx=ast.Load(), | |
| ) | |
| def as_ast_node(self) -> ast.Attribute | ast.Call: | |
| if not self.ut_method.has_args(): | |
| return self._attr_node | |
| args = [] | |
| if self.cond: | |
| args.append(ast.parse(self.cond).body[0].value) | |
| args.append(ast.Constant(value=self._reason)) | |
| return ast.Call(func=self._attr_node, args=args, keywords=[]) |
In scripts/lib_updater.py around lines 91 to 105, the constructed AST nodes lack ctx which breaks ast.unparse; update the node constructors to include ctx=ast.Load() — set ast.Name(..., ctx=ast.Load()) and ast.Attribute(..., ctx=ast.Load()) where created (and ensure any other manually-constructed AST Name/Attribute in this function uses ctx=ast.Load()); leave ast.Call/ast.Constant as-is and keep existing conditional arg parsing.
Sorry, something went wrong.
| # If our ut_method has args then, | ||
| # we need to search for a constant that contains our `COMMENT`. | ||
| # Otherwise we need to search it in the raw source code :/ | ||
| if ut_method.has_args(): | ||
| reason = next( | ||
| ( | ||
| node.value | ||
| for node in ast.walk(dec_node) | ||
| if isinstance(node, ast.Constant) | ||
| and isinstance(node.value, str) | ||
| and COMMENT in node.value | ||
| ), | ||
| None, | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden reason/cond extraction against edge cases.
- Guard against missing args when has_cond() is true to avoid IndexError.
- Make the regex more robust and anchored to end-of-line to avoid over-capturing.
- Optionally accept f-strings (ast.JoinedStr) as reasons.
Apply this diff:
- if ut_method.has_args():
+ if ut_method.has_args():
reason = next(
(
- node.value
- for node in ast.walk(dec_node)
- if isinstance(node, ast.Constant)
- and isinstance(node.value, str)
- and COMMENT in node.value
+ node.value
+ for node in ast.walk(dec_node)
+ if isinstance(node, ast.Constant)
+ and isinstance(node.value, str)
+ and COMMENT in node.value
),
None,
)
@@
- if ut_method.has_cond():
- cond = ast.unparse(dec_node.args[0])
+ if ut_method.has_cond() and getattr(dec_node, "args", []):
+ cond = ast.unparse(dec_node.args[0])
else:
# Search first on decorator line, then in the line before
for line in lines[dec_node.lineno - 1 : dec_node.lineno - 3 : -1]:
- if found := re.search(rf"{COMMENT}.?(.*)", line):
- reason = found.group()
+ if found := re.search(rf"{re.escape(COMMENT)}\s*[:;,-]?\s*(.*)$", line):
+ reason = found.group()
break
else:
# Didn't find our `COMMENT` :)
continue
- reason = reason.removeprefix(COMMENT).strip(";:, ")
+ reason = reason.removeprefix(COMMENT).strip(";:, ")If you want f-strings support, we can extend the walker to accept ast.JoinedStr and convert via ast.unparse(node).
Also applies to: 174-181, 182-191
🤖 Prompt for AI AgentsIn scripts/lib_updater.py around lines 159-173 (and similarly 174-181, 182-191), the extraction of reason/cond is brittle: it can IndexError when args are missing, the regex may over-capture, and f-strings (ast.JoinedStr) are not handled. Fix by first checking ut_method.has_args() and also guarding ut_method.args list length before indexing; when has_cond() is true verify args exist before accessing and return None if missing; tighten the regex to anchor the match to end-of-line (e.g., use a pattern that requires COMMENT followed by optional whitespace and end-of-line) to avoid over-capturing; and extend the AST walker to accept ast.JoinedStr nodes (convert them via ast.unparse(node)) in addition to ast.Constant strings so f-strings are supported; apply the same guards and parsing changes to the other two blocks at lines 174-181 and 182-191.
Sorry, something went wrong.
Summary by CodeRabbit