Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Unused variable" warning gives incorrect suggestion when binding fields of struct-like variants #47390

Closed
elidupree opened this issue Jan 12, 2018 · 6 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints

Comments

@elidupree
Copy link

Here's a short example:

enum Enum {
  Variant {field: u32},
  OtherVariant,
}

fn main() {
  let whatever = Enum::Variant{field:0};
  if let Enum::Variant {field} = whatever {
    // field left unused
  }
}

This produces the warning:

warning: unused variable: `field`
 --> src/main.rs:8:25
  |
8 |   if let Enum::Variant {field} = whatever {
  |                         ^^^^^
  |
  = note: #[warn(unused_variables)] on by default
  = note: to avoid this warning, consider using `_field` instead

But changing it to _field is an error because that's not the name of the field. The warning should probably suggest to use Enum::Variant {..} instead.

This also occurs in explicit match statements and irrefutable let patterns – probably anywhere you can bind a field of a struct-like variant.

@estebank
Copy link
Contributor

estebank commented Jan 12, 2018

The code to do so needs to account for cases where .. is already being used, and while at it probably collect multiple unused variables in the same struct in the same diagnostic:

warning: unused variables: `field` & `field2`
 --> src/main.rs:8:25
  |
8 |   if let Enum::Variant {field, field2, field3} = whatever {
  |                         ^^^^^  ^^^^^^
  |
  = note: #[warn(unused_variables)] on by default
  = help: to avoid this warning, consider using `..` instead:
  |
8 |   if let Enum::Variant {field3, ..} = whatever {
  |                         ^^^^^^^^^^

instead of

warning: unused variable: `field`
 --> src/main.rs:8:25
  |
8 |   if let Enum::Variant {field, field2, field3} = whatever {
  |                         ^^^^^
  |
  = note: #[warn(unused_variables)] on by default
  = note: to avoid this warning, consider using `_field` instead

warning: unused variable: `field2`
 --> src/main.rs:8:32
  |
8 |   if let Enum::Variant {field, field2, field3} = whatever {
  |                                ^^^^^^
  |
  = note: to avoid this warning, consider using `_field2` instead

In the suggestion there're four cases: we need to add .. because the struct list would end up empty, remove all the unused fields from the list and add , .. because the list is not empty, only remove the unused fields, because the , .. is already there, or replace everything with .. if , .. is already there and the list ends up empty.

@estebank estebank added the A-diagnostics Area: Messages for errors, warnings, and lints label Jan 12, 2018
@daboross
Copy link
Contributor

daboross commented Jan 13, 2018

Just an idea: what about suggesting field2: _ instead? This keeps the behavior of needing to handle new fields but still ignores the specific field value.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 13, 2018

Already reported in #46866

@Mark-Simulacrum
Copy link
Member

Closed #46866 in favor of this as it had less discussion.

@estebank
Copy link
Contributor

Just an idea: what about suggesting field2: _ instead? This keeps the behavior of needing to handle new fields but still ignores the specific field value.

I prefer the output I stated, mainly because it could introduce the concept to people, but if somebody implements that suggestion instead, I would gladly accept it. It might be even better if we suggested both alternatives, but that might be too noisy.

@zackmdavis
Copy link
Member

zackmdavis commented Feb 1, 2018

In the suggestion there're four cases [...]

This would be nontrivial (although probably not difficult) to implement compared to the fieldname: _ suggestions (about which, PR forthcoming), because the liveness checker currently doesn't bother remembering which local variables were created in the same pattern.

@bors bors closed this as completed in e4b1a79 Feb 7, 2018
zackmdavis added a commit to zackmdavis/rust that referenced this issue May 18, 2018
In e4b1a79 (rust-lang#47922), we corrected erroneous suggestions for unused
shorthand field pattern bindings, suggesting `field: _` where the
previous suggestion of `_field` wouldn't even have compiled
(rust-lang#47390). Soon, it was revealed that this was insufficient (rust-lang#50303), and
the fix was extended to references, slices, &c. (rust-lang#50327) But even this
proved inadequate, as the erroneous suggestions were still being issued
for patterns in local (`let`) bindings (rust-lang#50804). Here, we yank the
shorthand-detection and variable/node registration code into a new
common function that can be called while visiting both match arms and
`let` bindings.

Resolves rust-lang#50804.
kennytm added a commit to kennytm/rust that referenced this issue May 19, 2018
…ed_field_pattern_3_straight_to_video, r=estebank

in which the unused shorthand field pattern debacle/saga continues

In e4b1a79 (rust-lang#47922), we corrected erroneous suggestions for unused
shorthand field pattern bindings, suggesting `field: _` where the
previous suggestion of `_field` wouldn't even have compiled
(rust-lang#47390). Soon, it was revealed that this was insufficient (rust-lang#50303), and
the fix was extended to references, slices, &c. (rust-lang#50327) But even this
proved inadequate, as the erroneous suggestions were still being issued
for patterns in local (`let`) bindings (rust-lang#50804). Here, we yank the
shorthand-detection and variable/node registration code into a new
common function that can be called while visiting both match arms and
`let` bindings.

Resolves rust-lang#50804.

r? @estebank
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints
Projects
None yet
Development

No branches or pull requests

6 participants