diff --git a/CHANGELOG.md b/CHANGELOG.md index c8d41e3b31d..9fa2dcad4a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1410,6 +1410,7 @@ Released 2018-09-13 [`redundant_field_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_field_names [`redundant_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_pattern [`redundant_pattern_matching`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_pattern_matching +[`redundant_pub_crate`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_pub_crate [`redundant_static_lifetimes`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_static_lifetimes [`ref_in_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_in_deref [`regex_macro`]: https://rust-lang.github.io/rust-clippy/master/index.html#regex_macro diff --git a/README.md b/README.md index 2181c296e9b..7d6fcbc9098 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 361 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 362 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 791307ba43f..601bbdce704 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -284,6 +284,7 @@ macro_rules! declare_clippy_lint { pub mod redundant_clone; pub mod redundant_field_names; pub mod redundant_pattern_matching; +pub mod redundant_pub_crate; pub mod redundant_static_lifetimes; pub mod reference; pub mod regex; @@ -744,6 +745,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &redundant_clone::REDUNDANT_CLONE, &redundant_field_names::REDUNDANT_FIELD_NAMES, &redundant_pattern_matching::REDUNDANT_PATTERN_MATCHING, + &redundant_pub_crate::REDUNDANT_PUB_CRATE, &redundant_static_lifetimes::REDUNDANT_STATIC_LIFETIMES, &reference::DEREF_ADDROF, &reference::REF_IN_DEREF, @@ -1020,6 +1022,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box wildcard_imports::WildcardImports); store.register_early_pass(|| box macro_use::MacroUseImports); store.register_late_pass(|| box verbose_file_reads::VerboseFileReads); + store.register_late_pass(|| box redundant_pub_crate::RedundantPubCrate::default()); store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![ LintId::of(&arithmetic::FLOAT_ARITHMETIC), @@ -1669,6 +1672,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&mutex_atomic::MUTEX_INTEGER), LintId::of(&needless_borrow::NEEDLESS_BORROW), LintId::of(&path_buf_push_overwrite::PATH_BUF_PUSH_OVERWRITE), + LintId::of(&redundant_pub_crate::REDUNDANT_PUB_CRATE), LintId::of(&use_self::USE_SELF), ]); } diff --git a/clippy_lints/src/redundant_pub_crate.rs b/clippy_lints/src/redundant_pub_crate.rs new file mode 100644 index 00000000000..4c638da80a8 --- /dev/null +++ b/clippy_lints/src/redundant_pub_crate.rs @@ -0,0 +1,67 @@ +use crate::utils::span_lint_and_help; +use rustc_hir::{Item, ItemKind, VisibilityKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_tool_lint, impl_lint_pass}; + +declare_clippy_lint! { + /// **What it does:** Checks for items declared `pub(crate)` that are not crate visible because they + /// are inside a private module. + /// + /// **Why is this bad?** Writing `pub(crate)` is misleading when it's redundant due to the parent + /// module's visibility. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust + /// mod internal { + /// pub(crate) fn internal_fn() { } + /// } + /// ``` + /// This function is not visible outside the module and it can be declared with `pub` or + /// private visibility + /// ```rust + /// mod internal { + /// pub fn internal_fn() { } + /// } + /// ``` + pub REDUNDANT_PUB_CRATE, + nursery, + "Using `pub(crate)` visibility on items that are not crate visible due to the visibility of the module that contains them." +} + +#[derive(Default)] +pub struct RedundantPubCrate { + is_exported: Vec, +} + +impl_lint_pass!(RedundantPubCrate => [REDUNDANT_PUB_CRATE]); + +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantPubCrate { + fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx Item<'tcx>) { + if let VisibilityKind::Crate { .. } = item.vis.node { + if !cx.access_levels.is_exported(item.hir_id) { + if let Some(false) = self.is_exported.last() { + span_lint_and_help( + cx, + REDUNDANT_PUB_CRATE, + item.span, + &format!("pub(crate) {} inside private module", item.kind.descr()), + "consider using `pub` instead of `pub(crate)`", + ) + } + } + } + + if let ItemKind::Mod { .. } = item.kind { + self.is_exported.push(cx.access_levels.is_exported(item.hir_id)); + } + } + + fn check_item_post(&mut self, _cx: &LateContext<'a, 'tcx>, item: &'tcx Item<'tcx>) { + if let ItemKind::Mod { .. } = item.kind { + self.is_exported.pop().expect("unbalanced check_item/check_item_post"); + } + } +} diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index fd948953ea2..29d9c8b76d0 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -6,7 +6,7 @@ pub use lint::LINT_LEVELS; // begin lint list, do not remove this comment, it’s used in `update_lints` -pub const ALL_LINTS: [Lint; 361] = [ +pub const ALL_LINTS: [Lint; 362] = [ Lint { name: "absurd_extreme_comparisons", group: "correctness", @@ -1764,6 +1764,13 @@ deprecation: None, module: "redundant_pattern_matching", }, + Lint { + name: "redundant_pub_crate", + group: "nursery", + desc: "Using `pub(crate)` visibility on items that are not crate visible due to the visibility of the module that contains them.", + deprecation: None, + module: "redundant_pub_crate", + }, Lint { name: "redundant_static_lifetimes", group: "style", diff --git a/tests/ui/redundant_pub_crate.rs b/tests/ui/redundant_pub_crate.rs new file mode 100644 index 00000000000..c747a07dd90 --- /dev/null +++ b/tests/ui/redundant_pub_crate.rs @@ -0,0 +1,105 @@ +#![warn(clippy::redundant_pub_crate)] + +mod m1 { + fn f() {} + pub(crate) fn g() {} // private due to m1 + pub fn h() {} + + mod m1_1 { + fn f() {} + pub(crate) fn g() {} // private due to m1_1 and m1 + pub fn h() {} + } + + pub(crate) mod m1_2 { + // ^ private due to m1 + fn f() {} + pub(crate) fn g() {} // private due to m1_2 and m1 + pub fn h() {} + } + + pub mod m1_3 { + fn f() {} + pub(crate) fn g() {} // private due to m1 + pub fn h() {} + } +} + +pub(crate) mod m2 { + fn f() {} + pub(crate) fn g() {} // already crate visible due to m2 + pub fn h() {} + + mod m2_1 { + fn f() {} + pub(crate) fn g() {} // private due to m2_1 + pub fn h() {} + } + + pub(crate) mod m2_2 { + // ^ already crate visible due to m2 + fn f() {} + pub(crate) fn g() {} // already crate visible due to m2_2 and m2 + pub fn h() {} + } + + pub mod m2_3 { + fn f() {} + pub(crate) fn g() {} // already crate visible due to m2 + pub fn h() {} + } +} + +pub mod m3 { + fn f() {} + pub(crate) fn g() {} // ok: m3 is exported + pub fn h() {} + + mod m3_1 { + fn f() {} + pub(crate) fn g() {} // private due to m3_1 + pub fn h() {} + } + + pub(crate) mod m3_2 { + // ^ ok + fn f() {} + pub(crate) fn g() {} // already crate visible due to m3_2 + pub fn h() {} + } + + pub mod m3_3 { + fn f() {} + pub(crate) fn g() {} // ok: m3 and m3_3 are exported + pub fn h() {} + } +} + +mod m4 { + fn f() {} + pub(crate) fn g() {} // private: not re-exported by `pub use m4::*` + pub fn h() {} + + mod m4_1 { + fn f() {} + pub(crate) fn g() {} // private due to m4_1 + pub fn h() {} + } + + pub(crate) mod m4_2 { + // ^ private: not re-exported by `pub use m4::*` + fn f() {} + pub(crate) fn g() {} // private due to m4_2 + pub fn h() {} + } + + pub mod m4_3 { + fn f() {} + pub(crate) fn g() {} // ok: m4_3 is re-exported by `pub use m4::*` + pub fn h() {} + } +} + +pub use m4::*; + +fn main() {} diff --git a/tests/ui/redundant_pub_crate.stderr b/tests/ui/redundant_pub_crate.stderr new file mode 100644 index 00000000000..ecc038c7f4e --- /dev/null +++ b/tests/ui/redundant_pub_crate.stderr @@ -0,0 +1,146 @@ +error: pub(crate) function inside private module + --> $DIR/redundant_pub_crate.rs:5:5 + | +LL | pub(crate) fn g() {} // private due to m1 + | ^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::redundant-pub-crate` implied by `-D warnings` + = help: consider using `pub` instead of `pub(crate)` + +error: pub(crate) function inside private module + --> $DIR/redundant_pub_crate.rs:10:9 + | +LL | pub(crate) fn g() {} // private due to m1_1 and m1 + | ^^^^^^^^^^^^^^^^^^^^ + | + = help: consider using `pub` instead of `pub(crate)` + +error: pub(crate) module inside private module + --> $DIR/redundant_pub_crate.rs:14:5 + | +LL | / pub(crate) mod m1_2 { +LL | | // ^ private due to m1 +LL | | fn f() {} +LL | | pub(crate) fn g() {} // private due to m1_2 and m1 +LL | | pub fn h() {} +LL | | } + | |_____^ + | + = help: consider using `pub` instead of `pub(crate)` + +error: pub(crate) function inside private module + --> $DIR/redundant_pub_crate.rs:17:9 + | +LL | pub(crate) fn g() {} // private due to m1_2 and m1 + | ^^^^^^^^^^^^^^^^^^^^ + | + = help: consider using `pub` instead of `pub(crate)` + +error: pub(crate) function inside private module + --> $DIR/redundant_pub_crate.rs:23:9 + | +LL | pub(crate) fn g() {} // private due to m1 + | ^^^^^^^^^^^^^^^^^^^^ + | + = help: consider using `pub` instead of `pub(crate)` + +error: pub(crate) function inside private module + --> $DIR/redundant_pub_crate.rs:30:5 + | +LL | pub(crate) fn g() {} // already crate visible due to m2 + | ^^^^^^^^^^^^^^^^^^^^ + | + = help: consider using `pub` instead of `pub(crate)` + +error: pub(crate) function inside private module + --> $DIR/redundant_pub_crate.rs:35:9 + | +LL | pub(crate) fn g() {} // private due to m2_1 + | ^^^^^^^^^^^^^^^^^^^^ + | + = help: consider using `pub` instead of `pub(crate)` + +error: pub(crate) module inside private module + --> $DIR/redundant_pub_crate.rs:39:5 + | +LL | / pub(crate) mod m2_2 { +LL | | // ^ already crate visible due to m2 +LL | | fn f() {} +LL | | pub(crate) fn g() {} // already crate visible due to m2_2 and m2 +LL | | pub fn h() {} +LL | | } + | |_____^ + | + = help: consider using `pub` instead of `pub(crate)` + +error: pub(crate) function inside private module + --> $DIR/redundant_pub_crate.rs:42:9 + | +LL | pub(crate) fn g() {} // already crate visible due to m2_2 and m2 + | ^^^^^^^^^^^^^^^^^^^^ + | + = help: consider using `pub` instead of `pub(crate)` + +error: pub(crate) function inside private module + --> $DIR/redundant_pub_crate.rs:48:9 + | +LL | pub(crate) fn g() {} // already crate visible due to m2 + | ^^^^^^^^^^^^^^^^^^^^ + | + = help: consider using `pub` instead of `pub(crate)` + +error: pub(crate) function inside private module + --> $DIR/redundant_pub_crate.rs:60:9 + | +LL | pub(crate) fn g() {} // private due to m3_1 + | ^^^^^^^^^^^^^^^^^^^^ + | + = help: consider using `pub` instead of `pub(crate)` + +error: pub(crate) function inside private module + --> $DIR/redundant_pub_crate.rs:67:9 + | +LL | pub(crate) fn g() {} // already crate visible due to m3_2 + | ^^^^^^^^^^^^^^^^^^^^ + | + = help: consider using `pub` instead of `pub(crate)` + +error: pub(crate) function inside private module + --> $DIR/redundant_pub_crate.rs:80:5 + | +LL | pub(crate) fn g() {} // private: not re-exported by `pub use m4::*` + | ^^^^^^^^^^^^^^^^^^^^ + | + = help: consider using `pub` instead of `pub(crate)` + +error: pub(crate) function inside private module + --> $DIR/redundant_pub_crate.rs:85:9 + | +LL | pub(crate) fn g() {} // private due to m4_1 + | ^^^^^^^^^^^^^^^^^^^^ + | + = help: consider using `pub` instead of `pub(crate)` + +error: pub(crate) module inside private module + --> $DIR/redundant_pub_crate.rs:89:5 + | +LL | / pub(crate) mod m4_2 { +LL | | // ^ private: not re-exported by `pub use m4::*` +LL | | fn f() {} +LL | | pub(crate) fn g() {} // private due to m4_2 +LL | | pub fn h() {} +LL | | } + | |_____^ + | + = help: consider using `pub` instead of `pub(crate)` + +error: pub(crate) function inside private module + --> $DIR/redundant_pub_crate.rs:92:9 + | +LL | pub(crate) fn g() {} // private due to m4_2 + | ^^^^^^^^^^^^^^^^^^^^ + | + = help: consider using `pub` instead of `pub(crate)` + +error: aborting due to 16 previous errors +