Fix inference with ref parameter with maybe-null value

[jcouv]

Previously, the L-value of a ref argument would contribute its type+nullability. Now we're using the R-value.

Fixes #47663
Fixes #35534

Note that the inference in some scenario becomes a bit worse (for example with CompareExchange) when there is a ref argument and null. There is a known issue with null not contributing nullability to method type inference.

We could discuss whether to hold off this change until the issue with null is fixed.
I think the current change can be taken as-is, because (1) they involve having a nullable but not-null ref, which is uncommon for CompareExchange, and (2) they can be mitigated with explicit type parameters.

[jcouv]

Tagging @stephentoub @jkotas.
This PR changes the way ref arguments contribute to method type inference with nullability. Instead of using the declared type of the ref (L-value), we're using it's tracked value (R-value).

But, due to another open issue with null literals not contributing nullness to the inference, this makes the inference worse in some scenarios.

For example, calls to CompareExchange when the arguments are either thought be not-null or the null literal.
The tests show the 3 impacted scenarios for CompareExchange:

• [InlineData("locationNonNull", "valueNonNull", "null", true)]
• [InlineData("locationNonNull", "null", "comparandNonNull", false)]
• [InlineData("locationNonNull", "null", "null", false)]

I believe those are uncommon (the location argument would typically be maybe-null).

Any other generic API call with a ref and a null literal could be impacted in the same way. We were unable to fix the problem with null in VS 16.8. Aiming for 16.9 at this point.

[RikkiGibson]

How did your change affect this scenario? I can get how before we inferred a double[] type argument and now we infer a double[]? type argument, but how did that result in the postcondition for the NotNullIfNotNullAttribute starting to be respected? #Resolved

[jcouv]

We used to produce a warning on ref f1 for assigning maybe-null value inbound to the first parameter (which was inferred as double[]!.
For ref f2 we now produce a diagnostic on the outbound assignment from parameter double[]? to local typed double[]!.

---

In reply to: 493893383 [](ancestors = 493893383)

[RikkiGibson]

Makes complete sense now, thanks!

[stephentoub]

@jcouv, have you tried building dotnet/runtime? Does this introduce any new warnings-as-error as part of its build? Thanks.

[jaredpar]

Can you add // 1 comments here to help identify which diagnostic lines up where?

[jcouv]

Testing this change in runtime repo caused impact in one location. Here's the PR to update runtime repo: dotnet/runtime#42707

[jcouv]

The VS validation build has two new warnings because of this change:

Q:\cmd\1r\src\vc\projbld\ExternalBuildFramework\Impl\Services\BuildProjectScannerService.cs(1351,85): error CS8625: Cannot convert null literal to non-nullable reference type.
Q:\cmd\1r\src\vc\projbld\ExternalBuildFramework\Impl\Services\BuildProjectScannerService.RunningTaskMonitor.cs(78,72): error CS8625: Cannot convert null literal to non-nullable reference type.
Q:\cmd\0\src\vc\projbld\CMake\libcmake\CMakeServer\CMakeServerDaemon.cs(285,72): error CS8625: Cannot convert null literal to non-nullable reference type.

Q:\cmd\0\src\vc\projbld\CMake\libcmake.Linux\CMakeServer\WSLCMakeServerDaemon.cs(186,86): error CS8625: Cannot convert null literal to non-nullable reference type.
Q:\cmd\0\src\vc\projbld\CMake\libcmake.Linux\CMakeServer\RemoteCMakeServerDaemon.cs(180,86): error CS8625: Cannot convert null literal to non-nullable reference type.

BuildProjectScannerService.cs
BuildProjectScannerService.RunningTaskMonitor.cs

Those are consequences of the known issue with null literals not contributing properly to method type inference. The recommended fix is to use null! for now.

FYI @dibarbet

I'll add a test to this PR to illustrate that scenario.

[dibarbet]

The VS validation build has two new warnings because of this change:

@jcouv would you mind if I updated your validation PR (I assume https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/277093?_a=overview) to include the suggested fixes? That way I can be sure cloud build is passing with them and I can easily cherry-pick the commit over to the proper insertion

[jcouv]

@dibarbet Thanks. I've pushed two commits to the validation PR and will ping you once it is green.

[RikkiGibson]

Here are a few scenarios that we should try to make work in a future PR. I think M2() may not have desired behavior due to #43536-- it may warn on the null literal argument instead of on accessing members on the return value.

#nullable enable

public class C {
    public void M1<T>(ref T t1, T t2)
    {
        t1 = t2;
    }
    
    public void M2()
    {
        string? s = "a";
        M1(ref s, null);
        s.ToString(); // should warn
    }
    
    public void M3()
    {
        string? s1 = "a";
        string? s2 = null;
        M1(ref s1, s2);
        s1.ToString(); // should warn
    }
    
    public void M4()
    {
        string? s = "a";
        M1(ref s, "b");
        s.ToString(); // should not warn
    }
}

[dibarbet]

@jcouv forgot to mention - to get a passing cloudbuild you may need the two extra commits from this PR
https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/276948?_a=commits

[jcouv]

@dibarbet Thanks. I indeed had to add the two commits from that other PR to get the build to pass.
Feel free to grab the commits from https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_git/VS/pullrequest/277093 (there are five, each adding a suppression. They can be squashed).
We'll go ahead and merge the roslyn change.