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

Require AsObjectArg pass-by-ref, consistent with AsArg #947

Merged
merged 6 commits into from
Nov 10, 2024

Conversation

Bromeon
Copy link
Member

@Bromeon Bromeon commented Nov 9, 2024

A more conservative approach for initial v0.2, but more consistent with AsArg by-ref semantics and with the potential to expand in the future. Also cuts down on the number of monomorphizations without losing functionality.

After this PR, you can pass object arguments as follows:

Type Closest accepted type How to transform
Gd &Gd &arg
&Gd &Gd arg
&mut Gd &Gd &*arg
Option<Gd> Option<&Gd> arg.as_ref()
Option<&Gd> Option<&Gd> arg
Option<&mut Gd> Option<&Gd> arg.as_deref()
(null literal) Gd::null_arg()

We'll need to check the ergonomic impact and see how arguments are typically being passed in practice.
Below more detailed rationale on individual impls.


Remove impl AsObjectArg<T> for Gd<T> values

Possibly controversial, but the idea is to require pass-by-value for objects, in order to discourage accidental/unnecessary cloning. This is also consistent with AsArg by-refs, in particular impl AsArg<Gd<T>> when used in arrays and packed arrays.

The downside is that it needs an extra & in some places, and one can no longer use pass-by-value to "consume" objects, ending their lifetime with the call. What is also unclear is whether there are situations where we can benefit from moving "into" the engine (perf-wise). So this may need some more discussion.

One option is also to move toward using Copy for manually managed Gd<T> types, which would mean a) those could be passed by-value, and b) we would anyway lose the "consume" semantics.

Remove impl AsObjectArg<T> for &mut Gd<T> references

&mut obj args can only be passed once, since &mut T: !Copy. This means that passing it to a Godot API works once,
and then causes moved-out errors. This comes with a few issues:

  • It either needs the &* pattern or not, depending on the order of calls. Reordering breaks them.
  • It's inconsistent with AsArg, which is only implemented for &T in case of by-ref builtins.
  • Consuming &mut T should probably be allowed iff consuming T is allowed.

Note about impl AsObjectArg<T> for &Option<Gd<T>> (outer reference)

It's relatively common that Godot APIs return Option<Gd<T>> or pass this type in virtual functions. To avoid excessive as_ref() calls, we could directly support &Option<Gd> in addition to Option<&Gd>. However, this is currently not done as it hides nullability, especially in situations where a return type is directly propagated:

    api(create_obj().as_ref());  // without &Option impl
    api(&create_obj());          // with &Option impl

While the first is slightly longer, it looks different from a function create_obj() that returns Gd<T> and thus can never be null. In some scenarios, it's better to immediately ensure non-null (e.g. through unwrap()) instead of propagating nulls.

@Bromeon Bromeon added quality-of-life No new functionality, but improves ergonomics/internals c: core Core components labels Nov 9, 2024
@Bromeon Bromeon added this to the 0.2 milestone Nov 9, 2024
@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-947

The idea is to require pass-by-ref for objects, in order to discourage
accidental/unnecessary cloning. This is also consistent with AsArg
by-refs, in particular `impl AsArg<Gd<T>>` when used in arrays and
packed arrays.

The downside is that it needs an extra `&` in some places, and one can
no longer use pass-by-value to "consume" objects, ending their lifetime
with the call. What is also unclear is whether there are situations
where we can benefit from moving "into" the engine (perf-wise). So this
may need some more discussion.

One option is also to move toward using Copy for manually managed Gd<T>
types, which would mean a) those could be passed by-value, and b) we
would anyway lose the "consume" semantics.

This is a more conservative choice for now, with minor syntactic
drawbacks, but in line with AsArg<T> and the potential to expand in the
future.
`&mut obj` args can only be passed once, since &mut T: !Copy. This means
that passing it to a Godot API works once, and then causes moved-out
errors. This comes with a few issues:

* It either needs the `&*` pattern or not, depending on the order of
  calls. Reordering breaks them.

* It's inconsistent with `AsArg`, which is only implemented for `&T` in
  case of by-ref builtins.

* Consuming `&mut T` should probably be allowed iff consuming `T` is
  allowed.

That said, we'll need to check the ergonomic impact and see how
arguments are being passed in practice.
@Bromeon Bromeon force-pushed the qol/objects-by-ref branch 2 times, most recently from e724f5c to e8f72ff Compare November 9, 2024 19:28
It's relatively common that Godot APIs return `Option<Gd<T>>` or pass
this type in virtual functions. To avoid excessive `as_ref()` calls, we
**could** directly support `&Option<Gd>` in addition to `Option<&Gd>`.

However, this is currently not done as it hides nullability, especially
in situations where a return type is directly propagated:

    api(create_obj().as_ref())
    api(&create_obj())

While the first is slightly longer, it looks different from a function
create_obj() that returns Gd<T> and thus can never be null. It's also
quite idiomatic to use as_ref() for inner-option transforms in Rust.

For now, the code is still added but inactive.
@Bromeon Bromeon added this pull request to the merge queue Nov 10, 2024
Merged via the queue into master with commit de0841f Nov 10, 2024
15 checks passed
@Bromeon Bromeon deleted the qol/objects-by-ref branch November 10, 2024 09:21
@Bromeon Bromeon added feature Adds functionality to the library and removed quality-of-life No new functionality, but improves ergonomics/internals labels Nov 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: core Core components feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants