unreachable_pub lint: grab pub span from HIR rather than inferring it

This is a true fix for rust-lang#50455, superior to the mere bandage offered in rust-lang#50476.
zackmdavis · Jul 1, 2018 · 104985b · 104985b
1 parent 4ae8912
commit 104985b
Show file tree

Hide file tree

Showing 3 changed files with 46 additions and 47 deletions.
diff --git a/src/librustc_lint/builtin.rs b/src/librustc_lint/builtin.rs
@@ -1386,60 +1386,53 @@ impl LintPass for UnreachablePub {
 
 impl UnreachablePub {
     fn perform_lint(&self, cx: &LateContext, what: &str, id: ast::NodeId,
-                    vis: &hir::Visibility, span: Span, exportable: bool,
-                    mut applicability: Applicability) {
-        if !cx.access_levels.is_reachable(id) && vis.node.is_pub() {
-            if span.ctxt().outer().expn_info().is_some() {
-                applicability = Applicability::MaybeIncorrect;
-            }
-            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));
-            // We are presuming that 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.features().crate_visibility_modifier {
-                "crate"
-            } else {
-                "pub(crate)"
-            }.to_owned();
-            err.span_suggestion_with_applicability(pub_span,
-                                                   "consider restricting its visibility",
-                                                   replacement,
-                                                   applicability);
-            if exportable {
-                err.help("or consider exporting it for use by other crates");
-            }
-            err.emit();
+                    vis: &hir::Visibility, span: Span, exportable: bool) {
+        let mut applicability = Applicability::MachineApplicable;
+        match vis.node {
+            hir::VisibilityPublic if !cx.access_levels.is_reachable(id) => {
+                if span.ctxt().outer().expn_info().is_some() {
+                    applicability = Applicability::MaybeIncorrect;
+                }
+                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));
+                let replacement = if cx.tcx.features().crate_visibility_modifier {
+                    "crate"
+                } else {
+                    "pub(crate)"
+                }.to_owned();
+
+                err.span_suggestion_with_applicability(vis.span,
+                                                       "consider restricting its visibility",
+                                                       replacement,
+                                                       applicability);
+                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) {
-        let applicability = match item.node {
-            // suggestion span-manipulation is inadequate for `pub use
-            // module::{item}` (Issue #50455)
-            hir::ItemUse(..) => Applicability::MaybeIncorrect,
-            _ => Applicability::MachineApplicable,
-        };
-        self.perform_lint(cx, "item", item.id, &item.vis, item.span, true, applicability);
+        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, Applicability::MachineApplicable);
+                          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,
-                          Applicability::MachineApplicable);
+        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,
-                          Applicability::MachineApplicable);
+        self.perform_lint(cx, "item", impl_item.id, &impl_item.vis, impl_item.span, false);
     }
 }
 

diff --git a/src/test/ui/lint/unreachable_pub-pub_crate.stderr b/src/test/ui/lint/unreachable_pub-pub_crate.stderr
@@ -17,7 +17,9 @@ warning: unreachable `pub` item
   --> $DIR/unreachable_pub-pub_crate.rs:27:24
    |
 LL |     pub use std::env::{Args}; // braced-use has different item spans than unbraced
-   |                        ^^^^ help: consider restricting its visibility: `pub(crate)`
+   |     ---                ^^^^
+   |     |
+   |     help: consider restricting its visibility: `pub(crate)`
    |
    = help: or consider exporting it for use by other crates
 
@@ -121,12 +123,13 @@ warning: unreachable `pub` item
   --> $DIR/unreachable_pub-pub_crate.rs:50:47
    |
 LL |         ($visibility: vis, $name: ident) => { $visibility struct $name {} }
-   |                                               -----------^^^^^^^^^^^^^
-   |                                               |
-   |                                               help: consider restricting its visibility: `pub(crate)`
+   |                                               ^^^^^^^^^^^^^^^^^^^^^^^^
 LL |     }
 LL |     define_empty_struct_with_visibility!(pub, Fluorine);
-   |     ---------------------------------------------------- in this macro invocation
+   |     ----------------------------------------------------
+   |     |                                    |
+   |     |                                    help: consider restricting its visibility: `pub(crate)`
+   |     in this macro invocation
    |
    = help: or consider exporting it for use by other crates
 

diff --git a/src/test/ui/lint/unreachable_pub.stderr b/src/test/ui/lint/unreachable_pub.stderr
@@ -17,7 +17,9 @@ warning: unreachable `pub` item
   --> $DIR/unreachable_pub.rs:22:24
    |
 LL |     pub use std::env::{Args}; // braced-use has different item spans than unbraced
-   |                        ^^^^ help: consider restricting its visibility: `crate`
+   |     ---                ^^^^
+   |     |
+   |     help: consider restricting its visibility: `crate`
    |
    = help: or consider exporting it for use by other crates
 
@@ -121,12 +123,13 @@ warning: unreachable `pub` item
   --> $DIR/unreachable_pub.rs:45:47
    |
 LL |         ($visibility: vis, $name: ident) => { $visibility struct $name {} }
-   |                                               -----------^^^^^^^^^^^^^
-   |                                               |
-   |                                               help: consider restricting its visibility: `crate`
+   |                                               ^^^^^^^^^^^^^^^^^^^^^^^^
 LL |     }
 LL |     define_empty_struct_with_visibility!(pub, Fluorine);
-   |     ---------------------------------------------------- in this macro invocation
+   |     ----------------------------------------------------
+   |     |                                    |
+   |     |                                    help: consider restricting its visibility: `crate`
+   |     in this macro invocation
    |
    = help: or consider exporting it for use by other crates