Skip to content

Commit 5f98fe7

Browse files
borsbors
committed
Auto merge of #50476 - zackmdavis:tame_unreachable_pub_suggestion, 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 #50455 (but again, it would be desirable in the future to make a correct suggestion instead of copping out like this). r? @Manishearth
2 parents c705877 + 7006018 commit 5f98fe7

File tree

5 files changed

+67
-44
lines changed

5 files changed

+67
-44
lines changed

‎src/librustc_lint/builtin.rs

Lines changed: 25 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1287,32 +1287,27 @@ impl LintPass for UnreachablePub {
12871287

12881288
impl UnreachablePub {
12891289
fn perform_lint(&self, cx: &LateContext, what: &str, id: ast::NodeId,
1290-
vis: &hir::Visibility, span: Span, exportable: bool) {
1290+
vis: &hir::Visibility, span: Span, exportable: bool,
1291+
mut applicability: Applicability) {
12911292
if !cx.access_levels.is_reachable(id) && *vis == hir::Visibility::Public {
1293+
if span.ctxt().outer().expn_info().is_some() {
1294+
applicability = Applicability::MaybeIncorrect;
1295+
}
12921296
let def_span = cx.tcx.sess.codemap().def_span(span);
12931297
let mut err = cx.struct_span_lint(UNREACHABLE_PUB, def_span,
12941298
&format!("unreachable `pub` {}", what));
1295-
// visibility is token at start of declaration (can be macro
1296-
// variable rather than literal `pub`)
1299+
// We are presuming that visibility is token at start of
1300+
// declaration (can be macro variable rather than literal `pub`)
12971301
let pub_span = cx.tcx.sess.codemap().span_until_char(def_span, ' ');
12981302
let replacement = if cx.tcx.features().crate_visibility_modifier {
12991303
"crate"
13001304
} else {
13011305
"pub(crate)"
13021306
}.to_owned();
1303-
let app = if span.ctxt().outer().expn_info().is_none() {
1304-
// even if macros aren't involved the suggestion
1305-
// may be incorrect -- the user may have mistakenly
1306-
// hidden it behind a private module and this lint is
1307-
// a helpful way to catch that. However, we're trying
1308-
// not to change the nature of the code with this lint
1309-
// so it's marked as machine applicable.
1310-
Applicability::MachineApplicable
1311-
} else {
1312-
Applicability::MaybeIncorrect
1313-
};
1314-
err.span_suggestion_with_applicability(pub_span, "consider restricting its visibility",
1315-
replacement, app);
1307+
err.span_suggestion_with_applicability(pub_span,
1308+
"consider restricting its visibility",
1309+
replacement,
1310+
applicability);
13161311
if exportable {
13171312
err.help("or consider exporting it for use by other crates");
13181313
}
@@ -1321,21 +1316,31 @@ impl UnreachablePub {
13211316
}
13221317
}
13231318

1319+
13241320
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnreachablePub {
13251321
fn check_item(&mut self, cx: &LateContext, item: &hir::Item) {
1326-
self.perform_lint(cx, "item", item.id, &item.vis, item.span, true);
1322+
let applicability = match item.node {
1323+
// suggestion span-manipulation is inadequate for `pub use
1324+
// module::{item}` (Issue #50455)
1325+
hir::ItemUse(..) => Applicability::MaybeIncorrect,
1326+
_ => Applicability::MachineApplicable,
1327+
};
1328+
self.perform_lint(cx, "item", item.id, &item.vis, item.span, true, applicability);
13271329
}
13281330

13291331
fn check_foreign_item(&mut self, cx: &LateContext, foreign_item: &hir::ForeignItem) {
1330-
self.perform_lint(cx, "item", foreign_item.id, &foreign_item.vis, foreign_item.span, true);
1332+
self.perform_lint(cx, "item", foreign_item.id, &foreign_item.vis,
1333+
foreign_item.span, true, Applicability::MachineApplicable);
13311334
}
13321335

13331336
fn check_struct_field(&mut self, cx: &LateContext, field: &hir::StructField) {
1334-
self.perform_lint(cx, "field", field.id, &field.vis, field.span, false);
1337+
self.perform_lint(cx, "field", field.id, &field.vis, field.span, false,
1338+
Applicability::MachineApplicable);
13351339
}
13361340

13371341
fn check_impl_item(&mut self, cx: &LateContext, impl_item: &hir::ImplItem) {
1338-
self.perform_lint(cx, "item", impl_item.id, &impl_item.vis, impl_item.span, false);
1342+
self.perform_lint(cx, "item", impl_item.id, &impl_item.vis, impl_item.span, false,
1343+
Applicability::MachineApplicable);
13391344
}
13401345
}
13411346

‎src/test/ui/lint/unreachable_pub-pub_crate.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
mod private_mod {
2525
// non-leaked `pub` items in private module should be linted
2626
pub use std::fmt;
27+
pub use std::env::{Args}; // braced-use has different item spans than unbraced
2728

2829
pub struct Hydrogen {
2930
// `pub` struct fields, too

‎src/test/ui/lint/unreachable_pub-pub_crate.stderr

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,15 @@ LL | #![warn(unreachable_pub)]
1414
= help: or consider exporting it for use by other crates
1515

1616
warning: unreachable `pub` item
17-
--> $DIR/unreachable_pub-pub_crate.rs:28:5
17+
--> $DIR/unreachable_pub-pub_crate.rs:27:24
18+
|
19+
LL | pub use std::env::{Args}; // braced-use has different item spans than unbraced
20+
| ^^^^ help: consider restricting its visibility: `pub(crate)`
21+
|
22+
= help: or consider exporting it for use by other crates
23+
24+
warning: unreachable `pub` item
25+
--> $DIR/unreachable_pub-pub_crate.rs:29:5
1826
|
1927
LL | pub struct Hydrogen {
2028
| ---^^^^^^^^^^^^^^^^
@@ -24,23 +32,23 @@ LL | pub struct Hydrogen {
2432
= help: or consider exporting it for use by other crates
2533

2634
warning: unreachable `pub` field
27-
--> $DIR/unreachable_pub-pub_crate.rs:30:9
35+
--> $DIR/unreachable_pub-pub_crate.rs:31:9
2836
|
2937
LL | pub neutrons: usize,
3038
| ---^^^^^^^^^^^^^^^^
3139
| |
3240
| help: consider restricting its visibility: `pub(crate)`
3341

3442
warning: unreachable `pub` item
35-
--> $DIR/unreachable_pub-pub_crate.rs:36:9
43+
--> $DIR/unreachable_pub-pub_crate.rs:37:9
3644
|
3745
LL | pub fn count_neutrons(&self) -> usize { self.neutrons }
3846
| ---^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
3947
| |
4048
| help: consider restricting its visibility: `pub(crate)`
4149

4250
warning: unreachable `pub` item
43-
--> $DIR/unreachable_pub-pub_crate.rs:40:5
51+
--> $DIR/unreachable_pub-pub_crate.rs:41:5
4452
|
4553
LL | pub enum Helium {}
4654
| ---^^^^^^^^^^^^
@@ -50,7 +58,7 @@ LL | pub enum Helium {}
5058
= help: or consider exporting it for use by other crates
5159

5260
warning: unreachable `pub` item
53-
--> $DIR/unreachable_pub-pub_crate.rs:41:5
61+
--> $DIR/unreachable_pub-pub_crate.rs:42:5
5462
|
5563
LL | pub union Lithium { c1: usize, c2: u8 }
5664
| ---^^^^^^^^^^^^^^
@@ -60,7 +68,7 @@ LL | pub union Lithium { c1: usize, c2: u8 }
6068
= help: or consider exporting it for use by other crates
6169

6270
warning: unreachable `pub` item
63-
--> $DIR/unreachable_pub-pub_crate.rs:42:5
71+
--> $DIR/unreachable_pub-pub_crate.rs:43:5
6472
|
6573
LL | pub fn beryllium() {}
6674
| ---^^^^^^^^^^^^^^^
@@ -70,7 +78,7 @@ LL | pub fn beryllium() {}
7078
= help: or consider exporting it for use by other crates
7179

7280
warning: unreachable `pub` item
73-
--> $DIR/unreachable_pub-pub_crate.rs:43:5
81+
--> $DIR/unreachable_pub-pub_crate.rs:44:5
7482
|
7583
LL | pub trait Boron {}
7684
| ---^^^^^^^^^^^^
@@ -80,7 +88,7 @@ LL | pub trait Boron {}
8088
= help: or consider exporting it for use by other crates
8189

8290
warning: unreachable `pub` item
83-
--> $DIR/unreachable_pub-pub_crate.rs:44:5
91+
--> $DIR/unreachable_pub-pub_crate.rs:45:5
8492
|
8593
LL | pub const CARBON: usize = 1;
8694
| ---^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -90,7 +98,7 @@ LL | pub const CARBON: usize = 1;
9098
= help: or consider exporting it for use by other crates
9199

92100
warning: unreachable `pub` item
93-
--> $DIR/unreachable_pub-pub_crate.rs:45:5
101+
--> $DIR/unreachable_pub-pub_crate.rs:46:5
94102
|
95103
LL | pub static NITROGEN: usize = 2;
96104
| ---^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -100,7 +108,7 @@ LL | pub static NITROGEN: usize = 2;
100108
= help: or consider exporting it for use by other crates
101109

102110
warning: unreachable `pub` item
103-
--> $DIR/unreachable_pub-pub_crate.rs:46:5
111+
--> $DIR/unreachable_pub-pub_crate.rs:47:5
104112
|
105113
LL | pub type Oxygen = bool;
106114
| ---^^^^^^^^^^^^^^^^^^^^
@@ -110,7 +118,7 @@ LL | pub type Oxygen = bool;
110118
= help: or consider exporting it for use by other crates
111119

112120
warning: unreachable `pub` item
113-
--> $DIR/unreachable_pub-pub_crate.rs:49:47
121+
--> $DIR/unreachable_pub-pub_crate.rs:50:47
114122
|
115123
LL | ($visibility: vis, $name: ident) => { $visibility struct $name {} }
116124
| -----------^^^^^^^^^^^^^
@@ -123,7 +131,7 @@ LL | define_empty_struct_with_visibility!(pub, Fluorine);
123131
= help: or consider exporting it for use by other crates
124132

125133
warning: unreachable `pub` item
126-
--> $DIR/unreachable_pub-pub_crate.rs:54:9
134+
--> $DIR/unreachable_pub-pub_crate.rs:55:9
127135
|
128136
LL | pub fn catalyze() -> bool;
129137
| ---^^^^^^^^^^^^^^^^^^^^^^^

‎src/test/ui/lint/unreachable_pub.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
mod private_mod {
2020
// non-leaked `pub` items in private module should be linted
2121
pub use std::fmt;
22+
pub use std::env::{Args}; // braced-use has different item spans than unbraced
2223

2324
pub struct Hydrogen {
2425
// `pub` struct fields, too

‎src/test/ui/lint/unreachable_pub.stderr

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,15 @@ LL | #![warn(unreachable_pub)]
1414
= help: or consider exporting it for use by other crates
1515

1616
warning: unreachable `pub` item
17-
--> $DIR/unreachable_pub.rs:23:5
17+
--> $DIR/unreachable_pub.rs:22:24
18+
|
19+
LL | pub use std::env::{Args}; // braced-use has different item spans than unbraced
20+
| ^^^^ help: consider restricting its visibility: `crate`
21+
|
22+
= help: or consider exporting it for use by other crates
23+
24+
warning: unreachable `pub` item
25+
--> $DIR/unreachable_pub.rs:24:5
1826
|
1927
LL | pub struct Hydrogen {
2028
| ---^^^^^^^^^^^^^^^^
@@ -24,23 +32,23 @@ LL | pub struct Hydrogen {
2432
= help: or consider exporting it for use by other crates
2533

2634
warning: unreachable `pub` field
27-
--> $DIR/unreachable_pub.rs:25:9
35+
--> $DIR/unreachable_pub.rs:26:9
2836
|
2937
LL | pub neutrons: usize,
3038
| ---^^^^^^^^^^^^^^^^
3139
| |
3240
| help: consider restricting its visibility: `crate`
3341

3442
warning: unreachable `pub` item
35-
--> $DIR/unreachable_pub.rs:31:9
43+
--> $DIR/unreachable_pub.rs:32:9
3644
|
3745
LL | pub fn count_neutrons(&self) -> usize { self.neutrons }
3846
| ---^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
3947
| |
4048
| help: consider restricting its visibility: `crate`
4149

4250
warning: unreachable `pub` item
43-
--> $DIR/unreachable_pub.rs:35:5
51+
--> $DIR/unreachable_pub.rs:36:5
4452
|
4553
LL | pub enum Helium {}
4654
| ---^^^^^^^^^^^^
@@ -50,7 +58,7 @@ LL | pub enum Helium {}
5058
= help: or consider exporting it for use by other crates
5159

5260
warning: unreachable `pub` item
53-
--> $DIR/unreachable_pub.rs:36:5
61+
--> $DIR/unreachable_pub.rs:37:5
5462
|
5563
LL | pub union Lithium { c1: usize, c2: u8 }
5664
| ---^^^^^^^^^^^^^^
@@ -60,7 +68,7 @@ LL | pub union Lithium { c1: usize, c2: u8 }
6068
= help: or consider exporting it for use by other crates
6169

6270
warning: unreachable `pub` item
63-
--> $DIR/unreachable_pub.rs:37:5
71+
--> $DIR/unreachable_pub.rs:38:5
6472
|
6573
LL | pub fn beryllium() {}
6674
| ---^^^^^^^^^^^^^^^
@@ -70,7 +78,7 @@ LL | pub fn beryllium() {}
7078
= help: or consider exporting it for use by other crates
7179

7280
warning: unreachable `pub` item
73-
--> $DIR/unreachable_pub.rs:38:5
81+
--> $DIR/unreachable_pub.rs:39:5
7482
|
7583
LL | pub trait Boron {}
7684
| ---^^^^^^^^^^^^
@@ -80,7 +88,7 @@ LL | pub trait Boron {}
8088
= help: or consider exporting it for use by other crates
8189

8290
warning: unreachable `pub` item
83-
--> $DIR/unreachable_pub.rs:39:5
91+
--> $DIR/unreachable_pub.rs:40:5
8492
|
8593
LL | pub const CARBON: usize = 1;
8694
| ---^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -90,7 +98,7 @@ LL | pub const CARBON: usize = 1;
9098
= help: or consider exporting it for use by other crates
9199

92100
warning: unreachable `pub` item
93-
--> $DIR/unreachable_pub.rs:40:5
101+
--> $DIR/unreachable_pub.rs:41:5
94102
|
95103
LL | pub static NITROGEN: usize = 2;
96104
| ---^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -100,7 +108,7 @@ LL | pub static NITROGEN: usize = 2;
100108
= help: or consider exporting it for use by other crates
101109

102110
warning: unreachable `pub` item
103-
--> $DIR/unreachable_pub.rs:41:5
111+
--> $DIR/unreachable_pub.rs:42:5
104112
|
105113
LL | pub type Oxygen = bool;
106114
| ---^^^^^^^^^^^^^^^^^^^^
@@ -110,7 +118,7 @@ LL | pub type Oxygen = bool;
110118
= help: or consider exporting it for use by other crates
111119

112120
warning: unreachable `pub` item
113-
--> $DIR/unreachable_pub.rs:44:47
121+
--> $DIR/unreachable_pub.rs:45:47
114122
|
115123
LL | ($visibility: vis, $name: ident) => { $visibility struct $name {} }
116124
| -----------^^^^^^^^^^^^^
@@ -123,7 +131,7 @@ LL | define_empty_struct_with_visibility!(pub, Fluorine);
123131
= help: or consider exporting it for use by other crates
124132

125133
warning: unreachable `pub` item
126-
--> $DIR/unreachable_pub.rs:49:9
134+
--> $DIR/unreachable_pub.rs:50:9
127135
|
128136
LL | pub fn catalyze() -> bool;
129137
| ---^^^^^^^^^^^^^^^^^^^^^^^

0 commit comments

Comments
 (0)