Auto merge of #83759 - SkiFire13:fix-diag, r=estebank

Handle more span edge cases in generics diagnostics

This should fix invalid suggestions that didn't account for empty bracket pairs (`<>`) or type bindings.

Author: bors

compiler/rustc_ast/src/ast.rs

@@ -278,7 +278,7 @@ impl ParenthesizedArgs {
             .cloned()
             .map(|input| AngleBracketedArg::Arg(GenericArg::Type(input)))
             .collect();
-        AngleBracketedArgs { span: self.span, args }
+        AngleBracketedArgs { span: self.inputs_span, args }
     }
 }
 

compiler/rustc_ast_lowering/src/lib.rs

@@ -62,7 +62,7 @@ use rustc_span::edition::Edition;
 use rustc_span::hygiene::ExpnId;
 use rustc_span::source_map::{respan, DesugaringKind};
 use rustc_span::symbol::{kw, sym, Ident, Symbol};
-use rustc_span::Span;
+use rustc_span::{Span, DUMMY_SP};
 use rustc_target::spec::abi::Abi;
 
 use smallvec::{smallvec, SmallVec};
@@ -2084,6 +2084,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
             args: &[],
             bindings: arena_vec![self; self.output_ty_binding(span, output_ty)],
             parenthesized: false,
+            span_ext: DUMMY_SP,
         });
 
         hir::GenericBound::LangItemTrait(
@@ -2788,6 +2789,7 @@ struct GenericArgsCtor<'hir> {
     args: SmallVec<[hir::GenericArg<'hir>; 4]>,
     bindings: &'hir [hir::TypeBinding<'hir>],
     parenthesized: bool,
+    span: Span,
 }
 
 impl<'hir> GenericArgsCtor<'hir> {
@@ -2800,6 +2802,7 @@ impl<'hir> GenericArgsCtor<'hir> {
             args: arena.alloc_from_iter(self.args),
             bindings: self.bindings,
             parenthesized: self.parenthesized,
+            span_ext: self.span,
         }
     }
 }

compiler/rustc_ast_lowering/src/path.rs

@@ -10,7 +10,7 @@ use rustc_hir::GenericArg;
 use rustc_session::lint::builtin::ELIDED_LIFETIMES_IN_PATHS;
 use rustc_session::lint::BuiltinLintDiagnostics;
 use rustc_span::symbol::Ident;
-use rustc_span::Span;
+use rustc_span::{BytePos, Span, DUMMY_SP};
 
 use smallvec::smallvec;
 use tracing::debug;
@@ -267,23 +267,34 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
                 },
             }
         } else {
-            self.lower_angle_bracketed_parameter_data(&Default::default(), param_mode, itctx)
+            (
+                GenericArgsCtor {
+                    args: Default::default(),
+                    bindings: &[],
+                    parenthesized: false,
+                    span: path_span.shrink_to_hi(),
+                },
+                param_mode == ParamMode::Optional,
+            )
         };
 
         let has_lifetimes =
             generic_args.args.iter().any(|arg| matches!(arg, GenericArg::Lifetime(_)));
-        let first_generic_span = generic_args
-            .args
-            .iter()
-            .map(|a| a.span())
-            .chain(generic_args.bindings.iter().map(|b| b.span))
-            .next();
         if !generic_args.parenthesized && !has_lifetimes {
+            // Note: these spans are used for diagnostics when they can't be inferred.
+            // See rustc_resolve::late::lifetimes::LifetimeContext::add_missing_lifetime_specifiers_label
+            let elided_lifetime_span = if generic_args.span.is_empty() {
+                // If there are no brackets, use the identifier span.
+                segment.ident.span
+            } else if generic_args.is_empty() {
+                // If there are brackets, but not generic arguments, then use the opening bracket
+                generic_args.span.with_hi(generic_args.span.lo() + BytePos(1))
+            } else {
+                // Else use an empty span right after the opening bracket.
+                generic_args.span.with_lo(generic_args.span.lo() + BytePos(1)).shrink_to_lo()
+            };
             generic_args.args = self
-                .elided_path_lifetimes(
-                    first_generic_span.map_or(segment.ident.span, |s| s.shrink_to_lo()),
-                    expected_lifetimes,
-                )
+                .elided_path_lifetimes(elided_lifetime_span, expected_lifetimes)
                 .map(GenericArg::Lifetime)
                 .chain(generic_args.args.into_iter())
                 .collect();
@@ -292,15 +303,15 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
                 let no_non_lt_args = generic_args.args.len() == expected_lifetimes;
                 let no_bindings = generic_args.bindings.is_empty();
                 let (incl_angl_brckt, insertion_sp, suggestion) = if no_non_lt_args && no_bindings {
-                    // If there are no (non-implicit) generic args or associated type
-                    // bindings, our suggestion includes the angle brackets.
+                    // If there are no generic args, our suggestion can include the angle brackets.
                     (true, path_span.shrink_to_hi(), format!("<{}>", anon_lt_suggestion))
                 } else {
-                    // Otherwise (sorry, this is kind of gross) we need to infer the
-                    // place to splice in the `'_, ` from the generics that do exist.
-                    let first_generic_span = first_generic_span
-                        .expect("already checked that non-lifetime args or bindings exist");
-                    (false, first_generic_span.shrink_to_lo(), format!("{}, ", anon_lt_suggestion))
+                    // Otherwise we'll insert a `'_, ` right after the opening bracket.
+                    let span = generic_args
+                        .span
+                        .with_lo(generic_args.span.lo() + BytePos(1))
+                        .shrink_to_lo();
+                    (false, span, format!("{}, ", anon_lt_suggestion))
                 };
                 match self.anonymous_lifetime_mode {
                     // In create-parameter mode we error here because we don't want to support
@@ -362,7 +373,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
             hir_id: Some(id),
             res: Some(self.lower_res(res)),
             infer_args,
-            args: if generic_args.is_empty() {
+            args: if generic_args.is_empty() && generic_args.span.is_empty() {
                 None
             } else {
                 Some(self.arena.alloc(generic_args.into_generic_args(self.arena)))
@@ -395,7 +406,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
             }
             AngleBracketedArg::Arg(_) => None,
         }));
-        let ctor = GenericArgsCtor { args, bindings, parenthesized: false };
+        let ctor = GenericArgsCtor { args, bindings, parenthesized: false, span: data.span };
         (ctor, !has_non_lt_args && param_mode == ParamMode::Optional)
     }
 
@@ -420,7 +431,12 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
             let args = smallvec![GenericArg::Type(this.ty_tup(*inputs_span, inputs))];
             let binding = this.output_ty_binding(output_ty.span, output_ty);
             (
-                GenericArgsCtor { args, bindings: arena_vec![this; binding], parenthesized: true },
+                GenericArgsCtor {
+                    args,
+                    bindings: arena_vec![this; binding],
+                    parenthesized: true,
+                    span: data.inputs_span,
+                },
                 false,
             )
         })
@@ -436,7 +452,12 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
         let kind = hir::TypeBindingKind::Equality { ty };
         let args = arena_vec![self;];
         let bindings = arena_vec![self;];
-        let gen_args = self.arena.alloc(hir::GenericArgs { args, bindings, parenthesized: false });
+        let gen_args = self.arena.alloc(hir::GenericArgs {
+            args,
+            bindings,
+            parenthesized: false,
+            span_ext: DUMMY_SP,
+        });
         hir::TypeBinding { hir_id: self.next_id(), gen_args, span, ident, kind }
     }
 }

compiler/rustc_hir/src/hir.rs

@@ -12,7 +12,7 @@ pub use rustc_ast::{CaptureBy, Movability, Mutability};
 use rustc_ast::{InlineAsmOptions, InlineAsmTemplatePiece};
 use rustc_data_structures::sync::{par_for_each_in, Send, Sync};
 use rustc_macros::HashStable_Generic;
-use rustc_span::source_map::{SourceMap, Spanned};
+use rustc_span::source_map::Spanned;
 use rustc_span::symbol::{kw, sym, Ident, Symbol};
 use rustc_span::{def_id::LocalDefId, BytePos};
 use rustc_span::{MultiSpan, Span, DUMMY_SP};
@@ -314,11 +314,18 @@ pub struct GenericArgs<'hir> {
     /// This is required mostly for pretty-printing and diagnostics,
     /// but also for changing lifetime elision rules to be "function-like".
     pub parenthesized: bool,
+    /// The span encompassing arguments and the surrounding brackets `<>` or `()`
+    ///       Foo<A, B, AssocTy = D>           Fn(T, U, V) -> W
+    ///          ^^^^^^^^^^^^^^^^^^^             ^^^^^^^^^
+    /// Note that this may be:
+    /// - empty, if there are no generic brackets (but there may be hidden lifetimes)
+    /// - dummy, if this was generated while desugaring
+    pub span_ext: Span,
 }
 
 impl GenericArgs<'_> {
     pub const fn none() -> Self {
-        Self { args: &[], bindings: &[], parenthesized: false }
+        Self { args: &[], bindings: &[], parenthesized: false, span_ext: DUMMY_SP }
     }
 
     pub fn inputs(&self) -> &[Ty<'_>] {
@@ -356,33 +363,17 @@ impl GenericArgs<'_> {
         own_counts
     }
 
+    /// The span encompassing the text inside the surrounding brackets.
+    /// It will also include bindings if they aren't in the form `-> Ret`
+    /// Returns `None` if the span is empty (e.g. no brackets) or dummy
     pub fn span(&self) -> Option<Span> {
-        self.args
-            .iter()
-            .filter(|arg| !arg.is_synthetic())
-            .map(|arg| arg.span())
-            .reduce(|span1, span2| span1.to(span2))
+        let span_ext = self.span_ext()?;
+        Some(span_ext.with_lo(span_ext.lo() + BytePos(1)).with_hi(span_ext.hi() - BytePos(1)))
     }
 
     /// Returns span encompassing arguments and their surrounding `<>` or `()`
-    pub fn span_ext(&self, sm: &SourceMap) -> Option<Span> {
-        let mut span = self.span()?;
-
-        let (o, c) = if self.parenthesized { ('(', ')') } else { ('<', '>') };
-
-        if let Ok(snippet) = sm.span_to_snippet(span) {
-            let snippet = snippet.as_bytes();
-
-            if snippet[0] != (o as u8) || snippet[snippet.len() - 1] != (c as u8) {
-                span = sm.span_extend_to_prev_char(span, o, true);
-                span = span.with_lo(span.lo() - BytePos(1));
-
-                span = sm.span_extend_to_next_char(span, c, true);
-                span = span.with_hi(span.hi() + BytePos(1));
-            }
-        }
-
-        Some(span)
+    pub fn span_ext(&self) -> Option<Span> {
+        Some(self.span_ext).filter(|span| !span.is_empty())
     }
 
     pub fn is_empty(&self) -> bool {

compiler/rustc_resolve/src/late/diagnostics.rs

@@ -1821,7 +1821,7 @@ impl<'tcx> LifetimeContext<'_, 'tcx> {
     crate fn add_missing_lifetime_specifiers_label(
         &self,
         err: &mut DiagnosticBuilder<'_>,
-        spans_with_counts: Vec<(Span, usize)>,
+        mut spans_with_counts: Vec<(Span, usize)>,
         lifetime_names: &FxHashSet<Symbol>,
         lifetime_spans: Vec<Span>,
         params: &[ElisionFailureInfo],
@@ -1831,13 +1831,21 @@ impl<'tcx> LifetimeContext<'_, 'tcx> {
             .map(|(span, _)| self.tcx.sess.source_map().span_to_snippet(*span).ok())
             .collect();
 
-        for (span, count) in &spans_with_counts {
+        // Empty generics are marked with a span of "<", but since from now on
+        // that information is in the snippets it can be removed from the spans.
+        for ((span, _), snippet) in spans_with_counts.iter_mut().zip(&snippets) {
+            if snippet.as_deref() == Some("<") {
+                *span = span.shrink_to_hi();
+            }
+        }
+
+        for &(span, count) in &spans_with_counts {
             err.span_label(
-                *span,
+                span,
                 format!(
                     "expected {} lifetime parameter{}",
-                    if *count == 1 { "named".to_string() } else { count.to_string() },
-                    pluralize!(*count),
+                    if count == 1 { "named".to_string() } else { count.to_string() },
+                    pluralize!(count),
                 ),
             );
         }
@@ -1982,6 +1990,14 @@ impl<'tcx> LifetimeContext<'_, 'tcx> {
                                                 .collect::<Vec<_>>()
                                                 .join(", "),
                                         )
+                                    } else if snippet == "<" || snippet == "(" {
+                                        (
+                                            span.shrink_to_hi(),
+                                            std::iter::repeat("'static")
+                                                .take(count)
+                                                .collect::<Vec<_>>()
+                                                .join(", "),
+                                        )
                                     } else {
                                         (
                                             span.shrink_to_hi(),
@@ -1990,7 +2006,7 @@ impl<'tcx> LifetimeContext<'_, 'tcx> {
                                                 std::iter::repeat("'static")
                                                     .take(count)
                                                     .collect::<Vec<_>>()
-                                                    .join(", ")
+                                                    .join(", "),
                                             ),
                                         )
                                     }
@@ -2045,6 +2061,9 @@ impl<'tcx> LifetimeContext<'_, 'tcx> {
                         Some("&") => Some(Box::new(|name| format!("&{} ", name))),
                         Some("'_") => Some(Box::new(|n| n.to_string())),
                         Some("") => Some(Box::new(move |n| format!("{}, ", n).repeat(count))),
+                        Some("<") => Some(Box::new(move |n| {
+                            std::iter::repeat(n).take(count).collect::<Vec<_>>().join(", ")
+                        })),
                         Some(snippet) if !snippet.ends_with('>') => Some(Box::new(move |name| {
                             format!(
                                 "{}<{}>",
@@ -2071,6 +2090,9 @@ impl<'tcx> LifetimeContext<'_, 'tcx> {
                         Some("") => {
                             Some(std::iter::repeat("'a, ").take(count).collect::<Vec<_>>().join(""))
                         }
+                        Some("<") => {
+                            Some(std::iter::repeat("'a").take(count).collect::<Vec<_>>().join(", "))
+                        }
                         Some(snippet) => Some(format!(
                             "{}<{}>",
                             snippet,