dotnet / dotnet/roslyn

Breaking change in nullability analysis of inferred type argument

Open
#74,987 9 comments 0 reactions 0 assignees View on GitHub
Area-Compilers untriaged
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.