NullableWalker.LearnFromNullTest should not blindly strip conversions

[jcouv]

        private int LearnFromNullTest(BoundExpression expression, ref LocalState state)
        {
            var expressionWithoutConversion = RemoveConversion(expression, includeExplicitConversions: true).expression;
            var slot = MakeSlot(expressionWithoutConversion);
            return LearnFromNullTest(slot, expressionWithoutConversion.Type, ref state);
        }

Depending on the conversion, we can learn something about x when we learn that (C)x is null. For instance, in an implicit reference conversion.

Also, in a scenario with a user-defined conversion operator C(X x) with non-null input and output, I think we can infer from the contract that x was null if the output was null.

        [Fact]
        public void MaybeNull_OnExplicitConversion()
        {
            var c = CreateCompilation(new[] { @"
using System.Runtime.CompilerServices;
public class A
{
    public static explicit operator C(A? a) => throw null!;
}
public class C
{
    public void Main()
    {
        A a = new A();
        _ = IsNull((C)a)
            ? a.ToString()
            : a.ToString();
    }
    public static bool IsNull([MaybeNull] C c) => throw null!;
}
", MaybeNullAttributeDefinition }, options: WithNonNullTypesTrue());

            // Both diagnostics are unexpected 
            // We should not blindly strip conversions when learning that a value is null.
            // In this case, we shouldn't infer that `a` may be null from the fact that `(C)a` may be null.
            // Tracked by https://github.com/dotnet/roslyn/issues/36164
            c.VerifyDiagnostics(
                // (13,15): warning CS8602: Dereference of a possibly null reference.
                //             ? a.ToString()
                Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "a").WithLocation(13, 15),
                // (14,15): warning CS8602: Dereference of a possibly null reference.
                //             : a.ToString();
                Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "a").WithLocation(14, 15)
                );
        }

[RikkiGibson]

I think I figured out the answer for this inadvertently. May solve at least part of this as part of "improved definite assignment".

[RikkiGibson]

I think we should see what happens when we make the analysis a little more strict. RemoveConversion should not unwrap user-defined conversions except in the following scenarios:

• The user-defined conversion is lifted (i.e. we know on a language level that the null->null, notnull->notnull requirements are met)
• The conversion has a [return: NotNullIfNotNull] attribute referencing the parameter. This attribute is almost always used on conversions which meet the null->null, notnull->notnull requirements. I do not know if we should also require that the parameter and return types of the conversion are nullable.

If the impact of this on real-world code seems minimal, then I'd like to try shipping it as a bug fix level change. But if the impact is pretty severe, then we should look at what scenarios we broke, and whether those breaks are indicative of bugs in user code, or of an essentially valid usage pattern that wasn't properly annotated or that we didn't account for.

/cc @jcouv @333fred