Update ruff 0.14.1#6195
Conversation
Sorry, something went wrong.
WalkthroughThis PR updates ruff-related dependencies and adapts the codebase to ruff's revised AST and source-location shapes: adding Changes
Sequence Diagram(s)sequenceDiagram
participant Parser as Ruff Parser (new AST)
participant Compiler as Compiler / Codegen
participant VM as VM / Runtime
Note over Parser,Compiler: AST shape changes: node_index, InterpolatedString, SourceLocation(line+char_off)
Parser->>Compiler: provide AST nodes (with node_index, ranges)
Compiler->>Compiler: validate duplicate params
Compiler->>Compiler: convert/interpolate strings (detect await inside interpolations)
Compiler->>Compiler: compute source locations using PositionEncoding::Utf8
Compiler->>VM: emit bytecode + linetable entries (line, character_offset)
VM->>VM: build tracebacks and frame info using .line/.character_offset
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (10)
vm/src/stdlib/ast/type_parameters.rs (1)
75-94: TypeVar: default_value is dropped in ast_to_object (serialization asymmetry).ast_from_object reads "default_value", but ast_to_object doesn’t write it. This loses the default type parameter when round‑tripping.
Apply this diff to serialize the default:
fn ast_to_object(self, _vm: &VirtualMachine, source_file: &SourceFile) -> PyObjectRef { let Self { - node_index: _, - name, - bound, - range: _range, - default: _, + node_index: _, + name, + bound, + range: _range, + default, } = self; let node = NodeAst .into_ref_with_type(_vm, pyast::NodeTypeParamTypeVar::static_type().to_owned()) .unwrap(); let dict = node.as_object().dict().unwrap(); dict.set_item("name", name.ast_to_object(_vm, source_file), _vm) .unwrap(); dict.set_item("bound", bound.ast_to_object(_vm, source_file), _vm) .unwrap(); + dict.set_item( + "default_value", + default.ast_to_object(_vm, source_file), + _vm, + ) + .unwrap(); node_add_location(&dict, _range, _vm, source_file); node.into() }vm/src/stdlib/ast/string.rs (2)
20-33: Use-after-move: fstring_element consumed at len() then used again.
fstring_element.into_iter().len()movesfstring_element, then it’s borrowed again in the closure — this won’t compile. Also O(n^2) due to re-creating the iterator each time.Apply this safe pattern that avoids the move and keeps behavior:
-fn ruff_fstring_element_into_iter( - mut fstring_element: ruff::InterpolatedStringElements, -) -> impl Iterator<Item = ruff::InterpolatedStringElement> + 'static { - let default = - ruff::InterpolatedStringElement::Literal(ruff::InterpolatedStringLiteralElement { - node_index: Default::default(), - range: Default::default(), - value: Default::default(), - }); - (0..fstring_element.into_iter().len()).map(move |i| { - let fstring_element = &mut fstring_element; - let tmp = fstring_element.into_iter().nth(i).unwrap(); - std::mem::replace(tmp, default.clone()) - }) -} +fn ruff_fstring_element_into_iter( + mut fstring_element: ruff::InterpolatedStringElements, +) -> impl Iterator<Item = ruff::InterpolatedStringElement> + 'static { + let default = + ruff::InterpolatedStringElement::Literal(ruff::InterpolatedStringLiteralElement { + node_index: Default::default(), + range: Default::default(), + value: Default::default(), + }); + let len = fstring_element.len(); // avoid moving the container + (0..len).map(move |i| { + // create a fresh &mut iterator each time; consider optimizing later + let tmp = fstring_element.iter_mut().nth(i).expect("in-bounds"); + std::mem::replace(tmp, default.clone()) + }) +}Optionally, we can later switch to a linear-time drain-style iterator if
InterpolatedStringElementsexposes a mutable slice.
164-197: ExprFString loses source range (set to Default).Top-level
ExprFString { range: Default::default(), .. }drops location info. Should be theJoinedStr.rangefor accurate tracebacks/mappings.Apply this minimal fix:
impl JoinedStr { pub(super) fn into_expr(self) -> ruff::Expr { let Self { range, values } = self; ruff::Expr::FString(ruff::ExprFString { node_index: Default::default(), - range: Default::default(), + range, value: match values.len() { // unchanged…vm/src/stdlib/ast/other.rs (1)
109-133: WithItem misses location propagation in ast_to_object_range is captured but never applied; Alias does call node_add_location. Add node_add_location to keep locations consistent.
Apply:
let dict = node.as_object().dict().unwrap(); @@ 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()vm/src/stdlib/ast/parameter.rs (1)
352-361: Fix potential usize underflow when computing default_argument_countIf
defaults.len() > posonlyargs.len() + args.len(),len_a + len_b - len_cunderflows (debug panic / release wrap), leading to incorrect.skip()behavior. Use saturating subtraction and keep intent explicit.Apply:
- let default_argument_count = posonlyargs.len() + args.len() - defaults.len(); + let total = posonlyargs.len() + args.len(); + let default_argument_count = total.saturating_sub(defaults.len());vm/src/stdlib/ast/pattern.rs (3)
314-333: Runtime panic risk: split_pattern_match_class is todo! but invoked
PatternMatchClass::ast_to_objectcallssplit_pattern_match_class, which istodo!(). Any conversion of match-class patterns to objects will panic.
- Implement
split_pattern_match_class(extract positional patterns, keyword attrs, keyword patterns).- If not ready, gate the call and return a structured Python-side error instead of panicking, or temporarily
unreachable!()under a feature flag to avoid accidental invocation.Also applies to: 534-543
357-372: Runtime panic risk: merge_pattern_match_class is todo! but invoked
PatternMatchClass::ast_from_objectconstructs viamerge_pattern_match_class, which istodo!(). Deserializing match-class patterns from Python objects will panic.
- Implement
merge_pattern_match_classto combine(patterns, kwd_attrs, kwd_patterns)intoVec<ruff::Pattern>andVec<ruff::PatternKeyword>.- Until implemented, fail gracefully with a descriptive error rather than panicking.
Also applies to: 544-551
164-183: Unimplemented ruff::Singleton Node conversions break match-singleton paths
PatternMatchSingletondelegates toruff::SingletonNode conversion, butruff::Singletonhastodo!()in bothast_to_objectandast_from_object. Any (de)serialization ofMatchSingletonwill panic.I can draft the minimal
ruff::Singletonconversions for {None, True, False} to unblock this.Also applies to: 184-199, 201-213
compiler/codegen/src/symboltable.rs (1)
796-827: Reorder type parameter scope entry and consolidate.type_paramsregistration for PEP 695 compliance.The concerns in your review are verified:
Scope visibility confirmed:
enter_type_param_blockat lines 796-827 (ClassDef), 1419-1426 (FunctionDef), and 663-686 (TypeAlias) is called before entering the parent scope. Whenenter_type_param_blockchecksin_class = self.tables.last().is_some_and(|t| t.typ == CompilerScope::Class), it returns false for class definitions because the class scope is entered afterward, leavingcan_see_class_scope = false. This misaligns with PEP 695 semantics where type parameters should be visible to class members.Double registration confirmed:
.type_paramsis registered twice—once inenter_type_param_block(line 680 withSymbolUsage::Assigned) and again inscan_type_params(line 1421 withSymbolUsage::TypeParam).Required fixes:
- Reorder: enter parent scope (Class/Function) first, then enter type parameter block, or pass scope context to
enter_type_param_block- Remove one
.type_paramsregistration to eliminate redundancyvm/src/stdlib/ast/expression.rs (1)
1271-1292: Addnode_add_location()call for Comprehension objects to maintain consistency.The
node_add_location()function is called in every otherimpl_to_objectthroughout the AST module (70+ instances), but is missing from theComprehensionimplementation at lines 1271-1292. The_rangeparameter is available but unused, indicating this was an oversight. While Python's standard AST spec does not include location attributes oncomprehensionobjects, this RustPython implementation systematically adds location info to all node types. The proposed fix is correct:let dict = node.as_object().dict().unwrap(); dict.set_item("target", target.ast_to_object(vm, source_file), vm) .unwrap(); dict.set_item("iter", iter.ast_to_object(vm, source_file), vm) .unwrap(); dict.set_item("ifs", ifs.ast_to_object(vm, source_file), vm) .unwrap(); dict.set_item("is_async", is_async.ast_to_object(vm, source_file), vm) .unwrap(); + node_add_location(&dict, _range, vm, source_file); node.into()
🧹 Nitpick comments (9)
vm/src/stdlib/ast/elif_else_clause.rs (1)
10-15: Consistent handling of node_index in elif/else clauses.Ignoring incoming indices where appropriate and defaulting new ones is fine for internal indexing.
If node_index is intended for stable mapping across transforms, consider threading through (not defaulting) when lifting nested If into ElifElseClause.
Also applies to: 63-66, 75-76, 84-85, 92-93
vm/src/stdlib/ast/string.rs (1)
4-17: Quadratic iteration over fstring_value (perf nit).Repeated
iter_mut().nth(i)is O(n^2). If this path is hot, consider a linear iterator that replaces in place without re-scanning each time.Example direction (subject to API availability):
- Precompute
len, then iterate once with a mutable index and use direct indexing or a draining iterator.- Or, if
FStringValuesupports move-out iteration, preferinto_iter()to avoid replacements.vm/src/stdlib/ast/argument.rs (1)
160-165: Compute a meaningful range when merging class-def argsrange: Default::default() drops location data. Build the covered range from args/keywords for consistency with merge_function_call_arguments.
Apply:
- Some(Box::new(ruff::Arguments { - node_index: Default::default(), - range: Default::default(), // TODO - args, - keywords, - })) + // Derive range from any present args/keywords + let range = args + .iter() + .map(|a| a.range()) + .chain(keywords.iter().map(|k| k.range())) + .reduce(|acc, next| acc.cover(next)) + .unwrap_or_default(); + + Some(Box::new(ruff::Arguments { + node_index: Default::default(), + range, + args, + keywords, + }))compiler/codegen/src/ir.rs (2)
185-191: Location replication matches instruction expansionUsing repeat_n(info.location, arg.instr_size()) keeps locations aligned with extras + base opcode. LGTM. Consider a brief assert in debug to prevent drift if encoding changes.
424-427: Update comment to match renamed fields and zero-based columnThe comment still mentions “row/column (OneIndexed)”. Now it’s line (OneIndexed) and character_offset with to_zero_indexed(). Suggest updating the comment to avoid confusion.
vm/src/stdlib/ast/other.rs (1)
140-151: WithItem range set to Default in ast_from_objectUse range_from_object to preserve locations like Alias and ExceptHandler.
Apply:
- Ok(Self { - node_index: Default::default(), + 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")?, })vm/src/stdlib/ast/parameter.rs (1)
406-413: Guard against mismatched keyword/default lengths
zip(kw_only_args.keywords, defaults.defaults)truncates to the shorter length. If lengths diverge, defaults will be silently dropped or missing. Consider asserting equality in debug or returning an error.std::iter::zip(kw_only_args.keywords, defaults.defaults) .map(|(parameter, default)| ruff::ParameterWithDefault { node_index: Default::default(), parameter, default, range: Default::default(), }) .collect() + // debug-only sanity: + // debug_assert_eq!(kw_only_args_len, kw_defaults_len, "kwonlyargs/defaults length mismatch");vm/src/stdlib/ast/statement.rs (2)
963-969: Remove duplicate_clsfetch inStmtTry::ast_from_object.
let _cls = _object.class();is called twice; the second is redundant.- let _cls = _object.class(); - let is_star = _cls.is(pyast::NodeStmtTryStar::static_type()); - let _cls = _object.class(); + 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()) );
171-221: Unify identifier conversion style forFunctionDefvsClassDef.
FunctionDef::ast_to_objectwritesnameviavm.ctx.new_str(name.as_str()), whileClassDef::ast_to_objectcallsname.ast_to_object(...). Consider standardizing on one approach for consistency and to avoid surprises if either path gains behavior.If
IdentifierimplementsNode, prefer using it consistently. Otherwise, prefer directnew_str(...)for both.Also applies to: 268-307
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockLib/test/test_fstring.pyis excluded by!Lib/**
📒 Files selected for processing (29)
Cargo.toml(1 hunks)compiler/codegen/src/compile.rs(19 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:
vm/src/builtins/frame.rscompiler/codegen/src/lib.rscompiler/core/src/lib.rscompiler/codegen/src/ir.rswasm/lib/src/convert.rscompiler/core/src/marshal.rsvm/src/stdlib/ast/module.rscompiler/core/src/bytecode.rsstdlib/src/faulthandler.rsvm/src/vm/vm_new.rsvm/src/stdlib/ast/exception.rsvm/src/frame.rssrc/shell.rsvm/src/stdlib/ast/constant.rscompiler/codegen/src/unparse.rsvm/src/stdlib/ast/other.rsvm/src/stdlib/ast/type_parameters.rscompiler/src/lib.rsvm/src/stdlib/ast/elif_else_clause.rsvm/src/stdlib/ast/pattern.rsvm/src/stdlib/ast/string.rsvm/src/stdlib/ast.rsvm/src/stdlib/ast/expression.rsvm/src/stdlib/ast/statement.rsvm/src/stdlib/ast/parameter.rsvm/src/stdlib/ast/argument.rscompiler/codegen/src/symboltable.rscompiler/codegen/src/compile.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/builtins/frame.rsvm/src/stdlib/ast/module.rsstdlib/src/faulthandler.rsvm/src/vm/vm_new.rsvm/src/stdlib/ast/exception.rsvm/src/frame.rsvm/src/stdlib/ast/constant.rsvm/src/stdlib/ast/other.rsvm/src/stdlib/ast/type_parameters.rsvm/src/stdlib/ast/elif_else_clause.rsvm/src/stdlib/ast/pattern.rsvm/src/stdlib/ast/string.rsvm/src/stdlib/ast.rsvm/src/stdlib/ast/expression.rsvm/src/stdlib/ast/statement.rsvm/src/stdlib/ast/parameter.rsvm/src/stdlib/ast/argument.rs
🧬 Code graph analysis (5)
stdlib/src/faulthandler.rs (1)
vm/src/frame.rs (1)
current_location(173-175)
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/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)
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 snippets and cpython tests (macos-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
🔇 Additional comments (39)
Cargo.toml (1)
161-164: Ruff dependency updates verified and consistent.The tag
0.14.1exists in the Ruff repository, and all four ruff crates in the workspace are correctly pinned to this version. Member crates properly reference these dependencies via{ workspace = true }, with no conflicting versions detected elsewhere in the workspace. The updates are correctly coordinated.vm/src/vm/vm_new.rs (2)
320-334: LGTM! Correct update to InterpolatedStringErrorType.The migration from
FStringErrorTypetoInterpolatedStringErrorTypecorrectly reflects ruff's API changes. The error handling logic for unterminated triple-quoted strings remains intact.
412-418: LGTM! Correct field rename from row to line.The
SourceLocationfield rename fromrowtolineimproves semantic clarity while maintaining the same functionality for extracting error statement lines.src/shell.rs (2)
3-6: LGTM! Import correctly updated.The import of
InterpolatedStringErrorType(replacing the previousFStringErrorType) correctly reflects ruff's API changes.
52-58: LGTM! Correct type usage update.The pattern match correctly uses
InterpolatedStringErrorType::UnterminatedTripleQuotedString, maintaining the REPL's input continuation behavior for unterminated triple-quoted interpolated strings.vm/src/builtins/frame.rs (1)
62-64: Alignment with new SourceLocation fields looks good.The getter now uses line; consistent with vm/src/frame.rs. No issues here.
compiler/core/src/marshal.rs (1)
201-203: Location encoding is consistent (1-based line, 0-based column).Read uses OneIndexed::new for line and from_zero_indexed for character_offset; write uses line.get() and character_offset.to_zero_indexed(). Good.
Consider adding a quick round‑trip test for a few edge positions (line 1/col 0; last line/col end) to prevent regressions.
Also applies to: 659-661
vm/src/frame.rs (1)
391-400: Traceback construction updated to use loc.line — LGTM.Mapping to PyTraceback now passes OneIndexed line; consistent with new fields.
Double-check that frame.lasti() here matches the expected “last executed” index (preincrement happens earlier). If consumers assume CPython semantics, add a quick assertion in tests.
stdlib/src/faulthandler.rs (1)
11-15: Uses .line in faulthandler output — OK.Matches new SourceLocation fields. Safe once current_location handles lasti==0.
vm/src/stdlib/ast/module.rs (1)
69-73: node_index propagation is consistent.Destructuring ignores node_index on to_object and initializes it with Default on from_object. Matches the broader AST changes.
Also applies to: 99-101, 152-156, 173-174
vm/src/stdlib/ast.rs (1)
26-27: UTF‑8 position encoding + field rename wired correctly.
- PySourceLocation now maps row/column -> line/character_offset.
- Offsets computed with PositionEncoding::Utf8.
- Interactive mode wrapped into ModModule with node_index default. All good.
Add/adjust a parser test asserting lineno/col_offset round‑trip for multibyte UTF‑8 to lock in the encoding change.
Also applies to: 94-98, 196-201, 202-205, 276-280
wasm/lib/src/convert.rs (1)
254-255: The review comment is based on an incorrect premise—no action required.rustpython uses Python's convention where line numbers are 1-based and character offsets are 0-based. The code at lines 254-255 and 259-260 is already correct:
err.location().unwrap().line.get()provides 1-based row (standard for editors)err.location().unwrap().character_offset.get()provides 0-based column (standard for tooling and JavaScript error conventions)The review incorrectly claimed
character_offset.get()is 1-based. It is actually 0-based. Theto_zero_indexed()method does not exist and is not needed—the values are already in the correct indexing base for JavaScript consumers.vm/src/stdlib/ast/type_parameters.rs (1)
18-22: LGTM on node_index default initialization.Constructor now threads node_index consistently. No issues.
vm/src/stdlib/ast/string.rs (1)
89-107: Format-spec conversion looks correct with node_index threading.Destructure/mapping to JoinedStr preserves range and elements; node_index intentionally ignored. LGTM.
Please confirm
InterpolatedStringFormatSpec.elementsordering matches CPython fmt parsing expectations in downstream consumers.compiler/codegen/src/lib.rs (1)
59-60: Add TString name mapping — good coverage.This keeps error messages coherent for t-strings. No issues.
compiler/core/src/lib.rs (1)
11-13: Re-export PositionEncoding — consistent with location model.Public surface now matches downstream imports. LGTM.
vm/src/stdlib/ast/argument.rs (3)
59-64: node_index defaulting is fine hereSetting node_index: Default::default() for newly constructed ruff::Arguments is acceptable given we don't rely on stable IDs downstream.
70-75: Ignoring node_index on split is appropriateDiscarding node_index when splitting preserves behavior and avoids leaking internal IDs to wrappers.
109-114: Consistent destructuringIgnoring node_index on class-def split matches the function-call split. LGTM.
compiler/src/lib.rs (3)
46-55: Explicit UTF‑8 position encoding is correctUsing PositionEncoding::Utf8 for parse errors aligns python_location and linetable expectations. LGTM.
65-71: python_location stays 1‑based; consistent with user expectationsReturning (line.get(), character_offset.get()) keeps 1‑based reporting. LGTM.
Double-check any callers that expect zero-based columns; most human-facing errors expect 1-based, while linetable uses zero-based. Confirm no mismatch.
58-63: Code is correct; SourceLocation implements CopyThe review comment's concern is invalid. SourceLocation from ruff_source_file derives Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Debug, making
Option<SourceLocation>also Copy-compatible. Theconst fn location(&self) -> Option<SourceLocation>implementation at lines 58–63 is sound and requires no changes.vm/src/stdlib/ast/other.rs (2)
59-63: Decorator node_index wiring: OKSetting node_index: Default::default() and deriving range from expression is consistent. LGTM.
69-85: Alias ast_to_object ignores node_index as intendedLocation is propagated via node_add_location; looks good.
vm/src/stdlib/ast/exception.rs (2)
37-43: ExceptHandlerExceptHandler: ignoring node_index on to_object is fineMatches pattern across AST nodes. LGTM.
66-81: ExceptHandlerExceptHandler: node_index defaulting and location handling look goodLocation is preserved via range_from_object and node_add_location. LGTM.
vm/src/stdlib/ast/parameter.rs (1)
8-15: node_index plumbing looks consistentIgnoring on serialization and defaulting on deserialization aligns with the rest of the AST conversions.
Also applies to: 84-96, 107-116, 141-154, 162-171, 184-195, 335-350
vm/src/stdlib/ast/pattern.rs (1)
219-234: node_index defaulting across pattern nodes looks goodConsistent defaulting and ignoring in destructuring; location handling unchanged.
Also applies to: 241-250, 257-278, 286-302, 309-333, 341-372, 430-458, 463-498, 503-531
vm/src/stdlib/ast/constant.rs (1)
250-258: node_index propagation through constants: OKDefaulting
node_indexon construction and ignoring on object round-trips is consistent and low-risk.Also applies to: 260-270, 272-282, 284-288, 289-304, 305-328, 330-333, 335-340, 341-345
compiler/codegen/src/unparse.rs (1)
536-543: InterpolatedStringElement migration: looks solidUpdated signatures and traversal match the new AST shape.
Also applies to: 588-606
compiler/codegen/src/compile.rs (4)
150-162: Duplicate parameter check added — good, but verify coverage of vararg/kwargLogic is correct and fast. Ensure the iterator over
Parametersincludes:
- posonly + args,
- kwonly,
- vararg,
- kwarg.
Add tests that exercise duplicates across categories (e.g.,
def f(a, *a): ...,def f(*, a, a=1): ...,lambda a, a: ...) to confirm detection triggers everywhere.
1517-1529: Placement of duplicate-parameter validation is rightRunning validation before emitting function/lambda code prevents partial codegen.
Also applies to: 4678-4683
5570-5591: Await detection in interpolated strings covers nested specsRecursive walk through
InterpolatedStringElementand nestedformat_speclooks correct.
254-269: Utf8-based SourceLocation use is consistentSwitched to
PositionEncoding::Utf8across error/reporting helpers; good.Also applies to: 544-554, 5361-5369
vm/src/stdlib/ast/expression.rs (2)
686-688:parenthesized: truehard-coded; verify correctness.Both
ExprGeneratorandExprTuplesetparenthesized: trueunconditionally. This may misrepresent tuples without parens and certain generator contexts.Please confirm against Ruff 0.14.1 AST semantics. If fidelity matters, derive this flag during (de)serialization rather than forcing
true. I can help wire it from the parser tokens if exposed.Also applies to: 1179-1181
149-154: node_index threading looks correct.Destructuring with
node_index: _and defaulting it inast_from_objectis consistent and low-risk.Also applies to: 173-174, 236-242, 262-263, 286-292, 309-310, 329-334, 352-353, 372-377, 398-399, 422-427, 468-469, 478-483, 498-499
vm/src/stdlib/ast/statement.rs (1)
354-365: node_index propagation and location setting are consistent.Pattern matches introduce
node_indexand keep ranges vianode_add_location(...). Looks good.Also applies to: 385-397, 418-437, 465-489, 520-540, 570-597, 631-662, 709-729, 757-777, 788-814, 848-865, 889-906, 1001-1017, 1039-1053, 1072-1092, 1119-1132, 1151-1165, 1184-1198, 1217-1228, 1243-1253, 1269-1280
compiler/codegen/src/symboltable.rs (2)
22-23: UTF‑8 position encoding adoption is correct.The switch to
PositionEncoding::Utf8onsource_location(...)aligns locations with the new encoding model.Also applies to: 1051-1053, 1607-1609
1271-1279: F‑string scanning updates look correct and comprehensive.Using
elements().filter_map(|x| x.as_interpolation())and scanning nested format specs viainterpolations()is aligned with the new representation.Also applies to: 1310-1318, 1169-1176, 1186-1193, 1195-1213, 1241-1269
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
compiler/codegen/src/compile.rs (1)
4903-4908: Return a targeted SyntaxError for TString, not NotImplementedYet.Users should get a clear syntax error like “t-strings not yet supported” with the expression location, instead of a generic NotImplemented. Replace the Err accordingly.
- Expr::TString(_) => { - return Err(self.error(CodegenErrorType::NotImplementedYet)); - } + Expr::TString(_) => { + return Err(self.error(CodegenErrorType::SyntaxError( + "t-strings not yet supported".to_owned(), + ))); + }This aligns compiler behavior with user expectations and prior review guidance.
🧹 Nitpick comments (2)
compiler/codegen/src/compile.rs (2)
3600-3633: Duplicate mapping-key guard is good; tiny polish.Using
HashSetonUnparseExprfor literal keys prevents duplicate-key patterns. Optional: pre-allocateseenwithkeys.len()for small perf wins and moveself.compile_expression(key)?behind validation to fail fast.
5529-5567: Optional: check dict values in contains_await for completeness.
contains_awaitforExpr::Dictonly inspects keys; consider also checking values to keep symmetry, even if current call-sites already handle dictcomp specially.- Expr::Dict(ExprDict { items, .. }) => items - .iter() - .flat_map(|item| &item.key) - .any(Self::contains_await), + Expr::Dict(ExprDict { items, .. }) => items.iter().any(|item| { + item.key.as_deref().is_some_and(Self::contains_await) + || Self::contains_await(&item.value) + }), ```<!-- review_comment_end --> <!-- file_end --> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Path: .coderabbit.yml **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 0a487413c1bad7ab03811b149fef2ed1348bcc45 and fb2dc3b5bfa4d6c8df765f1e1f0e3af77b003451. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `compiler/codegen/src/compile.rs` (19 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (1)</summary> <details> <summary>**/*.rs</summary> **📄 CodeRabbit inference engine (.github/copilot-instructions.md)** > `**/*.rs`: Format Rust code with the default rustfmt style (run `cargo 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/codegen/src/compile.rs` </details> </details> </details> <details> <summary>⏰ 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). (5)</summary> * GitHub Check: Run snippets and cpython tests (windows-latest) * GitHub Check: Run snippets and cpython tests (macos-latest) * GitHub Check: Run rust tests (ubuntu-latest) * GitHub Check: Run rust tests (windows-latest) * GitHub Check: Run rust tests (macos-latest) </details> <details> <summary>🔇 Additional comments (4)</summary><blockquote> <details> <summary>compiler/codegen/src/compile.rs (4)</summary><blockquote> `1517-1529`: **Call-site integration for duplicate-parameter checks looks right.** Validating before compiling the function body is the correct timing. After fixing the validator, this should emit a SyntaxError early with proper location via `self.error(...)`. Add a small compile-time test that asserts a duplicate-parameters function raises a SyntaxError with the function span (not a generic internal error).<!-- review_comment_end --> --- `4679-4682`: **Good: Lambda parameters also validated.** Catching duplicates in lambdas keeps behavior consistent with functions. <!-- review_comment_end --> --- `253-269`: **Source location updates to PositionEncoding::Utf8 LGTM.** Switching to `PositionEncoding::Utf8` and using `line`/`character_offset` is consistent and improves diagnostics. <!-- review_comment_end --> Also applies to: 548-552, 5361-5369 --- `5570-5591`: **Nice: await detection in interpolated strings (f/t-strings).** The recursive walk over `InterpolatedStringElement` and nested format specs is correct and covers tricky cases. Consider adding a mirror test for `TString` like the existing `test_fstring_contains_await`. <!-- review_comment_end --> </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Sorry, something went wrong.
youknowone
merged commit fcf1969
into
RustPython:main
Oct 20, 2025
Reviewers
3 participants
Took @coolreader18 PR #6177 (he did 99% of the work) and updated ruff to 0.14.1 and patched our compile function as ruff moved duplicated params to be a semantic error, since 0.11.7 (astral-sh/ruff#17131)
Closes #6170
Closes #6177
Summary by CodeRabbit
Bug Fixes
Chores