From 2efec3c18050b4093ed8be9537145bdc2a50f7e7 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Sat, 14 Jan 2017 07:35:54 +0000 Subject: [PATCH] Improve `unused_extern_crate` warnings. --- src/librustc_resolve/build_reduced_graph.rs | 27 ++++++++++--------- src/librustc_resolve/check_unused.rs | 27 +++++++++++-------- src/librustc_resolve/lib.rs | 11 +++----- src/librustc_resolve/resolve_imports.rs | 2 ++ .../compile-fail/lint-unused-extern-crate.rs | 5 ++++ 5 files changed, 41 insertions(+), 31 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index 5e9856878865a..0217aefc227c3 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -249,6 +249,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 { @@ -260,11 +262,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 @@ -522,7 +524,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); @@ -532,22 +533,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 { @@ -566,9 +568,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 { @@ -577,6 +577,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 index 41391c65a128d..0e44f4556dd08 100644 --- 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 index 971b91ea313f3..8a206664a7d3a 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -1099,7 +1099,6 @@ pub struct Resolver<'a> { pub glob_map: GlobMap, used_imports: FxHashSet<(NodeId, Namespace)>, - used_crates: FxHashSet, pub maybe_unused_trait_imports: NodeSet, privacy_errors: Vec>, @@ -1130,6 +1129,8 @@ pub struct Resolver<'a> { // A set of procedural macros imported by `#[macro_use]` that have already been warned about warned_proc_macros: FxHashSet, + + potentially_unused_imports: Vec<&'a ImportDirective<'a>>, } pub struct ResolverArenas<'a> { @@ -1279,7 +1280,6 @@ impl<'a> Resolver<'a> { glob_map: NodeMap(), used_imports: FxHashSet(), - used_crates: FxHashSet(), maybe_unused_trait_imports: NodeSet(), privacy_errors: Vec::new(), @@ -1309,6 +1309,7 @@ impl<'a> Resolver<'a> { whitelisted_legacy_custom_derives: Vec::new(), proc_macro_enabled: features.proc_macro, warned_proc_macros: FxHashSet(), + potentially_unused_imports: Vec::new(), } } @@ -1354,15 +1355,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 index 65e599ac6c7e8..8d94fd86b1751 100644 --- 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, pub expansion: Mark, + pub used: Cell, } 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 index 52cb84f662dd0..515e3b833d9f0 100644 --- 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 = Vec::new(); let y = foo();