Skip to content

Commit ff36153

Browse files
committed
Partial fix for #17901: Be less conservative around unbound type
variables in the intracrate case. This requires a deeper distinction between inter- and intra-crate so as to keep coherence working. I suspect the best fix is to generalize the recursion check that exists today, but this requires a bit more refactoring to achieve. (In other words, where today it says OK for an exact match, we'd want to not detect exact matches but rather skolemize each trait-reference fresh and return AMBIG -- but that requires us to make builtin bounds work shallowly like everything else and move the cycle detection into the fulfillment context.)
1 parent f791473 commit ff36153

File tree

3 files changed

+85
-61
lines changed

3 files changed

+85
-61
lines changed

src/librustc/middle/traits/coherence.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ pub fn impl_can_satisfy(infcx: &InferCtxt,
4343
// Determine whether `impl2` can provide an implementation for those
4444
// same types.
4545
let param_env = ty::empty_parameter_environment();
46-
let mut selcx = SelectionContext::new(infcx, &param_env, infcx.tcx);
46+
let mut selcx = SelectionContext::intercrate(infcx, &param_env, infcx.tcx);
4747
let obligation = Obligation::misc(DUMMY_SP, impl1_trait_ref);
4848
debug!("impl_can_satisfy obligation={}", obligation.repr(infcx.tcx));
4949
selcx.evaluate_impl(impl2_def_id, &obligation)

src/librustc/middle/traits/select.rs

+83-59
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,22 @@ pub struct SelectionContext<'cx, 'tcx:'cx> {
4545
/// which is important for checking for trait bounds that
4646
/// recursively require themselves.
4747
skolemizer: TypeSkolemizer<'cx, 'tcx>,
48+
49+
/// If true, indicates that the evaluation should be conservative
50+
/// and consider the possibility of types outside this crate.
51+
/// This comes up primarily when resolving ambiguity. Imagine
52+
/// there is some trait reference `$0 : Bar` where `$0` is an
53+
/// inference variable. If `intercrate` is true, then we can never
54+
/// say for sure that this reference is not implemented, even if
55+
/// there are *no impls at all for `Bar`*, because `$0` could be
56+
/// bound to some type that in a downstream crate that implements
57+
/// `Bar`. This is the suitable mode for coherence. Elsewhere,
58+
/// though, we set this to false, because we are only interested
59+
/// in types that the user could actually have written --- in
60+
/// other words, we consider `$0 : Bar` to be unimplemented if
61+
/// there is no type that the user could *actually name* that
62+
/// would satisfy it. This avoids crippling inference, basically.
63+
intercrate: bool,
4864
}
4965

5066
// A stack that walks back up the stack frame.
@@ -142,6 +158,20 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
142158
param_env: param_env,
143159
typer: typer,
144160
skolemizer: infcx.skolemizer(),
161+
intercrate: false,
162+
}
163+
}
164+
165+
pub fn intercrate(infcx: &'cx InferCtxt<'cx, 'tcx>,
166+
param_env: &'cx ty::ParameterEnvironment,
167+
typer: &'cx Typer<'tcx>)
168+
-> SelectionContext<'cx, 'tcx> {
169+
SelectionContext {
170+
infcx: infcx,
171+
param_env: param_env,
172+
typer: typer,
173+
skolemizer: infcx.skolemizer(),
174+
intercrate: true,
145175
}
146176
}
147177

@@ -214,44 +244,20 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
214244
// The result is "true" if the obligation *may* hold and "false" if
215245
// we can be sure it does not.
216246

217-
pub fn evaluate_obligation_intercrate(&mut self,
218-
obligation: &Obligation)
219-
-> bool
220-
{
221-
/*!
222-
* Evaluates whether the obligation `obligation` can be
223-
* satisfied (by any means). This "intercrate" version allows
224-
* for the possibility that unbound type variables may be
225-
* instantiated with types from another crate. This is
226-
* important for coherence. In practice this means that
227-
* unbound type variables must always be considered ambiguous.
228-
*/
229-
230-
debug!("evaluate_obligation_intercrate({})",
231-
obligation.repr(self.tcx()));
232-
233-
let stack = self.push_stack(None, obligation);
234-
self.evaluate_stack_intercrate(&stack).may_apply()
235-
}
236-
237-
pub fn evaluate_obligation_intracrate(&mut self,
238-
obligation: &Obligation)
239-
-> bool
247+
pub fn evaluate_obligation(&mut self,
248+
obligation: &Obligation)
249+
-> bool
240250
{
241251
/*!
242252
* Evaluates whether the obligation `obligation` can be
243-
* satisfied (by any means). This "intracrate" version does
244-
* not allow for the possibility that unbound type variables
245-
* may be instantiated with types from another crate; hence,
246-
* if there are unbound inputs but no crates locally visible,
247-
* it considers the result to be unimplemented.
253+
* satisfied (by any means).
248254
*/
249255

250-
debug!("evaluate_obligation_intracrate({})",
256+
debug!("evaluate_obligation({})",
251257
obligation.repr(self.tcx()));
252258

253259
let stack = self.push_stack(None, obligation);
254-
self.evaluate_stack_intracrate(&stack).may_apply()
260+
self.evaluate_stack(&stack).may_apply()
255261
}
256262

257263
fn evaluate_builtin_bound_recursively(&mut self,
@@ -288,46 +294,53 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
288294

289295
let stack = self.push_stack(previous_stack.map(|x| x), obligation);
290296

291-
// FIXME(#17901) -- Intercrate vs intracrate resolution is a
292-
// tricky question here. For coherence, we want
293-
// intercrate. Also, there was a nasty cycle around impls like
294-
// `impl<T:Eq> Eq for Vec<T>` (which would wind up checking
295-
// whether `$0:Eq`, where $0 was the value substituted for
296-
// `T`, which could then be checked against the very same
297-
// impl). This problem is avoided by the stricter rules around
298-
// unbound type variables by intercrate. I suspect that in the
299-
// latter case a more fine-grained rule would suffice (i.e.,
300-
// consider it ambiguous if even 1 impl matches, no need to
301-
// figure out which one, but call it unimplemented if 0 impls
302-
// match).
303-
let result = self.evaluate_stack_intercrate(&stack);
297+
let result = self.evaluate_stack(&stack);
304298

305299
debug!("result: {}", result);
306300
result
307301
}
308302

309-
fn evaluate_stack_intercrate(&mut self,
303+
fn evaluate_stack(&mut self,
310304
stack: &ObligationStack)
311305
-> EvaluationResult
312306
{
313-
// Whenever any of the types are unbound, there can always be
314-
// an impl. Even if there are no impls in this crate, perhaps
315-
// the type would be unified with something from another crate
316-
// that does provide an impl.
307+
// In intercrate mode, whenever any of the types are unbound,
308+
// there can always be an impl. Even if there are no impls in
309+
// this crate, perhaps the type would be unified with
310+
// something from another crate that does provide an impl.
311+
//
312+
// In intracrate mode, we must still be conservative. The reason is
313+
// that we want to avoid cycles. Imagine an impl like:
314+
//
315+
// impl<T:Eq> Eq for Vec<T>
316+
//
317+
// and a trait reference like `$0 : Eq` where `$0` is an
318+
// unbound variable. When we evaluate this trait-reference, we
319+
// will unify `$0` with `Vec<$1>` (for some fresh variable
320+
// `$1`), on the condition that `$1 : Eq`. We will then wind
321+
// up with many candidates (since that are other `Eq` impls
322+
// that apply) and try to winnow things down. This results in
323+
// a recurssive evaluation that `$1 : Eq` -- as you can
324+
// imagine, this is just where we started. To avoid that, we
325+
// check for unbound variables and return an ambiguous (hence possible)
326+
// match if we've seen this trait before.
327+
//
328+
// This suffices to allow chains like `FnMut` implemented in
329+
// terms of `Fn` etc, but we could probably make this more
330+
// precise still.
317331
let input_types = stack.skol_trait_ref.input_types();
318-
if input_types.iter().any(|&t| ty::type_is_skolemized(t)) {
319-
debug!("evaluate_stack_intercrate({}) --> unbound argument, must be ambiguous",
332+
let unbound_input_types = input_types.iter().any(|&t| ty::type_is_skolemized(t));
333+
if
334+
unbound_input_types &&
335+
(self.intercrate ||
336+
stack.iter().skip(1).any(
337+
|prev| stack.skol_trait_ref.def_id == prev.skol_trait_ref.def_id))
338+
{
339+
debug!("evaluate_stack_intracrate({}) --> unbound argument, recursion --> ambiguous",
320340
stack.skol_trait_ref.repr(self.tcx()));
321341
return EvaluatedToAmbig;
322342
}
323343

324-
self.evaluate_stack_intracrate(stack)
325-
}
326-
327-
fn evaluate_stack_intracrate(&mut self,
328-
stack: &ObligationStack)
329-
-> EvaluationResult
330-
{
331344
// If there is any previous entry on the stack that precisely
332345
// matches this obligation, then we can assume that the
333346
// obligation is satisfied for now (still all other conditions
@@ -592,7 +605,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
592605
Err(_) => { return Err(()); }
593606
}
594607

595-
if self.evaluate_obligation_intracrate(obligation) {
608+
if self.evaluate_obligation(obligation) {
596609
Ok(())
597610
} else {
598611
Err(())
@@ -828,7 +841,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
828841
// be the case that you could still satisfy the obligation
829842
// from another crate by instantiating the type variables with
830843
// a type from another crate that does have an impl. This case
831-
// is checked for in `evaluate_obligation` (and hence users
844+
// is checked for in `evaluate_stack` (and hence users
832845
// who might care about this case, like coherence, should use
833846
// that function).
834847
if candidates.len() == 0 {
@@ -849,6 +862,17 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
849862
// global cache. We want the cache that is specific to this
850863
// scope whenever where clauses might affect the result.
851864

865+
// Avoid using the master cache during coherence and just rely
866+
// on the local cache. This effectively disables caching
867+
// during coherence. It is really just a simplification to
868+
// avoid us having to fear that coherence results "pollute"
869+
// the master cache. Since coherence executes pretty quickly,
870+
// it's not worth going to more trouble to increase the
871+
// hit-rate I don't think.
872+
if self.intercrate {
873+
return &self.param_env.selection_cache;
874+
}
875+
852876
// If the trait refers to any parameters in scope, then use
853877
// the cache of the param-environment.
854878
if

src/librustc/middle/typeck/check/method.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -235,7 +235,7 @@ pub fn lookup_in_trait_adjusted<'a, 'tcx>(
235235
let mut selcx = traits::SelectionContext::new(fcx.infcx(),
236236
&fcx.inh.param_env,
237237
fcx);
238-
if !selcx.evaluate_obligation_intracrate(&obligation) {
238+
if !selcx.evaluate_obligation(&obligation) {
239239
debug!("--> Cannot match obligation");
240240
return None; // Cannot be matched, no such method resolution is possible.
241241
}

0 commit comments

Comments
 (0)