Skip to content

False positive with use_self and generics #3410

Closed
@daboross

Description

@daboross

As far as I can tell, the use_self is incorrectly ignoring generic parameters when implementing traits for some types with generics.

Here's my example:

pub trait PhaseTransform<T>: Sized {
    fn transform_from(v: T) -> Self;
}

pub struct IntermediatePhase;

pub struct FinalPhase;

impl PhaseTransform<Vec<IntermediatePhase>> for Vec<FinalPhase> {
    fn transform_from(_: Vec<IntermediatePhase>) -> Self {
        unimplemented!()
    }
}

impl<T> PhaseTransform<Vec<IntermediatePhase>> for Vec<T>
where
    T: PhaseTransform<FinalPhase>,
{
    fn transform_from(intermediates: Vec<IntermediatePhase>) -> Self {
        <Vec<FinalPhase>>::transform_from(intermediates)
            .into_iter()
            .map(PhaseTransform::transform_from)
            .collect()
    }
}

This, as far as I can tell, uses Self as much as possible. But with -W clippy::pedantic, clippy warns of three more places where it thinks I should be able to use Self:

$ cargo clippy -- -W clippy::pedantic
    Checking clippy-false-positive-test v0.1.0 (/home/daboross/clippy-false-positive-test)                                              
warning: unnecessary structure name repetition                                                                                          
  --> src/lib.rs:10:26                                                                                                                  
   |                                                                                                                                    
10 |     fn transform_from(_: Vec<IntermediatePhase>) -> Self {                                                                         
   |                          ^^^^^^^^^^^^^^^^^^^^^^ help: use the applicable keyword: `Self`                                           
   |                                                                                                                                    
   = note: `-W clippy::use-self` implied by `-W clippy::pedantic`                                                                       
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#use_self                   
                                                                                                                                        
warning: unnecessary structure name repetition                                                                                          
  --> src/lib.rs:19:38                                                                                                                  
   |                                                                                                                                    
19 |     fn transform_from(intermediates: Vec<IntermediatePhase>) -> Self {                                                             
   |                                      ^^^^^^^^^^^^^^^^^^^^^^ help: use the applicable keyword: `Self`                               
   |                                                                                                                                    
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#use_self                   
                                                                                                                                        
warning: unnecessary structure name repetition                                                                                          
  --> src/lib.rs:20:10                                                                                                                  
   |                                                                                                                                    
20 |         <Vec<FinalPhase>>::transform_from(intermediates)                                                                           
   |          ^^^^^^^^^^^^^^^ help: use the applicable keyword: `Self`                                                                  
   |                                                                                                                                    
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#use_self                   
                                                                                                                                        
    Finished dev [unoptimized + debuginfo] target(s) in 0.21s

There are two mistakes: it's treating Vec<IntermediatePhase> as Vec<FinalPhase>, and it's treating Vec<FinalPhase> as Vec<T>.

Hope the example is minimal enough - my original trigger was with a custom trait like this, and it seemed hard to come up with any other reasonable use case which caused this situation.

Clippy version:

$ cargo clippy -V                    
clippy 0.0.212 (b1d0343 2018-10-19)

Related to #1993.

Activity

added
C-bugCategory: Clippy is not doing the correct thing
on Nov 4, 2018
ghost

ghost commented on Nov 7, 2018

@ghost

Here is a another example that doesn't involve traits.

#![warn(clippy::use_self)]
#![allow(dead_code)]

struct S<T> {
    t: T
}

impl S<u64> {
    fn foo(_: S<u32>) {
        let _: S<u32>;
    }
}
warning: unnecessary structure name repetition
 --> src/main.rs:9:15
  |
9 |     fn foo(_: S<u32>) {
  |               ^^^^^^ help: use the applicable keyword: `Self`
  |
note: lint level defined here
 --> src/main.rs:1:9
  |
1 | #![warn(clippy::use_self)]
  |         ^^^^^^^^^^^^^^^^
  = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#use_self

warning: unnecessary structure name repetition
  --> src/main.rs:10:16
   |
10 |         let _: S<u32>;
   |                ^^^^^^ help: use the applicable keyword: `Self`
   |
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#use_self

Playground

The problem is that the code checks if two paths refer to the same type by comparing the defs but this doesn't account for generics.

https://github.com/rust-lang-nursery/rust-clippy/blob/4c3408c61d1042bf0585de440041ee7edfc5b350/clippy_lints/src/use_self.rs#L221

I couldn't find a way to get the actual type including type parameters for a general path. Maybe someone has a suggestion.

I can see how to handle this for types in function signatures though. I'll submit a PR for that later.

added a commit that references this issue on Nov 10, 2018
zroug

zroug commented on Nov 13, 2018

@zroug

Not involving generics but another false positive:

#![warn(clippy::use_self)]
#![allow(dead_code)]

#[derive(Clone)]
struct S;

impl S {
    fn foo() {
        impl Copy for S {}
    }
}

fn main() {}

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=b33a49bdb926b36be6c2fbad5defe205

warning: unnecessary structure name repetition
 --> src/main.rs:9:23
  |
9 |         impl Copy for S {}
  |                       ^ help: use the applicable keyword: `Self`
  |

With generics, the error also happens with associated types:

#![warn(clippy::use_self)]
#![allow(dead_code)]

struct S<T1, T2> {
    t1: T1,
    t2: T2,
}

impl<T1: Copy, T2: Copy> Iterator for S<T1, T2> {
    type Item = S<T2, T1>;
    fn next(&mut self) -> Option<Self::Item> {
        Some(S {
            t1: self.t2,
            t2: self.t1,
        })
    }
}

fn main() {}

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=42f001263b6139a9333c08290ccd188c

warning: unnecessary structure name repetition
  --> src/main.rs:10:17
   |
10 |     type Item = S<T2, T1>;
   |                 ^^^^^^^^^ help: use the applicable keyword: `Self`
   |

warning: unnecessary structure name repetition
  --> src/main.rs:12:14
   |
12 |         Some(S {
   |              ^ help: use the applicable keyword: `Self`
   |

(Both warnings are wrong.)

zroug

zroug commented on Nov 13, 2018

@zroug

I couldn't find a way to get the actual type including type parameters for a general path. Maybe someone has a suggestion.

I don't know how to do that but there is a pull request #1997. There was no activity for a long time but there is a bit of discussion.

eddyb

eddyb commented on Apr 7, 2020

@eddyb
Member

IMO clippy should just use hir_ty_to_ty until a query alternative is provided.
This should be considered an essential operation and clippy should make use of it as much as possible.

It's unnecessary to compare hir::Tys in intricate manners when you can just get a Ty and == it.

added a commit that references this issue on Apr 20, 2020

rust-lang#3410: add missing false positive case in uitests

tnielens

tnielens commented on Apr 23, 2020

@tnielens
Contributor

I started working on this based on the recommendation and example committed by @eddyb .

added a commit that references this issue on Apr 23, 2020

rust-lang#3410: add missing false positive case in uitests

4 remaining items

added a commit that references this issue on Oct 13, 2020

Auto merge of #5531 - montrivo:bugfix/3410-use_self_generic_false_pos…

added
I-false-positiveIssue: The lint was triggered on code it shouldn't have
on Dec 21, 2020
added a commit that references this issue on Jan 19, 2021

Auto merge of #6179 - flip1995:rewrite_use_self, r=phansch

kawogi

kawogi commented on Jan 26, 2021

@kawogi

I think this is the same problem in a slightly different shape:

#![warn(clippy::use_self)]

pub trait Trait<T> {
    fn my_fn(_: T);
}

impl<T> Trait<T> for bool {
    fn my_fn(_: T) {
        println!("clippy: unnecessary structure name repetition");
        let _positive: bool = false;
    }
}

The false positive gets triggered twice: once somewhere in the macro (match by accident) and once in the declaration.

CAD97

CAD97 commented on Jan 26, 2021

@CAD97
Contributor

I think that one's different. You could say let positive: Self = false, but that's also probably not better. This lint should probably not fire for primitive types.

E.g. even in the body of an impl for !, it's probably better to refer to ! than Self. For uN you could make a case to use Self (using the same impl for multiple uN), but primitives are much more likely to be mentioned when it doesn't matter that they're Self, just that they're the primitive type.

added 2 commits that reference this issue on Feb 10, 2021

Auto merge of #6179 - flip1995:rewrite_use_self, r=<try>

Auto merge of #6179 - flip1995:rewrite_use_self, r=phansch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    C-bugCategory: Clippy is not doing the correct thingI-false-positiveIssue: The lint was triggered on code it shouldn't have

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Participants

      @eddyb@daboross@phansch@CAD97@flip1995

      Issue actions

        False positive with use_self and generics · Issue #3410 · rust-lang/rust-clippy