New Lint: Manual Flatten

[nahuakang]

This is a draft PR for Issue 6564.

r? @camsteffen

• Followed [lint naming conventions][lint_naming]
• Added passing UI tests (including committed .stderr file)
• cargo test passes locally
• Executed cargo dev update_lints
• Added lint documentation
• Run cargo dev fmt

---

Please write a short comment explaining your change (or "none" for internal only changes)
changelog: Add new lint [manual_flatten] to check for loops over a single if let expression with Result or Option.

[nahuakang]

@camsteffen Worked to this barebone version that checks for for loops and a single if let statement on Option/Result.

I have a few questions I'd like to ask for help on:

1. Is there a way I can check if the arg is an Iterator or implements IntoIterator so as to adjust the hint to cater to the arg type? Would it make sense to adjust the lint this way?
2. For using span_lint_and_help, is there a way to highlight the if let statement as well as the arg?

Thanks a lot 🙏

[camsteffen]

Good start!

r? @camsteffen

I'm not sure if I'm qualified as a reviewer around here 😄 but I'm glad to help you with this!

Is there a way I can check if the arg is an Iterator or implements IntoIterator so as to adjust the hint to cater to the arg type? Would it make sense to adjust the lint this way?

You mean for x in if let Some(y) = x? Hmm I don't think it will help to check x in that way. Once we see if let Some, we already know that Option implements IntoIterator. If some unrecognized enum implements IntoIterator, we can't assume it is implemented in a way that flatten() would be equivalent.

Here is a good example for what you are doing.

For using span_lint_and_help, is there a way to highlight the if let statement as well as the arg?

Sure. That method takes two Spans. Just use the Span from the Expr with ExprKind::Match and then the Span from the Expr inside: ExprKind::Match(Expr, ..). I assume the "help" would be "this implements IntoIterator" which is great!

Finally, I'd suggest naming the lint manual_flatten. The lint name typically should describe what is wrong with the code.

[camsteffen]

To clarify about the lint name, a for loop over Option is not inherently "bad". It is only "bad" if Nones are just skipped over.

[flip1995]

I'm not sure if I'm qualified as a reviewer around here

@camsteffen The question is if you want to get qualified as a reviewer here. :)

I think this is a "good-first-review" PR. If you think this PR is ready, just ping me to take a quick look and then merge it 👍

[camsteffen]

The question is if you want to get qualified as a reviewer here. :)

Sure!

just ping me

Will do.

[nahuakang]

@camsteffen Hey Cam :) Thanks for the help. I did not explain myself well (and perhaps don't understand the language well enough either). There are two scenarios from what I can see:

// Scenario 1: probably a rare case
for n in vec![Some(1), Some(2), Some(3)].iter() {
    if let Some(n) = n {
        println!("{}", n);
    }
}

// Scenario 2
let x = vec![Some(1), Some(2), Some(3)];
for n in x {
    if let Some(n) = n {
        println!("{}", n);
    }
}

For the first one, if we recommend flatten, we'd only need to say:

Consider using flatten on the Iterator: vec![Some(1), Some(2), Some(3)].iter().flatten()
For the second one, if we recommend flatten, we'd need to say:
x implements IntoIterator. Consider using flatten on the Iterator: x.iter().flatten().

So my first original question was how we could know if the type for the arg as in for ... in arg is an Iterator or it implements IntoIterator?

[camsteffen]

Ah right, that is a little tricky.

In for x in iter, iter implements IntoIterator as a rule. The question is whether iter is already an Iterator or if there is an implicit call to into_iter(). Implicit adjustments like this are called..."adjustments". You can get the type of an expression pre-adjustment using cx.typeck_results().expr_ty(_) (rather than expr_ty_adjusted). Check whether that type implements Iterator using get_trait_def_id and implements_trait from utils.

[nahuakang]

@camsteffen Check out commit 005b0fd. The new lint already brought an internal improvement via dogfood 🐶

Edit: I decided to do the linting as show in the stderr for now. Though it's not ideal, it marks both the for pat in arg as well as the if let statement. Did not make parent_iter work properly 🙈

[camsteffen]

Add a test for if let None = .. inside the loop (should not lint). I think you will have to adjust for this. See the link I posted above for an example of detecting a Some constructor.

I found out that you can get the Span for the entire for-loop by using arms[0].span in higher::for_loop. If you feel so inclined, you can adjust that function to additionally return a Span.

[camsteffen]

Oh you need to suggest into_iter() instead of iter().

[camsteffen]

A couple minor things and then it looks good! Can you rebase onto master and squash into fewer commits?

[camsteffen]

@flip1995 ready for final review

[flip1995]

@camsteffen great first review! You missed some things, that come up in some Clippy PRs, but those are things you'll pick up with experience 😉

@nahuakang Code LGTM so far. Mostly tests are missing (that may result in some changes that need to be made).

[flip1995]

r? @flip1995

[flip1995]

Two small things left.

(Remember to bless the tests again after those changes)

[flip1995]

@bors r+

Thanks!

[bors]

📌 Commit 2f8a8d3 has been approved by flip1995

[bors]

⌛ Testing commit 2f8a8d3 with merge 1551f19...

[bors]

💔 Test failed - checks-action_test

[flip1995]

@bors retry

[bors]

⌛ Testing commit 2f8a8d3 with merge 357c6a7...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 357c6a7 to master...

[nahuakang]

@flip1995 @camsteffen Thank you both so much for your patience and help!