Implement tool_lints #2977

flip1995 · 2018-07-30T10:17:30Z

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.

flip1995 · 2018-07-30T10:22:03Z

tests/run-pass/used_underscore_binding_macro.rs

 #[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)]


~~FIXME~~FIXED: 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

we just need to update our dependency version requirement to the most recent one, then it'll work

oli-obk · 2018-07-30T10:51:02Z

clippy_lints/src/literal_representation.rs

@@ -2,7 +2,7 @@
 //! floating-point literal expressions.

 use rustc::lint::*;
-use rustc::{declare_lint, lint_array};
+use rustc::{declare_tool_lint, lint_array};


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

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.

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.

can we just call rustc::declare_tool_lint! instead of importing it?

That should work. I'll try this.

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

:( ok, lets leave it as is then and punt to the future

@oli-obk

Make the tool_lints actually usable cc rust-lang#44690 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 ```rust #[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

Manishearth · 2018-08-01T07:08:15Z

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.

@oli-obk

Make the tool_lints actually usable cc rust-lang#44690 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 ```rust #[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

flip1995 · 2018-08-01T08:24:03Z

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

flip1995 · 2018-08-01T15:02:20Z

Whoops my update script for the ui-tests slipped a little bit there. I'll reduce the number of lines changed a bit 😄

Manishearth · 2018-08-01T15:45:20Z

@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"

Manishearth · 2018-08-01T15:46:07Z

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 😄

flip1995 · 2018-08-01T19:39:02Z

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. 🎉

flip1995 · 2018-08-01T20:03:01Z

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.

Manishearth · 2018-08-01T20:12:55Z

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.

flip1995 · 2018-08-01T20:28:28Z

Ahh, I got it now! Thanks for the clarification!

flip1995 · 2018-08-01T21:17:00Z

Travis should be green with the next nightly now. Do not merge this until the backwards compatibility is implemented in rustc.

phansch · 2018-08-06T11:52:22Z

@flip1995 looks like some conflicts appeared

flip1995 · 2018-08-06T15:41:15Z

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.

oli-obk · 2018-08-23T10:34:11Z

ping @flip1995 any update on the backwards compat?

flip1995 · 2018-08-23T10:53:35Z

I will open a PR in rust-lang/rust by the end of this week

flip1995 · 2018-08-27T12:40:12Z

It's almost finished, just need to prettify it a little. Sorry for the delay.

…t#53762

Manishearth · 2018-08-29T18:10:05Z

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)

@Manishearth

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

Manishearth · 2018-09-01T05:53:06Z

rust-lang/rust#53762 landed. I'll do a local test run and catch any merge issues that may have happened

flip1995 mentioned this pull request Jul 30, 2018

Make the tool_lints actually usable rust-lang/rust#52851

Merged

flip1995 commented Jul 30, 2018

View reviewed changes

oli-obk reviewed Jul 30, 2018

View reviewed changes

flip1995 force-pushed the tool_lints branch from 699f686 to a3c31c4 Compare August 1, 2018 12:11

flip1995 force-pushed the tool_lints branch from 5501d43 to a124642 Compare August 1, 2018 15:50

flip1995 force-pushed the tool_lints branch from 3c97aae to 4b79a5e Compare August 1, 2018 21:05

flip1995 closed this Aug 2, 2018

flip1995 reopened this Aug 2, 2018

phansch closed this Aug 6, 2018

phansch reopened this Aug 6, 2018

flip1995 force-pushed the tool_lints branch 2 times, most recently from a3aab0b to cbd178b Compare August 28, 2018 09:54

flip1995 force-pushed the tool_lints branch 3 times, most recently from 846fb4b to b4d31f4 Compare August 28, 2018 12:34

flip1995 mentioned this pull request Aug 28, 2018

Backwards compatibility for tool/clippy lints rust-lang/rust#53762

Merged

flip1995 added 11 commits August 29, 2018 11:08

Switch to tool_lints

392235d

Switch to declare_tool_lint macro

8c07772

Adapt documentation to the tool_lints

83baf8f

Adapt scripts to the tool_lints

bb49b31

Adapt ui-tests to the tool_lints

1b6f605

Adapt the *.stderr files of the ui-tests to the tool_lints

e9af09c

Adapt run-pass tests to the tool_lints

ea43fed

Adapt ui-toml-tests to the tool_lints

cfd4c53

Adapt codebase to the tool_lints

f3bb161

Fix some rebase fallout

d1f2f0c

Implement backwards compatibility changes introduced by rust-lang/rus…

daa4f0a

…t#53762

Manishearth force-pushed the tool_lints branch from 4f1e3f4 to daa4f0a Compare August 29, 2018 18:09

flip1995 changed the title ~~WIP: Implement tool_lints~~ Implement tool_lints Aug 30, 2018

Manishearth added 2 commits August 31, 2018 00:34

declare_lint -> declare_tool_lint

20dfaf7

Fix ptr offset tests

9abf6fc

Manishearth merged commit c81d70e into rust-lang:master Sep 1, 2018

flip1995 deleted the tool_lints branch September 6, 2018 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement tool_lints #2977

Implement tool_lints #2977

flip1995 commented Jul 30, 2018 •

edited

Loading

flip1995 Jul 30, 2018 •

edited

Loading

oli-obk Aug 1, 2018

oli-obk Jul 30, 2018

flip1995 Jul 30, 2018

flip1995 Jul 30, 2018

oli-obk Jul 30, 2018

flip1995 Jul 30, 2018

flip1995 Jul 30, 2018

oli-obk Jul 30, 2018

Manishearth commented Aug 1, 2018

flip1995 commented Aug 1, 2018 •

edited

Loading

flip1995 commented Aug 1, 2018

Manishearth commented Aug 1, 2018

Manishearth commented Aug 1, 2018

flip1995 commented Aug 1, 2018

flip1995 commented Aug 1, 2018

Manishearth commented Aug 1, 2018

flip1995 commented Aug 1, 2018

flip1995 commented Aug 1, 2018

phansch commented Aug 6, 2018

flip1995 commented Aug 6, 2018

oli-obk commented Aug 23, 2018

flip1995 commented Aug 23, 2018

flip1995 commented Aug 27, 2018

Manishearth commented Aug 29, 2018

Manishearth commented Sep 1, 2018

Implement tool_lints #2977

Implement tool_lints #2977

Conversation

flip1995 commented Jul 30, 2018 • edited Loading

flip1995 Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth commented Aug 1, 2018

flip1995 commented Aug 1, 2018 • edited Loading

flip1995 commented Aug 1, 2018

Manishearth commented Aug 1, 2018

Manishearth commented Aug 1, 2018

flip1995 commented Aug 1, 2018

flip1995 commented Aug 1, 2018

Manishearth commented Aug 1, 2018

flip1995 commented Aug 1, 2018

flip1995 commented Aug 1, 2018

phansch commented Aug 6, 2018

flip1995 commented Aug 6, 2018

oli-obk commented Aug 23, 2018

flip1995 commented Aug 23, 2018

flip1995 commented Aug 27, 2018

Manishearth commented Aug 29, 2018

Manishearth commented Sep 1, 2018

flip1995 commented Jul 30, 2018 •

edited

Loading

flip1995 Jul 30, 2018 •

edited

Loading

flip1995 commented Aug 1, 2018 •

edited

Loading