Warnings reported after comparing with object == or !=

[cston]

Warnings are reported for the second and third cases:

class C
{
    static void F(object? x, object y)
    {
        if (x != null) x.ToString();
        if ((object)x != null) x.ToString(); // warning
        if (x == y) x.ToString();            // warning
    }
}

[jcouv update:] Besides object equality, we should also consider other types (using attribute annotations).

[sharwell]

Why would the second line not be desired? I would expect the following to be the solution there:

-if ((object)x != null) x.ToString(); // warning
+if ((object?)x != null) x.ToString();

[cston]

@sharwell explicit cast should not remove top-level nullability so the inferred state of (object)x is object?.

[jcouv]

I think the first scenario ((object)x != null) was fixed.
@cston @333fred What do you think about the second scenario (x == y informing null-state)?

class C
{
    static void F(object? x, object y)
    {
        if (x != null) x.ToString();
        if ((object)x != null) x.ToString(); // no warning
        if (x == y) x.ToString();            // warning
    }
}

sharplab

Update:
To handle this properly, we should probably analyze custom operators (they may not allow for this inference), but recognize operator==(object, object) specially or annotate it with a special attribute.

[333fred]

My first instinct is that it should inform nullability, and I suspect that it will be most of our users first instinct as well.

[jcouv]

I'm not sure that we can learn from the x == y test.
Should we learn that x is non-null, or that y may be null? Both use cases seem plausible.
Maybe defensive checks for y == null should have been placed before checks for x == y, but that seems a strong assumption to make.

static void F(object? x, object y)
{
    if (x == y) x.ToString(); // warning is correct
    if (y == null) y = "";
}

[sharwell]

@sharwell explicit cast should not remove top-level nullability so the inferred state of (object)x is object?.

I find this unintuitive in the context of the current issue. Do you have a link to explanation of cases where this behavior is expected to produce behavior users are expecting?

[jcouv]

@sharwell I'll try to reconstruct the rationale supporting the current behavior of cast. I just remember it was to help with migrating existing code.

A few options:

1. current: (string) cast passes top-level nullability through, W-warns on maybe-null input
2. alternative: (string) cast produces a non-null value, and therefore should produce a safety warning on maybe-null input (otherwise we're decreasing safety)

I don't recall whether there are other options that are sensible. But between those two, (1) is preferable because it allows someone to migrate existing code with the safeonlywarning option (ie. no W-warning) with minimal annoyance if their code was correct/safe.

I'll go ahead and close this issue, as we've implemented the specified behavior.