From ed51ea7c1994698b151e93443b518149829009aa Mon Sep 17 00:00:00 2001 From: ember arlynx Date: Tue, 20 Oct 2020 15:40:45 -0400 Subject: [PATCH 1/5] WIP: unnecessary pub(crate) lint this lint will tell you if an item is `pub(crate)` but the crate would compile just fine if it weren't. --- CHANGELOG.md | 1 + clippy_lints/src/lib.rs | 4 + clippy_lints/src/unneeded_pub_crate.rs | 259 +++++++++++++++++++++++++ src/lintlist/mod.rs | 7 + tests/ui/unneeded_pub_crate.fixed | 37 ++++ tests/ui/unneeded_pub_crate.rs | 37 ++++ tests/ui/unneeded_pub_crate.stderr | 22 +++ 7 files changed, 367 insertions(+) create mode 100644 clippy_lints/src/unneeded_pub_crate.rs create mode 100644 tests/ui/unneeded_pub_crate.fixed create mode 100644 tests/ui/unneeded_pub_crate.rs create mode 100644 tests/ui/unneeded_pub_crate.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index d82f970b8bf2..c2c538b32974 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1995,6 +1995,7 @@ Released 2018-09-13 [`unnecessary_sort_by`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_sort_by [`unnecessary_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_unwrap [`unneeded_field_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#unneeded_field_pattern +[`unneeded_pub_crate`]: https://rust-lang.github.io/rust-clippy/master/index.html#unneeded_pub_crate [`unneeded_wildcard_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#unneeded_wildcard_pattern [`unnested_or_patterns`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnested_or_patterns [`unreachable`]: https://rust-lang.github.io/rust-clippy/master/index.html#unreachable diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index d4d2f92a6a69..4b0a8845ca93 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -318,6 +318,7 @@ mod unicode; mod unit_return_expecting_ord; mod unnamed_address; mod unnecessary_sort_by; +mod unneeded_pub_crate; mod unnested_or_patterns; mod unsafe_removed_from_name; mod unused_io_amount; @@ -869,6 +870,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &unnamed_address::FN_ADDRESS_COMPARISONS, &unnamed_address::VTABLE_ADDRESS_COMPARISONS, &unnecessary_sort_by::UNNECESSARY_SORT_BY, + &unneeded_pub_crate::UNNEEDED_PUB_CRATE, &unnested_or_patterns::UNNESTED_OR_PATTERNS, &unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME, &unused_io_amount::UNUSED_IO_AMOUNT, @@ -1137,6 +1139,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(move || box disallowed_method::DisallowedMethod::new(&disallowed_methods)); store.register_early_pass(|| box asm_syntax::InlineAsmX86AttSyntax); store.register_early_pass(|| box asm_syntax::InlineAsmX86IntelSyntax); + store.register_late_pass(|| box unneeded_pub_crate::UnneededPubCrate::default()); store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![ @@ -1257,6 +1260,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&types::OPTION_OPTION), LintId::of(&unicode::NON_ASCII_LITERAL), LintId::of(&unicode::UNICODE_NOT_NFC), + LintId::of(&unneeded_pub_crate::UNNEEDED_PUB_CRATE), LintId::of(&unnested_or_patterns::UNNESTED_OR_PATTERNS), LintId::of(&unused_self::UNUSED_SELF), LintId::of(&wildcard_imports::ENUM_GLOB_USE), diff --git a/clippy_lints/src/unneeded_pub_crate.rs b/clippy_lints/src/unneeded_pub_crate.rs new file mode 100644 index 000000000000..034dc4ce2f51 --- /dev/null +++ b/clippy_lints/src/unneeded_pub_crate.rs @@ -0,0 +1,259 @@ +use crate::utils::span_lint_and_then; +use rustc_data_structures::fx::FxHashMap; +use rustc_errors::Applicability; +use rustc_hir as hir; +use rustc_hir::def::Res; +use rustc_hir::def_id::DefId; +use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor}; +use rustc_hir::CRATE_HIR_ID; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::{self, TyCtxt}; +use rustc_session::{declare_tool_lint, impl_lint_pass}; +use rustc_span::Span; + +declare_clippy_lint! { + /// **What it does:** + /// + /// Checks if a `pub(crate)` visibility modifier was unnecessary given the + /// way the item was actually used. + /// + /// **Why is this bad?** + /// + /// You may be decieved into thinking an item is used far away, when it is not! + /// + /// **Known problems:** Does not check methods, const/static items, struct/enum/union fields, or type aliases. + /// + /// **Example:** + /// + /// ```rust + /// mod outer { + /// mod inner { + /// pub(crate) fn foo() { } // this function is never used in a `pub(crate)` fashion, does it really need to be `pub(crate)`? + /// pub(crate) fn bar() { foo() } + /// } + /// pub fn main() { + /// inner::bar(); + /// } + /// } + /// ``` + /// Use instead: + /// ```rust + /// mod outer { + /// mod inner { + /// fn foo() { } // this function is never used in a `pub(crate)` fashion, does it really need to be `pub(crate)`? + /// pub(crate) fn bar() { foo() } + /// } + /// pub fn main() { + /// inner::bar(); + /// } + /// } + /// ``` + pub UNNEEDED_PUB_CRATE, + pedantic, + "Using `pub(crate)` visibility on items that are only accessed from within the module that contains the item." +} + +#[derive(Default)] +pub struct UnneededPubCrate { + watched_item_map: FxHashMap, + current_module_path: Vec, +} + +impl_lint_pass!(UnneededPubCrate => [UNNEEDED_PUB_CRATE]); + +/// An item with `pub(crate)` visibiliy that we're watching to see if it's +/// referenced in ways where the visibility was useful. +struct WatchedItem { + enclosing_module: hir::HirId, + status: WatchStatus, + /// The span of the visibility modifier + span: Span, +} + +/// The status of an item we're watching, accumulated while we check the HIR. +#[derive(Copy, Clone)] +enum WatchStatus { + /// An item starts off unreferenced. If it nevers leaves this state, that + /// means the item is very dead. + Unreferenced, + /// We've only seen local references to this item. If an item ends in this + /// state, we can demote the `pub(crate)` to `priv`. + LocalReference, + /// We've seen at least one reference from somewhere in the crate that didn't qualify for + /// `LocalReference`. + CrateReference, +} +use WatchStatus::*; + +impl WatchStatus { + fn observe(&mut self, observation: WatchStatus) { + match (*self, observation) { + (Unreferenced, _) => *self = observation, + (LocalReference, CrateReference) => *self = CrateReference, + _ => {}, + } + } +} + +struct UseScanner<'tcx> { + tcx: TyCtxt<'tcx>, + maybe_typeck_results: Option<&'tcx ty::TypeckResults<'tcx>>, + watched_item_map: FxHashMap, + current_module_path: Vec, +} + +impl<'tcx> Visitor<'tcx> for UseScanner<'tcx> { + type Map = rustc_middle::hir::map::Map<'tcx>; + + fn nested_visit_map(&mut self) -> NestedVisitorMap { + NestedVisitorMap::All(self.tcx.hir()) + } + + fn visit_nested_body(&mut self, body: hir::BodyId) { + let old_maybe_typeck_results = self.maybe_typeck_results.replace(self.tcx.typeck_body(body)); + let body = self.tcx.hir().body(body); + self.visit_body(body); + self.maybe_typeck_results = old_maybe_typeck_results; + } + + fn visit_mod(&mut self, mod_: &'tcx hir::Mod<'tcx>, _span: Span, hir_id: hir::HirId) { + self.current_module_path.push(hir_id); + intravisit::walk_mod(self, mod_, hir_id); + self.current_module_path + .pop() + .expect("mismatched push/pop in UnneededPubCrate::check_mod_post"); + } + + fn visit_expr(&mut self, expr: &'tcx hir::Expr<'tcx>) { + let def = match expr.kind { + hir::ExprKind::MethodCall(..) => self + .maybe_typeck_results + .and_then(|typeck_results| typeck_results.type_dependent_def(expr.hir_id)) + .map(|(kind, def_id)| (kind, def_id)), + _ => None, + }; + if let Some((_kind, def_id)) = def { + self.examine_use(def_id); + } else { + intravisit::walk_expr(self, expr); + } + } + + fn visit_qpath(&mut self, qpath: &'tcx hir::QPath<'tcx>, hir_id: hir::HirId, span: Span) { + let def = match qpath { + hir::QPath::Resolved(_, path) => match path.res { + Res::Def(kind, def_id) => Some((kind, def_id)), + _ => None, + }, + hir::QPath::TypeRelative(..) | hir::QPath::LangItem(..) => self + .maybe_typeck_results + .and_then(|typeck_results| typeck_results.type_dependent_def(hir_id)), + }; + if let Some((_kind, def_id)) = def { + self.examine_use(def_id); + } + intravisit::walk_qpath(self, qpath, hir_id, span); + } + + fn visit_path(&mut self, path: &'tcx hir::Path<'tcx>, _hir_id: hir::HirId) { + match path.res { + Res::Def(_kind, def_id) => { + self.examine_use(def_id); + }, + _ => {}, + } + intravisit::walk_path(self, path); + } +} + +impl<'tcx> UseScanner<'tcx> { + fn examine_use(&mut self, def_id: DefId) { + if let Some(node) = self.tcx.hir().get_if_local(def_id) { + match node { + // TODO: TraitItem, ImplItem, Field + hir::Node::ForeignItem(hir::ForeignItem { hir_id, .. }) | hir::Node::Item(hir::Item { hir_id, .. }) => { + if let Some(watch_item) = self.watched_item_map.get_mut(&hir_id) { + if self + .current_module_path + .iter() + // if the current path contains the + // enclosing module of the watched item + // somewhere, we would have been able to + // reference it even if it weren't marked + // `pub(crate)`. + .any(|mod_| *mod_ == watch_item.enclosing_module) + { + watch_item.status.observe(LocalReference); + } else { + watch_item.status.observe(CrateReference); + } + } + }, + _ => {}, + } + } + } +} + +impl<'tcx> LateLintPass<'tcx> for UnneededPubCrate { + fn check_mod(&mut self, _cx: &LateContext<'tcx>, _mod: &'tcx hir::Mod<'tcx>, _span: Span, hir_id: hir::HirId) { + self.current_module_path.push(hir_id); + } + + fn check_mod_post(&mut self, _cx: &LateContext<'tcx>, _mod: &hir::Mod<'tcx>, _span: Span, hir_id: hir::HirId) { + assert_eq!( + self.current_module_path + .pop() + .expect("mismatched push/pop in UnneededPubCrate::check_mod_post"), + hir_id + ); + } + + // TODO: methods, struct fields, const/static, alias, modules. what else can have visibility? + fn check_item(&mut self, cx: &LateContext<'tcx>, item: &hir::Item<'tcx>) { + if matches!(item.vis.node, hir::VisibilityKind::Crate { .. }) && !cx.access_levels.is_exported(item.hir_id) { + self.watched_item_map.insert( + item.hir_id, + WatchedItem { + enclosing_module: *self.current_module_path.last().unwrap(), + status: Unreferenced, + span: item.vis.span, + }, + ); + } + } + + fn check_crate_post(&mut self, cx: &LateContext<'tcx>, crate_: &'tcx hir::Crate<'tcx>) { + // ok, now that we have scanned the entire crate for things with + // visibility and filled the watched item map, let's scan it again for + // any uses of those items. + let watched_item_map = std::mem::replace(&mut self.watched_item_map, FxHashMap::default()); + let mut use_scanner = UseScanner { + tcx: cx.tcx, + maybe_typeck_results: cx.maybe_typeck_results(), + watched_item_map, + current_module_path: vec![CRATE_HIR_ID], + }; + + intravisit::walk_crate(&mut use_scanner, crate_); + + for (_, watched_item) in use_scanner.watched_item_map { + if let LocalReference = watched_item.status { + span_lint_and_then( + cx, + UNNEEDED_PUB_CRATE, + watched_item.span, + "pub(crate) item is never used outside of its defining module", + |diag| { + diag.span_suggestion( + watched_item.span, + "consider removing pub(crate)", + String::new(), + Applicability::MachineApplicable, + ); + }, + ); + } + } + } +} diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 6301d623a2b1..d58ff54a7d17 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -2524,6 +2524,13 @@ vec![ deprecation: None, module: "misc_early", }, + Lint { + name: "unneeded_pub_crate", + group: "pedantic", + desc: "Using `pub(crate)` visibility on items that are only accessed from within the module that contains the item.", + deprecation: None, + module: "unneeded_pub_crate", + }, Lint { name: "unneeded_wildcard_pattern", group: "complexity", diff --git a/tests/ui/unneeded_pub_crate.fixed b/tests/ui/unneeded_pub_crate.fixed new file mode 100644 index 000000000000..143f174f28b2 --- /dev/null +++ b/tests/ui/unneeded_pub_crate.fixed @@ -0,0 +1,37 @@ +// run-rustfix +#![allow(dead_code)] +#![warn(clippy::unneeded_pub_crate)] + + struct Baz { + pub(crate) field: u8, +} + +mod outer { + mod inner { + // do these things _really_ need to be `pub(crate)`? + pub(crate) struct Foo; + trait Baz { + fn the_goods(&self); + fn secret_thing(&self); + } + impl Baz for crate::Baz { + fn the_goods(&self) {} + fn secret_thing(&self) {} + } + fn foo() {} + pub(crate) fn bar() { + foo(); + let _ = Foo; + let x = crate::Baz { field: 3 }; + x.the_goods(); + x.secret_thing(); + } + } + pub fn main() { + inner::bar(); + } +} + +fn main() { + crate::outer::main(); +} diff --git a/tests/ui/unneeded_pub_crate.rs b/tests/ui/unneeded_pub_crate.rs new file mode 100644 index 000000000000..09417073a148 --- /dev/null +++ b/tests/ui/unneeded_pub_crate.rs @@ -0,0 +1,37 @@ +// run-rustfix +#![allow(dead_code)] +#![warn(clippy::unneeded_pub_crate)] + +pub(crate) struct Baz { + pub(crate) field: u8, +} + +mod outer { + mod inner { + // do these things _really_ need to be `pub(crate)`? + pub(crate) struct Foo; + pub(crate) trait Baz { + fn the_goods(&self); + fn secret_thing(&self); + } + impl Baz for crate::Baz { + fn the_goods(&self) {} + fn secret_thing(&self) {} + } + pub(crate) fn foo() {} + pub(crate) fn bar() { + foo(); + let _ = Foo; + let x = crate::Baz { field: 3 }; + x.the_goods(); + x.secret_thing(); + } + } + pub fn main() { + inner::bar(); + } +} + +fn main() { + crate::outer::main(); +} diff --git a/tests/ui/unneeded_pub_crate.stderr b/tests/ui/unneeded_pub_crate.stderr new file mode 100644 index 000000000000..75d013b0e5e7 --- /dev/null +++ b/tests/ui/unneeded_pub_crate.stderr @@ -0,0 +1,22 @@ +error: pub(crate) item is never used outside of its defining module + --> $DIR/unneeded_pub_crate.rs:21:9 + | +LL | pub(crate) fn foo() {} + | ^^^^^^^^^^ help: consider removing pub(crate) + | + = note: `-D clippy::unneeded-pub-crate` implied by `-D warnings` + +error: pub(crate) item is never used outside of its defining module + --> $DIR/unneeded_pub_crate.rs:5:1 + | +LL | pub(crate) struct Baz { + | ^^^^^^^^^^ help: consider removing pub(crate) + +error: pub(crate) item is never used outside of its defining module + --> $DIR/unneeded_pub_crate.rs:13:9 + | +LL | pub(crate) trait Baz { + | ^^^^^^^^^^ help: consider removing pub(crate) + +error: aborting due to 3 previous errors + From b9dc175b83462a257e0729b33d6514a4b78a99bc Mon Sep 17 00:00:00 2001 From: ember arlynx Date: Tue, 20 Oct 2020 22:58:46 -0400 Subject: [PATCH 2/5] check more things for unneeded this includes struct fields, structs/enums/unions, and methods. a bit overzealous right now, actually. --- clippy_lints/src/unneeded_pub_crate.rs | 187 +++++++++++++++++++++---- tests/ui/unneeded_pub_crate.fixed | 21 ++- tests/ui/unneeded_pub_crate.rs | 17 ++- tests/ui/unneeded_pub_crate.stderr | 48 ++++++- tests/ui/unneeded_pub_crate.stdout | 41 ++++++ 5 files changed, 275 insertions(+), 39 deletions(-) create mode 100644 tests/ui/unneeded_pub_crate.stdout diff --git a/clippy_lints/src/unneeded_pub_crate.rs b/clippy_lints/src/unneeded_pub_crate.rs index 034dc4ce2f51..fc609b27323a 100644 --- a/clippy_lints/src/unneeded_pub_crate.rs +++ b/clippy_lints/src/unneeded_pub_crate.rs @@ -1,5 +1,5 @@ use crate::utils::span_lint_and_then; -use rustc_data_structures::fx::FxHashMap; +use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_errors::Applicability; use rustc_hir as hir; use rustc_hir::def::Res; @@ -9,7 +9,7 @@ use rustc_hir::CRATE_HIR_ID; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::{self, TyCtxt}; use rustc_session::{declare_tool_lint, impl_lint_pass}; -use rustc_span::Span; +use rustc_span::{Span, Symbol}; declare_clippy_lint! { /// **What it does:** @@ -53,7 +53,7 @@ declare_clippy_lint! { "Using `pub(crate)` visibility on items that are only accessed from within the module that contains the item." } -#[derive(Default)] +#[derive(Default, Debug)] pub struct UnneededPubCrate { watched_item_map: FxHashMap, current_module_path: Vec, @@ -63,15 +63,19 @@ impl_lint_pass!(UnneededPubCrate => [UNNEEDED_PUB_CRATE]); /// An item with `pub(crate)` visibiliy that we're watching to see if it's /// referenced in ways where the visibility was useful. +#[derive(Debug, Clone)] struct WatchedItem { enclosing_module: hir::HirId, status: WatchStatus, /// The span of the visibility modifier span: Span, + // the fates of these watched items are intertwined; if any of them become + // `CrateReference`, they all are considered `CrateReference`. + linked_fate: Vec, } /// The status of an item we're watching, accumulated while we check the HIR. -#[derive(Copy, Clone)] +#[derive(Copy, Clone, Debug)] enum WatchStatus { /// An item starts off unreferenced. If it nevers leaves this state, that /// means the item is very dead. @@ -124,19 +128,57 @@ impl<'tcx> Visitor<'tcx> for UseScanner<'tcx> { .expect("mismatched push/pop in UnneededPubCrate::check_mod_post"); } + fn visit_variant_data( + &mut self, + vd: &'tcx hir::VariantData<'tcx>, + _name: Symbol, + _generics: &'tcx hir::Generics<'tcx>, + parent_id: hir::HirId, + _span: Span, + ) { + if let Some(hir_id) = vd.ctor_hir_id() { + self.examine_use(self.tcx.hir().local_def_id(hir_id).to_def_id(), parent_id) + } + } + fn visit_expr(&mut self, expr: &'tcx hir::Expr<'tcx>) { let def = match expr.kind { hir::ExprKind::MethodCall(..) => self .maybe_typeck_results .and_then(|typeck_results| typeck_results.type_dependent_def(expr.hir_id)) .map(|(kind, def_id)| (kind, def_id)), + hir::ExprKind::Struct(qpath, fields, _base) => { + if let Some(hir::Node::Item(hir::Item { + kind: hir::ItemKind::Struct(variant_data, _generics), + hir_id: struct_hir_id, + .. + })) = self + .maybe_typeck_results + .and_then(|typeck_results| typeck_results.qpath_res(qpath, expr.hir_id).opt_def_id()) + .and_then(|def_id| self.tcx.hir().get_if_local(def_id)) + { + self.examine_use(self.tcx.hir().local_def_id(*struct_hir_id).to_def_id(), expr.hir_id); + // TODO: match up by name, not assume the positions are good? + for (field, decl) in fields.iter().zip(variant_data.fields()) { + self.examine_use(self.tcx.hir().local_def_id(decl.hir_id).to_def_id(), expr.hir_id); + if field.ident.name != decl.ident.name { + eprintln!("ERROR: fields mismatch :("); + } + } + } + None + }, _ => None, }; if let Some((_kind, def_id)) = def { - self.examine_use(def_id); - } else { - intravisit::walk_expr(self, expr); + self.examine_use(def_id, expr.hir_id); } + intravisit::walk_expr(self, expr); + } + + fn visit_trait_ref(&mut self, item: &'tcx hir::TraitRef<'tcx>) { + self.examine_use(item.trait_def_id().expect("impl of a non-trait? what?"), CRATE_HIR_ID); + intravisit::walk_trait_ref(self, item); } fn visit_qpath(&mut self, qpath: &'tcx hir::QPath<'tcx>, hir_id: hir::HirId, span: Span) { @@ -150,12 +192,12 @@ impl<'tcx> Visitor<'tcx> for UseScanner<'tcx> { .and_then(|typeck_results| typeck_results.type_dependent_def(hir_id)), }; if let Some((_kind, def_id)) = def { - self.examine_use(def_id); + self.examine_use(def_id, hir_id); } intravisit::walk_qpath(self, qpath, hir_id, span); } - fn visit_path(&mut self, path: &'tcx hir::Path<'tcx>, _hir_id: hir::HirId) { + /*fn visit_path(&mut self, path: &'tcx hir::Path<'tcx>, _hir_id: hir::HirId) { match path.res { Res::Def(_kind, def_id) => { self.examine_use(def_id); @@ -163,38 +205,73 @@ impl<'tcx> Visitor<'tcx> for UseScanner<'tcx> { _ => {}, } intravisit::walk_path(self, path); - } + }*/ } impl<'tcx> UseScanner<'tcx> { - fn examine_use(&mut self, def_id: DefId) { + fn observe(&mut self, what: hir::HirId, how: WatchStatus) { + let mut worklist = vec![what]; + let mut seen = FxHashSet::default(); + while let Some(work) = worklist.pop() { + seen.insert(work); + match self.watched_item_map.get_mut(&work) { + Some(watch_item) => { + watch_item.status.observe(how); + worklist.extend(watch_item.linked_fate.iter().cloned().filter(|e| !seen.contains(&e))) + }, + None => eprintln!("uh, why couldn't i find this? their fates are supposed to be linked!"), + } + } + } + + fn examine_use(&mut self, def_id: DefId, used_by: hir::HirId) { if let Some(node) = self.tcx.hir().get_if_local(def_id) { - match node { - // TODO: TraitItem, ImplItem, Field - hir::Node::ForeignItem(hir::ForeignItem { hir_id, .. }) | hir::Node::Item(hir::Item { hir_id, .. }) => { - if let Some(watch_item) = self.watched_item_map.get_mut(&hir_id) { - if self - .current_module_path - .iter() + print!("{:?} used in {}; ", def_id, self.tcx.hir().node_to_string(used_by)); + match node.hir_id() { + Some(hir_id) => { + if let Some(watch_item) = self.watched_item_map.get(&hir_id).map(WatchedItem::clone) { + if self.current_module_path.contains(&watch_item.enclosing_module) { // if the current path contains the // enclosing module of the watched item // somewhere, we would have been able to // reference it even if it weren't marked // `pub(crate)`. - .any(|mod_| *mod_ == watch_item.enclosing_module) - { - watch_item.status.observe(LocalReference); + println!("it's a local reference"); + self.observe(hir_id, LocalReference); } else { - watch_item.status.observe(CrateReference); + println!("it's a crate reference"); + self.observe(hir_id, CrateReference); } + } else { + println!("it's not tracked") } }, - _ => {}, + None => println!("ignoring use of {:?}, couldn't find hir_id", node), } + } else { + println!("ignoring use of {:?} from Not Here", def_id); } } } +impl UnneededPubCrate { + fn notice_variant_data<'tcx>(&mut self, vd: &hir::VariantData<'tcx>, span: Span, parent_id: hir::HirId) { + if let Some(ctor_hir_id) = vd.ctor_hir_id() { + self.watched_item_map.insert( + ctor_hir_id, + WatchedItem { + enclosing_module: *self.current_module_path.last().unwrap(), + status: Unreferenced, + span: span, + linked_fate: vec![parent_id], + }, + ); + self.watched_item_map + .get_mut(&parent_id) + .map(|item| item.linked_fate.push(ctor_hir_id)); + } + } +} impl<'tcx> LateLintPass<'tcx> for UnneededPubCrate { fn check_mod(&mut self, _cx: &LateContext<'tcx>, _mod: &'tcx hir::Mod<'tcx>, _span: Span, hir_id: hir::HirId) { self.current_module_path.push(hir_id); @@ -209,7 +286,6 @@ impl<'tcx> LateLintPass<'tcx> for UnneededPubCrate { ); } - // TODO: methods, struct fields, const/static, alias, modules. what else can have visibility? fn check_item(&mut self, cx: &LateContext<'tcx>, item: &hir::Item<'tcx>) { if matches!(item.vis.node, hir::VisibilityKind::Crate { .. }) && !cx.access_levels.is_exported(item.hir_id) { self.watched_item_map.insert( @@ -218,16 +294,73 @@ impl<'tcx> LateLintPass<'tcx> for UnneededPubCrate { enclosing_module: *self.current_module_path.last().unwrap(), status: Unreferenced, span: item.vis.span, + linked_fate: vec![], + }, + ); + match &item.kind { + hir::ItemKind::Union(vd, _generics) | hir::ItemKind::Struct(vd, _generics) => { + self.notice_variant_data(vd, item.vis.span, item.hir_id); + }, + hir::ItemKind::Enum(enum_def, _generics) => { + for variant in enum_def.variants { + self.notice_variant_data(&variant.data, item.vis.span, item.hir_id); + } + }, + _ => {}, + } + return; + } + } + + fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &hir::ImplItem<'tcx>) { + if matches!(item.vis.node, hir::VisibilityKind::Crate { .. }) && !cx.access_levels.is_exported(item.hir_id) { + self.watched_item_map.insert( + item.hir_id, + WatchedItem { + enclosing_module: *self.current_module_path.last().unwrap(), + status: Unreferenced, + span: item.vis.span, + linked_fate: vec![], }, ); } } + fn check_struct_field(&mut self, cx: &LateContext<'tcx>, item: &hir::StructField<'tcx>) { + if matches!(item.vis.node, hir::VisibilityKind::Crate { .. }) && !cx.access_levels.is_exported(item.hir_id) { + self.watched_item_map.insert( + item.hir_id, + WatchedItem { + enclosing_module: *self.current_module_path.last().unwrap(), + status: Unreferenced, + span: item.vis.span, + linked_fate: vec![], // link this to the struct itself? + }, + ); + } + } + + /* + fn check_variant(&mut self, cx: &LateContext<'tcx>, item: &hir::Variant<'tcx>) { + self.watched_item_map.insert( + item.data.ctor_hir_id(), + WatchedItem { + enclosing_module: *self.current_module_path.last().unwrap(), + status: Unreferenced, + span: + } + ) + }*/ + fn check_crate_post(&mut self, cx: &LateContext<'tcx>, crate_: &'tcx hir::Crate<'tcx>) { // ok, now that we have scanned the entire crate for things with // visibility and filled the watched item map, let's scan it again for // any uses of those items. let watched_item_map = std::mem::replace(&mut self.watched_item_map, FxHashMap::default()); + println!("about to look for:"); + for (k, v) in &watched_item_map { + println!("{:?} \t => {:?}", k, v); + } let mut use_scanner = UseScanner { tcx: cx.tcx, maybe_typeck_results: cx.maybe_typeck_results(), @@ -237,7 +370,7 @@ impl<'tcx> LateLintPass<'tcx> for UnneededPubCrate { intravisit::walk_crate(&mut use_scanner, crate_); - for (_, watched_item) in use_scanner.watched_item_map { + for (watched_id, watched_item) in use_scanner.watched_item_map { if let LocalReference = watched_item.status { span_lint_and_then( cx, @@ -254,6 +387,10 @@ impl<'tcx> LateLintPass<'tcx> for UnneededPubCrate { }, ); } + + if let Unreferenced = watched_item.status { + println!("why didn't i see {:?} aka {:?}?", watched_id, watched_item); + } } } } diff --git a/tests/ui/unneeded_pub_crate.fixed b/tests/ui/unneeded_pub_crate.fixed index 143f174f28b2..6cbf32fe697d 100644 --- a/tests/ui/unneeded_pub_crate.fixed +++ b/tests/ui/unneeded_pub_crate.fixed @@ -3,13 +3,13 @@ #![warn(clippy::unneeded_pub_crate)] struct Baz { - pub(crate) field: u8, + field: u8, } mod outer { mod inner { // do these things _really_ need to be `pub(crate)`? - pub(crate) struct Foo; + struct Foo; trait Baz { fn the_goods(&self); fn secret_thing(&self); @@ -19,19 +19,28 @@ mod outer { fn secret_thing(&self) {} } fn foo() {} - pub(crate) fn bar() { + pub(crate) fn bar() -> super::ReturnStruct { foo(); let _ = Foo; let x = crate::Baz { field: 3 }; x.the_goods(); x.secret_thing(); + super::ReturnStruct { + used_outside: 0, + not_used_outside: 0, + } } } - pub fn main() { - inner::bar(); + pub(crate) struct ReturnStruct { + pub(crate) used_outside: u8, + not_used_outside: u8, + } + pub fn main() -> ReturnStruct { + inner::bar() } } fn main() { - crate::outer::main(); + let value_outside = crate::outer::main(); + let _ = value_outside.used_outside; } diff --git a/tests/ui/unneeded_pub_crate.rs b/tests/ui/unneeded_pub_crate.rs index 09417073a148..7ac96aa78890 100644 --- a/tests/ui/unneeded_pub_crate.rs +++ b/tests/ui/unneeded_pub_crate.rs @@ -19,19 +19,28 @@ mod outer { fn secret_thing(&self) {} } pub(crate) fn foo() {} - pub(crate) fn bar() { + pub(crate) fn bar() -> super::ReturnStruct { foo(); let _ = Foo; let x = crate::Baz { field: 3 }; x.the_goods(); x.secret_thing(); + super::ReturnStruct { + used_outside: 0, + not_used_outside: 0, + } } } - pub fn main() { - inner::bar(); + pub(crate) struct ReturnStruct { + pub(crate) used_outside: u8, + pub(crate) not_used_outside: u8, + } + pub fn main() -> ReturnStruct { + inner::bar() } } fn main() { - crate::outer::main(); + let value_outside = crate::outer::main(); + let _ = value_outside.used_outside; } diff --git a/tests/ui/unneeded_pub_crate.stderr b/tests/ui/unneeded_pub_crate.stderr index 75d013b0e5e7..3b929241b874 100644 --- a/tests/ui/unneeded_pub_crate.stderr +++ b/tests/ui/unneeded_pub_crate.stderr @@ -1,11 +1,32 @@ +error[E0446]: crate-visible type `outer::ReturnStruct` in public interface + --> $DIR/unneeded_pub_crate.rs:38:5 + | +LL | pub(crate) struct ReturnStruct { + | ---------- `outer::ReturnStruct` declared as crate-visible +... +LL | pub fn main() -> ReturnStruct { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ can't leak crate-visible type + error: pub(crate) item is never used outside of its defining module - --> $DIR/unneeded_pub_crate.rs:21:9 + --> $DIR/unneeded_pub_crate.rs:6:5 | -LL | pub(crate) fn foo() {} - | ^^^^^^^^^^ help: consider removing pub(crate) +LL | pub(crate) field: u8, + | ^^^^^^^^^^ help: consider removing pub(crate) | = note: `-D clippy::unneeded-pub-crate` implied by `-D warnings` +error: pub(crate) item is never used outside of its defining module + --> $DIR/unneeded_pub_crate.rs:34:5 + | +LL | pub(crate) struct ReturnStruct { + | ^^^^^^^^^^ help: consider removing pub(crate) + +error: pub(crate) item is never used outside of its defining module + --> $DIR/unneeded_pub_crate.rs:12:9 + | +LL | pub(crate) struct Foo; + | ^^^^^^^^^^ help: consider removing pub(crate) + error: pub(crate) item is never used outside of its defining module --> $DIR/unneeded_pub_crate.rs:5:1 | @@ -18,5 +39,24 @@ error: pub(crate) item is never used outside of its defining module LL | pub(crate) trait Baz { | ^^^^^^^^^^ help: consider removing pub(crate) -error: aborting due to 3 previous errors +error: pub(crate) item is never used outside of its defining module + --> $DIR/unneeded_pub_crate.rs:21:9 + | +LL | pub(crate) fn foo() {} + | ^^^^^^^^^^ help: consider removing pub(crate) + +error: pub(crate) item is never used outside of its defining module + --> $DIR/unneeded_pub_crate.rs:35:9 + | +LL | pub(crate) used_outside: u8, + | ^^^^^^^^^^ help: consider removing pub(crate) + +error: pub(crate) item is never used outside of its defining module + --> $DIR/unneeded_pub_crate.rs:36:9 + | +LL | pub(crate) not_used_outside: u8, + | ^^^^^^^^^^ help: consider removing pub(crate) + +error: aborting due to 9 previous errors +For more information about this error, try `rustc --explain E0446`. diff --git a/tests/ui/unneeded_pub_crate.stdout b/tests/ui/unneeded_pub_crate.stdout new file mode 100644 index 000000000000..4c60eae97f45 --- /dev/null +++ b/tests/ui/unneeded_pub_crate.stdout @@ -0,0 +1,41 @@ +about to look for: +HirId { owner: DefId(0:3 ~ unneeded_pub_crate[317d]::Baz), local_id: 3 } \t => WatchedItem { enclosing_module: HirId { owner: DefId(0:0 ~ unneeded_pub_crate[317d]), local_id: 0 }, status: Unreferenced, span: $DIR/unneeded_pub_crate.rs:6:5: 6:15 (#0), linked_fate: [] } +HirId { owner: DefId(0:17 ~ unneeded_pub_crate[317d]::outer::ReturnStruct), local_id: 0 } \t => WatchedItem { enclosing_module: HirId { owner: DefId(0:5 ~ unneeded_pub_crate[317d]::outer), local_id: 0 }, status: Unreferenced, span: $DIR/unneeded_pub_crate.rs:34:5: 34:15 (#0), linked_fate: [] } +HirId { owner: DefId(0:7 ~ unneeded_pub_crate[317d]::outer::inner::Foo), local_id: 0 } \t => WatchedItem { enclosing_module: HirId { owner: DefId(0:6 ~ unneeded_pub_crate[317d]::outer::inner), local_id: 0 }, status: Unreferenced, span: $DIR/unneeded_pub_crate.rs:12:9: 12:19 (#0), linked_fate: [HirId { owner: DefId(0:7 ~ unneeded_pub_crate[317d]::outer::inner::Foo), local_id: 1 }] } +HirId { owner: DefId(0:3 ~ unneeded_pub_crate[317d]::Baz), local_id: 0 } \t => WatchedItem { enclosing_module: HirId { owner: DefId(0:0 ~ unneeded_pub_crate[317d]), local_id: 0 }, status: Unreferenced, span: $DIR/unneeded_pub_crate.rs:5:1: 5:11 (#0), linked_fate: [] } +HirId { owner: DefId(0:9 ~ unneeded_pub_crate[317d]::outer::inner::Baz), local_id: 0 } \t => WatchedItem { enclosing_module: HirId { owner: DefId(0:6 ~ unneeded_pub_crate[317d]::outer::inner), local_id: 0 }, status: Unreferenced, span: $DIR/unneeded_pub_crate.rs:13:9: 13:19 (#0), linked_fate: [] } +HirId { owner: DefId(0:15 ~ unneeded_pub_crate[317d]::outer::inner::foo), local_id: 0 } \t => WatchedItem { enclosing_module: HirId { owner: DefId(0:6 ~ unneeded_pub_crate[317d]::outer::inner), local_id: 0 }, status: Unreferenced, span: $DIR/unneeded_pub_crate.rs:21:9: 21:19 (#0), linked_fate: [] } +HirId { owner: DefId(0:17 ~ unneeded_pub_crate[317d]::outer::ReturnStruct), local_id: 3 } \t => WatchedItem { enclosing_module: HirId { owner: DefId(0:5 ~ unneeded_pub_crate[317d]::outer), local_id: 0 }, status: Unreferenced, span: $DIR/unneeded_pub_crate.rs:35:9: 35:19 (#0), linked_fate: [] } +HirId { owner: DefId(0:16 ~ unneeded_pub_crate[317d]::outer::inner::bar), local_id: 0 } \t => WatchedItem { enclosing_module: HirId { owner: DefId(0:6 ~ unneeded_pub_crate[317d]::outer::inner), local_id: 0 }, status: Unreferenced, span: $DIR/unneeded_pub_crate.rs:22:9: 22:19 (#0), linked_fate: [] } +HirId { owner: DefId(0:17 ~ unneeded_pub_crate[317d]::outer::ReturnStruct), local_id: 6 } \t => WatchedItem { enclosing_module: HirId { owner: DefId(0:5 ~ unneeded_pub_crate[317d]::outer), local_id: 0 }, status: Unreferenced, span: $DIR/unneeded_pub_crate.rs:36:9: 36:19 (#0), linked_fate: [] } +HirId { owner: DefId(0:7 ~ unneeded_pub_crate[317d]::outer::inner::Foo), local_id: 1 } \t => WatchedItem { enclosing_module: HirId { owner: DefId(0:6 ~ unneeded_pub_crate[317d]::outer::inner), local_id: 0 }, status: Unreferenced, span: $DIR/unneeded_pub_crate.rs:12:9: 12:19 (#0), linked_fate: [HirId { owner: DefId(0:7 ~ unneeded_pub_crate[317d]::outer::inner::Foo), local_id: 0 }] } +DefId(0:8 ~ unneeded_pub_crate[317d]::outer::inner::Foo::{constructor#0}) used in struct outer::inner::Foo (hir_id=HirId { owner: DefId(0:7 ~ unneeded_pub_crate[317d]::outer::inner::Foo), local_id: 0 }); it's a local reference +DefId(0:9 ~ unneeded_pub_crate[317d]::outer::inner::Baz) used in unknown node (hir_id=HirId { owner: DefId(0:0 ~ unneeded_pub_crate[317d]), local_id: 0 }); it's a local reference +DefId(0:3 ~ unneeded_pub_crate[317d]::Baz) used in type crate::Baz (hir_id=HirId { owner: DefId(0:12 ~ unneeded_pub_crate[317d]::outer::inner::{impl#0}), local_id: 3 }); it's a local reference +DefId(0:17 ~ unneeded_pub_crate[317d]::outer::ReturnStruct) used in type super::ReturnStruct (hir_id=HirId { owner: DefId(0:16 ~ unneeded_pub_crate[317d]::outer::inner::bar), local_id: 37 }); it's a local reference +DefId(0:15 ~ unneeded_pub_crate[317d]::outer::inner::foo) used in expr foo (hir_id=HirId { owner: DefId(0:16 ~ unneeded_pub_crate[317d]::outer::inner::bar), local_id: 4 }); it's a local reference +DefId(0:8 ~ unneeded_pub_crate[317d]::outer::inner::Foo::{constructor#0}) used in expr Foo (hir_id=HirId { owner: DefId(0:16 ~ unneeded_pub_crate[317d]::outer::inner::bar), local_id: 8 }); it's a local reference +DefId(0:3 ~ unneeded_pub_crate[317d]::Baz) used in expr crate::Baz { field: 3 } (hir_id=HirId { owner: DefId(0:16 ~ unneeded_pub_crate[317d]::outer::inner::bar), local_id: 15 }); it's a local reference +DefId(0:4 ~ unneeded_pub_crate[317d]::Baz::field) used in expr crate::Baz { field: 3 } (hir_id=HirId { owner: DefId(0:16 ~ unneeded_pub_crate[317d]::outer::inner::bar), local_id: 15 }); it's a local reference +DefId(0:3 ~ unneeded_pub_crate[317d]::Baz) used in expr crate::Baz { field: 3 } (hir_id=HirId { owner: DefId(0:16 ~ unneeded_pub_crate[317d]::outer::inner::bar), local_id: 15 }); it's a local reference +DefId(0:10 ~ unneeded_pub_crate[317d]::outer::inner::Baz::the_goods) used in expr x.the_goods() (hir_id=HirId { owner: DefId(0:16 ~ unneeded_pub_crate[317d]::outer::inner::bar), local_id: 21 }); it's not tracked +DefId(0:11 ~ unneeded_pub_crate[317d]::outer::inner::Baz::secret_thing) used in expr x.secret_thing() (hir_id=HirId { owner: DefId(0:16 ~ unneeded_pub_crate[317d]::outer::inner::bar), local_id: 26 }); it's not tracked +DefId(0:17 ~ unneeded_pub_crate[317d]::outer::ReturnStruct) used in expr super::ReturnStruct { + used_outside: 0, + not_used_outside: 0, + } (hir_id=HirId { owner: DefId(0:16 ~ unneeded_pub_crate[317d]::outer::inner::bar), local_id: 34 }); it's a local reference +DefId(0:18 ~ unneeded_pub_crate[317d]::outer::ReturnStruct::used_outside) used in expr super::ReturnStruct { + used_outside: 0, + not_used_outside: 0, + } (hir_id=HirId { owner: DefId(0:16 ~ unneeded_pub_crate[317d]::outer::inner::bar), local_id: 34 }); it's a local reference +DefId(0:19 ~ unneeded_pub_crate[317d]::outer::ReturnStruct::not_used_outside) used in expr super::ReturnStruct { + used_outside: 0, + not_used_outside: 0, + } (hir_id=HirId { owner: DefId(0:16 ~ unneeded_pub_crate[317d]::outer::inner::bar), local_id: 34 }); it's a local reference +DefId(0:17 ~ unneeded_pub_crate[317d]::outer::ReturnStruct) used in expr super::ReturnStruct { + used_outside: 0, + not_used_outside: 0, + } (hir_id=HirId { owner: DefId(0:16 ~ unneeded_pub_crate[317d]::outer::inner::bar), local_id: 34 }); it's a local reference +DefId(0:17 ~ unneeded_pub_crate[317d]::outer::ReturnStruct) used in type ReturnStruct (hir_id=HirId { owner: DefId(0:20 ~ unneeded_pub_crate[317d]::outer::main), local_id: 7 }); it's a local reference +DefId(0:16 ~ unneeded_pub_crate[317d]::outer::inner::bar) used in expr inner::bar (hir_id=HirId { owner: DefId(0:20 ~ unneeded_pub_crate[317d]::outer::main), local_id: 3 }); it's a crate reference +DefId(0:20 ~ unneeded_pub_crate[317d]::outer::main) used in expr crate::outer::main (hir_id=HirId { owner: DefId(0:21 ~ unneeded_pub_crate[317d]::main), local_id: 6 }); it's not tracked From 7eb9deddd247c00b1d3f6b65e0bb277e1ba409ca Mon Sep 17 00:00:00 2001 From: ember arlynx Date: Wed, 21 Oct 2020 09:56:56 -0400 Subject: [PATCH 3/5] check fields and variants more thoroughly --- clippy_lints/src/unneeded_pub_crate.rs | 130 +++++++++++-------------- tests/ui/unneeded_pub_crate.fixed | 2 +- tests/ui/unneeded_pub_crate.rs | 2 +- tests/ui/unneeded_pub_crate.stderr | 24 +---- tests/ui/unneeded_pub_crate.stdout | 41 -------- 5 files changed, 61 insertions(+), 138 deletions(-) delete mode 100644 tests/ui/unneeded_pub_crate.stdout diff --git a/clippy_lints/src/unneeded_pub_crate.rs b/clippy_lints/src/unneeded_pub_crate.rs index fc609b27323a..8944fd19fa18 100644 --- a/clippy_lints/src/unneeded_pub_crate.rs +++ b/clippy_lints/src/unneeded_pub_crate.rs @@ -9,7 +9,7 @@ use rustc_hir::CRATE_HIR_ID; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::{self, TyCtxt}; use rustc_session::{declare_tool_lint, impl_lint_pass}; -use rustc_span::{Span, Symbol}; +use rustc_span::{symbol::Ident, Span, Symbol}; declare_clippy_lint! { /// **What it does:** @@ -142,43 +142,53 @@ impl<'tcx> Visitor<'tcx> for UseScanner<'tcx> { } fn visit_expr(&mut self, expr: &'tcx hir::Expr<'tcx>) { - let def = match expr.kind { - hir::ExprKind::MethodCall(..) => self - .maybe_typeck_results - .and_then(|typeck_results| typeck_results.type_dependent_def(expr.hir_id)) - .map(|(kind, def_id)| (kind, def_id)), - hir::ExprKind::Struct(qpath, fields, _base) => { - if let Some(hir::Node::Item(hir::Item { - kind: hir::ItemKind::Struct(variant_data, _generics), - hir_id: struct_hir_id, - .. - })) = self + match expr.kind { + hir::ExprKind::MethodCall(..) => { + self.maybe_typeck_results + .and_then(|typeck_results| typeck_results.type_dependent_def(expr.hir_id)) + .map(|(_kind, def_id)| self.examine_use(def_id, expr.hir_id)); + }, + hir::ExprKind::Struct(_qpath, fields, _base) => { + if let Some(ty::Adt(adt_def, _substs)) = self .maybe_typeck_results - .and_then(|typeck_results| typeck_results.qpath_res(qpath, expr.hir_id).opt_def_id()) - .and_then(|def_id| self.tcx.hir().get_if_local(def_id)) + .map(|typeck_results| typeck_results.expr_ty(expr).kind()) { - self.examine_use(self.tcx.hir().local_def_id(*struct_hir_id).to_def_id(), expr.hir_id); - // TODO: match up by name, not assume the positions are good? - for (field, decl) in fields.iter().zip(variant_data.fields()) { - self.examine_use(self.tcx.hir().local_def_id(decl.hir_id).to_def_id(), expr.hir_id); - if field.ident.name != decl.ident.name { - eprintln!("ERROR: fields mismatch :("); - } + self.examine_use(adt_def.did, expr.hir_id); + let ty_fields = adt_def + .all_fields() + .map(|f| (f.ident, f)) + .collect::>(); + for field in fields.iter() { + let ty_field = ty_fields.get(&field.ident).expect("referenced a nonexistent field?"); + self.examine_use(ty_field.did, expr.hir_id); } } - None }, - _ => None, + hir::ExprKind::Field(base, field) => { + if let Some(ty::Adt(adt_def, _substs)) = self + .maybe_typeck_results + .map(|typeck_results| typeck_results.expr_ty(base).kind()) + { + self.examine_use(adt_def.did, expr.hir_id); + let our_field = adt_def + .all_fields() + .filter(|f| f.ident == field) + .next() + .expect("referenced a nonexistent field?"); + self.examine_use(our_field.did, expr.hir_id); + } + }, + _ => {}, }; - if let Some((_kind, def_id)) = def { - self.examine_use(def_id, expr.hir_id); - } intravisit::walk_expr(self, expr); } - fn visit_trait_ref(&mut self, item: &'tcx hir::TraitRef<'tcx>) { - self.examine_use(item.trait_def_id().expect("impl of a non-trait? what?"), CRATE_HIR_ID); - intravisit::walk_trait_ref(self, item); + fn visit_trait_ref(&mut self, trait_ref: &'tcx hir::TraitRef<'tcx>) { + self.examine_use( + trait_ref.trait_def_id().expect("impl of a non-trait? what?"), + CRATE_HIR_ID, + ); + intravisit::walk_trait_ref(self, trait_ref); } fn visit_qpath(&mut self, qpath: &'tcx hir::QPath<'tcx>, hir_id: hir::HirId, span: Span) { @@ -219,14 +229,14 @@ impl<'tcx> UseScanner<'tcx> { watch_item.status.observe(how); worklist.extend(watch_item.linked_fate.iter().cloned().filter(|e| !seen.contains(&e))) }, - None => eprintln!("uh, why couldn't i find this? their fates are supposed to be linked!"), + None => panic!("uh, why couldn't i find this? their fates are supposed to be linked!"), } } } - fn examine_use(&mut self, def_id: DefId, used_by: hir::HirId) { + fn examine_use(&mut self, def_id: DefId, _used_by: hir::HirId) { + // that _used_by is super useful when debugging :) if let Some(node) = self.tcx.hir().get_if_local(def_id) { - print!("{:?} used in {}; ", def_id, self.tcx.hir().node_to_string(used_by)); match node.hir_id() { Some(hir_id) => { if let Some(watch_item) = self.watched_item_map.get(&hir_id).map(WatchedItem::clone) { @@ -236,25 +246,24 @@ impl<'tcx> UseScanner<'tcx> { // somewhere, we would have been able to // reference it even if it weren't marked // `pub(crate)`. - println!("it's a local reference"); self.observe(hir_id, LocalReference); } else { - println!("it's a crate reference"); self.observe(hir_id, CrateReference); } } else { - println!("it's not tracked") + // not a tracked item } }, - None => println!("ignoring use of {:?}, couldn't find hir_id", node), + None => { /* ignore it if no HIR id */ } } } else { - println!("ignoring use of {:?} from Not Here", def_id); + // ignore it if not a local item } } } impl UnneededPubCrate { + /// link the fate of the constructors of data to the type definining them. fn notice_variant_data<'tcx>(&mut self, vd: &hir::VariantData<'tcx>, span: Span, parent_id: hir::HirId) { if let Some(ctor_hir_id) = vd.ctor_hir_id() { self.watched_item_map.insert( @@ -270,6 +279,17 @@ impl UnneededPubCrate { .get_mut(&parent_id) .map(|item| item.linked_fate.push(ctor_hir_id)); } + for field in vd.fields() { + self.watched_item_map.insert( + field.hir_id, + WatchedItem { + enclosing_module: *self.current_module_path.last().unwrap(), + status: Unreferenced, + span: field.vis.span, + linked_fate: vec![], // should the fields be linked as well? + } + ); + } } } impl<'tcx> LateLintPass<'tcx> for UnneededPubCrate { @@ -326,41 +346,11 @@ impl<'tcx> LateLintPass<'tcx> for UnneededPubCrate { } } - fn check_struct_field(&mut self, cx: &LateContext<'tcx>, item: &hir::StructField<'tcx>) { - if matches!(item.vis.node, hir::VisibilityKind::Crate { .. }) && !cx.access_levels.is_exported(item.hir_id) { - self.watched_item_map.insert( - item.hir_id, - WatchedItem { - enclosing_module: *self.current_module_path.last().unwrap(), - status: Unreferenced, - span: item.vis.span, - linked_fate: vec![], // link this to the struct itself? - }, - ); - } - } - - /* - fn check_variant(&mut self, cx: &LateContext<'tcx>, item: &hir::Variant<'tcx>) { - self.watched_item_map.insert( - item.data.ctor_hir_id(), - WatchedItem { - enclosing_module: *self.current_module_path.last().unwrap(), - status: Unreferenced, - span: - } - ) - }*/ - fn check_crate_post(&mut self, cx: &LateContext<'tcx>, crate_: &'tcx hir::Crate<'tcx>) { // ok, now that we have scanned the entire crate for things with // visibility and filled the watched item map, let's scan it again for // any uses of those items. let watched_item_map = std::mem::replace(&mut self.watched_item_map, FxHashMap::default()); - println!("about to look for:"); - for (k, v) in &watched_item_map { - println!("{:?} \t => {:?}", k, v); - } let mut use_scanner = UseScanner { tcx: cx.tcx, maybe_typeck_results: cx.maybe_typeck_results(), @@ -370,7 +360,7 @@ impl<'tcx> LateLintPass<'tcx> for UnneededPubCrate { intravisit::walk_crate(&mut use_scanner, crate_); - for (watched_id, watched_item) in use_scanner.watched_item_map { + for (_watched_id, watched_item) in use_scanner.watched_item_map { if let LocalReference = watched_item.status { span_lint_and_then( cx, @@ -387,10 +377,6 @@ impl<'tcx> LateLintPass<'tcx> for UnneededPubCrate { }, ); } - - if let Unreferenced = watched_item.status { - println!("why didn't i see {:?} aka {:?}?", watched_id, watched_item); - } } } } diff --git a/tests/ui/unneeded_pub_crate.fixed b/tests/ui/unneeded_pub_crate.fixed index 6cbf32fe697d..0e093d4965c6 100644 --- a/tests/ui/unneeded_pub_crate.fixed +++ b/tests/ui/unneeded_pub_crate.fixed @@ -31,7 +31,7 @@ mod outer { } } } - pub(crate) struct ReturnStruct { + struct ReturnStruct { pub(crate) used_outside: u8, not_used_outside: u8, } diff --git a/tests/ui/unneeded_pub_crate.rs b/tests/ui/unneeded_pub_crate.rs index 7ac96aa78890..170c099ad6b1 100644 --- a/tests/ui/unneeded_pub_crate.rs +++ b/tests/ui/unneeded_pub_crate.rs @@ -35,7 +35,7 @@ mod outer { pub(crate) used_outside: u8, pub(crate) not_used_outside: u8, } - pub fn main() -> ReturnStruct { + pub(crate) fn main() -> ReturnStruct { inner::bar() } } diff --git a/tests/ui/unneeded_pub_crate.stderr b/tests/ui/unneeded_pub_crate.stderr index 3b929241b874..e85556ce097e 100644 --- a/tests/ui/unneeded_pub_crate.stderr +++ b/tests/ui/unneeded_pub_crate.stderr @@ -1,12 +1,3 @@ -error[E0446]: crate-visible type `outer::ReturnStruct` in public interface - --> $DIR/unneeded_pub_crate.rs:38:5 - | -LL | pub(crate) struct ReturnStruct { - | ---------- `outer::ReturnStruct` declared as crate-visible -... -LL | pub fn main() -> ReturnStruct { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ can't leak crate-visible type - error: pub(crate) item is never used outside of its defining module --> $DIR/unneeded_pub_crate.rs:6:5 | @@ -15,12 +6,6 @@ LL | pub(crate) field: u8, | = note: `-D clippy::unneeded-pub-crate` implied by `-D warnings` -error: pub(crate) item is never used outside of its defining module - --> $DIR/unneeded_pub_crate.rs:34:5 - | -LL | pub(crate) struct ReturnStruct { - | ^^^^^^^^^^ help: consider removing pub(crate) - error: pub(crate) item is never used outside of its defining module --> $DIR/unneeded_pub_crate.rs:12:9 | @@ -45,18 +30,11 @@ error: pub(crate) item is never used outside of its defining module LL | pub(crate) fn foo() {} | ^^^^^^^^^^ help: consider removing pub(crate) -error: pub(crate) item is never used outside of its defining module - --> $DIR/unneeded_pub_crate.rs:35:9 - | -LL | pub(crate) used_outside: u8, - | ^^^^^^^^^^ help: consider removing pub(crate) - error: pub(crate) item is never used outside of its defining module --> $DIR/unneeded_pub_crate.rs:36:9 | LL | pub(crate) not_used_outside: u8, | ^^^^^^^^^^ help: consider removing pub(crate) -error: aborting due to 9 previous errors +error: aborting due to 6 previous errors -For more information about this error, try `rustc --explain E0446`. diff --git a/tests/ui/unneeded_pub_crate.stdout b/tests/ui/unneeded_pub_crate.stdout deleted file mode 100644 index 4c60eae97f45..000000000000 --- a/tests/ui/unneeded_pub_crate.stdout +++ /dev/null @@ -1,41 +0,0 @@ -about to look for: -HirId { owner: DefId(0:3 ~ unneeded_pub_crate[317d]::Baz), local_id: 3 } \t => WatchedItem { enclosing_module: HirId { owner: DefId(0:0 ~ unneeded_pub_crate[317d]), local_id: 0 }, status: Unreferenced, span: $DIR/unneeded_pub_crate.rs:6:5: 6:15 (#0), linked_fate: [] } -HirId { owner: DefId(0:17 ~ unneeded_pub_crate[317d]::outer::ReturnStruct), local_id: 0 } \t => WatchedItem { enclosing_module: HirId { owner: DefId(0:5 ~ unneeded_pub_crate[317d]::outer), local_id: 0 }, status: Unreferenced, span: $DIR/unneeded_pub_crate.rs:34:5: 34:15 (#0), linked_fate: [] } -HirId { owner: DefId(0:7 ~ unneeded_pub_crate[317d]::outer::inner::Foo), local_id: 0 } \t => WatchedItem { enclosing_module: HirId { owner: DefId(0:6 ~ unneeded_pub_crate[317d]::outer::inner), local_id: 0 }, status: Unreferenced, span: $DIR/unneeded_pub_crate.rs:12:9: 12:19 (#0), linked_fate: [HirId { owner: DefId(0:7 ~ unneeded_pub_crate[317d]::outer::inner::Foo), local_id: 1 }] } -HirId { owner: DefId(0:3 ~ unneeded_pub_crate[317d]::Baz), local_id: 0 } \t => WatchedItem { enclosing_module: HirId { owner: DefId(0:0 ~ unneeded_pub_crate[317d]), local_id: 0 }, status: Unreferenced, span: $DIR/unneeded_pub_crate.rs:5:1: 5:11 (#0), linked_fate: [] } -HirId { owner: DefId(0:9 ~ unneeded_pub_crate[317d]::outer::inner::Baz), local_id: 0 } \t => WatchedItem { enclosing_module: HirId { owner: DefId(0:6 ~ unneeded_pub_crate[317d]::outer::inner), local_id: 0 }, status: Unreferenced, span: $DIR/unneeded_pub_crate.rs:13:9: 13:19 (#0), linked_fate: [] } -HirId { owner: DefId(0:15 ~ unneeded_pub_crate[317d]::outer::inner::foo), local_id: 0 } \t => WatchedItem { enclosing_module: HirId { owner: DefId(0:6 ~ unneeded_pub_crate[317d]::outer::inner), local_id: 0 }, status: Unreferenced, span: $DIR/unneeded_pub_crate.rs:21:9: 21:19 (#0), linked_fate: [] } -HirId { owner: DefId(0:17 ~ unneeded_pub_crate[317d]::outer::ReturnStruct), local_id: 3 } \t => WatchedItem { enclosing_module: HirId { owner: DefId(0:5 ~ unneeded_pub_crate[317d]::outer), local_id: 0 }, status: Unreferenced, span: $DIR/unneeded_pub_crate.rs:35:9: 35:19 (#0), linked_fate: [] } -HirId { owner: DefId(0:16 ~ unneeded_pub_crate[317d]::outer::inner::bar), local_id: 0 } \t => WatchedItem { enclosing_module: HirId { owner: DefId(0:6 ~ unneeded_pub_crate[317d]::outer::inner), local_id: 0 }, status: Unreferenced, span: $DIR/unneeded_pub_crate.rs:22:9: 22:19 (#0), linked_fate: [] } -HirId { owner: DefId(0:17 ~ unneeded_pub_crate[317d]::outer::ReturnStruct), local_id: 6 } \t => WatchedItem { enclosing_module: HirId { owner: DefId(0:5 ~ unneeded_pub_crate[317d]::outer), local_id: 0 }, status: Unreferenced, span: $DIR/unneeded_pub_crate.rs:36:9: 36:19 (#0), linked_fate: [] } -HirId { owner: DefId(0:7 ~ unneeded_pub_crate[317d]::outer::inner::Foo), local_id: 1 } \t => WatchedItem { enclosing_module: HirId { owner: DefId(0:6 ~ unneeded_pub_crate[317d]::outer::inner), local_id: 0 }, status: Unreferenced, span: $DIR/unneeded_pub_crate.rs:12:9: 12:19 (#0), linked_fate: [HirId { owner: DefId(0:7 ~ unneeded_pub_crate[317d]::outer::inner::Foo), local_id: 0 }] } -DefId(0:8 ~ unneeded_pub_crate[317d]::outer::inner::Foo::{constructor#0}) used in struct outer::inner::Foo (hir_id=HirId { owner: DefId(0:7 ~ unneeded_pub_crate[317d]::outer::inner::Foo), local_id: 0 }); it's a local reference -DefId(0:9 ~ unneeded_pub_crate[317d]::outer::inner::Baz) used in unknown node (hir_id=HirId { owner: DefId(0:0 ~ unneeded_pub_crate[317d]), local_id: 0 }); it's a local reference -DefId(0:3 ~ unneeded_pub_crate[317d]::Baz) used in type crate::Baz (hir_id=HirId { owner: DefId(0:12 ~ unneeded_pub_crate[317d]::outer::inner::{impl#0}), local_id: 3 }); it's a local reference -DefId(0:17 ~ unneeded_pub_crate[317d]::outer::ReturnStruct) used in type super::ReturnStruct (hir_id=HirId { owner: DefId(0:16 ~ unneeded_pub_crate[317d]::outer::inner::bar), local_id: 37 }); it's a local reference -DefId(0:15 ~ unneeded_pub_crate[317d]::outer::inner::foo) used in expr foo (hir_id=HirId { owner: DefId(0:16 ~ unneeded_pub_crate[317d]::outer::inner::bar), local_id: 4 }); it's a local reference -DefId(0:8 ~ unneeded_pub_crate[317d]::outer::inner::Foo::{constructor#0}) used in expr Foo (hir_id=HirId { owner: DefId(0:16 ~ unneeded_pub_crate[317d]::outer::inner::bar), local_id: 8 }); it's a local reference -DefId(0:3 ~ unneeded_pub_crate[317d]::Baz) used in expr crate::Baz { field: 3 } (hir_id=HirId { owner: DefId(0:16 ~ unneeded_pub_crate[317d]::outer::inner::bar), local_id: 15 }); it's a local reference -DefId(0:4 ~ unneeded_pub_crate[317d]::Baz::field) used in expr crate::Baz { field: 3 } (hir_id=HirId { owner: DefId(0:16 ~ unneeded_pub_crate[317d]::outer::inner::bar), local_id: 15 }); it's a local reference -DefId(0:3 ~ unneeded_pub_crate[317d]::Baz) used in expr crate::Baz { field: 3 } (hir_id=HirId { owner: DefId(0:16 ~ unneeded_pub_crate[317d]::outer::inner::bar), local_id: 15 }); it's a local reference -DefId(0:10 ~ unneeded_pub_crate[317d]::outer::inner::Baz::the_goods) used in expr x.the_goods() (hir_id=HirId { owner: DefId(0:16 ~ unneeded_pub_crate[317d]::outer::inner::bar), local_id: 21 }); it's not tracked -DefId(0:11 ~ unneeded_pub_crate[317d]::outer::inner::Baz::secret_thing) used in expr x.secret_thing() (hir_id=HirId { owner: DefId(0:16 ~ unneeded_pub_crate[317d]::outer::inner::bar), local_id: 26 }); it's not tracked -DefId(0:17 ~ unneeded_pub_crate[317d]::outer::ReturnStruct) used in expr super::ReturnStruct { - used_outside: 0, - not_used_outside: 0, - } (hir_id=HirId { owner: DefId(0:16 ~ unneeded_pub_crate[317d]::outer::inner::bar), local_id: 34 }); it's a local reference -DefId(0:18 ~ unneeded_pub_crate[317d]::outer::ReturnStruct::used_outside) used in expr super::ReturnStruct { - used_outside: 0, - not_used_outside: 0, - } (hir_id=HirId { owner: DefId(0:16 ~ unneeded_pub_crate[317d]::outer::inner::bar), local_id: 34 }); it's a local reference -DefId(0:19 ~ unneeded_pub_crate[317d]::outer::ReturnStruct::not_used_outside) used in expr super::ReturnStruct { - used_outside: 0, - not_used_outside: 0, - } (hir_id=HirId { owner: DefId(0:16 ~ unneeded_pub_crate[317d]::outer::inner::bar), local_id: 34 }); it's a local reference -DefId(0:17 ~ unneeded_pub_crate[317d]::outer::ReturnStruct) used in expr super::ReturnStruct { - used_outside: 0, - not_used_outside: 0, - } (hir_id=HirId { owner: DefId(0:16 ~ unneeded_pub_crate[317d]::outer::inner::bar), local_id: 34 }); it's a local reference -DefId(0:17 ~ unneeded_pub_crate[317d]::outer::ReturnStruct) used in type ReturnStruct (hir_id=HirId { owner: DefId(0:20 ~ unneeded_pub_crate[317d]::outer::main), local_id: 7 }); it's a local reference -DefId(0:16 ~ unneeded_pub_crate[317d]::outer::inner::bar) used in expr inner::bar (hir_id=HirId { owner: DefId(0:20 ~ unneeded_pub_crate[317d]::outer::main), local_id: 3 }); it's a crate reference -DefId(0:20 ~ unneeded_pub_crate[317d]::outer::main) used in expr crate::outer::main (hir_id=HirId { owner: DefId(0:21 ~ unneeded_pub_crate[317d]::main), local_id: 6 }); it's not tracked From 4ca9c1c66618af22e5f3a21b701a4755eac4c362 Mon Sep 17 00:00:00 2001 From: ember arlynx Date: Wed, 21 Oct 2020 10:08:10 -0400 Subject: [PATCH 4/5] fix ICE --- clippy_lints/src/unneeded_pub_crate.rs | 43 +++++++++++++------------- tests/ui/unneeded_pub_crate.fixed | 4 +-- 2 files changed, 23 insertions(+), 24 deletions(-) diff --git a/clippy_lints/src/unneeded_pub_crate.rs b/clippy_lints/src/unneeded_pub_crate.rs index 8944fd19fa18..5379b004d7aa 100644 --- a/clippy_lints/src/unneeded_pub_crate.rs +++ b/clippy_lints/src/unneeded_pub_crate.rs @@ -159,8 +159,9 @@ impl<'tcx> Visitor<'tcx> for UseScanner<'tcx> { .map(|f| (f.ident, f)) .collect::>(); for field in fields.iter() { - let ty_field = ty_fields.get(&field.ident).expect("referenced a nonexistent field?"); - self.examine_use(ty_field.did, expr.hir_id); + if let Some(ty_field) = ty_fields.get(&field.ident) { + self.examine_use(ty_field.did, expr.hir_id); + } } } }, @@ -170,12 +171,9 @@ impl<'tcx> Visitor<'tcx> for UseScanner<'tcx> { .map(|typeck_results| typeck_results.expr_ty(base).kind()) { self.examine_use(adt_def.did, expr.hir_id); - let our_field = adt_def - .all_fields() - .filter(|f| f.ident == field) - .next() - .expect("referenced a nonexistent field?"); - self.examine_use(our_field.did, expr.hir_id); + if let Some(our_field) = adt_def.all_fields().filter(|f| f.ident == field).next() { + self.examine_use(our_field.did, expr.hir_id); + } } }, _ => {}, @@ -184,10 +182,9 @@ impl<'tcx> Visitor<'tcx> for UseScanner<'tcx> { } fn visit_trait_ref(&mut self, trait_ref: &'tcx hir::TraitRef<'tcx>) { - self.examine_use( - trait_ref.trait_def_id().expect("impl of a non-trait? what?"), - CRATE_HIR_ID, - ); + if let Some(trait_ref) = trait_ref.trait_def_id() { + self.examine_use(trait_ref, CRATE_HIR_ID); + } intravisit::walk_trait_ref(self, trait_ref); } @@ -254,7 +251,7 @@ impl<'tcx> UseScanner<'tcx> { // not a tracked item } }, - None => { /* ignore it if no HIR id */ } + None => { /* ignore it if no HIR id */ }, } } else { // ignore it if not a local item @@ -280,15 +277,17 @@ impl UnneededPubCrate { .map(|item| item.linked_fate.push(ctor_hir_id)); } for field in vd.fields() { - self.watched_item_map.insert( - field.hir_id, - WatchedItem { - enclosing_module: *self.current_module_path.last().unwrap(), - status: Unreferenced, - span: field.vis.span, - linked_fate: vec![], // should the fields be linked as well? - } - ); + if matches!(field.vis.node, hir::VisibilityKind::Crate{..}) { + self.watched_item_map.insert( + field.hir_id, + WatchedItem { + enclosing_module: *self.current_module_path.last().unwrap(), + status: Unreferenced, + span: field.vis.span, + linked_fate: vec![], // should the fields be linked as well? + }, + ); + } } } } diff --git a/tests/ui/unneeded_pub_crate.fixed b/tests/ui/unneeded_pub_crate.fixed index 0e093d4965c6..25d754862796 100644 --- a/tests/ui/unneeded_pub_crate.fixed +++ b/tests/ui/unneeded_pub_crate.fixed @@ -31,11 +31,11 @@ mod outer { } } } - struct ReturnStruct { + pub(crate) struct ReturnStruct { pub(crate) used_outside: u8, not_used_outside: u8, } - pub fn main() -> ReturnStruct { + pub(crate) fn main() -> ReturnStruct { inner::bar() } } From 6c6dd76cf9810876bd373bf16d9001ae2a6e62d5 Mon Sep 17 00:00:00 2001 From: ember arlynx Date: Wed, 21 Oct 2020 10:44:21 -0400 Subject: [PATCH 5/5] try testing positional fields --- clippy_lints/src/unneeded_pub_crate.rs | 2 +- tests/ui/unneeded_pub_crate.fixed | 6 ++++-- tests/ui/unneeded_pub_crate.rs | 6 ++++-- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/unneeded_pub_crate.rs b/clippy_lints/src/unneeded_pub_crate.rs index 5379b004d7aa..88019be6534d 100644 --- a/clippy_lints/src/unneeded_pub_crate.rs +++ b/clippy_lints/src/unneeded_pub_crate.rs @@ -21,7 +21,7 @@ declare_clippy_lint! { /// /// You may be decieved into thinking an item is used far away, when it is not! /// - /// **Known problems:** Does not check methods, const/static items, struct/enum/union fields, or type aliases. + /// **Known problems:** Does not check positional fields. /// /// **Example:** /// diff --git a/tests/ui/unneeded_pub_crate.fixed b/tests/ui/unneeded_pub_crate.fixed index 25d754862796..d551c3ed101e 100644 --- a/tests/ui/unneeded_pub_crate.fixed +++ b/tests/ui/unneeded_pub_crate.fixed @@ -30,17 +30,19 @@ mod outer { not_used_outside: 0, } } + pub(crate) struct S1(u8, pub(crate) u8); } pub(crate) struct ReturnStruct { pub(crate) used_outside: u8, not_used_outside: u8, } - pub(crate) fn main() -> ReturnStruct { + pub(crate) fn main() -> (ReturnStruct, inner::S1) { inner::bar() } } fn main() { - let value_outside = crate::outer::main(); + let (value_outside, s1) = crate::outer::main(); + s1.1 = 42; let _ = value_outside.used_outside; } diff --git a/tests/ui/unneeded_pub_crate.rs b/tests/ui/unneeded_pub_crate.rs index 170c099ad6b1..6fd7de0ad56e 100644 --- a/tests/ui/unneeded_pub_crate.rs +++ b/tests/ui/unneeded_pub_crate.rs @@ -30,17 +30,19 @@ mod outer { not_used_outside: 0, } } + pub(crate) struct S1(pub(crate) u8, pub(crate) u8); } pub(crate) struct ReturnStruct { pub(crate) used_outside: u8, pub(crate) not_used_outside: u8, } - pub(crate) fn main() -> ReturnStruct { + pub(crate) fn main() -> (ReturnStruct, inner::S1) { inner::bar() } } fn main() { - let value_outside = crate::outer::main(); + let (value_outside, s1) = crate::outer::main(); + s1.1 = 42; let _ = value_outside.used_outside; }