Add option_and_then_some lint

[tesuji]

changelog: Add complexity lint to warn about option.and_then(|o| Some(x)) and suggest replacing with option.map(|o| x).

Closes #4299

[flip1995]

It seems that this lint would fit in the methods module.

[tesuji]

Looks like this lint conflicts with tests in

rust-clippy/tests/ui/option_map_or_none.rs

Lines 6 to 13 in 34457fb

	// Check `OPTION_MAP_OR_NONE`.
	// Single line case.
	let _ = opt.map_or(None, |x| Some(x + 1));
	// Multi-line case.
	#[rustfmt::skip]
	let _ = opt.map_or(None, |x| {
	Some(x + 1)
	});

option_map_or_none lint suggest to use x.and_then(|v| Some(v.len())), but then this lint suggest
to use x.map(|v| v.len()).

Should I fix those test?

[flip1995]

2 step linting is totally fine. So I think adding an allow(option_and_then_some) to the test file is the best solution here.

[tesuji]

What about linting against .map_or(None, |x| y) and suggesting to use map(|x| y)
instead?

[flip1995]

That would also be possible. I don't really know how this lint is implemented, so this is a shot in the dark: My concern with this is, that the lint of map_or(None, expr) (porbably) just copies the expr of the closure and builds a suggestion from there. Directly suggesting map(expr') would require to change every return of expr of type Option<T> to a return of type T. Which obviously is way harder to do.

What could be done is, to add a special case to option_map_or_none that suggests map directly in the easy cases (similar to your implementation of this lint)

[bors]

☔ The latest upstream changes (presumably #4348) made this pull request unmergeable. Please resolve the merge conflicts.

[tesuji]

Blocked on #4395

[tesuji]

Ready for another round of review! 🎉

[flip1995]

Couldn't be the lint be applied to Result as well?

[tesuji]

I think we should open a different issue and PR.

[tesuji]

So I have a counter example which we do not want to lint:

fn foo() -> Option<String> {
    let x = Some(String::from("hello"));
    Some("hello".to_owned()).and_then(|s| Some(format!("{}{}", s, x?)))
}

How do I check if expression in Some have try block?

[flip1995]

You can write a visitor, that walks the expression inside the Some(_) and searches for return expressions.

You can search the Clippy repo on how to implement the Iterator or have a look at the documentation: https://doc.rust-lang.org/nightly/nightly-rustc/rustc/hir/intravisit/trait.Visitor.html

[tesuji]

Tests passed locally.

[ghost]

I made a Visitor like that in #3427. I prefer this one but you should probably check for return expressions as well. Does the lint handle the case below correctly?

pub fn example2(x: bool) -> Option<&'static str> {
    Some("a").and_then(|s| Some(if x { "b" } else { return None } ))
}

fn main() {
    println!("{:?}", example2(true));
    println!("{:?}", example2(false));
}

We should probably make a common function for this in util but maybe not in this PR.

[flip1995]

Thanks!

@bors r+

[bors]

📌 Commit 41eba2f has been approved by flip1995

[bors]

⌛ Testing commit 41eba2f with merge d1f1844...

[bors]

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing d1f1844 to master...