Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggestion for pub(crate) not always valid on multiple imports #50455

Closed
alexcrichton opened this issue May 4, 2018 · 2 comments · Fixed by #50476
Closed

Suggestion for pub(crate) not always valid on multiple imports #50455

alexcrichton opened this issue May 4, 2018 · 2 comments · Fixed by #50476
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-rust-2018-preview Area: The 2018 edition preview

Comments

@alexcrichton
Copy link
Member

Given code that looks like:

mod foo {
    pub struct Bar;
    pub struct Baz;
}

mod bar {
    pub use foo::{Bar, Baz};
}

you get:

$ rustc +nightly src/lib.rs --crate-type lib -W rust_2018_idioms
warning: struct is never used: `Bar`
 --> src/lib.rs:2:5
  |
2 |     pub struct Bar;
  |     ^^^^^^^^^^^^^^^
  |
  = note: #[warn(dead_code)] on by default

warning: struct is never used: `Baz`
 --> src/lib.rs:3:5
  |
3 |     pub struct Baz;
  |     ^^^^^^^^^^^^^^^

warning: unreachable `pub` item
 --> src/lib.rs:2:5
  |
2 |     pub struct Bar;
  |     ---^^^^^^^^^^^^
  |     |
  |     help: consider restricting its visibility: `pub(crate)`
  |
  = note: `-W unreachable-pub` implied by `-W rust-2018-idioms`
  = help: or consider exporting it for use by other crates

warning: unreachable `pub` item
 --> src/lib.rs:3:5
  |
3 |     pub struct Baz;
  |     ---^^^^^^^^^^^^
  |     |
  |     help: consider restricting its visibility: `pub(crate)`
  |
  = help: or consider exporting it for use by other crates

warning: unreachable `pub` item
 --> src/lib.rs:7:19
  |
7 |     pub use foo::{Bar, Baz};
  |                   ^^^ help: consider restricting its visibility: `pub(crate)`
  |
  = help: or consider exporting it for use by other crates

warning: unreachable `pub` item
 --> src/lib.rs:7:24
  |
7 |     pub use foo::{Bar, Baz};
  |                        ^^^ help: consider restricting its visibility: `pub(crate)`
  |
= help: or consider exporting it for use by other crates

which when auto-fixed by cargo fix will produce:

mod foo {
    pub(crate) struct Bar;
    pub(crate) struct Baz;
}

mod bar {
    pub use foo::{pub(crate), pub(crate)};
}

which fails to compile!

cc @Manishearth

@alexcrichton alexcrichton added the A-diagnostics Area: Messages for errors, warnings, and lints label May 4, 2018
@Manishearth
Copy link
Member

cc @zackmdavis who implemented this

I also wish we had a better answer for this lint than chopping up spans, but the current architecture makes this hard (this has been a problem for clippy forever).

The problem here is that when we do HIR lowering we split apart uses, and this totally breaks down.

But we throw away the AST by the time we have access level info.

We can skip this lint for pub use for now, but that's not too great.

I don't really have ideas for fixing this.

@zackmdavis
Copy link
Member

#50476 proposes not making suggestions for use items. (The lint itself still fires, of course.)

I also wish we had a better answer for this lint than chopping up spans, but the current architecture makes this hard (this has been a problem for clippy forever).

We could add spans to hir::Visibility?

zackmdavis added a commit to zackmdavis/rust that referenced this issue May 11, 2018
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 rust-lang#50455 (but again, it would be desirable in the future to
make a correct suggestion instead of copping out like this).
alexcrichton added a commit to alexcrichton/rust that referenced this issue May 11, 2018
…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 alexcrichton added the A-rust-2018-preview Area: The 2018 edition preview label May 11, 2018
bors added a commit that referenced this issue May 12, 2018
…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
zackmdavis added a commit to zackmdavis/rust that referenced this issue Jul 1, 2018
This is a true fix for rust-lang#50455, superior to the mere bandage offered
in rust-lang#50476.
bors added a commit that referenced this issue Jul 2, 2018
…petrochenkov

add modifier keyword spans to hir::Visibility; improve unreachable-pub, private-no-mangle lint suggestions

#50455 pointed out that the unreachable-pub suggestion for brace-grouped `use`s was bogus; #50476 partially ameliorated this by marking the suggestion as `Applicability::MaybeIncorrect`, but this is the actual fix.

Meanwhile, another application of having spans available in `hir::Visibility` is found in the private-no-mangle lints, where we can now issue a suggestion to use `pub` if the item has a more restricted visibility marker (this seems much less likely to come up in practice than not having any visibility keyword at all, but thoroughness is a virtue). While we're there, we can also add a helpful note if the item does have a `pub` (but triggered the lint presumably because enclosing modules were private).

![hir_vis](https://user-images.githubusercontent.com/1076988/42018064-ca830290-7a65-11e8-9c4c-48bc846f861f.png)

r? @nrc
cc @Manishearth
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-rust-2018-preview Area: The 2018 edition preview
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants