-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Implement version of normalize_erasing_regions that allows for normalization failure #91255
Changes from all commits
ff448cf
e0c98e2
a040b41
0b32cf3
84bcd40
4d9a0bf
6952470
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 |
---|---|---|
@@ -1,5 +1,6 @@ | ||
use crate::middle::codegen_fn_attrs::CodegenFnAttrFlags; | ||
use crate::mir::{GeneratorLayout, GeneratorSavedLocal}; | ||
use crate::ty::normalize_erasing_regions::NormalizationError; | ||
use crate::ty::subst::Subst; | ||
use crate::ty::{self, subst::SubstsRef, ReprOptions, Ty, TyCtxt, TypeFoldable}; | ||
use rustc_ast as ast; | ||
|
@@ -199,6 +200,7 @@ pub const MAX_SIMD_LANES: u64 = 1 << 0xF; | |
pub enum LayoutError<'tcx> { | ||
Unknown(Ty<'tcx>), | ||
SizeOverflow(Ty<'tcx>), | ||
NormalizationFailure(Ty<'tcx>, NormalizationError<'tcx>), | ||
} | ||
|
||
impl<'tcx> fmt::Display for LayoutError<'tcx> { | ||
|
@@ -208,16 +210,24 @@ impl<'tcx> fmt::Display for LayoutError<'tcx> { | |
LayoutError::SizeOverflow(ty) => { | ||
write!(f, "values of the type `{}` are too big for the current architecture", ty) | ||
} | ||
LayoutError::NormalizationFailure(t, e) => write!( | ||
f, | ||
"unable to determine layout for `{}` because `{}` cannot be normalized", | ||
t, | ||
e.get_type_for_failure() | ||
), | ||
} | ||
} | ||
} | ||
|
||
#[instrument(skip(tcx, query), level = "debug")] | ||
fn layout_of<'tcx>( | ||
tcx: TyCtxt<'tcx>, | ||
query: ty::ParamEnvAnd<'tcx, Ty<'tcx>>, | ||
) -> Result<TyAndLayout<'tcx>, LayoutError<'tcx>> { | ||
ty::tls::with_related_context(tcx, move |icx| { | ||
let (param_env, ty) = query.into_parts(); | ||
debug!(?ty); | ||
|
||
if !tcx.recursion_limit().value_within_limit(icx.layout_depth) { | ||
tcx.sess.fatal(&format!("overflow representing the type `{}`", ty)); | ||
|
@@ -229,7 +239,18 @@ fn layout_of<'tcx>( | |
ty::tls::enter_context(&icx, |_| { | ||
let param_env = param_env.with_reveal_all_normalized(tcx); | ||
let unnormalized_ty = ty; | ||
let ty = tcx.normalize_erasing_regions(param_env, ty); | ||
|
||
// FIXME: We might want to have two different versions of `layout_of`: | ||
// One that can be called after typecheck has completed and can use | ||
// `normalize_erasing_regions` here and another one that can be called | ||
// before typecheck has completed and uses `try_normalize_erasing_regions`. | ||
let ty = match tcx.try_normalize_erasing_regions(param_env, ty) { | ||
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 think this deserves a comment on why failing to fully normalize is valid here. I would potentially imagine that by the time we need to know layout, we should have all the information to normalize (though I'm not sure, this might not be true) 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 think it might make sense to have two different versions of 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. This is a good question! In #82039, IIRC we were basically coming to the conclusion that we shouldn't be trying to get the layout of things until after typechecking. So, I think splitting out I think this would be better as a followup. Can you add a FIXME or file an issue? |
||
Ok(t) => t, | ||
Err(normalization_error) => { | ||
return Err(LayoutError::NormalizationFailure(ty, normalization_error)); | ||
} | ||
}; | ||
|
||
if ty != unnormalized_ty { | ||
// Ensure this layout is also cached for the normalized type. | ||
return tcx.layout_of(param_env.and(ty)); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,14 @@ crate fn provide(p: &mut Providers) { | |
normalize_mir_const_after_erasing_regions: |tcx, goal| { | ||
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. Can this query be implemented in terms of 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. Yes, both could. We would just have to unwrap the result and more explicitly document that these can panic if some type is not normalizable. Do you think this would improve caching? 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 think this is a good idea, but fine for followup. Add a FIXME please? |
||
normalize_after_erasing_regions(tcx, goal) | ||
}, | ||
try_normalize_generic_arg_after_erasing_regions: |tcx, goal| { | ||
debug!("try_normalize_generic_arg_after_erasing_regions(goal={:#?}", goal); | ||
|
||
try_normalize_after_erasing_regions(tcx, goal) | ||
}, | ||
try_normalize_mir_const_after_erasing_regions: |tcx, goal| { | ||
try_normalize_after_erasing_regions(tcx, goal) | ||
}, | ||
..*p | ||
}; | ||
} | ||
|
@@ -56,6 +64,38 @@ fn normalize_after_erasing_regions<'tcx, T: TypeFoldable<'tcx> + PartialEq + Cop | |
}) | ||
} | ||
|
||
#[instrument(level = "debug", skip(tcx))] | ||
fn try_normalize_after_erasing_regions<'tcx, T: TypeFoldable<'tcx> + PartialEq + Copy>( | ||
tcx: TyCtxt<'tcx>, | ||
goal: ParamEnvAnd<'tcx, T>, | ||
) -> Result<T, NoSolution> { | ||
let ParamEnvAnd { param_env, value } = goal; | ||
tcx.infer_ctxt().enter(|infcx| { | ||
let cause = ObligationCause::dummy(); | ||
match infcx.at(&cause, param_env).normalize(value) { | ||
Ok(Normalized { value: normalized_value, obligations: normalized_obligations }) => { | ||
// We don't care about the `obligations`; they are | ||
// always only region relations, and we are about to | ||
// erase those anyway: | ||
debug_assert_eq!( | ||
normalized_obligations.iter().find(|p| not_outlives_predicate(&p.predicate)), | ||
None, | ||
); | ||
|
||
let resolved_value = infcx.resolve_vars_if_possible(normalized_value); | ||
// It's unclear when `resolve_vars` would have an effect in a | ||
// fresh `InferCtxt`. If this assert does trigger, it will give | ||
// us a test case. | ||
debug_assert_eq!(normalized_value, resolved_value); | ||
let erased = infcx.tcx.erase_regions(resolved_value); | ||
debug_assert!(!erased.needs_infer(), "{:?}", erased); | ||
Ok(erased) | ||
} | ||
Err(NoSolution) => Err(NoSolution), | ||
} | ||
}) | ||
} | ||
|
||
fn not_outlives_predicate(p: &ty::Predicate<'tcx>) -> bool { | ||
match p.kind().skip_binder() { | ||
ty::PredicateKind::RegionOutlives(..) | ty::PredicateKind::TypeOutlives(..) => false, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
trait NotFoo {} | ||
|
||
pub trait Foo: NotFoo { | ||
type OnlyFoo; | ||
} | ||
|
||
pub trait Service { | ||
type AssocType; | ||
} | ||
|
||
pub trait ThriftService<Bug: NotFoo>: | ||
//~^ ERROR the trait bound `Bug: Foo` is not satisfied | ||
//~| ERROR the trait bound `Bug: Foo` is not satisfied | ||
Service<AssocType = <Bug as Foo>::OnlyFoo> | ||
{ | ||
fn get_service( | ||
//~^ ERROR the trait bound `Bug: Foo` is not satisfied | ||
//~| ERROR the trait bound `Bug: Foo` is not satisfied | ||
&self, | ||
) -> Self::AssocType; | ||
} | ||
|
||
fn with_factory<H>(factory: dyn ThriftService<()>) {} | ||
//~^ ERROR the trait bound `(): Foo` is not satisfied | ||
|
||
fn main() {} |
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.
So, this is never actually emitted? Makes me wonder if we actually need to keep track of what we couldn't normalize.
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.
yeah I noticed that too and was debating whether we need the type, but think that it makes sense to output the type for which normalization fails here. There are a couple other issues which triggered this ICE (but not through
layout_of
and which have yet to be fixed (although I think the new function would make that fairly straightforward)), and we might need this type information there.I also added another test for issue 85103 which emits this error.