GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Update opcode, dis from 3.14.2 by youknowone · Pull Request #6869 · RustPython/RustPython Conversation
|
📝 WalkthroughWalkthroughThis PR centralizes Resume emission into a new Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
12b9e0e to
cd978d9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@crates/codegen/src/compile.rs`:
- Around line 1145-1187: emit_resume_for_scope currently resets
self.current_source_range to TextRange::default() before emitting RESUME, so
non-module scopes lose the actual definition line; update the call site (where
current_source_range is computed) to pass the real definition line into
emit_resume_for_scope (e.g., add a lineno: Option<u32> parameter) and inside
emit_resume_for_scope set lineno_override = Some(lineno) for non-module scopes
(or, alternatively, restore/populate self.current_source_range from that
definition range before emitting), ensuring the RESUME entry uses the correct
lineno; modify the emit_resume_for_scope signature and its callers accordingly
(refer to emit_resume_for_scope, current_source_range, and the RESUME
instruction emission).
In `@crates/codegen/src/ir.rs`:
- Around line 226-229: When handling a module-scope RESUME with lineno_override
== Some(0), you must also clear end_line and any column information so the
resulting LineTableLocation represents "no line" (line 0, no columns) per
CPython 3.13+ semantics. Update the code that constructs or pushes into
linetable_locations (and any related use of instructions/locations) to, when
lineno_override == Some(0), set start_line and end_line to 0 and clear/reset
start_col/end_col (or their equivalent fields in LineTableLocation) so no column
data is emitted for that entry.
In `@crates/stdlib/src/_opcode.rs`:
- Around line 328-343: The get_common_constants function currently swallows all
errors when calling vm.builtins.get_attr("all"/"any") via unwrap_or_else(|_|
vm.ctx.none()), which masks allocation/internal errors and diverges from frame
opcode behavior; change the signature of get_common_constants to return
PyResult<Vec<PyObjectRef>> (instead of Vec<...>) and propagate get_attr errors
with the ? operator so failures bubble up, or if the intent is to only treat
missing builtins as None explicitly, replace unwrap_or_else with .ok() and map
AttributeError to vm.ctx.none while still propagating other errors; update call
sites to handle the new PyResult as needed and reference get_common_constants,
get_attr, vm.builtins, vm.ctx.none, and VirtualMachine when making the change.
🧹 Nitpick comments (1)
crates/codegen/src/ir.rs (1)
171-186: Optimization helpers and finalize_code integration look good.The fold/convert/cleanup helpers keep the optimization pipeline readable. Operational note: after bytecode instruction changes, run a full clean build and remove any
.pycartifacts to avoid stale caches. Based on learnings, please runrm -r target/debug/build/rustpython-*and delete any.pycfiles.Also applies to: 444-521, 587-617, 619-679, 681-688
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
| let mut instructions = Vec::new(); | ||
| let mut locations = Vec::new(); | ||
| let mut linetable_locations: Vec<LineTableLocation> = Vec::new(); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file structure and the specific lines mentioned cd crates/codegen/src && wc -l ir.rs
Repository: RustPython/RustPython
Length of output: 75
🏁 Script executed:
# Examine the code at lines 226-229 sed -n '220,235p' ir.rs
Repository: RustPython/RustPython
Length of output: 113
🏁 Script executed:
# Check the other locations mentioned: 335-345, 360-368, 850-901 echo "=== Lines 335-345 ===" sed -n '330,350p' ir.rs echo "=== Lines 360-368 ===" sed -n '355,373p' ir.rs echo "=== Lines 850-901 ===" sed -n '845,905p' ir.rs
Repository: RustPython/RustPython
Length of output: 294
🏁 Script executed:
# Search for lineno_override and related structures to understand the context rg -B5 -A5 "lineno_override" ir.rs
Repository: RustPython/RustPython
Length of output: 110
🏁 Script executed:
# Search for LineTableLocation struct definition rg "struct LineTableLocation" ir.rs -A10
Repository: RustPython/RustPython
Length of output: 110
🌐 Web query:
CPython 3.13 3.14 line-0 RESUME instruction linetable encoding
💡 Result:
In CPython 3.13 and 3.14, the source-location info for each bytecode instruction (including the initial RESUME) is stored in code.co_linetable as a stream of location entries. Each entry starts with a header byte 1 cccc lll (MSB set), where cccc is the location kind (0–15) and lll is (length_in_code_units - 1); “code units” are 2-byte units (i.e., one wordcode instruction is length 1). The rest of the entry is kind-specific, using 6-bit-chunk varint/svarint encodings. [1][2]
Why RESUME can show line 0
The start line stored in the first entry is encoded as a delta from co_firstlineno (and subsequent entries delta from the previous start line). So if a code object has co_firstlineno == 0, then RESUME can legitimately decode to line 0 via a delta of 0. [1][2]
(Separately, PEP 626 notes that frames for modules start with f_lineno = 0, and that some bytecodes may be “artificial” and have no meaningful line. Tools should prefer co_lines() / co_positions() rather than assuming the first executable line is co_firstlineno + 1. [3])
The actual encoding you’ll typically see for “line 0 RESUME”
Most commonly, RESUME’s entry uses Kind 13 (“No column info”): it stores start/end line deltas but sets columns to None. [1][2]
For a single-instruction RESUME whose decoded line is 0 (i.e., first entry, start-line delta = 0, end-line delta = 0):
- header byte: kind=13, length=1 →
1 1101 000=0xE8 - start-line delta (svarint 0 → varint 0) →
0x00 - end-line delta (Δ from start line, 0) →
0x00
So the entry bytes are:
E8 00 00 (hex) [1][2]
If instead CPython marks that instruction range as having no location at all (Kind 15 “No location”), then for length=1 the entry is just:
F8 (hex) [1][2]
Note on “RESUME has the def-line number”
For functions, co_firstlineno is the line with the def statement, so the first entry’s delta-0 convention makes RESUME map to the function definition line, which is a behavior change that has affected some tools. [2][4]
Sources: CPython 3.13 Objects/locations.md (format reference) [1]; CPython 3.14 InternalDocs/code_objects.md (same format, moved docs) [2]; PEP 626 (line-number model, module line 0, “artificial” bytecodes) [3]; python.org discussion about RESUME line numbering changes [4].
🌐 Web query:
CPython linetable format line 0 no-line instructions bytecode
💡 Result:
In modern CPython (3.10+), line/position info is no longer “every instruction has a real source line”. The bytecode→source mapping is stored in the code object’s line table (co_linetable) and exposed via code.co_lines(), which can yield ranges whose line is None (“no line number”) for artificial / bookkeeping bytecode. This is explicitly part of the PEP 626 model: some bytecodes “must be marked as artificial, and not have a meaningful line number”, and the iterator’s line can be None. [1]
Internally, when CPython decodes the line table, it represents “no line number” as a negative line (range.line < 0). [1] In the older (3.10-era) internal notes for the delta-encoded line table, there’s also an explicit sentinel for “no line number” (a special line-delta value). [2]
So if you’re seeing line 0 attached to certain instructions, that’s almost always a tooling/formatting artifact where “no line” (None / <0) got coerced to 0 for display or storage. Typical examples of bytecode that may have “no-line” (or otherwise surprising) locations are interpreter/setup opcodes like RESUME (and related prologue opcodes around generators/coroutines), which has been discussed as a behavior change users notice when enumerating covered lines. [3]
Practical guidance:
- Treat “line 0” as “no line info”, not a real source line.
- Prefer
code.co_lines()(line can beNone) per PEP 626, rather than reverse-engineeringco_linetable. [1] - If you need per-instruction positions, use
dis.get_instructions()/dis.dis()(these are built to follow the current semantics). [4]
Sources:
[1] PEP 626 (Precise line numbers; co_lines() yields line or None, and internal decode treats <0 as no-line) https://peps.python.org/pep-0626/
[2] CPython Objects/lnotab_notes.txt (sentinel encoding for “no line number”) https://chromium.googlesource.com/external/github.com/python/cpython/%2B/refs/tags/v3.10.0b3/Objects/lnotab_notes.txt
[3] Discussion of RESUME line-number behavior in 3.11 https://discuss.python.org/t/the-resume-opcode-has-the-same-line-number-as-the-function-definition-and-breaks-previous-behaviour/21972
[4] dis module docs (3.11+) https://docs.python.org/3.11/library/dis.html
🏁 Script executed:
git ls-files | grep ir.rsRepository: RustPython/RustPython
Length of output: 89
🏁 Script executed:
# Once we have the path, read the file at specified lines git ls-files | grep ir.rs | head -1 | xargs -I {} sh -c 'echo "Lines 220-235:"; sed -n "220,235p" {}; echo -e "\nLines 330-350:"; sed -n "330,350p" {}; echo -e "\nLines 355-373:"; sed -n "355,373p" {}; echo -e "\nLines 845-905:"; sed -n "845,905p" {}'
Repository: RustPython/RustPython
Length of output: 4652
🏁 Script executed:
# Search for all LineTableLocation creations in the file rg "LineTableLocation" crates/codegen/src/ir.rs -B3 -A3
Repository: RustPython/RustPython
Length of output: 1069
🏁 Script executed:
# Look for the struct definition rg "struct LineTableLocation" crates/codegen/src/ -A5
Repository: RustPython/RustPython
Length of output: 309
🏁 Script executed:
# Search for lineno_override usage to understand the full context rg "lineno_override" crates/codegen/src/ir.rs -B10 -A5
Repository: RustPython/RustPython
Length of output: 1512
🏁 Script executed:
# Check lines 360-368 more carefully sed -n '355,375p' crates/codegen/src/ir.rs
Repository: RustPython/RustPython
Length of output: 696
🏁 Script executed:
# Check if there are other LineTableLocation creations rg "LineTableLocation {" crates/codegen/src/ir.rs -B1 -A5
Repository: RustPython/RustPython
Length of output: 166
🏁 Script executed:
# Verify the generate_linetable function handles these values sed -n '850,920p' crates/codegen/src/ir.rs
Repository: RustPython/RustPython
Length of output: 2870
🏁 Script executed:
# Search for all LineTableLocation creations without regex special chars grep -n "let lt_loc = LineTableLocation" crates/codegen/src/ir.rs
Repository: RustPython/RustPython
Length of output: 121
🏁 Script executed:
# Also search for direct LineTableLocation struct instantiations grep -n "LineTableLocation {" crates/codegen/src/ir.rs
Repository: RustPython/RustPython
Length of output: 151
🏁 Script executed:
# Let's see what's at lines 360-368 to understand "Also applies to" sed -n '355,375p' crates/codegen/src/ir.rs
Repository: RustPython/RustPython
Length of output: 696
🏁 Script executed:
# Check if there are multiple code blocks creating LineTableLocation with same pattern sed -n '300,330p' crates/codegen/src/ir.rs sed -n '340,370p' crates/codegen/src/ir.rs
Repository: RustPython/RustPython
Length of output: 2783
🏁 Script executed:
# Let me verify there's only one LineTableLocation creation by searching more carefully grep -n "LineTableLocation {" crates/codegen/src/ir.rs -A6
Repository: RustPython/RustPython
Length of output: 677
🏁 Script executed:
# Check if the code at 226-229 is actually the right location by looking at context sed -n '220,245p' crates/codegen/src/ir.rs
Repository: RustPython/RustPython
Length of output: 1066
🏁 Script executed:
# Verify the lineno_override handling one more time sed -n '330,350p' crates/codegen/src/ir.rs
Repository: RustPython/RustPython
Length of output: 1302
🏁 Script executed:
# Count total LineTableLocation creations grep -c "let lt_loc = LineTableLocation" crates/codegen/src/ir.rs
Repository: RustPython/RustPython
Length of output: 66
🏁 Script executed:
# Verify the line numbers in the "Also applies to" section - check 360-368 sed -n '360,375p' crates/codegen/src/ir.rs
Repository: RustPython/RustPython
Length of output: 583
🏁 Script executed:
# Let me check around line 850-901 to understand what that refers to sed -n '848,855p' crates/codegen/src/ir.rs
Repository: RustPython/RustPython
Length of output: 310
🏁 Script executed:
# Verify the issue: when lineno_override = Some(0), what happens in generate_linetable? # Check if line_delta could be 0 but end_line_delta could be non-zero sed -n '870,920p' crates/codegen/src/ir.rs
Repository: RustPython/RustPython
Length of output: 2265
🏁 Script executed:
# Let me understand the "Also applies to" references more carefully # Check what's at the line ranges mentioned echo "=== Checking 335-345 range ===" sed -n '335,345p' crates/codegen/src/ir.rs echo "=== Checking 360-368 range ===" sed -n '360,368p' crates/codegen/src/ir.rs echo "=== Checking 850-901 context - is this generate_linetable? ===" sed -n '845,860p' crates/codegen/src/ir.rs
Repository: RustPython/RustPython
Length of output: 1621
🏁 Script executed:
# Verify the fix would work correctly by checking generate_linetable logic # When col=0, end_col=0, what happens? sed -n '895,950p' crates/codegen/src/ir.rs
Repository: RustPython/RustPython
Length of output: 3063
🏁 Script executed:
# Let me check the actual line-0 RESUME context by searching for where lineno_override is set rg "lineno_override\s*=" crates/codegen/src/ -B2 -A2
Repository: RustPython/RustPython
Length of output: 392
🏁 Script executed:
# Verify the concern about end_line_delta when lineno_override=Some(0) # If lineno_override=Some(0) but end_location.line is non-zero, end_line_delta would be non-zero # This would not fall into the "line_delta == 0 && end_line_delta == 0" path # Let's check what happens in the else path sed -n '910,950p' crates/codegen/src/ir.rs
Repository: RustPython/RustPython
Length of output: 2306
🏁 Script executed:
# Let me check the complete flow for line-0 case # First, verify module scope sets lineno_override sed -n '1,50p' crates/codegen/src/compile.rs | grep -A5 "Module scope"
Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Check the line-0 flow: what happens when prev_line=first_line and line=0 sed -n '930,960p' crates/codegen/src/ir.rs
Repository: RustPython/RustPython
Length of output: 1703
🏁 Script executed:
# Let me verify one more time that the fix would be appropriate by checking # if there are tests or comments about line-0 RESUME semantics rg "RESUME\|line.*0\|line-0" crates/codegen/src/ir.rs | head -20
Repository: RustPython/RustPython
Length of output: 47
Override end_line and columns when lineno_override is set to 0.
When lineno_override = Some(0) is set for module-scope RESUME, the code currently overrides only line to 0 while keeping end_line and columns from the actual source location. Per CPython 3.13+ semantics, line 0 represents "no line" (artificial instructions), and such entries should have no meaningful column information. The current code creates a 0→first_line span with real columns, which violates CPython's linetable encoding (Kind 13 "NoColumns" expects no column data for line-0 entries).
🔧 Possible adjustment for line‑0 overrides
- let lt_loc = LineTableLocation { - line: info - .lineno_override - .unwrap_or_else(|| info.location.line.get() as i32), - end_line: info.end_location.line.get() as i32, - col: info.location.character_offset.to_zero_indexed() as i32, - end_col: info.end_location.character_offset.to_zero_indexed() as i32, - }; + let line_override = info.lineno_override; + let line = line_override.unwrap_or_else(|| info.location.line.get() as i32); + let (end_line, col, end_col) = if line_override == Some(0) { + (0, 0, 0) + } else { + ( + info.end_location.line.get() as i32, + info.location.character_offset.to_zero_indexed() as i32, + info.end_location.character_offset.to_zero_indexed() as i32, + ) + }; + let lt_loc = LineTableLocation { line, end_line, col, end_col };
🤖 Prompt for AI Agents
In `@crates/codegen/src/ir.rs` around lines 226 - 229, When handling a
module-scope RESUME with lineno_override == Some(0), you must also clear
end_line and any column information so the resulting LineTableLocation
represents "no line" (line 0, no columns) per CPython 3.13+ semantics. Update
the code that constructs or pushes into linetable_locations (and any related use
of instructions/locations) to, when lineno_override == Some(0), set start_line
and end_line to 0 and clear/reset start_col/end_col (or their equivalent fields
in LineTableLocation) so no column data is emitted for that entry.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/codegen/src/compile.rs`:
- Around line 1152-1188: The call to enter_scope is passing a byte-offset
(expr.range().start().to_u32()) which enter_scope treats as a line number and
corrupts the line table; locate the call site where enter_scope(...) is invoked
with expr.range().start().to_u32() (around the code that handles expression
scopes) and replace that argument with the actual source line number:
self.get_source_line_number().get().to_u32(), so enter_scope and downstream
SourceLocation creation receive a proper line number consistent with other
callers.
♻️ Duplicate comments (1)
crates/codegen/src/ir.rs (1)
335-344: Override end_line and columns when lineno_override is set to 0.When
lineno_override = Some(0)is set for module-scope RESUME, onlylineis overridden to 0 whileend_line,col, andend_colretain the actual source location. Per CPython 3.13+ semantics, line 0 represents "no line" (artificial instructions) and should have no meaningful column information.🔧 Suggested fix
- let lt_loc = LineTableLocation { - line: info - .lineno_override - .unwrap_or_else(|| info.location.line.get() as i32), - end_line: info.end_location.line.get() as i32, - col: info.location.character_offset.to_zero_indexed() as i32, - end_col: info.end_location.character_offset.to_zero_indexed() as i32, - }; + let line_override = info.lineno_override; + let line = line_override.unwrap_or_else(|| info.location.line.get() as i32); + let (end_line, col, end_col) = if line_override == Some(0) { + // Line 0 = no location info per CPython semantics + (0, 0, 0) + } else { + ( + info.end_location.line.get() as i32, + info.location.character_offset.to_zero_indexed() as i32, + info.end_location.character_offset.to_zero_indexed() as i32, + ) + }; + let lt_loc = LineTableLocation { line, end_line, col, end_col };
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [+] lib: cpython/Lib/dis.py dependencies:
dependent tests: (49 tests)
[+] lib: cpython/Lib/opcode.py dependencies:
dependent tests: (34 tests)
[+] test: cpython/Lib/test/test_compile.py dependencies: dependent tests: (no tests depend on compile) [+] test: cpython/Lib/test/test_peepholer.py dependencies: dependent tests: (no tests depend on peepholer) Legend:
|
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
1 participant
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.