Rollup merge of rust-lang#50476 - zackmdavis:tame_unreachable_pub_sug…

…gestion, r=Manishearth don't make crazy suggestion for unreachable braced pub-use The Higher Intermediate Representation doesn't have spans for visibility keywords, so we were assuming that the first whitespace-delimited token in the item span was the `pub` to be weakened. This doesn't work for brace-grouped `use`s, which get lowered as if they were several individual `use` statements, but with spans that only cover the braced path-segments. Constructing a correct suggestion here presents some challenges—until someone works those out, we can at least protect the dignity of our compiler by not offering any suggestion at all for `use` items. This resolves rust-lang#50455 (but again, it would be desirable in the future to make a correct suggestion instead of copping out like this). r? @Manishearth
alexcrichton · May 11, 2018 · 268b038 · 268b038
2 parents fe63e47 + 7006018
commit 268b038
Show file tree

Hide file tree

Showing 5 changed files with 67 additions and 44 deletions.
diff --git a/src/librustc_lint/builtin.rs b/src/librustc_lint/builtin.rs
@@ -1287,32 +1287,27 @@ impl LintPass for UnreachablePub {
 
 impl UnreachablePub {
     fn perform_lint(&self, cx: &LateContext, what: &str, id: ast::NodeId,
-                    vis: &hir::Visibility, span: Span, exportable: bool) {
+                    vis: &hir::Visibility, span: Span, exportable: bool,
+                    mut applicability: Applicability) {
         if !cx.access_levels.is_reachable(id) && *vis == hir::Visibility::Public {
+            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));
-            // visibility is token at start of declaration (can be macro
-            // variable rather than literal `pub`)
+            // 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();
-            let app = if span.ctxt().outer().expn_info().is_none() {
-                // even if macros aren't involved the suggestion
-                // may be incorrect -- the user may have mistakenly
-                // hidden it behind a private module and this lint is
-                // a helpful way to catch that. However, we're trying
-                // not to change the nature of the code with this lint
-                // so it's marked as machine applicable.
-                Applicability::MachineApplicable
-            } else {
-                Applicability::MaybeIncorrect
-            };
-            err.span_suggestion_with_applicability(pub_span, "consider restricting its visibility",
-                                                   replacement, app);
+            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");
             }
@@ -1321,21 +1316,31 @@ impl UnreachablePub {
     }
 }
 
+
 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);
+        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);
     }
 
     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);
+        self.perform_lint(cx, "item", foreign_item.id, &foreign_item.vis,
+                          foreign_item.span, true, Applicability::MachineApplicable);
     }
 
     fn check_struct_field(&mut self, cx: &LateContext, field: &hir::StructField) {
-        self.perform_lint(cx, "field", field.id, &field.vis, field.span, false);
+        self.perform_lint(cx, "field", field.id, &field.vis, field.span, false,
+                          Applicability::MachineApplicable);
     }
 
     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);
+        self.perform_lint(cx, "item", impl_item.id, &impl_item.vis, impl_item.span, false,
+                          Applicability::MachineApplicable);
     }
 }
 

diff --git a/src/test/ui/lint/unreachable_pub-pub_crate.rs b/src/test/ui/lint/unreachable_pub-pub_crate.rs
@@ -24,6 +24,7 @@
 mod private_mod {
     // non-leaked `pub` items in private module should be linted
     pub use std::fmt;
+    pub use std::env::{Args}; // braced-use has different item spans than unbraced
 
     pub struct Hydrogen {
         // `pub` struct fields, too

diff --git a/src/test/ui/lint/unreachable_pub-pub_crate.stderr b/src/test/ui/lint/unreachable_pub-pub_crate.stderr
@@ -14,7 +14,15 @@ LL | #![warn(unreachable_pub)]
    = help: or consider exporting it for use by other crates
 
 warning: unreachable `pub` item
-  --> $DIR/unreachable_pub-pub_crate.rs:28:5
+  --> $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: or consider exporting it for use by other crates
+
+warning: unreachable `pub` item
+  --> $DIR/unreachable_pub-pub_crate.rs:29:5
    |
 LL |     pub struct Hydrogen {
    |     ---^^^^^^^^^^^^^^^^
@@ -24,23 +32,23 @@ LL |     pub struct Hydrogen {
    = help: or consider exporting it for use by other crates
 
 warning: unreachable `pub` field
-  --> $DIR/unreachable_pub-pub_crate.rs:30:9
+  --> $DIR/unreachable_pub-pub_crate.rs:31:9
    |
 LL |         pub neutrons: usize,
    |         ---^^^^^^^^^^^^^^^^
    |         |
    |         help: consider restricting its visibility: `pub(crate)`
 
 warning: unreachable `pub` item
-  --> $DIR/unreachable_pub-pub_crate.rs:36:9
+  --> $DIR/unreachable_pub-pub_crate.rs:37:9
    |
 LL |         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:40:5
+  --> $DIR/unreachable_pub-pub_crate.rs:41:5
    |
 LL |     pub enum Helium {}
    |     ---^^^^^^^^^^^^
@@ -50,7 +58,7 @@ LL |     pub enum Helium {}
    = help: or consider exporting it for use by other crates
 
 warning: unreachable `pub` item
-  --> $DIR/unreachable_pub-pub_crate.rs:41:5
+  --> $DIR/unreachable_pub-pub_crate.rs:42:5
    |
 LL |     pub union Lithium { c1: usize, c2: u8 }
    |     ---^^^^^^^^^^^^^^
@@ -60,7 +68,7 @@ LL |     pub union Lithium { c1: usize, c2: u8 }
    = help: or consider exporting it for use by other crates
 
 warning: unreachable `pub` item
-  --> $DIR/unreachable_pub-pub_crate.rs:42:5
+  --> $DIR/unreachable_pub-pub_crate.rs:43:5
    |
 LL |     pub fn beryllium() {}
    |     ---^^^^^^^^^^^^^^^
@@ -70,7 +78,7 @@ LL |     pub fn beryllium() {}
    = help: or consider exporting it for use by other crates
 
 warning: unreachable `pub` item
-  --> $DIR/unreachable_pub-pub_crate.rs:43:5
+  --> $DIR/unreachable_pub-pub_crate.rs:44:5
    |
 LL |     pub trait Boron {}
    |     ---^^^^^^^^^^^^
@@ -80,7 +88,7 @@ LL |     pub trait Boron {}
    = help: or consider exporting it for use by other crates
 
 warning: unreachable `pub` item
-  --> $DIR/unreachable_pub-pub_crate.rs:44:5
+  --> $DIR/unreachable_pub-pub_crate.rs:45:5
    |
 LL |     pub const CARBON: usize = 1;
    |     ---^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -90,7 +98,7 @@ LL |     pub const CARBON: usize = 1;
    = help: or consider exporting it for use by other crates
 
 warning: unreachable `pub` item
-  --> $DIR/unreachable_pub-pub_crate.rs:45:5
+  --> $DIR/unreachable_pub-pub_crate.rs:46:5
    |
 LL |     pub static NITROGEN: usize = 2;
    |     ---^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -100,7 +108,7 @@ LL |     pub static NITROGEN: usize = 2;
    = help: or consider exporting it for use by other crates
 
 warning: unreachable `pub` item
-  --> $DIR/unreachable_pub-pub_crate.rs:46:5
+  --> $DIR/unreachable_pub-pub_crate.rs:47:5
    |
 LL |     pub type Oxygen = bool;
    |     ---^^^^^^^^^^^^^^^^^^^^
@@ -110,7 +118,7 @@ LL |     pub type Oxygen = bool;
    = help: or consider exporting it for use by other crates
 
 warning: unreachable `pub` item
-  --> $DIR/unreachable_pub-pub_crate.rs:49:47
+  --> $DIR/unreachable_pub-pub_crate.rs:50:47
    |
 LL |         ($visibility: vis, $name: ident) => { $visibility struct $name {} }
    |                                               -----------^^^^^^^^^^^^^
@@ -123,7 +131,7 @@ LL |     define_empty_struct_with_visibility!(pub, Fluorine);
    = help: or consider exporting it for use by other crates
 
 warning: unreachable `pub` item
-  --> $DIR/unreachable_pub-pub_crate.rs:54:9
+  --> $DIR/unreachable_pub-pub_crate.rs:55:9
    |
 LL |         pub fn catalyze() -> bool;
    |         ---^^^^^^^^^^^^^^^^^^^^^^^

diff --git a/src/test/ui/lint/unreachable_pub.rs b/src/test/ui/lint/unreachable_pub.rs
@@ -19,6 +19,7 @@
 mod private_mod {
     // non-leaked `pub` items in private module should be linted
     pub use std::fmt;
+    pub use std::env::{Args}; // braced-use has different item spans than unbraced
 
     pub struct Hydrogen {
         // `pub` struct fields, too

diff --git a/src/test/ui/lint/unreachable_pub.stderr b/src/test/ui/lint/unreachable_pub.stderr
@@ -14,7 +14,15 @@ LL | #![warn(unreachable_pub)]
    = help: or consider exporting it for use by other crates
 
 warning: unreachable `pub` item
-  --> $DIR/unreachable_pub.rs:23:5
+  --> $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: or consider exporting it for use by other crates
+
+warning: unreachable `pub` item
+  --> $DIR/unreachable_pub.rs:24:5
    |
 LL |     pub struct Hydrogen {
    |     ---^^^^^^^^^^^^^^^^
@@ -24,23 +32,23 @@ LL |     pub struct Hydrogen {
    = help: or consider exporting it for use by other crates
 
 warning: unreachable `pub` field
-  --> $DIR/unreachable_pub.rs:25:9
+  --> $DIR/unreachable_pub.rs:26:9
    |
 LL |         pub neutrons: usize,
    |         ---^^^^^^^^^^^^^^^^
    |         |
    |         help: consider restricting its visibility: `crate`
 
 warning: unreachable `pub` item
-  --> $DIR/unreachable_pub.rs:31:9
+  --> $DIR/unreachable_pub.rs:32:9
    |
 LL |         pub fn count_neutrons(&self) -> usize { self.neutrons }
    |         ---^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |         |
    |         help: consider restricting its visibility: `crate`
 
 warning: unreachable `pub` item
-  --> $DIR/unreachable_pub.rs:35:5
+  --> $DIR/unreachable_pub.rs:36:5
    |
 LL |     pub enum Helium {}
    |     ---^^^^^^^^^^^^
@@ -50,7 +58,7 @@ LL |     pub enum Helium {}
    = help: or consider exporting it for use by other crates
 
 warning: unreachable `pub` item
-  --> $DIR/unreachable_pub.rs:36:5
+  --> $DIR/unreachable_pub.rs:37:5
    |
 LL |     pub union Lithium { c1: usize, c2: u8 }
    |     ---^^^^^^^^^^^^^^
@@ -60,7 +68,7 @@ LL |     pub union Lithium { c1: usize, c2: u8 }
    = help: or consider exporting it for use by other crates
 
 warning: unreachable `pub` item
-  --> $DIR/unreachable_pub.rs:37:5
+  --> $DIR/unreachable_pub.rs:38:5
    |
 LL |     pub fn beryllium() {}
    |     ---^^^^^^^^^^^^^^^
@@ -70,7 +78,7 @@ LL |     pub fn beryllium() {}
    = help: or consider exporting it for use by other crates
 
 warning: unreachable `pub` item
-  --> $DIR/unreachable_pub.rs:38:5
+  --> $DIR/unreachable_pub.rs:39:5
    |
 LL |     pub trait Boron {}
    |     ---^^^^^^^^^^^^
@@ -80,7 +88,7 @@ LL |     pub trait Boron {}
    = help: or consider exporting it for use by other crates
 
 warning: unreachable `pub` item
-  --> $DIR/unreachable_pub.rs:39:5
+  --> $DIR/unreachable_pub.rs:40:5
    |
 LL |     pub const CARBON: usize = 1;
    |     ---^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -90,7 +98,7 @@ LL |     pub const CARBON: usize = 1;
    = help: or consider exporting it for use by other crates
 
 warning: unreachable `pub` item
-  --> $DIR/unreachable_pub.rs:40:5
+  --> $DIR/unreachable_pub.rs:41:5
    |
 LL |     pub static NITROGEN: usize = 2;
    |     ---^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -100,7 +108,7 @@ LL |     pub static NITROGEN: usize = 2;
    = help: or consider exporting it for use by other crates
 
 warning: unreachable `pub` item
-  --> $DIR/unreachable_pub.rs:41:5
+  --> $DIR/unreachable_pub.rs:42:5
    |
 LL |     pub type Oxygen = bool;
    |     ---^^^^^^^^^^^^^^^^^^^^
@@ -110,7 +118,7 @@ LL |     pub type Oxygen = bool;
    = help: or consider exporting it for use by other crates
 
 warning: unreachable `pub` item
-  --> $DIR/unreachable_pub.rs:44:47
+  --> $DIR/unreachable_pub.rs:45:47
    |
 LL |         ($visibility: vis, $name: ident) => { $visibility struct $name {} }
    |                                               -----------^^^^^^^^^^^^^
@@ -123,7 +131,7 @@ LL |     define_empty_struct_with_visibility!(pub, Fluorine);
    = help: or consider exporting it for use by other crates
 
 warning: unreachable `pub` item
-  --> $DIR/unreachable_pub.rs:49:9
+  --> $DIR/unreachable_pub.rs:50:9
    |
 LL |         pub fn catalyze() -> bool;
    |         ---^^^^^^^^^^^^^^^^^^^^^^^