dead_code lint incorrectly warns about struct fields that aren't dead due to transmuting

[lilyball]

The dead_code lint was recently (#14696) modified to detect dead struct fields. Unfortunately, it warns about fields even when it can't be 100% certain they're dead.

One case where this happens was filed as #15101, where the struct is used as C FFI. That case has the (undocumented) solution of using #[repr(C)] (even though #[repr(C)] hasn't been implemented to actually do anything yet).

Another case that doesn't have a clear solution beyond turning off the warning is a struct that contains fields for the purpose of being memory-layout-compatible with another struct.

Notably, in lilyball/rust-lua@be46e268c0, the file lib.rs defines 3 structs with identical layout. Two of the structs properly use all defined fields, but one of the structs does not use one field (beyond initialization). However, there are methods that use men::transmute() to convert between the struct types. If this method is used, then the allegedly-dead field becomes live, as the destination struct uses that field.

I recognize that this is a bit of an edge case, but this isn't the only one. Besides C FFI, a quick test demonstrates that a supposedly-unused struct field that is in fact printed via a format!("{:?}", foo) directive is also considered dead. I wouldn't be surprised to find out that there are more edge cases either. The basic point here is that the dead_code lint is making an unqualified statement that the field is never used, but it doesn't actually have 100% certainty of that fact. It should be modified to recognize cases where the struct is used in ways the lint can't see into. So far the list of cases includes:

1. Being passed as a *mut to a C FFI function
2. Being passed to mem::transmute()
3. Being passed to ::debug::fmt::secret_poly

And we can probably add casting *mut StructType to any *mut T (via as, e.g. foo as *mut ()) to the list.

/cc @jakub-

You could rename the phantom fields such that they start with an underscore, then the lint will ignore them (much like the unused variable lint ignores variables starting with underscores).

[lilyball]

@jakub- Yeah I discovered that after filing this issue, and that's what I ended up doing, although I am not terribly happy about having my 3 struct definitions now actually differ.

In any case, my basic point here is the dead_code lint is making claims about factual knowledge it doesn't have. There are various constructs (of which I've listed the ones I can think of) that the dead_code lint simply can't see into. It would be great if the dead_code lint could consider these constructs to touch every single field of the struct.

@kballard Right, I agree that the lint's wording seems to imply it being precise (or conservative at the very least) rather than overly pessimistic, which it is now. I'm not sure if the solution to that is to make the lint even more complicated or simply changing the wording to reflect its nature.

I find this lint useful but if the noise/signal ratio has turned out to be too high, then maybe it shouldn't be on by default.

[ktt3ja]

I'm obviously biased, but I think it should be kept enabled by default. The dead-code lint has these two utilities:

• Cleaning the code base of unused code.
• Warning the user of short-term forgetfulness.

I believe the second part is much more useful. From my experience, it's a relief to have the lint warn you that the code you spent the whole day writing is dead because you forget to call that one function. Disabling the lint makes the second utility nonexistent. By the time you think of turning it on you probably have already figured out the problem.

Yes, the lint was written to be conservative so the user can be confident that they may remove the code (the lint went through a lot of bug fix at the beginning to address that concern).

If the noise/signal ratio becomes too high and no good solution can be found, I would prefer that dead items and dead fields be classified differently and keep the warning for the former on by default.

[zwarich]

False positives in on-by-default compiler warnings are terrible and will only cause people to disable useful warnings. Clang has taken an approach of limiting false positives and it has been quite successful.

[lilyball]

@ktt3ja I agree that the dead_code lint should be on-by-default. But that's precisely why it needs to be as conservative as possible, because as @zwarich points out, false positives cause people to start disabling the lint outright.

And it's for this reason that I think that the lint needs to try to learn about things like transmute() and * pointers that can cause a field to be accessed in ways the dead_code lint can't actually see.

[huonw]

Notably, in lilyball/rust-lua@be46e26, the file lib.rs defines 3 structs with identical layout

There's no guarantee they have identical layout, not at the moment anyway. The only way to ensure this (and the only correct way to transmute between different structs, IMO) is to fix the layout with #[repr(C)]. As such, I don't regard this part as a bug (really, only the :? case is a bug IMO, but :? has so many other downsides that I'm not sure it's a huge problem: code bloat, reading private fields, useless with Vec, Rc, Arc...).

An approach for your code would be to define

struct StateLayout { L: *mut raw::lua_State, stackspace: i32 }

pub struct State { data: StateLayout }

pub struct ExternState<'a> { data: StateLayout }
pub struct RawState<'a> { data: StateLayout }

and then you can even avoid transmutes when converting between the types. (Also, don't you need a ContravariantLifetime for ExternState and RawState?)

[lilyball]

@huonw Well, when the code was written, RFC 18 did not exist, so at the time they did in fact have a guarantee of identical layout. You're right in that, today, technically there's no guarantee, but the problem is also still purely theoretical, and practically speaking, if the compiler does start reordering structs, it will almost certainly reorder identically-defined structs into the same layout (yes I know there were examples of using randomized layout, but I would be surprised if that was ever made default behavior).

In any case, given that structs no longer guarantee fixed layout, you are right to point out that my two major sources of concern (transmute() and using * pointers casted to e.g. *u8) both really only make sense with a known struct layout. And the only way to create a fixed struct layout is #[repr(C)], which also squelches the dead_code warning. I do hope that if any other struct representation types are created (that guarantee a known fixed layout), they too will end up squelching the dead_code warning.

As for :?, presumably there are other uses of the debug crate's runtime reflection that would cause the same issue, but I suppose that it really is a bit of a stretch to claim this is a significant issue.

[lilyball]

@huonw Also, I think you're right, I should be using ContravariantLifetime. I actually have no idea what variance the compiler uses by default for lifetime parameters that are otherwise unused.

[steveklabnik]

Triage: here's a test case for the transmute issue:

#[derive(Debug)]
struct Foo {
    x: i32,
}

struct Bar {
    y: i32,
}

fn main() {
    let bar = Bar{ y: 4 };

    let foo: Foo = unsafe { std::mem::transmute(bar) };
    format!("{:?}", foo);
}

This does in fact warn about y but not x.

[ruuda]

An other case where dead_code improperly warns about unused struct fields is if the field is of a type that implements Drop. It might be there to keep something alive even if it is not used otherwise, for instance to keep a file open. This should be fixable: suppress the warning if the field implements Drop.

[huonw]

@ruud-v-a the convention for locals that only care about the destructor works for struct fields too, that is, prefix it with an underscore:

struct Foo {
    _guard: GuardType
}
// similar to
let _guard = mutex.lock();

(#21775 covers that situation specifically.)

[bltavares]

Triaging:

The following test case still warns on y, but not on x.

#[derive(Debug)]
struct Foo {
    x: i32,
}

struct Bar {
    y: i32,
}

fn main() {
    let bar = Bar{ y: 4 };

    let foo: Foo = unsafe { std::mem::transmute(bar) };
    format!("{:?}", foo);
}

Adding #[repr(C)] to the Bar struct does indeed remove the warning.

[Mark-Simulacrum]

As far as I can tell, the only remaining problem is that dead_code warns on fields that are used through transmute. However, #[repr(C)] fixes that; and using transmute without #[repr(C)] is a Bad Idea. Closing. Please reopen if I missed something.