Skip to content

Commit

Permalink
Rollup merge of #72493 - nikomatsakis:move-leak-check, r=matthewjasper
Browse files Browse the repository at this point in the history
 move leak-check to during coherence, candidate eval

Implementation of MCP rust-lang/compiler-team#295.

I'd like to do a crater run on this.

Note to @rust-lang/lang: This PR is a breaking change (bugfix). It causes tests like the following to go from a future-compatibility warning #56105 to a hard error:

```rust
trait Trait {}
impl Trait for for<'a, 'b> fn(&'a u32, &'b u32) {}
impl Trait for for<'c> fn(&'c u32, &'c u32) {} // now rejected, used to warn
```

I am not aware of any instances of this code in the wild, but that is why we are doing a crater run. The reason for this change is that those two types are, in fact, the same type, and hence the two impls are overlapping.

There will still be impls that trigger #56105 after this lands, however -- I hope that we will eventually just accept those impls without warning, for the most part. One example of such an impl is this pattern, which is used by wasm-bindgen and other crates as well:

```rust
trait Trait {}
impl<T> Trait for fn(&T) { }
impl<T> Trait for fn(T) { } // still accepted, but warns
```
  • Loading branch information
Manishearth authored Jun 23, 2020
2 parents 59e87c0 + d57689f commit 903823c
Show file tree
Hide file tree
Showing 135 changed files with 2,143 additions and 1,250 deletions.
23 changes: 16 additions & 7 deletions src/librustc_infer/infer/higher_ranked/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,10 @@ 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, placeholder_map) = self.infcx.replace_bound_vars_with_placeholders(b);
let (b_prime, _) = self.infcx.replace_bound_vars_with_placeholders(b);

// Next, we instantiate each bound region in the subtype
// with a fresh region variable. These region variables --
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, &placeholder_map, 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 All @@ -119,7 +129,6 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
pub fn leak_check(
&self,
overly_polymorphic: bool,
placeholder_map: &PlaceholderMap<'tcx>,
snapshot: &CombinedSnapshot<'_, 'tcx>,
) -> RelateResult<'tcx, ()> {
// If the user gave `-Zno-leak-check`, or we have been
Expand All @@ -135,7 +144,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
self.inner.borrow_mut().unwrap_region_constraints().leak_check(
self.tcx,
overly_polymorphic,
placeholder_map,
self.universe(),
snapshot,
)
}
Expand Down
11 changes: 4 additions & 7 deletions src/librustc_infer/infer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -991,14 +991,12 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
return None;
}

Some(self.commit_if_ok(|snapshot| {
let (ty::SubtypePredicate { a_is_expected, a, b }, placeholder_map) =
Some(self.commit_if_ok(|_snapshot| {
let (ty::SubtypePredicate { a_is_expected, a, b }, _) =
self.replace_bound_vars_with_placeholders(&predicate);

let ok = self.at(cause, param_env).sub_exp(a_is_expected, a, b)?;

self.leak_check(false, &placeholder_map, snapshot)?;

Ok(ok.unit())
}))
}
Expand All @@ -1008,14 +1006,13 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
cause: &traits::ObligationCause<'tcx>,
predicate: ty::PolyRegionOutlivesPredicate<'tcx>,
) -> UnitResult<'tcx> {
self.commit_if_ok(|snapshot| {
let (ty::OutlivesPredicate(r_a, r_b), placeholder_map) =
self.commit_if_ok(|_snapshot| {
let (ty::OutlivesPredicate(r_a, r_b), _) =
self.replace_bound_vars_with_placeholders(&predicate);
let origin = SubregionOrigin::from_obligation_cause(cause, || {
RelateRegionParamBound(cause.span)
});
self.sub_regions(origin, r_b, r_a); // `b : a` ==> `a <= b`
self.leak_check(false, &placeholder_map, snapshot)?;
Ok(())
})
}
Expand Down
8 changes: 7 additions & 1 deletion src/librustc_infer/infer/nll_relate/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -522,7 +522,13 @@ where
}

if a == b {
return Ok(a);
// Subtle: if a or b has a bound variable that we are lazilly
// substituting, then even if a == b, it could be that the values we
// will substitute for those bound variables are *not* the same, and
// hence returning `Ok(a)` is incorrect.
if !a.has_escaping_bound_vars() && !b.has_escaping_bound_vars() {
return Ok(a);
}
}

match (&a.kind, &b.kind) {
Expand Down
Loading

0 comments on commit 903823c

Please sign in to comment.