Skip to content

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ShaharNaveh
Copy link
Contributor

@ShaharNaveh ShaharNaveh commented Aug 18, 2025

closes #6098

Summary by CodeRabbit

  • New Features

    • Increased supported function parameters from 7 to 12, enabling higher-arity functions to be bound and called.
    • Expanded native/builtin function wrapping to handle functions with up to 12 arguments.
  • Documentation

    • Updated arity guidance to reflect support for up to 12 parameters.

Copy link
Contributor

coderabbitai bot commented Aug 18, 2025

Walkthrough

Expanded 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

Cohort / File(s) Summary
Argument binding arity expansion
vm/src/function/argument.rs
Added FromArgs implementations for 8–12 tuples; added IntoFuncArgs/IntoMethodArgs macro invocations for 7–12; updated doc note on max arity.
IntoPyNativeFn arity expansion
vm/src/function/builtin.rs
Added into_py_native_fn_tuple! macro invocations for 8–11 parameters to generate PyNativeFnInternal implementations.

Sequence Diagram(s)

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Assessment against linked issues

Objective Addressed Explanation
Increase parameter limit for IntoPyNativeFn beyond 7 arguments (#6098)
Support methods with 11 parameters as in example (#6098)
Extend up to 15–20 parameters (suggested) (#6098) Extended to 11 (IntoPyNativeFn) and 12 (args); not 15–20.

Assessment against linked issues: Out-of-scope changes

Poem

I twitch my ears at tuples tall,
From seven hops to twelve in all!
Now functions stretch their carrot-span,
Eleven peas in one hot pan.
I thump: “More args? No fright!”
Code burrows deeper—works just right.
🥕✨

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 Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@coderabbitai coderabbitai bot changed the title @coderabbitai Expand arg tuple handling to 12 and IntoPyNativeFn to 11 params Aug 18, 2025
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: 4

🧹 Nitpick comments (1)
vm/src/function/builtin.rs (1)

218-266: Add a regression test for high arity (11–12) function wrapping

No 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.

📥 Commits

Reviewing files that changed from the base of the PR and between a9a9e3b and cfcf1a2.

📒 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 correct

Mirrors 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 — LGTM

Arity aggregation logic remains correct; min/max sums over component arities. This is the right underpinning for high-arity signatures.

Comment on lines +118 to +131
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)
);
Copy link
Contributor

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.

Comment on lines +655 to 657
// 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).
Copy link
Contributor

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.

Comment on lines +218 to +266
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)
);
Copy link
Contributor

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:

📝 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
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.

@youknowone
Copy link
Member

I left a comment to the original issue

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.

Increase parameter limit for trait beyond 7 argumentsIntoPyNativeFn
2 participants