From 265c75c53ffbc2c4cd13014e958af6e2f743e9bf Mon Sep 17 00:00:00 2001 From: Ryo Yoshida Date: Mon, 5 Sep 2022 18:35:50 +0900 Subject: [PATCH] fix: sort all bounds on trait object types --- crates/hir-ty/src/chalk_ext.rs | 2 + crates/hir-ty/src/lower.rs | 73 +++++++++++++++++++++---------- crates/hir-ty/src/tests/traits.rs | 28 ++++++++++++ 3 files changed, 81 insertions(+), 22 deletions(-) diff --git a/crates/hir-ty/src/chalk_ext.rs b/crates/hir-ty/src/chalk_ext.rs index a9c124b42d..4a5533c648 100644 --- a/crates/hir-ty/src/chalk_ext.rs +++ b/crates/hir-ty/src/chalk_ext.rs @@ -164,6 +164,8 @@ impl TyExt for Ty { fn dyn_trait(&self) -> Option { let trait_ref = match self.kind(Interner) { + // The principal trait bound should be the first element of the bounds. This is an + // invariant ensured by `TyLoweringContext::lower_dyn_trait()`. TyKind::Dyn(dyn_ty) => dyn_ty.bounds.skip_binders().interned().get(0).and_then(|b| { match b.skip_binders() { WhereClause::Implemented(trait_ref) => Some(trait_ref), diff --git a/crates/hir-ty/src/lower.rs b/crates/hir-ty/src/lower.rs index 4a37a79453..532544fee5 100644 --- a/crates/hir-ty/src/lower.rs +++ b/crates/hir-ty/src/lower.rs @@ -981,43 +981,72 @@ impl<'a> TyLoweringContext<'a> { fn lower_dyn_trait(&self, bounds: &[Interned]) -> Ty { let self_ty = TyKind::BoundVar(BoundVar::new(DebruijnIndex::INNERMOST, 0)).intern(Interner); + // INVARIANT: The principal trait bound must come first. Others may be in any order but + // should be in the same order for the same set but possibly different order of bounds in + // the input. + // This invariant is used by `TyExt::dyn_trait()` and chalk. let bounds = self.with_shifted_in(DebruijnIndex::ONE, |ctx| { - let bounds = - bounds.iter().flat_map(|b| ctx.lower_type_bound(b, self_ty.clone(), false)); + let mut bounds: Vec<_> = bounds + .iter() + .flat_map(|b| ctx.lower_type_bound(b, self_ty.clone(), false)) + .collect(); - let mut auto_traits = SmallVec::<[_; 8]>::new(); - let mut regular_traits = SmallVec::<[_; 2]>::new(); - let mut other_bounds = SmallVec::<[_; 8]>::new(); - for bound in bounds { - if let Some(id) = bound.trait_id() { - if ctx.db.trait_data(from_chalk_trait_id(id)).is_auto { - auto_traits.push(bound); - } else { - regular_traits.push(bound); + let mut multiple_regular_traits = false; + let mut multiple_same_projection = false; + bounds.sort_unstable_by(|lhs, rhs| { + use std::cmp::Ordering; + match (lhs.skip_binders(), rhs.skip_binders()) { + (WhereClause::Implemented(lhs), WhereClause::Implemented(rhs)) => { + let lhs_id = lhs.trait_id; + let lhs_is_auto = ctx.db.trait_data(from_chalk_trait_id(lhs_id)).is_auto; + let rhs_id = rhs.trait_id; + let rhs_is_auto = ctx.db.trait_data(from_chalk_trait_id(rhs_id)).is_auto; + + if !lhs_is_auto && !rhs_is_auto { + multiple_regular_traits = true; + } + // Note that the ordering here is important; this ensures the invariant + // mentioned above. + (lhs_is_auto, lhs_id).cmp(&(rhs_is_auto, rhs_id)) } - } else { - other_bounds.push(bound); + (WhereClause::Implemented(_), _) => Ordering::Less, + (_, WhereClause::Implemented(_)) => Ordering::Greater, + (WhereClause::AliasEq(lhs), WhereClause::AliasEq(rhs)) => { + match (&lhs.alias, &rhs.alias) { + (AliasTy::Projection(lhs_proj), AliasTy::Projection(rhs_proj)) => { + // We only compare the `associated_ty_id`s. We shouldn't have + // multiple bounds for an associated type in the correct Rust code, + // and if we do, we error out. + if lhs_proj.associated_ty_id == rhs_proj.associated_ty_id { + multiple_same_projection = true; + } + lhs_proj.associated_ty_id.cmp(&rhs_proj.associated_ty_id) + } + // We don't produce `AliasTy::Opaque`s yet. + _ => unreachable!(), + } + } + // We don't produce `WhereClause::{TypeOutlives, LifetimeOutlives}` yet. + _ => unreachable!(), } - } + }); - if regular_traits.len() > 1 { + if multiple_regular_traits || multiple_same_projection { return None; } - auto_traits.sort_unstable_by_key(|b| b.trait_id().unwrap()); - auto_traits.dedup(); + // As multiple occurrences of the same auto traits *are* permitted, we dedulicate the + // bounds. We shouldn't have repeated elements besides auto traits at this point. + bounds.dedup(); - Some(QuantifiedWhereClauses::from_iter( - Interner, - regular_traits.into_iter().chain(other_bounds).chain(auto_traits), - )) + Some(QuantifiedWhereClauses::from_iter(Interner, bounds)) }); if let Some(bounds) = bounds { let bounds = crate::make_single_type_binders(bounds); TyKind::Dyn(DynTy { bounds, lifetime: static_lifetime() }).intern(Interner) } else { - // FIXME: report error (additional non-auto traits) + // FIXME: report error (additional non-auto traits or associated type rebound) TyKind::Error.intern(Interner) } } diff --git a/crates/hir-ty/src/tests/traits.rs b/crates/hir-ty/src/tests/traits.rs index e67c27aa2d..21a8631976 100644 --- a/crates/hir-ty/src/tests/traits.rs +++ b/crates/hir-ty/src/tests/traits.rs @@ -3900,6 +3900,34 @@ fn g(t: &(dyn Sync + T + Send)) { ); } +#[test] +fn dyn_multiple_projection_bounds() { + check_no_mismatches( + r#" +trait Trait { + type T; + type U; +} + +fn f(t: &dyn Trait) {} +fn g(t: &dyn Trait) { + f(t); +} + "#, + ); + + check_types( + r#" +trait Trait { + type T; +} + +fn f(t: &dyn Trait) {} + //^&{unknown} + "#, + ); +} + #[test] fn dyn_duplicate_auto_trait() { check_no_mismatches(