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

unneeded_field_pattern #1741

Closed
nipunn1313 opened this issue May 10, 2017 · 6 comments · Fixed by #5200
Closed

unneeded_field_pattern #1741

nipunn1313 opened this issue May 10, 2017 · 6 comments · Fixed by #5200
Labels
L-correctness Lint: Belongs in the correctness lint group S-needs-discussion Status: Needs further discussion before merging or work can be started

Comments

@nipunn1313
Copy link
Contributor

warning: You matched a field with a wildcard pattern. Consider using `..` instead, #[warn(unneeded_field_pattern)] on by default
   --> src/engine/phases/incremental_sync_step.rs:398:55
    |
398 |         let UpdateHiddenStateReq{root_relative_paths, infinitize_unignores: _} = req;
    |                                                       ^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: Try with `UpdateHiddenStateReq { root_relative_paths, .. }`
    = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#unneeded_field_pattern

Here is an an exact warning from our code. We have a large codebase (several tens of thousands of lines) and get a lot of refactor value out of having a complete destructure without .. syntax.

When we use {..} it increases the probability that someone changes the fields of UpdateHiddenStateReq and forgets to change the usage site here.

One of rust's biggest advantages for us is how it prevents us from shooting ourselves in the foot during refactors or edits in situations like this.

Now I understand that not everyone has this concern and the .. syntax definitely has its benefits, but I do think it's worth discussing if this should be a warning. What do you think?

Thanks

@oli-obk oli-obk added L-correctness Lint: Belongs in the correctness lint group S-needs-discussion Status: Needs further discussion before merging or work can be started labels May 10, 2017
@oli-obk
Copy link
Contributor

oli-obk commented May 10, 2017

I've not been totally happy with this lint either and would be fine with moving this to allow-by-default, and only have it warn when .. is used together with name: _

We definitely need the opposite lint as a restriction lint for safety minded projects to never ever use .. in patterns.

@mcarton
Copy link
Member

mcarton commented May 12, 2017

👍

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 6, 2019

I also ran into this, and tried to made a case here: #4253:

I is not clear that:

  • using .. is better for all cases,
  • even if the type is defined within the same module (which hints that the code has "close" control over its layout), .. is better than exhaustively matching all fields,

This lint helps users discover that .. exists, but I haven't found an heuristic for which it is always clear that .. is a better choice than exhaustively matching, so I don't think this lint should be enabled by default nor w/o clippy::pedantic. It should be an opt-in lint.

The only place where I see no serious drawback of using .. vs explicitly naming the fields is when constructing new types let foo = MyType { x: y, .. }, in case one is using Default::default explicitly for all other fields or something. But even then, if somebody writes all fields, the argument that they might want an error if the type changes is still a good one =/

@jakubadamw
Copy link
Contributor

I also think that this lint doesn't serve a good purpose. It is an advantage of the language that one must explicitly list all fields of a struct when destructuring. It allows the compiler to help identify all sites in the code that ought to be reconsidered when adding a new field to a struct. That gives more correctness confidence when modifying existing structures and code, which is very much in the spirit of Rust.

.. is there for you when you really need it but I don't think Clippy should ever encourage using it. IMHO, quite the opposite.

Herschel added a commit to ruffle-rs/ruffle that referenced this issue Oct 2, 2019
Often times we want to explicty destructure instead of using ..
because the compiler will emit errors if the structure changes.

(see rust-lang/rust-clippy#1741 and #69)
@nagisa
Copy link
Member

nagisa commented Feb 19, 2020

This should definitely become allow-by-default.

@nipunn1313
Copy link
Contributor Author

nipunn1313 commented Feb 20, 2020

I went ahead and created #5200 5200 to execute this

nipunn1313 added a commit to nipunn1313/rust-clippy that referenced this issue Feb 20, 2020
bors added a commit that referenced this issue Feb 20, 2020
Move unneeded_field_pattern to restriction group

Fixes #1741

changelog: Move unneeded_field_pattern to pedantic group
bors added a commit that referenced this issue Feb 20, 2020
Move unneeded_field_pattern to restriction group

Fixes #1741

changelog: Move unneeded_field_pattern to pedantic group
bors added a commit that referenced this issue Feb 21, 2020
Move unneeded_field_pattern to restriction group

Fixes #1741

changelog: Move unneeded_field_pattern to pedantic group
bors added a commit that referenced this issue Feb 21, 2020
Move unneeded_field_pattern to restriction group

Fixes #1741

changelog: Move unneeded_field_pattern to pedantic group
@bors bors closed this as completed in 78a2507 Feb 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L-correctness Lint: Belongs in the correctness lint group S-needs-discussion Status: Needs further discussion before merging or work can be started
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants