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

Improve fast rejection. #97136

Closed
Closed
Changes from 1 commit
Commits
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
133 changes: 118 additions & 15 deletions compiler/rustc_trait_selection/src/traits/select/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ use rustc_infer::infer::LateBoundRegionConversionTime;
use rustc_middle::dep_graph::{DepKind, DepNodeIndex};
use rustc_middle::mir::interpret::ErrorHandled;
use rustc_middle::thir::abstract_const::NotConstEvaluatable;
use rustc_middle::ty::fast_reject::{self, TreatParams};
use rustc_middle::ty::fold::BottomUpFolder;
use rustc_middle::ty::print::with_no_trimmed_paths;
use rustc_middle::ty::relate::TypeRelation;
Expand Down Expand Up @@ -214,6 +213,122 @@ enum BuiltinImplConditions<'tcx> {
Ambiguous,
}

fn tys_may_be_equivalent<'tcx>(oblig_ty: Ty<'tcx>, impl_ty: Ty<'tcx>) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

please move this into the fast_reject module

let k = impl_ty.kind();

// First, look for obvious equivalences, e.g. the types are equal. The most
// common case is to find such an equivalence trivially, e.g. `ty::Bool`
// and `ty::Bool`.
let mut maybe_equiv = match oblig_ty.kind() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer one large match (oblig_ty.kind(), impl_ty.kind()) here, starting with

(_, ty::Param(_) | ty::Projection(_) | ty::Error(_)) => true,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what I was referring to when I said I chose to structure it with exhaustive matches. Using _ here makes it easy to overlook updating this when new variants are added to Ty.

ty::Bool => matches!(k, ty::Bool),
ty::Char => matches!(k, ty::Char),
ty::Int(ity) => matches!(k, ty::Int(ity2) if ity == ity2),
ty::Uint(uty) => matches!(k, ty::Uint(uty2) if uty == uty2),
ty::Float(fty) => matches!(k, ty::Float(fty2) if fty == fty2),
ty::Adt(def_id, substs1) => match k {
ty::Adt(def_id2, substs2) => {
def_id == def_id2 && {
// Check for cases like `Foo<3>` vs `Foo<4>`, which are hot
// in crates such as `bitmaps` and `nalgebra`.
let differ_by_single_const_arg = if
substs1.len() == 1
&& substs2.len() == 1
&& let ty::subst::GenericArgKind::Const(c1) = substs1[0].unpack()
&& let ty::subst::GenericArgKind::Const(c2) = substs2[0].unpack()
&& let Some(s1) = c1.val().try_to_scalar_int()
&& let Some(s2) = c2.val().try_to_scalar_int()
&& s1 != s2
Comment on lines +234 to +240
Copy link
Contributor

Choose a reason for hiding this comment

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

please iterate over all generic arguments here.

For types, you can again call tys_may_be_equalivalent while for constant you can also return false for ConstKind::Param on the lhs and anything apart from Param | Unevaluated on the rhs.

I think it makes sense to add fn generic_arg_may_be_equivalent for that

{
true
} else {
false
};
!differ_by_single_const_arg
}
}
_ => false,
},
ty::Str => matches!(k, ty::Str),
ty::Array(..) => matches!(k, ty::Array(..)),
Copy link
Contributor

Choose a reason for hiding this comment

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

you can recurse into the element type and length here

ty::Slice(..) => matches!(k, ty::Slice(..)),
Copy link
Contributor

Choose a reason for hiding this comment

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

elem type

ty::RawPtr(ptr) => matches!(k, ty::RawPtr(ptr2) if ptr.mutbl == ptr2.mutbl),
Copy link
Contributor

Choose a reason for hiding this comment

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

target ty

ty::Dynamic(trait_info, ..) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

you can also recurse into that one, though iirc that requires you to first sort the existential predicates of the trait object, which might not be worth it

matches!(k, ty::Dynamic(trait_info2, ..) if
trait_info.principal_def_id() == trait_info2.principal_def_id()
)
}
ty::Ref(_, _, mutbl) => matches!(k, ty::Ref(_, _, mutbl2) if mutbl == mutbl2),
Copy link
Contributor

Choose a reason for hiding this comment

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

target ty

ty::FnDef(def_id, _) => matches!(k, ty::FnDef(def_id2, _) if def_id == def_id2),
Copy link
Contributor

Choose a reason for hiding this comment

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

... for all other types

ty::Closure(def_id, _) => matches!(k, ty::Closure(def_id2, _) if def_id == def_id2),
ty::Generator(def_id, ..) => {
matches!(k, ty::Generator(def_id2, ..) if def_id == def_id2)
}
ty::Never => matches!(k, ty::Never),
ty::Tuple(tys) => matches!(k, ty::Tuple(tys2) if tys.len() == tys2.len()),
ty::FnPtr(tys) => {
matches!(
k,
ty::FnPtr(tys2)
if tys.skip_binder().inputs().len() == tys2.skip_binder().inputs().len()
)
}
ty::Opaque(def_id, _) => matches!(k, ty::Opaque(def_id2, _) if def_id == def_id2),
ty::Foreign(def_id) => matches!(k, ty::Foreign(def_id2) if def_id == def_id2),
ty::Param(_) => {
// Note, we simplify parameters for the obligation but not the impl
// so that we do not reject a blanket impl but do reject more
// concrete impls if we're searching for `T: Trait`.
matches!(k, ty::Placeholder(..))
}
Comment on lines +277 to +282
Copy link
Contributor

Choose a reason for hiding this comment

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

this can just be

(ty::Param(_), _) => false,

parameters only unify with themselves (or inference vars/projections which get normalized/inferred to that param)

ty::Projection(_) => {
// When treating `ty::Param` as a placeholder, projections also
// don't unify with anything else as long as they are fully normalized.
//
// We will have to be careful with lazy normalization here.
oblig_ty.has_infer_types_or_consts() || matches!(k, ty::Placeholder(..))
}
Comment on lines +283 to +289
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

(ty::Projection(_), _) => oblig_ty.has_infer_types_or_consts(),

ty::Infer(_) => true,

ty::GeneratorWitness(..) | ty::Placeholder(..) | ty::Bound(..) | ty::Error(_) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

surprised that ty::Placeholder is unreachable here, that variant should be reachable afaik

unreachable!()
}
};

// Next, if necessary, look for less obvious sources of equivalence.
maybe_equiv = maybe_equiv
|| match impl_ty.kind() {
ty::Bool
| ty::Char
| ty::Int(_)
| ty::Uint(_)
| ty::Float(_)
| ty::Adt(..)
| ty::Str
| ty::Array(..)
| ty::Slice(..)
| ty::RawPtr(..)
| ty::Dynamic(..)
| ty::Ref(..)
| ty::Never
| ty::Tuple(..)
| ty::FnPtr(..)
| ty::Foreign(..) => false,

ty::Param(_) | ty::Projection(_) | ty::Error(_) => true,

ty::FnDef(..)
| ty::Closure(..)
| ty::Generator(..)
| ty::GeneratorWitness(..)
| ty::Placeholder(..)
| ty::Opaque(..)
| ty::Bound(..)
| ty::Infer(_) => unreachable!(),
};

maybe_equiv
}

impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
pub fn new(infcx: &'cx InferCtxt<'cx, 'tcx>) -> SelectionContext<'cx, 'tcx> {
SelectionContext {
Expand Down Expand Up @@ -2142,20 +2257,8 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
|(obligation_arg, impl_arg)| {
match (obligation_arg.unpack(), impl_arg.unpack()) {
(GenericArgKind::Type(obligation_ty), GenericArgKind::Type(impl_ty)) => {
// Note, we simplify parameters for the obligation but not the
// impl so that we do not reject a blanket impl but do reject
// more concrete impls if we're searching for `T: Trait`.
let simplified_obligation_ty = fast_reject::simplify_type(
self.tcx(),
obligation_ty,
TreatParams::AsPlaceholder,
);
let simplified_impl_ty =
fast_reject::simplify_type(self.tcx(), impl_ty, TreatParams::AsInfer);

simplified_obligation_ty.is_some()
&& simplified_impl_ty.is_some()
&& simplified_obligation_ty != simplified_impl_ty
let tys_definitely_differ = !tys_may_be_equivalent(obligation_ty, impl_ty);
tys_definitely_differ
}
(GenericArgKind::Lifetime(_), GenericArgKind::Lifetime(_)) => {
// Lifetimes can never cause a rejection.
Expand Down