Skip to content
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

Install projection from RPITIT to default trait method opaque correctly #109198

Merged
merged 9 commits into from
Mar 17, 2023
28 changes: 17 additions & 11 deletions compiler/rustc_hir_analysis/src/check/wfcheck.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1545,21 +1545,27 @@ fn check_return_position_impl_trait_in_trait_bounds<'tcx>(
if let Some(assoc_item) = tcx.opt_associated_item(fn_def_id.to_def_id())
&& assoc_item.container == ty::AssocItemContainer::TraitContainer
{
// FIXME(-Zlower-impl-trait-in-trait-to-assoc-ty): Even with the new lowering
// strategy, we can't just call `check_associated_item` on the new RPITITs,
// because tests like `tests/ui/async-await/in-trait/implied-bounds.rs` will fail.
// That's because we need to check that the bounds of the RPITIT hold using
// the special substs that we create during opaque type lowering, otherwise we're
// getting a bunch of early bound and free regions mixed up... Haven't looked too
// deep into this, though.
for arg in fn_output.walk() {
if let ty::GenericArgKind::Type(ty) = arg.unpack()
&& let ty::Alias(ty::Opaque, proj) = ty.kind()
// FIXME(-Zlower-impl-trait-in-trait-to-assoc-ty) we should just check
// `tcx.def_kind(proj.def_id) == DefKind::ImplTraitPlaceholder`. Right now
// `check_associated_type_bounds` is not called for RPITITs synthesized as
// associated types. See `check_mod_type_wf` to see how synthesized associated
// types are missed due to iterating over HIR.
&& tcx.is_impl_trait_in_trait(proj.def_id)
&& tcx.impl_trait_in_trait_parent_fn(proj.def_id) == fn_def_id.to_def_id()
// RPITITs are always eagerly normalized into opaques, so always look for an
// opaque here.
&& let ty::Alias(ty::Opaque, opaque_ty) = ty.kind()
&& let Some(opaque_def_id) = 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
&& source == fn_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.

If I understand correct this is equivalent to what was before but done a little bit better, or are there actual difference? if so in which cases?.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not equivalent. If we call is_impl_trait_in_trait on an opaque def id, then it returns false.

Copy link
Member Author

Choose a reason for hiding this comment

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

Without this, some tests begin to spuriously pass wfcheck.

Copy link
Member Author

@compiler-errors compiler-errors Mar 16, 2023

Choose a reason for hiding this comment

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

the first commit in the stack "breaks" tests/ui/impl-trait/in-trait/wf-bounds.rs by actually projecting the RPITIT to a real opaque type, so this commit fixes the test by always checking for an opaque type.

Copy link
Member

Choose a reason for hiding this comment

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

the first commit in the stack "breaks" tests/ui/impl-trait/in-trait/wf-bounds.rs by actually projecting the RPITIT to a real opaque type, so this commit fixes the test by always checking for an opaque type.

Ahh right, because we project the projection to an opaque, that's the thing.

{
let span = tcx.def_span(proj.def_id);
let bounds = wfcx.tcx().explicit_item_bounds(proj.def_id);
let span = tcx.def_span(opaque_ty.def_id);
let bounds = wfcx.tcx().explicit_item_bounds(opaque_ty.def_id);
let wf_obligations = bounds.iter().flat_map(|&(bound, bound_span)| {
let bound = ty::EarlyBinder(bound).subst(tcx, proj.substs);
let bound = ty::EarlyBinder(bound).subst(tcx, opaque_ty.substs);
let normalized_bound = wfcx.normalize(span, None, bound);
traits::wf::predicate_obligations(
wfcx.infcx,
Expand Down
22 changes: 15 additions & 7 deletions compiler/rustc_ty_utils/src/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,16 +117,22 @@ fn adt_sized_constraint(tcx: TyCtxt<'_>, def_id: DefId) -> &[Ty<'_>] {

/// See `ParamEnv` struct definition for details.
fn param_env(tcx: TyCtxt<'_>, def_id: DefId) -> ty::ParamEnv<'_> {
// When computing the param_env of an RPITIT, copy param_env of the containing function. The
// synthesized associated type doesn't have extra predicates to assume.
if let Some(ImplTraitInTraitData::Trait { fn_def_id, .. }) = tcx.opt_rpitit_info(def_id) {
return tcx.param_env(fn_def_id);
}

// Compute the bounds on Self and the type parameters.
let ty::InstantiatedPredicates { mut predicates, .. } =
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't be more clear if this is an else of the if let from below?.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that works too.

tcx.predicates_of(def_id).instantiate_identity(tcx);

// When computing the param_env of an RPITIT, use predicates of the containing function,
// *except* for the additional assumption that the RPITIT normalizes to the trait method's
// default opaque type. This is needed to properly check the item bounds of the assoc
// type hold (`check_type_bounds`), since that method already installs a similar projection
// bound, so they will conflict.
// FIXME(-Zlower-impl-trait-in-trait-to-assoc-ty): I don't like this, we should
// at least be making sure that the generics in RPITITs and their parent fn don't
// get out of alignment, or else we do actually need to substitute these predicates.
if let Some(ImplTraitInTraitData::Trait { fn_def_id, .. }) = tcx.opt_rpitit_info(def_id) {
predicates = tcx.predicates_of(fn_def_id).instantiate_identity(tcx).predicates;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this part of the change is what makes the tests included in this commit pass, right?.

Copy link
Member Author

Choose a reason for hiding this comment

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

both changes are needed to make the test pass, otherwise the error message goes from a "needs type info" error to a wfchecking error, iirc.


// Finally, we have to normalize the bounds in the environment, in
// case they contain any associated type projections. This process
// can yield errors if the put in illegal associated types, like
Expand Down Expand Up @@ -160,7 +166,9 @@ fn param_env(tcx: TyCtxt<'_>, def_id: DefId) -> ty::ParamEnv<'_> {
}

let local_did = def_id.as_local();
let hir_id = local_did.map(|def_id| tcx.hir().local_def_id_to_hir_id(def_id));
// FIXME(-Zlower-impl-trait-in-trait-to-assoc-ty): This isn't correct for
// RPITITs in const trait fn.
let hir_id = local_did.and_then(|def_id| tcx.opt_local_def_id_to_hir_id(def_id));

// FIXME(consts): This is not exactly in line with the constness query.
let constness = match hir_id {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
warning: the feature `return_position_impl_trait_in_trait` is incomplete and may not be safe to use and/or cause compiler crashes
--> $DIR/box-coerce-span-in-default.rs:3:12
--> $DIR/box-coerce-span-in-default.rs:5:12
|
LL | #![feature(return_position_impl_trait_in_trait)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
warning: the feature `return_position_impl_trait_in_trait` is incomplete and may not be safe to use and/or cause compiler crashes
--> $DIR/box-coerce-span-in-default.rs:5:12
|
LL | #![feature(return_position_impl_trait_in_trait)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: see issue #91611 <https://github.com/rust-lang/rust/issues/91611> for more information
= note: `#[warn(incomplete_features)]` on by default

warning: 1 warning emitted

2 changes: 2 additions & 0 deletions tests/ui/impl-trait/in-trait/box-coerce-span-in-default.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
// check-pass
// [next] compile-flags: -Zlower-impl-trait-in-trait-to-assoc-ty
// revisions: current next

#![feature(return_position_impl_trait_in_trait)]
//~^ WARN the feature `return_position_impl_trait_in_trait` is incomplete
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error[E0308]: mismatched types
--> $DIR/default-body-type-err-2.rs:8:9
--> $DIR/default-body-type-err-2.rs:10:9
|
LL | 42
| ^^- help: try using a conversion method: `.to_string()`
Expand Down
11 changes: 11 additions & 0 deletions tests/ui/impl-trait/in-trait/default-body-type-err-2.next.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
error[E0308]: mismatched types
--> $DIR/default-body-type-err-2.rs:10:9
|
LL | 42
| ^^- help: try using a conversion method: `.to_string()`
| |
| expected `String`, found integer

error: aborting due to previous error

For more information about this error, try `rustc --explain E0308`.
2 changes: 2 additions & 0 deletions tests/ui/impl-trait/in-trait/default-body-type-err-2.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
// edition:2021
// [next] compile-flags: -Zlower-impl-trait-in-trait-to-assoc-ty
// revisions: current next

#![allow(incomplete_features)]
#![feature(async_fn_in_trait)]
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error[E0271]: type mismatch resolving `<&i32 as Deref>::Target == String`
--> $DIR/default-body-type-err.rs:7:22
--> $DIR/default-body-type-err.rs:10:22
|
LL | fn lol(&self) -> impl Deref<Target = String> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `i32`, found `String`
Expand Down
12 changes: 12 additions & 0 deletions tests/ui/impl-trait/in-trait/default-body-type-err.next.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
error[E0271]: type mismatch resolving `<&i32 as Deref>::Target == String`
--> $DIR/default-body-type-err.rs:10:22
|
LL | fn lol(&self) -> impl Deref<Target = String> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `i32`, found `String`
LL |
LL | &1i32
| ----- return type was inferred to be `&i32` here

error: aborting due to previous error

For more information about this error, try `rustc --explain E0271`.
3 changes: 3 additions & 0 deletions tests/ui/impl-trait/in-trait/default-body-type-err.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
// [next] compile-flags: -Zlower-impl-trait-in-trait-to-assoc-ty
// revisions: current next

#![allow(incomplete_features)]
#![feature(return_position_impl_trait_in_trait)]

Expand Down
2 changes: 2 additions & 0 deletions tests/ui/impl-trait/in-trait/default-body.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
// check-pass
// edition:2021
// [next] compile-flags: -Zlower-impl-trait-in-trait-to-assoc-ty
// revisions: current next

#![feature(async_fn_in_trait, return_position_impl_trait_in_trait)]
#![allow(incomplete_features)]
Expand Down