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

Macro checks thwart unreachable_pub lint #52665

Open
alexcrichton opened this issue Jul 24, 2018 · 3 comments
Open

Macro checks thwart unreachable_pub lint #52665

alexcrichton opened this issue Jul 24, 2018 · 3 comments
Labels
A-edition-2018-lints Area: lints supporting the 2018 edition A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

Recently we landed a change which squashes all lints tied to foreign macros, but this can thwart lints like unreachable_pub in unusual fashions. The unreachable_pub lint can't be fixed unless all its warnings are fixed, which means the following examples fails to compile after being fixed:

// bar.rs
#![crate_type = "lib"]

#[macro_export]
macro_rules! a {
    (pub static ref $name:ident : $t:ty = $val:expr) => (
        pub struct $name { _wut: () }
        pub static $name: $t = $val;
    )
}

and then

// foo.rs
#![crate_type = "lib"]
#![feature(rust_2018_preview)]
#![warn(rust_2018_idioms)]

#[macro_use]
#[allow(macro_use_extern_crate)]
extern crate bar;

mod m1 {
    pub struct Foo;
}

mod lookup {
    use crate::m1::Foo;

    a!(pub static ref F: Foo = Foo);
}

pub fn f() {
    drop(&lookup::F);
}

compiled with:

$ rustc +nightly bar.rs
$ rustc +nightly foo.rs -L .
warning: unreachable `pub` item
  --> foo.rs:10:5
   |
10 |     pub struct Foo;
   |     ---^^^^^^^^^^^^
   |     |
   |     help: consider restricting its visibility: `crate`
   |
note: lint level defined here
  --> foo.rs:3:9
   |
3  | #![warn(rust_2018_idioms)]
   |         ^^^^^^^^^^^^^^^^
   = note: #[warn(unreachable_pub)] implied by #[warn(rust_2018_idioms)]
   = help: or consider exporting it for use by other crates

but the corrected code doesn't compile!

This is a reuced example where a! is lazy_static!, but I believe the issue here is that the lints behind the lazy_static! invocation are being ignored which means that Foo is actually fully public (because F is) and the lint is no longer applicable as a result.

cc @Manishearth, @oli-obk

@alexcrichton alexcrichton added the A-edition-2018-lints Area: lints supporting the 2018 edition label Jul 24, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Jul 24, 2018

I don't know if this is solvable in general.

Or are you asking for not squashing the lint behind macros? That's easy, just add report_in_external_macro: true to

declare_lint! {
pub UNREACHABLE_PUB,
Allow,
"`pub` items not reachable from crate root"
}

@alexcrichton
Copy link
Member Author

I'm also not sure it's solvable! It may be a bit of a red herring here to say this is related to macro quashing, it's just sort of hiding the real bug.

I think a better way to put this may be that:

  • The unreachable_pub lint's suggestions don't work unless you apply all the suggestions.
  • Not all suggestions can be applied because they originate in foreign macros (like lazy_static!'s generated static). In the macro above there's no way to use crate visibility.
  • As a result, unreachable_pub may not provide correct suggestions.

The macro quashing is meaning you don't even see one of the suggestions (the one in the macro invocation), but even if we display the lint it wouldn't be actionable by rustfix anyway.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 24, 2018

So...

For each lint candidate that has not been squished:

  1. find all uses of the item in question
  2. if any of the uses have expansion info on them, don't lint

This is a bit extreme and might lead to loads of false negatives, so we should check to ensure that

mod bar {
    #[derive(Debug)]
    pub struct FOO;
}
fn main() {
    println!("{:?}", bar::FOO);
}

still lints.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Aug 1, 2018
These migration lints aren't all up to par in terms of a good migration
experience. Some, like `unreachable_pub`, hit bugs like rust-lang#52665 and unprepared
macros to be handled enough of the time. Others like linting against
`#[macro_use]` are swimming upstream in an ecosystem that's not quite ready (and
slightly buggy pending a few current PRs).

The general idea is that we will continue to recommend the `rust_2018_idioms`
lint group as part of the transition guide (as an optional step) but we'll be
much more selective about which lints make it into this group. Only those with a
strong track record of not causing too much churn will make the cut.

cc rust-lang#52679
pietroalbini added a commit to pietroalbini/rust that referenced this issue Aug 1, 2018
…li-obk

rustc: Trim down the `rust_2018_idioms` lint group

These migration lints aren't all up to par in terms of a good migration
experience. Some, like `unreachable_pub`, hit bugs like rust-lang#52665 and unprepared
macros to be handled enough of the time. Others like linting against
`#[macro_use]` are swimming upstream in an ecosystem that's not quite ready (and
slightly buggy pending a few current PRs).

The general idea is that we will continue to recommend the `rust_2018_idioms`
lint group as part of the transition guide (as an optional step) but we'll be
much more selective about which lints make it into this group. Only those with a
strong track record of not causing too much churn will make the cut.

cc rust-lang#52679
@alexcrichton alexcrichton added the A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. label Jan 7, 2019
@Enselic Enselic added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edition-2018-lints Area: lints supporting the 2018 edition A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants