Warn about dead tuple struct fields by FabianWolff · Pull Request #95977 · rust-lang/rust (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation47 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

FabianWolff

Continuation of #92972. Fixes #92790.

The language team has already commented on this in #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 (#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

@rust-highfive

Some changes occurred in clean/types.rs.

cc @camelid

Thank you for submitting a new PR for the library teams! If this PR contains a stabilization of a library feature that has not already completed FCP in its tracking issue, introduces new or changes existing unstable library APIs, or changes our public documentation in ways that create new stability guarantees then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary

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

cc @eddyb

Some changes occurred in src/tools/rustfmt.

cc @rust-lang/rustfmt

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

@rustbot 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

Apr 12, 2022

calebcartwright

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review only applicable to the rustfmt subset of the diff

@rust-log-analyzer

This comment has been minimized.

eddyb

@bors

@rust-log-analyzer

This comment has been minimized.

@bors

@pnkfelix

camelid

@joshtriplett

Just want T-lang to confirm that all the items from #92972 (comment) are addressed.

Confirming that this does do what that comment asked for: the lint is allow-by-default.

@joshtriplett joshtriplett added relnotes

Marks issues that should be documented in the release notes of the next release.

and removed I-lang-nominated

Nominated for discussion during a lang team meeting.

labels

May 31, 2022

@joshtriplett

Labeling relnotes so that this gets mentioned in the release notes, and people have an opportunity to enable it and fix issues before it becomes warn-by-default.

Mark-Simulacrum

@apiraino

What's the current status? Maybe ready for another round of review? Please switch back to the author if more work is needed, thanks!

@rustbot ready

@apiraino

Re-roll for another reviewer (see Zulip comment)

r? rust-lang/compiler

estebank

estebank

estebank

Contributor

@estebank estebank left a comment • Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes look good. r=me after:

@rustbot

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

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

cc @eddyb

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

cjgillot

@@ -644,32 +689,46 @@ struct DeadVisitor<'tcx> {
ignored_derived_traits: &'tcx FxHashMap<LocalDefId, Vec<(DefId, DefId)>>,
}
enum ShouldWarnAboutField {
Yes(bool), // positional?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes(bool), // positional?
Named,
Positional,

This will probably be clearer.

cjgillot

@@ -702,6 +765,21 @@ impl<'tcx> DeadVisitor<'tcx> {
are = pluralize!("is", span_len),
));
if is_positional {
err.multipart_suggestion(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The suggestion seems a bit convoluted, and may be more confusing that helping. Since we don't give a suggestion in the non-positional case, do we need a suggestion?

cjgillot

@@ -829,24 +909,37 @@ fn check_mod_deathness(tcx: TyCtxt<'_>, module: LocalDefId) {
continue;
}
let mut is_positional = false;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Positional fields are all or nothing. Can is_positional be computed only once, and passed around?

flip1995

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clippy changes LGTM now. Thanks for addressing those!

@Dylan-DPC

@bors

📌 Commit d59b34b has been approved by estebank

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Aug 2, 2022

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request

Aug 2, 2022

@Dylan-DPC

… 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

@Dylan-DPC

@FabianWolff

@FabianWolff

@Dylan-DPC The test failure seems to have been due to some expected line numbers shifting because of the merge. I have rebased the branch again and adjusted the line numbers, so it should be good to go again, I hope.

@Dylan-DPC

@bors

📌 Commit e3c7e04 has been approved by estebank

It is now in the queue for this repository.

@bors

@bors

@rust-timer

Finished benchmarking commit (9bbbf60): comparison url.

Instruction count

mean1 max count2
Regressions 😿 (primary) N/A N/A 0
Regressions 😿 (secondary) 1.2% 1.5% 3
Improvements 🎉 (primary) N/A N/A 0
Improvements 🎉 (secondary) -1.4% -1.6% 6
All 😿🎉 (primary) N/A N/A 0

Max RSS (memory usage)

Results

mean1 max count2
Regressions 😿 (primary) N/A N/A 0
Regressions 😿 (secondary) N/A N/A 0
Improvements 🎉 (primary) N/A N/A 0
Improvements 🎉 (secondary) -2.5% -5.8% 13
All 😿🎉 (primary) N/A N/A 0

Cycles

Results

mean1 max count2
Regressions 😿 (primary) N/A N/A 0
Regressions 😿 (secondary) 2.9% 4.8% 3
Improvements 🎉 (primary) N/A N/A 0
Improvements 🎉 (secondary) N/A N/A 0
All 😿🎉 (primary) N/A N/A 0

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Footnotes

  1. the arithmetic mean of the percent change ↩ ↩2 ↩3
  2. number of relevant changes ↩ ↩2 ↩3

@nnethercote

Perf changes here are pretty much a wash: lots of small improvements and regressions, which makes sense for this kind of broad-but-shallow change. I don't think anything more needs to be done here.

@rustbot label: +perf-regression-triaged

wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request

Oct 11, 2022

@he32

Pkgsrc changes:

Upstream changes:

Version 1.64.0 (2022-09-22)

Language

Compiler

Libraries

Stabilized APIs

These types were previously stable in std::ffi, but are now also available in core and alloc:

These types were previously stable in std::os::raw, but are now also available in core::ffi and std::ffi:

These APIs are now usable in const contexts:

Cargo

Misc

Compatibility Notes

Internal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request

Nov 16, 2022

@he32

Pkgsrc changes:

Upstream changes:

Version 1.64.0 (2022-09-22)

Language

Compiler

Libraries

Stabilized APIs

These types were previously stable in std::ffi, but are now also available in core and alloc:

These types were previously stable in std::os::raw, but are now also available in core::ffi and std::ffi:

These APIs are now usable in const contexts:

Cargo

Misc

Compatibility Notes

Internal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.

Labels

merged-by-bors

This PR was explicitly merged by bors.

perf-regression

Performance regression.

perf-regression-triaged

The performance regression has been triaged.

relnotes

Marks issues that should be documented in the release notes of the next release.

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

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.