Allow omitting 'owner' in general

[bluenote10]

This is something I have been wondering about for a while, and since it was also mentioned in #848, but only in the context of construction, I wanted to open a more general discussion.

It's a common complaint that the signature of the mandatory new is too verbose, the owner argument being seldom used.

Being forced to consume an owner argument is often awkward for methods in general. I often have classes that I want to use from both GDScript and Rust side. These classes may have simple functions, like e.g. a unary start() or stop() (think of for instance an audio sequencer). For many of such functions it is totally unnecessary to consumer the owning node in the method. As long as the native class is called only from GScript this isn't much of an issue. But when using this class from Rust as well, the method signature feels wrong: Calling start(some_node) and stop(some_node) requires passing around unnecessary data, and in some contexts the owning node is simply not available, and there is no good way to call these methods at all. Then I started to implement methods in pairs, one with the owner exported to GDScript and one without the owner for usage from within Rust, where the first delegates to the latter. This is also awkward, because of the boilerplate, and because of using two function names: My functions in Rust are now for instance often called start_impl() or stop_impl(). (I just noticed that with the export rename feature, I might actually by able to call the exported functions something like start_exported(), rename it towards GDScript to "start", and call the underlying implementation start() -- in any case, quite some extra work.) Therefore, it would be great if consuming the owning node would be optional in general.

I'm not very familiar with Rust macros, but in Nim it would be quite straightforward to actually figure out whether an implemented method actually wants to consume an owner argument, and generate the wrapper call accordingly. The rule would be something like: Given the AST of the method, if the first argument after self has the appropriate type to consume the owner, the owner will be passed in as second argument. Otherwise the owner is discard in the wrapper. This would allow using an owner argument only when actually necessary. Is something like that not possible in Rust?

[Bromeon]

I agree on this, the owner is often unnecessary, and it needing to be part of the signature has mostly technical reasons. We need to investigate what would be an ergonomic way to avoid it -- maybe even default to "no-owner" and only add it on demand.

Apart from that, I think "owner" is an unfortunate name -- "base" is better. After all, owner already has a different meaning in the scene tree, and what we are really talking about is the (inherited) base class with a built-in Godot type.

---

But when using this class from Rust as well, the method signature feels wrong: Calling start(some_node) and stop(some_node) requires passing around unnecessary data, and in some contexts the owning node is simply not available, and there is no good way to call these methods at all.

The way how exported methods are currently implemented, is that they are not really intended to be called from Rust. Primarily, they serve as an interface to GDScript code. But I see that the possibility of calling them within Rust makes it appealing.

---

The rule would be something like: Given the AST of the method, if the first argument after self has the appropriate type to consume the owner, the owner will be passed in as second argument. Otherwise the owner is discard in the wrapper. This would allow using an owner argument only when actually necessary. Is something like that not possible in Rust?

A lot of things are possible with Rust macros, but it's also a question of "how much magic do we want in our APIs".

For example,

#[export]
fn collides_with(self, owner: &Node, other: &Node) -> bool { ... }

Pretty straightforward to call from GDScript:

if a.collides_with(b):
   do_something()

Now, we apply magic to omit the owner:

#[export]
fn collides_with(self, other: &Node) -> bool { ... }

How do we know that other is not the owner? Having different semantics based on parameter name opens doors for all kinds of havoc, and such surprises can be very user-hostile. Those things don't happen because you accidentally write "other" when you mean "owner", but during large-scale refactorings, and can be absolute nightmares to debug. A statically typed language focused on safety should try to minimize such hazards.

But I'm optimistic there are ways to make it explicit without being overly verbose, especially if we find "the right defaults" (e.g. most methods do or do not need an owner). Something along the lines of:

#[export(with_base)]
fn collides_with(self, base: &Node, other: &Node) -> bool { ... }

#[export]
fn collides_with(self, other: &Node) -> bool { ... }

[chitoyuu]

But I'm optimistic there are ways to make it explicit without being overly verbose, especially if we find "the right defaults" (e.g. most methods do or do not need an owner).

I would suppose that this would largely be dependent on the architecture/style of the project. We are likely to see a lot of uses of the owner argument in script-style code, and rarely any in API-style code. It comes down to which one we want to make more convenient by default.

Something along the lines of:

#[export(with_base)]
fn collides_with(self, base: &Node, other: &Node) -> bool { ... }

#[export]
fn collides_with(self, other: &Node) -> bool { ... }

Another possible syntax might be:

#[export]
fn collides_with(self, #[base] b: &Node, other: &Node) -> bool { ... }

...where the presence (and position) of the owner/base argument is determined by an attribute. Owner-less would implicitly become the default, of course.

[Bromeon]

Another possible syntax might be:

#[export]
fn collides_with(self, #[base] b: &Node, other: &Node) -> bool { ... }

...where the presence (and position) of the owner/base argument is determined by an attribute. Owner-less would implicitly become the default, of course.

That's very nice. I haven't really encountered attributes in parameter position in Rust, but all the time in other languages (e.g. for REST clients). A small extra advantage would be that the user could choose which parameter acts as the base.

[bluenote10]

I like all the proposals a lot -- they'd all solve my use case just fine.

I also don't have a strong opinion on what should be the default. For me personally ~95% of the methods don't need a base/owner. The one method that almost always uses it is _ready though. Typically I do cache static tree reference there in the spirit of an onready var, which is why most other methods don't need tree access anymore. On the other hand, keeping the default as it is would mean no breaking change, and easier discoverability of the feature for newcomers -- opting out when desired would work perfectly fine as well.

[B-head]

My suggestion.

Use the base keyword, like the self keyword.
base is the argument of GodotObject and its type is TRef<InheritedType>.

For example:

#[inter] // Renamed from "export".
fn collides_with(&self, base, other: &Node) -> bool { ... }

omitted:

#[inter]
fn collides_with(&self, other: &Node) -> bool { ... }

Change the name of the attribute from "export" to "inter" because I dislike that name.
It is used in GDScript in the different sense.
(and compatibility is maintained)

I will implement this now for personal use. Will open a PR later.

[Bromeon]

@B-head We discussed this in #633, the conclusion was that it's "too much magic".

Generally, we would like to keep the syntax as natural as possible -- first, there's fewer special cases that humans have to memorize; second, it tends to confuse IDEs less. In other words, the method now needs the macro or its syntax is invalid. While at the moment, it's just a Rust method with attributes for exporting.

#[export] naming can be discussed, but if at all, then I'd rather opt for something like #[method] (mirroring #[property]), and definitely not before v0.11. #[inter] doesn't seem to relate to any term in this domain (even assuming it extends to "interface" or similar).

Nevertheless thanks for the suggestions! Maybe a note regarding PRs: to avoid extra work on your side, feel free to first discuss such designs in a GitHub issue or on Discord 🙂

[B-head]

@Bromeon

We discussed this in #633, the conclusion was that it's "too much magic".

Thank you for the information.
To follow the discussion at that time, here is the link.

Reconsideration of the discussion at that time

Confusion in IDE

My only fear is that IDEs will have a harder time and it looks more alien since _ is generally not allowed.

True. It did confuse rust-analyzer when I tried it.

I used the code in #633 to try to see if works well with the current rust-analyzer.
Currently there seems to be only slight confusion.

Type inference

There is an argument that the code is not clear because the type of owner (or base) is inferred.
First, clarify what is problem with type inference.

For example:

fn foo(bar) {
    bar.do_something();
}

The type of bar is inferred. The type of bar depends on how foo is called.

Problem with this code.
The lack of any information about do_something().
And the user of this function needs to know the internal implementation to pass the proper type to bar.
Such code would easily make bugs.

With this in mind, consider the following example:

#[export]
fn foo(&self, base) {
    self.do_something(base.do_something());
}

The type of self and base is inferred. The type of self is &Self. The type of base is TRef<'_, <Self as NativeClass>::Base>.
Unlike the previous example, the type being inferred is definitive.
We can see what the two do_something() are.
And the user of this function can easily pass the proper types to self and base.
This would not make bugs by type inference.

Reply to comment

#[export] naming can be discussed, but if at all, then I'd rather opt for something like #[method] (mirroring #[property])

#[method] is my first choice.
Still, the reason I went with something else is that #[methods] already exists.
If there is no confusion with #[methods], I too would prefer #[method].

Maybe a note regarding PRs: to avoid extra work on your side, feel free to first discuss such designs in a GitHub issue or on Discord 🙂

It's okay. I have not written any code yet 🙄

[Bromeon]

Type inference is not the issue, I'm aware that it would not be ambiguous.
My argument against implicit owner parameter was this:

Generally, we would like to keep the syntax as natural as possible -- first, there's fewer special cases that humans have to memorize; second, it tends to confuse IDEs less. In other words, the method now needs the macro or its syntax is invalid. While at the moment, it's just a Rust method with attributes for exporting.

I'm not convinced that turning godot-rust into a domain-specific language (DSL) is the way to go, when it's possible to achieve the same with near-zero effort in standard Rust. Procedural macros always come at a cost (complexity, debuggability, compile time), and I see their main strength in avoiding boilerplate and repetition. Adding &Node or TRef<Node> is not something that strikes me as overly verbose or repetitive; on the contrary, it can help make code clearer.

Also, we need to keep GDExtensions in mind, where some things may work differently. E.g. one idea was to store the owner in the native class itself, not pass it in via method parameter. So I'd like to avoid that we spend a large amount of time of cosmetics now, which may soon become obsolete.

[bluenote10]

I'm not convinced that turning godot-rust into a domain-specific language (DSL) is the way to go, when it's possible to achieve the same with near-zero effort in standard Rust.

I fully support this. There are quite some ongoing discussions on how IDEs should handle compilation errors in proc macros. It is not only an rust-analyzer issue (iirc Clion sidesteps the issue via some heuristics), but I think this issue summarizes the challenges pretty well: rust-lang/rust-analyzer#11014

I've only skimmed these discussions and have limited knowledge of proc macros. But my conclusion was that library authors are better off by keeping it simple. If I understand it correctly, proc macros should try to fall back to valid Rust code if possible, so that IDEs have a chance to capture the original semantics of the underlying Rust code. I think letting a failed macro expansion result in invalid Rust code is calling for trouble.

Side note: I was even wondering if it would make sense for godot-rust to make #[export] a valid stand-alone macro, so that we no longer need to have a proc macro on the full impl scope (in which the #[export] attributes aren't valid Rust code, leading to issue described in godot-rust/book#57) -- but that is a different story and unclear if it would be feasible.

[B-head]

Apparently, no one (including me) needs the feature to omission of type. It can be supported as an optional syntax, but should not be included in the PR.

And I assume that the "the right defaults" is the omission of owner argument, since no one needs the omission of type.

[Bromeon]

And I assume that the "the right defaults" is the omission of owner argument, since no one needs the omission of type.

Yes, we would then have something like in chitoyuu's example:

// No owner/base parameter
#[export]
fn collides_with(self, other: &Node) -> bool { ... }

// Owner/base parameter
#[export]
fn collides_with(self, #[base] b: &Node, other: &Node) -> bool { ... }

This is also why I marked it as breaking change and we'll need to schedule it for v0.11 (I don't want last-minute features of this magnitude in v0.10, given that we should also change the terminology surrounding owner -> base).