Auto merge of rust-lang#119820 - lcnr:leak-check-2, r=<try>

instantiate higher ranked goals outside of candidate selection, remove the `coherence_leak_check` future compat lint

- rust-lang#59490
- rust-lang#56105

---

and adapt the old solver to not rely on universe errors for higher ranked goals to impact candidate selection. This matches the behavior of the new solver: rust-lang/trait-system-refactor-initiative#34. See https://hackmd.io/gstwC83ORaG7V29w54nMdA for more details about this change

r? `@nikomatsakis`

compiler/rustc_hir_analysis/src/coherence/inherent_impls_overlap.rs

@@ -7,7 +7,7 @@ use rustc_index::IndexVec;
 use rustc_middle::traits::specialization_graph::OverlapMode;
 use rustc_middle::ty::{self, TyCtxt};
 use rustc_span::{ErrorGuaranteed, Symbol};
-use rustc_trait_selection::traits::{self, SkipLeakCheck};
+use rustc_trait_selection::traits;
 use smallvec::SmallVec;
 use std::collections::hash_map::Entry;
 
@@ -145,15 +145,8 @@ impl<'tcx> InherentOverlapChecker<'tcx> {
         impl1_def_id: DefId,
         impl2_def_id: DefId,
     ) -> Result<(), ErrorGuaranteed> {
-        let maybe_overlap = traits::overlapping_impls(
-            self.tcx,
-            impl1_def_id,
-            impl2_def_id,
-            // We go ahead and just skip the leak check for
-            // inherent impls without warning.
-            SkipLeakCheck::Yes,
-            overlap_mode,
-        );
+        let maybe_overlap =
+            traits::overlapping_impls(self.tcx, impl1_def_id, impl2_def_id, overlap_mode);
 
         if let Some(overlap) = maybe_overlap {
             self.check_for_common_items_in_impls(impl1_def_id, impl2_def_id, overlap)

compiler/rustc_infer/src/infer/at.rs

@@ -78,7 +78,6 @@ 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(),
             lexical_region_resolutions: self.lexical_region_resolutions.clone(),
             selection_cache: self.selection_cache.clone(),

compiler/rustc_infer/src/infer/mod.rs

@@ -258,12 +258,6 @@ pub struct InferCtxt<'tcx> {
     /// solving is left to borrowck instead.
     pub considering_regions: bool,
 
-    /// 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.
-    skip_leak_check: bool,
-
     pub inner: RefCell<InferCtxtInner<'tcx>>,
 
     /// Once region inference is done, the values for each variable.
@@ -611,7 +605,6 @@ 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,
     /// Whether we should use the new trait solver in the local inference context,
@@ -629,7 +622,6 @@ impl<'tcx> TyCtxtInferExt<'tcx> for TyCtxt<'tcx> {
             tcx: self,
             defining_use_anchor: DefiningAnchor::Error,
             considering_regions: true,
-            skip_leak_check: false,
             intercrate: false,
             next_trait_solver: self.next_trait_solver_globally(),
         }
@@ -663,11 +655,6 @@ 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
@@ -693,15 +680,13 @@ impl<'tcx> InferCtxtBuilder<'tcx> {
             tcx,
             defining_use_anchor,
             considering_regions,
-            skip_leak_check,
             intercrate,
             next_trait_solver,
         } = *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(),

compiler/rustc_infer/src/infer/relate/higher_ranked.rs

@@ -116,13 +116,11 @@ impl<'tcx> InferCtxt<'tcx> {
         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 {
+        // If the user gave `-Zno-leak-check`, then skip the leak check
+        // completely. While we considered to remove the leak check at
+        // some point, we are now confident that it will remain in some
+        // form or another.
+        if self.tcx.sess.opts.unstable_opts.no_leak_check {
             return Ok(());
         }
 

compiler/rustc_lint/src/lib.rs

@@ -416,6 +416,11 @@ fn register_builtins(store: &mut LintStore) {
         "converted into hard error, see issue #48950 \
          <https://github.com/rust-lang/rust/issues/48950> for more information",
     );
+    store.register_removed(
+        "coherence_leak_check",
+        "no longer a warning, see issue #119820 \
+         <https://github.com/rust-lang/rust/issues/119820> for more information",
+    );
     store.register_removed(
         "resolve_trait_on_defaulted_unit",
         "converted into hard error, see issue #48950 \

compiler/rustc_lint_defs/src/builtin.rs

@@ -25,7 +25,6 @@ declare_lint_pass! {
         BREAK_WITH_LABEL_AND_LOOP,
         BYTE_SLICE_IN_PACKED_STRUCT_WITH_DERIVE,
         CENUM_IMPL_DROP_CAST,
-        COHERENCE_LEAK_CHECK,
         CONFLICTING_REPR_HINTS,
         CONST_EVALUATABLE_UNCHECKED,
         CONST_ITEM_MUTATION,
@@ -1467,46 +1466,6 @@ declare_lint! {
     };
 }
 
-declare_lint! {
-    /// The `coherence_leak_check` lint detects conflicting implementations of
-    /// a trait that are only distinguished by the old leak-check code.
-    ///
-    /// ### Example
-    ///
-    /// ```rust
-    /// trait SomeTrait { }
-    /// impl SomeTrait for for<'a> fn(&'a u8) { }
-    /// impl<'a> SomeTrait for fn(&'a u8) { }
-    /// ```
-    ///
-    /// {{produces}}
-    ///
-    /// ### Explanation
-    ///
-    /// In the past, the compiler would accept trait implementations for
-    /// identical functions that differed only in where the lifetime binder
-    /// appeared. Due to a change in the borrow checker implementation to fix
-    /// several bugs, this is no longer allowed. However, since this affects
-    /// existing code, this is a [future-incompatible] lint to transition this
-    /// to a hard error in the future.
-    ///
-    /// Code relying on this pattern should introduce "[newtypes]",
-    /// like `struct Foo(for<'a> fn(&'a u8))`.
-    ///
-    /// See [issue #56105] for more details.
-    ///
-    /// [issue #56105]: https://github.com/rust-lang/rust/issues/56105
-    /// [newtypes]: https://doc.rust-lang.org/book/ch19-04-advanced-types.html#using-the-newtype-pattern-for-type-safety-and-abstraction
-    /// [future-incompatible]: ../index.md#future-incompatible-lints
-    pub COHERENCE_LEAK_CHECK,
-    Warn,
-    "distinct impls distinguished only by the leak-check code",
-    @future_incompatible = FutureIncompatibleInfo {
-        reason: FutureIncompatibilityReason::FutureReleaseErrorDontReportInDeps,
-        reference: "issue #56105 <https://github.com/rust-lang/rust/issues/56105>",
-    };
-}
-
 declare_lint! {
     /// The `deprecated` lint detects use of deprecated items.
     ///

compiler/rustc_trait_selection/src/traits/coherence.rs

@@ -14,7 +14,6 @@ use crate::traits::query::evaluate_obligation::InferCtxtExt;
 use crate::traits::select::IntercrateAmbiguityCause;
 use crate::traits::structural_normalize::StructurallyNormalizeExt;
 use crate::traits::NormalizeExt;
-use crate::traits::SkipLeakCheck;
 use crate::traits::{
     Obligation, ObligationCause, ObligationCtxt, PredicateObligation, PredicateObligations,
     SelectionContext,
@@ -85,12 +84,11 @@ impl TrackAmbiguityCauses {
 /// If there are types that satisfy both impls, returns `Some`
 /// with a suitably-freshened `ImplHeader` with those types
 /// substituted. Otherwise, returns `None`.
-#[instrument(skip(tcx, skip_leak_check), level = "debug")]
+#[instrument(skip(tcx), level = "debug")]
 pub fn overlapping_impls(
     tcx: TyCtxt<'_>,
     impl1_def_id: DefId,
     impl2_def_id: DefId,
-    skip_leak_check: SkipLeakCheck,
     overlap_mode: OverlapMode,
 ) -> Option<OverlapResult<'_>> {
     // Before doing expensive operations like entering an inference context, do
@@ -115,27 +113,14 @@ pub fn overlapping_impls(
         return None;
     }
 
-    let _overlap_with_bad_diagnostics = overlap(
-        tcx,
-        TrackAmbiguityCauses::No,
-        skip_leak_check,
-        impl1_def_id,
-        impl2_def_id,
-        overlap_mode,
-    )?;
+    let _overlap_with_bad_diagnostics =
+        overlap(tcx, TrackAmbiguityCauses::No, 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 overlap = overlap(
-        tcx,
-        TrackAmbiguityCauses::Yes,
-        skip_leak_check,
-        impl1_def_id,
-        impl2_def_id,
-        overlap_mode,
-    )
-    .unwrap();
+    let overlap =
+        overlap(tcx, TrackAmbiguityCauses::Yes, impl1_def_id, impl2_def_id, overlap_mode).unwrap();
     Some(overlap)
 }
 
@@ -177,7 +162,6 @@ fn fresh_impl_header_normalized<'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,
@@ -193,7 +177,6 @@ fn overlap<'tcx>(
     let infcx = tcx
         .infer_ctxt()
         .with_opaque_type_inference(DefiningAnchor::Bubble)
-        .skip_leak_check(skip_leak_check.is_yes())
         .intercrate(true)
         .with_next_trait_solver(tcx.next_trait_solver_in_coherence())
         .build();
@@ -231,8 +214,15 @@ fn overlap<'tcx>(
         }
     }
 
-    // We toggle the `leak_check` by using `skip_leak_check` when constructing the
-    // inference context, so this may be a noop.
+    // Detect any region errors caused by equating these two impls.
+    //
+    // Only higher ranked region errors are possible here, given that we
+    // replaced all parameter regions with existentials.
+    //
+    // Unlike a full region check, which sometimes incompletely handles
+    // `TypeOutlives` constraints, the leak check is a complete. While the
+    // leak check does not detect all region errors, it never
+    // fails in cases which would later pass full region checking.
     if infcx.leak_check(ty::UniverseIndex::ROOT, None).is_err() {
         debug!("overlap: leak check failed");
         return None;

compiler/rustc_trait_selection/src/traits/mod.rs

@@ -71,24 +71,6 @@ pub use self::util::{get_vtable_index_of_object_method, impl_item_is_final, upca
 
 pub use rustc_infer::traits::*;
 
-/// Whether to skip the leak check, as part of a future compatibility warning step.
-///
-/// The "default" for skip-leak-check corresponds to the current
-/// behavior (do not skip the leak check) -- not the behavior we are
-/// transitioning into.
-#[derive(Copy, Clone, PartialEq, Eq, Debug, Default)]
-pub enum SkipLeakCheck {
-    Yes,
-    #[default]
-    No,
-}
-
-impl SkipLeakCheck {
-    fn is_yes(self) -> bool {
-        self == SkipLeakCheck::Yes
-    }
-}
-
 /// The mode that trait queries run in.
 #[derive(Copy, Clone, PartialEq, Eq, Debug)]
 pub enum TraitQueryMode {

compiler/rustc_trait_selection/src/traits/select/mod.rs

@@ -419,7 +419,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
                     let mut no_candidates_apply = true;
 
                     for c in candidate_set.vec.iter() {
-                        if self.evaluate_candidate(stack, c)?.may_apply() {
+                        if self.evaluate_candidate(stack, c, false)?.may_apply() {
                             no_candidates_apply = false;
                             break;
                         }
@@ -490,7 +490,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
         // is needed for specialization. Propagate overflow if it occurs.
         let mut candidates = candidates
             .into_iter()
-            .map(|c| match self.evaluate_candidate(stack, &c) {
+            .map(|c| match self.evaluate_candidate(stack, &c, false) {
                 Ok(eval) if eval.may_apply() => {
                     Ok(Some(EvaluatedCandidate { candidate: c, evaluation: eval }))
                 }
@@ -580,7 +580,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
         obligation: &PredicateObligation<'tcx>,
     ) -> Result<EvaluationResult, OverflowError> {
         debug_assert!(!self.infcx.next_trait_solver());
-        self.evaluation_probe(|this| {
+        self.evaluation_probe(|this, _outer_universe| {
             let goal =
                 this.infcx.resolve_vars_if_possible((obligation.predicate, obligation.param_env));
             let mut result = this.evaluate_predicate_recursively(
@@ -596,13 +596,18 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
         })
     }
 
+    /// Computes the evaluation result of `op`, discarding any constraints.
+    ///
+    /// This also runs for leak check to allow higher ranked region errors to impact
+    /// selection. By default it checks for leaks from all universes created inside of
+    /// `op`, but this can be overwritten if necessary.
     fn evaluation_probe(
         &mut self,
-        op: impl FnOnce(&mut Self) -> Result<EvaluationResult, OverflowError>,
+        op: impl FnOnce(&mut Self, &mut ty::UniverseIndex) -> Result<EvaluationResult, OverflowError>,
     ) -> Result<EvaluationResult, OverflowError> {
         self.infcx.probe(|snapshot| -> Result<EvaluationResult, OverflowError> {
-            let outer_universe = self.infcx.universe();
-            let result = op(self)?;
+            let mut outer_universe = self.infcx.universe();
+            let result = op(self, &mut outer_universe)?;
 
             match self.infcx.leak_check(outer_universe, Some(snapshot)) {
                 Ok(()) => {}
@@ -1229,7 +1234,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
         }
 
         match self.candidate_from_obligation(stack) {
-            Ok(Some(c)) => self.evaluate_candidate(stack, &c),
+            Ok(Some(c)) => self.evaluate_candidate(stack, &c, true),
             Ok(None) => Ok(EvaluatedToAmbig),
             Err(Overflow(OverflowError::Canonical)) => Err(OverflowError::Canonical),
             Err(..) => Ok(EvaluatedToErr),
@@ -1264,10 +1269,23 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
         &mut self,
         stack: &TraitObligationStack<'o, 'tcx>,
         candidate: &SelectionCandidate<'tcx>,
+        leak_check_higher_ranked_goal: bool,
     ) -> Result<EvaluationResult, OverflowError> {
-        let mut result = self.evaluation_probe(|this| {
-            let candidate = (*candidate).clone();
-            match this.confirm_candidate(stack.obligation, candidate) {
+        let mut result = self.evaluation_probe(|this, outer_universe| {
+            // We eagerly instantiate higher ranked goals to prevent universe errors
+            // from impacting candidate selection. This matches the behavior of the new
+            // solver. This slightly weakens type inference.
+            //
+            // In case there are no unresolved type or const variables this
+            // should still not be necessary to select a unique impl as any overlap
+            // relying on a universe error from higher ranked goals should have resulted
+            // in an overlap error in coherence.
+            let p = self.infcx.instantiate_binder_with_placeholders(stack.obligation.predicate);
+            let obligation = stack.obligation.with(this.tcx(), ty::Binder::dummy(p));
+            if !leak_check_higher_ranked_goal {
+                *outer_universe = self.infcx.universe();
+            }
+            match this.confirm_candidate(&obligation, candidate.clone()) {
                 Ok(selection) => {
                     debug!(?selection);
                     this.evaluate_predicates_recursively(
@@ -1704,8 +1722,14 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
         stack: &TraitObligationStack<'o, 'tcx>,
         where_clause_trait_ref: ty::PolyTraitRef<'tcx>,
     ) -> Result<EvaluationResult, OverflowError> {
-        self.evaluation_probe(|this| {
-            match this.match_where_clause_trait_ref(stack.obligation, where_clause_trait_ref) {
+        self.evaluation_probe(|this, outer_universe| {
+            // Eagerly instantiate higher ranked goals.
+            //
+            // See the comment in `evaluate_candidate` to see why.
+            let p = self.infcx.instantiate_binder_with_placeholders(stack.obligation.predicate);
+            let obligation = stack.obligation.with(this.tcx(), ty::Binder::dummy(p));
+            *outer_universe = self.infcx.universe();
+            match this.match_where_clause_trait_ref(&obligation, where_clause_trait_ref) {
                 Ok(obligations) => this.evaluate_predicates_recursively(stack.list(), obligations),
                 Err(()) => Ok(EvaluatedToErr),
             }