Add decimal module (#218)#538
Conversation
Merging this PR will not alter performance
Comparing 
Footnotes
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 944dd528e2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ExcType::DecimalException => decimal_exc(py, "DecimalException", msg), | ||
| ExcType::DecimalInvalidOperation => decimal_exc(py, "InvalidOperation", msg), | ||
| ExcType::DecimalDivisionByZero => decimal_exc(py, "DivisionByZero", msg), |
There was a problem hiding this comment.
This adds Monty→Python reconstruction for decimal.* exceptions, but the reverse path still goes through py_err_to_exc_type, whose ArithmeticError branch only recognizes built-in ZeroDivisionError/OverflowError. When a Python external callback raises decimal.InvalidOperation (or decimal.DivisionByZero, etc.), it is converted back into ArithmeticError/ZeroDivisionError, so sandbox code with except decimal.InvalidOperation: will not catch it even though the decimal exception classes are now exposed. Add the symmetric decimal isinstance checks before the built-in arithmetic fallback.
Useful? React with 👍 / 👎.
| field_names, | ||
| vec![sign, digits_tuple, exponent], | ||
| ); | ||
| Ok(Value::Ref(vm.heap.allocate(HeapData::NamedTuple(named))?)) |
There was a problem hiding this comment.
If Decimal.as_tuple() runs under a memory limit and the final NamedTuple heap allocation fails, the already-created digits_tuple (and the special-value exponent string just above) are dropped without drop_with_heap because they are inside the NamedTuple passed to heap.allocate. That leaks refcounts in production and can panic under memory-model-checks; guard or explicitly clean up these Value::Refs on the allocation error path.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
this file is almost certainly not vendored, looks suspicious?
There was a problem hiding this comment.
Yep, it's duplicate of crates/monty-typeshed/custom/decimal.pyi, the duplication seems intended according to the crate's README. (copied by monty-typeshed/update.py#L332-L337, introduced in #48)
| /// Python's `==` *operator* (CPython's `do_richcompare`), which — unlike | ||
| /// [`Self::py_eq`] — has no same-object shortcut: identity is only the | ||
| /// default `object.__eq__`, consulted after the type's own equality. | ||
| /// | ||
| /// The distinction matters for the first type with non-reflexive equality: | ||
| /// `x = Decimal('NaN'); x == x` must be `False` (and `Decimal('sNaN')` | ||
| /// must raise `InvalidOperation`) even though `{x: 1}[x]` still succeeds | ||
| /// via the container shortcut. For every type whose `py_eq_impl` returns | ||
| /// `NotImplemented` on itself (class instances, files, …), the identity | ||
| /// fallback keeps `obj == obj` true. | ||
| pub fn py_eq_operator(&self, other: &Self, vm: &mut VM<'_, impl ResourceTracker>) -> RunResult<bool> { | ||
| if let (Self::Ref(id), Self::Ref(other_id)) = (self, other) | ||
| && id == other_id | ||
| { | ||
| match vm.heap.read(*id).py_eq_impl(other, vm)? { | ||
| Some(result) => Ok(result), | ||
| None => Ok(true), // default object identity | ||
| } | ||
| } else { | ||
| self.py_eq(other, vm) | ||
| } | ||
| } |
There was a problem hiding this comment.
Please revert this split; instead we should fix py_eq_impl to not have the short-circuit.
There was a problem hiding this comment.
The changes in this file are pretty gross; we should probably clean up the trait so that these changes aren't necessary and they are more localised to the decimal implementation.
| /// Returns an error if deserialization fails. | ||
| pub fn load(bytes: &[u8]) -> Result<Self, postcard::Error> { | ||
| postcard::from_bytes(bytes) | ||
| from_snapshot_bytes(bytes) |
There was a problem hiding this comment.
This general set of changes seems suspect, should either be split out and justified separately or reverted.
There was a problem hiding this comment.
This seems problematic; why not use a rust implementation such as rust-decimal?
There was a problem hiding this comment.
I initially tried to base the implementation on fastnum, which seemed like a good fit given it tries to follow IBM’s General Decimal Arithmetic Specification, like python stdlib decimal. But Claude kept layering on workarounds for various operations because of subtle differences in the observed behavior in tests (e.g. Decimal('-0') // Decimal('1') produces Decimal('-0') in Python preserving the sign, which wasn't the case with the out of the box implementation in fastnum). I also had implementation of decimal Context in-scope in that fastnum based port, which compounded the complexity. When I decided to re-implement again, I thought letting Claude port limited part of _pydecimal may not be a bad idea, and it also avoided the need to bring in another dependency.
Let me know if you'd prefer me to open an alternative PR with the fastnum based implementation, or iterate on this PR to make it merge-able.
2 participants
Closes #218
Summary
Implements
decimal.Decimalas a Rust port of CPython 3.14's_pydecimal.pynumeric core, running under a fixed context matching CPython's defaults (prec=28,ROUND_HALF_EVEN,Emax=±999999). Arithmetic results are digit-for-digit identical to CPython's — the ported functions cite their_pydecimalline numbers so they can be diffed against the original.Divergences from CPython (fixed context, reduced method surface, input caps, qualified
__name__s) are documented inlimitations/decimal.md.Assisted-by: Claude:claude-5-fable
Implementation notes
crates/monty/src/types/decimal/): construction fromstr | int | float | Decimaland theas_tupleform;add/sub/mul/div,floordiv/mod/divmod, 2- and 3-argpow,sqrt/ln/log10/exp,quantize/normalizeto_integral_value, theis_*predicates,as_tuple/adjusted/copy_*, and the builtinsint()/float()/round()/abs()/divmod()/pow()/hash()plusmath.floor/ceil/trunc.int↔strlimit) and bounds literal exponents; attacker-scalable allocations (powers of ten, format zero-pads) are charged to theResourceTracker, raisingMemoryErrorwhere CPython would build a multi-GB value.decimal::binary_op_valueprobe serves all seven binary operators and thepow()builtin — promotion runs before the coefficient is cloned, and the probe owns CPython's sequence-repeat special case ('a' * Decimal(2)).nan != naneven for the same object);hash(Decimal(5)) == hash(5)cross-type consistency in-sandbox.InvalidOperation,DivisionByZero,Overflow) ever raise.str/reprand the full__format__spec mini-language, emitted through tracker-reservedStringBuilders.HeapData::Decimal+HeapReadOutputvariants (appended last for snapshot index stability, likeType/MontyObject/MontyType); snapshot deserialization validates Decimal payloads and rejects corrupt bytes safely.Decimalcrosses losslessly as its canonical string —MontyObject::Decimalthrough the wire protocol (proto tag appended),decimal.Decimalinpydantic_monty, a tagged string in@pydantic/monty; hand-authored typeshed stub advertises only the implemented surface.test_cases/decimal__*.pyfiles assert exact values and exception messages under both CPython and Monty;decimal_guards.rscovers the resource-limit paths; binary/JSON serde, host-input, and Python/JS binding tests cover the boundary.Summary by cubic
Adds a
decimalmodule implementingdecimal.Decimalunder a fixed context matching CPython defaults (prec=28,ROUND_HALF_EVEN, Emax=±999999). Results, formatting, comparisons, and exceptions match CPython; values cross the host boundary losslessly as canonical strings.New Features
decimal.Decimalport with constructors (str/int/float/Decimal/as_tuple), arithmetic (+ - * / // % divmod powincl. 3-arg), transcendentals (sqrt ln log10 exp), methods (quantize normalize to_integral_value copy_sign as_tuple), predicates, and__format__; builtinsint/float/round/abs/divmod/pow/hashandmath.floor/ceil/truncwired up.hash(Decimal(5)) == hash(5)); VM equality operator updated to honor Decimal semantics.decimalexception hierarchy (decimal.DecimalException,InvalidOperation,DivisionByZero,Overflow, etc.); only the default-trapped signals raise.MontyObject::Decimalcarried as the canonical string (e.g."1.20","1E+5","NaN"); Protobuf adds field 29; JSON uses externally tagged form{"Decimal":"<str>"}.{ __monty_type__: 'Decimal', value: '<str>' }round-trips; Python:Decimalconverts viastr(); bindings updated inpydantic_montyand@pydantic/monty.10**kand format zero-pads are charged to the resource tracker; oversized boundary inputs are rejected.decimal.pyisubset (fixed-context surface) and enabled in version tables; type-checking tests cover common operations.decimalis now importable; README updated.Migration
MontyObject::Decimal(Protobuf tag 29; JSON{"Decimal":"<str>"}) and, in JS,{ __monty_type__: 'Decimal', value: '<str>' }.decimal.*exception names soexcept decimal.*in-sandbox catches mapped host errors.Written for commit 944dd52. Summary will update on new commits.