Skip to content
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

relax leak-check #112999

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
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
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
Copy link
Member

Choose a reason for hiding this comment

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

Oh, so it "relaxes" the coherence leak check by making the check more accurate w.r.t. 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