Skip to content

Commit

Permalink
relax leak-check
Browse files Browse the repository at this point in the history
  • Loading branch information
aliemjay committed Jul 10, 2023
1 parent e0ba2d0 commit f5ad1f7
Show file tree
Hide file tree
Showing 15 changed files with 218 additions and 159 deletions.
101 changes: 19 additions & 82 deletions compiler/rustc_infer/src/infer/region_constraints/leak_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,9 @@ impl<'tcx> RegionConstraintCollector<'_, 'tcx> {
/// coherence and elsewhere -- see #56105 and #59490 for more details. The
/// eventual fate of the leak checker is not yet settled.
///
/// The leak checker works by searching for the following error patterns:
/// The leak checker works by searching for the following error pattern:
///
/// * P1: P2, where P1 != P2
/// * P1: R, where R is in some universe that cannot name P1
/// * P: R, where R is in some universe that cannot name P
///
/// The idea here is that each of these patterns represents something that
/// the region solver would eventually report as an error, so we can detect
Expand All @@ -45,16 +44,16 @@ impl<'tcx> RegionConstraintCollector<'_, 'tcx> {
/// For each SCC S, we compute:
///
/// * what placeholder P it must be equal to, if any
/// * if there are multiple placeholders that must be equal, report an error because `P1: P2`
/// * if there are multiple placeholders that must be equal, we pick the one with the higher
/// universe. It will eventually be an error in the next step if the placeholders are in
/// different universes.
/// * the minimum universe of its constituents
///
/// Then we walk the SCCs in dependency order and compute
///
/// * what placeholder they must outlive transitively
/// * if they must also be equal to a placeholder, report an error because `P1: P2`
/// * minimum universe U of all SCCs they must outlive
/// * if they must also be equal to a placeholder P, and U cannot name P, report an error, as that
/// indicates `P: R` and `R` is in an incompatible universe
/// * if the SCC must also be equal to a placeholder P, and U cannot name P, report an error,
/// as that indicates `P: R` and `R` is in a universe that cannot name P
///
/// To improve performance and for the old trait solver caching to be sound, this takes
/// an optional snapshot in which case we only look at region constraints added in that
Expand All @@ -69,6 +68,7 @@ impl<'tcx> RegionConstraintCollector<'_, 'tcx> {
///
/// * R: P1, even if R cannot name P1, because R = 'static is a valid sol'n
/// * R: P1, R: P2, as above
/// * P1: P2, when P2 lives in a universe that *can* name P1.
#[instrument(level = "debug", skip(self, tcx, only_consider_snapshot), ret)]
pub fn leak_check(
&mut self,
Expand All @@ -83,26 +83,18 @@ impl<'tcx> RegionConstraintCollector<'_, 'tcx> {

let mini_graph = &MiniGraph::new(tcx, &self, only_consider_snapshot);

let mut leak_check = LeakCheck::new(tcx, outer_universe, max_universe, mini_graph, self);
leak_check.assign_placeholder_values()?;
leak_check.propagate_scc_value()?;
Ok(())
let mut leak_check = LeakCheck::new(mini_graph, self);
leak_check.assign_placeholder_values();
leak_check.propagate_scc_value()
}
}

struct LeakCheck<'me, 'tcx> {
tcx: TyCtxt<'tcx>,
outer_universe: ty::UniverseIndex,
mini_graph: &'me MiniGraph<'tcx>,
rcc: &'me RegionConstraintCollector<'me, 'tcx>,

// Initially, for each SCC S, stores a placeholder `P` such that `S = P`
// must hold.
//
// Later, during the [`LeakCheck::propagate_scc_value`] function, this array
// is repurposed to store some placeholder `P` such that the weaker
// condition `S: P` must hold. (This is true if `S: S1` transitively and `S1
// = P`.)
scc_placeholders: IndexVec<LeakCheckScc, Option<ty::PlaceholderRegion>>,

// For each SCC S, track the minimum universe that flows into it. Note that
Expand All @@ -119,16 +111,11 @@ struct LeakCheck<'me, 'tcx> {

impl<'me, 'tcx> LeakCheck<'me, 'tcx> {
fn new(
tcx: TyCtxt<'tcx>,
outer_universe: ty::UniverseIndex,
max_universe: ty::UniverseIndex,
mini_graph: &'me MiniGraph<'tcx>,
rcc: &'me RegionConstraintCollector<'me, 'tcx>,
) -> Self {
let dummy_scc_universe = SccUniverse { universe: max_universe, region: None };
let dummy_scc_universe = SccUniverse { universe: ty::UniverseIndex::MAX, region: None };
Self {
tcx,
outer_universe,
mini_graph,
rcc,
scc_placeholders: IndexVec::from_elem_n(None, mini_graph.sccs.num_sccs()),
Expand All @@ -138,7 +125,7 @@ impl<'me, 'tcx> LeakCheck<'me, 'tcx> {

/// Compute what placeholders (if any) each SCC must be equal to.
/// Also compute the minimum universe of all the regions in each SCC.
fn assign_placeholder_values(&mut self) -> RelateResult<'tcx, ()> {
fn assign_placeholder_values(&mut self) {
// First walk: find each placeholder that is from a newly created universe.
for (region, leak_check_node) in &self.mini_graph.nodes {
let scc = self.mini_graph.sccs.scc(*leak_check_node);
Expand All @@ -152,34 +139,14 @@ impl<'me, 'tcx> LeakCheck<'me, 'tcx> {
self.scc_universes[scc].take_min(universe, *region);

// Detect those SCCs that directly contain a placeholder
if let ty::RePlaceholder(placeholder) = **region {
if self.outer_universe.cannot_name(placeholder.universe) {
self.assign_scc_value(scc, placeholder)?;
}
if let ty::RePlaceholder(p1) = **region {
let max_placeholder = match self.scc_placeholders[scc] {
Some(p2) => std::cmp::max_by_key(p1, p2, |p| p.universe),
None => p1,
};
self.scc_placeholders[scc] = Some(max_placeholder);
}
}

Ok(())
}

// assign_scc_value(S, P): Update `scc_values` to account for the fact that `P: S` must hold.
// This may create an error.
fn assign_scc_value(
&mut self,
scc: LeakCheckScc,
placeholder: ty::PlaceholderRegion,
) -> RelateResult<'tcx, ()> {
match self.scc_placeholders[scc] {
Some(p) => {
assert_ne!(p, placeholder);
return Err(self.placeholder_error(p, placeholder));
}
None => {
self.scc_placeholders[scc] = Some(placeholder);
}
};

Ok(())
}

/// For each SCC S, iterate over each successor S1 where `S: S1`:
Expand All @@ -201,8 +168,6 @@ impl<'me, 'tcx> LeakCheck<'me, 'tcx> {
//
// For each successor `scc2` where `scc1: scc2`:
//
// * `scc_placeholder[scc2]` stores some placeholder `P` where
// `scc2: P` (if any)
// * `scc_universes[scc2]` contains the minimum universe of the
// constituents of `scc2` and any of its successors
for scc1 in self.mini_graph.sccs.all_sccs() {
Expand All @@ -214,19 +179,12 @@ impl<'me, 'tcx> LeakCheck<'me, 'tcx> {
// Walk over each `scc2` such that `scc1: scc2` and compute:
//
// * `scc1_universe`: the minimum universe of `scc2` and the constituents of `scc1`
// * `succ_bound`: placeholder `P` that the successors must outlive, if any (if there are multiple,
// we pick one arbitrarily)
let mut scc1_universe = self.scc_universes[scc1];
let mut succ_bound = None;
for &scc2 in self.mini_graph.sccs.successors(scc1) {
let SccUniverse { universe: scc2_universe, region: scc2_region } =
self.scc_universes[scc2];

scc1_universe.take_min(scc2_universe, scc2_region.unwrap());

if let Some(b) = self.scc_placeholders[scc2] {
succ_bound = Some(b);
}
}

// Update minimum universe of scc1.
Expand All @@ -245,32 +203,11 @@ impl<'me, 'tcx> LeakCheck<'me, 'tcx> {
if scc1_universe.universe.cannot_name(scc1_placeholder.universe) {
return Err(self.error(scc1_placeholder, scc1_universe.region.unwrap()));
}

// Check if we have some placeholder where `S: P2`
// (transitively). In that case, since `S = P1`, that implies
// `P1: P2`, which is an error condition.
if let Some(scc2_placeholder) = succ_bound {
assert_ne!(scc1_placeholder, scc2_placeholder);
return Err(self.placeholder_error(scc1_placeholder, scc2_placeholder));
}
} else {
// Otherwise, we can reach a placeholder if some successor can.
self.scc_placeholders[scc1] = succ_bound;
}

// At this point, `scc_placeholder[scc1]` stores some placeholder that `scc1` must outlive (if any).
}
Ok(())
}

fn placeholder_error(
&self,
placeholder1: ty::PlaceholderRegion,
placeholder2: ty::PlaceholderRegion,
) -> TypeError<'tcx> {
self.error(placeholder1, ty::Region::new_placeholder(self.tcx, placeholder2))
}

fn error(
&self,
placeholder: ty::PlaceholderRegion,
Expand Down
17 changes: 0 additions & 17 deletions tests/ui/coherence/coherence-fn-implied-bounds.rs
Original file line number Diff line number Diff line change
@@ -1,26 +1,9 @@
// Test that our leak-check is not smart enough to take implied bounds
// into account (yet). Here we have two types that look like they
// should not be equivalent, but because of the rules on implied
// bounds we ought to know that, in fact, `'a = 'b` must always hold,
// and hence they are.
//
// Rustc can't figure this out and hence it accepts the impls but
// gives a future-compatibility warning (because we'd like to make
// this an error someday).
//
// Note that while we would like to make this a hard error, we also
// give the same warning for `coherence-wasm-bindgen.rs`, which ought
// to be accepted.

#![deny(coherence_leak_check)]

trait Trait {}

impl Trait for for<'a, 'b> fn(&'a &'b u32, &'b &'a u32) -> &'b u32 {}

impl Trait for for<'c> fn(&'c &'c u32, &'c &'c u32) -> &'c u32 {
//~^ ERROR conflicting implementations
//~| WARNING this was previously accepted by the compiler
}

fn main() {}
12 changes: 3 additions & 9 deletions tests/ui/coherence/coherence-fn-implied-bounds.stderr
Original file line number Diff line number Diff line change
@@ -1,20 +1,14 @@
error: conflicting implementations of trait `Trait` for type `for<'a, 'b> fn(&'a &'b u32, &'b &'a u32) -> &'b u32`
--> $DIR/coherence-fn-implied-bounds.rs:21:1
error[E0119]: conflicting implementations of trait `Trait` for type `for<'a, 'b> fn(&'a &'b u32, &'b &'a u32) -> &'b u32`
--> $DIR/coherence-fn-implied-bounds.rs:5:1
|
LL | impl Trait for for<'a, 'b> fn(&'a &'b u32, &'b &'a u32) -> &'b u32 {}
| ------------------------------------------------------------------ first implementation here
LL |
LL | impl Trait for for<'c> fn(&'c &'c u32, &'c &'c u32) -> &'c u32 {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `for<'a, 'b> fn(&'a &'b u32, &'b &'a u32) -> &'b u32`
|
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #56105 <https://github.com/rust-lang/rust/issues/56105>
= note: this behavior recently changed as a result of a bug fix; see rust-lang/rust#56105 for details
note: the lint level is defined here
--> $DIR/coherence-fn-implied-bounds.rs:15:9
|
LL | #![deny(coherence_leak_check)]
| ^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

For more information about this error, try `rustc --explain E0119`.
8 changes: 1 addition & 7 deletions tests/ui/coherence/coherence-subtyping.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,5 @@
// Test that two distinct impls which match subtypes of one another
// yield coherence errors (or not) depending on the variance.
//
// Note: This scenario is currently accepted, but as part of the
// universe transition (#56105) may eventually become an error.

// check-pass

trait TheTrait {
fn foo(&self) {}
Expand All @@ -13,8 +8,7 @@ trait TheTrait {
impl TheTrait for for<'a, 'b> fn(&'a u8, &'b u8) -> &'a u8 {}

impl TheTrait for for<'a> fn(&'a u8, &'a u8) -> &'a u8 {
//~^ WARNING conflicting implementation
//~^^ WARNING this was previously accepted by the compiler but is being phased out
//~^ ERROR conflicting implementation
}

fn main() {}
10 changes: 4 additions & 6 deletions tests/ui/coherence/coherence-subtyping.stderr
Original file line number Diff line number Diff line change
@@ -1,16 +1,14 @@
warning: conflicting implementations of trait `TheTrait` for type `for<'a, 'b> fn(&'a u8, &'b u8) -> &'a u8`
--> $DIR/coherence-subtyping.rs:15:1
error[E0119]: conflicting implementations of trait `TheTrait` for type `for<'a, 'b> fn(&'a u8, &'b u8) -> &'a u8`
--> $DIR/coherence-subtyping.rs:10:1
|
LL | impl TheTrait for for<'a, 'b> fn(&'a u8, &'b u8) -> &'a u8 {}
| ---------------------------------------------------------- first implementation here
LL |
LL | impl TheTrait for for<'a> fn(&'a u8, &'a u8) -> &'a u8 {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `for<'a, 'b> fn(&'a u8, &'b u8) -> &'a u8`
|
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #56105 <https://github.com/rust-lang/rust/issues/56105>
= note: this behavior recently changed as a result of a bug fix; see rust-lang/rust#56105 for details
= note: `#[warn(coherence_leak_check)]` on by default

warning: 1 warning emitted
error: aborting due to previous error

For more information about this error, try `rustc --explain E0119`.
81 changes: 81 additions & 0 deletions tests/ui/coherence/leak-check.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
// compile-flags: -Ztrait-solver=next
trait Relate {}

struct Outlives<'a, 'b>(&'a u8, &'b u8);
impl<'a, 'b> Relate for Outlives<'a, 'b> where 'a: 'b, {}

struct Equal<'a, 'b>(&'a u8, &'b u8);
impl<'a> Relate for Equal<'a, 'a> {}

macro_rules! rule {
( $name:ident<$($lt:lifetime),*> :- $( ($($bound:tt)*) ),* ) => {
struct $name<$($lt),*>($(&$lt u8),*);
impl<$($lt),*> $crate::Relate for $name<$($lt),*>
where $( $($bound)*: $crate::Relate, )*
{}
};
}

// ----

trait CoherenceTrait {}
impl<T> CoherenceTrait for T {}

macro_rules! assert_false_by_leak_check {
( exist<$($lt:lifetime),*> $( ($($bound:tt)*) ),* ) => {
struct Unique<$($lt),*>($(&$lt u8),*);
impl<$($lt),*> $crate::CoherenceTrait for Unique<$($lt),*>
//~^ ERROR for type `test1::Unique`
//~| ERROR for type `test3::Unique`
//~| ERROR for type `test6::Unique`
where $( $($bound)*: $crate::Relate, )*
{}
};
}

mod test1 {
use super::*;
assert_false_by_leak_check!(
exist<> (for<'a, 'b> Outlives<'a, 'b>)
);
}

mod test2 {
use super::*;
assert_false_by_leak_check!(
exist<'a> (for<'b> Outlives<'b, 'a>)
);
}

mod test3 {
use super::*;
rule!( OutlivesPlaceholder<'a> :- (for<'b> Outlives<'a, 'b>) );
assert_false_by_leak_check!(
exist<> (for<'a> OutlivesPlaceholder<'a>)
);
}

mod test4 {
use super::*;
rule!( OutlivedByPlaceholder<'a> :- (for<'b> Outlives<'b, 'a>) );
assert_false_by_leak_check!(
exist<> (for<'a> OutlivedByPlaceholder<'a>)
);
}

mod test5 {
use super::*;
rule!( EqualsPlaceholder<'a> :- (for<'b> Equal<'b, 'a>) );
assert_false_by_leak_check!(
exist<> (for<'a> EqualsPlaceholder<'a>)
);
}

mod test6 {
use super::*;
assert_false_by_leak_check!(
exist<> (for<'a, 'b> Equal<'a, 'b>)
);
}

fn main() {}
Loading

0 comments on commit f5ad1f7

Please sign in to comment.