Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The mask of shiftAmount in IShiftOperators<byte, int, byte> should be 7 instead of 31 #103506

Closed
skyoxZ opened this issue Jun 15, 2024 · 3 comments · Fixed by #103900
Closed

The mask of shiftAmount in IShiftOperators<byte, int, byte> should be 7 instead of 31 #103506

skyoxZ opened this issue Jun 15, 2024 · 3 comments · Fixed by #103900
Labels
area-System.Numerics breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. bug in-pr There is an active PR which will close this issue when it is merged needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
Milestone

Comments

@skyoxZ
Copy link
Contributor

skyoxZ commented Jun 15, 2024

Description

/// <inheritdoc cref="IShiftOperators{TSelf, TOther, TResult}.op_LeftShift(TSelf, TOther)" />
static byte IShiftOperators<byte, int, byte>.operator <<(byte value, int shiftAmount) => (byte)(value << shiftAmount);
/// <inheritdoc cref="IShiftOperators{TSelf, TOther, TResult}.op_RightShift(TSelf, TOther)" />
static byte IShiftOperators<byte, int, byte>.operator >>(byte value, int shiftAmount) => (byte)(value >> shiftAmount);
/// <inheritdoc cref="IShiftOperators{TSelf, TOther, TResult}.op_UnsignedRightShift(TSelf, TOther)" />
static byte IShiftOperators<byte, int, byte>.operator >>>(byte value, int shiftAmount) => (byte)(value >>> shiftAmount);

Reproduction Steps

using System;
using System.Numerics;

BinaryIntegerWrapper<byte> a = new(2);
Console.WriteLine((a << 11).Value);

public struct BinaryIntegerWrapper<T> : IShiftOperators<BinaryIntegerWrapper<T>, int, BinaryIntegerWrapper<T>>
            where T : IBinaryInteger<T>
{
    public T Value;

    public BinaryIntegerWrapper(T value) => Value = value;

    public static implicit operator BinaryIntegerWrapper<T>(T value) => new BinaryIntegerWrapper<T>(value);

    public static BinaryIntegerWrapper<T> operator <<(BinaryIntegerWrapper<T> value, int shiftAmount) => value.Value << shiftAmount;
    public static BinaryIntegerWrapper<T> operator >>(BinaryIntegerWrapper<T> value, int shiftAmount) => value.Value >> shiftAmount;
    public static BinaryIntegerWrapper<T> operator >>>(BinaryIntegerWrapper<T> value, int shiftAmount) => value.Value >>> shiftAmount;
}

Expected behavior

16

Actual behavior

0

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

Per this comment from @tannergooding:

This is also notably an issue with sbyte, short, and ushort. It is also, unfortunately, a breaking change and will need a bar check.

The intent had indeed been to correctly mask, which is being done in other operations but was missed for these ones.

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jun 15, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

@jeffhandley jeffhandley added this to the 10.0.0 milestone Jul 20, 2024
@jeffhandley jeffhandley added bug breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. and removed untriaged New issue has not been triaged by the area owner labels Jul 20, 2024
@dotnet-policy-service dotnet-policy-service bot added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jul 20, 2024
Copy link
Contributor

Added needs-breaking-change-doc-created label because this issue has the breaking-change label.

  1. Create and link to this issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.

Tagging @dotnet/compat for awareness of the breaking change.

@jeffhandley
Copy link
Member

I've added this to the 10.0.0 milestone as it would be beneficial to apply the breaking change fix early in an LTS release.

@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Numerics breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. bug in-pr There is an active PR which will close this issue when it is merged needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants