From 64e40c24aade1c26c8c7d6103311a59e32ee3d16 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Thu, 17 Aug 2017 22:00:07 +0300 Subject: [PATCH 1/3] Revert "save the subobligations as well" This reverts commit 309ab478d31a699493fdb1593d9cb133705f51f0. --- src/librustc/traits/project.rs | 18 +++----- src/test/run-pass/issue-43132.rs | 74 -------------------------------- 2 files changed, 6 insertions(+), 86 deletions(-) delete mode 100644 src/test/run-pass/issue-43132.rs diff --git a/src/librustc/traits/project.rs b/src/librustc/traits/project.rs index 9e095945624cf..b5284852747f9 100644 --- a/src/librustc/traits/project.rs +++ b/src/librustc/traits/project.rs @@ -462,19 +462,13 @@ fn opt_normalize_projection_type<'a, 'b, 'gcx, 'tcx>( selcx.infcx().report_overflow_error(&obligation, false); } Err(ProjectionCacheEntry::NormalizedTy(ty)) => { - // If we find the value in the cache, then return it along - // with the obligations that went along with it. Note - // that, when using a fulfillment context, these - // obligations could in principle be ignored: they have - // already been registered when the cache entry was - // created (and hence the new ones will quickly be - // discarded as duplicated). But when doing trait - // evaluation this is not the case, and dropping the trait - // evaluations can causes ICEs (e.g. #43132). + // If we find the value in the cache, then the obligations + // have already been returned from the previous entry (and + // should therefore have been honored). debug!("opt_normalize_projection_type: \ found normalized ty `{:?}`", ty); - return Some(ty); + return Some(NormalizedTy { value: ty, obligations: vec![] }); } Err(ProjectionCacheEntry::Error) => { debug!("opt_normalize_projection_type: \ @@ -1342,7 +1336,7 @@ enum ProjectionCacheEntry<'tcx> { InProgress, Ambiguous, Error, - NormalizedTy(NormalizedTy<'tcx>), + NormalizedTy(Ty<'tcx>), } // NB: intentionally not Clone @@ -1395,7 +1389,7 @@ impl<'tcx> ProjectionCache<'tcx> { let fresh_key = if cacheable { debug!("ProjectionCacheEntry::complete: adding cache entry: key={:?}, value={:?}", key, value); - self.map.insert(key, ProjectionCacheEntry::NormalizedTy(value.clone())) + self.map.insert(key, ProjectionCacheEntry::NormalizedTy(value.value)) } else { debug!("ProjectionCacheEntry::complete: cannot cache: key={:?}, value={:?}", key, value); diff --git a/src/test/run-pass/issue-43132.rs b/src/test/run-pass/issue-43132.rs deleted file mode 100644 index 64b3b092b8936..0000000000000 --- a/src/test/run-pass/issue-43132.rs +++ /dev/null @@ -1,74 +0,0 @@ -// Copyright 2017 The Rust Project Developers. See the COPYRIGHT -// file at the top-level directory of this distribution and at -// http://rust-lang.org/COPYRIGHT. -// -// Licensed under the Apache License, Version 2.0 or the MIT license -// , at your -// option. This file may not be copied, modified, or distributed -// except according to those terms. - -#![allow(unused)] - -fn main() { -} - -fn foo() { - let b = mk::< - Forward<(Box>,)>, - >(); - b.map_err(|_| ()).join(); -} - -fn mk() -> T { - loop {} -} - -impl, E> Future for (I,) { - type Error = E; -} - -struct Forward { - _a: T, -} - -impl Future for Forward -where - T::Error: From, -{ - type Error = T::Error; -} - -trait Future { - type Error; - - fn map_err(self, _: F) -> (Self, F) - where - F: FnOnce(Self::Error) -> E, - Self: Sized, - { - loop {} - } - - fn join(self) -> (MaybeDone, ()) - where - Self: Sized, - { - loop {} - } -} - -impl Future for Box { - type Error = S::Error; -} - -enum MaybeDone { - _Done(A::Error), -} - -impl Future for (A, F) -where - F: FnOnce(A::Error) -> U, -{ - type Error = U; -} From 80a0504994df15317f934634628b1a4d999bdc57 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Thu, 17 Aug 2017 22:03:27 +0300 Subject: [PATCH 2/3] Revert "Auto merge of #43184 - nikomatsakis:incr-comp-anonymize-trait-selection, r=michaelwoerister" This reverts commit b4502f7c0b51526d0177204a71dc2b3200f7348b, reversing changes made to 23ecebd6bd4362142ac586014aec44070a177a3d. --- src/librustc/dep_graph/dep_node.rs | 2 +- src/librustc/dep_graph/dep_tracking_map.rs | 40 +++++++--- src/librustc/traits/select.rs | 88 ++++++---------------- src/librustc/traits/trans/mod.rs | 39 ++++++++-- src/librustc/ty/mod.rs | 28 +++++++ src/librustc/util/common.rs | 6 +- 6 files changed, 120 insertions(+), 83 deletions(-) diff --git a/src/librustc/dep_graph/dep_node.rs b/src/librustc/dep_graph/dep_node.rs index 8e2c44a427b70..c6275e8464584 100644 --- a/src/librustc/dep_graph/dep_node.rs +++ b/src/librustc/dep_graph/dep_node.rs @@ -495,7 +495,7 @@ define_dep_nodes!( <'tcx> // imprecision in our dep-graph tracking. The important thing is // that for any given trait-ref, we always map to the **same** // trait-select node. - [anon] TraitSelect, + [] TraitSelect { trait_def_id: DefId, input_def_id: DefId }, // For proj. cache, we just keep a list of all def-ids, since it is // not a hotspot. diff --git a/src/librustc/dep_graph/dep_tracking_map.rs b/src/librustc/dep_graph/dep_tracking_map.rs index 2d19b34c5040e..ca53fd7a43311 100644 --- a/src/librustc/dep_graph/dep_tracking_map.rs +++ b/src/librustc/dep_graph/dep_tracking_map.rs @@ -12,9 +12,10 @@ use rustc_data_structures::fx::FxHashMap; use std::cell::RefCell; use std::hash::Hash; use std::marker::PhantomData; +use ty::TyCtxt; use util::common::MemoizationMap; -use super::{DepKind, DepNodeIndex, DepGraph}; +use super::{DepNode, DepGraph}; /// A DepTrackingMap offers a subset of the `Map` API and ensures that /// we make calls to `read` and `write` as appropriate. We key the @@ -22,13 +23,13 @@ use super::{DepKind, DepNodeIndex, DepGraph}; pub struct DepTrackingMap { phantom: PhantomData, graph: DepGraph, - map: FxHashMap, + map: FxHashMap, } pub trait DepTrackingMapConfig { type Key: Eq + Hash + Clone; type Value: Clone; - fn to_dep_kind() -> DepKind; + fn to_dep_node(tcx: TyCtxt, key: &Self::Key) -> DepNode; } impl DepTrackingMap { @@ -39,6 +40,27 @@ impl DepTrackingMap { map: FxHashMap(), } } + + /// Registers a (synthetic) read from the key `k`. Usually this + /// is invoked automatically by `get`. + fn read(&self, tcx: TyCtxt, k: &M::Key) { + let dep_node = M::to_dep_node(tcx, k); + self.graph.read(dep_node); + } + + pub fn get(&self, tcx: TyCtxt, k: &M::Key) -> Option<&M::Value> { + self.read(tcx, k); + self.map.get(k) + } + + pub fn contains_key(&self, tcx: TyCtxt, k: &M::Key) -> bool { + self.read(tcx, k); + self.map.contains_key(k) + } + + pub fn keys(&self) -> Vec { + self.map.keys().cloned().collect() + } } impl MemoizationMap for RefCell> { @@ -76,22 +98,22 @@ impl MemoizationMap for RefCell> { /// The key is the line marked `(*)`: the closure implicitly /// accesses the body of the item `item`, so we register a read /// from `Hir(item_def_id)`. - fn memoize(&self, key: M::Key, op: OP) -> M::Value + fn memoize(&self, tcx: TyCtxt, key: M::Key, op: OP) -> M::Value where OP: FnOnce() -> M::Value { let graph; { let this = self.borrow(); - if let Some(&(ref result, dep_node)) = this.map.get(&key) { - this.graph.read_index(dep_node); + if let Some(result) = this.map.get(&key) { + this.read(tcx, &key); return result.clone(); } graph = this.graph.clone(); } - let (result, dep_node) = graph.with_anon_task(M::to_dep_kind(), op); - self.borrow_mut().map.insert(key, (result.clone(), dep_node)); - graph.read_index(dep_node); + let _task = graph.in_task(M::to_dep_node(tcx, &key)); + let result = op(); + self.borrow_mut().map.insert(key, result.clone()); result } } diff --git a/src/librustc/traits/select.rs b/src/librustc/traits/select.rs index c690bebed8c00..79da04df1df08 100644 --- a/src/librustc/traits/select.rs +++ b/src/librustc/traits/select.rs @@ -30,7 +30,6 @@ use super::{VtableImplData, VtableObjectData, VtableBuiltinData, VtableClosureData, VtableDefaultImplData, VtableFnPointerData}; use super::util; -use dep_graph::{DepNodeIndex, DepKind}; use hir::def_id::DefId; use infer; use infer::{InferCtxt, InferOk, TypeFreshener}; @@ -106,7 +105,7 @@ struct TraitObligationStack<'prev, 'tcx: 'prev> { #[derive(Clone)] pub struct SelectionCache<'tcx> { hashmap: RefCell, - WithDepNode>>>>, + SelectionResult<'tcx, SelectionCandidate<'tcx>>>>, } /// The selection process begins by considering all impls, where @@ -370,7 +369,7 @@ impl EvaluationResult { #[derive(Clone)] pub struct EvaluationCache<'tcx> { - hashmap: RefCell, WithDepNode>> + hashmap: RefCell, EvaluationResult>> } impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { @@ -467,6 +466,8 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { assert!(!obligation.predicate.has_escaping_regions()); let tcx = self.tcx(); + let dep_node = obligation.predicate.dep_node(tcx); + let _task = tcx.dep_graph.in_task(dep_node); let stack = self.push_stack(TraitObligationStackList::empty(), obligation); let ret = match self.candidate_from_obligation(&stack)? { @@ -709,12 +710,12 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { return result; } - let (result, dep_node) = self.in_task(|this| this.evaluate_stack(&stack)); + let result = self.evaluate_stack(&stack); debug!("CACHE MISS: EVAL({:?})={:?}", fresh_trait_ref, result); - self.insert_evaluation_cache(obligation.param_env, fresh_trait_ref, dep_node, result); + self.insert_evaluation_cache(obligation.param_env, fresh_trait_ref, result); result } @@ -869,23 +870,22 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { trait_ref: ty::PolyTraitRef<'tcx>) -> Option { - let tcx = self.tcx(); if self.can_use_global_caches(param_env) { - let cache = tcx.evaluation_cache.hashmap.borrow(); + let cache = self.tcx().evaluation_cache.hashmap.borrow(); if let Some(cached) = cache.get(&trait_ref) { - return Some(cached.get(tcx)); + let dep_node = trait_ref + .to_poly_trait_predicate() + .dep_node(self.tcx()); + self.tcx().hir.dep_graph.read(dep_node); + return Some(cached.clone()); } } - self.infcx.evaluation_cache.hashmap - .borrow() - .get(&trait_ref) - .map(|v| v.get(tcx)) + self.infcx.evaluation_cache.hashmap.borrow().get(&trait_ref).cloned() } fn insert_evaluation_cache(&mut self, param_env: ty::ParamEnv<'tcx>, trait_ref: ty::PolyTraitRef<'tcx>, - dep_node: DepNodeIndex, result: EvaluationResult) { // Avoid caching results that depend on more than just the trait-ref: @@ -902,14 +902,12 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { if self.can_use_global_caches(param_env) { let mut cache = self.tcx().evaluation_cache.hashmap.borrow_mut(); if let Some(trait_ref) = self.tcx().lift_to_global(&trait_ref) { - cache.insert(trait_ref, WithDepNode::new(dep_node, result)); + cache.insert(trait_ref, result); return; } } - self.infcx.evaluation_cache.hashmap - .borrow_mut() - .insert(trait_ref, WithDepNode::new(dep_node, result)); + self.infcx.evaluation_cache.hashmap.borrow_mut().insert(trait_ref, result); } /////////////////////////////////////////////////////////////////////////// @@ -951,32 +949,19 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { } // If no match, compute result and insert into cache. - let (candidate, dep_node) = self.in_task(|this| { - this.candidate_from_obligation_no_cache(stack) - }); + let candidate = self.candidate_from_obligation_no_cache(stack); if self.should_update_candidate_cache(&cache_fresh_trait_pred, &candidate) { debug!("CACHE MISS: SELECT({:?})={:?}", cache_fresh_trait_pred, candidate); self.insert_candidate_cache(stack.obligation.param_env, cache_fresh_trait_pred, - dep_node, candidate.clone()); } candidate } - fn in_task(&mut self, op: OP) -> (R, DepNodeIndex) - where OP: FnOnce(&mut Self) -> R - { - let (result, dep_node) = self.tcx().dep_graph.with_anon_task(DepKind::TraitSelect, || { - op(self) - }); - self.tcx().dep_graph.read_index(dep_node); - (result, dep_node) - } - // Treat negative impls as unimplemented fn filter_negative_impls(&self, candidate: SelectionCandidate<'tcx>) -> SelectionResult<'tcx, SelectionCandidate<'tcx>> { @@ -1166,41 +1151,33 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { cache_fresh_trait_pred: &ty::PolyTraitPredicate<'tcx>) -> Option>> { - let tcx = self.tcx(); let trait_ref = &cache_fresh_trait_pred.0.trait_ref; if self.can_use_global_caches(param_env) { - let cache = tcx.selection_cache.hashmap.borrow(); + let cache = self.tcx().selection_cache.hashmap.borrow(); if let Some(cached) = cache.get(&trait_ref) { - return Some(cached.get(tcx)); + return Some(cached.clone()); } } - self.infcx.selection_cache.hashmap - .borrow() - .get(trait_ref) - .map(|v| v.get(tcx)) + self.infcx.selection_cache.hashmap.borrow().get(trait_ref).cloned() } fn insert_candidate_cache(&mut self, param_env: ty::ParamEnv<'tcx>, cache_fresh_trait_pred: ty::PolyTraitPredicate<'tcx>, - dep_node: DepNodeIndex, candidate: SelectionResult<'tcx, SelectionCandidate<'tcx>>) { - let tcx = self.tcx(); let trait_ref = cache_fresh_trait_pred.0.trait_ref; if self.can_use_global_caches(param_env) { - let mut cache = tcx.selection_cache.hashmap.borrow_mut(); - if let Some(trait_ref) = tcx.lift_to_global(&trait_ref) { - if let Some(candidate) = tcx.lift_to_global(&candidate) { - cache.insert(trait_ref, WithDepNode::new(dep_node, candidate)); + let mut cache = self.tcx().selection_cache.hashmap.borrow_mut(); + if let Some(trait_ref) = self.tcx().lift_to_global(&trait_ref) { + if let Some(candidate) = self.tcx().lift_to_global(&candidate) { + cache.insert(trait_ref, candidate); return; } } } - self.infcx.selection_cache.hashmap - .borrow_mut() - .insert(trait_ref, WithDepNode::new(dep_node, candidate)); + self.infcx.selection_cache.hashmap.borrow_mut().insert(trait_ref, candidate); } fn should_update_candidate_cache(&mut self, @@ -3161,20 +3138,3 @@ impl<'o,'tcx> fmt::Debug for TraitObligationStack<'o,'tcx> { write!(f, "TraitObligationStack({:?})", self.obligation) } } - -#[derive(Clone)] -pub struct WithDepNode { - dep_node: DepNodeIndex, - cached_value: T -} - -impl WithDepNode { - pub fn new(dep_node: DepNodeIndex, cached_value: T) -> Self { - WithDepNode { dep_node, cached_value } - } - - pub fn get(&self, tcx: TyCtxt) -> T { - tcx.dep_graph.read_index(self.dep_node); - self.cached_value.clone() - } -} diff --git a/src/librustc/traits/trans/mod.rs b/src/librustc/traits/trans/mod.rs index 827a5092c0042..9c6047b28b5b0 100644 --- a/src/librustc/traits/trans/mod.rs +++ b/src/librustc/traits/trans/mod.rs @@ -13,7 +13,9 @@ // seems likely that they should eventually be merged into more // general routines. -use dep_graph::{DepGraph, DepKind, DepTrackingMap, DepTrackingMapConfig}; +use dep_graph::{DepGraph, DepNode, DepTrackingMap, DepTrackingMapConfig, + DepConstructor}; +use hir::def_id::DefId; use infer::TransNormalize; use std::cell::RefCell; use std::marker::PhantomData; @@ -39,7 +41,7 @@ impl<'a, 'tcx> TyCtxt<'a, 'tcx, 'tcx> { // Remove any references to regions; this helps improve caching. let trait_ref = self.erase_regions(&trait_ref); - self.trans_trait_caches.trait_cache.memoize(trait_ref, || { + self.trans_trait_caches.trait_cache.memoize(self, trait_ref, || { debug!("trans::fulfill_obligation(trait_ref={:?}, def_id={:?})", trait_ref, trait_ref.def_id()); @@ -137,7 +139,7 @@ impl<'a, 'gcx> TypeFolder<'gcx, 'gcx> for AssociatedTypeNormalizer<'a, 'gcx> { if !ty.has_projection_types() { ty } else { - self.tcx.trans_trait_caches.project_cache.memoize(ty, || { + self.tcx.trans_trait_caches.project_cache.memoize(self.tcx, ty, || { debug!("AssociatedTypeNormalizer: ty={:?}", ty); self.tcx.normalize_associated_type(&ty) }) @@ -169,8 +171,8 @@ pub struct TraitSelectionCache<'tcx> { impl<'tcx> DepTrackingMapConfig for TraitSelectionCache<'tcx> { type Key = ty::PolyTraitRef<'tcx>; type Value = Vtable<'tcx, ()>; - fn to_dep_kind() -> DepKind { - DepKind::TraitSelect + fn to_dep_node(tcx: TyCtxt, key: &ty::PolyTraitRef<'tcx>) -> DepNode { + key.to_poly_trait_predicate().dep_node(tcx) } } @@ -183,8 +185,31 @@ pub struct ProjectionCache<'gcx> { impl<'gcx> DepTrackingMapConfig for ProjectionCache<'gcx> { type Key = Ty<'gcx>; type Value = Ty<'gcx>; - fn to_dep_kind() -> DepKind { - DepKind::TraitSelect + fn to_dep_node(tcx: TyCtxt, key: &Self::Key) -> DepNode { + // Ideally, we'd just put `key` into the dep-node, but we + // can't put full types in there. So just collect up all the + // def-ids of structs/enums as well as any traits that we + // project out of. It doesn't matter so much what we do here, + // except that if we are too coarse, we'll create overly + // coarse edges between impls and the trans. For example, if + // we just used the def-id of things we are projecting out of, + // then the key for `::T` and `::T` would both share a dep-node + // (`TraitSelect(SomeTrait)`), and hence the impls for both + // `Foo` and `Bar` would be considered inputs. So a change to + // `Bar` would affect things that just normalized `Foo`. + // Anyway, this heuristic is not ideal, but better than + // nothing. + let def_ids: Vec = + key.walk() + .filter_map(|t| match t.sty { + ty::TyAdt(adt_def, _) => Some(adt_def.did), + ty::TyProjection(ref proj) => Some(proj.item_def_id), + _ => None, + }) + .collect(); + + DepNode::new(tcx, DepConstructor::ProjectionCache { def_ids: def_ids }) } } diff --git a/src/librustc/ty/mod.rs b/src/librustc/ty/mod.rs index 5aaba526e265f..3f1302e72e974 100644 --- a/src/librustc/ty/mod.rs +++ b/src/librustc/ty/mod.rs @@ -15,6 +15,7 @@ pub use self::IntVarValue::*; pub use self::LvaluePreference::*; pub use self::fold::TypeFoldable; +use dep_graph::{DepNode, DepConstructor}; use hir::{map as hir_map, FreevarMap, TraitMap}; use hir::def::{Def, CtorKind, ExportMap}; use hir::def_id::{CrateNum, DefId, DefIndex, CRATE_DEF_INDEX, LOCAL_CRATE}; @@ -946,6 +947,28 @@ impl<'tcx> TraitPredicate<'tcx> { self.trait_ref.def_id } + /// Creates the dep-node for selecting/evaluating this trait reference. + fn dep_node(&self, tcx: TyCtxt) -> DepNode { + // Extact the trait-def and first def-id from inputs. See the + // docs for `DepNode::TraitSelect` for more information. + let trait_def_id = self.def_id(); + let input_def_id = + self.input_types() + .flat_map(|t| t.walk()) + .filter_map(|t| match t.sty { + ty::TyAdt(adt_def, ..) => Some(adt_def.did), + ty::TyClosure(def_id, ..) => Some(def_id), + ty::TyFnDef(def_id, ..) => Some(def_id), + _ => None + }) + .next() + .unwrap_or(trait_def_id); + DepNode::new(tcx, DepConstructor::TraitSelect { + trait_def_id, + input_def_id, + }) + } + pub fn input_types<'a>(&'a self) -> impl DoubleEndedIterator> + 'a { self.trait_ref.input_types() } @@ -960,6 +983,11 @@ impl<'tcx> PolyTraitPredicate<'tcx> { // ok to skip binder since trait def-id does not care about regions self.0.def_id() } + + pub fn dep_node(&self, tcx: TyCtxt) -> DepNode { + // ok to skip binder since depnode does not care about regions + self.0.dep_node(tcx) + } } #[derive(Clone, Copy, PartialEq, Eq, Hash, Debug, RustcEncodable, RustcDecodable)] diff --git a/src/librustc/util/common.rs b/src/librustc/util/common.rs index 17564671a1e36..40ee3cd28f562 100644 --- a/src/librustc/util/common.rs +++ b/src/librustc/util/common.rs @@ -19,6 +19,8 @@ use std::iter::repeat; use std::path::Path; use std::time::{Duration, Instant}; +use ty::TyCtxt; + // The name of the associated type for `Fn` return types pub const FN_OUTPUT_NAME: &'static str = "Output"; @@ -209,7 +211,7 @@ pub trait MemoizationMap { /// needed in the `op` to ensure that the correct edges are /// added into the dep graph. See the `DepTrackingMap` impl for /// more details! - fn memoize(&self, key: Self::Key, op: OP) -> Self::Value + fn memoize(&self, tcx: TyCtxt, key: Self::Key, op: OP) -> Self::Value where OP: FnOnce() -> Self::Value; } @@ -219,7 +221,7 @@ impl MemoizationMap for RefCell> type Key = K; type Value = V; - fn memoize(&self, key: K, op: OP) -> V + fn memoize(&self, _tcx: TyCtxt, key: K, op: OP) -> V where OP: FnOnce() -> V { let result = self.borrow().get(&key).cloned(); From 97e9c7ec483b0ac53c4c68415c9f272d74c771cf Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Thu, 17 Aug 2017 22:03:39 +0300 Subject: [PATCH 3/3] Revert "Auto merge of #42840 - arielb1:poison-smoke-and-mirrors, r=nikomatsakis" This reverts commit 9b85e1cfa5aa2aaa4b5df4359a023ad793983ffc, reversing changes made to 13157c4ebcca735a0842bd03c3dad1de7c429f9f. --- src/librustc/traits/fulfill.rs | 131 ++++++++++++++--- src/librustc/traits/mod.rs | 2 +- src/librustc/traits/select.rs | 207 +++++++-------------------- src/librustc/ty/context.rs | 8 ++ src/test/compile-fail/issue-39970.rs | 31 ---- src/test/compile-fail/issue-42796.rs | 29 ---- 6 files changed, 169 insertions(+), 239 deletions(-) delete mode 100644 src/test/compile-fail/issue-39970.rs delete mode 100644 src/test/compile-fail/issue-42796.rs diff --git a/src/librustc/traits/fulfill.rs b/src/librustc/traits/fulfill.rs index 4f1eb6169209b..fab89918376b0 100644 --- a/src/librustc/traits/fulfill.rs +++ b/src/librustc/traits/fulfill.rs @@ -8,14 +8,15 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +use dep_graph::DepGraph; use infer::{InferCtxt, InferOk}; -use ty::{self, Ty, TypeFoldable, ToPolyTraitRef, ToPredicate}; +use ty::{self, Ty, TypeFoldable, ToPolyTraitRef, TyCtxt, ToPredicate}; use ty::error::ExpectedFound; use rustc_data_structures::obligation_forest::{ObligationForest, Error}; use rustc_data_structures::obligation_forest::{ForestObligation, ObligationProcessor}; use std::marker::PhantomData; use syntax::ast; -use util::nodemap::NodeMap; +use util::nodemap::{FxHashSet, NodeMap}; use hir::def_id::DefId; use super::CodeAmbiguity; @@ -33,6 +34,11 @@ impl<'tcx> ForestObligation for PendingPredicateObligation<'tcx> { fn as_predicate(&self) -> &Self::Predicate { &self.obligation.predicate } } +pub struct GlobalFulfilledPredicates<'tcx> { + set: FxHashSet>, + dep_graph: DepGraph, +} + /// The fulfillment context is used to drive trait resolution. It /// consists of a list of obligations that must be (eventually) /// satisfied. The job is to track which are satisfied, which yielded @@ -177,6 +183,13 @@ impl<'a, 'gcx, 'tcx> FulfillmentContext<'tcx> { assert!(!infcx.is_in_snapshot()); + let tcx = infcx.tcx; + + if tcx.fulfilled_predicates.borrow().check_duplicate(tcx, &obligation.predicate) { + debug!("register_predicate_obligation: duplicate"); + return + } + self.predicates.register_obligation(PendingPredicateObligation { obligation, stalled_on: vec![] @@ -251,6 +264,13 @@ impl<'a, 'gcx, 'tcx> FulfillmentContext<'tcx> { }); debug!("select: outcome={:?}", outcome); + // these are obligations that were proven to be true. + for pending_obligation in outcome.completed { + let predicate = &pending_obligation.obligation.predicate; + selcx.tcx().fulfilled_predicates.borrow_mut() + .add_if_global(selcx.tcx(), predicate); + } + errors.extend( outcome.errors.into_iter() .map(|e| to_fulfillment_error(e))); @@ -298,7 +318,7 @@ impl<'a, 'b, 'gcx, 'tcx> ObligationProcessor for FulfillProcessor<'a, 'b, 'gcx, _marker: PhantomData<&'c PendingPredicateObligation<'tcx>>) where I: Clone + Iterator>, { - if self.selcx.coinductive_match(cycle.clone().map(|s| s.obligation.predicate)) { + if coinductive_match(self.selcx, cycle.clone()) { debug!("process_child_obligations: coinductive match"); } else { let cycle : Vec<_> = cycle.map(|c| c.obligation.clone()).collect(); @@ -355,31 +375,21 @@ fn process_predicate<'a, 'gcx, 'tcx>( match obligation.predicate { ty::Predicate::Trait(ref data) => { - let trait_obligation = obligation.with(data.clone()); - - if data.is_global() { - // no type variables present, can use evaluation for better caching. - // FIXME: consider caching errors too. - if - // make defaulted unit go through the slow path for better warnings, - // please remove this when the warnings are removed. - !trait_obligation.predicate.skip_binder().self_ty().is_defaulted_unit() && - selcx.evaluate_obligation_conservatively(&obligation) { - debug!("selecting trait `{:?}` at depth {} evaluated to holds", - data, obligation.recursion_depth); - return Ok(Some(vec![])) - } + let tcx = selcx.tcx(); + if tcx.fulfilled_predicates.borrow().check_duplicate_trait(tcx, data) { + return Ok(Some(vec![])); } + let trait_obligation = obligation.with(data.clone()); match selcx.select(&trait_obligation) { Ok(Some(vtable)) => { debug!("selecting trait `{:?}` at depth {} yielded Ok(Some)", - data, obligation.recursion_depth); + data, obligation.recursion_depth); Ok(Some(vtable.nested_obligations())) } Ok(None) => { debug!("selecting trait `{:?}` at depth {} yielded Ok(None)", - data, obligation.recursion_depth); + data, obligation.recursion_depth); // This is a bit subtle: for the most part, the // only reason we can fail to make progress on @@ -540,6 +550,40 @@ fn process_predicate<'a, 'gcx, 'tcx>( } } +/// For defaulted traits, we use a co-inductive strategy to solve, so +/// that recursion is ok. This routine returns true if the top of the +/// stack (`cycle[0]`): +/// - is a defaulted trait, and +/// - it also appears in the backtrace at some position `X`; and, +/// - all the predicates at positions `X..` between `X` an the top are +/// also defaulted traits. +fn coinductive_match<'a,'c,'gcx,'tcx,I>(selcx: &mut SelectionContext<'a,'gcx,'tcx>, + cycle: I) -> bool + where I: Iterator>, + 'tcx: 'c +{ + let mut cycle = cycle; + cycle + .all(|bt_obligation| { + let result = coinductive_obligation(selcx, &bt_obligation.obligation); + debug!("coinductive_match: bt_obligation={:?} coinductive={}", + bt_obligation, result); + result + }) +} + +fn coinductive_obligation<'a,'gcx,'tcx>(selcx: &SelectionContext<'a,'gcx,'tcx>, + obligation: &PredicateObligation<'tcx>) + -> bool { + match obligation.predicate { + ty::Predicate::Trait(ref data) => { + selcx.tcx().trait_has_default_impl(data.def_id()) + } + _ => { + false + } + } +} fn register_region_obligation<'tcx>(t_a: Ty<'tcx>, r_b: ty::Region<'tcx>, @@ -559,6 +603,55 @@ fn register_region_obligation<'tcx>(t_a: Ty<'tcx>, } +impl<'a, 'gcx, 'tcx> GlobalFulfilledPredicates<'gcx> { + pub fn new(dep_graph: DepGraph) -> GlobalFulfilledPredicates<'gcx> { + GlobalFulfilledPredicates { + set: FxHashSet(), + dep_graph, + } + } + + pub fn check_duplicate(&self, tcx: TyCtxt, key: &ty::Predicate<'tcx>) -> bool { + if let ty::Predicate::Trait(ref data) = *key { + self.check_duplicate_trait(tcx, data) + } else { + false + } + } + + pub fn check_duplicate_trait(&self, tcx: TyCtxt, data: &ty::PolyTraitPredicate<'tcx>) -> bool { + // For the global predicate registry, when we find a match, it + // may have been computed by some other task, so we want to + // add a read from the node corresponding to the predicate + // processing to make sure we get the transitive dependencies. + if self.set.contains(data) { + debug_assert!(data.is_global()); + self.dep_graph.read(data.dep_node(tcx)); + debug!("check_duplicate: global predicate `{:?}` already proved elsewhere", data); + + true + } else { + false + } + } + + fn add_if_global(&mut self, tcx: TyCtxt<'a, 'gcx, 'tcx>, key: &ty::Predicate<'tcx>) { + if let ty::Predicate::Trait(ref data) = *key { + // We only add things to the global predicate registry + // after the current task has proved them, and hence + // already has the required read edges, so we don't need + // to add any more edges here. + if data.is_global() { + if let Some(data) = tcx.lift_to_global(data) { + if self.set.insert(data.clone()) { + debug!("add_if_global: global predicate `{:?}` added", data); + } + } + } + } + } +} + fn to_fulfillment_error<'tcx>( error: Error, FulfillmentErrorCode<'tcx>>) -> FulfillmentError<'tcx> diff --git a/src/librustc/traits/mod.rs b/src/librustc/traits/mod.rs index e14203b34a180..6b243ffa5feb5 100644 --- a/src/librustc/traits/mod.rs +++ b/src/librustc/traits/mod.rs @@ -31,7 +31,7 @@ use syntax_pos::{Span, DUMMY_SP}; pub use self::coherence::orphan_check; pub use self::coherence::overlapping_impls; pub use self::coherence::OrphanCheckErr; -pub use self::fulfill::{FulfillmentContext, RegionObligation}; +pub use self::fulfill::{FulfillmentContext, GlobalFulfilledPredicates, RegionObligation}; pub use self::project::MismatchedProjectionTypes; pub use self::project::{normalize, normalize_projection_type, Normalized}; pub use self::project::{ProjectionCache, ProjectionCacheSnapshot, Reveal}; diff --git a/src/librustc/traits/select.rs b/src/librustc/traits/select.rs index 79da04df1df08..884f6bdb70b5c 100644 --- a/src/librustc/traits/select.rs +++ b/src/librustc/traits/select.rs @@ -43,7 +43,6 @@ use middle::lang_items; use rustc_data_structures::bitvec::BitVector; use rustc_data_structures::snapshot_vec::{SnapshotVecDelegate, SnapshotVec}; use std::cell::RefCell; -use std::cmp; use std::fmt; use std::marker::PhantomData; use std::mem; @@ -272,101 +271,17 @@ enum BuiltinImplConditions<'tcx> { /// The result of trait evaluation. The order is important /// here as the evaluation of a list is the maximum of the /// evaluations. -/// -/// The evaluation results are ordered: -/// - `EvaluatedToOk` implies `EvaluatedToAmbig` implies `EvaluatedToUnknown` -/// - `EvaluatedToErr` implies `EvaluatedToRecur` -/// - the "union" of evaluation results is equal to their maximum - -/// all the "potential success" candidates can potentially succeed, -/// so they are no-ops when unioned with a definite error, and within -/// the categories it's easy to see that the unions are correct. enum EvaluationResult { /// Evaluation successful EvaluatedToOk, - /// Evaluation is known to be ambiguous - it *might* hold for some - /// assignment of inference variables, but it might not. - /// - /// While this has the same meaning as `EvaluatedToUnknown` - we can't - /// know whether this obligation holds or not - it is the result we - /// would get with an empty stack, and therefore is cacheable. - EvaluatedToAmbig, - /// Evaluation failed because of recursion involving inference - /// variables. We are somewhat imprecise there, so we don't actually - /// know the real result. - /// - /// This can't be trivially cached for the same reason as `EvaluatedToRecur`. + /// Evaluation failed because of recursion - treated as ambiguous EvaluatedToUnknown, - /// Evaluation failed because we encountered an obligation we are already - /// trying to prove on this branch. - /// - /// We know this branch can't be a part of a minimal proof-tree for - /// the "root" of our cycle, because then we could cut out the recursion - /// and maintain a valid proof tree. However, this does not mean - /// that all the obligations on this branch do not hold - it's possible - /// that we entered this branch "speculatively", and that there - /// might be some other way to prove this obligation that does not - /// go through this cycle - so we can't cache this as a failure. - /// - /// For example, suppose we have this: - /// - /// ```rust,ignore (pseudo-Rust) - /// pub trait Trait { fn xyz(); } - /// // This impl is "useless", but we can still have - /// // an `impl Trait for SomeUnsizedType` somewhere. - /// impl Trait for T { fn xyz() {} } - /// - /// pub fn foo() { - /// ::xyz(); - /// } - /// ``` - /// - /// When checking `foo`, we have to prove `T: Trait`. This basically - /// translates into this: - /// - /// (T: Trait + Sized →_\impl T: Trait), T: Trait ⊢ T: Trait - /// - /// When we try to prove it, we first go the first option, which - /// recurses. This shows us that the impl is "useless" - it won't - /// tell us that `T: Trait` unless it already implemented `Trait` - /// by some other means. However, that does not prevent `T: Trait` - /// does not hold, because of the bound (which can indeed be satisfied - /// by `SomeUnsizedType` from another crate). - /// - /// FIXME: when an `EvaluatedToRecur` goes past its parent root, we - /// ought to convert it to an `EvaluatedToErr`, because we know - /// there definitely isn't a proof tree for that obligation. Not - /// doing so is still sound - there isn't any proof tree, so the - /// branch still can't be a part of a minimal one - but does not - /// re-enable caching. - EvaluatedToRecur, + /// Evaluation is known to be ambiguous + EvaluatedToAmbig, /// Evaluation failed EvaluatedToErr, } -impl EvaluationResult { - fn may_apply(self) -> bool { - match self { - EvaluatedToOk | - EvaluatedToAmbig | - EvaluatedToUnknown => true, - - EvaluatedToErr | - EvaluatedToRecur => false - } - } - - fn is_stack_dependent(self) -> bool { - match self { - EvaluatedToUnknown | - EvaluatedToRecur => true, - - EvaluatedToOk | - EvaluatedToAmbig | - EvaluatedToErr => false, - } - } -} - #[derive(Clone)] pub struct EvaluationCache<'tcx> { hashmap: RefCell, EvaluationResult>> @@ -577,12 +492,15 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { let eval = self.evaluate_predicate_recursively(stack, obligation); debug!("evaluate_predicate_recursively({:?}) = {:?}", obligation, eval); - if let EvaluatedToErr = eval { - // fast-path - EvaluatedToErr is the top of the lattice, - // so we don't need to look on the other predicates. - return EvaluatedToErr; - } else { - result = cmp::max(result, eval); + match eval { + EvaluatedToErr => { return EvaluatedToErr; } + EvaluatedToAmbig => { result = EvaluatedToAmbig; } + EvaluatedToUnknown => { + if result < EvaluatedToUnknown { + result = EvaluatedToUnknown; + } + } + EvaluatedToOk => { } } } result @@ -596,11 +514,21 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { debug!("evaluate_predicate_recursively({:?})", obligation); + let tcx = self.tcx(); + + // Check the cache from the tcx of predicates that we know + // have been proven elsewhere. This cache only contains + // predicates that are global in scope and hence unaffected by + // the current environment. + if tcx.fulfilled_predicates.borrow().check_duplicate(tcx, &obligation.predicate) { + return EvaluatedToOk; + } + match obligation.predicate { ty::Predicate::Trait(ref t) => { assert!(!t.has_escaping_regions()); let obligation = obligation.with(t.clone()); - self.evaluate_trait_predicate_recursively(previous_stack, obligation) + self.evaluate_obligation_recursively(previous_stack, &obligation) } ty::Predicate::Equate(ref p) => { @@ -685,23 +613,15 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { } } - fn evaluate_trait_predicate_recursively<'o>(&mut self, - previous_stack: TraitObligationStackList<'o, 'tcx>, - mut obligation: TraitObligation<'tcx>) - -> EvaluationResult + fn evaluate_obligation_recursively<'o>(&mut self, + previous_stack: TraitObligationStackList<'o, 'tcx>, + obligation: &TraitObligation<'tcx>) + -> EvaluationResult { - debug!("evaluate_trait_predicate_recursively({:?})", + debug!("evaluate_obligation_recursively({:?})", obligation); - if !self.intercrate && obligation.is_global() { - // If a param env is consistent, global obligations do not depend on its particular - // value in order to work, so we can clear out the param env and get better - // caching. (If the current param env is inconsistent, we don't care what happens). - debug!("evaluate_trait_predicate_recursively({:?}) - in global", obligation); - obligation.param_env = ty::ParamEnv::empty(obligation.param_env.reveal); - } - - let stack = self.push_stack(previous_stack, &obligation); + let stack = self.push_stack(previous_stack, obligation); let fresh_trait_ref = stack.fresh_trait_ref; if let Some(result) = self.check_evaluation_cache(obligation.param_env, fresh_trait_ref) { debug!("CACHE HIT: EVAL({:?})={:?}", @@ -756,9 +676,8 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { } if unbound_input_types && stack.iter().skip(1).any( - |prev| stack.obligation.param_env == prev.obligation.param_env && - self.match_fresh_trait_refs(&stack.fresh_trait_ref, - &prev.fresh_trait_ref)) + |prev| self.match_fresh_trait_refs(&stack.fresh_trait_ref, + &prev.fresh_trait_ref)) { debug!("evaluate_stack({:?}) --> unbound argument, recursive --> giving up", stack.fresh_trait_ref); @@ -784,25 +703,14 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { // affect the inferencer state and (b) that if we see two // skolemized types with the same index, they refer to the // same unbound type variable. - if let Some(rec_index) = + if stack.iter() .skip(1) // skip top-most frame - .position(|prev| stack.obligation.param_env == prev.obligation.param_env && - stack.fresh_trait_ref == prev.fresh_trait_ref) + .any(|prev| stack.fresh_trait_ref == prev.fresh_trait_ref) { debug!("evaluate_stack({:?}) --> recursive", stack.fresh_trait_ref); - let cycle = stack.iter().skip(1).take(rec_index+1); - let cycle = cycle.map(|stack| ty::Predicate::Trait(stack.obligation.predicate)); - if self.coinductive_match(cycle) { - debug!("evaluate_stack({:?}) --> recursive, coinductive", - stack.fresh_trait_ref); - return EvaluatedToOk; - } else { - debug!("evaluate_stack({:?}) --> recursive, inductive", - stack.fresh_trait_ref); - return EvaluatedToRecur; - } + return EvaluatedToOk; } match self.candidate_from_obligation(stack) { @@ -812,33 +720,6 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { } } - /// For defaulted traits, we use a co-inductive strategy to solve, so - /// that recursion is ok. This routine returns true if the top of the - /// stack (`cycle[0]`): - /// - is a defaulted trait, and - /// - it also appears in the backtrace at some position `X`; and, - /// - all the predicates at positions `X..` between `X` an the top are - /// also defaulted traits. - pub fn coinductive_match(&mut self, cycle: I) -> bool - where I: Iterator> - { - let mut cycle = cycle; - cycle.all(|predicate| self.coinductive_predicate(predicate)) - } - - fn coinductive_predicate(&self, predicate: ty::Predicate<'tcx>) -> bool { - let result = match predicate { - ty::Predicate::Trait(ref data) => { - self.tcx().trait_has_default_impl(data.def_id()) - } - _ => { - false - } - }; - debug!("coinductive_predicate({:?}) = {:?}", predicate, result); - result - } - /// Further evaluate `candidate` to decide whether all type parameters match and whether nested /// obligations are met. Returns true if `candidate` remains viable after this further /// scrutiny. @@ -873,10 +754,6 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { if self.can_use_global_caches(param_env) { let cache = self.tcx().evaluation_cache.hashmap.borrow(); if let Some(cached) = cache.get(&trait_ref) { - let dep_node = trait_ref - .to_poly_trait_predicate() - .dep_node(self.tcx()); - self.tcx().hir.dep_graph.read(dep_node); return Some(cached.clone()); } } @@ -889,10 +766,10 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { result: EvaluationResult) { // Avoid caching results that depend on more than just the trait-ref: - // The stack can create recursion, and closure signatures + // The stack can create EvaluatedToUnknown, and closure signatures // being yet uninferred can create "spurious" EvaluatedToAmbig // and EvaluatedToOk. - if result.is_stack_dependent() || + if result == EvaluatedToUnknown || ((result == EvaluatedToAmbig || result == EvaluatedToOk) && trait_ref.has_closure_types()) { @@ -3138,3 +3015,15 @@ impl<'o,'tcx> fmt::Debug for TraitObligationStack<'o,'tcx> { write!(f, "TraitObligationStack({:?})", self.obligation) } } + +impl EvaluationResult { + fn may_apply(&self) -> bool { + match *self { + EvaluatedToOk | + EvaluatedToAmbig | + EvaluatedToUnknown => true, + + EvaluatedToErr => false + } + } +} diff --git a/src/librustc/ty/context.rs b/src/librustc/ty/context.rs index c234f43e38779..48d6bdf95b55b 100644 --- a/src/librustc/ty/context.rs +++ b/src/librustc/ty/context.rs @@ -507,6 +507,12 @@ pub struct GlobalCtxt<'tcx> { /// Merge this with `selection_cache`? pub evaluation_cache: traits::EvaluationCache<'tcx>, + /// A set of predicates that have been fulfilled *somewhere*. + /// This is used to avoid duplicate work. Predicates are only + /// added to this set when they mention only "global" names + /// (i.e., no type or lifetime parameters). + pub fulfilled_predicates: RefCell>, + /// Maps Expr NodeId's to `true` iff `&expr` can have 'static lifetime. pub rvalue_promotable_to_static: RefCell>, @@ -680,6 +686,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { let interners = CtxtInterners::new(arena); let common_types = CommonTypes::new(&interners); let dep_graph = hir.dep_graph.clone(); + let fulfilled_predicates = traits::GlobalFulfilledPredicates::new(dep_graph.clone()); let max_cnum = s.cstore.crates().iter().map(|c| c.as_usize()).max().unwrap_or(0); let mut providers = IndexVec::from_elem_n(extern_providers, max_cnum + 1); providers[LOCAL_CRATE] = local_providers; @@ -728,6 +735,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { named_region_map, trait_map: resolutions.trait_map, export_map: resolutions.export_map, + fulfilled_predicates: RefCell::new(fulfilled_predicates), hir, def_path_hash_to_def_id, maps: maps::Maps::new(providers), diff --git a/src/test/compile-fail/issue-39970.rs b/src/test/compile-fail/issue-39970.rs deleted file mode 100644 index 65ea1baa4a126..0000000000000 --- a/src/test/compile-fail/issue-39970.rs +++ /dev/null @@ -1,31 +0,0 @@ -// Copyright 2017 The Rust Project Developers. See the COPYRIGHT -// file at the top-level directory of this distribution and at -// http://rust-lang.org/COPYRIGHT. -// -// Licensed under the Apache License, Version 2.0 or the MIT license -// , at your -// option. This file may not be copied, modified, or distributed -// except according to those terms. - -trait Array<'a> { - type Element: 'a; -} - -trait Visit { - fn visit() {} -} - -impl<'a> Array<'a> for () { - type Element = &'a (); -} - -impl Visit for () where - //(): for<'a> Array<'a, Element=&'a ()>, // No ICE - (): for<'a> Array<'a, Element=()>, // ICE -{} - -fn main() { - <() as Visit>::visit(); - //~^ ERROR type mismatch resolving `for<'a> <() as Array<'a>>::Element == ()` -} diff --git a/src/test/compile-fail/issue-42796.rs b/src/test/compile-fail/issue-42796.rs deleted file mode 100644 index 10622eccbdcd1..0000000000000 --- a/src/test/compile-fail/issue-42796.rs +++ /dev/null @@ -1,29 +0,0 @@ -// Copyright 2017 The Rust Project Developers. See the COPYRIGHT -// file at the top-level directory of this distribution and at -// http://rust-lang.org/COPYRIGHT. -// -// Licensed under the Apache License, Version 2.0 or the MIT license -// , at your -// option. This file may not be copied, modified, or distributed -// except according to those terms. - -pub trait Mirror { - type Image; -} - -impl Mirror for T { - type Image = T; -} - -pub fn poison(victim: String) where >::Image: Copy { - loop { drop(victim); } //~ ERROR use of moved value -} - -fn main() { - let s = "Hello!".to_owned(); - let mut s_copy = s; - s_copy.push_str("World!"); - "0wned!".to_owned(); - println!("{}", s); //~ ERROR use of moved value -}