dead_code suggestion would break the semantics of code using#[repr(transparent)]

[tamird]

This occurs in aya:
https://github.com/aya-rs/aya/blob/e1aefa4e87970553dc60549141b3b4cf883f4366/bpf/aya-bpf/src/helpers.rs#L734-L737

On a nightly that includes #118297 I get:

error: field `0` is never read
   --> bpf/aya-bpf/src/helpers.rs:737:22
    |
737 | pub struct PrintkArg(u64);
    |            --------- ^^^
    |            |
    |            field in this struct
    |
    = note: `PrintkArg` has a derived impl for the trait `Clone`, but this is intentionally ignored during dead code analysis
    = note: `-D dead-code` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(dead_code)]`
help: consider changing the field to be of unit type to suppress this warning while preserving the field numbering, or remove the field
    |
737 | pub struct PrintkArg(());

In a strict sense the compiler is correct. That's because PrintkArg is used like so:

pub unsafe fn bpf_printk_impl<const FMT_LEN: usize, const NUM_ARGS: usize>(
    fmt: &[u8; FMT_LEN],
    args: &[PrintkArg; NUM_ARGS],
) -> i64 {
    // This function can't be wrapped in `helpers.rs` because it has variadic
    // arguments. We also can't turn the definitions in `helpers.rs` into
    // `const`s because MIRI believes casting arbitrary integers to function
    // pointers to be an error.
    let printk: unsafe extern "C" fn(fmt: *const c_char, fmt_size: u32, ...) -> c_long =
        mem::transmute(6usize);

    let fmt_ptr = fmt.as_ptr() as *const c_char;
    let fmt_size = fmt.len() as u32;

    match NUM_ARGS {
        0 => printk(fmt_ptr, fmt_size),
        1 => printk(fmt_ptr, fmt_size, args[0]),
        2 => printk(fmt_ptr, fmt_size, args[0], args[1]),
        3 => printk(fmt_ptr, fmt_size, args[0], args[1], args[2]),
        _ => gen::bpf_trace_vprintk(fmt_ptr, fmt_size, args.as_ptr() as _, (NUM_ARGS * 8) as _),
    }
}

Thus the compiler's suggestion would break the semantics of this code. Playground. /cc @shepmaster

[shepmaster]

Yes, the standard library had something similar and we allowed the warning:

rust/library/alloc/src/boxed/thin.rs

Lines 173 to 175 in 26194a3

	#[repr(transparent)]
	#[allow(dead_code)] // Field only used through `WithHeader` type above.
	struct WithOpaqueHeader(NonNull<u8>);

Note that this isn't actually new, just recently applied to tuple struct fields. If you had used a named field:

#[repr(transparent)]
pub struct PrintkArg {
    z: u64,
}

You'd get less of a suggestion:

warning: field `z` is never read
 --> src/lib.rs:2:24
  |
2 | pub struct PrintkArg { z: u64 }
  |            ---------   ^
  |            |
  |            field in this struct
  |
  = note: `#[warn(dead_code)]` on by default

In #119645 we are discussing improvements to the lint message.

My own opinion is that the combination of #[repr(transparent)] and never reading the field is such a niche case that I don't expect that the diagnostic suggestion should mention it at all. Perhaps the lint documentation could.

[shepmaster]

Of note is that #[repr(C)] appears to disable the warning here. That may be evidence towards doing the same for repr(transparent).

[chorman0773]

Since repr(transparent) means you can either transmute to the field or use it on an ABI boundary, I'd call the dead_code lint on it a false positive.

We had the same issue pop up in lccc for a type with a single ignored field that exists for a padding byte so that the definition can be matched in pre-C++20 code.

[traviscross]

@rustbot labels +T-lang +I-lang-nominated

Nominating so we can answer the question, "should we suppress this lint for #[repr(transparent)] as we do for #[repr(C)], for both field and tuple structs?"

[asquared31415]

I think there's some utility in having users allow (or even expect) dead_code on a transparent struct that is only accessed indirectly. It documents the purpose of the code better. While this is a style concern rather than a semantics concern, I think that it is worth considering.

However it is weird that transparent and C aren't the same. I think that the decision should be made for both explicit reprs simultaneously.

[joshtriplett]

We talked about this in today's @rust-lang/lang meeting. We agreed that we should treat repr(transparent) like repr(C) in this regard.

[traviscross]

@rustbot labels -I-lang-nominated

As Josh said, there was a clear consensus to do this. Let's unnominate.

[compiler-errors]

I'm assigning this P-high because we definitely should fix it since it may inadvertently affect unsafe code via the transparent repr.