Fix traceback #6569
Fix traceback #6569
Conversation
📝 WalkthroughWalkthroughThis PR changes per-instruction source tracking from a single SourceLocation to start/end SourceLocation pairs across codegen, IR, bytecode, marshal, VM construction, and traceback; it also adds a CompileOpts.debug_ranges flag and propagates it to linetable generation and VM settings. Changes
Sequence Diagram(s)(No sequence diagram generated — changes are data-shape and API propagation focused, not a new multi-component control flow requiring sequential visualization.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
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 |
c3ed088 to
a8ab1b4
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 (1)
crates/compiler-core/src/marshal.rs (1)
7-7: Consider incrementing FORMAT_VERSION for breaking marshal format change.The locations serialization format changed from 2
u32values per entry (line, character_offset) to 4u32values (start line, start char, end line, end char). This is a breaking change that will cause deserialization failures when loading old marshaled code objects. Consider incrementingFORMAT_VERSIONfrom 4 to 5 to prevent incompatible bytecode from being loaded.🔎 Suggested fix
-pub const FORMAT_VERSION: u32 = 4; +pub const FORMAT_VERSION: u32 = 5;Also applies to: 656-661
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Lib/test/test_code.pyis excluded by!Lib/**Lib/test/test_traceback.pyis excluded by!Lib/**Lib/test/test_zipimport.pyis excluded by!Lib/**Lib/traceback.pyis excluded by!Lib/**
📒 Files selected for processing (7)
crates/codegen/src/compile.rscrates/codegen/src/ir.rscrates/compiler-core/src/bytecode.rscrates/compiler-core/src/marshal.rscrates/vm/src/builtins/code.rscrates/vm/src/builtins/frame.rscrates/vm/src/frame.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by 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:
crates/vm/src/builtins/frame.rscrates/vm/src/frame.rscrates/compiler-core/src/marshal.rscrates/compiler-core/src/bytecode.rscrates/vm/src/builtins/code.rscrates/codegen/src/compile.rscrates/codegen/src/ir.rs
🧬 Code graph analysis (1)
crates/codegen/src/compile.rs (1)
crates/compiler/src/lib.rs (1)
location(58-63)
🔇 Additional comments (10)
crates/vm/src/builtins/frame.rs (1)
60-62: LGTM - f_lasti byte offset conversion is correct.The change to return
self.lasti() * 2correctly converts the instruction index to a byte offset, matching CPython'sf_lastibehavior where each instruction word is 2 bytes.crates/compiler-core/src/bytecode.rs (2)
258-261: Core type change for location pairs looks correct.The change from
Box<[SourceLocation]>toBox<[(SourceLocation, SourceLocation)]>enables tracking both start and end locations per instruction, which is necessary for pytest'send_linenorequirement.
1485-1492: LGTM - Display logic correctly uses start location.The display functions appropriately access
.0.lineto use the start location for line number display, maintaining backward-compatible output.crates/vm/src/builtins/code.rs (1)
469-484: Default locations use identical start/end - reasonable fallback.When constructing a code object from
PyCodeNewArgs, end location info isn't available, so creating(loc, loc)pairs where both start and end are identical is a sensible default. This maintains structural consistency while avoiding fabricated end location data.crates/compiler-core/src/marshal.rs (1)
191-203: LGTM - Deserialization correctly handles location pairs.The deserialization logic properly reads four
u32values per location entry to construct the(start, end)SourceLocationpair, maintaining consistency with the new data structure.crates/vm/src/frame.rs (1)
387-394: The end location is captured but not exposed in PyTraceback.The destructuring pattern
(loc, _end_loc)correctly suppresses the unused variable warning (following Rust idiom), but if the PR objective is to provide pytest'send_linenosupport in tracebacks, thePyTracebackstruct would need anend_linenofield added to its definition and the field must be passed during construction.crates/codegen/src/compile.rs (2)
1488-1505: Raise now uses the statement range for its source span; this is consistent and precise.Binding
rangefromStmtRaiseand callingself.set_source_range(*range)right before emittingInstruction::Raiseensures the raise instruction’s start/end locations reflect the wholeraisestatement, not just the last compiled subexpression. This aligns with how other statements useset_source_rangeand should improve traceback accuracy without altering semantics.
5643-5653: End locations are correctly threaded intoInstructionInfoin_emit.Using the current
TextRangeto compute bothlocationandend_location, then pushing them intoInstructionInfo, cleanly enables per-instruction start/end tracking. Repeating the same pair for each code unit (including anyEXTENDED_ARGs) keepsinstructionsandlocationsin lockstep and matches how the IR is later linearized.crates/codegen/src/ir.rs (2)
416-517: Linetable generation correctly incorporates end locations; please keep it in sync with CPython’s encoding.
generate_linetablenow takes(start,end)locations, computesline_deltaandend_line_delta = end_line - line, and chooses between short, one-line, and long forms while encoding columns as varints ofcol+1/end_col+1. This matches the expected 3.11+ location table shape and should allow accurateend_lineno/end_colnoreconstruction as long as callers maintain the invariantend_line >= lineand monotonic character offsets from theTextRange. The control flow (chunking up to 8 instructions, grouping identical locations, and using signed deltas) looks consistent with CPython’s scheme.
84-91: InstructionInfo now carries end locations and locations are stored as (start,end) pairs; wiring looks sound.Adding
end_location: SourceLocationtoInstructionInfoand switchinglocationsto aVec<(SourceLocation, SourceLocation)>that is populated vialocations.extend(repeat_n((info.location, info.end_location), arg.instr_size()))keeps the per-bytecode-unit location data aligned withinstructionsand matches the newCodeObject.locations/generate_linetableexpectations. This gives downstream traceback/FrameSummary code bothlinenoandend_linenowithout changing control-flow semantics. All InstructionInfo construction sites correctly initialize end_location, and the linetable generation properly encodes both line deltas and end-line deltas.
a8ab1b4 to
095a0cc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/vm/src/frame.rs (1)
174-176: Still need to guardcurrent_location()againstlasti() == 0(panic risk) and confirm new traceback offset semantics
Frame::current_location()still does:pub fn current_location(&self) -> SourceLocation { self.code.locations[self.lasti() as usize - 1].0 }If
lasti()is 0 (e.g., on a freshly-created or not-yet-started frame),self.lasti() as usize - 1underflows tousize::MAX, causing an out-of-bounds index and panic. This was already raised in a previous review and remains unfixed with the new(start, end)layout. A minimal fix is to special-caselasti() == 0and fall back to a safe location (e.g., the first entry or one synthesized fromfirst_line_number) before indexing:Suggested guard for `current_location()`
- pub fn current_location(&self) -> SourceLocation { - self.code.locations[self.lasti() as usize - 1].0 - } + pub fn current_location(&self) -> SourceLocation { + let lasti = self.lasti(); + if lasti == 0 { + // Frame has not executed any instruction yet: use the first known + // location if present, or synthesize one from first_line_number. + if let Some((loc, _)) = self.code.locations.first() { + return *loc; + } + return SourceLocation { + line: self + .code + .first_line_number + .unwrap_or(rustpython_compiler_core::OneIndexed::MIN), + character_offset: rustpython_compiler_core::OneIndexed::from_zero_indexed(0), + }; + } + self.code.locations[lasti as usize - 1].0 + }Also, in
handle_exceptionyou now compute:let (loc, _end_loc) = frame.code.locations[idx]; let new_traceback = PyTraceback::new( next, frame.object.to_owned(), frame.lasti() * 2, loc.line, );Using
frame.lasti() * 2as the offset should match “codeunit-bytes” addressing, but it’s tightly coupled to howtb_lastiandco_positionsinterpret offsets. Please make sure this lines up with the linetable decoding by running the traceback/pytest scenarios that motivated this PR.Also applies to: 388-395
🧹 Nitpick comments (3)
src/settings.rs (1)
263-291: CLI/env plumbing forcode_debug_rangesis correct; consider documenting the new -X
-X no_debug_rangesandPYTHONNODEBUGRANGESboth correctly togglesettings.code_debug_rangesvia the existingimplementation_option/env_boolplumbing and respect-E/-I. As a follow-up, you might want to extend--help-xoptions/ docs to mentionno_debug_rangesandPYTHONNODEBUGRANGES, but the runtime behavior itself looks fine.Also applies to: 294-300
crates/vm/src/builtins/code.rs (1)
470-486: Locations initialization as(loc, loc)is acceptable given existing FIXMEFor
PyCode::py_new, seedinglocationsas(loc, loc)for every instruction keeps prior behavior (single-line fallback based onfirstlineno) while satisfying the new(start, end)shape. Inreplace, reusingself.code.locations.clone()is consistent with the existing “locations duplicate linetable” FIXME; the semantics aren’t worsened by the tuple change. Once you eventually switch everything to linetable-only, this field can indeed be retired.Also applies to: 968-983
crates/codegen/src/compile.rs (1)
5653-5665: Core range tracking in_emitis correct; behavior now hinges on callers settingcurrent_source_rangeComputing both
locationandend_locationfromcurrent_source_range.start()/.end()and recording them intoInstructionInfois the key step that enables per-instruction ranges throughout the pipeline. Given that:
compile_statementsets the range for each statement,compile_expressionsets the range for each expression, and- specialized sites (e.g.
compile_with,Stmt::Raise) now override the range for their control/raise instructions,the resulting start/end spans should be coherent. Just be aware that any future emission path that forgets to set
current_source_rangefirst will now produce misleading locations on both start and end, so it’s worth treatingset_source_rangeas part of the “contract” for any new emission helpers.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Lib/test/test_code.pyis excluded by!Lib/**Lib/test/test_traceback.pyis excluded by!Lib/**Lib/test/test_zipimport.pyis excluded by!Lib/**Lib/traceback.pyis excluded by!Lib/**
📒 Files selected for processing (10)
crates/codegen/src/compile.rscrates/codegen/src/ir.rscrates/compiler-core/src/bytecode.rscrates/compiler-core/src/marshal.rscrates/vm/src/builtins/code.rscrates/vm/src/builtins/frame.rscrates/vm/src/frame.rscrates/vm/src/vm/mod.rscrates/vm/src/vm/setting.rssrc/settings.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/vm/src/builtins/frame.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by 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:
crates/vm/src/vm/setting.rscrates/vm/src/frame.rscrates/compiler-core/src/marshal.rscrates/compiler-core/src/bytecode.rscrates/codegen/src/compile.rscrates/vm/src/builtins/code.rssrc/settings.rscrates/vm/src/vm/mod.rscrates/codegen/src/ir.rs
🧬 Code graph analysis (2)
crates/vm/src/builtins/code.rs (1)
crates/compiler-core/src/marshal.rs (4)
len(191-203)len(226-228)len(233-235)len(239-244)
src/settings.rs (1)
src/interpreter.rs (1)
settings(59-62)
⏰ 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). (9)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check Rust code with clippy
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run tests under miri
- GitHub Check: Ensure compilation on various targets
🔇 Additional comments (8)
crates/compiler-core/src/bytecode.rs (1)
259-262: Start/end location tuple wiring looks consistent with existing usage
CodeObject.locationsnow stores(start, end)and all callers here consistently use the start half (.0) for line display and copy the whole slice throughmap_bag/map_clone_bag, preserving existing semantics while enabling richer ranges elsewhere. This keeps the previous invariant thatlocations.len()matchesinstructions.len(); just make sure downstream consumers relying on the publiclocationsfield are updated to the new tuple shape.Also applies to: 1485-1494, 1562-1564, 1594-1596
crates/vm/src/vm/setting.rs (1)
60-62: Newcode_debug_rangessetting is well-integratedThe
code_debug_rangesflag is documented, exposed onSettings, and defaulted totrueinDefault, which matches the intended-X no_debug_rangessemantics.Also applies to: 170-207
crates/vm/src/vm/mod.rs (1)
505-513:debug_rangespropagated cleanly intoCompileOptsLinking
CompileOpts.debug_rangesdirectly toself.state.config.settings.code_debug_rangesis the right place to surface the new setting into the compiler.crates/codegen/src/compile.rs (4)
118-125:CompileOptsextension is reasonable and default behavior matches new flag semanticsThe addition of
debug_ranges: booltoCompileOptsplus the explicitDefaultimpl (optimizations off, debug ranges on) cleanly exposes the new knob without breaking existing call sites that rely onDefault::default(). This aligns with the CLI/env design where column/range info is enabled by default and disabled via a dedicated flag.Also applies to: 127-134
1500-1503: Good:raisenow uses the full statement range for its traceback locationDestructuring
Stmt::Raiseto getrangeand then callingself.set_source_range(*range)immediately before emittingInstruction::Raiseensures the raise instruction’s start/end positions reflect the wholeraise ... from ...statement, not just the last expression that happened to be compiled. That’s exactly what you want for accurate traceback ranges andend_linenocomputation.Also applies to: 1516-1518
3231-3233:with-statement source range handling is improved and consistentCapturing
with_range = self.current_source_rangeat the start ofcompile_withand reapplying it:
- before
SetupWith/SetupAsyncWith,- before binding
optional_vars,- before recursive nested
compile_withcalls, and- for the cleanup (
PopBlock/EnterFinally/WithCleanup*),makes the control-flow scaffolding for a
withconsistently point back to the overallwithstatement span, while still letting inner expressions use their own ranges. This should yield saner linetable entries and tracebacks around multi-contextwithblocks.Also applies to: 3241-3242, 3259-3260, 3276-3277, 3282-3285
870-874:exit_scope→finalize_code(&self.opts)wiring is correct and consistentThere is only one call site to
finalize_codein the codebase (at this location), and it correctly passes&self.optsto match the function signature. Optimization and debug-range behavior are properly tied to the compiler's options.crates/codegen/src/ir.rs (1)
84-91: Per-instruction ranges and linetable encoding are sound; verification confirms both concerns are addressedAdding
end_locationtoInstructionInfoand wiring it through_emitintolocationsprovides the correct data model forend_lineno/ range-based tracebacks. Thefinalize_codeAPI change to accept&CompileOptsand useopts.optimize/opts.debug_rangesis clean and keeps knobs centralized.The
generate_linetableimplementation correctly follows CPython's 3.11+ scheme (NoColumns vs short/one-line/long encodings, chunking into up to 8 instructions, relative line deltas), and using(start, end)pairs gives the VM enough information for both line-only and full-range modes.Both verification points pass:
core::iter::repeat_nis stable as of Rust 1.82.0; your MSRV is 1.89.0, so it's available.- The encoding safely assumes
end_location≥locationwith explicit checks (e.g.,end_col >= colin short-form selection) and proper varint-based overflow handling in the long form, making reversed or zero-width ranges impossible.
| let len = rdr.read_u32()?; | ||
| let locations = (0..len) | ||
| .map(|_| { | ||
| Ok(SourceLocation { | ||
| let start = SourceLocation { | ||
| line: OneIndexed::new(rdr.read_u32()? as _).ok_or(MarshalError::InvalidLocation)?, | ||
| character_offset: OneIndexed::from_zero_indexed(rdr.read_u32()? as _), | ||
| }) | ||
| }; | ||
| let end = SourceLocation { | ||
| line: OneIndexed::new(rdr.read_u32()? as _).ok_or(MarshalError::InvalidLocation)?, | ||
| character_offset: OneIndexed::from_zero_indexed(rdr.read_u32()? as _), | ||
| }; | ||
| Ok((start, end)) | ||
| }) | ||
| .collect::<Result<Box<[SourceLocation]>>>()?; | ||
| .collect::<Result<Box<[(SourceLocation, SourceLocation)]>>>()?; | ||
|
|
There was a problem hiding this comment.
Marshal format for locations changed without bumping FORMAT_VERSION
The code at lines 190-204 (deserialize) and 649-661 (serialize) is now symmetric and correctly treats each location entry as a (start, end) pair, reading and writing four u32 values per entry (start line, start column, end line, end column). However, FORMAT_VERSION remains at 4 with no version-based conditional logic.
If this represents a format change from prior versions (where locations might have had a different layout), then any marshalled code produced with earlier versions of FORMAT_VERSION 4 will be incompatible with the current reader—and vice versa. Existing frozen stdlib artifacts, cached bytecode, or external tools relying on the old layout would fail to deserialize.
To address this:
- Bump
FORMAT_VERSIONand guard the(start, end)tuple handling on the new version, or - Add backward-compat deserialization that can handle both old and new layouts
- Document whether marshal format 4 is intended to be stable across this change
Otherwise, if locations always had this layout, this concern does not apply.
🤖 Prompt for AI Agents
In crates/compiler-core/src/marshal.rs around lines 190-204 (reader) and 649-661
(writer), the marshal layout for `locations` was changed to a `(start, end)`
tuple of four u32s but FORMAT_VERSION was not bumped or handled; either bump
FORMAT_VERSION and gate the new read/write behavior on the new version, or add
backward-compatible logic: detect the FORMAT_VERSION during deserialization and
branch to the old layout reader when the version is older, and when serializing
emit the format corresponding to the chosen FORMAT_VERSION; update the serialize
code at 649-661 to be conditional on version, update any tests/fixtures and the
format documentation, and ensure unit tests cover both old and new
deserialization paths.
095a0cc to
4103350
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/compiler-core/src/marshal.rs (1)
190-204: Marshal layout forlocationschanged, but FORMAT_VERSION stayed at 4Deserialization now reads
(start, end)as a pair ofSourceLocations (four u32s per entry) andserialize_codewrites the same shape, so the new reader/writer are symmetric. However,FORMAT_VERSIONis still4, with no version gating or backward-compat branch for older artifacts that encoded only a single location (two u32s) per entry.If any existing
.pyc/marshal payloads or frozen stdlib artifacts were produced with the old layout under version 4, they will now be incompatible with this reader, and newly written artifacts will be unreadable by older RustPython builds that also expect version 4.You likely want to either:
- Bump
FORMAT_VERSIONand tie the(start, end)shape to the new version, or- Explicitly state that marshal format 4 was never stable and all artifacts must be regenerated.
Also applies to: 655-661
🧹 Nitpick comments (3)
crates/codegen/src/compile.rs (1)
5655-5665: end_location tracking in _emit is consistent, but note potential cost of repeated to_source_code()Recording both
locationandend_locationper instruction fromcurrent_source_rangeis the right shape for range-aware tracebacks and aligns with the new IR/CodeObject layout. The implementation assumescurrent_source_rangeis always set before emitting, which holds given all uses ofset_source_range. If you ever seeto_source_code()show up hot in profiles, consider caching theSourceCodeforsource_filein theCompilerrather than recomputing it here.crates/codegen/src/ir.rs (2)
178-222: Location vector construction looks correct; double‑check MSRV forcore::iter::repeat_nUsing
block_to_offsetplusinstr.arg.instr_size()to reserve and populate bothinstructionsandlocations, and repeating(info.location, info.end_location)once per code unit, keepslocations.len()in sync with the encoded bytecode length across extended-arg recompiles.The only thing to confirm is that your MSRV includes
core::iter::repeat_n; if not, this can be trivially replaced withstd::iter::repeat((info.location, info.end_location)).take(arg.instr_size() as usize)or a small loop.
423-541: Linetable generation logic is reasonable; verify NoColumns path vs CPython’s no_debug_ranges behaviorThe new
generate_linetablecorrectly:
- Groups consecutive instructions with identical
(start,end)locations,- Limits each entry to a run of at most 8 instructions,
- Computes
line_deltarelative tofirst_line/prev_lineandend_line_deltafor multi-line spans, and- Chooses between
Short*/OneLine*/Longencodings with appropriate signed/unsigned varint encodings for deltas and columns whendebug_rangesis enabled.When
debug_rangesisfalse, the function always emitsPyCodeLocationInfoKind::NoColumnsentries and writes only a signedline_deltavarint, ignoringend_lineand all column data. That matches the intent of “no column info”, but it’s worth double-checking against CPython’s 3.11+ encoding to ensure that disabling debug ranges is supposed to drop end-line information entirely rather than just column ranges.I’d recommend:
- Verifying this against CPython’s
_PyLineTableWriter/PyCode_Addr2Locationbehavior for-X no_debug_ranges.- Adding or updating tests that round-trip a few multi-line constructs with
debug_rangeson and off (e.g., disassembly snapshot tests or addr2location tests) to confirm compatibility.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Lib/test/test_code.pyis excluded by!Lib/**Lib/test/test_traceback.pyis excluded by!Lib/**Lib/test/test_zipimport.pyis excluded by!Lib/**Lib/traceback.pyis excluded by!Lib/**
📒 Files selected for processing (12)
.cspell.dict/python-more.txtcrates/codegen/src/compile.rscrates/codegen/src/ir.rscrates/compiler-core/src/bytecode.rscrates/compiler-core/src/marshal.rscrates/vm/src/builtins/code.rscrates/vm/src/builtins/frame.rscrates/vm/src/frame.rscrates/vm/src/vm/mod.rscrates/vm/src/vm/setting.rsexamples/dis.rssrc/settings.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/settings.rs
- crates/vm/src/vm/mod.rs
- crates/compiler-core/src/bytecode.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by 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:
crates/vm/src/vm/setting.rscrates/codegen/src/ir.rscrates/vm/src/frame.rscrates/vm/src/builtins/code.rsexamples/dis.rscrates/codegen/src/compile.rscrates/compiler-core/src/marshal.rscrates/vm/src/builtins/frame.rs
🧬 Code graph analysis (1)
crates/codegen/src/compile.rs (2)
crates/vm/src/vm/setting.rs (1)
default(172-207)crates/compiler/src/lib.rs (1)
location(58-63)
⏰ 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: Check the WASM package and demo
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
🔇 Additional comments (13)
.cspell.dict/python-more.txt (1)
181-181: LGTM!The addition of
PYTHONNODEBUGRANGESto the spell-check dictionary aligns with the new environment variable controlling debug range emission, mirroring CPython's behavior.examples/dis.rs (1)
56-59: LGTM!Explicit
debug_ranges: trueis appropriate for a disassembly tool where detailed source location information is valuable for debugging and inspection.crates/vm/src/builtins/frame.rs (2)
60-63: LGTM!Converting instruction index to byte offset (
lasti * 2) correctly aligns with CPython'sf_lastisemantics where each instruction unit is 2 bytes.
66-74: LGTM!This fix properly handles the
lasti() == 0case to prevent the underflow panic that was identified in previous reviews. When execution hasn't started, it correctly falls back toco_firstlineno, mirroring CPython'sPyCode_Addr2Linebehavior.crates/vm/src/vm/setting.rs (2)
60-61: LGTM!The new
code_debug_rangesfield is well-documented and appropriately placed within the Settings struct, enabling control over column information in bytecode.
196-196: LGTM!Defaulting
code_debug_rangestotruealigns with CPython's behavior where debug ranges are enabled by default unlessPYTHONNODEBUGRANGESis set.crates/vm/src/frame.rs (2)
388-396: LGTM!The location tuple destructuring
(loc, _end_loc)correctly adapts to the new per-instruction (start, end) location pair format. Usingframe.lasti() * 2for the traceback offset properly converts from instruction index to byte offset for CPython compatibility.
174-176: The concern in this review is not valid. Both call sites ofcurrent_location()already guard againstlasti == 0:
crates/stdlib/src/faulthandler.rs:360– guards withif frame.lasti() == 0 { ... } else { frame.current_location()...}crates/vm/src/builtins/frame.rs:72– guards withif self.lasti() == 0 { ... } else { self.current_location()...}The method is only invoked when
lasti != 0, making the subtraction safe. No additional guard or documentation is needed.Likely an incorrect or invalid review comment.
crates/vm/src/builtins/code.rs (1)
470-485: LGTM!The locations initialization correctly adapts to the new
(start, end)tuple format. Using(loc, loc)where start equals end is appropriate when precise end location information isn't available from the constructor arguments.crates/codegen/src/compile.rs (3)
118-134: Explicit CompileOpts default with debug_ranges looks goodSwitching to a manual
Defaultimpl and defaultingdebug_rangestotrueis consistent with the documented behavior and keeps existing callers (CompileOpts::default()) behaving as expected.
870-874: Passing full CompileOpts into finalize_code is appropriateRouting
&self.optsintofinalize_code(instead of just an optimize level) lets IR finalization make consistent decisions (DCE + linetable) based on compile options without changing scope semantics.
1500-1518: Using StmtRaise.range for the Raise instruction location improves traceback accuracyCapturing the
rangefromStmt::Raiseand callingset_source_range(*range)right before emittingInstruction::Raiseensures the emitted raise is associated with the statement span instead of the last evaluated subexpression, which should give better tracebacks and exception locations.crates/codegen/src/ir.rs (1)
84-91: end_location plumbed through InstructionInfo and finalize_code cleanlyAdding
end_locationtoInstructionInfoand buildinglocations: Vec<(SourceLocation, SourceLocation)>infinalize_code(then passing&locationsplusopts.debug_rangesintogenerate_linetable) is a coherent way to support range-based debugging without disturbing existing stack-depth and metadata logic. This also ensures CodeObjects carry full (start,end) data irrespective of whether debug ranges are enabled in the linetable encoder.Also applies to: 135-177
close #6548
Summary by CodeRabbit
Improvements
Bug Fixes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.