Open fpdotmonkey opened 6 months ago
i think we dont preserve spans properly in some places, so this is probably related to that
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?
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.
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
fromINode::process
as aFn(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 byfn_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 🙂
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? :slightly_smiling_face:
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.
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.
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? :thinking: 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? :slightly_smiling_face:
I wrote a process function like so, forgetting its
delta
argument.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]
withprocess(/* f64 */)
, which is completely wrong.