Upgrade math,cmath to 3.14.2 and totally delegate to pymath#6705
Conversation
Sorry, something went wrong.
📝 WalkthroughWalkthroughWorkspace excludes the Changes
Sequence Diagram(s)sequenceDiagram
participant PyCaller as Python code
participant StdLib as stdlib::math / stdlib::cmath
participant VM as VirtualMachine
participant PyMath as pymath crate
PyCaller->>StdLib: call math/cmath function(args)
StdLib->>VM: validate args / prepare context
StdLib->>PyMath: delegate computation(args)
PyMath-->>StdLib: result or pymath::Error
alt result
StdLib-->>PyCaller: PyResult<T> (value)
else error
StdLib->>VM: pymath_exception(err, vm)
StdLib-->>PyCaller: PyResult (PyErr)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
Sorry, something went wrong.
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin math |
Sorry, something went wrong.
youknowone
force-pushed
the
math
branch
6 times, most recently
from
c456550 to
f133d7e
Compare
January 12, 2026 11:36
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @crates/stdlib/src/math.rs:
- Around line 851-863: The overflow error messages are inconsistent: both
branches convert to u64 (ki) but the first error says i64::MAX; update both
vm.new_overflow_error calls to reference the correct limit for the conversion
target (u64::MAX) and use consistent formatting (e.g.,
vm.new_overflow_error(format!("n must not exceed {}", u64::MAX)) and
vm.new_overflow_error(format!("k must not exceed {}", u64::MAX))). Ensure you
update the message for the n branch (when k_big is None) and keep the existing
formatted message for the k branch but with u64::MAX.
- Around line 929-937: The overflow error message for the effective_k -> u64
conversion is inconsistent: it mentions i64::MAX while the value is being
converted to u64; update the vm.new_overflow_error call in the effective_k
conversion branch (where ki: u64 is set) to report the correct limit (u64::MAX)
or otherwise match the actual conversion target, ensuring the message text
references u64::MAX and keep the surrounding function names (effective_k, ki,
vm.new_overflow_error) unchanged.
🧹 Nitpick comments (2)
crates/stdlib/src/math.rs (2)
593-598: Potential precision loss when converting large integers to float.When a
PyIntis encountered in the float fast path, it's converted toi64first, then cast tof64. For integers that fit ini64but exceedf64's 53-bit mantissa precision, this may lose precision. Consider usingtry_bigint_to_f64for consistency with other conversions in this file.♻️ Suggested improvement
if let Some(i) = item.downcast_ref::<PyInt>() - && let Ok(v) = i.as_bigint().try_into() as Result<i64, _> + && let Ok(v) = try_bigint_to_f64(i.as_bigint(), vm) { - flt_result *= v as f64; + flt_result *= v; continue; }
632-632: Unused mutable variableflt_path_enabled.The variable
flt_path_enabledis declared as mutable but is never modified (always remainstrue). This makes theif flt_path_enabledcheck at line 686 always true. Either remove the variable and the condition, or implement the logic to disable the float path when appropriate.♻️ Suggested fix
// Fast path state let mut int_path_enabled = true; - let flt_path_enabled = true; let mut int_total: i64 = 0;And at line 686:
- // Float fast path - only when at least one value is exact float type - // (not subclasses, to preserve custom __mul__/__add__ behavior) - if flt_path_enabled { + // Float fast path - only when at least one value is exact float type + // (not subclasses, to preserve custom __mul__/__add__ behavior) + {
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
Cargo.lockis excluded by!**/*.lockLib/test/mathdata/cmath_testcases.txtis excluded by!Lib/**Lib/test/mathdata/ieee754.txtis excluded by!Lib/**Lib/test/test_cmath.pyis excluded by!Lib/**Lib/test/test_math.pyis excluded by!Lib/**
📒 Files selected for processing (4)
Cargo.tomlcrates/stdlib/src/cmath.rscrates/stdlib/src/math.rscrates/vm/src/builtins/float.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by 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:
crates/vm/src/builtins/float.rscrates/stdlib/src/math.rscrates/stdlib/src/cmath.rs
🧠 Learnings (1)
📚 Learning: 2025-12-27T14:03:49.034Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/stdlib/src/math.rs
⏰ 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). (8)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-2025)
- GitHub Check: Run rust tests (windows-2025)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Check Rust code with clippy
🔇 Additional comments (12)
crates/vm/src/builtins/float.rs (1)
117-128: LGTM! Error message aligned with CPython 3.14.2.The updated message
"zero to a negative power"matches CPython's exactZeroDivisionErrormessage for this case, which is consistent with the PR's objective to upgrade compatibility to Python 3.14.2.crates/stdlib/src/cmath.rs (3)
14-27: LGTM!Constants are correctly sourced from
pymath::cmath, maintaining consistency with the delegation approach.
29-146: LGTM!All complex math functions consistently delegate to
pymath::cmathwith proper error mapping throughpymath_exception. The pattern is uniform and maintainable.
160-169: LGTM!The
iscloseimplementation correctly delegates topymath::cmath::isclosewith appropriate tolerance handling. The custom error message for negative tolerances is more user-friendly than a generic domain error.crates/stdlib/src/math.rs (7)
29-69: LGTM!Basic math functions (
fabs,isfinite,isinf,isnan,isclose,copysign) correctly delegate topymath::mathwith consistent error handling.
94-123: LGTM!The
logfunction correctly handles BigInt inputs specially to avoid precision loss, validates the base before conversion, and provides descriptive error messages including the input value.
366-400: LGTM!The
ceilandfloorimplementations correctly check for__ceil__/__floor__attribute existence before attempting to call the magic method, with appropriate fallback to float conversion.
447-463: LGTM!The
nextafterfunction correctly validates that steps is non-negative before casting tou64, and properly handles the optional steps parameter.
767-778: LGTM!The
fmafunction correctly delegates topymath::math::fmawith appropriate custom error messages for domain and range errors.
811-823: LGTM!The
factorialfunction correctly validates input order (negative check before overflow), provides a clear overflow message, and defensively handles errors from the underlying implementation.
944-970: LGTM!The
pymath_exceptionhelper provides consistent error mapping, andfloat_reprcorrectly formats floats in Python style with proper handling of special values (nan, inf) and ensures integer-valued floats display with a trailing.0.Cargo.toml (1)
187-187: The pymath dependency specification is correct. The git revision564ebb2780a05a9460ad12a2752a67244dc5f89eis valid and accessible, and all three specified features (mul_add,malachite-bigint,complex) are properly defined in pymath's Cargo.toml at that revision. No issues found.
Sorry, something went wrong.
ShaharNaveh
left a comment
ShaharNaveh
left a comment
There was a problem hiding this comment.
Overall it looks great!
Sorry, something went wrong.
|
Is there a reason why |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/stdlib/src/math.rs (3)
630-636: Dead variable:flt_path_enabledis never modified.The variable
flt_path_enabledis initialized totruebut never set tofalse, unlikeint_path_enabled. Since the float path check runs conditionally anyway and the generic path returns early, this doesn't cause incorrect behavior, but the variable is effectively unused.Consider removing or using the variable
- let flt_path_enabled = true;Either remove the variable entirely and just use the condition directly, or properly disable it when transitioning out of the float path for consistency with
int_path_enabled.
789-809: Consider reducing allocations ingcdandlcm.Both functions create an intermediate
Vec<PyRef<PyInt>>before creatingVec<&BigInt>. This is fine for correctness but involves extra allocations.Single-pass alternative
#[pyfunction] fn gcd(args: PosArgs<ArgIndex>) -> BigInt { - let ints: Vec<_> = args - .into_vec() - .into_iter() - .map(|x| x.into_int_ref()) - .collect(); - let refs: Vec<_> = ints.iter().map(|x| x.as_bigint()).collect(); - pymath::math::integer::gcd(&refs) + let ints: Vec<_> = args.into_vec(); + let refs: Vec<_> = ints.iter().map(|x| x.as_bigint()).collect(); + pymath::math::integer::gcd(&refs) }This assumes
ArgIndexprovides direct access toBigInt. If not, the current approach is necessary to keep references valid.
811-823: Unreachable error path infactorial.Line 822's error case is unreachable since line 814-816 already handles negative values. The pymath
factorialcall at line 820 should only receive non-negative values.Simplify error handling
pymath::math::integer::factorial(n) .map(|r| r.into()) - .map_err(|_| vm.new_value_error("factorial() not defined for negative values")) + .expect("pymath factorial should not fail for non-negative i64")Or keep the defensive error handling if pymath might have other failure modes not documented.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.tomlcrates/stdlib/src/math.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by 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:
crates/stdlib/src/math.rs
🧠 Learnings (1)
📚 Learning: 2025-12-27T14:03:49.034Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/stdlib/src/math.rs
⏰ 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). (10)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-2025)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run tests under miri
- GitHub Check: Check the WASM package and demo
- GitHub Check: Ensure compilation on various targets
🔇 Additional comments (11)
crates/stdlib/src/math.rs (11)
1-16: LGTM!The imports are properly organized, bringing in the necessary types for pymath delegation (
pymath_exception,float_repr) and the traits needed for BigInt operations (Signed,ToPrimitive).
28-75: LGTM!The basic math functions correctly delegate to
pymathwith appropriate error mapping. Theisclosefunction's simplified error handling is acceptable since tolerance validation is the only expected error case.
94-123: LGTM!The
logfunction properly validates the base before calling pymath, handles BigInt values specially for precision with large integers, and provides appropriate error messages matching Python's behavior.
232-235: LGTM!The
hypotfunction correctly delegates to pymath. Since hypot mathematically cannot fail (it computes sqrt of sum of squares, always non-negative), returningf64directly without error handling is appropriate.
367-400: LGTM!The
ceilandfloorfunctions correctly prioritize the magic method (__ceil__/__floor__) when the attribute exists, falling back to float conversion only when the method is not defined. This properly propagates errors for non-callable attributes.
509-550: Integer fast path inprodis well-structured.The overflow handling is correct: when the start value or intermediate result overflows
i64, the code properly transitions to the generic PyObject path. The use ofchecked_mulprevents silent overflow.
767-778: LGTM!The
fmafunction properly delegates to pymath and maps domain errors toValueErrorand range errors toOverflowError, matching Python's expected behavior.
880-941: LGTM!The
combfunction correctly implements the binomial coefficient with:
- Proper validation of non-negative inputs
- Symmetry optimization using
min(k, n-k)to reduce computation- Fast path for values fitting in
i64- BigInt fallback for large values
944-970: LGTM!The helper functions are well-implemented:
pymath_exceptionprovides clean error mapping between pymath and Python exceptionsfloat_reprcorrectly handles special values (NaN, ±Inf) and ensures Python-style formatting with trailing.0for whole numbers
447-463: LGTM!The
nextafterfunction correctly validates that steps is non-negative before casting tou64, and properly handles the optional steps parameter.
579-582: No action needed. The project's MSRV is 1.91.0, which fully supportsOption::is_none_or(stabilized in Rust 1.82). The method usage is stable and appropriate for this codebase.Likely an incorrect or invalid review comment.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @crates/stdlib/src/math.rs:
- Around line 929-937: The error message in the u64 conversion block for
effective_k (where ki: u64 is assigned) incorrectly references i64::MAX; update
the new_overflow_error message in this branch to reflect the correct limit
(e.g., u64::MAX or an appropriate u64-bound) so it aligns with the attempted
to_u64() conversion; locate the match on effective_k.to_u64() that sets ki and
change the formatted string used in vm.new_overflow_error to mention the u64
limit (mirroring the fix made in the perm function).
- Around line 857-863: The error message is inconsistent: code converts k_val
via to_u64() but the overflow message mentions i64::MAX; update the
vm.new_overflow_error call in this match branch (the block handling
k_val.to_u64()) to reference u64::MAX (or otherwise the correct unsigned limit)
so the message matches the actual conversion/limit used by k_val and to_u64().
🧹 Nitpick comments (1)
crates/stdlib/src/math.rs (1)
561-577: The0.0fallback at line 565 is unreachable but confusing.The condition at line 562 (
if let Some(f) = obj.downcast_ref::<PyFloat>()) means the else branch at line 564-566 returning0.0is unreachable given the outer condition check. Consider restructuring to make this clearer or useunreachable!()to document the invariant.♻️ Suggested clarification
- let mut flt_result: f64 = if let Some(ref obj) = obj_result { - if let Some(f) = obj.downcast_ref::<PyFloat>() { - f.to_f64() - } else { - 0.0 // Won't be used - } + let mut flt_result: f64 = if let Some(ref obj) = obj_result { + // obj_result is guaranteed to be Some(PyFloat) here due to outer condition + obj.downcast_ref::<PyFloat>().map(|f| f.to_f64()).unwrap_or_else(|| unreachable!())
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
Cargo.lockis excluded by!**/*.lockLib/test/mathdata/cmath_testcases.txtis excluded by!Lib/**Lib/test/mathdata/ieee754.txtis excluded by!Lib/**Lib/test/test_cmath.pyis excluded by!Lib/**Lib/test/test_math.pyis excluded by!Lib/**
📒 Files selected for processing (4)
Cargo.tomlcrates/stdlib/src/cmath.rscrates/stdlib/src/math.rscrates/vm/src/builtins/float.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/vm/src/builtins/float.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by 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:
crates/stdlib/src/math.rscrates/stdlib/src/cmath.rs
🧠 Learnings (1)
📚 Learning: 2025-12-27T14:03:49.034Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/stdlib/src/math.rs
🧬 Code graph analysis (1)
crates/stdlib/src/cmath.rs (1)
crates/stdlib/src/math.rs (8)
pymath_exception(944-949)x(109-109)x(133-133)x(152-152)exp(79-81)log(94-123)log10(150-166)isclose(61-69)
⏰ 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: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (windows-2025)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-2025)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Check the WASM package and demo
🔇 Additional comments (13)
Cargo.toml (2)
119-119: LGTM - Workspace exclusion for local pymath directory.This exclusion prevents Cargo from treating a local
pymathdirectory as a workspace member, which is correct when using the crates.io dependency.
188-188: Dependency configuration is correct.Verified that pymath 0.1.4 exists on crates.io with all three required features:
mul_add,malachite-bigint, andcomplex. Version 0.1.4 is the latest available release.crates/stdlib/src/math.rs (8)
1-16: LGTM - Clean import structure.The imports are well-organized, separating module-level exports (
pymath_exception,float_repr) from the innermathmodule's imports. The addition ofAsObjectandToPrimitivealigns with the BigInt-aware implementations.
28-75: LGTM - Clean delegation pattern for basic math functions.The delegation to
pymath::mathfunctions is consistent and the error handling viapymath_exceptionis correctly applied where needed. Theisclosefunction appropriately provides a specific error message for negative tolerances.
93-166: LGTM - BigInt-aware logarithm implementations.The log functions correctly handle:
- Base validation (positive, not 1.0) before processing the value
- BigInt special path to avoid overflow for large integers
- Descriptive error messages including the actual value via
float_reprThis aligns with CPython's behavior for handling large integers in logarithmic functions.
232-245: LGTM - Variadic hypot and dist implementations.The
hypotfunction correctly delegates to pymath without error handling (hypot handles infinities gracefully). Thedistfunction properly validates that both vectors have the same length before computing the Euclidean distance.
366-400: LGTM - Correct__ceil__/__floor__dispatch logic.The implementation correctly:
- Checks if the class defines the magic method
- Calls it if present (propagating errors for non-callable attributes)
- Falls back to float conversion only when the method is not defined
This matches the expected Python 3.14 behavior.
619-765: LGTM - Well-structured sumprod with appropriate fast paths.The implementation correctly:
- Uses exact type checks (
.class().is()) to preserve subclass behavior- Falls back to the generic Python path on overflow or non-numeric types
- Leverages
pymath::math::sumprodfor numerically accurate float summation- Handles length mismatches with proper error messages
The complexity is justified for performance with numeric sequences.
767-778: LGTM - fma with specific error messages.The fused multiply-add correctly maps domain and range errors to appropriate Python exceptions with operation-specific messages, rather than using the generic
pymath_exception.
944-970: LGTM - Well-designed helper functions.
pymath_exceptionprovides centralized error mapping matching Python's math module conventions.float_reprcorrectly handles special values (nan, inf) and ensures integer-valued floats display with a trailing.0for clarity in error messages.crates/stdlib/src/cmath.rs (3)
11-27: LGTM - Constants sourced from pymath for consistency.Using
pymath::cmathconstants ensures the cmath module's mathematical constants are consistent with the underlying implementation. The import ofpymath_exceptionfrom the sibling math module correctly shares the centralized error mapping.
29-146: LGTM - Consistent delegation pattern for complex math functions.All functions correctly:
- Add the
vmparameter for error propagation- Return
PyResult<T>for proper exception handling- Delegate to
pymath::cmathequivalents- Map errors through the centralized
pymath_exceptionThe predicate functions (
isinf,isfinite,isnan) appropriately remain non-fallible.
160-169: LGTM - Complex isclose with tolerance validation.The implementation mirrors the real
math.isclosepattern, correctly handling optional tolerances and providing a clear error message for negative tolerance values.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/stdlib/src/math.rs (1)
490-617:prod(): fast paths should only apply to exact int/float types (subclass operator semantics).Right now
start_is_int/start_is_floatand per-item checks are based ondowncast_ref::<PyInt/PyFloat>(). If that matches int/float subclasses in RustPython, the fast path can bypass overridden__mul__implementations (Python requires honoring dynamic dispatch for subclasses).sumprod()takes care to checkclass().is(int_type/float_type)—prod()should do the same.Proposed fix (tighten to exact types for fast paths)
@@ - let (mut obj_result, start_is_int, start_is_float) = match &start { + let (mut obj_result, start_is_int, start_is_float) = match &start { OptionalArg::Present(s) => { - let is_int = s.downcast_ref::<PyInt>().is_some(); - let is_float = s.downcast_ref::<PyFloat>().is_some(); + let is_int = s.class().is(vm.ctx.types.int_type); + let is_float = s.class().is(vm.ctx.types.float_type); (Some(s.clone()), is_int, is_float) } OptionalArg::Missing => (None, true, false), // Default is int 1 }; @@ - if let Some(int_item) = item.downcast_ref::<PyInt>() + if item.class().is(vm.ctx.types.int_type) + && let Some(int_item) = item.downcast_ref::<PyInt>() && let Ok(b) = int_item.as_bigint().try_into() as Result<i64, _> && let Some(product) = int_result.checked_mul(b) @@ - if let Some(f) = item.downcast_ref::<PyFloat>() { + if item.class().is(vm.ctx.types.float_type) + && let Some(f) = item.downcast_ref::<PyFloat>() { flt_result *= f.to_f64(); continue; }
🤖 Fix all issues with AI agents
In @crates/stdlib/src/math.rs:
- Around line 782-941: The perm and comb implementations enforce Rust i64/u64
limits by using .to_i64()/.to_u64() and return OverflowError messages
referencing i64::MAX; remove those artificial limits so arbitrary-precision
BigInt inputs follow Python behavior: in perm (function perm) stop returning
vm.new_overflow_error when n_big.to_u64()/k_val.to_u64() or n_big.to_i64() fails
and instead route to the BigInt path (call pymath::math::perm_bigint) for large
values; likewise in comb (function comb) do not error/mention i64::MAX when
effective_k.to_u64() or n_big.to_i64() conversion fails — either fall back to
pymath::math::comb_bigint or return Ok(0) / ValueError only for negative inputs;
also remove/replace error messages that include "i64::MAX" with generic overflow
or avoid them entirely by using bigint paths.
- Around line 944-970: The custom float_repr function should be replaced to use
RustPython's canonical float formatter: remove the manual formatting logic in
float_repr and call crate::literal::float::to_string(value) (or the
crate::literal::float module's public conversion function) to produce
Python-compatible float representations; keep the function signature
float_repr(value: f64) -> String and return the result of the to_string helper
so error messages and reprs use the standard Python formatting.
🧹 Nitpick comments (2)
crates/stdlib/src/math.rs (2)
3-17: Import + helper wiring looks coherent; keeppymath_exception/float_reprusage consistent.Minor readability nit: some call sites use
super::float_repr(...)while others usefloat_repr(...)directly (same module import). Consider standardizing on one style. Based on learnings, macro usage is already correct.
619-765:sumprod(): avoid O(n) buffering + avoid collecting “remaining” into Vec for the generic path.Two concerns:
- The float fast path buffers
flt_p_values/flt_q_valuesfor the whole run; that can be a lot of memory for large iterables.- The fallback path collects all remaining elements into
Vecs; you already have synchronized iteration logic, so you can compute generically in a streaming loop without materializing.Suggested direction (streaming generic fallback)
@@ - // Collect current + remaining elements - let p_remaining: Result<Vec<PyObjectRef>, _> = - std::iter::once(Ok(p_i)).chain(p_iter).collect(); - let q_remaining: Result<Vec<PyObjectRef>, _> = - std::iter::once(Ok(q_i)).chain(q_iter).collect(); - let (p_vec, q_vec) = (p_remaining?, q_remaining?); - - if p_vec.len() != q_vec.len() { - return Err(vm.new_value_error("Inputs are not the same length")); - } - - let mut total = obj_total.unwrap_or_else(|| vm.ctx.new_int(0).into()); - for (p_item, q_item) in p_vec.into_iter().zip(q_vec) { - let prod = vm._mul(&p_item, &q_item)?; - total = vm._add(&total, &prod)?; - } - return Ok(total); + let mut total = obj_total.unwrap_or_else(|| vm.ctx.new_int(0).into()); + // current pair + total = vm._add(&total, &vm._mul(&p_i, &q_i)?)?; + // remaining pairs (stream) + loop { + match (p_iter.next(), q_iter.next()) { + (Some(rp), Some(rq)) => { + let prod = vm._mul(&rp?, &rq?)?; + total = vm._add(&total, &prod)?; + } + (None, None) => return Ok(total), + _ => return Err(vm.new_value_error("Inputs are not the same length")), + } + }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockLib/test/test_cmath.pyis excluded by!Lib/**Lib/test/test_math.pyis excluded by!Lib/**
📒 Files selected for processing (4)
Cargo.tomlcrates/stdlib/src/cmath.rscrates/stdlib/src/math.rscrates/vm/src/builtins/float.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/vm/src/builtins/float.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by 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:
crates/stdlib/src/math.rscrates/stdlib/src/cmath.rs
🧠 Learnings (1)
📚 Learning: 2025-12-27T14:03:49.034Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/stdlib/src/math.rs
🧬 Code graph analysis (1)
crates/stdlib/src/cmath.rs (1)
crates/stdlib/src/math.rs (5)
pymath_exception(944-949)x(109-109)x(133-133)x(152-152)isclose(61-69)
⏰ 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 (macos-latest)
- GitHub Check: Run rust tests (windows-2025)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-2025)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run tests under miri
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check Rust code with clippy
🔇 Additional comments (10)
crates/stdlib/src/math.rs (5)
366-400:ceil/floor: good “only call if defined” behavior; verifyhas_attrmatches CPython’s lookup rules.The change to only call
__ceil__/__floor__when the class defines it (and otherwise fall back to float conversion) seems right for propagating non-callable attribute errors. Please confirmhas_attrhere mirrors Python’s attribute resolution expectations (class dict vs__getattr__etc.) for edge cases.
448-463:nextafter(steps=...): conversion toi64thenu64is fine; confirm overflow behavior.If
stepsdoesn’t fit ini64,try_to_primitivelikely raises (OverflowError?). Please ensure the exception type/message matches CPython for “too large steps”.
767-778:fma(): error mapping seems correct; confirm EDOM/ERANGE conditions match CPython.Please verify when CPython raises ValueError vs OverflowError for
math.fmaedge cases (NaNs, infinities, overflow).
29-75: Confirm pymath error scope for tolerances.The error message
"tolerances must be non-negative"matches CPython's behavior for negativerel_tolorabs_tol. However, verify thatpymath::math::isclose()only errors on negative tolerances—if pymath can error for other reasons, the current mapping may mask those issues.
408-418: Theldexpimplementation correctly maps allpymatherrors viapymath_exception. The error mapping for overflow (ERANGE→OverflowError) is correct and matches CPython's behavior, which is validated by the comprehensive test suite inLib/test/test_math.py. The tests confirm that huge positive exponents raiseOverflowErrorand huge negative exponents return0.0, with special values (infinity, NaN) handled correctly.crates/stdlib/src/cmath.rs (3)
11-28: Constants delegated topymath::cmath: verify exact values/const-ness across platforms.Looks clean. Please confirm
pymath::cmath::{E,PI,TAU,INF,NAN,INFJ,NANJ}exactly match CPython’s published constants (including NaN payload expectations if tests care).
29-146: Function delegation + centralized error mapping is consistent.Signatures now vm-aware and
PyResult-based, and you consistently map errors viapymath_exception. That aligns well with the math.rs pattern.
160-169:cmath.isclose: ensure error mapping doesn’t hide other pymath failures.Same note as
math.isclose: mapping all errors to “tolerances must be non-negative” assumes that’s the only failure mode. Please confirm pymath doesn’t error for other reasons here.Cargo.toml (2)
188-188: Remove invalid feature flags frompymathdependency.The
pymathcrate version 0.1.5 does not support any Cargo feature flags. The featuresmul_add,malachite-bigint, andcomplexdo not exist in this crate version and will cause build failures or be silently ignored. Update the dependency to:pymath = "0.1.5"Likely an incorrect or invalid review comment.
113-120: Consider whether[workspace].exclude = ["pymath"]is necessary / sufficient.The concern is valid from a Cargo workspace perspective: the exclude directive only prevents Cargo from treating a
./pymath/path as a workspace member. If pymath exists or might be vendored elsewhere (e.g.,vendor/pymath), this won't prevent accidental inclusion. However, web searches indicate pymath is a published external crate on crates.io, which suggests the exclude may be a defensive measure or addressing a specific historical layout decision. Without access to the PR description or actual repository structure, the rationale cannot be fully verified. Confirm the intent in the PR discussion or changelog.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/stdlib/src/math.rs (1)
855-868: Verify edge cases forki <= 1in perm fast path.The condition
ki > 1on line 858 means cases wherekis 0 or 1 skip the fast path and fall through toperm_bigint. While this should work correctly, it's less efficient for these common cases. Consider adding explicit handling:if ki == 0 { return Ok(BigInt::from(1u8)); } if ki == 1 { return Ok(n_big.clone()); }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
Cargo.lockis excluded by!**/*.lockLib/test/mathdata/cmath_testcases.txtis excluded by!Lib/**Lib/test/mathdata/ieee754.txtis excluded by!Lib/**Lib/test/test_cmath.pyis excluded by!Lib/**Lib/test/test_math.pyis excluded by!Lib/**
📒 Files selected for processing (4)
Cargo.tomlcrates/stdlib/src/cmath.rscrates/stdlib/src/math.rscrates/vm/src/builtins/float.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by 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:
crates/vm/src/builtins/float.rscrates/stdlib/src/math.rscrates/stdlib/src/cmath.rs
🧠 Learnings (1)
📚 Learning: 2025-12-27T14:03:49.034Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/stdlib/src/math.rs
🧬 Code graph analysis (1)
crates/stdlib/src/cmath.rs (1)
crates/stdlib/src/math.rs (5)
pymath_exception(934-939)x(109-109)x(133-133)x(152-152)isclose(61-69)
⏰ 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). (10)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-2025)
- GitHub Check: Run rust tests (windows-2025)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Check the WASM package and demo
🔇 Additional comments (15)
crates/vm/src/builtins/float.rs (1)
117-128: LGTM!The error message update aligns with CPython 3.14.2 messaging conventions. The function logic remains correct—zero raised to a negative power correctly returns a
ZeroDivisionError.crates/stdlib/src/math.rs (10)
1-16: LGTM!Clean import structure with proper separation between module-level and inner module imports. The
pymath_exceptionhelper import enables consistent error mapping.
28-75: LGTM!The delegation to
pymathfor basic functions is clean and consistent. Theisclosetolerance validation properly rejects negative values with an appropriate error message.
93-166: LGTM!Excellent handling of BigInt inputs for logarithm functions—this preserves precision for large integers. The explicit base validation (rejecting
<= 0and== 1.0) prevents domain errors early with descriptive messages.
366-400: LGTM!The
ceilandfloorimplementations correctly prioritize the__ceil__/__floor__magic methods when defined, falling back to float conversion otherwise. The comment about propagating errors for non-callable attributes is accurate.
509-552: Integer fast path overflow handling looks correct.When
checked_mulfails or encounters a non-int, the code correctly converts the currentint_resultto aPyObjectand continues with generic multiplication. The type checks for exactint_type(excluding subclasses) properly preserve custom__mul__behavior.
730-749: Iterator consumption during fallback is handled correctly.When falling back to the generic path, the code properly collects both the current item and all remaining items from both iterators into vectors, then validates lengths match. This ensures no items are lost during the path transition.
799-811: LGTM!The factorial implementation correctly validates negative inputs before attempting the i64 conversion. The overflow error message helpfully specifies the maximum allowed value.
870-931: LGTM!The
combimplementation is well-structured with proper symmetry optimization (min(k, n-k)) to reduce computation. Edge cases forki == 0andki == 1are handled explicitly in the fast path, which is efficient.
941-960: LGTM!The
float_reprhelper correctly handles special float values (nan, inf) and ensures integer-like floats display with a trailing.0for Python consistency. The check for scientific notation (e/E) prevents incorrect formatting of numbers like1e10.
755-766: LGTM!The
fmafunction correctly provides context-specific error messages rather than generic "math domain error" messages, improving debuggability.crates/stdlib/src/cmath.rs (4)
13-27: LGTM!Referencing constants from
pymath::cmathensures consistency and simplifies maintenance when pymath is updated.
29-146: LGTM!Clean and consistent delegation to
pymath::cmathwith proper error mapping. Reusingpymath_exceptionfrom themathmodule eliminates code duplication.
90-101: LGTM!The
logfunction correctly accepts an optional complex base, delegating topymath::cmath::log. This matches CPython'scmath.log(x[, base])signature.
160-169: LGTM!The
iscloseimplementation is consistent withmath.isclose, properly adapted for complex numbers while maintaining the same tolerance validation.
Sorry, something went wrong.
|
@ShaharNaveh Not necessarily separated repository. It is quite standalone with zero dependency to any other RustPython details, so I'd like to promote it as a separated product. In the similar way, we incubated malachite-bigint out of RustPython main repository. (The project is not belong to RustPython project anymore because we found donating it to malachite project could be better.) |
Sorry, something went wrong.
youknowone
merged commit f9e2f9d
into
RustPython:main
Jan 13, 2026
…on#6705) * Upgrade test_math, test_cmath from 3.14.2 * pymath 0.1.5
Reviewers
2 participants
We are not shipping 3.14 libraries yet, but pymath is 3.14 compatible.
Also totally remove math calculation from RustPython, and delegate everything to pymath, which is smaller and easier to test compatibilities.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.