Warn about dead tuple struct fields #92972

FabianWolff · 2022-01-16T16:28:36Z

Fixes #92790. Since the introduction of warnings about unused fields (commit 0271224 in 2014), only named fields got warnings, but not positional fields in tuple structs or tuple enum variants.

Warning about positional fields is probably somewhat controversial because

It is harder to fix than removing named fields, because removing a positional field changes the indices of the following fields, and pattern matches will also have to be adapted.
It will uncover lots of dead fields in existing code, as it already did in the compiler code, which will lead to similar issues as in Ignore derived Clone and Debug implementations during dead code analysis #85200/Unactionable "field is never read" warning for printed structs with the Debug derive #88900.

I am putting this up for discussion now. Maybe this should be nominated for language team sign-off as well?

cc @shepmaster
r? @camelid

rust-highfive · 2022-01-16T16:28:39Z

Some changes occurred in clean/types.rs.

cc @camelid

Changes rustc_apfloat. rustc_apfloat is currently in limbo and you almost certainly don't want to change it (see #55993).

cc @eddyb

camelid · 2022-01-17T19:18:57Z

src/librustdoc/clean/types.rs

@@ -2312,7 +2312,7 @@ impl TypeBinding {
 crate enum SubstParam {
    Type(Type),
    Lifetime(Lifetime),
-    Constant(Constant),
+    Constant,


Hmm... I'm not sure about this change. I'll have to look into it.

eddyb · 2022-01-19T02:11:58Z

compiler/rustc_apfloat/src/ppc.rs

@@ -30,7 +30,7 @@ pub type DoubleDouble = DoubleFloat<ieee::Double>;
 // FIXME: Implement all operations in DoubleDouble, and delete these
 // semantics.
 // FIXME(eddyb) This shouldn't need to be `pub`, it's only used in bounds.
-pub struct FallbackS<F>(F);
+pub struct FallbackS<F>(#[allow(dead_code)] F);


I wish we had a better name for this than "dead code". I'm actually surprised it's not "unused" instead.

I suppose this change is small enough, and just a lint attribute, that it's probably fine.

Well, you could use unused as well, but that is the lint group containing the dead_code lint, so the latter is more precise (not that it makes a difference here). Also, just to be clear: I didn't come up with the name here, it is also called dead_code for named fields.

In any case: Thanks for having a look!

camelid · 2022-01-21T01:19:54Z

I don't think I have enough knowledge of this code nor time to review this PR in its entirety, but I'll try to review some of the rustdoc changes when I get a chance. Thanks for working on this! :)

r? rust-lang/compiler

joshtriplett · 2022-01-21T07:48:59Z

I do think this needs a lang signoff; nominating.

joshtriplett · 2022-01-21T07:52:25Z

Given that this may be harder for people to change, I think it deserves a separate sub-lint of dead_code, such as dead_tuple_field.

I do think this is a good idea, and we should make this change.

joshtriplett · 2022-01-21T07:53:21Z

@bors try @rust-timer queue

rust-timer · 2022-01-21T07:53:24Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-01-21T07:53:37Z

⌛ Trying commit ee9a896 with merge 4b8f3ac2f6e62978663873ba7b287f5df5f314d9...

rust-log-analyzer · 2022-01-21T08:06:38Z

The job dist-x86_64-linux failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[RUSTC-TIMING] build_script_build test:false 0.574
error: field is never read: `0`
  --> library/core/src/iter/sources/empty.rs:28:23
   |
28 | struct FnReturning<T>(fn() -> T);
   |
   |
   = note: `-D dead-code` implied by `-D warnings`
[RUSTC-TIMING] core test:false 11.151
error: could not compile `core` due to previous error
Build completed unsuccessfully in 0:09:42

bors · 2022-01-21T08:06:50Z

💔 Test failed - checks-actions

joshtriplett · 2022-01-25T19:02:20Z

We discussed this in today's @rust-lang/lang meeting. We'd like to go with a gradual approach:

Introduce this as dead_tuple_field, allow-by-default at first.
Announce this, and tell people to try it.
Later, bump it up to warn. People will have the option of disabling dead_tuple_field specifically, if they need to.

Thanks for working on this!

pnkfelix · 2022-01-26T02:59:02Z

I couldn't tell from skimming the PR: What does this warning do for fields of unit type, ()?

I ask because my first reaction when I read:

Warning about positional fields is probably somewhat controversial because

It is harder to fix than removing named fields, because removing a positional field changes the indices of the following fields, and pattern matches will also have to be adapted.
[...]

is that I thought: "well, we should let people include () fields (i.e. not warn about them), and perhaps even have the lint diagnostic here suggest either removing the field or making it have () type. That would keep the indices unchanged."

But, like I said above, I cannot tell from a quick read what this does for (). (I did see it does something special for unit structs (struct S;), but that is a slightly different situation and the handling there is to address a different problem.)

bors · 2022-02-01T20:14:43Z

☔ The latest upstream changes (presumably #93548) made this pull request unmergeable. Please resolve the merge conflicts.

JohnCSimon · 2022-02-27T19:32:06Z

Ping from triage:
@FabianWolff can you please address the comments, merge conflicts, build issues, and post your status on this PR?

Dylan-DPC · 2022-04-08T16:21:44Z

closing this as inactive

FabianWolff · 2022-04-12T13:36:31Z

@rustbot label -S-inactive

@Dylan-DPC Sorry for the delay here, I didn't mean to abandon this. I've updated the branch, can you reopen this PR, please?

Specifically, I have added this functionality as a new allow-by-default lint unused_tuple_struct_fields (name bikesheddable) instead of as part of dead_code, as requested in #92972 (comment).

Furthermore, I have disabled the lint for fields of unit type as requested in #92972 (comment), so error messages now look like this:

error: field is never read: `1`
  --> $DIR/tuple-struct-field.rs:6:21
   |
LL | struct Wrapper(i32, [u8; LEN], String);
   |                     ^^^^^^^^^
   |
help: change the field to unit type to suppress this warning while preserving the field numbering
   |
LL | struct Wrapper(i32, (), String);
   |                     ~~

mati865 · 2022-04-12T13:39:42Z

@FabianWolff if you force push closed PR it can no longer be reopened.
Please create new one and include link to this one.

FabianWolff · 2022-04-12T13:41:01Z

Ah, I didn't know this, I thought I was lacking the necessary permissions for reopening. Thanks for the hint @mati865!

shepmaster · 2022-04-12T14:07:18Z

@Dylan-DPC you should mention the force-push caveat as part of your canned closing message.

bjorn3 · 2022-05-12T15:05:40Z

I believe you can reopen a PR if you change the commit of the branch back to what it was when you closed it.

… r=estebank Warn about dead tuple struct fields Continuation of rust-lang#92972. Fixes rust-lang#92790. The language team has already commented on this in rust-lang#92972 (comment); I have incorporated their requests here. Specifically, there is now a new allow-by-default `unused_tuple_struct_fields` lint (name bikesheddable), and fields of unit type are ignored (rust-lang#92972 (comment)), so error messages look like this: ``` error: field is never read: `1` --> $DIR/tuple-struct-field.rs:6:21 | LL | struct Wrapper(i32, [u8; LEN], String); | ^^^^^^^^^ | help: change the field to unit type to suppress this warning while preserving the field numbering | LL | struct Wrapper(i32, (), String); | ~~ ``` r? `@joshtriplett`

…=estebank Warn about dead tuple struct fields Continuation of rust-lang#92972. Fixes rust-lang#92790. The language team has already commented on this in rust-lang#92972 (comment); I have incorporated their requests here. Specifically, there is now a new allow-by-default `unused_tuple_struct_fields` lint (name bikesheddable), and fields of unit type are ignored (rust-lang#92972 (comment)), so error messages look like this: ``` error: field is never read: `1` --> $DIR/tuple-struct-field.rs:6:21 | LL | struct Wrapper(i32, [u8; LEN], String); | ^^^^^^^^^ | help: change the field to unit type to suppress this warning while preserving the field numbering | LL | struct Wrapper(i32, (), String); | ~~ ``` r? `@joshtriplett`

…ent, r=Nilstrieb dead_code treats #[repr(transparent)] the same as #[repr(C)] In rust-lang#92972 we enabled linting on unused fields in tuple structs. In rust-lang#118297 that lint was enabled by default. That exposed issues like rust-lang#119659, where the fields of a struct marked `#[repr(transparent)]` were reported by the `dead_code` lint. The language team [decided](rust-lang#119659 (comment)) that the lint should treat `repr(transparent)` the same as `#[repr(C)]`. Fixes rust-lang#119659

Rollup merge of rust-lang#120107 - shepmaster:dead-code-repr-transparent, r=Nilstrieb dead_code treats #[repr(transparent)] the same as #[repr(C)] In rust-lang#92972 we enabled linting on unused fields in tuple structs. In rust-lang#118297 that lint was enabled by default. That exposed issues like rust-lang#119659, where the fields of a struct marked `#[repr(transparent)]` were reported by the `dead_code` lint. The language team [decided](rust-lang#119659 (comment)) that the lint should treat `repr(transparent)` the same as `#[repr(C)]`. Fixes rust-lang#119659

rust-highfive assigned camelid Jan 16, 2022

rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jan 16, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 16, 2022