From 561ce442def02aba880b091d8d5e6a150855869e Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Wed, 20 Feb 2019 05:39:04 -0500 Subject: [PATCH] restore the actual leak-check --- src/librustc/infer/higher_ranked/mod.rs | 19 +- .../infer/region_constraints/leak_check.rs | 162 ++++++++++++++++++ src/librustc/infer/region_constraints/mod.rs | 2 + src/librustc/ty/error.rs | 16 +- src/librustc/ty/structural_impls.rs | 8 + src/test/ui/hrtb/issue-46989.rs | 42 +++++ src/test/ui/hrtb/issue-57639.rs | 29 ++++ src/test/ui/hrtb/issue-58451.rs | 13 ++ 8 files changed, 277 insertions(+), 14 deletions(-) create mode 100644 src/librustc/infer/region_constraints/leak_check.rs create mode 100644 src/test/ui/hrtb/issue-46989.rs create mode 100644 src/test/ui/hrtb/issue-57639.rs create mode 100644 src/test/ui/hrtb/issue-58451.rs diff --git a/src/librustc/infer/higher_ranked/mod.rs b/src/librustc/infer/higher_ranked/mod.rs index 50487f488852d..7c83fe7fd6946 100644 --- a/src/librustc/infer/higher_ranked/mod.rs +++ b/src/librustc/infer/higher_ranked/mod.rs @@ -113,21 +113,14 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { (result, map) } - /// Searches region constraints created since `snapshot` that - /// affect one of the placeholders in `placeholder_map`, returning - /// an error if any of the placeholders are related to another - /// placeholder or would have to escape into some parent universe - /// that cannot name them. - /// - /// This is a temporary backwards compatibility measure to try and - /// retain the older (arguably incorrect) behavior of the - /// compiler. + /// See `infer::region_constraints::RegionConstraintCollector::leak_check`. pub fn leak_check( &self, - _overly_polymorphic: bool, - _placeholder_map: &PlaceholderMap<'tcx>, - _snapshot: &CombinedSnapshot<'_, 'tcx>, + overly_polymorphic: bool, + placeholder_map: &PlaceholderMap<'tcx>, + snapshot: &CombinedSnapshot<'_, 'tcx>, ) -> RelateResult<'tcx, ()> { - Ok(()) + self.borrow_region_constraints() + .leak_check(self.tcx, overly_polymorphic, placeholder_map, snapshot) } } diff --git a/src/librustc/infer/region_constraints/leak_check.rs b/src/librustc/infer/region_constraints/leak_check.rs new file mode 100644 index 0000000000000..4056b9e2d83bc --- /dev/null +++ b/src/librustc/infer/region_constraints/leak_check.rs @@ -0,0 +1,162 @@ +use super::*; +use crate::infer::{CombinedSnapshot, PlaceholderMap}; +use crate::ty::error::TypeError; +use crate::ty::relate::RelateResult; + +impl<'tcx> RegionConstraintCollector<'tcx> { + /// Searches region constraints created since `snapshot` that + /// affect one of the placeholders in `placeholder_map`, returning + /// an error if any of the placeholders are related to another + /// placeholder or would have to escape into some parent universe + /// that cannot name them. + /// + /// This is a temporary backwards compatibility measure to try and + /// retain the older (arguably incorrect) behavior of the + /// compiler. + /// + /// NB. The use of snapshot here is mostly an efficiency thing -- + /// we could search *all* region constraints, but that'd be a + /// bigger set and the data structures are not setup for that. If + /// we wind up keeping some form of this check long term, it would + /// probably be better to remove the snapshot parameter and to + /// refactor the constraint set. + pub fn leak_check( + &mut self, + tcx: TyCtxt<'_, '_, 'tcx>, + overly_polymorphic: bool, + placeholder_map: &PlaceholderMap<'tcx>, + _snapshot: &CombinedSnapshot<'_, 'tcx>, + ) -> RelateResult<'tcx, ()> { + debug!("leak_check(placeholders={:?})", placeholder_map); + + assert!(self.in_snapshot()); + + // Go through each placeholder that we created. + for (_, &placeholder_region) in placeholder_map { + // Find the universe this placeholder inhabits. + let placeholder = match placeholder_region { + ty::RePlaceholder(p) => p, + _ => bug!( + "leak_check: expected placeholder found {:?}", + placeholder_region, + ), + }; + + // Find all regions that are related to this placeholder + // in some way. This means any region that either outlives + // or is outlived by a placeholder. + let mut taint_set = TaintSet::new( + TaintDirections::both(), + placeholder_region, + ); + taint_set.fixed_point(tcx, &self.undo_log, &self.data.verifys); + let tainted_regions = taint_set.into_set(); + + // Report an error if two placeholders in the same universe + // are related to one another, or if a placeholder is related + // to something from a parent universe. + for &tainted_region in &tainted_regions { + if let ty::RePlaceholder(_) = tainted_region { + // Two placeholders cannot be related: + if tainted_region == placeholder_region { + continue; + } + } else if self.universe(tainted_region).can_name(placeholder.universe) { + continue; + } + + return Err(if overly_polymorphic { + debug!("Overly polymorphic!"); + TypeError::RegionsOverlyPolymorphic(placeholder.name, tainted_region) + } else { + debug!("Not as polymorphic!"); + TypeError::RegionsInsufficientlyPolymorphic(placeholder.name, tainted_region) + }); + } + } + + Ok(()) + } +} + +#[derive(Debug)] +struct TaintSet<'tcx> { + directions: TaintDirections, + regions: FxHashSet>, +} + +impl<'tcx> TaintSet<'tcx> { + fn new(directions: TaintDirections, initial_region: ty::Region<'tcx>) -> Self { + let mut regions = FxHashSet::default(); + regions.insert(initial_region); + TaintSet { + directions: directions, + regions: regions, + } + } + + fn fixed_point( + &mut self, + tcx: TyCtxt<'_, '_, 'tcx>, + undo_log: &[UndoLog<'tcx>], + verifys: &[Verify<'tcx>], + ) { + let mut prev_len = 0; + while prev_len < self.len() { + debug!( + "tainted: prev_len = {:?} new_len = {:?}", + prev_len, + self.len() + ); + + prev_len = self.len(); + + for undo_entry in undo_log { + match undo_entry { + &AddConstraint(Constraint::VarSubVar(a, b)) => { + self.add_edge(tcx.mk_region(ReVar(a)), tcx.mk_region(ReVar(b))); + } + &AddConstraint(Constraint::RegSubVar(a, b)) => { + self.add_edge(a, tcx.mk_region(ReVar(b))); + } + &AddConstraint(Constraint::VarSubReg(a, b)) => { + self.add_edge(tcx.mk_region(ReVar(a)), b); + } + &AddConstraint(Constraint::RegSubReg(a, b)) => { + self.add_edge(a, b); + } + &AddGiven(a, b) => { + self.add_edge(a, tcx.mk_region(ReVar(b))); + } + &AddVerify(i) => span_bug!( + verifys[i].origin.span(), + "we never add verifications while doing higher-ranked things", + ), + &Purged | &AddCombination(..) | &AddVar(..) => {} + } + } + } + } + + fn into_set(self) -> FxHashSet> { + self.regions + } + + fn len(&self) -> usize { + self.regions.len() + } + + fn add_edge(&mut self, source: ty::Region<'tcx>, target: ty::Region<'tcx>) { + if self.directions.incoming { + if self.regions.contains(&target) { + self.regions.insert(source); + } + } + + if self.directions.outgoing { + if self.regions.contains(&source) { + self.regions.insert(target); + } + } + } +} diff --git a/src/librustc/infer/region_constraints/mod.rs b/src/librustc/infer/region_constraints/mod.rs index 45d614167ea91..8389f0ab1aa79 100644 --- a/src/librustc/infer/region_constraints/mod.rs +++ b/src/librustc/infer/region_constraints/mod.rs @@ -17,6 +17,8 @@ use crate::ty::{Region, RegionVid}; use std::collections::BTreeMap; use std::{cmp, fmt, mem, u32}; +mod leak_check; + #[derive(Default)] pub struct RegionConstraintCollector<'tcx> { /// For each `RegionVid`, the corresponding `RegionVariableOrigin`. diff --git a/src/librustc/ty/error.rs b/src/librustc/ty/error.rs index e3e0ce147741f..f58e5e4fb69f6 100644 --- a/src/librustc/ty/error.rs +++ b/src/librustc/ty/error.rs @@ -1,5 +1,5 @@ use crate::hir::def_id::DefId; -use crate::ty::{self, Region, Ty, TyCtxt}; +use crate::ty::{self, BoundRegion, Region, Ty, TyCtxt}; use std::borrow::Cow; use std::fmt; use rustc_target::spec::abi; @@ -27,6 +27,8 @@ pub enum TypeError<'tcx> { ArgCount, RegionsDoesNotOutlive(Region<'tcx>, Region<'tcx>), + RegionsInsufficientlyPolymorphic(BoundRegion, Region<'tcx>), + RegionsOverlyPolymorphic(BoundRegion, Region<'tcx>), RegionsPlaceholderMismatch, Sorts(ExpectedFound>), @@ -101,6 +103,18 @@ impl<'tcx> fmt::Display for TypeError<'tcx> { RegionsDoesNotOutlive(..) => { write!(f, "lifetime mismatch") } + RegionsInsufficientlyPolymorphic(br, _) => { + write!(f, + "expected bound lifetime parameter{}{}, found concrete lifetime", + if br.is_named() { " " } else { "" }, + br) + } + RegionsOverlyPolymorphic(br, _) => { + write!(f, + "expected concrete lifetime, found bound lifetime parameter{}{}", + if br.is_named() { " " } else { "" }, + br) + } RegionsPlaceholderMismatch => { write!(f, "one type is more general than the other") } diff --git a/src/librustc/ty/structural_impls.rs b/src/librustc/ty/structural_impls.rs index d09cfa84a1690..f9173836cc627 100644 --- a/src/librustc/ty/structural_impls.rs +++ b/src/librustc/ty/structural_impls.rs @@ -434,6 +434,12 @@ impl<'a, 'tcx> Lift<'tcx> for ty::error::TypeError<'a> { RegionsDoesNotOutlive(a, b) => { return tcx.lift(&(a, b)).map(|(a, b)| RegionsDoesNotOutlive(a, b)) } + RegionsInsufficientlyPolymorphic(a, b) => { + return tcx.lift(&b).map(|b| RegionsInsufficientlyPolymorphic(a, b)) + } + RegionsOverlyPolymorphic(a, b) => { + return tcx.lift(&b).map(|b| RegionsOverlyPolymorphic(a, b)) + } RegionsPlaceholderMismatch => RegionsPlaceholderMismatch, IntMismatch(x) => IntMismatch(x), FloatMismatch(x) => FloatMismatch(x), @@ -1021,6 +1027,8 @@ EnumTypeFoldableImpl! { (ty::error::TypeError::FixedArraySize)(x), (ty::error::TypeError::ArgCount), (ty::error::TypeError::RegionsDoesNotOutlive)(a, b), + (ty::error::TypeError::RegionsInsufficientlyPolymorphic)(a, b), + (ty::error::TypeError::RegionsOverlyPolymorphic)(a, b), (ty::error::TypeError::RegionsPlaceholderMismatch), (ty::error::TypeError::IntMismatch)(x), (ty::error::TypeError::FloatMismatch)(x), diff --git a/src/test/ui/hrtb/issue-46989.rs b/src/test/ui/hrtb/issue-46989.rs new file mode 100644 index 0000000000000..2c85905545807 --- /dev/null +++ b/src/test/ui/hrtb/issue-46989.rs @@ -0,0 +1,42 @@ +// Regression test for #46989: +// +// In the move to universes, this test started passing. +// It is not necessarily WRONG to do so, but it was a bit +// surprising. The reason that it passed is that when we were +// asked to prove that +// +// for<'a> fn(&'a i32): Foo +// +// we were able to use the impl below to prove +// +// fn(&'empty i32): Foo +// +// and then we were able to prove that +// +// fn(&'empty i32) = for<'a> fn(&'a i32) +// +// This last fact is somewhat surprising, but essentially "falls out" +// from handling variance correctly. In particular, consider the subtyping +// relations. First: +// +// fn(&'empty i32) <: for<'a> fn(&'a i32) +// +// This holds because -- intuitively -- a fn that takes a reference but doesn't use +// it can be given a reference with any lifetime. Similarly, the opposite direction: +// +// for<'a> fn(&'a i32) <: fn(&'empty i32) +// +// holds because 'a can be instantiated to 'empty. + +trait Foo { + +} + +impl Foo for fn(A) { } + +fn assert_foo() {} + +fn main() { + assert_foo::(); + //~^ ERROR the trait bound `for<'r> fn(&'r i32): Foo` is not satisfied +} diff --git a/src/test/ui/hrtb/issue-57639.rs b/src/test/ui/hrtb/issue-57639.rs new file mode 100644 index 0000000000000..4bcaef3616bd5 --- /dev/null +++ b/src/test/ui/hrtb/issue-57639.rs @@ -0,0 +1,29 @@ +// Regression test for #57639: +// +// In the move to universes, this test stopped working. The problem +// was that when the trait solver was asked to prove `for<'a> T::Item: +// Foo<'a>` as part of WF checking, it wound up "eagerly committing" +// to the where clause, which says that `T::Item: Foo<'a>`, but it +// should instead have been using the bound found in the trait +// declaration. Pre-universe, this used to work out ok because we got +// "eager errors" due to the leak check. +// +// See [this comment on GitHub][c] for more details. +// +// run-pass +// +// [c]: https://github.com/rust-lang/rust/issues/57639#issuecomment-455685861 + +trait Foo<'a> {} + +trait Bar { + type Item: for<'a> Foo<'a>; +} + +fn foo<'a, T>(_: T) +where + T: Bar, + T::Item: Foo<'a>, +{} + +fn main() { } diff --git a/src/test/ui/hrtb/issue-58451.rs b/src/test/ui/hrtb/issue-58451.rs new file mode 100644 index 0000000000000..7ca6914d2ea96 --- /dev/null +++ b/src/test/ui/hrtb/issue-58451.rs @@ -0,0 +1,13 @@ +// Regression test for #58451: +// +// Error reporting here encountered an ICE in the shift to universes. + +fn f(i: I) +where + I: IntoIterator, + I::Item: for<'a> Into<&'a ()>, +{} + +fn main() { + f(&[f()]); +}