From a106c69d4c4e96741eb682b9f42f7c85eb6be280 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Fri, 30 Dec 2022 01:15:04 +0200 Subject: [PATCH 1/3] fix infinite recursion on complex traits The code previously only checked for cases like $0: Eq, $1: Eq, etc. Now it also works in cases where the type variables are buried in some data structure, like Union<$0, $1>: Eq. --- .../src/traits/select/mod.rs | 27 ++++++++----------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/compiler/rustc_trait_selection/src/traits/select/mod.rs b/compiler/rustc_trait_selection/src/traits/select/mod.rs index 81e8f9e914c23..f9caee6a3e43a 100644 --- a/compiler/rustc_trait_selection/src/traits/select/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/select/mod.rs @@ -1123,29 +1123,24 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { // unbound variable. When we evaluate this trait-reference, we // will unify `$0` with `Vec<$1>` (for some fresh variable // `$1`), on the condition that `$1 : Eq`. We will then wind - // up with many candidates (since that are other `Eq` impls + // up with many candidates (since there are other `Eq` impls // that apply) and try to winnow things down. This results in // a recursive evaluation that `$1 : Eq` -- as you can // imagine, this is just where we started. To avoid that, we - // check for unbound variables and return an ambiguous (hence possible) - // match if we've seen this trait before. + // check if the stack contains an obligation that is identical + // up to fresh variables. // // This suffices to allow chains like `FnMut` implemented in // terms of `Fn` etc, but we could probably make this more // precise still. - let unbound_input_types = - stack.fresh_trait_pred.skip_binder().trait_ref.substs.types().any(|ty| ty.is_fresh()); - - if unbound_input_types - && stack.iter().skip(1).any(|prev| { - stack.obligation.param_env == prev.obligation.param_env - && self.match_fresh_trait_refs( - stack.fresh_trait_pred, - prev.fresh_trait_pred, - prev.obligation.param_env, - ) - }) - { + if stack.iter().skip(1).any(|prev| { + stack.obligation.param_env == prev.obligation.param_env + && self.match_fresh_trait_refs( + stack.fresh_trait_pred, + prev.fresh_trait_pred, + prev.obligation.param_env, + ) + }) { debug!("evaluate_stack --> unbound argument, recursive --> giving up",); return Ok(EvaluatedToUnknown); } From 3d3a2b0b4712be98f6ae496ef36a68e960987cc9 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Fri, 30 Dec 2022 02:19:15 +0200 Subject: [PATCH 2/3] actually require that only typevars differ --- .../src/traits/select/mod.rs | 7 +- .../src/traits/select/only_fresh_differ.rs | 116 ++++++++++++++++++ 2 files changed, 120 insertions(+), 3 deletions(-) create mode 100644 compiler/rustc_trait_selection/src/traits/select/only_fresh_differ.rs diff --git a/compiler/rustc_trait_selection/src/traits/select/mod.rs b/compiler/rustc_trait_selection/src/traits/select/mod.rs index f9caee6a3e43a..cd4727d3bb12b 100644 --- a/compiler/rustc_trait_selection/src/traits/select/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/select/mod.rs @@ -59,6 +59,7 @@ use rustc_middle::ty::print::with_no_trimmed_paths; mod candidate_assembly; mod confirmation; +mod only_fresh_differ; #[derive(Clone, Debug, Eq, PartialEq, Hash)] pub enum IntercrateAmbiguityCause { @@ -1135,7 +1136,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { // precise still. if stack.iter().skip(1).any(|prev| { stack.obligation.param_env == prev.obligation.param_env - && self.match_fresh_trait_refs( + && self.only_typevars_differ( stack.fresh_trait_pred, prev.fresh_trait_pred, prev.obligation.param_env, @@ -2452,13 +2453,13 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { /////////////////////////////////////////////////////////////////////////// // Miscellany - fn match_fresh_trait_refs( + fn only_typevars_differ( &self, previous: ty::PolyTraitPredicate<'tcx>, current: ty::PolyTraitPredicate<'tcx>, param_env: ty::ParamEnv<'tcx>, ) -> bool { - let mut matcher = ty::_match::Match::new(self.tcx(), param_env); + let mut matcher = only_fresh_differ::FreshDiffer::new(self.tcx(), param_env); matcher.relate(previous, current).is_ok() } diff --git a/compiler/rustc_trait_selection/src/traits/select/only_fresh_differ.rs b/compiler/rustc_trait_selection/src/traits/select/only_fresh_differ.rs new file mode 100644 index 0000000000000..e2f886c6b8c69 --- /dev/null +++ b/compiler/rustc_trait_selection/src/traits/select/only_fresh_differ.rs @@ -0,0 +1,116 @@ +use rustc_middle::ty::error::TypeError; +use rustc_middle::ty::relate::{self, Relate, RelateResult, TypeRelation}; +use rustc_middle::ty::{self, InferConst, Ty, TyCtxt}; + +/// Requires that the types can be unified by substituting fresh variables +/// for fresh variable only. +pub struct FreshDiffer<'tcx> { + tcx: TyCtxt<'tcx>, + param_env: ty::ParamEnv<'tcx>, +} + +impl<'tcx> FreshDiffer<'tcx> { + pub fn new(tcx: TyCtxt<'tcx>, param_env: ty::ParamEnv<'tcx>) -> Self { + Self { tcx, param_env } + } +} + +impl<'tcx> TypeRelation<'tcx> for FreshDiffer<'tcx> { + fn tag(&self) -> &'static str { + "FreshDiffer" + } + fn tcx(&self) -> TyCtxt<'tcx> { + self.tcx + } + + fn intercrate(&self) -> bool { + false + } + + fn param_env(&self) -> ty::ParamEnv<'tcx> { + self.param_env + } + fn a_is_expected(&self) -> bool { + true + } // irrelevant + + fn mark_ambiguous(&mut self) { + bug!() + } + + fn relate_with_variance>( + &mut self, + _: ty::Variance, + _: ty::VarianceDiagInfo<'tcx>, + a: T, + b: T, + ) -> RelateResult<'tcx, T> { + self.relate(a, b) + } + + #[instrument(skip(self), level = "debug")] + fn regions( + &mut self, + a: ty::Region<'tcx>, + b: ty::Region<'tcx>, + ) -> RelateResult<'tcx, ty::Region<'tcx>> { + Ok(a) + } + + #[instrument(skip(self), level = "debug")] + fn tys(&mut self, a: Ty<'tcx>, b: Ty<'tcx>) -> RelateResult<'tcx, Ty<'tcx>> { + if a == b { + return Ok(a); + } + + match (a.kind(), b.kind()) { + (&ty::Infer(ty::FreshTy(_)), &ty::Infer(ty::FreshTy(_))) + | (&ty::Infer(ty::FreshIntTy(_)), &ty::Infer(ty::FreshIntTy(_))) + | (&ty::Infer(ty::FreshFloatTy(_)), &ty::Infer(ty::FreshFloatTy(_))) => Ok(a), + + (&ty::Infer(_), _) | (_, &ty::Infer(_)) => { + Err(TypeError::Sorts(relate::expected_found(self, a, b))) + } + + (&ty::Error(_), _) | (_, &ty::Error(_)) => Ok(self.tcx().ty_error()), + + _ => relate::super_relate_tys(self, a, b), + } + } + + fn consts( + &mut self, + a: ty::Const<'tcx>, + b: ty::Const<'tcx>, + ) -> RelateResult<'tcx, ty::Const<'tcx>> { + debug!("{}.consts({:?}, {:?})", self.tag(), a, b); + if a == b { + return Ok(a); + } + + match (a.kind(), b.kind()) { + (_, ty::ConstKind::Infer(InferConst::Fresh(_))) => { + return Ok(a); + } + + (ty::ConstKind::Infer(_), _) | (_, ty::ConstKind::Infer(_)) => { + return Err(TypeError::ConstMismatch(relate::expected_found(self, a, b))); + } + + _ => {} + } + + relate::super_relate_consts(self, a, b) + } + + fn binders( + &mut self, + a: ty::Binder<'tcx, T>, + b: ty::Binder<'tcx, T>, + ) -> RelateResult<'tcx, ty::Binder<'tcx, T>> + where + T: Relate<'tcx>, + { + Ok(a.rebind(self.relate(a.skip_binder(), b.skip_binder())?)) + } +} From 2961d86993f0477cdf774f568b5d581f853fd447 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Fri, 30 Dec 2022 20:27:53 +0200 Subject: [PATCH 3/3] use old AND new rule to detect infinite recursion --- .../src/traits/select/mod.rs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_trait_selection/src/traits/select/mod.rs b/compiler/rustc_trait_selection/src/traits/select/mod.rs index cd4727d3bb12b..56c3fd71aedf7 100644 --- a/compiler/rustc_trait_selection/src/traits/select/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/select/mod.rs @@ -1136,7 +1136,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { // precise still. if stack.iter().skip(1).any(|prev| { stack.obligation.param_env == prev.obligation.param_env - && self.only_typevars_differ( + && self.is_repetition( stack.fresh_trait_pred, prev.fresh_trait_pred, prev.obligation.param_env, @@ -2453,14 +2453,21 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { /////////////////////////////////////////////////////////////////////////// // Miscellany - fn only_typevars_differ( + fn is_repetition( &self, - previous: ty::PolyTraitPredicate<'tcx>, - current: ty::PolyTraitPredicate<'tcx>, + new: ty::PolyTraitPredicate<'tcx>, + old: ty::PolyTraitPredicate<'tcx>, param_env: ty::ParamEnv<'tcx>, ) -> bool { let mut matcher = only_fresh_differ::FreshDiffer::new(self.tcx(), param_env); - matcher.relate(previous, current).is_ok() + if matcher.relate(new, old).is_ok() { + return true; + } + + let mut matcher = ty::_match::Match::new(self.tcx(), param_env); + + new.skip_binder().trait_ref.substs.types().any(|ty| ty.is_fresh()) + && matcher.relate(new, old).is_ok() } fn push_stack<'o>(