Skip to content
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

Unify all attributes into one; support every item; add default #1

Merged
merged 4 commits into from
Apr 29, 2023

Conversation

wackbyte
Copy link
Collaborator

Adds support for all items and the default qualifier, all with one attribute.

Implementation notes

Behavior change

My implementation of the attribute has a slightly different behavior:

// In the original implementation:
#[fn_qualifiers()] // removes the `const` qualifier!
const fn const_fn_or_is_it() {}

// In this new implementation:
#[qualifiers()] // doesn't touch the item's qualifiers at all
const fn definitely_a_const_fn() {}

Parsing logic

In order to support const, fn, static, and type items in all positions, I had to implement custom item types to parse them specially.

// Example:
#[qualifiers(pub)]
static ITEM_STATIC: u8 = 100;

extern "C" {
    #[qualifiers(pub)]
    static FOREIGN_ITEM_STATIC: u8;
    // If this was parsed as a plain `Item`/`ItemStatic`, it would require the `= <expr>` at the end!
}

// (custom `const`, `fn`, and `type` items are required for `default` support)

Questions

Modules

Modules can't be supported due to hygiene issues.

Is this true? In my testing they seemed to work fine (except for the rust-analyzer issue I noted).

Fields

I did not implement support for named fields. I have a concept in mind, though:

#[field_qualifiers(x(pub), y(pub))]
struct InHeader {
    x: String,
    y: u32,
    // TODO: how does this work for tuple structs? `#[field_qualifiers(_0(pub))]`?
}

@@ -0,0 +1,154 @@
//! A test for every supported item.
//! This test requires the nightly toolchain to compile.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This file is named the way it is so that cargo test ignores it (so you don't need nightly installed to compile)

- Fix the example's formatting and spelling
- Wrap some lines in the README
- Update the crate description
- "common" -> "flexible"
  - syn does something like this
  - It's clearer
  - I think I like it better
The code looks kind of silly but it works
@wackbyte
Copy link
Collaborator Author

I've implemented #[field_qualifiers] as I described it above.

@JohnScience
Copy link
Owner

@wackbyte You were given the contributor rights. You can merge the pull request if you want.

@wackbyte
Copy link
Collaborator Author

Thanks!

@wackbyte wackbyte merged commit d3c621f into JohnScience:main Apr 29, 2023
@wackbyte wackbyte deleted the unify branch April 29, 2023 14:55
@ryoqun
Copy link

ryoqun commented Aug 12, 2023

@JohnScience @wackbyte hey, this pr looks quite impressive. :) when will this be published on crates.io as new ver?

this crate is so useful for our usecase like this: solana-labs/solana#32822 (comment) thanks for the great work.

seems the latest published version v0.1.6 works for now. but I'm planning to heavily use this crate across the codebase.

@wackbyte
Copy link
Collaborator Author

@JohnScience would have the ability to do that for you.

@JohnScience
Copy link
Owner

JohnScience commented Aug 13, 2023

@wackbyte You have been given access on crates.io as well

@JohnScience
Copy link
Owner

JohnScience commented Aug 13, 2023

@ryoqun Restored the legacy API to avoid breakage and published the 0.2 version on crates.io. Please take a look at the note on legacy attributes.

@ryoqun
Copy link

ryoqun commented Aug 15, 2023

@ryoqun Restored the legacy API to avoid breakage and published the 0.2 version on crates.io. Please take a look at the note on legacy attributes.

really thanks so much (even with polishes with bunch of v0.2.x rels); just created update pr at our side: solana-labs/solana#32838

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants