Add detection of unused struct fields

[ghost]

This uncovered some dead code, most notably in middle/liveness.rs, which I think suggests there must be something fishy with that part of the code.

The #[allow(dead_code)] annotations on some of the fields I am not super happy about but as I understand, marker type may disappear at some point.

[thestinger]

It would be nice to teach this to ignore the marker types. They're lang items so it's possible to detect these without too much effort. It would also make sense to ignore a struct if it has #[repr(C)].

[ghost]

@thestinger I think #[repr(C)] can only be used for enums.

As for markers, that does sound like a good idea! I made that change.

The warning is still a false positive for resources (locks) but I do not see a way to whitelist those other than whitelisting types implementing Drop but that would introduce some false negatives as well.

[thestinger]

It's also going to be used for every struct requiring C layout now, but I'm unsure if that pull request landed. The RFC for that was accepted.

[thestinger]

Ah, it didn't land - it's #14479.

[ghost]

@thestinger I see, thanks! I went ahead and I added repr(C) handling to this PR.

[huonw]

This needs a rebase.

[alexcrichton]

I'm a little worried about how noisy our lints are becoming, it seems there are some legitimate use cases here that are being warned about and it's annoying to have to put #[allow(dead_code)] all over the place.

I think that whitelisting fields of #[repr(C)] will go a long way, but I also think another piece that could help is not warning about fields that start with an underscore (like the mutability/unused variable lints). I imagine that the amount of #[allow(dead_code)] will go down, but perhaps not all the way. I would ideally like to see 0 additions of #[allow(dead_code)] as part of this PR, but that's not necessarily a requirement per-se.

I added repr(C) handling to this PR.

Was this left out by accident? I may have missed it as well.

[huonw]

I also think another piece that could help is not warning about fields that start with an underscore (like the mutability/unused variable lints).

👍 mirrors the use of local-variables guards too: let _guard = lock.lock(); ....

[ghost]

Was this left out by accident? I may have missed it as well.

Yes, no idea how that'd happened. I couldn't even find it in my reflog!

I agree that noisy lints are bad. But I was concerned that limiting the warnings to a smaller subset of scenarios reduces their utility. In this case, though, the leading underscore rule does seem like a good solution!

Updated to include both repr(C) and leading _ changes.

[huonw]

It seems... weird that this field would be unused. Why is it creating a mutex without using it?

[ghost]

It is using it but not on Linux. I'll make the field compile-time conditional to non-Linux.

[ghost]

Okay, that's not really possible. I'll stick with an underscore, then.

[alexcrichton]

Awesome work! I'm quite happy with how the leading underscore turned out.

[huonw]

Agreed. (Travis failed.)

[ghost]

@alexcrichton So this failed on the 32-bit Linux bot, which I've fixed, and it looks like it's bootstrapped on the other bots before the builds were interrupted so hopefully it should go through next time.

[ghost]

@alexcrichton Oh dear, it's Android now. Third time's a charm?

I also went ahead and cherry-picked @cmr's fc44834 from #14479.

[emberian]

The compiler, today, should reject #[repr(C)] on structs, so that's not a very good idea.

[emberian]

Ah I see, nevermind. First time seeing this patch.