Adapt bitflagset to use enum#7419
Conversation
📝 WalkthroughWalkthroughReplace bitflags-based Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Sorry, something went wrong.
moreal
left a comment
There was a problem hiding this comment.
It looks better 👍🏻
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/codegen/src/compile.rs (1)
4344-4400: Collapse the repeatedSetFunctionAttributebranches into an ordered loop.These branches only vary by
MakeFunctionFlagand duplicate the same emission logic. A small ordered list keeps the application order explicit and makes future flag additions easier to maintain.♻️ Proposed refactor
if has_freevars { emit!( self, Instruction::SetFunctionAttribute { flag: bytecode::MakeFunctionFlag::Closure } ); } - // Set annotations if present - if flags.contains(&bytecode::MakeFunctionFlag::Annotations) { - emit!( - self, - Instruction::SetFunctionAttribute { - flag: bytecode::MakeFunctionFlag::Annotations - } - ); - } - - // Set __annotate__ closure if present (PEP 649) - if flags.contains(&bytecode::MakeFunctionFlag::Annotate) { - emit!( - self, - Instruction::SetFunctionAttribute { - flag: bytecode::MakeFunctionFlag::Annotate - } - ); - } - - // Set kwdefaults if present - if flags.contains(&bytecode::MakeFunctionFlag::KwOnlyDefaults) { - emit!( - self, - Instruction::SetFunctionAttribute { - flag: bytecode::MakeFunctionFlag::KwOnlyDefaults - } - ); - } - - // Set defaults if present - if flags.contains(&bytecode::MakeFunctionFlag::Defaults) { - emit!( - self, - Instruction::SetFunctionAttribute { - flag: bytecode::MakeFunctionFlag::Defaults - } - ); - } - - // Set type_params if present - if flags.contains(&bytecode::MakeFunctionFlag::TypeParams) { - emit!( - self, - Instruction::SetFunctionAttribute { - flag: bytecode::MakeFunctionFlag::TypeParams - } - ); - } + for flag in [ + bytecode::MakeFunctionFlag::Annotations, + bytecode::MakeFunctionFlag::Annotate, + bytecode::MakeFunctionFlag::KwOnlyDefaults, + bytecode::MakeFunctionFlag::Defaults, + bytecode::MakeFunctionFlag::TypeParams, + ] { + if flags.contains(&flag) { + emit!(self, Instruction::SetFunctionAttribute { flag }); + } + }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 4344 - 4400, The repeated branches setting SetFunctionAttribute for each MakeFunctionFlag should be replaced with a single ordered iteration: create an ordered array of the flags (e.g., bytecode::MakeFunctionFlag::Closure, ::Annotations, ::Annotate, ::KwOnlyDefaults, ::Defaults, ::TypeParams), then loop over that array and for each flag call if flags.contains(&flag) { emit!(self, Instruction::SetFunctionAttribute { flag }) }; this preserves the explicit order, removes duplicated emit logic, and keeps the behavior of SetFunctionAttribute in one place.
🤖 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/jit/tests/common.rs`:
- Around line 276-279: The wildcard match arm in crates/jit/tests/common.rs
currently treats all remaining MakeFunctionFlag variants as no-ops and simply
pushes StackValue::Function(func), which ignores Closure, Defaults,
KwOnlyDefaults, and TypeParams and diverges from the VM behavior; update the
match in the function that assembles functions to either implement handling for
MakeFunctionFlag::Closure, ::Defaults, ::KwOnlyDefaults, and ::TypeParams to
mutate the function state the same way as crates/vm/src/builtins/function.rs
does, or replace the wildcard arm with an explicit panic/unreachable for
unhandled MakeFunctionFlag variants so tests fail-fast (refer to the
MakeFunctionFlag enum, the code that constructs a
Function/StackValue::Function(func), and the function-building logic in this
module to mirror the VM behavior).
---
Nitpick comments:
In `@crates/codegen/src/compile.rs`:
- Around line 4344-4400: The repeated branches setting SetFunctionAttribute for
each MakeFunctionFlag should be replaced with a single ordered iteration: create
an ordered array of the flags (e.g., bytecode::MakeFunctionFlag::Closure,
::Annotations, ::Annotate, ::KwOnlyDefaults, ::Defaults, ::TypeParams), then
loop over that array and for each flag call if flags.contains(&flag) {
emit!(self, Instruction::SetFunctionAttribute { flag }) }; this preserves the
explicit order, removes duplicated emit logic, and keeps the behavior of
SetFunctionAttribute in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 7db04139-be6a-4f0b-aa40-f367ec2f8c46
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.cspell.dict/rust-more.txtCargo.tomlcrates/codegen/src/compile.rscrates/compiler-core/Cargo.tomlcrates/compiler-core/src/bytecode.rscrates/compiler-core/src/bytecode/instruction.rscrates/compiler-core/src/bytecode/oparg.rscrates/jit/tests/common.rscrates/vm/src/builtins/function.rscrates/vm/src/frame.rs
Sorry, something went wrong.
be6025a
into
RustPython:main
Mar 14, 2026
Summary by CodeRabbit