Improve unused_extern_crate warnings.

jseyfried · jseyfried · commit bcf3418a496d · 2017-01-14T10:16:53.000Z
diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs
@@ -246,6 +246,8 @@ impl<'a> Resolver<'a> {
                 // n.b. we don't need to look at the path option here, because cstore already did
                 let crate_id = self.session.cstore.extern_mod_stmt_cnum(item.id).unwrap();
                 let module = self.get_extern_crate_root(crate_id);
+                self.populate_module_if_necessary(module);
+                let used = self.process_legacy_macro_imports(item, module, expansion);
                 let binding =
                     (module, ty::Visibility::Public, sp, expansion).to_name_binding(self.arenas);
                 let directive = self.arenas.alloc_import_directive(ImportDirective {
@@ -257,11 +259,11 @@ impl<'a> Resolver<'a> {
                     module_path: Vec::new(),
                     vis: Cell::new(vis),
                     expansion: expansion,
+                    used: Cell::new(used),
                 });
+                self.potentially_unused_imports.push(directive);
                 let imported_binding = self.import(binding, directive);
                 self.define(parent, ident, TypeNS, imported_binding);
-                self.populate_module_if_necessary(module);
-                self.process_legacy_macro_imports(item, module, expansion);
             }
 
             ItemKind::Mod(..) if item.ident == keywords::Invalid.ident() => {} // Crate root
@@ -519,7 +521,6 @@ impl<'a> Resolver<'a> {
                            binding: &'a NameBinding<'a>,
                            span: Span,
                            allow_shadowing: bool) {
-        self.used_crates.insert(binding.def().def_id().krate);
         self.macro_names.insert(name);
         if self.builtin_macros.insert(name, binding).is_some() && !allow_shadowing {
             let msg = format!("`{}` is already in scope", name);
@@ -529,22 +530,23 @@ impl<'a> Resolver<'a> {
         }
     }
 
-    fn process_legacy_macro_imports(&mut self, item: &Item, module: Module<'a>, expansion: Mark) {
+    // This returns true if we should consider the underlying `extern crate` to be used.
+    fn process_legacy_macro_imports(&mut self, item: &Item, module: Module<'a>, expansion: Mark)
+                                    -> bool {
         let allow_shadowing = expansion == Mark::root();
         let legacy_imports = self.legacy_macro_imports(&item.attrs);
-        let cnum = module.def_id().unwrap().krate;
+        let mut used = legacy_imports != LegacyMacroImports::default();
 
         // `#[macro_use]` and `#[macro_reexport]` are only allowed at the crate root.
-        if self.current_module.parent.is_some() && legacy_imports != LegacyMacroImports::default() {
+        if self.current_module.parent.is_some() && used {
             span_err!(self.session, item.span, E0468,
                       "an `extern crate` loading macros must be at the crate root");
-        } else if !self.use_extern_macros &&
-                  self.session.cstore.dep_kind(cnum).macros_only() &&
-                  legacy_imports == LegacyMacroImports::default() {
+        } else if !self.use_extern_macros && !used &&
+                  self.session.cstore.dep_kind(module.def_id().unwrap().krate).macros_only() {
             let msg = "custom derive crates and `#[no_link]` crates have no effect without \
                        `#[macro_use]`";
             self.session.span_warn(item.span, msg);
-            self.used_crates.insert(cnum); // Avoid the normal unused extern crate warning
+            used = true; // Avoid the normal unused extern crate warning
         }
 
         if let Some(span) = legacy_imports.import_all {
@@ -563,9 +565,7 @@ impl<'a> Resolver<'a> {
             }
         }
         for (name, span) in legacy_imports.reexports {
-            let krate = module.def_id().unwrap().krate;
-            self.used_crates.insert(krate);
-            self.session.cstore.export_macros(krate);
+            self.session.cstore.export_macros(module.def_id().unwrap().krate);
             let ident = Ident::with_empty_ctxt(name);
             let result = self.resolve_ident_in_module(module, ident, MacroNS, false, None);
             if let Ok(binding) = result {
@@ -574,6 +574,7 @@ impl<'a> Resolver<'a> {
                 span_err!(self.session, span, E0470, "reexported macro not found");
             }
         }
+        used
     }
 
     // does this attribute list contain "macro_use"?
diff --git a/src/librustc_resolve/check_unused.rs b/src/librustc_resolve/check_unused.rs
@@ -22,8 +22,9 @@
 use std::ops::{Deref, DerefMut};
 
 use Resolver;
+use resolve_imports::ImportDirectiveSubclass;
 
-use rustc::lint;
+use rustc::{lint, ty};
 use rustc::util::nodemap::NodeMap;
 use syntax::ast::{self, ViewPathGlob, ViewPathList, ViewPathSimple};
 use syntax::visit::{self, Visitor};
@@ -86,16 +87,6 @@ impl<'a, 'b> Visitor<'a> for UnusedImportCheckVisitor<'a, 'b> {
         }
 
         match item.node {
-            ast::ItemKind::ExternCrate(_) => {
-                if let Some(crate_num) = self.session.cstore.extern_mod_stmt_cnum(item.id) {
-                    if !self.used_crates.contains(&crate_num) {
-                        self.session.add_lint(lint::builtin::UNUSED_EXTERN_CRATES,
-                                              item.id,
-                                              item.span,
-                                              "unused extern crate".to_string());
-                    }
-                }
-            }
             ast::ItemKind::Use(ref p) => {
                 match p.node {
                     ViewPathSimple(..) => {
@@ -124,6 +115,20 @@ impl<'a, 'b> Visitor<'a> for UnusedImportCheckVisitor<'a, 'b> {
 }
 
 pub fn check_crate(resolver: &mut Resolver, krate: &ast::Crate) {
+    for directive in resolver.potentially_unused_imports.iter() {
+        match directive.subclass {
+            _ if directive.used.get() ||
+                 directive.vis.get() == ty::Visibility::Public ||
+                 directive.span.source_equal(&DUMMY_SP) => {}
+            ImportDirectiveSubclass::ExternCrate => {
+                let lint = lint::builtin::UNUSED_EXTERN_CRATES;
+                let msg = "unused extern crate".to_string();
+                resolver.session.add_lint(lint, directive.id, directive.span, msg);
+            }
+            _ => {}
+        }
+    }
+
     let mut visitor = UnusedImportCheckVisitor {
         resolver: resolver,
         unused_imports: NodeMap(),
diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs
@@ -1098,7 +1098,6 @@ pub struct Resolver<'a> {
     pub glob_map: GlobMap,
 
     used_imports: FxHashSet<(NodeId, Namespace)>,
-    used_crates: FxHashSet<CrateNum>,
     pub maybe_unused_trait_imports: NodeSet,
 
     privacy_errors: Vec<PrivacyError<'a>>,
@@ -1123,6 +1122,8 @@ pub struct Resolver<'a> {
 
     // Avoid duplicated errors for "name already defined".
     name_already_seen: FxHashMap<Name, Span>,
+
+    potentially_unused_imports: Vec<&'a ImportDirective<'a>>,
 }
 
 pub struct ResolverArenas<'a> {
@@ -1270,7 +1271,6 @@ impl<'a> Resolver<'a> {
             glob_map: NodeMap(),
 
             used_imports: FxHashSet(),
-            used_crates: FxHashSet(),
             maybe_unused_trait_imports: NodeSet(),
 
             privacy_errors: Vec::new(),
@@ -1296,6 +1296,7 @@ impl<'a> Resolver<'a> {
             invocations: invocations,
             name_already_seen: FxHashMap(),
             whitelisted_legacy_custom_derives: Vec::new(),
+            potentially_unused_imports: Vec::new(),
         }
     }
 
@@ -1341,15 +1342,11 @@ impl<'a> Resolver<'a> {
 
     fn record_use(&mut self, ident: Ident, ns: Namespace, binding: &'a NameBinding<'a>, span: Span)
                   -> bool /* true if an error was reported */ {
-        // track extern crates for unused_extern_crate lint
-        if let Some(DefId { krate, .. }) = binding.module().and_then(ModuleData::def_id) {
-            self.used_crates.insert(krate);
-        }
-
         match binding.kind {
             NameBindingKind::Import { directive, binding, ref used, legacy_self_import }
                     if !used.get() => {
                 used.set(true);
+                directive.used.set(true);
                 if legacy_self_import {
                     self.warn_legacy_self_import(directive);
                     return false;
diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs
@@ -62,6 +62,7 @@ pub struct ImportDirective<'a> {
     pub span: Span,
     pub vis: Cell<ty::Visibility>,
     pub expansion: Mark,
+    pub used: Cell<bool>,
 }
 
 impl<'a> ImportDirective<'a> {
@@ -257,6 +258,7 @@ impl<'a> Resolver<'a> {
             id: id,
             vis: Cell::new(vis),
             expansion: expansion,
+            used: Cell::new(false),
         });
 
         self.indeterminate_imports.push(directive);
diff --git a/src/test/compile-fail/lint-unused-extern-crate.rs b/src/test/compile-fail/lint-unused-extern-crate.rs
@@ -33,6 +33,11 @@ use rand::isaac::IsaacRng;
 
 use other::*;
 
+mod foo {
+    // Test that this is unused even though an earler `extern crate rand` is used.
+    extern crate rand; //~ ERROR unused extern crate
+}
+
 fn main() {
     let x: collecs::vec::Vec<usize> = Vec::new();
     let y = foo();
