Skip to content

revert #75443, update mir validator #78410

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

Merged
merged 2 commits into from
Nov 8, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
96 changes: 21 additions & 75 deletions compiler/rustc_mir/src/transform/validate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,16 @@ use crate::util::storage::AlwaysLiveLocals;

use super::MirPass;
use rustc_index::bit_set::BitSet;
use rustc_infer::infer::TyCtxtInferExt;
use rustc_middle::mir::interpret::Scalar;
use rustc_middle::mir::traversal;
use rustc_middle::mir::visit::{PlaceContext, Visitor};
use rustc_middle::mir::{
AggregateKind, BasicBlock, Body, BorrowKind, Local, Location, MirPhase, Operand, PlaceRef,
Rvalue, SourceScope, Statement, StatementKind, Terminator, TerminatorKind, VarDebugInfo,
};
use rustc_middle::ty::relate::{Relate, RelateResult, TypeRelation};
use rustc_middle::ty::{self, ParamEnv, Ty, TyCtxt};
use rustc_middle::ty::fold::BottomUpFolder;
use rustc_middle::ty::{self, ParamEnv, Ty, TyCtxt, TypeFoldable};
use rustc_target::abi::Size;

#[derive(Copy, Clone, Debug)]
Expand Down Expand Up @@ -77,79 +78,24 @@ pub fn equal_up_to_regions(
return true;
}

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

impl TypeRelation<'tcx> for LifetimeIgnoreRelation<'tcx> {
fn tcx(&self) -> TyCtxt<'tcx> {
self.tcx
}

fn param_env(&self) -> ty::ParamEnv<'tcx> {
self.param_env
}

fn tag(&self) -> &'static str {
"librustc_mir::transform::validate"
}

fn a_is_expected(&self) -> bool {
true
}

fn relate_with_variance<T: Relate<'tcx>>(
&mut self,
_: ty::Variance,
a: T,
b: T,
) -> RelateResult<'tcx, T> {
// Ignore variance, require types to be exactly the same.
self.relate(a, b)
}

fn tys(&mut self, a: Ty<'tcx>, b: Ty<'tcx>) -> RelateResult<'tcx, Ty<'tcx>> {
if a == b {
// Short-circuit.
return Ok(a);
}
ty::relate::super_relate_tys(self, a, b)
}

fn regions(
&mut self,
a: ty::Region<'tcx>,
_b: ty::Region<'tcx>,
) -> RelateResult<'tcx, ty::Region<'tcx>> {
// Ignore regions.
Ok(a)
}

fn consts(
&mut self,
a: &'tcx ty::Const<'tcx>,
b: &'tcx ty::Const<'tcx>,
) -> RelateResult<'tcx, &'tcx ty::Const<'tcx>> {
ty::relate::super_relate_consts(self, a, b)
}

fn binders<T>(
&mut self,
a: ty::Binder<T>,
b: ty::Binder<T>,
) -> RelateResult<'tcx, ty::Binder<T>>
where
T: Relate<'tcx>,
{
self.relate(a.skip_binder(), b.skip_binder())?;
Ok(a)
}
}

// Instantiate and run relation.
let mut relator: LifetimeIgnoreRelation<'tcx> = LifetimeIgnoreRelation { tcx: tcx, param_env };
relator.relate(src, dest).is_ok()
// Normalize lifetimes away on both sides, then compare.
let param_env = param_env.with_reveal_all_normalized(tcx);
let normalize = |ty: Ty<'tcx>| {
tcx.normalize_erasing_regions(
param_env,
ty.fold_with(&mut BottomUpFolder {
tcx,
// We just erase all late-bound lifetimes, but this is not fully correct (FIXME):
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 fully understand this comment. I'm not sure how much it matters, but it'd be nice to have a more detailed explanation of how lifetimes could matter. I guess it depends on a bit of context that I seem to be missing, about the broader role of this function.

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 the concern that @eddyb keeps raising; another instance of that is here. It's something about using lifetimes in invariant positions to force two different trait implementations to be used which could have different associated types which could make this check unsound. I don't have more than a fuzzy understanding of this, and I forgot if we ever were able to come up with a concrete example.

Copy link
Member

Choose a reason for hiding this comment

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

Apologies for not providing an example initially, given how trivial it is to construct "transmute in safe code" gadgets from associated type unsoundness (the hard part is you have to intentionally make rustc unsound enough to truly confirm something else doesn't defeat the example).
I've just posted #77196 (comment), hope that helps.

Copy link
Member

Choose a reason for hiding this comment

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

@eddyb also pointed out that we should probably clarify the comment to explicitly mention projections -- specifically, if I understood correctly, the concern is about erasing a lifetime in T when the type is T::Assoc.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, so the concern here is that we are not preserving the binding levels? Let me re-read this code then.

Copy link
Member

Choose a reason for hiding this comment

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

I see, so the concern here is that we are not preserving the binding levels? Let me re-read this code then.

Binding levels? I have no idea what you are talking about.^^

I guess the question is why we are not using an existing function to erase or normalize regions and instead rolling our own here.

I tried all the existing ones (that I could find) and none of them worked.^^ (As in, none of them accepted all the code out there.) Then I asked @eddyb what to do and we ended up with this.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know that it has to block this PR from landing

No, it certainly is not a blocker. This is an old FIXME that was removed when we switched to TypeRelation and now re-appears since we are switching back to BottomUpFolder.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I meant by "binding level" is that for<'a> fn(fn(&'a u32)) and fn(for<'a> fn(&'a u32)) are considered distinct types (and fn(&'a u32), where 'a appears free, is also distinct) by various parts of Rust. We've been slowly changing the model here but I think we'll not be able to change this completely. In particular, the coherence code accepts distinct impls (as eddyb demonstrated) for some such cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

see also #56105 and #72493

Copy link
Member

Choose a reason for hiding this comment

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

What I meant by "binding level" is that for<'a> fn(fn(&'a u32)) and fn(for<'a> fn(&'a u32)) are considered distinct types

Sure, it would be really strange if those would be the same type. The outer function in the latter may pick different lifetimes for different calls of the inner function; that is not the case for the former.

// lifetimes in invariant positions could matter (e.g. through associated types).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// lifetimes in invariant positions could matter (e.g. through associated types).
// lifetimes in projections could matter (e.g. `T<'a>::Assoc`).

We should clarify this comment, but I am not sure how to best do that.^^

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should add a link to #77196 (comment).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd probably inline the comment, but I agree we can improve the wording here. Something like:

FIXME: We erase all late-bound lifetimes, but this is not fully correct. If you have a type like
<for<'a> fn(&'a u32) as SomeTrait>::Assoc, this is not necessarily equivalent to <fn(&'static u32) as SomeTrait>::Assoc, since one may have an impl SomeTrait for fn(&32) and impl SomeTrait for fn(&'static u32) at the same time which specify distinct values for Assoc. (See also #56105)

// We rely on the fact that layout was confirmed to be equal above.
lt_op: |_| tcx.lifetimes.re_erased,
// Leave consts and types unchanged.
ct_op: |ct| ct,
ty_op: |ty| ty,
}),
)
};
tcx.infer_ctxt().enter(|infcx| infcx.can_eq(param_env, normalize(src), normalize(dest)).is_ok())
Copy link
Member

Choose a reason for hiding this comment

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

This seems reasonable -- it is pretty close to what I had before we went for TypeRelation, except for this infcx.can_eq thing which I never saw before so I cannot comment on it.^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using normalize(src) == normalize(dest) would fail to unify 2 generic consts, so the following would fail mir validation

#[feature(const_generics, lazy_normalization_consts)]
fn test<const N: usize>() -> [u8; N + 1] {
    [0; N + 1]
}

Copy link
Member

@RalfJung RalfJung Oct 27, 2020

Choose a reason for hiding this comment

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

So generic consts are not interned the same way as other types? Or what does the difference stem from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the 2 N + 1 are two distinct mir bodies so we have ConstKind::Unevaluated(DefId1, substs: [N]) and ConstKind::Unevaluated(DefId2, substs: [N]) which should still be considered equal. This doesn't really exist for types afaik as we can partially normalize them.

We may want to change this in the future but I don't yet know how we would do that

Copy link
Contributor

Choose a reason for hiding this comment

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

"which would still be considered unequal", you mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ConstKind::Unevaluated(DefId1, substs: [N]) and ConstKind::Unevaluated(DefId2, substs: [N]) are considered unequal when using Ty == Ty atm but are - and should be - considered equal with feature(const_evaluatable_checked).

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Yes, that makes sense. The point here is that normalize doesn't do any kind of evaluation of unevaluated constants, so simple equality is not especially meaningful.

}

struct TypeChecker<'a, 'tcx> {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_trait_selection/src/traits/project.rs
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ impl<'a, 'b, 'tcx> TypeFolder<'tcx> for AssocTypeNormalizer<'a, 'b, 'tcx> {

let ty = ty.super_fold_with(self);
match *ty.kind() {
ty::Opaque(def_id, substs) => {
ty::Opaque(def_id, substs) if !substs.has_escaping_bound_vars() => {
// Only normalize `impl Trait` after type-checking, usually in codegen.
match self.param_env.reveal() {
Reveal::UserFacing => ty,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ impl<'cx, 'tcx> TypeFolder<'tcx> for QueryNormalizer<'cx, 'tcx> {

let ty = ty.super_fold_with(self);
let res = (|| match *ty.kind() {
ty::Opaque(def_id, substs) => {
ty::Opaque(def_id, substs) if !substs.has_escaping_bound_vars() => {
// Only normalize `impl Trait` after type-checking, usually in codegen.
match self.param_env.reveal() {
Reveal::UserFacing => ty,
Expand Down
27 changes: 0 additions & 27 deletions src/test/ui/type-alias-impl-trait/issue-72793.rs

This file was deleted.