-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Expand arg tuple handling to 12 and IntoPyNativeFn to 11 params #6101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughExpanded argument binding and native function wrapper generation to support higher-arity functions: argument tuple handling increased to 12 in vm/src/function/argument.rs, and IntoPyNativeFn macro coverage extended up to 11 parameters in vm/src/function/builtin.rs. Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesPoem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Nitpick comments (1)
vm/src/function/builtin.rs (1)
218-266
: Add a regression test for high arity (11–12) function wrappingNo tests exercise the newly supported higher arities. A small compile-time/use test would prevent regressions when refactoring these macros.
I can draft a test that defines simple functions with 11 and 12 parameters (with and without &VirtualMachine) and asserts they can be turned into PyNativeFn via into_func/static_func. Want me to push a snippet?
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
vm/src/function/argument.rs
(3 hunks)vm/src/function/builtin.rs
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.rs
: Follow the default rustfmt code style (cargo fmt
to format)
Always run clippy to lint code (cargo clippy
) before completing tasks. Fix any warnings or lints that are introduced by your changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass
,pymodule
,pyfunction
, etc.) when implementing Python functionality in Rust
Files:
vm/src/function/argument.rs
vm/src/function/builtin.rs
🧬 Code Graph Analysis (1)
vm/src/function/builtin.rs (1)
extra_tests/snippets/syntax_class.py (5)
T1
(135-136)T2
(140-141)T3
(145-148)T4
(152-158)T5
(168-169)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- 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 tests under miri
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
🔇 Additional comments (6)
vm/src/function/argument.rs (6)
58-67
: Arity 7 IntoFuncArgs tuple coverage looks correctMirrors existing pattern; consistent with earlier arities.
68-77
: Arity 8 IntoFuncArgs tuple coverage looks correct
79-89
: Arity 9 IntoFuncArgs tuple coverage looks correct
91-102
: Arity 10 IntoFuncArgs tuple coverage looks correct
104-116
: Arity 11 IntoFuncArgs tuple coverage looks correct
666-669
: FromArgs tuple coverage extended to 12 — LGTMArity aggregation logic remains correct; min/max sums over component arities. This is the right underpinning for high-arity signatures.
into_func_args_from_tuple!( | ||
(v1, T1), | ||
(v2, T2), | ||
(v3, T3), | ||
(v4, T4), | ||
(v5, T5), | ||
(v6, T6), | ||
(v7, T7), | ||
(v8, T8), | ||
(v9, T9), | ||
(v10, T10), | ||
(v11, T11), | ||
(v12, T12) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Arity 12 IntoFuncArgs tuple coverage looks correct; ensure builtin.rs is kept in sync
This tops out IntoFuncArgs at 12; builtin.rs currently only wraps up to 11. Please add a 12-arity wrapper there to prevent a mismatch.
I included a diff for builtin.rs in my other comment.
🤖 Prompt for AI Agents
In vm/src/function/argument.rs around lines 118 to 131 the IntoFuncArgs macro is
extended to support 12-arity tuples, but vm/src/builtin.rs currently only
provides wrappers up to 11; add a corresponding 12-arity wrapper in
vm/src/builtin.rs that matches the existing 11-arity pattern (same function
signature shape, trait bounds and conversion logic), update any relevant use
sites to accept the 12-tuple, and run the build/tests to ensure the new wrapper
is compiled and keeps both files in sync.
// Implement `FromArgs` for up to 12-tuples, allowing built-in functions to bind | ||
// up to 12 top-level parameters (note that `PosArgs`, `KwArgs`, nested tuples, etc. | ||
// count as 1, so this should actually be more than enough). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Doc note updated to 12-tuples — align all generators accordingly
The doc now claims support up to 12. builtin.rs must match (currently missing 12), or the doc will be misleading for #[pymethod]/#[pyfunction] authors hitting 12 parameters.
Would you like me to open a follow-up to extend both modules to 16 or 20 and add a small compile test to keep them in lockstep?
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Snippet around tuple support in vm/src/function/argument.rs ==="
sed -n '640,700p' vm/src/function/argument.rs || echo "Failed to read argument.rs snippet"
echo
echo "=== Locating builtin.rs ==="
builtin_file=$(fd --type f builtin.rs | head -n1)
if [ -z "$builtin_file" ]; then
echo "builtin.rs not found"
exit 1
else
echo "Found builtin.rs at: $builtin_file"
fi
echo
echo "=== First 200 lines of builtin.rs ==="
sed -n '1,200p' "$builtin_file" || echo "Failed to read builtin.rs snippet"
Length of output: 9310
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Searching for invocation of into_py_native_fn_tuple! in vm/src/function/builtin.rs ==="
rg -n "into_py_native_fn_tuple!" vm/src/function/builtin.rs || echo "No invocations found"
Length of output: 833
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Snippet of into_py_native_fn_tuple! invocations in builtin.rs ==="
sed -n '180,300p' vm/src/function/builtin.rs || echo "Failed to read snippet"
Length of output: 2440
builtin.rs tuple support is out of sync with the “12-tuples” doc note
The docs in vm/src/function/argument.rs
now advertise support for up to 12 positional parameters, but in vm/src/function/builtin.rs
the into_py_native_fn_tuple!
macro is only instantiated through 11 parameters. To avoid confusing #[pymethod]/#[pyfunction] authors and prevent compile-time surprises:
• In vm/src/function/builtin.rs
, add a 12-parameter invocation:
into_py_native_fn_tuple!(
(v1, T1), …, (v11, T11), (v12, T12)
);
• Add a small compile-test (e.g., bind a 12-argument dummy fn) to both modules to catch any future drift.
• Optionally, consider extending both tuple_from_py_func_args!
(argument.rs) and into_py_native_fn_tuple!
(builtin.rs) to 16 or 20 parameters in one follow-up PR so they remain in lockstep.
🤖 Prompt for AI Agents
In vm/src/function/argument.rs around lines 655-657 the doc comment states
support for up to 12-tuples but vm/src/function/builtin.rs is only instantiated
through 11 parameters; update vm/src/function/builtin.rs to add the missing
12-parameter invocation of into_py_native_fn_tuple! (i.e., add the (v12, T12)
arm), and add a small compile-only test in both vm/src/function/argument.rs and
vm/src/function/builtin.rs that binds a dummy function with 12 arguments to
ensure future drift is caught at compile time.
into_py_native_fn_tuple!( | ||
(v1, T1), | ||
(v2, T2), | ||
(v3, T3), | ||
(v4, T4), | ||
(v5, T5), | ||
(v6, T6), | ||
(v7, T7), | ||
(v8, T8) | ||
); | ||
|
||
into_py_native_fn_tuple!( | ||
(v1, T1), | ||
(v2, T2), | ||
(v3, T3), | ||
(v4, T4), | ||
(v5, T5), | ||
(v6, T6), | ||
(v7, T7), | ||
(v8, T8), | ||
(v9, T9) | ||
); | ||
|
||
into_py_native_fn_tuple!( | ||
(v1, T1), | ||
(v2, T2), | ||
(v3, T3), | ||
(v4, T4), | ||
(v5, T5), | ||
(v6, T6), | ||
(v7, T7), | ||
(v8, T8), | ||
(v9, T9), | ||
(v10, T10) | ||
); | ||
|
||
into_py_native_fn_tuple!( | ||
(v1, T1), | ||
(v2, T2), | ||
(v3, T3), | ||
(v4, T4), | ||
(v5, T5), | ||
(v6, T6), | ||
(v7, T7), | ||
(v8, T8), | ||
(v9, T9), | ||
(v10, T10), | ||
(v11, T11) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Sanity-check macro parity across files with a quick repo scan
Let’s verify there’s a 12-arity wrapper here and both 12-arity binding and IntoFuncArgs in argument.rs.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check builtin.rs for 12-arity wrapper:"
rg -nP -C2 $'into_py_native_fn_tuple!\\s*\\(\\s*(?s).*\\(v12,\\s*T12\\)' vm/src/function/builtin.rs || {
echo "Missing v12 block in builtin.rs"; exit 1;
}
echo "Check argument.rs for 12-arity FromArgs tuple:"
rg -n $'tuple_from_py_func_args!\\(A, B, C, D, E, F, G, H, I, J, K, L\\);' vm/src/function/argument.rs
echo "Check argument.rs for 12-arity IntoFuncArgs-from-tuple:"
rg -nP -C2 $'into_func_args_from_tuple!\\s*\\(\\s*(?s).*\\(v12,\\s*T12\\)' vm/src/function/argument.rs
Length of output: 283
Add missing 12-arity support
The into_py_native_fn_tuple!
invocation for 12 parameters is not present in vm/src/function/builtin.rs
. Please add it and ensure the matching 12-arity macros exist in argument.rs
.
• In vm/src/function/builtin.rs
(after the 11-arity block around lines 218–266), insert:
into_py_native_fn_tuple!(
(v1, T1),
(v2, T2),
(v3, T3),
(v4, T4),
(v5, T5),
(v6, T6),
(v7, T7),
(v8, T8),
(v9, T9),
(v10, T10),
(v11, T11),
(v12, T12)
);
• In vm/src/function/argument.rs
, verify you have:
- A 12-arity
tuple_from_py_func_args!
invocation:tuple_from_py_func_args!(A, B, C, D, E, F, G, H, I, J, K, L);
- A 12-arity
into_func_args_from_tuple!
block including(v12, T12)
.
These additions are required to maintain parity and enable functions with 12 native parameters.
🤖 Prompt for AI Agents
In vm/src/function/builtin.rs around lines 218–266 add the missing
into_py_native_fn_tuple! invocation for 12 parameters (v1..v12, T1..T12)
immediately after the existing 11-arity block; then update
vm/src/function/argument.rs to ensure there is a 12-arity
tuple_from_py_func_args! entry (A..L) and that the into_func_args_from_tuple!
macro/block includes the (v12, T12) mapping so the 12-arity conversion is
supported and names align with the other arities.
🛠️ Refactor suggestion
Extend wrappers to 12-arity to match argument.rs and avoid immediate ceiling at 11
argument.rs now supports binding up to 12 top-level parameters; builtin.rs tops out at 11, so a #[pymethod]/#[pyfunction] with 12 parameters will still fail to compile due to missing IntoPyNativeFn coverage. Aligning both to 12 prevents a new hard limit right away and matches the updated doc note in argument.rs.
Apply this diff to add the missing 12-arity tuple:
into_py_native_fn_tuple!(
(v1, T1),
(v2, T2),
(v3, T3),
(v4, T4),
(v5, T5),
(v6, T6),
(v7, T7),
(v8, T8),
(v9, T9),
(v10, T10),
(v11, T11)
);
+into_py_native_fn_tuple!(
+ (v1, T1),
+ (v2, T2),
+ (v3, T3),
+ (v4, T4),
+ (v5, T5),
+ (v6, T6),
+ (v7, T7),
+ (v8, T8),
+ (v9, T9),
+ (v10, T10),
+ (v11, T11),
+ (v12, T12)
+);
Follow-up:
- If you want to fully close Increase parameter limit for trait beyond 7 argumentsIntoPyNativeFn #6098 per the suggested target, consider extending both files to 16 or 20 in the same PR. Otherwise, please at least sync the upper bound to 12 across modules to avoid user confusion and new compilation failures at 12.
📝 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.
into_py_native_fn_tuple!( | |
(v1, T1), | |
(v2, T2), | |
(v3, T3), | |
(v4, T4), | |
(v5, T5), | |
(v6, T6), | |
(v7, T7), | |
(v8, T8) | |
); | |
into_py_native_fn_tuple!( | |
(v1, T1), | |
(v2, T2), | |
(v3, T3), | |
(v4, T4), | |
(v5, T5), | |
(v6, T6), | |
(v7, T7), | |
(v8, T8), | |
(v9, T9) | |
); | |
into_py_native_fn_tuple!( | |
(v1, T1), | |
(v2, T2), | |
(v3, T3), | |
(v4, T4), | |
(v5, T5), | |
(v6, T6), | |
(v7, T7), | |
(v8, T8), | |
(v9, T9), | |
(v10, T10) | |
); | |
into_py_native_fn_tuple!( | |
(v1, T1), | |
(v2, T2), | |
(v3, T3), | |
(v4, T4), | |
(v5, T5), | |
(v6, T6), | |
(v7, T7), | |
(v8, T8), | |
(v9, T9), | |
(v10, T10), | |
(v11, T11) | |
); | |
into_py_native_fn_tuple!( | |
(v1, T1), | |
(v2, T2), | |
(v3, T3), | |
(v4, T4), | |
(v5, T5), | |
(v6, T6), | |
(v7, T7), | |
(v8, T8), | |
(v9, T9), | |
(v10, T10), | |
(v11, T11) | |
); | |
into_py_native_fn_tuple!( | |
(v1, T1), | |
(v2, T2), | |
(v3, T3), | |
(v4, T4), | |
(v5, T5), | |
(v6, T6), | |
(v7, T7), | |
(v8, T8), | |
(v9, T9), | |
(v10, T10), | |
(v11, T11), | |
(v12, T12) | |
); |
🤖 Prompt for AI Agents
In vm/src/function/builtin.rs around lines 218 to 266 the IntoPyNativeFn tuple
wrappers stop at arity 11, causing functions with 12 parameters to lack
coverage; add a 12-arity invocation by copying the existing pattern and
inserting an into_py_native_fn_tuple! block with (v1, T1) … (v12, T12)
immediately after the 11-arity block so the file provides tuples up to 12
parameters, keep identical formatting/commas as the other blocks, and run cargo
test/compile to verify there are no macro or formatting issues.
I left a comment to the original issue |
closes #6098
Summary by CodeRabbit
New Features
Documentation