Improve AsArg<T> compile error messages by using marker traits

[Yarwin]

What this PR does solve?

Diagnostic on AsArg<T> was ultra-confusing because of associated item type mismatch. Given snippet such as:

        let my_stringname = StringName::from("Some StringName!");
        let my_gstring = GString::from("heyoooo");
        let my_nodepath = NodePath::from("root/mynode");

        // AsArg<StringName>
        self.base_mut().set_name(my_stringname);

        // AsArg<GString>
        self.base().find_child(my_gstring);

        // AsArg<NodePath>
        self.base().has_node_and_resource(my_nodepath);

One was getting following compile errors:

Compile errors!

error[E0271]: type mismatch resolving `<StringName as ToGodot>::Pass == ByValue`
   --> src/lib.rs:71:34
    |
 71 |         self.base_mut().set_name(my_stringname);
    |                         -------- ^^^^^^^^^^^^^ expected `ByValue`, found `ByRef`
    |                         |
    |                         required by a bound introduced by this call
    |
    = note: required for `godot::prelude::StringName` to implement `AsArg<godot::prelude::StringName>`
note: required by a bound in `godot::prelude::Node::set_name`
   --> /target/debug/build/godot-core-80fd3d4653caad96/out/classes/node.rs:224:47
    |
224 |         pub fn set_name(&mut self, name: impl AsArg < StringName >,) {
    |                                               ^^^^^^^^^^^^^^^^^^^^ required by this bound in `Node::set_name`

error[E0271]: type mismatch resolving `<GString as ToGodot>::Pass == ByValue`
   --> src/lib.rs:74:32
    |
 74 |         self.base().find_child(my_gstring);
    |                     ---------- ^^^^^^^^^^ expected `ByValue`, found `ByRef`
    |                     |
    |                     required by a bound introduced by this call
    |
    = note: required for `godot::prelude::GString` to implement `AsArg<godot::prelude::GString>`
note: required by a bound in `godot::prelude::Node::find_child`
   --> /target/debug/build/godot-core-80fd3d4653caad96/out/classes/node.rs:379:48
    |
379 |         pub fn find_child(&self, pattern: impl AsArg < GString >,) -> Option < Gd < crate::classes::Node > > {
    |                                                ^^^^^^^^^^^^^^^^^ required by this bound in `Node::find_child`

error[E0271]: type mismatch resolving `<NodePath as ToGodot>::Pass == ByValue`
   --> src/lib.rs:77:43
    |
 77 |         self.base().has_node_and_resource(my_nodepath);
    |                     --------------------- ^^^^^^^^^^^ expected `ByValue`, found `ByRef`
    |                     |
    |                     required by a bound introduced by this call
    |
    = note: required for `godot::prelude::NodePath` to implement `AsArg<godot::prelude::NodePath>`
note: required by a bound in `godot::prelude::Node::has_node_and_resource`
   --> /target/debug/build/godot-core-80fd3d4653caad96/out/classes/node.rs:413:56
    |
413 |         pub fn has_node_and_resource(&self, path: impl AsArg < NodePath >,) -> bool {
    |                                                        ^^^^^^^^^^^^^^^^^^ required by this bound in `Node::has_node_and_resource`

Which is very unhelpful and confusing.

After changes in PR following compile errors are being reported:

Even more compile errors!

error[E0277]: Argument of type `godot::prelude::StringName` cannot be passed to an impl AsArg<T> parameter. If you pass by value, consider borrowing instead.
              See also `AsArg` docs: https://godot-rust.github.io/docs/gdext/master/godot/meta/trait.AsArg.html
   --> src/lib.rs:71:34
    |
 71 |         self.base_mut().set_name(my_stringname);
    |                         -------- ^^^^^^^^^^^^^ the trait `ShouldBePassedAsRef` is not implemented for `godot::prelude::StringName`
    |                         |
    |                         required by a bound introduced by this call
    |
    = note: the trait bound `godot::prelude::StringName: ShouldBePassedAsRef` is not satisfied
note: required by a bound in `godot::prelude::Node::set_name`
   --> /target/debug/build/godot-core-955e37c5b200e330/out/classes/node.rs:224:70
    |
224 |         pub fn set_name(&mut self, name: impl AsArg < StringName > + ShouldBePassedAsRef,) {
    |                                                                      ^^^^^^^^^^^^^^^^^^^ required by this bound in `Node::set_name`
help: consider borrowing here
    |
 71 |         self.base_mut().set_name(&my_stringname);
    |                                  +

error[E0271]: type mismatch resolving `<StringName as ToGodot>::Pass == ByValue`
   --> src/lib.rs:71:34
    |
 71 |         self.base_mut().set_name(my_stringname);
    |                         -------- ^^^^^^^^^^^^^ expected `ByValue`, found `ByRef`
    |                         |
    |                         required by a bound introduced by this call
    |
    = note: required for `godot::prelude::StringName` to implement `AsArg<godot::prelude::StringName>`
note: required by a bound in `godot::prelude::Node::set_name`
   --> /target/debug/build/godot-core-955e37c5b200e330/out/classes/node.rs:224:47
    |
224 |         pub fn set_name(&mut self, name: impl AsArg < StringName > + ShouldBePassedAsRef,) {
    |                                               ^^^^^^^^^^^^^^^^^^^^ required by this bound in `Node::set_name`

error[E0277]: Argument of type `godot::prelude::GString` cannot be passed to an impl AsArg<T> parameter. If you pass by value, consider borrowing instead.
              See also `AsArg` docs: https://godot-rust.github.io/docs/gdext/master/godot/meta/trait.AsArg.html
   --> src/lib.rs:74:32
    |
 74 |         self.base().find_child(my_gstring);
    |                     ---------- ^^^^^^^^^^ the trait `ShouldBePassedAsRef` is not implemented for `godot::prelude::GString`
    |                     |
    |                     required by a bound introduced by this call
    |
    = note: the trait bound `godot::prelude::GString: ShouldBePassedAsRef` is not satisfied
note: required by a bound in `godot::prelude::Node::find_child`
   --> /target/debug/build/godot-core-955e37c5b200e330/out/classes/node.rs:379:68
    |
379 |         pub fn find_child(&self, pattern: impl AsArg < GString > + ShouldBePassedAsRef,) -> Option < Gd < crate::classes::Node > > {
    |                                                                    ^^^^^^^^^^^^^^^^^^^ required by this bound in `Node::find_child`
help: consider borrowing here
    |
 74 |         self.base().find_child(&my_gstring);
    |                                +

error[E0271]: type mismatch resolving `<GString as ToGodot>::Pass == ByValue`
   --> src/lib.rs:74:32
    |
 74 |         self.base().find_child(my_gstring);
    |                     ---------- ^^^^^^^^^^ expected `ByValue`, found `ByRef`
    |                     |
    |                     required by a bound introduced by this call
    |
    = note: required for `godot::prelude::GString` to implement `AsArg<godot::prelude::GString>`
note: required by a bound in `godot::prelude::Node::find_child`
   --> /target/debug/build/godot-core-955e37c5b200e330/out/classes/node.rs:379:48
    |
379 |         pub fn find_child(&self, pattern: impl AsArg < GString > + ShouldBePassedAsRef,) -> Option < Gd < crate::classes::Node > > {
    |                                                ^^^^^^^^^^^^^^^^^ required by this bound in `Node::find_child`

error[E0277]: Argument of type `godot::prelude::NodePath` cannot be passed to an impl AsArg<T> parameter. If you pass by value, consider borrowing instead.
              See also `AsArg` docs: https://godot-rust.github.io/docs/gdext/master/godot/meta/trait.AsArg.html
   --> src/lib.rs:77:43
    |
 77 |         self.base().has_node_and_resource(my_nodepath);
    |                     --------------------- ^^^^^^^^^^^ the trait `ShouldBePassedAsRef` is not implemented for `godot::prelude::NodePath`
    |                     |
    |                     required by a bound introduced by this call
    |
    = note: the trait bound `godot::prelude::NodePath: ShouldBePassedAsRef` is not satisfied
note: required by a bound in `godot::prelude::Node::has_node_and_resource`
   --> /target/debug/build/godot-core-955e37c5b200e330/out/classes/node.rs:413:77
    |
413 |         pub fn has_node_and_resource(&self, path: impl AsArg < NodePath > + ShouldBePassedAsRef,) -> bool {
    |                                                                             ^^^^^^^^^^^^^^^^^^^ required by this bound in `Node::has_node_and_resource`
help: consider borrowing here
    |
 77 |         self.base().has_node_and_resource(&my_nodepath);
    |                                           +

error[E0271]: type mismatch resolving `<NodePath as ToGodot>::Pass == ByValue`
   --> src/lib.rs:77:43
    |
 77 |         self.base().has_node_and_resource(my_nodepath);
    |                     --------------------- ^^^^^^^^^^^ expected `ByValue`, found `ByRef`
    |                     |
    |                     required by a bound introduced by this call
    |
    = note: required for `godot::prelude::NodePath` to implement `AsArg<godot::prelude::NodePath>`
note: required by a bound in `godot::prelude::Node::has_node_and_resource`
   --> /target/debug/build/godot-core-955e37c5b200e330/out/classes/node.rs:413:56
    |
413 |         pub fn has_node_and_resource(&self, path: impl AsArg < NodePath > + ShouldBePassedAsRef,) -> bool {
    |                                                        ^^^^^^^^^^^^^^^^^^ required by this bound in `Node::has_node_and_resource`

Some errors have detailed explanations: E0271, E0277.
For more information about an error, try `rustc --explain E0271`.

In short – the old error is still being preserved, but we spawn additional one with explanation and link to docs while compiler generates proper, helpful resolution how to make problem go away. It also always spawn first, which is nice.

Other cases:

Everything else stays the same. GString/StringName/NodePath shows error coming from AsArg diagnostic if passing one as an another might result in non-obvious implicit conversion:

        self.base_mut().set_name(&my_gstring);

error[E0277]: Argument of type `&godot::prelude::GString` cannot be passed to an `impl AsArg<godot::prelude::StringName>` parameter
   --> src/lib.rs:72:34
    |
 72 |         self.base_mut().set_name(&my_gstring);
    |                         -------- ^^^^^^^^^^^ the trait `AsArg<godot::prelude::StringName>` is not implemented for `&godot::prelude::GString`
    |                         |
    |                         required by a bound introduced by this call
    |
    = note: if you pass by value, consider borrowing instead.
    = note: GString/StringName/NodePath aren't implicitly convertible for performance reasons; use their `arg()` method.
    = note: see also `AsArg` docs: https://godot-rust.github.io/docs/gdext/master/godot/meta/trait.AsArg.html
    = help: the following other types implement trait `AsArg<T>`:

Problems considerations etc.

Marker trait is independent from AsArg – firstly because it must be (duh), secondly to prevent more type silliness, thirdly to make sure that AsArg behavior won't change.

label and note fields on #[diagnostic(...)] attribute are being swallowed by compiler. It is probably a bug I should report, albeit rust reference states that The hints provided by these attributes are not guaranteed to be used. (fun!).

Arguably it makes signature a little more cluttered, but it is good enough tradeoff I think.

Idk if ShouldBePassedByRef is good enough name 🤷

Some generic APIs (notably Arrays) are not covered because we can't do so in easy and convenient way.

And finally – ByValue/ByOption/ByObject are not covered since I haven't seen a need to do so

[GodotRust]

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

[Bromeon]

Thanks!

I'm not sure if this is a good trade-off... looking at GString docs for example:

The extra bound ShouldBePassedAsRef adds mental load -- not only to doc readers, but to everyone who has to interact with the code as well, especially us. Yes, it does improve error messages, but that only benefits people who pass GString wrong. Whereas everyone has to suffer from the extra bound on screen, whether they fully understood the concept or not. The picture also shows an inconsistency of ShouldBePassedAsRef being present in some but not all cases, which is a mistake that's going to happen over and over, and seeing this as a user raises more questions than it answers.

I was under the impression this would be more like You_forgot_the_attribute__godot_api (could be named Should_be_passed_by_ref). That one also appears in docs, but it's relatively contained:

(And yes, I would also remove that if I could -- but it solved a very frequent mistake that compiled fine but caused runtime issues, so the RoI was completely different).

[Bromeon]

Other example, generated Label API -- I chose this class because it has many text-related APIs:

The same methods on master fit on one line each:

[Yarwin]

hmmm, I think there is one more approach I can try