Lint for `pub(crate)` items that are not crate visible due to the visibility of the module that contains them

Closes #5274.

7 files changed, 332 insertions, 2 deletions

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 @@ pub mod ranges;
 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<bool>,
+}
+
+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;
 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",
@@ -1765,6 +1765,13 @@ pub const ALL_LINTS: [Lint; 361] = [
         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",
         desc: "Using explicit `\'static` lifetime for constants or statics when elision rules would allow omitting them.",
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
+