From 9d83cc8331adb0098bad1d6f1863b0f612c6f2c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Wed, 4 Mar 2020 10:50:05 -0800 Subject: [PATCH 1/9] Handle `impl Trait` where `Trait` has an assoc type with missing bounds Fix #69638. --- .../traits/error_reporting/suggestions.rs | 152 +++++++++++++++--- .../impl-trait-with-missing-bounds.rs | 29 ++++ .../impl-trait-with-missing-bounds.stderr | 48 ++++++ 3 files changed, 208 insertions(+), 21 deletions(-) create mode 100644 src/test/ui/suggestions/impl-trait-with-missing-bounds.rs create mode 100644 src/test/ui/suggestions/impl-trait-with-missing-bounds.stderr diff --git a/src/librustc_trait_selection/traits/error_reporting/suggestions.rs b/src/librustc_trait_selection/traits/error_reporting/suggestions.rs index 3edc066d49a..ca2d0a8c3ed 100644 --- a/src/librustc_trait_selection/traits/error_reporting/suggestions.rs +++ b/src/librustc_trait_selection/traits/error_reporting/suggestions.rs @@ -163,23 +163,123 @@ fn suggest_restricting_param_bound( }; let suggest_restriction = - |generics: &hir::Generics<'_>, msg, err: &mut DiagnosticBuilder<'_>| { + |generics: &hir::Generics<'_>, + msg, + err: &mut DiagnosticBuilder<'_>, + fn_sig: Option<&hir::FnSig<'_>>| { + // 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. 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" + // Given `fn foo(t: impl Trait)` where `Trait` requires assoc type `A`... + if let Some((name, fn_sig)) = fn_sig.and_then(|sig| { + projection.and_then(|p| { + // Shenanigans to get the `Trait` from the `impl Trait`. + match p.self_ty().kind { + ty::Param(param) if param.name.as_str().starts_with("impl ") => { + let n = param.name.as_str(); + // `fn foo(t: impl Trait)` + // ^^^^^ get this string + n.split_whitespace() + .skip(1) + .next() + .map(|n| (n.to_string(), sig)) + } + _ => None, + } + }) + }) { + // FIXME: Cleanup. + let mut ty_spans = vec![]; + let impl_name = format!("impl {}", name); + for i in fn_sig.decl.inputs { + if let hir::TyKind::Path(hir::QPath::Resolved(None, path)) = i.kind { + match path.segments { + [segment] if segment.ident.to_string() == impl_name => { + // `fn foo(t: impl Trait)` + // ^^^^^^^^^^ get this to suggest + // `T` instead + + // There might be more than one `impl Trait`. + ty_spans.push(i.span); + } + _ => {} + } + } + } + + let type_param = format!("{}: {}", "T", name); + // FIXME: modify the `trait_ref` instead of string shenanigans. + // Turn `::Bar: Qux` into `::Bar: Qux`. + let pred = trait_ref.without_const().to_predicate().to_string(); + let pred = pred.replace(&impl_name, "T"); + 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 `` here + None => (generics.span, format!("<{}>", type_param)), + Some(param) => { + (param.span.shrink_to_hi(), format!(", {}", type_param)) + } }, - trait_ref.without_const().to_predicate(), - ), - Applicability::MachineApplicable, - ); + ( + // `fn foo(t: impl Trait)` + // ^ suggest `where ::A: Bound` + generics + .where_clause + .span_for_predicates_or_empty_place() + .shrink_to_hi(), + format!( + "{} {} ", + if !generics.where_clause.predicates.is_empty() { + "," + } else { + " where" + }, + pred, + ), + ), + ]; + sugg.extend(ty_spans.into_iter().map(|s| (s, "T".to_string()))); + // Suggest `fn foo(t: T) where ::A: Bound`. + 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. + 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, + ); + } } }; @@ -187,6 +287,10 @@ fn suggest_restricting_param_bound( // don't suggest `T: Sized + ?Sized`. let mut hir_id = body_id; while let Some(node) = self.tcx.hir().find(hir_id) { + debug!( + "suggest_restricting_param_bound {:?} {:?} {:?} {:?}", + trait_ref, self_ty.kind, projection, node + ); match node { hir::Node::TraitItem(hir::TraitItem { generics, @@ -194,27 +298,33 @@ fn suggest_restricting_param_bound( .. }) 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); 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 associated type bound. + suggest_restriction(&generics, "the associated type", err, Some(fn_sig)); + 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); + suggest_restriction(&generics, "the associated type", err, None); return; } diff --git a/src/test/ui/suggestions/impl-trait-with-missing-bounds.rs b/src/test/ui/suggestions/impl-trait-with-missing-bounds.rs new file mode 100644 index 00000000000..bef9ba9f91f --- /dev/null +++ b/src/test/ui/suggestions/impl-trait-with-missing-bounds.rs @@ -0,0 +1,29 @@ +// 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 `::Item` doesn't implement `std::fmt::Debug` + } +} + +fn bar(t: T, constraints: impl Iterator) where T: std::fmt::Debug { + for constraint in constraints { + qux(t); + qux(constraint); +//~^ ERROR `::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 `::Item` doesn't implement `std::fmt::Debug` + } +} + +fn qux(_: impl std::fmt::Debug) {} + +fn main() {} diff --git a/src/test/ui/suggestions/impl-trait-with-missing-bounds.stderr b/src/test/ui/suggestions/impl-trait-with-missing-bounds.stderr new file mode 100644 index 00000000000..5f84e6d44ea --- /dev/null +++ b/src/test/ui/suggestions/impl-trait-with-missing-bounds.stderr @@ -0,0 +1,48 @@ +error[E0277]: `::Item` doesn't implement `std::fmt::Debug` + --> $DIR/impl-trait-with-missing-bounds.rs:6:13 + | +LL | qux(constraint); + | ^^^^^^^^^^ `::Item` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug` +... +LL | fn qux(_: impl std::fmt::Debug) {} + | --- --------------- required by this bound in `qux` + | + = help: the trait `std::fmt::Debug` is not implemented for `::Item` +help: introduce a type parameter with a trait bound instead of using `impl Trait` + | +LL | fn foo(constraints: T) where ::Item: std::fmt::Debug { + | ^^^^^^^^^^^^^ ^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error[E0277]: `::Item` doesn't implement `std::fmt::Debug` + --> $DIR/impl-trait-with-missing-bounds.rs:14:13 + | +LL | qux(constraint); + | ^^^^^^^^^^ `::Item` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug` +... +LL | fn qux(_: impl std::fmt::Debug) {} + | --- --------------- required by this bound in `qux` + | + = help: the trait `std::fmt::Debug` is not implemented for `::Item` +help: introduce a type parameter with a trait bound instead of using `impl Trait` + | +LL | fn bar(t: T, constraints: T) where T: std::fmt::Debug, ::Item: std::fmt::Debug { + | ^^^^^^^^^^^^^ ^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error[E0277]: `::Item` doesn't implement `std::fmt::Debug` + --> $DIR/impl-trait-with-missing-bounds.rs:22:13 + | +LL | qux(constraint); + | ^^^^^^^^^^ `::Item` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug` +... +LL | fn qux(_: impl std::fmt::Debug) {} + | --- --------------- required by this bound in `qux` + | + = help: the trait `std::fmt::Debug` is not implemented for `::Item` +help: introduce a type parameter with a trait bound instead of using `impl Trait` + | +LL | fn baz(t: impl std::fmt::Debug, constraints: T) where ::Item: std::fmt::Debug { + | ^^^^^^^^^^^^^ ^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 3 previous errors + +For more information about this error, try `rustc --explain E0277`. From 2c998aa8bb463f21fd164e2c701f891f5d5660d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Sat, 4 Apr 2020 17:31:32 -0700 Subject: [PATCH 2/9] review comments --- src/librustc_trait_selection/lib.rs | 1 + .../traits/error_reporting/suggestions.rs | 269 +++++++++--------- 2 files changed, 140 insertions(+), 130 deletions(-) diff --git a/src/librustc_trait_selection/lib.rs b/src/librustc_trait_selection/lib.rs index 21315cc89ca..fb82a50cd16 100644 --- a/src/librustc_trait_selection/lib.rs +++ b/src/librustc_trait_selection/lib.rs @@ -16,6 +16,7 @@ #![feature(in_band_lifetimes)] #![feature(crate_visibility_modifier)] #![feature(or_patterns)] +#![feature(str_strip)] #![recursion_limit = "512"] // For rustdoc #[macro_use] diff --git a/src/librustc_trait_selection/traits/error_reporting/suggestions.rs b/src/librustc_trait_selection/traits/error_reporting/suggestions.rs index ca2d0a8c3ed..fb70ae53a4f 100644 --- a/src/librustc_trait_selection/traits/error_reporting/suggestions.rs +++ b/src/librustc_trait_selection/traits/error_reporting/suggestions.rs @@ -148,6 +148,126 @@ fn note_obligation_cause_code( 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_none() { + // Given `fn foo(t: impl Trait)` where `Trait` requires assoc type `A`... + if let Some((name, fn_sig)) = fn_sig.and_then(|sig| { + projection.and_then(|p| { + // Shenanigans to get the `Trait` from the `impl Trait`. + match p.self_ty().kind { + 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_name = format!("impl {}", name); + 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_name.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); + } + } + } + + // The type param `T: Trait` we will suggest to introduce. + let type_param = format!("{}: {}", "T", name); + + // FIXME: modify the `trait_ref` instead of string shenanigans. + // Turn `::Bar: Qux` into `::Bar: Qux`. + let pred = trait_ref.without_const().to_predicate().to_string(); + let pred = pred.replace(&impl_name, "T"); + 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 `` here + None => (generics.span, format!("<{}>", type_param)), + // `fn foo(t: impl Trait)` + // ^^^ suggest `` here + Some(param) => (param.span.shrink_to_hi(), format!(", {}", type_param)), + }, + // `fn foo(t: impl Trait)` + // ^ suggest `where ::A: Bound` + predicate_constraint(generics, pred), + ]; + sugg.extend(ty_spans.into_iter().map(|s| (s, "T".to_string()))); + + // Suggest `fn foo(t: T) where ::A: Bound`. + 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, s) = predicate_constraint( + generics, + trait_ref.without_const().to_predicate().to_string(), + ); + let appl = Applicability::MachineApplicable; + err.span_suggestion(sp, &format!("consider further restricting {}", msg), s, appl); + } + } +} + impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { fn suggest_restricting_param_bound( &self, @@ -162,135 +282,10 @@ fn suggest_restricting_param_bound( _ => return, }; - let suggest_restriction = - |generics: &hir::Generics<'_>, - msg, - err: &mut DiagnosticBuilder<'_>, - fn_sig: Option<&hir::FnSig<'_>>| { - // 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. - let span = generics.where_clause.span_for_predicates_or_empty_place(); - if !span.from_expansion() && span.desugaring_kind().is_none() { - // Given `fn foo(t: impl Trait)` where `Trait` requires assoc type `A`... - if let Some((name, fn_sig)) = fn_sig.and_then(|sig| { - projection.and_then(|p| { - // Shenanigans to get the `Trait` from the `impl Trait`. - match p.self_ty().kind { - ty::Param(param) if param.name.as_str().starts_with("impl ") => { - let n = param.name.as_str(); - // `fn foo(t: impl Trait)` - // ^^^^^ get this string - n.split_whitespace() - .skip(1) - .next() - .map(|n| (n.to_string(), sig)) - } - _ => None, - } - }) - }) { - // FIXME: Cleanup. - let mut ty_spans = vec![]; - let impl_name = format!("impl {}", name); - for i in fn_sig.decl.inputs { - if let hir::TyKind::Path(hir::QPath::Resolved(None, path)) = i.kind { - match path.segments { - [segment] if segment.ident.to_string() == impl_name => { - // `fn foo(t: impl Trait)` - // ^^^^^^^^^^ get this to suggest - // `T` instead - - // There might be more than one `impl Trait`. - ty_spans.push(i.span); - } - _ => {} - } - } - } - - let type_param = format!("{}: {}", "T", name); - // FIXME: modify the `trait_ref` instead of string shenanigans. - // Turn `::Bar: Qux` into `::Bar: Qux`. - let pred = trait_ref.without_const().to_predicate().to_string(); - let pred = pred.replace(&impl_name, "T"); - 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 `` here - None => (generics.span, format!("<{}>", type_param)), - Some(param) => { - (param.span.shrink_to_hi(), format!(", {}", type_param)) - } - }, - ( - // `fn foo(t: impl Trait)` - // ^ suggest `where ::A: Bound` - generics - .where_clause - .span_for_predicates_or_empty_place() - .shrink_to_hi(), - format!( - "{} {} ", - if !generics.where_clause.predicates.is_empty() { - "," - } else { - " where" - }, - pred, - ), - ), - ]; - sugg.extend(ty_spans.into_iter().map(|s| (s, "T".to_string()))); - // Suggest `fn foo(t: T) where ::A: Bound`. - 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. - 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; while let Some(node) = self.tcx.hir().find(hir_id) { - debug!( - "suggest_restricting_param_bound {:?} {:?} {:?} {:?}", - trait_ref, self_ty.kind, projection, node - ); match node { hir::Node::TraitItem(hir::TraitItem { generics, @@ -298,7 +293,7 @@ fn suggest_restricting_param_bound( .. }) if param_ty && self_ty == self.tcx.types.self_param => { // Restricting `Self` for a single method. - suggest_restriction(&generics, "`Self`", err, None); + suggest_restriction(&generics, "`Self`", err, None, projection, trait_ref); return; } @@ -315,16 +310,30 @@ fn suggest_restricting_param_bound( | hir::Node::Item(hir::Item { kind: hir::ItemKind::Fn(fn_sig, generics, _), .. }) if projection.is_some() => { - // Missing associated type bound. - suggest_restriction(&generics, "the associated type", err, Some(fn_sig)); + // 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, None); + // Missing restriction on associated type of type parameter (unmet projection). + suggest_restriction( + &generics, + "the associated type", + err, + None, + projection, + trait_ref, + ); return; } From 794b644f0be7c4acbac8ca5de182af6efe12345f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Sun, 5 Apr 2020 10:52:54 -0700 Subject: [PATCH 3/9] review comments --- src/librustc_trait_selection/lib.rs | 1 + .../traits/error_reporting/mod.rs | 2 +- .../traits/error_reporting/suggestions.rs | 177 +++++++++--------- 3 files changed, 87 insertions(+), 93 deletions(-) diff --git a/src/librustc_trait_selection/lib.rs b/src/librustc_trait_selection/lib.rs index fb82a50cd16..9ada88098a5 100644 --- a/src/librustc_trait_selection/lib.rs +++ b/src/librustc_trait_selection/lib.rs @@ -17,6 +17,7 @@ #![feature(crate_visibility_modifier)] #![feature(or_patterns)] #![feature(str_strip)] +#![feature(option_zip)] #![recursion_limit = "512"] // For rustdoc #[macro_use] diff --git a/src/librustc_trait_selection/traits/error_reporting/mod.rs b/src/librustc_trait_selection/traits/error_reporting/mod.rs index f2640cec710..fef7adf0224 100644 --- a/src/librustc_trait_selection/traits/error_reporting/mod.rs +++ b/src/librustc_trait_selection/traits/error_reporting/mod.rs @@ -388,7 +388,7 @@ fn report_selection_error( // which is somewhat confusing. self.suggest_restricting_param_bound( &mut err, - &trait_ref, + trait_ref, obligation.cause.body_id, ); } else { diff --git a/src/librustc_trait_selection/traits/error_reporting/suggestions.rs b/src/librustc_trait_selection/traits/error_reporting/suggestions.rs index fb70ae53a4f..b3209ddc7fe 100644 --- a/src/librustc_trait_selection/traits/error_reporting/suggestions.rs +++ b/src/librustc_trait_selection/traits/error_reporting/suggestions.rs @@ -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, ); @@ -168,103 +168,96 @@ fn suggest_restriction( err: &mut DiagnosticBuilder<'_>, fn_sig: Option<&hir::FnSig<'_>>, projection: Option<&ty::ProjectionTy<'_>>, - trait_ref: &ty::PolyTraitRef<'_>, + trait_ref: ty::PolyTraitRef<'_>, ) { let span = generics.where_clause.span_for_predicates_or_empty_place(); - if !span.from_expansion() && span.desugaring_kind().is_none() { - // Given `fn foo(t: impl Trait)` where `Trait` requires assoc type `A`... - if let Some((name, fn_sig)) = fn_sig.and_then(|sig| { - projection.and_then(|p| { - // Shenanigans to get the `Trait` from the `impl Trait`. - match p.self_ty().kind { - 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. + 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((name, 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_name = format!("impl {}", name); - 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_name.as_str() { - // `fn foo(t: impl Trait)` - // ^^^^^^^^^^ get this to suggest - // `T` instead + // 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_name = format!("impl {}", name); + 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_name.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); - } + // There might be more than one `impl Trait`. + ty_spans.push(input.span); } } - - // The type param `T: Trait` we will suggest to introduce. - let type_param = format!("{}: {}", "T", name); - - // FIXME: modify the `trait_ref` instead of string shenanigans. - // Turn `::Bar: Qux` into `::Bar: Qux`. - let pred = trait_ref.without_const().to_predicate().to_string(); - let pred = pred.replace(&impl_name, "T"); - 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 `` here - None => (generics.span, format!("<{}>", type_param)), - // `fn foo(t: impl Trait)` - // ^^^ suggest `` here - Some(param) => (param.span.shrink_to_hi(), format!(", {}", type_param)), - }, - // `fn foo(t: impl Trait)` - // ^ suggest `where ::A: Bound` - predicate_constraint(generics, pred), - ]; - sugg.extend(ty_spans.into_iter().map(|s| (s, "T".to_string()))); - - // Suggest `fn foo(t: T) where ::A: Bound`. - 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, s) = predicate_constraint( - generics, - trait_ref.without_const().to_predicate().to_string(), - ); - let appl = Applicability::MachineApplicable; - err.span_suggestion(sp, &format!("consider further restricting {}", msg), s, appl); } + + // The type param `T: Trait` we will suggest to introduce. + let type_param = format!("{}: {}", "T", name); + + // FIXME: modify the `trait_ref` instead of string shenanigans. + // Turn `::Bar: Qux` into `::Bar: Qux`. + let pred = trait_ref.without_const().to_predicate().to_string(); + let pred = pred.replace(&impl_name, "T"); + 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 `` here + None => (generics.span, format!("<{}>", type_param)), + // `fn foo(t: impl Trait)` + // ^^^ suggest `` here + Some(param) => (param.span.shrink_to_hi(), format!(", {}", type_param)), + }, + // `fn foo(t: impl Trait)` + // ^ suggest `where ::A: Bound` + predicate_constraint(generics, pred), + ]; + sugg.extend(ty_spans.into_iter().map(|s| (s, "T".to_string()))); + + // Suggest `fn foo(t: T) where ::A: Bound`. + 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, s) = + predicate_constraint(generics, trait_ref.without_const().to_predicate().to_string()); + let appl = Applicability::MachineApplicable; + err.span_suggestion(sp, &format!("consider further restricting {}", msg), s, appl); } } @@ -272,7 +265,7 @@ 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(); From c85fde126ed936685b0cdf9d28a3baa96bb0aa3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Sun, 5 Apr 2020 12:32:34 -0700 Subject: [PATCH 4/9] Account for type params with bounds --- .../traits/error_reporting/suggestions.rs | 11 ++++++----- .../impl-trait-with-missing-bounds.rs | 8 ++++++++ .../impl-trait-with-missing-bounds.stderr | 17 ++++++++++++++++- 3 files changed, 30 insertions(+), 6 deletions(-) diff --git a/src/librustc_trait_selection/traits/error_reporting/suggestions.rs b/src/librustc_trait_selection/traits/error_reporting/suggestions.rs index b3209ddc7fe..7e210cb7d25 100644 --- a/src/librustc_trait_selection/traits/error_reporting/suggestions.rs +++ b/src/librustc_trait_selection/traits/error_reporting/suggestions.rs @@ -203,8 +203,7 @@ fn suggest_restriction( { if segment.ident.as_str() == impl_name.as_str() { // `fn foo(t: impl Trait)` - // ^^^^^^^^^^ get this to suggest - // `T` instead + // ^^^^^^^^^^ get this to suggest `T` instead // There might be more than one `impl Trait`. ty_spans.push(input.span); @@ -237,7 +236,10 @@ fn suggest_restriction( None => (generics.span, format!("<{}>", type_param)), // `fn foo(t: impl Trait)` // ^^^ suggest `` here - Some(param) => (param.span.shrink_to_hi(), format!(", {}", type_param)), + Some(param) => ( + param.bounds_span().unwrap_or(param.span).shrink_to_hi(), + format!(", {}", type_param), + ), }, // `fn foo(t: impl Trait)` // ^ suggest `where ::A: Bound` @@ -247,8 +249,7 @@ fn suggest_restriction( // Suggest `fn foo(t: T) where ::A: Bound`. err.multipart_suggestion( - "introduce a type parameter with a trait bound instead of using \ - `impl Trait`", + "introduce a type parameter with a trait bound instead of using `impl Trait`", sugg, Applicability::MaybeIncorrect, ); diff --git a/src/test/ui/suggestions/impl-trait-with-missing-bounds.rs b/src/test/ui/suggestions/impl-trait-with-missing-bounds.rs index bef9ba9f91f..d831bafa2b5 100644 --- a/src/test/ui/suggestions/impl-trait-with-missing-bounds.rs +++ b/src/test/ui/suggestions/impl-trait-with-missing-bounds.rs @@ -24,6 +24,14 @@ fn baz(t: impl std::fmt::Debug, constraints: impl Iterator) { } } +fn bat(t: T, constraints: impl Iterator) { + for constraint in constraints { + qux(t); + qux(constraint); +//~^ ERROR `::Item` doesn't implement `std::fmt::Debug` + } +} + fn qux(_: impl std::fmt::Debug) {} fn main() {} diff --git a/src/test/ui/suggestions/impl-trait-with-missing-bounds.stderr b/src/test/ui/suggestions/impl-trait-with-missing-bounds.stderr index 5f84e6d44ea..f0f47876ed0 100644 --- a/src/test/ui/suggestions/impl-trait-with-missing-bounds.stderr +++ b/src/test/ui/suggestions/impl-trait-with-missing-bounds.stderr @@ -43,6 +43,21 @@ help: introduce a type parameter with a trait bound instead of using `impl Trait LL | fn baz(t: impl std::fmt::Debug, constraints: T) where ::Item: std::fmt::Debug { | ^^^^^^^^^^^^^ ^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 3 previous errors +error[E0277]: `::Item` doesn't implement `std::fmt::Debug` + --> $DIR/impl-trait-with-missing-bounds.rs:30:13 + | +LL | qux(constraint); + | ^^^^^^^^^^ `::Item` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug` +... +LL | fn qux(_: impl std::fmt::Debug) {} + | --- --------------- required by this bound in `qux` + | + = help: the trait `std::fmt::Debug` is not implemented for `::Item` +help: introduce a type parameter with a trait bound instead of using `impl Trait` + | +LL | fn bat(t: T, constraints: T) where ::Item: std::fmt::Debug { + | ^^^^^^^^^^^^^ ^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 4 previous errors For more information about this error, try `rustc --explain E0277`. From 01169572a23dc599a5a4c2f338afe68e62b295fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Sun, 5 Apr 2020 13:13:13 -0700 Subject: [PATCH 5/9] Account for existing names when suggesting adding a type param --- src/librustc_hir/hir.rs | 23 +++++++++++++++++++ .../traits/error_reporting/suggestions.rs | 9 ++++---- src/librustc_typeck/collect.rs | 17 ++------------ .../impl-trait-with-missing-bounds.rs | 9 +++++++- .../impl-trait-with-missing-bounds.stderr | 23 +++++++++++++++---- .../typeck_type_placeholder_item.stderr | 6 ++--- 6 files changed, 60 insertions(+), 27 deletions(-) diff --git a/src/librustc_hir/hir.rs b/src/librustc_hir/hir.rs index b719d576d6f..b4744a7d6db 100644 --- a/src/librustc_hir/hir.rs +++ b/src/librustc_hir/hir.rs @@ -437,6 +437,29 @@ pub fn bounds_span(&self) -> Option { } } +pub trait NextTypeParamName { + fn next_type_param_name(&self) -> &'static str; +} + +impl NextTypeParamName for &[GenericParam<'_>] { + fn next_type_param_name(&self) -> &'static str { + // This is the whitelist of possible parameter names that we might suggest. + let possible_names = ["T", "U", "V", "X", "Y", "Z", "A", "B", "C", "D", "E", "F", "G"]; + let used_names = self + .iter() + .filter_map(|p| match p.name { + ParamName::Plain(ident) => Some(ident.name), + _ => None, + }) + .collect::>(); + + possible_names + .iter() + .find(|n| !used_names.contains(&Symbol::intern(n))) + .unwrap_or(&"ParamName") + } +} + #[derive(Default)] pub struct GenericParamCount { pub lifetimes: usize, diff --git a/src/librustc_trait_selection/traits/error_reporting/suggestions.rs b/src/librustc_trait_selection/traits/error_reporting/suggestions.rs index 7e210cb7d25..16bdfe5d0d1 100644 --- a/src/librustc_trait_selection/traits/error_reporting/suggestions.rs +++ b/src/librustc_trait_selection/traits/error_reporting/suggestions.rs @@ -10,7 +10,7 @@ use rustc_hir::def::DefKind; use rustc_hir::def_id::DefId; use rustc_hir::intravisit::Visitor; -use rustc_hir::Node; +use rustc_hir::{NextTypeParamName, Node}; use rustc_middle::ty::TypeckTables; use rustc_middle::ty::{ self, AdtKind, DefIdTree, ToPredicate, Ty, TyCtxt, TypeFoldable, WithConstness, @@ -211,13 +211,14 @@ fn suggest_restriction( } } + let type_param_name = generics.params.next_type_param_name(); // The type param `T: Trait` we will suggest to introduce. - let type_param = format!("{}: {}", "T", name); + let type_param = format!("{}: {}", type_param_name, name); // FIXME: modify the `trait_ref` instead of string shenanigans. // Turn `::Bar: Qux` into `::Bar: Qux`. let pred = trait_ref.without_const().to_predicate().to_string(); - let pred = pred.replace(&impl_name, "T"); + let pred = pred.replace(&impl_name, type_param_name); let mut sugg = vec![ match generics .params @@ -245,7 +246,7 @@ fn suggest_restriction( // ^ suggest `where ::A: Bound` predicate_constraint(generics, pred), ]; - sugg.extend(ty_spans.into_iter().map(|s| (s, "T".to_string()))); + sugg.extend(ty_spans.into_iter().map(|s| (s, type_param_name.to_string()))); // Suggest `fn foo(t: T) where ::A: Bound`. err.multipart_suggestion( diff --git a/src/librustc_typeck/collect.rs b/src/librustc_typeck/collect.rs index 23bee8e7aad..eb8f46e83bb 100644 --- a/src/librustc_typeck/collect.rs +++ b/src/librustc_typeck/collect.rs @@ -29,7 +29,7 @@ use rustc_hir::def_id::{DefId, LOCAL_CRATE}; use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor}; use rustc_hir::weak_lang_items; -use rustc_hir::{GenericParamKind, Node, Unsafety}; +use rustc_hir::{GenericParamKind, NextTypeParamName, Node, Unsafety}; use rustc_middle::hir::map::blocks::FnLikeNode; use rustc_middle::hir::map::Map; use rustc_middle::middle::codegen_fn_attrs::{CodegenFnAttrFlags, CodegenFnAttrs}; @@ -135,20 +135,7 @@ struct CollectItemTypesVisitor<'tcx> { 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::>(); - - 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(); let mut sugg: Vec<_> = placeholder_types.iter().map(|sp| (*sp, (*type_name).to_string())).collect(); diff --git a/src/test/ui/suggestions/impl-trait-with-missing-bounds.rs b/src/test/ui/suggestions/impl-trait-with-missing-bounds.rs index d831bafa2b5..6947bc0a734 100644 --- a/src/test/ui/suggestions/impl-trait-with-missing-bounds.rs +++ b/src/test/ui/suggestions/impl-trait-with-missing-bounds.rs @@ -24,7 +24,7 @@ fn baz(t: impl std::fmt::Debug, constraints: impl Iterator) { } } -fn bat(t: T, constraints: impl Iterator) { +fn bat(t: T, constraints: impl Iterator, _: K) { for constraint in constraints { qux(t); qux(constraint); @@ -32,6 +32,13 @@ fn bat(t: T, constraints: impl Iterator) { } } +fn bak(constraints: impl Iterator + std::fmt::Debug) { + for constraint in constraints { + qux(constraint); +//~^ ERROR `::Item` doesn't implement + } +} + fn qux(_: impl std::fmt::Debug) {} fn main() {} diff --git a/src/test/ui/suggestions/impl-trait-with-missing-bounds.stderr b/src/test/ui/suggestions/impl-trait-with-missing-bounds.stderr index f0f47876ed0..2d48be42233 100644 --- a/src/test/ui/suggestions/impl-trait-with-missing-bounds.stderr +++ b/src/test/ui/suggestions/impl-trait-with-missing-bounds.stderr @@ -25,7 +25,7 @@ LL | fn qux(_: impl std::fmt::Debug) {} = help: the trait `std::fmt::Debug` is not implemented for `::Item` help: introduce a type parameter with a trait bound instead of using `impl Trait` | -LL | fn bar(t: T, constraints: T) where T: std::fmt::Debug, ::Item: std::fmt::Debug { +LL | fn bar(t: T, constraints: U) where T: std::fmt::Debug, ::Item: std::fmt::Debug { | ^^^^^^^^^^^^^ ^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error[E0277]: `::Item` doesn't implement `std::fmt::Debug` @@ -55,9 +55,24 @@ LL | fn qux(_: impl std::fmt::Debug) {} = help: the trait `std::fmt::Debug` is not implemented for `::Item` help: introduce a type parameter with a trait bound instead of using `impl Trait` | -LL | fn bat(t: T, constraints: T) where ::Item: std::fmt::Debug { - | ^^^^^^^^^^^^^ ^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | fn bat(t: T, constraints: U, _: K) where ::Item: std::fmt::Debug { + | ^^^^^^^^^^^^^ ^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 4 previous errors +error[E0277]: `::Item` doesn't implement `std::fmt::Debug` + --> $DIR/impl-trait-with-missing-bounds.rs:37:13 + | +LL | qux(constraint); + | ^^^^^^^^^^ `::Item` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug` +... +LL | fn qux(_: impl std::fmt::Debug) {} + | --- --------------- required by this bound in `qux` + | + = help: the trait `std::fmt::Debug` is not implemented for `::Item` +help: introduce a type parameter with a trait bound instead of using `impl Trait` + | +LL | fn bak(constraints: T) where ::Item: std::fmt::Debug { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 5 previous errors For more information about this error, try `rustc --explain E0277`. diff --git a/src/test/ui/typeck/typeck_type_placeholder_item.stderr b/src/test/ui/typeck/typeck_type_placeholder_item.stderr index db67e0c9b7d..6c0653d5fcb 100644 --- a/src/test/ui/typeck/typeck_type_placeholder_item.stderr +++ b/src/test/ui/typeck/typeck_type_placeholder_item.stderr @@ -106,7 +106,7 @@ LL | fn test6_b(_: _, _: T) { } | help: use type parameters instead | -LL | fn test6_b(_: K, _: T) { } +LL | fn test6_b(_: U, _: T) { } | ^^^ ^ error[E0121]: the type placeholder `_` is not allowed within types on item signatures @@ -117,7 +117,7 @@ LL | fn test6_c(_: _, _: (T, K, L, A, B)) { } | help: use type parameters instead | -LL | fn test6_c(_: C, _: (T, K, L, A, B)) { } +LL | fn test6_c(_: U, _: (T, K, L, A, B)) { } | ^^^ ^ error[E0121]: the type placeholder `_` is not allowed within types on item signatures @@ -377,7 +377,7 @@ LL | struct BadStruct2<_, T>(_, T); | help: use type parameters instead | -LL | struct BadStruct2(K, T); +LL | struct BadStruct2(U, T); | ^ ^ error[E0121]: the type placeholder `_` is not allowed within types on item signatures From 1e3bdc08c955a55f69a4c2df4a829a52f5b0c21c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Sun, 5 Apr 2020 14:55:06 -0700 Subject: [PATCH 6/9] Try to use the first char in the trait name as type param --- src/librustc_hir/hir.rs | 9 ++++++--- .../traits/error_reporting/suggestions.rs | 4 ++-- src/librustc_typeck/collect.rs | 2 +- .../ui/suggestions/impl-trait-with-missing-bounds.rs | 2 +- .../suggestions/impl-trait-with-missing-bounds.stderr | 10 +++++----- 5 files changed, 15 insertions(+), 12 deletions(-) diff --git a/src/librustc_hir/hir.rs b/src/librustc_hir/hir.rs index b4744a7d6db..f26fc402a9a 100644 --- a/src/librustc_hir/hir.rs +++ b/src/librustc_hir/hir.rs @@ -438,13 +438,15 @@ pub fn bounds_span(&self) -> Option { } pub trait NextTypeParamName { - fn next_type_param_name(&self) -> &'static str; + fn next_type_param_name(&self, name: Option<&str>) -> String; } impl NextTypeParamName for &[GenericParam<'_>] { - fn next_type_param_name(&self) -> &'static str { + fn next_type_param_name(&self, name: Option<&str>) -> String { // This is the whitelist of possible parameter names that we might suggest. - let possible_names = ["T", "U", "V", "X", "Y", "Z", "A", "B", "C", "D", "E", "F", "G"]; + 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 { @@ -457,6 +459,7 @@ fn next_type_param_name(&self) -> &'static str { .iter() .find(|n| !used_names.contains(&Symbol::intern(n))) .unwrap_or(&"ParamName") + .to_string() } } diff --git a/src/librustc_trait_selection/traits/error_reporting/suggestions.rs b/src/librustc_trait_selection/traits/error_reporting/suggestions.rs index 16bdfe5d0d1..152b4fb7c56 100644 --- a/src/librustc_trait_selection/traits/error_reporting/suggestions.rs +++ b/src/librustc_trait_selection/traits/error_reporting/suggestions.rs @@ -211,14 +211,14 @@ fn suggest_restriction( } } - let type_param_name = generics.params.next_type_param_name(); + let type_param_name = generics.params.next_type_param_name(Some(&name)); // The type param `T: Trait` we will suggest to introduce. let type_param = format!("{}: {}", type_param_name, name); // FIXME: modify the `trait_ref` instead of string shenanigans. // Turn `::Bar: Qux` into `::Bar: Qux`. let pred = trait_ref.without_const().to_predicate().to_string(); - let pred = pred.replace(&impl_name, type_param_name); + let pred = pred.replace(&impl_name, &type_param_name); let mut sugg = vec![ match generics .params diff --git a/src/librustc_typeck/collect.rs b/src/librustc_typeck/collect.rs index eb8f46e83bb..47754c3704c 100644 --- a/src/librustc_typeck/collect.rs +++ b/src/librustc_typeck/collect.rs @@ -135,7 +135,7 @@ struct CollectItemTypesVisitor<'tcx> { if placeholder_types.is_empty() { return; } - let type_name = generics.next_type_param_name(); + let type_name = generics.next_type_param_name(None); let mut sugg: Vec<_> = placeholder_types.iter().map(|sp| (*sp, (*type_name).to_string())).collect(); diff --git a/src/test/ui/suggestions/impl-trait-with-missing-bounds.rs b/src/test/ui/suggestions/impl-trait-with-missing-bounds.rs index 6947bc0a734..6e9e8821cfe 100644 --- a/src/test/ui/suggestions/impl-trait-with-missing-bounds.rs +++ b/src/test/ui/suggestions/impl-trait-with-missing-bounds.rs @@ -24,7 +24,7 @@ fn baz(t: impl std::fmt::Debug, constraints: impl Iterator) { } } -fn bat(t: T, constraints: impl Iterator, _: K) { +fn bat(t: T, constraints: impl Iterator, _: I) { for constraint in constraints { qux(t); qux(constraint); diff --git a/src/test/ui/suggestions/impl-trait-with-missing-bounds.stderr b/src/test/ui/suggestions/impl-trait-with-missing-bounds.stderr index 2d48be42233..e1c40e2537b 100644 --- a/src/test/ui/suggestions/impl-trait-with-missing-bounds.stderr +++ b/src/test/ui/suggestions/impl-trait-with-missing-bounds.stderr @@ -10,7 +10,7 @@ LL | fn qux(_: impl std::fmt::Debug) {} = help: the trait `std::fmt::Debug` is not implemented for `::Item` help: introduce a type parameter with a trait bound instead of using `impl Trait` | -LL | fn foo(constraints: T) where ::Item: std::fmt::Debug { +LL | fn foo(constraints: I) where ::Item: std::fmt::Debug { | ^^^^^^^^^^^^^ ^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error[E0277]: `::Item` doesn't implement `std::fmt::Debug` @@ -25,7 +25,7 @@ LL | fn qux(_: impl std::fmt::Debug) {} = help: the trait `std::fmt::Debug` is not implemented for `::Item` help: introduce a type parameter with a trait bound instead of using `impl Trait` | -LL | fn bar(t: T, constraints: U) where T: std::fmt::Debug, ::Item: std::fmt::Debug { +LL | fn bar(t: T, constraints: I) where T: std::fmt::Debug, ::Item: std::fmt::Debug { | ^^^^^^^^^^^^^ ^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error[E0277]: `::Item` doesn't implement `std::fmt::Debug` @@ -40,7 +40,7 @@ LL | fn qux(_: impl std::fmt::Debug) {} = help: the trait `std::fmt::Debug` is not implemented for `::Item` help: introduce a type parameter with a trait bound instead of using `impl Trait` | -LL | fn baz(t: impl std::fmt::Debug, constraints: T) where ::Item: std::fmt::Debug { +LL | fn baz(t: impl std::fmt::Debug, constraints: I) where ::Item: std::fmt::Debug { | ^^^^^^^^^^^^^ ^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error[E0277]: `::Item` doesn't implement `std::fmt::Debug` @@ -55,7 +55,7 @@ LL | fn qux(_: impl std::fmt::Debug) {} = help: the trait `std::fmt::Debug` is not implemented for `::Item` help: introduce a type parameter with a trait bound instead of using `impl Trait` | -LL | fn bat(t: T, constraints: U, _: K) where ::Item: std::fmt::Debug { +LL | fn bat(t: T, constraints: U, _: I) where ::Item: std::fmt::Debug { | ^^^^^^^^^^^^^ ^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error[E0277]: `::Item` doesn't implement `std::fmt::Debug` @@ -70,7 +70,7 @@ LL | fn qux(_: impl std::fmt::Debug) {} = help: the trait `std::fmt::Debug` is not implemented for `::Item` help: introduce a type parameter with a trait bound instead of using `impl Trait` | -LL | fn bak(constraints: T) where ::Item: std::fmt::Debug { +LL | fn bak(constraints: I) where ::Item: std::fmt::Debug { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: aborting due to 5 previous errors From 44394707e55ddc5402afbc27853de05a58905002 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Sun, 5 Apr 2020 15:33:33 -0700 Subject: [PATCH 7/9] review comments --- src/librustc_hir/hir.rs | 26 --------------- .../traits/error_reporting/suggestions.rs | 32 +++++++++++++++++-- src/librustc_typeck/collect.rs | 3 +- 3 files changed, 32 insertions(+), 29 deletions(-) diff --git a/src/librustc_hir/hir.rs b/src/librustc_hir/hir.rs index f26fc402a9a..b719d576d6f 100644 --- a/src/librustc_hir/hir.rs +++ b/src/librustc_hir/hir.rs @@ -437,32 +437,6 @@ pub fn bounds_span(&self) -> Option { } } -pub trait NextTypeParamName { - fn next_type_param_name(&self, name: Option<&str>) -> String; -} - -impl NextTypeParamName for &[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 { - ParamName::Plain(ident) => Some(ident.name), - _ => None, - }) - .collect::>(); - - possible_names - .iter() - .find(|n| !used_names.contains(&Symbol::intern(n))) - .unwrap_or(&"ParamName") - .to_string() - } -} - #[derive(Default)] pub struct GenericParamCount { pub lifetimes: usize, diff --git a/src/librustc_trait_selection/traits/error_reporting/suggestions.rs b/src/librustc_trait_selection/traits/error_reporting/suggestions.rs index 152b4fb7c56..6d627bd2865 100644 --- a/src/librustc_trait_selection/traits/error_reporting/suggestions.rs +++ b/src/librustc_trait_selection/traits/error_reporting/suggestions.rs @@ -10,12 +10,12 @@ use rustc_hir::def::DefKind; use rustc_hir::def_id::DefId; use rustc_hir::intravisit::Visitor; -use rustc_hir::{NextTypeParamName, Node}; +use rustc_hir::Node; 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; @@ -249,6 +249,8 @@ fn suggest_restriction( sugg.extend(ty_spans.into_iter().map(|s| (s, type_param_name.to_string()))); // Suggest `fn foo(t: T) where ::A: Bound`. + // FIXME: once `#![feature(associated_type_bounds)]` is stabilized, we should suggest + // `fn foo(t: impl Trait)` instead. err.multipart_suggestion( "introduce a type parameter with a trait bound instead of using `impl Trait`", sugg, @@ -1702,3 +1704,29 @@ fn visit_body(&mut self, body: &'v hir::Body<'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::>(); + + possible_names + .iter() + .find(|n| !used_names.contains(&Symbol::intern(n))) + .unwrap_or(&"ParamName") + .to_string() + } +} diff --git a/src/librustc_typeck/collect.rs b/src/librustc_typeck/collect.rs index 47754c3704c..8ae779a4783 100644 --- a/src/librustc_typeck/collect.rs +++ b/src/librustc_typeck/collect.rs @@ -29,7 +29,7 @@ use rustc_hir::def_id::{DefId, LOCAL_CRATE}; use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor}; use rustc_hir::weak_lang_items; -use rustc_hir::{GenericParamKind, NextTypeParamName, Node, Unsafety}; +use rustc_hir::{GenericParamKind, Node, Unsafety}; use rustc_middle::hir::map::blocks::FnLikeNode; use rustc_middle::hir::map::Map; use rustc_middle::middle::codegen_fn_attrs::{CodegenFnAttrFlags, CodegenFnAttrs}; @@ -45,6 +45,7 @@ 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; From ca25e2868dc2109701367073304857ead72cd202 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Fri, 10 Apr 2020 10:40:47 -0700 Subject: [PATCH 8/9] review comments --- .../traits/error_reporting/suggestions.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/librustc_trait_selection/traits/error_reporting/suggestions.rs b/src/librustc_trait_selection/traits/error_reporting/suggestions.rs index 6d627bd2865..14029f29151 100644 --- a/src/librustc_trait_selection/traits/error_reporting/suggestions.rs +++ b/src/librustc_trait_selection/traits/error_reporting/suggestions.rs @@ -175,7 +175,7 @@ fn suggest_restriction( return; } // Given `fn foo(t: impl Trait)` where `Trait` requires assoc type `A`... - if let Some((name, fn_sig)) = + 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) => { @@ -194,14 +194,14 @@ fn suggest_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_name = format!("impl {}", name); + 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_name.as_str() { + if segment.ident.as_str() == impl_trait_str.as_str() { // `fn foo(t: impl Trait)` // ^^^^^^^^^^ get this to suggest `T` instead @@ -211,14 +211,14 @@ fn suggest_restriction( } } - let type_param_name = generics.params.next_type_param_name(Some(&name)); + 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, name); + let type_param = format!("{}: {}", type_param_name, bound_str); // FIXME: modify the `trait_ref` instead of string shenanigans. // Turn `::Bar: Qux` into `::Bar: Qux`. let pred = trait_ref.without_const().to_predicate().to_string(); - let pred = pred.replace(&impl_name, &type_param_name); + let pred = pred.replace(&impl_trait_str, &type_param_name); let mut sugg = vec![ match generics .params @@ -258,10 +258,10 @@ fn suggest_restriction( ); } else { // Trivial case: `T` needs an extra bound: `T: Bound`. - let (sp, s) = + 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), s, appl); + err.span_suggestion(sp, &format!("consider further restricting {}", msg), sugg, appl); } } From 984aac6eed8fe80ed97cbc3f9fe585579733db60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Sat, 11 Apr 2020 14:59:15 -0700 Subject: [PATCH 9/9] fix rebase --- .../suggestions/impl-trait-with-missing-bounds.stderr | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/test/ui/suggestions/impl-trait-with-missing-bounds.stderr b/src/test/ui/suggestions/impl-trait-with-missing-bounds.stderr index e1c40e2537b..6fc629b33a2 100644 --- a/src/test/ui/suggestions/impl-trait-with-missing-bounds.stderr +++ b/src/test/ui/suggestions/impl-trait-with-missing-bounds.stderr @@ -5,7 +5,7 @@ LL | qux(constraint); | ^^^^^^^^^^ `::Item` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug` ... LL | fn qux(_: impl std::fmt::Debug) {} - | --- --------------- required by this bound in `qux` + | --------------- required by this bound in `qux` | = help: the trait `std::fmt::Debug` is not implemented for `::Item` help: introduce a type parameter with a trait bound instead of using `impl Trait` @@ -20,7 +20,7 @@ LL | qux(constraint); | ^^^^^^^^^^ `::Item` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug` ... LL | fn qux(_: impl std::fmt::Debug) {} - | --- --------------- required by this bound in `qux` + | --------------- required by this bound in `qux` | = help: the trait `std::fmt::Debug` is not implemented for `::Item` help: introduce a type parameter with a trait bound instead of using `impl Trait` @@ -35,7 +35,7 @@ LL | qux(constraint); | ^^^^^^^^^^ `::Item` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug` ... LL | fn qux(_: impl std::fmt::Debug) {} - | --- --------------- required by this bound in `qux` + | --------------- required by this bound in `qux` | = help: the trait `std::fmt::Debug` is not implemented for `::Item` help: introduce a type parameter with a trait bound instead of using `impl Trait` @@ -50,7 +50,7 @@ LL | qux(constraint); | ^^^^^^^^^^ `::Item` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug` ... LL | fn qux(_: impl std::fmt::Debug) {} - | --- --------------- required by this bound in `qux` + | --------------- required by this bound in `qux` | = help: the trait `std::fmt::Debug` is not implemented for `::Item` help: introduce a type parameter with a trait bound instead of using `impl Trait` @@ -65,7 +65,7 @@ LL | qux(constraint); | ^^^^^^^^^^ `::Item` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug` ... LL | fn qux(_: impl std::fmt::Debug) {} - | --- --------------- required by this bound in `qux` + | --------------- required by this bound in `qux` | = help: the trait `std::fmt::Debug` is not implemented for `::Item` help: introduce a type parameter with a trait bound instead of using `impl Trait`