-
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
Deny rustc::internal lints for rustdoc and clippy #80573
Conversation
jyn514
commented
Jan 1, 2021
•
edited
Loading
edited
- Fix rustc::internal lints for rustdoc
- Deny internal lints only for rustdoc and clippy (previously the lints were ignored for clippy because -Zunstable-options didn't get passed)
This comment has been minimized.
This comment has been minimized.
r? @ollie27 (rust-highfive has picked a reviewer for you, use r? to override) |
I changed this to be more general and deny the lints right away - I can split it into two PRs if you like. |
This comment has been minimized.
This comment has been minimized.
The job Click to see the possible cause of the failure (guessed by this bot)
|
Hmm, this errors out in |
I'm good with adding |
The job Click to see the possible cause of the failure (guessed by this bot)
|
The failure seems spurious, not sure why it wouldn't have libstd for other targets installed. |
The job Click to see the possible cause of the failure (guessed by this bot)
|
Hmm, it seems this affects rustfmt too.
@calebcartwright how do you feel about fixing those lints for rustfmt? Or would you prefer I ignore them? |
Admittedly haven't read the full thread/linked issues, but I feel like rustfmt should be ignored/excluded from this. Although we periodically run clippy and selectively apply fixes, it's not really part of our inner dev loop and not a fixed gate, and I don't foresee that changing any time soon. If we were to include the internal lints against rustfmt here then we'll inevitably end up in a scenario where we don't find out about a lint failure til an attempted submod bump which would then require a restart of our upgrade dance. Also, since rustfmt utilizes the rustc internals via the auto publish crates, I really don't want to increase our dependence on said internals unless absolutely necessary or unless the benefits significantly outweigh the drawbacks. Retroactively dealing with breaking rustc internal changes in rustfmt is already painful enough, and that pain gets exacerbated the more we couple ourselves to those internals. |
I think it would be best to make internal lints opt-in rather than opt-out, if that's somehow possible. |
This comment has been minimized.
This comment has been minimized.
The job Click to see the possible cause of the failure (guessed by this bot)
|
It looks like CI is currently failing. Since unlike rustc, rustdoc and clippy are both currently just one crate (and we don't need to lint their dependencies) maybe just easier to directly add the two attributes to their lib.rs? |
Hmm, that will work now, but will break after #80524, which was the original motivation. I'll see if I can improve #80524 not to need bootstrap changes, since I agree |
Hm I admit I haven't followed that PR closely, if this helps it somehow I can take a look. Note you want to use warn, not deny - we deny warnings in bootstrap and that should include these lints, right? |
Alright, this is finally working. It
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks! @bors: r+ |
@GuillaumeGomez bors doesn't react to commands in review comments, only in normal comments. |
It was a normal comment. I didn't post a review comment. But let's try again. @bors: r+ |
📌 Commit 0797ffe has been approved by |
Rollup of 10 pull requests Successful merges: - rust-lang#80573 (Deny rustc::internal lints for rustdoc and clippy) - rust-lang#81173 (Expand docs on Iterator::intersperse) - rust-lang#81194 (Stabilize std::panic::panic_any.) - rust-lang#81202 (Don't prefix 0x for each segments in `dbg!(Ipv6)`) - rust-lang#81225 (Make 'docs' nullable in rustdoc-json output) - rust-lang#81227 (Remove doctree::StructType) - rust-lang#81233 (Document why not use concat! in dbg! macro) - rust-lang#81236 (Gracefully handle loop labels missing leading `'` in different positions) - rust-lang#81241 (Turn alloc's force_expr macro into a regular macro_rules.) - rust-lang#81242 (Enforce statically that `MIN_NON_ZERO_CAP` is calculated at compile time) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup