From 200ed9f8cd49e823c788b6ce1096030d9c40ec02 Mon Sep 17 00:00:00 2001 From: lcnr Date: Tue, 23 May 2023 16:32:59 +0200 Subject: leak_check: remove unused codepath --- compiler/rustc_hir_typeck/src/coercion.rs | 2 +- .../rustc_infer/src/infer/higher_ranked/mod.rs | 7 +----- .../src/infer/region_constraints/leak_check.rs | 25 +++++----------------- compiler/rustc_middle/src/ty/error.rs | 13 ----------- .../rustc_trait_selection/src/traits/coherence.rs | 2 +- .../rustc_trait_selection/src/traits/select/mod.rs | 2 +- 6 files changed, 9 insertions(+), 42 deletions(-) (limited to 'compiler') diff --git a/compiler/rustc_hir_typeck/src/coercion.rs b/compiler/rustc_hir_typeck/src/coercion.rs index 369dd4ae595..ee36936a06e 100644 --- a/compiler/rustc_hir_typeck/src/coercion.rs +++ b/compiler/rustc_hir_typeck/src/coercion.rs @@ -824,7 +824,7 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> { // want the coerced type to be the actual supertype of these two, // but for now, we want to just error to ensure we don't lock // ourselves into a specific behavior with NLL. - self.leak_check(false, snapshot)?; + self.leak_check(snapshot)?; result }) diff --git a/compiler/rustc_infer/src/infer/higher_ranked/mod.rs b/compiler/rustc_infer/src/infer/higher_ranked/mod.rs index 1c298e7d4ec..00e10820683 100644 --- a/compiler/rustc_infer/src/infer/higher_ranked/mod.rs +++ b/compiler/rustc_infer/src/infer/higher_ranked/mod.rs @@ -108,11 +108,7 @@ impl<'tcx> InferCtxt<'tcx> { /// See [RegionConstraintCollector::leak_check][1]. /// /// [1]: crate::infer::region_constraints::RegionConstraintCollector::leak_check - pub fn leak_check( - &self, - overly_polymorphic: bool, - snapshot: &CombinedSnapshot<'tcx>, - ) -> RelateResult<'tcx, ()> { + pub fn leak_check(&self, snapshot: &CombinedSnapshot<'tcx>) -> RelateResult<'tcx, ()> { // If the user gave `-Zno-leak-check`, or we have been // configured to skip the leak check, then skip the leak check // completely. The leak check is deprecated. Any legitimate @@ -125,7 +121,6 @@ impl<'tcx> InferCtxt<'tcx> { self.inner.borrow_mut().unwrap_region_constraints().leak_check( self.tcx, - overly_polymorphic, self.universe(), snapshot, ) diff --git a/compiler/rustc_infer/src/infer/region_constraints/leak_check.rs b/compiler/rustc_infer/src/infer/region_constraints/leak_check.rs index 5f4093c1c3c..7b9e6983212 100644 --- a/compiler/rustc_infer/src/infer/region_constraints/leak_check.rs +++ b/compiler/rustc_infer/src/infer/region_constraints/leak_check.rs @@ -65,13 +65,12 @@ impl<'tcx> RegionConstraintCollector<'_, 'tcx> { pub fn leak_check( &mut self, tcx: TyCtxt<'tcx>, - overly_polymorphic: bool, max_universe: ty::UniverseIndex, snapshot: &CombinedSnapshot<'tcx>, ) -> RelateResult<'tcx, ()> { debug!( - "leak_check(max_universe={:?}, snapshot.universe={:?}, overly_polymorphic={:?})", - max_universe, snapshot.universe, overly_polymorphic + "leak_check(max_universe={:?}, snapshot.universe={:?})", + max_universe, snapshot.universe ); assert!(UndoLogs::>::in_snapshot(&self.undo_log)); @@ -84,14 +83,8 @@ impl<'tcx> RegionConstraintCollector<'_, 'tcx> { let mini_graph = &MiniGraph::new(tcx, self.undo_log.region_constraints(), &self.storage.data.verifys); - let mut leak_check = LeakCheck::new( - tcx, - universe_at_start_of_snapshot, - max_universe, - overly_polymorphic, - mini_graph, - self, - ); + let mut leak_check = + LeakCheck::new(tcx, universe_at_start_of_snapshot, max_universe, mini_graph, self); leak_check.assign_placeholder_values()?; leak_check.propagate_scc_value()?; Ok(()) @@ -101,8 +94,6 @@ impl<'tcx> RegionConstraintCollector<'_, 'tcx> { struct LeakCheck<'me, 'tcx> { tcx: TyCtxt<'tcx>, universe_at_start_of_snapshot: ty::UniverseIndex, - /// Only used when reporting region errors. - overly_polymorphic: bool, mini_graph: &'me MiniGraph<'tcx>, rcc: &'me RegionConstraintCollector<'me, 'tcx>, @@ -132,7 +123,6 @@ impl<'me, 'tcx> LeakCheck<'me, 'tcx> { tcx: TyCtxt<'tcx>, universe_at_start_of_snapshot: ty::UniverseIndex, max_universe: ty::UniverseIndex, - overly_polymorphic: bool, mini_graph: &'me MiniGraph<'tcx>, rcc: &'me RegionConstraintCollector<'me, 'tcx>, ) -> Self { @@ -140,7 +130,6 @@ impl<'me, 'tcx> LeakCheck<'me, 'tcx> { Self { tcx, universe_at_start_of_snapshot, - overly_polymorphic, mini_graph, rcc, scc_placeholders: IndexVec::from_elem_n(None, mini_graph.sccs.num_sccs()), @@ -289,11 +278,7 @@ impl<'me, 'tcx> LeakCheck<'me, 'tcx> { other_region: ty::Region<'tcx>, ) -> TypeError<'tcx> { debug!("error: placeholder={:?}, other_region={:?}", placeholder, other_region); - if self.overly_polymorphic { - TypeError::RegionsOverlyPolymorphic(placeholder.bound.kind, other_region) - } else { - TypeError::RegionsInsufficientlyPolymorphic(placeholder.bound.kind, other_region) - } + TypeError::RegionsInsufficientlyPolymorphic(placeholder.bound.kind, other_region) } } diff --git a/compiler/rustc_middle/src/ty/error.rs b/compiler/rustc_middle/src/ty/error.rs index 49ab9b79e96..66293f19eef 100644 --- a/compiler/rustc_middle/src/ty/error.rs +++ b/compiler/rustc_middle/src/ty/error.rs @@ -45,7 +45,6 @@ pub enum TypeError<'tcx> { RegionsDoesNotOutlive(Region<'tcx>, Region<'tcx>), RegionsInsufficientlyPolymorphic(BoundRegionKind, Region<'tcx>), - RegionsOverlyPolymorphic(BoundRegionKind, Region<'tcx>), RegionsPlaceholderMismatch, Sorts(ExpectedFound>), @@ -74,7 +73,6 @@ impl TypeError<'_> { match self { TypeError::RegionsDoesNotOutlive(_, _) | TypeError::RegionsInsufficientlyPolymorphic(_, _) - | TypeError::RegionsOverlyPolymorphic(_, _) | TypeError::RegionsPlaceholderMismatch => true, _ => false, } @@ -98,11 +96,6 @@ impl<'tcx> TypeError<'tcx> { } } - let br_string = |br: ty::BoundRegionKind| match br { - ty::BrNamed(_, name) => format!(" {}", name), - _ => String::new(), - }; - match self { CyclicTy(_) => "cyclic type of infinite size".into(), CyclicConst(_) => "encountered a self-referencing constant".into(), @@ -144,11 +137,6 @@ impl<'tcx> TypeError<'tcx> { RegionsInsufficientlyPolymorphic(..) => { "one type is more general than the other".into() } - RegionsOverlyPolymorphic(br, _) => format!( - "expected concrete lifetime, found bound lifetime parameter{}", - br_string(br) - ) - .into(), RegionsPlaceholderMismatch => "one type is more general than the other".into(), ArgumentSorts(values, _) | Sorts(values) => { let expected = values.expected.sort_string(tcx); @@ -228,7 +216,6 @@ impl<'tcx> TypeError<'tcx> { | FieldMisMatch(..) | RegionsDoesNotOutlive(..) | RegionsInsufficientlyPolymorphic(..) - | RegionsOverlyPolymorphic(..) | RegionsPlaceholderMismatch | Traits(_) | ProjectionMismatched(_) diff --git a/compiler/rustc_trait_selection/src/traits/coherence.rs b/compiler/rustc_trait_selection/src/traits/coherence.rs index e8c5a8fab2a..dda77c2a461 100644 --- a/compiler/rustc_trait_selection/src/traits/coherence.rs +++ b/compiler/rustc_trait_selection/src/traits/coherence.rs @@ -200,7 +200,7 @@ fn overlap_within_probe<'cx, 'tcx>( // We disable the leak when creating the `snapshot` by using // `infcx.probe_maybe_disable_leak_check`. - if infcx.leak_check(true, snapshot).is_err() { + if infcx.leak_check(snapshot).is_err() { debug!("overlap: leak check failed"); return None; } diff --git a/compiler/rustc_trait_selection/src/traits/select/mod.rs b/compiler/rustc_trait_selection/src/traits/select/mod.rs index ac4f845a005..2c0b911c805 100644 --- a/compiler/rustc_trait_selection/src/traits/select/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/select/mod.rs @@ -563,7 +563,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { self.infcx.probe(|snapshot| -> Result { let result = op(self)?; - match self.infcx.leak_check(true, snapshot) { + match self.infcx.leak_check(snapshot) { Ok(()) => {} Err(_) => return Ok(EvaluatedToErr), } -- cgit 1.4.1-3-g733a5 From 0a6ae29fe855a07b204c5b0e490c59c3ab3d443a Mon Sep 17 00:00:00 2001 From: lcnr Date: Tue, 23 May 2023 18:10:30 +0200 Subject: coherence: don't add hidden types for opaques we can otherwise assign a hidden type to the opaque which causes ICE if we don't use `take_opaque_types` during coherence. This is annoying so I didn't bother. Added a test showing the behavior this prevents. --- compiler/rustc_infer/src/infer/opaque_types.rs | 32 +++++++++++++++------- .../coherence_different_hidden_ty.rs | 25 +++++++++++++++++ .../coherence_different_hidden_ty.stderr | 12 ++++++++ 3 files changed, 59 insertions(+), 10 deletions(-) create mode 100644 tests/ui/type-alias-impl-trait/coherence_different_hidden_ty.rs create mode 100644 tests/ui/type-alias-impl-trait/coherence_different_hidden_ty.stderr (limited to 'compiler') diff --git a/compiler/rustc_infer/src/infer/opaque_types.rs b/compiler/rustc_infer/src/infer/opaque_types.rs index 9d5ec228d82..105a3f08c82 100644 --- a/compiler/rustc_infer/src/infer/opaque_types.rs +++ b/compiler/rustc_infer/src/infer/opaque_types.rs @@ -533,17 +533,29 @@ impl<'tcx> InferCtxt<'tcx> { // these are the same span, but not in cases like `-> (impl // Foo, impl Bar)`. let span = cause.span; - let prev = self.inner.borrow_mut().opaque_types().register( - opaque_type_key, - OpaqueHiddenType { ty: hidden_ty, span }, - origin, - ); - let mut obligations = if let Some(prev) = prev { - self.at(&cause, param_env) - .eq_exp(DefineOpaqueTypes::Yes, a_is_expected, prev, hidden_ty)? - .obligations + let mut obligations = if self.intercrate { + // During intercrate we do not define opaque types but instead always + // force ambiguity unless the hidden type is known to not implement + // our trait. + vec![traits::Obligation::new( + self.tcx, + cause.clone(), + param_env, + ty::PredicateKind::Ambiguous, + )] } else { - Vec::new() + let prev = self.inner.borrow_mut().opaque_types().register( + opaque_type_key, + OpaqueHiddenType { ty: hidden_ty, span }, + origin, + ); + if let Some(prev) = prev { + self.at(&cause, param_env) + .eq_exp(DefineOpaqueTypes::Yes, a_is_expected, prev, hidden_ty)? + .obligations + } else { + Vec::new() + } }; self.add_item_bounds_for_hidden_type( diff --git a/tests/ui/type-alias-impl-trait/coherence_different_hidden_ty.rs b/tests/ui/type-alias-impl-trait/coherence_different_hidden_ty.rs new file mode 100644 index 00000000000..39b3d535ad4 --- /dev/null +++ b/tests/ui/type-alias-impl-trait/coherence_different_hidden_ty.rs @@ -0,0 +1,25 @@ +// When checking whether these two impls overlap, we could detect that we +// would require the hidden type of `TAIT` to be equal to both `u32` and `i32` +// and therefore accept them as disjoint. That is annoying to implement with +// the current system because we would have to add the following to each +// returning branch in coherence. +// +// let _ = infcx.take_opaque_types(); +// +// @lcnr: Because of this I decided to not bother and cause this to fail instead. +// In the future we can definitely modify the compiler to accept this +// again. +#![feature(type_alias_impl_trait)] + +trait Trait {} + +type TAIT = impl Sized; + +impl Trait for (TAIT, TAIT) {} + +impl Trait for (u32, i32) {} +//~^ ERROR conflicting implementations of trait `Trait` for type `(TAIT, TAIT)` + +fn define() -> TAIT {} + +fn main() {} diff --git a/tests/ui/type-alias-impl-trait/coherence_different_hidden_ty.stderr b/tests/ui/type-alias-impl-trait/coherence_different_hidden_ty.stderr new file mode 100644 index 00000000000..f2aee798608 --- /dev/null +++ b/tests/ui/type-alias-impl-trait/coherence_different_hidden_ty.stderr @@ -0,0 +1,12 @@ +error[E0119]: conflicting implementations of trait `Trait` for type `(TAIT, TAIT)` + --> $DIR/coherence_different_hidden_ty.rs:20:1 + | +LL | impl Trait for (TAIT, TAIT) {} + | --------------------------- first implementation here +LL | +LL | impl Trait for (u32, i32) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `(TAIT, TAIT)` + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0119`. -- cgit 1.4.1-3-g733a5 From a0245bb3cb51c0e9a19c20abac6473da446832e0 Mon Sep 17 00:00:00 2001 From: lcnr Date: Tue, 23 May 2023 18:56:25 +0200 Subject: rework the leak_check to take the outer_universe clean up coherence to not rely on probes anymore --- compiler/rustc_hir_typeck/src/coercion.rs | 4 +- compiler/rustc_infer/src/infer/at.rs | 2 +- .../rustc_infer/src/infer/higher_ranked/mod.rs | 15 ++- compiler/rustc_infer/src/infer/mod.rs | 55 ++++----- .../src/infer/region_constraints/leak_check.rs | 125 ++++++++++++--------- .../src/infer/region_constraints/mod.rs | 11 +- compiler/rustc_infer/src/infer/undo_log.rs | 9 -- .../rustc_trait_selection/src/traits/coherence.rs | 111 ++++++++++-------- .../src/traits/query/evaluate_obligation.rs | 2 +- .../rustc_trait_selection/src/traits/select/mod.rs | 10 +- 10 files changed, 178 insertions(+), 166 deletions(-) (limited to 'compiler') diff --git a/compiler/rustc_hir_typeck/src/coercion.rs b/compiler/rustc_hir_typeck/src/coercion.rs index ee36936a06e..905781ec8f5 100644 --- a/compiler/rustc_hir_typeck/src/coercion.rs +++ b/compiler/rustc_hir_typeck/src/coercion.rs @@ -808,6 +808,8 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> { G: FnOnce(Ty<'tcx>) -> Vec>, { self.commit_if_ok(|snapshot| { + let outer_universe = self.infcx.universe(); + let result = if let ty::FnPtr(fn_ty_b) = b.kind() && let (hir::Unsafety::Normal, hir::Unsafety::Unsafe) = (fn_ty_a.unsafety(), fn_ty_b.unsafety()) @@ -824,7 +826,7 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> { // want the coerced type to be the actual supertype of these two, // but for now, we want to just error to ensure we don't lock // ourselves into a specific behavior with NLL. - self.leak_check(snapshot)?; + self.leak_check(outer_universe, Some(snapshot))?; result }) diff --git a/compiler/rustc_infer/src/infer/at.rs b/compiler/rustc_infer/src/infer/at.rs index 0c8854e962a..6b2dd0a2b4f 100644 --- a/compiler/rustc_infer/src/infer/at.rs +++ b/compiler/rustc_infer/src/infer/at.rs @@ -70,8 +70,8 @@ impl<'tcx> InferCtxt<'tcx> { tcx: self.tcx, defining_use_anchor: self.defining_use_anchor, considering_regions: self.considering_regions, + skip_leak_check: self.skip_leak_check, inner: self.inner.clone(), - skip_leak_check: self.skip_leak_check.clone(), lexical_region_resolutions: self.lexical_region_resolutions.clone(), selection_cache: self.selection_cache.clone(), evaluation_cache: self.evaluation_cache.clone(), diff --git a/compiler/rustc_infer/src/infer/higher_ranked/mod.rs b/compiler/rustc_infer/src/infer/higher_ranked/mod.rs index 00e10820683..974bc2f1153 100644 --- a/compiler/rustc_infer/src/infer/higher_ranked/mod.rs +++ b/compiler/rustc_infer/src/infer/higher_ranked/mod.rs @@ -105,24 +105,31 @@ impl<'tcx> InferCtxt<'tcx> { self.tcx.replace_bound_vars_uncached(binder, delegate) } - /// See [RegionConstraintCollector::leak_check][1]. + /// See [RegionConstraintCollector::leak_check][1]. We only check placeholder + /// leaking into `outer_universe`, i.e. placeholders which cannot be named by that + /// universe. /// /// [1]: crate::infer::region_constraints::RegionConstraintCollector::leak_check - pub fn leak_check(&self, snapshot: &CombinedSnapshot<'tcx>) -> RelateResult<'tcx, ()> { + pub fn leak_check( + &self, + outer_universe: ty::UniverseIndex, + only_consider_snapshot: Option<&CombinedSnapshot<'tcx>>, + ) -> RelateResult<'tcx, ()> { // If the user gave `-Zno-leak-check`, or we have been // configured to skip the leak check, then skip the leak check // completely. The leak check is deprecated. Any legitimate // subtyping errors that it would have caught will now be // caught later on, during region checking. However, we // continue to use it for a transition period. - if self.tcx.sess.opts.unstable_opts.no_leak_check || self.skip_leak_check.get() { + if self.tcx.sess.opts.unstable_opts.no_leak_check || self.skip_leak_check { return Ok(()); } self.inner.borrow_mut().unwrap_region_constraints().leak_check( self.tcx, + outer_universe, self.universe(), - snapshot, + only_consider_snapshot, ) } } diff --git a/compiler/rustc_infer/src/infer/mod.rs b/compiler/rustc_infer/src/infer/mod.rs index b49282726fd..447d4c9f84b 100644 --- a/compiler/rustc_infer/src/infer/mod.rs +++ b/compiler/rustc_infer/src/infer/mod.rs @@ -251,14 +251,13 @@ pub struct InferCtxt<'tcx> { /// solving is left to borrowck instead. pub considering_regions: bool, - pub inner: RefCell>, - /// If set, this flag causes us to skip the 'leak check' during /// higher-ranked subtyping operations. This flag is a temporary one used /// to manage the removal of the leak-check: for the time being, we still run the - /// leak-check, but we issue warnings. This flag can only be set to true - /// when entering a snapshot. - skip_leak_check: Cell, + /// leak-check, but we issue warnings. + skip_leak_check: bool, + + pub inner: RefCell>, /// Once region inference is done, the values for each variable. lexical_region_resolutions: RefCell>>, @@ -543,6 +542,7 @@ pub struct InferCtxtBuilder<'tcx> { tcx: TyCtxt<'tcx>, defining_use_anchor: DefiningAnchor, considering_regions: bool, + skip_leak_check: bool, /// Whether we are in coherence mode. intercrate: bool, } @@ -557,6 +557,7 @@ impl<'tcx> TyCtxtInferExt<'tcx> for TyCtxt<'tcx> { tcx: self, defining_use_anchor: DefiningAnchor::Error, considering_regions: true, + skip_leak_check: false, intercrate: false, } } @@ -584,6 +585,11 @@ impl<'tcx> InferCtxtBuilder<'tcx> { self } + pub fn skip_leak_check(mut self, skip_leak_check: bool) -> Self { + self.skip_leak_check = skip_leak_check; + self + } + /// Given a canonical value `C` as a starting point, create an /// inference context that contains each of the bound values /// within instantiated as a fresh variable. The `f` closure is @@ -605,11 +611,18 @@ impl<'tcx> InferCtxtBuilder<'tcx> { } pub fn build(&mut self) -> InferCtxt<'tcx> { - let InferCtxtBuilder { tcx, defining_use_anchor, considering_regions, intercrate } = *self; + let InferCtxtBuilder { + tcx, + defining_use_anchor, + considering_regions, + skip_leak_check, + intercrate, + } = *self; InferCtxt { tcx, defining_use_anchor, considering_regions, + skip_leak_check, inner: RefCell::new(InferCtxtInner::new()), lexical_region_resolutions: RefCell::new(None), selection_cache: Default::default(), @@ -619,7 +632,6 @@ impl<'tcx> InferCtxtBuilder<'tcx> { tainted_by_errors: Cell::new(None), err_count_on_creation: tcx.sess.err_count(), in_snapshot: Cell::new(false), - skip_leak_check: Cell::new(false), universe: Cell::new(ty::UniverseIndex::ROOT), intercrate, } @@ -815,32 +827,9 @@ impl<'tcx> InferCtxt<'tcx> { r } - /// If `should_skip` is true, then execute `f` then unroll any bindings it creates. - #[instrument(skip(self, f), level = "debug")] - pub fn probe_maybe_skip_leak_check(&self, should_skip: bool, f: F) -> R - where - F: FnOnce(&CombinedSnapshot<'tcx>) -> R, - { - let snapshot = self.start_snapshot(); - let was_skip_leak_check = self.skip_leak_check.get(); - if should_skip { - self.skip_leak_check.set(true); - } - let r = f(&snapshot); - self.rollback_to("probe", snapshot); - self.skip_leak_check.set(was_skip_leak_check); - r - } - - /// Scan the constraints produced since `snapshot` began and returns: - /// - /// - `None` -- if none of them involves "region outlives" constraints. - /// - `Some(true)` -- if there are `'a: 'b` constraints where `'a` or `'b` is a placeholder. - /// - `Some(false)` -- if there are `'a: 'b` constraints but none involve placeholders. - pub fn region_constraints_added_in_snapshot( - &self, - snapshot: &CombinedSnapshot<'tcx>, - ) -> Option { + /// Scan the constraints produced since `snapshot` and check whether + /// we added any region constraints. + pub fn region_constraints_added_in_snapshot(&self, snapshot: &CombinedSnapshot<'tcx>) -> bool { self.inner .borrow_mut() .unwrap_region_constraints() diff --git a/compiler/rustc_infer/src/infer/region_constraints/leak_check.rs b/compiler/rustc_infer/src/infer/region_constraints/leak_check.rs index 7b9e6983212..dd65f66ccd1 100644 --- a/compiler/rustc_infer/src/infer/region_constraints/leak_check.rs +++ b/compiler/rustc_infer/src/infer/region_constraints/leak_check.rs @@ -3,7 +3,6 @@ use crate::infer::CombinedSnapshot; use rustc_data_structures::{ fx::FxIndexMap, graph::{scc::Sccs, vec_graph::VecGraph}, - undo_log::UndoLogs, }; use rustc_index::Idx; use rustc_middle::ty::error::TypeError; @@ -13,7 +12,9 @@ impl<'tcx> RegionConstraintCollector<'_, 'tcx> { /// Searches new universes created during `snapshot`, looking for /// placeholders that may "leak" out from the universes they are contained /// in. If any leaking placeholders are found, then an `Err` is returned - /// (typically leading to the snapshot being reversed). + /// (typically leading to the snapshot being reversed). This algorithm + /// only looks at placeholders which cannot be named by `outer_universe`, + /// as this is the universe we're currently checking for a leak. /// /// The leak check *used* to be the only way we had to handle higher-ranked /// obligations. Now that we have integrated universes into the region @@ -55,6 +56,12 @@ impl<'tcx> RegionConstraintCollector<'_, 'tcx> { /// * 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 /// + /// 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 + /// snapshot. If we were to not do that the `leak_check` during evaluation can rely on + /// region constraints added outside of that evaluation. As that is not reflected in the + /// cache key this would be unsound. + /// /// # Historical note /// /// Older variants of the leak check used to report errors for these @@ -62,29 +69,21 @@ 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 + #[instrument(level = "debug", skip(self, tcx, only_consider_snapshot), ret)] pub fn leak_check( &mut self, tcx: TyCtxt<'tcx>, + outer_universe: ty::UniverseIndex, max_universe: ty::UniverseIndex, - snapshot: &CombinedSnapshot<'tcx>, + only_consider_snapshot: Option<&CombinedSnapshot<'tcx>>, ) -> RelateResult<'tcx, ()> { - debug!( - "leak_check(max_universe={:?}, snapshot.universe={:?})", - max_universe, snapshot.universe - ); - - assert!(UndoLogs::>::in_snapshot(&self.undo_log)); - - let universe_at_start_of_snapshot = snapshot.universe; - if universe_at_start_of_snapshot == max_universe { + if outer_universe == max_universe { return Ok(()); } - let mini_graph = - &MiniGraph::new(tcx, self.undo_log.region_constraints(), &self.storage.data.verifys); + let mini_graph = &MiniGraph::new(tcx, &self, only_consider_snapshot); - let mut leak_check = - LeakCheck::new(tcx, universe_at_start_of_snapshot, max_universe, mini_graph, self); + 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(()) @@ -93,7 +92,7 @@ impl<'tcx> RegionConstraintCollector<'_, 'tcx> { struct LeakCheck<'me, 'tcx> { tcx: TyCtxt<'tcx>, - universe_at_start_of_snapshot: ty::UniverseIndex, + outer_universe: ty::UniverseIndex, mini_graph: &'me MiniGraph<'tcx>, rcc: &'me RegionConstraintCollector<'me, 'tcx>, @@ -121,7 +120,7 @@ struct LeakCheck<'me, 'tcx> { impl<'me, 'tcx> LeakCheck<'me, 'tcx> { fn new( tcx: TyCtxt<'tcx>, - universe_at_start_of_snapshot: ty::UniverseIndex, + outer_universe: ty::UniverseIndex, max_universe: ty::UniverseIndex, mini_graph: &'me MiniGraph<'tcx>, rcc: &'me RegionConstraintCollector<'me, 'tcx>, @@ -129,7 +128,7 @@ impl<'me, 'tcx> LeakCheck<'me, 'tcx> { let dummy_scc_universe = SccUniverse { universe: max_universe, region: None }; Self { tcx, - universe_at_start_of_snapshot, + outer_universe, mini_graph, rcc, scc_placeholders: IndexVec::from_elem_n(None, mini_graph.sccs.num_sccs()), @@ -154,7 +153,7 @@ impl<'me, 'tcx> LeakCheck<'me, 'tcx> { // Detect those SCCs that directly contain a placeholder if let ty::RePlaceholder(placeholder) = **region { - if self.universe_at_start_of_snapshot.cannot_name(placeholder.universe) { + if self.outer_universe.cannot_name(placeholder.universe) { self.assign_scc_value(scc, placeholder)?; } } @@ -364,56 +363,70 @@ struct MiniGraph<'tcx> { } impl<'tcx> MiniGraph<'tcx> { - fn new<'a>( + fn new( tcx: TyCtxt<'tcx>, - undo_log: impl Iterator>, - verifys: &[Verify<'tcx>], - ) -> Self - where - 'tcx: 'a, - { + region_constraints: &RegionConstraintCollector<'_, 'tcx>, + only_consider_snapshot: Option<&CombinedSnapshot<'tcx>>, + ) -> Self { let mut nodes = FxIndexMap::default(); let mut edges = Vec::new(); // Note that if `R2: R1`, we get a callback `r1, r2`, so `target` is first parameter. - Self::iterate_undo_log(tcx, undo_log, verifys, |target, source| { - let source_node = Self::add_node(&mut nodes, source); - let target_node = Self::add_node(&mut nodes, target); - edges.push((source_node, target_node)); - }); + Self::iterate_region_constraints( + tcx, + region_constraints, + only_consider_snapshot, + |target, source| { + let source_node = Self::add_node(&mut nodes, source); + let target_node = Self::add_node(&mut nodes, target); + edges.push((source_node, target_node)); + }, + ); let graph = VecGraph::new(nodes.len(), edges); let sccs = Sccs::new(&graph); Self { nodes, sccs } } /// Invokes `each_edge(R1, R2)` for each edge where `R2: R1` - fn iterate_undo_log<'a>( + fn iterate_region_constraints( tcx: TyCtxt<'tcx>, - undo_log: impl Iterator>, - verifys: &[Verify<'tcx>], + region_constraints: &RegionConstraintCollector<'_, 'tcx>, + only_consider_snapshot: Option<&CombinedSnapshot<'tcx>>, mut each_edge: impl FnMut(ty::Region<'tcx>, ty::Region<'tcx>), - ) where - 'tcx: 'a, - { - for undo_entry in undo_log { - match undo_entry { - &AddConstraint(Constraint::VarSubVar(a, b)) => { - each_edge(ty::Region::new_var(tcx, a), ty::Region::new_var(tcx, b)); - } - &AddConstraint(Constraint::RegSubVar(a, b)) => { - each_edge(a, ty::Region::new_var(tcx, b)); - } - &AddConstraint(Constraint::VarSubReg(a, b)) => { - each_edge(ty::Region::new_var(tcx, a), b); - } - &AddConstraint(Constraint::RegSubReg(a, b)) => { - each_edge(a, b); + ) { + let mut each_constraint = |constraint| match constraint { + &Constraint::VarSubVar(a, b) => { + each_edge(ty::Region::new_var(tcx, a), ty::Region::new_var(tcx, b)); + } + &Constraint::RegSubVar(a, b) => { + each_edge(a, ty::Region::new_var(tcx, b)); + } + &Constraint::VarSubReg(a, b) => { + each_edge(ty::Region::new_var(tcx, a), b); + } + &Constraint::RegSubReg(a, b) => { + each_edge(a, b); + } + }; + + if let Some(snapshot) = only_consider_snapshot { + for undo_entry in + region_constraints.undo_log.region_constraints_in_snapshot(&snapshot.undo_snapshot) + { + match undo_entry { + AddConstraint(constraint) => { + each_constraint(constraint); + } + &AddVerify(i) => span_bug!( + region_constraints.data().verifys[i].origin.span(), + "we never add verifications while doing higher-ranked things", + ), + &AddCombination(..) | &AddVar(..) => {} } - &AddVerify(i) => span_bug!( - verifys[i].origin.span(), - "we never add verifications while doing higher-ranked things", - ), - &AddCombination(..) | &AddVar(..) => {} + } + } else { + for (constraint, _origin) in ®ion_constraints.data().constraints { + each_constraint(constraint) } } } diff --git a/compiler/rustc_infer/src/infer/region_constraints/mod.rs b/compiler/rustc_infer/src/infer/region_constraints/mod.rs index cd8d23bf635..613da8a0b45 100644 --- a/compiler/rustc_infer/src/infer/region_constraints/mod.rs +++ b/compiler/rustc_infer/src/infer/region_constraints/mod.rs @@ -400,7 +400,7 @@ impl<'tcx> RegionConstraintCollector<'_, 'tcx> { data } - pub(super) fn data(&self) -> &RegionConstraintData<'tcx> { + pub fn data(&self) -> &RegionConstraintData<'tcx> { &self.data } @@ -683,15 +683,10 @@ impl<'tcx> RegionConstraintCollector<'_, 'tcx> { } /// See `InferCtxt::region_constraints_added_in_snapshot`. - pub fn region_constraints_added_in_snapshot(&self, mark: &Snapshot<'tcx>) -> Option { + pub fn region_constraints_added_in_snapshot(&self, mark: &Snapshot<'tcx>) -> bool { self.undo_log .region_constraints_in_snapshot(mark) - .map(|&elt| match elt { - AddConstraint(constraint) => Some(constraint.involves_placeholders()), - _ => None, - }) - .max() - .unwrap_or(None) + .any(|&elt| matches!(elt, AddConstraint(_))) } #[inline] diff --git a/compiler/rustc_infer/src/infer/undo_log.rs b/compiler/rustc_infer/src/infer/undo_log.rs index 955c54e8515..8dd1a7c5f21 100644 --- a/compiler/rustc_infer/src/infer/undo_log.rs +++ b/compiler/rustc_infer/src/infer/undo_log.rs @@ -183,15 +183,6 @@ impl<'tcx> InferCtxtUndoLogs<'tcx> { self.logs[s.undo_len..].iter().any(|log| matches!(log, UndoLog::OpaqueTypes(..))) } - pub(crate) fn region_constraints( - &self, - ) -> impl Iterator> + Clone { - self.logs.iter().filter_map(|log| match log { - UndoLog::RegionConstraintCollector(log) => Some(log), - _ => None, - }) - } - fn assert_open_snapshot(&self, snapshot: &Snapshot<'tcx>) { // Failures here may indicate a failure to follow a stack discipline. assert!(self.logs.len() >= snapshot.undo_len); diff --git a/compiler/rustc_trait_selection/src/traits/coherence.rs b/compiler/rustc_trait_selection/src/traits/coherence.rs index dda77c2a461..d6fd457de06 100644 --- a/compiler/rustc_trait_selection/src/traits/coherence.rs +++ b/compiler/rustc_trait_selection/src/traits/coherence.rs @@ -5,7 +5,7 @@ //! [trait-specialization]: https://rustc-dev-guide.rust-lang.org/traits/specialization.html use crate::infer::outlives::env::OutlivesEnvironment; -use crate::infer::{CombinedSnapshot, InferOk}; +use crate::infer::InferOk; use crate::traits::outlives_bounds::InferCtxtExt as _; use crate::traits::select::IntercrateAmbiguityCause; use crate::traits::util::impl_subject_and_oblig; @@ -62,6 +62,21 @@ pub fn add_placeholder_note(err: &mut Diagnostic) { ); } +#[derive(Debug, Clone, Copy)] +enum TrackAmbiguityCauses { + Yes, + No, +} + +impl TrackAmbiguityCauses { + fn is_yes(self) -> bool { + match self { + TrackAmbiguityCauses::Yes => true, + TrackAmbiguityCauses::No => false, + } + } +} + /// If there are types that satisfy both impls, returns `Some` /// with a suitably-freshened `ImplHeader` with those types /// substituted. Otherwise, returns `None`. @@ -97,29 +112,28 @@ pub fn overlapping_impls( return None; } - let infcx = tcx - .infer_ctxt() - .with_opaque_type_inference(DefiningAnchor::Bubble) - .intercrate(true) - .build(); - let selcx = &mut SelectionContext::new(&infcx); - let overlaps = - overlap(selcx, skip_leak_check, impl1_def_id, impl2_def_id, overlap_mode).is_some(); - if !overlaps { - return None; - } + let _overlap_with_bad_diagnostics = overlap( + tcx, + TrackAmbiguityCauses::No, + skip_leak_check, + impl1_def_id, + impl2_def_id, + overlap_mode, + )?; // In the case where we detect an error, run the check again, but // this time tracking intercrate ambiguity causes for better // diagnostics. (These take time and can lead to false errors.) - let infcx = tcx - .infer_ctxt() - .with_opaque_type_inference(DefiningAnchor::Bubble) - .intercrate(true) - .build(); - let selcx = &mut SelectionContext::new(&infcx); - selcx.enable_tracking_intercrate_ambiguity_causes(); - Some(overlap(selcx, skip_leak_check, impl1_def_id, impl2_def_id, overlap_mode).unwrap()) + let overlap = overlap( + tcx, + TrackAmbiguityCauses::Yes, + skip_leak_check, + impl1_def_id, + impl2_def_id, + overlap_mode, + ) + .unwrap(); + Some(overlap) } fn with_fresh_ty_vars<'cx, 'tcx>( @@ -146,40 +160,34 @@ fn with_fresh_ty_vars<'cx, 'tcx>( /// Can both impl `a` and impl `b` be satisfied by a common type (including /// where-clauses)? If so, returns an `ImplHeader` that unifies the two impls. -fn overlap<'cx, 'tcx>( - selcx: &mut SelectionContext<'cx, 'tcx>, +#[instrument(level = "debug", skip(tcx))] +fn overlap<'tcx>( + tcx: TyCtxt<'tcx>, + track_ambiguity_causes: TrackAmbiguityCauses, skip_leak_check: SkipLeakCheck, impl1_def_id: DefId, impl2_def_id: DefId, overlap_mode: OverlapMode, ) -> Option> { - debug!( - "overlap(impl1_def_id={:?}, impl2_def_id={:?}, overlap_mode={:?})", - impl1_def_id, impl2_def_id, overlap_mode - ); - - selcx.infcx.probe_maybe_skip_leak_check(skip_leak_check.is_yes(), |snapshot| { - overlap_within_probe(selcx, impl1_def_id, impl2_def_id, overlap_mode, snapshot) - }) -} - -fn overlap_within_probe<'cx, 'tcx>( - selcx: &mut SelectionContext<'cx, 'tcx>, - impl1_def_id: DefId, - impl2_def_id: DefId, - overlap_mode: OverlapMode, - snapshot: &CombinedSnapshot<'tcx>, -) -> Option> { - let infcx = selcx.infcx; - if overlap_mode.use_negative_impl() { - if negative_impl(infcx.tcx, impl1_def_id, impl2_def_id) - || negative_impl(infcx.tcx, impl2_def_id, impl1_def_id) + if negative_impl(tcx, impl1_def_id, impl2_def_id) + || negative_impl(tcx, impl2_def_id, impl1_def_id) { return None; } } + let infcx = tcx + .infer_ctxt() + .with_opaque_type_inference(DefiningAnchor::Bubble) + .skip_leak_check(skip_leak_check.is_yes()) + .intercrate(true) + .build(); + let selcx = &mut SelectionContext::new(&infcx); + if track_ambiguity_causes.is_yes() { + selcx.enable_tracking_intercrate_ambiguity_causes(); + } + // For the purposes of this check, we don't bring any placeholder // types into scope; instead, we replace the generic types with // fresh type variables, and hence we do our evaluations in an @@ -198,18 +206,23 @@ fn overlap_within_probe<'cx, 'tcx>( } } - // We disable the leak when creating the `snapshot` by using - // `infcx.probe_maybe_disable_leak_check`. - if infcx.leak_check(snapshot).is_err() { + // We toggle the `leak_check` by using `skip_leak_check` when constructing the + // inference context, so this may be a noop. + if infcx.leak_check(ty::UniverseIndex::ROOT, None).is_err() { debug!("overlap: leak check failed"); return None; } let intercrate_ambiguity_causes = selcx.take_intercrate_ambiguity_causes(); debug!("overlap: intercrate_ambiguity_causes={:#?}", intercrate_ambiguity_causes); - - let involves_placeholder = - matches!(selcx.infcx.region_constraints_added_in_snapshot(snapshot), Some(true)); + let involves_placeholder = infcx + .inner + .borrow_mut() + .unwrap_region_constraints() + .data() + .constraints + .iter() + .any(|c| c.0.involves_placeholders()); let impl_header = selcx.infcx.resolve_vars_if_possible(impl1_header); Some(OverlapResult { impl_header, intercrate_ambiguity_causes, involves_placeholder }) diff --git a/compiler/rustc_trait_selection/src/traits/query/evaluate_obligation.rs b/compiler/rustc_trait_selection/src/traits/query/evaluate_obligation.rs index edbe2de8105..a8a74d7501a 100644 --- a/compiler/rustc_trait_selection/src/traits/query/evaluate_obligation.rs +++ b/compiler/rustc_trait_selection/src/traits/query/evaluate_obligation.rs @@ -90,7 +90,7 @@ impl<'tcx> InferCtxtExt<'tcx> for InferCtxt<'tcx> { Ok(EvaluationResult::EvaluatedToAmbig) } else if self.opaque_types_added_in_snapshot(snapshot) { Ok(EvaluationResult::EvaluatedToOkModuloOpaqueTypes) - } else if self.region_constraints_added_in_snapshot(snapshot).is_some() { + } else if self.region_constraints_added_in_snapshot(snapshot) { Ok(EvaluationResult::EvaluatedToOkModuloRegions) } else { Ok(EvaluationResult::EvaluatedToOk) diff --git a/compiler/rustc_trait_selection/src/traits/select/mod.rs b/compiler/rustc_trait_selection/src/traits/select/mod.rs index 2c0b911c805..42c1b629ac2 100644 --- a/compiler/rustc_trait_selection/src/traits/select/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/select/mod.rs @@ -561,9 +561,10 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { op: impl FnOnce(&mut Self) -> Result, ) -> Result { self.infcx.probe(|snapshot| -> Result { + let outer_universe = self.infcx.universe(); let result = op(self)?; - match self.infcx.leak_check(snapshot) { + match self.infcx.leak_check(outer_universe, Some(snapshot)) { Ok(()) => {} Err(_) => return Ok(EvaluatedToErr), } @@ -572,9 +573,10 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { return Ok(result.max(EvaluatedToOkModuloOpaqueTypes)); } - match self.infcx.region_constraints_added_in_snapshot(snapshot) { - None => Ok(result), - Some(_) => Ok(result.max(EvaluatedToOkModuloRegions)), + if self.infcx.region_constraints_added_in_snapshot(snapshot) { + Ok(result.max(EvaluatedToOkModuloRegions)) + } else { + Ok(result) } }) } -- cgit 1.4.1-3-g733a5 From 6b92d45342f14f7a38355923f75ab3702852ad8a Mon Sep 17 00:00:00 2001 From: lcnr Date: Tue, 23 May 2023 19:00:47 +0200 Subject: change redundant `clear()` to assertion --- compiler/rustc_infer/src/infer/undo_log.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) (limited to 'compiler') diff --git a/compiler/rustc_infer/src/infer/undo_log.rs b/compiler/rustc_infer/src/infer/undo_log.rs index 8dd1a7c5f21..25d06b21ec8 100644 --- a/compiler/rustc_infer/src/infer/undo_log.rs +++ b/compiler/rustc_infer/src/infer/undo_log.rs @@ -138,11 +138,9 @@ impl<'tcx> InferCtxtInner<'tcx> { } if self.undo_log.num_open_snapshots == 1 { - // The root snapshot. It's safe to clear the undo log because - // there's no snapshot further out that we might need to roll back - // to. + // After the root snapshot the undo log should be empty. assert!(snapshot.undo_len == 0); - self.undo_log.logs.clear(); + assert!(self.undo_log.logs.is_empty()); } self.undo_log.num_open_snapshots -= 1; -- cgit 1.4.1-3-g733a5 From 6f9041bd1572239bec2d8653b84ea4ddd2a7d22d Mon Sep 17 00:00:00 2001 From: lcnr Date: Tue, 23 May 2023 19:34:17 +0200 Subject: add the leak check to the new solver --- .../src/solve/eval_ctxt/canonical.rs | 5 +++++ tests/ui/higher-ranked/leak-check-in-selection.rs | 24 ++++++++++++++++++++++ 2 files changed, 29 insertions(+) create mode 100644 tests/ui/higher-ranked/leak-check-in-selection.rs (limited to 'compiler') diff --git a/compiler/rustc_trait_selection/src/solve/eval_ctxt/canonical.rs b/compiler/rustc_trait_selection/src/solve/eval_ctxt/canonical.rs index fdb209fbff8..91a093f30bd 100644 --- a/compiler/rustc_trait_selection/src/solve/eval_ctxt/canonical.rs +++ b/compiler/rustc_trait_selection/src/solve/eval_ctxt/canonical.rs @@ -137,6 +137,11 @@ impl<'tcx> EvalCtxt<'_, 'tcx> { #[instrument(level = "debug", skip(self), ret)] fn compute_external_query_constraints(&self) -> Result, NoSolution> { + self.infcx.leak_check(ty::UniverseIndex::ROOT, None).map_err(|e| { + debug!(?e, "failed the leak check"); + NoSolution + })?; + // Cannot use `take_registered_region_obligations` as we may compute the response // inside of a `probe` whenever we have multiple choices inside of the solver. let region_obligations = self.infcx.inner.borrow().region_obligations().to_owned(); diff --git a/tests/ui/higher-ranked/leak-check-in-selection.rs b/tests/ui/higher-ranked/leak-check-in-selection.rs new file mode 100644 index 00000000000..e8d6cff856c --- /dev/null +++ b/tests/ui/higher-ranked/leak-check-in-selection.rs @@ -0,0 +1,24 @@ +// run-pass +// revisions: old next +//[next] compile-flags: -Ztrait-solver=next +#![allow(coherence_leak_check)] + +trait Trait: Sized { + fn is_higher_ranked(self) -> bool; +} + +impl Trait for for<'a> fn(&'a ()) { + fn is_higher_ranked(self) -> bool { + true + } +} +impl<'a> Trait for fn(&'a ()) { + fn is_higher_ranked(self) -> bool { + false + } +} + +fn main() { + let x: for<'a> fn(&'a ()) = |&()| (); + assert!(x.is_higher_ranked()); +} -- cgit 1.4.1-3-g733a5 From 0b81f992e9cc41d4fb957f1b9d2a99b1f73f8160 Mon Sep 17 00:00:00 2001 From: lcnr Date: Mon, 29 May 2023 14:35:11 +0200 Subject: update universe used by the leak check --- compiler/rustc_trait_selection/src/solve/eval_ctxt/canonical.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'compiler') diff --git a/compiler/rustc_trait_selection/src/solve/eval_ctxt/canonical.rs b/compiler/rustc_trait_selection/src/solve/eval_ctxt/canonical.rs index 91a093f30bd..bca2343e424 100644 --- a/compiler/rustc_trait_selection/src/solve/eval_ctxt/canonical.rs +++ b/compiler/rustc_trait_selection/src/solve/eval_ctxt/canonical.rs @@ -137,7 +137,9 @@ impl<'tcx> EvalCtxt<'_, 'tcx> { #[instrument(level = "debug", skip(self), ret)] fn compute_external_query_constraints(&self) -> Result, NoSolution> { - self.infcx.leak_check(ty::UniverseIndex::ROOT, None).map_err(|e| { + // We only check for leaks from universes which were entered inside + // of the query. + self.infcx.leak_check(self.max_input_universe, None).map_err(|e| { debug!(?e, "failed the leak check"); NoSolution })?; -- cgit 1.4.1-3-g733a5