option_if_let_else suggestion inapplicable when mutable borrow or owned value is used in both branches

[Arnavion]

#[deny(clippy::option_if_let_else)]
pub fn foo(o: Option<i32>, mut f: impl FnMut(i32)) {
    if let Some(i) = o {
        f(i)
    }
    else {
        f(5)
    }
}

Apart from having the same problem as #5821, in this particular case even the suggestion of o.map_or_else(|| f(5), |i| f(i)) would be wrong because f is being used as a &mut, so it cannot be used with two closures at the same time. This also happens for f: impl FnOnce(i32).

I don't think there's any better way to write this code using any of the Option combinators, so it seems to be the lint should not fire at all.

Meta

• cargo clippy -V: clippy 0.0.212 (39d5a61 2020-07-17)
• rustc -Vv:

  rustc 1.47.0-nightly (39d5a61f2 2020-07-17)
  binary: rustc
  commit-hash: 39d5a61f2e4e237123837f5162cc275c2fd7e625
  commit-date: 2020-07-17
  host: x86_64-unknown-linux-gnu
  release: 1.47.0-nightly
  LLVM version: 10.0
  

[rail-rain]

I've found a real world example inside Clippy where presumably this bug led to an incorrect use of crate::util::snippet_with_applicability, which slipped into the code base along with the lint itself. I'm not sure whether we should immediately revert the change with #[allow(clippy::option_if_let_else)] for the time being or can leave it until this issue gets fixed though.

rust-clippy/clippy_lints/src/write.rs

Lines 299 to 306 in 23db542

	let mut applicability = Applicability::MachineApplicable;
	let suggestion = expr.map_or_else(
	|| {
	applicability = Applicability::HasPlaceholders;
	Cow::Borrowed("v")
	},
	|e| snippet_with_applicability(cx, e.span, "v", &mut Applicability::MachineApplicable),
	);

[ebroto]

Nice catch! I would say let's allow the lint for now and fix that.

BTW wouldn't this be a nice candidate for a lint? I can't think now of a case where you actually want to pass a &mut to a temporary to a function.

[rail-rain]

OK, I made a PR for it (and it's just quickly merged while I'm writing this). Thanks for the reply!

Regarding an new lint, it makes sense for me; considering a function taking a mutable reference probably means the reference is some kind of an "out" parameter.

[brightly-salty]

I still have this issue on nightly, I don't know if it's been fixed or not.

[edward-shen]

I can confirm this issue still exists as I'm running into this on cargo 1.54.0-nightly (e51522ab3 2021-05-07).