resharper-unity icon indicating copy to clipboard operation
resharper-unity copied to clipboard

Disable UnityObjectNullCoalescingWarning at Call site.

Open MHDante opened this issue 3 years ago • 2 comments

Concerning: Possible unintended bypass of lifetime check of underlying Unity engine object

I have some objects that represent singleton modules: These modules are Null when disabled.

public static class ServiceProvider {
    public static SingletonBehaviour1 Singleton1;
    public static SingletonBehaviour2 Singleton2;
    public static SingletonBehaviour3 Singleton3;
}

// current code
public void Update(){
    if(ServiceProvider.Singleton1) ServiceProvider.Singleton1.DoSomething();
}

// want to write:
public void Update(){
    ServiceProvider.Singleton1?.DoSomething(); // raises UnityObjectNullCoalescingWarning 
}

I Understand the potential risks with doing this (destroyed checks and whatnot), but these are objects that I know will either be (true) null or a non-destroyed instance.

I know I can currently do this:

public void Update(){
     // ReSharper disable once PossibleNullReferenceException
    ServiceProvider.Singleton1?.DoSomething();
}

But the singletons are accessed in many more areas, and this would pollute the code with a lot of warning disables. Instead. I'd like to be able to mark the fields themselves (preferably with a [CanBeNull]), thus disabling the warnings elsewhere.

public static class ServiceProvider {
    [CanBeNull] public static SingletonBehaviour1 Singleton1;
    [CanBeNull] public static SingletonBehaviour2 Singleton2;
    [CanBeNull] public static SingletonBehaviour3 Singleton3;
}

MHDante avatar Nov 09 '22 19:11 MHDante

Currently, your only options are to use a comment like // ReSharper disable once Unity.NoNullPropagation (for ?.) or // ReSharper disable once Unity.NoNullCoalescing (for ??), or to disable the inspections for the whole project/globally, or to change the severity to be e.g. a hint, which can be much less intrusive.

Going forward, we plan to change how these inspections work, because C# has many ways of checking for null and not null that avoid the custom Equals implementation that it's easier to highlight when you are making the additional lifetime check, than when you're not. This would mean the inspection is no longer a warning (checking for C# reference null is the default, expected operation) and would become something informational (oh look, here you're doing an additional check). That should help in scenarios like this, and should provide enough context that you should notice when it's missing.

citizenmatt avatar Nov 10 '22 15:11 citizenmatt

I appreciate the change. That said. In cases other than these, the warning is extremely useful (unset editor references primarily). Looking forward to trying the new implementation.

MHDante avatar Nov 11 '22 18:11 MHDante

Fixed in 2024.1 as part of RIDER-106007.

citizenmatt avatar Mar 04 '24 16:03 citizenmatt