Auto merge of #37456 - estebank:unused-imports-verbosity, r=jonathandturner

bors · web-flow · commit 5293b913c457 · 2016-11-11T09:04:17.000-08:00
Group unused import warnings per import list Given a file ``` rust use std::collections::{BinaryHeap, BTreeMap, BTreeSet}; fn main() {} ``` Show a single warning, instead of three for each unused import: ``` nocode warning: unused imports, #[warn(unused_imports)] on by default --> file2.rs:1:24 | 1 | use std::collections::{BinaryHeap, BTreeMap, BTreeSet}; | ^^^^^^^^^^ ^^^^^^^^ ^^^^^^^^ ``` Include support for lints pointing at `MultilineSpan`s, instead of just `Span`s. Fixes #16132.
diff --git a/src/librustc/lint/context.rs b/src/librustc/lint/context.rs
@@ -106,7 +106,7 @@ pub trait IntoEarlyLint {
     fn into_early_lint(self, id: LintId) -> EarlyLint;
 }
 
-impl<'a> IntoEarlyLint for (Span, &'a str) {
+impl<'a, S: Into<MultiSpan>> IntoEarlyLint for (S, &'a str) {
     fn into_early_lint(self, id: LintId) -> EarlyLint {
         let (span, msg) = self;
         let mut diagnostic = Diagnostic::new(errors::Level::Warning, msg);
@@ -530,7 +530,10 @@ pub trait LintContext: Sized {
         })
     }
 
-    fn lookup_and_emit(&self, lint: &'static Lint, span: Option<Span>, msg: &str) {
+    fn lookup_and_emit<S: Into<MultiSpan>>(&self,
+                                           lint: &'static Lint,
+                                           span: Option<S>,
+                                           msg: &str) {
         let (level, src) = match self.level_src(lint) {
             None => return,
             Some(pair) => pair,
@@ -553,7 +556,7 @@ pub trait LintContext: Sized {
     }
 
     /// Emit a lint at the appropriate level, for a particular span.
-    fn span_lint(&self, lint: &'static Lint, span: Span, msg: &str) {
+    fn span_lint<S: Into<MultiSpan>>(&self, lint: &'static Lint, span: S, msg: &str) {
         self.lookup_and_emit(lint, Some(span), msg);
     }
 
@@ -601,7 +604,7 @@ pub trait LintContext: Sized {
 
     /// Emit a lint at the appropriate level, with no associated span.
     fn lint(&self, lint: &'static Lint, msg: &str) {
-        self.lookup_and_emit(lint, None, msg);
+        self.lookup_and_emit(lint, None as Option<Span>, msg);
     }
 
     /// Merge the lints specified by any lint attributes into the
diff --git a/src/librustc/session/mod.rs b/src/librustc/session/mod.rs
@@ -258,14 +258,15 @@ impl Session {
     pub fn unimpl(&self, msg: &str) -> ! {
         self.diagnostic().unimpl(msg)
     }
-    pub fn add_lint(&self,
-                    lint: &'static lint::Lint,
-                    id: ast::NodeId,
-                    sp: Span,
-                    msg: String)
+    pub fn add_lint<S: Into<MultiSpan>>(&self,
+                                        lint: &'static lint::Lint,
+                                        id: ast::NodeId,
+                                        sp: S,
+                                        msg: String)
     {
         self.add_lint_diagnostic(lint, id, (sp, &msg[..]))
     }
+
     pub fn add_lint_diagnostic<M>(&self,
                                   lint: &'static lint::Lint,
                                   id: ast::NodeId,
diff --git a/src/librustc_resolve/check_unused.rs b/src/librustc_resolve/check_unused.rs
@@ -25,13 +25,16 @@ use Resolver;
 use Namespace::{TypeNS, ValueNS};
 
 use rustc::lint;
+use rustc::util::nodemap::NodeMap;
 use syntax::ast::{self, ViewPathGlob, ViewPathList, ViewPathSimple};
 use syntax::visit::{self, Visitor};
-use syntax_pos::{Span, DUMMY_SP};
+use syntax_pos::{Span, MultiSpan, DUMMY_SP};
 
 
 struct UnusedImportCheckVisitor<'a, 'b: 'a> {
     resolver: &'a mut Resolver<'b>,
+    /// All the (so far) unused imports, grouped path list
+    unused_imports: NodeMap<NodeMap<Span>>,
 }
 
 // Deref and DerefMut impls allow treating UnusedImportCheckVisitor as Resolver.
@@ -52,23 +55,21 @@ impl<'a, 'b> DerefMut for UnusedImportCheckVisitor<'a, 'b> {
 impl<'a, 'b> UnusedImportCheckVisitor<'a, 'b> {
     // We have information about whether `use` (import) directives are actually
     // used now. If an import is not used at all, we signal a lint error.
-    fn check_import(&mut self, id: ast::NodeId, span: Span) {
+    fn check_import(&mut self, item_id: ast::NodeId, id: ast::NodeId, span: Span) {
         if !self.used_imports.contains(&(id, TypeNS)) &&
            !self.used_imports.contains(&(id, ValueNS)) {
             if self.maybe_unused_trait_imports.contains(&id) {
                 // Check later.
                 return;
             }
-            let msg = if let Ok(snippet) = self.session.codemap().span_to_snippet(span) {
-                format!("unused import: `{}`", snippet)
-            } else {
-                "unused import".to_string()
-            };
-            self.session.add_lint(lint::builtin::UNUSED_IMPORTS, id, span, msg);
+            self.unused_imports.entry(item_id).or_insert_with(NodeMap).insert(id, span);
         } else {
             // This trait import is definitely used, in a way other than
             // method resolution.
             self.maybe_unused_trait_imports.remove(&id);
+            if let Some(i) = self.unused_imports.get_mut(&item_id) {
+                i.remove(&id);
+            }
         }
     }
 }
@@ -98,16 +99,16 @@ impl<'a, 'b> Visitor for UnusedImportCheckVisitor<'a, 'b> {
             ast::ItemKind::Use(ref p) => {
                 match p.node {
                     ViewPathSimple(..) => {
-                        self.check_import(item.id, p.span)
+                        self.check_import(item.id, item.id, p.span)
                     }
 
                     ViewPathList(_, ref list) => {
                         for i in list {
-                            self.check_import(i.node.id, i.span);
+                            self.check_import(item.id, i.node.id, i.span);
                         }
                     }
                     ViewPathGlob(_) => {
-                        self.check_import(item.id, p.span)
+                        self.check_import(item.id, item.id, p.span);
                     }
                 }
             }
@@ -117,6 +118,35 @@ impl<'a, 'b> Visitor for UnusedImportCheckVisitor<'a, 'b> {
 }
 
 pub fn check_crate(resolver: &mut Resolver, krate: &ast::Crate) {
-    let mut visitor = UnusedImportCheckVisitor { resolver: resolver };
+    let mut visitor = UnusedImportCheckVisitor {
+        resolver: resolver,
+        unused_imports: NodeMap(),
+    };
     visit::walk_crate(&mut visitor, krate);
+
+    for (id, spans) in &visitor.unused_imports {
+        let len = spans.len();
+        let mut spans = spans.values().map(|s| *s).collect::<Vec<Span>>();
+        spans.sort();
+        let ms = MultiSpan::from_spans(spans.clone());
+        let mut span_snippets = spans.iter()
+            .filter_map(|s| {
+                match visitor.session.codemap().span_to_snippet(*s) {
+                    Ok(s) => Some(format!("`{}`", s)),
+                    _ => None,
+                }
+            }).collect::<Vec<String>>();
+        span_snippets.sort();
+        let msg = format!("unused import{}{}",
+                          if len > 1 { "s" } else { "" },
+                          if span_snippets.len() > 0 {
+                              format!(": {}", span_snippets.join(", "))
+                          } else {
+                              String::new()
+                          });
+        visitor.session.add_lint(lint::builtin::UNUSED_IMPORTS,
+                                 *id,
+                                 ms,
+                                 msg);
+    }
 }
diff --git a/src/libsyntax_pos/lib.rs b/src/libsyntax_pos/lib.rs
@@ -52,7 +52,7 @@ pub type FileName = String;
 /// able to use many of the functions on spans in codemap and you cannot assume
 /// that the length of the span = hi - lo; there may be space in the BytePos
 /// range between files.
-#[derive(Clone, Copy, Hash, PartialEq, Eq)]
+#[derive(Clone, Copy, Hash, PartialEq, Eq, Ord, PartialOrd)]
 pub struct Span {
     pub lo: BytePos,
     pub hi: BytePos,
@@ -67,7 +67,7 @@ pub struct Span {
 ///   the error, and would be rendered with `^^^`.
 /// - they can have a *label*. In this case, the label is written next
 ///   to the mark in the snippet when we render.
-#[derive(Clone, Debug, PartialEq, Eq)]
+#[derive(Clone, Debug, Hash, PartialEq, Eq)]
 pub struct MultiSpan {
     primary_spans: Vec<Span>,
     span_labels: Vec<(Span, String)>,
@@ -254,7 +254,7 @@ impl From<Span> for MultiSpan {
     }
 }
 
-#[derive(PartialEq, Eq, Clone, Debug, Hash, RustcEncodable, RustcDecodable, Copy)]
+#[derive(PartialEq, Eq, Clone, Debug, Hash, RustcEncodable, RustcDecodable, Copy, Ord, PartialOrd)]
 pub struct ExpnId(pub u32);
 
 pub const NO_EXPANSION: ExpnId = ExpnId(!0);
diff --git a/src/test/compile-fail/lint-unused-imports.rs b/src/test/compile-fail/lint-unused-imports.rs
@@ -17,8 +17,9 @@ use std::mem::*;            // shouldn't get errors for not using
                             // everything imported
 
 // Should get errors for both 'Some' and 'None'
-use std::option::Option::{Some, None}; //~ ERROR unused import: `Some`
-                                    //~^ ERROR unused import: `None`
+use std::option::Option::{Some, None};
+//~^ ERROR unused imports: `None`, `Some`
+//~| ERROR unused imports: `None`, `Some`
 
 use test::A;       //~ ERROR unused import: `test::A`
 // Be sure that if we just bring some methods into scope that they're also
diff --git a/src/test/ui/span/multispan-import-lint.rs b/src/test/ui/span/multispan-import-lint.rs
@@ -0,0 +1,15 @@
+// Copyright 2016 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.
+
+use std::cmp::{Eq, Ord, min, PartialEq, PartialOrd};
+
+fn main() {
+    let _ = min(1, 2);
+}
diff --git a/src/test/ui/span/multispan-import-lint.stderr b/src/test/ui/span/multispan-import-lint.stderr
@@ -0,0 +1,6 @@
+warning: unused imports: `Eq`, `Ord`, `PartialEq`, `PartialOrd`, #[warn(unused_imports)] on by default
+  --> $DIR/multispan-import-lint.rs:11:16
+   |
+11 | use std::cmp::{Eq, Ord, min, PartialEq, PartialOrd};
+   |                ^^  ^^^       ^^^^^^^^^  ^^^^^^^^^^
+
