Skip to content

Conversation

@dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Dec 12, 2023

Implements proposal godotengine/godot-proposals#2241

The core problem that this is solving is that in some languages, different language constructs/features are used with nullable and non-nullable values, where the non-nullable ones are usually friendlier for the developer to use. This has come up numerous times specifically in the context of Rust, Swift and C#, but would also apply to Kotlin and probably others.

The goal of this PR is to mark various arguments and return values in extension_api.json as "required", which means:

  1. Passing null for a required parameter is an error, which will prevent the method from doing its intended function, and so the GDExtension binding can attempt to prevent passing null into that argument using language features. (But this isn't required.)
  2. Returning null for a required return value is an error, and the GDExtension language binding can use that as a signal to do whatever error behavior makes the most sense for that language, for example, throwing an exception (ex in Python, C#, etc) or panicking (ex Rust). (But this isn't required.)

How this is accomplished:

  • Instead of using Object * or const Ref<T> & in method arguments, you can use RequiredParam<T>, which will get the parameter marked as "required" in ClassDB.
  • Some new error macros are added, like EXTRACT_REQUIRED_PARAM_OR_FAIL(m_name, m_param) which acts just like ERR_FAILED_NULL(m_param), except it also assigns a local variable with the object, if it wasn't null. Using the macro is required in order to get the underlying value out of the parameter, which has two advantages:
    • It will help prevent contributors from using RequiredParam<T> when they don't really mean it, so (hopefully) these markers are less likely to become out-of-date
    • This will help contributors to remember to check for null (like we do currently do with the ERR_* macros) which is beneficial to engine development, even outside the context of GDExtension
  • On return values, instead of using RequiredParam<T> there is RequiredResult<T>. This separate object is used because return values are more of a "soft" requirement and don't need a special process in order to extract the value
  • In ClassDB, the meta for any of required parameters or return values will be METADATA_OBJECT_IS_REQUIRED, which will look like this in the extension_api.json:
    "arguments": [
      {
        "name": "node",
        "type": "Node",
        "meta": "required"
      }
    ]
    
  • Just to test, this PR has a 2nd commit which marks a bunch of parameters and return values as required
  • In the parameters, I've used a new naming convention of rp_* (rather than p_*) in order to minimize the diff when switching to RequiredParam<T>. This makes it so each change is just +/- 2 lines in the diff, and it's easy to review and see that there were no other functional changes
  • When compiling a release build, the goal is for RequiredParam<T> and RequiredResult<T> to be optimized away, and generate basically the same binary as using a plain Object * or Ref<T>.

UPDATE (2024-12-10): The original version of this had this whole auto-vivifying thing that was terrible, and was replaced with an excellent suggestion from @Bromeon in this comment.

UPDATE (2025-10-17): The semantics of parameters and return values turned out to be different enough that this now has two templates (RequiredParam<T> and RequiredResult<T> instead of the single RequiredPtr<T>).

@dsnopek dsnopek added this to the 4.x milestone Dec 12, 2023
@dsnopek dsnopek requested review from a team as code owners December 12, 2023 15:37
@dsnopek dsnopek marked this pull request as draft December 12, 2023 15:37
@AThousandShips
Copy link
Member

AThousandShips commented Dec 12, 2023

One option would be to have this requirement be strict and crash on failure to fulfil it, I feel this is the area where that'd be appropriate, as if it's not so strict an error print should be used

At least I think such an option for this decoration feels appropriate, if not for the complete case

@dsnopek
Copy link
Contributor Author

dsnopek commented Dec 12, 2023

And code that works with RequiredPtr<T> will still need to use the error macros like ERR_FAIL_NULL(), because those will be able to make an error message with the file and line number of where the problem is. The error message when creating a RequiredPtr<T> has no idea what called it, again, because we don't have exceptions so we can't unwind the stack.

Actually, I think I may be wrong about this part!

With gcc/clang there are the backtrace() and backtrace_symbols() functions that may be able to get this information even when compiled with -fno-exceptions. We'd probably need to do this in a couple of different ways for different compilers and platforms, but it does seem to be possible.

EDIT: Heh, and we're even using them in Godot for the crash handler already. :-)

@Bromeon
Copy link
Contributor

Bromeon commented Dec 12, 2023

Very cool, and that was fast! 😎


Presently, it's marked by using METADATA_OBJECT_IS_REQUIRED in the same metadata field that holds things like METADATA_INT_IS_UINT32, which means it'll end up in the extension_api.json like this, for example:

"arguments": [
  {
    "name": "node",
    "type": "Node",
    "meta": "required"
  }
]

Could these metadata flags potentially overlap (maybe not now with the ints, but as we add more over time)?
If yes, how would they be represented in the JSON value for the meta key?


  • If nullptr is passed into a RequiredPtr<T>, then it'll automatically create a new instance of the object in order to satisfy the "contract" (aka "auto-vivifying"), and print an error. I'm not sure this is the right thing to do. Since we don't have exceptions, we can't really do anything smarter at runtime. And code that works with RequiredPtr<T> will still need to use the error macros like ERR_FAIL_NULL(), because those will be able to make an error message with the file and line number of where the problem is. The error message when creating a RequiredPtr<T> has no idea what called it, again, because we don't have exceptions so we can't unwind the stack.

I'm probably missing something here, but if the expected usage is to always abort the function (i.e. print error + return null) after a "required" precondition fails, do we actually need to create a default instance?

Not too worried about performance in the error case, but some custom classes might have side effects in their constructors?

(Edit: since we're talking about the engine API here, there are probably no custom classes involved; side effects might still apply to some Godot classes though.)

@dsnopek
Copy link
Contributor Author

dsnopek commented Dec 12, 2023

@Bromeon:

Could these metadata flags potentially overlap (maybe not now with the ints, but as we add more over time)?
If yes, how would they be represented in the JSON value for the meta key?

I really have no idea what metadata could be added in the future. But like I said in the description, something more extensible like the the options you described in godotengine/godot-proposals#2550 would probably be better.

I'm probably missing something here, but if the expected usage is to always abort the function (i.e. print error + return null) after a "required" precondition fails, do we actually need to create a default instance?

Nope, you're not missing anything, we don't need to create a default instance. I added that because I was worried that engine contributors could mistakenly believe that RequiredPtr<T> somehow guarantees that the value is not null, and this would prevent crashes in that case. We could just trust that contributors will know they need to still check for null? Or perhaps (as @AThousandShips writes above) a crash is actually what we want is this case?

I'm really not sure what the right behavior is, so that's what I'm hoping to discuss here :-)

@AThousandShips
Copy link
Member

AThousandShips commented Dec 12, 2023

I'd say it depends on how we interpret the argument limitation, is it shouldn't or can't be null?

From the proposal:

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.

One use for this would be to remove checks, at least in non-debug builds, the annotation states that "the responsibility of ensuring this is non-null is on the caller" and no checks needs to be done on the callee side

In the same sense the check can be used before calling any extension methods or before calling anything from an extension, and exiting there, acting as a barrier

I'd prefer it to be a guarantee for the method implementer, a "I can safely skip doing any null checks in this method, someone else handles it for me", or "this can safely be optimized in some sense", akin to the LLVM nonnull, as an optimization hint

I'm also not familiar with what the other languages here do for this, so matching some norm there would make sense

@Bromeon
Copy link
Contributor

Bromeon commented Dec 12, 2023

I really have no idea what metadata could be added in the future. But like I said in the description, something more extensible like the the options you described in godotengine/godot-proposals#2550 would probably be better.

It's hard to plan ahead here. Most flexible would probably be a dictionary (JSON object), but it might also be overkill?

{
    // ...
    "metadata": {
        "exact_type": "float64",
        "angle": "radian",
        "nullable": true
    }
}

Also, we will need to keep the "meta" field around anyway, for backwards compatibility. If nullability and other meta types don't overlap, we could technically reuse that key. But are we sure that nullability isn't something we might at some point consider for non-object parameters, as well?


What I'm a bit worried is that (in a few years), this might devolve into:

{
    // ...
    "meta": "float64|angle=radian|nullable"
}

That is, we have a "protocol inside a protocol" -- which would also be breaking btw, since existing code likely checks for the entire value string and would not expect any separators.


In other words, we have multiple options:

  1. reuse the meta field and either
    • hope potential values will stay exclusive, even in the future
    • keep using it only for those values which are exclusive, add new fields like angle (just an example) for specific metadata
  2. keep using meta only for the existing meaning.
    add a new key nullable (or maybe rather required, since it's new) on the same level
  3. design something more future-proof like a metadata object or array, meaning
    • we have to provide both representations due to backwards compatibility
    • if we choose object instead of array, technically it's similar to today's meta key, but with 1 extra level of nesting

I tend to favor 1) or 2), but maybe you have more thoughts on this?

@Bromeon
Copy link
Contributor

Bromeon commented Dec 12, 2023

I'm also not familiar with what the other languages here do for this, so matching some norm there would make sense

There are many examples of languages that didn't have a concept of nullability/optionality and retrofitted it late. Some I know:

  1. Java: everything was nullable by default
    • @NonNull annotations to give stronger guarantees, @Nullable to confirm it's indeed nullable
    • Optional<T> which is a competing mechanism, more FP but semantically the same as the annotations
    • annoyingly, a simple T can still mean "possibly nullable or not, depending on API design"
  2. PHP 7.1 introduced ?Type syntax
    • The good thing is that Type was strictly non-nullable before, so it was an easy addition
  3. C++ with std::optional
    • This is probably the worst example... Because it came so late, everyone already had their own workarounds (null pointers, bool flags, hand-written optionals, unions, ...). Including the standard library.
    • Changing T to optional<T> is possible due to implicit conversions, but being C++, there's probably a ton of edge cases around type inference and name lookup where this means breaking code.
    • At least, C++ had a culture of "using values", so Java's billion-dollar-mistake doesn't really exist here. Being nullable is the exception rather than the rule.
  4. C# 8.0 introduced string? in addition to string to denote explicit nullability
    • Since existing code has used string in the sense "possibly null", this needs an opt-in, allowing compatibility
    • Not being a heavy C# user myself, this looks quite elegant from the outside. It allows modern APIs to become more expressive without breaking code.

@Bromeon
Copy link
Contributor

Bromeon commented Sep 3, 2024

An idea I mentioned during today's GDExtension meeting is the type-state pattern, meaning that multiple types are used to represent different states of an object.

Concretely, we could split RequiredPtr<T> into two parts (names TBD):

  • RequiredPtr<T> as the parameter type in the function signature
    • not yet verified if non-null
    • does not allow access to inner pointer/reference
  • VerifiedPtr<T> inside the function
    • converted from RequiredPtr<T> via new macro similar to ERR_FAIL_NULL
    • can only be brought into existence if that macro succeeds
    • allows access to pointer and is always valid to dereference (no UB, no more checks)

The core idea here is that you cannot obtain VerifiedPtr, without going through one of the macros that verifies the non-null property. The macro returns from the current function if the check fails, making subsequent code safe.

@dsnopek
Copy link
Contributor Author

dsnopek commented Dec 10, 2024

@Bromeon I finally got around to reworking this PR based on your suggestion of using the "type-state pattern" from several months ago

This requires using new error macros to unpack the underlying pointer, which I think would also lead to encouraging more consistent use of the error macros for pointer parameters, which I think is something @AThousandShips would like per her earlier comments.

Please let me know what you think!

Copy link
Contributor

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for this! ❤️

Added some first feedback.

It's also convenient that required meta is mutually exclusive with other metas, since it's an object and others are numbers 🙂

@dsnopek dsnopek force-pushed the required-args branch 2 times, most recently from 821baf1 to ae96d1b Compare February 19, 2025 18:46
@dsnopek dsnopek changed the title Add RequiredParam<T> and RequiredValue<T> to mark Object * arguments and return values as required Add RequiredParam<T> and RequiredResult<T> to mark Object * arguments and return values as required Oct 28, 2025
@dsnopek
Copy link
Contributor Author

dsnopek commented Oct 28, 2025

Per our discussion at today's GDExtension meeting, I've renamed RequiredValue to RequiredResult

Copy link
Contributor

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Re-tested 17028c6 with godot-rust, all our tests pass.

This looks good from my side!

@dsnopek dsnopek force-pushed the required-args branch 2 times, most recently from 659ef86 to b800215 Compare November 6, 2025 12:30
Copy link
Member

@Ivorforce Ivorforce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sure is high in C++ magic complexity 😄

After all your feedback and discussion, I agree it's the right way forward. And I think it's close to mergable too. Here's a small batch of feedback.

@dsnopek dsnopek force-pushed the required-args branch 3 times, most recently from ebcf91b to 6ba81e0 Compare November 22, 2025 00:02
Copy link
Contributor

@Repiteo Repiteo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that the structs themselves conditionally check for their internal value. As much as I wish we could do that, that would completely prevent the use of this struct for forward-declared types or declarations in functions declaring that very type. This is why the static assertions for the structs are generic

I'm really sorry I didn't check the altered PR earlier, this could've been caught much sooner

@Repiteo Repiteo dismissed their stale review November 22, 2025 00:30

On second thought, not actually a blocker. It will just limit how much these structs can be integrated in their current form

@dsnopek dsnopek force-pushed the required-args branch 4 times, most recently from b4130c1 to ef9dc0a Compare November 23, 2025 23:38
@dsnopek
Copy link
Contributor Author

dsnopek commented Nov 23, 2025

@Ivorforce All your previous feedback should be addressed at this point!

@dsnopek dsnopek requested a review from Ivorforce November 23, 2025 23:41
Copy link
Member

@Ivorforce Ivorforce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Besides the Godot gotcha this adds, I have no concerns (correctness, performance, ambiguity) over this PR. After our discussions at the GDExtension meetings, I agree we should move forward with this.

Great work!

@dsnopek dsnopek force-pushed the required-args branch 3 times, most recently from b02efc2 to 34f5f70 Compare November 24, 2025 17:22
@Repiteo Repiteo merged commit 369afc7 into godotengine:master Nov 24, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 24, 2025

Thanks!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

6 participants