search

JetBrains / resharper-unity

Unity support for both ReSharper and Rider
Apache License 2.0
1.21k stars 134 forks source link

Mark transform as not-nullable #1503

Open JeromeJ opened 4 years ago

JeromeJ commented 4 years ago

The ReadMe states

Mark Component.gameObject and Object.name as not-nullable

But transform is not explicitly marked as not-null as well despite Unity official documentation stating

Every GameObject has a Transform

Source: https://docs.unity3d.com/Manual/class-Transform.html

Is there a specific reason it isn't covered as well? Could this be fixed / added?\

EDIT: Both Component.transform and GameObject.transform are affected. First one is accessed when just typing transform and second one when typing gameObject.transform explicitly.

citizenmatt commented 4 years ago

The docs from way back in 5.2 (GameObject and Component) state:

The Transform attached to this GameObject (null if there is none attached).

But that snippet has gone in 5.3 and above, and we get the "Every GameObject has a Transform" message above. I'll speak to some Unity folks and try and get a definitive answer.

citizenmatt commented 4 years ago

Good news and bad news.

The good news is that Unity have confirmed that transform is always not null - the 5.2 docs were in error.

The bad news is that the Transform type derives from UnityEngine.Object, and so inherits Object's custom equality operators, where comparing against null is actually a lifetime check on the underlying native object. In other words, even if we know that myObject is a valid, non-null C# instance, myObject == null can still return true.

Because of this, Rider/ReSharper will not show any null analysis warnings for types that implement custom equality. So adding [NotNull] to GameObject.transform makes no difference because we can't tell you that any if statements are unnecessary, even though we (and Rider/ReSharper) know that transform is not null, and we know that the underlying object is still alive (but which Rider/ReSharper has no knowledge about).

The only place where it is helpful is in the expression transform?.forward - Rider/ReSharper will tell you that the ?. is unnecessary because transform is never null. However, the Unity plugin will then tell you not to use ?. on UnityEngine.Object derived classes because it avoids the native object lifetime check!

Quite an interesting clash of features and I'm not sure what the way forward is. I'll speak to the devs responsible for null analysis and see if they have any ideas.