Macro checks thwart `unreachable_pub` lint

alexcrichton commented 6 years ago

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

oli-obk commented 6 years ago

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 https://github.com/rust-lang/rust/blob/c7cba3d33f564be140275c4fb9e33c6dc2c97b21/src/librustc_lint/builtin.rs#L1392-L1396

alexcrichton commented 6 years ago

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 commented 6 years ago

So...

For each lint candidate that has not been squished:

find all uses of the item in question
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.

rust-lang / rust

Macro checks thwart `unreachable_pub` lint #52665