Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add method hints for non-nullable return types and nullable arguments #2241

Open
Calinou opened this issue Feb 4, 2021 · 5 comments
Open

Comments

@Calinou
Copy link
Member

Calinou commented Feb 4, 2021

See also #162 and #2242.

Describe the project you are working on

The Godot editor 🙂

Describe the problem or limitation you are having in your project

Currently, GDNative bindings must assume that all non-primitive return types (such as Node) may be null. This is problematic for safety-oriented languages such as Rust and Swift as it leads to uglier and unsafe code.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

  • Add method hints to ClassDB to signal that a return value will never be null. This hint should only be added to methods that return Object-derived types, not primitive types such as int or Vector2 as these are non-nullable by design. If this hint is not present, the method is assumed to have a nullable return type (for Object-derived types only).
  • Add a way to hint specific method arguments as being nullable. In other words, it means that the method will behave correctly if the specified arguments are null. If this hint is not present, the argument is assumed to be non-nullable.

This information can also be displayed in the generated class reference to improve documentation.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

  • Non-null return type hinting could be implemented as a ClassDB method flag such as METHOD_FLAG_RETURN_NON_NULL.
  • Nullable argument hinting could be implemented using argument metadata feature (GodotTypeInfo::Metadata).

If this enhancement will not be used often, can it be worked around with a few lines of script?

No, as this is core engine functionality.

Is there a reason why this should be core and not an add-on in the asset library?

This functionality is part of the engine itself.

@migueldeicaza
Copy link

Couple of observations:

  • These annotations could be used to improve C#, Rust and Swift bindings - as they could all make the parameters explicitly nullable or not-nullable.

  • The annotations could help both the arguments, as well as the return values. In languages like Rust and Swift, nil return values must be tested for, you you end up writing code like:

if let window = object.getWindow {
    // window is guaranteed to not be nil
   window.title = "Hello"
} else {
    /// window was nil
}

So it can become quite cumbersome in scenarios where we would always know that no nil can ever be produced.

To achieve this, we would need to do an audit of every API entry point, to annotate properly which arguments can be NULL, and which methods could return NULL. In the argument case, the good news is that there is extensive use of macros that indicate that this is an error condition (like ERR_FAIL_NULL, so at least part of the work has been done).

@CedNaru
Copy link

CedNaru commented Feb 8, 2021

I am all for that addition.
My group and I are working on a Kotlin Script implementation for Godot and the lack of nullable information is a struggle so far because we have to assume that any Object parameter/return type can be null for now. As nullable/non nullable types are part of the language, it makes the users add extra null check code when it's not necessary.

migueldeicaza added a commit to migueldeicaza/SwiftGodot that referenced this issue May 25, 2023
Godot sadly does not annotate their functions and properties to
include information whether they are optional or not.  This is an
issue that has been raised a long time ago and has gained little to no
traction:

godotengine/godot-proposals#2241

This poses a problem, because certain Godot APIs would return nil, and
I would try to create an object out of it, and crash, while a nil
return is ok.

It also posed a problem for APIs that could take a nil parameter, but
instead, in SwiftGodot, you had to create an instance of the object,
even if it is unnecesary or not the desired outcome.

So I have now settled on an evolutionary plan:

* For now, all reference types that could have been nil either as a
  parameter or a return value, are declared as swift optionals.

* I have introduced a hardcoded list of method return types and method
  arguments that can be used to flag exceptions to this: scenarios
  where we can verify that the return value would never be nil, or the
  parameter demands a non-nil argument.

The list currently is hardcoded in Swift, but will eventually move to
a Json file to make it easier to work with.

Sadly, this means that there will be some churn, until all this is
properly validated.

This is a fix for this issue:

#63

Some additional data:

* Some 114 property returns were affected
* Some 432 method returns were affected
migueldeicaza added a commit to migueldeicaza/SwiftGodot that referenced this issue Sep 21, 2023
Godot sadly does not annotate their functions and properties to
include information whether they are optional or not.  This is an
issue that has been raised a long time ago and has gained little to no
traction:

godotengine/godot-proposals#2241

This poses a problem, because certain Godot APIs would return nil, and
I would try to create an object out of it, and crash, while a nil
return is ok.

It also posed a problem for APIs that could take a nil parameter, but
instead, in SwiftGodot, you had to create an instance of the object,
even if it is unnecesary or not the desired outcome.

So I have now settled on an evolutionary plan:

* For now, all reference types that could have been nil either as a
  parameter or a return value, are declared as swift optionals.

* I have introduced a hardcoded list of method return types and method
  arguments that can be used to flag exceptions to this: scenarios
  where we can verify that the return value would never be nil, or the
  parameter demands a non-nil argument.

The list currently is hardcoded in Swift, but will eventually move to
a Json file to make it easier to work with.

Sadly, this means that there will be some churn, until all this is
properly validated.

This is a fix for this issue:

#63

Some additional data:

* Some 114 property returns were affected
* Some 432 method returns were affected
@dsnopek
Copy link

dsnopek commented Nov 30, 2023

Copying some discussion from the #gdextension channel on RocketChat:

@akien-mga wrote:

And for this I see two possible options:

  • Implement Optional<T> in C++, and refactor the whole API to use it. Then it will be included automatically in ClassDB with compile time checks, like BitField. That will require quite some discussion and work, probably spanning multiple release cycles -- if it can be done without breaking compat and forcing us to release 5.0.
  • Add it manually as a piece of metadata in _bind_methods for the methods which have nullable parameters or returns. That doesn't require changing the core API so it's easier to get done within a reasonable timespan, but that's going to be very tedious and error prone. Can also be done the other way around with some way to flag parameters/returns which cannot be null, which might be less common than stuff which may be null.

And I wrote:

I worry that if this is just metadata, and isn't supported by GDScript or C++, that folks would forget to set it when necessary, or set it incorrectly, since most aren't actively using GDExtension. Something more like the Optional<T> suggestion would be better, since you'd need to interact with it right away in C++. But, yeah, that'd be a pretty big undertaking. But like Remi suggests for the 2nd point, maybe it'd be easier to add it iteratively via a Required<T> that represents a pointer/ref that cannot be null?

This would be good to discuss at a (or several) GDExtension meetings! I've added it to the agenda.

@dsnopek
Copy link

dsnopek commented Dec 1, 2023

We discussed this at the GDExtension meeting earlier (sorry, I forgot to comment during the meeting) and came up with the following next steps:

  • The GDScript team is working on a "unified type system" proposal that will likely address nullability in some way. So, we'll wait until that's posted, and see if we can add our needs to that proposal. It would be nice to address the type system holistically, rather than just trying to squeeze in this bit we need on its own.
  • I'm going to make a PR with an experimental implementation of Required<T> so that we can look at the specifics and discuss. There's a couple of tricky questions with multiple answers that have different pro's/con's.

@Repiteo
Copy link

Repiteo commented Dec 1, 2023

From everything I've read so far, there seems to be two primary routes. One is utilizing both Optional<T> and Required<T>, the former for value types & the latter for reference types. The second is utilizing only a single wrapper, one that's able to handle value/reference types simultaneously. Both methods are guaranteed to require a significant API sweep to some degree, which would be an inevitability with this upcoming "unified type system", so I wouldn't want to put too much weight in deciding a system based on how easy/hard it might be

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants