fix(values): declare every implicit widening on the Int128 polyfill - #23
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the netstandard2.0 System.Int128 polyfill to match the BCL’s full set of implicit widening conversions, preventing consumer code that compiles on newer TFMs from failing to compile on downlevel targets (as described in #18 item 1).
Changes:
- Promotes
uint/ulongconversions into the polyfill fromexplicittoimplicit, and adds the remaining missing implicit widenings (byte,char,ushort,nint,nuint) to avoid CS0457 ambiguity scenarios. - Adds a new conversion-focused test suite that exercises the widenings in assignment and operator contexts (and guards against partial/ambiguous operator sets).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/Clast.DatabaseDecimal.Tests/Int128ConversionTests.cs | Adds compile-time-sensitive tests to ensure widenings are present and non-ambiguous on the polyfill path. |
| src/Clast.DatabaseDecimal/Polyfills/Int128.cs | Declares the full BCL-aligned set of implicit widening conversions (including ulong/nuint) in the polyfill. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
CurtHagenlocher
added a commit
that referenced
this pull request
Aug 22, 2026
Both from Copilot on #23, and both right. The comment claimed there are no casts in the class, which is not true and was never true: the tests cast to pick a source type, the last two cast to and from Int128 deliberately, and the repro casts high before shifting it. What is actually true, and what the comment meant, is narrower — the widening tests never cast *to* Int128, because a cast would bind to an explicit conversion and keep compiling with the implicit one missing. The four assertions in SignedWidenings_KeepTheirSign passed the computed value first and the literal second, so a failure there would have reported them the wrong way round. Every other assertion in the file, and in the suite, puts the expected value first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Code that widens into Int128 without a cast builds on net8.0 and net10.0, where the BCL type wins, and fails on netstandard2.0, where the polyfill does. #18 has the repro: reading a signed 96-bit little-endian integer needs `((Int128)high << 64) | low` with a ulong `low`, and that is CS0019 downlevel. Only two widenings actually break today, though, and the issue's table of eleven overstates it: C# will chain a standard conversion into a user-defined one, so byte reaches Int128 through int, and uint through long. What has no route is ulong and nuint, neither of which converts implicitly to long. The fix is still to declare all eleven, and that is not tidiness. C# resolves a user-defined conversion by finding the most encompassed source type among the candidates, so adding one operator can leave no unique answer where there was one before. Promoting only uint and ulong — which is the first thing the issue suggests — makes byte, char and ushort ambiguous between the int and long operators, so a partial fix trades two broken widenings for three. Declaring the whole set gives every source type an exact match and the question never arises. Promoting a widening from explicit to implicit is source-compatible in that direction, so callers who already wrote the cast keep building. The narrowing conversions are untouched and stay explicit. The tests assert at compile time before they assert anything at run time: every conversion is written without a cast, so the file fails to build on net472 if a widening goes missing or ambiguous, which is the failure mode that is invisible on net8.0 and net10.0. Checked in both directions — it fails against the current polyfill and against the partial fix, and passes against this one. UInt128 does not have the defect; it already declares its unsigned widenings implicitly. Refs #18 — item 1 (the conversions) only. Parse/TryParse, the non-generic interfaces and the static helpers are untouched, so the issue stays open for them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both from Copilot on #23, and both right. The comment claimed there are no casts in the class, which is not true and was never true: the tests cast to pick a source type, the last two cast to and from Int128 deliberately, and the repro casts high before shifting it. What is actually true, and what the comment meant, is narrower — the widening tests never cast *to* Int128, because a cast would bind to an explicit conversion and keep compiling with the implicit one missing. The four assertions in SignedWidenings_KeepTheirSign passed the computed value first and the literal second, so a failure there would have reported them the wrong way round. Every other assertion in the file, and in the suite, puts the expected value first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CurtHagenlocher
force-pushed
the
fix/int128-implicit-widenings
branch
from
August 22, 2026 20:50
74d3d16 to
75bb357
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.
Addresses item 1 of #18 — the conversions — and only that. Deliberately not a closing reference:
Parse/TryParse, the non-genericIComparable/IFormattable, and the static helpers (DivRem,Sign,Min/Max,LeadingZeroCount,PopCount) are untouched, so #18 should stay open after this merges. Item 1 is the one that breaks a consumer build, which is why it goes first.Two corrections to the issue
The break is narrower than the table suggests. Probing all eleven widenings against the polyfill, only two fail:
ulongandnuint. The other nine already compile, because C# will chain a standard conversion into a user-defined one —bytereachesInt128throughint,uintthroughlong.ulongandnuinthave no such route, which is exactly why the issue's own repro trips onInt128 | ulong.But the fix suggested first would make things worse. Item 1 opens with "Make
uint/ulongimplicit". Doing that and nothing else breaks three widenings that currently work:C# picks the most encompassed source type among the candidate operators. Adding an operator changes the candidate set, and
ulongleaves no unique answer forbyte,charandushort. That is why the BCL declares all eleven rather than the four it strictly needs, and it is what this PR does. Every source type then has an exact match and the resolution question never arises.What is in here
All eleven implicit widenings —
byte,char,sbyte,short,ushort,int,uint,long,ulong,nint,nuint. Promotinguint/ulongfrom explicit to implicit is source-compatible in that direction, so callers who already wrote the cast keep building; there is a test for that. Narrowing conversions are untouched and stay explicit; there is a test for that too.UInt128does not have the defect — it already declares its unsigned widenings implicitly, so nothing changed there.On the tests
They assert at compile time before they assert anything at run time. Every conversion in
Int128ConversionTestsis written without a cast, so on net472 — which binds the netstandard2.0 build and therefore the polyfill — the file fails to build if a widening goes missing or ambiguous. That is the failure mode worth guarding, because it is invisible on net8.0 and net10.0 where the BCL type wins.Checked in both directions rather than assumed:
mainuint+ulongonly)The run-time assertions use the existing
NumericOracle, so they check the BCL type on net8.0/net10.0 and the polyfill on net472 — the differential patternNumericOraclealready documents. The unsigned cases all useMaxValue, so a conversion that sign-extended would land on a negativeInt128and be caught.Verification
916 tests pass on net8.0 and net10.0; net472 builds clean locally. As with #21 the net472 tests cannot be run here (no Mono on macOS), so their first execution is this PR's Windows CI job.
🤖 Generated with Claude Code