Read a BigInteger's bytes as bytes, so converting one to a Number stops throwing (#585) - #944
Merged
Merged
Conversation
…ps throwing (#585) Entity and Entity.Number both offer an implicit conversion from System.Numerics.BigInteger, and they did not agree. Entity's reads the two's-complement bytes. Number's handed the same bytes to EInteger.FromString, whose byte[] overload reads them as ASCII digits: Entity fine = new BigInteger(123456789); // 123456789 Entity.Number threw = new BigInteger(123456789); // FormatException: Illegal character found So it failed for every value whose bytes are not digit characters, which is nearly all of them and includes 1. The handful that worked worked by accident: 12594 is the two bytes '2' and '1', and came back as 21. Nothing inside the library reaches that conversion, which is why nothing caught it -- it exists only for callers. Both now read the bytes, and the test requires the two conversions to agree rather than each to work on its own, since one value converted two ways giving two answers is the defect and not merely the exception. Found while documenting the members a CS1591 pragma was covering. The two conversions are in different files and neither looks wrong alone; reading them side by side is what showed it, and reading them side by side is what documenting them required. Measured: suite 7175 passed / 0 failed, 58 of them new, over values on both sides of every byte boundary that matters -- 127, 128, 255, 256, the long limits, and a value past 2^127. #585 Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…utside the parser (#585) The block this fix edits was the one region still suppressing the missing-documentation warning once the rest were dealt with, and it is documented here rather than on the documentation branch because two branches editing one block conflict for whichever merges second. Each conversion lands on the narrowest kind that holds the value exactly, and the float and double ones read the binary value that was stored rather than the decimal that was written. The BigInteger one now says in its remarks what it used to do and why that threw. With this and the documentation branch, no CS1591 pragma remains in the library outside the generated parser, which keeps its own because it is regenerated from the grammar. Measured: suite 7175 passed / 0 failed. #585 Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.
EntityandEntity.Numberboth offer an implicit conversion fromSystem.Numerics.BigInteger, and they did not agree.Entity's reads the two's-complement bytes.Number's handed the same bytes toEInteger.FromString, whosebyte[]overload reads them as ASCII digits:So it failed for every value whose bytes are not digit characters — nearly all of them,
1included. The handful that worked did so by accident:12594is the two bytes'2'and'1', and came back as21.Why nothing caught it
Nothing inside the library reaches that conversion. It exists only for callers, so no internal path and no test ever crossed it — the failure mode where a public API is broken precisely because it is public-only.
The test therefore requires the two conversions to agree, not merely that each works. One value converted two ways giving two answers is the defect; the exception is only how it showed up.
58 cases, over values on both sides of every byte boundary that matters — 127, 128, 255, 256, the
longlimits either side, and a value past 2^127 — plus the accidental-success case pinned so its old behaviour is on record.How it was found
While documenting the members a
#pragma warning disable CS1591was covering, for #585. The two conversions live in different files and neither looks wrong on its own; reading them side by side is what showed it, and reading them side by side is exactly what documenting them required.That is the second thing that sweep has turned up after the pragmas that were suppressing nothing.
Measured
Suite 7175 passed / 0 failed, 58 new. Recorded in
BREAKING-CHANGES.mdunder a new Unreleased — since 2.2.0 section: an exception becoming a value is a changed answer.🤖 Generated with Claude Code