From 7006018745566fae2f09f6fc201cf4f6de6a4414 Mon Sep 17 00:00:00 2001 From: "Zack M. Davis" Date: Sat, 5 May 2018 22:14:33 -0700 Subject: [PATCH] don't make crazy suggestion for unreachable braced pub-use MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 marking the suggestion for `use` items as potentially incorrect. This resolves #50455 (but again, it would be desirable in the future to make a correct suggestion instead of copping out like this). --- src/librustc_lint/builtin.rs | 45 ++++++++++--------- src/test/ui/lint/unreachable_pub-pub_crate.rs | 1 + .../ui/lint/unreachable_pub-pub_crate.stderr | 32 ++++++++----- src/test/ui/lint/unreachable_pub.rs | 1 + src/test/ui/lint/unreachable_pub.stderr | 32 ++++++++----- 5 files changed, 67 insertions(+), 44 deletions(-) diff --git a/src/librustc_lint/builtin.rs b/src/librustc_lint/builtin.rs index 5102bfe766eef..251b95a6fcb79 100644 --- 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 index f5e6b4d3b4862..0a1926f8ae56a 100644 --- 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 index d1711be456bcc..2948deb23009c 100644 --- 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,7 +32,7 @@ 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, | ---^^^^^^^^^^^^^^^^ @@ -32,7 +40,7 @@ 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 } | ---^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -40,7 +48,7 @@ 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 index 347579c3e7bb9..5bb67670d85c2 100644 --- 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 index 1d693161108db..ad88c55d54013 100644 --- 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,7 +32,7 @@ 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, | ---^^^^^^^^^^^^^^^^ @@ -32,7 +40,7 @@ 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 } | ---^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -40,7 +48,7 @@ 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; | ---^^^^^^^^^^^^^^^^^^^^^^^