Replace compiler::source module with ruff_source_file
#6016
Conversation
WalkthroughThis change removes the dependency on the deprecated Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant VM
participant Compiler
participant RuffSourceFile
User->>VM: Provide source string and path
VM->>RuffSourceFile: Build SourceFile from string/path
VM->>Compiler: Call compile/parse with SourceFile
Compiler->>RuffSourceFile: Query source locations, slices, etc.
Compiler-->>VM: Return result (AST, code object, etc.)
VM-->>User: Return result or error
Estimated code review effort3 (~45 minutes) Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ 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)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
compiler/codegen/src/compile.rs (1)
340-361: Duplicateflagsfield – this will not compile
ir::CodeInfoalready receives aflagsvalue at line 341, but the same field is initialised again at line 360.
Rust does not allow duplicate struct fields; this is a hard‐error and will break the build.- stacksize: 0, - flags: bytecode::CodeFlags::empty(), + stacksize: 0,Remove the second occurrence (or rename it if you intended a different field).
🧹 Nitpick comments (7)
vm/src/vm/vm_new.rs (1)
289-290: Stale “TODO” comment—should referenceSourceFile, notSourceCodeNow that the file imports
ruff_source_file::SourceLocation, the comment a few lines below (// TODO: replace to SourceCode) is misleading. Recommend updating or deleting it to avoid confusion.- // TODO: replace to SourceCode + // TODO: revisit: `SourceFile` and `SourceLocation` now come from `ruff_source_file`Cargo.toml (1)
117-135: Consider keeping workspace member list alphabetisedThe reordered member list still contains
"."alongside crate paths, but the sequence is no longer alphabetical ("compiler"precedes"."). Sorting keeps large workspace manifests easier to scan and merge-conflict-free.
No functional impact—purely a readability / maintenance nit.vm/src/builtins/traceback.rs (1)
55-58: Minor API ergonomics: consider exposingtb_linenoasusizeandOneIndexed
tb_lineno()returns the rawusize, which mirrors CPython, but any Rust-side consumer that already has aOneIndexedwill need to convert back.
Optionally expose a second getter:+#[pygetset(name = "tb_lineno_oneindexed")] +const fn tb_lineno_oneindexed(&self) -> OneIndexed { + self.lineno +}Purely ergonomic – feel free to skip.
Also applies to: 74-78
compiler/codegen/src/compile.rs (4)
240-248: Avoid repeatedto_source_code()conversionsCalling
self.source_file.to_source_code()twice per location calculation is wasteful and clutters the code.
Store theSourceCodeonce (or call the location helpers directly onSourceFileif available).- let start = zelf - .source_file - .to_source_code() - .source_location(zelf.current_source_range.start()); + let sc = zelf.source_file.to_source_code(); + let start = sc.source_location(zelf.current_source_range.start());
5318-5322: Minor: cacheSourceCodefor line look-ups
get_source_line_numberre-creates aSourceCodewrapper on every call.
When walking large ASTs this shows up in profiles. Consider caching or adding a thin helper onSourceFile.
5224-5231: Hot path alloc – consider re-usingSourceLocation
_emitallocates aSourceLocationfor every bytecode instruction.
For large inputs this dominates compile-time. You can:
- Compute the location once per line and reuse.
- Delay the translation until after IR generation.
Not blocking, but worth a TODO.
5848-5855:SourceFileBuilder::new("source_path", …)leaks dummy path into error messagesTests hard-code
"source_path"which shows up in tracebacks and diagnostics.
Consider passing the real test-file name (or at least a sentinel like"<test>") to avoid confusion downstream.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
Cargo.toml(1 hunks)compiler/Cargo.toml(1 hunks)compiler/codegen/Cargo.toml(0 hunks)compiler/codegen/src/compile.rs(22 hunks)compiler/codegen/src/symboltable.rs(8 hunks)compiler/codegen/src/unparse.rs(4 hunks)compiler/source/Cargo.toml(0 hunks)compiler/source/src/lib.rs(0 hunks)compiler/src/lib.rs(5 hunks)vm/Cargo.toml(2 hunks)vm/src/builtins/code.rs(2 hunks)vm/src/builtins/traceback.rs(4 hunks)vm/src/compiler.rs(1 hunks)vm/src/frame.rs(1 hunks)vm/src/lib.rs(0 hunks)vm/src/stdlib/ast.rs(9 hunks)vm/src/stdlib/ast/argument.rs(2 hunks)vm/src/stdlib/ast/basic.rs(3 hunks)vm/src/stdlib/ast/constant.rs(9 hunks)vm/src/stdlib/ast/elif_else_clause.rs(1 hunks)vm/src/stdlib/ast/exception.rs(3 hunks)vm/src/stdlib/ast/expression.rs(23 hunks)vm/src/stdlib/ast/module.rs(8 hunks)vm/src/stdlib/ast/node.rs(4 hunks)vm/src/stdlib/ast/operator.rs(8 hunks)vm/src/stdlib/ast/other.rs(6 hunks)vm/src/stdlib/ast/parameter.rs(9 hunks)vm/src/stdlib/ast/pattern.rs(16 hunks)vm/src/stdlib/ast/statement.rs(27 hunks)vm/src/stdlib/ast/string.rs(5 hunks)vm/src/stdlib/ast/type_ignore.rs(4 hunks)vm/src/stdlib/ast/type_parameters.rs(6 hunks)vm/src/vm/vm_new.rs(1 hunks)
💤 Files with no reviewable changes (4)
- compiler/codegen/Cargo.toml
- compiler/source/Cargo.toml
- vm/src/lib.rs
- compiler/source/src/lib.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style (cargo fmtto format)
Always run clippy to lint code (cargo clippy) before completing tasks. Fix any warnings or lints that are introduced by your changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
vm/src/vm/vm_new.rsvm/src/frame.rsvm/src/compiler.rsvm/src/builtins/code.rsvm/src/stdlib/ast/operator.rsvm/src/builtins/traceback.rsvm/src/stdlib/ast/node.rsvm/src/stdlib/ast/module.rsvm/src/stdlib/ast/argument.rsvm/src/stdlib/ast/type_ignore.rscompiler/src/lib.rsvm/src/stdlib/ast/basic.rsvm/src/stdlib/ast/string.rsvm/src/stdlib/ast/parameter.rsvm/src/stdlib/ast/elif_else_clause.rscompiler/codegen/src/symboltable.rscompiler/codegen/src/unparse.rsvm/src/stdlib/ast/statement.rsvm/src/stdlib/ast/type_parameters.rsvm/src/stdlib/ast/other.rsvm/src/stdlib/ast.rsvm/src/stdlib/ast/exception.rsvm/src/stdlib/ast/constant.rsvm/src/stdlib/ast/expression.rscompiler/codegen/src/compile.rsvm/src/stdlib/ast/pattern.rs
🧠 Learnings (17)
📓 Common learnings
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: In most cases, Python code should not be edited. Bug fixes should be made through Rust code modifications only
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to Lib/test/**/*.py : When tests fail due to unsupported syntax, keep the test as `@unittest.expectedFailure`, document that it requires PEP 695 support, and focus on tests that can be fixed through Rust code changes only
Cargo.toml (1)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
compiler/Cargo.toml (7)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: In most cases, Python code should not be edited. Bug fixes should be made through Rust code modifications only
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Generate documentation with cargo doc --no-deps --all
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to Lib/test/**/*.py : When tests fail due to unsupported syntax, keep the test as @unittest.expectedFailure, document that it requires PEP 695 support, and focus on tests that can be fixed through Rust code changes only
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to Lib/**/*.py : Minimize modifications to CPython standard library files in Lib/
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.py : Use ruff for linting Python code
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Always run clippy to lint code (cargo clippy) before completing tasks. Fix any warnings or lints that are introduced by your changes
vm/src/frame.rs (1)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
vm/Cargo.toml (5)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.py : Use ruff for linting Python code
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
Learnt from: moreal
PR: #5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.
Learnt from: moreal
PR: #5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to Lib/test/**/*.py : When tests fail due to unsupported syntax, keep the test as @unittest.expectedFailure, document that it requires PEP 695 support, and focus on tests that can be fixed through Rust code changes only
vm/src/compiler.rs (4)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: In most cases, Python code should not be edited. Bug fixes should be made through Rust code modifications only
Learnt from: moreal
PR: #5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.
Learnt from: moreal
PR: #5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.
vm/src/builtins/traceback.rs (1)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
vm/src/stdlib/ast/module.rs (1)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
compiler/src/lib.rs (4)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Follow the default rustfmt code style (cargo fmt to format)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Follow Rust best practices for error handling and memory management
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.py : Use ruff for linting Python code
compiler/codegen/src/symboltable.rs (2)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Follow Rust best practices for error handling and memory management
compiler/codegen/src/unparse.rs (2)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.py : Use ruff for linting Python code
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
vm/src/stdlib/ast/statement.rs (1)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
vm/src/stdlib/ast.rs (1)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
vm/src/stdlib/ast/exception.rs (1)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Follow Rust best practices for error handling and memory management
vm/src/stdlib/ast/constant.rs (1)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
compiler/codegen/src/compile.rs (1)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
vm/src/stdlib/ast/pattern.rs (1)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
🧬 Code Graph Analysis (10)
vm/src/stdlib/ast/module.rs (2)
vm/src/stdlib/ast/node.rs (10)
ast_to_object(5-5)ast_to_object(19-27)ast_to_object(39-41)ast_to_object(57-62)ast_to_object(81-83)ast_from_object(6-10)ast_from_object(29-35)ast_from_object(43-49)ast_from_object(64-75)ast_from_object(85-93)vm/src/stdlib/ast.rs (2)
node 8000 _add_location(208-231)get_node_field(54-57)
vm/src/stdlib/ast/string.rs (4)
vm/src/stdlib/ast/constant.rs (4)
ast_to_object(101-119)ast_to_object(140-167)ast_from_object(121-136)ast_from_object(169-244)vm/src/stdlib/ast/other.rs (11)
ast_to_object(7-9)ast_to_object(31-33)ast_to_object(48-50)ast_to_object(65-81)ast_to_object(104-127)ast_from_object(11-26)ast_from_object(35-44)ast_from_object(52-60)ast_from_object(83-99)ast_from_object(129-145)object(40-40)vm/src/stdlib/ast/expression.rs (18)
ast_to_object(11-52)ast_to_object(147-159)ast_to_object(184-200)ast_to_object(225-244)ast_to_object(274-286)ast_to_object(310-326)ast_to_object(351-370)ast_to_object(400-420)ast_to_object(451-461)ast_from_object(54-142)ast_from_object(161-179)ast_from_object(202-220)ast_from_object(246-269)ast_from_object(287-305)ast_from_object(328-346)ast_from_object(372-395)ast_from_object(422-446)ast_from_object(462-475)vm/src/stdlib/ast.rs (4)
node_add_location(208-231)get_node_field(54-57)range_from_object(161-184)get_node_field_opt(59-67)
vm/src/stdlib/ast/parameter.rs (2)
vm/src/stdlib/ast/node.rs (10)
ast_to_object(5-5)ast_to_object(19-27)ast_to_object(39-41)ast_to_object(57-62)ast_to_object(81-83)ast_from_object(6-10)ast_from_object(29-35)ast_from_object(43-49)ast_from_object(64-75)ast_from_object(85-93)vm/src/stdlib/ast.rs (4)
node_add_location(208-231)get_node_field(54-57)get_node_field_opt(59-67)range_from_object(161-184)
vm/src/stdlib/ast/elif_else_clause.rs (5)
vm/src/stdlib/ast/exception.rs (4)
ast_to_object(6-10)ast_to_object(36-58)ast_from_object(11-31)ast_from_object(60-79)vm/src/stdlib/ast/constant.rs (4)
ast_to_object(101-119)ast_to_object(140-167)ast_from_object(121-136)ast_from_object(169-244)vm/src/stdlib/ast/node.rs (4)
ast_to_object(5-5)ast_to_object(19-27)ast_from_object(6-10)ast_from_object(29-35)vm/src/stdlib/ast.rs (3)
node_add_location(208-231)get_node_field(54-57)range_from_object(161-184)vm/src/stdlib/ast/statement.rs (1)
get_node_field(1072-1075)
vm/src/stdlib/ast/type_parameters.rs (2)
vm/src/stdlib/ast/basic.rs (6)
ast_to_object(7-10)ast_to_object(23-25)ast_to_object(40-42)ast_from_object(12-19)ast_from_object(27-36)ast_from_object(44-50)vm/src/stdlib/ast.rs (4)
node_add_location(208-231)get_node_field(54-57)get_node_field_opt(59-67)range_from_object(161-184)
vm/src/stdlib/ast/other.rs (1)
vm/src/stdlib/ast.rs (4)
node_add_location(208-231)get_node_field(54-57)get_node_field_opt(59-67)range_from_object(161-184)
vm/src/stdlib/ast.rs (6)
compiler/src/lib.rs (1)
compile(90-120)compiler/codegen/src/compile.rs (3)
new(311-318)new(339-381)compile_top(152-166)vm/src/stdlib/ast/constant.rs (2)
ast_from_object(121-136)ast_from_object(169-244)vm/src/stdlib/ast/other.rs (6)
ast_from_object(11-26)ast_from_object(35-44)ast_from_object(52-60)ast_from_object(83-99)ast_from_object(129-145)object(40-40)vm/src/stdlib/ast/expression.rs (9)
ast_from_object(54-142)ast_from_object(161-179)ast_from_object(202-220)ast_from_object(246-269)ast_from_object(287-305)ast_from_object(328-346)ast_from_object(372-395)ast_from_object(422-446)ast_from_object(462-475)vm/src/stdlib/builtins.rs (1)
compile(117-226)
vm/src/stdlib/ast/exception.rs (1)
vm/src/stdlib/ast.rs (4)
node_add_location(208-231)get_node_field_opt(59-67)get_node_field(54-57)range_from_object(161-184)
compiler/codegen/src/compile.rs (2)
compiler/codegen/src/unparse.rs (2)
unparse_expr(63-453)unparse_expr(610-612)compiler/codegen/src/symboltable.rs (6)
scan_program(78-82)new(63-76)new(177-184)new(632-642)scan_expr(84-88)default(272-274)
vm/src/stdlib/ast/pattern.rs (2)
vm/src/stdlib/ast/node.rs (10)
ast_to_object(5-5)ast_to_object(19-27)ast_to_object(39-41)ast_to_object(57-62)ast_to_object(81-83)ast_from_object(6-10)ast_from_object(29-35)ast_from_object(43-49)ast_from_object(64-75)ast_from_object(85-93)vm/src/stdlib/ast.rs (4)
get_node_field(54-57)get_node_field_opt(59-67)node_add_location(208-231)range_from_object(161-184)
🔇 Additional comments (65)
compiler/Cargo.toml (1)
23-25: Good move to inherit lints from workspaceForwarding the lint configuration (
workspace = true) removes duplication and ensures compilation uses the central policy consistently. No issues spotted.vm/src/frame.rs (1)
24-24: LGTM - Import updated correctly for the source module refactor.The import change from
crate::source::SourceLocationtoruff_source_file::SourceLocationcorrectly implements the intended refactor to use the upstreamruff_source_filecrate instead of the customcompiler::sourcemodule.vm/src/builtins/code.rs (2)
18-18: LGTM - Import correctly updated to use upstream type.The import change from a custom
LineNumbertype toOneIndexedfromruff_source_filealigns with the PR's objective to use upstream source handling types.
359-359: LGTM - Constructor usage correctly updated.The change from
LineNumber::newtoOneIndexed::newcorrectly implements the migration to the upstream type while maintaining the same 1-indexed line number semantics.vm/src/compiler.rs (1)
3-3: LGTM - Correctly removes obsolete source re-export.The removal of the
rustpython_compiler_sourcere-export assourceis consistent with the PR's objective to eliminate the custom source module entirely in favor ofruff_source_file. Since thecompiler/sourcecrate has been removed, this re-export is no longer needed.vm/Cargo.toml (2)
23-23: LGTM: AST feature dependency update is correct.Removing
ruff_source_filefrom the "ast" feature dependencies makes sense since it's now a mandatory dependency rather than optional.
39-39: LGTM: Dependency change aligns with refactoring goals.Converting
ruff_source_filefrom an optional to mandatory dependency is correct for this refactoring, asSourceFileis now the unified source representation throughout the codebase.vm/src/stdlib/ast/elif_else_clause.rs (4)
2-2: LGTM: Import statement correctly updated.The import change from
rustpython-compiler-sourcetoruff_source_file::SourceFilealigns with the refactoring objectives.
4-9: LGTM: Function signature correctly updated.The parameter change from
source_code: &SourceCodeOwnedtosource_file: &SourceFileis consistent with the overall refactoring to useruff_source_filetypes.
13-38: LGTM: All method calls properly updated.All calls to
ast_to_object,ast_from_object, andnode_add_locationcorrectly use the newsource_fileparameter. The logic remains unchanged.
42-54: LGTM: ast_from_object function consistently updated.The function signature and all internal calls properly use
source_fileinstead ofsource_code, maintaining consistency with the refactoring.vm/src/stdlib/ast/basic.rs (4)
1-1: LGTM: Import correctly updated for the refactoring.The import change to
ruff_source_file::SourceFileis consistent with replacing the custom source handling.
6-20: LGTM: Identifier Node implementation properly updated.Both
ast_to_objectandast_from_objectmethods correctly use the newSourceFileparameter type. Using_source_fileindicates it's unused for this basic type, which is appropriate.
22-37: LGTM: Int Node implementation consistently updated.The method signatures are properly updated to use
SourceFile, maintaining consistency with the refactoring goals.
39-51: LGTM: Bool Node implementation correctly updated.Both trait methods properly use the new
SourceFileparameter type, completing the consistent update across all basic types.vm/src/stdlib/ast/node.rs (6)
2-2: LGTM: Import statement correctly updated.The import change to
ruff_source_file::SourceFilealigns with the refactoring to use the upstream source handling.
4-16: LGTM: Node trait definition properly updated.The trait methods correctly use
source_file: &SourceFileparameter, establishing the new interface for all AST node conversions throughout the system.
18-36: LGTM: Vec implementation consistently updated.Both
ast_to_objectandast_from_objectmethods properly use the newSourceFileparameter and correctly propagate it to the underlying node conversions.
38-54: LGTM: Box implementation correctly updated.The implementation properly delegates to the underlying type while maintaining the new parameter signature.
56-76: LGTM: Option implementation properly updated.The optional node handling correctly uses the new
SourceFileparameter for both conversion directions.
80-94: LGTM: BoxedSlice implementation consistently updated.The implementation correctly converts to/from Vec and properly uses the new parameter signature.
compiler/codegen/src/unparse.rs (4)
2-2: LGTM: Import statement correctly updated.The import change to
ruff_source_file::SourceFilealigns with the refactoring goals.
30-38: LGTM: Unparser struct and constructor properly updated.The struct field and constructor signature correctly use
SourceFileinstead of the previousSourceCodetype.
533-533: LGTM: Method call updated for new SourceFile API.The change from
get_range(val.range())toslice(val.range())reflects the different API ofSourceFilecompared to the previousSourceCodetype. This appears to be the correct method for extracting source text ranges.
605-612: LGTM: UnparseExpr struct and function properly updated.Both the struct field and the
unparse_exprfunction signature correctly useSourceFile, maintaining consistency with the refactoring.vm/src/stdlib/ast/operator.rs (1)
2-3: Migration looks correct — no functional or stylistic issues spottedAll
Nodeimpls were updated to accept&SourceFile, the unused parameter is correctly prefixed with an underscore to silence clippy, and no other logic changed.
Looks good.Also applies to: 6-20, 38-63, 101-119, 139-163
vm/src/builtins/traceback.rs (1)
6-18: 👍 Clean switch toOneIndexed; validation is explicitImporting
ruff_source_file::OneIndexedand storing it directly in the struct eliminates the previous alias and enforces 1-based invariants.py_newnow checkslineno > 0, so runtime safety is preserved.
No further action needed.vm/src/stdlib/ast/argument.rs (1)
10-13: Consistent use ofsource_filepropagated correctly
PositionalArgumentsandKeywordArgumentsnow forwardsource_fileto inner calls, ensuring location information isn’t lost after the refactor. No further issues detected.Also applies to: 34-38
vm/src/stdlib/ast/other.rs (1)
65-81:Aliasconversion updated correctly – thanks for addingnode_add_locationConversion now passes
source_fileto every nested node and retains range info; implementation looks solid.vm/src/stdlib/ast/module.rs (2)
28-35: Top-level enum delegates updated – no issuesEach variant forwards the new
source_fileparameter; matches trait changes elsewhere.
65-90:ModModule::ast_to_objectpreserves location correctly after refactor
node_add_locationnow receivessource_file, ensuring correct line/column mapping. Implementation is spot-on.vm/src/stdlib/ast/statement.rs (1)
1-1237: LGTM! Systematic parameter type refactor completed correctly.This refactor successfully replaces
SourceCodeOwnedwithSourceFileacross all AST statement node implementations. The changes are:
- Consistent parameter type updates in all
Nodetrait methods- Proper import addition for
ruff_source_file::SourceFile- Systematic variable name updates from
source_codetosource_file- All helper function calls (
node_add_location,range_from_object) correctly updatedThe refactor maintains all existing logic and error handling while unifying the source representation type across the codebase.
vm/src/stdlib/ast/type_ignore.rs (1)
1-75: LGTM! Consistent parameter type refactor.The changes correctly update all
Nodetrait implementations to useSourceFileinstead ofSourceCodeOwned, maintaining consistency with the broader codebase refactor. All method signatures and helper function calls are properly updated.vm/src/stdlib/ast/type_parameters.rs (1)
1-206: LGTM! Type parameter nodes consistently refactored.All type parameter AST node implementations have been properly updated to use
SourceFileinstead ofSourceCodeOwned. The refactor is systematic and maintains all existing functionality while aligning with the broader source representation unification.vm/src/stdlib/ast/parameter.rs (1)
1-406: LGTM! Parameter handling consistently refactored.The refactor successfully updates all parameter-related AST nodes and helper structures to use
SourceFile. The complex parameter default extraction and merging logic is preserved while the source representation type is updated throughout. All method signatures and internal calls are properly updated.vm/src/stdlib/ast/exception.rs (1)
1-81: LGTM! Exception handler nodes consistently refactored.The exception handler AST node implementations are properly updated to use
SourceFileinstead ofSourceCodeOwned. All method signatures, helper function calls, and error handling are correctly maintained while aligning with the source representation refactor.vm/src/stdlib/ast/string.rs (1)
216-353: LGTM: Systematic parameter type migration executed correctly.The migration from
SourceCodeOwnedtoSourceFilehas been implemented consistently across allNodetrait implementations:
- All method signatures properly updated from
source_code: &SourceCodeOwnedtosource_file: &SourceFile- All internal calls to helper functions (
node_add_location,range_from_object,ast_to_object,ast_from_object) correctly updated to usesource_file- Logic and control flow remain unchanged, maintaining correctness
This change aligns with the broader refactor to replace the custom source representation with
ruff_source_file::SourceFile.vm/src/stdlib/ast/expression.rs (3)
2-7: Import organization and SourceFile integration look good.The import changes properly support the migration:
- Added
use ruff_source_file::SourceFile;for the new parameter type- Reorganized related imports for better grouping and clarity
11-142: Comprehensive parameter migration in main expression dispatch.The central
ruff::Exprimplementation has been systematically updated:
- All
ast_to_objectcalls correctly passsource_fileinstead ofsource_code- The
ast_from_objectmethod signature properly updated to use&SourceFile- All recursive calls to individual expression types updated consistently
- Logic flow and match patterns remain unchanged, preserving correctness
146-1238: All expression node implementations consistently updated.Every individual expression node type (
ExprBoolOp,ExprNamed,ExprBinOp, etc.) has been properly migrated:
- Method signatures updated from
source_code: &SourceCodeOwnedtosource_file: &SourceFile- All internal calls to helper functions (
node_add_location,range_from_object,get_node_field, etc.) correctly usesource_file- Recursive calls to nested AST nodes properly pass the
source_fileparameter- No changes to the underlying logic, maintaining semantic correctness
This comprehensive update ensures all expression types work with the new
SourceFilerepresentation.compiler/codegen/src/symboltable.rs (8)
21-21: Import update looks correct.The import change from the previous source types to
SourceFileandSourceLocationfromruff_source_fileis appropriate and necessary for the migration.
78-88: Function signature updates are correct.The changes to
scan_programandscan_exprto acceptSourceFileinstead of the previous source type are consistent with the migration and properly pass the parameter toSymbolTableBuilder::new.
90-93: Method relocation is appropriate.Moving the
lookupmethod into the mainimpl SymbolTableblock is a good organizational change that doesn't affect functionality.
608-616: Struct refactoring improves design.The removal of the lifetime parameter from
SymbolTableBuilderand storingSourceFile< 8000 /code> directly is an excellent improvement. This simplifies the type signature and is efficient sinceSourceFileusesArcinternally, making it cheap to clone.Also applies to: 631-642
699-703: Method adaptation is correct.The call to
source_file.to_source_code().line_index()properly adapts to the newSourceFileAPI while maintaining compatibility with existing line index logic.
1050-1053: Error location handling updated correctly.The adaptation to use
source_file.to_source_code().source_location()for error reporting maintains proper location information in the new API.
1290-1290: Error location mapping is correct.The change to use
source_file.to_source_code().source_location()for named expression error reporting is appropriate.
1570-1573: Source location extraction updated properly.The adaptation to extract source location using
source_file.to_source_code().source_location()in theregister_namemethod maintains correct location tracking.compiler/src/lib.rs (5)
1-1: Import update is correct.The import of
SourceFile,SourceFileBuilder, andSourceLocationfromruff_source_fileproperly replaces the previous source handling types.
46-56: Error construction properly updated.The
from_ruff_parse_errormethod correctly adapts to useSourceFilemethods (to_source_code().source_location()andname()) for error location and path information.
99-104: Source file creation is appropriate.The use of
SourceFileBuilder::new(source_path, source).finish()to create aSourceFileinstance is the correct pattern for the new API.
122-138: Internal compile function updated correctly.The function signature change to accept
SourceFileand the usage ofsource_file.source_text()for parsing are consistent with the migration to the new source representation.
145-172: Symbol table compilation functions updated properly.Both
compile_symtableand_compile_symtableare correctly updated to useSourceFilecreation and API calls. The error conversion at the end properly usessource_file.name()for the error context.vm/src/stdlib/ast.rs (7)
24-24: Import update is comprehensive and correct.The import of
LineIndex,OneIndexed,SourceFile,SourceFileBuilder, andSourceLocationfromruff_source_fileproperly replaces the previous source handling imports and includes all necessary types.
127-159: Source range conversion properly updated.The
text_range_to_source_rangefunction correctly adapts to useSourceFilemethods (clone().source_text()andsource_text()) to access the source content and maintain line indexing functionality.
161-184: Range parsing function updated correctly.The
range_from_objectfunction properly accepts a&SourceFileparameter and passes it tosource_range_to_text_range, maintaining the API consistency.
186-206: Source-to-text range conversion updated appropriately.The
source_range_to_text_rangefunction correctly uses the newSourceFileAPI withclone().source_text()andsource_text()calls for line indexing operations.
208-231: Location addition function updated correctly.The
node_add_locationfunction properly accepts a&SourceFileparameter and uses it withtext_range_to_source_rangeto maintain location information in AST nodes.
234-255: Parse function migration is correct.The
parsefunction properly creates aSourceFileusingSourceFileBuilderand uses it throughout the parsing pipeline, including error handling and AST object creation.
258-286: Compile function updated appropriately.The
compilefunction correctly createsSourceFileinstances usingSourceFileBuilderfor both the initial parsing and the final compilation steps, maintaining the proper API usage throughout.vm/src/stdlib/ast/constant.rs (3)
4-4: LGTM: Import statement correctly addedThe import of
ruff_source_file::SourceFileis properly added to support the type migration.
101-119: LGTM: Node trait implementations consistently updatedAll
Nodetrait implementations forConstantandConstantLiteralhave been systematically updated to:
- Replace
source_code: &SourceCodeOwnedparameters withsource_file: &SourceFile- Update all internal method calls to use the new parameter
- Maintain identical functionality and logic
The refactoring is complete and consistent across all implementations.
Also applies to: 123-136, 140-167, 171-244
333-398: LGTM: Helper functions consistently updatedAll helper functions (
number_literal_to_object,string_literal_to_object,bytes_literal_to_object,boolean_literal_to_object,none_literal_to_object,ellipsis_literal_to_object) have been systematically updated with the newsource_file: &SourceFileparameter type and all calls are properly updated.vm/src/stdlib/ast/pattern.rs (2)
2-2: LGTM: Import statement correctly addedThe import of
ruff_source_file::SourceFileis properly added to support the type migration.
6-48: LGTM: Comprehensive Node trait implementation updatesAll
Nodetrait implementations for pattern-related AST types have been systematically updated to:
- Replace
source_code: &SourceCodeOwnedparameters withsource_file: &SourceFile- Update all internal method calls (
ast_to_object,ast_from_object,node_add_location,range_from_object) to use the new parameter- Maintain identical functionality and control flow
The refactoring covers all pattern matching AST node types including:
ruff::MatchCaseruff::Patternand its variantsruff::PatternMatchValue,ruff::PatternMatchSingleton, etc.- Helper structs with placeholder implementations
The migration is complete and consistent throughout the file.
Also applies to: 52-125, 128-157, 161-193, 196-207, 211-243, 247-293, 297-360, 367-378, 385-396, 403-414, 417-444, 448-481, 485-513
| impl Node for ruff::Decorator { | ||
| fn ast_to_object(self, vm: &VirtualMachine, source_code: &SourceCodeOwned) -> PyObjectRef { | ||
| ruff::Expr::ast_to_object(self.expression, vm, source_code) | ||
| fn ast_to_object(self, vm: &VirtualMachine, source_file: &SourceFile) -> PyObjectRef { | ||
| ruff::Expr::ast_to_object(self.expression, vm, source_file) | ||
| } | ||
|
|
||
| fn ast_from_object( | ||
| vm: &VirtualMachine, | ||
| source_code: &SourceCodeOwned, | ||
| source_file: &SourceFile, | ||
| object: PyObjectRef, | ||
| ) -> PyResult<Self> { | ||
| let expression = ruff::Expr::ast_from_object(vm, source_code, object)?; | ||
| let expression = ruff::Expr::ast_from_object(vm, source_file, object)?; | ||
| let range = expression.range(); | ||
| Ok(Self { expression, range }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Decorator nodes still drop location info – easy to regain
ruff::Decorator::ast_to_object forwards only the inner Expr, so the resulting Python ast.Decorator lacks lineno/col_offset data.
Adding the usual dict construction restores parity with other node kinds:
-fn ast_to_object(self, vm: &VirtualMachine, source_file: &SourceFile) -> PyObjectRef {
- ruff::Expr::ast_to_object(self.expression, vm, source_file)
+fn ast_to_object(self, vm: &VirtualMachine, source_file: &SourceFile) -> PyObjectRef {
+ let range = self.range();
+ let node = NodeAst
+ .into_ref_with_type(vm, pyast::NodeDecorator::static_type().to_owned())
+ .unwrap();
+ let dict = node.as_object().dict().unwrap();
+ dict.set_item(
+ "expression",
+ self.expression.ast_to_object(vm, source_file),
+ vm,
+ )
+ .unwrap();
+ node_add_location(&dict, range, vm, source_file);
+ node.into()
}Without this, round-tripping ASTs will silently lose position metadata for decorators.
📝 Committable suggestion
‼️ 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.
| impl Node for ruff::Decorator { | |
| fn ast_to_object(self, vm: &VirtualMachine, source_code: &SourceCodeOwned) -> PyObjectRef { | |
| ruff::Expr::ast_to_object(self.expression, vm, source_code) | |
| fn ast_to_object(self, vm: &VirtualMachine, source_file: &SourceFile) -> PyObjectRef { | |
| ruff::Expr::ast_to_object(self.expression, vm, source_file) | |
| } | |
| fn ast_from_object( | |
| vm: &VirtualMachine, | |
| source_code: &SourceCodeOwned, | |
| source_file: &SourceFile, | |
| object: PyObjectRef, | |
| ) -> PyResult<Self> { | |
| let expression = ruff::Expr::ast_from_object(vm, source_code, object)?; | |
| let expression = ruff::Expr::ast_from_object(vm, source_file, object)?; | |
| let range = expression.range(); | |
| Ok(Self { expression, range }) | |
| } | |
| impl Node for ruff::Decorator { | |
| fn ast_to_object(self, vm: &VirtualMachine, source_file: &SourceFile) -> PyObjectRef { | |
| let range = self.range(); | |
| let node = NodeAst | |
| .into_ref_with_type(vm, pyast::NodeDecorator::static_type().to_owned()) | |
| .unwrap(); | |
| let dict = node.as_object().dict().unwrap(); | |
| dict.set_item( | |
| "expression", | |
| self.expression.ast_to_object(vm, source_file), | |
| vm, | |
| ) | |
| .unwrap(); | |
| node_add_location(&dict, range, vm, source_file); | |
| node.into() | |
| } | |
| fn ast_from_object( | |
| vm: &VirtualMachine, | |
| source_file: &SourceFile, | |
| object: PyObjectRef, | |
| ) -> PyResult<Self> { | |
| let expression = ruff::Expr::ast_from_object(vm, source_file, object)?; | |
| let range = expression.range(); | |
| Ok(Self { expression, range }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In vm/src/stdlib/ast/other.rs around lines 47 to 60, the ast_to_object
implementation for ruff::Decorator only forwards the inner expression, causing
the resulting Python ast.Decorator to lose lineno and col_offset location
metadata. To fix this, modify ast_to_object to construct a Python dict that
includes the expression object plus the decorator's location info (lineno,
col_offset) so the Python AST node retains full position data consistent with
other node types.
There was a problem hiding this comment.
Thanks! This is a lot better. Maybe we'd better to check if those ruff types are exported through public API or not, and add some re-export later though.
Could you check the removed crate to let us to publish a deprecated marked version?
| [package] | ||
| name = "rustpython-compiler-source" | ||
| description = "RustPython Source and Index" | ||
| version.workspace = true |
There was a problem hiding this comment.
we have to publish a deprecated version of this crate before removeing it from source code.
| version.workspace = true | |
| version = "0.5.0+deprecated" |
There was a problem hiding this comment.
ofc, didn't realized that it was published, thought it was completely internal
Ruff don't release it's crates to https://cates.io so I'm not sure what "public API" you're referring to. Or do you mean our public API?
ofc, just did |
|
Public API means RustPython API. It will be confusing if RustPython APIs are directly using ruff's types or functions. |
Completely agree, want me to open a PR reexporting |
|
probably |
I saw that
compiler/sourcewas mainly re-exporting items fromruff_source_file, so I replaced it with the upstreamruff_source_file::SourceFilewhich is a bit better implementation of what we have currently as it's usesArcinternally so it's practically free to clone & replaces the need for a dedicatedSourceCodeOwnedasSourceFileis already owned.This PR contains a lot of noise as I renamed:
source_code->source_fileSourceCode->SourceFileSourceCodeOwned->SourceFileFeel free to close this PR if this is an unwanted change.
Summary by CodeRabbit
Refactor
Chores