Add NotNull/CanBeNull annotations for known API methods

[citizenmatt]

E.g. GetComponent will return null if the component isn't found. Adding [CanBeNull] will show warnings to check for null before use.

This will require trawling through the docs to see what is at least documented as returning null.

[Discipol]

I wholeheartedly agree! Even partial coverage will vastly improve the code with simple checks.

[Razenpok]

Hey @citizenmatt, should nullness annotations be grouped under <!-- Nullness --> region or should they be each under a region corresponding to a class? I'm afraid that this nullness region can get super bloated.

https://github.com/Razenpok/resharper-unity/blob/net202/resharper/resharper-unity/src/annotations/UnityEngine.xml#L3

[citizenmatt]

Given the amount of annotations we have right now, it doesn't really matter. Do you have anything planned? If you're thinking of adding a couple, then it will be fine to add them here. If doing this on a larger scale, e.g. via automation, then I would likely create a new file such as UnityEngine.nullness.xml to contain just nullness annotations, and it would make sense to have that ordered by fully qualified class name.

I did some work a little while back to generate null annotations based on ReSharper's data flow, but it hit a bit of a wall when it got to the native APIs. There's no way of knowing if they can accept or return null, so I'm not sure on confidence levels of the results. We also need to be careful about UnityEngine.Object's custom equality operator - ReSharper's data flow can't reason about this, and disables a number of inspections, so creating annotations for methods/properties that are Unity objects might not be effective. GameObject.transform is a good example. We might (correctly) mark it as [NotNull], but go.transform == null could still be true, thanks to the custom equality operator (see this comment for more details). I think this needs revisiting in 203.

[Razenpok]

I planned on adding at least a couple of most useful annotations - like [CanBeNull] on GetComponent to warn about possible NREs. No automation, pure manual labor motivated by frustration.

Regarding [NotNull] with UnityEngine.Objects - the only time when we can guarantee a non-nullability is when we access it from the "place of origin". For example, possible outcomes of accessing go.transform:

• go is null. We get an NRE by dereferencing null go.
• go is not null. We get a non-null go.transform since the engine guarantees its non-nullability.
• We store go.transform somewhere (field, variable, etc.). We get a can-be-null scenario since we cannot guarantee that go wasn't destroyed before we access this stored transform the next time (when it can be null).

With this kind of "place of origin" detection we could provide an adequate check for non-nullability.

But, even without that, I believe that [CanBeNull]s could be of the most usefulness. Are there any pitfalls regarding this attribute as well or is it all-good? And what are the pitfalls of other annotations?

[Razenpok]

By the way, is UnityEngine.nullness.xml a valid name for an annotations file? According to documentation, it should be [Assembly.Name].xml

https://www.jetbrains.com/help/resharper/Code_Analysis__External_Annotations.html#creating

[citizenmatt]

Good question. Yes, it's valid, but only inside a directory named after the assembly. So UnityEngine.xml is valid, and so is UnityEngine/AnythingAtAll.xml.

[Razenpok]

Ok, cool. Does it make sense to split annotations into files corresponding to each class?

- UnityEngine.CoreModule
|- UnityEngine.Object.xml
|- UnityEngine.Component.xml
|- UnityEngine.GameObject.xml

- UnityEditor
|- UnityEditor.Editor.xml

[citizenmatt]

The main pitfalls with [CanBeNull] and [NotNull] are really the interaction with the custom equality operator. It's worth creating some non-Unity test code with two objects, one that implements custom equality like UnityEngine.Object and one that doesn't, and then create empty, mock GetComponent like methods that you can easily add the attributes to and see how they work.

The class without the equality operator will show what Rider/ReSharper's analysis would normally do - if the return value is [NotNull], is a resulting if (component == null) method highlighted as redundant? If it's [CanBeNull], will component.Foo() be flagged as possible NRE. Basically, create a bunch of example usage to ensure that it's going to deliver useful inspections.

The other pitfall is annotating a method that can definitely return null, but in normal usage won't. So GetComponent can return null, but if it's called correctly for the current game, it won't. Adding [CanBeNull] could introduce a lot of noise for potential NREs when I know that I won't get any. (For example, in the IntelliJ codebase, we have a GetService method that under very specific circumstances can return null, but will be not-null for all other usages. So it is annotated as "not null", even though that's not strictly true). So the main concern is that annotations will make the inspections too noisy, and devalue the warning.

[citizenmatt]

Ok, cool. Does it make sense to split annotations into files corresponding to each class?

- UnityEngine.CoreModule
|- UnityEngine.Object.xml
|- UnityEngine.Component.xml
|- UnityEngine.GameObject.xml

- UnityEditor
|- UnityEditor.Editor.xml

I don't think so. We don't have that many annotations, so it's best to keep them in a single file per assembly until they get too big. We also need to remember that we have both UnityEngine.xml and UnityEngine.CoreModule.xml, and other modules, and the same for UnityEditor.xml. Splitting per class would end up with a lot more files.

[Razenpok]

So GetComponent can return null, but if it's called correctly for the current game, it won't.

Well, yes, if GetComponent is called correctly for the current game, it will return not-null. And "correctly" here means the object has a corresponding component on it. This can be enforced via Unitys' [RequireComponent] attribute.

So let's say we add [CanBeNull] to GetComponent - is there a way to tell resharper-unity to ignore it for classes which have [RequireComponent] for the queried component?

[RequireComponent(typeof(Rigidbody))]
public class Foo : MonoBehavior
{
    private void Awake()
    {
        var boxCollider = GetComponent<BoxCollider>();
         // Will warn about null
        boxCollider.enabled = false;

        var rigidBody = GetComponent<Rigidbody>();
         // Will not warn about null
        rigidBody.enabled = false;
    }
}

public class Bar : MonoBehavior
{
    public Foo foo;

    private void Awake()
    {
        var boxCollider = foo.GetComponent<BoxCollider>();
         // Will warn about null
        boxCollider.enabled = false;

        var rigidBody = foo.GetComponent<Rigidbody>();
         // Will not warn about null
        rigidBody.enabled = false;
    }
}

[citizenmatt]

I'm not sure, tbh. We can provide annotations programmatically, for situations where we can't annotate via external annotations, such as a field marked with Unity's [Range] attribute, we can programmatically add ReSharper's [ValueRange], but I don't think we can apply that to a method call. That said, I haven't tried...