-
-
Notifications
You must be signed in to change notification settings - Fork 200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix usage of segmented path attribute with #[func] #439
Conversation
I would put tests in |
f4c81f5
to
2970d3a
Compare
Thanks! Added a test in |
2970d3a
to
37a8d4a
Compare
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-439 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! 🙂
If I understand correctly, this only fixes the issue for attributes that are segmented? What about things like #[cfg(...)]
?
I wonder if we should not retain all attributes that are not recognized in the generated code, and remove those that are gdext-specific. That way, unknown attributes would still cause compile errors, but others should work as intended.
Correct. And only in the specific scenario mentioned in the linked reply (non-virtual function impls).
I gave it a shot locally, but it is more difficult to implement unfortunately. It will require some further discussions. It's also nice to keep this PR simple 😄 (I saw a quick fix and applied it!) Note that cfg should already work when using #[func] (or similar attributes) - the only requirement is that #[func] must currently appear on the top of the attributes. (This should eventually be lifted in an eventual PR, of course.) However, currently cfg does not work at all with virtual functions (which do not use #[func]). This is mostly because we apparently process the attributes before they can have an effect when processing the virtual trait impl, so we wouldn't be able to discern whether or not a function was removed or renamed by cfg-like macros before creating the FFI block. Still thinking on how to work around this (other than having the user manually instruct the macros to apply to the generated FFI block as well (e.g. cfg), which, in principle, sounds like more of a last resort approach, and thus undesirable).
I agree this is probably the best way forward. I still haven't thought of the best implementation for that idea however (despite having experimented a bit), due to the problem I mention above (order of processing of macros). I'll be keeping an open eye for further ideas though; I'll make sure to share if I implement something. |
This is already an improvement over the status quo; as such I'll merge it 🙂 thanks for the initiative! |
Currently, using
#[godot_api]
on bare (non-virtual)impl
s will panic if a#[segmented::path::attr]
is used together with#[func]
(as described in #379 (comment)). This is due to an.expect()
while searching for#[func]
,#[signal]
or#[constant]
ingodot-macros/../godot_api.rs
- those attribute macros (the expected attributes) don't have more than one path segment, which is fair, but a panic is excessive (since other unexpected macros that don't have a segmented path are just kept there, above the function, so the same should happen for unexpected segmented path attributes - they should just stay). Note that the same problem would happen with a#[signal]
or#[constant]
for the same reason.This PR fixes the problem by replacing the panic with a
continue;
(skip this attribute - it's clearly not#[func]
,#[signal]
or#[constant]
).(As a consequence, attribute macros such as
#[::something]
will also be considered unexpected and skipped - I don't think this is a problem at all.)I don't know where I'd add tests for this; feel free to suggest a location if needed.(Local tests on the Dodge the Creeps example have worked, however.)Update: Added tests under
itest/rust/src/register_tests/func_test.rs
, as suggested by @lilizoey, anditest/rust/src/register_tests/constant_test.rs
.NB: It seems the panicking behavior observed here was introduced with the rest of the logic in this commit: 078d8cb