Make the tool_lints actually usable #52851

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

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 #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

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

src/librustc/lint/levels.rs

+                        }
+                        //FIXME: if Tool(None) is returned than the lint either does not exist in
+                        //the lint tool or the code doesn't get compiled with the lint tool and
+                        //therefore the lint cannot exist.


I'm not sure what to do here... Can we somehow detect if we currently use the clippy_driver or the rustc_driver to find out if the lint does not exist or wasn't even added in the first place?

The driver could register itself as a tool. Not sure where that information belongs, maybe a new field in the Session?

I think tool attributes are required to be checked by the tool. So maybe instead we leave the "unknown lint" error to clippy?

Leaving it to the tool/Clippy was also my idea. We basically need a lint, which looks for allow(clippy::_)/... and checks if the lint exists, I think.

So... I guess just turn the FIXME comment into a comment explaining that

Ok will do. And I will squash these commits while doing this.

oli-obk · 2018-07-30T10:46:10Z

src/librustc/lint/levels.rs

+                        }
+                        //FIXME: if Tool(None) is returned than the lint either does not exist in
+                        //the lint tool or the code doesn't get compiled with the lint tool and
+                        //therefore the lint cannot exist.


The driver could register itself as a tool. Not sure where that information belongs, maybe a new field in the Session?

oli-obk · 2018-07-30T10:47:08Z

src/librustc/lint/levels.rs

+                        }
+                        //FIXME: if Tool(None) is returned than the lint either does not exist in
+                        //the lint tool or the code doesn't get compiled with the lint tool and
+                        //therefore the lint cannot exist.


I think tool attributes are required to be checked by the tool. So maybe instead we leave the "unknown lint" error to clippy?

oli-obk · 2018-07-30T10:48:30Z

src/librustc/lint/context.rs

@@ -69,7 +69,7 @@ pub struct LintStore {

    /// Map of registered lint groups to what lints they expand to. The bool
    /// is true if the lint group was added by a plugin.
-    lint_groups: FxHashMap<&'static str, (Vec<LintId>, bool)>,
+    lint_groups: FxHashMap<String, (Vec<LintId>, bool)>,


What's the motivation for this change?

In the check_lint_name function I copied the check for lint_groups in the match of the tool_lints:
https://github.com/rust-lang/rust/pull/52851/files#diff-771d6215d1fb5e7f5503514f9895f7bdR341

This lead to the error

error[E0277]: the trait bound `&str: std::borrow::Borrow<std::string::String>` is not satisfied --> librustc/lint/context.rs:341:48 | 341 | None => match self.lint_groups.get(&complete_name) { | ^^^ the trait `std::borrow::Borrow<std::string::String>` is not implemented for `&str`

The weird part was that this error appeared only once and not in both cases. But I can't reproduce this anymore. Switching to String for the FxHashMap keys solved the problem.

While thinking about this a little longer and not at midnight, I found an easier solution...

flip1995 · 2018-07-30T13:37:55Z

I will squash the last 3 commits before merging.

oli-obk

r=me with comment adjusted and fixup commits squashed

oli-obk · 2018-07-30T13:55:31Z

src/librustc/lint/levels.rs

+                        }
+                        //FIXME: if Tool(None) is returned than the lint either does not exist in
+                        //the lint tool or the code doesn't get compiled with the lint tool and
+                        //therefore the lint cannot exist.


So... I guess just turn the FIXME comment into a comment explaining that

flip1995 · 2018-07-30T15:33:56Z

@bors: r=oli-obk

bors · 2018-07-30T15:33:57Z

@flip1995: 🔑 Insufficient privileges: Not in reviewers

oli-obk · 2018-07-30T15:39:18Z

@bors r+

bors · 2018-07-30T15:39:19Z

📌 Commit 7b9388b has been approved by oli-obk

@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

@ghost

Rollup of 31 pull requests Successful merges: - #52332 (dead-code lint: say "constructed" for structs) - #52340 (Document From trait implementations for OsStr, OsString, CString, and CStr) - #52628 (Cleanup some rustdoc code) - #52732 (Remove unstable and deprecated APIs) - #52745 (Update clippy to latest master) - #52756 (rustc: Disallow machine applicability in foreign macros) - #52771 (Clarify thread::park semantics) - #52810 ([NLL] Don't make "fake" match variables mutable) - #52821 (pretty print for std::collections::vecdeque) - #52822 (Fix From<LocalWaker>) - #52824 (Fix -Wpessimizing-move warnings in rustllvm/PassWrapper) - #52831 (remove references to AUTHORS.txt file) - #52835 (Fix Alias intra doc ICE) - #52842 (update comment) - #52846 (Add timeout to use of `curl` in bootstrap.py.) - #52851 (Make the tool_lints actually usable) - #52853 (Improve bootstrap help on stages) - #52859 (Use Vec::extend in SmallVec::extend when applicable) - #52861 (Add targets for HermitCore (https://hermitcore.org) to the Rust compiler and port libstd to it.) - #52867 (releases.md: fix 2 typos) - #52870 (Implement Unpin for FutureObj and LocalFutureObj) - #52876 (run-pass/const-endianness: negate before to_le()) - #52878 (Fix wrong issue number in the test name) - #52883 (Include lifetime in mutability suggestion in NLL messages) - #52904 (NLL: sort diagnostics by span) - #52905 (Fix a typo in unsize.rs) - #52907 (NLL: On "cannot move out of type" error, print original before rewrite) - #52908 (Use SetLenOnDrop in Vec::truncate()) - #52914 (Only run the sparc-abi test on sparc) - #52918 (Backport 1.27.2 release notes) - #52929 (Update compatibility note for 1.28.0 to be correct) Failed merges: - #52758 (Cleanup for librustc::session) - #52799 (Use BitVector for global sets of AttrId) r? @ghost

@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

@ghost

Rollup of 30 pull requests Successful merges: - #52340 (Document From trait implementations for OsStr, OsString, CString, and CStr) - #52628 (Cleanup some rustdoc code) - #52732 (Remove unstable and deprecated APIs) - #52745 (Update clippy to latest master) - #52771 (Clarify thread::park semantics) - #52778 (Improve readability of serialize.rs) - #52810 ([NLL] Don't make "fake" match variables mutable) - #52821 (pretty print for std::collections::vecdeque) - #52822 (Fix From<LocalWaker>) - #52824 (Fix -Wpessimizing-move warnings in rustllvm/PassWrapper) - #52825 (Make sure #47772 does not regress) - #52831 (remove references to AUTHORS.txt file) - #52842 (update comment) - #52846 (Add timeout to use of `curl` in bootstrap.py.) - #52851 (Make the tool_lints actually usable) - #52853 (Improve bootstrap help on stages) - #52859 (Use Vec::extend in SmallVec::extend when applicable) - #52861 (Add targets for HermitCore (https://hermitcore.org) to the Rust compiler and port libstd to it.) - #52867 (releases.md: fix 2 typos) - #52870 (Implement Unpin for FutureObj and LocalFutureObj) - #52876 (run-pass/const-endianness: negate before to_le()) - #52878 (Fix wrong issue number in the test name) - #52883 (Include lifetime in mutability suggestion in NLL messages) - #52888 (Use suggestions for shell format arguments) - #52904 (NLL: sort diagnostics by span) - #52905 (Fix a typo in unsize.rs) - #52907 (NLL: On "cannot move out of type" error, print original before rewrite) - #52914 (Only run the sparc-abi test on sparc) - #52918 (Backport 1.27.2 release notes) - #52929 (Update compatibility note for 1.28.0 to be correct) Failed merges: r? @ghost

Introduce the declare_tool_lint macro

4b466ee

rust-highfive assigned oli-obk Jul 30, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 30, 2018

flip1995 commented Jul 30, 2018

View reviewed changes

flip1995 mentioned this pull request Jul 30, 2018

Implement tool_lints rust-lang/rust-clippy#2977

Merged

oli-obk reviewed Jul 30, 2018

View reviewed changes

oli-obk approved these changes Jul 30, 2018

View reviewed changes

flip1995 added 3 commits July 30, 2018 16:10

Check if the lint_name is from a tool and if the tool_lint exists

57c7742

Add a test for the declare_tool_lint macro

d4ff949

Explain that the tool is responsible for unknown tool_lints

7b9388b

flip1995 force-pushed the tool_lints branch from a7f7e88 to 7b9388b Compare July 30, 2018 14:12

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 30, 2018

Mark-Simulacrum mentioned this pull request Jul 31, 2018

Rollup of 21 pull requests #52902

Closed

Mark-Simulacrum mentioned this pull request Aug 1, 2018

Rollup of 31 pull requests #52931

Closed

pietroalbini mentioned this pull request Aug 1, 2018

Rollup of 30 pull requests #52937

Merged

bors merged commit 7b9388b into rust-lang:master Aug 1, 2018

flip1995 deleted the tool_lints branch August 1, 2018 11:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the tool_lints actually usable #52851

Make the tool_lints actually usable #52851

flip1995 commented Jul 30, 2018 •

edited

Loading

flip1995 Jul 30, 2018 •

edited

Loading

oli-obk Jul 30, 2018

oli-obk Jul 30, 2018

flip1995 Jul 30, 2018

oli-obk Jul 30, 2018

flip1995 Jul 30, 2018

oli-obk Jul 30, 2018

oli-obk Jul 30, 2018

oli-obk Jul 30, 2018

flip1995 Jul 30, 2018

flip1995 commented Jul 30, 2018

oli-obk left a comment

oli-obk Jul 30, 2018

flip1995 commented Jul 30, 2018

bors commented Jul 30, 2018

oli-obk commented Jul 30, 2018

bors commented Jul 30, 2018

Make the tool_lints actually usable #52851

Make the tool_lints actually usable #52851

Conversation

flip1995 commented Jul 30, 2018 • edited Loading

What comes with this PR:

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

Choose a reason for hiding this comment

flip1995 commented Jul 30, 2018

oli-obk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flip1995 commented Jul 30, 2018

bors commented Jul 30, 2018

oli-obk commented Jul 30, 2018

bors commented Jul 30, 2018

flip1995 commented Jul 30, 2018 •

edited

Loading

flip1995 Jul 30, 2018 •

edited

Loading