Auto merge of #53762 - flip1995:tool_lints, r=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

Author: bors

Diff for: src/librustc/lint/context.rs

@@ -67,9 +67,10 @@ pub struct LintStore {
     /// Lints indexed by name.
     by_name: FxHashMap<String, TargetLint>,
 
-    /// 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)>,
+    /// Map of registered lint groups to what lints they expand to. The first
+    /// bool is true if the lint group was added by a plugin. The optional string
+    /// is used to store the new names of deprecated lint group names.
+    lint_groups: FxHashMap<&'static str, (Vec<LintId>, bool, Option<&'static str>)>,
 
     /// Extra info for future incompatibility lints, describing the
     /// issue or RFC that caused the incompatibility.
@@ -138,7 +139,7 @@ pub enum CheckLintNameResult<'a> {
     /// 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]>),
+    Tool(Result<&'a [LintId], (Option<&'a [LintId]>, String)>),
 }
 
 impl LintStore {
@@ -221,7 +222,7 @@ impl LintStore {
             let lints = lints.iter().filter(|f| f.edition == Some(*edition)).map(|f| f.id)
                              .collect::<Vec<_>>();
             if !lints.is_empty() {
-                self.register_group(sess, false, edition.lint_name(), lints)
+                self.register_group(sess, false, edition.lint_name(), None, lints)
             }
         }
 
@@ -231,19 +232,35 @@ impl LintStore {
             self.future_incompatible.insert(lint.id, lint);
         }
 
-        self.register_group(sess, false, "future_incompatible", future_incompatible);
-
-
+        self.register_group(
+            sess,
+            false,
+            "future_incompatible",
+            None,
+            future_incompatible,
+        );
     }
 
     pub fn future_incompatible(&self, id: LintId) -> Option<&FutureIncompatibleInfo> {
         self.future_incompatible.get(&id)
     }
 
-    pub fn register_group(&mut self, sess: Option<&Session>,
-                          from_plugin: bool, name: &'static str,
-                          to: Vec<LintId>) {
-        let new = self.lint_groups.insert(name, (to, from_plugin)).is_none();
+    pub fn register_group(
+        &mut self,
+        sess: Option<&Session>,
+        from_plugin: bool,
+        name: &'static str,
+        deprecated_name: Option<&'static str>,
+        to: Vec<LintId>,
+    ) {
+        let new = self
+            .lint_groups
+            .insert(name, (to, from_plugin, None))
+            .is_none();
+        if let Some(deprecated) = deprecated_name {
+            self.lint_groups
+                .insert(deprecated, (vec![], from_plugin, Some(name)));
+        }
 
         if !new {
             let msg = format!("duplicate specification of lint group {}", name);
@@ -336,34 +353,79 @@ impl LintStore {
         } else {
             lint_name.to_string()
         };
+        // If the lint was scoped with `tool::` check if the tool lint exists
         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)),
+                    None => return CheckLintNameResult::Tool(Err((None, String::new()))),
+                    Some(ids) => return CheckLintNameResult::Tool(Ok(&ids.0)),
                 },
-                Some(&Id(ref id)) => return CheckLintNameResult::Tool(Some(slice::from_ref(id))),
+                Some(&Id(ref id)) => return CheckLintNameResult::Tool(Ok(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),
+                format!(
+                    "lint `{}` has been renamed to `{}`",
+                    complete_name, new_name
+                ),
                 Some(new_name.to_owned()),
             ),
             Some(&Removed(ref reason)) => CheckLintNameResult::Warning(
-                format!("lint `{}` has been removed: `{}`", lint_name, reason),
+                format!("lint `{}` has been removed: `{}`", complete_name, reason),
                 None,
             ),
             None => match self.lint_groups.get(&*complete_name) {
-                None => CheckLintNameResult::NoLint,
-                Some(ids) => CheckLintNameResult::Ok(&ids.0),
+                // If neither the lint, nor the lint group exists check if there is a `clippy::`
+                // variant of this lint
+                None => self.check_tool_name_for_backwards_compat(&complete_name, "clippy"),
+                Some(ids) => {
+                    // Check if the lint group name is deprecated
+                    if let Some(new_name) = ids.2 {
+                        let lint_ids = self.lint_groups.get(new_name).unwrap();
+                        return CheckLintNameResult::Tool(Err((
+                            Some(&lint_ids.0),
+                            new_name.to_string(),
+                        )));
+                    }
+                    CheckLintNameResult::Ok(&ids.0)
+                }
             },
             Some(&Id(ref id)) => CheckLintNameResult::Ok(slice::from_ref(id)),
         }
     }
+
+    fn check_tool_name_for_backwards_compat(
+        &self,
+        lint_name: &str,
+        tool_name: &str,
+    ) -> CheckLintNameResult {
+        let complete_name = format!("{}::{}", tool_name, lint_name);
+        match self.by_name.get(&complete_name) {
+            None => match self.lint_groups.get(&*complete_name) {
+                // Now we are sure, that this lint exists nowhere
+                None => CheckLintNameResult::NoLint,
+                Some(ids) => {
+                    // Reaching this would be weird, but lets cover this case anyway
+                    if let Some(new_name) = ids.2 {
+                        let lint_ids = self.lint_groups.get(new_name).unwrap();
+                        return CheckLintNameResult::Tool(Err((
+                            Some(&lint_ids.0),
+                            new_name.to_string(),
+                        )));
+                    }
+                    CheckLintNameResult::Tool(Err((Some(&ids.0), complete_name)))
+                }
+            },
+            Some(&Id(ref id)) => {
+                CheckLintNameResult::Tool(Err((Some(slice::from_ref(id)), complete_name)))
+            }
+            _ => CheckLintNameResult::NoLint,
+        }
+    }
 }
 
 /// Context for lint checking after type checking.

Diff for: src/librustc/lint/levels.rs

@@ -231,12 +231,13 @@ impl<'a> LintLevelsBuilder<'a> {
                     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));
+                        feature_gate::emit_feature_err(
+                            &sess.parse_sess,
+                            "tool_lints",
+                            word.span,
+                            feature_gate::GateIssue::Language,
+                            &format!("scoped lint `{}` is experimental", word.ident),
+                        );
                     }
                     if !known_tool {
                         span_err!(
@@ -249,7 +250,7 @@ impl<'a> LintLevelsBuilder<'a> {
                     }
 
                     if gate_feature || !known_tool {
-                        continue
+                        continue;
                     }
 
                     Some(lint_tool.as_str())
@@ -266,17 +267,52 @@ impl<'a> LintLevelsBuilder<'a> {
                     }
 
                     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));
+                        match result {
+                            Ok(ids) => {
+                                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));
+                                }
+                            }
+                            Err((Some(ids), new_lint_name)) => {
+                                let lint = builtin::RENAMED_AND_REMOVED_LINTS;
+                                let (lvl, src) =
+                                    self.sets
+                                        .get_lint_level(lint, self.cur, Some(&specs), &sess);
+                                let msg = format!(
+                                    "lint name `{}` is deprecated \
+                                     and may not have an effect in the future. \
+                                     Also `cfg_attr(cargo-clippy)` won't be necessary anymore",
+                                    name
+                                );
+                                let mut err = lint::struct_lint_level(
+                                    self.sess,
+                                    lint,
+                                    lvl,
+                                    src,
+                                    Some(li.span.into()),
+                                    &msg,
+                                );
+                                err.span_suggestion_with_applicability(
+                                    li.span,
+                                    "change it to",
+                                    new_lint_name.to_string(),
+                                    Applicability::MachineApplicable,
+                                ).emit();
+
+                                let src = LintSource::Node(Symbol::intern(&new_lint_name), li.span);
+                                for id in ids {
+                                    specs.insert(*id, (level, src));
+                                }
+                            }
+                            Err((None, _)) => {
+                                // If Tool(Err(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 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 => {}

Diff for: src/librustc_driver/driver.rs

@@ -924,8 +924,8 @@ where
             ls.register_late_pass(Some(sess), true, pass);
         }
 
-        for (name, to) in lint_groups {
-            ls.register_group(Some(sess), true, name, to);
+        for (name, (to, deprecated_name)) in lint_groups {
+            ls.register_group(Some(sess), true, name, deprecated_name, to);
         }
 
         *sess.plugin_llvm_passes.borrow_mut() = llvm_passes;

Diff for: src/librustc_lint/lib.rs

@@ -105,7 +105,7 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
 
     macro_rules! add_lint_group {
         ($sess:ident, $name:expr, $($lint:ident),*) => (
-            store.register_group($sess, false, $name, vec![$(LintId::of($lint)),*]);
+            store.register_group($sess, false, $name, None, vec![$(LintId::of($lint)),*]);
             )
     }
 

Diff for: src/librustc_plugin/registry.rs

@@ -53,7 +53,7 @@ pub struct Registry<'a> {
     pub late_lint_passes: Vec<LateLintPassObject>,
 
     #[doc(hidden)]
-    pub lint_groups: FxHashMap<&'static str, Vec<LintId>>,
+    pub lint_groups: FxHashMap<&'static str, (Vec<LintId>, Option<&'static str>)>,
 
     #[doc(hidden)]
     pub llvm_passes: Vec<String>,
@@ -170,8 +170,15 @@ impl<'a> Registry<'a> {
         self.late_lint_passes.push(lint_pass);
     }
     /// Register a lint group.
-    pub fn register_lint_group(&mut self, name: &'static str, to: Vec<&'static Lint>) {
-        self.lint_groups.insert(name, to.into_iter().map(|x| LintId::of(x)).collect());
+    pub fn register_lint_group(
+        &mut self,
+        name: &'static str,
+        deprecated_name: Option<&'static str>,
+        to: Vec<&'static Lint>
+    ) {
+        self.lint_groups.insert(name,
+                                (to.into_iter().map(|x| LintId::of(x)).collect(),
+                                 deprecated_name));
     }
 
     /// Register an LLVM pass.

Diff for: src/test/compile-fail-fulldeps/auxiliary/lint_group_plugin_test.rs

@@ -49,5 +49,5 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
 #[plugin_registrar]
 pub fn plugin_registrar(reg: &mut Registry) {
     reg.register_late_lint_pass(box Pass);
-    reg.register_lint_group("lint_me", vec![TEST_LINT, PLEASE_LINT]);
+    reg.register_lint_group("lint_me", None, vec![TEST_LINT, PLEASE_LINT]);
 }

Diff for: src/test/ui-fulldeps/auxiliary/lint_group_plugin_test.rs

@@ -49,5 +49,5 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
 #[plugin_registrar]
 pub fn plugin_registrar(reg: &mut Registry) {
     reg.register_late_lint_pass(box Pass);
-    reg.register_lint_group("lint_me", vec![TEST_LINT, PLEASE_LINT]);
+    reg.register_lint_group("lint_me", None, vec![TEST_LINT, PLEASE_LINT]);
 }

Diff for: src/test/ui-fulldeps/auxiliary/lint_tool_test.rs

@@ -25,12 +25,13 @@ use rustc::lint::{EarlyContext, LintContext, LintPass, EarlyLintPass,
 use rustc_plugin::Registry;
 use syntax::ast;
 declare_tool_lint!(pub clippy::TEST_LINT, Warn, "Warn about stuff");
+declare_tool_lint!(pub clippy::TEST_GROUP, Warn, "Warn about other stuff");
 
 struct Pass;
 
 impl LintPass for Pass {
     fn get_lints(&self) -> LintArray {
-        lint_array!(TEST_LINT)
+        lint_array!(TEST_LINT, TEST_GROUP)
     }
 }
 
@@ -39,10 +40,14 @@ impl EarlyLintPass for Pass {
         if it.ident.name == "lintme" {
             cx.span_lint(TEST_LINT, it.span, "item is named 'lintme'");
         }
+        if it.ident.name == "lintmetoo" {
+            cx.span_lint(TEST_GROUP, it.span, "item is named 'lintmetoo'");
+        }
     }
 }
 
 #[plugin_registrar]
 pub fn plugin_registrar(reg: &mut Registry) {
     reg.register_early_lint_pass(box Pass);
+    reg.register_lint_group("clippy::group", Some("clippy_group"), vec![TEST_LINT, TEST_GROUP]);
 }

Diff for: src/test/ui-fulldeps/lint_tool_test.rs

@@ -8,17 +8,33 @@
 // 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
+// compile-flags: --cfg foo
 #![feature(plugin)]
 #![feature(tool_lints)]
 #![plugin(lint_tool_test)]
 #![allow(dead_code)]
+#![cfg_attr(foo, warn(test_lint))]
+//~^ WARNING lint name `test_lint` is deprecated and may not have an effect in the future
+//~^^ WARNING lint name `test_lint` is deprecated and may not have an effect in the future
+#![deny(clippy_group)]
+//~^ WARNING lint name `clippy_group` is deprecated and may not have an effect in the future
 
-fn lintme() { } //~ WARNING item is named 'lintme'
+fn lintme() { } //~ ERROR item is named 'lintme'
+
+#[allow(clippy::group)]
+fn lintmetoo() {}
 
 #[allow(clippy::test_lint)]
 pub fn main() {
     fn lintme() { }
+    fn lintmetoo() { } //~ ERROR item is named 'lintmetoo'
+}
+
+#[allow(test_group)]
+//~^ WARNING lint name `test_group` is deprecated and may not have an effect in the future
+#[deny(this_lint_does_not_exist)] //~ WARNING unknown lint: `this_lint_does_not_exist`
+fn hello() {
+    fn lintmetoo() { }
 }