Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clippy incorrectly complains about borrowed_box when trait object is boxed #3971

Closed
idubrov opened this issue Apr 15, 2019 · 9 comments
Closed

Comments

@idubrov
Copy link

idubrov commented Apr 15, 2019

The following code triggers clippy::borrowed_box:

trait Whatever {}
struct Something;
impl Whatever for Something {}
fn func(flag: bool, secret: &mut Box<Whatever>) {
    *secret = Box::new(Something);
}

However, I don't think it should be complaining here since Whatever is not Sized (so I cannot overwrite it directly)

I think, this issue (or similar) was raised before: #1884 (also mentioned in #3128)

The fix for #1884 was, however, just to check for Any trait as a special case.

Playground

@flip1995
Copy link
Member

borrowed_box doesn't trigger on the code anymore.

@MarcelCoding
Copy link

@flip1995
Copy link
Member

What prevents you from just using &dyn Whatever in this example? According to the lint documentation this is exactly the case this lint was made for.

@MarcelCoding
Copy link

MarcelCoding commented Dec 23, 2021

Whatever is a trait and at the point without a reference I have to box the trait. (not in the example, I could add it )

@xanathar
Copy link
Contributor

xanathar commented Dec 23, 2021

Can you do like this_is_fine in this example?

trait Whatever {}
struct Something;
impl Whatever for Something {}

fn this_is_fine(_: &dyn Whatever) { }

fn this_is_also_fine_but_moves(_: Box<dyn Whatever>) { }

fn this_triggers_clippy(_: &Box<dyn Whatever>) { }


fn main() {
    let wt: Box<dyn Whatever> = Box::new(Something);

    this_triggers_clippy(&wt);
    this_is_fine(&*wt);
    this_is_also_fine_but_moves(wt);
}

@flip1995
Copy link
Member

Alternatively this_is_fine(wt.as_ref()). The advantage of having it as just &dyn _ is that if you have a concrete type that you want to pass to the function, you don't have to box it. So with this_is_fine you can just call this_is_fine(&Something). But with this_triggers_clippy you have to box Something first: this_triggers_clippy(&Box::new(Something)). Playground

@MarcelCoding
Copy link

Thanks, with the help of your example I got it working. I think it would be helpful for others to include this conversation in the message provided by clippy.

@flip1995
Copy link
Member

Hard to include all of this in the message, but we should improve the documentation a bit.

@MarcelCoding
Copy link

Ether that, I rather thought about a link to this conversation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants