generic mathematics interfaces should use nullable reference types

[ZacharyPatten]

In my opinion, all members in the generic mathematics interfaces should be nullable reference with a ? appended to the end of the type. For example, the current interface:

public interface IEqualityOperators<TSelf, TOther> : IEquatable<TOther> where TSelf : IEqualityOperators<TSelf, TOther>
{
	static abstract bool operator ==(TSelf left, TOther right);
	static abstract bool operator !=(TSelf left, TOther right);
}

Should instead be something like:

public interface IEqualityOperators<TSelf, TOther> : IEquatable<TOther> where TSelf : IEqualityOperators<TSelf, TOther>
{
	static abstract bool operator ==(TSelf? left, TOther? right);
	static abstract bool operator !=(TSelf? left, TOther? right);
}

Why? It causes CS8604 in various scenarios like the one below:

public class Matrix<T>
	where T : INumber<T>
{

...

public static Matrix<T> FactoryIdentity(int rows, int columns)
{
	if (sourceof(rows < 1, out string c1)) throw new ArgumentOutOfRangeException(nameof(rows), rows, c1);
	if (sourceof(columns < 1, out string c2)) throw new ArgumentOutOfRangeException(nameof(columns), columns, c2);
	Matrix<T> matrix = new(rows, columns);
	if (default(T) is not null && T.Zero == default) // CS8604
	{
		int minimum = Math.Min(rows, columns);
		for (int i = 0; i < minimum; i++)
		{
			matrix._matrix[i * columns + i] = T.One;
		}
	}
	else
	{
		for (int row = 0, i = 0; row < rows; row++)
		{
			for (int column = 0; column < columns; column++)
			{
				matrix._matrix[i++] = row == column ? T.One : T.Zero;
			}
		}
	}
	return matrix;
}

^{Note: "Matrix" is a custom type that represents a standard matrix as a flattened array.}

In my opinion this should go for all the generic mathematics interfaces, not just the IEqualityOperators members.

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

Issue Details

---

In my opinion, all members in the generic mathematics interfaces should be nullable reference with a ? appended to the end of the type. For example, the current interface:

public interface IEqualityOperators<TSelf, TOther> : IEquatable<TOther> where TSelf : IEqualityOperators<TSelf, TOther>
{
	static abstract bool operator ==(TSelf left, TOther right);
	static abstract bool operator !=(TSelf left, TOther right);
}

Should instead be:

public interface IEqualityOperators<TSelf, TOther> : IEquatable<TOther> where TSelf : IEqualityOperators<TSelf, TOther>
{
	static abstract bool operator ==(TSelf? left, TOther? right);
	static abstract bool operator !=(TSelf? left, TOther? right);
}

Why? It causes CS8604 in various scenarios like the one below:

public static Matrix<T> FactoryIdentity(int rows, int columns)
{
	if (sourceof(rows < 1, out string c1)) throw new ArgumentOutOfRangeException(nameof(rows), rows, c1);
	if (sourceof(columns < 1, out string c2)) throw new ArgumentOutOfRangeException(nameof(columns), columns, c2);
	Matrix<T> matrix = new(rows, columns);
	if (default(T) is not null && T.Zero == default) // CS8604
	{
		int minimum = Math.Min(rows, columns);
		for (int i = 0; i < minimum; i++)
		{
			matrix._matrix[i * columns + i] = T.One;
		}
	}
	else
	{
		for (int row = 0, i = 0; row < rows; row++)
		{
			for (int column = 0; column < columns; column++)
			{
				matrix._matrix[i++] = row == column ? T.One : T.Zero;
			}
		}
	}
	return matrix;
}

^{Note: "Matrix" is a custom type that represents a standard matrix as a flattened array.}

In my opinion this should go for all the generic mathematics interfaces, not just the IEqualityOperators members.

Author:	ZacharyPatten
Assignees:	-
Labels:	area-System.Numerics
Milestone:	-

[Joe4evr]

public interface IEqualityOperators<TSelf, TOther> : IEquatable<TOther> where TSelf : IEqualityOperators<TSelf, TOther>
{
    static abstract bool operator ==(TSelf? left, TOther? right);
    static abstract bool operator !=(TSelf? left, TOther? right);
}

Wouldn't this require where TSelf : default where TOther : default or something added to the constraints as well? Wouldn't that be a bit overkill on generic constraints? (Consider someone making a helper method that takes in a T constrained to this interface, they'd likely have to write out all those extra constraints to make the compiler happy. In general, that's a good thing, but it wouldn't feel great in this scenario.)

When in doubt, it could use the attribute like how it's used on IEqualityComparer<T>:

public interface IEqualityOperators<TSelf, TOther> : IEquatable<TOther> where TSelf : IEqualityOperators<TSelf, TOther>
{
    static abstract bool operator ==([AllowNull] TSelf left, [AllowNull] TOther right);
    static abstract bool operator !=([AllowNull] TSelf left, [AllowNull] TOther right);
}

(Well, I thought IEqualityComparer<T> used [AllowNull], but now I'm in doubt.)

[tannergooding]

So I thought we already had this configured correctly, but generic NRT annotations are always wonky...

I think the definition needs to be either:

public interface IEqualityOperators<TSelf, TOther>
    where TSelf : IEqualityOperators<TSelf, TOther>?
{
	static abstract bool operator ==(TSelf? left, TOther? right);
	static abstract bool operator !=(TSelf? left, TOther? right);
}

-or-

public interface IEqualityOperators<TSelf, TOther>
    where TSelf : IEqualityOperators<TSelf?, TOther?>?
{
	static abstract bool operator ==(TSelf? left, TOther? right);
	static abstract bool operator !=(TSelf? left, TOther? right);
}

but I'm not positive which is correct and am waiting to hear back from some of the Roslyn team members.

@dotnet/fxdc needs to be looped in as would @jeffhandley

[tannergooding]

The TL;DR: I think the correct is actually:

public interface IEqualityOperators<TSelf, TOther>
    where TSelf : IEqualityOperators<TSelf, TOther>?
{
	static abstract bool operator ==(TSelf left, TOther right);
	static abstract bool operator !=(TSelf left, TOther right);
}

=============================

So here is an example of I<T> with no recursive constraint: https://sharplab.io/#v2:EYLgtghglgdgNAFxFANgHwMQwK4pRYFAUwAIiYDiBYAKFoAEBmE2BIgJwDMIBjUgSQA8AFQB8tAN60SMkvQCMANhIEAzgna8EJYSQCyACl0APAJQBuWgF9aDZvQBMJAMLySIEkNfiaUmrLl7JRc3Q1cSMxIAXlEIyxobOiSmOSdnJw8vBwB+Hz8AlIVldOz9AxKI02jYnDx4xNsaQrTGSWlZQuDdPXkRUSNK9oCZAHcACw5SXUy+6p0AOkMzeIChwLku0r0HPqNSszWA8cmdd08RXLnhRYNltbXO4rcADXlyt0iY/V7vW4t7/wdIJPUrPBzvfZVL7bQS/O6AmQPYEuJwATTe6Uqcx6sIc/XhqwR6yKKNKqPBFU+sRh6Xx/wStCAA

Directly annotating the method as static abstract T? M(T x) is "incorrect" because this means that usage of I<T>.M() can always return null for a reference type.

Directly annotating the method as static abstract T M(T? x) is "incorrect" because this means that implementors of I<T>M() must always accept null for a reference type.

M(T? x) is basically the same as M([AllowNull] T x) afaict
T? M(...) is basically the same as [MaybeNull] T M(...) afaict

I don't see a difference for DisallowNull or NotNull because they aren't impactful in this scenario. They would be impactful if the user had I<object?> and we wanted to say the input or return couldn't be null in these scenarios.

So I think the member annotations are correct, the issue is that the recursive constraint messes things up
and so we need, to the best of my knowledge, where T : I<T>? to get things working as expected

annotating the recursive constraint as where T : I<T?> or where T : I<T?>? means that the usage of that type parameter always allows null for a refence type T which would be incorrect

[tannergooding]

I've pinged the relevant compiler devs internally. I'll likely a get a response sometime early next week.

I should get a draft PR up that covers what I believe are the correct changes shortly.

[jeffhandley]

@ZacharyPatten Thanks so much for filing this issue (in time for us to address it in .NET 7)! 🚀

[stephentoub]

The TL;DR: I think the correct is actually

I agree... mostly.

The ? at the end of the IEqualityOperators<TSelf, TOther>? is important because it says that it's valid to specify TSelf as a nullable reference type; without that, the constraint is saying it can't be nullable.

One controversial part is:

Directly annotating the method as static abstract T M(T? x) is "incorrect" because this means that implementors of IM() must always accept null for a reference type.

We do explicitly mark the arguments to IEqualityComparer<T>. Equals and IEquatable<T>. Equals as nullable (T? ), but we do so largely for historical/compat reasons, namely that the annotations came about long after these interfaces were defined and used, and tons of use expected to be able to pass in null and implementations (most) were tolerant of null.

So we need to decide whether we want these new interfaces to follow the same approach as the existing equality interfaces or change direction.

We also have this related guideline:
https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/api-guidelines/nullability.md#interface-implementations