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

Too much path qualification in recommended fix #7220

Closed
kinnison opened this issue May 13, 2021 · 4 comments · Fixed by #7493
Closed

Too much path qualification in recommended fix #7220

kinnison opened this issue May 13, 2021 · 4 comments · Fixed by #7493
Assignees
Labels
C-bug Category: Clippy is not doing the correct thing

Comments

@kinnison
Copy link
Contributor

kinnison commented May 13, 2021

The new_without_default lint (indirectly) points at an impl block containing the new() method and suggests something to insert. However because the name is "fully qualified" if you insert the suggested impl Default alongside the impl block, it will not compile.

I tried this code:

pub mod submod {
    pub struct Foo<T> {
        _bar: *mut T,
    }

    impl<T> Foo<T> {
        pub fn new() -> Self {
            todo!()
        }
    }
}

I expected to see this happen:

warning: you should consider adding a `Default` implementation for `submod::Foo<T>`
 --> src/lib.rs:7:9
  |
7 | /         pub fn new() -> Self {
8 | |             todo!()
9 | |         }
  | |_________^
  |
  = note: `#[warn(clippy::new_without_default)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#new_without_default
help: try this
  |
6 |     impl<T> Default for Foo<T> {
7 |         fn default() -> Self {
8 |             Self::new()
9 |         }
10|     }
11| 
...

Instead, this happened (specifically note the qualified submod::Foo in the suggested impl):

warning: you should consider adding a `Default` implementation for `submod::Foo<T>`
 --> src/lib.rs:7:9
  |
7 | /         pub fn new() -> Self {
8 | |             todo!()
9 | |         }
  | |_________^
  |
  = note: `#[warn(clippy::new_without_default)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#new_without_default
help: try this
  |
6 |     impl<T> Default for submod::Foo<T> {
7 |         fn default() -> Self {
8 |             Self::new()
9 |         }
10|     }
11| 
...

Meta

  • cargo clippy -V: clippy 0.1.54 (5c02926 2021-05-11)
  • rustc -Vv:

rustc 1.54.0-nightly (5c02926 2021-05-11)
binary: rustc
commit-hash: 5c02926
commit-date: 2021-05-11
host: x86_64-unknown-linux-gnu
release: 1.54.0-nightly
LLVM version: 12.0.1




<!-- TRIAGEBOT_START -->

<!-- TRIAGEBOT_ASSIGN_START -->

<!-- TRIAGEBOT_ASSIGN_DATA_START$${"user":"xFrednet"}$$TRIAGEBOT_ASSIGN_DATA_END -->

<!-- TRIAGEBOT_ASSIGN_END -->
<!-- TRIAGEBOT_END -->
@kinnison kinnison added the C-bug Category: Clippy is not doing the correct thing label May 13, 2021
@xFrednet
Copy link
Member

Most likely related to #6385

@mvforell
Copy link

Stumbled into this today as well. As pointed out by @estebank in this Twitter thread, this is bad because rustc does not suggest a fix for the incorrect path qualification ("I'm ok if you get a suggestion that gets you another suggestion that fixes your code, but if it leads to a dead end I'm not ok with it.").

Not sure if I should open a separate issue at for rustc (it probably already exists, haven't looked extensively).

@xFrednet
Copy link
Member

The issue is most likely a result of code in Clippy. The code for new_without_default calls format!(".. {} ..", ..) on the type, and this is probably the reason why the complete path is prefixed.

A possible solution could be to snip the type part from the original new impl block. Meaning this:

impl<T> Foo<T> {
//      ^^^^^^
    pub fn new() -> Self {
        todo!()
    }
}

I might look into this over the next week 🙃. In any case, I think this should not get an issue in the rustc repo.

@xFrednet
Copy link
Member

@rustbot claim

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

Successfully merging a pull request may close this issue.

3 participants