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

[jnm2]

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]

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:

Versus for exact name matches on the tuple elements:

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

[jnm2]

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]

@jcouv Is this connected to #27961?

roslyn/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

Lines 7748 to 7754 in 0cd65d1

	private ImmutableArray<BoundExpression> GetArgumentsForMethodTypeInference(ImmutableArray<VisitResult> argumentResults, ImmutableArray<BoundExpression> arguments)
	{
	// https://github.com/dotnet/roslyn/issues/27961 MethodTypeInferrer.Infer relies
	// on the BoundExpressions for tuple element types and method groups.
	// By using a generic BoundValuePlaceholder, we're losing inference in those cases.
	// https://github.com/dotnet/roslyn/issues/27961 Inference should be based on
	// unconverted arguments. Consider cases such as `default`, lambdas, tuples.

[jnm2]

"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.

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]

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

roslyn/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

Lines 7776 to 7797 in 0cd65d1

	if (argument.Kind == BoundKind.CollectionExpression)
	{
	// MethodTypeInferrer must infer types using the elements of an unconverted collection expression
	var collectionExpressionVisitResults = visitResult.NestedVisitResults;
	var collection = (BoundCollectionExpression)argument;
	Debug.Assert(collectionExpressionVisitResults is not null);
	Debug.Assert(collectionExpressionVisitResults.Length == collection.Elements.Length);
	var elementsBuilder = ArrayBuilder<BoundNode>.GetInstance(collectionExpressionVisitResults.Length);
	for (int i = 0; i < collectionExpressionVisitResults.Length; i++)
	{
	if (collection.Elements[i] is BoundExpression elementExpression)
	{
	var (elementNoConversion, _) = RemoveConversion(elementExpression, includeExplicitConversions: false);
	elementsBuilder.Add(getArgumentForMethodTypeInference(elementNoConversion, collectionExpressionVisitResults[i]));
	}
	else
	{
	elementsBuilder.Add(collection.Elements[i]);
	}
	}

[jcouv]

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.