Skip to content

Require TAITs to be mentioned in the signatures of functions that register hidden types for them #112652

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 11 commits into from
Jul 8, 2023
Merged
3 changes: 1 addition & 2 deletions compiler/rustc_error_messages/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ fn register_functions(bundle: &mut FluentBundle) {
pub type LazyFallbackBundle = Lrc<Lazy<FluentBundle, impl FnOnce() -> FluentBundle>>;

/// Return the default `FluentBundle` with standard "en-US" diagnostic messages.
#[instrument(level = "trace")]
#[instrument(level = "trace", skip(resources))]
pub fn fallback_fluent_bundle(
resources: Vec<&'static str>,
with_directionality_markers: bool,
Expand All @@ -242,7 +242,6 @@ pub fn fallback_fluent_bundle(
for resource in resources {
let resource = FluentResource::try_new(resource.to_string())
.expect("failed to parse fallback fluent resource");
trace!(?resource);
fallback_bundle.add_resource_overriding(resource);
}

Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_hir_analysis/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,9 @@ hir_analysis_static_specialize = cannot specialize on `'static` lifetime

hir_analysis_substs_on_overridden_impl = could not resolve substs on overridden impl

hir_analysis_tait_forward_compat = item constrains opaque type that is not in its signature
.note = this item must mention the opaque type in its signature in order to be able to register hidden types

hir_analysis_target_feature_on_main = `main` function is not allowed to have `#[target_feature]`

hir_analysis_too_large_static = extern static is too large for the current architecture
Expand Down
11 changes: 10 additions & 1 deletion compiler/rustc_hir_analysis/src/collect/type_of/opaque.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use rustc_middle::hir::nested_filter;
use rustc_middle::ty::{self, Ty, TyCtxt, TypeVisitableExt};
use rustc_span::DUMMY_SP;

use crate::errors::UnconstrainedOpaqueType;
use crate::errors::{TaitForwardCompat, UnconstrainedOpaqueType};

/// Checks "defining uses" of opaque `impl Trait` types to ensure that they meet the restrictions
/// laid for "higher-order pattern unification".
Expand Down Expand Up @@ -139,6 +139,15 @@ impl TaitConstraintLocator<'_> {
continue;
}
constrained = true;
if !self.tcx.opaque_types_defined_by(item_def_id).contains(&self.def_id) {
self.tcx.sess.emit_err(TaitForwardCompat {
span: hidden_type.span,
item_span: self
.tcx
.def_ident_span(item_def_id)
.unwrap_or_else(|| self.tcx.def_span(item_def_id)),
});
}
Comment on lines +142 to +150
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the main change of this PR. We double check that any item that produces hidden types also has the corresponding opaque type in its signature

let concrete_type =
self.tcx.erase_regions(hidden_type.remap_generic_params_to_declaration_params(
opaque_type_key,
Expand Down
10 changes: 10 additions & 0 deletions compiler/rustc_hir_analysis/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,16 @@ pub struct UnconstrainedOpaqueType {
pub what: &'static str,
}

#[derive(Diagnostic)]
#[diag(hir_analysis_tait_forward_compat)]
#[note]
pub struct TaitForwardCompat {
#[primary_span]
pub span: Span,
#[note]
pub item_span: Span,
}

pub struct MissingTypeParams {
pub span: Span,
pub def_span: Span,
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_hir_typeck/src/inherited.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ impl<'tcx> Inherited<'tcx> {
let infcx = tcx
.infer_ctxt()
.ignoring_regions()
.with_opaque_type_inference(DefiningAnchor::Bind(hir_owner.def_id))
.with_opaque_type_inference(DefiningAnchor::Bind(def_id))
.build();
let typeck_results = RefCell::new(ty::TypeckResults::new(hir_owner));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ 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) => {
(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::Fn | DefKind::Static(_) | DefKind::Const | 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()) {
let sp = tcx.def_ident_span(body_owner_def_id).unwrap_or_else(|| tcx.def_span(body_owner_def_id));
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_trait_selection/src/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ mod fulfill;
pub mod misc;
mod object_safety;
pub mod outlives_bounds;
mod project;
pub mod project;
pub mod query;
#[cfg_attr(not(bootstrap), allow(hidden_glob_reexports))]
mod select;
Expand Down
152 changes: 117 additions & 35 deletions compiler/rustc_ty_utils/src/opaque_types.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
use rustc_data_structures::fx::FxHashSet;
use rustc_hir::intravisit::Visitor;
use rustc_hir::{def::DefKind, def_id::LocalDefId};
use rustc_hir::{intravisit, CRATE_HIR_ID};
use rustc_middle::query::Providers;
use rustc_middle::ty::util::{CheckRegions, NotUniqueParam};
use rustc_middle::ty::{self, Ty, TyCtxt};
Expand Down Expand Up @@ -51,7 +53,7 @@ impl<'tcx> OpaqueTypeCollector<'tcx> {

fn parent(&self) -> Option<LocalDefId> {
match self.tcx.def_kind(self.item) {
DefKind::Fn => None,
DefKind::AnonConst | DefKind::InlineConst | DefKind::Fn | DefKind::TyAlias => None,
DefKind::AssocFn | DefKind::AssocTy | DefKind::AssocConst => {
Some(self.tcx.local_parent(self.item))
}
Expand All @@ -61,6 +63,73 @@ impl<'tcx> OpaqueTypeCollector<'tcx> {
),
}
}

/// Returns `true` if `opaque_hir_id` is a sibling or a child of a sibling of `self.item`.
///
/// Example:
/// ```ignore UNSOLVED (is this a bug?)
/// # #![feature(type_alias_impl_trait)]
/// pub mod foo {
/// pub mod bar {
/// pub trait Bar { /* ... */ }
/// pub type Baz = impl Bar;
///
/// # impl Bar for () {}
/// fn f1() -> Baz { /* ... */ }
/// }
/// fn f2() -> bar::Baz { /* ... */ }
/// }
/// ```
///
/// and `opaque_def_id` is the `DefId` of the definition of the opaque type `Baz`.
/// For the above example, this function returns `true` for `f1` and `false` for `f2`.
#[instrument(level = "trace", skip(self), ret)]
fn check_tait_defining_scope(&self, opaque_def_id: LocalDefId) -> bool {
let mut hir_id = self.tcx.hir().local_def_id_to_hir_id(self.item);
let opaque_hir_id = self.tcx.hir().local_def_id_to_hir_id(opaque_def_id);

// Named opaque types can be defined by any siblings or children of siblings.
let scope = self.tcx.hir().get_defining_scope(opaque_hir_id);
// We walk up the node tree until we hit the root or the scope of the opaque type.
while hir_id != scope && hir_id != CRATE_HIR_ID {
hir_id = self.tcx.hir().get_parent_item(hir_id).into();
}
// Syntactically, we are allowed to define the concrete type if:
hir_id == scope
}

fn collect_body_and_predicate_taits(&mut self) {
// Look at all where bounds.
self.tcx.predicates_of(self.item).instantiate_identity(self.tcx).visit_with(self);
// An item is allowed to constrain opaques declared within its own body (but not nested within
// nested functions).
self.collect_taits_declared_in_body();
}

#[instrument(level = "trace", skip(self))]
fn collect_taits_declared_in_body(&mut self) {
let body = self.tcx.hir().body(self.tcx.hir().body_owned_by(self.item)).value;
struct TaitInBodyFinder<'a, 'tcx> {
collector: &'a mut OpaqueTypeCollector<'tcx>,
}
impl<'v> intravisit::Visitor<'v> for TaitInBodyFinder<'_, '_> {
#[instrument(level = "trace", skip(self))]
fn visit_nested_item(&mut self, id: rustc_hir::ItemId) {
let id = id.owner_id.def_id;
if let DefKind::TyAlias = self.collector.tcx.def_kind(id) {
let items = self.collector.tcx.opaque_types_defined_by(id);
self.collector.opaques.extend(items);
}
}
#[instrument(level = "trace", skip(self))]
// Recurse into these, as they are type checked with their parent
fn visit_nested_body(&mut self, id: rustc_hir::BodyId) {
let body = self.collector.tcx.hir().body(id);
self.visit_body(body);
}
}
TaitInBodyFinder { collector: self }.visit_expr(body);
}
}

impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for OpaqueTypeCollector<'tcx> {
Expand All @@ -73,6 +142,21 @@ impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for OpaqueTypeCollector<'tcx> {
return ControlFlow::Continue(());
}

// TAITs outside their defining scopes are ignored.
let origin = self.tcx.opaque_type_origin(alias_ty.def_id.expect_local());
trace!(?origin);
match origin {
rustc_hir::OpaqueTyOrigin::FnReturn(_)
| rustc_hir::OpaqueTyOrigin::AsyncFn(_) => {}
rustc_hir::OpaqueTyOrigin::TyAlias { in_assoc_ty } => {
if !in_assoc_ty {
if !self.check_tait_defining_scope(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) {
Expand Down Expand Up @@ -188,65 +272,63 @@ impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for OpaqueTypeCollector<'tcx> {
fn opaque_types_defined_by<'tcx>(tcx: TyCtxt<'tcx>, item: LocalDefId) -> &'tcx [LocalDefId] {
let kind = tcx.def_kind(item);
trace!(?kind);
// FIXME(type_alias_impl_trait): This is definitely still wrong except for RPIT and impl trait in assoc types.
let mut collector = OpaqueTypeCollector::new(tcx, item);
match kind {
Copy link
Member

Choose a reason for hiding this comment

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

I really dislike that we're matching on kind 3 times... is there a way to flatten this, even if it means duplicating let mut collector = OpaqueTypeCollector::new(tcx, item); a couple of times?

Copy link
Contributor Author

@oli-obk oli-obk Jun 29, 2023

Choose a reason for hiding this comment

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

I got rid of the extra levels of matching

// 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));
}
collector.collect_body_and_predicate_taits();
}
// Walk over the type of the item to find opaques.
DefKind::Static(_) | DefKind::Const | DefKind::AssocConst | DefKind::AnonConst => {
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());
collector.collect_body_and_predicate_taits();
}
// 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 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));
}
}
// 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());
}
_ => unreachable!(),
DefKind::TyAlias | DefKind::AssocTy => {
tcx.type_of(item).subst_identity().visit_with(&mut collector);
}
DefKind::OpaqueTy => {
for (pred, span) in tcx.explicit_item_bounds(item).subst_identity_iter_copied() {
collector.visit_spanned(span, pred);
}
tcx.arena.alloc_from_iter(collector.opaques)
}
DefKind::Mod
| DefKind::Struct
| DefKind::Union
| DefKind::Enum
| DefKind::Variant
| DefKind::Trait
| DefKind::TyAlias
| DefKind::ForeignTy
| DefKind::TraitAlias
| DefKind::TyParam
| DefKind::Const
| DefKind::ConstParam
| DefKind::Static(_)
| DefKind::Ctor(_, _)
| DefKind::Macro(_)
| DefKind::ExternCrate
| DefKind::Use
| DefKind::ForeignMod
| DefKind::AnonConst
| DefKind::InlineConst
| DefKind::OpaqueTy
| DefKind::ImplTraitPlaceholder
| DefKind::Field
| DefKind::LifetimeParam
| DefKind::GlobalAsm
| DefKind::Impl { .. }
| DefKind::Closure
| DefKind::Generator => {
span_bug!(tcx.def_span(item), "{kind:?} is type checked as part of its parent")
| DefKind::Impl { .. } => {}
// Closures and generators are type checked with their parent, so there is no difference here.
DefKind::Closure | DefKind::Generator | DefKind::InlineConst => {
return tcx.opaque_types_defined_by(tcx.local_parent(item));
}
}
tcx.arena.alloc_from_iter(collector.opaques)
}

pub(super) fn provide(providers: &mut Providers) {
Expand Down
6 changes: 3 additions & 3 deletions tests/ui/feature-gates/feature-gate-type_alias_impl_trait.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ fn define() -> Bar {

type Foo2 = impl Debug;

fn define2() {
fn define2(_: Foo2) {
let x = || -> Foo2 { 42 };
}

Expand All @@ -20,13 +20,13 @@ type Foo3 = impl Debug;
fn define3(x: Foo3) {
let y: i32 = x;
}
fn define3_1() {
fn define3_1(_: Foo3) {
define3(42)
}

type Foo4 = impl Debug;

fn define4() {
fn define4(_: Foo4) {
let y: Foo4 = 42;
}

Expand Down
23 changes: 23 additions & 0 deletions tests/ui/generic-associated-types/issue-90014-tait.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
//! This test is reporting the wrong error. We need
//! more inherent associated type tests that use opaque types
//! in general. Some variant of this test should compile successfully.
// known-bug: unknown
// edition:2018

#![feature(impl_trait_in_assoc_type, inherent_associated_types)]
#![allow(incomplete_features)]

use std::future::Future;

struct Foo<'a>(&'a mut ());

impl Foo<'_> {
type Fut<'a> = impl Future<Output = ()>;
//^ ERROR: the type `&mut ()` does not fulfill the required lifetime

fn make_fut<'a>(&'a self) -> Self::Fut<'a> {
async { () }
}
}

fn main() {}
22 changes: 22 additions & 0 deletions tests/ui/generic-associated-types/issue-90014-tait.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
error[E0308]: mismatched types
--> $DIR/issue-90014-tait.rs:19:9
|
LL | type Fut<'a> = impl Future<Output = ()>;
| ------------------------ the expected future
...
LL | fn make_fut<'a>(&'a self) -> Self::Fut<'a> {
| ------------- expected `Foo<'_>::Fut<'a>` because of return type
LL | async { () }
| ^^^^^^^^^^^^ expected future, found `async` block
|
= note: expected opaque type `Foo<'_>::Fut<'a>`
found `async` block `[async block@$DIR/issue-90014-tait.rs:19:9: 19:21]`
note: this item must have the opaque type in its signature in order to be able to register hidden types
--> $DIR/issue-90014-tait.rs:18:8
|
LL | fn make_fut<'a>(&'a self) -> Self::Fut<'a> {
| ^^^^^^^^

error: aborting due to previous error

For more information about this error, try `rustc --explain E0308`.
Loading