Skip to content

Commit

Permalink
Rollup merge of rust-lang#69044 - jonas-schievink:dont-run-coherence-…
Browse files Browse the repository at this point in the history
…twice, r=davidtwco

Don't run coherence twice for future-compat lints

This fixes the regression introduced by rust-lang#65232 (which I mentioned in rust-lang#65232 (comment)).

Old algorithm:
* Run coherence with all future-incompatible checks off, reporting errors on any overlap.
* If there's no overlap (common case), run it *again*, with the future-incompatible checks on. Report warnings for any overlap found.

New algorithm:
* Run coherence with all additional future-incompatible checks *on*, which means that we'll find *all* potentially overlapping impls immediately.
* If this found overlap, run coherence again, with the future-incompatible checks off. If that *still* gives an error, we report it. If not, it ought to be a warning.

This reduces time spent in coherence checking for the nrf52810-pac by roughly 50% relative to current master.
  • Loading branch information
Dylan-DPC authored Feb 11, 2020
2 parents dc98cb0 + f667937 commit 0ba5e8a
Showing 1 changed file with 43 additions and 30 deletions.
73 changes: 43 additions & 30 deletions src/librustc/traits/specialize/specialization_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,10 @@ impl<'tcx> Children {
impl_def_id, simplified_self, possible_sibling,
);

let overlap_error = |overlap: traits::coherence::OverlapResult<'_>| {
// Found overlap, but no specialization; error out.
let create_overlap_error = |overlap: traits::coherence::OverlapResult<'_>| {
let trait_ref = overlap.impl_header.trait_ref.unwrap();
let self_ty = trait_ref.self_ty();

OverlapError {
with_impl: possible_sibling,
trait_desc: trait_ref.print_only_trait_path().to_string(),
Expand All @@ -106,21 +106,49 @@ impl<'tcx> Children {
}
};

let allowed_to_overlap =
tcx.impls_are_allowed_to_overlap(impl_def_id, possible_sibling);
let report_overlap_error = |overlap: traits::coherence::OverlapResult<'_>,
last_lint: &mut _| {
// Found overlap, but no specialization; error out or report future-compat warning.

// Do we *still* get overlap if we disable the future-incompatible modes?
let should_err = traits::overlapping_impls(
tcx,
possible_sibling,
impl_def_id,
traits::SkipLeakCheck::default(),
|_| true,
|| false,
);

let error = create_overlap_error(overlap);

if should_err {
Err(error)
} else {
*last_lint = Some(FutureCompatOverlapError {
error,
kind: FutureCompatOverlapErrorKind::LeakCheck,
});

Ok((false, false))
}
};

let last_lint_mut = &mut last_lint;
let (le, ge) = traits::overlapping_impls(
tcx,
possible_sibling,
impl_def_id,
traits::SkipLeakCheck::default(),
traits::SkipLeakCheck::Yes,
|overlap| {
if let Some(overlap_kind) = &allowed_to_overlap {
if let Some(overlap_kind) =
tcx.impls_are_allowed_to_overlap(impl_def_id, possible_sibling)
{
match overlap_kind {
ty::ImplOverlapKind::Permitted { marker: _ } => {}
ty::ImplOverlapKind::Issue33140 => {
last_lint = Some(FutureCompatOverlapError {
error: overlap_error(overlap),
*last_lint_mut = Some(FutureCompatOverlapError {
error: create_overlap_error(overlap),
kind: FutureCompatOverlapErrorKind::Issue33140,
});
}
Expand All @@ -132,7 +160,11 @@ impl<'tcx> Children {
let le = tcx.specializes((impl_def_id, possible_sibling));
let ge = tcx.specializes((possible_sibling, impl_def_id));

if le == ge { Err(overlap_error(overlap)) } else { Ok((le, ge)) }
if le == ge {
report_overlap_error(overlap, last_lint_mut)
} else {
Ok((le, ge))
}
},
|| Ok((false, false)),
)?;
Expand All @@ -153,27 +185,8 @@ impl<'tcx> Children {

replace_children.push(possible_sibling);
} else {
if let None = allowed_to_overlap {
// Do future-compat checks for overlap.

if last_lint.is_none() {
traits::overlapping_impls(
tcx,
possible_sibling,
impl_def_id,
traits::SkipLeakCheck::Yes,
|overlap| {
last_lint = Some(FutureCompatOverlapError {
error: overlap_error(overlap),
kind: FutureCompatOverlapErrorKind::LeakCheck,
});
},
|| (),
);
}
}

// no overlap (error bailed already via ?)
// Either there's no overlap, or the overlap was already reported by
// `overlap_error`.
}
}

Expand Down

0 comments on commit 0ba5e8a

Please sign in to comment.