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] 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 21315cc89ca5..fb82a50cd16e 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 ca2d0a8c3edd..fb70ae53a4fe 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 @@ 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_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 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { _ => 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 @@ 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, None); + suggest_restriction(&generics, "`Self`", err, None, projection, trait_ref); return; } @@ -315,16 +310,30 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { | 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; }