Skip to content

Reject extra positional arguments to float() - #8509

Merged
youknowone merged 2 commits into
RustPython:mainfrom
jseop-lim:fix-float-arity
Aug 13, 2026
Merged

youknowone merged 2 commits into
RustPython:mainfrom
jseop-lim:fix-float-arity

Conversation

@jseop-lim

@jseop-lim jseop-lim commented Aug 13, 2026 •

Copy link
Copy Markdown
Contributor

Summary

PyFloat::slot_new's exact-float fast path tested args.args.first(), which succeeds for any number of positional arguments, so float(1.5, True) returned the first one and never reached args.bind(vm). CPython's clinic-generated float_new runs _PyArg_CheckPositional("float", nargs, 0, 1) before it fetches the first argument.

AS-IS

>>> float(1.5, True)
1.5

TO-BE

>>> float(1.5, True)
TypeError: expected at most 1 arguments, got 2

(matching CPython, which raises TypeError: float expected at most 1 argument, got 2; the wording comes from FuncArgs::bind and is a separate repo-wide difference)

Changes

  • Add args.args.len() == 1 to the fast path condition in PyFloat::slot_new, so an extra positional argument falls through to args.bind(vm). PyInt::slot_new, PyStr::slot_new and PyComplex::slot_new already guard the same optimization this way; float was the one that did not.
  • Add regression tests in extra_tests/snippets/builtin_float.py for two and three positional arguments, plus the unchanged float(1.5) and float() forms.

Test plan

Built and run on macOS aarch64 against upstream/main 525ba8cf0, using the CI feature set --no-default-features --features stdlib,importlib,stdio,encodings,sqlite,ssl-rustls-aws-lc,host_env.

  • On an unpatched build of that commit float(1.5, True) returned 1.5; with the patch it raises TypeError. float(1.5), float(), float("1.5") and float subclass construction are unchanged.
  • cargo run --release -- -m test test_float test_builtin test_complex: 229 run, 18 skipped, all pass.
  • cargo test --workspace --exclude rustpython-capi --exclude rustpython_wasm --exclude rustpython-compiler-source --exclude rustpython-venvlauncher --features threading: 1107 pass, 0 fail. cargo test in crates/capi: 102 pass.
  • extra_tests builtin snippets under RustPython: 66 of 67 pass, the new builtin_float.py cases among them. The one failure, builtin_thread.py, asserts _thread.TIMEOUT_MAX in [9223372036.0, 4294967.0] and my build reports 2147483648, which is an unrelated platform constant.
  • cargo fmt --check clean, cargo clippy -p rustpython-vm --all-targets reports nothing on the changed file, and pre-commit run --files passes on both changed files.

I used Claude Code (claude-opus-5) to draft the analysis, the patch and this description; I reviewed the change and ran every command above myself.

Summary by CodeRabbit

  • Bug Fixes

    • Corrected float() so it rejects calls with multiple positional arguments.
    • Preserved existing behavior for valid single-argument calls, no-argument calls, and exact float values.
  • Tests

    • Added coverage for invalid multi-argument calls involving floats and strings.

PyFloat::slot_new's exact-float fast path tested args.args.first(), which
succeeds for any number of positional arguments, so float(1.5, True)
returned the first one and never reached args.bind(vm). CPython's clinic
generated float_new runs _PyArg_CheckPositional("float", nargs, 0, 1)
before it fetches the first argument.

Add args.args.len() == 1 to the condition, as PyInt::slot_new,
PyStr::slot_new and PyComplex::slot_new already do; the extra argument
then falls through to args.bind(vm) and raises TypeError.

Assisted-by: Claude Code:claude-opus-5
@coderabbitai

coderabbitai Bot commented Aug 13, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: f7ec6304-5576-43e8-9282-3a39313e097e

📥 Commits

Reviewing files that changed from the base of the PR and between b089c9f and 6fa759e.

📒 Files selected for processing (1)
  • crates/vm/src/builtins/float.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/vm/src/builtins/float.rs

📝 Walkthrough

Walkthrough

The float constructor binds arguments before its exact-float reuse path. Extra positional arguments now raise TypeError. Tests cover invalid calls and preserve valid zero- and one-argument behavior.

Changes

Float constructor validation

Layer / File(s) Summary
Arity guard and regression tests
crates/vm/src/builtins/float.rs, extra_tests/snippets/builtin_float.py
The exact-float fast path runs only after valid argument binding. Tests verify that extra arguments raise TypeError and that zero- and one-argument calls remain valid.

Estimated code review effort: 2 (Simple) | ~10 minutes

Mergeability Score: ⚪ Minimal · up to 6fa75

The change makes float() reject extra positional arguments while preserving existing valid forms, with regression tests covering the affected behavior; no actionable merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly states the primary fix: reject extra positional arguments passed to float().
Linked Issues check ✅ Passed The implementation validates arguments before the exact-float fast path, and tests cover the reported failure and preserved valid behavior [#8461].
Out of Scope Changes check ✅ Passed The code and tests directly address float() positional-argument validation without unrelated changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with 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.

❤️ Share

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

Comment thread crates/vm/src/builtins/float.rs Resolved
The exact-float fast path repeated float()'s argument count inline, which
duplicates what FromArgs::arity already knows. Bind first and match on the
resulting OptionalArg instead, so the positional and keyword rules stay in
one place and the bound value is reused by py_new.

Assisted-by: Claude Code:claude-opus-5
@youknowone youknowone added the z-ca-2026 Tag to track Contribution Academy 2026 label Aug 13, 2026

@ShaharNaveh ShaharNaveh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ty:)

@youknowone
youknowone merged commit c9a6244 into RustPython:main Aug 13, 2026
27 checks passed
@jseop-lim
jseop-lim deleted the fix-float-arity branch August 13, 2026 16:14
youknowone pushed a commit that referenced this pull request Sep 16, 2026
* Reject extra positional arguments to float()

PyFloat::slot_new's exact-float fast path tested args.args.first(), which
succeeds for any number of positional arguments, so float(1.5, True)
returned the first one and never reached args.bind(vm). CPython's clinic
generated float_new runs _PyArg_CheckPositional("float", nargs, 0, 1)
before it fetches the first argument.

Add args.args.len() == 1 to the condition, as PyInt::slot_new,
PyStr::slot_new and PyComplex::slot_new already do; the extra argument
then falls through to args.bind(vm) and raises TypeError.

Assisted-by: Claude Code:claude-opus-5

* Bind float() arguments before the fast path

The exact-float fast path repeated float()'s argument count inline, which
duplicates what FromArgs::arity already knows. Bind first and match on the
resulting OptionalArg instead, so the positional and keyword rules stay in
one place and the bound value is reused by py_new.

Assisted-by: Claude Code:claude-opus-5
@youknowone youknowone removed the z-ca-2026 Tag to track Contribution Academy 2026 label Oct 5, 2026
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.

float(1.5, True) returns the first argument instead of raising TypeError

3 participants