Fix nullable annotations on generic math interfaces

[stephentoub]

• All where TSelf : ... constraints become where TSelf : ...?. Without this, trying to define a type like Matrix<T> where T : INumber<T>? in order to support nullable T types warns because INumber<T> constrains its T (TSelf) to be non-nullable.
• All where TOther : ... constraints are changed to be oblivious. They can't be correctly annotated as there's no way to express the nullability relationship with the nullability of TSelf.
• Use [MaybeNullWhen(false)] out T instead of [NotNullWhen(true)] out T?, as we do with other generics, since if the instantiation of T is nullable, we can't guarantee NotNullWhen(true).
• Make IEqualityOperators == and != accept TSelf?. This keeps it consistent with IEquatable<T>.Equals(T?), IEqualityComparer<in T>.Equals(T?, T?), IEqualityComparer.Equals(object?, object?), IStructuralEquatable.Equals(object?, IEqualityComparer), and object.Equals(object?) which all allow null even if generic and the generic is non-null. It in turn enables checks like T.Zero == default without nullability warnings.
• A few methods get null checks on the argument in places where our default implementations dereferenced the argument.

cc: @jeffhandley, @tannergooding, @dakersnar, @jaredpar, @RikkiGibson, @MadsTorgersen
Fixes #73855

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

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

Issue Details

---

• All where TSelf : ... constraints become where TSelf : ...?. Without this, trying to define a type like Matrix<T> where T : INumber<T>? in order to support nullable T types warns because INumber<T> constrains its T (TSelf) to be non-nullable.
• All where TOther : ... constraints are changed to be oblivious. They can't be correctly annotated as there's no way to express the nullability relationship with the nullability of TSelf.
• Use [MaybeNullWhen(false)] out T instead of [NotNullWhen(true)] out T?, as we do with other generics, since if the instantiation of T is nullable, we can't guarantee NotNullWhen(true).
• Make IEqualityOperators == and != accept TSelf?. This keeps it consistent with IEquatable<T>.Equals(T?), IEqualityComparer<in T>.Equals(T?, T?), IEqualityComparer.Equals(object?, object?), IStructuralEquatable.Equals(object?, IEqualityComparer), and object.Equals(object?) which all allow null even if generic and the generic is non-null. It in turn enables checks like T.Zero == default without nullability warnings.
• A few methods get null checks on the argument in places where our default implementations dereferenced the argument.

cc: @jeffhandley, @tannergooding, @drewnoakes, @jaredpar, @RikkiGibson, @MadsTorgersen
Fixes #73855

Author:	stephentoub
Assignees:	-
Labels:	area-System.Numerics, new-api-needs-documentation
Milestone:	7.0.0

[RikkiGibson]

I agree with all the changes except the "oblivious TOther" change—but that's only because I haven't paged in that particular problem yet. I did want to raise some additional potential usability issues that maybe we can reason through together:

If someone decides to implement IEqualityOperators<MyReferenceType?, MyReferenceType?, bool>​, then they might get a warning even if the signatures of everything are actually compatible. This is perhaps solvable by adding the in modifier to the IEqualityOperators type parameters used in inputs. SharpLab

Conversely if someone was trying to ensure their caller provides an operator which can accept null, they might use a constraint like where T : IEqualityOperators<T?, T?, bool>?​, which will warn if the user's operator definition isn't declared with nullable type arguments. This isn't solvable by adjusting type parameter variance. SharpLab

It feels like the most "robust" pattern is for type authors to implement IEqualityOperators with nullable reference types for concrete operands--this is in keeping with existing guidance for IComparable/IEquatable, and for the input type parameters to be marked in.

In general it seems like it would make sense to sprinkle in and out on type parameters only used as inputs and outputs in these interfaces. However, I'm not aware of whether this introduces some unreasonable overhead for the sake of niche usages.

[stephentoub]

This is perhaps solvable by adding the in modifier to the IEqualityOperators type parameters used in inputs

Interesting. Earlier today I actually asked @jaredpar whether he thought there were any uses for us properly annotating the interfaces for co/contravariance, as I couldn't think of any. I guess there is one. I'll push an additional commit with what that would look like and we can decide whether it makes sense.

[stephentoub]

I pushed a commit with what I think the co/contravariance would look like. Two oddities are IAdditiveIdentity and IMultiplicativeIdentity, in which TSelf isn’t used in any of the members, so I left it as invariant even though it could be either covariant or contravariant.

[stephentoub]

cc: @terrajobst, @bartonjs

[terrajobst]

• Use [MaybeNullWhen(false)] out T instead of [NotNullWhen(true)] out T?, as we do with other generics, since if the instantiation of T is nullable, we can't guarantee NotNullWhen(true).

What's the null state for the out parameter in [MaybeNullWhen(false)] out T in case the method returns true? Oblivious?

[stephentoub]

What's the null state for the out parameter in [MaybeNullWhen(false)] out T in case the method returns true? Oblivious?

Depends on the T. If T is non-nullable, then the null state is not-null. If T is nullable, then the null state is maybe-null.

We use [MaybeNullWhen(false)] out T everywhere we have the try pattern with a generic, e.g. Dictionary<TKey, TValue>.TryGetValue.

[dakersnar]

First commit LGTM, besides a couple clarifying questions.

[tannergooding]

I pushed a commit with what I think the co/contravariance would look like. Two oddities are IAdditiveIdentity and IMultiplicativeIdentity, in which TSelf isn’t used in any of the members, so I left it as invariant even though it could be either covariant or contravariant.

For the TSelf parameter, was this covered with Jared/Mads and potential impact of future C# changes around a self constraint?

The intent here is that TSelf is a fill for the lack of a self constraint and so it must be exactly the type implementing the struct. The TOther parameter, where it exists, can normally be any type without restriction. So we don't want to change the variance if it will restrict or hurt the ability to adopt a future self constraint the language actually gets.

I know there are also various edge cases with class Y : X, INumber<X> { } and class B : INumber<B> { } class D : B { } due to the requirement that operators have at least one parameter and/or return be of the declaring type and that you can't have a static abstract/static virtual in a class (meaning that B must implement and "seal" the exposed interface member, D cannot then override it further).

It might overall be safer if we say that TSelf has no variance and allow TOther to have the correct variance.

[stephentoub]

I'm fine removing the variance commit; I added it in response to Rikki's comments, and I didn't have any scenarios for it. His scenario doesn't actually involve anything other than TSelf from a runtime perspective, but the possibility of TSelf vs TSelf?, and using variance to allow for that distinction. But that scenario also requires it on TSelf, so if we're talking about reverting the variance changes, I'd be inclined to just revert them all-up, and revisit them in .NET 8 (though Jared raised the possibility that adding variance later could be a breaking change in its impact on overload resolution).

[tannergooding]

I don't have a strong preference here. I'm also unsure on how impactful such a break will be in practice considering the expected usage for most of these types between .NET 7 and .NET 8.

---

For IEqualityOperators<TSelf, TOther, TResult> and IComparisonOperators<TSelf, TOther, TResult>:

• TOther roughly corresponds to the T in IComparable/IEquatable.
• TSelf then roughly corresponds to the this parameter that implementations of CompareTo and Equals have access to
  • The same is true of TSelf for all these generic math interfaces
• TResult is going to be exclusively bool in the vast majority of cases.
  • It might be something different for 3rd party vector like types -- e.g. some user might expose Vector4 operator ==(Vector4 lhs, Vector4 rhs) which returns per element equality rather than total equality.

I know that we have IComparable<in T>, but IEquatable<T>. The latter explicitly calls it out as being that way due to questionable semantics around equality and inheritance. -- The former having in is then a bit odd since returning 0 implies "equality", but I guess that's how it is today.

For the other operators they need at least one parameter to be the implementing type which should always be TSelf. There isn't any requirement on what TOther (or TResult) is.

[stephentoub]

IComparable<in T>, but IEquatable<T>

Yup. But, we also have public interface IEqualityComparer<in T>. So... yay for consistency? :)

[stephentoub]

Removed the extraneous ! and removed the variance commit.

@RikkiGibson, are you ok with this?

Any other feedback?

[RikkiGibson]

Top line: yeah, I think it is fine to keep these type parameters invariant for .NET 7.

IEquatable is invariant somewhat because of compat and it is justified somewhat in terms of design (maybe a comparer written for Base is not going to do the right thing on Derived if Derived introduces additional state).

To "make up" for this the compiler special cases IEquatable to make it "nullable-variant". SharpLab. I actually wonder to what extent being able to do this explicitly on interfaces like IEqualityOperators would solve the usability problem with reference types here. Relates to dotnet/csharplang#3950.

we don't want to change the variance if it will restrict or hurt the ability to adopt a future self constraint the language actually gets.

I don't think the concept of a self constraint is incompatible with contravariance/in. For example, even if we introduced a language-level constraint like interface MyInterface<T> where T : self, and class MyType : Interface<MyType> required exactly MyType as a type argument, it still might be reasonable to convert MyInterface<MyType> to MyInterface<DerivedType>, depending on the meaning of the interface and signatures of its members.

Now, thinking about whether type-arguments to self-type parameters can be nullable leaves me shrugging my shoulders.. :)

This actually does seem suspect. Why would it make sense to convert from MyInterface<BaseType> to MyInterface<DerivedType> if it wouldn't even be allowed in the first place to implement MyInterface<DerivedType> on the thing we are converting?

adding variance later could be a breaking change in its impact on overload resolution

Yeah, it means that some conversions that used to be invalid would become valid, and that would affect the viability/betterness of overloads, etc. My gut feeling is that is likely to be a low-impact break.

Would it be helpful to "protect" this space by constraining some of the type parameters to struct and removing the constraint in a future .NET version?

[stephentoub]

Would it be helpful to "protect" this space by constraining some of the type parameters to struct and removing the constraint in a future .NET version?

Blocking classes from participating in generic math is a gigantic hammer, one I'm certainly not going to wield while @tannergooding is on vacation. I'd like to get these NRT changes in for RC1, and I'll open a separate issue for the remaining things to be investigated when Tanner is back online.

[terrajobst]

Agreed. Let's not exclude classes.

[stephentoub]

/backport to release/7.0-rc1

[github-actions]

Started backporting to release/7.0-rc1: https://github.com/dotnet/runtime/actions/runs/2879292036