C#: Enable nullability for generated & compatibility code

[Repiteo]

Extracted from #83117 to focus exclusively on allowing generated/compatability code be null-aware. This trims out a lot of extra fluff from that PR, as it doesn't put any real focus on any internal system; this is strictly for the exposed layer of the API. As such, it focuses on these key points that are always true in the context of Godot:

• Value types are inherently non-nullable, naturally.
• GodotObject is inherently nullable at all times, by the nature of being derived from Godot's Object class.
  • There is exactly one exception: singletons. A singleton is guaranteed to not be null.
• All other reference types are inherently non-nullable at all times. Godot simply has no concept of a reference type that can be null outside of its own Object class.
  • This includes all arrays, which are always treated/returned as empty instead

Yes, Godot should ideally have a means of specifying if a given Object is guaranteed to be non-nullable, but that's not what this PR is about. What it is about is unambiguously representing the inherent nullability of Godot's classes.

[paulloz]

Finally came to look at this, I think it looks good. Would have to be rebased to be properly tested.

[raulsntos]

Thanks for the PR, and thanks @paulloz for taking a look. A couple comments:

GodotObject is inherently nullable at all times.

Adding ? for every GodotObject return when we don't know if the method can ever return null is fine but could be annoying for users that will now have to check for null potentially unnecessarily.

However, I'm more worried about adding ? for GodotObjects in parameters since we need to make sure that the Godot method does actually support receiving null Objects, otherwise the engine would crash if it's not checking (although I assume we do ERR_FAIL_NULL in all exposed methods).

That said, as I understand it, nullability in parameters is not about what's allowed but about intention. For example, if you have a method void MyMethod(GodotObject obj) where the parameter is not nullable, you should still check that obj is not null and throw an ArgumentException otherwise. But the parameter should not be nullable if the caller is not meant to pass null even though you check.

I feel like all these nullability annotations are only being added because we know the GodotObject can be null, but we don't know if it should. I think this is probably impossible to automatically determine if we don't have ClassDB telling us this information.

Here's some guidance for adding nullability annotations: https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/api-guidelines/nullability.md#annotation-guidance

A singleton is guaranteed to not be null.

I don't think this is true, I can imagine it can be null if it the requested singleton wasn't registered (like editor singletons in a game running outside the editor). Do we throw if the retrieved singleton is null? In that case it's fine to consider them always being non-null.

All other reference types are inherently non-nullable at all times.

I don't know about this, I would have to check.

[paulloz]

Adding ? for every GodotObject return when we don't know if the method can ever return null is fine but could be annoying for users that will now have to check for null potentially unnecessarily.

Arguably, this is implicitly how it currently works: we return reference types from a <Nullable>disable</Nullable> context.

[Repiteo]

That said, as I understand it, nullability in parameters is not about what's allowed but about intention.

For the most part, this is true, and for the most part this is also how the PR handles things. The sole exception is GodotObject and its derived classes, which cannot convey that information at this time. Therefore, in this lull period where we're lacking that information, I would still rather represent what the type itself represents, rather than being completely oblivious.

I think this is probably impossible to automatically determine if we don't have ClassDB telling us this information.

I agree, which is why this PR isn't even attempting to do so.

I can imagine it can be null if it the requested singleton wasn't registered (like editor singletons in a game running outside the editor).

Wouldn't editor singletons be part of GodotSharpEditor & not reachable in the first place in that scenario? There doesn't seem to be any real checks for a singleton not being registered in any case, so I was operating under the assumption that it simply cannot occur.

I don't know about this, I would have to check.

I guess more specifically, Object is the only "reference type" in regards to the exposed API. Only Object is passed by pointer in said API, while all other types are passed either by value or C++ reference (inherently non-nullable by design). This is why there were further adjustments to the non-Object reference types to ensure they will always return a valid value instead of null (Array.Empty, new(), etc).

I suppose, with that in mind, this PR could be adjusted to make anything GodotObject-related null-oblivious; but that would probably be too much of a headache. If we'd rather standby until there's something concrete to work with, there's always waiting on #86079 to come to fruition, which seems to address all of your concerns.

[raulsntos]

The sole exception is GodotObject and its derived classes, which cannot convey that information at this time. Therefore, in this lull period where we're lacking that information, I would still rather represent what the type itself represents, rather than being completely oblivious.

That's why I said it's fine for return types, but for parameters I think the safest option is to never allow null just in case the method doesn't accept it. Either way I feel like being oblivious may be better since it would communicate to the user that we don't know if it can be null or not, and I think that's better than saying we know but we're actually just guessing.

Wouldn't editor singletons be part of GodotSharpEditor & not reachable in the first place in that scenario?

Yes but that's only a difference we make in C#. What I was trying to say is there could be singletons in the engine that are never assigned under certain circumstances and therefore remain null. Whether this is something that happens in practice or not I don't know, but it seems like it's at least possible.

There doesn't seem to be any real checks for a singleton not being registered in any case, so I was operating under the assumption that it simply cannot occur.

Not having check simply means we were probably happily returning null before, since the API wasn't annotated everything can be implicitly null. In this case I think the intention was for singletons to not be null, so I think you made the right assumption. But maybe we should check and throw an exception otherwise, although this would break compat (but I'd expect not many users rely on this behavior).

This is why there were further adjustments to the non-Object reference types to ensure they will always return a valid value instead of null (Array.Empty, new(), etc).

I think Array.Empty makes sense, but I'm not sure about new(). If you are saying these reference types coming from the engine are never null, then we shouldn't need to instantiate them.

If we'd rather standby until there's something concrete to work with, there's always waiting on #86079 to come to fruition, which seems to address all of your concerns.

I haven't looked in depth at that PR but it looks like it adds the nullability information to ClassDB so I think that'd be perfect. It may be better to way until then to add nullability annotations to generated code, I'm a bit wary of adding nullability annotations that we'd have to change later since that could be considered breaking compat (although a bit minor probably).