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

box-default wrongly suggest on dyn trait #9621

Closed
flisky opened this issue Oct 10, 2022 · 7 comments · Fixed by #9622
Closed

box-default wrongly suggest on dyn trait #9621

flisky opened this issue Oct 10, 2022 · 7 comments · Fixed by #9622
Assignees
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

Comments

@flisky
Copy link

flisky commented Oct 10, 2022

Summary

I have a function signature like fn with_bar(self, bar: Box<dyn Bar>), and box-default will suggest Foo::new().with_bar(Box::default()) over Foo::new().with_bar(Box::new(BarImpl::default)), which clearly cannot be compiled.

Lint Name

box-default

Reproducer

No response

Version

rustc 1.66.0-nightly (8796e7a9c 2022-10-08)
binary: rustc
commit-hash: 8796e7a9cfd4c5c4f1de15ec1c53994ddf288665
commit-date: 2022-10-08
host: aarch64-apple-darwin
release: 1.66.0-nightly
LLVM version: 15.0.2

Additional Labels

No response

@flisky flisky added C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have labels Oct 10, 2022
@llogiq
Copy link
Contributor

llogiq commented Oct 10, 2022

@rustbot claim

I think it should still work with Box::<BarImpl>::default(). Can you check that it does?

@Vlad-Shcherbina
Copy link
Contributor

Box::<T>::default() works, but Clippy suggests Box::default(), not Box::<T>::default(). The suggestion is incorrect.

Consider this artifical example:

trait MyTrait {}

#[derive(Default)]
struct MyStruct;

impl MyTrait for MyStruct {}

fn take_box_dyn(_: Box<dyn MyTrait>) {}

fn main() {
    let _: Box<dyn MyTrait> = Box::new(MyStruct::default());
    take_box_dyn(Box::new(MyStruct::default()));
}

Clippy says this:

warning: `Box::new(_)` of default value
  --> src\main.rs:11:31
   |
11 |     let _: Box<dyn MyTrait> = Box::new(MyStruct::default());
   |                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<MyStruct>::default()`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#box_default
   = note: `#[warn(clippy::box_default)]` on by default

warning: `Box::new(_)` of default value
  --> src\main.rs:12:18
   |
12 |     take_box_dyn(Box::new(MyStruct::default()));
   |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::default()`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#box_default

So, for a variable of type Box<dyn Trait> it correctly suggests Box::<T>::default(), but for a function expecting Box<dyn Trait> it incorrectly and inconsistently suggests Box::default().

> cargo clippy --version
clippy 0.1.68 (0468a00a 2022-12-17)

@rekby
Copy link

rekby commented Dec 21, 2022

I have same problem: box to dyn trait and clippy suggest variant with Box::default(), which can't compiled.

@Eugeny
Copy link

Eugeny commented Jan 11, 2023

Same on the latest nightly - this issue is only partially fixed.

@rwthompsonii
Copy link

I'm having an issue with a crate that provides a builder-pattern "default" function and clippy is complaining about it, even though the object does not (and cannot) implement Default. I'm silencing the warning for now.

@llogiq
Copy link
Contributor

llogiq commented Mar 21, 2023

@rwthompsonii can you provide code?

@rwthompsonii
Copy link

It's closed source, I'll try and get an unencumbered example tomorrow.

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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants