-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Add errors for unknown, stable and duplicate feature attributes #52644
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
44a660688bdf05e999abb2f6c51b681292b3349f looks like it replaces a tidy check, so we could probably also remove that in this PR (or leave it as future work, double checking can't really hurt). |
@Mark-Simulacrum: yeah, you're right — it looks like it should be checked here: rust/src/tools/tidy/src/features.rs Lines 54 to 78 in 2e6fc3e
However, I suspect the check wasn't functioning correctly, because when I originally checked out master for this branch, there was an inconsistency in the stability attributes in stdsimd, which I imagine should have been caught when the submodule was updated? Edit: maybe it just doesn't check submodules. |
Yes, it's quite possible the check was broken before. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
d2ccfc0
to
5298536
Compare
Any reasons not to merge this into the single existing Completely unknown and duplicated lints are typos/mistakes that don't happen naturally due to version migration or something like this (like stable features) and also should be rare. |
Duplicated lints seem like they could be merged into
I initially implemented Duplicated lints should be able to be made into a hard error without any repercussions. |
5298536
to
28385c9
Compare
Ok, sounds good.
Most of those |
48aff9b
to
5f18e7d
Compare
This comment has been minimized.
This comment has been minimized.
5f18e7d
to
905b7e5
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Okay, now I've converted |
This comment has been minimized.
This comment has been minimized.
2b41798
to
22a0936
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
180d806
to
09ee7ae
Compare
This comment has been minimized.
This comment has been minimized.
09ee7ae
to
d0437b1
Compare
This comment has been minimized.
This comment has been minimized.
📣 Toolstate changed by #52644! Tested on commit b239743. 💔 clippy-driver on windows: test-pass → test-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra). |
Tested on commit rust-lang/rust@b239743. Direct link to PR: <rust-lang/rust#52644> 💔 clippy-driver on windows: test-pass → test-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra). 💔 clippy-driver on linux: test-pass → test-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra). 💔 rls on windows: test-fail → build-fail (cc @nrc, @rust-lang/infra). 💔 rls on linux: test-fail → build-fail (cc @nrc, @rust-lang/infra). 💔 rustfmt on windows: test-pass → build-fail (cc @nrc, @rust-lang/infra). 💔 rustfmt on linux: test-pass → build-fail (cc @nrc, @rust-lang/infra). 💔 nomicon on windows: test-pass → test-fail (cc @frewsxcv @gankro, @rust-lang/infra). 💔 nomicon on linux: test-pass → test-fail (cc @frewsxcv @gankro, @rust-lang/infra).
🎉 |
@varkor I expect neither |
Fixes compilation on newest nightly Rust due to rust-lang/rust#52644.
Unfortunately, this regressed compile speed across a lot of benchmarks, by up to 4.4% @varkor: any idea why this might be? |
@nnethercote: oh no :( I suspect it's because it's unconditionally traversing the entire crate looking for attributes. I wonder how #53397 affects that... I can think of one possible improvement, but off the top of my head, I'm not sure how to avoid this traversal... I'll try to give it some thought. |
@varkor Couldn't it be integrated in the existing stability pass? |
[WIP] Only fetch lib_features when there are unknown feature attributes An attempt to win back some of the performance lost in #52644 (comment). cc @nnethercote
[WIP] Only fetch lib_features when there are unknown feature attributes An attempt to win back some of the performance lost in #52644 (comment). cc @nnethercote
…ister Only fetch lib_features when there are unknown feature attributes An attempt to win back some of the performance lost in #52644 (comment). cc @nnethercote
This breaks the |
@jethrogb: this just produces a warning for activating stable Rust features, which can be ignored completely with |
I'm already doing that: #![allow(stable_features,unused_features)]
#![feature(alloc,collections,str_char,unicode)]
fn main() {}
|
@jethrogb: ah, it seems some past features were simply removed outright rather than being deprecated as they should be. Could you open a new issue for this, so it doesn't get lost? (If you have/could generate a list of all the features that aren't recognised, but should be, that would be really helpful, but don't worry if not.) I think we'll need to retroactively make these features deprecated. |
Hmm, it's worse than that, because I also rely on not-yet-known features being silently ignored in older compilers. |
@jethrogb: I don't see a strong reason not to make the unknown feature error a deny-by-default lint instead, which would allow you to silence it. If you could make a separate issue for that too, it'll help it not to get lost. |
Fixes #52053, fixes #53032 and address some of the problems noted in #44232 (though not unused features).
There are a few outstanding problems, that I haven't narrowed down yet:
cfg
attributes are not taken into account.