-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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 implements StructuralEq
and PartialStructuralEq
#72177
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -778,6 +778,27 @@ impl<'tcx> ty::TyS<'tcx> { | |
} | ||
} | ||
|
||
/// Returns `true` if this type implements both `PartialStructuralEq` and `StructuralEq`. | ||
/// | ||
/// These marker traits are implemented along with `PartialEq` and `Eq` when implementations | ||
/// for those traits are derived. They indicate that two values of this type are equal if all | ||
/// of their fields are equal. A quirk of the marker traits is that their implementation is | ||
/// never conditional on generic parameter bounds. For that reason, this helper function does | ||
/// not take a `ParamEnv`. | ||
/// | ||
/// 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. | ||
#[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() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is incomplete at the moment, since many builtin types don't actually implement There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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....... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So right now, this will return If you agree, there's two ways to accomplish this, we could either move this special-casing into the query itself or implement There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My main reaction is: I think you're rightfully worried about the method being prone to misuse.
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 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. |
||
return true; | ||
} | ||
|
||
tcx.is_structural_eq_raw(self) | ||
} | ||
|
||
pub fn same_type(a: Ty<'tcx>, b: Ty<'tcx>) -> bool { | ||
match (&a.kind, &b.kind) { | ||
(&Adt(did_a, substs_a), &Adt(did_b, substs_b)) => { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.