New syntax for #[export] methods and #[base] parameters

[B-head]

Resolve #850

Feature

Allows onwer argument to be omitted.

#[method]
fn foo(&self, a: i32, b: i32) -> i32 {
    a + b
}

By adding the #[base] attribute to the argument, it is defined as the owner argument. Only the second argument can be the owner argument. Also, there is no way to define the owner argument without #[base].

#[method]
fn foo(&self, #[base] base: TRef<&Node>, value: i32) {
    base.do_something(value);
}

All optional parameters that can be used with the #[export] attribute can also be used with #[method].

Compatibility

The old syntax #[export] attribute will continue to be supported. (It does not support omission of the owner argument.)

#[export]
fn foo(&self, base: TRef<&Node>, value: i32) {
    base.do_something(value);
}

The godot_wrap_method! macro is not compatible because its definition has changed. (Can be fixed for compatibility if needed)

Written tests. (B-head#1)

[Bromeon]

Thanks a lot for this pull request, and sorry for the late review!

If I am not missing something, this PR changes several things at once:

1. Add #[base] and redefine the "owner parameter" syntax
2. Introduce #[method] in addition to #[export]
3. Deprecate reference return
4. Something with Deref (is this somehow overlapping with Returns the dereferenced type in the exported method #870?)
5. Some other refactorings (macro simplification, rpc_mode: Option<RpcMode>)
6. Something else?

For the future, I would appreciate if we could keep a pull request limited to more or less one related functionality. This makes it easier to review, reduces the risk of bugs/untested functionality and typically also means less time until it can be merged.

For now (unless it's very simple to split by commit), I don't want you to spend huge effort on tearing those things apart, but maybe you could elaborate each of the changes a bit here? That would help tremendously to understand the motivation behind each change and the design choices in its implementation! 🙂

[Bromeon]

Regarding #[export] -> #[method] transition, that might need some more discussion + input from other contributors.

Some points to consider:

• #[method] is consistent with #[property]
• #[export] is more descriptive than #[method] ("method" has literally no information if it's annotated over a method)
• The concept of exporting in godot-rust currently encompasses this book chapter, or the c: export GitHub label
• In GDScript on the other hand, exporting is limited to variables (turning them into properties); methods can only be exported indirectly as custom getter/setter. So maybe our choice of terms is non-ideal already.

I'm open to ideas, it should just be concise (ideally 1 word). Could even be something like #[godot] 💡

Is the #[export] syntax in this current implementation fully backwards-compatible and not changing in semantics (i.e. no support for #[base], always a required 2nd base parameter)? We could deprecate it and ease the transition for users from 0.10 to 0.11, plus merge this before 0.11.

Also,
bors try

[bors]

try

Build succeeded:

• full-ci

[B-head]

#[export] is more descriptive than #[method] ("method" has literally no information if it's annotated over a method)

It is described as exporting as a method. This would be useful in the future to add features like the #[getter] macro that exports a "non-variable based getter" or other features.

Is the #[export] syntax in this current implementation fully backwards-compatible and not changing in semantics (i.e. no support for #[base], always a required 2nd base parameter)? We could deprecate it and ease the transition for users from 0.10 to 0.11, plus merge this before 0.11.

At least, it is backward compatible with the range of tests originally in the library.

The #[export] syntax always requires a second parameter, and the second parameter is the base parameter without #[base]. However, #[base] can be explicit. This is just an addition to the syntax and does not break compatibility.

Something with Deref (is this somehow overlapping with #870?)

This pull request was branched from the deref-return branch. Sorry if this is confusing. I'm not yet familiar with Git.

[Bromeon]

It is described as exporting as a method. This would be useful in the future to add features like the #[getter] macro that exports a "non-variable based getter" or other features.

I see. I think the whole topic would look different if people qualified proc macros and attributes:

#[gdnative::methods]
impl MyClass {
    #[gdnative::method]
    fn exposed_via_gdnative(&self, owner: &Node, i: i32) -> String {
        ...
    }

    pub fn pure_rust(&self) -> bool {
        ...
    }
}

But without that, it's not really clear:

#[methods]
impl MyClass {
    #[method]
    fn exposed_via_gdnative(&self, owner: &Node, i: i32) -> String {
        ...
    }

    pub fn pure_rust(&self) -> bool {
        ...
    }
}

Which is why I was thinking maybe something related to Godot (or at least the exporting) should be made part of the name. But I appreciate other input on that topic 🙂

---

At least, it is backward compatible with the range of tests originally in the library.

The #[export] syntax always requires a second parameter, and the second parameter is the base parameter without #[base]. However, #[base] can be explicit. This is just an addition to the syntax and does not break compatibility.

Do we actually want to support #[export] with #[base]? If it's deprecated, then I'd say we keep the semantics as-is, and encourage everyone to switch to the new syntax. I don't see much reason to further improve something we officially declare obsolete.

---

This pull request was branched from the deref-return branch. Sorry if this is confusing. I'm not yet familiar with Git.

No worries! Just for the future, it would probably be easier to keep unrelated improvements in separate PRs (even if they touch the same code, like here the export proc-macros) 🙂

But we can also keep this pull request bigger if you prefer -- in that case, could you edit your initial post here to add the other improvements (I'm not sure if my list fully captures them)? That way we have a complete overview of any changes.

[bors]

try

Build succeeded:

• full-ci

[B-head]

Do we actually want to support #[export] with #[base]?

No. I did this because it makes for a simpler code. Additional code would be needed to make this unsupported, would you do so?

could you edit your initial post here to add the other improvements (I'm not sure if #872 (review) fully captures them)? That way we have a complete overview of any changes.

I think it is better to have an description in the commit, so I wrote it in the commit message. If this is not clear, I will add the explanation to the first comment.

I checked all the changes. No changes were found that were not included in the list. See the commit message for a summary of the refactoring. (Mouse-over the commit link for a quick view)

No worries! Just for the future, it would probably be easier to keep unrelated improvements in separate PRs (even if they touch the same code, like here the export proc-macros) 🙂

But we can also keep this pull request bigger if you prefer

If I rebase the history now, I will make it more confusing. So I will go ahead and study it.

[Bromeon]

The commit history looks very nice now, thanks! 👍

No. I did this because it makes for a simpler code. Additional code would be needed to make this unsupported, would you do so?

Ah ok. I think we can live with the way it is now, maybe just don't advertise #[export] + #[base] as a feature. Then it's an undocumented thing which is anyway deprecated.

So the only topic left is the naming 😅
More opinions from others on that?

[bluenote10]

More opinions from others on that?

I haven't spend much thought on it, but on first glance I like #[method] in combination with #[methods] and the fact that it is a non-breaking change with a simple deprecation strategy.

[jacobsky]

Regarding some of the opinions on the naming, I would say that I'm not too picky on which direction, but I would prefer if whichever attribute that is being used is descriptive on the functionality.

Since the idea is that we are exporting or connecting methods/properties to godot, using the term #[export] makes a lot more sense than #[method], imo. It serves both code generation and -- assuming that the user is proficient in English -- makes it clear that the method is being exposed.

If it were feasible, I might actually be more inclined to go the otherway by changing #[property] into #[export] for consistency with Godot's terminology (since it's only possible to export properties in GDScript and the like).

Then again, since it's also possible to use the explicit module path with the proc macro, I really like the readability of #[gdnative::method] with how clear it makes that the attributed function is exposed via gdnative. In my own projects, I would probably call it this way instead.

I'm be happy with these changes.

[Bromeon]

Everyone: more input on #[export] vs. #[method] vs. #[godot_api]?

If there's no progress in the naming, I might merge this soon and we handle the naming discussion separately (with a potentially breaking change on master, but not in released versions).

This would allow us to integrate the proc-macro changes already, and make room free for other improvements on that codebase.

[Bromeon]

This PR looks fairly good! 🙂

I added some commits:

• Renamed #[methods] to #[godot]
• Compile-time warning about using deprecated #[export] syntax.
  • We still need to update all the occurrences in godot-rust itself...
• Updated documentation, especially in NativeClass derive

Feel free to merge/rebase with your own commits as you wish, I don't insist on my commits (or their authorship) to be retained as-is. Just kept it as 3 commits so it's easier to see independent changes, we can also squash them.

Note that #[godot] is not a final choice, but I'd like to move on with this PR, we can still rename this in isolation later.
Why this naming?

• It's more expressive than #[method] and makes clear the annotation is related to Godot.
• Some libraries tend to unify attribute names, even if they appear in different positions. #[serde] is a famous example.
  We currently have quite some variety with #[no_constructor], #[inherit] etc. See also #848.
• We can't keep #[export] for backward compatibility reasons (that is, unless we simultaneously rename #[methods], but that's out of scope here).

Let me know what you think! Would be nice to merge this soon 🚀

[B-head]

Thanks for the great documentation! 😄

I am a machine translated english as you can see, so it would be very helpful for me to write the document instead.

Renamed #[methods] to #[godot]

Oh my god! 😱

However, it is unlikely that we will reach a consensus if we continue to discuss this, so this will do for now. Once #350 is implemented, we can truly write #[gdnative::method], and we can start another religious war at that time. 🔥

Finally, I have committed some fixes, so take a look.

[Bromeon]

Thanks a lot for all this great effort! 👍

Oh my god! 😱 [...] and we can start another religious war at that time. 🔥

Haha 😅
As mentioned, I wouldn't say this is final, we can still change it until the next release (likely 0.10.1). Also, this PR contains several other valuable changes which are subject to code rot and merge conflict, if they stay as is.

But to get more feedback, we need people to try the new syntax, and this won't happen in an unmerged PR. The new #[base] attribute might also spark some controversies, maybe people would rather write something like:

#[godot(base)]
fn my_method(&self, base: &Node);

So I'm definitely open for further discussion on this matter!

bors r+

[bors]

Build succeeded:

• full-ci