Improve diagnostic for when field is never read

[sunjay]

Related to (but does not close) #81658

This completes the first step of @pnkfelix's mentoring instructions but does not actually improve the diagnostics (yet!). The two tests are heavily reduced versions of code from the original bug report.

I've confirmed that the reduced field-used-in-ffi test fails on nightly but passes on stable. This confirms that the regression is reproduced correctly. The drop-only-field test is a case that @pnkfelix mentioned in his mentoring instructions. It is not a regression, but will come in handy when we make the diagnostic smarter by looking at whether the field type implements Drop.

Per the rustc-dev-guide, each test includes a comment summarizing what it is about.

[rust-highfive]

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

[sunjay]

r? @pnkfelix

[sunjay]

@pnkfelix I had to use rotate_right to force the note I added before the note about the lint level that is automatically added by lint.build(...). If there is a way to avoid this hack, I would definitely love to see a better way to do this. :)

My PR now adds the same help message we use for the unused variables lint:

warning: unused variable: `x`
 --> src/main.rs:2:9
  |
2 |     let x = 3;
  |         ^ help: if this is intentional, prefix it with an underscore: `_x`
  |

Here's what that looks like for a field that is never read:

error: field is never read: `items`
  --> $DIR/field-used-in-ffi-issue-81658.rs:13:5
   |
LL |     items: Option<Vec<T>>,
   |     ^^^^^^^^^^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_items`
   |
   = note: The leading underscore signals to the reader that while the field may not be read
           by any Rust code, it still serves some other purpose that isn't detected by rustc.
           (e.g. some values are used for their effect when dropped or used in FFI code
           exclusively through raw pointers)

I thought this consistency would work well for providing people with a quick fix in a format that they're probably already familiar with from the unused variable warning.

I've gone through many iterations of the note at the bottom of the diagnostic. It definitely isn't perfect and it's still longer than I would probably like. I'd love some suggestions for how we can do better here. The diagnostic message matters a lot! :)

The tricky thing is that we need something that also works for items (e.g. functions, structs, enums, associated constants, etc.). Most of the messages I can think of only work well for values or fields. Here's the same error but for an associated constant:

error: associated constant is never used: `BAR`
  --> $DIR/associated-const-dead-code.rs:6:5
   |
LL |     const BAR: u32 = 1;
   |     ^^^^^^^^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_BAR`
   |
   = note: The leading underscore signals to the reader that while the associated constant may not be used
           by any Rust code, it still serves some other purpose that isn't detected by rustc.
           (e.g. some values are used for their effect when dropped or used in FFI code
           exclusively through raw pointers)

This works okay, but we can probably improve on it. If you have some advice for how to get the word wrap to work in all cases, I'd appreciate that too.

---

A previous iteration that I liked better but definitely doesn't work well for items:

error: field is never read: `items`
  --> $DIR/field-used-in-ffi-issue-81658.rs:13:5
   |
LL |     items: Option<Vec<T>>,
   |     ^^^^^^^^^^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_items`
   |
   = note: the leading underscore helps signal to the reader that the field may still serve
           a purpose even if it isn't used in a way that we can detect (e.g. the field
           is only used through FFI or used only for its effect when dropped)

Here's what that looks like for the associated constant example:

error: associated constant is never used: `BAR`
  --> $DIR/associated-const-dead-code.rs:6:5
   |
LL |     const BAR: u32 = 1;
   |     ^^^^^^^^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_BAR`
   |
   = note: the leading underscore helps signal to the reader that the associated constant may still serve
           a purpose even if it isn't used in a way that we can detect (e.g. the associated constant
           is only used through FFI or used only for its effect when dropped)

[pnkfelix]

@rustbot label +S-waiting-on-author -S-waiting-on-review

[sunjay]

@pnkfelix The diagnostic messages are much shorter now! I think the help combined with the note does a good job of conveying both the fix and the reason why it works. Let me know if you have any feedback. :)

Here's what it looks like for fields:

error: field is never read: `items`
  --> $DIR/field-used-in-ffi-issue-81658.rs:13:5
   |
LL |     items: Option<Vec<T>>,
   |     ^^^^^^^^^^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_items`
   |
   = note: the leading underscore signals that this field serves some other purpose
           even if it isn't used in a way that we can detect. (e.g. for its effect
           when dropped or in foreign code)

And for associated constants:

error: associated constant is never used: `BAR`
  --> $DIR/associated-const-dead-code.rs:6:5
   |
LL |     const BAR: u32 = 1;
   |     ^^^^^^^^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_BAR`
   |
   = note: the leading underscore signals that this associated constant serves some other purpose
           even if it isn't used in a way that we can detect.

Let me know if the wording needs to be adjusted. 😁

[sunjay]

It would be more future proof to use a match so Rust's exhaustiveness checks tell us what code to change when we add more variants to the enum. I opted to keep it simple though for the sake of this PR. Let me know if you have thoughts on that.

[estebank]

The structured suggestions are slightly incorrect. We want to point only at Z, not the whole item. I believe this is a problem for enum variants with data and consts as well.

[sunjay]

I was able to get it to look like this, is this sufficient? I'm passing in the right span now but it's still printing the ^ underline.

warning: type alias is never used: `Z`
  --> $DIR/issue-37515.rs:5:1
   |
LL | type Z = dyn for<'x> Send;
   | ^^^^^-^^^^^^^^^^^^^^^^^^^^
   |      |
   |      help: if this is intentional, prefix it with an underscore: `_Z`
   |
   = note: the leading underscore signals that this type alias serves some other purpose even if it isn't used in a way that we can detect.

[sunjay]

@pnkfelix sorry for the delay on getting back to this. I've updated the PR and addressed all the comments. Let me know what the next steps should be. :)

[jyn514]

@sunjay fyi you can change the labels with @rustbot label: +S-waiting-on-review -S-waiting-on-author; I have a feeling that's why this fell under the radar (members of t-compiler get a lot of pings).

[pnkfelix]

@bors r+

lets merge this; the only thing I'd consider changing is the enum variant that controls the emission of the extra parenthetical in the note, and I think such changes can wait until we see motivation from somewhere else.

[bors]

📌 Commit 3e34eb8 has been approved by pnkfelix

[pnkfelix]

@bors rollup

[crlf0710]

@sunjay Ping from triage, it seems this PR might be cause of rustfix check failure. Could you test the related unit tests locally to verify? Thanks!

[pnkfelix]

#81658 (comment) reports that issue #81658 is being worked around on 1.53-beta (by reverting the associated PR), and that issue #81658 solely remains on 1.54-nightly at the moment -- so it would be good to resolve this before 1.54-beta branches.

I'm not sure how much time I have to look into the issues here, but if we don't see word from @sunjay then I'll try to follow up myself.

[pnkfelix]

I still haven't had a chance to look into this.

The schedule for when betas are cut is Friday before release

(Note that the beta cut happens on Tuesday before release, but it will use the commit from Friday, so effectively Friday is the deadline from a compiler contribution point of view.)

Either I'll get chance to look at this today, or I'll put up revert of PR #81473 on mainline today.

[sunjay]

Sorry for the delay. Had a bunch of life things come up that took my time and energy away from this. Working on reproducing/fixing the issue now during my lunch break.

[sunjay]

@pnkfelix Okay so here's what I think is happening: My change introduces a new suggestion which rustfix uses to remove dead code.

The test that is breaking is fix_deny_warnings_but_not_others in fix.rs in the rust-lang/cargo repo. It compiles this program:

#![deny(unused_mut)]
pub fn foo() -> u32 {
    let mut x = 3;
    x
}

fn bar() {}

It then asserts that mut x is changed to x and the dead bar function still remains in the file. I think what is happening is that the new suggestion I added is getting rustfix to delete bar for some reason. (I don't know much about how rustfix works so I am still working on verifying this.) I know that there was no suggestion in that warning before my change, so it makes sense that that is causing an issue now. Here's the old warning:

warning: function is never used: `bar`
 --> src/lib.rs:6:4
  |
6 | fn bar() {}
  |    ^^^
  |
  = note: `#[warn(dead_code)]` on by default

I tried a few different variations on the test but it seems that any dead code gets removed by rustfix with my changes from this PR.

I can get the test running again by using an unused mut as shown below:

#![deny(unused_mut)]

pub fn foo() -> u32 {
    let mut x = 3;
    x
}

pub fn bar() {
    #[allow(unused_mut)]
    let mut _y = 4;
}

With this change all the cargo tests pass and it seems to work fine.

If this is an acceptable fix, does anyone know how to get that landed? I'm in the process of creating a PR for cargo but don't know what the process is after that.

[sunjay]

@pnkfelix PR for rust-lang/cargo is ready now: rust-lang/cargo#9571

The fix I've suggested doesn't break cargo or rely on any of the details of this PR, so we should be able to land that separately, then update the submodules in rust-lang/rust to get this PR through. I've never needed to do this while contributing to rustc before (this is just by best guess at the process) so any guidance would be appreciated. :)

[jyn514]

@sunjay you can update submodules like this (in a clone of rust-lang/rust):

cd src/tools/cargo
git fetch origin
git checkout origin/master
cd ../../../
git add src/tools/cargo
git commit -m "update cargo submodule"

[ehuss]

@sunjay If you need to update cargo to get the fix in, I think it will be tricky because there are some integration issues with the current batch of cargo updates. I have a branch with the fixes, but it may be difficult to get everything in before the beta cutoff.

I'll go ahead and prep a PR tonight, but I'm not sure everything will land in time.

[sunjay]

Thanks for chiming in everyone! I think we could probably get it in quickly by updating the submodules, but I spoke to Manish and Felix and I think we will probably hold off for now. @pnkfelix said this to me in our DMs:

pnkfelix: I think the right strategy will be to revert on mainline tonight , before it turns into the beta tomorrow
pnkfelix: (Revert the PR that we’ve been reverting this whole time on stable + beta, that is)
pnkfelix: And then we can take our time refining your PR
pnkfelix: Into the right thing. This change is not worth the stress of trying to rush in the whole thing in time for the beta by tomorrow.
pnkfelix: (Especially since there are a bunch of other things that do need to happen on the beta to fix other regressions, independent of this one. It’s just too much stuff to be changing in risky ways.)

I sat down with Manish and he showed me another potential way to fix this that is probably a better solution. He mentioned that I might be passing the wrong Applicability to my suggestion and there may be a way to fix this by adjusting that. I'll try that out and report back if I'm able to get it working (just a quick change and test did not work but I might be doing something wrong).

[ehuss]

Sounds good. I went ahead and posted #86207 anyways, just in case you need it. It didn't contain the change I was thinking of, so it was a little simpler than expected.

[JohnCSimon]

Triage:
@sunjay - from reading this PR it looks like it's ready for review.

[sunjay]

Since the revert has landed and the original issue has been closed, I'm guessing this PR is no longer relevant. Happy to re-open and keep working on it if it is still needed.

[Enselic]

@sunjay Hi! Are you still around to resume work on this? The revert was temporary (as indicated by the added FIXMEs) and the intention was always (as far as I can tell) to undo the revert once this PR was finished.