dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.95k stars 4.02k forks source link

Incorrect "Nullability doesn't match the target delegate" when tuple element names don't explicitly match #74493

Open jnm2 opened 2 months ago

jnm2 commented 2 months ago

Version Used: 17.11.0 Preview 3.0

The following code erroneously causes this warning to appear for each lambda in the Error List and at the command line, though squiggles are not shown:

⚠️ CS8621 Nullability of reference types in return type of 'lambda expression' doesn't match the target delegate 'Func<(string, object?), ?>' (possibly because of nullability attributes).

class C
{
    void M((string Name, object? Value)[] parameters)
    {
        _ = parameters.Append(("A", "A")).ToDictionary(
            tuple => tuple.Name,
            tuple => tuple.Value);
    }
}

If you name the elements in the tuple expression to exactly match the tuple element names in the collection, the warning goes away.

class C
{
    void M((string Name, object? Value)[] parameters)
    {
        _ = parameters.Append((Name: "A", Value: "A")).ToDictionary(
            tuple => tuple.Name,
            tuple => tuple.Value);
    }
}

For each element where a name is missing or does not exactly match, a nullability warning returns.

jnm2 commented 1 month ago

The problem goes further than nullability. An error type is getting created by the method type inferrer for the ToDictionary call. This can even be seen in the IDE:

image

Versus for exact name matches on the tuple elements:

image

There are no diagnostics or emit issues in the final compilation, though. This is the cause of the detection of a "nullability mismatch."

jnm2 commented 1 month ago

I looked into this further, and found that the nullability phase might be getting something right. I'm not sure this code should be compiling. If you insert an object cast, the code stops compiling because the tuple element names conflict and are discarded, and tuple.Name and tuple.Value can't be found:

#nullable disable
class C
{
    void M((string Name, object Value)[] parameters)
    {
        _ = parameters.Append(("A", (object)"A")).ToDictionary(
            tuple => tuple.Name, // '(string, object)' does not contain a definition for 'Name' [...]
            tuple => tuple.Value); // '(string, object)' does not contain a definition for 'Value' [...]
    }
}

Alternatively to adding the object cast, you could change the parameters to (string Name, string Value)[].

So, potentially, I don't think the original example should be compiling at all. The sad part is I know I've taken a dependency on this "only consider names of the receiver's tuple" behavior, over and over, with these LINQ methods.

What's happening is that when the tuple types aren't equal ((string, object) versus (string, string)), the Append method type inference takes (string, object) as an exact bounds for T via the T[] expression being passed as the receiver parameter, because the spec says that the array element type is an exact bound if the element type is a value type (impl). Whereas the (string, string) expression being passed as Append's second parameter becomes a lower bound on Append's T. Then, when there is an exact bound, only the exact bounds become candidates for T (impl). Then the other bounds are merged with the candidates or removed (impl). When it comes time to merge and remove, it all comes down to which way this line evaluates: https://github.com/dotnet/roslyn/blob/main/src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/MethodTypeInference.cs#L3299 (string, object) is not equal to (string, string), so it never calls MergeAndReplaceIfStillCandidate with the lower bound. The lower bound ends up being totally ignored. This means that the conflicting tuple element names are never taken into consideration, and it infers all tuple element names from Append's receiver parameter and ignores the tuple element names from Append's second argument.

When the nullability pass gets here, it does call MergeAndReplaceIfStillCandidate, which removes tuple element names which conflict between the two arguments to Append (impl, impl).

jnm2 commented 1 month ago

@jcouv Is this connected to https://github.com/dotnet/roslyn/issues/27961?

https://github.com/dotnet/roslyn/blob/0cd65d1399a13ef6e0e4ea0ba9575996a957ceea/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs#L7748-L7754

jnm2 commented 1 month ago

"Inference should be based on unconverted arguments" seems like it sums up the exact discrepancy that this issue was about. The arguments being passed back in to method type inference have already been target-typed using the result of the prior inference for the same type parameter.

image

This explains why the inference invoked by the nullability pass is evaluating to true at https://github.com/dotnet/roslyn/blob/main/src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/MethodTypeInference.cs#L3299 and is therefore removing conflicting tuple element names.

jnm2 commented 1 month ago

Perhaps a fix could be to do something like what was done for lambdas and collection expressions in getArgumentForMethodTypeInference?

https://github.com/dotnet/roslyn/blob/0cd65d1399a13ef6e0e4ea0ba9575996a957ceea/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs#L7776-L7797

jcouv commented 1 month ago

Perhaps a fix could be to do something like what was done for lambdas and collection expressions in getArgumentForMethodTypeInference?

Yes, that sounds right. The infrastructure for target-typed expressions has evolved over time and I think that collection expressions are the latest example, so may be a good example to follow.