From 5344ed23fadce0bd2af6a50cb4623704b1fe6383 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Wed, 21 Jun 2023 05:32:35 +0000 Subject: [PATCH 1/2] Don't substitute a GAT that has mismatched generics in OpaqueTypeCollector --- .../rustc_trait_selection/src/traits/mod.rs | 8 +- .../src/traits/project.rs | 42 +--------- .../rustc_trait_selection/src/traits/util.rs | 41 ++++++++++ compiler/rustc_ty_utils/src/opaque_types.rs | 80 ++++++++++++------- ...mpl-trait-in-type-alias-with-bad-substs.rs | 28 +++++++ ...trait-in-type-alias-with-bad-substs.stderr | 20 +++++ 6 files changed, 144 insertions(+), 75 deletions(-) create mode 100644 tests/ui/type-alias-impl-trait/impl-trait-in-type-alias-with-bad-substs.rs create mode 100644 tests/ui/type-alias-impl-trait/impl-trait-in-type-alias-with-bad-substs.stderr diff --git a/compiler/rustc_trait_selection/src/traits/mod.rs b/compiler/rustc_trait_selection/src/traits/mod.rs index 15081d6568299..a5481714e3e5e 100644 --- a/compiler/rustc_trait_selection/src/traits/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/mod.rs @@ -65,12 +65,12 @@ pub use self::specialize::{ pub use self::structural_match::search_for_structural_match_violation; pub use self::structural_normalize::StructurallyNormalizeExt; pub use self::util::elaborate; -pub use self::util::{expand_trait_aliases, TraitAliasExpander}; -pub use self::util::{get_vtable_index_of_object_method, impl_item_is_final, upcast_choices}; pub use self::util::{ - supertrait_def_ids, supertraits, transitive_bounds, transitive_bounds_that_define_assoc_item, - SupertraitDefIds, + check_substs_compatible, supertrait_def_ids, supertraits, transitive_bounds, + transitive_bounds_that_define_assoc_item, SupertraitDefIds, }; +pub use self::util::{expand_trait_aliases, TraitAliasExpander}; +pub use self::util::{get_vtable_index_of_object_method, impl_item_is_final, upcast_choices}; pub use self::chalk_fulfill::FulfillmentContext as ChalkFulfillmentContext; diff --git a/compiler/rustc_trait_selection/src/traits/project.rs b/compiler/rustc_trait_selection/src/traits/project.rs index de76587c7b954..81b1ecc0d8819 100644 --- a/compiler/rustc_trait_selection/src/traits/project.rs +++ b/compiler/rustc_trait_selection/src/traits/project.rs @@ -1,5 +1,6 @@ //! Code for projecting associated types out of trait references. +use super::check_substs_compatible; use super::specialization_graph; use super::translate_substs; use super::util; @@ -2378,47 +2379,6 @@ fn confirm_impl_candidate<'cx, 'tcx>( } } -// Verify that the trait item and its implementation have compatible substs lists -fn check_substs_compatible<'tcx>( - tcx: TyCtxt<'tcx>, - assoc_item: ty::AssocItem, - substs: ty::SubstsRef<'tcx>, -) -> bool { - fn check_substs_compatible_inner<'tcx>( - tcx: TyCtxt<'tcx>, - generics: &'tcx ty::Generics, - args: &'tcx [ty::GenericArg<'tcx>], - ) -> bool { - if generics.count() != args.len() { - return false; - } - - let (parent_args, own_args) = args.split_at(generics.parent_count); - - if let Some(parent) = generics.parent - && let parent_generics = tcx.generics_of(parent) - && !check_substs_compatible_inner(tcx, parent_generics, parent_args) { - return false; - } - - for (param, arg) in std::iter::zip(&generics.params, own_args) { - match (¶m.kind, arg.unpack()) { - (ty::GenericParamDefKind::Type { .. }, ty::GenericArgKind::Type(_)) - | (ty::GenericParamDefKind::Lifetime, ty::GenericArgKind::Lifetime(_)) - | (ty::GenericParamDefKind::Const { .. }, ty::GenericArgKind::Const(_)) => {} - _ => return false, - } - } - - true - } - - let generics = tcx.generics_of(assoc_item.def_id); - // Chop off any additional substs (RPITIT) substs - let substs = &substs[0..generics.count().min(substs.len())]; - check_substs_compatible_inner(tcx, generics, substs) -} - fn confirm_impl_trait_in_trait_candidate<'tcx>( selcx: &mut SelectionContext<'_, 'tcx>, obligation: &ProjectionTyObligation<'tcx>, diff --git a/compiler/rustc_trait_selection/src/traits/util.rs b/compiler/rustc_trait_selection/src/traits/util.rs index 906c357e8ca7e..05a7f3e3b024a 100644 --- a/compiler/rustc_trait_selection/src/traits/util.rs +++ b/compiler/rustc_trait_selection/src/traits/util.rs @@ -302,3 +302,44 @@ pub enum TupleArgumentsFlag { Yes, No, } + +// Verify that the trait item and its implementation have compatible substs lists +pub fn check_substs_compatible<'tcx>( + tcx: TyCtxt<'tcx>, + assoc_item: ty::AssocItem, + substs: ty::SubstsRef<'tcx>, +) -> bool { + fn check_substs_compatible_inner<'tcx>( + tcx: TyCtxt<'tcx>, + generics: &'tcx ty::Generics, + args: &'tcx [ty::GenericArg<'tcx>], + ) -> bool { + if generics.count() != args.len() { + return false; + } + + let (parent_args, own_args) = args.split_at(generics.parent_count); + + if let Some(parent) = generics.parent + && let parent_generics = tcx.generics_of(parent) + && !check_substs_compatible_inner(tcx, parent_generics, parent_args) { + return false; + } + + for (param, arg) in std::iter::zip(&generics.params, own_args) { + match (¶m.kind, arg.unpack()) { + (ty::GenericParamDefKind::Type { .. }, ty::GenericArgKind::Type(_)) + | (ty::GenericParamDefKind::Lifetime, ty::GenericArgKind::Lifetime(_)) + | (ty::GenericParamDefKind::Const { .. }, ty::GenericArgKind::Const(_)) => {} + _ => return false, + } + } + + true + } + + let generics = tcx.generics_of(assoc_item.def_id); + // Chop off any additional substs (RPITIT) substs + let substs = &substs[0..generics.count().min(substs.len())]; + check_substs_compatible_inner(tcx, generics, substs) +} diff --git a/compiler/rustc_ty_utils/src/opaque_types.rs b/compiler/rustc_ty_utils/src/opaque_types.rs index 4e91dd380e865..34c7b9f445182 100644 --- a/compiler/rustc_ty_utils/src/opaque_types.rs +++ b/compiler/rustc_ty_utils/src/opaque_types.rs @@ -6,7 +6,7 @@ use rustc_middle::ty::util::{CheckRegions, NotUniqueParam}; use rustc_middle::ty::{self, Ty, TyCtxt}; use rustc_middle::ty::{TypeSuperVisitable, TypeVisitable, TypeVisitor}; use rustc_span::Span; -use rustc_type_ir::AliasKind; +use rustc_trait_selection::traits::check_substs_compatible; use std::ops::ControlFlow; use crate::errors::{DuplicateArg, NotParam}; @@ -36,6 +36,15 @@ impl<'tcx> OpaqueTypeCollector<'tcx> { self.tcx.def_span(self.item) } + fn parent_trait_ref(&self) -> Option> { + let parent = self.parent()?; + if matches!(self.tcx.def_kind(parent), DefKind::Impl { .. }) { + Some(self.tcx.impl_trait_ref(parent)?.subst_identity()) + } else { + None + } + } + fn parent(&self) -> Option { match self.tcx.def_kind(self.item) { DefKind::Fn => None, @@ -56,7 +65,7 @@ impl<'tcx> TypeVisitor> for OpaqueTypeCollector<'tcx> { #[instrument(skip(self), ret, level = "trace")] fn visit_ty(&mut self, t: Ty<'tcx>) -> ControlFlow { match t.kind() { - ty::Alias(AliasKind::Opaque, alias_ty) if alias_ty.def_id.is_local() => { + ty::Alias(ty::Opaque, alias_ty) if alias_ty.def_id.is_local() => { if !self.seen.insert(alias_ty.def_id.expect_local()) { return ControlFlow::Continue(()); } @@ -98,37 +107,48 @@ impl<'tcx> TypeVisitor> for OpaqueTypeCollector<'tcx> { } } } - ty::Alias(AliasKind::Projection, alias_ty) => { - if let Some(parent) = self.parent() { - trace!(?alias_ty); - let (trait_ref, own_substs) = alias_ty.trait_ref_and_own_substs(self.tcx); - - trace!(?trait_ref, ?own_substs); - // This avoids having to do normalization of `Self::AssocTy` by only - // supporting the case of a method defining opaque types from assoc types - // in the same impl block. - if trait_ref.self_ty() == self.tcx.type_of(parent).subst_identity() { - for assoc in self.tcx.associated_items(parent).in_definition_order() { + ty::Alias(ty::Projection, alias_ty) => { + // This avoids having to do normalization of `Self::AssocTy` by only + // supporting the case of a method defining opaque types from assoc types + // in the same impl block. + if let Some(parent_trait_ref) = self.parent_trait_ref() { + // If the trait ref of the associated item and the impl differs, + // then we can't use the impl's identity substitutions below, so + // just skip. + if alias_ty.trait_ref(self.tcx) == parent_trait_ref { + let parent = self.parent().expect("we should have a parent here"); + + for &assoc in self.tcx.associated_items(parent).in_definition_order() { trace!(?assoc); - if assoc.trait_item_def_id == Some(alias_ty.def_id) { - // We reconstruct the generic args of the associated type within the impl - // from the impl's generics and the generic args passed to the type via the - // projection. - let substs = ty::InternalSubsts::identity_for_item( - self.tcx, - parent.to_def_id(), + if assoc.trait_item_def_id != Some(alias_ty.def_id) { + continue; + } + + // If the type is further specializable, then the type_of + // is not actually correct below. + if !assoc.defaultness(self.tcx).is_final() { + continue; + } + + let impl_substs = alias_ty.substs.rebase_onto( + self.tcx, + parent_trait_ref.def_id, + ty::InternalSubsts::identity_for_item(self.tcx, parent), + ); + + if !check_substs_compatible(self.tcx, assoc, impl_substs) { + self.tcx.sess.delay_span_bug( + self.tcx.def_span(assoc.def_id), + "item had incorrect substs", ); - trace!(?substs); - let substs: Vec<_> = - substs.iter().chain(own_substs.iter().copied()).collect(); - trace!(?substs); - // Find opaque types in this associated type. - return self - .tcx - .type_of(assoc.def_id) - .subst(self.tcx, &substs) - .visit_with(self); + return ControlFlow::Continue(()); } + + return self + .tcx + .type_of(assoc.def_id) + .subst(self.tcx, impl_substs) + .visit_with(self); } } } diff --git a/tests/ui/type-alias-impl-trait/impl-trait-in-type-alias-with-bad-substs.rs b/tests/ui/type-alias-impl-trait/impl-trait-in-type-alias-with-bad-substs.rs new file mode 100644 index 0000000000000..a3f65146f75fb --- /dev/null +++ b/tests/ui/type-alias-impl-trait/impl-trait-in-type-alias-with-bad-substs.rs @@ -0,0 +1,28 @@ +#![feature(impl_trait_in_assoc_type)] + +// We weren't checking that the trait and impl generics line up in the +// normalization-shortcut code in `OpaqueTypeCollector`. + +use std::ops::Deref; + +trait Foo { + type Bar<'a>; + + type Baz<'a>; + + fn test<'a>() -> Self::Bar<'a>; +} + +impl Foo for () { + type Bar<'a> = impl Deref>; + + type Baz = impl Sized; + //~^ ERROR type `Baz` has 1 type parameter but its trait declaration has 0 type parameters + //~| ERROR unconstrained opaque type + + fn test<'a>() -> Self::Bar<'a> { + &() + } +} + +fn main() {} diff --git a/tests/ui/type-alias-impl-trait/impl-trait-in-type-alias-with-bad-substs.stderr b/tests/ui/type-alias-impl-trait/impl-trait-in-type-alias-with-bad-substs.stderr new file mode 100644 index 0000000000000..13f5d8b8ea6e2 --- /dev/null +++ b/tests/ui/type-alias-impl-trait/impl-trait-in-type-alias-with-bad-substs.stderr @@ -0,0 +1,20 @@ +error[E0049]: type `Baz` has 1 type parameter but its trait declaration has 0 type parameters + --> $DIR/impl-trait-in-type-alias-with-bad-substs.rs:19:14 + | +LL | type Baz<'a>; + | -- expected 0 type parameters +... +LL | type Baz = impl Sized; + | ^ found 1 type parameter + +error: unconstrained opaque type + --> $DIR/impl-trait-in-type-alias-with-bad-substs.rs:19:19 + | +LL | type Baz = impl Sized; + | ^^^^^^^^^^ + | + = note: `Baz` must be used in combination with a concrete type within the same impl + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0049`. From c4cd6071007d12867112eb2a9ae10d07332d1d9a Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Wed, 21 Jun 2023 16:41:23 +0000 Subject: [PATCH 2/2] Additional test demonstrating check for full trait ref --- .../not-matching-trait-refs-isnt-defining.rs | 33 +++++++++++++++++++ ...t-matching-trait-refs-isnt-defining.stderr | 22 +++++++++++++ 2 files changed, 55 insertions(+) create mode 100644 tests/ui/type-alias-impl-trait/not-matching-trait-refs-isnt-defining.rs create mode 100644 tests/ui/type-alias-impl-trait/not-matching-trait-refs-isnt-defining.stderr diff --git a/tests/ui/type-alias-impl-trait/not-matching-trait-refs-isnt-defining.rs b/tests/ui/type-alias-impl-trait/not-matching-trait-refs-isnt-defining.rs new file mode 100644 index 0000000000000..131f8d999d878 --- /dev/null +++ b/tests/ui/type-alias-impl-trait/not-matching-trait-refs-isnt-defining.rs @@ -0,0 +1,33 @@ +#![feature(impl_trait_in_assoc_type)] + +trait Foo { + type Assoc; + + fn test() -> u32; +} + +struct DefinesOpaque; +impl Foo for () { + type Assoc = impl Sized; + + // This test's return type is `u32`, *not* the opaque that is defined above. + // Previously we were only checking that the self type of the assoc matched, + // but this doesn't account for other impls with different trait substs. + fn test() -> <() as Foo>::Assoc { + let _: >::Assoc = ""; + //~^ ERROR mismatched types + + 1 + } +} + +struct NoOpaques; +impl Foo for () { + type Assoc = u32; + + fn test() -> u32 { + 1 + } +} + +fn main() {} diff --git a/tests/ui/type-alias-impl-trait/not-matching-trait-refs-isnt-defining.stderr b/tests/ui/type-alias-impl-trait/not-matching-trait-refs-isnt-defining.stderr new file mode 100644 index 0000000000000..d2d007490915b --- /dev/null +++ b/tests/ui/type-alias-impl-trait/not-matching-trait-refs-isnt-defining.stderr @@ -0,0 +1,22 @@ +error[E0308]: mismatched types + --> $DIR/not-matching-trait-refs-isnt-defining.rs:17:54 + | +LL | type Assoc = impl Sized; + | ---------- the expected opaque type +... +LL | let _: >::Assoc = ""; + | ----------------------------------- ^^ expected opaque type, found `&str` + | | + | expected due to this + | + = note: expected opaque type `<() as Foo>::Assoc` + found reference `&'static str` +note: this item must have the opaque type in its signature in order to be able to register hidden types + --> $DIR/not-matching-trait-refs-isnt-defining.rs:16:5 + | +LL | fn test() -> <() as Foo>::Assoc { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0308`.