Skip to content

Querify whether a type implements StructuralEq and PartialStructuralEq #72177

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Querify whether a type implements StructuralEq and PartialStructuralEq #72177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/librustc_middle/query/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -719,6 +719,11 @@ rustc_queries! {
desc { "computing whether `{}` needs drop", env.value }
}

/// Query backing `TyS::is_structural_eq_shallow`.
query is_structural_eq_raw(ty: Ty<'tcx>) -> bool {
desc { "computing whether `{:?}` has a derived implementation of `PartialEq` and `Eq`", ty }
}

/// A list of types where the ADT requires drop if and only if any of
/// those types require drop. If the ADT is known to always need drop
/// then `Err(AlwaysRequiresDrop)` is returned.
Expand Down
21 changes: 21 additions & 0 deletions src/librustc_middle/ty/util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

#[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() {
Copy link
Contributor Author

@ecstatic-morse ecstatic-morse May 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

The 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 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 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...)

Copy link
Contributor

Choose a reason for hiding this comment

The 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.......

Copy link
Contributor Author

@ecstatic-morse ecstatic-morse May 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pnkfelix ^

Copy link
Contributor

@pnkfelix pnkfelix Jun 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

The 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.

  • 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.)

Copy link
Contributor

Choose a reason for hiding this comment

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

(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.

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)) => {
Expand Down
6 changes: 1 addition & 5 deletions src/librustc_mir/transform/check_consts/qualifs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
//!
//! See the `Qualif` trait for more info.

use rustc_infer::infer::TyCtxtInferExt;
use rustc_middle::mir::*;
use rustc_middle::ty::{self, subst::SubstsRef, AdtDef, Ty};
use rustc_span::DUMMY_SP;
Expand Down Expand Up @@ -137,10 +136,7 @@ impl Qualif for CustomEq {
substs: SubstsRef<'tcx>,
) -> bool {
let ty = cx.tcx.mk_ty(ty::Adt(adt, substs));
let id = cx.tcx.hir().local_def_id_to_hir_id(cx.def_id.as_local().unwrap());
cx.tcx
.infer_ctxt()
.enter(|infcx| !traits::type_marked_structural(id, cx.body.span, &infcx, ty))
!ty.is_structural_eq_shallow(cx.tcx)
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir_build/hair/pattern/const_to_pat.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ impl<'a, 'tcx> ConstToPat<'a, 'tcx> {
}

fn type_marked_structural(&self, ty: Ty<'tcx>) -> bool {
traits::type_marked_structural(self.id, self.span, &self.infcx, ty)
ty.is_structural_eq_shallow(self.infcx.tcx)
}

fn to_pat(
Expand Down
1 change: 1 addition & 0 deletions src/librustc_trait_selection/traits/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,7 @@ fn type_implements_trait<'tcx>(

pub fn provide(providers: &mut ty::query::Providers<'_>) {
object_safety::provide(providers);
structural_match::provide(providers);
*providers = ty::query::Providers {
specialization_graph_of: specialize::specialization_graph_provider,
specializes: specialize::specializes,
Expand Down
36 changes: 21 additions & 15 deletions src/librustc_trait_selection/traits/structural_match.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
use crate::infer::{InferCtxt, TyCtxtInferExt};
use crate::traits::ObligationCause;
use crate::traits::{self, ConstPatternStructural, TraitEngine};
use crate::traits::{self, TraitEngine};

use rustc_data_structures::fx::FxHashSet;
use rustc_hir as hir;
use rustc_hir::lang_items::{StructuralPeqTraitLangItem, StructuralTeqTraitLangItem};
use rustc_middle::ty::query::Providers;
use rustc_middle::ty::{self, AdtDef, Ty, TyCtxt, TypeFoldable, TypeVisitor};
use rustc_span::Span;

Expand Down Expand Up @@ -41,14 +42,14 @@ pub enum NonStructuralMatchTy<'tcx> {
/// that arose when the requirement was not enforced completely, see
/// Rust RFC 1445, rust-lang/rust#61188, and rust-lang/rust#62307.
pub fn search_for_structural_match_violation<'tcx>(
id: hir::HirId,
span: Span,
_id: hir::HirId,
_span: Span,
tcx: TyCtxt<'tcx>,
ty: Ty<'tcx>,
) -> Option<NonStructuralMatchTy<'tcx>> {
// FIXME: we should instead pass in an `infcx` from the outside.
tcx.infer_ctxt().enter(|infcx| {
let mut search = Search { id, span, infcx, found: None, seen: FxHashSet::default() };
let mut search = Search { infcx, found: None, seen: FxHashSet::default() };
ty.visit_with(&mut search);
search.found
})
Expand All @@ -62,26 +63,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(
id: hir::HirId,
span: Span,
infcx: &InferCtxt<'_, 'tcx>,
adt_ty: Ty<'tcx>,
cause: ObligationCause<'tcx>,
) -> bool {
let mut fulfillment_cx = traits::FulfillmentContext::new();
let cause = ObligationCause::new(span, id, ConstPatternStructural);
// require `#[derive(PartialEq)]`
let structural_peq_def_id = infcx.tcx.require_lang_item(StructuralPeqTraitLangItem, Some(span));
let structural_peq_def_id =
infcx.tcx.require_lang_item(StructuralPeqTraitLangItem, Some(cause.span));
fulfillment_cx.register_bound(
infcx,
ty::ParamEnv::empty(),
adt_ty,
structural_peq_def_id,
cause,
cause.clone(),
);
// for now, require `#[derive(Eq)]`. (Doing so is a hack to work around
// the type `for<'a> fn(&'a ())` failing to implement `Eq` itself.)
let cause = ObligationCause::new(span, id, ConstPatternStructural);
let structural_teq_def_id = infcx.tcx.require_lang_item(StructuralTeqTraitLangItem, Some(span));
let structural_teq_def_id =
infcx.tcx.require_lang_item(StructuralTeqTraitLangItem, Some(cause.span));
fulfillment_cx.register_bound(
infcx,
ty::ParamEnv::empty(),
Expand All @@ -106,9 +106,6 @@ pub fn type_marked_structural(
/// find instances of ADTs (specifically structs or enums) that do not implement
/// the structural-match traits (`StructuralPartialEq` and `StructuralEq`).
struct Search<'a, 'tcx> {
id: hir::HirId,
span: Span,

infcx: InferCtxt<'a, 'tcx>,

/// Records first ADT that does not implement a structural-match trait.
Expand All @@ -125,7 +122,7 @@ impl Search<'a, 'tcx> {
}

fn type_marked_structural(&self, adt_ty: Ty<'tcx>) -> bool {
type_marked_structural(self.id, self.span, &self.infcx, adt_ty)
adt_ty.is_structural_eq_shallow(self.tcx())
}
}

Expand Down Expand Up @@ -220,3 +217,12 @@ impl<'a, 'tcx> TypeVisitor<'tcx> for Search<'a, 'tcx> {
false
}
}

pub fn provide(providers: &mut Providers<'_>) {
providers.is_structural_eq_raw = |tcx, ty| {
tcx.infer_ctxt().enter(|infcx| {
let cause = ObligationCause::dummy();
type_marked_structural(&infcx, ty, cause)
})
};
}