Skip to content

Commit 6ca6b09

Browse files
authored
Fix the primary span of redundant_pub_crate when flagging nameless items (rust-lang#14516)
Found this while reviewing PR rust-lang#138384: See rust-lang#138384 (comment) in which I suggested a FIXME to be added that I'm now fixing in this PR. --- Before this PR, `redundant_pub_crate` computed a broken primary Span when flagging items (`hir::Item`s) that never bear a name (`ForeignMod`, `GlobalAsm`, `Impl`, `Use(UseKind::Glob)`, `Use(UseKind::ListStem)`). Namely, it created a span whose high byte index is `DUMMY_SP.hi()` which is quite broken (`DUMMY_SP` is synonymous with `0..0` wrt. the entire `SourceMap` meaning it points at the start of the very first source file in the `SourceMap`). Why did this happen? Before PR rust-lang#138384, the offending line looked like `let span = item.span.with_hi(item.ident.span.hi());`. For nameless items, `item.ident` used to be set to `Ident(sym::empty, DUMMY_SP)`. This is where the `DUMMY_SP` came from. The code means to compute a "shorter item span" that doesn't include the "body" of items, only the "head" (similar to `TyCtxt::def_span`). <details><summary>Examples of Clippy's butchered output on master</summary> ```rs #![deny(clippy::redundant_pub_crate)] mod m5 { pub mod m5_1 {} pub(crate) use m5_1::*; } ``` ``` error: pub(crate) import inside private module --> file.rs:1:1 | 1 | / #![deny(clippy::redundant_pub_crate)] 2 | | 3 | | mod m5 { 4 | | pub mod m5_1 {} 5 | | pub(crate) use m5_1::*; | | ^---------- help: consider using: `pub` | |____| | ``` Or if the `SourceMap` contains multiple files (notice how it leaks `clippy.toml`!): ``` error: pub(crate) import inside private module --> /home/fmease/programming/rust/clippy/clippy.toml:1:1 | 1 | / avoid-breaking-exported-api = false 2 | | 3 | | check-inconsistent-struct-field-initializers = true ... | 6 | | | |_^ | ::: file.rs:6:5 | 6 | pub(crate) use m5_1::{*}; // Glob | ---------- help: consider using: `pub` | ``` </details> --- **Note**: Currently, the only nameless item kind that can also have a visibility is `Use(UseKind::{Glob,ListStem})`. Thus I'm just falling back to the entire item's Span which wouldn't be that verbose. However, in the future Rust will feature impl restrictions (like `pub(crate) impl Trait for Type {}`, see [RFC 3323](https://rust-lang.github.io/rfcs/3323-restrictions.html) and rust-lang#106074). Using `item.span` for these would be quite bad (it would include all associated items). Should I add a FIXME? --- changelog: [`redundant_pub_crate`]: Fix the code highlighting for nameless items.
2 parents 227e43a + 54994b2 commit 6ca6b09

File tree

4 files changed

+37
-8
lines changed

4 files changed

+37
-8
lines changed

clippy_lints/src/redundant_pub_crate.rs

+4-7
Original file line numberDiff line numberDiff line change
@@ -52,13 +52,10 @@ impl<'tcx> LateLintPass<'tcx> for RedundantPubCrate {
5252
&& is_not_macro_export(item)
5353
&& !item.span.in_external_macro(cx.sess().source_map())
5454
{
55-
// FIXME: `DUMMY_SP` isn't right here, because it causes the
56-
// resulting span to begin at the start of the file.
57-
let span = item.span.with_hi(
58-
item.kind
59-
.ident()
60-
.map_or(rustc_span::DUMMY_SP.hi(), |ident| ident.span.hi()),
61-
);
55+
let span = item
56+
.kind
57+
.ident()
58+
.map_or(item.span, |ident| item.span.with_hi(ident.span.hi()));
6259
let descr = cx.tcx.def_kind(item.owner_id).descr(item.owner_id.to_def_id());
6360
span_lint_and_then(
6461
cx,

tests/ui/redundant_pub_crate.fixed

+8
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,14 @@ mod m4 {
131131
}
132132
}
133133

134+
mod m5 {
135+
pub mod m5_1 {}
136+
// Test that the primary span isn't butchered for item kinds that don't have an ident.
137+
pub use m5_1::*; //~ redundant_pub_crate
138+
#[rustfmt::skip]
139+
pub use m5_1::{*}; //~ redundant_pub_crate
140+
}
141+
134142
pub use m4::*;
135143

136144
mod issue_8732 {

tests/ui/redundant_pub_crate.rs

+8
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,14 @@ mod m4 {
131131
}
132132
}
133133

134+
mod m5 {
135+
pub mod m5_1 {}
136+
// Test that the primary span isn't butchered for item kinds that don't have an ident.
137+
pub(crate) use m5_1::*; //~ redundant_pub_crate
138+
#[rustfmt::skip]
139+
pub(crate) use m5_1::{*}; //~ redundant_pub_crate
140+
}
141+
134142
pub use m4::*;
135143

136144
mod issue_8732 {

tests/ui/redundant_pub_crate.stderr

+17-1
Original file line numberDiff line numberDiff line change
@@ -129,5 +129,21 @@ LL | pub(crate) fn g() {} // private due to m4_2
129129
| |
130130
| help: consider using: `pub`
131131

132-
error: aborting due to 16 previous errors
132+
error: pub(crate) import inside private module
133+
--> tests/ui/redundant_pub_crate.rs:137:5
134+
|
135+
LL | pub(crate) use m5_1::*;
136+
| ----------^^^^^^^^^^^^^
137+
| |
138+
| help: consider using: `pub`
139+
140+
error: pub(crate) import inside private module
141+
--> tests/ui/redundant_pub_crate.rs:139:27
142+
|
143+
LL | pub(crate) use m5_1::{*};
144+
| ---------- ^
145+
| |
146+
| help: consider using: `pub`
147+
148+
error: aborting due to 18 previous errors
133149

0 commit comments

Comments
 (0)