Upgrade math,cmath to 3.14.2 and totally delegate to pymath #6705
Conversation
📝 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 |
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin math |
youknowone
force-pushed
the
math
branch
6 times, most recently
from
c456550 to
f133d7e
Compare
![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.
ShaharNaveh
left a comment
ShaharNaveh
left a comment
There was a problem hiding this comment.
Overall it looks great!
| self.assertEqual(sumprod([True, False] * 10, [0.1] * 20), 1.0) | ||
| self.assertEqual(sumprod([1.0, 10E100, 1.0, -10E100], [1.0]*4), 2.0) | ||
|
|
||
| @unittest.skip("TODO: RUSTPYTHON, Taking a few minutes.") |
|
Is there a reason why |
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.
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.
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.
| fn log(x: PyObjectRef, base: OptionalArg<ArgIntoFloat>, vm: &VirtualMachine) -> PyResult<f64> { | ||
| let base: f64 = base.map(Into::into).unwrap_or(core::f64::consts::E); | ||
| if base.is_sign_negative() { | ||
| return Err(vm.new_value_error("math domain error")); | ||
| let base = base.into_option().map(|v| v.into_float()); | ||
| // Check base first for proper error messages | ||
| if let Some(b) = base { | ||
| if b <= 0.0 { | ||
| return Err(vm.new_value_error(format!( | ||
| "expected a positive input, got {}", | ||
| super::float_repr(b) | ||
| ))); | ||
| } | ||
| if b == 1.0 { | ||
| return Err(vm.new_value_error("math domain error".to_owned())); | ||
| } | ||
| } | ||
| // Handle BigInt specially for large values (only for actual int type, not float) | ||
| if let Some(i) = x.downcast_ref::<PyInt>() { | ||
| return pymath::math::log_bigint(i.as_bigint(), base).map_err(|err| match err { | ||
| pymath::Error::EDOM => vm.new_value_error("expected a positive input".to_owned()), | ||
| _ => pymath_exception(err, vm), | ||
| }); | ||
| } | ||
| log2(x, vm).map(|log_x| log_x / base.log2()) | ||
| let val = x.try_float(vm)?.to_f64(); | ||
| pymath::math::log(val, base).map_err(|err| match err { | ||
| pymath::Error::EDOM => vm.new_value_error(format!( | ||
| "expected a positive input, got {}", | ||
| super::float_repr(val) | ||
| )), | ||
| _ => pymath_exception(err, vm), | ||
| }) | ||
| } | ||
|
|
||
| #[pyfunction] | ||
| fn log1p(x: ArgIntoFloat, vm: &VirtualMachine) -> PyResult<f64> { | ||
| let x = x.into_float(); | ||
| if x.is_nan() || x > -1.0_f64 { | ||
| Ok(x.ln_1p()) | ||
| } else { | ||
| Err(vm.new_value_error("math domain error")) | ||
| } | ||
| } | ||
|
|
||
| /// Generates the base-2 logarithm of a BigInt `x` | ||
| fn int_log2(x: &BigInt) -> f64 { | ||
| // log2(x) = log2(2^n * 2^-n * x) = n + log2(x/2^n) | ||
| // If we set 2^n to be the greatest power of 2 below x, then x/2^n is in [1, 2), and can | ||
| // thus be converted into a float. | ||
| let n = x.bits() as u32 - 1; | ||
| let frac = true_div(x, &BigInt::from(2).pow(n)); | ||
| f64::from(n) + frac.log2() | ||
| pymath::math::log1p(x.into_float()).map_err(|err| pymath_exception(err, vm)) | ||
| } | ||
|
|
||
| #[pyfunction] | ||
| fn log2(x: PyObjectRef, vm: &VirtualMachine) -> PyResult<f64> { | ||
| match x.try_float(vm) { | ||
| Ok(x) => { | ||
| let x = x.to_f64(); | ||
| if x.is_nan() || x > 0.0_f64 { | ||
| Ok(x.log2()) | ||
| } else { | ||
| Err(vm.new_value_error("math domain error")) | ||
| } | ||
| } | ||
| Err(float_err) => { | ||
| if let Ok(x) = x.try_int(vm) { | ||
| let x = x.as_bigint(); | ||
| if x.is_positive() { | ||
| Ok(int_log2(x)) | ||
| } else { | ||
| Err(vm.new_value_error("math domain error")) | ||
| } | ||
| } else { | ||
| // Return the float error, as it will be more intuitive to users | ||
| Err(float_err) | ||
| } | ||
| } | ||
| // Handle BigInt specially for large values (only for actual int type, not float) | ||
| if let Some(i) = x.downcast_ref::<PyInt>() { | ||
| return pymath::math::log2_bigint(i.as_bigint()).map_err(|err| match err { | ||
| pymath::Error::EDOM => vm.new_value_error("expected a positive input".to_owned()), | ||
| _ => pymath_exception(err, vm), | ||
| }); | ||
| } | ||
| let val = x.try_float(vm)?.to_f64(); | ||
| pymath::math::log2(val).map_err(|err| match err { | ||
| pymath::Error::EDOM => vm.new_value_error(format!( | ||
| "expected a positive input, got {}", | ||
| super::float_repr(val) | ||
| )), | ||
| _ => pymath_exception(err, vm), | ||
| }) | ||
| } | ||
|
|
||
| #[pyfunction] | ||
| fn log10(x: PyObjectRef, vm: &VirtualMachine) -> PyResult<f64> { | ||
| log2(x, vm).map(|log_x| log_x / 10f64.log2()) | ||
| } | ||
|
|
||
| #[pyfunction] | ||
| fn pow(x: ArgIntoFloat, y: ArgIntoFloat, vm: &VirtualMachine) -> PyResult<f64> { | ||
| let x = x.into_float(); | ||
| let y = y.into_float(); | ||
|
|
||
| if x < 0.0 && x.is_finite() && y.fract() != 0.0 && y.is_finite() | ||
| || x == 0.0 && y < 0.0 && y != f64::NEG_INFINITY | ||
| { | ||
| return Err(vm.new_value_error("math domain error")); | ||
| } | ||
|
|
||
| let value = x.powf(y); | ||
|
|
||
| if x.is_finite() && y.is_finite() && value.is_infinite() { | ||
| return Err(vm.new_overflow_error("math range error")); | ||
| // Handle BigInt specially for large values (only for actual int type, not float) | ||
| if let Some(i) = x.downcast_ref::<PyInt>() { | ||
| return pymath::math::log10_bigint(i.as_bigint()).map_err(|err| match err { | ||
| pymath::Error::EDOM => vm.new_value_error("expected a positive input".to_owned()), | ||
| _ => pymath_exception(err, vm), | ||
| }); | ||
| } | ||
|
|
||
| Ok(value) | ||
| let val = x.try_float(vm)?.to_f64(); | ||
| pymath::math::log10(val).map_err(|err| match err { | ||
| pymath::Error::EDOM => vm.new_value_error(format!( | ||
| "expected a positive input, got {}", | ||
| super::float_repr(val) | ||
| )), | ||
| _ => pymath_exception(err, vm), | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Python 3.14.2 math.log math.log2 math.log10 error messages ValueError for negative input base validation
💡 Result:
Short answer — cause and validation:
-
math.log(x), math.log2(x), math.log10(x) raise ValueError for x <= 0 (Python prints "ValueError: math domain error"). Validate with x > 0 before calling. [1][2]
-
math.log(x, base) is computed as log(x)/log(base); therefore base must be > 0 and base != 1 (because log(base) must be defined and nonzero). Validate with: x > 0 and base > 0 and base != 1. [1]
Minimal validation example:
import math
def safe_log(x, base=None):
if x <= 0:
raise ValueError("x must be > 0")
if base is None:
return math.log(x)
if base <= 0 or base == 1:
raise ValueError("base must be > 0 and != 1")
return math.log(x, base)References: Python math docs (math.log, log2, log10). [1] Example/error text showing "ValueError: math domain error". [2]
Sources:
[1] Python 3.14 math module documentation — math.log / math.log2 / math.log10.
[2] Examples showing the ValueError: math domain error for log on non‑positive inputs.
🏁 Script executed:
# First, let's look at the exact implementation in the file
head -200 crates/stdlib/src/math.rs | tail -120Repository: RustPython/RustPython
Length of output: 186
🌐 Web query:
Python 3.14 math.log base <= 0 base == 1.0 exact error message ValueError
💡 Result:
Short answer:
- If x <= 0: math.log(...) raises ValueError with message "math domain error". [1]
- If base == 1.0 (two-argument form): math.log(x, base) is computed as log(x)/log(base); log(1.0) == 0 so you get a ZeroDivisionError with message "float division by zero". [1][2]
Sources:
- math.log documentation (shows log(x, base) computed as log(x)/log(base)). [1]
- Example/diagnosis of the ZeroDivisionError message ("ZeroDivisionError: float division by zero"). [2]
References:
[1] Python 3.14 math — Mathematical functions.
[2] Stack Overflow / common Python ZeroDivisionError examples.
🌐 Web query:
CPython math.log source code error message "base must be" positive
💡 Result:
Short answer: CPython's math.log doesn't raise a custom "base must be …" message — it calls the internal loghelper which raises "math domain error" for non-positive integers, and for other numeric types the underlying libm/errno handling produces ValueError/ZeroDivisionError as appropriate. See the loghelper and math_log_impl implementation in Modules/mathmodule.c. [1]
References:
- loghelper implementation (raises "math domain error" for non-positive PyLongs) and math.log implementation calling it. [1]
[1] Modules/mathmodule.c (CPython source).
🏁 Script executed:
# Try to access the file using git
cd /tmp && git clone --depth 1 https://github.com/RustPython/RustPython.git 2>&1 | head -20Repository: RustPython/RustPython
Length of output: 155
🏁 Script executed:
cat /tmp/RustPython/crates/stdlib/src/math.rs | head -200 | tail -120Repository: RustPython/RustPython
Length of output: 4103
🏁 Script executed:
# Also search for pymath module to understand the EDOM error handling
grep -r "EDOM" /tmp/RustPython/crates/stdlib/src/math.rs -A 3 -B 3Repository: RustPython/RustPython
Length of output: 331
log/log2/log10: error messages and base validation do not match CPython 3.14 behavior.
The proposed changes introduce incompatibilities with CPython:
-
Base validation (base ≤ 0): Uses
"expected a positive input, got {b}"instead of CPython's"math domain error". Message should match CPython's standard error. -
Base == 1.0: Currently returns
ValueError("math domain error"), but CPython raisesZeroDivisionError("float division by zero")since the function computeslog(x) / log(base), makinglog(1.0) = 0cause division by zero. This must be handled separately. -
x ≤ 0 error messages: Both float and BigInt paths use
"expected a positive input..."instead of CPython's standard"math domain error". -
Inconsistent BigInt/float messages: BigInt EDOM returns
"expected a positive input"while float returns"expected a positive input, got {val}"— either match CPython's standard message or ensure consistency.
Align all error messages with CPython's actual ValueError/ZeroDivisionError behavior and message text.
| pub(crate) fn pymath_exception(err: pymath::Error, vm: &VirtualMachine) -> PyBaseExceptionRef { | ||
| match err { | ||
| pymath::Error::EDOM => vm.new_value_error("math domain error"), | ||
| pymath::Error::ERANGE => vm.new_overflow_error("math range error"), | ||
| } | ||
| } | ||
|
|
||
| /// Format a float in Python style (ensures trailing .0 for integers). | ||
| fn float_repr(value: f64) -> String { | ||
| if value.is_nan() { | ||
| "nan".to_owned() | ||
| } else if value.is_infinite() { | ||
| if value.is_sign_positive() { | ||
| "inf".to_owned() | ||
| } else { | ||
| "-inf".to_owned() | ||
| } | ||
| } else { | ||
| let s = format!("{}", value); | ||
| // If no decimal point and not in scientific notation, add .0 | ||
| if !s.contains('.') && !s.contains('e') && !s.contains('E') { | ||
| format!("{}.0", s) | ||
| } else { | ||
| s | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Python 3.14 float repr formatting rules scientific notation thresholds
💡 Result:
Short answer
- The formatting “general” type (g/G or default none) switches to scientific (exponent) when the decimal exponent would be < −4 or ≥ precision. Otherwise it prints fixed-point. The rule is: compute the result as if using 'e' with precision p−1 and exponent exp; use fixed if −4 ≤ exp < p, else use exponent form. [1][2]
Details & useful defaults
- For format(..., 'g') the default precision p = 6. A precision of 0 is treated as 1. Trailing insignificant zeros and an unnecessary decimal point are removed. [1][2]
- The empty format spec (str(x)) behaves like 'g' with Python’s str/repr defaults: repr/str now emit the shortest decimal string that round-trips to the same float (the “shortest round‑trip” algorithm), so str/repr will produce as‑needed digits rather than always 6 or 17. (Python adopted a shortest-round‑trip algorithm for float reprs; see “What’s New”.) [3]
References
- Python Format Specification Mini‑Language (floating types: e, f, g semantics). [1]
- Explanation of general-format switching (−4 ≤ exp < p rule) and default precision behavior. [2]
- Python “What’s New” / repr change: shortest round‑trip representations for floats. [3]
If you want, I can show short examples illustrating the threshold behavior in code.
🏁 Script executed:
# Find where float_repr() is used in the codebase
rg "float_repr" --type rs -B 2 -A 2Repository: RustPython/RustPython
Length of output: 186
🏁 Script executed:
# Look for existing float repr utilities in RustPython codebase
find . -name "*.rs" -type f | xargs grep -l "repr.*float\|float.*repr" | head -20Repository: RustPython/RustPython
Length of output: 186
🏁 Script executed:
# Check if there's a float type with repr method in the codebase
rg "impl.*PyFloat\|fn.*repr.*float" --type rs -A 3 | head -50Repository: RustPython/RustPython
Length of output: 186
🏁 Script executed:
# Search for float_repr usage in the current repository
rg "float_repr" --type rs -B 3 -A 3Repository: RustPython/RustPython
Length of output: 91
🏁 Script executed:
# Look for float repr utilities in the current repository
rg "float.*repr|repr.*float" --type rs -i | head -30Repository: RustPython/RustPython
Length of output: 91
🏁 Script executed:
# Check the broader math module structure and float utilities
cd crates/stdlib/src && ls -la | grep -E "math|float" && cd -Repository: RustPython/RustPython
Length of output: 202
🏁 Script executed:
# Search for any existing float formatting utilities in RustPython crate
find . -path "*/src/*" -name "*.rs" -type f | xargs grep -l "PyFloat\|float.*to_string\|float.*format" | head -20Repository: RustPython/RustPython
Length of output: 695
🏁 Script executed:
# Search for float_repr usage without type flag
rg "float_repr" -B 3 -A 3Repository: RustPython/RustPython
Length of output: 17467
🏁 Script executed:
# Check the PyFloat implementation for repr
cat crates/vm/src/builtins/float.rs | head -100Repository: RustPython/RustPython
Length of output: 3180
🏁 Script executed:
# Search for where float_repr is called from
rg "float_repr\(" -B 5 -A 2Repository: RustPython/RustPython
Length of output: 7373
🏁 Script executed:
# Check the literal float module
head -50 crates/literal/src/float.rsRepository: RustPython/RustPython
Length of output: 1343
🏁 Script executed:
# Search for Representable trait implementation for PyFloat
rg "impl.*Representable.*PyFloat|fn repr.*float" -A 5Repository: RustPython/RustPython
Length of output: 445
🏁 Script executed:
# Check how PyFloat implements repr
rg "PyFloat" crates/vm/src/builtins/float.rs -A 3 -B 1 | head -80Repository: RustPython/RustPython
Length of output: 2760
🏁 Script executed:
# Look for any existing float repr utilities accessible from stdlib
rg "to_string|repr" crates/vm/src/builtins/float.rs | head -20Repository: RustPython/RustPython
Length of output: 317
🏁 Script executed:
# Check if there are tests for math error messages to understand the impact
rg "math.*error\|math.*domain\|expected.*input" Lib/test/ 2>/dev/null | head -15Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Check the signature and full implementation of literal::float::to_string
rg "pub fn to_string" crates/literal/src/float.rs -A 20Repository: RustPython/RustPython
Length of output: 702
🏁 Script executed:
# Verify that literal::float is accessible from stdlib/math.rs context
head -30 crates/stdlib/src/math.rs | grep -E "use|mod"Repository: RustPython/RustPython
Length of output: 388
🏁 Script executed:
# Search for any other uses of literal::float in the stdlib to see the import pattern
rg "literal::float" crates/stdlib/src/ -B 2 -A 2Repository: RustPython/RustPython
Length of output: 47
Replace float_repr() with RustPython's standard float formatting to ensure Python-compatible output in error messages.
The current implementation using format!("{}", value) does not match Python's float repr behavior. RustPython provides crate::literal::float::to_string() which correctly implements Python's float formatting rules (including shortest round-trip representation and proper exponent thresholds). Use that instead of the custom implementation to match Python's output in math domain error messages.
🤖 Prompt for AI Agents
In @crates/stdlib/src/math.rs around lines 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.
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.
|
@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.) |
…on#6705) * Upgrade test_math, test_cmath from 3.14.2 * pymath 0.1.5
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.