From de2edb226b213c414546b4e739356f0524b871f6 Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Tue, 12 Apr 2022 11:29:23 +0400 Subject: [PATCH 1/2] Fix wrong suggestions for `T:` This commit fixes a corner case in `suggest_constraining_type_params` that was causing incorrect suggestions. For the following functions: ```rust fn a(t: T) { [t, t]; } fn b(t: T) where T: { [t, t]; } ``` We previously suggested the following: ```text ... help: consider restricting type parameter `T` | 1 | fn a(t: T) { [t, t]; } | ++++++ ... help: consider further restricting this bound | 2 | fn b(t: T) where T: + Copy { [t, t]; } | ++++++ ``` Note that neither `T: Copy:` not `where T: + Copy` is a correct bound. With this commit the suggestions are correct: ```text ... help: consider restricting type parameter `T` | 1 | fn a(t: T) { [t, t]; } | ++++ ... help: consider further restricting this bound | 2 | fn b(t: T) where T: Copy { [t, t]; } | ++++ ``` --- compiler/rustc_hir/src/hir.rs | 36 ++++++++++++++++++++- compiler/rustc_middle/src/ty/diagnostics.rs | 35 ++++++++++++++++---- 2 files changed, 63 insertions(+), 8 deletions(-) diff --git a/compiler/rustc_hir/src/hir.rs b/compiler/rustc_hir/src/hir.rs index 15118073b3343..76971d7ad3a05 100644 --- a/compiler/rustc_hir/src/hir.rs +++ b/compiler/rustc_hir/src/hir.rs @@ -17,7 +17,7 @@ use rustc_error_messages::MultiSpan; use rustc_index::vec::IndexVec; use rustc_macros::HashStable_Generic; use rustc_span::hygiene::MacroKind; -use rustc_span::source_map::Spanned; +use rustc_span::source_map::{SourceMap, Spanned}; use rustc_span::symbol::{kw, sym, Ident, Symbol}; use rustc_span::{def_id::LocalDefId, BytePos, Span, DUMMY_SP}; use rustc_target::asm::InlineAsmRegOrRegClass; @@ -523,6 +523,40 @@ impl<'hir> GenericParam<'hir> { }) .map(|sp| sp.shrink_to_hi()) } + + /// Returns the span of `:` after a generic parameter. + /// + /// For example: + /// + /// ```text + /// fn a() + /// ^ + /// | here + /// here | + /// v + /// fn b() + /// + /// fn c() + /// ^ + /// | + /// here + /// ``` + pub fn colon_span_for_suggestions(&self, source_map: &SourceMap) -> Option { + let sp = source_map + .span_extend_while(self.span.shrink_to_hi(), |c| c.is_whitespace() || c == ':') + .ok()?; + + let snippet = source_map.span_to_snippet(sp).ok()?; + let offset = snippet.find(':')?; + + let colon_sp = sp + .with_lo(BytePos(sp.lo().0 + offset as u32)) + .with_hi(BytePos(sp.lo().0 + (offset + ':'.len_utf8()) as u32)); + + Some(colon_sp) + } } #[derive(Default)] diff --git a/compiler/rustc_middle/src/ty/diagnostics.rs b/compiler/rustc_middle/src/ty/diagnostics.rs index 49d0ce5205205..3b044b19259d0 100644 --- a/compiler/rustc_middle/src/ty/diagnostics.rs +++ b/compiler/rustc_middle/src/ty/diagnostics.rs @@ -336,10 +336,14 @@ pub fn suggest_constraining_type_params<'a>( } let constraint = constraints.iter().map(|&(c, _)| c).collect::>().join(" + "); - let mut suggest_restrict = |span| { + let mut suggest_restrict = |span, bound_list_non_empty| { suggestions.push(( span, - format!(" + {}", constraint), + if bound_list_non_empty { + format!(" + {}", constraint) + } else { + format!(" {}", constraint) + }, SuggestChangingConstraintsMessage::RestrictBoundFurther, )) }; @@ -360,7 +364,10 @@ pub fn suggest_constraining_type_params<'a>( // | // replace with: `impl Foo + Bar` - suggest_restrict(param.span.shrink_to_hi()); + // `impl Trait` must have at least one trait in the list + let bound_list_non_empty = true; + + suggest_restrict(param.span.shrink_to_hi(), bound_list_non_empty); continue; } @@ -383,15 +390,25 @@ pub fn suggest_constraining_type_params<'a>( // -- // | // replace with: `T: Bar +` - suggest_restrict(span); + + // `bounds_span_for_suggestions` returns `None` if the list is empty + let bound_list_non_empty = true; + + suggest_restrict(span, bound_list_non_empty); } else { + let (colon, span) = match param.colon_span_for_suggestions(tcx.sess.source_map()) { + // If there is already a colon after generic, do not suggest adding it again + Some(sp) => ("", sp.shrink_to_hi()), + None => (":", param.span.shrink_to_hi()), + }; + // If user hasn't provided any bounds, suggest adding a new one: // // fn foo(t: T) { ... } // - help: consider restricting this type parameter with `T: Foo` suggestions.push(( - param.span.shrink_to_hi(), - format!(": {}", constraint), + span, + format!("{colon} {constraint}"), SuggestChangingConstraintsMessage::RestrictType { ty: param_name }, )); } @@ -459,17 +476,21 @@ pub fn suggest_constraining_type_params<'a>( )); } else { let mut param_spans = Vec::new(); + let mut non_empty = false; for predicate in generics.where_clause.predicates { if let WherePredicate::BoundPredicate(WhereBoundPredicate { span, bounded_ty, + bounds, .. }) = predicate { if let TyKind::Path(QPath::Resolved(_, path)) = &bounded_ty.kind { if let Some(segment) = path.segments.first() { if segment.ident.to_string() == param_name { + non_empty = !bounds.is_empty(); + param_spans.push(span); } } @@ -478,7 +499,7 @@ pub fn suggest_constraining_type_params<'a>( } match param_spans[..] { - [¶m_span] => suggest_restrict(param_span.shrink_to_hi()), + [¶m_span] => suggest_restrict(param_span.shrink_to_hi(), non_empty), _ => { suggestions.push(( generics.where_clause.tail_span_for_suggestion(), From e4710fe2214ba85fd9a3514c76c7722253a3a275 Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Tue, 12 Apr 2022 12:14:43 +0400 Subject: [PATCH 2/2] Add test for `T:` suggestions --- .../use_of_moved_value_copy_suggestions.fixed | 14 ++++++++ .../use_of_moved_value_copy_suggestions.rs | 14 ++++++++ ...use_of_moved_value_copy_suggestions.stderr | 34 ++++++++++++++++++- 3 files changed, 61 insertions(+), 1 deletion(-) diff --git a/src/test/ui/moves/use_of_moved_value_copy_suggestions.fixed b/src/test/ui/moves/use_of_moved_value_copy_suggestions.fixed index d31046c77006e..fb6cf7e8996c0 100644 --- a/src/test/ui/moves/use_of_moved_value_copy_suggestions.fixed +++ b/src/test/ui/moves/use_of_moved_value_copy_suggestions.fixed @@ -69,4 +69,18 @@ where (t, t) //~ use of moved value: `t` } +#[rustfmt::skip] +fn existing_colon(t: T) { + //~^ HELP consider restricting type parameter `T` + [t, t]; //~ use of moved value: `t` +} + +fn existing_colon_in_where(t: T) +where + T: Copy, + //~^ HELP consider further restricting this bound +{ + [t, t]; //~ use of moved value: `t` +} + fn main() {} diff --git a/src/test/ui/moves/use_of_moved_value_copy_suggestions.rs b/src/test/ui/moves/use_of_moved_value_copy_suggestions.rs index 7cc5189fac017..cadbf2a54cc37 100644 --- a/src/test/ui/moves/use_of_moved_value_copy_suggestions.rs +++ b/src/test/ui/moves/use_of_moved_value_copy_suggestions.rs @@ -69,4 +69,18 @@ where (t, t) //~ use of moved value: `t` } +#[rustfmt::skip] +fn existing_colon(t: T) { + //~^ HELP consider restricting type parameter `T` + [t, t]; //~ use of moved value: `t` +} + +fn existing_colon_in_where(t: T) +where + T:, + //~^ HELP consider further restricting this bound +{ + [t, t]; //~ use of moved value: `t` +} + fn main() {} diff --git a/src/test/ui/moves/use_of_moved_value_copy_suggestions.stderr b/src/test/ui/moves/use_of_moved_value_copy_suggestions.stderr index 8e72697ca30bb..f5252084d6884 100644 --- a/src/test/ui/moves/use_of_moved_value_copy_suggestions.stderr +++ b/src/test/ui/moves/use_of_moved_value_copy_suggestions.stderr @@ -142,6 +142,38 @@ help: consider further restricting this bound LL | T: B + Trait + Copy, | ++++++++++++++ -error: aborting due to 9 previous errors +error[E0382]: use of moved value: `t` + --> $DIR/use_of_moved_value_copy_suggestions.rs:83:9 + | +LL | fn existing_colon_in_where(t: T) + | - move occurs because `t` has type `T`, which does not implement the `Copy` trait +... +LL | [t, t]; + | - ^ value used here after move + | | + | value moved here + | +help: consider further restricting this bound + | +LL | T: Copy, + | ++++ + +error[E0382]: use of moved value: `t` + --> $DIR/use_of_moved_value_copy_suggestions.rs:75:9 + | +LL | fn existing_colon(t: T) { + | - move occurs because `t` has type `T`, which does not implement the `Copy` trait +LL | +LL | [t, t]; + | - ^ value used here after move + | | + | value moved here + | +help: consider restricting type parameter `T` + | +LL | fn existing_colon(t: T) { + | ++++ + +error: aborting due to 11 previous errors For more information about this error, try `rustc --explain E0382`.