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

Attribute proc-macro follow up #36

Merged
merged 4 commits into from
Jan 24, 2022
Merged

Conversation

daxpedda
Copy link
Collaborator

Fixes #35.

I added a note in the README, but noticed that actually setting the path can produce some pretty unexpected behavior.

What do you think of adding an error if a derive_where::derive_where is found as not the first attribute? This isn't bulletproof of course because derive_where_export::derive_where could still remain undetected.

@daxpedda daxpedda requested a review from ModProg January 21, 2022 09:36
@daxpedda
Copy link
Collaborator Author

I just stumbled on an idea. We could leave a marker to signify that this item was already handled by a derive-where macro. We control the outputted item, so we could just add a comment: /* derive_where was here! */. If that comment was found by a follow up proc macro attribute it could error out.

@daxpedda
Copy link
Collaborator Author

Actually I couldn't find a way to mark the item. Comments aren't represented in the AST apparently, only doc comments, and I can't figure out how to make a "hidden" doc comment, so it doesn't show up in the documentation by the user.

The other idea I had while writing this was to introduce another attribute called derive_where_visited, but it actually hits the same problem, it won't work without proper qualifying it unless it's imported by the user. We also don't have $crate for proc macros, see rust-lang/rust#54363, which would solve that problem.

So I think the best would be to introduce a hidden doc comment or find some other attribute that can be no-op. If you come up with anything hit me.

@daxpedda
Copy link
Collaborator Author

daxpedda commented Jan 21, 2022

To solve the problem with derive_where_visited we could also introduce a crate setting: #[derive_where_::derive_where(crate = "derive_where_"; Clone; T)]?

@daxpedda
Copy link
Collaborator Author

Let's figure out #37 before we decide to merge this.

Copy link
Owner

@ModProg ModProg left a comment

Choose a reason for hiding this comment

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

Looks good AFAICT. If attribute macros on fields were introduced, we could circumvent all of this, probably.

CHANGELOG.md Show resolved Hide resolved
@daxpedda
Copy link
Collaborator Author

Alright, re-based and adjusted to the latest changes we did in #37.

@daxpedda daxpedda marked this pull request as ready for review January 23, 2022 14:10
@daxpedda daxpedda requested a review from ModProg January 23, 2022 14:10
@daxpedda
Copy link
Collaborator Author

Build failed because of some connection issue, would be nice if you re-start it @ModProg.

@daxpedda daxpedda mentioned this pull request Jan 23, 2022
README.md Outdated Show resolved Hide resolved
@daxpedda daxpedda merged commit b0decf7 into ModProg:main Jan 24, 2022
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.

Test and document usage errors/problems with attribute_macro
2 participants