FP redundant_pattern_matching: try! macro

[matthiaskrgr]

This code from serde

        fn visit_map<M>(self, mut access: M) -> Result<(), M::Error>
        where
            M: MapAccess<'de>,
        {
            while let Some(_) = try!(access.next_entry::<IgnoredAny, IgnoredAny>()) {}
            Ok(())
        }

caused the warning

warning: redundant pattern matching, consider using `is_some()`
    --> serde/src/private/de.rs:2475:23
     |
2475 |             while let Some(_) = try!(access.next_entry::<IgnoredAny, IgnoredAny>()) {}
     |                       ^^^^^^^   --------------------------------------------------- help: try this: `while _.is_some()`
     |

_.is_some() does not look like it is going to work 😄

clippy 0.0.212 (891e1a8 2020-04-18)

[alex-700]

😄 hm, it seems like snippet here didn't do right thing. We can

• drop the suggestion if snippet_opt(..) is None
• decrease the applicability level
• see carefully, why the snippet_opt(..) is None here

[alex-700]

I investigated the problem. I was wrong in comment about expr.span. It's false in the try! example. In this example I have

expr.span = /rustc/8d67f576b56e8fc98a31123e5963f8d00e40611c/src/libcore/macros/mod.rs:355:9: 360:10

inside the find_sugg_for_if_let method.
Actually it's very strange for me. I expect that this expr.span should point to the full expression while let pats = exprs {}. Yeah, I understand that it's desugaring and I have no "real" expression for this match, but common 😄.
Should I report a bug to rustc? But it's not easy to me to localize it. It seems that a bug can be near the controlflow.

Well, now we need to fix it here in clippy. I suggest to replace let expr_span = expr.span.to(arms[1].span); to let expr_span = arms[1].pat.span;. This is span what I need, because of this and this. But I don't know can I rely on it or not?

I need an advice, @flip1995

[flip1995]

Minimal reproducer:

macro_rules! m {
    () => {
        Some(42u32)
    };
}

fn main() {
    if let Some(_) = m!() {}
    if m!().is_some() {} // This should be suggested
    if Some(42u32).is_some() {} // This is suggested
}

Playground

The problem here is that the macro gets expanded, when building the suggestion. A possible fix is to use snippet_with_macro_callsite to get the snippet. I don't know which spans are involved in the building of the suggestion. Best thing to do here would be to write the test case and then just try to get the correct suggestion.

Should I report a bug to rustc?

Nope, this is not a rustc bug.

[alex-700]

I don't talk about macros here. It's an important thing, but out-of-scope a bit. I talk about the question: what span should corresponds to the match expression with source MatchSource::WhileLetDesugaring? IMHO, it should be the whole while let {} statement, but not a part of it (nor even be a place in an other file in the case of macros).

And the other question is "What should I use to get the span pointed to a whole while let {}"?. I think that arms[1].pat.span will be a good answer, because of links from my previous comment, can we rely on this part of rustc?

Best thing to do here would be to write the test case and then just try to get the correct suggestion.

Of course. I added this test. And already get the correct suggestion. Thanks.

[flip1995]

what span should corresponds to the match expression with source MatchSource::WhileLetDesugaring?

Ah now I get your question. That would be the span of the expression, that destructures to ExprKind::Match

I think that arms[1].pat.span will be a good answer,

I disagree, since the arm is the thing matched against. In this case Some(_)

   while let Some(_) = opt {}
// ^^^^^^^^^^^^^^^^^^^^^^^^^^   expr.span, where expr.kind = ExprKind::Match(cond, arms, MatchSource::WhileLetDesugar)
//           ^^^^^^^            arms[0].pat.span
//                     ^^^      cond.span

// gets lowered to:
   loop {
       match opt {         // cond
           Some(_) => {},  // arms[0]
           _ => break,
       }
   }

[alex-700]

Arrgh... You didn't completely got what I mean, but now much better 😄 Maybe it's because of my english 😞

Ah now I get your question. That would be the span of the expression, that destructures to ExprKind::Match

Yes, it's a good point. I thought the same, but
expr.span = /rustc/8d67f576b56e8fc98a31123e5963f8d00e40611c/src/libcore/macros/mod.rs:355:9: 360:10

This is the result of dbg!(expr.span) from find_sugg_for_if_let function (in the code of our lint), when I run it on while let Some(_) = try!(foo()) {}. Here it's obvious that expr.kind = ExprKind::Match(cond, arms, MatchSource::WhileLetDesugar), because of match in the code above. And I meant this, when talk about rustc bug.

I disagree, since the arm is the thing matched against. In this case Some(_)

I talk about arms[1].pat.span, but not about arms[0].pat.span (you're right about arms[0].pat.span). Of course I understand what arms means. But two lines (here and here) guarantee that arms[1].pat.span is a span pointed to whole our while/if let ... = ... { ... }.

And now I have another question: Does there exists a documentation about "spans", where I can read some stable properties of spans?

[flip1995]

expr.span = /rustc/8d67f576b56e8fc98a31123e5963f8d00e40611c/src/libcore/macros/mod.rs:355:9: 360:10

Oh, that is indeed weird. That should be the whole while let 🤔

I talk about arms[1].pat.span

Ah, I see. I guess the two lines you referenced are kind of a hack, since the _ => break code does not have a span in the user code, so it just gets some span. I wouldn't trust spans, that don't have a couterpart in the source code (i.e. spans from expansions).

Does there exists a documentation about "spans", where I can read some stable properties of spans?

Nothing of rustc internals is anywhere close to stable, that's why we have so much fun with rustup. 😭:smile: The Span documentation maybe. In the rustc-dev-guide there's sadly not much about Spans: Link

[alex-700]

I wouldn't trust spans, that don't have a couterpart in the source code (i.e. spans from expansions).

But I need some span... 😄 In the PR I use arms[1].pat.span with comment which span I want. And IMHO it's ok here. I believe that current tests find the problem, if smth changed.

Actually expr.span didn't must point to whole while let. Speak formally this expr doesn't exists in the source code too. So this behavior can changed too.

[flip1995]

expr.kind = ExprKind::Match(.., WhileLetDesugar), so I would expect, that the expr.span is the whole while let. I asked about this on discord in the #compiler channel. I currently also run a test if it produces any fallout in the rust compiler if this span is changed to the whole while let expression.

Just have a little patience, with the currently open PR, until I figured this out. IMO you were right and this is a bug in the lowering.