Implement tool_lints by flip1995 · Pull Request #2977 · rust-lang/rust-clippy (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation26 Commits13 Checks0 Files changed
Conversation
This file contains hidden or 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
[ Show hidden characters]({{ revealButtonHref }})
cc #2955
Waiting for rust-lang/rust#52851
The (ui-)tests need to be adapted to the tool lints, by renaming every clippy_lint to clippy::clippy_lint. Also #![feature(tool_lints)] needs to be added to the tests, until tool_lints are stable.
| #[macro_use] |
|---|
| extern crate serde_derive; |
| /// Test that we do not lint for unused underscores in a `MacroAttribute` |
| /// expansion |
| #[deny(used_underscore_binding)] |
| #[deny(clippy::used_underscore_binding)] |
| #[derive(Deserialize)] |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FIXMEFIXED: I get a weird error here:
error: proc-macro derive panicked
--> tests/run-pass/used_underscore_binding_macro.rs:7:10
|
7 | #[derive(Deserialize)]
| ^^^^^^^^^^^
|
= help: message: called Result::unwrap() on an Err value: "failed to parse derive input: "/// Test that we do not lint for unused underscores in a MacroAttribute\n/// expansion\n#[deny(clippy::used_underscore_binding)]\nstruct MacroAttributesTest {\n _foo: u32,\n}""
That's because somehow syn fails to parse the new tool_lints...
See serde and syn
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we just need to update our dependency version requirement to the most recent one, then it'll work
| @@ -2,7 +2,7 @@ |
|---|
| //! floating-point literal expressions. |
| use rustc::lint::*; |
| use rustc::{declare_lint, lint_array}; |
| use rustc::{declare_tool_lint, lint_array}; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we even need this imported here? Shouldn't declare_clippy_lint be enough? Maybe we need to import it in the expansion of declare_clippy_lint
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we use the declare_clippy_lint! which expands to ...declare_(tool_)lint!....
Maybe we need to import it in the expansion of declare_clippy_lint
That would probably be cleaner.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or not: If we use declare_clippy_lint multiple times in one file we would get multiple use rustc::declare_tool_lint. And that happens pretty often.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we just call rustc::declare_tool_lint! instead of importing it?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should work. I'll try this.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use rustc::declare_tool_lint! in the macro_rule, we have to import use rustc in each file, or else:
error[E0433]: failed to resolve. Use of undeclared type or module rustc
--> clippy_lints/src/lib.rs:24:9
|
24 | rustc::declare_tool_lint! { pub clippy::$name, Warn, $description, report_in_external_macro: true }
| ^^^^^ Use of undeclared type or module rustc
|
::: clippy_lints/src/bit_mask.rs:87:1
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:( ok, lets leave it as is then and punt to the future
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request
Make the tool_lints actually usable
Necessary for rust-lang/rust-clippy#2955 and rust-lang/rust-clippy#2977
This PR makes it possible for lint tools (at the moment only for Clippy) to implement the tool_lints, like it was documented in rust-lang#52018.
Because the declare_lint macro is pretty cluttered right now, there was not really a good way to add the tool_name as an additional argument of the macro. That's why I chose to introduce the new declare_tool_lint macro.
The switch from &str to String in the lint_groups FxHashMap is because I got weird error messages in the check_lint_name method. And the by_name field of the LintStore also uses String.
What comes with this PR:
If this PR lands and Clippy switches to the tool_lints, the currently used methods
#[cfg_attr(feature = "cargo-clippy", allow(clippy_lint))]
#[allow(unknown_lints, clippy_lint)]to allow/warn/deny/forbid Clippy lints, won't have any effects anymore, but also won't produce a warning. That is because the name of clippy_lint will then be clippy::clippy_lint. (Maybe we can add a clippy lint to search for cfg_attr appearances with the cargo-clippy feature?)
r? @oli-obk
Does rustc allow you to use the old way of specifying these?
I feel like we should have a week or two where rustc will match both clippy::foo and foo and warn on the latter. Or we can register deprecated aliases or something here. Not sure how to best make this work.
pietroalbini added a commit to pietroalbini/rust that referenced this pull request
Make the tool_lints actually usable
Necessary for rust-lang/rust-clippy#2955 and rust-lang/rust-clippy#2977
This PR makes it possible for lint tools (at the moment only for Clippy) to implement the tool_lints, like it was documented in rust-lang#52018.
Because the declare_lint macro is pretty cluttered right now, there was not really a good way to add the tool_name as an additional argument of the macro. That's why I chose to introduce the new declare_tool_lint macro.
The switch from &str to String in the lint_groups FxHashMap is because I got weird error messages in the check_lint_name method. And the by_name field of the LintStore also uses String.
What comes with this PR:
If this PR lands and Clippy switches to the tool_lints, the currently used methods
#[cfg_attr(feature = "cargo-clippy", allow(clippy_lint))]
#[allow(unknown_lints, clippy_lint)]to allow/warn/deny/forbid Clippy lints, won't have any effects anymore, but also won't produce a warning. That is because the name of clippy_lint will then be clippy::clippy_lint. (Maybe we can add a clippy lint to search for cfg_attr appearances with the cargo-clippy feature?)
r? @oli-obk
You can still use the declare_lint! macro and nothing would change. If we switch to the declare_tool_lint! macro the old ones won't work anymore. That is because I implemented this macro to just concat the tool name onto the lint name. This means there is no good way for rustc to allow both IMO.
I could have added a double check for tool_name::lint_name and just lint_name to the compiler and remove it in several weeks again. But then most of the handling of the tool_attributes and tool_lints should be done by the tools and not the compiler, like the RFC stated.
My solution to this would be to write a lint for occurrences of #[cfg_attr(feature="cargo-clippy", ...) before making this change (And maybe wait a week or two).
We also need a lint for unknown_clippy_lints in the near future, after landing this.
...I should list this in #2955
Whoops my update script for the ui-tests slipped a little bit there. I'll reduce the number of lines changed a bit 😄
@flip1995 I meant that we can introduce such a warning into the rustc codebase itself, yeah? "If you use a lint that shares a name with a tool lint, here's a warning"
for tool lint groups we can temporarily make the registration API ask for a deprecated fallback name
We're the only consumers of this API, we can make changes like this 😄
I think I finished the ui-tests update. I looked through every .stderr file and github reports +2,243 −2,243 for the two stderr commits. 🎉
Yes, we could lint in the rustc codebase for lints that are from tools aka Clippy. But this could only detect unscoped lints from Clippy if we compile the code with Clippy in the first place. In that case also Clippy can lint for usage of the unscoped lint names. (I'm sorry, if I'm missing the point of doing this in the compiler. Doing the ui-tests update was pretty annoying exhausting 😄)
for tool lint groups we can temporarily make the registration API ask for a deprecated fallback name
That makes sense to me.
But this could only detect unscoped lints from Clippy if we compile the code with Clippy in the first place. In that case also Clippy can lint for usage of the unscoped lint names.
Yes, this is only if you're running with clippy.
(I'm sorry, if I'm missing the point of doing this in the compiler. Doing the ui-tests update was pretty annoying exhausting smile)
Doing this in the compiler lets us have the lint allow still work. Otherwise the clippy user will get the deprecation warnings, as well as tons of unknown lint warnings and tons of silenced clippy warnings that get unsilenced.
Ahh, I got it now! Thanks for the clarification!
Travis should be green with the next nightly now. Do not merge this until the backwards compatibility is implemented in rustc.
Yeah, I currently working on the backwards compatibility of the tool_lints. After that I'll have to make adjustments here anyway. Also it will need a rebase. But I want to do this all at once.
ping @flip1995 any update on the backwards compat?
I will open a PR in rust-lang/rust by the end of this week
It's almost finished, just need to prettify it a little. Sorry for the delay.
Rebased and pushed, and pinned daa4f0a to the tool-lints branch on this repo. When rust-lang/rust#53762 lands, please ensure that that commit gets into clippy master via git merge (NOT rebase)
WIP: Implement tool_lints Implement tool_lints
bors added a commit to rust-lang/rust that referenced this pull request
Backwards compatibility for tool/clippy lints
cc #44690 cc rust-lang/rust-clippy#2977 (comment)
This is the next step towards tool_lints.
This makes Clippy lints still work without scoping, but will warn and suggest the new scoped name. This warning will only appear if the code is checked with Clippy itself.
There is still an issue with using the old lint name in inner attributes. For inner attributes the warning gets emitted twice. I'm currently not really sure why this happens, but will try to fix this ASAP.
r? @Manishearth
rust-lang/rust#53762 landed. I'll do a local test run and catch any merge issues that may have happened