Auto merge of #45569 - zackmdavis:unexported_pub_lint, r=petrochenkov

`unreachable-pub` lint (as authorized by RFC 2126) To whom it may concern: RFC 2126 commissions the creation of a lint for `pub` items that are not visible from crate root (#45521). We understand (but seek confirmation from more knowledgable compiler elders) that this can be implemented by linting HIR items that are _not_ ~~`cx.access_levels.is_exported`~~ `cx.access_levels.is_reachable` but have a `vis` (-ibility) field of `hir::Visibility::Public`. The lint, tentatively called ~~`unexported-pub`~~ `unreachable-pub` (with the understanding that much could be written on the merits of various names, as it is said of the colors of bicycle-sheds), suggests `crate` as a replacement for `pub` if the `crate_visibility_modifier` feature is enabled (see #45388), and `pub(crate)` otherwise. We also use help messaging to suggest the other potential fix of exporting the item; feedback is desired as to whether this may be confusing or could be worded better. As a preview of what respecting the proposed lint would look like (and to generate confirmatory evidence that this implementation doesn't issue false positives), ~~we take its suggestions for `libcore`~~ (save one, which is deferred to another pull request because it brings up an unrelated technical matter). I remain your obedient servant. ![unexported_pub](https://user-images.githubusercontent.com/1076988/32089794-fbd02420-baa0-11e7-87e5-3ec01f18924a.png) r? @petrochenkov
rust-lang · Nov 3, 2017 · 59d4845 · 59d4845
2 parents 525b81d + 085ec6d
commit 59d4845
Show file tree

Hide file tree

Showing 6 changed files with 469 additions and 0 deletions.
diff --git a/src/librustc_lint/builtin.rs b/src/librustc_lint/builtin.rs
@@ -1301,3 +1301,60 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnionsWithDropFields {
         }
     }
 }
+
+/// Lint for items marked `pub` that aren't reachable from other crates
+pub struct UnreachablePub;
+
+declare_lint! {
+    UNREACHABLE_PUB,
+    Allow,
+    "`pub` items not reachable from crate root"
+}
+
+impl LintPass for UnreachablePub {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(UNREACHABLE_PUB)
+    }
+}
+
+impl UnreachablePub {
+    fn perform_lint(&self, cx: &LateContext, what: &str, id: ast::NodeId,
+                    vis: &hir::Visibility, span: Span, exportable: bool) {
+        if !cx.access_levels.is_reachable(id) && *vis == hir::Visibility::Public {
+            let def_span = cx.tcx.sess.codemap().def_span(span);
+            let mut err = cx.struct_span_lint(UNREACHABLE_PUB, def_span,
+                                              &format!("unreachable `pub` {}", what));
+            // visibility is token at start of declaration (can be macro
+            // variable rather than literal `pub`)
+            let pub_span = cx.tcx.sess.codemap().span_until_char(def_span, ' ');
+            let replacement = if cx.tcx.sess.features.borrow().crate_visibility_modifier {
+                "crate"
+            } else {
+                "pub(crate)"
+            }.to_owned();
+            err.span_suggestion(pub_span, "consider restricting its visibility", replacement);
+            if exportable {
+                err.help("or consider exporting it for use by other crates");
+            }
+            err.emit();
+        }
+    }
+}
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnreachablePub {
+    fn check_item(&mut self, cx: &LateContext, item: &hir::Item) {
+        self.perform_lint(cx, "item", item.id, &item.vis, item.span, true);
+    }
+
+    fn check_foreign_item(&mut self, cx: &LateContext, foreign_item: &hir::ForeignItem) {
+        self.perform_lint(cx, "item", foreign_item.id, &foreign_item.vis, foreign_item.span, true);
+    }
+
+    fn check_struct_field(&mut self, cx: &LateContext, field: &hir::StructField) {
+        self.perform_lint(cx, "field", field.id, &field.vis, field.span, false);
+    }
+
+    fn check_impl_item(&mut self, cx: &LateContext, impl_item: &hir::ImplItem) {
+        self.perform_lint(cx, "item", impl_item.id, &impl_item.vis, impl_item.span, false);
+    }
+}
diff --git a/src/librustc_lint/lib.rs b/src/librustc_lint/lib.rs
@@ -137,6 +137,7 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
                  PluginAsLibrary,
                  MutableTransmutes,
                  UnionsWithDropFields,
+                 UnreachablePub,
                  );
 
     add_builtin_with_new!(sess,

diff --git a/src/test/ui/lint/unreachable_pub-pub_crate.rs b/src/test/ui/lint/unreachable_pub-pub_crate.rs
@@ -0,0 +1,74 @@
+// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// This is just like unreachable_pub.rs, but without the
+// `crate_visibility_modifier` feature (so that we can test the suggestions to
+// use `pub(crate)` that are given when that feature is off, as opposed to the
+// suggestions to use `crate` given when it is on). When that feature becomes
+// stable, this test can be deleted.
+
+#![feature(macro_vis_matcher)]
+
+#![allow(unused)]
+#![warn(unreachable_pub)]
+
+mod private_mod {
+    // non-leaked `pub` items in private module should be linted
+    pub use std::fmt;
+
+    pub struct Hydrogen {
+        // `pub` struct fields, too
+        pub neutrons: usize,
+        // (... but not more-restricted fields)
+        pub(crate) electrons: usize
+    }
+    impl Hydrogen {
+        // impls, too
+        pub fn count_neutrons(&self) -> usize { self.neutrons }
+        pub(crate) fn count_electrons(&self) -> usize { self.electrons }
+    }
+
+    pub enum Helium {}
+    pub union Lithium { c1: usize, c2: u8 }
+    pub fn beryllium() {}
+    pub trait Boron {}
+    pub const CARBON: usize = 1;
+    pub static NITROGEN: usize = 2;
+    pub type Oxygen = bool;
+
+    macro_rules! define_empty_struct_with_visibility {
+        ($visibility: vis, $name: ident) => { $visibility struct $name {} }
+    }
+    define_empty_struct_with_visibility!(pub, Fluorine);
+
+    extern {
+        pub fn catalyze() -> bool;
+    }
+
+    // items leaked through signatures (see `get_neon` below) are OK
+    pub struct Neon {}
+
+    // crate-visible items are OK
+    pub(crate) struct Sodium {}
+}
+
+pub mod public_mod {
+    // module is public: these are OK, too
+    pub struct Magnesium {}
+    pub(crate) struct Aluminum {}
+}
+
+pub fn get_neon() -> private_mod::Neon {
+    private_mod::Neon {}
+}
+
+fn main() {
+    let _ = get_neon();
+}
diff --git a/src/test/ui/lint/unreachable_pub-pub_crate.stderr b/src/test/ui/lint/unreachable_pub-pub_crate.stderr
@@ -0,0 +1,134 @@
+warning: unreachable `pub` item
+  --> $DIR/unreachable_pub-pub_crate.rs:24:5
+   |
+24 |     pub use std::fmt;
+   |     ---^^^^^^^^^^^^^^
+   |     |
+   |     help: consider restricting its visibility: `pub(crate)`
+   |
+note: lint level defined here
+  --> $DIR/unreachable_pub-pub_crate.rs:20:9
+   |
+20 | #![warn(unreachable_pub)]
+   |         ^^^^^^^^^^^^^^^
+   = help: or consider exporting it for use by other crates
+
+warning: unreachable `pub` item
+  --> $DIR/unreachable_pub-pub_crate.rs:26:5
+   |
+26 |     pub struct Hydrogen {
+   |     ---^^^^^^^^^^^^^^^^
+   |     |
+   |     help: consider restricting its visibility: `pub(crate)`
+   |
+   = help: or consider exporting it for use by other crates
+
+warning: unreachable `pub` field
+  --> $DIR/unreachable_pub-pub_crate.rs:28:9
+   |
+28 |         pub neutrons: usize,
+   |         ---^^^^^^^^^^^^^^^^
+   |         |
+   |         help: consider restricting its visibility: `pub(crate)`
+
+warning: unreachable `pub` item
+  --> $DIR/unreachable_pub-pub_crate.rs:34:9
+   |
+34 |         pub fn count_neutrons(&self) -> usize { self.neutrons }
+   |         ---^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |         |
+   |         help: consider restricting its visibility: `pub(crate)`
+
+warning: unreachable `pub` item
+  --> $DIR/unreachable_pub-pub_crate.rs:38:5
+   |
+38 |     pub enum Helium {}
+   |     ---^^^^^^^^^^^^
+   |     |
+   |     help: consider restricting its visibility: `pub(crate)`
+   |
+   = help: or consider exporting it for use by other crates
+
+warning: unreachable `pub` item
+  --> $DIR/unreachable_pub-pub_crate.rs:39:5
+   |
+39 |     pub union Lithium { c1: usize, c2: u8 }
+   |     ---^^^^^^^^^^^^^^
+   |     |
+   |     help: consider restricting its visibility: `pub(crate)`
+   |
+   = help: or consider exporting it for use by other crates
+
+warning: unreachable `pub` item
+  --> $DIR/unreachable_pub-pub_crate.rs:40:5
+   |
+40 |     pub fn beryllium() {}
+   |     ---^^^^^^^^^^^^^^^
+   |     |
+   |     help: consider restricting its visibility: `pub(crate)`
+   |
+   = help: or consider exporting it for use by other crates
+
+warning: unreachable `pub` item
+  --> $DIR/unreachable_pub-pub_crate.rs:41:5
+   |
+41 |     pub trait Boron {}
+   |     ---^^^^^^^^^^^^
+   |     |
+   |     help: consider restricting its visibility: `pub(crate)`
+   |
+   = help: or consider exporting it for use by other crates
+
+warning: unreachable `pub` item
+  --> $DIR/unreachable_pub-pub_crate.rs:42:5
+   |
+42 |     pub const CARBON: usize = 1;
+   |     ---^^^^^^^^^^^^^^^^^^^^^^^^^
+   |     |
+   |     help: consider restricting its visibility: `pub(crate)`
+   |
+   = help: or consider exporting it for use by other crates
+
+warning: unreachable `pub` item
+  --> $DIR/unreachable_pub-pub_crate.rs:43:5
+   |
+43 |     pub static NITROGEN: usize = 2;
+   |     ---^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |     |
+   |     help: consider restricting its visibility: `pub(crate)`
+   |
+   = help: or consider exporting it for use by other crates
+
+warning: unreachable `pub` item
+  --> $DIR/unreachable_pub-pub_crate.rs:44:5
+   |
+44 |     pub type Oxygen = bool;
+   |     ---^^^^^^^^^^^^^^^^^^^^
+   |     |
+   |     help: consider restricting its visibility: `pub(crate)`
+   |
+   = help: or consider exporting it for use by other crates
+
+warning: unreachable `pub` item
+  --> $DIR/unreachable_pub-pub_crate.rs:47:47
+   |
+47 |         ($visibility: vis, $name: ident) => { $visibility struct $name {} }
+   |                                               -----------^^^^^^^^^^^^^
+   |                                               |
+   |                                               help: consider restricting its visibility: `pub(crate)`
+48 |     }
+49 |     define_empty_struct_with_visibility!(pub, Fluorine);
+   |     ---------------------------------------------------- in this macro invocation
+   |
+   = help: or consider exporting it for use by other crates
+
+warning: unreachable `pub` item
+  --> $DIR/unreachable_pub-pub_crate.rs:52:9
+   |
+52 |         pub fn catalyze() -> bool;
+   |         ---^^^^^^^^^^^^^^^^^^^^^^^
+   |         |
+   |         help: consider restricting its visibility: `pub(crate)`
+   |
+   = help: or consider exporting it for use by other crates
+
diff --git a/src/test/ui/lint/unreachable_pub.rs b/src/test/ui/lint/unreachable_pub.rs
@@ -0,0 +1,69 @@
+// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+#![feature(crate_visibility_modifier)]
+#![feature(macro_vis_matcher)]
+
+#![allow(unused)]
+#![warn(unreachable_pub)]
+
+mod private_mod {
+    // non-leaked `pub` items in private module should be linted
+    pub use std::fmt;
+
+    pub struct Hydrogen {
+        // `pub` struct fields, too
+        pub neutrons: usize,
+        // (... but not more-restricted fields)
+        crate electrons: usize
+    }
+    impl Hydrogen {
+        // impls, too
+        pub fn count_neutrons(&self) -> usize { self.neutrons }
+        crate fn count_electrons(&self) -> usize { self.electrons }
+    }
+
+    pub enum Helium {}
+    pub union Lithium { c1: usize, c2: u8 }
+    pub fn beryllium() {}
+    pub trait Boron {}
+    pub const CARBON: usize = 1;
+    pub static NITROGEN: usize = 2;
+    pub type Oxygen = bool;
+
+    macro_rules! define_empty_struct_with_visibility {
+        ($visibility: vis, $name: ident) => { $visibility struct $name {} }
+    }
+    define_empty_struct_with_visibility!(pub, Fluorine);
+
+    extern {
+        pub fn catalyze() -> bool;
+    }
+
+    // items leaked through signatures (see `get_neon` below) are OK
+    pub struct Neon {}
+
+    // crate-visible items are OK
+    crate struct Sodium {}
+}
+
+pub mod public_mod {
+    // module is public: these are OK, too
+    pub struct Magnesium {}
+    crate struct Aluminum {}
+}
+
+pub fn get_neon() -> private_mod::Neon {
+    private_mod::Neon {}
+}
+
+fn main() {
+    let _ = get_neon();
+}