Rollup merge of rust-lang#52851 - flip1995:tool_lints, r=oli-obk

pietroalbini · web-flow · commit 5fb7c65f545c · 2018-08-01T10:12:51.000+02:00
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
diff --git a/src/librustc/lint/context.rs b/src/librustc/lint/context.rs
@@ -42,7 +42,7 @@ use util::nodemap::FxHashMap;
 use std::default::Default as StdDefault;
 use syntax::ast;
 use syntax::edition;
-use syntax_pos::{MultiSpan, Span};
+use syntax_pos::{MultiSpan, Span, symbol::LocalInternedString};
 use errors::DiagnosticBuilder;
 use hir;
 use hir::def_id::LOCAL_CRATE;
@@ -133,6 +133,12 @@ pub enum CheckLintNameResult<'a> {
     /// The lint is either renamed or removed. This is the warning
     /// message, and an optional new name (`None` if removed).
     Warning(String, Option<String>),
+    /// The lint is from a tool. If the Option is None, then either
+    /// the lint does not exist in the tool or the code was not
+    /// compiled with the tool and therefore the lint was never
+    /// added to the `LintStore`. Otherwise the `LintId` will be
+    /// returned as if it where a rustc lint.
+    Tool(Option<&'a [LintId]>),
 }
 
 impl LintStore {
@@ -288,14 +294,15 @@ impl LintStore {
                                    sess: &Session,
                                    lint_name: &str,
                                    level: Level) {
-        let db = match self.check_lint_name(lint_name) {
+        let db = match self.check_lint_name(lint_name, None) {
             CheckLintNameResult::Ok(_) => None,
             CheckLintNameResult::Warning(ref msg, _) => {
                 Some(sess.struct_warn(msg))
             },
             CheckLintNameResult::NoLint => {
                 Some(struct_err!(sess, E0602, "unknown lint: `{}`", lint_name))
             }
+            CheckLintNameResult::Tool(_) => unreachable!(),
         };
 
         if let Some(mut db) = db {
@@ -319,26 +326,41 @@ impl LintStore {
     /// it emits non-fatal warnings and there are *two* lint passes that
     /// inspect attributes, this is only run from the late pass to avoid
     /// printing duplicate warnings.
-    pub fn check_lint_name(&self, lint_name: &str) -> CheckLintNameResult {
-        match self.by_name.get(lint_name) {
-            Some(&Renamed(ref new_name, _)) => {
-                CheckLintNameResult::Warning(
-                    format!("lint `{}` has been renamed to `{}`", lint_name, new_name),
-                    Some(new_name.to_owned())
-                )
-            },
-            Some(&Removed(ref reason)) => {
-                CheckLintNameResult::Warning(
-                    format!("lint `{}` has been removed: `{}`", lint_name, reason),
-                    None
-                )
-            },
-            None => {
-                match self.lint_groups.get(lint_name) {
-                    None => CheckLintNameResult::NoLint,
-                    Some(ids) => CheckLintNameResult::Ok(&ids.0),
-                }
+    pub fn check_lint_name(
+        &self,
+        lint_name: &str,
+        tool_name: Option<LocalInternedString>,
+    ) -> CheckLintNameResult {
+        let complete_name = if let Some(tool_name) = tool_name {
+            format!("{}::{}", tool_name, lint_name)
+        } else {
+            lint_name.to_string()
+        };
+        if let Some(_) = tool_name {
+            match self.by_name.get(&complete_name) {
+                None => match self.lint_groups.get(&*complete_name) {
+                    None => return CheckLintNameResult::Tool(None),
+                    Some(ids) => return CheckLintNameResult::Tool(Some(&ids.0)),
+                },
+                Some(&Id(ref id)) => return CheckLintNameResult::Tool(Some(slice::from_ref(id))),
+                // If the lint was registered as removed or renamed by the lint tool, we don't need
+                // to treat tool_lints and rustc lints different and can use the code below.
+                _ => {}
             }
+        }
+        match self.by_name.get(&complete_name) {
+            Some(&Renamed(ref new_name, _)) => CheckLintNameResult::Warning(
+                format!("lint `{}` has been renamed to `{}`", lint_name, new_name),
+                Some(new_name.to_owned()),
+            ),
+            Some(&Removed(ref reason)) => CheckLintNameResult::Warning(
+                format!("lint `{}` has been removed: `{}`", lint_name, reason),
+                None,
+            ),
+            None => match self.lint_groups.get(&*complete_name) {
+                None => CheckLintNameResult::NoLint,
+                Some(ids) => CheckLintNameResult::Ok(&ids.0),
+            },
             Some(&Id(ref id)) => CheckLintNameResult::Ok(slice::from_ref(id)),
         }
     }
diff --git a/src/librustc/lint/levels.rs b/src/librustc/lint/levels.rs
@@ -227,17 +227,18 @@ impl<'a> LintLevelsBuilder<'a> {
                         continue
                     }
                 };
-                if let Some(lint_tool) = word.is_scoped() {
-                    if !self.sess.features_untracked().tool_lints {
+                let tool_name = if let Some(lint_tool) = word.is_scoped() {
+                    let gate_feature = !self.sess.features_untracked().tool_lints;
+                    let known_tool = attr::is_known_lint_tool(lint_tool);
+                    if gate_feature {
                         feature_gate::emit_feature_err(&sess.parse_sess,
                                                        "tool_lints",
                                                        word.span,
                                                        feature_gate::GateIssue::Language,
                                                        &format!("scoped lint `{}` is experimental",
                                                                 word.ident));
                     }
-
-                    if !attr::is_known_lint_tool(lint_tool) {
+                    if !known_tool {
                         span_err!(
                             sess,
                             lint_tool.span,
@@ -247,17 +248,37 @@ impl<'a> LintLevelsBuilder<'a> {
                         );
                     }
 
-                    continue
-                }
+                    if gate_feature || !known_tool {
+                        continue
+                    }
+
+                    Some(lint_tool.as_str())
+                } else {
+                    None
+                };
                 let name = word.name();
-                match store.check_lint_name(&name.as_str()) {
+                match store.check_lint_name(&name.as_str(), tool_name) {
                     CheckLintNameResult::Ok(ids) => {
                         let src = LintSource::Node(name, li.span);
                         for id in ids {
                             specs.insert(*id, (level, src));
                         }
                     }
 
+                    CheckLintNameResult::Tool(result) => {
+                        if let Some(ids) = result {
+                            let complete_name = &format!("{}::{}", tool_name.unwrap(), name);
+                            let src = LintSource::Node(Symbol::intern(complete_name), li.span);
+                            for id in ids {
+                                specs.insert(*id, (level, src));
+                            }
+                        }
+                        // If Tool(None) is returned, then either the lint does not exist in the
+                        // tool or the code was not compiled with the tool and therefore the lint
+                        // was never added to the `LintStore`. To detect this is the responsibility
+                        // of the lint tool.
+                    }
+
                     _ if !self.warn_about_weird_lints => {}
 
                     CheckLintNameResult::Warning(msg, renamed) => {
@@ -298,7 +319,7 @@ impl<'a> LintLevelsBuilder<'a> {
                         if name.as_str().chars().any(|c| c.is_uppercase()) {
                             let name_lower = name.as_str().to_lowercase().to_string();
                             if let CheckLintNameResult::NoLint =
-                                    store.check_lint_name(&name_lower) {
+                                    store.check_lint_name(&name_lower, tool_name) {
                                 db.emit();
                             } else {
                                 db.span_suggestion_with_applicability(
diff --git a/src/librustc/lint/mod.rs b/src/librustc/lint/mod.rs
@@ -139,6 +139,26 @@ macro_rules! declare_lint {
     );
 }
 
+#[macro_export]
+macro_rules! declare_tool_lint {
+    ($vis: vis $tool: ident ::$NAME: ident, $Level: ident, $desc: expr) => (
+        declare_tool_lint!{$vis $tool::$NAME, $Level, $desc, false}
+    );
+    ($vis: vis $tool: ident ::$NAME: ident, $Level: ident, $desc: expr,
+     report_in_external_macro: $rep: expr) => (
+         declare_tool_lint!{$vis $tool::$NAME, $Level, $desc, $rep}
+    );
+    ($vis: vis $tool: ident ::$NAME: ident, $Level: ident, $desc: expr, $external: expr) => (
+        $vis static $NAME: &$crate::lint::Lint = &$crate::lint::Lint {
+            name: &concat!(stringify!($tool), "::", stringify!($NAME)),
+            default_level: $crate::lint::$Level,
+            desc: $desc,
+            edition_lint_opts: None,
+            report_in_external_macro: $external,
+        };
+    );
+}
+
 /// Declare a static `LintArray` and return it as an expression.
 #[macro_export]
 macro_rules! lint_array {
diff --git a/src/test/ui-fulldeps/auxiliary/lint_tool_test.rs b/src/test/ui-fulldeps/auxiliary/lint_tool_test.rs
@@ -0,0 +1,48 @@
+// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+#![feature(plugin_registrar)]
+#![feature(box_syntax, rustc_private)]
+#![feature(macro_vis_matcher)]
+#![feature(macro_at_most_once_rep)]
+
+extern crate syntax;
+
+// Load rustc as a plugin to get macros
+#[macro_use]
+extern crate rustc;
+extern crate rustc_plugin;
+
+use rustc::lint::{EarlyContext, LintContext, LintPass, EarlyLintPass,
+                  LintArray};
+use rustc_plugin::Registry;
+use syntax::ast;
+declare_tool_lint!(pub clippy::TEST_LINT, Warn, "Warn about stuff");
+
+struct Pass;
+
+impl LintPass for Pass {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(TEST_LINT)
+    }
+}
+
+impl EarlyLintPass for Pass {
+    fn check_item(&mut self, cx: &EarlyContext, it: &ast::Item) {
+        if it.ident.name == "lintme" {
+            cx.span_lint(TEST_LINT, it.span, "item is named 'lintme'");
+        }
+    }
+}
+
+#[plugin_registrar]
+pub fn plugin_registrar(reg: &mut Registry) {
+    reg.register_early_lint_pass(box Pass);
+}
diff --git a/src/test/ui-fulldeps/lint_tool_test.rs b/src/test/ui-fulldeps/lint_tool_test.rs
@@ -0,0 +1,24 @@
+// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// run-pass
+// aux-build:lint_tool_test.rs
+// ignore-stage1
+#![feature(plugin)]
+#![feature(tool_lints)]
+#![plugin(lint_tool_test)]
+#![allow(dead_code)]
+
+fn lintme() { } //~ WARNING item is named 'lintme'
+
+#[allow(clippy::test_lint)]
+pub fn main() {
+    fn lintme() { }
+}
diff --git a/src/test/ui-fulldeps/lint_tool_test.stderr b/src/test/ui-fulldeps/lint_tool_test.stderr
@@ -0,0 +1,8 @@
+warning: item is named 'lintme'
+  --> $DIR/lint_tool_test.rs:19:1
+   |
+LL | fn lintme() { } //~ WARNING item is named 'lintme'
+   | ^^^^^^^^^^^^^^^
+   |
+   = note: #[warn(clippy::test_lint)] on by default
+
