Cleanup: move code to their own methods and deduplicate actions

Author: estebank

src/librustc_typeck/astconv.rs

@@ -783,6 +783,8 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
         (substs, assoc_bindings, potential_assoc_types)
     }
 
+    /// On missing type parameters, emit an E0393 error and provide a structured suggestion using
+    /// the type parameter's name as a placeholder.
     fn complain_about_missing_type_params(
         &self,
         missing_type_params: Vec<String>,
@@ -956,8 +958,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
         trait_def_id: DefId,
         self_ty: Ty<'tcx>,
         trait_segment: &hir::PathSegment
-    ) -> ty::TraitRef<'tcx>
-    {
+    ) -> ty::TraitRef<'tcx> {
         let (substs, assoc_bindings, _) =
             self.create_substs_for_ast_trait_ref(span,
                                                  trait_def_id,
@@ -967,15 +968,14 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
         ty::TraitRef::new(trait_def_id, substs)
     }
 
-    fn create_substs_for_ast_trait_ref<'a>(
+    /// When the code is using the `Fn` traits directly, instead of the `Fn(A) -> B` syntax, emit
+    /// an error and attempt to build a reasonable structured suggestion.
+    fn complain_about_internal_fn_trait(
         &self,
         span: Span,
         trait_def_id: DefId,
-        self_ty: Ty<'tcx>,
         trait_segment: &'a hir::PathSegment,
-    ) -> (SubstsRef<'tcx>, Vec<ConvertedBinding<'a, 'tcx>>, Option<Vec<Span>>) {
-        debug!("create_substs_for_ast_trait_ref(trait_segment={:?})", trait_segment);
-
+    ) {
         let trait_def = self.tcx().trait_def(trait_def_id);
 
         if !self.tcx().features().unboxed_closures &&
@@ -1020,19 +1020,33 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
             }
             err.emit();
         }
+    }
+
+    fn create_substs_for_ast_trait_ref<'a>(
+        &self,
+        span: Span,
+        trait_def_id: DefId,
+        self_ty: Ty<'tcx>,
+        trait_segment: &'a hir::PathSegment,
+    ) -> (SubstsRef<'tcx>, Vec<ConvertedBinding<'a, 'tcx>>, Option<Vec<Span>>) {
+        debug!("create_substs_for_ast_trait_ref(trait_segment={:?})", trait_segment);
+
+        self.complain_about_internal_fn_trait(span, trait_def_id, trait_segment);
 
-        self.create_substs_for_ast_path(span,
-                                        trait_def_id,
-                                        trait_segment.generic_args(),
-                                        trait_segment.infer_args,
-                                        Some(self_ty))
+        self.create_substs_for_ast_path(
+            span,
+            trait_def_id,
+            trait_segment.generic_args(),
+            trait_segment.infer_args,
+            Some(self_ty),
+        )
     }
 
-    fn trait_defines_associated_type_named(&self,
-                                           trait_def_id: DefId,
-                                           assoc_name: ast::Ident)
-                                           -> bool
-    {
+    fn trait_defines_associated_type_named(
+        &self,
+        trait_def_id: DefId,
+        assoc_name: ast::Ident,
+    ) -> bool {
         self.tcx().associated_items(trait_def_id).any(|item| {
             item.kind == ty::AssocKind::Type &&
             self.tcx().hygienic_eq(assoc_name, item.ident, trait_def_id)
@@ -1400,48 +1414,49 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
             .filter(|(trait_ref, _)| !tcx.trait_is_auto(trait_ref.def_id()));
 
         for (base_trait_ref, span) in regular_traits_refs_spans {
-            debug!("conv_object_ty_poly_trait_ref regular_trait_ref `{:?}`", base_trait_ref);
-            let mut new_bounds = vec![];
             for trait_ref in traits::elaborate_trait_ref(tcx, base_trait_ref) {
-            debug!("conv_object_ty_poly_trait_ref: observing object predicate `{:?}`", trait_ref);
-            match trait_ref {
-                ty::Predicate::Trait(pred) => {
-                    associated_types.entry(span).or_default()
-                        .extend(tcx.associated_items(pred.def_id())
-                            .filter(|item| item.kind == ty::AssocKind::Type)
-                            .map(|item| item.def_id));
-                }
-                ty::Predicate::Projection(pred) => {
-                    // A `Self` within the original bound will be substituted with a
-                    // `trait_object_dummy_self`, so check for that.
-                    let references_self =
-                        pred.skip_binder().ty.walk().any(|t| t == dummy_self);
-
-                    // If the projection output contains `Self`, force the user to
-                    // elaborate it explicitly to avoid a lot of complexity.
-                    //
-                    // The "classicaly useful" case is the following:
-                    // ```
-                    //     trait MyTrait: FnMut() -> <Self as MyTrait>::MyOutput {
-                    //         type MyOutput;
-                    //     }
-                    // ```
-                    //
-                    // Here, the user could theoretically write `dyn MyTrait<Output = X>`,
-                    // but actually supporting that would "expand" to an infinitely-long type
-                    // `fix $ τ → dyn MyTrait<MyOutput = X, Output = <τ as MyTrait>::MyOutput`.
-                    //
-                    // Instead, we force the user to write `dyn MyTrait<MyOutput = X, Output = X>`,
-                    // which is uglier but works. See the discussion in #56288 for alternatives.
-                    if !references_self {
-                        // Include projections defined on supertraits.
-                        new_bounds.push((pred, span));
+                debug!(
+                    "conv_object_ty_poly_trait_ref: observing object predicate `{:?}`",
+                    trait_ref
+                );
+                match trait_ref {
+                    ty::Predicate::Trait(pred) => {
+                        associated_types.entry(span).or_default()
+                            .extend(tcx.associated_items(pred.def_id())
+                                .filter(|item| item.kind == ty::AssocKind::Type)
+                                .map(|item| item.def_id));
+                    }
+                    ty::Predicate::Projection(pred) => {
+                        // A `Self` within the original bound will be substituted with a
+                        // `trait_object_dummy_self`, so check for that.
+                        let references_self =
+                            pred.skip_binder().ty.walk().any(|t| t == dummy_self);
+
+                        // If the projection output contains `Self`, force the user to
+                        // elaborate it explicitly to avoid a lot of complexity.
+                        //
+                        // The "classicaly useful" case is the following:
+                        // ```
+                        //     trait MyTrait: FnMut() -> <Self as MyTrait>::MyOutput {
+                        //         type MyOutput;
+                        //     }
+                        // ```
+                        //
+                        // Here, the user could theoretically write `dyn MyTrait<Output = X>`,
+                        // but actually supporting that would "expand" to an infinitely-long type
+                        // `fix $ τ → dyn MyTrait<MyOutput = X, Output = <τ as MyTrait>::MyOutput`.
+                        //
+                        // Instead, we force the user to write
+                        // `dyn MyTrait<MyOutput = X, Output = X>`, which is uglier but works. See
+                        // the discussion in #56288 for alternatives.
+                        if !references_self {
+                            // Include projections defined on supertraits.
+                            bounds.projection_bounds.push((pred, span));
+                        }
                     }
+                    _ => ()
                 }
-                _ => ()
-            }
             }
-            bounds.projection_bounds.extend(new_bounds);
         }
 
         for (projection_bound, _) in &bounds.projection_bounds {
@@ -1534,27 +1549,37 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
         ty
     }
 
+    /// When there are any missing associated types, emit an E0191 error and attempt to supply a
+    /// reasonable suggestion on how to write it. For the case of multiple associated types in the
+    /// same trait bound have the same name (as they come from different super-traits), we instead
+    /// emit a generic note suggesting using a `where` clause to constraint instead.
     fn complain_about_missing_associated_types(
         &self,
-        mut associated_types: FxHashMap<Span, BTreeSet<DefId>>,
+        associated_types: FxHashMap<Span, BTreeSet<DefId>>,
         potential_assoc_types: Vec<Span>,
         trait_bounds: &[hir::PolyTraitRef],
     ) {
         if !associated_types.values().any(|v| v.len() > 0) {
             return;
         }
         let tcx = self.tcx();
+        // FIXME: Marked `mut` so that we can replace the spans further below with a more
+        // appropriate one, but this should be handled earlier in the span assignment.
+        let mut associated_types: FxHashMap<Span, Vec<_>> = associated_types.into_iter()
+            .map(|(span, def_ids)| (
+                span,
+                def_ids.into_iter().map(|did| tcx.associated_item(did)).collect(),
+            )).collect();
         let mut names = vec![];
 
         // Account for things like `dyn Foo + 'a`, like in tests `issue-22434.rs` and
         // `issue-22560.rs`.
         let mut trait_bound_spans: Vec<Span> = vec![];
-        for (span, item_def_ids) in &associated_types {
-            if !item_def_ids.is_empty() {
+        for (span, items) in &associated_types {
+            if !items.is_empty() {
                 trait_bound_spans.push(*span);
             }
-            for item_def_id in item_def_ids {
-                let assoc_item = tcx.associated_item(*item_def_id);
+            for assoc_item in items {
                 let trait_def_id = assoc_item.container.id();
                 names.push(format!(
                     "`{}` (from trait `{}`)",
@@ -1592,7 +1617,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
                 [segment] if segment.args.is_none() => {
                     trait_bound_spans = vec![segment.ident.span];
                     associated_types = associated_types.into_iter()
-                        .map(|(_, defs)| (segment.ident.span, defs))
+                        .map(|(_, items)| (segment.ident.span, items))
                         .collect();
                 }
                 _ => {}
@@ -1610,17 +1635,14 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
         let mut suggestions = vec![];
         let mut types_count = 0;
         let mut where_constraints = vec![];
-        for (span, def_ids) in &associated_types {
-            let assoc_items: Vec<_> = def_ids.iter()
-                .map(|def_id| tcx.associated_item(*def_id))
-                .collect();
+        for (span, assoc_items) in &associated_types {
             let mut names: FxHashMap<_, usize> = FxHashMap::default();
-            for item in &assoc_items {
+            for item in assoc_items {
                 types_count += 1;
                 *names.entry(item.ident.name).or_insert(0) += 1;
             }
             let mut dupes = false;
-            for item in &assoc_items {
+            for item in assoc_items {
                 let prefix = if names[&item.ident.name] > 1 {
                     let trait_def_id = item.container.id();
                     dupes = true;
@@ -1681,17 +1703,14 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
         }
         if suggestions.len() != 1 {
             // We don't need this label if there's an inline suggestion, show otherwise.
-            for (span, def_ids) in &associated_types {
-                let assoc_items: Vec<_> = def_ids.iter()
-                    .map(|def_id| tcx.associated_item(*def_id))
-                    .collect();
+            for (span, assoc_items) in &associated_types {
                 let mut names: FxHashMap<_, usize> = FxHashMap::default();
-                for item in &assoc_items {
+                for item in assoc_items {
                     types_count += 1;
                     *names.entry(item.ident.name).or_insert(0) += 1;
                 }
                 let mut label = vec![];
-                for item in &assoc_items {
+                for item in assoc_items {
                     let postfix = if names[&item.ident.name] > 1 {
                         let trait_def_id = item.container.id();
                         format!(" (from trait `{}`)", tcx.def_path_str(trait_def_id))

src/test/ui/associated-types/missing-associated-types.stderr

@@ -38,8 +38,8 @@ LL | type Bar<Rhs> = dyn Add<Rhs> + Sub<Rhs> + X<Rhs> + Z<Rhs>;
    |                     first non-auto trait
    |                     trait alias used in trait object type (first use)
 
-error[E0191]: the value of the associated types `Output` (from trait `std::ops::Add`), `Output` (from trait `std::ops::Sub`), `A` (from trait `Z`), `B` (from trait `Z`), `Output` (from trait `std::ops::Div`), `Output` (from trait `std::ops::Mul`), `Output` (from trait `std::ops::Div`) must be specified
-  --> $DIR/missing-associated-types.rs:15:21
+error[E0191]: the value of the associated types `Output` (from trait `std::ops::Mul`), `Output` (from trait `std::ops::Div`), `Output` (from trait `std::ops::Sub`), `A` (from trait `Z`), `B` (from trait `Z`), `Output` (from trait `std::ops::Div`), `Output` (from trait `std::ops::Add`) must be specified
+  --> $DIR/missing-associated-types.rs:15:43
    |
 LL |     type A;
    |     ------- `A` defined here