-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
||
|
@@ -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( | ||
&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; | ||
|
@@ -78,6 +82,24 @@ impl<'tcx> RegionInferenceContext<'tcx> { | |
} | ||
} | ||
} | ||
|
||
if !infcx.tcx.is_typeck_child(anchor.to_def_id()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
""
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you have thoughts either, @aliemjay? 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 {
""
};
}
}
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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(); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: comment