Auto merge of rust-lang#69707 - estebank:impl-trait-missing-bounds, r=Centril

Handle `impl Trait` where `Trait` has an assoc type with missing bounds

When encountering a type parameter that needs more bounds the trivial case is `T` `where T: Bound`, but it can also be an `impl Trait` param that needs to be decomposed to a type param for cleaner code. For example, given

```rust
fn foo(constraints: impl Iterator) {
    for constraint in constraints {
        println!("{:?}", constraint);
    }
}
```

the previous output was

```
error[E0277]: `<impl Iterator as std::iter::Iterator>::Item` doesn't implement `std::fmt::Debug`
 --> src/main.rs:3:26
  |
1 | fn foo(constraints: impl Iterator) {
  |                                    - help: consider further restricting the associated type: `where <impl Iterator as std::iter::Iterator>::Item: std::fmt::Debug`
2 |     for constraint in constraints {
3 |         println!("{:?}", constraint);
  |                          ^^^^^^^^^^ `<impl Iterator as std::iter::Iterator>::Item` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug`
  |
  = help: the trait `std::fmt::Debug` is not implemented for `<impl Iterator as std::iter::Iterator>::Item`
  = note: required by `std::fmt::Debug::fmt`
```

which is incorrect as `where <impl Iterator as std::iter::Iterator>::Item: std::fmt::Debug` is not valid syntax nor would it restrict the positional `impl Iterator` parameter if it were.

The output being introduced is

```
error[E0277]: `<impl Iterator as std::iter::Iterator>::Item` doesn't implement `std::fmt::Debug`
 --> src/main.rs:3:26
  |
3 |         println!("{:?}", constraint);
  |                          ^^^^^^^^^^ `<impl Iterator as std::iter::Iterator>::Item` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug`
  |
  = help: the trait `std::fmt::Debug` is not implemented for `<impl Iterator as std::iter::Iterator>::Item`
  = note: required by `std::fmt::Debug::fmt`
help: introduce a type parameter with a trait bound instead of using `impl Trait`
   |
LL | fn foo<T: Iterator>(constraints: T) where <T as std::iter::Iterator>::Item: std::fmt::Debug  {
   |       ^^^^^^^^^^^^^              ^  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```

This suggestion is correct and lead the user in the right direction: because you have an associated type restriction you can no longer use `impl Trait`, the only reasonable alternative is to introduce a named type parameter, bound by `Trait` and with a `where` binding on the associated type for the new type parameter `as Trait` for the missing bound.

*Ideally*, we would want to suggest something like the following, but that is not valid syntax today

```
error[E0277]: `<impl Iterator as std::iter::Iterator>::Item` doesn't implement `std::fmt::Debug`
 --> src/main.rs:3:26
  |
3 |         println!("{:?}", constraint);
  |                          ^^^^^^^^^^ `<impl Iterator as std::iter::Iterator>::Item` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug`
  |
  = help: the trait `std::fmt::Debug` is not implemented for `<impl Iterator as std::iter::Iterator>::Item`
  = note: required by `std::fmt::Debug::fmt`
help: introduce a type parameter with a trait bound instead of using `impl Trait`
   |
LL | fn foo(constraints: impl Iterator<Item: std::fmt::Debug>) {
   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```

Fix rust-lang#69638.

Author: bors

src/librustc_trait_selection/lib.rs

@@ -16,6 +16,8 @@
 #![feature(in_band_lifetimes)]
 #![feature(crate_visibility_modifier)]
 #![feature(or_patterns)]
+#![feature(str_strip)]
+#![feature(option_zip)]
 #![recursion_limit = "512"] // For rustdoc
 
 #[macro_use]

src/librustc_trait_selection/traits/error_reporting/mod.rs

@@ -388,7 +388,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
                             // which is somewhat confusing.
                             self.suggest_restricting_param_bound(
                                 &mut err,
-                                &trait_ref,
+                                trait_ref,
                                 obligation.cause.body_id,
                             );
                         } else {

src/librustc_trait_selection/traits/error_reporting/suggestions.rs

@@ -15,7 +15,7 @@ use rustc_middle::ty::TypeckTables;
 use rustc_middle::ty::{
     self, AdtKind, DefIdTree, ToPredicate, Ty, TyCtxt, TypeFoldable, WithConstness,
 };
-use rustc_span::symbol::{kw, sym};
+use rustc_span::symbol::{kw, sym, Symbol};
 use rustc_span::{MultiSpan, Span, DUMMY_SP};
 use std::fmt;
 
@@ -27,7 +27,7 @@ pub trait InferCtxtExt<'tcx> {
     fn suggest_restricting_param_bound(
         &self,
         err: &mut DiagnosticBuilder<'_>,
-        trait_ref: &ty::PolyTraitRef<'_>,
+        trait_ref: ty::PolyTraitRef<'_>,
         body_id: hir::HirId,
     );
 
@@ -148,11 +148,128 @@ pub trait InferCtxtExt<'tcx> {
     fn suggest_new_overflow_limit(&self, err: &mut DiagnosticBuilder<'_>);
 }
 
+fn predicate_constraint(generics: &hir::Generics<'_>, pred: String) -> (Span, String) {
+    (
+        generics.where_clause.span_for_predicates_or_empty_place().shrink_to_hi(),
+        format!(
+            "{} {} ",
+            if !generics.where_clause.predicates.is_empty() { "," } else { " where" },
+            pred,
+        ),
+    )
+}
+
+/// Type parameter needs more bounds. The trivial case is `T` `where T: Bound`, but
+/// it can also be an `impl Trait` param that needs to be decomposed to a type
+/// param for cleaner code.
+fn suggest_restriction(
+    generics: &hir::Generics<'_>,
+    msg: &str,
+    err: &mut DiagnosticBuilder<'_>,
+    fn_sig: Option<&hir::FnSig<'_>>,
+    projection: Option<&ty::ProjectionTy<'_>>,
+    trait_ref: ty::PolyTraitRef<'_>,
+) {
+    let span = generics.where_clause.span_for_predicates_or_empty_place();
+    if span.from_expansion() || span.desugaring_kind().is_some() {
+        return;
+    }
+    // Given `fn foo(t: impl Trait)` where `Trait` requires assoc type `A`...
+    if let Some((bound_str, fn_sig)) =
+        fn_sig.zip(projection).and_then(|(sig, p)| match p.self_ty().kind {
+            // Shenanigans to get the `Trait` from the `impl Trait`.
+            ty::Param(param) => {
+                // `fn foo(t: impl Trait)`
+                //                 ^^^^^ get this string
+                param.name.as_str().strip_prefix("impl").map(|s| (s.trim_start().to_string(), sig))
+            }
+            _ => None,
+        })
+    {
+        // We know we have an `impl Trait` that doesn't satisfy a required projection.
+
+        // Find all of the ocurrences of `impl Trait` for `Trait` in the function arguments'
+        // types. There should be at least one, but there might be *more* than one. In that
+        // case we could just ignore it and try to identify which one needs the restriction,
+        // but instead we choose to suggest replacing all instances of `impl Trait` with `T`
+        // where `T: Trait`.
+        let mut ty_spans = vec![];
+        let impl_trait_str = format!("impl {}", bound_str);
+        for input in fn_sig.decl.inputs {
+            if let hir::TyKind::Path(hir::QPath::Resolved(
+                None,
+                hir::Path { segments: [segment], .. },
+            )) = input.kind
+            {
+                if segment.ident.as_str() == impl_trait_str.as_str() {
+                    // `fn foo(t: impl Trait)`
+                    //            ^^^^^^^^^^ get this to suggest `T` instead
+
+                    // There might be more than one `impl Trait`.
+                    ty_spans.push(input.span);
+                }
+            }
+        }
+
+        let type_param_name = generics.params.next_type_param_name(Some(&bound_str));
+        // The type param `T: Trait` we will suggest to introduce.
+        let type_param = format!("{}: {}", type_param_name, bound_str);
+
+        // FIXME: modify the `trait_ref` instead of string shenanigans.
+        // Turn `<impl Trait as Foo>::Bar: Qux` into `<T as Foo>::Bar: Qux`.
+        let pred = trait_ref.without_const().to_predicate().to_string();
+        let pred = pred.replace(&impl_trait_str, &type_param_name);
+        let mut sugg = vec![
+            match generics
+                .params
+                .iter()
+                .filter(|p| match p.kind {
+                    hir::GenericParamKind::Type {
+                        synthetic: Some(hir::SyntheticTyParamKind::ImplTrait),
+                        ..
+                    } => false,
+                    _ => true,
+                })
+                .last()
+            {
+                // `fn foo(t: impl Trait)`
+                //        ^ suggest `<T: Trait>` here
+                None => (generics.span, format!("<{}>", type_param)),
+                // `fn foo<A>(t: impl Trait)`
+                //        ^^^ suggest `<A, T: Trait>` here
+                Some(param) => (
+                    param.bounds_span().unwrap_or(param.span).shrink_to_hi(),
+                    format!(", {}", type_param),
+                ),
+            },
+            // `fn foo(t: impl Trait)`
+            //                       ^ suggest `where <T as Trait>::A: Bound`
+            predicate_constraint(generics, pred),
+        ];
+        sugg.extend(ty_spans.into_iter().map(|s| (s, type_param_name.to_string())));
+
+        // Suggest `fn foo<T: Trait>(t: T) where <T as Trait>::A: Bound`.
+        // FIXME: once `#![feature(associated_type_bounds)]` is stabilized, we should suggest
+        // `fn foo(t: impl Trait<A: Bound>)` instead.
+        err.multipart_suggestion(
+            "introduce a type parameter with a trait bound instead of using `impl Trait`",
+            sugg,
+            Applicability::MaybeIncorrect,
+        );
+    } else {
+        // Trivial case: `T` needs an extra bound: `T: Bound`.
+        let (sp, sugg) =
+            predicate_constraint(generics, trait_ref.without_const().to_predicate().to_string());
+        let appl = Applicability::MachineApplicable;
+        err.span_suggestion(sp, &format!("consider further restricting {}", msg), sugg, appl);
+    }
+}
+
 impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
     fn suggest_restricting_param_bound(
         &self,
         mut err: &mut DiagnosticBuilder<'_>,
-        trait_ref: &ty::PolyTraitRef<'_>,
+        trait_ref: ty::PolyTraitRef<'_>,
         body_id: hir::HirId,
     ) {
         let self_ty = trait_ref.self_ty();
@@ -162,27 +279,6 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
             _ => return,
         };
 
-        let suggest_restriction =
-            |generics: &hir::Generics<'_>, msg, err: &mut DiagnosticBuilder<'_>| {
-                let span = generics.where_clause.span_for_predicates_or_empty_place();
-                if !span.from_expansion() && span.desugaring_kind().is_none() {
-                    err.span_suggestion(
-                        generics.where_clause.span_for_predicates_or_empty_place().shrink_to_hi(),
-                        &format!("consider further restricting {}", msg),
-                        format!(
-                            "{} {} ",
-                            if !generics.where_clause.predicates.is_empty() {
-                                ","
-                            } else {
-                                " where"
-                            },
-                            trait_ref.without_const().to_predicate(),
-                        ),
-                        Applicability::MachineApplicable,
-                    );
-                }
-            };
-
         // FIXME: Add check for trait bound that is already present, particularly `?Sized` so we
         //        don't suggest `T: Sized + ?Sized`.
         let mut hir_id = body_id;
@@ -194,27 +290,47 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
                     ..
                 }) if param_ty && self_ty == self.tcx.types.self_param => {
                     // Restricting `Self` for a single method.
-                    suggest_restriction(&generics, "`Self`", err);
+                    suggest_restriction(&generics, "`Self`", err, None, projection, trait_ref);
                     return;
                 }
 
                 hir::Node::TraitItem(hir::TraitItem {
                     generics,
-                    kind: hir::TraitItemKind::Fn(..),
+                    kind: hir::TraitItemKind::Fn(fn_sig, ..),
                     ..
                 })
                 | hir::Node::ImplItem(hir::ImplItem {
                     generics,
-                    kind: hir::ImplItemKind::Fn(..),
+                    kind: hir::ImplItemKind::Fn(fn_sig, ..),
                     ..
                 })
-                | hir::Node::Item(
-                    hir::Item { kind: hir::ItemKind::Fn(_, generics, _), .. }
-                    | hir::Item { kind: hir::ItemKind::Trait(_, _, generics, _, _), .. }
+                | hir::Node::Item(hir::Item {
+                    kind: hir::ItemKind::Fn(fn_sig, generics, _), ..
+                }) if projection.is_some() => {
+                    // Missing restriction on associated type of type parameter (unmet projection).
+                    suggest_restriction(
+                        &generics,
+                        "the associated type",
+                        err,
+                        Some(fn_sig),
+                        projection,
+                        trait_ref,
+                    );
+                    return;
+                }
+                hir::Node::Item(
+                    hir::Item { kind: hir::ItemKind::Trait(_, _, generics, _, _), .. }
                     | hir::Item { kind: hir::ItemKind::Impl { generics, .. }, .. },
                 ) if projection.is_some() => {
-                    // Missing associated type bound.
-                    suggest_restriction(&generics, "the associated type", err);
+                    // Missing restriction on associated type of type parameter (unmet projection).
+                    suggest_restriction(
+                        &generics,
+                        "the associated type",
+                        err,
+                        None,
+                        projection,
+                        trait_ref,
+                    );
                     return;
                 }
 
@@ -1588,3 +1704,29 @@ impl<'v> Visitor<'v> for ReturnsVisitor<'v> {
         hir::intravisit::walk_body(self, body);
     }
 }
+
+pub trait NextTypeParamName {
+    fn next_type_param_name(&self, name: Option<&str>) -> String;
+}
+
+impl NextTypeParamName for &[hir::GenericParam<'_>] {
+    fn next_type_param_name(&self, name: Option<&str>) -> String {
+        // This is the whitelist of possible parameter names that we might suggest.
+        let name = name.and_then(|n| n.chars().next()).map(|c| c.to_string().to_uppercase());
+        let name = name.as_ref().map(|s| s.as_str());
+        let possible_names = [name.unwrap_or("T"), "T", "U", "V", "X", "Y", "Z", "A", "B", "C"];
+        let used_names = self
+            .iter()
+            .filter_map(|p| match p.name {
+                hir::ParamName::Plain(ident) => Some(ident.name),
+                _ => None,
+            })
+            .collect::<Vec<_>>();
+
+        possible_names
+            .iter()
+            .find(|n| !used_names.contains(&Symbol::intern(n)))
+            .unwrap_or(&"ParamName")
+            .to_string()
+    }
+}

src/librustc_typeck/collect.rs

@@ -45,6 +45,7 @@ use rustc_session::parse::feature_err;
 use rustc_span::symbol::{kw, sym, Symbol};
 use rustc_span::{Span, DUMMY_SP};
 use rustc_target::spec::abi;
+use rustc_trait_selection::traits::error_reporting::suggestions::NextTypeParamName;
 
 mod type_of;
 
@@ -135,20 +136,7 @@ crate fn placeholder_type_error(
     if placeholder_types.is_empty() {
         return;
     }
-    // This is the whitelist of possible parameter names that we might suggest.
-    let possible_names = ["T", "K", "L", "A", "B", "C"];
-    let used_names = generics
-        .iter()
-        .filter_map(|p| match p.name {
-            hir::ParamName::Plain(ident) => Some(ident.name),
-            _ => None,
-        })
-        .collect::<Vec<_>>();
-
-    let type_name = possible_names
-        .iter()
-        .find(|n| !used_names.contains(&Symbol::intern(n)))
-        .unwrap_or(&"ParamName");
+    let type_name = generics.next_type_param_name(None);
 
     let mut sugg: Vec<_> =
         placeholder_types.iter().map(|sp| (*sp, (*type_name).to_string())).collect();

src/test/ui/suggestions/impl-trait-with-missing-bounds.rs

@@ -0,0 +1,44 @@
+// The double space in `impl  Iterator` is load bearing! We want to make sure we don't regress by
+// accident if the internal string representation changes.
+#[rustfmt::skip]
+fn foo(constraints: impl  Iterator) {
+    for constraint in constraints {
+        qux(constraint);
+//~^ ERROR `<impl Iterator as std::iter::Iterator>::Item` doesn't implement `std::fmt::Debug`
+    }
+}
+
+fn bar<T>(t: T, constraints: impl Iterator) where T: std::fmt::Debug {
+    for constraint in constraints {
+        qux(t);
+        qux(constraint);
+//~^ ERROR `<impl Iterator as std::iter::Iterator>::Item` doesn't implement `std::fmt::Debug`
+    }
+}
+
+fn baz(t: impl std::fmt::Debug, constraints: impl Iterator) {
+    for constraint in constraints {
+        qux(t);
+        qux(constraint);
+//~^ ERROR `<impl Iterator as std::iter::Iterator>::Item` doesn't implement `std::fmt::Debug`
+    }
+}
+
+fn bat<I, T: std::fmt::Debug>(t: T, constraints: impl Iterator, _: I) {
+    for constraint in constraints {
+        qux(t);
+        qux(constraint);
+//~^ ERROR `<impl Iterator as std::iter::Iterator>::Item` doesn't implement `std::fmt::Debug`
+    }
+}
+
+fn bak(constraints: impl  Iterator + std::fmt::Debug) {
+    for constraint in constraints {
+        qux(constraint);
+//~^ ERROR `<impl Iterator + std::fmt::Debug as std::iter::Iterator>::Item` doesn't implement
+    }
+}
+
+fn qux(_: impl std::fmt::Debug) {}
+
+fn main() {}