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

#[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

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), 
        
           );

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.

Fix the wrong use of `snippet_with_applicability`

Auto merge of #5968 - rail-rain:fix_use_snippet_with_applicability, r…

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.

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

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

Auto merge of #7568 - dtolnay-contrib:ifletelse, r=llogiq

Auto merge of #7573 - Jarcho:option_if_let_else, r=giraffate

Navigation Menu

Repository navigation

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

Meta

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	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),
	);

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

Description

Meta

Activity

rail-rain commented on Aug 26, 2020

ebroto commented on Aug 26, 2020

rail-rain commented on Aug 26, 2020

brightly-salty commented on Jan 24, 2021

edward-shen commented on May 11, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions