correct unused field pattern suggestions #47922
Merged
Conversation
This file contains 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
The glossaries in the draft rustc-guide book and librustc/README.md state that `NodeId` is being gradually phased out in favor of `HirId`; as such, the present author claims that we ought to have a typedef for efficient `HirId` maps and sets in the module for such, even if no use for them has been made as yet (compatibility constraints preventing the use of it in the author's present unit of work): it is important to create the psychological "affordance" (in James J. Gibson's sense) that `HirId`s are a thing that compiler developers can work with.
Previously, unused variables would get a note that the warning could be silenced by prefixing the variable with an underscore, but that doesn't work for field shorthand patterns, which the liveness analysis didn't know about. The "to avoid this warning" verbiage seemed unnecessary. Resolves rust-lang#47390.
estebank
reviewed
Feb 1, 2018
| 30 | mut hours_are_suns, | ||
| | ^^^^^^^^^^^^^^^^^^ | ||
| | | ||
| = note: consider using `_hours_are_suns` instead |
There was a problem hiding this comment.
This warning is emitted somewhere else. Would you mind checking how hard it'd be to modify that note by checking if it is a field in a struct literal?
There was a problem hiding this comment.
@estebank The PR touches this code (on master, the note starts with "to avoid this warning", which I thought was unnecessary). I had started working on making this a structured suggestion, too, but then backed off when I realized that getting the suggestion right would involve more refactoring than I wanted to do now. Notice that the span includes the mut: in the case of an entirely unused variable, that's fine (the mut is also unused, so replacing both the variable and any muts or refs with variable: _ is correct), but in the case of a mere unused assignment (where we mutate the variable, but don't do anything interesting with it), not only is the mut significant, but there are also other appearances of the variable (requiring a multi-span suggestion).
|
@bors r=estebank |
|
📌 Commit e4b1a79 has been approved by |
kennytm
added
the
S-waiting-on-bors
|
⌛ Testing commit e4b1a79 with merge c0efdab7d210301ff339b52db9ce52b90f1553be... |
|
💔 Test failed - status-appveyor |
|
@bors retry 3 hour timeout |
bors
added a commit
that referenced
this pull request
Feb 6, 2018
…ttern, r=estebank correct unused field pattern suggestions  r? @estebank
|
💔 Test failed - status-appveyor |
bors
added
S-waiting-on-review
|
#46903 is killing our throughput 💔 ☠️ (although this time it was the 64-bit build that timed out) |
|
@bors retry 3 hour timeout |
bors
added
S-waiting-on-bors
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Feb 7, 2018
…ed_field_pattern, r=estebank correct unused field pattern suggestions  r? @estebank
kennytm
added a commit
to kennytm/rust
that referenced
this pull request
Apr 30, 2018
…=estebank Display correct unused field suggestion for nested struct patterns Extends rust-lang#47922 by checking more sophisticated patterns (e.g. references, slices, etc.). Before: ``` warning: unused variable: `bar` --> src/main.rs:37:21 | 37 | &Foo::Bar { bar } => true, | ^^^ help: consider using `_bar` instead | = note: #[warn(unused_variables)] on by default ``` After: ``` warning: unused variable: `bar` --> src/main.rs:37:21 | 37 | &Foo::Bar { bar } => true, | ^^^ help: try ignoring the field: `bar: _` | = note: #[warn(unused_variables)] on by default ``` Fixes rust-lang#50303. r? @estebank
zackmdavis
added a commit
to zackmdavis/rust
that referenced
this pull request
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 pull request
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
r? @estebank