Update to ruff 0.13.1 #6177
Update to ruff 0.13.1 #6177
Conversation
WalkthroughUpdates dependencies and migrates string interpolation from FString to InterpolatedString types, introduces TString handling, and propagates a new node_index across AST structures. Adopts UTF-8 position encoding and renames SourceLocation fields to line/character_offset. Adjusts linetable, bytecode formatting, error mapping, and multiple VM/stdlib paths to the new location and interpolation APIs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User Code
participant P as Parser
participant AST as AST (ruff)
participant ST as SymbolTable
participant CG as Codegen
participant VM as VM
U->>P: Source text
P->>AST: Build nodes (InterpolatedString / TString)
AST->>ST: Scope analysis (UTF-8 locations)
ST-->>AST: Symbols, errors (line/character_offset)
AST->>CG: Emit IR/bytecode (iterate InterpolatedStringElements)
CG-->>VM: CodeObject + linetable (UTF-8 indexed)
Note over CG,VM: Locations use line + character_offset
sequenceDiagram
autonumber
participant Err as Error (Parse/Codegen)
participant SF as SourceFile
participant M as Mappers
participant Out as Consumers (VM/WASM/Stdlib)
Err->>SF: to_source_code()
SF->>M: source_location(range, PositionEncoding::Utf8)
M-->>Out: SourceLocation{ line, character_offset }
Out->>Out: Display/traceback/JS mapping
Note over M,Out: row/column replaced with line/character_offset
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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.
Please see the documentation for more information. 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. Comment |
|
I'm thinking about possibly seeing if it'd make sense to publish a |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
vm/src/stdlib/ast.rs (1)
188-205: Fix UTF-8 column offset conversion
LineIndex::offsetinterpretscharacter_offsetaccording to the suppliedPositionEncoding. Now that we passPositionEncoding::Utf8, it expects the offset to be counted in UTF‑8 code points. However,PySourceLocation::to_source_locationstill feeds it the result ofColumn::get_one_indexed(), andColumnis populated from raw byte deltas (TextSize) in bothtext_range_to_source_rangeandrange_from_object. As soon as a line contains multi-byte characters, we end up handingoffset()a value that is too large, so the computedTextRangeno longer matches the actual span (try a line with"é"to reproduce). We need to translate between byte offsets and UTF‑8 character offsets before callingoffset(), and likewise convert incoming character offsets back to byte positions when building aTextRange. Please adjust the conversions accordingly so the byte/character accounting stays consistent.vm/src/stdlib/ast/string.rs (1)
28-31: Compile error:fstring_elementis moved when computing length.Calling
fstring_element.into_iter().len()consumesfstring_element, then it’s used again. Compute length on&mutto avoid the move.- (0..fstring_element.into_iter().len()).map(move |i| { - let fstring_element = &mut fstring_element; - let tmp = fstring_element.into_iter().nth(i).unwrap(); + let len = (&mut fstring_element).into_iter().len(); + (0..len).map(move |i| { + let tmp = (&mut fstring_element) + .into_iter() + .nth(i) + .expect("element exists"); std::mem::replace(tmp, default.clone()) })vm/src/stdlib/ast/pattern.rs (1)
307-334: Prevent panic on class-patterns by implementing or guarding split/merge functionsBoth
split_pattern_match_class(vm/src/stdlib/ast/pattern.rs:534–542) andmerge_pattern_match_class(vm/src/stdlib/ast/pattern.rs:545–551) currently calltodo!(), leading to a runtime panic for any class-pattern. Implement these methods before merging or wrap this code path in an explicit error with a clear message—and file an issue to track the missing functionality.
🧹 Nitpick comments (12)
Cargo.toml (1)
161-164: Ruff crates aligned to 0.13.1 — good. Consider pinning to a commit for reproducibility.Keeping all ruff crates on the same tag is correct for API coherence. For hermetic builds, consider adding
rev = "<commit-sha>"alongsidetag(or instead oftag). Optional, but helps avoid tag retargeting surprises.compiler/codegen/src/lib.rs (1)
58-60: HandledExpr::TString— nice coverage update.This keeps error messages consistent with new AST variants introduced by Ruff 0.13.x.
If feasible, replace the
IpyEscapeCommand(_) => todo!()arm nearby with a stable name to avoid panics in diagnostics.compiler/codegen/src/compile.rs (6)
239-256: Utf8 location printing looks correct; consider a small helper to avoid drift.You call source_location(..., PositionEncoding::Utf8) here and in a few other places. A tiny helper (e.g., to_utf8_loc(offset)) would keep encoding consistent across all call sites.
530-540: Error locations switched to Utf8 — good. Centralize encoding selection.Same note as above: a single helper on Compiler for building SourceLocation would prevent future mismatches.
5550-5571: Recursive await detection inside format specs — solid approach.The recursion over nested format_spec elements mirrors CPython behavior; early-returns should keep it cheap.
If this gets hot, consider short-circuiting on first true at the outer iterator, but current code already does .any() which is fine.
5337-5345: Instruction location emission uses Utf8 — unify with other call sites.Same helper recommendation to keep encoding usage consistent in _emit/eprint/error paths.
5869-5987: Nice tests for await-in-fstrings; add one for debug text and one for nested debug+spec.Consider adding:
- f"{x=}" (no await)
- f"{x={y=}}" (nested debug text in spec)
1-1: Run rustfmt and clippy.Given the scope of refactors, please run cargo fmt and cargo clippy and fix any new lints before merge.
vm/src/stdlib/ast/string.rs (1)
13-16: Avoid O(n^2) traversal by repeatednth(i)on fresh iterators.Both iterators rebuild from the start per element. Consider draining in one pass to avoid quadratic behavior.
Example (no extra allocations on elements, collects output once):
let mut out = Vec::with_capacity(fstring_value.as_slice().len()); for slot in fstring_value.iter_mut() { out.push(std::mem::replace(slot, default.clone())); } out.into_iter()Apply similarly to
ruff_fstring_element_into_iterusing(&mut fstring_element).into_iter(). As per coding guidelines, ensure clippy is clean.Also applies to: 28-31
vm/src/stdlib/ast/argument.rs (1)
160-166: Preserve range when merging class-def args.
range: Default::default()loses the span. Consider covering the spans of provided positional/keyword arguments, mirroringmerge_function_call_arguments.- Some(Box::new(ruff::Arguments { - node_index: Default::default(), - range: Default::default(), // TODO + // Compute a best-effort range from provided parts + let range = match (positional_arguments.as_ref(), keyword_arguments.as_ref()) { + (Some(p), Some(k)) => p.range.cover(k.range), + (Some(p), None) => p.range, + (None, Some(k)) => k.range, + (None, None) => TextRange::default(), + }; + Some(Box::new(ruff::Arguments { + node_index: Default::default(), + range, args, keywords, }))vm/src/stdlib/ast/statement.rs (1)
963-969: Avoid redundant class lookup and shadowing in Try::ast_from_object.You fetch
_object.class()twice and shadow_cls. Fold into one lookup and reuse in the debug assertion.- let _cls = _object.class(); - let is_star = _cls.is(pyast::NodeStmtTryStar::static_type()); - let _cls = _object.class(); - debug_assert!( + let _cls = _object.class(); + let is_star = _cls.is(pyast::NodeStmtTryStar::static_type()); + debug_assert!( _cls.is(pyast::NodeStmtTry::static_type()) || _cls.is(pyast::NodeStmtTryStar::static_type()) );vm/src/stdlib/ast/pattern.rs (1)
358-371: PatternArguments constructed with defaulted range.If range info matters downstream, consider threading a meaningful range into
PatternArguments; otherwise this is fine as a stopgap.Ensure consumers of
PatternArgumentsdon’t rely on a non-default range.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
Cargo.toml(1 hunks)compiler/codegen/src/compile.rs(15 hunks)compiler/codegen/src/ir.rs(2 hunks)compiler/codegen/src/lib.rs(1 hunks)compiler/codegen/src/symboltable.rs(16 hunks)compiler/codegen/src/unparse.rs(24 hunks)compiler/core/src/bytecode.rs(1 hunks)compiler/core/src/lib.rs(1 hunks)compiler/core/src/marshal.rs(2 hunks)compiler/src/lib.rs(3 hunks)src/shell.rs(2 hunks)stdlib/src/faulthandler.rs(1 hunks)vm/src/builtins/frame.rs(1 hunks)vm/src/frame.rs(2 hunks)vm/src/stdlib/ast.rs(4 hunks)vm/src/stdlib/ast/argument.rs(4 hunks)vm/src/stdlib/ast/constant.rs(10 hunks)vm/src/stdlib/ast/elif_else_clause.rs(4 hunks)vm/src/stdlib/ast/exception.rs(2 hunks)vm/src/stdlib/ast/expression.rs(51 hunks)vm/src/stdlib/ast/module.rs(4 hunks)vm/src/stdlib/ast/other.rs(4 hunks)vm/src/stdlib/ast/parameter.rs(8 hunks)vm/src/stdlib/ast/pattern.rs(18 hunks)vm/src/stdlib/ast/statement.rs(46 hunks)vm/src/stdlib/ast/string.rs(10 hunks)vm/src/stdlib/ast/type_parameters.rs(7 hunks)vm/src/vm/vm_new.rs(2 hunks)wasm/lib/src/convert.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Format Rust code with the default rustfmt style (runcargo fmt)
Run clippy and fix any warnings or lints introduced by your changes
Follow Rust best practices for error handling and memory management
Files:
compiler/core/src/bytecode.rscompiler/codegen/src/ir.rscompiler/core/src/marshal.rscompiler/core/src/lib.rsvm/src/stdlib/ast/argument.rscompiler/codegen/src/symboltable.rsvm/src/stdlib/ast/parameter.rsvm/src/stdlib/ast/module.rscompiler/codegen/src/unparse.rsvm/src/stdlib/ast/pattern.rswasm/lib/src/convert.rsvm/src/stdlib/ast/elif_else_clause.rsvm/src/stdlib/ast/other.rssrc/shell.rsvm/src/stdlib/ast.rsvm/src/stdlib/ast/string.rscompiler/codegen/src/compile.rsstdlib/src/faulthandler.rsvm/src/vm/vm_new.rsvm/src/stdlib/ast/type_parameters.rsvm/src/stdlib/ast/constant.rscompiler/src/lib.rscompiler/codegen/src/lib.rsvm/src/builtins/frame.rsvm/src/stdlib/ast/statement.rsvm/src/frame.rsvm/src/stdlib/ast/expression.rsvm/src/stdlib/ast/exception.rs
{vm,stdlib}/**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use RustPython macros (
pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
vm/src/stdlib/ast/argument.rsvm/src/stdlib/ast/parameter.rsvm/src/stdlib/ast/module.rsvm/src/stdlib/ast/pattern.rsvm/src/stdlib/ast/elif_else_clause.rsvm/src/stdlib/ast/other.rsvm/src/stdlib/ast.rsvm/src/stdlib/ast/string.rsstdlib/src/faulthandler.rsvm/src/vm/vm_new.rsvm/src/stdlib/ast/type_parameters.rsvm/src/stdlib/ast/constant.rsvm/src/builtins/frame.rsvm/src/stdlib/ast/statement.rsvm/src/frame.rsvm/src/stdlib/ast/expression.rsvm/src/stdlib/ast/exception.rs
🧠 Learnings (1)
📚 Learning: 2025-09-11T05:06:59.971Z
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-11T05:06:59.971Z
Learning: Applies to {vm,stdlib}/**/*.rs : Use RustPython macros (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
src/shell.rs
🧬 Code graph analysis (6)
vm/src/stdlib/ast/pattern.rs (2)
vm/src/stdlib/ast/expression.rs (12)
ast_from_object(55-143)ast_from_object(167-186)ast_from_object(210-229)ast_from_object(256-280)ast_from_object(303-322)ast_from_object(346-365)ast_from_object(392-416)ast_from_object(447-472)ast_from_object(492-506)ast_from_object(530-549)ast_from_object(573-592)ast_from_object(619-643)vm/src/stdlib/ast.rs (2)
get_node_field(56-59)range_from_object(163-186)
vm/src/stdlib/ast/other.rs (1)
vm/src/stdlib/ast/expression.rs (14)
ast_to_object(11-53)ast_to_object(148-165)ast_to_object(191-208)ast_to_object(234-254)ast_to_object(285-302)ast_to_object(327-344)ast_to_object(370-390)ast_to_object(421-445)ast_to_object(477-491)ast_to_object(511-528)ast_to_object(554-571)ast_to_object(597-617)ast_to_object(648-666)ast_to_object(694-708)
vm/src/stdlib/ast.rs (1)
compiler/src/lib.rs (1)
location(58-63)
vm/src/stdlib/ast/string.rs (1)
vm/src/stdlib/ast/constant.rs (1)
new_str(13-23)
stdlib/src/faulthandler.rs (1)
vm/src/frame.rs (1)
current_location(173-175)
vm/src/stdlib/ast/statement.rs (3)
vm/src/stdlib/ast/exception.rs (2)
ast_to_object(6-10)ast_to_object(36-59)vm/src/stdlib/ast/expression.rs (14)
ast_to_object(11-53)ast_to_object(148-165)ast_to_object(191-208)ast_to_object(234-254)ast_to_object(285-302)ast_to_object(327-344)ast_to_object(370-390)ast_to_object(421-445)ast_to_object(477-491)ast_to_object(511-528)ast_to_object(554-571)ast_to_object(597-617)ast_to_object(648-666)ast_to_object(694-708)vm/src/stdlib/ast.rs (1)
range_from_object(163-186)
⏰ 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 tests under miri
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (windows-2025)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-2025)
🔇 Additional comments (31)
vm/src/builtins/frame.rs (1)
61-64: Switch tolinelooks right; confirm 1-based semantics and early-frame safety.This aligns with the row→line migration. Please confirm
SourceLocation.lineis 1-based to match CPython’sf_lineno. Also note: iflasti() == 0,current_location()will index with-1and panic; see suggested fix in vm/src/frame.rs.vm/src/frame.rs (1)
391-400: Tracebacklastivsidxoff-by-one — please confirm intent.Here,
idxis computed beforeupdate_lasti(|i| *i += 1), soframe.lasti()equalsidx + 1. You’re recordinglocforidxbut passingframe.lasti()toPyTraceback::new. If traceback expects the index of the instruction that raised, consider passingidx as u32 + 1explicitly oridxdepending on that API’s contract. Please verify.stdlib/src/faulthandler.rs (1)
11-15: LGTM: switched tolinefor consistency with new location fields.compiler/codegen/src/ir.rs (2)
185-187: LeverageSourceLocation’sCopysemanticsNice touch swapping
info.location.clone()for the by-value repeat; it keeps the iterator allocation-free while still duplicating the metadata for multi-word instructions.
425-493: Column math matches the UTF-8 offset semanticsUsing
loc.character_offset.to_zero_indexed()keeps the short/long encodings in sync withPositionEncoding::Utf8, so downstream consumers will continue to get zero-based columns without needing any extra conversions.wasm/lib/src/convert.rs (1)
254-260: JS error surface stays aligned with the new location fieldsConfirming that wiring
line.get()/character_offset.get()through preserves the 1-based numbers the browser tooling expects, so there’s no behavioral regression for WASM consumers.vm/src/vm/vm_new.rs (1)
323-417: Parser error mapping follows the renamed lexer diagnosticsThanks for updating both the enum path and the
line.to_zero_indexed()helper—error classification and snippet extraction remain consistent with the parser rename.compiler/core/src/marshal.rs (2)
201-206: Deserialization handles zero-indexed character offsetsReading the offset back via
OneIndexed::from_zero_indexedmirrors the data written on disk, so existing marshaled blobs round-trip without loss.
659-661: Serialized payload mirrors the new location schemaWriting
lineand zero-basedcharacter_offsetkeeps marshal format compatibility with the broaderSourceLocationrename while avoiding any off-by-one surprises.compiler/core/src/lib.rs (1)
11-13: Re-exportingPositionEncodingsmooths downstream transitionsPulling the new enum into the public surface saves dependents an extra import churn during the Ruff 0.13 bump—nice proactive step.
compiler/codegen/src/compile.rs (9)
633-637: Confirm character_offset initialization semantics.character_offset is set to OneIndexed::MIN. Please confirm this is the intended “column 1” equivalent with Utf8 encoding for all consumers of SourceLocation.
769-775: Module-scope RESUME location: line/character MIN — verify downstream expectations.Using OneIndexed::MIN for both fields diverges from CPython’s “-1” semantics. If any tooling assumes 1-based positions, this is fine; otherwise, worth double-checking.
416-473: Subscript/slice lowering: two-element slice path looks right; add coverage for Del and step.The special-case for [lower:upper] builds a slice and selects Subscript/StoreSubscript. For Del and for [: :step], you fall through to the generic path. Please add tests for:
- a[1:3] (load), a[1:3] = v (store), del a[1:3] (delete)
- a[1:3:2] (step present)
4873-4877: Named expression: ignoring node_index in codegen is fine.Plumbing the new field without affecting semantics is appropriate here.
5529-5540: Await detection for FString/TString integrated via helper — LGTM.
5612-5700: F-string lowering: debug text => repr, empty format-spec sentinel, BuildString sizing — looks correct; add a couple of tests.Please add coverage for:
- f"{x=}" (ensures debug_text forces repr)
- nested format specs and surrogate-literal reparse paths
27-33: AST/type import updates align with Ruff 0.13.x — good.Interpolated* types and PositionEncoding adoption here match the rest of the changes.
Also applies to: 36-37
1-1: Refine sanity checks and verify slice-lowering tests
- The lone
.source_location(call withoutPositionEncoding::Utf8is theSourceCode::index.source_location(offset, self.text)invocation incompiler/source/src/lib.rs; it’s an instance method that doesn’t take an encoding parameter—static calls all usePositionEncoding::Utf8.- All
Expr::TStringoccurrences are confined tocodegen/src/{unparse.rs,symboltable.rs,compile.rs}, so no stray handlers outside codegen.- No slice-lowering tests detected in Rust sources; please confirm that Python-level tests for slice deletion (e.g.
del a[1:3]) exist under/testsor add them, and update your regex to target.pyfiles rather than Rust code.
4885-4887: TString returns NotImplementedYet — gate or test this path.TString is present in the AST and referenced by symboltable/unparse/vm, but the compiler currently returns NotImplemented (compiler/codegen/src/compile.rs:4885–4887). Either:
- reuse the existing interpolated-string lowering (compile_fstring_elements at compiler/codegen/src/compile.rs:5613) to handle TString, or
- add a focused test that asserts the exact NotImplemented error for TString until full support is implemented.
I can sketch the minimal wiring to reuse compile_fstring_elements.
vm/src/stdlib/ast/argument.rs (3)
59-65: LGTM:Argumentsnow initializesnode_index.Initialization with
Default::default()is consistent across the PR.
70-75: LGTM: Destructuring ignoresnode_indexsafely.Discarding
node_indexhere has no behavioral impact.
110-114: LGTM: Destructuring in class-def split ignoresnode_index.Matches the function-call split behavior.
vm/src/stdlib/ast/module.rs (4)
69-74: LGTM:ModModuleignoresnode_indexin object conversion.Consistent with treating it as an internal field.
98-101: LGTM:ModModule::ast_from_objectinitializesnode_index.Matches the new field requirements.
152-156: LGTM:ModExpressiondestructuring includesnode_indexand preservesrange.No concerns.
173-180: LGTM:ModExpression::ast_from_objectsetsnode_indexdefault.Aligned with other nodes.
vm/src/stdlib/ast/elif_else_clause.rs (1)
10-15: LGTM: node_index destructuring is consistent.Ignoring
node_indexin to_object and defaulting it when constructing new clauses matches the pattern used across the AST conversions.vm/src/stdlib/ast/constant.rs (2)
250-345: LGTM: node_index initialization across Constant→Expr is thorough.All literal variants now initialize
node_indexand destructuring ignores it as intended.
366-437: LGTM: literal-to-object helpers handle the new field safely.Destructuring with
node_index: _avoids churn; behavior unchanged.vm/src/stdlib/ast/expression.rs (2)
148-186: LGTM: BoolOp conversion updated correctly with node_index.
286-301: LGTM: UnaryOp conversion mirrors the established pattern.
3887ae9 to
de8c06d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
vm/src/stdlib/ast/argument.rs (1)
160-165: Range is dropped inmerge_class_def_args.Using
range: Default::default()loses source locations and can degrade diagnostics. Compute the covered range from available parts, mirroringmerge_function_call_arguments.Apply:
- Some(Box::new(ruff::Arguments { - node_index: Default::default(), - range: Default::default(), // TODO - args, - keywords, - })) + // Compute range from whichever parts are present. + let range = match (positional_arguments, keyword_arguments) { + (Some(PositionalArguments { range: pr, .. }), Some(KeywordArguments { range: kr, .. })) => pr.cover(kr), + (Some(PositionalArguments { range: pr, .. }), None) => pr, + (None, Some(KeywordArguments { range: kr, .. })) => kr, + (None, None) => TextRange::default(), + }; + Some(Box::new(ruff::Arguments { + node_index: Default::default(), + range, + args, + keywords, + }))As per coding guidelines, run clippy/rustfmt after edits.
vm/src/stdlib/ast/other.rs (2)
111-133: Missingnode_add_locationforWithItem.Unlike
Alias, this builder never attaches therangeto the dict. Add it to avoid losing locations in Python-facing objects.dict.set_item( "optional_vars", optional_vars.ast_to_object(vm, source_file), vm, ) .unwrap(); + node_add_location(&dict, _range, vm, source_file); node.into()
141-151:WithItemdrops itsrangeon import.Constructing with
range: Default::default()loses source positions. Retrieve it like other nodes.- Ok(Self { + Ok(Self { node_index: Default::default(), context_expr: Node::ast_from_object( vm, source_file, get_node_field(vm, &object, "context_expr", "withitem")?, )?, optional_vars: get_node_field_opt(vm, &object, "optional_vars")? .map(|obj| Node::ast_from_object(vm, source_file, obj)) .transpose()?, - range: Default::default(), + range: range_from_object(vm, source_file, object, "withitem")?, })As per coding guidelines, run clippy/rustfmt.
compiler/core/src/marshal.rs (1)
362-375: Fix infinite recursion in MarshalBag impl for make_int/make_tuple/make_codeIn
impl<Bag: ConstantBag> MarshalBag for Bag, themake_int,make_tuple, andmake_codemethods currently call themselves, causing a stack overflow at runtime. They must dispatch to theConstantBagtrait implementation instead. For example:impl<Bag: ConstantBag> MarshalBag for Bag { @@ - fn make_int(&self, value: BigInt) -> Self::Value { - self.make_int(value) - } + fn make_int(&self, value: BigInt) -> Self::Value { + <Bag as ConstantBag>::make_int(self, value) + } @@ - fn make_tuple(&self, elements: impl Iterator<Item = Self::Value>) -> Self::Value { - self.make_tuple(elements) - } + fn make_tuple(&self, elements: impl Iterator<Item = Self::Value>) -> Self::Value { + <Bag as ConstantBag>::make_tuple(self, elements) + } @@ - fn make_code( - &self, - code: CodeObject<<Self::ConstantBag as ConstantBag>::Constant>, - ) -> Self::Value { - self.make_code(code) - } + fn make_code( + &self, + code: CodeObject<<Self::ConstantBag as ConstantBag>::Constant>, + ) -> Self::Value { + <Bag as ConstantBag>::make_code(self, code) + }Add a unit test exercising these via
deserialize_value(Type::Int/Tuple/Code)to catch regressions.vm/src/stdlib/ast/constant.rs (1)
304-328: frozenset(...) is built with multiple positional args (invalid); pass a single iterable.Python’s frozenset takes at most one positional argument (an iterable). Current construction emits frozenset(a, b, c), which is a runtime error. Wrap elements in a tuple (or a set) and pass it as the single arg.
- ConstantLiteral::FrozenSet(value) => ruff::Expr::Call(ruff::ExprCall { + ConstantLiteral::FrozenSet(value) => ruff::Expr::Call(ruff::ExprCall { node_index: Default::default(), range, // idk lol func: Box::new(ruff::Expr::Name(ruff::ExprName { node_index: Default::default(), range: TextRange::default(), id: ruff::name::Name::new_static("frozenset"), ctx: ruff::ExprContext::Load, })), arguments: ruff::Arguments { node_index: Default::default(), range, - args: value - .into_iter() - .map(|value| { - constant_to_ruff_expr(Constant { - range: TextRange::default(), - value, - }) - }) - .collect(), + args: vec![ruff::Expr::Tuple(ruff::ExprTuple { + node_index: Default::default(), + range: TextRange::default(), + elts: value + .into_iter() + .map(|value| { + constant_to_ruff_expr(Constant { + range: TextRange::default(), + value, + }) + }) + .collect(), + ctx: ruff::ExprContext::Load, + parenthesized: true, + })], keywords: Box::default(), }, }),vm/src/stdlib/ast/pattern.rs (2)
306-333: Fix MatchClass: remove todo! split/merge and inline the conversionsplit_pattern_match_class/merge_pattern_match_class are todo! and the wrapper structs don’t carry data, so this will panic at runtime. Inline the conversion instead.
Apply this diff:
@@ - let (patterns, kwd_attrs, kwd_patterns) = split_pattern_match_class(arguments); + // Inline split of arguments into the three PyAST fields. + let patterns_vec = arguments.patterns; + let (kwd_attrs_vec, kwd_patterns_vec): (Vec<_>, Vec<_>) = arguments + .keywords + .into_iter() + .map(|kw| (kw.attr, kw.pattern)) + .unzip(); @@ - dict.set_item("patterns", patterns.ast_to_object(vm, source_file), vm) + dict.set_item("patterns", patterns_vec.ast_to_object(vm, source_file), vm) .unwrap(); - dict.set_item("kwd_attrs", kwd_attrs.ast_to_object(vm, source_file), vm) + dict.set_item("kwd_attrs", kwd_attrs_vec.ast_to_object(vm, source_file), vm) .unwrap(); @@ - kwd_patterns.ast_to_object(vm, source_file), + kwd_patterns_vec.ast_to_object(vm, source_file), vm, )@@ - let (patterns, keywords) = merge_pattern_match_class(patterns, kwd_attrs, kwd_patterns); + // Inline merge of fields back into PatternArguments. + let keywords = kwd_attrs + .into_iter() + .zip(kwd_patterns) + .map(|(attr, pattern)| ruff::PatternKeyword { attr, pattern }) + .collect(); + let patterns = patterns; @@ - arguments: ruff::PatternArguments { + arguments: ruff::PatternArguments { node_index: Default::default(), range: Default::default(), - patterns, - keywords, + patterns, + keywords, },This removes the runtime panics and keeps the data flow explicit.
Also applies to: 335-372
201-213: Implement ruff::Singleton Node conversion (vm/src/stdlib/ast/pattern.rs:201-213)
Thetodo!()stubs will panic at runtime; implement bothast_to_objectandast_from_objectto mapNone,True, andFalseto the corresponding_ast.MatchSingleton.value.
🧹 Nitpick comments (10)
vm/src/stdlib/ast/argument.rs (3)
60-64: Consider preservingnode_indexwhen re-merging Arguments.You default the
node_indexon construction. If these args originated from an existingruff::Arguments, this loses stable identity. If feasible, threadnode_indexthroughsplit_*and accept it here to preserve it. Otherwise, confirm that defaulting is intentional.
71-75:node_indexis dropped on split.You explicitly ignore
node_index. If callers later re-merge, the index is lost. Consider returning it (e.g., an extraOption<NodeIndex>or embedding it in a wrapper) somerge_*can restore it.
110-114: Same as above:node_indexignored in class-def arg split.Preserve and propagate
node_indexto enable lossless round‑trips.vm/src/vm/vm_new.rs (1)
413-418: Useto_zero_indexed()to index lines.Good fix for off‑by‑one with 1‑based
line. Optionally, prefer.lines()over.split('\n')to handle CRLF uniformly (you still append the trailing newline explicitly).vm/src/stdlib/ast/string.rs (2)
20-33: Iterator pattern is hard to follow; align withFStringValueversion.Current use of
into_iter()twice and shadowingfstring_elementis easy to misread. Consider mirroringruff_fstring_value_into_iterwithlen()+iter_mut()(or exposeas_slice()/iter_mut()onInterpolatedStringElements) to avoid repeatedinto_iter()calls and shadowing.
50-57: Flags are intentionally dropped; re‑confirm.The
flags: _, // TODOinFStringPart::FStringbranch discards string flags. If this is still TODO, consider threading them through to avoid losing raw-prefix/case info.compiler/codegen/src/lib.rs (1)
59-60: LGTM; optional naming tweakMapping TString to “t-string expression” looks fine. Optionally consider “interpolated string” to keep error text consistent with f-strings.
compiler/src/lib.rs (1)
88-96: Normalize CRLF consistently in compile_symtable() too.compile() replaces CRLF on Windows but compile_symtable() doesn’t, which can desync locations/symtable offsets. Mirror the normalization to avoid skewed ranges.
pub fn compile_symtable( source: &str, mode: Mode, source_path: &str, ) -> Result<symboltable::SymbolTable, CompileError> { - let source_file = SourceFileBuilder::new(source_path, source).finish(); + #[cfg(windows)] + let source = source.replace("\r\n", "\n"); + #[cfg(windows)] + let source = source.as_str(); + let source_file = SourceFileBuilder::new(source_path, source).finish(); _compile_symtable(source_file, mode) }Also applies to: 133-140
vm/src/stdlib/ast/pattern.rs (2)
375-427: Remove dead wrappers and helpers or fully implement themPatternMatchClassPatterns/KeywordAttributes/KeywordPatterns structs and the split/merge helpers are unused after inlining and currently all todo!(). Delete them to avoid dead code, or fully implement them if you keep the helper design.
Also applies to: 534-552
47-48: Populate MatchCase.range (avoid losing locations)range is Default::default() here; prefer range_from_object(vm, source_file, object, "match_case") to preserve locations for error reporting.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
Cargo.toml(1 hunks)compiler/codegen/src/compile.rs(15 hunks)compiler/codegen/src/ir.rs(2 hunks)compiler/codegen/src/lib.rs(1 hunks)compiler/codegen/src/symboltable.rs(16 hunks)compiler/codegen/src/unparse.rs(24 hunks)compiler/core/src/bytecode.rs(1 hunks)compiler/core/src/lib.rs(1 hunks)compiler/core/src/marshal.rs(2 hunks)compiler/src/lib.rs(3 hunks)src/shell.rs(2 hunks)stdlib/src/faulthandler.rs(1 hunks)vm/src/builtins/frame.rs(1 hunks)vm/src/frame.rs(2 hunks)vm/src/stdlib/ast.rs(4 hunks)vm/src/stdlib/ast/argument.rs(4 hunks)vm/src/stdlib/ast/constant.rs(10 hunks)vm/src/stdlib/ast/elif_else_clause.rs(4 hunks)vm/src/stdlib/ast/exception.rs(2 hunks)vm/src/stdlib/ast/expression.rs(51 hunks)vm/src/stdlib/ast/module.rs(4 hunks)vm/src/stdlib/ast/other.rs(4 hunks)vm/src/stdlib/ast/parameter.rs(8 hunks)vm/src/stdlib/ast/pattern.rs(18 hunks)vm/src/stdlib/ast/statement.rs(46 hunks)vm/src/stdlib/ast/string.rs(10 hunks)vm/src/stdlib/ast/type_parameters.rs(7 hunks)vm/src/vm/vm_new.rs(2 hunks)wasm/lib/src/convert.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- vm/src/builtins/frame.rs
- compiler/core/src/bytecode.rs
- vm/src/stdlib/ast/elif_else_clause.rs
- vm/src/stdlib/ast/parameter.rs
- Cargo.toml
- compiler/core/src/lib.rs
- vm/src/stdlib/ast.rs
- compiler/codegen/src/symboltable.rs
- compiler/codegen/src/ir.rs
- vm/src/stdlib/ast/exception.rs
- vm/src/stdlib/ast/module.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Format Rust code with the default rustfmt style (runcargo fmt)
Run clippy and fix any warnings or lints introduced by your changes
Follow Rust best practices for error handling and memory management
Files:
wasm/lib/src/convert.rsvm/src/stdlib/ast/constant.rscompiler/codegen/src/lib.rsvm/src/stdlib/ast/other.rssrc/shell.rsvm/src/stdlib/ast/type_parameters.rsvm/src/stdlib/ast/argument.rsvm/src/stdlib/ast/statement.rsvm/src/stdlib/ast/expression.rsvm/src/frame.rsstdlib/src/faulthandler.rscompiler/codegen/src/unparse.rsvm/src/stdlib/ast/pattern.rsvm/src/vm/vm_new.rsvm/src/stdlib/ast/string.rscompiler/codegen/src/compile.rscompiler/core/src/marshal.rscompiler/src/lib.rs
{vm,stdlib}/**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use RustPython macros (
pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
vm/src/stdlib/ast/constant.rsvm/src/stdlib/ast/other.rsvm/src/stdlib/ast/type_parameters.rsvm/src/stdlib/ast/argument.rsvm/src/stdlib/ast/statement.rsvm/src/stdlib/ast/expression.rsvm/src/frame.rsstdlib/src/faulthandler.rsvm/src/stdlib/ast/pattern.rsvm/src/vm/vm_new.rsvm/src/stdlib/ast/string.rs
🧠 Learnings (1)
📚 Learning: 2025-09-11T05:06:59.971Z
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-11T05:06:59.971Z
Learning: Applies to {vm,stdlib}/**/*.rs : Use RustPython macros (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
src/shell.rs
🧬 Code graph analysis (5)
vm/src/stdlib/ast/other.rs (1)
vm/src/stdlib/ast/expression.rs (14)
ast_to_object(11-53)ast_to_object(148-165)ast_to_object(191-208)ast_to_object(234-254)ast_to_object(285-302)ast_to_object(327-344)ast_to_object(370-390)ast_to_object(421-445)ast_to_object(477-491)ast_to_object(511-528)ast_to_object(554-571)ast_to_object(597-617)ast_to_object(648-666)ast_to_object(694-708)
vm/src/stdlib/ast/statement.rs (3)
vm/src/stdlib/ast/exception.rs (2)
ast_to_object(6-10)ast_to_object(36-59)vm/src/stdlib/ast/expression.rs (14)
ast_to_object(11-53)ast_to_object(148-165)ast_to_object(191-208)ast_to_object(234-254)ast_to_object(285-302)ast_to_object(327-344)ast_to_object(370-390)ast_to_object(421-445)ast_to_object(477-491)ast_to_object(511-528)ast_to_object(554-571)ast_to_object(597-617)ast_to_object(648-666)ast_to_object(694-708)vm/src/stdlib/ast.rs (1)
range_from_object(163-186)
stdlib/src/faulthandler.rs (1)
vm/src/frame.rs (1)
current_location(173-175)
vm/src/stdlib/ast/pattern.rs (2)
vm/src/stdlib/ast/expression.rs (12)
ast_from_object(55-143)ast_from_object(167-186)ast_from_object(210-229)ast_from_object(256-280)ast_from_object(303-322)ast_from_object(346-365)ast_from_object(392-416)ast_from_object(447-472)ast_from_object(492-506)ast_from_object(530-549)ast_from_object(573-592)ast_from_object(619-643)vm/src/stdlib/ast.rs (2)
get_node_field(56-59)range_from_object(163-186)
vm/src/stdlib/ast/string.rs (1)
vm/src/stdlib/ast/constant.rs (1)
new_str(13-23)
⏰ 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). (10)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run rust tests (windows-2025)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run snippets and cpython tests (windows-2025)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
🔇 Additional comments (21)
vm/src/frame.rs (2)
391-399: LGTM! Consistent migration tolinefield.The traceback construction correctly uses
loc.lineinstead ofloc.rowand the logging statement is updated consistently. This aligns with the SourceLocation API changes in ruff 0.13.1.Based on learnings
173-175: Fix underflow incurrent_locationwhenlasti()==0.This is the same critical underflow issue identified in previous reviews that remains unfixed. Indexing with
self.lasti() as usize - 1will underflow for a brand‑new frame (lasti==0), causing OOB panic ifcurrent_location()is queried early (e.g., tracing/diagnostics).As per coding guidelines
stdlib/src/faulthandler.rs (1)
13-13: LGTM! Consistent API migration tolinefield.The change from
frame.current_location().rowtoframe.current_location().linecorrectly follows the SourceLocation API update in ruff 0.13.1. However, note that this depends on the critical underflow fix needed inFrame::current_location()method.Based on learnings
vm/src/vm/vm_new.rs (1)
324-326: Correct enum rename toInterpolatedStringErrorType.This aligns with the parser’s new error type. LGTM.
vm/src/stdlib/ast/other.rs (2)
59-63: Decorator mapping: defaultingnode_index.Defaulting and ignoring
node_indexin the Python object path is reasonable since Python’s AST has noDecoratornode; only expressions are exposed. LGTM.
70-84: Alias: includenode_indexbut ignore on export.Pattern matches and
node_add_locationuse are consistent with other nodes. LGTM.vm/src/stdlib/ast/string.rs (3)
358-363: DestructureExprFString: ignoringnode_indexis fine; keeprange.This path now carries
rangetoJoinedStrcorrectly; with the fix above, end‑to‑end locations are preserved. LGTM after range fix.
166-169: ExprFString drops its sourcerange.
range: Default::default()loses location info. Use the actualrangecaptured fromJoinedStr.- ruff::Expr::FString(ruff::ExprFString { - node_index: Default::default(), - range: Default::default(), + ruff::Expr::FString(ruff::ExprFString { + node_index: Default::default(), + range,
126-133: Use-after-move:elementmoved bymatch, then used.
match element { ... }moveselement. Referencing it invec![element]uses a moved value. Bind the inner value and reconstruct.- ruff::InterpolatedStringElement::Interpolation(ruff::InterpolatedElement { - range, .. - }) => ruff::FStringPart::FString(ruff::FString { - node_index: Default::default(), - range, - elements: vec![element].into(), - flags: ruff::FStringFlags::empty(), - }), + ruff::InterpolatedStringElement::Interpolation(el) => { + let range = el.range; + ruff::FStringPart::FString(ruff::FString { + node_index: Default::default(), + range, + elements: vec![ruff::InterpolatedStringElement::Interpolation(el)].into(), + flags: ruff::FStringFlags::empty(), + }) + }compiler/core/src/marshal.rs (2)
197-206: Location rename read path looks correct; confirm on-disk compatibilityDeserialization now reads (line: 1-based) and (character_offset: stored 0-based → OneIndexed). This matches the serialize path below. Please confirm the marshal format version (FORMAT_VERSION = 4) didn’t change semantics from previous row/column storage; otherwise a version bump is warranted.
659-661: Write path matches read path (1-based line, 0-based char offset)The round-trip for SourceLocation is consistent: write line.get() and character_offset.to_zero_indexed(). LGTM.
Consider adding a quick round‑trip test for mixed ASCII/UTF‑8 to validate character_offset behavior.
src/shell.rs (1)
3-6: LGTM: updated to InterpolatedStringErrorTypeMatches the parser API change; keeps the unterminated triple‑quoted detection working.
Please run the REPL quick check: enter an unterminated t/f‑triple‑quoted string and confirm it prompts for continuation.
Also applies to: 53-59
compiler/codegen/src/unparse.rs (1)
368-369: Preserve TString contents when unparsing (don’t emit empty t"")Current branch drops literal text and embedded expressions.
Minimal safe fix: print the original source slice for this node.
- Expr::TString(_) => self.p("t\"\"")?, + Expr::TString(ruff::ExprTString { range, .. }) => { + // Emit exactly as written to preserve contents/interpolations. + self.p(self.source.slice(*range))? + }Preferred fix: mirror FString handling by rendering the TString value via the same interpolated‑string machinery (prefix with "t" and feed elements to unparse_fstring_body), so escapes/normalization match f-strings. If you provide the exact ExprTString shape, I can draft that version too.
wasm/lib/src/convert.rs (1)
249-260: Guard optional error location to avoid WASM panic
CompileError::location()returns anOption<SourceLocation>, so callingunwrap()can abort the WASM module when there’s no location (e.g., non-parse errors). Wrap theReflect::setcalls inif let Some(loc) = err.location()or supply a default (e.g.0) forrow/colwhenNone.compiler/src/lib.rs (1)
46-56: Confirm encoding/offset semantics match Python expectations.We now compute locations via PositionEncoding::Utf8 and expose python_location as (line.get(), character_offset.get()). Please confirm this aligns with how vm/src/stdlib/ast.rs expects lineno/col_offset (1‑based line, 0‑based UTF‑8 column), to avoid off‑by‑one or encoding mismatches when round‑tripping.
Based on learnings
Also applies to: 65-71
vm/src/stdlib/ast/statement.rs (1)
171-186: node_index plumbed correctly through FunctionDef.Destructuring ignores node_index in ast_to_object and initializes it via Default in ast_from_object; consistent with the rest of the module.
Also applies to: 221-265
vm/src/stdlib/ast/type_parameters.rs (1)
18-22: node_index initialization looks correctDefaulting node_index here aligns with the broader migration; range is derived from first/last and defaults on empty, which is fine. Please confirm no downstream code assumes a non-empty range for TypeParams.
compiler/codegen/src/compile.rs (3)
239-256: Good: switched source_location to PositionEncoding::Utf8The move to Utf8 for start/end and instruction locations is consistent and LGTM.
Also applies to: 530-540, 5340-5348
632-637: Confirm character_offset semanticsYou set SourceLocation { line: OneIndexed, character_offset: OneIndexed::MIN }. Verify character_offset is intended to be 1-based here; if it’s a zero-based character index, this should be 0 (or an appropriate type) rather than OneIndexed::MIN.
Also applies to: 771-775
5550-5571: Nice helper: await detection in interpolated strings + element compilerThe recursive contains_await for Interpolated* and the updated compile_fstring_elements logic look solid.
Also applies to: 5616-5681
vm/src/stdlib/ast/expression.rs (1)
39-41: Handle TString instead of panickingSelf::TString(_) => unimplemented!() will panic. Implement the conversion (e.g., mirror FString’s mapping to PyAST) or temporarily map to the closest supported representation until full support lands.
#!/bin/bash # Locate any existing TString helpers to reuse rg -nP -C2 '\btstring_to_object\b|TString' vm/src/stdlib/ast
| Expr::TString(_) => { | ||
| return Err(self.error(CodegenErrorType::NotImplementedYet)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
TString not compiled (returns NotImplementedYet)
The parser can produce Expr::TString; this path will error during compilation. Either implement TString codegen (ideally reusing compile_fstring_elements with the new Interpolated* types) or gate it behind a feature/parse guard until supported.
Run to see where TString appears:
🏁 Script executed:
#!/bin/bash
rg -nP -C2 '\bTString\b' --compiler rustLength of output: 435
🏁 Script executed:
#!/bin/bash
# Locate all occurrences of `TString` in the codebase
rg -nP '\bTString\b' -C2Length of output: 2596
Add codegen or gate parsing for Expr::TString
Expr::TString(_) in compiler/codegen/src/compile.rs (lines 4885–4887) still returns NotImplementedYet. Support for t-strings is unimplemented across codegen, symbol table, and stdlib. Either implement codegen by reusing compile_expr_fstring on tstring.value.elements(), or gate its parsing behind a feature flag.
🤖 Prompt for AI Agents
In compiler/codegen/src/compile.rs around lines 4885–4887, Expr::TString(_)
currently returns NotImplementedYet; implement codegen for t-strings by
extracting the tstring value elements and calling the existing
compile_expr_fstring logic (e.g., pass tstring.value.elements() into
compile_expr_fstring and wire the returned IR/value through the same path as
f-strings), and ensure corresponding symbol-table entries and stdlib support are
present or updated; alternatively, if you don't want runtime support yet, gate
the parsing by adding a feature flag so TString nodes are not produced unless
the feature is enabled (update parser/AST creation and any callers accordingly)
and keep the NotImplemented error for the disabled case.
|
see also #6170 |
|
I somehow missed this PR. It's way better than #6170 |
The biggest contributor to the diff is from ruff adding a
node_indexfield to every AST node. Unfortunately, I had to add them all manually, but I'd like to at some point bring back the asdl_rs.py codegen script to make it less odious to edit.Summary by CodeRabbit
New Features
Bug Fixes
Chores