Skip to content

Commit ff89fcf

Browse files
committed
Add a (somewhat hacky) cache to the tcx that tracks "global" trait refs
that are known to have been satisfied *somewhere*. This means that if one fn finds that `SomeType: Foo`, then every other fn can just consider that to hold. Unfortunately, there are some complications: 1. If `SomeType: Foo` includes dependent conditions, those conditions may trigger an error. This error will be repored in the first fn where `SomeType: Foo` is evaluated, but not in the other fns, which can lead to uneven error reporting (which is sometimes confusing). 2. This kind of caching can be unsound in the presence of unsatisfiable where clauses. For example, suppose that the first fn has a where-clause like `i32: Bar<u32>`, which in fact does not hold. This will "fool" trait resolution into thinking that `i32: Bar<u32>` holds. This is ok currently, because it means that the first fn can never be calle (since its where clauses cannot be satisfied), but if the first fn's successful resolution is cached, it can allow other fns to compile that should not. This problem is fixed in the next commit.
1 parent 0d82fb5 commit ff89fcf

File tree

10 files changed

+142
-17
lines changed

10 files changed

+142
-17
lines changed

src/librustc/middle/check_const.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -285,7 +285,7 @@ impl<'a, 'tcx> CheckCrateVisitor<'a, 'tcx> {
285285
fn check_static_type(&self, e: &ast::Expr) {
286286
let ty = ty::node_id_to_type(self.tcx, e.id);
287287
let infcx = infer::new_infer_ctxt(self.tcx);
288-
let mut fulfill_cx = traits::FulfillmentContext::new();
288+
let mut fulfill_cx = traits::FulfillmentContext::new(false);
289289
let cause = traits::ObligationCause::new(e.span, e.id, traits::SharedStatic);
290290
fulfill_cx.register_builtin_bound(&infcx, ty, ty::BoundSync, cause);
291291
let env = ty::empty_parameter_environment(self.tcx);

src/librustc/middle/traits/fulfill.rs

Lines changed: 68 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,10 @@ use super::select::SelectionContext;
2828
use super::Unimplemented;
2929
use super::util::predicate_for_builtin_bound;
3030

31+
pub struct FulfilledPredicates<'tcx> {
32+
set: HashSet<ty::Predicate<'tcx>>
33+
}
34+
3135
/// The fulfillment context is used to drive trait resolution. It
3236
/// consists of a list of obligations that must be (eventually)
3337
/// satisfied. The job is to track which are satisfied, which yielded
@@ -44,7 +48,7 @@ pub struct FulfillmentContext<'tcx> {
4448
// than the `SelectionCache`: it avoids duplicate errors and
4549
// permits recursive obligations, which are often generated from
4650
// traits like `Send` et al.
47-
duplicate_set: HashSet<ty::Predicate<'tcx>>,
51+
duplicate_set: FulfilledPredicates<'tcx>,
4852

4953
// A list of all obligations that have been registered with this
5054
// fulfillment context.
@@ -80,6 +84,8 @@ pub struct FulfillmentContext<'tcx> {
8084
// obligations (otherwise, it's easy to fail to walk to a
8185
// particular node-id).
8286
region_obligations: NodeMap<Vec<RegionObligation<'tcx>>>,
87+
88+
errors_will_be_reported: bool,
8389
}
8490

8591
#[derive(Clone)]
@@ -90,12 +96,30 @@ pub struct RegionObligation<'tcx> {
9096
}
9197

9298
impl<'tcx> FulfillmentContext<'tcx> {
93-
pub fn new() -> FulfillmentContext<'tcx> {
99+
/// Creates a new fulfillment context.
100+
///
101+
/// `errors_will_be_reported` indicates whether ALL errors that
102+
/// are generated by this fulfillment context will be reported to
103+
/// the end user. This is used to inform caching, because it
104+
/// allows us to conclude that traits that resolve successfully
105+
/// will in fact always resolve successfully (in particular, it
106+
/// guarantees that if some dependent obligation encounters a
107+
/// problem, compilation will be aborted). If you're not sure of
108+
/// the right value here, pass `false`, as that is the more
109+
/// conservative option.
110+
///
111+
/// FIXME -- a better option would be to hold back on modifying
112+
/// the global cache until we know that all dependent obligations
113+
/// are also satisfied. In that case, we could actually remove
114+
/// this boolean flag, and we'd also avoid the problem of squelching
115+
/// duplicate errors that occur across fns.
116+
pub fn new(errors_will_be_reported: bool) -> FulfillmentContext<'tcx> {
94117
FulfillmentContext {
95-
duplicate_set: HashSet::new(),
118+
duplicate_set: FulfilledPredicates::new(),
96119
predicates: Vec::new(),
97120
attempted_mark: 0,
98121
region_obligations: NodeMap(),
122+
errors_will_be_reported: errors_will_be_reported,
99123
}
100124
}
101125

@@ -165,7 +189,7 @@ impl<'tcx> FulfillmentContext<'tcx> {
165189

166190
assert!(!obligation.has_escaping_regions());
167191

168-
if !self.duplicate_set.insert(obligation.predicate.clone()) {
192+
if self.is_duplicate_or_add(infcx.tcx, &obligation.predicate) {
169193
debug!("register_predicate({}) -- already seen, skip", obligation.repr(infcx.tcx));
170194
return;
171195
}
@@ -231,6 +255,28 @@ impl<'tcx> FulfillmentContext<'tcx> {
231255
&self.predicates
232256
}
233257

258+
fn is_duplicate_or_add(&mut self, tcx: &ty::ctxt<'tcx>,
259+
predicate: &ty::Predicate<'tcx>)
260+
-> bool {
261+
// This is a kind of dirty hack to allow us to avoid "rederiving"
262+
// things that we have already proven in other methods.
263+
//
264+
// The idea is that any predicate that doesn't involve type
265+
// parameters and which only involves the 'static region (and
266+
// no other regions) is universally solvable, since impls are global.
267+
//
268+
// This is particularly important since even if we have a
269+
// cache hit in the selection context, we still wind up
270+
// evaluating the 'nested obligations'. This cache lets us
271+
// skip those.
272+
273+
if self.errors_will_be_reported && predicate.is_global() {
274+
tcx.fulfilled_predicates.borrow_mut().is_duplicate_or_add(predicate)
275+
} else {
276+
self.duplicate_set.is_duplicate_or_add(predicate)
277+
}
278+
}
279+
234280
/// Attempts to select obligations using `selcx`. If `only_new_obligations` is true, then it
235281
/// only attempts to select obligations that haven't been seen before.
236282
fn select<'a>(&mut self,
@@ -442,3 +488,21 @@ fn register_region_obligation<'tcx>(tcx: &ty::ctxt<'tcx>,
442488
.push(region_obligation);
443489

444490
}
491+
492+
impl<'tcx> FulfilledPredicates<'tcx> {
493+
pub fn new() -> FulfilledPredicates<'tcx> {
494+
FulfilledPredicates {
495+
set: HashSet::new()
496+
}
497+
}
498+
499+
pub fn is_duplicate(&self, p: &ty::Predicate<'tcx>) -> bool {
500+
self.set.contains(p)
501+
}
502+
503+
fn is_duplicate_or_add(&mut self, p: &ty::Predicate<'tcx>) -> bool {
504+
!self.set.insert(p.clone())
505+
}
506+
}
507+
508+

src/librustc/middle/traits/mod.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ pub use self::error_reporting::suggest_new_overflow_limit;
3232
pub use self::coherence::orphan_check;
3333
pub use self::coherence::overlapping_impls;
3434
pub use self::coherence::OrphanCheckErr;
35-
pub use self::fulfill::{FulfillmentContext, RegionObligation};
35+
pub use self::fulfill::{FulfillmentContext, FulfilledPredicates, RegionObligation};
3636
pub use self::project::MismatchedProjectionTypes;
3737
pub use self::project::normalize;
3838
pub use self::project::Normalized;
@@ -315,7 +315,7 @@ pub fn evaluate_builtin_bound<'a,'tcx>(infcx: &InferCtxt<'a,'tcx>,
315315
ty.repr(infcx.tcx),
316316
bound);
317317

318-
let mut fulfill_cx = FulfillmentContext::new();
318+
let mut fulfill_cx = FulfillmentContext::new(false);
319319

320320
// We can use a dummy node-id here because we won't pay any mind
321321
// to region obligations that arise (there shouldn't really be any
@@ -460,7 +460,7 @@ pub fn fully_normalize<'a,'tcx,T>(infcx: &InferCtxt<'a,'tcx>,
460460
debug!("normalize_param_env(value={})", value.repr(tcx));
461461

462462
let mut selcx = &mut SelectionContext::new(infcx, closure_typer);
463-
let mut fulfill_cx = FulfillmentContext::new();
463+
let mut fulfill_cx = FulfillmentContext::new(false);
464464
let Normalized { value: normalized_value, obligations } =
465465
project::normalize(selcx, cause, value);
466466
debug!("normalize_param_env: normalized_value={} obligations={}",

src/librustc/middle/traits/select.rs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -435,6 +435,14 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
435435
debug!("evaluate_predicate_recursively({})",
436436
obligation.repr(self.tcx()));
437437

438+
// Check the cache from the tcx of predicates that we know
439+
// have been proven elsewhere. This cache only contains
440+
// predicates that are global in scope and hence unaffected by
441+
// the current environment.
442+
if self.tcx().fulfilled_predicates.borrow().is_duplicate(&obligation.predicate) {
443+
return EvaluatedToOk;
444+
}
445+
438446
match obligation.predicate {
439447
ty::Predicate::Trait(ref t) => {
440448
assert!(!t.has_escaping_regions());

src/librustc/middle/ty.rs

Lines changed: 55 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -753,6 +753,11 @@ pub struct ctxt<'tcx> {
753753
/// for things that do not have to do with the parameters in scope.
754754
pub selection_cache: traits::SelectionCache<'tcx>,
755755

756+
/// A set of predicates that have been fulfilled *somewhere*.
757+
/// This is used to avoid duplicate work. Predicates are only
758+
/// added to this set when they
759+
pub fulfilled_predicates: RefCell<traits::FulfilledPredicates<'tcx>>,
760+
756761
/// Caches the representation hints for struct definitions.
757762
pub repr_hint_cache: RefCell<DefIdMap<Rc<Vec<attr::ReprAttr>>>>,
758763

@@ -815,6 +820,11 @@ bitflags! {
815820
const HAS_TY_ERR = 1 << 6,
816821
const HAS_PROJECTION = 1 << 7,
817822
const HAS_TY_CLOSURE = 1 << 8,
823+
824+
// true if there are "names" of types and regions and so forth
825+
// that are local to a particular fn
826+
const HAS_LOCAL_NAMES = 1 << 8,
827+
818828
const NEEDS_SUBST = TypeFlags::HAS_PARAMS.bits |
819829
TypeFlags::HAS_SELF.bits |
820830
TypeFlags::HAS_RE_EARLY_BOUND.bits,
@@ -830,7 +840,8 @@ bitflags! {
830840
TypeFlags::HAS_FREE_REGIONS.bits |
831841
TypeFlags::HAS_TY_ERR.bits |
832842
TypeFlags::HAS_PROJECTION.bits |
833-
TypeFlags::HAS_TY_CLOSURE.bits,
843+
TypeFlags::HAS_TY_CLOSURE.bits |
844+
TypeFlags::HAS_LOCAL_NAMES.bits,
834845

835846
// Caches for type_is_sized, type_moves_by_default
836847
const SIZEDNESS_CACHED = 1 << 16,
@@ -986,6 +997,9 @@ pub fn type_has_ty_infer(ty: Ty) -> bool {
986997
pub fn type_needs_infer(ty: Ty) -> bool {
987998
ty.flags.get().intersects(TypeFlags::HAS_TY_INFER | TypeFlags::HAS_RE_INFER)
988999
}
1000+
pub fn type_is_global(ty: Ty) -> bool {
1001+
!ty.flags.get().intersects(TypeFlags::HAS_LOCAL_NAMES)
1002+
}
9891003
pub fn type_has_projection(ty: Ty) -> bool {
9901004
ty.flags.get().intersects(TypeFlags::HAS_PROJECTION)
9911005
}
@@ -1288,6 +1302,15 @@ pub struct UpvarBorrow {
12881302
pub type UpvarCaptureMap = FnvHashMap<UpvarId, UpvarCapture>;
12891303

12901304
impl Region {
1305+
pub fn is_global(&self) -> bool {
1306+
// does this represent a region that can be named in a global
1307+
// way? used in fulfillment caching.
1308+
match *self {
1309+
ty::ReStatic | ty::ReEmpty => true,
1310+
_ => false,
1311+
}
1312+
}
1313+
12911314
pub fn is_bound(&self) -> bool {
12921315
match *self {
12931316
ty::ReEarlyBound(..) => true,
@@ -2022,6 +2045,29 @@ impl<'tcx> Predicate<'tcx> {
20222045
Predicate::Projection(ty::Binder(data.subst(tcx, substs))),
20232046
}
20242047
}
2048+
2049+
// Indicates whether this predicate references only 'global'
2050+
// types/lifetimes that are the same regardless of what fn we are
2051+
// in. This is used for caching. Errs on the side of returning
2052+
// false.
2053+
pub fn is_global(&self) -> bool {
2054+
match *self {
2055+
ty::Predicate::Trait(ref data) => {
2056+
let substs = data.skip_binder().trait_ref.substs;
2057+
2058+
substs.types.iter().all(|t| ty::type_is_global(t)) && {
2059+
match substs.regions {
2060+
subst::ErasedRegions => true,
2061+
subst::NonerasedRegions(ref r) => r.iter().all(|r| r.is_global()),
2062+
}
2063+
}
2064+
}
2065+
2066+
_ => {
2067+
false
2068+
}
2069+
}
2070+
}
20252071
}
20262072

20272073
#[derive(Clone, PartialEq, Eq, Hash, Debug)]
@@ -2798,6 +2844,7 @@ pub fn mk_ctxt<'tcx>(s: Session,
27982844
trait_defs: RefCell::new(DefIdMap()),
27992845
predicates: RefCell::new(DefIdMap()),
28002846
super_predicates: RefCell::new(DefIdMap()),
2847+
fulfilled_predicates: RefCell::new(traits::FulfilledPredicates::new()),
28012848
map: map,
28022849
freevars: freevars,
28032850
tcache: RefCell::new(DefIdMap()),
@@ -3010,6 +3057,7 @@ impl FlagComputation {
30103057
}
30113058

30123059
&TyParam(ref p) => {
3060+
self.add_flags(TypeFlags::HAS_LOCAL_NAMES);
30133061
if p.space == subst::SelfSpace {
30143062
self.add_flags(TypeFlags::HAS_SELF);
30153063
} else {
@@ -3018,11 +3066,12 @@ impl FlagComputation {
30183066
}
30193067

30203068
&TyClosure(_, substs) => {
3021-
self.add_flags(TypeFlags::HAS_TY_CLOSURE);
3069+
self.add_flags(TypeFlags::HAS_LOCAL_NAMES);
30223070
self.add_substs(substs);
30233071
}
30243072

30253073
&TyInfer(_) => {
3074+
self.add_flags(TypeFlags::HAS_LOCAL_NAMES); // it might, right?
30263075
self.add_flags(TypeFlags::HAS_TY_INFER)
30273076
}
30283077

@@ -3102,6 +3151,10 @@ impl FlagComputation {
31023151
ty::ReStatic => {}
31033152
_ => { self.add_flags(TypeFlags::HAS_FREE_REGIONS); }
31043153
}
3154+
3155+
if !r.is_global() {
3156+
self.add_flags(TypeFlags::HAS_LOCAL_NAMES);
3157+
}
31053158
}
31063159

31073160
fn add_projection_predicate(&mut self, projection_predicate: &ProjectionPredicate) {

src/librustc_trans/trans/common.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1041,7 +1041,7 @@ pub fn fulfill_obligation<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
10411041
// Currently, we use a fulfillment context to completely resolve
10421042
// all nested obligations. This is because they can inform the
10431043
// inference of the impl's type parameters.
1044-
let mut fulfill_cx = traits::FulfillmentContext::new();
1044+
let mut fulfill_cx = traits::FulfillmentContext::new(true);
10451045
let vtable = selection.map(|predicate| {
10461046
fulfill_cx.register_predicate_obligation(&infcx, predicate);
10471047
});
@@ -1069,7 +1069,7 @@ pub fn normalize_and_test_predicates<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
10691069
let infcx = infer::new_infer_ctxt(tcx);
10701070
let typer = NormalizingClosureTyper::new(tcx);
10711071
let mut selcx = traits::SelectionContext::new(&infcx, &typer);
1072-
let mut fulfill_cx = traits::FulfillmentContext::new();
1072+
let mut fulfill_cx = traits::FulfillmentContext::new(false);
10731073
let cause = traits::ObligationCause::dummy();
10741074
let traits::Normalized { value: predicates, obligations } =
10751075
traits::normalize(&mut selcx, cause.clone(), &predicates);

src/librustc_trans/trans/monomorphize.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -337,7 +337,7 @@ pub fn normalize_associated_type<'tcx,T>(tcx: &ty::ctxt<'tcx>, value: &T) -> T
337337
result.repr(tcx),
338338
obligations.repr(tcx));
339339

340-
let mut fulfill_cx = traits::FulfillmentContext::new();
340+
let mut fulfill_cx = traits::FulfillmentContext::new(true);
341341
for obligation in obligations {
342342
fulfill_cx.register_predicate_obligation(&infcx, obligation);
343343
}

src/librustc_typeck/check/compare_method.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ pub fn compare_impl_method<'tcx>(tcx: &ty::ctxt<'tcx>,
4545
impl_trait_ref.repr(tcx));
4646

4747
let infcx = infer::new_infer_ctxt(tcx);
48-
let mut fulfillment_cx = traits::FulfillmentContext::new();
48+
let mut fulfillment_cx = traits::FulfillmentContext::new(true);
4949

5050
let trait_to_impl_substs = &impl_trait_ref.substs;
5151

@@ -422,7 +422,7 @@ pub fn compare_const_impl<'tcx>(tcx: &ty::ctxt<'tcx>,
422422
impl_trait_ref.repr(tcx));
423423

424424
let infcx = infer::new_infer_ctxt(tcx);
425-
let mut fulfillment_cx = traits::FulfillmentContext::new();
425+
let mut fulfillment_cx = traits::FulfillmentContext::new(true);
426426

427427
// The below is for the most part highly similar to the procedure
428428
// for methods above. It is simpler in many respects, especially

src/librustc_typeck/check/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -386,7 +386,7 @@ impl<'a, 'tcx> Inherited<'a, 'tcx> {
386386
closure_tys: RefCell::new(DefIdMap()),
387387
closure_kinds: RefCell::new(DefIdMap()),
388388
fn_sig_map: RefCell::new(NodeMap()),
389-
fulfillment_cx: RefCell::new(traits::FulfillmentContext::new()),
389+
fulfillment_cx: RefCell::new(traits::FulfillmentContext::new(true)),
390390
deferred_call_resolutions: RefCell::new(DefIdMap()),
391391
deferred_cast_checks: RefCell::new(Vec::new()),
392392
}

src/librustc_typeck/coherence/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -539,7 +539,7 @@ impl<'a, 'tcx> CoherenceChecker<'a, 'tcx> {
539539
}
540540
};
541541

542-
let mut fulfill_cx = traits::FulfillmentContext::new();
542+
let mut fulfill_cx = traits::FulfillmentContext::new(true);
543543

544544
// Register an obligation for `A: Trait<B>`.
545545
let cause = traits::ObligationCause::misc(span, impl_did.node);

0 commit comments

Comments
 (0)