Add RequiredPtr<T> to mark Object * arguments and return values as required

[dsnopek]

This PR is a draft for discussion around proposal godotengine/godot-proposals#2241

What this PR does:

• Instead of using Object * in method arguments or return values, you can use const RequiredPtr<Object> &, which will get the parameter marked as "required" in ClassDB.
• 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"
    }
  ]
  

• Just to test, this also marks the first argument of Node::add_child() as required, although, that's probably not where we'd start with this.
• Some new error macros are added, like ERR_FAILED_REQUIRED_PTR(m_name, m_param) which acts just like ERR_FAILED_NULL(m_param), except it also assigns a local variable with the object pointer if it wasn't null. So, in Node::add_child(const RequiredPtr<Node> &p_child) it does ERR_FAILED_REQUIRED_PTR(child, p_child) which will assign a Node *child variable with the pointer that was inside of p_child.
• When compiling a release build, the goal is for RequiredPtr<T> to be optimized away, and generate basically the same code as using a plain Object *.

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.

[AThousandShips]

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]

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]

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]

@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]

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]

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]

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]

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]

@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!

[Bromeon]

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 🙂

[Repiteo]

A fantastic foundation for this paradigm, but one that still needs some refinement. You've already mentioned the lack of Ref<T> support, but there's still a few tricky edge-cases to be considered

[Repiteo]

While this is clever, it goes against the self-contained paradigm of the other macros. That is: this isn't fully wrapped, thanks to the first line needing to be out of scope. If the variable was declared beforehand than this could be fully wrapped, but that would probably end up bloating the code. I don't know if a better alternative exists, but it's something to keep in mind

[Repiteo]

Not a fan of this naming scheme. It does a great job of reducing code changes, but I think this sort of change should be disruptive. That, and it would mean making an internal variable use the p_* syntax, which we really shouldn't be encouraging.

[Bromeon]

As far as I understood, the idea here was to minimize the risk of causing any unintended changes. It's much easier to review just one line of code and have the confidence that nothing breaks.

Maybe the rp_ -> p_ change could still be done in follow-up PRs? These PRs would then become "pure" refactorings and would be completely isolated from semantic changes, which could make it easier to pinpoint the issue in case of regressions.

[Repiteo]

It'd be nice if there was a way to print an error on null assignment, as that would be much clearer to users/maintainers when an improper value is passed. That could be done with a wrapper macro of sorts for construction, but a more convenient option will be added in C++20 via source_location. Maybe that's outside the scope of this implementation, but I felt it worth mentioning all the same