Remove most of PartialEq and Hash impls from AST and HIR structures
#51829
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @eddyb |
|
A bit of a policy question - what to do with
|
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
eddyb
reviewed
Jun 27, 2018
| @@ -1922,7 +1922,7 @@ impl<'a> LoweringContext<'a> { | |||
| variadic: decl.variadic, | |||
| has_implicit_self: decl.inputs.get(0).map_or(false, |arg| match arg.ty.node { | |||
| TyKind::ImplicitSelf => true, | |||
| TyKind::Rptr(_, ref mt) => mt.ty.node == TyKind::ImplicitSelf, | |||
| TyKind::Rptr(_, ref mt) => mt.ty.node.is_implicit_self(), | |||
There was a problem hiding this comment.
I think this should just use an inline match - since it's really the TyKind::ImplicitSelf => true, line above, applied to the pointee of the reference.
Nevermind, it's used at least twice.
eddyb
reviewed
Jun 27, 2018
| attr.id.hash(hasher); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Yes :(
Rustdoc keeps things like full types in sets/maps for whatever reasons (see e.g. fn make_final_bounds). I don't want to touch that code.
cc @QuietMisdreavus
eddyb
reviewed
Jun 27, 2018
src/librustdoc/clean/mod.rs
Outdated
| @@ -1841,6 +1866,8 @@ pub enum GenericParamDefKind { | |||
| }, | |||
| } | |||
|
|
|||
| impl Eq for GenericParamDefKind {} | |||
There was a problem hiding this comment.
What's up with these? Can they be avoided? Wait this is rustdoc, huh.
There was a problem hiding this comment.
Yeah, I just forgot to add Eq impl for hir::SyntheticTyParamKind and didn't want to recompile everything starting with rustc (will fix).
This is very much relevant to #51829 (comment) though.
eddyb
reviewed
Jun 27, 2018
| @@ -84,7 +84,7 @@ impl< | |||
|
|
|||
| impl< | |||
| K: Decodable + PartialEq + Ord, | |||
| V: Decodable + PartialEq | |||
| V: Decodable | |||
| > Decodable for BTreeMap<K, V> { | |||
eddyb
reviewed
Jun 27, 2018
src/libsyntax/ast.rs
Outdated
| pub fn is_implicit_self(&self) -> bool { | ||
| if let TyKind::ImplicitSelf = *self { true } else { false } | ||
| } | ||
| pub(crate) fn is_empty_tuple(&self) -> bool { |
|
cc @rust-lang/compiler @rust-lang/dev-tools on #51829 (comment) |
|
I don't really have a strong opinion about when to derive |
|
rustdoc is in the repo, so 'as needed' should cover that. Rustfmt typically does not do much with nodes other than passing them around by reference, so I'm pretty sure it will be ok. Would be great if you could test building rustfmt before landing, but if not we'll catch it when we try and update rustc_ap_syntax and can put back any necessary impls/derives. |
|
☔ The latest upstream changes (presumably #51762) made this pull request unmergeable. Please resolve the merge conflicts. |
petrochenkov
commented
Jul 1, 2018
src/libsyntax/ast.rs
Outdated
| pub enum VisibilityKind { | ||
| Public, | ||
| Crate(CrateSugar), | ||
| Restricted { path: P<Path>, id: NodeId }, | ||
| Inherited, | ||
| } | ||
|
|
||
| impl VisibilityKind { | ||
| pub fn is_public(&self) -> bool { |
There was a problem hiding this comment.
FIXME: Rename to is_pub for compatibility with #51866.
petrochenkov
added
S-waiting-on-author
|
@eddyb |
petrochenkov
added
S-waiting-on-review
|
☔ The latest upstream changes (presumably #52168) made this pull request unmergeable. Please resolve the merge conflicts. |
|
r=me, sorry for not saying this earlier |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
1 similar comment
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Jul 14, 2018
Remove most of `PartialEq` and `Hash` impls from AST and HIR structures Continuation of #49326, prerequisite for removing `PartialEq` for `Ident`.
|
☀️ Test successful - status-appveyor, status-travis |
|
📣 Toolstate changed by #51829! Tested on commit 0db03e6. 💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra). |
rust-highfive
added a commit
to rust-lang-nursery/rust-toolstate
that referenced
this pull request
Jul 14, 2018
Tested on commit rust-lang/rust@0db03e6. Direct link to PR: <rust-lang/rust#51829> 💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra). 💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra).
|
Clippy error log: |
|
Why remove it from LitKind? It doesn't contain idents. Same thing for Spanned. |
|
I don't think complexity wise it makes sense to recreate these impls by hand inside clippy. I will reintroduce the minimally required set of impls (at least for types like Spanned and LitKind, which have a straight forward behaviour that doesn't have footguns) |
|
Also ISTM the bug here is that you're using == where you shouldn't be, I don't think this warrants removing the Eq impls. We can work around this but it's not ideal and it's certainly going to be more brittle. |
This PR simply removed everything that's not used during |
🙁 can you explain what these cases are? |
|
I initially thought "oh my god we can never get clippy to work in a sensible way after this". And I'm very sorry for having thought that and not first considered the full implications. Clippy's comparisons were absolutely useless in some situations. I fixed them by using our |
|
@Manishearth In general comparing ASTs is a bad idea, what you want is to pattern-match them except Rust makes this very hard around I didn't realize clippy already had some machinery (the |
|
Yes, many of the cases here should be pattern matching, but I don't think that's true for all For example, what if you want to compare vectors like Pattern matching works for the simple cases, this falls apart for anything even slightly more complicated. |
I have a suggestion for that,
That's where the second part, "logic or fuzzy unification", comes in. |
|
Yeah, there are many cases where the nodes are involved so we can't use them, but this PR went much further and removed it from things which actually have decent PartialEq impls. |
|
To drive the point home, @oli-obk added 7 new uses of And even this one is only legitimate because it "unifies" types, which should be done on |
ghost
mentioned this pull request
topecongiro
added a commit
to topecongiro/rustfmt
that referenced
this pull request
Jul 20, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Continuation of #49326, prerequisite for removing
PartialEqforIdent.