Breaking change in nullability analysis of inferred type argument

[idg10]

It's possible that this change is by design but we (me and my colleague @mwadams ) couldn't find a description of an intention to change this. In any case, the effect is that we get a compiler warning in code that was previously acceptable.

Version Used:

Visual Studio 17.11.2 .NET SDK 8.0.400

Steps to Reproduce:

The code at this example on sharplab produces this warning:

warning CS8631: The type 'ErrorProvider' cannot be used as type parameter 'TState' in the generic type or method 'Ext.TransientFailure<TState, TErrorDetails>(TState, TErrorDetails)'. Nullability of type argument 'ErrorProvider' doesn't match constraint type 'IErrorProvider<ErrorProvider, string>'.

We have this interface and implementation:

public interface IErrorProvider<TSelf, TError>
    where TSelf : struct, IErrorProvider<TSelf, TError>
{
    TError ErrorDetails { get; init; }
}

public struct ErrorProvider : IErrorProvider<ErrorProvider, string?>
{
    public string? ErrorDetails { get; init; }
}

and this extension method:

public static class Ext
{
    public static TState TransientFailure<TState, TErrorDetails>(
        this TState capability,
        [DisallowNull] TErrorDetails errorDetails)
    where TState : struct, IErrorProvider<TState, TErrorDetails>
    {
        return capability with { ErrorDetails = errorDetails };
    }
}

And when we invoke it thus:

ErrorProvider ep = new();
ep.TransientFailure("Oops");

we get the warning shown above.

Back in July of this year, we did not get this warning. So this appears to be a fairly recent change.

What seems to be happening is that it is resolving the type arguments as:

ep.TransientFailure<ErrorProvider, string>("Oops");

If we state them explicitly:

ep.TransientFailure<ErrorProvider, string?>("Oops");

then we no longer get a warning, although interestingly, Visual Studio greys out the type arguments, and offers a quick fix to "simplify" them by removing them.

We're not sure whether it was previously inferring the type arguments differently (e.g., had it inferred that since TState was ErrorProvider, TError must therefore be string?) or whether there's no change to how the compiler interprets the code, but the null warnings have tightened up.

Diagnostic Id:

CS8631: The type 'ErrorProvider' cannot be used as type parameter 'TState' in the generic type or method 'Ext.TransientFailure<TState, TErrorDetails>(TState, TErrorDetails)'. Nullability of type argument 'ErrorProvider' doesn't match constraint type 'IErrorProvider<ErrorProvider, string>'.

Expected Behavior:

We previously didn't get this warning. Our expectation was that code that has been compiling without problems for months would continue to compile without warnings.

Of course, it's possible that it was a bug that this code used to compile without warnings. We're reporting this in case this wasn't meant to change.

Notes

It's relatively easy for us to work around this. We can change the interface definition to:

public interface IErrorProvider<TSelf, TError>
    where TSelf : struct, IErrorProvider<TSelf, TError>
    where TError : notnull
{
    /// <summary>
    /// Gets the error details if available.
    /// </summary>
    TError? ErrorDetails { get; init; }
}

There was a time when this wasn't possible. TError is unconstrained, and the compiler used to reject it because the behaviour you might expect when TError is a reference type is different from what you might reasonably expect for a value type. However, this changed (fairly recently I think), meaning we can now use that last code snippet, which in some ways better expresses what we mean. (We also have to add a notnull constraint to the extension method of course, and we can then remove the [DisallowNull] from that.)

So this isn't a showstopper for us.

If this change is not considered to be an bug, the fact that Visual Studio suggests changing this:

ep.TransientFailure<ErrorProvider, string?>("Oops");

to this:

ep.TransientFailure("Oops");

is a bug because this quick fix will convert code that compiles without warnings into code that generates the warning described above.

[jjonescz]

You are saying this is a breaking change. Can you specify which version of .NET SDK or VS this worked in?

[mwadams]

@jjonescz - As this was working in June 2024, I'd suggest it was working with VS 2022 17.10 although @idg10 can probably confirm this by looking at his update logs.

[jaredpar]

@RikkiGibson, @cston are u aware of any type inference changes we made that would've impacted this scenario?

At a glance this new behavior makes sense. The string constant is causing us to infer string not string? and that doesn't line up with the constraint. I can't think of any changes we made recently though that would impact this scenario.

[cston]

It looks like the same error is reported when compiling with Visual Studio 16.11.

Microsoft (R) Visual C# Compiler version 3.11.0-4.22108.8 (d9bef045) Copyright (C) Microsoft Corporation. All rights reserved.

c.cs(4,1): warning CS8631: The type 'ErrorProvider' cannot be used as type parameter 'TState' in the generic type or method 'Ext.TransientFailure<TState, TErrorDetails>(TState, TErrorDetails)'. Nullability of type argument 'ErrorProvider' doesn't match constraint type 'IErrorProvider<ErrorProvider, string>'.

[RikkiGibson]

Feature request https://github.com/dotnet/csharplang/discussions/741 is about constraints contributing to type argument inference.

It seems expected that type argument string is inferred for TErrorDetails in the current language design. I am unsure why the warning may have been missing in the past. I don't recall a recent change that would have impacted this.

[mwadams]

The odd thing is that it also doesn't align with the code fix provider which is offering to simplify the explicit generic method invocation, despite the fact that it would then produce the warning.

[RikkiGibson]

I have seen such bugs before with nullable reference types (scenarios where compiler needs the type arguments and editor feature thinks they are unnecessary). Maybe we can ask @CyrusNajmabadi when he is back from vacation.

[CyrusNajmabadi]

Because the compiler doesn't expose an API to know if the types are unnecessary, the ide had to reimplement all the rules. This is often error prone and easy to get wrong. Given how little time we've had to add nrt support, it is not surprising to be that a very old feature like this would get it wrong.