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

Implement version of normalize_erasing_regions that allows for normalization failure #91255

Merged
merged 7 commits into from
Dec 1, 2021
Merged
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: 3 additions & 1 deletion compiler/rustc_lint/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1337,7 +1337,9 @@ impl<'tcx> LateLintPass<'tcx> for VariantSizeDifferences {
let layout = match cx.layout_of(ty) {
Ok(layout) => layout,
Err(
ty::layout::LayoutError::Unknown(_) | ty::layout::LayoutError::SizeOverflow(_),
ty::layout::LayoutError::Unknown(_)
| ty::layout::LayoutError::SizeOverflow(_)
| ty::layout::LayoutError::NormalizationFailure(_, _),
) => return,
};
let (variants, tag) = match layout.variants {
Expand Down
3 changes: 0 additions & 3 deletions compiler/rustc_middle/src/mir/interpret/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -492,9 +492,6 @@ impl dyn MachineStopType {
}
}

#[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))]
static_assert_size!(InterpError<'_>, 64);

pub enum InterpError<'tcx> {
/// The program caused undefined behavior.
UndefinedBehavior(UndefinedBehaviorInfo<'tcx>),
Expand Down
19 changes: 19 additions & 0 deletions compiler/rustc_middle/src/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1644,6 +1644,11 @@ rustc_queries! {
desc { "normalizing `{:?}`", goal }
}

// FIXME: Implement `normalize_generic_arg_after_erasing_regions` and
// `normalize_mir_const_after_erasing_regions` in terms of
// `try_normalize_generic_arg_after_erasing_regions` and
// `try_normalize_mir_const_after_erasing_regions`, respectively.

/// Do not call this query directly: invoke `normalize_erasing_regions` instead.
query normalize_generic_arg_after_erasing_regions(
goal: ParamEnvAnd<'tcx, GenericArg<'tcx>>
Expand All @@ -1658,6 +1663,20 @@ rustc_queries! {
desc { "normalizing `{}`", goal.value }
}

/// Do not call this query directly: invoke `try_normalize_erasing_regions` instead.
query try_normalize_generic_arg_after_erasing_regions(
goal: ParamEnvAnd<'tcx, GenericArg<'tcx>>
) -> Result<GenericArg<'tcx>, NoSolution> {
desc { "normalizing `{}`", goal.value }
}

/// Do not call this query directly: invoke `try_normalize_erasing_regions` instead.
query try_normalize_mir_const_after_erasing_regions(
goal: ParamEnvAnd<'tcx, mir::ConstantKind<'tcx>>
) -> Result<mir::ConstantKind<'tcx>, NoSolution> {
desc { "normalizing `{}`", goal.value }
}

query implied_outlives_bounds(
goal: CanonicalTyGoal<'tcx>
) -> Result<
Expand Down
23 changes: 22 additions & 1 deletion compiler/rustc_middle/src/ty/layout.rs
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;
Expand Down Expand Up @@ -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> {
Expand All @@ -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",
Copy link
Member

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.

Copy link
Contributor Author

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.

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));
Expand All @@ -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) {
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 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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it might make sense to have two different versions of layout_of. Currently most of the calls of that function happen after typecheck and normalization is always guaranteed to succeed, but there are some calls in typecheck and some lints that use it. Does it makes sense to have one version which requires normalization to succeed and another in which it can fail?

Copy link
Member

Choose a reason for hiding this comment

The 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 layout_of into two version is a decent intermediate step to "audit", in some way, incorrect behavior.

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));
Expand Down
115 changes: 115 additions & 0 deletions compiler/rustc_middle/src/ty/normalize_erasing_regions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,28 @@
//! or constant found within. (This underlying query is what is cached.)

use crate::mir;
use crate::traits::query::NoSolution;
use crate::ty::fold::{TypeFoldable, TypeFolder};
use crate::ty::subst::{Subst, SubstsRef};
use crate::ty::{self, Ty, TyCtxt};

#[derive(Debug, Copy, Clone, HashStable, TyEncodable, TyDecodable)]
pub enum NormalizationError<'tcx> {
Type(Ty<'tcx>),
Const(ty::Const<'tcx>),
ConstantKind(mir::ConstantKind<'tcx>),
}

impl<'tcx> NormalizationError<'tcx> {
pub fn get_type_for_failure(&self) -> String {
match self {
NormalizationError::Type(t) => format!("{}", t),
NormalizationError::Const(c) => format!("{}", c),
NormalizationError::ConstantKind(ck) => format!("{}", ck),
}
}
}

impl<'tcx> TyCtxt<'tcx> {
/// Erase the regions in `value` and then fully normalize all the
/// types found within. The result will also have regions erased.
Expand All @@ -32,6 +50,8 @@ impl<'tcx> TyCtxt<'tcx> {
// Erase first before we do the real query -- this keeps the
// cache from being too polluted.
let value = self.erase_regions(value);
debug!(?value);

if !value.has_projections() {
value
} else {
Expand All @@ -41,6 +61,39 @@ impl<'tcx> TyCtxt<'tcx> {
}
}

/// Tries to erase the regions in `value` and then fully normalize all the
/// types found within. The result will also have regions erased.
///
/// Contrary to `normalize_erasing_regions` this function does not assume that normalization
/// succeeds.
pub fn try_normalize_erasing_regions<T>(
self,
param_env: ty::ParamEnv<'tcx>,
value: T,
) -> Result<T, NormalizationError<'tcx>>
where
T: TypeFoldable<'tcx>,
{
debug!(
"try_normalize_erasing_regions::<{}>(value={:?}, param_env={:?})",
std::any::type_name::<T>(),
value,
param_env,
);

// Erase first before we do the real query -- this keeps the
// cache from being too polluted.
let value = self.erase_regions(value);
debug!(?value);

if !value.has_projections() {
Ok(value)
} else {
let mut folder = TryNormalizeAfterErasingRegionsFolder::new(self, param_env);
value.fold_with(&mut folder)
}
}

/// If you have a `Binder<'tcx, T>`, you can do this to strip out the
/// late-bound regions and then normalize the result, yielding up
/// a `T` (with regions erased). This is appropriate when the
Expand Down Expand Up @@ -91,11 +144,14 @@ struct NormalizeAfterErasingRegionsFolder<'tcx> {
}

impl<'tcx> NormalizeAfterErasingRegionsFolder<'tcx> {
#[instrument(skip(self), level = "debug")]
fn normalize_generic_arg_after_erasing_regions(
&self,
arg: ty::GenericArg<'tcx>,
) -> ty::GenericArg<'tcx> {
let arg = self.param_env.and(arg);
debug!(?arg);

self.tcx.normalize_generic_arg_after_erasing_regions(arg)
}
}
Expand Down Expand Up @@ -126,3 +182,62 @@ impl TypeFolder<'tcx> for NormalizeAfterErasingRegionsFolder<'tcx> {
Ok(self.tcx.normalize_mir_const_after_erasing_regions(arg))
}
}

struct TryNormalizeAfterErasingRegionsFolder<'tcx> {
tcx: TyCtxt<'tcx>,
param_env: ty::ParamEnv<'tcx>,
}

impl<'tcx> TryNormalizeAfterErasingRegionsFolder<'tcx> {
fn new(tcx: TyCtxt<'tcx>, param_env: ty::ParamEnv<'tcx>) -> Self {
TryNormalizeAfterErasingRegionsFolder { tcx, param_env }
}

#[instrument(skip(self), level = "debug")]
fn try_normalize_generic_arg_after_erasing_regions(
&self,
arg: ty::GenericArg<'tcx>,
) -> Result<ty::GenericArg<'tcx>, NoSolution> {
let arg = self.param_env.and(arg);
debug!(?arg);

self.tcx.try_normalize_generic_arg_after_erasing_regions(arg)
}
}

impl TypeFolder<'tcx> for TryNormalizeAfterErasingRegionsFolder<'tcx> {
type Error = NormalizationError<'tcx>;

fn tcx(&self) -> TyCtxt<'tcx> {
self.tcx
}

fn fold_ty(&mut self, ty: Ty<'tcx>) -> Result<Ty<'tcx>, Self::Error> {
match self.try_normalize_generic_arg_after_erasing_regions(ty.into()) {
Ok(t) => Ok(t.expect_ty()),
Err(_) => Err(NormalizationError::Type(ty)),
}
}

fn fold_const(
&mut self,
c: &'tcx ty::Const<'tcx>,
) -> Result<&'tcx ty::Const<'tcx>, Self::Error> {
match self.try_normalize_generic_arg_after_erasing_regions(c.into()) {
Ok(t) => Ok(t.expect_const()),
Err(_) => Err(NormalizationError::Const(*c)),
}
}

fn fold_mir_const(
&mut self,
c: mir::ConstantKind<'tcx>,
) -> Result<mir::ConstantKind<'tcx>, Self::Error> {
// FIXME: This *probably* needs canonicalization too!
let arg = self.param_env.and(c);
match self.tcx.try_normalize_mir_const_after_erasing_regions(arg) {
Ok(c) => Ok(c),
Err(_) => Err(NormalizationError::ConstantKind(c)),
}
}
}
40 changes: 40 additions & 0 deletions compiler/rustc_traits/src/normalize_erasing_regions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,14 @@ crate fn provide(p: &mut Providers) {
normalize_mir_const_after_erasing_regions: |tcx, goal| {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this query be implemented in terms of try_normalize_mir_const_after_erasing_regions? What about normalize_generic_arg_after_erasing_regions and try_normalize_generic_arg_after_erasing_regions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

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 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
};
}
Expand Down Expand Up @@ -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,
Expand Down
7 changes: 7 additions & 0 deletions src/librustdoc/html/render/print_item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1769,6 +1769,13 @@ fn document_type_layout(w: &mut Buffer, cx: &Context<'_>, ty_def_id: DefId) {
the type was too big.</p>"
);
}
Err(LayoutError::NormalizationFailure(_, _)) => {
writeln!(
w,
"<p><strong>Note:</strong> Encountered an error during type layout; \
the type failed to be normalized.</p>"
)
}
}

writeln!(w, "</div>");
Expand Down
26 changes: 26 additions & 0 deletions src/test/ui/associated-types/issue-59324.rs
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() {}
Loading