Skip to content

Adapt bitflagset to use enum#7419

Merged
youknowone merged 1 commit intoRustPython:mainfrom
youknowone:bitflagset
Mar 14, 2026
Merged

Adapt bitflagset to use enum#7419
youknowone merged 1 commit intoRustPython:mainfrom
youknowone:bitflagset

Conversation

@youknowone
Copy link
Member

@youknowone youknowone commented Mar 13, 2026

Summary by CodeRabbit

  • Refactor
    • Optimized internal flag handling system for improved maintainability and code organization.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 13, 2026

📝 Walkthrough

Walkthrough

Replace bitflags-based MakeFunctionFlags with a bitflagset-based MakeFunctionFlag enum and wrapper, updating all flag-handling call sites across bytecode definitions, codegen, VM execution, and JIT tests to adopt the new flag API.

Changes

Cohort / File(s) Summary
Dependencies
Cargo.toml, crates/compiler-core/Cargo.toml, .cspell.dict/rust-more.txt
Added bitflagset v0.0.3 as a workspace dependency and updated spell-check dictionary with "bitflagset".
Bytecode Core
crates/compiler-core/src/bytecode/oparg.rs
Replaced bitflags-based MakeFunctionFlags struct with a bitflagset-backed enum MakeFunctionFlag (variants: Closure, Annotations, KwOnlyDefaults, Defaults, TypeParams, Annotate) and lightweight wrapper type; updated TryFrom<u32> and From<MakeFunctionFlag> trait implementations.
Bytecode Exports
crates/compiler-core/src/bytecode.rs, crates/compiler-core/src/bytecode/instruction.rs
Added MakeFunctionFlag to public re-exports; updated SetFunctionAttribute instruction to use Arg<MakeFunctionFlag> instead of Arg<MakeFunctionFlags>.
Codegen
crates/codegen/src/compile.rs
Replaced all MakeFunctionFlags::empty() with MakeFunctionFlags::new() and migrated from bitwise |= operators to .insert() calls with new enum variants; updated all flag checks to use contains() with enum variants.
VM Execution
crates/vm/src/builtins/function.rs, crates/vm/src/frame.rs
Updated set_function_attribute and execute_set_function_attribute method signatures to accept MakeFunctionFlag; refactored match-based flag handling to directly pattern-match on enum variants with corresponding attribute logic (defaults, annotations, closure, type parameters).
JIT Tests
crates/jit/tests/common.rs
Refactored SET_FUNCTION_ATTRIBUTE handling to use match-based branching on MakeFunctionFlag variants, replacing previous flag.contains() conditionals.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • SetFunctionAttribute #5968: Directly related refactoring that modifies the same function-creation and attribute-setting codepaths by replacing MakeFunctionFlags bitflags with MakeFunctionFlag enum across bytecode, codegen, and VM layers.
  • Move OpArg to its own file #6703: Related change that modifies the bytecode oparg surface, specifically the MakeFunctionFlags/MakeFunctionFlag type definitions and their public exports.

Suggested reviewers

  • ShaharNaveh

Poem

Through bitflags old to flags so new,
Enum variants dance in vibrant hue,
Each bit now bears a clearer name,
Closures, annotations—all the same,
A rabbit hops through cleaner code! 🐰✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 53.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: migration from bitflags-based MakeFunctionFlags to a bitflagset-based enum approach (MakeFunctionFlag) with wrapper type.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

.clone()
.downcast_exact::<PyTuple>(vm)
.map_err(|obj| {
match attr {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better for this use case

Copy link
Contributor

@moreal moreal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks better 👍🏻

@youknowone youknowone marked this pull request as ready for review March 13, 2026 12:50
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
crates/codegen/src/compile.rs (1)

4344-4400: Collapse the repeated SetFunctionAttribute branches into an ordered loop.

These branches only vary by MakeFunctionFlag and 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

📥 Commits

Reviewing files that changed from the base of the PR and between 073adbd and 3f93b22.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (10)
  • .cspell.dict/rust-more.txt
  • Cargo.toml
  • crates/codegen/src/compile.rs
  • crates/compiler-core/Cargo.toml
  • crates/compiler-core/src/bytecode.rs
  • crates/compiler-core/src/bytecode/instruction.rs
  • crates/compiler-core/src/bytecode/oparg.rs
  • crates/jit/tests/common.rs
  • crates/vm/src/builtins/function.rs
  • crates/vm/src/frame.rs

Comment on lines +276 to 279
_ => {
// For other attributes, just push the function back unchanged
self.stack.push(StackValue::Function(func));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
_ => {
// 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).

@youknowone youknowone merged commit be6025a into RustPython:main Mar 14, 2026
39 of 41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants