False positive for question_mark on borrowed content

[fuerstenau]

Version

$ cargo clippy -V
0.0.212

Code & Confusion

Hopefully sufficient extract:

fn next(&mut self) -> Option<Self::Item> {
	// Fail quickly when at the end
	if self.current_row.is_none() {
		return None;
	};
	// More code could follow here.
	// Hier hingegen könnte ihre Werbung stehen.
}

When linting this, Clippy remarks:

this block may be rewritten with the ? operator
[…]
help: replace_it_with: self.current_row?;

Hence, we obtain:

fn next(&mut self) -> Option<Self::Item> {
	// Fail quickly when at the end
	self.current_row?;
	// More code (but only if we want).
	// Advertising with us is cheap!
	// Only one quatloo per comment that roughly three people will read.
	// (When we include me in the count. Twice.)
}

Then, the compiler complains:

error[E0507]: cannot move out of borrowed content

Speculation

Until someone tells me otherwise, I'll assume that the ? operator is not compatible with borrowed content and Clippy fails to check for this.

[oli-obk]

Hmm... We have two ways to go on from here:

1. suggest self.current_row.as_ref()?
2. do not suggest anything

I'm leaning towards 1. just for the fun of it.

Anyway, to detect this case we need to check the type for moves_by_default and if it does, do one of the two solutions above.

[fuerstenau]

Winter break is coming up and that seems fun. Since this is apparently a “good first issue” and I'm here anyway, I'd like to take a stab at it.

[oli-obk]

Great!

You can get the type by copying

rust-clippy/clippy_lints/src/question_mark.rs

Line 122 in 379c934

let expr_ty = cx.tables.expr_ty(expression);

The arguments to moves_by_default can usually be obtained from the LateContext (e.g. param_env). Other than that it's just some moving around of the code in the file linked above

[fuerstenau]

Apart from the fact that CONTRIBUTING.md could use some work, this was mostly quite doable. However, a new problem appeared.

Consider the following code:

#[derive(Clone)]
pub struct MoveStruct {
	pub opt: Option<Vec<u32>>
}

impl MoveStruct {
	pub fn ref_func(&self) -> Option<Vec<u32>> {
		if self.opt.is_none() {
			return None;
		}
		self.opt.clone()
	}

	pub fn mov_func_reuse(self) -> Option<Vec<u32>> {
		if self.opt.is_none() {
			return None;
		 }
		self.opt
	}

	pub fn mov_func_no_use(self) -> Option<Vec<u32>> {
		if self.opt.is_none() {
			return None;
		}
		Some(Vec::new())
	 }

// Compilation error: Value moved
/*	pub fn mov_func_reuse_with_qm(self) -> Option<Vec<u32>> {
		self.opt?
		self.opt
	}*/

	pub fn mov_func_no_use_with_qm(self) -> Option<Vec<u32>> {
	        self.opt?;
		Some(Vec::new())
	}
}

fn main() {
	let move_struct = MoveStruct { opt: Some(vec![42, 1337]) };
	move_struct.ref_func();
	move_struct.clone().mov_func_reuse();
	move_struct.clone().mov_func_no_use();
	move_struct.clone().mov_func_no_use_with_qm();
}

All three variants without the ? trigger the lint in the variant moves_by_default. In the last case, however, using self.opt? instead of self.opt.as_ref()? is actually possible. Should we distinguish these again as a sub special case and recommend the shorter possibility?

[oli-obk]

In the last case, however, using self.opt? instead of self.opt.as_ref()? is actually possible.

That's very hard to figure out in general. I think it's fine. We can revisit if we actually encounter real world examples.

Apart from the fact that CONTRIBUTING.md could use some work

Specific complaints are very welcome. It's hard to improve when we don't know the stumbling stones.

[fuerstenau]

See PR #3561. That should solve this particular issue.

Specific complaints are very welcome. It's hard to improve when we don't know the stumbling stones.

I'll try to write something on the matter. If there were only a few well-visible stumbling stones, I would have already pointed them out.

[ThomasdenH]

Since the PR has been merged, should this be closed? I think this deserves its own issue.