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

False positive for boxed local in default Trait method implementation #4804

Closed
MentalAllergen opened this issue Nov 11, 2019 · 4 comments · Fixed by #6571
Closed

False positive for boxed local in default Trait method implementation #4804

MentalAllergen opened this issue Nov 11, 2019 · 4 comments · Fixed by #6571
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied

Comments

@MentalAllergen
Copy link

Output of cargo clippy -V: clippy 0.0.212 (3aea8603 2019-09-03)

Given the following code:

trait Foo {
    fn bar(self: Box<Self>) -> u32 {
        5
    }
}

Clippy lint boxed local is triggered, advising us to remove the Box and have plain self in the argument list, like this:

trait Foo {
    fn bar(self) -> u32 {
        5
    }
}

However, doing this makes the code fail compilation with the following error:
the size for values of type "Self" cannot be known at compilation time,
which is always the case for traits.

I've encountered this while implementing the state design pattern with dynamic dispatch. I wouldn't usually opt for dynamic dispatch, however the problem domain requires it.

Ideally this lint should check weather the implementation is a default trait method implementation before triggering.

@MentalAllergen
Copy link
Author

Looking at the implementation, I should be able to make a fix for this. Will give it a try and report here. How would I test the changes locally?

@flip1995
Copy link
Member

flip1995 commented Nov 19, 2019

Hey @MentalAllergen, sorry for the late reply. I think every Clippy maintainer is currently pretty busy.

How the testing in Clippy works is described here: https://github.com/rust-lang/rust-clippy/blob/master/doc/adding_lints.md#testing

TL;DR: Add a test case for this in the corresponding test file in tests/ui/ and then test it with TESTNAME=boxed_local cargo +master uitest (I'm not sure if the corresponding test file is really called boxed_local.rs). You can install the master toolchain with the setup-toolchain.sh script in the project root.

@flip1995 flip1995 added the C-bug Category: Clippy is not doing the correct thing label Nov 19, 2019
@MentalAllergen
Copy link
Author

Thanks for the explanation @flip1995!
PR is up.

I still couldn't test locally as cargo +master build failed for the master branch of clippy. It seems to be a problem with a change from rust-lang itself so until that is resolved, testing will not be possible.

@benmkw
Copy link

benmkw commented Nov 19, 2020

I think this is another/ the same case where this lint may not be accurate https://github.com/probe-rs/probe-rs/pull/437/files#file-probe-rs-src-probe-mod-rs-L459. I tried to apply the suggestion at some point but I'm pretty convinced that its not possible.

@phansch phansch added the I-false-positive Issue: The lint was triggered on code it shouldn't have label Dec 18, 2020
@phansch phansch added the I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied label Jan 9, 2021
@bors bors closed this as completed in ee0598e Jan 9, 2021
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 I-false-positive Issue: The lint was triggered on code it shouldn't have I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied
Projects
None yet
4 participants