!(x is T) and x is not T unequal semantics when x is of T type

[controlflow]

Version Used:

master branch 10.11.2020

Steps to Reproduce:

Compile and run the following code:

using static System.Console;

var c = new C();
M1(c);
M2(c);

void M1(C c) {
  if (!(c is C c1)) return;

  WriteLine(c1.Foo); // prints 42
}
    
void M2(C c) {
  if (c is not C c1) return;

  WriteLine(c1.Foo); // throws NRE
}

public class C 
{
    public int Foo => 42;
}

Expected Behavior:

Two 42 values being printed.

Actual Behavior:

One 42 is printed followed by NullReferenceException.

https://twitter.com/gafter/status/1326187196856954881

[CyrusNajmabadi]

This seems like a bug. if (c is not C c1) return; this should trigger for null.

[LeroyK]

I also ran into this because I have a case where nullability analysis warns for CS8602 (possible null ref) after an if not pattern matching expression (in a case similar to T10 below). It doesn't give this warning in this example though and I'm trying to create a repro.

I did however do some more tests and it looks like c1 doesn't get assigned when both the left expression type and the matching type of the is not pattern matching expression are equal and if (and only if?) the left expression is a local variable.

This should be fixed in 16.8.1.

#nullable enable
using static System.Console;

var c = new C();

int index = 0;

Write($"T{++index}: ");
T1(c);
Write($"T{++index}: ");
T2(c);
Write($"T{++index}: ");
T3(c);
Write($"T{++index}: ");
T4(c);
Write($"T{++index}: ");
T5(c);
Write($"T{++index}: ");
T6(c);
Write($"T{++index}: ");
T7(c);
Write($"T{++index}: ");
T8(new CSub());
Write($"T{++index}: ");
T9(new S(c));
Write($"T{++index}: ");
T10(new S(c));
Write($"T{++index}: ");
T11(c);
Write($"T{++index}: ");
T12(new S(c)); // throws
Write($"T{++index}: ");
T13(c); // throws
Write($"T{++index}: ");
T14(c); // throws

static void T1(C c)
{
    if (!(c is C c1)) return;
   
    WriteLine(c1.Foo); // prints 42
}

static void T2(object c)
{
    if (c is not C c1) return;
  
    WriteLine(c1.Foo); // prints 42
}

static void T3(CRoot c)
{
    if (c is not C c1) return;
   
    WriteLine(c1.Foo); // prints 42
}

static void T4(CBase c)
{
    if (c is not C c1) return;
    
    WriteLine(c1.Foo); // prints 42
}

static void T5(IInterface c)
{
    if (c is not C c1) return;
    
    WriteLine(c1.Foo); // prints 42
}

static void T6(IInterface c)
{
    if (c is not IInterface2 c1) return;
    
    WriteLine(c1.Foo); // prints 42
}

static void T7(C c)
{
    if (c is not IInterface2 c1) return;

    WriteLine(c1.Foo); // prints 42
}

static void T8(CSub c)
{
    if (c is not C c1) return;

    WriteLine(c1.Foo); // prints 42
}

static void T9(S s)
{
    if (s.C is not C c1) return;

    WriteLine(c1.Foo); // prints 42
}

static void T10(S s)
{
    while (true)
    {
        if (s.C is not C c1) continue;

        WriteLine(c1.Foo); // prints 42
        break;
    }
}

static void T11(C c)
{
    if ((c ?? c) is not C c1) return;

    WriteLine(c1.Foo); // prints 42
}

static void T12(S s)
{
    var c = s.C;

    if (c is not C c1) return;

    WriteLine(c1.Foo); // throws NRE
}

static void T13(C c)
{
    var c2 = c;

    if (c2 is not C c1) return;

    WriteLine(c1.Foo); // throws NRE
}

static void T14(C c)
{
    if (c is not C c1) return;

    WriteLine(c1.Foo); // throws NRE
}

public abstract class CBase : CRoot { }

public class CRoot { }

public interface IInterface { }

public interface IInterface2
{
    int Foo { get; }
}

public class C : CBase, IInterface, IInterface2
{
    public int Foo => 42;
}

public class CSub : C { }

public readonly struct S
{
    public readonly C? C;

    public S(C? c) => C = c;
}

[alrz]

@LeroyK You can use if (c is not { } c1) return; to workaround this for now. This only happens when the matched type is identical to the target. That's what you get with { } as well.

[LeroyK]

@alrz I have just tried this and it also produces incorrect result (c1 is still not assigned). A workaround that seems to work is the one used in T11 in my example: if ((c ?? c) is not { } c1) return;.

[alrz]

Yeah, I haven't tested it. if (c is not {} c1) has the same problem. Thanks.

https://sharplab.io/#v2:EYLgtghgzgL...

[cston]

Thanks for the bug reports. I'm investigating how to fix this.

[springy76]

Is this part of net 5.0.1? The "blog roundup" tells nothing about any Roslyn fixes.

As VS suggests and even can automatically bulk apply rewrites of code which then will do different things than before, I can neither allow C#9 nor net5 to be used at our company while such catastrophic bugs exist!

[CyrusNajmabadi]

@springy76 #49369 has not been merged in yet.

[springy76]

Still no mention in neither NET nor VS release note blog posts. Where/when will this fix be deployed? NET SDK version? Visual studio version?

[CyrusNajmabadi]

@springy76 It shows here:

[springy76]

I really wonder how many VS releases you will publish with this hold-the-press show-stopper: One click on a green squiggled word quicktip by any developer anywhere in any company will automatically transform working code to possibly NRE throwing code.

[CyrusNajmabadi]

@springy76 see #49262 (comment)