Wrong suggestion from single_match

[killercup]

I'm trying to add clippy to diesel (again), and just got this:

error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let`
   --> /Users/pascal/Projekte/tools/diesel/diesel/src/types/impls/tuples.rs:145:25
    |
145 |                           match self.$idx {
    |  _________________________^ starting here...
146 | |                             ColumnInsertValue::Expression(_, ref value) => {
147 | |                                 try!(value.collect_binds(out));
148 | |                             }
149 | |                             _ => {}
150 | |                         }
    | |_________________________^ ...ending here
...
296 |   tuple_impls! {
    |   - in this macro invocation
    |
note: lint level defined here
   --> /Users/pascal/Projekte/tools/diesel/diesel/src/lib.rs:7:9
    |
7   | #![deny(warnings, missing_debug_implementations, missing_copy_implementations)]
    |         ^^^^^^^^
help: try this
    |                         if let ColumnInsertValue::Expression(_, ref value) = self.$idx {
    |                             ColumnInsertValue::Expression(_, ref value) => {
    |                                 try!(value.collect_binds(out));
    |                             }
    |                             _ => {}
    |                         }
...
    = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#single_match

The suggestion contains the correct line for if-let itself, but the body is an exact copy of the match arms, not the expression on the right side of the single match arm.

The correct suggestion would be:

if let ColumnInsertValue::Expression(_, ref value) = self.$idx {
    try!(value.collect_binds(out));
}

As you can see, this is inside a macro. I can try to make a smaller example to reproduce this if you want.

[oli-obk]

We're probably just taking the wrong span. Since I'm sure this worked before, we probably messed up a rustup.

[oli-obk]

We should probably add a test with a single line match to produce a single line if let so we can test te suggestion

[killercup]

There are tests for this, but the problem seems to be that this is in a macro. Investigating further, it seems that the snippet helper is not doing what it should: This call evaluates to

"t.$idx { Foo::A(ref val) => { println!(\"42\"); }, _ => {} }\n            //~^ ERROR you seem to be trying to use match\n            //~| HELP try\n            //~| SUGGESTION if let Foo::A(ref val) = t.$idx { println!(\"4\"); };\n        }}\n    }\n\n    issue_1404!(0"
"{ println!(\"42\"); }"

when given this example file to lint:

#![feature(plugin)]

#![plugin(clippy)]
#![deny(clippy)]
#![allow(unused, if_let_redundant_pattern_matching)]

fn main() {
    #[derive(Debug)]
    enum Foo {
        A(String),
        B,
    }

    struct Thingy(Foo);

    macro_rules! issue_1404 {
        ($idx:tt) => {{
            let thingy = Thingy(Foo::A("Foo".into()));
            let t = &thingy;

            match t.$idx { Foo::A(ref val) => { println!("42"); }, _ => {} }
        }}
    }

    issue_1404!(0)
}

which causes it to show this error:

error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let`
  --> tmp/repl.rs:21:13
   |
21 |             match t.$idx { Foo::A(ref val) => { println!("42"); }, _ => {} }
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
25 |     issue_1404!(0)
   |     -------------- in this macro invocation
   |
note: lint level defined here
  --> tmp/repl.rs:4:9
   |
4  | #![deny(clippy)]
   |         ^^^^^^
help: try this
   |             if let Foo::A(ref val) = t.$idx { Foo::A(ref val) => { println!("42"); }, _ => {} }
   |         }}
   |     }
   |
   |     issue_1404!(0 { println!("42"); }
   = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#single_match

error: aborting due to previous error

[oli-obk]

Thanks for the detailed report and minimal example! Cooking up a fix now.

[oli-obk]

ok... looks like rustc is generating the wrong span here.

[oli-obk]

ok, so I know what's going on.

rustc expands issue_1404!(0) to macro_tokens 0 macro_tokens. When these tokens are parsed, the parser stores a tokens position, parses on, and creates a span from the stored position to where it is now. But when you parse 0 inside those macro_tokens, you get a span from inside the macro_rules to behind 0, which makes no sense.

Not sure how to fix it yet though.

[oli-obk]

hmm... looking at it again. Rustc is not doing anything wrong.

We have two options:

1. don't give suggestions when an expansion is involved
2. figure out a way to find the macro_rules arm, which was responsible for this expansion and lint somewhere in there. This has the problem that some expansions might lint, some not, depending on the types involved.

Anyone against simply removing the suggestion if macros are involved? Or even go as far as to not lint at all if macros are involved, since we can't guarantee that changing the macro won't have unintended side-effects (like in this case, suddenly having more arms, so the lint stops applying)

[killercup]

Wow, thanks for further digging into this. I hope regular people don't find themselves having these problem inside of macros, so I'm okay with disabling the lint there 😅

…

Am 28.01.2017 um 19:22 schrieb Oliver Schneider ***@***.***>: hmm... looking at it again. Rustc is not doing anything wrong. We have two options: • don't give suggestions when an expansion is involved • figure out a way to find the macro_rules arm, which was responsible for this expansion and lint somewhere in there. This has the problem that some expansions might lint, some not, depending on the types involved. Anyone against simply removing the suggestion if macros are involved? Or even go as far as to not lint at all if macros are involved, since we can't guarantee that changing the macro won't have unintended side-effects (like in this case, suddenly having more arms, so the lint stops applying) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[ThibsG]

It seems we cannot reproduce this (Playground).

I think that the code for single_match lint (here and here) prevents to lint from a macro or an external macro.

[oli-obk]

Ah great, thanks!