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 |
| .clone() | ||
| .downcast_exact::<PyTuple>(vm) | ||
| .map_err(|obj| { | ||
| match attr { |
There was a problem hiding this comment.
better for this use case
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
| _ => { | ||
| // For other attributes, just push the function back unchanged | ||
| self.stack.push(StackValue::Function(func)); | ||
| } |
There was a problem hiding this comment.
Don't treat the remaining MakeFunctionFlag cases as no-ops.
The wildcard arm at Line 276 drops Closure, Defaults, KwOnlyDefaults, and TypeParams, while the real VM path in crates/vm/src/builtins/function.rs mutates state for all four. That means this JIT test harness can construct functions whose setup no longer matches production, which can hide regressions instead of surfacing them. Please either model those variants here or fail fast when they appear.
🛠️ Fail-fast alternative
- _ => {
- // For other attributes, just push the function back unchanged
- self.stack.push(StackValue::Function(func));
- }
+ rustpython_compiler_core::bytecode::MakeFunctionFlag::Closure
+ | rustpython_compiler_core::bytecode::MakeFunctionFlag::Defaults
+ | rustpython_compiler_core::bytecode::MakeFunctionFlag::KwOnlyDefaults
+ | rustpython_compiler_core::bytecode::MakeFunctionFlag::TypeParams => {
+ panic!("Unsupported SET_FUNCTION_ATTRIBUTE flag in JIT test harness");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _ => { | |
| // For other attributes, just push the function back unchanged | |
| self.stack.push(StackValue::Function(func)); | |
| } | |
| rustpython_compiler_core::bytecode::MakeFunctionFlag::Closure | |
| | rustpython_compiler_core::bytecode::MakeFunctionFlag::Defaults | |
| | rustpython_compiler_core::bytecode::MakeFunctionFlag::KwOnlyDefaults | |
| | rustpython_compiler_core::bytecode::MakeFunctionFlag::TypeParams => { | |
| panic!("Unsupported SET_FUNCTION_ATTRIBUTE flag in JIT test harness"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/jit/tests/common.rs` around lines 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).
Summary by CodeRabbit