Skip to content

Various impl trait in assoc tys cleanups #112891

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

Merged
merged 13 commits into from
Jun 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
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
23 changes: 23 additions & 0 deletions compiler/rustc_hir/src/hir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3743,6 +3743,29 @@ impl<'hir> Node<'hir> {
}
}

/// Get the type for constants, assoc types, type aliases and statics.
pub fn ty(self) -> Option<&'hir Ty<'hir>> {
match self {
Node::Item(it) => match it.kind {
ItemKind::TyAlias(ty, _) | ItemKind::Static(ty, _, _) | ItemKind::Const(ty, _) => {
Some(ty)
}
_ => None,
},
Node::TraitItem(it) => match it.kind {
TraitItemKind::Const(ty, _) => Some(ty),
TraitItemKind::Type(_, ty) => ty,
_ => None,
},
Node::ImplItem(it) => match it.kind {
ImplItemKind::Const(ty, _) => Some(ty),
ImplItemKind::Type(ty) => Some(ty),
_ => None,
},
_ => None,
}
}

pub fn alias_ty(self) -> Option<&'hir Ty<'hir>> {
match self {
Node::Item(Item { kind: ItemKind::TyAlias(ty, ..), .. }) => Some(ty),
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_hir_analysis/src/check/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -563,8 +563,8 @@ fn check_item_type(tcx: TyCtxt<'_>, id: hir::ItemId) {
check_union(tcx, id.owner_id.def_id);
}
DefKind::OpaqueTy => {
let opaque = tcx.hir().expect_item(id.owner_id.def_id).expect_opaque_ty();
if let hir::OpaqueTyOrigin::FnReturn(fn_def_id) | hir::OpaqueTyOrigin::AsyncFn(fn_def_id) = opaque.origin
let origin = tcx.opaque_type_origin(id.owner_id.def_id);
if let hir::OpaqueTyOrigin::FnReturn(fn_def_id) | hir::OpaqueTyOrigin::AsyncFn(fn_def_id) = origin
&& let hir::Node::TraitItem(trait_item) = tcx.hir().get_by_def_id(fn_def_id)
&& let (_, hir::TraitFn::Required(..)) = trait_item.expect_fn()
{
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_hir_analysis/src/check/wfcheck.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1543,8 +1543,8 @@ impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for ImplTraitInTraitFinder<'_, 'tcx> {
if let ty::Alias(ty::Opaque, unshifted_opaque_ty) = *ty.kind()
&& self.seen.insert(unshifted_opaque_ty.def_id)
&& let Some(opaque_def_id) = unshifted_opaque_ty.def_id.as_local()
&& let opaque = tcx.hir().expect_item(opaque_def_id).expect_opaque_ty()
&& let hir::OpaqueTyOrigin::FnReturn(source) | hir::OpaqueTyOrigin::AsyncFn(source) = opaque.origin
&& let origin = tcx.opaque_type_origin(opaque_def_id)
&& let hir::OpaqueTyOrigin::FnReturn(source) | hir::OpaqueTyOrigin::AsyncFn(source) = origin
&& source == self.fn_def_id
{
let opaque_ty = tcx.fold_regions(unshifted_opaque_ty, |re, _depth| {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,8 @@ impl<T> Trait<T> for X {
(ty::Alias(ty::Opaque, alias), _) | (_, ty::Alias(ty::Opaque, alias)) if alias.def_id.is_local() && matches!(tcx.def_kind(body_owner_def_id), DefKind::AssocFn | DefKind::AssocConst) => {
if tcx.is_type_alias_impl_trait(alias.def_id) {
if !tcx.opaque_types_defined_by(body_owner_def_id.expect_local()).contains(&alias.def_id.expect_local()) {
diag.span_note(tcx.def_span(body_owner_def_id), "\
let sp = tcx.def_ident_span(body_owner_def_id).unwrap_or_else(|| tcx.def_span(body_owner_def_id));
diag.span_note(sp, "\
this item must have the opaque type in its signature \
in order to be able to register hidden types");
}
Expand Down
9 changes: 1 addition & 8 deletions compiler/rustc_infer/src/infer/opaque_types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ impl<'tcx> InferCtxt<'tcx> {
DefiningAnchor::Bind(bind) => bind,
};

let origin = self.opaque_type_origin_unchecked(def_id);
let origin = self.tcx.opaque_type_origin(def_id);
let in_definition_scope = match origin {
// Async `impl Trait`
hir::OpaqueTyOrigin::AsyncFn(parent) => parent == parent_def_id,
Expand All @@ -395,13 +395,6 @@ impl<'tcx> InferCtxt<'tcx> {
};
in_definition_scope.then_some(origin)
}

/// Returns the origin of the opaque type `def_id` even if we are not in its
/// defining scope.
#[instrument(skip(self), level = "trace", ret)]
fn opaque_type_origin_unchecked(&self, def_id: LocalDefId) -> OpaqueTyOrigin {
self.tcx.hir().expect_item(def_id).expect_opaque_ty().origin
}
}

/// Visitor that requires that (almost) all regions in the type visited outlive
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_metadata/src/rmeta/encoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1138,8 +1138,8 @@ fn should_encode_type(tcx: TyCtxt<'_>, def_id: LocalDefId, def_kind: DefKind) ->
| DefKind::InlineConst => true,

DefKind::OpaqueTy => {
let opaque = tcx.hir().expect_item(def_id).expect_opaque_ty();
if let hir::OpaqueTyOrigin::FnReturn(fn_def_id) | hir::OpaqueTyOrigin::AsyncFn(fn_def_id) = opaque.origin
let origin = tcx.opaque_type_origin(def_id);
if let hir::OpaqueTyOrigin::FnReturn(fn_def_id) | hir::OpaqueTyOrigin::AsyncFn(fn_def_id) = origin
&& let hir::Node::TraitItem(trait_item) = tcx.hir().get_by_def_id(fn_def_id)
&& let (_, hir::TraitFn::Required(..)) = trait_item.expect_fn()
{
Expand Down
6 changes: 6 additions & 0 deletions compiler/rustc_middle/src/ty/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1189,6 +1189,12 @@ impl<'tcx> TyCtxt<'tcx> {
pub fn local_visibility(self, def_id: LocalDefId) -> Visibility {
self.visibility(def_id).expect_local()
}

/// Returns the origin of the opaque type `def_id`.
#[instrument(skip(self), level = "trace", ret)]
pub fn opaque_type_origin(self, def_id: LocalDefId) -> hir::OpaqueTyOrigin {
self.hir().expect_item(def_id).expect_opaque_ty().origin
}
}

/// A trait implemented for all `X<'a>` types that can be safely and
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_ty_utils/src/assoc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ fn associated_type_for_impl_trait_in_trait(
opaque_ty_def_id: LocalDefId,
) -> LocalDefId {
let (hir::OpaqueTyOrigin::FnReturn(fn_def_id) | hir::OpaqueTyOrigin::AsyncFn(fn_def_id)) =
tcx.hir().expect_item(opaque_ty_def_id).expect_opaque_ty().origin
tcx.opaque_type_origin(opaque_ty_def_id)
else {
bug!("expected opaque for {opaque_ty_def_id:?}");
};
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_ty_utils/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ pub struct DuplicateArg<'tcx> {
}

#[derive(Diagnostic)]
#[diag(ty_utils_impl_trait_not_param)]
#[diag(ty_utils_impl_trait_not_param, code = "E0792")]
pub struct NotParam<'tcx> {
pub arg: GenericArg<'tcx>,
#[primary_span]
Expand Down
130 changes: 83 additions & 47 deletions compiler/rustc_ty_utils/src/opaque_types.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use rustc_data_structures::fx::FxHashSet;
use rustc_errors::ErrorGuaranteed;
use rustc_hir::{def::DefKind, def_id::LocalDefId};
use rustc_middle::query::Providers;
use rustc_middle::ty::util::{CheckRegions, NotUniqueParam};
Expand All @@ -19,21 +18,26 @@ struct OpaqueTypeCollector<'tcx> {

/// Avoid infinite recursion due to recursive declarations.
seen: FxHashSet<LocalDefId>,

span: Option<Span>,
}

impl<'tcx> OpaqueTypeCollector<'tcx> {
fn collect(
tcx: TyCtxt<'tcx>,
item: LocalDefId,
val: ty::Binder<'tcx, impl TypeVisitable<TyCtxt<'tcx>>>,
) -> Vec<LocalDefId> {
let mut collector = Self { tcx, opaques: Vec::new(), item, seen: Default::default() };
val.skip_binder().visit_with(&mut collector);
collector.opaques
fn new(tcx: TyCtxt<'tcx>, item: LocalDefId) -> Self {
Self { tcx, opaques: Vec::new(), item, seen: Default::default(), span: None }
}

fn span(&self) -> Span {
self.tcx.def_span(self.item)
self.span.unwrap_or_else(|| {
self.tcx.def_ident_span(self.item).unwrap_or_else(|| self.tcx.def_span(self.item))
})
}

fn visit_spanned(&mut self, span: Span, value: impl TypeVisitable<TyCtxt<'tcx>>) {
let old = self.span;
self.span = Some(span);
value.visit_with(self);
self.span = old;
}

fn parent_trait_ref(&self) -> Option<ty::TraitRef<'tcx>> {
Expand All @@ -60,53 +64,57 @@ impl<'tcx> OpaqueTypeCollector<'tcx> {
}

impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for OpaqueTypeCollector<'tcx> {
type BreakTy = ErrorGuaranteed;

#[instrument(skip(self), ret, level = "trace")]
fn visit_ty(&mut self, t: Ty<'tcx>) -> ControlFlow<ErrorGuaranteed> {
fn visit_ty(&mut self, t: Ty<'tcx>) -> ControlFlow<!> {
t.super_visit_with(self)?;
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain to me what the material change is in this commit ("Stop failing eagerly, and collect all opaque types even if some are erroneous")?

I don't see any tests associated with it, so is it to just suppress spurious errors or what?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll build a test. It's just so that when an error occurs, we'll still want to treat things as defining scopes instead of following up with a type mismatch error. This would occur before with erroneous uses of opaque types in the defining scope.

Similarly, the first error would hide subsequent correct uses of an (possibly other) opaque type, causing follow up type mismatch errors that don't really help anyone.

match t.kind() {
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(());
}

self.opaques.push(alias_ty.def_id.expect_local());

match self.tcx.uses_unique_generic_params(alias_ty.substs, CheckRegions::Bound) {
Ok(()) => {
// FIXME: implement higher kinded lifetime bounds on nested opaque types. They are not
// supported at all, so this is sound to do, but once we want to support them, you'll
// start seeing the error below.

self.opaques.push(alias_ty.def_id.expect_local());

// Collect opaque types nested within the associated type bounds of this opaque type.
for (pred, _span) in self
// We use identity substs here, because we already know that the opaque type uses
// only generic parameters, and thus substituting would not give us more information.
for (pred, span) in self
.tcx
.explicit_item_bounds(alias_ty.def_id)
.subst_iter_copied(self.tcx, alias_ty.substs)
.subst_identity_iter_copied()
{
trace!(?pred);
pred.visit_with(self)?;
self.visit_spanned(span, pred);
}

ControlFlow::Continue(())
}
Err(NotUniqueParam::NotParam(arg)) => {
let err = self.tcx.sess.emit_err(NotParam {
self.tcx.sess.emit_err(NotParam {
arg,
span: self.span(),
opaque_span: self.tcx.def_span(alias_ty.def_id),
});
ControlFlow::Break(err)
}
Err(NotUniqueParam::DuplicateParam(arg)) => {
let err = self.tcx.sess.emit_err(DuplicateArg {
self.tcx.sess.emit_err(DuplicateArg {
arg,
span: self.span(),
opaque_span: self.tcx.def_span(alias_ty.def_id),
});
ControlFlow::Break(err)
}
}
}
ty::Alias(ty::Weak, alias_ty) if alias_ty.def_id.is_local() => {
self.tcx
.type_of(alias_ty.def_id)
.subst(self.tcx, alias_ty.substs)
.visit_with(self)?;
}
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
Expand Down Expand Up @@ -136,26 +144,44 @@ impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for OpaqueTypeCollector<'tcx> {
ty::InternalSubsts::identity_for_item(self.tcx, parent),
);

if !check_substs_compatible(self.tcx, assoc, impl_substs) {
if check_substs_compatible(self.tcx, assoc, impl_substs) {
return self
.tcx
.type_of(assoc.def_id)
.subst(self.tcx, impl_substs)
.visit_with(self);
} else {
self.tcx.sess.delay_span_bug(
self.tcx.def_span(assoc.def_id),
"item had incorrect substs",
);
return ControlFlow::Continue(());
}

return self
.tcx
.type_of(assoc.def_id)
.subst(self.tcx, impl_substs)
.visit_with(self);
}
}
}
t.super_visit_with(self)
}
_ => t.super_visit_with(self),
ty::Adt(def, _) if def.did().is_local() => {
if !self.seen.insert(def.did().expect_local()) {
return ControlFlow::Continue(());
}
for variant in def.variants().iter() {
for field in variant.fields.iter() {
// Don't use the `ty::Adt` substs, we either
// * found the opaque in the substs
// * will find the opaque in the unsubstituted fields
// The only other situation that can occur is that after substituting,
// some projection resolves to an opaque that we would have otherwise
// not found. While we could substitute and walk those, that would mean we
// would have to walk all substitutions of an Adt, which can quickly
// degenerate into looking at an exponential number of types.
let ty = self.tcx.type_of(field.did).subst_identity();
self.visit_spanned(self.tcx.def_span(field.did), ty);
}
}
}
_ => trace!(kind=?t.kind()),
}
ControlFlow::Continue(())
}
}

Expand All @@ -166,21 +192,29 @@ fn opaque_types_defined_by<'tcx>(tcx: TyCtxt<'tcx>, item: LocalDefId) -> &'tcx [
match kind {
// We're also doing this for `AssocTy` for the wf checks in `check_opaque_meets_bounds`
DefKind::Fn | DefKind::AssocFn | DefKind::AssocTy | DefKind::AssocConst => {
let defined_opaques = match kind {
DefKind::Fn => {
OpaqueTypeCollector::collect(tcx, item, tcx.fn_sig(item).subst_identity())
let mut collector = OpaqueTypeCollector::new(tcx, item);
match kind {
// Walk over the signature of the function-like to find the opaques.
DefKind::AssocFn | DefKind::Fn => {
let ty_sig = tcx.fn_sig(item).subst_identity();
let hir_sig = tcx.hir().get_by_def_id(item).fn_sig().unwrap();
// Walk over the inputs and outputs manually in order to get good spans for them.
collector.visit_spanned(hir_sig.decl.output.span(), ty_sig.output());
for (hir, ty) in hir_sig.decl.inputs.iter().zip(ty_sig.inputs().iter()) {
collector.visit_spanned(hir.span, ty.map_bound(|x| *x));
}
}
DefKind::AssocFn => {
OpaqueTypeCollector::collect(tcx, item, tcx.fn_sig(item).subst_identity())
// Walk over the type of the item to find opaques.
DefKind::AssocTy | DefKind::AssocConst => {
let span = match tcx.hir().get_by_def_id(item).ty() {
Some(ty) => ty.span,
_ => tcx.def_span(item),
};
collector.visit_spanned(span, tcx.type_of(item).subst_identity());
}
DefKind::AssocTy | DefKind::AssocConst => OpaqueTypeCollector::collect(
tcx,
item,
ty::Binder::dummy(tcx.type_of(item).subst_identity()),
),
_ => unreachable!(),
};
tcx.arena.alloc_from_iter(defined_opaques)
}
tcx.arena.alloc_from_iter(collector.opaques)
}
DefKind::Mod
| DefKind::Struct
Expand Down Expand Up @@ -209,7 +243,9 @@ fn opaque_types_defined_by<'tcx>(tcx: TyCtxt<'tcx>, item: LocalDefId) -> &'tcx [
| DefKind::GlobalAsm
| DefKind::Impl { .. }
| DefKind::Closure
| DefKind::Generator => &[],
| DefKind::Generator => {
span_bug!(tcx.def_span(item), "{kind:?} is type checked as part of its parent")
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,19 @@ pub type Type9 = impl Send;
pub type Type10 = impl Send;
pub type Type11 = impl Send;

pub fn fn1<'a>() {
pub fn fn1<'a>() where
Type1: 'static,
Type2: 'static,
Type3: 'static,
Type4: 'static,
Type5: 'static,
Type6: 'static,
Type7: 'static,
Type8: 'static,
Type9: 'static,
Type10: 'static,
Type11: 'static,
{
// Closure
let closure1 = || { };
let _: Type1 = closure1;
Expand Down
1 change: 0 additions & 1 deletion tests/ui/generic-associated-types/issue-88595.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,4 @@ impl<'a> A<'a> for C {
type B<'b> = impl Clone;

fn a(&'a self) -> Self::B<'a> {} //~ ERROR: non-defining opaque type use in defining scope
//~^ ERROR: mismatched types
}
Loading