Using an `Option<&mut T>` in a loop? Clippy complains

I have code similar to what is suggested in How to cleanly use Option<&mut> multiple times? ; I have an Option<&mut T> that I want to use in a loop. But now clippy complains about my use of opt.as_deref_mut(), claiming the type I get is the same as the type I have, so I should just use opt.

But if I do as clippy says, the first iteration of the loop consumes the opt, so any remaining iterations can't use it and my code fails to compile.

Is this just a "false positive" in clippy? Or is there some better way to write the code?

My real code is rsass/transform.rs at 7ef8b2ce6c283e5c3546640e57e62f306ed96c32 · kaj/rsass · GitHub (and a bunch of more instances in the same file). I have done a minimal program to reproduce the issue on the playground.

It is not a false positive: the diagnosis is correct, but the suggested fix is indeed wrong. Since <&mut T as DerefMut>::Target == &mut T, the as_deref() has indeed no effect on the type of the wrapped value. However, as_deref_mut() has another effect which is apparently not accounted for by Clippy: it borrows the Option itself, too!

What you are trying to achieve here is effectively internal reborrowing-within-the-Option. The use of as_deref_mut() is indeed a bit weird since you are not actually using it to perform a Deref conversion. A solution using an explicit borrow_mut() seems to do the trick.

It looks like there are already multiple half-year-old issues ([1], [2]) about this bug.

2 Likes

That does indeed work, thanks! But I'm not really sure what BorrowMut does here, as it seems to convert an &mut &mut T to an &mut T, that sounds more like unborrow to me ...

Also, the adapter code got twice as long. Ah, well, since I use it in multiple places, I might just put it inside an extension trait of myself and have do opt.clopty() what I want. And then I can use your code to imlement that trait without clippy complaining. Thanks!

IMO this is a perfectly fine use-case of Option::as_deref_mut and a false positive warning from Clippy. You should open an issue. The rule even claims to be a "machine applicable" fix, so it's not supposed to result in compilation failure. IMO, the lint should make sure to only apply when the value is even allowed to be moved, e. g. on the last usage and not in loops. The false positive does by the way only apply to the mutable case, since an immutable Option<&T> is Copy.

Edit: Ah, I missed @H2CO3 pointing out there are existing issues already.

I disagree with the suggested fix of using an additional .map(BorrowMut::borrow_mut). The correct fix is to #![allow(...)] the faulty Clippy lint until it's fixed. Don't bother making your code more ugly, just globally disable that lint.

4 Likes

If you don't like the alternative, or view it as a workaround, then a Clippy bug is a perfect reason to temporarily and locally turn off the lint by slapping #[allow(clippy::needless_option_as_deref)] on the offending expression/function until it's fixed.

2 Likes

Why though? The as_deref_mut() call isn't used to achieve what it was designed for (converting to the target type of Deref), so it's a slight abuse, while borrow_mut() states exactly what the point is (reborrowing).

Pin::as_mut is used to re-borrow Pin<&mut T>, too, even though its signature is more general and supports any &mut Pin<P> with P: DerefMut. That's no abuse. Re-borrowing an Option<&mut T>, i. e. a method, &'a mut Option<&mut T> -> &'a mut T is a really useful operation and it would deserve its own method, however it doesn't need a dedicated method because it's just a special case of Option::as_deref_mut.

For dereferencing, which is a &'a mut P -> &'a mut P::Target operation, it's generally not always “morally” 100% clear whether the inner or the outer pointer is being dereferenced. Especially if it's a &mut &mut T -> &mut T operation. So similarly, one could question which level is the one being dereferenced for &'a mut Option<P> -> Option<&'a mut P::Target>. Especially on a hardware level, the real dereferencing that happens here often is only on the outer &mut. My main point being: there is still something being dereferenced when calling the as_deref_mut method on &mut Option<&mut T> so the name of the method seems appropriate IMO.

What it maybe was or wasn't “designed for” I don't know but also I don't care [1]. Ultimately it's just a shorthand for .as_mut().map(DerefMut::deref_mut), this long form being something that looks a lot like your expression involving BorrowMut. What BorrowMut was designed for is yet another potential side discussion, but let me just say it doesn't scream “re-borrowing” to me either, and AFAIK a main use-case is for writing generic methods like HashMap's or BTreeMap's get_mut method. I'm not sure whether or not that's what it's designed for; if so, then this use-case here would be pretty far away from what BorrowMut was designed for.

Re-borrowing any pointer type explicitly, including &mut T itself, is usually done by dereferencing and creating a new mutable reference, in either order, i. e. &mut *x or *&mut x [2]. The former is often preferred when being explicit, because it's slightly more general, but the latter is also commonly used, especially when the dereferencing step is left implicit, e. g. if you have a x: MutexGuard<T> and a method foo accepting &mut T, then you can write foo(&mut x) relying on deref coercion. Given that re-borrowing is borrowing+dereferrncing, and calling foo.as_deref_mut() on an Option<&mut T> is borrowing (implicitly) + dereferencing, this is the legitimate way to reborrow an Option<&mut T> AFAICT.


  1. though there is an argument to be had whether perhaps the documentation should just be updated to feature an example use case of re-borrowing a Option<&mut T> just to fully settle the debate ↩︎

  2. actually, I double checked and this latter approach doesn't work too well in a context that isn't implicitly re-borrowing, the more proper way to write this is perhaps &mut **&mut x or DerefMut::deref_mut(&mut x) i.e. x.deref_mut() ↩︎

2 Likes

Just in case you don't want to disable the Clippy lint for some reasony, I just found that (&mut opt).as_deref_mut() is another way to circumvent the problem.

I agree with @steffahn but I do think we should state that in the docs like we do in Pin::as_mut()'s docs.

1 Like