Use RequiredParam/RequiredResult in some high value places

[dsnopek]

Now that #86079 is merged, we've got to mark some more things as required!

Since we don't have too much time before the feature freeze, the goal of this PR is to try and mark the things that would provide the highest value to developers (and we'll get everything else later for Godot 4.7).

Which things are high value is guided by some crowd-sourced feedback that @Bromeon got from the Godot Rust community (see spreadsheet) and some notes from @migueldeicaza with regard to SwiftGodot.

I've focused on these areas:

• The classes high-up in the hierarchy (like Node, Control, CanvasItem, etc)
• Physics and Input (because they're used in most games)
• Tween (because it was called out by Rust developers)

Total number of things marked as required: 139 (tracking spreadsheet)

Marking this as DRAFT for now, because I'd still like to mark more stuff, but I'll take it out of DRAFT in 1-2 days.

This is also a great opportunity to test RequiredParam<T> and RequiredResult<T> in more contexts, and see if we missed anything in their design.

It's already turn up a couple of things:

• Some changes are required for them to work in GDVIRTUAL*(). One of the changes is something I've wanted to do for a long time (use PtrToArg<T>::encode() to encode the arguments - doing this with a cast has bothered me for so long)
• There is an "ambiguous overload for 'operator='" error for Ref<T> v = RequiredResult<T>() which I'm not sure how to fix - advice would be appreciated! I have a solution for this now. I've removed the operator Variant() from RequiredResult<T> and added an internal helper method for the handful of situations when we need to generically convert from RequiredResult<T> to Variant

[dsnopek]

I think this is a pretty good batch, so I'm taking this out of DRAFT

Given that the spreadsheet created by the Rust folks (which used a Python script to gather all possible parameters or return values that could be made required) has ~1700 entries, and not all will end up being marked required, I suspect there will only be something like ~500 things (certainly less than 1000) that we end up marking as required in the end, so getting ~150 is a decent start

That's something we can finish up for Godot 4.7

[Ivorforce]

Most of the changes look pretty trivial. In particular, the RequiredParam changes mostly verify themselves, due to the protections of the class :)

I'm a bit more reserved about the RequiredResult changes. There is no way of making absolutely certain that a function doesn't introduce a non-erroneous nullptr return path in the future. Having it marked as RequiredResult may be incompatible with projects that assume nullptr is an error.
I have no ideas to avoid this, but since nullptr does mean some kind of error for many getter functions maybe we don't need to address this either?

The only other caveat i have is that in its current state, RequiredParam is slightly slower than const Ref &, since it requires a copy of the Ref be made. This overhead is hopefully not relevant in any of the functions, but I can't say for sure.
(I have a plan to optimize this in the future, so the regression should hopefully be temporary anyway)

[Ivorforce]

Is making this public necessary?
When returning nullptr from RequiredResult, I think we should always use ERR_FAIL_* macros.
If we need to return without error (for example, the error was already posted), I would prefer a public factory function like the following, so it's obvious that it's an error:

static RequiredResult make_error_state() { return RequiredResult }

[dsnopek]

Yes, for virtual functions. The way they work is like (psuedo code):

Type ret;
virtual_function(&ret);

We'd have to do some big refactoring to avoid using the default constructor in the macros for calling virtual functions, but I don't think it's really worth doing, given that nullptr is a valid value for RequiredResult to indicate an error

[Ivorforce]

Could you provide an example for this pattern (in the changes)?

[dsnopek]

This would come up any time there's an EXBIND*R(RequiredResult<T>, ...) or GDVIRTUAL*R(RequiredResult<T>, ...).

It looks like the only ones in this PR are:

EXBIND0R(RequiredResult<PhysicsDirectSpaceState2D>, get_space_state)

... and:

EXBIND0R(RequiredResult<PhysicsDirectSpaceState3D>, get_space_state)

I suppose I could remove those from this PR, and we could try to figure it out later?

[dsnopek]

(However, @Bromeon did specifically call out these methods in a comment here, so I think they would qualify as "high value", and it would be great to include them in this PR, if possible.)

[Ivorforce]

Ah, I see the problem. This is a rather inconvenient 'incompatibility' between the two designs.
I would prefer if RequiredResult wasn't default constructible, but it's not a blocker for this PR. So I'm ok with the public change for the moment.

[dsnopek]

I think we could rework the macros to avoid this. I poked at it a bit, and these changes almost work:

Diff

diff --git a/core/extension/make_wrappers.py b/core/extension/make_wrappers.py
index 96936d8cf3b..eab2aae7db7 100644
--- a/core/extension/make_wrappers.py
+++ b/core/extension/make_wrappers.py
@@ -71,7 +71,7 @@ def generate_ex_version(argcount, const=False, returns=False):
         sproto += "R"
         s = s.replace("$RETTYPE", "m_ret, ")
         s = s.replace("$RETVAL", "m_ret")
-        s = s.replace("$RETPRE", "m_ret ret; ZeroInitializer<m_ret>::initialize(ret);\\\n")
+        s = s.replace("$RETPRE", "GDExtWrapperRet<m_ret>::Type ret; ZeroInitializer<m_ret>::initialize(ret);\\\n")
         s = s.replace("$RETPOST", "return ret;\\\n")
 
     else:
@@ -119,7 +119,21 @@ def generate_ex_version(argcount, const=False, returns=False):
 def run(target, source, env):
     max_versions = 12
 
-    txt = "#pragma once"
+    txt = """/* THIS FILE IS GENERATED DO NOT EDIT */
+#pragma once
+
+template <typename T>
+class RequiredResult;
+
+template <typename T>
+struct GDExtWrapperRet {
+    using Type = T;
+};
+template <typename U>
+struct GDExtWrapperRet<RequiredResult<U>> {
+    using Type = typename RequiredResult<U>::ptr_type;
+};
+"""
 
     for i in range(max_versions + 1):
         txt += "\n/* Extension Wrapper " + str(i) + " Arguments */\n"

But it would require a bunch more to get all the way there, namely that we'd need similar changes in make_virtuals.py.

[Ivorforce]

Thanks for having a look. Since making this public isn't a huge problem or anything, let's save this for a (potential) follow-up PR.

[Ivorforce]

Passing RequiredParam through is unusual. Especially, in this case we will run set_global_transform even if p_parent is nullptr and Node::reparent fails.

I suppose it's no different from before, but is it a case that we want to support? It should be possible to avoid this by deleting the RequiredParam copy constructor.

[dsnopek]

Yeah, these functions are a little weird, but my changes aren't really adding or removing from the pre-existing weirdness :-)

I suppose it's no different from before, but is it a case that we want to support? It should be possible to avoid this by deleting the RequiredParam copy constructor.

Speaking generally, though, I think we should support this. If a function is just a "frontend" to some internal function, and that internal function needs to check for null anyway, I think passing along the RequiredParam<T> and letting the internal function do the unwrapping is fine. I don't think we need to force RequiredParam<T> to be unwrapped in each function

[Bromeon]

I'm a bit more reserved about the RequiredResult changes. There is no way of making absolutely certain that a function doesn't introduce a non-erroneous nullptr return path in the future. Having it marked as RequiredResult may be incompatible with projects that assume nullptr is an error.

This is a great example of the strenghts of the new system, because it codifies a previously implicit assumption into the type system. Let me elaborate:

There is already a lot of code that relies on certain methods not returning null, simply because they never do in practice. If you now change this behavior by introducing a non-erroneous nullptr, you will break such code. There are two scenarios here:

1. The return type was always nullable, but users don't expect it due to existing semantics (and maybe even docs!). So they will blindly dereference the nullpointer, std::optional, Rust Option::unwrap() etc. Runtime error at best, UB at worst.

2. The return type was previously non-null, but now becomes nullable. A binding can react to this by changing generated type signature. This causes a compile error in the extension code, but that signals to the user "there is now a potential null I have to take care of".

I'm 100% in favor of option 2). In my opinion, enabling such type safety is one of the big wins of the RequiredPtr proposal. And bindings are free to not use the "required" information if they prefer not to.

It's probably important to clearly document what RequiredResult means, to avoid misunderstandings. See also my comment #86079 (comment). Maybe we should not tie any compatibility guarantees to it -- although I'd argue that introducing unexpected nullptr returns is bad practice, independent of RequiredPtr or not.

[dsnopek]

@Ivorforce Thanks for the review!

I'm a bit more reserved about the RequiredResult changes. There is no way of making absolutely certain that a function doesn't introduce a non-erroneous nullptr return path in the future.

Right, RequiredResult is a "soft" requirement that we aren't verifying on the C++ side - it's really only there to add the "required" metadata. And, in the case of return values, that metadata means that if the function returns null, that is an error. This was a big part of the reason to split RequiredResult and RequiredParam.

We could try to make RequiredResult into a harder requirement in the future? Although, I'm not sure how we do that, given that null is a valid return value in order to indicate an error

Having it marked as RequiredResult may be incompatible with projects that assume nullptr is an error.

If I'm understanding correctly, I don't think so?

For bindings that don't do anything with "required" this won't have any effect. For those that do, it will change the return value or signature of the method, forcing the developer to update their code and think about this. (But that's only if they update their bindings to be Godot 4.6+ compatible - staying on an older version will still work since nothing will change with regard to binary compatibility)

The only other caveat i have is that in its current state, RequiredParam is slightly slower than const Ref &, since it requires a copy of the Ref be made. This overhead is hopefully not relevant in any of the functions, but I can't say for sure.

I don't think we can really know until this is used in the wild. I think the beta process will allow us to do that, and we can always rollback some of these if they turn out to be problematic. We won't be merging any more additions of RequiredResult/RequiredParam after this PR for Godot 4.6; we will have the time during beta to focus on fixing any issues that may come up

[dsnopek]

@Bromeon:

It's probably important to clearly document what RequiredResult means, to avoid misunderstandings.

Yes, I feel like we've had some misunderstandings about this already. :-) In my latest push, I've written a short comment above each class definition which attempts to explain what using them signifies

[Ivorforce]

The return type was previously non-null, but now becomes nullable. A binding can react to this by changing generated type signature. This causes a compile error in the extension code, but that signals to the user "there is now a potential null I have to take care of".

Right, as long as the bindings also check that the returned value is not null (and otherwise panic), I guess this is the best possible solution. (if the bindings don't check, then extensions not re-compiled for newer versions will UB if a previously non-nullable return type returns null)

We could try to make RequiredResult into a harder requirement in the future? Although, I'm not sure how we do that, given that null is a valid return value in order to indicate an error

It should be possible by disallowing a return of nullptr from the function, and forcing use of e.g. ERR_* macros instead, or the RequiredResult::make_error_state I proposed above.
But it's not required for the PR, we could amend that in a follow-up.

[Ivorforce]

LGTM

Thank you Jan and Miguel for collecting important bindings to amend, and David for sifting through and implementing them!

[Bromeon]

Also just regenerated Rust with the latest commit, looks good! I'm confident that over time, we'll find more APIs. Thanks for all the great work 💪

[Repiteo]

Thanks!

[KoBeWi]

This introduces new error, which is likely what caused #113490

[dsnopek]

Ah, I guess that one isn't really required then. I can make a PR to revert this

[AThousandShips]

So do these, they didn't require non-null before, probably not critical but worth looking at

[dsnopek]

These do have a required check, but it happens later in InputMap::_find_event(). I suppose I could remove the check there because now we have a check for it earlier?

[AThousandShips]

I'd say doubled checks are unhelpful (though we could add an error message here to make it clearer, as moving the error back might help for clarity)