| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
📝 Walkthrough
WalkthroughModifies compiler codegen and symbol-table behavior: reworks cellvar classification and module/class conditional-annotation handling, emits module MakeCell and adjusts RESUME ordering, optimizes for over list literals, reorders boolean short-circuit ToBool emission, refines yield/exception-depth marking, and tightens PEP 649 annotation/free-name propagation. 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)
✏️ 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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
📦 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.
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
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)🤖 Prompt for all review comments with AI agents1799-1806: Extract the module __conditional_annotations__ setup into one helper.
compile_program now open-codes the same cellvar insertion and MAKE_CELL emission that enter_scope also knows about, but the real module path still bypasses enter_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 AgentsVerify 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
📥 CommitsReviewing files that changed from the base of the PR and between 9282a87 and af963bc.
⛔ Files ignored due to path filters (2)
Sorry, something went wrong.
…ons to nested scopes
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agentsVerify 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
📥 CommitsReviewing files that changed from the base of the PR and between af963bc and 860a3db.
⛔ Files ignored due to path filters (1)
Sorry, something went wrong.
| 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.
⚠️ Potential issue | 🟡 Minor
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 AgentsVerify 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.
| Back | FazBrowse Home | New Git URL |
Summary by CodeRabbit