Actionable comments posted: 3

crates/compiler-core/src/bytecode.rs (1)

26-30: Consider re-exporting the replacement resume types here too.

ResumeType used to be available from rustpython_compiler_core::bytecode::*. Removing it without adding ResumeContext / ResumeLocation makes downstream code change module paths as well as type names, even though the rest of the oparg surface is still re-exported from this module.

Possible re-export update

-        MakeFunctionFlag, MakeFunctionFlags, NameIdx, OpArg, OpArgByte, OpArgState, OpArgType,
-        RaiseKind, SpecialMethod, UnpackExArgs,
+        MakeFunctionFlag, MakeFunctionFlags, NameIdx, OpArg, OpArgByte, OpArgState, OpArgType,
+        RaiseKind, ResumeContext, ResumeLocation, SpecialMethod, UnpackExArgs,

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/compiler-core/src/bytecode.rs` around lines 26 - 30, The bytecode
re-exports removed ResumeType and now downstream code breaks; update the oparg
re-exports to include the new resume types by adding public re-exports for
ResumeContext and ResumeLocation from the oparg module (and to preserve
compatibility optionally add a pub type alias ResumeType = ResumeContext so
existing code that expects ResumeType continues to compile). Ensure you
reference the symbols ResumeContext, ResumeLocation (and ResumeType if creating
the alias) in the bytecode re-export list alongside the other OpArg items.

crates/codegen/src/compile.rs (2)

1280-1282: Centralize ResumeContext creation.

The extra RESUME bit is now part of the emitted bytecode, but these sites still encode it with bare booleans, and the open TODOs on Line 7384 and Line 7609 show how easy that invariant is to lose. A tiny helper / named constructor would keep the manual OpArg::new(...) path and the yield/await sites aligned.

♻️ Suggested direction

+    const fn resume_context(location: oparg::ResumeLocation) -> oparg::ResumeContext {
+        match location {
+            oparg::ResumeLocation::AtFuncStart => oparg::ResumeContext::new(location, false),
+            oparg::ResumeLocation::AfterYield => oparg::ResumeContext::new(location, true),
+            oparg::ResumeLocation::AfterYieldFrom | oparg::ResumeLocation::AfterAwait => {
+                oparg::ResumeContext::new(location, false)
+            }
+        }
+    }

Then reuse Self::resume_context(...) at the four Instruction::Resume call sites.

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: 7204-7211, 7382-7385, 7607-7610

🤖 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 1280 - 1282, The code constructs
ResumeContext inline with OpArg::new(...) (e.g., using
oparg::ResumeContext::new(oparg::ResumeLocation::AtFuncStart, false).into()) at
multiple Instruction::Resume emission sites; centralize this by adding a helper
named constructor (e.g., Self::resume_context(location, resume_bit)) that
returns the properly encoded OpArg/ResumeContext and replace the four raw sites
(the Instruction::Resume call locations) to call Self::resume_context(...)
instead of duplicating the inline construction so the RESUME bit handling is
consistent and the TODOs at the referenced locations are resolved.

---

1280-1282: Pin the new RESUME variants with local disassembly tests.

This PR changes the emitted oparg for scope entry and suspension points. Adding a few display_expand_code_objects() snapshots here for a plain function, direct yield, yield from, await, and a genexpr would make future opcode-arg refactors much safer.

If helpful, I can sketch the minimal snapshot cases.

Also applies to: 7204-7211, 7382-7385, 7607-7610

🤖 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 1280 - 1282, Add local
disassembly snapshot tests that pin the new RESUME variants: for each place you
construct an OpArg::new(oparg::ResumeContext::new(...)) (e.g., the Emit sites
using oparg::ResumeLocation::AtFuncStart), add display_expand_code_objects()
snapshots for a plain function, a direct yield, a yield from, an await, and a
generator-expression so future opcode-arg refactors are caught; repeat the same
snapshot additions for the other ResumeContext construction sites in this module
so all RESUME variants are covered.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/compiler-core/src/bytecode/instruction.rs`:
- Around line 306-308: The VM-side InstrumentedResume decoder still branches on
the raw u32 value of the oparg (so depth-1 RESUME values no longer match);
change the decoder in the frame resume-handling code to parse the oparg via
oparg::ResumeContext::from_u32(u32::from(arg)) and switch on context.location()
(ResumeLocation::AtFuncStart / AfterYield / AfterYieldFrom / AfterAwait) instead
of comparing raw integers, updating any match arms that currently check numeric
values to use those ResumeLocation variants.

In `@crates/compiler-core/src/bytecode/oparg.rs`:
- Around line 799-803: The intra-doc links in the Resume instruction comment
currently reference non-existent symbols `Context::location` and
`Context::is_exception_depth1`; update those links to point to the actual struct
`ResumeContext` members by changing them to `ResumeContext::location` and
`ResumeContext::is_exception_depth1` so the docs resolve correctly (locate the
comment around the `Instruction::Resume`/`ResumeContext` definitions and replace
the `Context::...` links accordingly).
- Around line 834-838: The is_exception_depth1() check is incorrect because it
compares (self.as_u32() & Self::DEPTH1_MASK) == 1 while DEPTH1_MASK is 0x4;
update is_exception_depth1 to test against the mask correctly (e.g., compare to
Self::DEPTH1_MASK or check != 0) so it returns true when the DEPTH1 bit is set;
locate the method is_exception_depth1 and replace the equality with a proper
mask comparison using DEPTH1_MASK.

---

Nitpick comments:
In `@crates/codegen/src/compile.rs`:
- Around line 1280-1282: The code constructs ResumeContext inline with
OpArg::new(...) (e.g., using
oparg::ResumeContext::new(oparg::ResumeLocation::AtFuncStart, false).into()) at
multiple Instruction::Resume emission sites; centralize this by adding a helper
named constructor (e.g., Self::resume_context(location, resume_bit)) that
returns the properly encoded OpArg/ResumeContext and replace the four raw sites
(the Instruction::Resume call locations) to call Self::resume_context(...)
instead of duplicating the inline construction so the RESUME bit handling is
consistent and the TODOs at the referenced locations are resolved.
- Around line 1280-1282: Add local disassembly snapshot tests that pin the new
RESUME variants: for each place you construct an
OpArg::new(oparg::ResumeContext::new(...)) (e.g., the Emit sites using
oparg::ResumeLocation::AtFuncStart), add display_expand_code_objects() snapshots
for a plain function, a direct yield, a yield from, an await, and a
generator-expression so future opcode-arg refactors are caught; repeat the same
snapshot additions for the other ResumeContext construction sites in this module
so all RESUME variants are covered.

In `@crates/compiler-core/src/bytecode.rs`:
- Around line 26-30: The bytecode re-exports removed ResumeType and now
downstream code breaks; update the oparg re-exports to include the new resume
types by adding public re-exports for ResumeContext and ResumeLocation from the
oparg module (and to preserve compatibility optionally add a pub type alias
ResumeType = ResumeContext so existing code that expects ResumeType continues to
compile). Ensure you reference the symbols ResumeContext, ResumeLocation (and
ResumeType if creating the alias) in the bytecode re-export list alongside the
other OpArg items.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 2a5f4ddf-f907-4a68-be63-ad4b914893d4

Reviewing files that changed from the base of the PR and between e3ac1bf and f18e0fe.

• crates/codegen/src/compile.rs
• crates/codegen/src/ir.rs
• crates/compiler-core/src/bytecode.rs
• crates/compiler-core/src/bytecode/instruction.rs
• crates/compiler-core/src/bytecode/oparg.rs
• crates/vm/src/frame.rs

Actionable comments posted: 2

crates/compiler-core/src/bytecode/oparg.rs (1)

799-803: ⚠️ Potential issue | 🟡 Minor

Fix the ResumeContext intra-doc links.

Context::location and Context::is_exception_depth1 do not exist here, so rustdoc will not resolve these links.

📝 Suggested doc fix

-    /// 1. [`Context::location`]: Indicates where the instruction occurs.
-    /// 2. [`Context::is_exception_depth1`]: Is the instruction is at except-depth 1.
+    /// 1. [`ResumeContext::location`]: Indicates where the instruction occurs.
+    /// 2. [`ResumeContext::is_exception_depth1`]: Whether the instruction is at except-depth 1.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/compiler-core/src/bytecode/oparg.rs` around lines 799 - 803, The
intra-doc links point to non-existent symbols `Context::location` and
`Context::is_exception_depth1`; update them to reference the actual type/fields
on the resume context (e.g. `ResumeContext::location` and
`ResumeContext::is_exception_depth1`) so rustdoc can resolve the links and keep
the `Instruction::Resume` reference unchanged.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/codegen/src/compile.rs`:
- Around line 7382-7385: There are two instantiations of
oparg::ResumeContext::new using oparg::ResumeLocation::AfterYield that pass the
exception-depth flag as true; change that boolean to false in both places so the
AfterYield resume points use false for the depth bit (i.e., replace true with
false for the third parameter in the oparg::ResumeContext::new(...) calls that
use ResumeLocation::AfterYield), ensuring the IR post-processing in ir.rs can
conditionally set the DEPTH1 bit correctly.

In `@crates/compiler-core/src/bytecode/oparg.rs`:
- Around line 798-807: Update the ResumeContext documentation links to point to
ResumeContext::location and ResumeContext::is_exception_depth1, and add an
explicit TryFrom<u32> impl for ResumeContext that rejects any u32 with bits
outside the allowed mask (LOCATION_MASK | DEPTH1_MASK); specifically, implement
ResumeContext::try_from(value: u32) to check (value & !ALLOWED_MASK) == 0 and
return Err for non-zero unknown bits, otherwise construct ResumeContext(value)
(replacing the implicit infallible conversion created by newtype_oparg!), so
invalid RESUME opargs are rejected rather than silently masked.

---

Duplicate comments:
In `@crates/compiler-core/src/bytecode/oparg.rs`:
- Around line 799-803: The intra-doc links point to non-existent symbols
`Context::location` and `Context::is_exception_depth1`; update them to reference
the actual type/fields on the resume context (e.g. `ResumeContext::location` and
`ResumeContext::is_exception_depth1`) so rustdoc can resolve the links and keep
the `Instruction::Resume` reference unchanged.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: e26b0201-e13f-41af-9fc1-b4e89d763c79

Reviewing files that changed from the base of the PR and between f18e0fe and d682618.

• crates/codegen/src/compile.rs
• crates/codegen/src/ir.rs
• crates/compiler-core/src/bytecode.rs
• crates/compiler-core/src/bytecode/instruction.rs
• crates/compiler-core/src/bytecode/oparg.rs
• crates/jit/src/instructions.rs
• crates/vm/src/frame.rs

• crates/vm/src/frame.rs

• crates/compiler-core/src/bytecode.rs
• crates/compiler-core/src/bytecode/instruction.rs

crates/codegen/src/compile.rs (1)

7599-7602: ⚠️ Potential issue | 🟠 Major

Stop hardcoding AfterYield depth in the generator-expression path.

Line 7601 still passes true here, so this one AfterYield site keeps deciding exception depth in codegen even though the rest of the file now delegates that to IR. It also diverges from the plain yield case at Line 7377.

🔧 Suggested fix

                         emit!(
                             compiler,
                             Instruction::Resume {
-                                context: oparg::ResumeContext::new(
-                                    oparg::ResumeLocation::AfterYield,
-                                    true, // TODO: Is this always true?
-                                )
+                                context: oparg::ResumeContext::from(
+                                    oparg::ResumeLocation::AfterYield,
+                                )
                             }
                         );

🤖 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 7599 - 7602, The
generator-expression path is still hardcoding oparg::ResumeContext::new(...,
oparg::ResumeLocation::AfterYield, true) which forces exception depth decision
in codegen; change it to delegate depth to the IR like the plain yield path does
by removing the literal true and using the IR-provided depth/flag (the same
value used when constructing ResumeContext for the plain yield case) so
ResumeContext::new(...) receives the IR-determined depth/flag instead of true,
and ensure you use oparg::ResumeLocation::AfterYield as before.

crates/compiler-core/src/bytecode/oparg.rs (1)

853-856: Make ResumeContext construction explicit.

ResumeLocation no longer determines the whole oparg once exception depth comes from IR. This From impl hard-codes false, so a stray .into() at a new emission site can silently drop the depth bit. Prefer ResumeContext::new(location, is_exception_depth1) at call sites.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/compiler-core/src/bytecode/oparg.rs` around lines 853 - 856, The
blanket impl "impl From<ResumeLocation> for ResumeContext" hard-codes the
exception-depth bit to false and allows stray .into() calls to drop the depth;
remove this From<ResumeLocation> for ResumeContext implementation and update
call sites to construct explicitly via ResumeContext::new(location,
is_exception_depth1) (using the correct boolean for exception depth) so the
depth bit is never implicitly lost; search for any uses of .into() or
ResumeLocation -> ResumeContext conversions and replace them with explicit
ResumeContext::new(...) calls.

crates/codegen/src/compile.rs (1)

7203-7205: Extract the ResumeLocation before building the context.

Lines 7203-7205 only vary by ResumeLocation; ResumeContext::from(...) is the shared part.

♻️ Suggested refactor

+        let resume_location = if is_await {
+            oparg::ResumeLocation::AfterAwait
+        } else {
+            oparg::ResumeLocation::AfterYieldFrom
+        };
+
         emit!(
             self,
             Instruction::Resume {
-                context: if is_await {
-                    oparg::ResumeContext::from(oparg::ResumeLocation::AfterAwait)
-                } else {
-                    oparg::ResumeContext::from(oparg::ResumeLocation::AfterYieldFrom)
-                }
+                context: oparg::ResumeContext::from(resume_location)
             }
         );

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".

🤖 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 7203 - 7205, Extract the
differing ResumeLocation into a local variable (e.g. let resume_loc =
oparg::ResumeLocation::AfterAwait or AfterYieldFrom based on the branch
condition) and then call oparg::ResumeContext::from(resume_loc) once instead of
repeating oparg::ResumeContext::from(...) in each branch; update the branches to
only set the resume_loc and perform the shared oparg::ResumeContext::from call
after the conditional.

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@crates/compiler-core/src/bytecode/oparg.rs`:
- Around line 706-718: The macro newtype_oparg currently generates only a const
fn from_u32(value: u32) -> Self and removes the legacy public new() constructor;
restore compatibility by emitting a public new(...) shim that forwards to
from_u32 (or define pub const fn new(value: u32) -> Self { Self::from_u32(value)
}), so every generated struct $name keeps the old new() API while still adding
from_u32(); update the impl block for $name inside newtype_oparg to include this
forwarding alias.

---

Duplicate comments:
In `@crates/codegen/src/compile.rs`:
- Around line 7599-7602: The generator-expression path is still hardcoding
oparg::ResumeContext::new(..., oparg::ResumeLocation::AfterYield, true) which
forces exception depth decision in codegen; change it to delegate depth to the
IR like the plain yield path does by removing the literal true and using the
IR-provided depth/flag (the same value used when constructing ResumeContext for
the plain yield case) so ResumeContext::new(...) receives the IR-determined
depth/flag instead of true, and ensure you use oparg::ResumeLocation::AfterYield
as before.

---

Nitpick comments:
In `@crates/codegen/src/compile.rs`:
- Around line 7203-7205: Extract the differing ResumeLocation into a local
variable (e.g. let resume_loc = oparg::ResumeLocation::AfterAwait or
AfterYieldFrom based on the branch condition) and then call
oparg::ResumeContext::from(resume_loc) once instead of repeating
oparg::ResumeContext::from(...) in each branch; update the branches to only set
the resume_loc and perform the shared oparg::ResumeContext::from call after the
conditional.

In `@crates/compiler-core/src/bytecode/oparg.rs`:
- Around line 853-856: The blanket impl "impl From<ResumeLocation> for
ResumeContext" hard-codes the exception-depth bit to false and allows stray
.into() calls to drop the depth; remove this From<ResumeLocation> for
ResumeContext implementation and update call sites to construct explicitly via
ResumeContext::new(location, is_exception_depth1) (using the correct boolean for
exception depth) so the depth bit is never implicitly lost; search for any uses
of .into() or ResumeLocation -> ResumeContext conversions and replace them with
explicit ResumeContext::new(...) calls.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: afe87ae8-07bf-461f-ae2d-7af8b581900f

Reviewing files that changed from the base of the PR and between fb7c109 and de5e1f6.

• crates/codegen/src/compile.rs
• crates/compiler-core/src/bytecode/oparg.rs

crates/compiler-core/src/bytecode/oparg.rs (1)

798-839: ⚠️ Potential issue | 🟠 Major

ResumeContext still accepts reserved RESUME bits as valid.

Because this type comes from newtype_oparg!, it gets From<u32>/from_u32, and Rust's blanket TryFrom<U> for T where U: Into<T> makes OpArgType::try_from effectively infallible here. Bits outside LOCATION_MASK | DEPTH1_MASK are therefore silently preserved and then ignored by location() / is_exception_depth1() instead of rejecting malformed bytecode. ResumeContext needs a hand-written definition or constructor path that drops the generic From<u32> decode path and validates raw values explicitly.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/compiler-core/src/bytecode/oparg.rs` around lines 798 - 839, The
ResumeContext type must reject raw u32 values with reserved bits set instead of
relying on the auto-derived From<u32> from newtype_oparg!, so replace the
infallible decode path with a validating constructor/TryFrom that ensures (value
& !(ResumeContext::LOCATION_MASK | ResumeContext::DEPTH1_MASK)) == 0.
Concretely: stop exposing/using the generic From<u32>/from_u32 for decoding
(either remove that impl or shadow it by adding a TryFrom<u32> implementation
that returns Err on reserved bits), add a public const fn (e.g. try_from_raw or
an impl TryFrom<u32> for ResumeContext) that validates the mask and returns
Err/None on invalid values, and update ResumeContext::new (and any callsites) to
construct via the new validated path while keeping From<ResumeContext>->u32 for
encoding if needed; keep location(), is_exception_depth1() as-is since they rely
on validated internal state.

crates/compiler-core/src/bytecode/oparg.rs (1)

706-718: This renames new(u32) for every public oparg newtype, not just Label.

newtype_oparg! is also used by ConstIdx, VarNum, VarNums, LoadAttr, LoadSuperAttr, Label, and ResumeContext, so if compiler-core is consumed outside the workspace this is a much broader source break than the PR description suggests. If that scope is not intentional, I'd keep the rename local to the types that need future typed constructors or plan a transition for downstream callers.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/compiler-core/src/bytecode/oparg.rs` around lines 706 - 718, The macro
newtype_oparg! currently renames the public constructor from new(u32) to
from_u32 for all generated types (ConstIdx, VarNum, VarNums, LoadAttr,
LoadSuperAttr, Label, ResumeContext), which is a breaking API change; restore a
pub const fn new(value: u32) -> Self in the macro so all generated newtypes keep
their original public constructor, and if you want a typed alternative for Label
add an extra from_u32 or from_label constructor only where Label is defined
rather than changing the macro output for every type.

crates/codegen/src/compile.rs (1)

7199-7205: Add a regression snapshot for IR-populated RESUME depth.

These emitters now intentionally leave the DEPTH1 bit to ir.rs. A small disassembly test that covers yield, yield from, and await from an exception-depth-1 path would make this cross-file contract much harder to regress.

Also applies to: 7374-7378, 7596-7602

🤖 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 7199 - 7205, Add a small
regression snapshot test that verifies the IR-populated RESUME depth behavior
when the DEPTH1 bit is intentionally left to ir.rs: create a disassembly test
that exercises yield, yield from, and await paths coming from an
exception-depth-1 path and assert the emitted Instruction::Resume uses
oparg::ResumeContext/ResumeLocation as expected (cover the same cases as the
emit! sites that use Instruction::Resume and
oparg::ResumeContext::from(oparg::ResumeLocation::AfterAwait/AfterYieldFrom));
place the test alongside existing compile/disassembly tests so it snapshots the
bytecode/disassembly output to prevent regressions across compile.rs and ir.rs.

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@crates/compiler-core/src/bytecode/oparg.rs`:
- Around line 798-839: The ResumeContext type must reject raw u32 values with
reserved bits set instead of relying on the auto-derived From<u32> from
newtype_oparg!, so replace the infallible decode path with a validating
constructor/TryFrom that ensures (value & !(ResumeContext::LOCATION_MASK |
ResumeContext::DEPTH1_MASK)) == 0. Concretely: stop exposing/using the generic
From<u32>/from_u32 for decoding (either remove that impl or shadow it by adding
a TryFrom<u32> implementation that returns Err on reserved bits), add a public
const fn (e.g. try_from_raw or an impl TryFrom<u32> for ResumeContext) that
validates the mask and returns Err/None on invalid values, and update
ResumeContext::new (and any callsites) to construct via the new validated path
while keeping From<ResumeContext>->u32 for encoding if needed; keep location(),
is_exception_depth1() as-is since they rely on validated internal state.

---

Nitpick comments:
In `@crates/codegen/src/compile.rs`:
- Around line 7199-7205: Add a small regression snapshot test that verifies the
IR-populated RESUME depth behavior when the DEPTH1 bit is intentionally left to
ir.rs: create a disassembly test that exercises yield, yield from, and await
paths coming from an exception-depth-1 path and assert the emitted
Instruction::Resume uses oparg::ResumeContext/ResumeLocation as expected (cover
the same cases as the emit! sites that use Instruction::Resume and
oparg::ResumeContext::from(oparg::ResumeLocation::AfterAwait/AfterYieldFrom));
place the test alongside existing compile/disassembly tests so it snapshots the
bytecode/disassembly output to prevent regressions across compile.rs and ir.rs.

In `@crates/compiler-core/src/bytecode/oparg.rs`:
- Around line 706-718: The macro newtype_oparg! currently renames the public
constructor from new(u32) to from_u32 for all generated types (ConstIdx, VarNum,
VarNums, LoadAttr, LoadSuperAttr, Label, ResumeContext), which is a breaking API
change; restore a pub const fn new(value: u32) -> Self in the macro so all
generated newtypes keep their original public constructor, and if you want a
typed alternative for Label add an extra from_u32 or from_label constructor only
where Label is defined rather than changing the macro output for every type.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: dbc82573-27ca-407e-a5e3-68c47095373a

Reviewing files that changed from the base of the PR and between de5e1f6 and 07d019a.

• crates/codegen/src/compile.rs
• crates/compiler-core/src/bytecode/oparg.rs