Roslyn fails to verify nullabilty after an equality expression with non-lifted struct operator accepting nullable values

[TessenR]

Version Used:

Branch main (9 Nov 2021)
Latest commit 973b3aa by CyrusNajmabadi:
Merge pull request #57632 from CyrusNajmabadi/depProjectChecksum

Move hte dependent checksum computation directly into the CompilationTracker component

Steps to Reproduce:

Compile and run the following code:

#nullable enable
struct S
{
    public static bool operator ==(S? left, S? right) => false;
    public static bool operator !=(S? left, S? right) => false;
    public override bool Equals(object? obj) => false;
    public override int GetHashCode() => 0;

    public S M0(out object x) { x = 42; return this; }

    static void Main()
    {
        M1(null, null);
    }
    public static void M1(S? s, object? x)
    {
        _ = s?.M0(out x) != new S()
            ? ""
            : x.ToString();
    }
}

Expected Behavior: CS8602: Dereference of a possibly null reference. warning reported for x.ToString in M1

Actual Behavior: No warnings at all in the program above. It crashes with a NullReferenceException at runtime.

Notes: It looks like Roslyn fails to differentiate lifted operators from explicitly provided user-defined operators that accept nullable values.

Roslyn behavior would have been correct for an operator that accepts parameters of type S since such operator would've been lifted to accept null values.

However, the operator used in this code sample is user defined which means that it can return anything e.g. based on whether a developer wants to treat an empty struct the same as null values or not. If a developer deemed it necessary to explicitly override the default lifted operator behavior by providing an operator that explicitly deals with nullable values it's not safe to assume anything about the operator's behavior.

[RikkiGibson]

I was writing a comment here with nice examples and everything but accidentally closed the tab, got sidetracked, etc.. Unfortunate.

You are making a fair point. It just wasn't where we landed when we considered how different kinds of user defined equality operators should behave. We made an assumption for nullable analysis that a well behaved user-defined operator would treat null as not-equal to not-null things in general.

This assumption seems useful for user defined ==/!= operators on reference types, where people often just want to ensure that their custom equality is used for the object instances, while still treating null inputs as not-equal to not-null inputs.

Perhaps a different behavior really is more appropriate for value types. It is true that if the user simply wanted to make null be not-equal to not-null for their nullable value type, they would simply implement the custom operator on the non-nullable version of the type. The non-assumption suggested in the issue description is what we do for definite assignment since we consider it a more serious safety issue on that side.

I think the suggested change here would be fine.