Skip to content

Conversation

nnethercote
Copy link
Contributor

This PR removes unnecessary trait bounds and derivations from many types.

r? @nikomatsakis

@nnethercote
Copy link
Contributor Author

I did this because I was trying to work out why Symbol implemented Hash, and in doing so I found a bunch of types that derived Hash unnecessarily. Then I took a semi-mechanical approach:

  • I removed Hash trait bounds from all types, except those in tests and lib{alloc,core,std}.
  • I compiled (and recompiled...), and added back the Hash trait bounds everywhere the compiler said it was necessary.
  • I then removed all Hash derives, except those in tests and lib{alloc,core,std}.
  • I compiled/recompiled, adding back all Hash derives everywhere the compiler said was necessary.
  • I figure the types with unnecessary Hash derives were good candidates for possibly having other unnecessary dervies. So I then removed all derives from these types, and the re-added them back in as necessary.

Overall, I did a thorough job eliminating unnecessary Hash bounds and derives, and a scattershot job for other trait bounds and derives.

The whole process was very tedious, but it did find a few unnecessary trait bounds and many unnecessary derives. It would be nice if the compiler could identify at least some of these, the way it identifies unnecessary use items.

@nnethercote nnethercote force-pushed the rm-unnecessary-traits branch from d0d6d21 to f467bc5 Compare October 21, 2019 04:24
@JohnTitor JohnTitor added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 21, 2019
@nnethercote nnethercote force-pushed the rm-unnecessary-traits branch from f467bc5 to ac6daed Compare October 21, 2019 09:59
@Centril
Copy link
Contributor

Centril commented Oct 21, 2019

r? @Centril @bors r+

-- Looks great; some tools may break as a result of this but we can re-add specific bounds as needed.

@bors
Copy link
Collaborator

bors commented Oct 21, 2019

📌 Commit ac6daed has been approved by Centril

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 21, 2019
Centril added a commit to Centril/rust that referenced this pull request Oct 21, 2019
…r=Centril

Remove unnecessary trait bounds and derivations

This PR removes unnecessary trait bounds and derivations from many types.

r? @nikomatsakis
Centril added a commit to Centril/rust that referenced this pull request Oct 21, 2019
…r=Centril

Remove unnecessary trait bounds and derivations

This PR removes unnecessary trait bounds and derivations from many types.

r? @nikomatsakis
bors added a commit that referenced this pull request Oct 22, 2019
Rollup of 7 pull requests

Successful merges:

 - #62330 (Change untagged_unions to not allow union fields with drop)
 - #65092 (make is_power_of_two a const function)
 - #65621 (miri: add write_bytes method to Memory doing bounds-checks and supporting iterators)
 - #65647 (Remove unnecessary trait bounds and derivations)
 - #65653 (keep the root dir clean from debugging)
 - #65660 (Rename `ConstValue::Infer(InferConst::Canonical(..))` to `ConstValue::Bound(..)`)
 - #65663 (Fix typo from #65214)

Failed merges:

r? @ghost
@bors bors merged commit ac6daed into rust-lang:master Oct 22, 2019
@nnethercote nnethercote deleted the rm-unnecessary-traits branch October 22, 2019 04:43
@matthiaskrgr
Copy link
Member

Clippy needs some of these bounds.

error[E0369]: binary operation `==` cannot be applied to type `syntax::ast::LitKind`
   --> clippy_lints/src/utils/hir_utils.rs:117:70
    |
117 |             (&ExprKind::Lit(ref l), &ExprKind::Lit(ref r)) => l.node == r.node,
    |                                                               ------ ^^ ------ syntax::ast::LitKind
    |                                                               |
    |                                                               syntax::ast::LitKind
    |
    = note: an implementation of `std::cmp::PartialEq` might be missing for `syntax::ast::LitKind`

error[E0599]: no method named `hash` found for type `&syntax::source_map::Spanned<rustc::hir::BinOpKind>` in the current scope
   --> clippy_lints/src/utils/hir_utils.rs:414:19
    |
414 |                 o.hash(&mut self.s);
    |                   ^^^^ method not found in `&syntax::source_map::Spanned<rustc::hir::BinOpKind>`
    |
    = note: the method `hash` exists but the following trait bounds were not satisfied:
            `&syntax::source_map::Spanned<rustc::hir::BinOpKind> : std::hash::Hash`

error[E0599]: no method named `hash` found for type `rustc::hir::BinOpKind` in the current scope
   --> clippy_lints/src/utils/hir_utils.rs:422:25
    |
422 |                 op.node.hash(&mut self.s);
    |                         ^^^^ method not found in `rustc::hir::BinOpKind`

error[E0599]: no method named `hash` found for type `&syntax::source_map::Spanned<syntax::ast::LitKind>` in the current scope
   --> clippy_lints/src/utils/hir_utils.rs:463:19
    |
463 |                 l.hash(&mut self.s);
    |                   ^^^^ method not found in `&syntax::source_map::Spanned<syntax::ast::LitKind>`
    |
    = note: the method `hash` exists but the following trait bounds were not satisfied:
            `&syntax::source_map::Spanned<syntax::ast::LitKind> : std::hash::Hash`

error[E0599]: no method named `hash` found for type `rustc::hir::UnOp` in the current scope
   --> clippy_lints/src/utils/hir_utils.rs:522:21
    |
522 |                 lop.hash(&mut self.s);
    |                     ^^^^ method not found in `rustc::hir::UnOp`

error: aborting due to 5 previous errors

Some errors have detailed explanations: E0369, E0599.
For more information about an error, try `rustc --explain E0369`.
error: could not compile `clippy_lints`.
warning: build failed, waiting for other jobs to finish...
error[E0369]: binary operation `==` cannot be applied to type `syntax::ast::LitKind`
   --> clippy_lints/src/utils/hir_utils.rs:117:70
    |
117 |             (&ExprKind::Lit(ref l), &ExprKind::Lit(ref r)) => l.node == r.node,
    |                                                               ------ ^^ ------ syntax::ast::LitKind
    |                                                               |
    |                                                               syntax::ast::LitKind
    |
    = note: an implementation of `std::cmp::PartialEq` might be missing for `syntax::ast::LitKind`

error[E0599]: no method named `hash` found for type `&syntax::source_map::Spanned<rustc::hir::BinOpKind>` in the current scope
   --> clippy_lints/src/utils/hir_utils.rs:414:19
    |
414 |                 o.hash(&mut self.s);
    |                   ^^^^ method not found in `&syntax::source_map::Spanned<rustc::hir::BinOpKind>`
    |
    = note: the method `hash` exists but the following trait bounds were not satisfied:
            `&syntax::source_map::Spanned<rustc::hir::BinOpKind> : std::hash::Hash`

error[E0599]: no method named `hash` found for type `rustc::hir::BinOpKind` in the current scope
   --> clippy_lints/src/utils/hir_utils.rs:422:25
    |
422 |                 op.node.hash(&mut self.s);
    |                         ^^^^ method not found in `rustc::hir::BinOpKind`

error[E0599]: no method named `hash` found for type `&syntax::source_map::Spanned<syntax::ast::LitKind>` in the current scope
   --> clippy_lints/src/utils/hir_utils.rs:463:19
    |
463 |                 l.hash(&mut self.s);
    |                   ^^^^ method not found in `&syntax::source_map::Spanned<syntax::ast::LitKind>`
    |
    = note: the method `hash` exists but the following trait bounds were not satisfied:
            `&syntax::source_map::Spanned<syntax::ast::LitKind> : std::hash::Hash`

error[E0599]: no method named `hash` found for type `rustc::hir::UnOp` in the current scope
   --> clippy_lints/src/utils/hir_utils.rs:522:21
    |
522 |                 lop.hash(&mut self.s);
    |                     ^^^^ method not found in `rustc::hir::UnOp`

error: aborting due to 5 previous errors

Can we revert this?

@tesuji
Copy link
Contributor

tesuji commented Oct 22, 2019

Just add necessary bounds for clippy I think.

@matthiaskrgr
Copy link
Member

Can we do this for external types?

@tesuji
Copy link
Contributor

tesuji commented Oct 22, 2019

I mean re-adding those bounds in rust repo, not in clippy side.

@mati865
Copy link
Member

mati865 commented Oct 22, 2019

Maybe Clippy could use Hashstable instead of Hash.
Probably PartialEq is unavoidable.

bors added a commit that referenced this pull request Oct 22, 2019
Readd some PartialEq and Hash derives used by Clippy

cc #65647

r? @Centril cc @Manishearth
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 22, 2019
bring back some Debug instances for Miri

These were erroneously removed in rust-lang#65647, but Miri needs them.

r? @Centril Cc @nnethercote @oli-obk
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 23, 2019
bring back some Debug instances for Miri

These were erroneously removed in rust-lang#65647, but Miri needs them.

r? @Centril Cc @nnethercote @oli-obk
bors added a commit to rust-lang/rust-clippy that referenced this pull request Oct 23, 2019
Centril added a commit to Centril/rust that referenced this pull request Oct 26, 2019
…etrochenkov

Derive Eq and Hash for SourceInfo again

In https://github.com/bjorn3/rustc_codegen_cranelift/blob/75c24b9c9677600422ec86fa9f4c78fe3678d2ce/src/common.rs#L368 I store it in a `indexmap::IndexSet`, which requires `Eq` and `Hash`. Unfortunately they were removed in rust-lang#65647, so I can't update to latest nightly.
@eddyb
Copy link
Member

eddyb commented Oct 28, 2019

@nnethercote Looks like this caused a lot of fallout - why not just remove Hash from Symbol and see what breaks?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants