Add real suggestion to option_map_unwrap_or

[m-ober]

changelog: Add real suggestion to option_map_unwrap_or

Fixes #2320

[llogiq]

I'm afraid even with the above suggestions, we will still have to wait for the rustup.

[llogiq]

Now that the rustup is through, we can continue.

[m-ober]

Thanks for the feedback @llogiq - I applied your suggestions. I also added the macro context check, doing nothing in case they differ. Not sure if this is the correct behaviour (but it looks like this is how it's handled in other tests).

[llogiq]

Ok, the build is blocked on the rustfmt check. You can call util/dev fmt from the clippy project root to call rustfmt on all subprojects. Also please prepend a //run-rustfix line to the UI test, run cargo test and bless the result (this will call rustfix to actually apply our suggestion). Otherwise it looks good. Good job for a first-time contribution! 👍

[m-ober]

Thanks again. Should be good now 😀

[flip1995]

Impl looks really good!

As @llogiq suggested:

Also please prepend a //run-rustfix line to the UI test

Since the tests for this lint are in the pile of tests in methods.rs you first have to extract them in a new file. (This would also help with #2038) Just move these tests:

rust-clippy/tests/ui/methods.rs

Lines 165 to 204 in 2f500a5

	/// Checks implementation of the following lints:
	/// * `OPTION_MAP_UNWRAP_OR`
	/// * `OPTION_MAP_UNWRAP_OR_ELSE`
	#[rustfmt::skip]
	fn option_methods() {
	let opt = Some(1);
	// Check `OPTION_MAP_UNWRAP_OR`.
	// Single line case.
	let _ = opt.map(|x| x + 1)
	// Should lint even though this call is on a separate line.
	.unwrap_or(0);
	// Multi-line cases.
	let _ = opt.map(|x| {
	x + 1
	}
	).unwrap_or(0);
	let _ = opt.map(|x| x + 1)
	.unwrap_or({
	0
	});
	// Single line `map(f).unwrap_or(None)` case.
	let _ = opt.map(|x| Some(x + 1)).unwrap_or(None);
	// Multi-line `map(f).unwrap_or(None)` cases.
	let _ = opt.map(|x| {
	Some(x + 1)
	}
	).unwrap_or(None);
	let _ = opt
	.map(|x| Some(x + 1))
	.unwrap_or(None);
	// macro case
	let _ = opt_map!(opt, |x| x + 1).unwrap_or(0); // should not lint
	// Should not lint if not copyable
	let id: String = "identifier".to_string();
	let _ = Some("prefix").map(|p| format!("{}.{}", p, id)).unwrap_or(id);
	// ...but DO lint if the `unwrap_or` argument is not used in the `map`
	let id: String = "identifier".to_string();
	let _ = Some("prefix").map(|p| format!("{}.", p)).unwrap_or(id);

in a new test file named tests/ui/option_map_unwrap_or.rs. Then add // run-rustfix to the top of the file and go through the test update process again.

[bors]

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

[llogiq]

@m-ober is there anything you need besides time to get this PR to a mergeable state? If we can somehow help, feel free to ask.

[m-ober]

I tried to finish the PR according to @flip1995's instructions but it seems there is a problem:

error: called `map(f).unwrap_or(None)` on an Option value. This can be done more directly by calling `and_then(f)` instead
  --> $DIR/option_map_unwrap_or.rs:33:13
   |
LL |     let _ = opt.map(|x| Some(x + 1)).unwrap_or(None);
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: use `map_or` instead
   |
LL |     let _ = opt.map_or(None, |x| Some(x + 1));
   |                 ^^^^^^ ^^^^^                --

The former suggestion was to use and_then here, but my implementation just replaces everything with map_or 😦 I guess I should extend the lint to correctly apply and_then for these cases?

Looking at option_map_unwrap_or.fixed it seems that nothing at all is changed. Why is that? Did I forget something?

[llogiq]

No idea, perhaps the applicability is not set correctly?

[flip1995]

Looking at option_map_unwrap_or.fixed it seems that nothing at all is changed. Why is that? Did I forget something?

Oh multipart suggestions can't be applied currently. See rust-lang/rustfix#162, rust-lang/rust#53934

Just remove the // run-rustfix comment and add a FIXME comment to add run-rustfix once this is supported by rustfix.

[m-ober]

I have removed the "WIP" tag as I think the PR should be in good shape now. The failing pipeline should not be related to my changes.

[flip1995]

LGTM now. ping @llogiq

[flip1995]

ping from triage @m-ober. It seems the only open comment is #4634 (comment). Do you need some help with that?

[m-ober]

@flip1995 Sorry, I hope I will find some time on the weekend to finally finish this pr 😃

[bors]

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

[m-ober]

I have implemented the change suggested by @llogiq - I hope everything is fine now :)

[llogiq]

I'll look into that later today or tomorrow. Thank you!

[llogiq]

LGTM, but we're blocked on #4972.

[llogiq]

Ok, this is ready to merge now.

[llogiq]

@bors: r+

[bors]

📌 Commit c5046fd has been approved by llogiq

[bors]

⌛ Testing commit c5046fd with merge 0ddccc4...

[bors]

💔 Test failed - checks-travis

[m-ober]

Oops, added the "changelog:" line.

[llogiq]

No biggie! @bors retry

[bors]

⌛ Testing commit c5046fd with merge cecaca3...

[bors]

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