diff --git a/CHANGELOG.md b/CHANGELOG.md index 816d25bcd93e..f9d764c946d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2009,6 +2009,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 20b38cbb6d0d..17c1387fceef 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -323,6 +323,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; @@ -892,6 +893,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, @@ -1166,6 +1168,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_late_pass(|| box undropped_manually_drops::UndroppedManuallyDrops); @@ -1290,6 +1293,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..88019be6534d --- /dev/null +++ b/clippy_lints/src/unneeded_pub_crate.rs @@ -0,0 +1,381 @@ +use crate::utils::span_lint_and_then; +use rustc_data_structures::fx::{FxHashMap, FxHashSet}; +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::{symbol::Ident, Span, Symbol}; + +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 positional fields. + /// + /// **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, Debug)] +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. +#[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, Debug)] +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_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>) { + 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 + .map(|typeck_results| typeck_results.expr_ty(expr).kind()) + { + 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() { + if let Some(ty_field) = ty_fields.get(&field.ident) { + self.examine_use(ty_field.did, expr.hir_id); + } + } + } + }, + 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); + if let Some(our_field) = adt_def.all_fields().filter(|f| f.ident == field).next() { + self.examine_use(our_field.did, expr.hir_id); + } + } + }, + _ => {}, + }; + intravisit::walk_expr(self, expr); + } + + fn visit_trait_ref(&mut self, trait_ref: &'tcx hir::TraitRef<'tcx>) { + 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); + } + + 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, hir_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 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 => 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) { + // that _used_by is super useful when debugging :) + if let Some(node) = self.tcx.hir().get_if_local(def_id) { + 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)`. + self.observe(hir_id, LocalReference); + } else { + self.observe(hir_id, CrateReference); + } + } else { + // not a tracked item + } + }, + None => { /* ignore it if no HIR id */ }, + } + } else { + // 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( + 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)); + } + for field in vd.fields() { + 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? + }, + ); + } + } + } +} +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 + ); + } + + 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, + 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_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_id, 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 69acd3d9b8bc..4f7f42f444ff 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -2615,6 +2615,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..d551c3ed101e --- /dev/null +++ b/tests/ui/unneeded_pub_crate.fixed @@ -0,0 +1,48 @@ +// run-rustfix +#![allow(dead_code)] +#![warn(clippy::unneeded_pub_crate)] + + struct Baz { + field: u8, +} + +mod outer { + mod inner { + // do these things _really_ need to be `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() -> 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(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, inner::S1) { + inner::bar() + } +} + +fn 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 new file mode 100644 index 000000000000..6fd7de0ad56e --- /dev/null +++ b/tests/ui/unneeded_pub_crate.rs @@ -0,0 +1,48 @@ +// 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() -> 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(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, inner::S1) { + inner::bar() + } +} + +fn main() { + let (value_outside, s1) = crate::outer::main(); + s1.1 = 42; + let _ = value_outside.used_outside; +} diff --git a/tests/ui/unneeded_pub_crate.stderr b/tests/ui/unneeded_pub_crate.stderr new file mode 100644 index 000000000000..e85556ce097e --- /dev/null +++ b/tests/ui/unneeded_pub_crate.stderr @@ -0,0 +1,40 @@ +error: pub(crate) item is never used outside of its defining module + --> $DIR/unneeded_pub_crate.rs:6:5 + | +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: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 + | +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: 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:36:9 + | +LL | pub(crate) not_used_outside: u8, + | ^^^^^^^^^^ help: consider removing pub(crate) + +error: aborting due to 6 previous errors +