Skip to content

Commit ebe5c30

Browse files
nikomatsakisnikomatsakis
committed
make evaluation track whether outlives relationships mattered
Previously, evaluation ignored outlives relationships. Since we using evaluation to skip the "normal" trait selection (which enforces outlives relationships) this led to incorrect results in some cases.
1 parent ae36663 commit ebe5c30

File tree

9 files changed

+125
-40
lines changed

9 files changed

+125
-40
lines changed

‎src/librustc/infer/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1390,7 +1390,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
13901390
// rightly refuses to work with inference variables, but
13911391
// moves_by_default has a cache, which we want to use in other
13921392
// cases.
1393-
!traits::type_known_to_meet_bound(self, param_env, ty, copy_def_id, span)
1393+
!traits::type_known_to_meet_bound_modulo_regions(self, param_env, ty, copy_def_id, span)
13941394
}
13951395

13961396
/// Obtains the latest type of the given closure; this may be a

‎src/librustc/traits/fulfill.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -293,10 +293,10 @@ impl<'a, 'b, 'gcx, 'tcx> ObligationProcessor for FulfillProcessor<'a, 'b, 'gcx,
293293
ty::Predicate::Trait(ref data) => {
294294
let trait_obligation = obligation.with(data.clone());
295295

296-
if data.is_global() && !data.has_late_bound_regions() {
296+
if data.is_global() {
297297
// no type variables present, can use evaluation for better caching.
298298
// FIXME: consider caching errors too.
299-
if self.selcx.infcx().predicate_must_hold(&obligation) {
299+
if self.selcx.infcx().predicate_must_hold_considering_regions(&obligation) {
300300
debug!("selecting trait `{:?}` at depth {} evaluated to holds",
301301
data, obligation.recursion_depth);
302302
return ProcessResult::Changed(vec![])

‎src/librustc/traits/mod.rs

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -564,14 +564,14 @@ pub fn predicates_for_generics<'tcx>(cause: ObligationCause<'tcx>,
564564
/// `bound` or is not known to meet bound (note that this is
565565
/// conservative towards *no impl*, which is the opposite of the
566566
/// `evaluate` methods).
567-
pub fn type_known_to_meet_bound<'a, 'gcx, 'tcx>(infcx: &InferCtxt<'a, 'gcx, 'tcx>,
568-
param_env: ty::ParamEnv<'tcx>,
569-
ty: Ty<'tcx>,
570-
def_id: DefId,
571-
span: Span)
572-
-> bool
573-
{
574-
debug!("type_known_to_meet_bound(ty={:?}, bound={:?})",
567+
pub fn type_known_to_meet_bound_modulo_regions<'a, 'gcx, 'tcx>(
568+
infcx: &InferCtxt<'a, 'gcx, 'tcx>,
569+
param_env: ty::ParamEnv<'tcx>,
570+
ty: Ty<'tcx>,
571+
def_id: DefId,
572+
span: Span,
573+
) -> bool {
574+
debug!("type_known_to_meet_bound_modulo_regions(ty={:?}, bound={:?})",
575575
ty,
576576
infcx.tcx.item_path_str(def_id));
577577

@@ -586,7 +586,7 @@ pub fn type_known_to_meet_bound<'a, 'gcx, 'tcx>(infcx: &InferCtxt<'a, 'gcx, 'tcx
586586
predicate: trait_ref.to_predicate(),
587587
};
588588

589-
let result = infcx.predicate_must_hold(&obligation);
589+
let result = infcx.predicate_must_hold_modulo_regions(&obligation);
590590
debug!("type_known_to_meet_ty={:?} bound={} => {:?}",
591591
ty, infcx.tcx.item_path_str(def_id), result);
592592

@@ -613,13 +613,13 @@ pub fn type_known_to_meet_bound<'a, 'gcx, 'tcx>(infcx: &InferCtxt<'a, 'gcx, 'tcx
613613
// assume it is move; linear is always ok.
614614
match fulfill_cx.select_all_or_error(infcx) {
615615
Ok(()) => {
616-
debug!("type_known_to_meet_bound: ty={:?} bound={} success",
616+
debug!("type_known_to_meet_bound_modulo_regions: ty={:?} bound={} success",
617617
ty,
618618
infcx.tcx.item_path_str(def_id));
619619
true
620620
}
621621
Err(e) => {
622-
debug!("type_known_to_meet_bound: ty={:?} bound={} errors={:?}",
622+
debug!("type_known_to_meet_bound_modulo_regions: ty={:?} bound={} errors={:?}",
623623
ty,
624624
infcx.tcx.item_path_str(def_id),
625625
e);

‎src/librustc/traits/query/evaluate_obligation.rs

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,11 +26,26 @@ impl<'cx, 'gcx, 'tcx> InferCtxt<'cx, 'gcx, 'tcx> {
2626
/// Evaluates whether the predicate can be satisfied in the given
2727
/// `ParamEnv`, and returns `false` if not certain. However, this is
2828
/// not entirely accurate if inference variables are involved.
29-
pub fn predicate_must_hold(
29+
///
30+
/// This version may conservatively fail when outlives obligations
31+
/// are required.
32+
pub fn predicate_must_hold_considering_regions(
3033
&self,
3134
obligation: &PredicateObligation<'tcx>,
3235
) -> bool {
33-
self.evaluate_obligation(obligation) == EvaluationResult::EvaluatedToOk
36+
self.evaluate_obligation(obligation).must_apply_considering_regions()
37+
}
38+
39+
/// Evaluates whether the predicate can be satisfied in the given
40+
/// `ParamEnv`, and returns `false` if not certain. However, this is
41+
/// not entirely accurate if inference variables are involved.
42+
///
43+
/// This version ignores all outlives constraints.
44+
pub fn predicate_must_hold_modulo_regions(
45+
&self,
46+
obligation: &PredicateObligation<'tcx>,
47+
) -> bool {
48+
self.evaluate_obligation(obligation).must_apply_modulo_regions()
3449
}
3550

3651
// Helper function that canonicalizes and runs the query, as well as handles

‎src/librustc/traits/select.rs

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -315,7 +315,7 @@ enum BuiltinImplConditions<'tcx> {
315315
/// evaluations.
316316
///
317317
/// The evaluation results are ordered:
318-
/// - `EvaluatedToOk` implies `EvaluatedToAmbig` implies `EvaluatedToUnknown`
318+
/// - `EvaluatedToOk` implies `EvaluatedToOkModuloRegions` implies `EvaluatedToAmbig` implies `EvaluatedToUnknown`
319319
/// - `EvaluatedToErr` implies `EvaluatedToRecur`
320320
/// - the "union" of evaluation results is equal to their maximum -
321321
/// all the "potential success" candidates can potentially succeed,
@@ -324,6 +324,8 @@ enum BuiltinImplConditions<'tcx> {
324324
pub enum EvaluationResult {
325325
/// Evaluation successful
326326
EvaluatedToOk,
327+
/// Evaluation successful, but there were unevaluated region obligations
328+
EvaluatedToOkModuloRegions,
327329
/// Evaluation is known to be ambiguous - it *might* hold for some
328330
/// assignment of inference variables, but it might not.
329331
///
@@ -387,9 +389,22 @@ pub enum EvaluationResult {
387389
}
388390

389391
impl EvaluationResult {
392+
/// True if this evaluation result is known to apply, even
393+
/// considering outlives constraints.
394+
pub fn must_apply_considering_regions(self) -> bool {
395+
self == EvaluatedToOk
396+
}
397+
398+
/// True if this evaluation result is known to apply, ignoring
399+
/// outlives constraints.
400+
pub fn must_apply_modulo_regions(self) -> bool {
401+
self <= EvaluatedToOkModuloRegions
402+
}
403+
390404
pub fn may_apply(self) -> bool {
391405
match self {
392406
EvaluatedToOk |
407+
EvaluatedToOkModuloRegions |
393408
EvaluatedToAmbig |
394409
EvaluatedToUnknown => true,
395410

@@ -404,6 +419,7 @@ impl EvaluationResult {
404419
EvaluatedToRecur => true,
405420

406421
EvaluatedToOk |
422+
EvaluatedToOkModuloRegions |
407423
EvaluatedToAmbig |
408424
EvaluatedToErr => false,
409425
}
@@ -412,6 +428,7 @@ impl EvaluationResult {
412428

413429
impl_stable_hash_for!(enum self::EvaluationResult {
414430
EvaluatedToOk,
431+
EvaluatedToOkModuloRegions,
415432
EvaluatedToAmbig,
416433
EvaluatedToUnknown,
417434
EvaluatedToRecur,
@@ -693,7 +710,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
693710
ty::Predicate::TypeOutlives(..) | ty::Predicate::RegionOutlives(..) => {
694711
// we do not consider region relationships when
695712
// evaluating trait matches
696-
Ok(EvaluatedToOk)
713+
Ok(EvaluatedToOkModuloRegions)
697714
}
698715

699716
ty::Predicate::ObjectSafe(trait_def_id) => {
@@ -900,7 +917,13 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
900917
{
901918
debug!("evaluate_stack({:?}) --> recursive",
902919
stack.fresh_trait_ref);
903-
let cycle = stack.iter().skip(1).take(rec_index + 1);
920+
921+
// Subtle: when checking for a coinductive cycle, we do
922+
// not compare using the "freshened trait refs" (which
923+
// have erased regions) but rather the fully explicit
924+
// trait refs. This is important because it's only a cycle
925+
// if the regions match exactly.
926+
let cycle = stack.iter().skip(1).take(rec_index+1);
904927
let cycle = cycle.map(|stack| ty::Predicate::Trait(stack.obligation.predicate));
905928
if self.coinductive_match(cycle) {
906929
debug!("evaluate_stack({:?}) --> recursive, coinductive",
@@ -2095,7 +2118,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
20952118
// See if we can toss out `victim` based on specialization.
20962119
// This requires us to know *for sure* that the `other` impl applies
20972120
// i.e. EvaluatedToOk:
2098-
if other.evaluation == EvaluatedToOk {
2121+
if other.evaluation.must_apply_modulo_regions() {
20992122
match victim.candidate {
21002123
ImplCandidate(victim_def) => {
21012124
let tcx = self.tcx().global_tcx();
@@ -2122,7 +2145,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
21222145
ParamCandidate(ref cand) => {
21232146
// Prefer these to a global where-clause bound
21242147
// (see issue #50825)
2125-
is_global(cand) && other.evaluation == EvaluatedToOk
2148+
is_global(cand) && other.evaluation.must_apply_modulo_regions()
21262149
}
21272150
_ => false,
21282151
}

‎src/librustc/ty/util.rs

Lines changed: 21 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -862,11 +862,13 @@ fn is_copy_raw<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
862862
let (param_env, ty) = query.into_parts();
863863
let trait_def_id = tcx.require_lang_item(lang_items::CopyTraitLangItem);
864864
tcx.infer_ctxt()
865-
.enter(|infcx| traits::type_known_to_meet_bound(&infcx,
866-
param_env,
867-
ty,
868-
trait_def_id,
869-
DUMMY_SP))
865+
.enter(|infcx| traits::type_known_to_meet_bound_modulo_regions(
866+
&infcx,
867+
param_env,
868+
ty,
869+
trait_def_id,
870+
DUMMY_SP,
871+
))
870872
}
871873

872874
fn is_sized_raw<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
@@ -876,11 +878,13 @@ fn is_sized_raw<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
876878
let (param_env, ty) = query.into_parts();
877879
let trait_def_id = tcx.require_lang_item(lang_items::SizedTraitLangItem);
878880
tcx.infer_ctxt()
879-
.enter(|infcx| traits::type_known_to_meet_bound(&infcx,
880-
param_env,
881-
ty,
882-
trait_def_id,
883-
DUMMY_SP))
881+
.enter(|infcx| traits::type_known_to_meet_bound_modulo_regions(
882+
&infcx,
883+
param_env,
884+
ty,
885+
trait_def_id,
886+
DUMMY_SP,
887+
))
884888
}
885889

886890
fn is_freeze_raw<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
@@ -890,11 +894,13 @@ fn is_freeze_raw<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
890894
let (param_env, ty) = query.into_parts();
891895
let trait_def_id = tcx.require_lang_item(lang_items::FreezeTraitLangItem);
892896
tcx.infer_ctxt()
893-
.enter(|infcx| traits::type_known_to_meet_bound(&infcx,
894-
param_env,
895-
ty,
896-
trait_def_id,
897-
DUMMY_SP))
897+
.enter(|infcx| traits::type_known_to_meet_bound_modulo_regions(
898+
&infcx,
899+
param_env,
900+
ty,
901+
trait_def_id,
902+
DUMMY_SP,
903+
))
898904
}
899905

900906
fn needs_drop_raw<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,

‎src/librustc_typeck/check/cast.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
9898
return Err(ErrorReported);
9999
}
100100

101-
if self.type_is_known_to_be_sized(t, span) {
101+
if self.type_is_known_to_be_sized_modulo_regions(t, span) {
102102
return Ok(Some(PointerKind::Thin));
103103
}
104104

@@ -406,7 +406,7 @@ impl<'a, 'gcx, 'tcx> CastCheck<'tcx> {
406406
self.expr_ty,
407407
self.cast_ty);
408408

409-
if !fcx.type_is_known_to_be_sized(self.cast_ty, self.span) {
409+
if !fcx.type_is_known_to_be_sized_modulo_regions(self.cast_ty, self.span) {
410410
self.report_cast_to_unsized_type(fcx);
411411
} else if self.expr_ty.references_error() || self.cast_ty.references_error() {
412412
// No sense in giving duplicate error messages
@@ -627,8 +627,8 @@ impl<'a, 'gcx, 'tcx> CastCheck<'tcx> {
627627
}
628628

629629
impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
630-
fn type_is_known_to_be_sized(&self, ty: Ty<'tcx>, span: Span) -> bool {
630+
fn type_is_known_to_be_sized_modulo_regions(&self, ty: Ty<'tcx>, span: Span) -> bool {
631631
let lang_item = self.tcx.require_lang_item(lang_items::SizedTraitLangItem);
632-
traits::type_known_to_meet_bound(self, self.param_env, ty, lang_item, span)
632+
traits::type_known_to_meet_bound_modulo_regions(self, self.param_env, ty, lang_item, span)
633633
}
634634
}

‎src/test/ui/hrtb/hrtb-cache-issue-54302.rs

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
// Regression test for #54302.
2+
//
3+
// We were incorrectly using the "evaluation cache" (which ignored
4+
// region results) to conclude that `&'static str: Deserialize`, even
5+
// though it would require that `for<'de> 'de: 'static`, which is
6+
// clearly false.
7+
8+
trait Deserialize<'de> {}
9+
10+
trait DeserializeOwned: for<'de> Deserialize<'de> {}
11+
impl<T> DeserializeOwned for T where T: for<'de> Deserialize<'de> {}
12+
13+
// Based on this impl, `&'static str` only implements Deserialize<'static>.
14+
// It does not implement for<'de> Deserialize<'de>.
15+
impl<'de: 'a, 'a> Deserialize<'de> for &'a str {}
16+
17+
fn main() {
18+
fn assert_deserialize_owned<T: DeserializeOwned>() {}
19+
assert_deserialize_owned::<&'static str>(); //~ ERROR
20+
21+
// It correctly does not implement for<'de> Deserialize<'de>.
22+
// fn assert_hrtb<T: for<'de> Deserialize<'de>>() {}
23+
// assert_hrtb::<&'static str>();
24+
}

‎src/test/ui/hrtb/hrtb-cache-issue-54302.stderr

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
error[E0279]: the requirement `for<'de> 'de : ` is not satisfied (`expected bound lifetime parameter 'de, found concrete lifetime`)
2+
--> $DIR/hrtb-cache-issue-54302.rs:19:5
3+
|
4+
LL | assert_deserialize_owned::<&'static str>(); //~ ERROR
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
6+
|
7+
= note: required because of the requirements on the impl of `for<'de> Deserialize<'de>` for `&'static str`
8+
= note: required because of the requirements on the impl of `DeserializeOwned` for `&'static str`
9+
note: required by `main::assert_deserialize_owned`
10+
--> $DIR/hrtb-cache-issue-54302.rs:18:5
11+
|
12+
LL | fn assert_deserialize_owned<T: DeserializeOwned>() {}
13+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
14+
15+
error: aborting due to previous error
16+
17+
For more information about this error, try `rustc --explain E0279`.

0 commit comments

Comments
 (0)