Return early if the method is a default trait impl for boxed_local

[MentalAllergen]

fixes #4804
changelog: check for a default trait impl and a corresponding test case

[MentalAllergen]

Is this enough to cover this fix? There might be some more niche cases which I hadn't encountered but which should be tested as well.

[flip1995]

On a more general note: Why do you take a box instead of just a reference?

trait Trait {
    fn foo(&self) {}
}

should also work.

[flip1995]

I don't know, why I always forget about this: #4351 (comment)

The fix is not to allow this in traits or allow the lint for unsized types, but to add a suggestion that this should be written as &T instead of Box<T>.

Are you willing to add such a suggestion in this PR or should this just be tracked in an issue (#4740) until someone else decides to add a suggestion?

It would definitely be great if you could add to the lint documentation, that this should be rewritten to &T

[MentalAllergen]

On a more general note: Why do you take a box instead of just a reference?

trait Trait {
    fn foo(&self) {}
}

should also work.

The case which led me to open an issue was something akin to the following:

trait Trait {
    fn foo(self: Box<Self>) -> Result<Box<dyn Trait>, ()> {
        Err(())
    }
}

struct Foo {
    // some data
}

impl Trait for Foo {
    fn foo(self: Box<Self>) -> Result<Box<dyn Trait>, ()> {
        // actually do something with "self"
    }
}

where my intention was to consume the value and receive a new boxed trait object.

Due to having a non negligible count of structs which needed to impl Trait and Trait itself having about 7 or 8 methods of the same return type, having the default trait impl return an Err variant reduced code duplication. Upon encountering an error, the consumed boxed trait object was not necessary for error handling so letting it drop was fine.

In that situation, having this lint suggest using a reference instead of a boxed trait object would still not align with my intentions.

I've later managed to refactor all of that code so it doesn't use dynamic dispatch at all.

[MentalAllergen]

I am willing to give this a try and change the lint documentation to suggest changing to a &T.
I'm fairly new to the clippy code base so any help is appreciated.

This can be implemented to suggest changing self: Box<Self> only for unsized types while keeping the old message if the type is sized. Should we prefer that to always suggesting a reference?

[flip1995]

Ah I see now. I think the best way to do this would be to disable this lint for trait definitions, but only for unsized arguments. Diabling it for trait definitions completely will lead to too many FNs.

[flip1995]

I am willing to give this a try and change the lint documentation to suggest changing to a &T.

Adding this to the documentation is as easy as adding a bad+good example here

rust-clippy/clippy_lints/src/escape.rs

Lines 29 to 36 in b4f1769

	/// **Example:**
	/// ```rust
	/// # fn foo(bar: usize) {}
	/// let x = Box::new(1);
	/// foo(*x);
	/// println!("{}", *x);
	/// ```
	pub BOXED_LOCAL,

Should we prefer that to always suggesting a reference?

Yes the suggestion should be to just remove the box, if the type is sized and change it to &T for unsized types.

[flip1995]

Oh sorry, I started the review, but never pressed "Submit review" 😄

We have to look at every argument of default trait impls, not only self. But only for arguments that are sized. See is_sized for the documentation.

[flip1995]

That will test-fail. Please leave the foo(*x); and rename this to bar

[MentalAllergen]

Ok

[MentalAllergen]

Oh sorry, I started the review, but never pressed "Submit review" 😄

We have to look at every argument of default trait impls, not only self. But only for arguments that are sized. See is_sized for the documentation.

Thanks for the linked docs, I'll get to changing the code in a few days as Uni is heating up at the moment. Should've realized sooner that skipping linting the whole method will not suffice.

[bors]

☔ The latest upstream changes (presumably #4938) made this pull request unmergeable. Please resolve the merge conflicts.

[flip1995]

Thanks for contributing to Clippy! Sadly this PR was not updated in quite some time. If you waited on input from a reviewer, we're sorry that this fell under the radar. If you want to continue to work on this, just reopen the PR and/or ping a reviewer.

[HKalbasi]

finished in #6571
@rustbot label -S-inactive-closed