Enable PEP 709 inlined comprehensions#7412
Open
youknowone wants to merge 3 commits intoRustPython:mainfrom
Open
Enable PEP 709 inlined comprehensions#7412youknowone wants to merge 3 commits intoRustPython:mainfrom
youknowone wants to merge 3 commits intoRustPython:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefines comprehension inlining decisions and symbol-table merging for inlined comprehensions, adjusts compile state toggling during inlined compilation, simplifies inlined iterator/loop emission, and changes fast-local load/store to use Option semantics for empty slots. Changes
Sequence DiagramsequenceDiagram
participant Compiler
participant SymbolTable
participant CodeInfo
participant VMFrame
Compiler->>SymbolTable: analyze_symbol_table(comprehension)
SymbolTable->>SymbolTable: detect nested scopes\ntrack comp_added_symbols\ndecide comp_inlined
alt Inlining Allowed
SymbolTable-->>Compiler: comp_inlined = true
else Inlining Blocked
SymbolTable-->>Compiler: comp_inlined = false
end
Compiler->>CodeInfo: is_inlined_comprehension_context(comprehension_type)
CodeInfo->>CodeInfo: check in_func && sub_table.comp_inlined
alt Should Inline
CodeInfo->>CodeInfo: set in_inlined_comp = true
Compiler->>Compiler: compile_inlined_comprehension()\nsplice sub-tables, merge locals
CodeInfo->>CodeInfo: set in_inlined_comp = false
else Standard Path
Compiler->>Compiler: compile comprehension normally
end
Note right of VMFrame: fast-local semantics updated
Compiler->>VMFrame: emit LoadFastAndClear / StoreFast
VMFrame->>VMFrame: use push_value_opt / pop_value_opt (Option semantics)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Contributor
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/modulefinder.py dependencies:
dependent tests: (2 tests)
[x] lib: cpython/Lib/poplib.py dependencies:
dependent tests: (1 tests)
[ ] test: cpython/Lib/test/test_generators.py (TODO: 12) dependencies: dependent tests: (no tests depend on generator) Legend:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
crates/stdlib/src/_asyncio.rs (1)
501-515: Extract the shared cancelled-error helper.These two implementations are now byte-for-byte identical. Pulling the common logic into one helper would make future asyncio parity fixes much less likely to drift between
PyFutureandPyTask.♻️ Possible extraction
+fn take_or_create_cancelled_error( + cancelled_exc: &PyRwLock<Option<PyObjectRef>>, + cancel_msg: &PyRwLock<Option<PyObjectRef>>, + vm: &VirtualMachine, +) -> PyBaseExceptionRef { + if let Some(exc) = cancelled_exc.write().take() + && let Ok(exc) = exc.downcast::<PyBaseException>() + { + return exc; + } + + let args = cancel_msg.read().clone().into_iter().collect::<Vec<_>>(); + match get_cancelled_error_type(vm) { + Ok(cancelled_error) => vm.new_exception(cancelled_error, args), + Err(_) => vm.new_runtime_error("cancelled"), + } +}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: 1310-1324
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/stdlib/src/_asyncio.rs` around lines 501 - 515, Both PyFuture::make_cancelled_error_impl and PyTask::make_cancelled_error_impl contain identical logic; extract a single helper (e.g., make_cancelled_error_helper) that takes the shared inputs (the stored cancelled-exception cell/reference and the cancelled-message cell/reference plus &VirtualMachine) and returns PyBaseExceptionRef using the same logic: take and downcast fut_cancelled_exc, fallback to reading fut_cancel_msg and building args, then call get_cancelled_error_type(vm) to construct the exception or vm.new_runtime_error("cancelled"). Replace both existing make_cancelled_error_impl bodies to call this new helper (and update the other duplicated block around 1310-1324 similarly), keeping the helper visible to both PyFuture and PyTask implementations.crates/vm/src/gc_state.rs (2)
425-440: Consider extracting duplicate reset-and-return logic into a helper.The same pattern appears four times: calculate
reset_end, loop to reset counts, compute duration, update stats, and returnCollectResult. This violates DRY.♻️ Suggested refactor to reduce duplication
// Add this helper method to GcState fn finalize_collection( &self, generation: usize, collected: usize, uncollectable: usize, candidates: usize, start_time: &impl TimingStart, ) -> CollectResult { let reset_end = if generation >= 2 { 2 } else { generation + 1 }; for i in 0..reset_end { self.generations[i].count.store(0, Ordering::SeqCst); } let duration = elapsed_secs(start_time); self.generations[generation].update_stats(collected, uncollectable, candidates, duration); CollectResult { collected, uncollectable, candidates, duration, } }Then replace each occurrence with a call to this helper.
Also applies to: 536-551, 556-572, 705-720
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/gc_state.rs` around lines 425 - 440, Extract the repeated reset-and-return pattern into a GcState helper (e.g., fn finalize_collection(&self, generation: usize, collected: usize, uncollectable: usize, candidates: usize, start_time: &impl TimingStart) -> CollectResult) that computes reset_end (if generation >= 2 { 2 } else { generation + 1 }), loops over self.generations[0..reset_end] to store(0, Ordering::SeqCst), calls elapsed_secs(start_time), calls self.generations[generation].update_stats(collected, uncollectable, candidates, duration), and returns the CollectResult; then replace the four duplicate blocks (the ones that currently perform the reset loop, call elapsed_secs, update_stats, and return CollectResult) with calls to finalize_collection to remove the duplication.
713-713:uncollectableis always 0 in stats updates.Looking at all
update_statscalls andCollectResultconstructions,uncollectableis hardcoded to0. If uncollectable object detection isn't implemented yet, consider adding a TODO comment to clarify this is intentional.- self.generations[generation].update_stats(collected, 0, candidates, duration); + // TODO: Uncollectable detection (objects with __del__ in cycles) not yet implemented + self.generations[generation].update_stats(collected, 0, candidates, duration);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/gc_state.rs` at line 713, The call to self.generations[generation].update_stats(collected, 0, candidates, duration) always passes 0 for the uncollectable count; either compute and pass the real uncollectable count from the corresponding CollectResult (or other collector return value) and propagate that value into update_stats, or if uncollectable detection is not implemented yet, replace the hardcoded 0 with a named variable (e.g., uncollectable) and add a TODO comment near update_stats / CollectResult construction clarifying that uncollectable counting is not yet implemented and should be filled in when implemented; refer to update_stats and CollectResult to locate where to change the call and where to surface the TODO.crates/codegen/src/compile.rs (2)
7652-7660: Restore the previousin_inlined_compstate instead of forcingfalse.Line 7659 hard-resets the flag. That works with today's no-nested-scope policy, but it will drop an outer
trueif this path ever becomes re-entrant. Saving and restoring the old value keeps the state transition local.♻️ Suggested tweak
- self.current_code_info().in_inlined_comp = true; + let was_in_inlined_comp = self.current_code_info().in_inlined_comp; + self.current_code_info().in_inlined_comp = true; let result = self.compile_inlined_comprehension( init_collection, generators, compile_element, has_an_async_gen, ); - self.current_code_info().in_inlined_comp = false; + self.current_code_info().in_inlined_comp = was_in_inlined_comp; return result;🤖 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 7652 - 7660, The current code unconditionally sets self.current_code_info().in_inlined_comp = false after calling compile_inlined_comprehension which can clobber an outer true; instead capture the previous value (e.g., let old = self.current_code_info().in_inlined_comp), set in_inlined_comp = true before calling compile_inlined_comprehension, then restore it to old afterwards so the state change is local to this call.
7849-7865: Add a regression test that rewindsnext_sub_table.This helper now owns the cursor advance for inlined comprehensions. A case that compiles the same AST twice—an inlined comprehension inside
finallyis the easiest one here—would lock down the reset logic incompile_try_statementand protect this fix from future cursor regressions.🤖 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 7849 - 7865, The regression is that advancing next_sub_table when inlining a comprehension can leave the symbol table cursor moved for subsequent compilations of the same AST; add a regression test that compiles the same AST twice (best case: an inlined comprehension inside a finally block) to ensure compile_try_statement properly rewinds next_sub_table on exit. In the test, exercise compile_try_statement (via a small snippet that triggers an inlined comprehension in a finally) and assert that after the first compilation a second compilation succeeds without panicking or mis-indexing symbol_table_stack.next_sub_table; this ensures the fix that mutates next_sub_table in the comprehension-splicing code is protected against cursor regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/codegen/src/compile.rs`:
- Around line 7652-7660: The current code unconditionally sets
self.current_code_info().in_inlined_comp = false after calling
compile_inlined_comprehension which can clobber an outer true; instead capture
the previous value (e.g., let old = self.current_code_info().in_inlined_comp),
set in_inlined_comp = true before calling compile_inlined_comprehension, then
restore it to old afterwards so the state change is local to this call.
- Around line 7849-7865: The regression is that advancing next_sub_table when
inlining a comprehension can leave the symbol table cursor moved for subsequent
compilations of the same AST; add a regression test that compiles the same AST
twice (best case: an inlined comprehension inside a finally block) to ensure
compile_try_statement properly rewinds next_sub_table on exit. In the test,
exercise compile_try_statement (via a small snippet that triggers an inlined
comprehension in a finally) and assert that after the first compilation a second
compilation succeeds without panicking or mis-indexing
symbol_table_stack.next_sub_table; this ensures the fix that mutates
next_sub_table in the comprehension-splicing code is protected against cursor
regressions.
In `@crates/stdlib/src/_asyncio.rs`:
- Around line 501-515: Both PyFuture::make_cancelled_error_impl and
PyTask::make_cancelled_error_impl contain identical logic; extract a single
helper (e.g., make_cancelled_error_helper) that takes the shared inputs (the
stored cancelled-exception cell/reference and the cancelled-message
cell/reference plus &VirtualMachine) and returns PyBaseExceptionRef using the
same logic: take and downcast fut_cancelled_exc, fallback to reading
fut_cancel_msg and building args, then call get_cancelled_error_type(vm) to
construct the exception or vm.new_runtime_error("cancelled"). Replace both
existing make_cancelled_error_impl bodies to call this new helper (and update
the other duplicated block around 1310-1324 similarly), keeping the helper
visible to both PyFuture and PyTask implementations.
In `@crates/vm/src/gc_state.rs`:
- Around line 425-440: Extract the repeated reset-and-return pattern into a
GcState helper (e.g., fn finalize_collection(&self, generation: usize,
collected: usize, uncollectable: usize, candidates: usize, start_time: &impl
TimingStart) -> CollectResult) that computes reset_end (if generation >= 2 { 2 }
else { generation + 1 }), loops over self.generations[0..reset_end] to store(0,
Ordering::SeqCst), calls elapsed_secs(start_time), calls
self.generations[generation].update_stats(collected, uncollectable, candidates,
duration), and returns the CollectResult; then replace the four duplicate blocks
(the ones that currently perform the reset loop, call elapsed_secs,
update_stats, and return CollectResult) with calls to finalize_collection to
remove the duplication.
- Line 713: The call to self.generations[generation].update_stats(collected, 0,
candidates, duration) always passes 0 for the uncollectable count; either
compute and pass the real uncollectable count from the corresponding
CollectResult (or other collector return value) and propagate that value into
update_stats, or if uncollectable detection is not implemented yet, replace the
hardcoded 0 with a named variable (e.g., uncollectable) and add a TODO comment
near update_stats / CollectResult construction clarifying that uncollectable
counting is not yet implemented and should be filled in when implemented; refer
to update_stats and CollectResult to locate where to change the call and where
to surface the TODO.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 43f83f0b-5f5a-467e-996a-810da133e093
⛔ Files ignored due to path filters (3)
Lib/test/_test_gc_fast_cycles.pyis excluded by!Lib/**Lib/test/test_gc.pyis excluded by!Lib/**Lib/test/test_symtable.pyis excluded by!Lib/**
📒 Files selected for processing (7)
crates/codegen/src/compile.rscrates/codegen/src/symboltable.rscrates/stdlib/src/_asyncio.rscrates/stdlib/src/socket.rscrates/vm/src/frame.rscrates/vm/src/gc_state.rscrates/vm/src/stdlib/gc.rs
Contributor
|
We may need to rebase this branch on the main branch because some commits seem like already merged by #7354 |
youknowone
force-pushed
the
pep709-inlined-comprehensions
branch
from
e58f796 to
1219da9
Compare
Contributor
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/symboltable.rs`:
- Around line 475-518: found_in_inner_scope() currently examines sub_tables when
deciding to promote outer locals to Cells but doesn’t skip comprehension tables
that were inlined (st.comp_inlined), so closures get wrongly promoted after
we've already merged those symbols; update found_in_inner_scope() to ignore any
sub-table where st.comp_inlined is true (e.g., add a guard at the start of the
loop or conditional to continue if st.comp_inlined) so inlined comprehension
tables are not considered during closure promotion after merging via
analyze_symbol().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 7de56247-53ae-4d7d-bd0d-ecea9530cad4
📒 Files selected for processing (3)
crates/codegen/src/compile.rscrates/codegen/src/symboltable.rscrates/vm/src/frame.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/vm/src/frame.rs
Contributor
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 7662-7671: The inlined path is incorrectly used for async
comprehensions; modify the branch around current_code_info().in_inlined_comp and
compile_inlined_comprehension so that if has_an_async_gen is true you do NOT set
in_inlined_comp or call compile_inlined_comprehension (i.e., fall through to the
non-inlined comprehension code path that builds the AsyncComprehensionGenerator
and emits SetupFinally/END_ASYNC_FOR). Specifically, add a guard on
has_an_async_gen before calling compile_inlined_comprehension and only use the
inlined path when has_an_async_gen is false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: b992609c-f04d-4ac2-9caf-68edfb5b9e72
📒 Files selected for processing (1)
crates/codegen/src/compile.rs
Contributor
|
Better to merge after #7427 |
Activate the existing compile_inlined_comprehension() implementation by fixing 6 bugs that prevented it from working: - LoadFastAndClear: push NULL (not None) when slot is empty so StoreFast can restore empty state after comprehension - StoreFast: accept NULL from stack for the restore path - sub_tables.remove(0) replaced with next_sub_table cursor to match the pattern used elsewhere in the compiler - in_inlined_comp flag moved from non-inlined to inlined path - is_inlined_comprehension_context() now checks comp_inlined flag and restricts inlining to function-like scopes - comp_inlined set only when parent scope uses fastlocals Symbol table analysis handles conflict detection: - Nested scopes in comprehension → skip inlining - Bound name conflicts with parent symbol → skip inlining - Cross-comprehension reference conflicts → skip inlining - Splice comprehension sub_tables into parent for nested scope tracking
youknowone
force-pushed
the
pep709-inlined-comprehensions
branch
2 times, most recently
from
3603dc9 to
a6e3a80
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/codegen/src/symboltable.rs (1)
718-740:⚠️ Potential issue | 🔴 CriticalSkip inlined comprehensions during closure promotion.
The
found_in_inner_scope()function still lacks a guard to skipcomp_inlinedtables. After merging inlined comprehension symbols into the parent (lines 475-518), the analysis at line 523 still passes the fullsub_tablesslice. An outer local referenced from an inlined comprehension will be incorrectly promoted toCell, undermining the PEP 709 fast-local model.Example: In
def f(x): return [x + y for y in z], variablexshould remain a fastlocal, not become a Cell.,
Proposed fix
fn found_in_inner_scope( &self, sub_tables: &[SymbolTable], name: &str, st_typ: CompilerScope, ) -> Option<SymbolScope> { sub_tables.iter().find_map(|st| { + if st.comp_inlined { + return None; + } let sym = st.symbols.get(name)?; if sym.scope == SymbolScope::Free || sym.flags.contains(SymbolFlags::FREE_CLASS) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/codegen/src/symboltable.rs` around lines 718 - 740, The function found_in_inner_scope should skip inlined-comprehension symbol tables when searching sub_tables to avoid promoting outer locals used only inside inlined comprehensions; modify the iterator in found_in_inner_scope to ignore any st where st.flags.contains(SymbolFlags::COMP_INLINED) (or the equivalent comp_inlined marker on SymbolTable) before checking st.symbols.get(name), so only real inner scopes are considered for Cell/GlobalExplicit decisions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/codegen/src/symboltable.rs`:
- Around line 718-740: The function found_in_inner_scope should skip
inlined-comprehension symbol tables when searching sub_tables to avoid promoting
outer locals used only inside inlined comprehensions; modify the iterator in
found_in_inner_scope to ignore any st where
st.flags.contains(SymbolFlags::COMP_INLINED) (or the equivalent comp_inlined
marker on SymbolTable) before checking st.symbols.get(name), so only real inner
scopes are considered for Cell/GlobalExplicit decisions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 15d44e4e-24eb-4383-831e-b6516b3b7586
📒 Files selected for processing (2)
crates/codegen/src/compile.rscrates/codegen/src/symboltable.rs
Replace conservative bail-out approach with CPython-matching strategy: Symbol table (symboltable.rs): - Remove conflict-based bail-out checks (bound conflict, ref conflict, nested scopes) - Implement inline_comprehension() matching CPython's symtable.c: track inlined_cells, handle __class__ in ClassBlock, merge symbols with proper is_free_in_any_child check - Promote LOCAL to CELL for inlined_cells, set COMP_CELL flag - Splice inlined comp children into parent's sub_tables - Add can_see_class_scope check to inlining decision Compiler (compile.rs): - Implement TweakInlinedComprehensionScopes: temporarily swap symbol scopes during comprehension body compilation - Implement RevertInlinedComprehensionScopes: restore original scopes - Push ALL locally-bound names (not just iteration targets) for save/restore, excluding walrus operator targets - Emit MakeCell/RestoreCell for CELL variables in comprehensions - Re-enable async comprehension inlining with proper SetupFinally/EndAsyncFor handling - Include COMP_CELL symbols in cellvars VM (frame.rs, instruction.rs): - Add RestoreCell instruction for proper cell save/restore - MakeCell now saves old cell to stack and creates new empty cell
youknowone
force-pushed
the
pep709-inlined-comprehensions
branch
from
a6e3a80 to
3b7a16b
Compare
ShaharNaveh
reviewed
Mar 14, 2026
| /// Pops the old cell object from the stack and writes it back to the | ||
| /// cell slot at `fastlocals[nlocals + i]`, replacing the temporary cell | ||
| /// created by `MakeCell`. | ||
| RestoreCell { |
Contributor
There was a problem hiding this comment.
How does CPython do it? can we not break the full CPython 3.14 bytecode compatibility?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
compile_inlined_comprehension()implementation by fixing 6 bugs that prevented it from workingChanges
frame.rs:LoadFastAndClearpushes NULL when slot is empty (not None),StoreFastaccepts NULL for restore pathcompile.rs: Enableis_inlined_comprehension_context(), fixin_inlined_compflag placement, fixsub_tables.remove(0)→next_sub_tablecursor, splice nested sub_tables into parentsymboltable.rs: Setcomp_inlinedonly in function-like parent scopes, conflict detection (nested scopes, bound name conflicts, cross-comprehension reference conflicts)Current limitations
Test plan
test_listcomps— 59/59 passtest_setcomps— passtest_symtable— passtest_dictcomps— 1 pre-existing failure (unrelated)test_compile,test_syntax— passcargo test -p rustpython-codegen -p rustpython-compiler— pass🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Bug Fixes