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 |
4cd51b6 to
71f8b14
Compare
📦 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:
|
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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
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.
🤖 Prompt for all review comments with AI agents
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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 7ba5c833-290b-4a20-8322-a1a86c7ba71e
⛔ Files ignored due to path filters (2)
Lib/test/test_peepholer.pyis excluded by!Lib/**crates/codegen/src/snapshots/rustpython_codegen__compile__tests__nested_bool_op.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
crates/codegen/src/compile.rscrates/codegen/src/ir.rscrates/codegen/src/symboltable.rs
…ons to nested scopes
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
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).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 44fb417d-3c03-4e6a-9d7b-1706d30790c5
⛔ Files ignored due to path filters (1)
Lib/test/_test_multiprocessing.pyis excluded by!Lib/**
📒 Files selected for processing (2)
crates/codegen/src/compile.rscrates/codegen/src/symboltable.rs
| 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.
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
🤖 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 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).
Summary by CodeRabbit