Incorrect "CS8602 Dereference of a possibly null reference" after pattern-matching through a derived type

jnm2 commented 5 months ago

Version Used: VS 17.10.0-p2 and SharpLab

The repro disappears if the pattern matching is done via Base instead of Derived or if Foo is not read in the Derived property pattern.

var d = new Derived { Foo = new object() };

_ = (object)d switch
{
    Derived { Foo: 4 } => 0,
    // ⚠️ CS8602 Dereference of a possibly null reference.
    //                     ↓
    Base { Foo: { } f } => f.GetHashCode(),
    _ => 0,
};

public class Base
{
    public object? Foo { get; set; }
}

public class Derived : Base;

Diagnostic Id:

CS8602

Expected Behavior:

No warning.

Actual Behavior:

Warning as shown in the code sample.

jjonescz commented 4 months ago

Notes from investigation so far:

The DAG of the switch is:

[0]: t0 is Derived ? [1] : [9]
[1]: t1 = (Derived)t0; [2]
[2]: t2 = t1.Foo; [3]
[3]: t2 is int ? [4] : [10]
[4]: t3 = (int)t2; [5]
[5]: t3 == 4 ? [6] : [7]
[6]: leaf <arm> `Derived { Foo: 4 } => 0`
[7]: t4 = (Base)t0; [8]
[8]: t5 = t4.Foo; [13]
[9]: t0 is Base ? [10] : [15]
[10]: t4 = (Base)t0; [11]
[11]: t5 = t4.Foo; [12]
[12]: t5 != null ? [13] : [15]
[13]: when <true> ? [14] : <unreachable>
[14]: leaf <arm> `Base { Foo: { } f } => f.GetHashCode()`
[15]: leaf <arm> `_ => 0`

Note that in path 0-1-2-3-4-5-7-8-13-14 the property is accessed twice (in [2] and [8]), each time saved into a different temp. This is an optimization which happens when (object)d is determined to be Derived, its property Foo is determined to be int but then it doesn't have the value 4, we fast-forward into the second switch arm by casting d to Base, accessing Foo again but not checking it for != null again.

But nullability analysis does not track aliased references, for example:

var p1 = C.P;
if (p1 != null)
{
    p1.GetHashCode();
    var p2 = C.P;
    p2.GetHashCode(); // warning: p2 may be null
}

static class C
{
    public static object? P { get; set; }
}

Given this, I think there are different ways to fix the superfluous nullability warning:

Make nullability analysis track aliased references like in the example above which I think in effect would also fix the original bug. I guess that would be a large change.
Change nullability analysis of patterns only, so the change is smaller than in option 1. Feels like undesirable inconsistency with normal nullable analysis. Maybe only the specific scenario could be targeted, not arbitrary aliased references.
Change the DAG so it checks the property for != null after it accesses it the second time, although that's not necessary, but would silence the nullability warning. That means effectively disabling the optimization that is there (the != null check is deliberately dropped during the DAG construction since is int implies it).
Change the DAG to not access the property twice somehow via some other optimization - it's accessed once in Derived, other time in Base.
Change nullability analysis to not work on the optimized DAG graph, but an unoptimized one (not that we have it available).

jaredpar commented 4 months ago

This is a variant of the track nullness through bool or an alias. That does not work today and is considered "By Design". This is the language issue tracking it.

https://github.com/dotnet/csharplang/discussions/2388