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

Silence some warnings from derive(Properties) #2266

Merged
merged 5 commits into from Dec 16, 2021

Conversation

WorldSEnder
Copy link
Member

@WorldSEnder WorldSEnder commented Dec 12, 2021

Description

The derive macro for Properties should preserve some attributes on fields.

On the one hand, warnings on setters and fields of the generated builder are reported on the original field due to spanned quotes. It can be confusing to users that adding silencing annotations have no effect before this patch.

On the other hand, cfg attributes are passed on to work around an issue with rust-analyzer; Normally cfg attributes are evaluated by the compiler before the token tree is passed to the proc-macro. rust-analyzer seems not to do that in some cases, but still generates errors and lints based on the result. When trying to connect those errors back to the source code, it reports "no-such-field" since cfg attributes on the original struct are respect and the field is invisible.
To be clear, cfg attributes are not expected to show up during normal compilation, but are a quirk in some IDEs. In any case, it shouldn't hurt to respect them if they are found.

Checklist

  • I have run cargo make pr-flow
  • I have reviewed my own code
  • I have added tests

@WorldSEnder
Copy link
Member Author

I have difficulties replicating this in a test case since I don't know how trybuild can be made to run clippy lints and reproducing an IDE setup to run rust-analyzer is also not an accessible goal for me.

@WorldSEnder WorldSEnder marked this pull request as ready for review December 14, 2021 03:52
@hamza1311
Copy link
Member

#[allow(_)] and #[deny(_)] work with cargo lints too. If possible, we should add test cases for those. I don't know if #[cfg(_)] can be tested though

voidpumpkin
voidpumpkin previously approved these changes Dec 15, 2021
Copy link
Member

@voidpumpkin voidpumpkin left a comment

Choose a reason for hiding this comment

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

LGTM
Thank you for the PR!

@voidpumpkin voidpumpkin added the A-yew-macro Area: The yew-macro crate label Dec 15, 2021
siku2
siku2 previously approved these changes Dec 15, 2021
@voidpumpkin voidpumpkin dismissed stale reviews from siku2 and themself via 803e0be December 15, 2021 22:18
@WorldSEnder
Copy link
Member Author

My pleasure 👍

Copy link
Member

@hamza1311 hamza1311 left a comment

Choose a reason for hiding this comment

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

LGTM

@hamza1311 hamza1311 merged commit f5b9219 into yewstack:master Dec 16, 2021
@WorldSEnder WorldSEnder deleted the silence-field-warnings branch December 16, 2021 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew-macro Area: The yew-macro crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants