Possible false positive with UNT0008 when nullable type is used

[NoTuxNoBux]

Bug description

UNT0008 (null propagation) also triggers when (explicit) nullable types are used for Unity types.

• Version of analyzers assembly: 1.13.0
• Analyzer rule: UNT0008
• Error (exception message, type, and callstack where applicable):

Unity objects should not use null propagation.

To Reproduce

class A
{
    private GameObject foo;
    private GameObject? bar;

    public void Method()
    {
        foo?.SetActive(true); // Reports correctly.
        bar?.SetActive(true); // Reports incorrectly (?).
    }
}

Expected behavior

In the case of explicitly nullable types, null propagation can be used validly to check if the object is actually (and only) null, much like is null and is not null, all of which disregard the equality operator (intentionally).

Additional context

tl;dr: I can argue for both sides of disabling or enabling the warning in the case of an explicitly nullable type, but there seem to always be cases where you either get the warning undesirably, or don't get the warning when you need it.

I'm not entirely sure what the desired behaviour is with explicitly nullable types. It's still a risk to use null propagation because of Unity's equality operator overload, but this may be done intentionally (e.g. initialize-once variable, which doesn't care if the underlying object is destroyed or not). At some point, though, explicitly nullable types will become the default, and if the warning is disabled in this scenario, people might accidentally be using it again.

Still, in a future scenario where every type is explicitly nullable or not at all, comparing a non-nullable GameObject through == null makes little sense; it can't ever be actually null, so you're abusing the == null to call Unity's operator overload. In this scenario it would probably make sense if Unity replaced it with GameObject.IsDestroyed or something instead, which makes it implicit the instance exists and you're calling something on it (you already know it's not truly null). If it is nullable, is null and null propagation skip the overload, which is likely intentional, as you said your type could be null.

[sailro]

cc @jbevain

[tannergooding]

Was going to come here and open an issue as well.

?. for reference types in C# does not use operator ==. It is equivalent to using is null or (object)x == null and does a pure null check ignoring any user defined semantics.

It is therefore safe to use since it is purely a defense against dereferencing a null.

You can trivially see this in an example such as https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBLANgHwAEAmARgFgAoQgZgAIS6BhOgbyrs4fsNKTuAQIuOhAAOMKAEMM0OgF55ACha4AFgGc0zOlE0BKBQD46AMym4NMANwcudzrQZ8BQkeMky5AQmWrN2ix6GobyJuaWNg500U68/LwADHQAKjAaGCp0YKEmYAD8AHQpEADKGFDYAHYA5kr6tpQAvkA===

Where the IL for static string Test(C c) => c?.ToString(); is:

    .method public hidebysig static 
        string Test (
            class C c
        ) cil managed 
    {
        // Method begins at RVA 0x20a0
        // Code size 12 (0xc)
        .maxstack 8

        IL_0000: ldarg.0
        IL_0001: brtrue.s IL_0005

        IL_0003: ldnull
        IL_0004: ret

        IL_0005: ldarg.0
        IL_0006: callvirt instance string [System.Runtime]System.Object::ToString()
        IL_000b: ret
    } // end of method C::Test

[jbevain]

@tannergooding it is safe to use but using null propagation and null coalescing on Unity objects is inherently ambiguous due to Unity's special handling of null comparisons for destroyed game objects. Our analyzer provides an information here (and not a warning) that you might want to refactor your code to be explicit about the intent.

[jbevain]

@NoTuxNoBux C# nullable types is also something that is going to be ambiguous with regards to Unity's special null handling. Imagine you have:

#nullable enable

public static class Foo
{
    public static void Bar(GameObject go)
    {
        go.Destroy();
        if (go == null)
        {
            Debug.Log("GameObject is null");
        }
    }
}

In the method Bar, the C# compiler thinks that go can not be null. But after a call to .Destroy, == null is returning true. That goes pretty much against the feature. GameObject? is ambiguous too. I'm not sure how we should approach nullable reference types in the context of Unity.

[NoTuxNoBux]

In the method Bar, the C# compiler thinks that go can not be null. But after a call to .Destroy, == null is returning true. That goes pretty much against the feature.

I understand what you mean, and I wanted to add some context here: Unity's GameObject explicitly overloads operator==, so even in a nullable context the compiler could see that == null makes sense.

As Unity's operator== overload references Object as type, and is not in a nullable context, the compiler could deduce that == null makes sense in this context - it can't not do so because whether it is depends on what the operator overload says (at runtime), even if the actual variable can never be fully null (i.e. it can still be truthy to compare to value null even though the object itself is never null).

(For reference, there was some related discussion for Roslynator with Unity here: dotnet/roslynator#852.)