borrowed_box should not be emitted for mutable boxed trait object

[antoyo]

Hello.
The borrowed_box suggestion can be a false positive for mutable boxed trait object, because it seems there's no way to apply the suggestion (it only works for immutable boxed trait object).

For example, the following code triggers this warning:

fn get_mut<'a>(map: &'a mut HashMap<String, Box<Display>>, key: &str) -> Option<&'a mut Box<Display>> {
    map.get_mut(key)
}

But if I try to fix it by:

fn get_mut<'a>(map: &'a mut HashMap<String, Box<Display>>, key: &str) -> Option<&'a mut Display> {
    map.get_mut(key)
        .map(AsMut::as_mut)
}

I get a compilation error:

error[E0308]: mismatched types
  --> src/main.rs:12:5
   |
12 | /     map.get_mut(key)
13 | |         .map(AsMut::as_mut)
   | |___________________________^ lifetime mismatch
   |
   = note: expected type `std::option::Option<&'a mut std::fmt::Display + 'a>`
              found type `std::option::Option<&mut std::fmt::Display + 'static>`
note: the lifetime 'a as defined on the function body at 11:1...
  --> src/main.rs:11:1
   |
11 | / fn get_mut<'a>(map: &'a mut HashMap<String, Box<Display>>, key: &str) -> Option<&'a mut Display> {
12 | |     map.get_mut(key)
13 | |         .map(AsMut::as_mut)
14 | | }
   | |_^
   = note: ...does not necessarily outlive the static lifetime

(To be honest, I don't understand why I get this error for returning a mutable reference, but not for an immutable reference.)

Thanks to remove the warning in this case.

[Manishearth]

I think you need to do something like map.get_mut(key).as_mut_ref().map(...)

[Manishearth]

And probably use an explicit map of |x| &mut **x instead of relying on asmut

[antoyo]

The following trigger the same error:

fn get_mut<'a>(map: &'a mut HashMap<String, Box<Display>>, key: &str) -> Option<&'a mut Display> {
    map.get_mut(key).as_mut()
        .map(|x| &mut ***x)
}

I found out the following works, but I don't think it is the same:

fn get_mut(map: &'static mut HashMap<String, Box<Display>>, key: &str) -> Option<&'static mut Display> {
    map.get_mut(key)
        .map(AsMut::as_mut)
}

[Manishearth]

fn get_mut<'a>(map: &'a mut HashMap<String, Box<Display + 'static >>, key: &str) -> Option<&'a mut (Display + 'static)> {
    map.get_mut(key).map(|x| &mut **x)
}

[antoyo]

Again, this has changed the requirements of the function.
When an immutable reference is needed, we can apply clippy's suggestion:

fn get<'a>(map: &'a HashMap<String, Box<Display>>, key: &str) -> Option<&'a Box<Display>> {
    map.get(key)
}

to

fn get<'a>(map: &'a HashMap<String, Box<Display>>, key: &str) -> Option<&'a Display> {
    map.get(key)
        .map(AsRef::as_ref)
}

but not when the reference is mutable.
How do you explain that?

[Manishearth]

Variance. And iirc in the box case there was an implicit static bound.

[oli-obk]

Indeed, @Manishearth's suggestion is the correct version. Box<Trait> in general is Box<Trait + 'static>, while &'a Trait is &'a Trait + 'a. We should probably document this "suggestion false positive" in the lint docs or figure out a way to improve the suggestion where needed.

[rail-rain]

FYI, this problem doesn't occur at the moment as the lint ignores every mutable references (#5491). However, if someone implements the enhancement suggested here (#5491 (comment)), it will reappear.