Review comments

3 files changed, 68 insertions, 41 deletions

diff --git a/compiler/rustc_mir/src/borrow_check/type_check/input_output.rs b/compiler/rustc_mir/src/borrow_check/type_check/input_output.rs
index c815ed0d1cb..ba9b6926526 100644
--- a/compiler/rustc_mir/src/borrow_check/type_check/input_output.rs
+++ b/compiler/rustc_mir/src/borrow_check/type_check/input_output.rs
@@ -169,8 +169,8 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
         {
             // FIXME(jackh726): This is a hack. It's somewhat like
             // `rustc_traits::normalize_after_erasing_regions`. Ideally, we'd
-            // like to normalize *before* inserting into `local_decls`, but I
-            // couldn't figure out where the heck that was.
+            // like to normalize *before* inserting into `local_decls`, but
+            // doing so ends up causing some other trouble.
             let b = match self
                 .infcx
                 .at(&ObligationCause::dummy(), ty::ParamEnv::empty())
diff --git a/compiler/rustc_trait_selection/src/traits/project.rs b/compiler/rustc_trait_selection/src/traits/project.rs
index df1dca9831a..91b9ad0af35 100644
--- a/compiler/rustc_trait_selection/src/traits/project.rs
+++ b/compiler/rustc_trait_selection/src/traits/project.rs
@@ -363,12 +363,28 @@ impl<'a, 'b, 'tcx> TypeFolder<'tcx> for AssocTypeNormalizer<'a, 'b, 'tcx> {
             return ty;
         }
 
-        // N.b. while we want to call `super_fold_with(self)` on `ty` before
-        // normalization, we wait until we know whether we need to normalize the
-        // current type. If we do, then we only fold the ty *after* replacing bound
-        // vars with placeholders. This means that nested types don't need to replace
-        // bound vars at the current binder level or above. A key assumption here is
-        // that folding the type can't introduce new bound vars.
+        // We try to be a little clever here as a performance optimization in
+        // cases where there are nested projections under binders.
+        // For example:
+        // ```
+        // for<'a> fn(<T as Foo>::One<'a, Box<dyn Bar<'a, Item=<T as Foo>::Two<'a>>>>)
+        // ```
+        // We normalize the substs on the projection before the projecting, but
+        // if we're naive, we'll
+        //   replace bound vars on inner, project inner, replace placeholders on inner,
+        //   replace bound vars on outer, project outer, replace placeholders on outer
+        //
+        // However, if we're a bit more clever, we can replace the bound vars
+        // on the entire type before normalizing nested projections, meaning we
+        //   replace bound vars on outer, project inner,
+        //   project outer, replace placeholders on outer
+        //
+        // This is possible because the inner `'a` will already be a placeholder
+        // when we need to normalize the inner projection
+        //
+        // On the other hand, this does add a bit of complexity, since we only
+        // replace bound vars if the current type is a `Projection` and we need
+        // to make sure we don't forget to fold the substs regardless.
 
         match *ty.kind() {
             ty::Opaque(def_id, substs) => {
@@ -380,7 +396,6 @@ impl<'a, 'b, 'tcx> TypeFolder<'tcx> for AssocTypeNormalizer<'a, 'b, 'tcx> {
                         // N.b. there is an assumption here all this code can handle
                         // escaping bound vars.
 
-                        let substs = substs.super_fold_with(self);
                         let recursion_limit = self.tcx().recursion_limit();
                         if !recursion_limit.value_within_limit(self.depth) {
                             let obligation = Obligation::with_depth(
@@ -392,6 +407,7 @@ impl<'a, 'b, 'tcx> TypeFolder<'tcx> for AssocTypeNormalizer<'a, 'b, 'tcx> {
                             self.selcx.infcx().report_overflow_error(&obligation, true);
                         }
 
+                        let substs = substs.super_fold_with(self);
                         let generic_ty = self.tcx().type_of(def_id);
                         let concrete_ty = generic_ty.subst(self.tcx(), substs);
                         self.depth += 1;
@@ -430,12 +446,16 @@ impl<'a, 'b, 'tcx> TypeFolder<'tcx> for AssocTypeNormalizer<'a, 'b, 'tcx> {
 
             ty::Projection(data) => {
                 // If there are escaping bound vars, we temporarily replace the
-                // bound vars with placeholders. Note though, that in the cas
+                // bound vars with placeholders. Note though, that in the case
                 // that we still can't project for whatever reason (e.g. self
                 // type isn't known enough), we *can't* register an obligation
                 // and return an inference variable (since then that obligation
                 // would have bound vars and that's a can of worms). Instead,
                 // we just give up and fall back to pretending like we never tried!
+                //
+                // Note: this isn't necessarily the final approach here; we may
+                // want to figure out how to register obligations with escaping vars
+                // or handle this some other way.
 
                 let infcx = self.selcx.infcx();
                 let (data, mapped_regions, mapped_types, mapped_consts) =
@@ -451,16 +471,18 @@ impl<'a, 'b, 'tcx> TypeFolder<'tcx> for AssocTypeNormalizer<'a, 'b, 'tcx> {
                 )
                 .ok()
                 .flatten()
+                .map(|normalized_ty| {
+                    PlaceholderReplacer::replace_placeholders(
+                        infcx,
+                        mapped_regions,
+                        mapped_types,
+                        mapped_consts,
+                        &self.universes,
+                        normalized_ty,
+                    )
+                })
                 .unwrap_or_else(|| ty.super_fold_with(self));
 
-                let normalized_ty = PlaceholderReplacer::replace_placeholders(
-                    infcx,
-                    mapped_regions,
-                    mapped_types,
-                    mapped_consts,
-                    &self.universes,
-                    normalized_ty,
-                );
                 debug!(
                     ?self.depth,
                     ?ty,
diff --git a/compiler/rustc_trait_selection/src/traits/query/normalize.rs b/compiler/rustc_trait_selection/src/traits/query/normalize.rs
index 8f8de24981b..21e1bd8f464 100644
--- a/compiler/rustc_trait_selection/src/traits/query/normalize.rs
+++ b/compiler/rustc_trait_selection/src/traits/query/normalize.rs
@@ -67,6 +67,16 @@ impl<'cx, 'tcx> AtExt<'tcx> for At<'cx, 'tcx> {
             universes: vec![],
         };
 
+        // This is actually a consequence by the way `normalize_erasing_regions` works currently.
+        // Because it needs to call the `normalize_generic_arg_after_erasing_regions`, it folds
+        // through tys and consts in a `TypeFoldable`. Importantly, it skips binders, leaving us
+        // with trying to normalize with escaping bound vars.
+        //
+        // Here, we just add the universes that we *would* have created had we passed through the binders.
+        //
+        // We *could* replace escaping bound vars eagerly here, but it doesn't seem really necessary.
+        // The rest of the code is already set up to be lazy about replacing bound vars,
+        // and only when we actually have to normalize.
         if value.has_escaping_bound_vars() {
             let mut max_visitor =
                 MaxEscapingBoundVarVisitor { outer_index: ty::INNERMOST, escaping: 0 };
@@ -183,12 +193,8 @@ impl<'cx, 'tcx> TypeFolder<'tcx> for QueryNormalizer<'cx, 'tcx> {
             return ty;
         }
 
-        // N.b. while we want to call `super_fold_with(self)` on `ty` before
-        // normalization, we wait until we know whether we need to normalize the
-        // current type. If we do, then we only fold the ty *after* replacing bound
-        // vars with placeholders. This means that nested types don't need to replace
-        // bound vars at the current binder level or above. A key assumption here is
-        // that folding the type can't introduce new bound vars.
+        // See note in `rustc_trait_selection::traits::project` about why we
+        // wait to fold the substs.
 
         // Wrap this in a closure so we don't accidentally return from the outer function
         let res = (|| match *ty.kind() {
@@ -253,7 +259,7 @@ impl<'cx, 'tcx> TypeFolder<'tcx> for QueryNormalizer<'cx, 'tcx> {
                         // We don't expect ambiguity.
                         if result.is_ambiguous() {
                             self.error = true;
-                            return ty;
+                            return ty.super_fold_with(self);
                         }
 
                         match self.infcx.instantiate_query_response_and_region_obligations(
@@ -271,14 +277,14 @@ impl<'cx, 'tcx> TypeFolder<'tcx> for QueryNormalizer<'cx, 'tcx> {
 
                             Err(_) => {
                                 self.error = true;
-                                ty
+                                ty.super_fold_with(self)
                             }
                         }
                     }
 
                     Err(NoSolution) => {
                         self.error = true;
-                        ty
+                        ty.super_fold_with(self)
                     }
                 }
             }
@@ -304,12 +310,12 @@ impl<'cx, 'tcx> TypeFolder<'tcx> for QueryNormalizer<'cx, 'tcx> {
                     .canonicalize_query_keep_static(self.param_env.and(data), &mut orig_values);
                 debug!("QueryNormalizer: c_data = {:#?}", c_data);
                 debug!("QueryNormalizer: orig_values = {:#?}", orig_values);
-                let normalized_ty = match tcx.normalize_projection_ty(c_data) {
+                match tcx.normalize_projection_ty(c_data) {
                     Ok(result) => {
                         // We don't expect ambiguity.
                         if result.is_ambiguous() {
                             self.error = true;
-                            return ty;
+                            return ty.super_fold_with(self);
                         }
                         match self.infcx.instantiate_query_response_and_region_obligations(
                             self.cause,
@@ -321,27 +327,26 @@ impl<'cx, 'tcx> TypeFolder<'tcx> for QueryNormalizer<'cx, 'tcx> {
                                 debug!("QueryNormalizer: result = {:#?}", result);
                                 debug!("QueryNormalizer: obligations = {:#?}", obligations);
                                 self.obligations.extend(obligations);
-                                result.normalized_ty
+                                crate::traits::project::PlaceholderReplacer::replace_placeholders(
+                                    infcx,
+                                    mapped_regions,
+                                    mapped_types,
+                                    mapped_consts,
+                                    &self.universes,
+                                    result.normalized_ty,
+                                )
                             }
                             Err(_) => {
                                 self.error = true;
-                                ty
+                                ty.super_fold_with(self)
                             }
                         }
                     }
                     Err(NoSolution) => {
                         self.error = true;
-                        ty
+                        ty.super_fold_with(self)
                     }
-                };
-                crate::traits::project::PlaceholderReplacer::replace_placeholders(
-                    infcx,
-                    mapped_regions,
-                    mapped_types,
-                    mapped_consts,
-                    &self.universes,
-                    normalized_ty,
-                )
+                }
             }
 
             _ => ty.super_fold_with(self),