Update methods from initial binding based on inferred nullability

[cston]

There are a number of cases in NullableWalker where methods from initial binding should be updated based on the inferred nullability.

For instance, both calls to .ToString() below should report warnings but no warning is reported for the second case currently.

#nullable enable
class C<T>
{
    internal T F() => throw null;
}
class Program
{
    static C<T> Create<T>(T t)
    {
        return new C<T>();
    }
    static void F(string? s)
    {
        new C<string?>().F().ToString(); // warning: maybe null
        Create(s).F().ToString();        // no warning
    }
}

Test cases affected: NullableReferenceTypesTests.ImplicitConversions_07

[jcouv update:] both calls to .ToString() now report warnings (sharplab) but more references to this issue remain in the source.

[jcouv]

Note that I've replace one or more PROTOTYPE markers with references to this issue since they should be considered when fixing this issue:

        public override BoundNode VisitCall(BoundCall node)
        {
            // Note: we analyze even omitted calls
            var method = node.Method;
            var receiverOpt = node.ReceiverOpt;
            if (receiverOpt != null && method.MethodKind != MethodKind.Constructor)
            {
                VisitRvalue(receiverOpt);
                CheckPossibleNullReceiver(receiverOpt);
                // Update method based on inferred receiver type: see https://github.com/dotnet/roslyn/issues/29605.
            }

            // https://github.com/dotnet/roslyn/issues/29605 Can we handle some error cases?
            // (Compare with CSharpOperationFactory.CreateBoundCallOperation.)
            if (!node.HasErrors)
            {
                ImmutableArray<RefKind> refKindsOpt = node.ArgumentRefKindsOpt;
                (ImmutableArray<BoundExpression> arguments, ImmutableArray<Conversion> conversions) = RemoveArgumentConversions(node.Arguments, refKindsOpt);
                ImmutableArray<int> argsToParamsOpt = node.ArgsToParamsOpt;

                method = VisitArguments(node, arguments, refKindsOpt, method.Parameters, argsToParamsOpt, node.Expanded, node.InvokedAsExtensionMethod, conversions, method);
            }

            UpdateStateForCall(node);

            if (method.MethodKind == MethodKind.LocalFunction)
            {
                var localFunc = (LocalFunctionSymbol)method.OriginalDefinition;
                ReplayReadsAndWrites(localFunc, node.Syntax, writes: true);
            }

            //if (this.State.Reachable) // Consider reachability: see https://github.com/dotnet/roslyn/issues/28798
            {
                _resultType = method.ReturnType;
            }

            return null;
        }

[RikkiGibson]

It seems like the second warning is getting produced as of 21 May. Is this still an issue @cston?

https://sharplab.io/#v2:EYLgtghgzgLgpgJwDQxASwDYB8DEA7AVwwwmAzgAI49TyBYAKAAEAmCgYQB4AVAPkYDejCiIpo88BDQwVuFAGIAKAJQUAvLwowAFggD2AdwqFiAbkYBfRqwpMAjAHZBw0fYBsHHpvYI4EeF6KcjDKLiJCDKJRtg7GcEZcfCrmkaJWqSLutgAsCor2AAwA/BRQoRkUEdGiePGehUW8KgB0SsrN3HoAyjAI4gDmyRQA9MMUBhBSAyAUkACewJQmGGHRPn7wimWtLZ09fXiDyqbVIqPGeuOTeAOr6RZAA==

[cston]

There are still a number of references to this issue in NullableWalker but it looks like the case in the original repro has been fixed.

[jaredpar]

@cston the original issue appears to be fixed now. I see two other references to this issue in NullableWalker but not clear what should happen based on those comments.

How can we move this issue forward?

[TessenR]

@jaredpar Since I'm subscribed to this issue based on one of these comments I can suggest a problematic scenario derived from one of them.

Version Used:

Branch master (9 Feb 2021)
Latest commit 2d3ffe0 by msftbot[bot]:
Merge pull request #49990 from ryzngard/feature/namespace_analyzer

Add sync namespace analyzer

Steps to Reproduce: Compile and run the following code:

#nullable enable
class A<T>
{
  public T field;

  public A(T t) => field = t;

  public static A<T> operator &(A<T> x, A<T> y) { y.field = x.field; return x; }
  public static bool operator true(A<T> a) => true;
  public static bool operator false(A<T> a) => false;
}

class Test { }

class Program
{
  static void Main(string[] args)
  {
    A<Test> f = F<Test>(null);
    f.field.ToString();
  }

  static A<T> CreateA<T>(T t) => new A<T>(t);

  static A<T> F<T>(T? x) where T : class?, new()
  {
    T y = new();
    var ax = CreateA(x);
    var ay = CreateA(y);
    _ = ax && ay; // here
    return ay;
  }
}

Expected result: Warning for ay in ax && ay. CS8620: Argument of type 'A<T>' cannot be used for parameter 'y' of type 'A<T?>' in 'A<T>.&' due to differences in the nullability of reference types

Actual result: No warnings at all. The program crashes at runtime with a NullReferenceException

Notes It looks like Roslyn doesn't update the type argument of A<T>.& based on nullability inferred from the nullable analysis

[jaredpar]

@TessenR excellent! Thank you for the scenario here.