Skip to content
Merged
Show file tree
Hide file tree
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
Next Next commit
Add a simpler and more targetted code path for impl trait in assoc items
  • Loading branch information
oli-obk committed Jan 22, 2024
commit f58af9ba28f1bb49f880da43227c65cc10673be2
6 changes: 5 additions & 1 deletion compiler/rustc_hir_analysis/src/collect/type_of.rs
Original file line number Diff line number Diff line change
Expand Up @@ -530,9 +530,13 @@ pub(super) fn type_of_opaque(
Ok(ty::EarlyBinder::bind(match tcx.hir_node_by_def_id(def_id) {
Node::Item(item) => match item.kind {
ItemKind::OpaqueTy(OpaqueTy {
origin: hir::OpaqueTyOrigin::TyAlias { .. },
origin: hir::OpaqueTyOrigin::TyAlias { in_assoc_ty: false },
..
}) => opaque::find_opaque_ty_constraints_for_tait(tcx, def_id),
ItemKind::OpaqueTy(OpaqueTy {
origin: hir::OpaqueTyOrigin::TyAlias { in_assoc_ty: true },
..
}) => opaque::find_opaque_ty_constraints_for_impl_trait_in_assoc_type(tcx, def_id),
Copy link
Member

Choose a reason for hiding this comment

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

One question -- why does this need to be separate? Shouldn't this work with the regular find_opaque_ty_constraints_for_tait, as long as typeck uses the right def-ids collected from ImplTraitInAssocTypeCollector?

Copy link
Member

Choose a reason for hiding this comment

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

Are we still checking the nested bodies of associated items? Pls check if this example (which constrains an ITIAT incorrectly inside of a closure body) still fails:

#![feature(impl_trait_in_assoc_type)]

trait Foo {
  type Assoc;
  fn bar() -> Self::Assoc;
}

impl Foo for () {
  type Assoc = impl Sized;
  fn bar() -> Self::Assoc {
    let closure = || -> Self::Assoc { let x: Self::Assoc = (); x };
    let _: i32 = closure();
    1i32
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't need to be separate per se, but the new impl is so straight forward... and if the TAIT impl changes, we don't need to worry about the ITAIT impl at all.

// Opaque types desugared from `impl Trait`.
ItemKind::OpaqueTy(&OpaqueTy {
origin:
Expand Down
71 changes: 65 additions & 6 deletions compiler/rustc_hir_analysis/src/collect/type_of/opaque.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,60 @@ pub fn test_opaque_hidden_types(tcx: TyCtxt<'_>) -> Result<(), ErrorGuaranteed>
res
}

/// Checks "defining uses" of opaque `impl Trait` in associated types.
/// These can only be defined by associated items of the same trait.
#[instrument(skip(tcx), level = "debug")]
pub(super) fn find_opaque_ty_constraints_for_impl_trait_in_assoc_type(
tcx: TyCtxt<'_>,
def_id: LocalDefId,
) -> Ty<'_> {
let mut parent_def_id = def_id;
while tcx.def_kind(parent_def_id) == def::DefKind::OpaqueTy {
// Account for `type Alias = impl Trait<Foo = impl Trait>;` (#116031)
parent_def_id = tcx.local_parent(parent_def_id);
}
let impl_def_id = tcx.local_parent(parent_def_id);
match tcx.def_kind(impl_def_id) {
DefKind::Impl { .. } => {}
other => bug!("invalid impl trait in assoc type parent: {other:?}"),
}

let mut locator = TaitConstraintLocator { def_id, tcx, found: None, typeck_types: vec![] };

for &assoc_id in tcx.associated_item_def_ids(impl_def_id) {
let assoc = tcx.associated_item(assoc_id);
match assoc.kind {
ty::AssocKind::Const | ty::AssocKind::Fn => {
locator.check(assoc_id.expect_local(), true)
}
// Associated types don't have bodies, so they can't constrain hidden types
ty::AssocKind::Type => {}
}
}

if let Some(hidden) = locator.found {
// Only check against typeck if we didn't already error
if !hidden.ty.references_error() {
for concrete_type in locator.typeck_types {
if concrete_type.ty != tcx.erase_regions(hidden.ty)
&& !(concrete_type, hidden).references_error()
{
hidden.report_mismatch(&concrete_type, def_id, tcx).emit();
}
}
}

hidden.ty
} else {
let reported = tcx.dcx().emit_err(UnconstrainedOpaqueType {
span: tcx.def_span(def_id),
name: tcx.item_name(parent_def_id.to_def_id()),
what: "impl",
});
Ty::new_error(tcx, reported)
}
}

/// Checks "defining uses" of opaque `impl Trait` types to ensure that they meet the restrictions
/// laid for "higher-order pattern unification".
/// This ensures that inference is tractable.
Expand Down Expand Up @@ -130,7 +184,7 @@ struct TaitConstraintLocator<'tcx> {

impl TaitConstraintLocator<'_> {
#[instrument(skip(self), level = "debug")]
fn check(&mut self, item_def_id: LocalDefId) {
fn check(&mut self, item_def_id: LocalDefId, impl_trait_in_assoc_type: bool) {
// Don't try to check items that cannot possibly constrain the type.
if !self.tcx.has_typeck_results(item_def_id) {
debug!("no constraint: no typeck results");
Expand Down Expand Up @@ -182,7 +236,12 @@ impl TaitConstraintLocator<'_> {
continue;
}
constrained = true;
if !self.tcx.opaque_types_defined_by(item_def_id).contains(&self.def_id) {
let opaque_types_defined_by = if impl_trait_in_assoc_type {
self.tcx.impl_trait_in_assoc_types_defined_by(item_def_id)
} else {
self.tcx.opaque_types_defined_by(item_def_id)
};
if !opaque_types_defined_by.contains(&self.def_id) {
self.tcx.dcx().emit_err(TaitForwardCompat {
span: hidden_type.span,
item_span: self
Expand Down Expand Up @@ -240,29 +299,29 @@ impl<'tcx> intravisit::Visitor<'tcx> for TaitConstraintLocator<'tcx> {
}
fn visit_expr(&mut self, ex: &'tcx Expr<'tcx>) {
if let hir::ExprKind::Closure(closure) = ex.kind {
self.check(closure.def_id);
self.check(closure.def_id, false);
}
intravisit::walk_expr(self, ex);
}
fn visit_item(&mut self, it: &'tcx Item<'tcx>) {
trace!(?it.owner_id);
// The opaque type itself or its children are not within its reveal scope.
if it.owner_id.def_id != self.def_id {
self.check(it.owner_id.def_id);
self.check(it.owner_id.def_id, false);
intravisit::walk_item(self, it);
}
}
fn visit_impl_item(&mut self, it: &'tcx ImplItem<'tcx>) {
trace!(?it.owner_id);
// The opaque type itself or its children are not within its reveal scope.
if it.owner_id.def_id != self.def_id {
self.check(it.owner_id.def_id);
self.check(it.owner_id.def_id, false);
intravisit::walk_impl_item(self, it);
}
}
fn visit_trait_item(&mut self, it: &'tcx TraitItem<'tcx>) {
trace!(?it.owner_id);
self.check(it.owner_id.def_id);
self.check(it.owner_id.def_id, false);
intravisit::walk_trait_item(self, it);
}
fn visit_foreign_item(&mut self, it: &'tcx hir::ForeignItem<'tcx>) {
Expand Down
9 changes: 9 additions & 0 deletions compiler/rustc_middle/src/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,15 @@ rustc_queries! {
}
}

query impl_trait_in_assoc_types_defined_by(
key: LocalDefId
) -> &'tcx ty::List<LocalDefId> {
desc {
|tcx| "computing the opaque types defined by `{}`",
tcx.def_path_str(key.to_def_id())
}
}

/// Returns the list of bounds that can be used for
/// `SelectionCandidate::ProjectionCandidate(_)` and
/// `ProjectionTyCandidate::TraitDef`.
Expand Down
10 changes: 9 additions & 1 deletion compiler/rustc_ty_utils/src/opaque_types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,13 @@ impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for OpaqueTypeCollector<'tcx> {
}
}

fn impl_trait_in_assoc_types_defined_by<'tcx>(
tcx: TyCtxt<'tcx>,
item: LocalDefId,
) -> &'tcx ty::List<LocalDefId> {
opaque_types_defined_by(tcx, item)
}

fn opaque_types_defined_by<'tcx>(
tcx: TyCtxt<'tcx>,
item: LocalDefId,
Expand Down Expand Up @@ -321,5 +328,6 @@ fn opaque_types_defined_by<'tcx>(
}

pub(super) fn provide(providers: &mut Providers) {
*providers = Providers { opaque_types_defined_by, ..*providers };
*providers =
Providers { opaque_types_defined_by, impl_trait_in_assoc_types_defined_by, ..*providers };
}