Auto merge of #97121 - pvdrz:do-subdiagnostics-later, r=davidtwco

Avoid double binding of subdiagnostics inside `#[derive(SessionDiagnostic)]`

r? `@davidtwco`

compiler/rustc_macros/src/diagnostics/diagnostic.rs

@@ -13,7 +13,7 @@ use quote::{format_ident, quote};
 use std::collections::HashMap;
 use std::str::FromStr;
 use syn::{spanned::Spanned, Attribute, Meta, MetaList, MetaNameValue, Type};
-use synstructure::Structure;
+use synstructure::{BindingInfo, Structure};
 
 /// The central struct for constructing the `into_diagnostic` method from an annotated struct.
 pub(crate) struct SessionDiagnosticDerive<'a> {
@@ -71,55 +71,42 @@ impl<'a> SessionDiagnosticDerive<'a> {
                     }
                 };
 
+                // Keep track of which fields are subdiagnostics or have no attributes.
+                let mut subdiagnostics_or_empty = std::collections::HashSet::new();
+
                 // Generates calls to `span_label` and similar functions based on the attributes
                 // on fields. Code for suggestions uses formatting machinery and the value of
                 // other fields - because any given field can be referenced multiple times, it
-                // should be accessed through a borrow. When passing fields to `set_arg` (which
-                // happens below) for Fluent, we want to move the data, so that has to happen
-                // in a separate pass over the fields.
-                let attrs = structure.each(|field_binding| {
-                    let field = field_binding.ast();
-                    let result = field.attrs.iter().map(|attr| {
-                        builder
-                            .generate_field_attr_code(
-                                attr,
-                                FieldInfo {
-                                    vis: &field.vis,
-                                    binding: field_binding,
-                                    ty: &field.ty,
-                                    span: &field.span(),
-                                },
-                            )
-                            .unwrap_or_else(|v| v.to_compile_error())
-                    });
+                // should be accessed through a borrow. When passing fields to `add_subdiagnostic`
+                // or `set_arg` (which happens below) for Fluent, we want to move the data, so that
+                // has to happen in a separate pass over the fields.
+                let attrs = structure
+                    .clone()
+                    .filter(|field_binding| {
+                        let attrs = &field_binding.ast().attrs;
 
-                    quote! { #(#result);* }
-                });
+                        (!attrs.is_empty()
+                            && attrs.iter().all(|attr| {
+                                "subdiagnostic"
+                                    != attr.path.segments.last().unwrap().ident.to_string()
+                            }))
+                            || {
+                                subdiagnostics_or_empty.insert(field_binding.binding.clone());
+                                false
+                            }
+                    })
+                    .each(|field_binding| builder.generate_field_attrs_code(field_binding));
 
-                // When generating `set_arg` calls, move data rather than borrow it to avoid
-                // requiring clones - this must therefore be the last use of each field (for
-                // example, any formatting machinery that might refer to a field should be
-                // generated already).
                 structure.bind_with(|_| synstructure::BindStyle::Move);
-                let args = structure.each(|field_binding| {
-                    let field = field_binding.ast();
-                    // When a field has attributes like `#[label]` or `#[note]` then it doesn't
-                    // need to be passed as an argument to the diagnostic. But when a field has no
-                    // attributes then it must be passed as an argument to the diagnostic so that
-                    // it can be referred to by Fluent messages.
-                    if field.attrs.is_empty() {
-                        let diag = &builder.diag;
-                        let ident = field_binding.ast().ident.as_ref().unwrap();
-                        quote! {
-                            #diag.set_arg(
-                                stringify!(#ident),
-                                #field_binding
-                            );
-                        }
-                    } else {
-                        quote! {}
-                    }
-                });
+                // When a field has attributes like `#[label]` or `#[note]` then it doesn't
+                // need to be passed as an argument to the diagnostic. But when a field has no
+                // attributes or a `#[subdiagnostic]` attribute then it must be passed as an
+                // argument to the diagnostic so that it can be referred to by Fluent messages.
+                let args = structure
+                    .filter(|field_binding| {
+                        subdiagnostics_or_empty.contains(&field_binding.binding)
+                    })
+                    .each(|field_binding| builder.generate_field_attrs_code(field_binding));
 
                 let span = ast.span().unwrap();
                 let (diag, sess) = (&builder.diag, &builder.sess);
@@ -347,36 +334,60 @@ impl SessionDiagnosticDeriveBuilder {
         Ok(tokens.drain(..).collect())
     }
 
-    fn generate_field_attr_code(
-        &mut self,
-        attr: &syn::Attribute,
-        info: FieldInfo<'_>,
-    ) -> Result<TokenStream, SessionDiagnosticDeriveError> {
-        let field_binding = &info.binding.binding;
+    fn generate_field_attrs_code(&mut self, binding_info: &BindingInfo<'_>) -> TokenStream {
+        let field = binding_info.ast();
+        let field_binding = &binding_info.binding;
 
-        let inner_ty = FieldInnerTy::from_type(&info.ty);
-        let name = attr.path.segments.last().unwrap().ident.to_string();
-        let (binding, needs_destructure) = match (name.as_str(), &inner_ty) {
-            // `primary_span` can accept a `Vec<Span>` so don't destructure that.
-            ("primary_span", FieldInnerTy::Vec(_)) => (quote! { #field_binding.clone() }, false),
-            _ => (quote! { *#field_binding }, true),
-        };
+        let inner_ty = FieldInnerTy::from_type(&field.ty);
 
-        let generated_code = self.generate_inner_field_code(
-            attr,
-            FieldInfo {
-                vis: info.vis,
-                binding: info.binding,
-                ty: inner_ty.inner_type().unwrap_or(&info.ty),
-                span: info.span,
-            },
-            binding,
-        )?;
-
-        if needs_destructure {
-            Ok(inner_ty.with(field_binding, generated_code))
+        // When generating `set_arg` or `add_subdiagnostic` calls, move data rather than
+        // borrow it to avoid requiring clones - this must therefore be the last use of
+        // each field (for example, any formatting machinery that might refer to a field
+        // should be generated already).
+        if field.attrs.is_empty() {
+            let diag = &self.diag;
+            let ident = field.ident.as_ref().unwrap();
+            quote! {
+                #diag.set_arg(
+                    stringify!(#ident),
+                    #field_binding
+                );
+            }
         } else {
-            Ok(generated_code)
+            field
+                .attrs
+                .iter()
+                .map(move |attr| {
+                    let name = attr.path.segments.last().unwrap().ident.to_string();
+                    let (binding, needs_destructure) = match (name.as_str(), &inner_ty) {
+                        // `primary_span` can accept a `Vec<Span>` so don't destructure that.
+                        ("primary_span", FieldInnerTy::Vec(_)) => {
+                            (quote! { #field_binding.clone() }, false)
+                        }
+                        // `subdiagnostics` are not derefed because they are bound by value.
+                        ("subdiagnostic", _) => (quote! { #field_binding }, true),
+                        _ => (quote! { *#field_binding }, true),
+                    };
+
+                    let generated_code = self
+                        .generate_inner_field_code(
+                            attr,
+                            FieldInfo {
+                                binding: binding_info,
+                                ty: inner_ty.inner_type().unwrap_or(&field.ty),
+                                span: &field.span(),
+                            },
+                            binding,
+                        )
+                        .unwrap_or_else(|v| v.to_compile_error());
+
+                    if needs_destructure {
+                        inner_ty.with(field_binding, generated_code)
+                    } else {
+                        generated_code
+                    }
+                })
+                .collect()
         }
     }
 

compiler/rustc_macros/src/diagnostics/subdiagnostic.rs

@@ -303,7 +303,6 @@ impl<'a> SessionSubdiagnosticDeriveBuilder<'a> {
 
         let inner_ty = FieldInnerTy::from_type(&ast.ty);
         let info = FieldInfo {
-            vis: &ast.vis,
             binding: binding,
             ty: inner_ty.inner_type().unwrap_or(&ast.ty),
             span: &ast.span(),

compiler/rustc_macros/src/diagnostics/utils.rs

@@ -4,7 +4,7 @@ use proc_macro2::TokenStream;
 use quote::{format_ident, quote, ToTokens};
 use std::collections::BTreeSet;
 use std::str::FromStr;
-use syn::{spanned::Spanned, Attribute, Meta, Type, TypeTuple, Visibility};
+use syn::{spanned::Spanned, Attribute, Meta, Type, TypeTuple};
 use synstructure::BindingInfo;
 
 /// Checks whether the type name of `ty` matches `name`.
@@ -158,7 +158,6 @@ impl<'ty> FieldInnerTy<'ty> {
 /// Field information passed to the builder. Deliberately omits attrs to discourage the
 /// `generate_*` methods from walking the attributes themselves.
 pub(crate) struct FieldInfo<'a> {
-    pub(crate) vis: &'a Visibility,
     pub(crate) binding: &'a BindingInfo<'a>,
     pub(crate) ty: &'a Type,
     pub(crate) span: &'a proc_macro2::Span,

compiler/rustc_parse/src/parser/diagnostics.rs

@@ -254,23 +254,23 @@ struct AmbiguousPlus {
 
 #[derive(SessionDiagnostic)]
 #[error(code = "E0178", slug = "parser-maybe-recover-from-bad-type-plus")]
-struct BadTypePlus<'a> {
+struct BadTypePlus {
     pub ty: String,
     #[primary_span]
     pub span: Span,
     #[subdiagnostic]
-    pub sub: BadTypePlusSub<'a>,
+    pub sub: BadTypePlusSub,
 }
 
-#[derive(SessionSubdiagnostic, Clone, Copy)]
-pub enum BadTypePlusSub<'a> {
+#[derive(SessionSubdiagnostic)]
+pub enum BadTypePlusSub {
     #[suggestion(
         slug = "parser-add-paren",
         code = "{sum_with_parens}",
         applicability = "machine-applicable"
     )]
     AddParen {
-        sum_with_parens: &'a str,
+        sum_with_parens: String,
         #[primary_span]
         span: Span,
     },
@@ -1289,11 +1289,9 @@ impl<'a> Parser<'a> {
         let bounds = self.parse_generic_bounds(None)?;
         let sum_span = ty.span.to(self.prev_token.span);
 
-        let sum_with_parens: String;
-
         let sub = match ty.kind {
             TyKind::Rptr(ref lifetime, ref mut_ty) => {
-                sum_with_parens = pprust::to_string(|s| {
+                let sum_with_parens = pprust::to_string(|s| {
                     s.s.word("&");
                     s.print_opt_lifetime(lifetime);
                     s.print_mutability(mut_ty.mutbl, false);
@@ -1303,7 +1301,7 @@ impl<'a> Parser<'a> {
                     s.pclose()
                 });
 
-                BadTypePlusSub::AddParen { sum_with_parens: &sum_with_parens, span: sum_span }
+                BadTypePlusSub::AddParen { sum_with_parens, span: sum_span }
             }
             TyKind::Ptr(..) | TyKind::BareFn(..) => BadTypePlusSub::ForgotParen { span: sum_span },
             _ => BadTypePlusSub::ExpectPath { span: sum_span },