-
Notifications
You must be signed in to change notification settings - Fork 20
[legacy] Allow other attributes in #[pin_data] structs #93
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
base: legacy
Are you sure you want to change the base?
Conversation
When the `__pin_data!(find_pinned_fields:` part of the macro encounters
an unknown attribute (anything apart from `$[pin]`) before a field, it
is put into the accumulator and the macro proceeds further. Whatever is
in the accumulator is later saved into `$fields` and then also into
`$pinned` or `$not_pinned` depending on whether that field is pinned.
Pinned fields, with all their unknown attributes are also used in the
internal `__Unpin` struct.
A field can have multiple different attributes, mostly belonging into
two different categories. Built-in (defined by the language itself) and
arbitrary (any other attribute that is used by another proc-macro
crate).
Out of the built-in ones [1] the only things that make sense to be
usable for struct fields are most probably just "cfg", "doc", lint
levels ("allow", "expect", "warn", "deny", "forbid"), and "deprecated".
Out of these the only one that makes sense to keep around for the
pin_data macro is "cfg" since that one does a conditional compilation
and we only want the members to be included if they are included in the
original struct.
From the arbitrary (basically unknown) ones there is no reason for them
to be used, mainly because they will likely be part of a derive which
will not be included for the `__Unpin` struct, therefore making the code
fail to compile if included. Since those need to be kept for the
original struct, move them to the `$fields` instead of `$accum` in order
not to pollute the `struct __Unpin` with unknown attributes.
To put this all together, add a test with a custom attribute that fails
without this fix. Unfortunately, another crate needs to be added for
the test.
[1] https://doc.rust-lang.org/reference/attributes.html#built-in-attributes-index
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
|
Thanks for your PR! I'm currently very busy and won't be taking a look for the next few weeks, sorry about that. This limitation is annoying and I wanted to get rid of it in the future. I will be merging #89 in the next kernel cycle (in around 5-6 weeks). Would you mind re-doing the work based on The test looks good by the way! |
|
I'll see what I can do. I finally understood how to fix it in the legacy branch and I have to figure out how it works after #89 but I'll do my best. |
BennoLossin
left a comment
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.
Sorry for the long delay. I'm planning to merge #89 in this kernel cycle, so we should have a new version in around 5 weeks. Following that, I will publish the crate under pin-init. I also plan to make a new legacy release at that time that explains that the move to/merge with pin-init is complete.
Now if you really need this earlier, I can also make a new legacy release in the meantime.
| version = "0.0.1" | ||
| edition = "2021" | ||
|
|
||
| authors = ["y86-dev"] |
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.
This should be you, right?
| version = "0.0.1" | ||
| edition = "2021" |
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.
| version = "0.0.1" | |
| edition = "2021" | |
| version = "0.0.0" | |
| edition = "2024" |
| @@ -0,0 +1,13 @@ | |||
| [package] | |||
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.
I think it would be better to move this crate to tests/dummy.
When the
__pin_data!(find_pinned_fields:part of the macro encounters an unknown attribute (anything apart from$[pin]) before a field, it is put into the accumulator and the macro proceeds further. Whatever is in the accumulator is later saved into$fieldsand then also into$pinnedor$not_pinneddepending on whether that field is pinned. Pinned fields, with all their unknown attributes are also used in the internal__Unpinstruct.A field can have multiple different attributes, mostly belonging into two different categories. Built-in (defined by the language itself) and arbitrary (any other attribute that is used by another proc-macro crate).
Out of the built-in ones [1] the only things that make sense to be usable for struct fields are most probably just "cfg", "doc", lint levels ("allow", "expect", "warn", "deny", "forbid"), and "deprecated". Out of these the only one that makes sense to keep around for the pin_data macro is "cfg" since that one does a conditional compilation and we only want the members to be included if they are included in the original struct.
From the arbitrary (basically unknown) ones there is no reason for them to be used, mainly because they will likely be part of a derive which will not be included for the
__Unpinstruct, therefore making the code fail to compile if included. Since those need to be kept for the original struct, move them to the$fieldsinstead of$accumin order not to pollute thestruct __Unpinwith unknown attributes.To put this all together, add a test with a custom attribute that fails without this fix. Unfortunately, another crate needs to be added for the test.
[1] https://doc.rust-lang.org/reference/attributes.html#built-in-attributes-index