Skip to content

Commit

Permalink
Auto merge of rust-lang#119820 - lcnr:leak-check-2, r=<try>
Browse files Browse the repository at this point in the history 
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`
  • Loading branch information
@bors
bors committed Feb 1, 2024
2 parents 11f32b7 + cab2f4e commit d085246
Show file tree
Hide file tree
Showing 49 changed files with 466 additions and 576 deletions.
13 changes: 3 additions & 10 deletions compiler/rustc_hir_analysis/src/coherence/inherent_impls_overlap.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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)
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_infer/src/infer/at.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
15 changes: 0 additions & 15 deletions compiler/rustc_infer/src/infer/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand All @@ -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(),
}
Expand Down Expand Up @@ -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
Expand All @@ -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(),
Expand Down
12 changes: 5 additions & 7 deletions compiler/rustc_infer/src/infer/relate/higher_ranked.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(());
}

Expand Down
5 changes: 5 additions & 0 deletions compiler/rustc_lint/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Expand Down
41 changes: 0 additions & 41 deletions compiler/rustc_lint_defs/src/builtin.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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.
///
Expand Down
38 changes: 14 additions & 24 deletions compiler/rustc_trait_selection/src/traits/coherence.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand All @@ -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)
}

Expand Down Expand Up @@ -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,
Expand All @@ -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();
Expand Down Expand Up @@ -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;
Expand Down
18 changes: 0 additions & 18 deletions compiler/rustc_trait_selection/src/traits/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
48 changes: 36 additions & 12 deletions compiler/rustc_trait_selection/src/traits/select/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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 }))
}
Expand Down Expand Up @@ -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(
Expand All @@ -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(()) => {}
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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),
}
Expand Down
Loading

0 comments on commit d085246

Please sign in to comment.