Querify whether a type implements `StructuralEq` and `PartialStructuralEq` #72177

ecstatic-morse · 2020-05-13T20:50:12Z

At present, computing this is relatively expensive. This PR adds a query along with a helper method on TyS to cache the "shallow" version of this check. See #67343 (comment) for background. The deep version will still need to go through the type visitor in the structural_match module.

I'm not quite sure how to handle obligation causes, since they require a Span and a HirId. For now, this PR switches to ObligationCause::dummy(), which is tolerable since we don't report a fulfilment error in the structural_match module. I don't really know what the right choice is here.

r? @pnkfelix

ecstatic-morse · 2020-05-13T21:33:29Z

src/librustc_middle/ty/util.rs

+    #[inline]
+    pub fn is_structural_eq_shallow(&'tcx self, tcx: TyCtxt<'tcx>) -> bool {
+        // Fast path for some builtin types
+        if self.is_primitive() || self.is_str() {


This is incomplete at the moment, since many builtin types don't actually implement StructuralEq but are handled specially in the type visitor in structural_match. I don't know if we want to resolve this problem by checking for them here or by implementing the trait for various builtins (arrays, pointers, references, etc.), but it seems like we should figure this out before merging this.

I suspect if we attempt to handle this by implementing the trait in all the cases of interest, we're going to run into the problem where we cannot implement it for types with a for <'a> ..., which arise from function pointers with a reference argument. (See also #46989 (comment) ... and I'm struggling to find a specific issue about this particular wart in the language today...)

Is it bad to leave them unhandled here for now? All that means is that you miss the fast path for those types, right?

Then that is not a correctness concern; it is at most a performance concern, right? Which to me implies that we can merge this without figuring out the answer to your question.......

So right now, this will return false for types such as [i64; 4] and &str. This is correct at the moment because is_structural_eq_shallow is always used in conjunction with type visitors that handle those cases separately, only invoking the query for TyKind::Adt. We could just document this fact and call it a day, or switch this query to accept only TyKind::Adts, but I think it will be less prone to misuse if we handle the fundamental types inside the query instead of making the caller do it.

If you agree, there's two ways to accomplish this, we could either move this special-casing into the query itself or implement StructuralEq for fundamental types. Your point about function pointers suggests that we just do the first one, since we can't cover all the fundamental types via trait impls anyway.

@pnkfelix ^

My main reaction is: I think you're rightfully worried about the method being prone to misuse.

In particular, I'm worried that the doc-comment you've put above is_structurally_eq_shallow might actually detract from readers' understanding of the code as written. (It adopts a relatively simple specification ("when this returns true, you can conclude X; when it returns false, you cannot conclude anything"), which is what led me to say "can't we just land this code as is, and expand it later?"). But as you point out, all the current call-sites are making stronger assumptions about its behavior ("when given an ADT, then true implies X, and false implies not-X").

My first thought is: It would be nice to decide what interface we want, and then have the calls match it. But then my second thought is: It seems silly to put the effort into making all the call-sites handle the general case when they know that the method will produce usable results immediately when given an ADT.

In the long-term, you're probably right that we should move the special-casing into the query itself, so that it will handle any input ty properly.

However, I do not want to block this PR on that addition. For various reasons (such as ease of identifying source of performance regressions), I think we should land this PR as is, with an addition to the doc-comment saying that the is_structurally_eq_* methods happen to be precise when given an ADT. (i.e., I am advising that we indeed "just document this fact and call it a day", except I'm only saying "call it a day" for the short term; maybe add a FIXME for the longer term goal.)

(It adopts a relatively simple specification ("when this returns true, you can conclude X; when it returns false, you cannot conclude anything")

actually maybe you never intended this interpretation? I personally read into the first sentence quite literally as just "if" and not "if and only if".

In any case, I still recommend the approach of adding the comment explaining the situation, landing the PR, and leaving further generalization for later work.

bors · 2020-05-16T18:00:28Z

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

ecstatic-morse · 2020-05-17T21:14:19Z

This should fix some of the perf loss from #67343.

@bors try @rust-timer queue

rust-timer · 2020-05-17T21:14:20Z

Awaiting bors try build completion

bors · 2020-05-17T21:14:32Z

⌛ Trying commit 37270ae with merge 24a2bc1a0f9dd3ad72bffa6f2b9ee946bb5f4fe9...

rust-timer · 2020-05-17T21:20:06Z

Awaiting bors try build completion

bors · 2020-05-17T21:20:14Z

⌛ Trying commit 37270ae with merge b8fb01d57e56c6d811238ef6891b1b6d963b5d59...

bors · 2020-05-17T23:53:46Z

☀️ Try build successful - checks-azure
Build commit: b8fb01d57e56c6d811238ef6891b1b6d963b5d59 (b8fb01d57e56c6d811238ef6891b1b6d963b5d59)

rust-timer · 2020-05-17T23:53:48Z

Queued b8fb01d57e56c6d811238ef6891b1b6d963b5d59 with parent d79f1bd, future comparison URL.

rust-timer · 2020-05-18T08:14:08Z

Finished benchmarking try commit b8fb01d57e56c6d811238ef6891b1b6d963b5d59, comparison URL.

pnkfelix · 2020-05-19T20:43:19Z

This seems fine to me. I want to hear more from @ecstatic-morse about the issue they raised up above before I mark it as r+'ed.

pnkfelix · 2020-05-19T20:44:01Z

(r=me if ecstatic-morse agrees with my logic that we don't have to find the answer to their question yet to land this...)

pnkfelix · 2020-06-05T17:56:20Z

@bors r+

bors · 2020-06-05T17:56:21Z

📌 Commit 37270ae has been approved by pnkfelix

pnkfelix · 2020-06-05T17:56:46Z

@bors r-

pnkfelix · 2020-06-05T19:51:31Z

src/librustc_middle/ty/util.rs

+    ///
+    /// This function is "shallow" because it may return `true` for a type whose fields are not
+    /// `StructuralEq`. You will need to use a type visitor if you want to know whether a call to
+    /// `PartialEq::eq` will proceed structurally for a given type.


Suggested change

/// `PartialEq::eq` will proceed structurally for a given type.

/// `PartialEq::eq` will proceed structurally for a given type.

///

/// Note: This function is "precise" when called on an ADT type;

/// that is, for an input ADT type, this will return `true` if

/// *and only if* the input ADT type implements both

/// `PartialStructuralEq` and `StructuralEq`. For non-ADT's, it

/// does not provide that strong a guarantee.

pnkfelix · 2020-06-05T19:53:06Z

I r+'ed this based on a misreading of @ecstatic-morse 's response above. Then after reflecting futher, I cancelled my r+, added a response to @ecstatic-morse along with my suggestion of how to proceed in the short term.

r=me with my suggested change. If @ecstatic-morse wants to take a different approach for this PR, then I'm happy to see it.

ecstatic-morse · 2020-06-11T21:00:25Z

Closing this PR. I opened #73066 almost a week ago which contains my preferred approach.

…, 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.

rust-highfive assigned pnkfelix May 13, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 13, 2020

ecstatic-morse changed the title ~~Add query to deterimine if type implements StructuralEq and PartialStructuralEq~~ Querify whether type implements StructuralEq and PartialStructuralEq May 13, 2020

ecstatic-morse changed the title ~~Querify whether type implements StructuralEq and PartialStructuralEq~~ Querify whether a type implements StructuralEq and PartialStructuralEq May 13, 2020

ecstatic-morse commented May 13, 2020

View reviewed changes

ecstatic-morse force-pushed the query-structural-eq branch from ca8eca8 to d53bea7 Compare May 16, 2020 23:43

Add query to deterimine if type implements StructuralEq

37270ae

ecstatic-morse force-pushed the query-structural-eq branch 2 times, most recently from c4eaa62 to 37270ae Compare May 17, 2020 00:00

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 May 19, 2020

Elinvynia 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 27, 2020

ecstatic-morse 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 (such as code changes or more information) from the author. labels Jun 3, 2020

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 Jun 5, 2020

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 5, 2020

pnkfelix reviewed Jun 5, 2020

View reviewed changes

ecstatic-morse mentioned this pull request Jun 6, 2020

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

Merged

ecstatic-morse closed this Jun 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Querify whether a type implements `StructuralEq` and `PartialStructuralEq` #72177

Querify whether a type implements `StructuralEq` and `PartialStructuralEq` #72177

ecstatic-morse commented May 13, 2020

ecstatic-morse May 13, 2020 •

edited

Loading

pnkfelix May 19, 2020

pnkfelix May 19, 2020

ecstatic-morse May 19, 2020 •

edited

Loading

ecstatic-morse Jun 3, 2020

pnkfelix Jun 5, 2020 •

edited

Loading

pnkfelix Jun 5, 2020

bors commented May 16, 2020

ecstatic-morse commented May 17, 2020 •

edited

Loading

rust-timer commented May 17, 2020

bors commented May 17, 2020

rust-timer commented May 17, 2020

bors commented May 17, 2020

bors commented May 17, 2020

rust-timer commented May 17, 2020

rust-timer commented May 18, 2020

pnkfelix commented May 19, 2020

pnkfelix commented May 19, 2020

pnkfelix commented Jun 5, 2020

bors commented Jun 5, 2020

pnkfelix commented Jun 5, 2020

pnkfelix Jun 5, 2020

pnkfelix commented Jun 5, 2020

ecstatic-morse commented Jun 11, 2020

-    /// `PartialEq::eq` will proceed structurally for a given type.
+    /// `PartialEq::eq` will proceed structurally for a given type.
+    ///
+    /// Note: This function is "precise" when called on an ADT type;
+    /// that is, for an input ADT type, this will return `true` if
+    /// *and only if* the input ADT type implements both
+    /// `PartialStructuralEq` and `StructuralEq`. For non-ADT's, it
+    /// does not provide that strong a guarantee.

Querify whether a type implements StructuralEq and PartialStructuralEq #72177

Querify whether a type implements StructuralEq and PartialStructuralEq #72177

Conversation

ecstatic-morse commented May 13, 2020

ecstatic-morse May 13, 2020 • edited Loading

Choose a reason for hiding this comment

pnkfelix May 19, 2020

Choose a reason for hiding this comment

pnkfelix May 19, 2020

Choose a reason for hiding this comment

ecstatic-morse May 19, 2020 • edited Loading

Choose a reason for hiding this comment

ecstatic-morse Jun 3, 2020

Choose a reason for hiding this comment

pnkfelix Jun 5, 2020 • edited Loading

Choose a reason for hiding this comment

pnkfelix Jun 5, 2020

Choose a reason for hiding this comment

bors commented May 16, 2020

ecstatic-morse commented May 17, 2020 • edited Loading

rust-timer commented May 17, 2020

bors commented May 17, 2020

rust-timer commented May 17, 2020

bors commented May 17, 2020

bors commented May 17, 2020

rust-timer commented May 17, 2020

rust-timer commented May 18, 2020

pnkfelix commented May 19, 2020

pnkfelix commented May 19, 2020

pnkfelix commented Jun 5, 2020

bors commented Jun 5, 2020

pnkfelix commented Jun 5, 2020

pnkfelix Jun 5, 2020

Choose a reason for hiding this comment

pnkfelix commented Jun 5, 2020

ecstatic-morse commented Jun 11, 2020

Querify whether a type implements `StructuralEq` and `PartialStructuralEq` #72177

Querify whether a type implements `StructuralEq` and `PartialStructuralEq` #72177

ecstatic-morse May 13, 2020 •

edited

Loading

ecstatic-morse May 19, 2020 •

edited

Loading

pnkfelix Jun 5, 2020 •

edited

Loading

ecstatic-morse commented May 17, 2020 •

edited

Loading