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

[jnm2]

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]

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:

1. 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.
2. 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.
3. 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).
4. Change the DAG to not access the property twice somehow via some other optimization - it's accessed once in Derived, other time in Base.
5. Change nullability analysis to not work on the optimized DAG graph, but an unoptimized one (not that we have it available).

[jaredpar]

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.

dotnet/csharplang#2388

[alrz]

Could this be a side effect of #34933?

[CyrusNajmabadi]

@jaredpar i'm not sure hwo this is through a bool or alias. in the above code it's simply:

    // ⚠️ CS8602 Dereference of a possibly null reference.
    //                     ↓
    Base { Foo: { } f } => f.GetHashCode(),

Because of hte {} f pattern we know 'f' to be non-null. to even be assigned. It's unclear ot me why the inheritance hierarchy of Base/Derived would come into play here.

[CyrusNajmabadi]

we fast-forward into the second switch arm by casting d to Base, accessing Foo again but not checking it for != null again.

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).

That seems to be hte issue. Even if we don't want to emit the != check, we still want to act as if it is there (cecause... well... it is :)).. I think that the nullability analysis is being impacted by internal representations and optimizations of that representation is problematic. It's effectively leaking an internal optimization detail.

Can we have the check be in the DAG, but mark it as superfluous for emitting?

[CyrusNajmabadi]

I don't see this as a case of an alias. In this case, if there is an alias, it's an internal impl detail of the compiler in how it chose to represent the code. Form the perspective of the lang, there is no alias, and this should not give a null-warning. I think we should fix the internal representation.

Could nullability analysis be done on the initial unoptimized DAG? THen we optimize the dag and use that in emit?

[jaredpar]

Could nullability analysis be done on the initial unoptimized DAG? THen we optimize the dag and use that in emit?

Can it be done: yes. Is it worth the significant code change for this bug: almost certainly no.

[CyrusNajmabadi]

What about having the != be in the dag, but with a bit htat says it doesn't need to be emitted? basically, instead of removing hte items from the dag, mark them. Then nullability analysis will see them and report properly, but emit can also know they're superfluous?

[jaredpar]

What about having the != be in the dag, but with a bit htat says it doesn't need to be emitted?

Possibly. It's a new data flow path we'd need to design and push into our NRT analysis which is already incredibly complex. Just don't see it meeting our bar anytime soon.

[CyrusNajmabadi]

That's fine. Can we leave this open though? To me, this seems more like a bug in our impl, versus an intentional limitation of the design of nullability analysis. I can totally see the bug and costs not meeting hte bar any time soon. But i think this issue would still be worthwhile to track this.