[release/8.0-staging] Fix Int128 checked-convert to signed IntX#105998
Merged
tannergooding merged 2 commits intorelease/8.0-stagingfrom Aug 8, 2024
Merged
[release/8.0-staging] Fix Int128 checked-convert to signed IntX#105998tannergooding merged 2 commits intorelease/8.0-stagingfrom
Int128 checked-convert to signed IntX#105998tannergooding merged 2 commits intorelease/8.0-stagingfrom
Conversation
Contributor
|
Tagging subscribers to this area: @dotnet/area-system-numerics |
Member
|
CC. @jeffhandley, @artl93. This has been fixed in .NET 9 for a bit now, but had a new issue opened up for it on VS Feedback and so should be considered for backport into .NET 8 |
3 tasks
jeffhandley
approved these changes
Aug 7, 2024
Member
jeffhandley
left a comment
There was a problem hiding this comment.
Looks good to me. I support backporting this to .NET 8 based on the customer reports and the fact that customers would expect exceptions here but we're returning the incorrect result instead. Please go ahead and request approval from Tactics, @tannergooding.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Backport of #100342 to release/8.0-staging
/cc @tannergooding @skyoxZ
Customer Impact
Int128to SignedInteger #99489Checked operators are exposed so that developers can produce an exception on overflow. However, for some
Int128inputs the exception would not be thrown when the value was downcast toInt64(long). This could result unexpected behavior as the value would be truncated instead. An example of this is which is expected to throw, but was not:Regression
The
Int128type was introduced with this bug in .NET 7Testing
Due to being two's complement its expected that all bits in
uppermatch the most significant bit fromlower. There was an edge case missed here forlongthat was being covered in other size conversions.Explicit tests covering the impacted scenarios were added and the various checked conversions were made consistent with eachother.
Risk
Low, potentially Medium.
Int128is a relatively new type to the ecosystem and checked conversions are not broadly used in the first place (in comparison to regular conversions).However, while this fixes the underlying issue, it does cause an exception to be thrown when the scenario is encountered. This is the intended and expected behavior, but of course could surprise users after the patch goes out if they were not aware of the bug.
This case of code being changed to throw an exception, even though it is expected for such code to be already throwing, is what may be surprising and adds the risk that could potentially bump it into the medium risk category. I personally lean more towards "Low" given the newness of the type, that it only impacts a subset of values that match a particular bit pattern (other inputs were already throwing as expected), and that it requires users to be doing
checkedarithmetic which is known to be less common.