Skip to content

False positive for boxed_local with trait default methods #3739

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

Closed
dmarcuse opened this issue Feb 4, 2019 · 5 comments
Closed

False positive for boxed_local with trait default methods #3739

dmarcuse opened this issue Feb 4, 2019 · 5 comments
Labels
C-bug Category: Clippy is not doing the correct thing

Comments

@dmarcuse
Copy link

dmarcuse commented Feb 4, 2019

When defining a trait that has a method accepting a self: Box<Self> which has a default implementation on the trait itself, the boxed_local lint is triggered. This was fixed for impls in #3321, but still triggers for traits.

Clippy version: clippy 0.0.212 (b2601be 2018-11-27)

Example Code
trait ExampleTrait {
    fn method(self: Box<Self>) { // boxed_local lint triggered here
        unimplemented!()
    }       
}

struct SomeObject;

impl ExampleTrait for SomeObject {}

#[allow(unused_variables)]
fn main() {
    let trait_object: Box<dyn ExampleTrait> = Box::new(SomeObject {});
    trait_object.method();
}
clippy output
warning: local variable doesn't need to be boxed here
 --> src/main.rs:2:15
  |
2 |     fn method(self: Box<Self>) {
  |               ^^^^
  |
  = note: #[warn(clippy::boxed_local)] on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#boxed_local
@phansch phansch added the C-bug Category: Clippy is not doing the correct thing label Feb 6, 2019
@ghost
Copy link

ghost commented Feb 15, 2019

method(self: Box<Self>) can't be replaced with method(self) because the Self isn't sized, but couldn't you change it to method(&self) or method(&mut self) instead?

@dmarcuse
Copy link
Author

Not for this particular case. The method spawns a thread which takes ownership of self, so using a reference wouldn't work as the thread's reference would outlive the object.

@ghost
Copy link

ghost commented Feb 17, 2019

The lint is supposed to check if the box is moved inside the body and not warn in that case. In the example below method2 will get a warning but method1 won't.

#![warn(clippy::boxed_local)]

use std::mem;

trait ExampleTrait {
    fn method1(self: Box<Self>) { // boxed_local lint is _not_ triggered here
        mem::drop(self)
    }       
    fn method2(self: Box<Self>) { // boxed_local lint triggered here
        unimplemented!();
    }       
}

fn main() {}

Playground

Can you post more of your code?

@dmarcuse
Copy link
Author

It seems to not detect the value being moved when it's implicitly moved into a closure (e.g. to spawn a thread)

Playground

@ghost ghost mentioned this issue Feb 21, 2019
bors added a commit that referenced this issue Mar 12, 2019
Fix `boxed_local` suggestion

Don't warn about an argument that is moved into a closure.

ExprUseVisitor doesn't walk into nested bodies so use a new
visitor that collects the variables that are moved into closures.

Fixes #3739
@dmarcuse
Copy link
Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing
Projects
None yet
Development

No branches or pull requests

2 participants