Skip to content

Commit af14db1

Browse files
jackh726jackh726
committed
Review comments
1 parent 9891e47 commit af14db1

File tree

3 files changed

+68
-41
lines changed

3 files changed

+68
-41
lines changed

‎compiler/rustc_mir/src/borrow_check/type_check/input_output.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -169,8 +169,8 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
169169
{
170170
// FIXME(jackh726): This is a hack. It's somewhat like
171171
// `rustc_traits::normalize_after_erasing_regions`. Ideally, we'd
172-
// like to normalize *before* inserting into `local_decls`, but I
173-
// couldn't figure out where the heck that was.
172+
// like to normalize *before* inserting into `local_decls`, but
173+
// doing so ends up causing some other trouble.
174174
let b = match self
175175
.infcx
176176
.at(&ObligationCause::dummy(), ty::ParamEnv::empty())

‎compiler/rustc_trait_selection/src/traits/project.rs‎

Lines changed: 38 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -363,12 +363,28 @@ impl<'a, 'b, 'tcx> TypeFolder<'tcx> for AssocTypeNormalizer<'a, 'b, 'tcx> {
363363
return ty;
364364
}
365365

366-
// N.b. while we want to call `super_fold_with(self)` on `ty` before
367-
// normalization, we wait until we know whether we need to normalize the
368-
// current type. If we do, then we only fold the ty *after* replacing bound
369-
// vars with placeholders. This means that nested types don't need to replace
370-
// bound vars at the current binder level or above. A key assumption here is
371-
// that folding the type can't introduce new bound vars.
366+
// We try to be a little clever here as a performance optimization in
367+
// cases where there are nested projections under binders.
368+
// For example:
369+
// ```
370+
// for<'a> fn(<T as Foo>::One<'a, Box<dyn Bar<'a, Item=<T as Foo>::Two<'a>>>>)
371+
// ```
372+
// We normalize the substs on the projection before the projecting, but
373+
// if we're naive, we'll
374+
// replace bound vars on inner, project inner, replace placeholders on inner,
375+
// replace bound vars on outer, project outer, replace placeholders on outer
376+
//
377+
// However, if we're a bit more clever, we can replace the bound vars
378+
// on the entire type before normalizing nested projections, meaning we
379+
// replace bound vars on outer, project inner,
380+
// project outer, replace placeholders on outer
381+
//
382+
// This is possible because the inner `'a` will already be a placeholder
383+
// when we need to normalize the inner projection
384+
//
385+
// On the other hand, this does add a bit of complexity, since we only
386+
// replace bound vars if the current type is a `Projection` and we need
387+
// to make sure we don't forget to fold the substs regardless.
372388

373389
match *ty.kind() {
374390
ty::Opaque(def_id, substs) => {
@@ -380,7 +396,6 @@ impl<'a, 'b, 'tcx> TypeFolder<'tcx> for AssocTypeNormalizer<'a, 'b, 'tcx> {
380396
// N.b. there is an assumption here all this code can handle
381397
// escaping bound vars.
382398

383-
let substs = substs.super_fold_with(self);
384399
let recursion_limit = self.tcx().recursion_limit();
385400
if !recursion_limit.value_within_limit(self.depth) {
386401
let obligation = Obligation::with_depth(
@@ -392,6 +407,7 @@ impl<'a, 'b, 'tcx> TypeFolder<'tcx> for AssocTypeNormalizer<'a, 'b, 'tcx> {
392407
self.selcx.infcx().report_overflow_error(&obligation, true);
393408
}
394409

410+
let substs = substs.super_fold_with(self);
395411
let generic_ty = self.tcx().type_of(def_id);
396412
let concrete_ty = generic_ty.subst(self.tcx(), substs);
397413
self.depth += 1;
@@ -430,12 +446,16 @@ impl<'a, 'b, 'tcx> TypeFolder<'tcx> for AssocTypeNormalizer<'a, 'b, 'tcx> {
430446

431447
ty::Projection(data) => {
432448
// If there are escaping bound vars, we temporarily replace the
433-
// bound vars with placeholders. Note though, that in the cas
449+
// bound vars with placeholders. Note though, that in the case
434450
// that we still can't project for whatever reason (e.g. self
435451
// type isn't known enough), we *can't* register an obligation
436452
// and return an inference variable (since then that obligation
437453
// would have bound vars and that's a can of worms). Instead,
438454
// we just give up and fall back to pretending like we never tried!
455+
//
456+
// Note: this isn't necessarily the final approach here; we may
457+
// want to figure out how to register obligations with escaping vars
458+
// or handle this some other way.
439459

440460
let infcx = self.selcx.infcx();
441461
let (data, mapped_regions, mapped_types, mapped_consts) =
@@ -451,16 +471,18 @@ impl<'a, 'b, 'tcx> TypeFolder<'tcx> for AssocTypeNormalizer<'a, 'b, 'tcx> {
451471
)
452472
.ok()
453473
.flatten()
474+
.map(|normalized_ty| {
475+
PlaceholderReplacer::replace_placeholders(
476+
infcx,
477+
mapped_regions,
478+
mapped_types,
479+
mapped_consts,
480+
&self.universes,
481+
normalized_ty,
482+
)
483+
})
454484
.unwrap_or_else(|| ty.super_fold_with(self));
455485

456-
let normalized_ty = PlaceholderReplacer::replace_placeholders(
457-
infcx,
458-
mapped_regions,
459-
mapped_types,
460-
mapped_consts,
461-
&self.universes,
462-
normalized_ty,
463-
);
464486
debug!(
465487
?self.depth,
466488
?ty,

‎compiler/rustc_trait_selection/src/traits/query/normalize.rs‎

Lines changed: 28 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,16 @@ impl<'cx, 'tcx> AtExt<'tcx> for At<'cx, 'tcx> {
6767
universes: vec![],
6868
};
6969

70+
// This is actually a consequence by the way `normalize_erasing_regions` works currently.
71+
// Because it needs to call the `normalize_generic_arg_after_erasing_regions`, it folds
72+
// through tys and consts in a `TypeFoldable`. Importantly, it skips binders, leaving us
73+
// with trying to normalize with escaping bound vars.
74+
//
75+
// Here, we just add the universes that we *would* have created had we passed through the binders.
76+
//
77+
// We *could* replace escaping bound vars eagerly here, but it doesn't seem really necessary.
78+
// The rest of the code is already set up to be lazy about replacing bound vars,
79+
// and only when we actually have to normalize.
7080
if value.has_escaping_bound_vars() {
7181
let mut max_visitor =
7282
MaxEscapingBoundVarVisitor { outer_index: ty::INNERMOST, escaping: 0 };
@@ -183,12 +193,8 @@ impl<'cx, 'tcx> TypeFolder<'tcx> for QueryNormalizer<'cx, 'tcx> {
183193
return ty;
184194
}
185195

186-
// N.b. while we want to call `super_fold_with(self)` on `ty` before
187-
// normalization, we wait until we know whether we need to normalize the
188-
// current type. If we do, then we only fold the ty *after* replacing bound
189-
// vars with placeholders. This means that nested types don't need to replace
190-
// bound vars at the current binder level or above. A key assumption here is
191-
// that folding the type can't introduce new bound vars.
196+
// See note in `rustc_trait_selection::traits::project` about why we
197+
// wait to fold the substs.
192198

193199
// Wrap this in a closure so we don't accidentally return from the outer function
194200
let res = (|| match *ty.kind() {
@@ -253,7 +259,7 @@ impl<'cx, 'tcx> TypeFolder<'tcx> for QueryNormalizer<'cx, 'tcx> {
253259
// We don't expect ambiguity.
254260
if result.is_ambiguous() {
255261
self.error = true;
256-
return ty;
262+
return ty.super_fold_with(self);
257263
}
258264

259265
match self.infcx.instantiate_query_response_and_region_obligations(
@@ -271,14 +277,14 @@ impl<'cx, 'tcx> TypeFolder<'tcx> for QueryNormalizer<'cx, 'tcx> {
271277

272278
Err(_) => {
273279
self.error = true;
274-
ty
280+
ty.super_fold_with(self)
275281
}
276282
}
277283
}
278284

279285
Err(NoSolution) => {
280286
self.error = true;
281-
ty
287+
ty.super_fold_with(self)
282288
}
283289
}
284290
}
@@ -304,12 +310,12 @@ impl<'cx, 'tcx> TypeFolder<'tcx> for QueryNormalizer<'cx, 'tcx> {
304310
.canonicalize_query_keep_static(self.param_env.and(data), &mut orig_values);
305311
debug!("QueryNormalizer: c_data = {:#?}", c_data);
306312
debug!("QueryNormalizer: orig_values = {:#?}", orig_values);
307-
let normalized_ty = match tcx.normalize_projection_ty(c_data) {
313+
match tcx.normalize_projection_ty(c_data) {
308314
Ok(result) => {
309315
// We don't expect ambiguity.
310316
if result.is_ambiguous() {
311317
self.error = true;
312-
return ty;
318+
return ty.super_fold_with(self);
313319
}
314320
match self.infcx.instantiate_query_response_and_region_obligations(
315321
self.cause,
@@ -321,27 +327,26 @@ impl<'cx, 'tcx> TypeFolder<'tcx> for QueryNormalizer<'cx, 'tcx> {
321327
debug!("QueryNormalizer: result = {:#?}", result);
322328
debug!("QueryNormalizer: obligations = {:#?}", obligations);
323329
self.obligations.extend(obligations);
324-
result.normalized_ty
330+
crate::traits::project::PlaceholderReplacer::replace_placeholders(
331+
infcx,
332+
mapped_regions,
333+
mapped_types,
334+
mapped_consts,
335+
&self.universes,
336+
result.normalized_ty,
337+
)
325338
}
326339
Err(_) => {
327340
self.error = true;
328-
ty
341+
ty.super_fold_with(self)
329342
}
330343
}
331344
}
332345
Err(NoSolution) => {
333346
self.error = true;
334-
ty
347+
ty.super_fold_with(self)
335348
}
336-
};
337-
crate::traits::project::PlaceholderReplacer::replace_placeholders(
338-
infcx,
339-
mapped_regions,
340-
mapped_types,
341-
mapped_consts,
342-
&self.universes,
343-
normalized_ty,
344-
)
349+
}
345350
}
346351

347352
_ => ty.super_fold_with(self),

0 commit comments

Comments
 (0)