Skip to content

Commit

Permalink
Auto merge of #99217 - lcnr:implied-bounds-pre-norm, r=lcnr
Browse files Browse the repository at this point in the history
consider unnormalized types for implied bounds

extracted, and slightly modified, from #98900

The idea here is that generally, rustc is split into things which can assume its inputs are well formed[^1], and things which have verify that themselves.

Generally most predicates should only deal with well formed inputs, e.g. a `&'a &'b (): Trait` predicate should be able to assume that `'b: 'a` holds. Normalization can loosen wf requirements (see #91068) and must therefore not be used in places which still have to check well formedness. The only such place should hopefully be `WellFormed` predicates

fixes #87748 and #98543

r? `@jackh726` cc `@rust-lang/types`

[^1]: These places may still encounter non-wf inputs and have to deal with them without causing an ICE as we may check for well formedness out of order.
  • Loading branch information
bors committed Aug 9, 2022
2 parents 6d3f1be + 8691b96 commit 63e4312
Show file tree
Hide file tree
Showing 28 changed files with 268 additions and 102 deletions.
38 changes: 23 additions & 15 deletions compiler/rustc_borrowck/src/type_check/constraint_conversion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use rustc_infer::infer::region_constraints::{GenericKind, VerifyBound};
use rustc_infer::infer::{self, InferCtxt, SubregionOrigin};
use rustc_middle::mir::ConstraintCategory;
use rustc_middle::ty::subst::GenericArgKind;
use rustc_middle::ty::TypeVisitable;
use rustc_middle::ty::TypeFoldable;
use rustc_middle::ty::{self, TyCtxt};
use rustc_span::{Span, DUMMY_SP};

Expand Down Expand Up @@ -109,23 +109,11 @@ impl<'a, 'tcx> ConstraintConversion<'a, 'tcx> {
self.add_outlives(r1_vid, r2_vid);
}

GenericArgKind::Type(mut t1) => {
GenericArgKind::Type(t1) => {
// we don't actually use this for anything, but
// the `TypeOutlives` code needs an origin.
let origin = infer::RelateParamBound(DUMMY_SP, t1, None);

// Placeholder regions need to be converted now because it may
// create new region variables, which can't be done later when
// verifying these bounds.
if t1.has_placeholders() {
t1 = tcx.fold_regions(t1, |r, _| match *r {
ty::RePlaceholder(placeholder) => {
self.constraints.placeholder_region(self.infcx, placeholder)
}
_ => r,
});
}

TypeOutlives::new(
&mut *self,
tcx,
Expand All @@ -143,14 +131,32 @@ impl<'a, 'tcx> ConstraintConversion<'a, 'tcx> {
}
}

/// Placeholder regions need to be converted eagerly because it may
/// create new region variables, which we must not do when verifying
/// our region bounds.
///
/// FIXME: This should get removed once higher ranked region obligations
/// are dealt with during trait solving.
fn replace_placeholders_with_nll<T: TypeFoldable<'tcx>>(&mut self, value: T) -> T {
if value.has_placeholders() {
self.tcx.fold_regions(value, |r, _| match *r {
ty::RePlaceholder(placeholder) => {
self.constraints.placeholder_region(self.infcx, placeholder)
}
_ => r,
})
} else {
value
}
}

fn verify_to_type_test(
&mut self,
generic_kind: GenericKind<'tcx>,
region: ty::Region<'tcx>,
verify_bound: VerifyBound<'tcx>,
) -> TypeTest<'tcx> {
let lower_bound = self.to_region_vid(region);

TypeTest { generic_kind, lower_bound, locations: self.locations, verify_bound }
}

Expand Down Expand Up @@ -198,6 +204,8 @@ impl<'a, 'b, 'tcx> TypeOutlivesDelegate<'tcx> for &'a mut ConstraintConversion<'
a: ty::Region<'tcx>,
bound: VerifyBound<'tcx>,
) {
let kind = self.replace_placeholders_with_nll(kind);
let bound = self.replace_placeholders_with_nll(bound);
let type_test = self.verify_to_type_test(kind, a, bound);
self.add_type_test(type_test);
}
Expand Down
12 changes: 6 additions & 6 deletions compiler/rustc_borrowck/src/type_check/free_region_relations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -242,10 +242,9 @@ impl<'tcx> UniversalRegionRelationsBuilder<'_, 'tcx> {
let constraint_sets: Vec<_> = unnormalized_input_output_tys
.flat_map(|ty| {
debug!("build: input_or_output={:?}", ty);
// We only add implied bounds for the normalized type as the unnormalized
// type may not actually get checked by the caller.
//
// Can otherwise be unsound, see #91068.
// We add implied bounds from both the unnormalized and normalized ty.
// See issue #87748
let constraints_implied1 = self.add_implied_bounds(ty);
let TypeOpOutput { output: norm_ty, constraints: constraints1, .. } = self
.param_env
.and(type_op::normalize::Normalize::new(ty))
Expand Down Expand Up @@ -273,9 +272,10 @@ impl<'tcx> UniversalRegionRelationsBuilder<'_, 'tcx> {
// }
// ```
// Both &Self::Bar and &() are WF
let constraints_implied = self.add_implied_bounds(norm_ty);
let constraints_implied2 =
if ty != norm_ty { self.add_implied_bounds(norm_ty) } else { None };
normalized_inputs_and_output.push(norm_ty);
constraints1.into_iter().chain(constraints_implied)
constraints1.into_iter().chain(constraints_implied1).chain(constraints_implied2)
})
.collect();

Expand Down
13 changes: 10 additions & 3 deletions compiler/rustc_borrowck/src/type_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1448,16 +1448,23 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
))
});
debug!(?sig);
let sig = self.normalize(sig, term_location);
self.check_call_dest(body, term, &sig, *destination, target, term_location);

// IMPORTANT: We have to prove well formed for the function signature before
// we normalize it, as otherwise types like `<&'a &'b () as Trait>::Assoc`
// get normalized away, causing us to ignore the `'b: 'a` bound used by the function.
//
// Normalization results in a well formed type if the input is well formed, so we
// don't have to check it twice.
//
// See #91068 for an example.
self.prove_predicates(
sig.inputs_and_output
.iter()
.map(|ty| ty::Binder::dummy(ty::PredicateKind::WellFormed(ty.into()))),
term_location.to_locations(),
ConstraintCategory::Boring,
);
let sig = self.normalize(sig, term_location);
self.check_call_dest(body, term, &sig, *destination, target, term_location);

// The ordinary liveness rules will ensure that all
// regions in the type of the callee are live here. We
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_infer/src/infer/region_constraints/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ pub enum GenericKind<'tcx> {
/// }
/// ```
/// This is described with an `AnyRegion('a, 'b)` node.
#[derive(Debug, Clone)]
#[derive(Debug, Clone, TypeFoldable, TypeVisitable)]
pub enum VerifyBound<'tcx> {
/// See [`VerifyIfEq`] docs
IfEq(ty::Binder<'tcx, VerifyIfEq<'tcx>>),
Expand Down
23 changes: 23 additions & 0 deletions compiler/rustc_middle/src/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -594,6 +594,29 @@ impl<'tcx> Predicate<'tcx> {
}
self
}

/// Whether this projection can be soundly normalized.
///
/// Wf predicates must not be normalized, as normalization
/// can remove required bounds which would cause us to
/// unsoundly accept some programs. See #91068.
#[inline]
pub fn allow_normalization(self) -> bool {
match self.kind().skip_binder() {
PredicateKind::WellFormed(_) => false,
PredicateKind::Trait(_)
| PredicateKind::RegionOutlives(_)
| PredicateKind::TypeOutlives(_)
| PredicateKind::Projection(_)
| PredicateKind::ObjectSafe(_)
| PredicateKind::ClosureKind(_, _, _)
| PredicateKind::Subtype(_)
| PredicateKind::Coerce(_)
| PredicateKind::ConstEvaluatable(_)
| PredicateKind::ConstEquate(_, _)
| PredicateKind::TypeWellFormedFromEnv(_) => true,
}
}
}

impl<'a, 'tcx> HashStable<StableHashingContext<'a>> for Predicate<'tcx> {
Expand Down
9 changes: 9 additions & 0 deletions compiler/rustc_trait_selection/src/traits/project.rs
Original file line number Diff line number Diff line change
Expand Up @@ -619,6 +619,15 @@ impl<'a, 'b, 'tcx> TypeFolder<'tcx> for AssocTypeNormalizer<'a, 'b, 'tcx> {
constant.eval(self.selcx.tcx(), self.param_env)
}
}

#[inline]
fn fold_predicate(&mut self, p: ty::Predicate<'tcx>) -> ty::Predicate<'tcx> {
if p.allow_normalization() && needs_normalization(&p, self.param_env.reveal()) {
p.super_fold_with(self)
} else {
p
}
}
}

pub struct BoundVarReplacer<'me, 'tcx> {
Expand Down
12 changes: 12 additions & 0 deletions compiler/rustc_trait_selection/src/traits/query/normalize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -351,4 +351,16 @@ impl<'cx, 'tcx> FallibleTypeFolder<'tcx> for QueryNormalizer<'cx, 'tcx> {
mir::ConstantKind::Val(_, _) => constant.try_super_fold_with(self)?,
})
}

#[inline]
fn try_fold_predicate(
&mut self,
p: ty::Predicate<'tcx>,
) -> Result<ty::Predicate<'tcx>, Self::Error> {
if p.allow_normalization() && needs_normalization(&p, self.param_env.reveal()) {
p.try_super_fold_with(self)
} else {
Ok(p)
}
}
}
7 changes: 6 additions & 1 deletion compiler/rustc_typeck/src/check/compare_method.rs
Original file line number Diff line number Diff line change
Expand Up @@ -264,8 +264,13 @@ fn compare_predicate_entailment<'tcx>(

let trait_sig = tcx.bound_fn_sig(trait_m.def_id).subst(tcx, trait_to_placeholder_substs);
let trait_sig = tcx.liberate_late_bound_regions(impl_m.def_id, trait_sig);
// Next, add all inputs and output as well-formed tys. Importantly,
// we have to do this before normalization, since the normalized ty may
// not contain the input parameters. See issue #87748.
wf_tys.extend(trait_sig.inputs_and_output.iter());
let trait_sig = ocx.normalize(norm_cause, param_env, trait_sig);
// Add the resulting inputs and output as well-formed.
// We also have to add the normalized trait signature
// as we don't normalize during implied bounds computation.
wf_tys.extend(trait_sig.inputs_and_output.iter());
let trait_fty = tcx.mk_fn_ptr(ty::Binder::dummy(trait_sig));

Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/associated-types/issue-59324.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ pub trait ThriftService<Bug: NotFoo>:
{
fn get_service(
//~^ ERROR the trait bound `Bug: Foo` is not satisfied
//~| ERROR the trait bound `Bug: Foo` is not satisfied
&self,
) -> Self::AssocType;
//~^ the trait bound `Bug: Foo` is not satisfied
}

fn with_factory<H>(factory: dyn ThriftService<()>) {}
Expand Down
9 changes: 5 additions & 4 deletions src/test/ui/associated-types/issue-59324.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ LL | |
LL | |
LL | | Service<AssocType = <Bug as Foo>::OnlyFoo>
... |
LL | |
LL | | ) -> Self::AssocType;
LL | | }
| |_^ the trait `Foo` is not implemented for `Bug`
|
Expand All @@ -34,6 +34,7 @@ error[E0277]: the trait bound `Bug: Foo` is not satisfied
|
LL | / fn get_service(
LL | |
LL | |
LL | | &self,
LL | | ) -> Self::AssocType;
| |_________________________^ the trait `Foo` is not implemented for `Bug`
Expand All @@ -50,10 +51,10 @@ LL | fn with_factory<H>(factory: dyn ThriftService<()>) {}
| ^^^^^^^^^^^^^^^^^^^^^ the trait `Foo` is not implemented for `()`

error[E0277]: the trait bound `Bug: Foo` is not satisfied
--> $DIR/issue-59324.rs:19:10
--> $DIR/issue-59324.rs:16:8
|
LL | ) -> Self::AssocType;
| ^^^^^^^^^^^^^^^ the trait `Foo` is not implemented for `Bug`
LL | fn get_service(
| ^^^^^^^^^^^ the trait `Foo` is not implemented for `Bug`
|
help: consider further restricting this bound
|
Expand Down
3 changes: 2 additions & 1 deletion src/test/ui/fn/implied-bounds-unnorm-associated-type-2.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// check-pass
// check-fail

trait Trait {
type Type;
Expand All @@ -17,6 +17,7 @@ where

fn g<'a, 'b>() {
f::<'a, 'b>(());
//~^ ERROR lifetime may not live long enough
}

fn main() {}
17 changes: 17 additions & 0 deletions src/test/ui/fn/implied-bounds-unnorm-associated-type-2.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
error: lifetime may not live long enough
--> $DIR/implied-bounds-unnorm-associated-type-2.rs:19:5
|
LL | fn g<'a, 'b>() {
| -- -- lifetime `'b` defined here
| |
| lifetime `'a` defined here
LL | f::<'a, 'b>(());
| ^^^^^^^^^^^^^^^ requires that `'b` must outlive `'a`
|
= help: consider adding the following bound: `'b: 'a`
= note: requirement occurs because of a function pointer to `f`
= note: the function `f` is invariant over the parameter `'a`
= help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance

error: aborting due to previous error

5 changes: 1 addition & 4 deletions src/test/ui/fn/implied-bounds-unnorm-associated-type-3.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
// check-fail
// See issue #91899. If we treat unnormalized args as WF, `Self` can also be a
// source of unsoundness.
// check-pass

pub trait Yokeable<'a>: 'static {
type Output: 'a;
Expand All @@ -17,7 +15,6 @@ pub trait ZeroCopyFrom<C: ?Sized>: for<'a> Yokeable<'a> {

impl<T> ZeroCopyFrom<[T]> for &'static [T] {
fn zero_copy_from<'b>(cart: &'b [T]) -> &'b [T] {
//~^ the parameter
cart
}
}
Expand Down
14 changes: 0 additions & 14 deletions src/test/ui/fn/implied-bounds-unnorm-associated-type-3.stderr

This file was deleted.

24 changes: 24 additions & 0 deletions src/test/ui/fn/implied-bounds-unnorm-associated-type-4.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// A regression test for #98543

trait Trait {
type Type;
}

impl<T> Trait for T {
type Type = ();
}

fn f<'a, 'b>(s: &'b str, _: <&'a &'b () as Trait>::Type) -> &'a str
where
&'a &'b (): Trait, // <- adding this bound is the change from #91068
{
s
}

fn main() {
let x = String::from("Hello World!");
let y = f(&x, ());
drop(x);
//~^ ERROR cannot move out of `x` because it is borrowed
println!("{}", y);
}
14 changes: 14 additions & 0 deletions src/test/ui/fn/implied-bounds-unnorm-associated-type-4.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error[E0505]: cannot move out of `x` because it is borrowed
--> $DIR/implied-bounds-unnorm-associated-type-4.rs:21:10
|
LL | let y = f(&x, ());
| -- borrow of `x` occurs here
LL | drop(x);
| ^ move out of `x` occurs here
LL |
LL | println!("{}", y);
| - borrow later used here

error: aborting due to previous error

For more information about this error, try `rustc --explain E0505`.
23 changes: 23 additions & 0 deletions src/test/ui/fn/implied-bounds-unnorm-associated-type-5.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
trait Trait<'a>: 'a {
type Type;
}

// if the `T: 'a` bound gets implied we would probably get ub here again
impl<'a, T> Trait<'a> for T {
//~^ ERROR the parameter type `T` may not live long enough
type Type = ();
}

fn f<'a, 'b>(s: &'b str, _: <&'b () as Trait<'a>>::Type) -> &'a str
where
&'b (): Trait<'a>,
{
s
}

fn main() {
let x = String::from("Hello World!");
let y = f(&x, ());
drop(x);
println!("{}", y);
}
Loading

0 comments on commit 63e4312

Please sign in to comment.