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

Add check on redundant _ variables in structs #482

Merged
merged 1 commit into from
Dec 7, 2015
Merged

Add check on redundant _ variables in structs #482

merged 1 commit into from
Dec 7, 2015

Conversation

GuillaumeGomez
Copy link
Member

cc #479

This is just preview. I need help on some point:

  • My code is called, but the lint is never displayed, why?
  • Is the code correct? I'm not sure the way I search for the "_" pattern is the best.
  • Is the comparison correct? I translate a type into a String, but isn't there another way?

Thanks in advance for your help. :)

@@ -115,6 +118,11 @@ fn check_pat(cx: &LateContext, pat: &Pat, init: &Option<&Expr>, span: Span,
}
} else {
for field in pfields {
if pfields.len() > 0 && pprust::pat_to_string(&*field.node.pat) == "_" {
Copy link
Member

Choose a reason for hiding this comment

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

You dont need pprust here; just check if its a PatWild

@Manishearth
Copy link
Member

I would prefer if this were in its own file (pat.rs?) as an EarlyLintPass. Remember to register the pass in lib.rs!

@GuillaumeGomez
Copy link
Member Author

I updated and added a more precise check. What do you think of it?

PS: it seems to break build because rut-clippy source code has issues like this in the code...

@llogiq
Copy link
Contributor

llogiq commented Dec 7, 2015

A good idea would be to give a hint what the pattern would look like, which should be possible using utils::snippet(..) on the remaining pattern elements. Also we should follow our own advice and change the code to no longer trigger the lint.

@GuillaumeGomez
Copy link
Member Author

@llogiq: This is a bit problematic because changing it seems to break. If we remove one field, it breaks because it says "[field] isn't bound".

@llogiq
Copy link
Contributor

llogiq commented Dec 7, 2015

Have you added , ..? If that doesn't work, perhaps the lint is overzealous?

@@ -15,7 +15,7 @@ declare_lint!(pub SHADOW_REUSE, Allow,
"rebinding a name to an expression that re-uses the original value, e.g. \
`let x = x + 1`");
declare_lint!(pub SHADOW_UNRELATED, Allow,
"The name is re-bound without even using the original value");
"The name is re-bound without even using the original value");
Copy link
Contributor

Choose a reason for hiding this comment

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

Does that single space change anything about the code? Otherwise I think we may remove it from the change to reduce diff clutter. 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Oups. :p

@GuillaumeGomez
Copy link
Member Author

@llogiq: I did, but I must have made a mistake somewhere else. Code updated.


use utils::span_lint;

declare_lint!(pub UNNEEDED_BINDING, Warn,
Copy link
Member

Choose a reason for hiding this comment

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

Should be only one lint, unneeded_bindings. The actual message can be tweaked to handle plurals, but we don't want to create two lints for plurality.

@Manishearth
Copy link
Member

dogfood failure

src/misc_early.rs:25:9: 51:10 error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let` [-D single-match]
src/misc_early.rs:25         match pat.node {
src/misc_early.rs:26             PatStruct(_, ref pfields, _) => {
src/misc_early.rs:27                 let mut wilds = 0;
src/misc_early.rs:28 
src/misc_early.rs:29                 for field in pfields {
src/misc_early.rs:30                     if field.node.pat.node == PatWild {
                     ...
src/misc_early.rs:25:9: 51:10 help: try
if let PatStruct(_, ref pfields, _) = pat.node {
    let mut wilds = 0;

    for field in pfields {
        if field.node.pat.node == PatWild {
            wilds += 1;
        }
    }
    if pfields.len() > 0 && wilds == pfields.len() {
        span_lint(cx, UNNEEDED_BINDINGS, pat.span,
                  "All your fields are bound but not used. \
                   Consider replacing them with `..`");
        return;
    }
    if wilds > 0 {
        for field in pfields {
            if field.node.pat.node == PatWild {
                span_lint(cx, UNNEEDED_BINDING, field.span,
                          "You bound a field that's not being used. \
                           Consider using `..`");
            }
        }
    }
}
for further information visit https://github.com/Manishearth/rust-clippy/wiki#single_match
src/misc_early.rs:34:20: 34:37 error: consider replacing the len comparison with `!pfields.is_empty()` [-D len-zero]
src/misc_early.rs:34                 if pfields.len() > 0 && wilds == pfields.len() {
                                        ^~~~~~~~~~~~~~~~~
src/misc_early.rs:34:20: 34:37 help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#len_zero
error: aborting due to 2 previous errors

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants