SetFunctionAttribute - #5968
Conversation
Sorry, something went wrong.
|
Warning Rate limit exceeded@youknowone has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 13 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📥 CommitsReviewing files that changed from the base of the PR and between c8a4e19 and 1dd0f57. 📒 Files selected for processing (6)
WalkthroughThe changes restructure function and closure compilation to match CPython 3.13 semantics. Function default arguments are compiled before entering the function scope. Function creation and attribute setting are now split into separate bytecode instructions, with a new SetFunctionAttribute instruction. Helper methods and enum variants are introduced to support symbol scope and type parameter handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Compiler
participant VM
participant Stack
participant PyFunction
Compiler->Stack: Push default arg values (if any)
Compiler->VM: Emit MakeFunction (with code object)
VM->Stack: Pop code object
VM->PyFunction: Create function object
loop For each attribute (closure, defaults, etc.)
Compiler->VM: Emit SetFunctionAttribute (attr flag)
VM->Stack: Pop function, pop value
VM->PyFunction: Set attribute based on flag
VM->Stack: Push function back
end
Possibly related PRs
Suggested reviewers
Poem✨ Finishing Touches
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 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)vm/src/frame.rs (1)📜 Review details1836-1837: Consider documenting the breaking API change.
The _flags parameter is now unused, which represents a breaking change in the bytecode format. This should be documented in the migration guide or changelog to help users understand that the compiler no longer passes flags to MakeFunction.
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Reviewing files that changed from the base of the PR and between 45f8c3a and a5291c5.
⛔ Files ignored due to path filters (3)- Lib/test/test_funcattrs.py is excluded by !Lib/**
- Lib/test/test_reprlib.py is excluded by !Lib/**
- Lib/test/test_typing.py is excluded by !Lib/**
- compiler/codegen/src/compile.rs (12 hunks)
- compiler/codegen/src/symboltable.rs (1 hunks)
- compiler/core/src/bytecode.rs (3 hunks)
- jit/tests/common.rs (1 hunks)
- vm/src/builtins/function.rs (2 hunks)
- vm/src/frame.rs (3 hunks)
- vm/src/object/core.rs (2 hunks)
- compiler/codegen/src/symboltable.rs
- jit/tests/common.rs
- vm/src/object/core.rs
- vm/src/builtins/function.rs
- compiler/core/src/bytecode.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .github/copilot-instructions.md
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
vm/src/frame.rs (2)1165-1167: LGTM!
The new instruction case correctly delegates to the execute_set_function_attribute method.
1886-1890: Confirmed set_function_attribute implementation
compiler/codegen/src/compile.rs (6)
The set_function_attribute method is defined on PyFunction in vm/src/builtins/function.rs, so no missing definition or import issues remain.1485-1524: LGTM: Clean separation of concerns in function entry.
The refactored enter_function method now focuses solely on setting up the function scope and registering parameter names, which is a good separation of concerns. The removal of default argument compilation from this method aligns with the PR's goal of compiling defaults before entering the function scope.
1828-1878: Well-structured default argument compilation.
The new approach of compiling default arguments before entering the function scope is well-implemented. The code correctly handles both positional and keyword-only defaults, building appropriate tuples and maps for the bytecode.
1991-2013: Excellent helper method for symbol scope resolution.
The get_ref_type method provides a clean abstraction for determining whether a variable should be CELL or FREE type. The special handling for __class__ and __classdict__ in class scopes is correctly implemented, and the error handling is appropriate.
2017-2164: Excellent refactoring to match CPython 3.13 semantics.
The make_closure method has been thoroughly refactored to follow CPython 3.13's approach of creating the function first, then setting attributes individually. Key improvements:
- Proper handling of closure variables with get_ref_type
- Clear separation between function creation and attribute setting
- Correct order of operations for SetFunctionAttribute instructions
- Good error handling for variable lookups
The logic for finding variables in parent cellvars/freevars with fallback lookups is particularly well-implemented.
4131-4182: Consistent lambda compilation with function compilation.
The lambda compilation changes mirror the function compilation approach, correctly compiling default arguments before entering the function scope. The consistency between lambda and regular function compilation is good for maintainability.
1974-1974: Proper removal of qualname parameter.
All calls to make_closure have been correctly updated to remove the qualname parameter, which aligns with the method signature changes. The qualified name handling is now properly managed within the make_closure method itself.
Also applies to: 2364-2364, 2413-2413, 2420-2420, 2748-2748, 4203-4203
Sorry, something went wrong.
| let code_obj: PyRef<PyCode> = self | ||
| .pop_value() | ||
| .downcast() | ||
| .expect("Second to top value on the stack must be a code object"); | ||
|
|
||
| let closure = if flags.contains(bytecode::MakeFunctionFlags::CLOSURE) { | ||
| let tuple = PyTupleRef::try_from_object(vm, self.pop_value()).unwrap(); | ||
| Some(tuple.try_into_typed(vm).expect("This is a compiler bug")) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| let annotations = if flags.contains(bytecode::MakeFunctionFlags::ANNOTATIONS) { | ||
| self.pop_value() | ||
| } else { | ||
| vm.ctx.new_dict().into() | ||
| }; | ||
|
|
||
| let type_params: PyTupleRef = if flags.contains(bytecode::MakeFunctionFlags::TYPE_PARAMS) { | ||
| self.pop_value() | ||
| .downcast() | ||
| .map_err(|_| vm.new_type_error("Type params must be a tuple."))? | ||
| } else { | ||
| vm.ctx.empty_tuple.clone() | ||
| }; | ||
|
|
||
| let kw_only_defaults = if flags.contains(bytecode::MakeFunctionFlags::KW_ONLY_DEFAULTS) { | ||
| Some( | ||
| self.pop_value() | ||
| .downcast::<PyDict>() | ||
| .expect("Stack value for keyword only defaults expected to be a dict"), | ||
| ) | ||
| } else { | ||
| None | ||
| }; | ||
| .expect("Stack value should be code object"); |
There was a problem hiding this comment.
⚠️ Potential issue
Replace expect with proper error handling.
Using expect here could cause a panic if the compiler generates incorrect bytecode. Consider returning a proper error instead.
- let code_obj: PyRef<PyCode> = self
- .pop_value()
- .downcast()
- .expect("Stack value should be code object");
+ let code_obj: PyRef<PyCode> = self
+ .pop_value()
+ .downcast()
+ .map_err(|_| vm.new_type_error("MakeFunction expects code object on stack"))?;In vm/src/frame.rs around lines 1839 to 1842, replace the use of expect on the downcast result with proper error handling to avoid panics. Instead of calling expect, check if the downcast returns Some or None, and if None, return an appropriate error from the function indicating the stack value was not a code object. This ensures safer handling of potentially incorrect bytecode without panicking.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)vm/src/frame.rs (2)🧹 Nitpick comments (2)1833-1836: Replace expect with proper error handling.
Using expect here could cause a panic if the compiler generates incorrect bytecode. Consider returning a proper error instead.
- let code_obj: PyRef<PyCode> = self - .pop_value() - .downcast() - .expect("Stack value should be code object"); + let code_obj: PyRef<PyCode> = self + .pop_value() + .downcast() + .map_err(|_| vm.new_type_error("MakeFunction expects code object on stack"))?;
1870-1876: Critical: Unsafe mutation violates Rust's aliasing rules.
This unsafe block casts a *const pointer to *mut and mutates through it, which violates Rust's aliasing rules. The assumption that "there are no other references to func" is fragile and could be violated by:
- Malicious or malformed bytecode
- Compiler bugs
- Future changes to bytecode generation
This could lead to undefined behavior and memory safety issues.
Consider safer alternatives:
- Use interior mutability (e.g., RefCell or Cell) in the PyFunction type for mutable fields
- Create a new function object with the updated attributes
- Add a safe mutation API that checks for unique ownership
Here's a safer approach using interior mutability:
// In PyFunction definition, wrap mutable fields: // pub defaults: RefCell<Option<PyTupleRef>>, // pub kwdefaults: RefCell<Option<PyDictRef>>, // etc. // Then in execute_set_function_attribute: func_ref.payload().set_function_attribute(attr, attr_value, vm)?;
compiler/codegen/src/compile.rs (1)📜 Review detailsvm/src/builtins/function.rs (1)1828-1879: Correctly implements CPython 3.13 default argument compilation.
The logic properly compiles default arguments before entering function scope, which is a key requirement for CPython 3.13 semantics. The handling of both positional and keyword-only defaults is correct, and the flag setting is appropriate.
Consider extracting the default argument compilation logic into separate helper methods for better readability:
+ fn compile_positional_defaults(&mut self, parameters: &Parameters) -> CompileResult<bool> { + // Extract lines 1829-1844 logic here + } + + fn compile_kwonly_defaults(&mut self, parameters: &Parameters) -> CompileResult<bool> { + // Extract lines 1846-1869 logic here + }This would make the main function definition method more readable and modular.
315-373: Optimize attribute setting logic and remove unnecessary clones
The set_function_attribute method correctly implements type checking and validation. However, there are optimization opportunities:
- Unnecessary .clone() calls can be avoided by moving values directly
- The use of contains() with multiple if statements suggests only one flag should be set at a time, but this isn't enforced
Apply this diff to optimize the implementation:
- if attr.contains(bytecode::MakeFunctionFlags::DEFAULTS) { - let defaults = attr_value.clone().downcast::<PyTuple>().map_err(|_| { + if attr.contains(bytecode::MakeFunctionFlags::DEFAULTS) { + let defaults = attr_value.downcast::<PyTuple>().map_err(|_| { vm.new_type_error(format!( "__defaults__ must be a tuple, not {}", attr_value.class().name() )) })?; self.defaults_and_kwdefaults.lock().0 = Some(defaults); - } else if attr.contains(bytecode::MakeFunctionFlags::KW_ONLY_DEFAULTS) { - let kwdefaults = attr_value.clone().downcast::<PyDict>().map_err(|_| { + } else if attr.contains(bytecode::MakeFunctionFlags::KW_ONLY_DEFAULTS) { + let kwdefaults = attr_value.downcast::<PyDict>().map_err(|_| { vm.new_type_error(format!( "__kwdefaults__ must be a dict, not {}", attr_value.class().name() )) })?; self.defaults_and_kwdefaults.lock().1 = Some(kwdefaults); - } else if attr.contains(bytecode::MakeFunctionFlags::ANNOTATIONS) { - let annotations = attr_value.clone().downcast::<PyDict>().map_err(|_| { + } else if attr.contains(bytecode::MakeFunctionFlags::ANNOTATIONS) { + let annotations = attr_value.downcast::<PyDict>().map_err(|_| { vm.new_type_error(format!( "__annotations__ must be a dict, not {}", attr_value.class().name() )) })?; *self.annotations.lock() = annotations; } else if attr.contains(bytecode::MakeFunctionFlags::CLOSURE) { let closure_tuple = attr_value - .clone() .downcast_exact::<PyTuple>(vm) .map_err(|obj| { vm.new_type_error(format!( "closure must be a tuple, not {}", obj.class().name() )) })? .into_pyref(); self.closure = Some(closure_tuple.try_into_typed::<PyCell>(vm)?); - } else if attr.contains(bytecode::MakeFunctionFlags::TYPE_PARAMS) { - let type_params = attr_value.clone().downcast::<PyTuple>().map_err(|_| { + } else if attr.contains(bytecode::MakeFunctionFlags::TYPE_PARAMS) { + let type_params = attr_value.downcast::<PyTuple>().map_err(|_| { vm.new_type_error(format!( "__type_params__ must be a tuple, not {}", attr_value.class().name() )) })?; *self.type_params.lock() = type_params; }Consider also adding a debug assertion to ensure only one flag is set at a time, if that's the intended behavior:
+ debug_assert_eq!(attr.bits().count_ones(), 1, "Only one attribute flag should be set at a time");
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Reviewing files that changed from the base of the PR and between a5291c5 and b0c61c6.
📒 Files selected for processing (6)- compiler/codegen/src/compile.rs (12 hunks)
- compiler/core/src/bytecode.rs (3 hunks)
- jit/tests/common.rs (1 hunks)
- vm/src/builtins/function.rs (4 hunks)
- vm/src/frame.rs (2 hunks)
- vm/src/vm/mod.rs (1 hunks)
- vm/src/vm/mod.rs
- jit/tests/common.rs
- compiler/core/src/bytecode.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .github/copilot-instructions.md
Learnt from: CR PR: RustPython/RustPython#0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-06-30T10:08:48.858Z Learning: In most cases, Python code should not be edited. Bug fixes should be made through Rust code modifications only
Learnt from: CR PR: RustPython/RustPython#0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-06-30T10:08:48.858Z Learning: Applies to **/*.rs : Follow Rust best practices for error handling and memory management
Learnt from: CR PR: RustPython/RustPython#0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-06-30T10:08:48.858Z Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Ensure compilation on various targets
compiler/codegen/src/compile.rs (4)1485-1524: LGTM! Function simplification aligns with CPython 3.13 semantics.
The enter_function method has been correctly simplified to only handle function scope setup and argument registration. The removal of default argument compilation logic is intentional and aligns with the new approach where defaults are compiled before entering function scope.
1991-2013: Well-implemented helper method for symbol scope determination.
The get_ref_type method correctly handles the special cases for __class__ and __classdict__ in class scopes, and properly maps symbol scopes to the appropriate CELL or FREE types. The error handling is also appropriate.
4128-4200: Lambda compilation correctly mirrors function definition changes.
The lambda compilation changes properly follow the same pattern as function definitions - compiling defaults before entering function scope and using the updated make_closure method. This maintains consistency across the codebase.
2015-2161: All MakeFunctionFlags variants are emitted and handled correctly
The make_closure implementation in compiler/codegen/src/compile.rs emits SetFunctionAttribute for each of the five flags (CLOSURE, ANNOTATIONS, KW_ONLY_DEFAULTS, DEFAULTS, TYPE_PARAMS), and vm/src/builtins/function.rs consumes each variant in its handler.
Please verify that:
vm/src/builtins/function.rs (3)
- The order of SetFunctionAttribute instructions in compile.rs (currently: CLOSURE → ANNOTATIONS → KW_ONLY_DEFAULTS → DEFAULTS → TYPE_PARAMS) matches CPython 3.13’s expected attribute‐setting sequence.
42-43: LGTM: Proper feature flag placement
The repositioning of the jitted_code field under the #[cfg(feature = "jit")] attribute is correctly implemented and maintains proper conditional compilation.
74-89: LGTM: Well-documented CPython 3.13 style constructor
The simplified constructor correctly implements the minimal initialization approach. The comments clearly explain the rationale behind setting attributes to defaults, with the expectation that they'll be set later via SET_FUNCTION_ATTRIBUTE.
680-698: LGTM: Proper implementation of CPython 3.13 style function construction
The Constructor implementation correctly follows the new pattern of creating a minimal function object and then setting additional attributes directly. The attribute setting logic is sound and properly handles the transition from constructor parameters to direct field assignment.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)compiler/codegen/src/compile.rs (1)📜 Review details2017-2161: Excellent implementation of CPython 3.13-style function creation.
The rewritten make_closure method correctly implements the new approach of creating a minimal function object first, then setting attributes individually. The defensive fallback logic for variable lookups ensures robustness.
Minor suggestion: Consider making the error messages in lines 2051-2053 and 2067-2069 more specific by indicating whether the lookup failed in cellvars or freevars.
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Reviewing files that changed from the base of the PR and between b0c61c6 and c7c2e27.
📒 Files selected for processing (6)- compiler/codegen/src/compile.rs (12 hunks)
- compiler/core/src/bytecode.rs (3 hunks)
- jit/tests/common.rs (1 hunks)
- vm/src/builtins/function.rs (4 hunks)
- vm/src/frame.rs (2 hunks)
- vm/src/vm/mod.rs (1 hunks)
- jit/tests/common.rs
- vm/src/frame.rs
- vm/src/vm/mod.rs
- compiler/core/src/bytecode.rs
- vm/src/builtins/function.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .github/copilot-instructions.md
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Ensure compilation on various targets
compiler/codegen/src/compile.rs (5)1485-1525: LGTM! Clean separation of concerns.
The refactoring to remove default argument compilation from enter_function improves code organization by separating scope entry from argument processing. This aligns well with the new bytecode structure where defaults are compiled before entering the function scope.
1829-1879: Correct implementation of CPython 3.13 function compilation semantics.
The restructuring properly compiles default arguments in the parent scope before entering the function scope. This matches CPython 3.13's approach where defaults are evaluated outside the function's local scope.
1991-2013: Well-designed helper method for reference type determination.
The get_ref_type method correctly implements CPython's logic for determining whether a variable should be a CELL or FREE reference, with proper special-case handling for class-related variables.
4128-4179: Lambda compilation correctly mirrors function compilation changes.
The restructuring of lambda compilation to evaluate defaults before entering the function scope is consistent with the changes to compile_function_def and properly implements CPython 3.13 semantics.
1974-1974: All make_closure call sites correctly updated.
The updates to remove the qualname parameter and adjust the flags parameter across all call sites are consistent with the new make_closure implementation.
Also applies to: 2361-2361, 2410-2410, 2417-2417, 4200-4200, 4745-4745
Sorry, something went wrong.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes