fix(bigint): treat empty octets as zero in BigInt::from_octets - #4115
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Aligns MoonBit’s BigInt::from_octets with mainstream bigint APIs by normalizing an empty magnitude (b"") to zero (instead of panicking for non-zero signum), and updates docs/tests accordingly.
Changes:
- Make
BigInt::from_octetsreturn zero on empty input for all signum paths (JS + non-JS implementations). - Update public doc comments and examples to describe/cover empty-input behavior.
- Adjust tests: extend empty-input assertions and remove now-obsolete panic tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| bigint/bigint_nonjs.mbt | Normalize empty input to zero and update docs/examples for the non-JS implementation. |
| bigint/bigint_js.mbt | Normalize empty input to 0N and update docs for the JS implementation. |
| bigint/bigint_test.mbt | Extend regression coverage for empty input across default/0/-1 signum. |
| bigint/panic_test.mbt | Remove panic tests that no longer apply after behavior change. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
1430
to
1434
| /// * `bytes` : A sequence of bytes representing the magnitude of the number in | ||
| /// big-endian order. The sequence must not be empty unless `sign` is 0. | ||
| /// big-endian order. An empty sequence represents a zero magnitude. | ||
| /// * `sign` : An integer specifying the sign of the resulting number (default: | ||
| /// 1). A value of 1 creates a positive number, -1 creates a negative number, and | ||
| /// 0 returns zero regardless of the input bytes. |
Contributor
Author
There was a problem hiding this comment.
Fixed in 2c92339 — the docstring now uses input/signum to match the signature.
Comment on lines
+1439
to
+1440
| /// An empty byte sequence yields zero for any `sign`, matching the behavior of | ||
| /// Java's `BigInteger`, Python's `int.from_bytes`, and Rust's `num-bigint`. |
Contributor
Author
There was a problem hiding this comment.
Fixed in 2c92339 — the note now says signum, consistent with the signature and examples.
bobzhang
force-pushed
the
hongbo/from_octets_empty_zero
branch
from
August 20, 2026 07:42
9465eb1 to
feaa577
Compare
Collaborator
Coverage Report for CI Build 6237Coverage remained the same at 90.734%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
bobzhang
added a commit
that referenced
this pull request
Aug 20, 2026
The docstring referred to `bytes`/`sign` while the actual parameters are `input`/`signum`. Addresses Copilot review comments on #4115. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bobzhang
added a commit
that referenced
this pull request
Aug 20, 2026
The docstring referred to `bytes`/`sign` while the actual parameters are `input`/`signum`. Addresses Copilot review comments on #4115. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bobzhang
force-pushed
the
hongbo/from_octets_empty_zero
branch
from
August 20, 2026 08:43
2c92339 to
a670704
Compare
bobzhang
enabled auto-merge (squash)
August 20, 2026 08:44
An empty big-endian magnitude naturally denotes zero, and mainstream bigint APIs agree: Java's BigInteger(signum, magnitude) permits a zero-length magnitude for any signum, Python's int.from_bytes(b"") returns 0, and Rust's num-bigint normalizes Sign::Minus with an empty magnitude to zero. Previously MoonBit was the outlier, aborting with "empty octet string" whenever signum != 0, while still accepting the morally identical all-zero magnitude (e.g. b"\x00" with signum=-1). Relax the panic on both backends to return zero, update the docs, and replace the panic tests with value tests covering every signum. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The docstring referred to `bytes`/`sign` while the actual parameters are `input`/`signum`. Addresses Copilot review comments on #4115. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bobzhang
force-pushed
the
hongbo/from_octets_empty_zero
branch
from
August 20, 2026 09:09
a670704 to
1d261a8
Compare
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
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
Follow mainstream bigint behavior by treating an empty octet sequence as zero in
BigInt::from_octets, instead of panicking whensignum != 0.An empty big-endian magnitude naturally denotes zero, and the mainstream APIs agree:
BigInteger(signum, magnitude)explicitly permits a zero-length magnitude and returns 0 for any signumint.from_bytes(b"")returns0num_bigint::BigInt::from_bytes_be(Sign::Minus, b"")normalizes to zero;BigUint::from_bytes_be(b"")returns 0 (same foribig,rug/GMP)MoonBit was the outlier with
abort("empty octet string"), and inconsistently so: the morally identical all-zero magnitude (b"\x00"withsignum=-1) was already accepted and returned 0, while the zero-length one panicked.Changes
bigint/bigint_nonjs.mbt,bigint/bigint_js.mbt: return zero for empty input on all signum paths; update doc comments (and add an empty-input line to the docstring example)bigint/bigint_test.mbt: extend the empty-input test to coversignumdefault,0, and-1bigint/panic_test.mbt: remove the two now-obsolete panic testsRelaxing a documented panic into defined behavior; no public API signature changes (
moon inforeports no.mbtidiff).Follow-up to the contract discussion around #3748 / #3750.
Validation
moon fmtmoon check --target allmoon test bigint --target all(187/187 on wasm, wasm-gc, native; 162/162 on js)moon info(no.mbtichanges)🤖 Generated with Claude Code