trait bounds lint - repeated types #3766

xd009642 · 2019-02-15T20:43:21Z

This PR is to tackle #3764 it's still a WIP and doesn't work but this is an initial stab. It builds though I haven't added any tests as I'm not sure where lint tests should go?

Unfortunately, it seems id isn't tied to the type itself but I guess where it is in the AST? Looking at https://manishearth.github.io/rust-internals-docs/syntax/ast/struct.Ty.html I can't see any members that would let me tell if a type was repeated in multiple trait bounds.

There may be other issues with how I've implemented this so any assistance is appreciated!

changelog: Add new lint: type_repetition_in_bounds

oli-obk · 2019-02-15T23:15:21Z

Correct, the ID is the id of the AST entry and has nothing to do with identity of the element at hand.

This makes comparing the types significantly harder, because there are also ids inside the TyKind enum in the kind field.

I suggest you create a newtype wrapper around &Ty and insert only those newtype wrappers into the HashSet. You'll need to implement Hash and PartialEq in ways that ignore all ids and spans and essentially only look at the kind field (recursively).

xd009642 · 2019-02-18T20:07:58Z

So I followed the TyKind down until I got to the Ident type which seemed the best fit. I didn't follow all the TyKind variants down as some seemed irrelevant to trait bounds but I might be wrong on some i.e. tuples.

Tested it on a simple example and it seems to work fine so I'm hoping anything else is just fine-tuning and tweaking.

oli-obk · 2019-02-19T10:51:21Z

I didn't follow all the TyKind variants down as some seemed irrelevant to trait bounds but I might be wrong on some i.e. tuples.

All of them are relevant. You can actually have bounds like (T, U): Trait

So instead of collecting the idents, you need to hash all the relevant information. For this you can create a wrapper type

struct HashTy<'ast>(&'ast Ty);

and then implement Hash and PartialEq manually for your type. Instead of inserting a specific part of the Ty, you can insert HashTy(&p.bounded_ty). The Hash and PartialEq impls will then take care of everything.

What I'm imagining is an impl like

impl Hash for HashTy {
    fn hash<H: Hasher>(&self, h: &mut H) {
        // make sure we hash the kind of type, so we can distinguish between
        // `Slice` and `Array`
        std::mem::discrimiant(&self.0.node).hash(h);
        match &self.0.node {
            Slice(p) => HashTy(p).hash(h),
            Tuple(ps) => for p in ps {
                HashTy(p).hash(h);
            },
            ... and so on
        }
    }
}

Something similar for the PartialEq impl.

xd009642 · 2019-02-26T18:17:54Z

So I've started the Hash implementation, I wasn't 100% sure on the Path, TraitObject, ImplTrait hashes and left out other variants like Never which are said to never be used. Also, I haven't gone into implementing hash for Expr which some of the types contain because I wanted to make sure it was necessary before undertaking such a big thing.

I'll start adding some PartialEq implementations just figured I'd get some of my work pushed in case there were any comments before I go too far down the rabbit hole.

flip1995 · 2019-03-09T12:17:42Z

Sorry for the long time no reply @xd009642. The Hash implementation LGTM so far. I would like to have more telling variable names in the hash implementation, but that's just a NIT.

Do you want to continue working on this? Any specific questions or cases you want help with?

xd009642 · 2019-03-09T14:14:56Z

Yeah I'd like to continue working on this. Just one question so far. When I've encountered an Expr I haven't followed it down, however I will need to if I want to spot that [T; 3] != [T;4]. Do I have to follow all the expressions or just literals?

flip1995 · 2019-03-09T17:34:44Z

There's

rust-clippy/clippy_lints/src/utils/hir_utils.rs

Line 65 in 920112d

pub fn eq_expr(&mut self, left: &Expr, right: &Expr) -> bool {

Does this help you with the [T; 3] != [T; 4] case? Otherwise I'll take a closer look at this.

xd009642 · 2019-03-09T17:38:32Z

This looks like it solves my partial_eq implementation, I'll still have to do some work for the hash function unless there's a similar impl for hash.

flip1995 · 2019-03-09T17:43:51Z

Below in this file there is also SpanlessHash, maybe you can use this. I think you should also look at other functions in the hir_utils module, I think you can reuse many of them.

Note, that this is based on HIR items, not on AST items. So in order to use these functions you have to make this lint either a LateLintPass or reimplement them for AST items.

xd009642 · 2019-03-09T17:45:19Z

What's the difference between the late and early pass? If there's reason why I can't make it a late pass I'll do that and reuse those functions.

flip1995 · 2019-03-09T17:49:25Z

Late pass lints are run on the HIR (High-level Intermediate Representation), while Early pass lints are run on the AST (Abstract Syntax Tree). Early pass lints have better performance, than Late pass lints. Also they're run earlier (well 😄). Also Late pass lints have full type information.

So you can just make this a Late pass lint.

xd009642 · 2019-03-23T13:11:54Z

Me again! Sorry for the length of time between tackling this again and again but I think I've got it mostly cracked now.

I had to add hash_ty to the StableHash and there are some parts I just want to double check:

What to hash for each PolyTraitRef in TyKind::TraitObject
What if anything do I need to hash in GenericParam - I'm assuming just the GenericBounds but am unsure.

clippy_lints/src/utils/hir_utils.rs

flip1995 · 2019-03-24T16:52:14Z

The impl LGTM now. Could you add some tests?

xd009642 · 2019-03-24T17:21:38Z

I've added a UI test, let me know if there are any issues 👍

tests/ui/type_repetition_in_bounds.stderr

clippy_lints/src/trait_bounds.rs

xd009642 · 2019-03-24T18:01:37Z

There's currently a bunch of compiler errors in compiletest post rebase i.e.

error[E0432]: unresolved import `test::ColorConfig`
  --> /home/xd009642/.cargo/registry/src/github.com-1ecc6299db9ec823/compiletest_rs-0.3.19/src/common.rs:20:5
   |
20 | use test::ColorConfig;
   |     ^^^^^^^^^^^^^^^^^ no `ColorConfig` in the root

I'm just updating things like rust version etc to see if that resolves it

flip1995 · 2019-03-25T15:39:59Z

There's currently a bunch of compiler errors in compiletest post rebase i.e.

Yeah, this thing goes a little bit deeper. See #3897. I'll ping you when this is fixed.

bors · 2019-04-08T07:45:55Z

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

flip1995 · 2019-04-08T07:48:10Z

ping @xd009642. Clippy master builds again! Do you want to pick this up?

xd009642 · 2019-04-08T15:42:27Z

Sweet I'll rebase tonight and try to finish it off 👍

xd009642 · 2019-04-08T19:02:37Z

So I've still got some build errors and from files I haven't touched? I'm going to update various things and see if I can resolve it, but any thoughts are appreciated if it can speed things along 😄

error[E0433]: failed to resolve: could not find `CtorOf` in `def`
   --> src/utils/mod.rs:872:60
    |
872 |             def::Def::Variant(..) | def::Def::Ctor(_, def::CtorOf::Variant, _)
    |                                                            ^^^^^^ could not find `CtorOf` in `def`

error[E0433]: failed to resolve: could not find `CtorOf` in `def`
   --> src/question_mark.rs:131:47
    |
131 |                 if let Def::Ctor(def_id, def::CtorOf::Variant, _) = cx.tables.qpath_def(qp, expression.hir_id) {
    |                                               ^^^^^^ could not find `CtorOf` in `def`

error[E0433]: failed to resolve: could not find `CtorOf` in `def`
   --> src/use_self.rs:236:52
    |
236 |             } else if let Def::Ctor(ctor_did, def::CtorOf::Struct, CtorKind::Fn) = path.def {
    |                                                    ^^^^^^ could not find `CtorOf` in `def`

error[E0599]: no variant named `Ctor` found for type `rustc::hir::map::DefPathData` in the current scope
   --> src/utils/mod.rs:183:29
    |
183 |         if let DefPathData::Ctor = disambiguated_data.data {
    |                             ^^^^ variant not found in `rustc::hir::map::DefPathData`

error[E0599]: no variant named `Ctor` found for type `rustc::hir::def::Def` in the current scope
   --> src/utils/mod.rs:872:47
    |
872 |             def::Def::Variant(..) | def::Def::Ctor(_, def::CtorOf::Variant, _)
    |                                               ^^^^ variant not found in `rustc::hir::def::Def`

error[E0308]: mismatched types
  --> src/enum_glob_use.rs:45:67
   |
45 |             self.lint_item(cx, map.expect_item(map.hir_to_node_id(item.id)));
   |                                                                   ^^^^^^^ expected struct `rustc::hir::HirId`, found struct `syntax::ast::NodeId`
   |
   = note: expected type `rustc::hir::HirId`
              found type `syntax::ast::NodeId`

error[E0308]: mismatched types
   --> src/lifetimes.rs:360:97
    |
360 |                 if let ItemKind::Existential(ref exist_ty) = map.expect_item(map.hir_to_node_id(item.id)).node {
    |                                                                                                 ^^^^^^^ expected struct `rustc::hir::HirId`, found struct `syntax::ast::NodeId`
    |
    = note: expected type `rustc::hir::HirId`
               found type `syntax::ast::NodeId`

error[E0609]: no field `ctor_def_id` on type `&&rustc::ty::VariantDef`
   --> src/matches.rs:519:55
    |
519 |                         missing_variants.retain(|e| e.ctor_def_id != Some(p.def.def_id()));
    |                                                       ^^^^^^^^^^^

error[E0609]: no field `ctor_def_id` on type `&&rustc::ty::VariantDef`
   --> src/matches.rs:523:55
    |
523 |                         missing_variants.retain(|e| e.ctor_def_id != Some(p.def.def_id()));
    |                                                       ^^^^^^^^^^^

error[E0609]: no field `def_id` on type `&&rustc::ty::VariantDef`
   --> src/matches.rs:542:68
    |
542 |                 format!("{}{}{}", ident_str, cx.tcx.def_path_str(v.def_id), suffix)
    |                                                                    ^^^^^^

error[E0599]: no variant named `Ctor` found for type `rustc::hir::def::Def` in the current scope
  --> src/no_effect.rs:75:63
   |
75 |                     Def::Struct(..) | Def::Variant(..) | Def::Ctor(..) => {
   |                                                               ^^^^ variant not found in `rustc::hir::def::Def`

error[E0599]: no variant named `Ctor` found for type `rustc::hir::def::Def` in the current scope
   --> src/no_effect.rs:169:63
    |
169 |                     Def::Struct(..) | Def::Variant(..) | Def::Ctor(..) if !has_drop(cx, cx.tables.expr_ty(expr)) => {
    |                                                               ^^^^ variant not found in `rustc::hir::def::Def`

error[E0599]: no variant named `Ctor` found for type `rustc::hir::def::Def` in the current scope
   --> src/question_mark.rs:131:29
    |
131 |                 if let Def::Ctor(def_id, def::CtorOf::Variant, _) = cx.tables.qpath_def(qp, expression.hir_id) {
    |                             ^^^^ variant not found in `rustc::hir::def::Def`

error[E0609]: no field `ty` on type `rustc::mir::tcx::PlaceTy<'_>`
   --> src/redundant_clone.rs:302:77
    |
302 |                     field = has_drop(cx, place.ty(&mir.local_decls, cx.tcx).ty);
    |                                                                             ^^

error[E0599]: no variant named `Ctor` found for type `rustc::hir::def::Def` in the current scope
   --> src/use_self.rs:236:32
    |
236 |             } else if let Def::Ctor(ctor_did, def::CtorOf::Struct, CtorKind::Fn) = path.def {
    |                                ^^^^ variant not found in `rustc::hir::def::Def`

error: aborting due to 15 previous errors

oli-obk · 2019-04-08T19:12:21Z

I think you just need to update your compiler to the latest nightly

xd009642 · 2019-04-08T19:26:36Z

Yeah I realised just now my rustup default was stable not nightly! It's all good now, bar the float_cmp failure which was already there and the feedback I'm yet to implement

flip1995 · 2019-07-27T22:06:27Z

Great! Thanks for all the work, let's wait if travis still has something to say and after that, this should be ready to merge.

flip1995 · 2019-07-27T22:32:17Z

Travis reported some dogfood errors. Please fix them.

xd009642 · 2019-07-28T08:31:55Z

Dogfood errors are now fixed

flip1995 · 2019-07-29T07:31:43Z

@bors r+

bors · 2019-07-29T07:31:44Z

📌 Commit 78ebcaa has been approved by flip1995

bors · 2019-07-29T07:31:53Z

⌛ Testing commit 78ebcaa with merge 62536e0...

trait bounds lint - repeated types This PR is to tackle #3764 it's still a WIP and doesn't work but this is an initial stab. It builds though I haven't added any tests as I'm not sure where lint tests should go? Unfortunately, it seems id isn't tied to the type itself but I guess where it is in the AST? Looking at https://manishearth.github.io/rust-internals-docs/syntax/ast/struct.Ty.html I can't see any members that would let me tell if a type was repeated in multiple trait bounds. There may be other issues with how I've implemented this so any assistance is appreciated!

bors · 2019-07-29T07:36:55Z

💔 Test failed - checks-travis

flip1995 · 2019-07-29T09:22:40Z

@bors retry

bors · 2019-07-29T09:22:49Z

⌛ Testing commit 78ebcaa with merge 23c6452...

trait bounds lint - repeated types This PR is to tackle #3764 it's still a WIP and doesn't work but this is an initial stab. It builds though I haven't added any tests as I'm not sure where lint tests should go? Unfortunately, it seems id isn't tied to the type itself but I guess where it is in the AST? Looking at https://manishearth.github.io/rust-internals-docs/syntax/ast/struct.Ty.html I can't see any members that would let me tell if a type was repeated in multiple trait bounds. There may be other issues with how I've implemented this so any assistance is appreciated! changelog: Add new lint: `type_repetition_in_bounds`

bors · 2019-07-29T09:35:15Z

💔 Test failed - status-appveyor

xd009642 · 2019-07-29T18:35:27Z

@flip1995 anything I need to do for these failures?

flip1995 · 2019-07-30T07:26:21Z

No that's unrelated, I just wait until those are fixed (in another PR) and then merge this.

flip1995 · 2019-07-30T12:11:14Z

@bors retry

bors · 2019-07-30T12:11:22Z

⌛ Testing commit 78ebcaa with merge c3e9136...

trait bounds lint - repeated types This PR is to tackle #3764 it's still a WIP and doesn't work but this is an initial stab. It builds though I haven't added any tests as I'm not sure where lint tests should go? Unfortunately, it seems id isn't tied to the type itself but I guess where it is in the AST? Looking at https://manishearth.github.io/rust-internals-docs/syntax/ast/struct.Ty.html I can't see any members that would let me tell if a type was repeated in multiple trait bounds. There may be other issues with how I've implemented this so any assistance is appreciated! changelog: Add new lint: `type_repetition_in_bounds`

bors · 2019-07-30T12:45:45Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing c3e9136 to master...

xd009642 marked this pull request as ready for review February 18, 2019 20:08

phansch added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Feb 24, 2019

xd009642 force-pushed the issue-3764 branch from b2b7914 to db01ece Compare March 9, 2019 14:50

flip1995 reviewed Mar 24, 2019

View reviewed changes

clippy_lints/src/utils/hir_utils.rs Outdated Show resolved Hide resolved

flip1995 reviewed Mar 24, 2019

View reviewed changes

clippy_lints/src/utils/hir_utils.rs Outdated Show resolved Hide resolved

flip1995 reviewed Mar 24, 2019

View reviewed changes

tests/ui/type_repetition_in_bounds.stderr Outdated Show resolved Hide resolved

flip1995 reviewed Mar 24, 2019

View reviewed changes

clippy_lints/src/trait_bounds.rs Outdated Show resolved Hide resolved

xd009642 force-pushed the issue-3764 branch from c39140a to 2a6708b Compare April 8, 2019 19:00

flip1995 approved these changes Jul 27, 2019

View reviewed changes

Fix dogfood test

78ebcaa

flip1995 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jul 29, 2019

bors merged commit 78ebcaa into rust-lang:master Jul 30, 2019

taiki-e mentioned this pull request Aug 2, 2019

Fix warnings on the latest nightly rust-lang/futures-rs#1772

Merged

dtolnay mentioned this pull request Aug 2, 2019

type_repetition_in_bounds should not trigger on macro-generated code #4326

Closed

flip1995 mentioned this pull request Sep 26, 2019

ICE with clippy on nightly #4579

Closed

trait bounds lint - repeated types #3766

trait bounds lint - repeated types #3766

Conversation

xd009642 commented Feb 15, 2019 • edited by flip1995 Loading

oli-obk commented Feb 15, 2019

xd009642 commented Feb 18, 2019

oli-obk commented Feb 19, 2019

xd009642 commented Feb 26, 2019

flip1995 commented Mar 9, 2019

xd009642 commented Mar 9, 2019

flip1995 commented Mar 9, 2019

xd009642 commented Mar 9, 2019

flip1995 commented Mar 9, 2019

xd009642 commented Mar 9, 2019

flip1995 commented Mar 9, 2019

xd009642 commented Mar 23, 2019

flip1995 commented Mar 24, 2019

xd009642 commented Mar 24, 2019

xd009642 commented Mar 24, 2019

flip1995 commented Mar 25, 2019

bors commented Apr 8, 2019

flip1995 commented Apr 8, 2019

xd009642 commented Apr 8, 2019

xd009642 commented Apr 8, 2019

oli-obk commented Apr 8, 2019

xd009642 commented Apr 8, 2019

flip1995 commented Jul 27, 2019

flip1995 commented Jul 27, 2019

xd009642 commented Jul 28, 2019

flip1995 commented Jul 29, 2019

bors commented Jul 29, 2019

bors commented Jul 29, 2019

bors commented Jul 29, 2019

flip1995 commented Jul 29, 2019

bors commented Jul 29, 2019

bors commented Jul 29, 2019

xd009642 commented Jul 29, 2019

flip1995 commented Jul 30, 2019

flip1995 commented Jul 30, 2019

bors commented Jul 30, 2019

bors commented Jul 30, 2019

xd009642 commented Feb 15, 2019 •

edited by flip1995

Loading