Breaking change in nullability analysis of inferred type argument
- Dominant language
- C#
- Stars
- 20.7k
- Forks
- 4.3k
- PR merge metrics
- PR metrics pending
Description
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](https://sharplab.io/#v2:CYLg1APgogTjD2MAKCBuBLYBTGACLADrgLy4B2WA7gBQCUA3ALABQhAdACowCGZAzuixkALgDFu6ADYBXGFmoAiAPLwCfBQxZbmAAQDMudCJwAzbgGMsuAJKwEyNJhwAeDgGUskkwBpcHO4gAfCy4obiUABY4Vu6eJrgguHzCMNLmwr62cIgo8BjYMK4eXr7+2TDBzADeIWEA9A24znzSALat3DAAnpVhuA11uADiWMJ8uMJR+OW42MISkuPo8dyoC9wARpJYbLWhA011Le2dPXt+AXiXACKjC+NVuADmo/SGZOjCbwC+LL/MLH0SRSaWEuEuuXyOASNghjgKzjheScMF8OgAjAAGAD8lRqzD6QIxOPB5Vu8ykD2er3enx+f20gIMGIAbLgdAAmcEAD2ELHxhOZ6LZ7nmwhiPH4ghE4iksiwRTFWFKNzulMC1HOfUm6HGou44tw5m4BE2Uk+XW8WrCZXs5Pu0ztasWtHOkWifjcSphyVS6UySKhhX14pVZOdfF6YQFfTCOgA7EaTWbJBbwp8IrhHqqKYsSI7EPbKbhvkwCWF/t8gA===) 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)'. Nullability of type argument 'ErrorProvider' doesn't match constraint type 'IErrorProvider'.
We have this interface and implementation:
```cs
public interface IErrorProvider
where TSelf : struct, IErrorProvider
{
TError ErrorDetails { get; init; }
}
public struct ErrorProvider : IErrorProvider
{
public string? ErrorDetails { get; init; }
}
```
and this extension method:
```cs
public static class Ext
{
public static TState TransientFailure(
this TState capability,
[DisallowNull] TErrorDetails errorDetails)
where TState : struct, IErrorProvider
{
return capability with { ErrorDetails = errorDetails };
}
}
```
And when we invoke it thus:
```cs
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:
```cs
ep.TransientFailure("Oops");
```
If we state them explicitly:
```cs
ep.TransientFailure("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)'. Nullability of type argument 'ErrorProvider' doesn't match constraint type 'IErrorProvider'.
**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:
```cs
public interface IErrorProvider
where TSelf : struct, IErrorProvider
where TError : notnull
{
///
/// Gets the error details if available.
///
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:
```cs
ep.TransientFailure("Oops");
```
to this:
```cs
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.
Contributor guide
Research direction
The issue names no repository files or tests; its entry points are the Ext.TransientFailure call, CS8631, and Visual Studio's type-argument simplification quick fix. Start by reproducing the supplied C# code in the linked SharpLab example and Visual Studio, then determine whether inference or the quick fix is wrong. Done means the intended warning behavior and any regression are established with coverage.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- compilers, devtools
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 25/100