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 — Bytecode parity by youknowone · Pull Request #7507 · RustPython/RustPython Conversation
|
📝 WalkthroughWalkthroughModifies compiler codegen and symbol-table behavior: reworks cellvar classification and module/class conditional-annotation handling, emits module Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
4cd51b6 to
71f8b14
Compare
There was an error while loading. Please reload this page.
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] test: cpython/Lib/test/test_compile.py (TODO: 27) dependencies: dependent tests: (no tests depend on compile) Legend:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
6dfb579 to
be0e81b
Compare
Compiler changes: - Emit TO_BOOL in and/or short-circuit evaluation (COPY+TO_BOOL+JUMP) - Add module-level __conditional_annotations__ cell (PEP 649) - Only set conditional annotations for AnnAssign, not function params - Skip __classdict__ cell when future annotations are active - Convert list literals to tuples in for-loop iterables - Fix cell variable ordering: parameters first, then alphabetical - Fix RESUME DEPTH1 flag for yield-from/await - Don't propagate __classdict__/__conditional_annotations__ freevar through regular functions — only annotation/type-param scopes - Inline string compilation path
be0e81b to
af963bc
Compare
SIGSEGV in _finalizer_registry dict access under aggressive GC and thread switching. Root cause is dict thread-safety in VM.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
crates/codegen/src/compile.rs (1)
1799-1806: Extract the module__conditional_annotations__setup into one helper.
compile_programnow open-codes the same cellvar insertion andMAKE_CELLemission thatenter_scopealso knows about, but the real module path still bypassesenter_scope. Keeping both init paths in sync will be easy to miss on the next parity tweak.As per coding guidelines "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code."
Also applies to: 1810-1817
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/codegen/src/compile.rs` around lines 1799 - 1806, Extract the duplicated module-level __conditional_annotations__ setup into a single helper (e.g., add a method like emit_module_conditional_annotations or init_conditional_annotations) and call it from both compile_program and enter_scope; specifically, move the logic that checks has_module_cond_ann, inserts "__conditional_annotations__" into current_code_info().metadata.cellvars, and emits the MAKE_CELL sequence into that helper so both code paths (the open‑coded path in compile_program and the one in enter_scope) invoke the same routine, passing only the differing value (has_module_cond_ann or the current scope) as needed to avoid duplication.
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/codegen/src/compile.rs`:
- Around line 5460-5477: The list→tuple optimization should be skipped when
compiling the iterable of an async for so GET_AITER sees the original list type;
modify the block that currently checks ast::Expr::List (the one that compiles
elts and emits Instruction::BuildTuple) to also verify we are NOT on the
async-for path (e.g., add a guard like !self.is_compiling_async_for() or check
the parent node kind) before building the tuple; if in an async for, fall
through to the existing self.compile_expression(iter)? branch so the list
literal is preserved and error messages remain source-faithful.
In `@crates/codegen/src/symboltable.rs`:
- Around line 300-301: The branch checking symbol_table.needs_classdict and
symbol_table.future_annotations can be wrong for nested class symbol tables
because SymbolTable::new() defaults future_annotations to false and finish()
only sets the top-level module; propagate the parent's future_annotations into
any nested SymbolTable instances when creating them (or have finish() propagate
the flag into child class tables) so nested class tables correctly see the from
__future__ import annotations flag before this check; update the code that
constructs nested symbol tables (where SymbolTable::new() is called for classes)
to copy parent.symbol_table.future_annotations into the child
SymbolTable.future_annotations, ensuring needs_classdict logic uses the correct
flag.
---
Nitpick comments:
In `@crates/codegen/src/compile.rs`:
- Around line 1799-1806: Extract the duplicated module-level
__conditional_annotations__ setup into a single helper (e.g., add a method like
emit_module_conditional_annotations or init_conditional_annotations) and call it
from both compile_program and enter_scope; specifically, move the logic that
checks has_module_cond_ann, inserts "__conditional_annotations__" into
current_code_info().metadata.cellvars, and emits the MAKE_CELL sequence into
that helper so both code paths (the open‑coded path in compile_program and the
one in enter_scope) invoke the same routine, passing only the differing value
(has_module_cond_ann or the current scope) as needed to avoid duplication.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 7ba5c833-290b-4a20-8322-a1a86c7ba71e
Lib/test/test_peepholer.py is excluded by !Lib/**crates/codegen/src/snapshots/rustpython_codegen__compile__tests__nested_bool_op.snap is excluded by !**/*.snapcrates/codegen/src/compile.rscrates/codegen/src/ir.rscrates/codegen/src/symboltable.rs
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
…ons to nested scopes
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/codegen/src/compile.rs`:
- Around line 1113-1116: The module conditional-annotation preamble must be
emitted identically for both entry paths; modify compile_program_single() to
share the same logic used by compile_program() by checking
ste.has_conditional_annotations (or computing the conditional-annotations flag
first) and then invoking the common preamble emissions that insert the
root-module cell into cellvar_cache (insert "__conditional_annotations__") and
emit the MAKE_CELL preamble instead of falling through to RESUME; refactor so
the differing entry value is determined first and the shared logic that updates
cellvar_cache and emits the MAKE_CELL preamble is called from both
compile_program() and compile_program_single(), ensuring bytecode parity (also
apply same refactor to the other occurrence around the code referenced at
~1799-1817).
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 44fb417d-3c03-4e6a-9d7b-1706d30790c5
Lib/test/_test_multiprocessing.py is excluded by !Lib/**crates/codegen/src/compile.rscrates/codegen/src/symboltable.rs
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| if ste.has_conditional_annotations | ||
| && matches!(scope_type, CompilerScope::Class | CompilerScope::Module) | ||
| { | ||
| cellvar_cache.insert("__conditional_annotations__".to_string()); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Keep the module conditional-annotation preamble shared across module entry paths.
This now wires the root-module cell + MAKE_CELL preamble only through compile_program(). compile_program_single() still goes straight to RESUME, so the same annotated module will compile differently between exec and single, which leaves a bytecode-parity gap in exactly the area this PR is tightening.
As per coding guidelines, "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code".
Also applies to: 1799-1817
Verify each finding against the current code and only fix it if needed.
In `@crates/codegen/src/compile.rs` around lines 1113 - 1116, The module
conditional-annotation preamble must be emitted identically for both entry
paths; modify compile_program_single() to share the same logic used by
compile_program() by checking ste.has_conditional_annotations (or computing the
conditional-annotations flag first) and then invoking the common preamble
emissions that insert the root-module cell into cellvar_cache (insert
"__conditional_annotations__") and emit the MAKE_CELL preamble instead of
falling through to RESUME; refactor so the differing entry value is determined
first and the shared logic that updates cellvar_cache and emits the MAKE_CELL
preamble is called from both compile_program() and compile_program_single(),
ensuring bytecode parity (also apply same refactor to the other occurrence
around the code referenced at ~1799-1817).
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
* Bytecode parity phase 3 Compiler changes: - Emit TO_BOOL in and/or short-circuit evaluation (COPY+TO_BOOL+JUMP) - Add module-level __conditional_annotations__ cell (PEP 649) - Only set conditional annotations for AnnAssign, not function params - Skip __classdict__ cell when future annotations are active - Convert list literals to tuples in for-loop iterables - Fix cell variable ordering: parameters first, then alphabetical - Fix RESUME DEPTH1 flag for yield-from/await - Don't propagate __classdict__/__conditional_annotations__ freevar through regular functions — only annotation/type-param scopes - Inline string compilation path * Skip test_thread_safety in _test_multiprocessing SIGSEGV in _finalizer_registry dict access under aggressive GC and thread switching. Root cause is dict thread-safety in VM. * Skip list→tuple optimization for async for; propagate future_annotations to nested scopes
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.