[release/9.0] Fix BigInteger.Rotate{Left,Right}
for backport
#112991
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 #112878 and #112879 to release/9.0
/cc @tannergooding @kzrnm
Customer Impact
This was customer reported via #112564. The customer contributed the fix and validated it for their scenarios.
Shifting and bit-rotation have defined behaviors for fixed sized integers and while the behaviors are harder to map to the concept of a
BigInteger
which can have an arbitrary number of bits, there is still an expected behavior that maps to those semantics. In particular it's expected to behave similarly to as if theBigInteger
was represented as the shortest two's complement representation rounded up to the nearest multiple of 32-bits.Customers were getting incorrect results for certain inputs due to the internal representation of
BigInteger
causing the sign bit to be lost after the value was converted to its two's complement representation.Regression
The addition of
unsigned right shift
andbitwise rotatation
APIs is recent to .NET and was done as part of the push to supportGeneric Math
.Testing
Some comprehensive tests were added that test the various edge case bit patterns that could be prone to triggering such issues.
Risk
Medium
The APIs impacted in BigInteger are relatively new (.NET 7) and so the risk of negatively impacting existing customers is relatively low. The largest risk effectively being that some customer is accidentally depending on the broken behavior without realizing it.
However, these breaks are also additional examples of some of the problems caused by the current internal representation of
BigInteger
(which has been that way since it was introduced in 2010). The current representation is effectively that it is "ones complement + sign" and while that can make some basic arithmetic operations (such as addition, subtraction, multiplication, and division) simpler, it makes many other operations more complex or slower than necessary (particularly where they involve considering the bits of the two's complement representation). This in turn means that the confidence bar in such changes tends to be lower and needs more comprehensive testing (as was added in this fix).The required confidence bar that the fix is correct has been met here, but the representation and additional testing nuance for edge cases make all such changes riskier, hence the categorization as
medium
.