cargo check points to #[godot_api] if delta: f64 is missing from process

[fpdotmonkey]

I wrote a process function like so, forgetting its delta argument.

use godot::prelude::*;

struct Example;

#[gdextension]
unsafe impl ExtensionLibrary for Example {}

#[derive(GodotClass)]
#[class(init, tool, base=Node)]
struct Thing {
    base: Base<Node>,
}

#[godot_api]
impl INode for Thing {
    fn process(&mut self) { // delta is missing
        todo!()
    }
}

Running cargo check against this yields the below errors. Note how the warning "an argument of type f64 is missing" points toward the #[godot_api] macro. This is problematic because text editors will use this output to suggest quick fixes to developers, but here the suggestion would be to replace #[godot_api] with process(/* f64 */), which is completely wrong.

error[E0050]: method `process` has 1 parameter but the declaration in trait `godot::prelude::INode::process` has 2
  --> src/lib.rs:16:16
   |
16 |     fn process(&mut self) {
   |                ^^^^^^^^^ expected 2 parameters, found 1
   |
   = note: `process` from trait: `fn(&mut Self, f64)`

error[E0061]: this method takes 1 argument but 0 arguments were supplied
  --> src/lib.rs:16:8
   |
14 | #[godot_api]
   | ------------ an argument of type `f64` is missing
15 | impl INode for Thing {
16 |     fn process(&mut self) {
   |        ^^^^^^^
   |
note: method defined here
  --> /home/fp/.cargo/git/checkouts/gdext-76630c89719e160c/9900076/godot-core/src/gen/classes/node.rs:79:12
   |
79 |         fn process(&mut self, delta: f64,) {
   |            ^^^^^^^
help: provide the argument
   |
14 | process(/* f64 */)
   |

Some errors have detailed explanations: E0050, E0061.
For more information about an error, try `rustc --explain E0050`.
error: could not compile `schematic` (lib) due to 2 previous errors

[lilizoey]

i think we dont preserve spans properly in some places, so this is probably related to that

[Bromeon]

There seems to be a discrepancy though. Without proc-macro, we just get this error:

error[E0050]: method `process` has 1 parameter but the declaration in trait `godot::prelude::INode::process` has 2
   --> examples\dodge-the-creeps\rust\src\main_scene.rs:151:16
    |
151 |     fn process(&mut self) { // delta is missing
    |                ^^^^^^^^^ expected 2 parameters, found 1
    |
    = note: `process` from trait: `fn(&mut Self, f64)`

error[E0061] does not appear since the method is not called, so there is no mention of "an argument of type f64 is missing" at all. So maybe we should look at the call site -- but I'm not sure if we can influence this at all within proc-macro code?

@fpdotmonkey would you have time to help look into this?

---

By the way, there's something to be said about spans in general -- it doesn't apply here, so slightly off-topic:

Rust has not managed to get useful span operations stabilized, even though they have worked well in nightly for years. Spans are absolutely crippled in stable -- for some reason it's not possible to join two spans properly (nightly can do it, stable just returns the first token). This is extremely frustrating and just one of these Rust features where people are afraid of any commitment and as a result, things are stuck in RFC hell forever.

It's the reason why even with great effort, we cannot provide the best possible user experience in proc-macros, unless users opt in to nightly. syn suffers from the same problems.

See also https://github.com/PoignardAzur/venial/blob/32ade31408eb0ffba4ce7b02d3cdb2e78b496644/src/lib.rs#L55-L96

[fpdotmonkey]

Ok so I've looked into it and have found the problem, but finding a solution seems difficult.

The code that's generated (according to cargo expand) that calls the errant process is this.

<((),) as ::godot::builtin::meta::PtrcallSignatureTuple>::in_ptrcall( // the tuple type is wrong
    instance_ptr,
    &::godot::builtin::meta::CallContext::func("Thing", "process"),
    args_ptr,
    ret,
    |instance_ptr, params| {
        let () = params;  // no parameters are being passed
        let storage = unsafe {
            ::godot::private::as_storage::<Thing>(instance_ptr)
        };
        let mut instance = ::godot::private::Storage::get_mut(
            storage,
        );
        instance.process() // this is the line that `cargo check` wants to have replaced
    },
    sys::PtrcallType::Virtual,
);

This generated code is wrong according to the trait definition because it relies on the user code to provide a correct signature. If the generated code is manually modified to follow the trait definition, cargo check only errors on user code and not generated code.

<((), f64) as ::godot::builtin::meta::PtrcallSignatureTuple>::in_ptrcall( // tuple is now correct
    instance_ptr,
    &::godot::builtin::meta::CallContext::func("Thing", "process"),
    args_ptr,
    ret,
    |instance_ptr, params| {
        let (delta,) = params; // parameter gets assigned
        let storage =
            unsafe { ::godot::private::as_storage::<Thing>(instance_ptr) };
        let mut instance = ::godot::private::Storage::get_mut(storage);
        instance.process(delta) // called with the correct number of arguments based on the trait
    },
    sys::PtrcallType::Virtual,
);
// user definition of Thing::process errors

So how to solve this?

The root issue is that the macro is getting the function signature from an error prone source, namely the user. It might be possible to generate PtrcallSignatureTuple from INode::process as a Fn(Args) -> Return, but I would be concerned that generating the function call itself would be difficult since tuples don't have known length and you might need that to create the relevant symbols (or maybe there's macro magic that just solves this). This would be alleviated by fn_traits being stabilized, but that's waiting on variadics and a larger type system rework to be stabilized. So this route is maybe possible, but I don't have the knowledge to see clearly.

With respect to issues in the #[godot_api] macro itself, I don't see anything that jumps out as wrong. I don't have really any familiarity with rust macros, so I could easily be missing something, but again, the code produced by the macro is correct if you can assume that the user is providing the correct signature.

There aren't any obvious band-aid fixes. If this were a warning, we could tell rustc to ignore this code, but it's an error, so there not much that can be done.

One positive change that could be done would be to call e.g. INode::process(&mut instance, delta) instead of instance.process(delta) in the macro. With it, cargo check gives the suggestion of using the snippet INode::process(&mut instance, /* f64 */) instead of process(/* f64 */) as above, which I think is a bit less cryptic. Still not good though.

[Bromeon]

Thanks so much for this thorough investigation, it's very appreciated! ❤️

The root issue is that the macro is getting the function signature from an error prone source, namely the user. It might be possible to generate PtrcallSignatureTuple from INode::process as a Fn(Args) -> Return, but I would be concerned that generating the function call itself would be difficult since tuples don't have known length and you might need that to create the relevant symbols (or maybe there's macro magic that just solves this). This would be alleviated by fn_traits being stabilized, but that's waiting on variadics and a larger type system rework to be stabilized. So this route is maybe possible, but I don't have the knowledge to see clearly.

Very good observation. I think it should be possible to infer the call signature from the API rather than user code, I'll need to dig a bit in the code to verify 🙂

[Bromeon]

I didn't get around to this and likely won't have the chance anytime soon.

@fpdotmonkey are you by any chance interested in pursuing this? 🙂

[fpdotmonkey]

Where I left off, I didn't really have much idea of how it could be fixed. If you have any leads toward how to get the signature from the API into the #[godot_api] macro, I could look into it, but otherwise I'm not really sure how I would pursue this.

[lilizoey]

one thing that seems relatively straightforward but im not sure if is ideal would be to change the codegen to add a bunch of type aliases to each class for the arguments tuple of each virtual trait method.

then we can use that type alias in here instead. like

let (delta,): ::godot::engine::node::virtual_aliases::process = ..

we'd probably make these doc(hidden) and have them be using the wrong naming convention to simplify referring to them.

however this may have an impact on compile times? and it would increase the number of symbols available.

otherwise im not entirely sure what the alternative is. i dont think it's possible to access the arguments tuple of a function pointer, currently that's defined in a FnOnce<Args> trait so you'd need to already know the arguments to refer to it. and progress on improving this in rust is effectively blocked on variadic tuples, which is blocked on the type system rewrite, which is likely at least a year away.

[Bromeon]

The type aliases are an interesting idea... But I'm not sure if generating a lot of technically redundant symbols is the way to go? 🤔 Other proc-macro APIs must have similar problems, or how do they deal with this?

Does anybody have the time and motivation to do some research on this front? 🙂