Incorrect nullability from method type inference

[cston]

The compiler reports a warning for F(x, y).ToString() but not for F(y, x).ToString().

// csc.exe /langversion:7 /t:library a.cs
public class A { }
public class B { public A F; }

// csc.exe /r:a.dll /t:library b.cs
class C
{
    static T F<T>(T x, T y) => x;
    static void G(A? x, B b)
    {
        var y = b.F;
        F(x, y).ToString();
        F(y, x).ToString();
    }
}

(jcouv update:)
Note that I referenced this issue to replace some PROTOTYPE markers. See details below.

[cston]

See unit test TypeInference_04.

[cston]

Similar issue with conditional operator and array element best type calculations. For instance, the following results in a warning for (b ? x : y) but not (b ? y : x):

csc.exe /t:library a.cs

public class A
{
    public static object F;
}
public class B<T>
{
    public T F;
}

csc.exe /t:library /r:a.dll program.cs

[module: System.Runtime.CompilerServices.NonNullTypes]
class Program
{
    static B<T> Create<T>(T t)
    {
        throw null;
    }
    static void F(bool b, B<object?> x)
    {
        var y = Create(A.F); // B<object~>
        (b ? x : y).F.ToString(); // warning
        (b ? y : x).F.ToString(); // no warning
    }
}

[cston]

See related issue: #29815

[jcouv]

I'm replacing some PROTOTYPE markers with a reference to this issue, as I think that code will be revised as part of the fix.

        private MethodSymbol InferMethodTypeArguments(BoundCall node, MethodSymbol method, ImmutableArray<BoundExpression> arguments)
        {
            Debug.Assert(method.IsGenericMethod);
            // https://github.com/dotnet/roslyn/issues/27961 OverloadResolution.IsMemberApplicableInNormalForm and
            // IsMemberApplicableInExpandedForm use the least overridden method. We need to do the same here.
            var definition = method.ConstructedFrom;
            var refKinds = ArrayBuilder<RefKind>.GetInstance();
            if (node.ArgumentRefKindsOpt != null)
            {
                refKinds.AddRange(node.ArgumentRefKindsOpt);
            }
            // https://github.com/dotnet/roslyn/issues/27961 Do we really need OverloadResolution.GetEffectiveParameterTypes?
            // Aren't we doing roughly the same calculations in GetCorrespondingParameter?
            OverloadResolution.GetEffectiveParameterTypes(
                definition,
                arguments.Length,
                node.ArgsToParamsOpt,
                refKinds,
                isMethodGroupConversion: false,
                // https://github.com/dotnet/roslyn/issues/27961 `allowRefOmittedArguments` should be
                // false for constructors and several other cases (see Binder use). Should we
                // capture the original value in the BoundCall?
                allowRefOmittedArguments: true,
                binder: _binder,
                expanded: node.Expanded,
                parameterTypes: out ImmutableArray<TypeSymbolWithAnnotations> parameterTypes,
                parameterRefKinds: out ImmutableArray<RefKind> parameterRefKinds);
            refKinds.Free();
            HashSet<DiagnosticInfo> useSiteDiagnostics = null;
            var result = MethodTypeInferrer.Infer(
                _binder,
                _conversions,
                definition.TypeParameters,
                definition.ContainingType,
                parameterTypes,
                parameterRefKinds,
                arguments,
                ref useSiteDiagnostics,
                getIsNullableOpt: expr => GetIsNullable(expr));
            if (result.Success)
            {
                return definition.Construct(result.InferredTypeArguments);
            }
            return method;
        }

        private ArrayTypeSymbol VisitArrayInitializer(BoundArrayCreation node)
        {
            var arrayType = (ArrayTypeSymbol)node.Type;
            var elementType = arrayType.ElementType;

            BoundArrayInitialization initialization = node.InitializerOpt;
            var elementBuilder = ArrayBuilder<BoundExpression>.GetInstance(initialization.Initializers.Length);
            GetArrayElements(initialization, elementBuilder);

            // https://github.com/dotnet/roslyn/issues/27961 Removing and recalculating conversions should not
            // be necessary for explicitly typed arrays. In those cases, VisitConversion should warn
            // on nullability mismatch (although we'll need to ensure we handle the case where
            // initial binding calculated an Identity conversion, even though nullability was distinct).
            int n = elementBuilder.Count;
            var conversionBuilder = ArrayBuilder<Conversion>.GetInstance(n);
            var resultBuilder = ArrayBuilder<TypeSymbolWithAnnotations>.GetInstance(n);
            for (int i = 0; i < n; i++)
            {
                (BoundExpression element, Conversion conversion) = RemoveConversion(elementBuilder[i], includeExplicitConversions: false);
                elementBuilder[i] = element;
                conversionBuilder.Add(conversion);
                resultBuilder.Add(VisitRvalueWithResult(element));
            }

            // Consider recording in the BoundArrayCreation
            // whether the array was implicitly typed, rather than relying on syntax.
            if (node.Syntax.Kind() == SyntaxKind.ImplicitArrayCreationExpression)
            {
                var resultTypes = resultBuilder.ToImmutable();
                // https://github.com/dotnet/roslyn/issues/27961 Initial binding calls InferBestType(ImmutableArray<BoundExpression>, ...)
                // overload. Why are we calling InferBestType(ImmutableArray<TypeSymbolWithAnnotations>, ...) here?
                // https://github.com/dotnet/roslyn/issues/27961 InferBestType(ImmutableArray<BoundExpression>, ...)
                // uses a HashSet<TypeSymbol> to reduce the candidates to the unique types before comparing.
                // Should do the same here.
                HashSet<DiagnosticInfo> useSiteDiagnostics = null;
                // If there are error types, use the first error type. (Matches InferBestType(ImmutableArray<BoundExpression>, ...).)
                var bestType = resultTypes.FirstOrDefault(t => !t.IsNull && t.IsErrorType());
                if (bestType.IsNull)
                {
                    bestType = BestTypeInferrer.InferBestType(resultTypes, _conversions, useSiteDiagnostics: ref useSiteDiagnostics);
                }
                else
                {
                    // Mark the error type as null-oblivious.
                    bestType = TypeSymbolWithAnnotations.Create(bestType.TypeSymbol);
                }
                // https://github.com/dotnet/roslyn/issues/27961 Report a special ErrorCode.WRN_NoBestNullabilityArrayElements
                // when InferBestType fails, and avoid reporting conversion warnings for each element in those cases.
                // (See similar code for conditional expressions: ErrorCode.WRN_NoBestNullabilityConditionalExpression.)
                if (!bestType.IsNull)
                {
                    elementType = bestType;
                }
                arrayType = arrayType.WithElementType(elementType);
            }

            if (!elementType.IsNull && !elementType.IsValueType)
            {
                for (int i = 0; i < n; i++)
                {
                    var conversion = conversionBuilder[i];
                    var element = elementBuilder[i];
                    var resultType = resultBuilder[i];
                    ApplyConversion(element, element, conversion, elementType, resultType, checkConversion: true, fromExplicitCast: false, useLegacyWarnings: false, AssignmentKind.Assignment);
                }
            }

            resultBuilder.Free();
            elementBuilder.Free();
            _resultType = _invalidType;
            return arrayType;
        }


        private bool MethodGroupReturnTypeInference(Binder binder, BoundExpression source, TypeSymbol target, ref HashSet<DiagnosticInfo> useSiteDiagnostics)
        {
...
            var returnType = MethodGroupReturnType(binder, (BoundMethodGroup)source, fixedDelegateParameters, delegateInvokeMethod.RefKind, ref useSiteDiagnostics);
            if ((object)returnType == null || returnType.SpecialType == SpecialType.System_Void)
            {
                return false;
            }

            bool? returnIsNullable = null; // https://github.com/dotnet/roslyn/issues/27961 Review this
            LowerBoundInference(TypeSymbolWithAnnotations.Create(returnType, returnIsNullable), delegateReturnType, ref useSiteDiagnostics);

            return true;
        }

     private bool Fix(int iParam, ref HashSet<DiagnosticInfo> useSiteDiagnostics)
        {
            Debug.Assert(IsUnfixed(iParam));

            var exact = _exactBounds[iParam];
            var lower = _lowerBounds[iParam];
            var upper = _upperBounds[iParam];

            // Fix considering nullability, if possible, otherwise fix ignoring nullability.
            // PROTOTYPE(NullableReferenceTypes): Avoid calling Fix ignoring nullability if the nullability call succeeds.

            // PROTOTYPE(NullableReferenceTypes): Avoid comparing with and without nullability here.
            // Initial binding should use includeNullability: false, and NullableWalker should use
            // includeNullability: true, and NullableWalker should compare the two.
            HashSet<DiagnosticInfo> ignoredDiagnostics = null;
            var withNullability = Fix(exact, lower, upper, ref ignoredDiagnostics, _conversions, includeNullability: true);
            var withoutNullability = Fix(exact, lower, upper, ref useSiteDiagnostics, _conversions, includeNullability: false);

            var best = withNullability;
            if (best.IsNull)
            {
                best = withoutNullability;
            }
            else
            {
                // If the first attempt succeeded, the result should be the same as
                // the second attempt, although perhaps with different nullability.

                // PROTOTYPE(NullableReferenceTypes): Results may differ by tuple names or dynamic.
                // See NullableReferenceTypesTests.TypeInference_TupleNameDifferences_01 for example.
                if (!withNullability.TypeSymbol.Equals(withoutNullability.TypeSymbol, TypeCompareKind.IgnoreDynamicAndTupleNames))
                {
                    // The two results differ other than nullability.
                    // Use the second result, for compatibility.
                    Debug.Assert(false);
                    best = withoutNullability;
                }
            }
...
        }

        private static TypeSymbolWithAnnotations MergeDynamic(TypeSymbolWithAnnotations firstWithAnnotations, TypeSymbolWithAnnotations secondWithAnnotations, AssemblySymbol corLibrary)
        {
...
            var result = DynamicTypeDecoder.TransformTypeWithoutCustomModifierFlags(first, corLibrary, RefKind.None, mergedFlags);
            return TypeSymbolWithAnnotations.Create(result); // PROTOTYPE(NullableReferenceTypes): Handle nullability.
        }


        private static TypeSymbolWithAnnotations MergeTupleNames(TypeSymbolWithAnnotations firstWithAnnotations, TypeSymbolWithAnnotations secondWithAnnotations)
        {
...

            var result = TupleTypeDecoder.DecodeTupleTypesIfApplicable(first, mergedNames);
            return TypeSymbolWithAnnotations.Create(result); // PROTOTYPE(NullableReferenceTypes): Handle nullability.
        }


        private static bool? GetIsNullable(ImmutableArray<TypeSymbolWithAnnotations> types)
        {
            bool? isNullable = false;
            foreach (var type in types)
            {
                if (type.IsNull)
                {
                    // PROTOTYPE(NullableReferenceTypes): Should ignore untyped
                    // expressions such as unbound lambdas and typeless tuples.
                    // See NullableReferenceTypesTests.LocalVar_Array_02 test.
                    isNullable = true;
                    continue;
                }          
...
        }

[jcouv]

Can this issue be resolved?

[cston]

There are still a number of comments in NullableWalker tagged with this issue.

[gafter]

The issue reported in OP no longer exists. It is necessary to go through and figure out what needs to be done anywhere this issue marks a problem in the code.

[RikkiGibson]

I may clean up the references to this issue as part of #43536