Add visitors for checking #[inline] #80641
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @oli-obk (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to see the possible cause of the failure (guessed by this bot) |
LingMan
reviewed
Jan 3, 2021
oli-obk
reviewed
Jan 3, 2021
There was a problem hiding this comment.
Similarly to Target::AssocConst, the check_inline function should emit lints instead of errors on the newly added Target kinds, as we did not emit errors on them before.
I think we also need to vet the other things that check_attributes runs on to see if we need to opt out of erroring on them
oli-obk
reviewed
Jan 3, 2021
There was a problem hiding this comment.
Ok... I only now see the extent of this change. We need to catch all field/arm/macrodef arms and return true for them in all the check functions at https://github.com/rust-lang/rust/blob/b3e39da0bc1c26b39b0a600b3735805722108992/compiler/rustc_passes/src/check_attr.rs#L188 or below. Ideally we start linting on them, too, but since it's strictly an improvement to start linting on #[inline] I propose to add an arm similar to what you did in check_allow_internal_usntable to all other check functions that ensures no error is emitted for them and leave a FIXME on these new arms. Then we can merge this PR now and address the FIXMEs in future PRs.
What do you think? Or do you prefer to do everything in this PR? It may take a bit longer for the PR to end up getting merged in that case, so your call :)
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
This has not been addressed yet. Would you like me to elaborate more on it or is it already clear what I mean with the text above? |
|
@Danue1 @oli-obk I didn't realize this was being worked on so I opened #80920 which does something similar. The PRs are not exactly the same so we should probably coordinate to make sure all the relevant changes make it into master. Two things in particular:
Also, see the PR's opening description for some thoughts on next steps for attribute validation. In general, I'm happy to close my PR in favor of this one if we can address the two points above. |
|
Oh heh, I guess we can crater it instead of emitting future incompat lints. I was just thinking about the minimal way forward, but if crater is deemed acceptable, then by all means, let's just forbid it directly. |
|
☔ The latest upstream changes (presumably #81113) made this pull request unmergeable. Please resolve the merge conflicts. |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
Please rebase instead of merging (you can also squash your commits at the same time to clean up the back and forth). You also need to run the test suite with The implementation looks good to me now |
|
I'm always a bit wary of crater runs, as they don't cover windows, embedded or private code at all. But I guess we do have precedents for this, so... My proposal would be to finish this PR (rebase, squash, r=me), then do your PR, which covers more constructs. I don't want to put more work on @Danue1 , so I think we should just merge the current PR, even if it makes some things future incompat warnings that could have been made hard errors. Would that be ok with you, or is the rebase over this PR going to be very painful? |
|
Sounds like a plan to me. |
|
Apologizing for lating... |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Add visitors for checking #[inline] Add visitors for checking #[inline] with struct field Fix test for #[inline] Add visitors for checking #[inline] with #[macro_export] macro Add visitors for checking #[inline] without #[macro_export] macro Add use alias with Visitor Fix lint error Reduce unnecessary variable Co-authored-by: LingMan <LingMan@users.noreply.github.com> Change error to warning Add warning for checking field, arm with #[allow_internal_unstable] Add name resolver Formatting Formatting Fix error fixture Add checking field, arm, macro def
|
@bors r+ Thanks! |
|
📌 Commit 838f487 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
As may have been expected, this is a performance regression -- largely for incremental benchmarks which presumably avoided these edges in the depgraph previously. I do not expect that we can do much about it, so seems fine to just let it slide. |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 16, 2021
…henkov Fix unused attributes on macro_rules. The `unused_attributes` lint wasn't firing on attributes of `macro_rules` definitions. The consequence is that many attributes are silently ignored on `macro_rules`. The reason is that `unused_attributes` is a late-lint pass, and only has access to the HIR, which does not have macro_rules definitions. My solution here is to change `non_exported_macro_attrs` to be `macro_attrs` (a list of all attributes used for `macro_rules`, instead of just those for `macro_export`), and then to check this list in the `unused_attributes` lint. There are a number of alternate approaches, but this seemed the most reliable and least invasive. I am open to completely different approaches, though. One concern is that I don't fully understand the implications of extending `non_exported_macro_attrs` to include non-exported macros. That list was originally added in rust-lang#62042 to handle stability attributes, so I suspect it was just an optimization since that was all that was needed. It was later extended to be included in SVH in rust-lang#83901. rust-lang#80641 also added a use to check for `invalid` attributes, which seems a little odd to me (it didn't validate non-exported macros, and seems highly specific). Overall, there doesn't seem to be a clear story of when `unused_attributes` should be used versus an error like E0518. I considered alternatively using an "allow list" of built-in attributes that can be used on macro_rules (allow, warn, deny, forbid, cfg, cfg_attr, macro_export, deprecated, doc), but I feel like that could be a pain to maintain. Some built-in attributes already present hard-errors when used with macro_rules. These are each hard-coded in various places: - `derive` - `test` and `bench` - `proc_macro` and `proc_macro_derive` - `inline` - `global_allocator` The primary motivation is that I sometimes see people use `#[macro_use]` in front of `macro_rules`, which indicates there is some confusion out there (evident that there was even a case of it in rustc).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For #80564