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

Querify whether a type has structural equality (Take 2) #73066

Merged
merged 3 commits into from
Jun 13, 2020

Conversation

ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Jun 6, 2020

Alternative to #72177.

Unlike in #72177, this helper method works for all types, falling back to a query for TyKind::Adts that determines whether the {Partial,}StructuralEq traits are implemented.

This is my preferred interface for this method. I think this is better than just documenting that the helper only works for ADTs. If others disagree, we can just merge #72177 with the fixes applied. This has already taken far too long.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 6, 2020
@ecstatic-morse
Copy link
Contributor Author

r? @pnkfelix

@ecstatic-morse ecstatic-morse force-pushed the query-structural-eq2 branch from 9dd072f to 9c1d643 Compare June 6, 2020 17:39
@ecstatic-morse ecstatic-morse changed the title Querify whether a type has structural equality Querify whether a type has structural equality (Take 2) Jun 6, 2020
@ecstatic-morse ecstatic-morse force-pushed the query-structural-eq2 branch from 9c1d643 to 38f6511 Compare June 6, 2020 17:49
@@ -778,6 +778,58 @@ impl<'tcx> ty::TyS<'tcx> {
}
}

/// Returns `true` if equality for this type is both total and structural.
Copy link
Contributor Author

@ecstatic-morse ecstatic-morse Jun 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "total" comes from "totally ordered". Maybe this terminology doesn't extend to equality?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what universally standard terminology is here, but in our own docs for PartialEq, the adjective "full" is used to differentiate, as in "full equivalence relation" versus "partial equivalence relation."

Copy link
Member

@pnkfelix pnkfelix Jun 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Or you could say "if equality for this type is both reflexive and structural", since the only thing differentiating an equivalence relation from a partial equivalence relation (PER) is whether the relation is reflexive.)

@ecstatic-morse
Copy link
Contributor Author

The perf improvement from this should be the same as #73021.

@@ -66,26 +67,25 @@ pub fn search_for_structural_match_violation<'tcx>(
/// Note that this does *not* recursively check if the substructure of `adt_ty`
/// implements the traits.
pub fn type_marked_structural(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type_marked_structural method is now only called from the has_structural_eq_impls query closure, right?

I think it would be better to make it non-pub in that case.

@pnkfelix
Copy link
Member

r=me.

I'm not telling bors myself, because I want to give @ecstatic-morse a chance to see if they can make type_marked_structural non-pub.

@pnkfelix pnkfelix added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 12, 2020
This helper method works for all types, falling back to a query for
`TyKind::Adt`s to determine whether the implement the
`{Partial,}StructuralEq` traits.
@ecstatic-morse
Copy link
Contributor Author

@pnkfelix Applied both fixes.

@bors r=pnkfelix

@bors
Copy link
Contributor

bors commented Jun 12, 2020

📌 Commit 2801761 has been approved by pnkfelix

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 12, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 12, 2020
…, r=pnkfelix

Querify whether a type has structural equality (Take 2)

Alternative to rust-lang#72177.

Unlike in rust-lang#72177, this helper method works for all types, falling back to a query for `TyKind::Adt`s that determines whether the `{Partial,}StructuralEq` traits are implemented.

This is my preferred interface for this method. I think this is better than just documenting that the helper only works for ADTs. If others disagree, we can just merge rust-lang#72177 with the fixes applied. This has already taken far too long.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 13, 2020
Rollup of 8 pull requests

Successful merges:

 - rust-lang#72932 (Clarify the behaviour of Pattern when used with methods like str::contains)
 - rust-lang#73066 (Querify whether a type has structural equality (Take 2))
 - rust-lang#73194 (Prefer the associated constants for pattern matching error)
 - rust-lang#73241 (Add/update comments about MinGW late_link_args)
 - rust-lang#73267 (Use the built cargo for cargotest.)
 - rust-lang#73290 (Fix links when pinging notification groups)
 - rust-lang#73302 (Adjusted some doctests in libcore to use `should_panic`.)
 - rust-lang#73308 (pretty/asm.rs should only be tested for x86_64 and not AArch64)

Failed merges:

r? @ghost
@bors bors merged commit 6ad8cbd into rust-lang:master Jun 13, 2020
@ecstatic-morse ecstatic-morse deleted the query-structural-eq2 branch October 6, 2020 01:42
@cuviper cuviper added this to the 1.46 milestone May 2, 2024
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.

6 participants