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

[WIP] Enforce may-define-must-define for ATPITs #123046

Closed
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
4 changes: 4 additions & 0 deletions compiler/rustc_borrowck/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,10 @@ borrowck_moved_due_to_usage_in_operator =
*[false] operator
}

borrowck_must_define_opaque =
method must define opaque type
.label = because it shows up in the args of this method

borrowck_opaque_type_lifetime_mismatch =
opaque type used twice with different lifetimes
.label = lifetime `{$arg}` used here
Expand Down
34 changes: 30 additions & 4 deletions compiler/rustc_borrowck/src/region_infer/opaque_types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@ use rustc_span::Span;
use rustc_trait_selection::traits::error_reporting::TypeErrCtxtExt as _;
use rustc_trait_selection::traits::ObligationCtxt;

use crate::session_diagnostics::LifetimeMismatchOpaqueParam;
use crate::session_diagnostics::NonGenericOpaqueTypeParam;
use crate::session_diagnostics::{
LifetimeMismatchOpaqueParam, MustDefineOpaque, NonGenericOpaqueTypeParam,
};

use super::RegionInferenceContext;

Expand All @@ -42,12 +43,15 @@ impl<'tcx> RegionInferenceContext<'tcx> {
/// Check that all opaque types have the same region parameters if they have the same
/// non-region parameters. This is necessary because within the new solver we perform various query operations
/// modulo regions, and thus could unsoundly select some impls that don't hold.
fn check_unique(
fn check_unique_and_defined(
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: comment

&self,
infcx: &InferCtxt<'tcx>,
anchor: LocalDefId,
opaque_ty_decls: &FxIndexMap<OpaqueTypeKey<'tcx>, OpaqueHiddenType<'tcx>>,
) {
let mut seen = FxIndexSet::default();
for (i, (a, a_ty)) in opaque_ty_decls.iter().enumerate() {
seen.insert(a.def_id);
for (b, b_ty) in opaque_ty_decls.iter().skip(i + 1) {
if a.def_id != b.def_id {
continue;
Expand Down Expand Up @@ -78,6 +82,24 @@ impl<'tcx> RegionInferenceContext<'tcx> {
}
}
}

if !infcx.tcx.is_typeck_child(anchor.to_def_id()) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Specifically, we can't enforce this on typeck children because it would mean that the given code fails:

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

impl Foo for () {
  type Assoc = impl Sized;
  fn foo() -> Self::Assoc {
    let x = || {};
    //      ^^^^^ <- this body doesn't define `Self::Assoc`, even though it could.
    ""
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have thoughts either, @aliemjay? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't remember if this landed, but I once had a commit where opaques from children got copied into their parents during borrowck. Then we could stop looking at children entirely. Well, modulo opaques that are only mentioned in the signature of children, but... oh we don't have ATPIT tests for that, do we

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't collect opaques for typeck children anyways:

trait Foo {
  type Assoc;
  fn foo();
}

impl Foo for () {
  type Assoc = impl Sized;
  fn foo() {
    let x = || -> Self::Assoc {
        ""
    };
  }
}
error[E0308]: mismatched types
  --> src/lib.rs:12:9
   |
9  |   type Assoc = impl Sized;
   |                ---------- the expected opaque type
...
12 |         ""
   |         ^^ expected opaque type, found `&str`
   |
   = note: expected opaque type `<() as Foo>::Assoc`
                found reference `&'static str`

Copy link
Member Author

@compiler-errors compiler-errors Mar 25, 2024

Choose a reason for hiding this comment

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

And I don't think we want to -- since if we don't, then we only need to check that the typeck root defines the given opaques.

// Check that all the in-scope ATPITs are defined by the anchor
for def_id in infcx.tcx.opaque_types_defined_by(anchor) {
if seen.contains(&def_id) {
continue;
}
match infcx.tcx.opaque_type_origin(def_id) {
OpaqueTyOrigin::TyAlias { in_assoc_ty: true, .. } => {
infcx.dcx().emit_err(MustDefineOpaque {
span: infcx.tcx.def_span(def_id),
item_span: infcx.tcx.def_span(anchor),
});
}
_ => {}
}
}
}
}

/// Resolve any opaque types that were encountered while borrow checking
Expand Down Expand Up @@ -125,7 +147,11 @@ impl<'tcx> RegionInferenceContext<'tcx> {
infcx: &InferCtxt<'tcx>,
opaque_ty_decls: FxIndexMap<OpaqueTypeKey<'tcx>, OpaqueHiddenType<'tcx>>,
) -> FxIndexMap<LocalDefId, OpaqueHiddenType<'tcx>> {
self.check_unique(infcx, &opaque_ty_decls);
self.check_unique_and_defined(
infcx,
self.universal_regions().defining_ty.def_id().expect_local(),
&opaque_ty_decls,
);

let mut result: FxIndexMap<LocalDefId, OpaqueHiddenType<'tcx>> = FxIndexMap::default();

Expand Down
9 changes: 9 additions & 0 deletions compiler/rustc_borrowck/src/session_diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,15 @@ pub(crate) struct NonGenericOpaqueTypeParam<'a, 'tcx> {
pub param_span: Span,
}

#[derive(Diagnostic)]
#[diag(borrowck_must_define_opaque)]
pub(crate) struct MustDefineOpaque {
#[primary_span]
pub span: Span,
#[label]
pub item_span: Span,
}

#[derive(Diagnostic)]
#[diag(borrowck_opaque_type_lifetime_mismatch)]
pub(crate) struct LifetimeMismatchOpaqueParam<'tcx> {
Expand Down
11 changes: 10 additions & 1 deletion tests/ui/generic-associated-types/issue-87258_a.stderr
Original file line number Diff line number Diff line change
@@ -1,3 +1,12 @@
error: method must define opaque type
--> $DIR/issue-87258_a.rs:17:26
|
LL | type FooFuture<'a> = impl Trait1;
| ^^^^^^^^^^^
LL |
LL | fn foo<'a>() -> Self::FooFuture<'a> {
| ----------------------------------- because it shows up in the args of this method

error: unconstrained opaque type
--> $DIR/issue-87258_a.rs:17:26
|
Expand All @@ -6,5 +15,5 @@ LL | type FooFuture<'a> = impl Trait1;
|
= note: `FooFuture` must be used in combination with a concrete type within the same impl

error: aborting due to 1 previous error
error: aborting due to 2 previous errors

12 changes: 11 additions & 1 deletion tests/ui/type-alias-impl-trait/in-assoc-ty-early-bound2.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,16 @@ LL | fn bar<'a: 'a>()
LL | let _: Self::Assoc<'a> = x;
| ^^^^^^^^^^^^^^^

error: method must define opaque type
--> $DIR/in-assoc-ty-early-bound2.rs:9:22
|
LL | type Assoc<'a> = impl Sized;
| ^^^^^^^^^^
LL | / fn bar<'a: 'a>()
LL | | where
LL | | Self::Assoc<'a>:,
| |_________________________- because it shows up in the args of this method

error: unconstrained opaque type
--> $DIR/in-assoc-ty-early-bound2.rs:9:22
|
Expand All @@ -17,6 +27,6 @@ LL | type Assoc<'a> = impl Sized;
|
= note: `Assoc` must be used in combination with a concrete type within the same impl

error: aborting due to 2 previous errors
error: aborting due to 3 previous errors

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