Add decimal package#3495
Open
bobzhang wants to merge 4 commits into
Open
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
No behavior change. Enriches the docstrings on every public function, trait impl, error variant, and the `Decimal` / `RoundingMode` / `DecimalError` types with worked examples, edge cases, and rationale: - Errors document each variant's trigger. - `RoundingMode` separates directional modes from half modes and explains why directional naming (`TowardZero`, `TowardNegativeInfinity`) is preferred over the ambiguous "up"/"down". - `Decimal`'s canonical model is called out, including its implications for trailing zeros and JSON-as-string. - Each constructor / accessor / arithmetic / division / rounding entry has examples and lists the failure modes (raise vs abort). - The deliberate absence of `/` is documented alongside `div_exact` / `div_round`. mbti is byte-identical with origin (verified via `diff` against HEAD). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three bug fixes from a code-review pass:
1. `Compare` no longer aborts on extreme exponents.
`dec("1e2147483647").compare(dec("1e-2147483648"))` previously
aborted because `align_coefficients` tried to materialize a
coefficient scaling factor whose 10^diff exponent did not fit in
`Int`. Compare now uses sign + adjusted-exponent (digit count from
`BigInt::bit_length` plus exponent) to decide cleanly when
magnitudes are clearly apart, and only falls through to scaled
comparison when the exponent gap is bounded.
2. `div_round` no longer aborts on extreme shifts.
The shift expression was a nested `sub_exponents` that could abort
before the existing pathological-shift handler at line ~140 had a
chance to run. Refactored to compute the shift in `Int64`
throughout, only narrowing to `Int` once we know the value is
within range. Out-of-range scale or shift now raises
`ExponentOutOfRange` (new variant) rather than aborting.
3. Parser rejects `_` adjacent to `.`.
`"1._2"` and `"._5"` were silently accepted because the underscore
rule only checked `has_digit && !after_underscore` and missed the
"previous token was a dot" case. Tracking `after_dot` plugs that
gap. The complementary `"1_.2"` case was already rejected.
Also:
- `DecimalError::ExponentOutOfRange` added so `round` and `div_round`
can report scale-driven exponent overflow as a typed error rather
than aborting (the prior `panic round scale out of range` test now
asserts the error instead).
- `round` signature now `raise DecimalError`; tests adjusted.
- Removed unused private helper `scale_to_exponent`.
mbti delta:
- adds `DecimalError::ExponentOutOfRange`;
- changes `Decimal::round` to `... raise DecimalError`.
Tests added for each fix; suite is 18/18 green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Coverage Report for CI Build 4066Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage decreased (-0.1%) to 94.741%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Comment on lines
+152
to
+161
| if neg_shift > max_exponent64 { | ||
| // The exact value is more than `max_exponent64` decimal places | ||
| // below the target scale, so the magnitude is strictly below | ||
| // half of one unit. Rounding therefore depends only on the | ||
| // sign of the ratio and the rounding mode; we settle the result | ||
| // without materializing the (infeasibly large) divisor. | ||
| return make_decimal( | ||
| rounded_ratio_below_half(self.coefficient, other.coefficient, mode), | ||
| target_exponent, | ||
| ) |
There was a problem hiding this comment.
🟡 rounded_ratio_below_half makes incorrect assumption when coefficients are very large
In div_round, when neg_shift > max_exponent64, the code assumes |self.coefficient / other.coefficient * 10^(-neg_shift)| < 0.5 and delegates to rounded_ratio_below_half. This assumption is only based on the exponent gap being very large, but ignores that self.coefficient could have far more decimal digits than 10^neg_shift. For example, if self has a canonical coefficient with 3 billion digits (exponent = -2147483648) and other has coefficient 1 (exponent = 0), with scale=0: neg_shift = 2147483648, triggering the below-half path. But the actual value is self.coefficient * 10^(-2147483648), which is approximately 10^(3e9 - 2.1e9) = 10^(8.5e8) — an astronomically large number, not below 0.5. The function would incorrectly return 0 (for HalfEven) instead of the correct huge integer.
Triggering this requires self.coefficient to have at least ~2.1 billion decimal digits (~2 GB of RAM for the coefficient alone), so it is practically infeasible on most systems, but the mathematical claim in the code is provably wrong for valid BigInt inputs.
Prompt for agents
In div_round (decimal/division.mbt), the branch at line 152 where neg_shift > max_exponent64 assumes |numerator/denominator| < 0.5 based solely on the exponent gap, but this ignores the magnitude of the coefficients. When self.coefficient has significantly more digits than 10^neg_shift, the actual ratio can be >= 0.5 or even very large.
To fix this properly, instead of calling rounded_ratio_below_half, the code should compare the digit counts (e.g., via bit_length) of self.coefficient versus other.coefficient plus neg_shift decimal digits. If self.coefficient is large enough that the ratio could be >= 0.5, the code needs to either raise ExponentOutOfRange (since materializing the result would be infeasible) or find a way to compute the ratio without materializing 10^neg_shift (e.g., by scaling both sides). The simplest correct fix is likely to check whether log10(|self.coefficient|) - log10(|other.coefficient|) > neg_shift (using bit_length bounds) and raise ExponentOutOfRange if so, since the result coefficient would itself be infeasibly large.
Was this helpful? React with 👍 or 👎 to provide feedback.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
moonbitlang/core/decimalpackage backed by@bigint.BigInt.coefficient * 10^exponent, including canonical zero and value-consistentEq/Hash./intentionally; division requires exactness or an explicit rounding scale/mode.Claude CLI feedback incorporated
BigInt::powand made negative powers fail loudly.Showoutput.suberrorusage.Validation
moon fmtmoon infomoon check decimal(passes; existing unrelated warnings only)moon test decimal(15 passed)moon check --target all(passes; existing unrelated warnings only)moon test(6403 passed)