est31 / warnalyzer

Show unused code from multi-crate Rust projects
Other
89 stars 4 forks source link

Very buggy on rustc itself #5

Open jyn514 opened 3 years ago

jyn514 commented 3 years ago

Probably most of these are because -Z save-analysis is buggy, but I don't know warnalyzer well enough to tell myself. Also, I want you to know this tool has been super useful :heart: and I'm going to make a PR removing dead code shortly; it just happens to also be very buggy at the same time :laughing: I'm going to put them all in one issue to avoid spamming your bug tracker, but happy to separate them out if you think it's useful.

You should be able to reproduce with RUSTFLAGS_BOOTSTRAP="-Z save-analysis" x.py check on https://github.com/rust-lang/rust/commit/2b5c78488e1a9bbc27ecf96d351e880a808077a0.

There are a bunch more macro issues, but I didn't report them all since you say in the readme they're imperfectly supported.

est31 commented 3 years ago

Thanks for the report, and thanks for trying out warnalyzer! As you have correctly estimated, most of the issues stem from the save analysis backend of rustc.

unused Function 'main' - clearly wrong, this is the entrypoint of the compiler unused Mod '' - I think this is trying to warn about the crate root? there's at least one bug here

Indeed it's wrong :). Entry point recognition is a legit missing warnalyzer feature I guess :). I've filed #6 as a report.

unused Method 'visit' - this is a trait function and can't be used. There are some implementations of the trait which are used.

The only place the function is used is in macros, which is I think what breaks it in this case. I'd put it in the "macros are buggy" category :).

unused Type 'Copy' - this is a derived trait, not a type.

When you have stuff like:

#[derive(Copy)]
struct T {}

it expands to (confirmed via playground):

struct T {
}
#[automatically_derived]
#[allow(unused_qualifications)]
impl ::core::marker::Copy for T { }

If I remember correctly, "unused type X" refers to that auto-generated impl block being unused.

unused Field '0' - this is for GenericArgs::AngleBracketed(AngleBracketedArgs). Not sure what's going on here, these are used quite a lot.

There used to be some enum field related bugs a while ago in the save analysis mode and since then the issues have been closed/fixed. The Readme still references them. It's possible that the fixes weren't complete. https://github.com/rust-lang/rust/issues/61385 https://github.com/rust-lang/rust/issues/61302

unused Mod 'temp_stable_hash_impls' - the impl in this mod is used, even though it has no other items.

I guess it doesn't regard the impl inside as "use" of the mod. Not sure whether one can put save-analysis to blame here. It might be a legit warnalyzer issue.

compiler/rustc_hir/src/hir.rs:3040:12: unused Method 'hir_id' - this is used outside of a macro, in compiler/rustc_middle/src/hir/map/mod.rs:304:28.

I'm not sure if I have the same checkout as you but my checkout says the method is on the Node type while the function in rustc_middle is this:

    pub fn item(&self, id: ItemId) -> &'hir Item<'hir> {
        match self.find(id.hir_id()).unwrap() {
            Node::Item(item) => item,
            _ => bug!(),
        }
    }

which calls hir_id on ItemId instead, which is a struct defined in the same file (in contrast to a type alias or something).

compiler/rustc_hir/src/definitions.rs:94:12: unused Method 'enumerated_keys_and_path_hashes' - this is used in compiler/rustc_metadata/src/rmeta/encoder.rs:462:62 compiler/rustc_middle/src/mir/mod.rs:2472:12: unused Method 'projections_and_spans'

Yeah this is because it doesn't consider the "in " part of for loops as "use". I remember encountering it myself. I think this is a save-analysis bug but it should still be documented in the README (done in f0a200d3274ca04d8374b8202e77a7884787f7bf). let it = <expr>; for _ in it { should do the trick I think.

jyn514 commented 3 years ago

If I remember correctly, "unused type X" refers to that auto-generated impl block being unused.

Could you change the diagnostics to say "unused impl block generated from X derive", maybe?

I'm not sure if I have the same checkout as you but my checkout says the method is on the Node type while the function in rustc_middle is this:

It's possible I just confused the two functions, I did this late at night :sweat_smile: I probably won't have time to double-check it in the near future.

Yeah this is because it doesn't consider the "in " part of for loops as "use". I remember encountering it myself. I think this is a save-analysis bug.

Is there an open issue in rust-lang/rust for this? and for the rest of the save-analysis bugs?

est31 commented 3 years ago

Could you change the diagnostics to say "unused impl block generated from X derive", maybe?

Right now the code just copies over the kind from the json output and prints it in the main function. However, it might be possible to study the json output and come up with a heuristic to detect impl blocks, and then change the kind value to "impl block" instead of Type.

Is there an open issue in rust-lang/rust for this? and for the rest of the save-analysis bugs?

Some bugs are linked to in the warnalyzer readme, especially the report about macros. Outside of the readme linked ones, I'm not aware of any reports. One should maybe first conduct a search and then file some reports? With the save-analysis bugs I reported I put a high "quality bar": reproduction steps in terms of rls, minimal reproducing example, and pasting json to point out the issue. Similar to how rustc devs express llvm bugs they hit using C code :).