Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve performance of coherence checks #68966

Merged
merged 4 commits into from
Feb 10, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/librustc/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -647,7 +647,8 @@ rustc_queries! {
query trait_impls_of(key: DefId) -> &'tcx ty::trait_def::TraitImpls {
desc { |tcx| "trait impls of `{}`", tcx.def_path_str(key) }
}
query specialization_graph_of(_: DefId) -> &'tcx specialization_graph::Graph {
query specialization_graph_of(key: DefId) -> &'tcx specialization_graph::Graph {
desc { |tcx| "building specialization graph of trait `{}`", tcx.def_path_str(key) }
cache_on_disk_if { true }
}
query is_object_safe(key: DefId) -> bool {
Expand Down
23 changes: 6 additions & 17 deletions src/librustc/traits/coherence.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

use crate::infer::{CombinedSnapshot, InferOk};
use crate::traits::select::IntercrateAmbiguityCause;
use crate::traits::IntercrateMode;
use crate::traits::SkipLeakCheck;
use crate::traits::{self, Normalized, Obligation, ObligationCause, SelectionContext};
use crate::ty::fold::TypeFoldable;
Expand All @@ -27,7 +26,7 @@ enum InCrate {
#[derive(Debug, Copy, Clone)]
pub enum Conflict {
Upstream,
Downstream { used_to_be_broken: bool },
Downstream,
}

pub struct OverlapResult<'tcx> {
Expand All @@ -53,7 +52,6 @@ pub fn overlapping_impls<F1, F2, R>(
tcx: TyCtxt<'_>,
impl1_def_id: DefId,
impl2_def_id: DefId,
intercrate_mode: IntercrateMode,
skip_leak_check: SkipLeakCheck,
on_overlap: F1,
no_overlap: F2,
Expand All @@ -65,13 +63,12 @@ where
debug!(
"overlapping_impls(\
impl1_def_id={:?}, \
impl2_def_id={:?},
intercrate_mode={:?})",
impl1_def_id, impl2_def_id, intercrate_mode
impl2_def_id={:?})",
impl1_def_id, impl2_def_id,
);

let overlaps = tcx.infer_ctxt().enter(|infcx| {
let selcx = &mut SelectionContext::intercrate(&infcx, intercrate_mode);
let selcx = &mut SelectionContext::intercrate(&infcx);
overlap(selcx, skip_leak_check, impl1_def_id, impl2_def_id).is_some()
});

Expand All @@ -83,7 +80,7 @@ where
// this time tracking intercrate ambuiguity causes for better
// diagnostics. (These take time and can lead to false errors.)
tcx.infer_ctxt().enter(|infcx| {
let selcx = &mut SelectionContext::intercrate(&infcx, intercrate_mode);
let selcx = &mut SelectionContext::intercrate(&infcx);
selcx.enable_tracking_intercrate_ambiguity_causes();
on_overlap(overlap(selcx, skip_leak_check, impl1_def_id, impl2_def_id).unwrap())
})
Expand Down Expand Up @@ -202,15 +199,7 @@ pub fn trait_ref_is_knowable<'tcx>(
if orphan_check_trait_ref(tcx, trait_ref, InCrate::Remote).is_ok() {
// A downstream or cousin crate is allowed to implement some
// substitution of this trait-ref.

// A trait can be implementable for a trait ref by both the current
// crate and crates downstream of it. Older versions of rustc
// were not aware of this, causing incoherence (issue #43355).
let used_to_be_broken = orphan_check_trait_ref(tcx, trait_ref, InCrate::Local).is_ok();
if used_to_be_broken {
debug!("trait_ref_is_knowable({:?}) - USED TO BE BROKEN", trait_ref);
}
return Some(Conflict::Downstream { used_to_be_broken });
return Some(Conflict::Downstream);
}

if trait_ref_is_local_or_fundamental(tcx, trait_ref) {
Expand Down
7 changes: 0 additions & 7 deletions src/librustc/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,6 @@ pub use self::chalk_fulfill::{

pub use self::types::*;

/// Whether to enable bug compatibility with issue #43355.
#[derive(Copy, Clone, PartialEq, Eq, Debug)]
pub enum IntercrateMode {
Issue43355,
Fixed,
}

/// Whether to skip the leak check, as part of a future compatibility warning step.
#[derive(Copy, Clone, PartialEq, Eq, Debug)]
pub enum SkipLeakCheck {
Expand Down
44 changes: 15 additions & 29 deletions src/librustc/traits/select.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ use super::DerivedObligationCause;
use super::Selection;
use super::SelectionResult;
use super::TraitNotObjectSafe;
use super::TraitQueryMode;
use super::{BuiltinDerivedObligation, ImplDerivedObligation, ObligationCauseCode};
use super::{IntercrateMode, TraitQueryMode};
use super::{ObjectCastObligation, Obligation};
use super::{ObligationCause, PredicateObligation, TraitObligation};
use super::{OutputTypeParameterMismatch, Overflow, SelectionError, Unimplemented};
Expand Down Expand Up @@ -80,7 +80,7 @@ pub struct SelectionContext<'cx, 'tcx> {
/// other words, we consider `$0: Bar` to be unimplemented if
/// there is no type that the user could *actually name* that
/// would satisfy it. This avoids crippling inference, basically.
intercrate: Option<IntercrateMode>,
intercrate: bool,

intercrate_ambiguity_causes: Option<Vec<IntercrateAmbiguityCause>>,

Expand Down Expand Up @@ -218,22 +218,18 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
SelectionContext {
infcx,
freshener: infcx.freshener(),
intercrate: None,
intercrate: false,
intercrate_ambiguity_causes: None,
allow_negative_impls: false,
query_mode: TraitQueryMode::Standard,
}
}

pub fn intercrate(
infcx: &'cx InferCtxt<'cx, 'tcx>,
mode: IntercrateMode,
) -> SelectionContext<'cx, 'tcx> {
debug!("intercrate({:?})", mode);
pub fn intercrate(infcx: &'cx InferCtxt<'cx, 'tcx>) -> SelectionContext<'cx, 'tcx> {
SelectionContext {
infcx,
freshener: infcx.freshener(),
intercrate: Some(mode),
intercrate: true,
intercrate_ambiguity_causes: None,
allow_negative_impls: false,
query_mode: TraitQueryMode::Standard,
Expand All @@ -248,7 +244,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
SelectionContext {
infcx,
freshener: infcx.freshener(),
intercrate: None,
intercrate: false,
intercrate_ambiguity_causes: None,
allow_negative_impls,
query_mode: TraitQueryMode::Standard,
Expand All @@ -263,7 +259,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
SelectionContext {
infcx,
freshener: infcx.freshener(),
intercrate: None,
intercrate: false,
intercrate_ambiguity_causes: None,
allow_negative_impls: false,
query_mode,
Expand All @@ -276,7 +272,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
/// false overflow results (#47139) and because it costs
/// computation time.
pub fn enable_tracking_intercrate_ambiguity_causes(&mut self) {
assert!(self.intercrate.is_some());
assert!(self.intercrate);
assert!(self.intercrate_ambiguity_causes.is_none());
self.intercrate_ambiguity_causes = Some(vec![]);
debug!("selcx: enable_tracking_intercrate_ambiguity_causes");
Expand All @@ -286,7 +282,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
/// was enabled and disables tracking at the same time. If
/// tracking is not enabled, just returns an empty vector.
pub fn take_intercrate_ambiguity_causes(&mut self) -> Vec<IntercrateAmbiguityCause> {
assert!(self.intercrate.is_some());
assert!(self.intercrate);
self.intercrate_ambiguity_causes.take().unwrap_or(vec![])
}

Expand Down Expand Up @@ -562,7 +558,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
) -> Result<EvaluationResult, OverflowError> {
debug!("evaluate_trait_predicate_recursively({:?})", obligation);

if self.intercrate.is_none()
if !self.intercrate
&& obligation.is_global()
&& obligation.param_env.caller_bounds.iter().all(|bound| bound.needs_subst())
{
Expand Down Expand Up @@ -727,7 +723,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
stack.fresh_trait_ref.skip_binder().input_types().any(|ty| ty.is_fresh());
// This check was an imperfect workaround for a bug in the old
// intercrate mode; it should be removed when that goes away.
if unbound_input_types && self.intercrate == Some(IntercrateMode::Issue43355) {
if unbound_input_types && self.intercrate {
debug!(
"evaluate_stack({:?}) --> unbound argument, intercrate --> ambiguous",
stack.fresh_trait_ref
Expand Down Expand Up @@ -1206,7 +1202,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
fn is_knowable<'o>(&mut self, stack: &TraitObligationStack<'o, 'tcx>) -> Option<Conflict> {
debug!("is_knowable(intercrate={:?})", self.intercrate);

if !self.intercrate.is_some() {
if !self.intercrate {
return None;
}

Expand All @@ -1218,17 +1214,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
// bound regions.
let trait_ref = predicate.skip_binder().trait_ref;

let result = coherence::trait_ref_is_knowable(self.tcx(), trait_ref);
if let (
Some(Conflict::Downstream { used_to_be_broken: true }),
Some(IntercrateMode::Issue43355),
) = (result, self.intercrate)
{
debug!("is_knowable: IGNORING conflict to be bug-compatible with #43355");
None
} else {
result
}
coherence::trait_ref_is_knowable(self.tcx(), trait_ref)
}

/// Returns `true` if the global caches can be used.
Expand All @@ -1249,7 +1235,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
// the master cache. Since coherence executes pretty quickly,
// it's not worth going to more trouble to increase the
// hit-rate, I don't think.
if self.intercrate.is_some() {
if self.intercrate {
return false;
}

Expand Down Expand Up @@ -3305,7 +3291,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
return Err(());
}

if self.intercrate.is_none()
if !self.intercrate
&& self.tcx().impl_polarity(impl_def_id) == ty::ImplPolarity::Reservation
{
debug!("match_impl: reservation impls only apply in intercrate mode");
Expand Down
7 changes: 1 addition & 6 deletions src/librustc/traits/specialize/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -332,14 +332,9 @@ pub(super) fn specialization_graph_provider(
let impl_span =
tcx.sess.source_map().def_span(tcx.span_of_impl(impl_def_id).unwrap());
let mut err = match used_to_be_allowed {
Some(FutureCompatOverlapErrorKind::Issue43355) | None => {
struct_span_err!(tcx.sess, impl_span, E0119, "{}", msg)
}
None => struct_span_err!(tcx.sess, impl_span, E0119, "{}", msg),
Some(kind) => {
let lint = match kind {
FutureCompatOverlapErrorKind::Issue43355 => {
unreachable!("converted to hard error above")
}
FutureCompatOverlapErrorKind::Issue33140 => {
ORDER_DEPENDENT_TRAIT_OBJECTS
}
Expand Down
30 changes: 6 additions & 24 deletions src/librustc/traits/specialize/specialization_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ pub use rustc::traits::types::specialization_graph::*;

#[derive(Copy, Clone, Debug)]
pub enum FutureCompatOverlapErrorKind {
Issue43355,
Issue33140,
LeakCheck,
}
Expand Down Expand Up @@ -107,16 +106,16 @@ impl<'tcx> Children {
}
};

let allowed_to_overlap =
tcx.impls_are_allowed_to_overlap(impl_def_id, possible_sibling);

let (le, ge) = traits::overlapping_impls(
tcx,
possible_sibling,
impl_def_id,
traits::IntercrateMode::Issue43355,
traits::SkipLeakCheck::default(),
|overlap| {
if let Some(overlap_kind) =
tcx.impls_are_allowed_to_overlap(impl_def_id, possible_sibling)
{
if let Some(overlap_kind) = &allowed_to_overlap {
match overlap_kind {
ty::ImplOverlapKind::Permitted { marker: _ } => {}
ty::ImplOverlapKind::Issue33140 => {
Expand Down Expand Up @@ -154,31 +153,14 @@ impl<'tcx> Children {

replace_children.push(possible_sibling);
} else {
if let None = tcx.impls_are_allowed_to_overlap(impl_def_id, possible_sibling) {
// do future-compat checks for overlap. Have issue #33140
// errors overwrite issue #43355 errors when both are present.

traits::overlapping_impls(
tcx,
possible_sibling,
impl_def_id,
traits::IntercrateMode::Fixed,
traits::SkipLeakCheck::default(),
|overlap| {
last_lint = Some(FutureCompatOverlapError {
error: overlap_error(overlap),
kind: FutureCompatOverlapErrorKind::Issue43355,
});
},
|| (),
);
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::IntercrateMode::Fixed,
traits::SkipLeakCheck::Yes,
|overlap| {
last_lint = Some(FutureCompatOverlapError {
Expand Down
12 changes: 5 additions & 7 deletions src/librustc_typeck/coherence/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,12 @@ use rustc_hir::def_id::DefId;
use rustc_hir::ItemKind;

pub fn check_trait(tcx: TyCtxt<'_>, trait_def_id: DefId) {
let lang_items = tcx.lang_items();
Checker { tcx, trait_def_id }
.check(tcx.lang_items().drop_trait(), visit_implementation_of_drop)
.check(tcx.lang_items().copy_trait(), visit_implementation_of_copy)
.check(tcx.lang_items().coerce_unsized_trait(), visit_implementation_of_coerce_unsized)
.check(
tcx.lang_items().dispatch_from_dyn_trait(),
visit_implementation_of_dispatch_from_dyn,
);
.check(lang_items.drop_trait(), visit_implementation_of_drop)
.check(lang_items.copy_trait(), visit_implementation_of_copy)
.check(lang_items.coerce_unsized_trait(), visit_implementation_of_coerce_unsized)
.check(lang_items.dispatch_from_dyn_trait(), visit_implementation_of_dispatch_from_dyn);
}

struct Checker<'tcx> {
Expand Down
3 changes: 1 addition & 2 deletions src/librustc_typeck/coherence/inherent_impls_overlap.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::namespace::Namespace;
use rustc::traits::{self, IntercrateMode, SkipLeakCheck};
use rustc::traits::{self, SkipLeakCheck};
use rustc::ty::{AssocItem, TyCtxt};
use rustc_errors::struct_span_err;
use rustc_hir as hir;
Expand Down Expand Up @@ -93,7 +93,6 @@ impl InherentOverlapChecker<'tcx> {
self.tcx,
impl1_def_id,
impl2_def_id,
IntercrateMode::Issue43355,
// We go ahead and just skip the leak check for
// inherent impls without warning.
SkipLeakCheck::Yes,
Expand Down
Loading