`borrowed_box` warnings on functions that returns `U<&V>` where `V` happens to be `Box<T>`

[OxidizedLoop]

Summary

As per the code bellow HashMap.get() returns type Option<&V>, V just happens to be Box<i32> leading to the return type of SuperHashMapsh_get() being Option<&Box<i32>>. We then get the borrowed_box warning on the resulting &Box<T>.

This happens not just with Option<&V> but to any function that the return value is a call to a different function that returns U<&V> where V is a Box<T>.

I would suggest for the lint not to trigger when &Box<T> is the result of a functioning returning U<&V> where V is Box<T> as its clear that the problem is not created by mistakenly creating a &Box<T>. Changing it to &T is not the correct solution as it would involve destroying U, changing &Box<T> to a &T, and creating a new U to hold &T .

I assume this could be fixed by checking if the return value of a function like sh_get() is the result of a function like get() that returns U<&V> and not give a warning in this situation.

Lint Name

clippy::borrowed_box

Reproducer

playground link

I tried this code: (I'm only using Box<i32> for the MRE.)

use std::{collections::HashMap, option::Option};
pub struct SuperHashmap {
    hashmap: HashMap<String, Box<i32>>,
}
impl SuperHashMap {
    pub fn sh_get(&self, key: String) -> Option<&Box<i32>> {
        self.hashmap.get(&key)
    }
}

I saw this happen:

warning: you seem to be trying to use `&Box<T>`. Consider using just `&T`
 --> src/lib.rs:6:49
  |
6 |     pub fn sh_get(&self, key: String) -> Option<&Box<i32>> {
  |                                                 ^^^^^^^^^ help: try: `&i32`
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#borrowed_box
  = note: `#[warn(clippy::borrowed_box)]` on by default

I expected to see this happen:

No warning from clippy

Version

rustc 1.76.0-nightly (6b771f6b5 2023-11-15)
binary: rustc
commit-hash: 6b771f6b5a6c8b03b6322a9c77ac77cb346148f0
commit-date: 2023-11-15
host: x86_64-pc-windows-msvc
release: 1.76.0-nightly
LLVM version: 17.0.5

Also occurs in stable as seen in the playground link above.

Additional Labels

@rustbot label +T-middle

[Jarcho]

Option<&T> would still be more convenient for the caller and can be gotten via as_deref.

[OxidizedLoop]

Option<&T> ... can be gotten via as_deref.

Could you provide code or a playground link on how you would use as_deref in the example above?
It's the first time I have seen as_deref so I might be using it wrong, but it doesn't seem to be doing anything as calling as_deref on Option<&Box<i32>> still gives you Option<&Box<i32>>.
And I'm not even managing to make that compile as just using as_deref gets me "error[E0515]: cannot return value referencing temporary value"

But yes its possible to change it to Option<&T> by doing the following:

pub fn sh_get(& self, key: String) ->  Option<&i32> {
        match self.hashmap.get(&key){
            Some(x) =>{Some(&**x)}
            None =>{None}
        }
    }

This is possible for Option<&Box<T>> but might not be the case for some other U<&Box<T>> and I don't agree with the following:

Option<&T> would still be more convenient for the caller ...

If the developer is creating a function that is calling another function that returns U<&V> and V happens to be Box<T> its probably by design.
In my case I'm upcasting Box<dyn Trait> to Box<dyn Any> to be able to downcast to T: Trait , so there is no point in removing the Box if later I'm gonna have to create another.

(This might be bad design and &dyn Trait should be used instead of Box<dyn Trait> but then this should be a different lint and not borrowed_box(and in my case I used Box because I needed to use it later anyway and I didn't want to deal with explicit lifetimes).)

For borrowed_box anyone who gets in the situation of having to return a U<&V> where V just happens to be Box<T> is likely doing it intentionally so they shouldn't be getting this warning.
But this is my assumption that this would be intentional, if there is an example of how someone could get in this situation by accident, where It's clearly wrong then borrowed_box should stay as it is. But without that example I believe its purely a false positive and should be fixed.

[Jarcho]

You're right. as_deref doesn't work here, it would need to be map(|x| &**x) or the match.

Aside from making types line up, there's nothing &Box<T> can do that &T can't do (without unsafe code). You can end up needing it due to external code that you don't control, or when writing trait impls. Ideally these would be detected, but that's not trivial to do in most cases.

If the developer is creating a function that is calling another function that returns U<&V> and V happens to be Box its probably by design.

This reasoning equally applies to functions returning &V. And yes a function returning a reference to a generic type is doing so on purpose. I'm going to assume that isn't the exact point you're making here since that on it's own doesn't affect the lint.

As far as I can make out, you're arguing for the lint not warning for return values. None of your reasoning applies to U<&T>, but not to &T. Note this is not to say the change won't happen, this is just to be sure exactly what you're arguing for.

[OxidizedLoop]

As far as I can make out, you're arguing for the lint not warning for return values.

Sorry for not explaining myself clearly, I'm arguing for the lint not to apply to a subset of return values, should still apply in other situations that aren't that subset.

This reasoning equally applies to functions returning &V

None of your reasoning applies to U<&T>, but not to &T.

My reasoning would be that for &Box<T> its trivial to convert it to &T, but for some U<&Box> it might not be.
But I see how that might not be a strong enough reason to remove this subsets of cases, but my point is also that in this subset of cases its not as trivial as simply &** a &Box<T> , and its likely also by design. If it is good and intentional design then it's fine, but it's bad design then there should be another lint pointing at the root cause of the problem.

And the subset mentioned above would be specific cases where its clear that some other function was called and we return its value/type without modifying it.
To give a better example of what I would expect the correct behaviour to be:

use std::{collections::HashMap, option::Option};
pub struct LiarHashMap {
    liar1: bool,
    liar2: bool,
    hashmap: HashMap<String, Box<i32>>,
    fake_number: Box<i32>,
    lucky_number: Box<i32>,
}
impl LiarHashMap {
    pub fn get1(&self, key: String) -> Option<&Box<i32>> { //lint should NOT warn here
        if self.liar1{
            return None
        }
        self.hashmap.get(&key)//because we got the return value/type from calling another function without changing it 
    }
    pub fn get2(&self, key: String) -> Option<&Box<i32>> {//lint should NOT warn here
        if self.liar1{
            return None
        } else if self.liar2{
            return Some(&self.fake_number)//even if we created this       (but lint should warn here)
        }
        self.hashmap.get(&key)//because we also get the return value/type from here
    }
    pub fn get_lucky_number(&self) -> Option<&Box<i32>> {//lint SHOULD warn here
        if self.liar1{
            return None
        } else if self.liar2{
            return Some(&self.fake_number)//because here and           (and warn here)
        }
        Some(&self.lucky_number)//here we are creating a Option<&Box<i32>> when we had the control to change it to Option<&i32> 
        //(and also warn here^)
    }
}

You can end up needing it due to external code that you don't control, or when writing trait impls.

I think that's part of my point that I didn't make clear. I don't want there to be a lint when I'm creating a function that ends up just returning what I get when I call HashMap::get() because I can't control the output of it. If the HashMap::get() or some other external code is outputting a U<&Box<T>> then its because it's supposed to and I'm just propagating its type.

And in the situation where I'm the one that is creating the problematic HashMap<K, Box<T>> then I should be getting a different lint, warning me that maybe I should use &T instead because by looking at HashMap's methods it's clear some of them would end up returning &Box<T> and Option<&Box<T>> .

Ideally these would be detected, but that's not trivial to do in most cases.

I think at least that the high level reasoning is simple: if at least one of the return statements of a function is a call to a function that returns U<&T> don't give a warning. (but this could potentially cause some false negatives)(And I realize that high level logic might not translate well into the implementation)

For the other lint (suggesting HashMap<K, &T> instead of HashMap<K, Box<T>> ) the high level reasoning would be to check if HashMap<K , V> has any methods that return &V or U<&V> and. But of course, generalized not only for HashMap. But I assume if this were to be another lint another issue should be made it for it to be discussed.

[QuineDot]

Aside from making types line up, there's nothing &Box can do that &T can't do (without unsafe code).

That's simply not true. For some examples from std,

• Box<str>, Box<[T /* : Clone */]>, and similar implement Clone, but the (unsized) boxed types do not (and cannot)
• Box<T>: AsRef<T> for all T, but there is no blanket T: AsRef<T>
  • And if T isn't your type, you can't make it do so

More generally, Box<_> is a fundamental type, so someone can make Box<YourType> do whatever you want, like have a &self method YourType does not -- or cannot, like implementing Clone for Box<dyn YourTrait>. No unsafe code required.