Skip to content

Commit

Permalink
move leak-check to during coherence, candidate eval
Browse files Browse the repository at this point in the history
In particular, it no longer occurs during the subtyping check. This is
important for enabling lazy normalization, because the subtyping check
will be producing sub-obligations that could affect its results.

Consider an example like

    for<'a> fn(<&'a as Mirror>::Item) =
      fn(&'b u8)

where `<T as Mirror>::Item = T` for all `T`. We will wish to produce a
new subobligation like

    <'!1 as Mirror>::Item = &'b u8

This will, after being solved, ultimately yield a constraint that `'!1
= 'b` which will fail. But with the leak-check being performed on
subtyping, there is no opportunity to normalize `<'!1 as
Mirror>::Item` (unless we invoke that normalization directly from
within subtyping, and I would prefer that subtyping and unification
are distinct operations rather than part of the trait solving stack).

The reason to keep the leak check during coherence and trait
evaluation is partly for backwards compatibility. The coherence change
permits impls for `fn(T)` and `fn(&T)` to co-exist, and the trait
evaluation change means that we can distinguish those two cases
without ambiguity errors. It also avoids recreating #57639, where we
were incorrectly choosing a where clause that would have failed the
leak check over the impl which succeeds.

The other reason to keep the leak check in those places is that I
think it is actually close to the model we want. To the point, I think
the trait solver ought to have the job of "breaking down"
higher-ranked region obligation like ``!1: '2` into into region
obligations that operate on things in the root universe, at which
point they should be handed off to polonius. The leak check isn't
*really* doing that -- these obligations are still handed to the
region solver to process -- but if/when we do adopt that model, the
decision to pass/fail would be happening in roughly this part of the
code.

This change had somewhat more side-effects than I anticipated. It
seems like there are cases where the leak-check was not being enforced
during method proving and trait selection. I haven't quite tracked
this down but I think it ought to be documented, so that we know what
precisely we are committing to.

One surprising test was `issue-30786.rs`. The behavior there seems a
bit "fishy" to me, but the problem is not related to the leak check
change as far as I can tell, but more to do with the closure signature
inference code and perhaps the associated type projection, which
together seem to be conspiring to produce an unexpected
signature. Nonetheless, it is an example of where changing the
leak-check can have some unexpected consequences: we're now failing to
resolve a method earlier than we were, which suggests we might change
some method resolutions that would have been ambiguous to be
successful.

TODO:

* figure out remainig test failures
* add new coherence tests for the patterns we ARE disallowing
  • Loading branch information
nikomatsakis committed Jun 22, 2020
1 parent f2cf994 commit 5a7a850
Show file tree
Hide file tree
Showing 60 changed files with 511 additions and 576 deletions.
18 changes: 14 additions & 4 deletions src/librustc_infer/infer/higher_ranked/mod.rs
Expand Up @@ -30,7 +30,7 @@ impl<'a, 'tcx> CombineFields<'a, 'tcx> {

let span = self.trace.cause.span;

self.infcx.commit_if_ok(|snapshot| {
self.infcx.commit_if_ok(|_| {
// First, we instantiate each bound region in the supertype with a
// fresh placeholder region.
let (b_prime, _) = self.infcx.replace_bound_vars_with_placeholders(b);
Expand All @@ -48,8 +48,6 @@ impl<'a, 'tcx> CombineFields<'a, 'tcx> {
// Compare types now that bound regions have been replaced.
let result = self.sub(a_is_expected).relate(&a_prime, &b_prime)?;

self.infcx.leak_check(!a_is_expected, snapshot)?;

debug!("higher_ranked_sub: OK result={:?}", result);

Ok(ty::Binder::bind(result))
Expand All @@ -75,7 +73,12 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
where
T: TypeFoldable<'tcx>,
{
let next_universe = self.create_next_universe();
// Figure out what the next universe will be, but don't actually create
// it until after we've done the substitution (in particular there may
// be no bound variables). This is a performance optimization, since the
// leak check for example can be skipped if no new universes are created
// (i.e., if there are no placeholders).
let next_universe = self.universe().next_universe();

let fld_r = |br| {
self.tcx.mk_region(ty::RePlaceholder(ty::PlaceholderRegion {
Expand Down Expand Up @@ -103,6 +106,13 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {

let (result, map) = self.tcx.replace_bound_vars(binder, fld_r, fld_t, fld_c);

// If there were higher-ranked regions to replace, then actually create
// the next universe (this avoids needlessly creating universes).
if !map.is_empty() {
let n_u = self.create_next_universe();
assert_eq!(n_u, next_universe);
}

debug!(
"replace_bound_vars_with_placeholders(\
next_universe={:?}, \
Expand Down
1 change: 1 addition & 0 deletions src/librustc_infer/infer/region_constraints/leak_check.rs
Expand Up @@ -286,6 +286,7 @@ impl<'me, 'tcx> LeakCheck<'me, 'tcx> {
placeholder: ty::PlaceholderRegion,
other_region: ty::Region<'tcx>,
) -> TypeError<'tcx> {
debug!("error: placeholder={:?}, other_region={:?}", placeholder, other_region);
if self.overly_polymorphic {
return TypeError::RegionsOverlyPolymorphic(placeholder.name, other_region);
} else {
Expand Down
10 changes: 9 additions & 1 deletion src/librustc_trait_selection/traits/coherence.rs
Expand Up @@ -120,12 +120,13 @@ fn overlap<'cx, 'tcx>(
debug!("overlap(a_def_id={:?}, b_def_id={:?})", a_def_id, b_def_id);

selcx.infcx().probe_maybe_skip_leak_check(skip_leak_check.is_yes(), |snapshot| {
overlap_within_probe(selcx, a_def_id, b_def_id, snapshot)
overlap_within_probe(selcx, skip_leak_check, a_def_id, b_def_id, snapshot)
})
}

fn overlap_within_probe(
selcx: &mut SelectionContext<'cx, 'tcx>,
skip_leak_check: SkipLeakCheck,
a_def_id: DefId,
b_def_id: DefId,
snapshot: &CombinedSnapshot<'_, 'tcx>,
Expand Down Expand Up @@ -180,6 +181,13 @@ fn overlap_within_probe(
return None;
}

if !skip_leak_check.is_yes() {
if let Err(_) = infcx.leak_check(true, snapshot) {
debug!("overlap: leak check failed");
return None;
}
}

let impl_header = selcx.infcx().resolve_vars_if_possible(&a_impl_header);
let intercrate_ambiguity_causes = selcx.take_intercrate_ambiguity_causes();
debug!("overlap: intercrate_ambiguity_causes={:#?}", intercrate_ambiguity_causes);
Expand Down
6 changes: 1 addition & 5 deletions src/librustc_trait_selection/traits/project.rs
Expand Up @@ -298,11 +298,7 @@ impl<'a, 'b, 'tcx> AssocTypeNormalizer<'a, 'b, 'tcx> {
fn fold<T: TypeFoldable<'tcx>>(&mut self, value: &T) -> T {
let value = self.selcx.infcx().resolve_vars_if_possible(value);

if !value.has_projections() {
value
} else {
value.fold_with(self)
}
if !value.has_projections() { value } else { value.fold_with(self) }
}
}

Expand Down
12 changes: 7 additions & 5 deletions src/librustc_trait_selection/traits/select/mod.rs
Expand Up @@ -347,6 +347,12 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
) -> Result<EvaluationResult, OverflowError> {
self.infcx.probe(|snapshot| -> Result<EvaluationResult, OverflowError> {
let result = op(self)?;

match self.infcx.leak_check(true, snapshot) {
Ok(()) => {}
Err(_) => return Ok(EvaluatedToErr),
}

match self.infcx.region_constraints_added_in_snapshot(snapshot) {
None => Ok(result),
Some(_) => Ok(result.max(EvaluatedToOkModuloRegions)),
Expand Down Expand Up @@ -2402,11 +2408,7 @@ impl<'o, 'tcx> TraitObligationStackList<'o, 'tcx> {
}

fn depth(&self) -> usize {
if let Some(head) = self.head {
head.depth
} else {
0
}
if let Some(head) = self.head { head.depth } else { 0 }
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/associated-types/associated-types-eq-hr.stderr
Expand Up @@ -74,7 +74,7 @@ LL | where T : for<'x,'y> TheTrait<(&'x isize, &'y isize), A = &'y isize>
| ------------- required by this bound in `tuple_two`
...
LL | tuple_two::<Tuple>();
| ^^^^^^^^^^^^^^^^^^ expected bound lifetime parameter 'y, found concrete lifetime
| ^^^^^^^^^^^^^^^^^^ expected bound lifetime parameter 'x, found concrete lifetime

error[E0277]: the trait bound `for<'x, 'y> Tuple: TheTrait<(&'x isize, &'y isize)>` is not satisfied
--> $DIR/associated-types-eq-hr.rs:107:18
Expand Down
@@ -1,23 +1,23 @@
error[E0623]: lifetime mismatch
--> $DIR/project-fn-ret-invariant.rs:53:21
--> $DIR/project-fn-ret-invariant.rs:54:22
|
LL | fn transmute<'a,'b>(x: Type<'a>, y: Type<'b>) -> (Type<'a>, Type<'b>) {
| -------- --------------------
| |
| this parameter and the return type are declared with different lifetimes...
LL | let a = bar(foo, y);
| ^ ...but data from `x` is returned here
LL | fn transmute<'a, 'b>(x: Type<'a>, y: Type<'b>) -> (Type<'a>, Type<'b>) {
| -------- --------------------
| |
| this parameter and the return type are declared with different lifetimes...
LL | let a = bar(foo, y);
| ^ ...but data from `x` is returned here

error[E0623]: lifetime mismatch
--> $DIR/project-fn-ret-invariant.rs:54:21
--> $DIR/project-fn-ret-invariant.rs:56:9
|
LL | fn transmute<'a,'b>(x: Type<'a>, y: Type<'b>) -> (Type<'a>, Type<'b>) {
| -------- --------------------
| |
| this parameter and the return type are declared with different lifetimes...
LL | let a = bar(foo, y);
LL | let b = bar(foo, x);
| ^ ...but data from `y` is returned here
LL | fn transmute<'a, 'b>(x: Type<'a>, y: Type<'b>) -> (Type<'a>, Type<'b>) {
| -------- --------------------
| |
| this parameter and the return type are declared with different lifetimes...
...
LL | (a, b)
| ^ ...but data from `x` is returned here

error: aborting due to 2 previous errors

Expand Down
@@ -1,8 +1,8 @@
error: fatal error triggered by #[rustc_error]
--> $DIR/project-fn-ret-invariant.rs:59:1
--> $DIR/project-fn-ret-invariant.rs:60:1
|
LL | fn main() { }
| ^^^^^^^^^^^^^
LL | fn main() {}
| ^^^^^^^^^^^^

error: aborting due to previous error

@@ -1,13 +1,13 @@
error[E0623]: lifetime mismatch
--> $DIR/project-fn-ret-invariant.rs:39:19
--> $DIR/project-fn-ret-invariant.rs:40:20
|
LL | fn baz<'a,'b>(x: Type<'a>, y: Type<'b>) -> (Type<'a>, Type<'b>) {
| -------- --------------------
| |
| this parameter and the return type are declared with different lifetimes...
LL | fn baz<'a, 'b>(x: Type<'a>, y: Type<'b>) -> (Type<'a>, Type<'b>) {
| -------- --------------------
| |
| this parameter and the return type are declared with different lifetimes...
...
LL | let b = bar(f, y);
| ^ ...but data from `x` is returned here
LL | let b = bar(f, y);
| ^ ...but data from `x` is returned here

error: aborting due to previous error

Expand Down
41 changes: 21 additions & 20 deletions src/test/ui/associated-types/cache/project-fn-ret-invariant.rs
@@ -1,60 +1,61 @@
#![feature(unboxed_closures)]
#![feature(rustc_attrs)]

// Test for projection cache. We should be able to project distinct
// lifetimes from `foo` as we reinstantiate it multiple times, but not
// if we do it just once. In this variant, the region `'a` is used in
// an invariant position, which affects the results.

// revisions: ok oneuse transmute krisskross

#![allow(dead_code, unused_variables)]

use std::marker::PhantomData;

struct Type<'a> {
// Invariant
data: PhantomData<fn(&'a u32) -> &'a u32>
data: PhantomData<fn(&'a u32) -> &'a u32>,
}

fn foo<'a>() -> Type<'a> { loop { } }
fn foo<'a>() -> Type<'a> {
loop {}
}

fn bar<T>(t: T, x: T::Output) -> T::Output
where T: FnOnce<()>
where
T: FnOnce<()>,
{
t()
}

#[cfg(ok)] // two instantiations: OK
fn baz<'a,'b>(x: Type<'a>, y: Type<'b>) -> (Type<'a>, Type<'b>) {
fn baz<'a, 'b>(x: Type<'a>, y: Type<'b>) -> (Type<'a>, Type<'b>) {
let a = bar(foo, x);
let b = bar(foo, y);
(a, b)
}

#[cfg(oneuse)] // one instantiation: BAD
fn baz<'a,'b>(x: Type<'a>, y: Type<'b>) -> (Type<'a>, Type<'b>) {
let f = foo; // <-- No consistent type can be inferred for `f` here.
let a = bar(f, x);
let b = bar(f, y); //[oneuse]~ ERROR lifetime mismatch [E0623]
(a, b)
fn baz<'a, 'b>(x: Type<'a>, y: Type<'b>) -> (Type<'a>, Type<'b>) {
let f = foo; // <-- No consistent type can be inferred for `f` here.
let a = bar(f, x);
let b = bar(f, y); //[oneuse]~ ERROR lifetime mismatch [E0623]
(a, b)
}

#[cfg(transmute)] // one instantiations: BAD
fn baz<'a,'b>(x: Type<'a>) -> Type<'static> {
// Cannot instantiate `foo` with any lifetime other than `'a`,
// since it is provided as input.
fn baz<'a, 'b>(x: Type<'a>) -> Type<'static> {
// Cannot instantiate `foo` with any lifetime other than `'a`,
// since it is provided as input.

bar(foo, x) //[transmute]~ ERROR E0495
bar(foo, x) //[transmute]~ ERROR E0495
}

#[cfg(krisskross)] // two instantiations, mixing and matching: BAD
fn transmute<'a,'b>(x: Type<'a>, y: Type<'b>) -> (Type<'a>, Type<'b>) {
let a = bar(foo, y); //[krisskross]~ ERROR E0623
let b = bar(foo, x); //[krisskross]~ ERROR E0623
(a, b)
fn transmute<'a, 'b>(x: Type<'a>, y: Type<'b>) -> (Type<'a>, Type<'b>) {
let a = bar(foo, y); //[krisskross]~ ERROR E0623
let b = bar(foo, x);
(a, b) //[krisskross]~ ERROR E0623
}

#[rustc_error]
fn main() { }
fn main() {}
//[ok]~^ ERROR fatal error triggered by #[rustc_error]
@@ -1,27 +1,27 @@
error[E0495]: cannot infer an appropriate lifetime due to conflicting requirements
--> $DIR/project-fn-ret-invariant.rs:48:4
error[E0495]: cannot infer an appropriate lifetime for lifetime parameter `'a` due to conflicting requirements
--> $DIR/project-fn-ret-invariant.rs:49:9
|
LL | bar(foo, x)
| ^^^^^^^^^^^
LL | bar(foo, x)
| ^^^
|
note: first, the lifetime cannot outlive the lifetime `'a` as defined on the function body at 44:8...
--> $DIR/project-fn-ret-invariant.rs:44:8
note: first, the lifetime cannot outlive the lifetime `'a` as defined on the function body at 45:8...
--> $DIR/project-fn-ret-invariant.rs:45:8
|
LL | fn baz<'a,'b>(x: Type<'a>) -> Type<'static> {
LL | fn baz<'a, 'b>(x: Type<'a>) -> Type<'static> {
| ^^
note: ...so that the expression is assignable
--> $DIR/project-fn-ret-invariant.rs:48:13
--> $DIR/project-fn-ret-invariant.rs:49:14
|
LL | bar(foo, x)
| ^
LL | bar(foo, x)
| ^
= note: expected `Type<'_>`
found `Type<'a>`
= note: but, the lifetime must be valid for the static lifetime...
note: ...so that the expression is assignable
--> $DIR/project-fn-ret-invariant.rs:48:4
--> $DIR/project-fn-ret-invariant.rs:49:5
|
LL | bar(foo, x)
| ^^^^^^^^^^^
LL | bar(foo, x)
| ^^^^^^^^^^^
= note: expected `Type<'static>`
found `Type<'_>`

Expand Down
17 changes: 9 additions & 8 deletions src/test/ui/closure-expected-type/expect-fn-supply-fn.rs
@@ -1,10 +1,12 @@
fn with_closure_expecting_fn_with_free_region<F>(_: F)
where F: for<'a> FnOnce(fn(&'a u32), &i32)
where
F: for<'a> FnOnce(fn(&'a u32), &i32),
{
}

fn with_closure_expecting_fn_with_bound_region<F>(_: F)
where F: FnOnce(fn(&u32), &i32)
where
F: FnOnce(fn(&u32), &i32),
{
}

Expand All @@ -28,14 +30,14 @@ fn expect_free_supply_bound() {
// Here, we are given a function whose region is bound at closure level,
// but we expect one bound in the argument. Error results.
with_closure_expecting_fn_with_free_region(|x: fn(&u32), y| {});
//~^ ERROR type mismatch
//~^ ERROR mismatched types
}

fn expect_bound_supply_free_from_fn<'x>(x: &'x u32) {
// Here, we are given a `fn(&u32)` but we expect a `fn(&'x
// u32)`. In principle, this could be ok, but we demand equality.
with_closure_expecting_fn_with_bound_region(|x: fn(&'x u32), y| {});
//~^ ERROR type mismatch
//~^ ERROR mismatched types
}

fn expect_bound_supply_free_from_closure() {
Expand All @@ -44,16 +46,15 @@ fn expect_bound_supply_free_from_closure() {
// the argument level.
type Foo<'a> = fn(&'a u32);
with_closure_expecting_fn_with_bound_region(|x: Foo<'_>, y| {
//~^ ERROR type mismatch
//~^ ERROR mismatched types
});
}

fn expect_bound_supply_bound<'x>(x: &'x u32) {
// No error in this case. The supplied type supplies the bound
// regions, and hence we are able to figure out the type of `y`
// from the expected type
with_closure_expecting_fn_with_bound_region(|x: for<'z> fn(&'z u32), y| {
});
with_closure_expecting_fn_with_bound_region(|x: for<'z> fn(&'z u32), y| {});
}

fn main() { }
fn main() {}

0 comments on commit 5a7a850

Please sign in to comment.