macros: simplify field ordering in diag derive

Following the approach taken in earlier commits to separate formatting
initialization from use in the subdiagnostic derive, simplify the
diagnostic derive by removing the field-ordering logic that previously
solved this problem.

Signed-off-by: David Wood <[email protected]>

Author: davidtwco

compiler/rustc_macros/src/diagnostics/diagnostic.rs

@@ -52,8 +52,10 @@ impl<'a> DiagnosticDerive<'a> {
                 }
             };
 
+            let formatting_init = &builder.formatting_init;
             quote! {
                 #init
+                #formatting_init
                 #preamble
                 #body
                 #diag
@@ -101,9 +103,10 @@ impl<'a> LintDiagnosticDerive<'a> {
             let body = builder.body(&variant);
 
             let diag = &builder.parent.diag;
-
+            let formatting_init = &builder.formatting_init;
             quote! {
                 #preamble
+                #formatting_init
                 #body
                 #diag
             }

compiler/rustc_macros/src/diagnostics/diagnostic_builder.rs

@@ -5,9 +5,9 @@ use crate::diagnostics::error::{
     DiagnosticDeriveError,
 };
 use crate::diagnostics::utils::{
-    bind_style_of_field, build_field_mapping, report_error_if_not_applied_to_span,
-    report_type_error, should_generate_set_arg, type_is_unit, type_matches_path, FieldInfo,
-    FieldInnerTy, FieldMap, HasFieldMap, SetOnce, SpannedOption, SubdiagnosticKind,
+    build_field_mapping, report_error_if_not_applied_to_span, report_type_error,
+    should_generate_set_arg, type_is_unit, type_matches_path, FieldInfo, FieldInnerTy, FieldMap,
+    HasFieldMap, SetOnce, SpannedOption, SubdiagnosticKind,
 };
 use proc_macro2::{Ident, Span, TokenStream};
 use quote::{format_ident, quote};
@@ -40,6 +40,9 @@ pub(crate) struct DiagnosticDeriveVariantBuilder<'parent> {
     /// The parent builder for the entire type.
     pub parent: &'parent DiagnosticDeriveBuilder,
 
+    /// Initialization of format strings for code suggestions.
+    pub formatting_init: TokenStream,
+
     /// Span of the struct or the enum variant.
     pub span: proc_macro::Span,
 
@@ -88,19 +91,7 @@ impl DiagnosticDeriveBuilder {
             }
         }
 
-        for variant in structure.variants_mut() {
-            // First, change the binding style of each field based on the code that will be
-            // generated for the field - e.g. `set_arg` calls needs by-move bindings, whereas
-            // `set_primary_span` only needs by-ref.
-            variant.bind_with(|bi| bind_style_of_field(bi.ast()).0);
-
-            // Then, perform a stable sort on bindings which generates code for by-ref bindings
-            // before code generated for by-move bindings. Any code generated for the by-ref
-            // bindings which creates a reference to the by-move fields will happen before the
-            // by-move bindings move those fields and make them inaccessible.
-            variant.bindings_mut().sort_by_cached_key(|bi| bind_style_of_field(bi.ast()));
-        }
-
+        structure.bind_with(|_| synstructure::BindStyle::Move);
         let variants = structure.each_variant(|variant| {
             let span = match structure.ast().data {
                 syn::Data::Struct(..) => span,
@@ -112,6 +103,7 @@ impl DiagnosticDeriveBuilder {
                 parent: &self,
                 span,
                 field_map: build_field_mapping(variant),
+                formatting_init: TokenStream::new(),
                 slug: None,
                 code: None,
             };
@@ -143,16 +135,14 @@ impl<'a> DiagnosticDeriveVariantBuilder<'a> {
 
     /// Generates calls to `span_label` and similar functions based on the attributes on fields or
     /// calls to `set_arg` when no attributes are present.
-    ///
-    /// Expects use of `Self::each_variant` which will have sorted bindings so that by-ref bindings
-    /// (which may create references to by-move bindings) have their code generated first -
-    /// necessary as code for suggestions uses formatting machinery and the value of other fields
-    /// (any given field can be referenced multiple times, so must be accessed through a borrow);
-    /// and when passing fields to `add_subdiagnostic` or `set_arg` for Fluent, fields must be
-    /// accessed by-move.
     pub fn body<'s>(&mut self, variant: &VariantInfo<'s>) -> TokenStream {
         let mut body = quote! {};
-        for binding in variant.bindings() {
+        // Generate `set_arg` calls first..
+        for binding in variant.bindings().iter().filter(|bi| should_generate_set_arg(bi.ast())) {
+            body.extend(self.generate_field_code(binding));
+        }
+        // ..and then subdiagnostic additions.
+        for binding in variant.bindings().iter().filter(|bi| !should_generate_set_arg(bi.ast())) {
             body.extend(self.generate_field_attrs_code(binding));
         }
         body
@@ -274,24 +264,27 @@ impl<'a> DiagnosticDeriveVariantBuilder<'a> {
         }
     }
 
-    fn generate_field_attrs_code(&mut self, binding_info: &BindingInfo<'_>) -> TokenStream {
+    fn generate_field_code(&mut self, binding_info: &BindingInfo<'_>) -> TokenStream {
+        let diag = &self.parent.diag;
+
         let field = binding_info.ast();
         let field_binding = &binding_info.binding;
 
-        if should_generate_set_arg(&field) {
-            let diag = &self.parent.diag;
-            let ident = field.ident.as_ref().unwrap();
-            // strip `r#` prefix, if present
-            let ident = format_ident!("{}", ident);
-            return quote! {
-                #diag.set_arg(
-                    stringify!(#ident),
-                    #field_binding
-                );
-            };
+        let ident = field.ident.as_ref().unwrap();
+        let ident = format_ident!("{}", ident); // strip `r#` prefix, if present
+
+        quote! {
+            #diag.set_arg(
+                stringify!(#ident),
+                #field_binding
+            );
         }
+    }
+
+    fn generate_field_attrs_code(&mut self, binding_info: &BindingInfo<'_>) -> TokenStream {
+        let field = binding_info.ast();
+        let field_binding = &binding_info.binding;
 
-        let needs_move = bind_style_of_field(&field).is_move();
         let inner_ty = FieldInnerTy::from_type(&field.ty);
 
         field
@@ -304,10 +297,8 @@ impl<'a> DiagnosticDeriveVariantBuilder<'a> {
                 let (binding, needs_destructure) = if needs_clone {
                     // `primary_span` can accept a `Vec<Span>` so don't destructure that.
                     (quote! { #field_binding.clone() }, false)
-                } else if needs_move {
-                    (quote! { #field_binding }, true)
                 } else {
-                    (quote! { *#field_binding }, true)
+                    (quote! { #field_binding }, true)
                 };
 
                 let generated_code = self
@@ -440,8 +431,8 @@ impl<'a> DiagnosticDeriveVariantBuilder<'a> {
                     .unwrap_or_else(|| quote! { rustc_errors::Applicability::Unspecified });
                 let style = suggestion_kind.to_suggestion_style();
 
+                self.formatting_init.extend(code_init);
                 Ok(quote! {
-                    #code_init
                     #diag.span_suggestion_with_style(
                         #span_field,
                         rustc_errors::fluent::#slug,
@@ -490,7 +481,7 @@ impl<'a> DiagnosticDeriveVariantBuilder<'a> {
             // If `ty` is `Span` w/out applicability, then use `Applicability::Unspecified`.
             ty @ Type::Path(..) if type_matches_path(ty, &["rustc_span", "Span"]) => {
                 let binding = &info.binding.binding;
-                Ok((quote!(*#binding), None))
+                Ok((quote!(#binding), None))
             }
             // If `ty` is `(Span, Applicability)` then return tokens accessing those.
             Type::Tuple(tup) => {

compiler/rustc_macros/src/diagnostics/utils.rs

@@ -5,13 +5,12 @@ use proc_macro::Span;
 use proc_macro2::TokenStream;
 use quote::{format_ident, quote, ToTokens};
 use std::cell::RefCell;
-use std::cmp::Ordering;
 use std::collections::{BTreeSet, HashMap};
 use std::fmt;
 use std::str::FromStr;
 use syn::{spanned::Spanned, Attribute, Field, Meta, Type, TypeTuple};
 use syn::{MetaList, MetaNameValue, NestedMeta, Path};
-use synstructure::{BindStyle, BindingInfo, VariantInfo};
+use synstructure::{BindingInfo, VariantInfo};
 
 use super::error::invalid_nested_attr;
 
@@ -650,65 +649,8 @@ impl quote::IdentFragment for SubdiagnosticKind {
     }
 }
 
-/// Wrapper around `synstructure::BindStyle` which implements `Ord`.
-#[derive(PartialEq, Eq)]
-pub(super) struct OrderedBindStyle(pub(super) BindStyle);
-
-impl OrderedBindStyle {
-    /// Is `BindStyle::Move` or `BindStyle::MoveMut`?
-    pub(super) fn is_move(&self) -> bool {
-        matches!(self.0, BindStyle::Move | BindStyle::MoveMut)
-    }
-}
-
-impl Ord for OrderedBindStyle {
-    fn cmp(&self, other: &Self) -> Ordering {
-        match (self.is_move(), other.is_move()) {
-            // If both `self` and `other` are the same, then ordering is equal.
-            (true, true) | (false, false) => Ordering::Equal,
-            // If `self` is not a move then it should be considered less than `other` (so that
-            // references are sorted first).
-            (false, _) => Ordering::Less,
-            // If `self` is a move then it must be greater than `other` (again, so that references
-            // are sorted first).
-            (true, _) => Ordering::Greater,
-        }
-    }
-}
-
-impl PartialOrd for OrderedBindStyle {
-    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
-        Some(self.cmp(other))
-    }
-}
-
 /// Returns `true` if `field` should generate a `set_arg` call rather than any other diagnostic
 /// call (like `span_label`).
 pub(super) fn should_generate_set_arg(field: &Field) -> bool {
     field.attrs.is_empty()
 }
-
-/// Returns `true` if `field` needs to have code generated in the by-move branch of the
-/// generated derive rather than the by-ref branch.
-pub(super) fn bind_style_of_field(field: &Field) -> OrderedBindStyle {
-    let generates_set_arg = should_generate_set_arg(field);
-    let is_multispan = type_matches_path(&field.ty, &["rustc_errors", "MultiSpan"]);
-    // FIXME(davidtwco): better support for one field needing to be in the by-move and
-    // by-ref branches.
-    let is_subdiagnostic = field
-        .attrs
-        .iter()
-        .map(|attr| attr.path.segments.last().unwrap().ident.to_string())
-        .any(|attr| attr == "subdiagnostic");
-
-    // `set_arg` calls take their argument by-move..
-    let needs_move = generates_set_arg
-        // If this is a `MultiSpan` field then it needs to be moved to be used by any
-        // attribute..
-        || is_multispan
-        // If this a `#[subdiagnostic]` then it needs to be moved as the other diagnostic is
-        // unlikely to be `Copy`..
-        || is_subdiagnostic;
-
-    OrderedBindStyle(if needs_move { BindStyle::Move } else { BindStyle::Ref })
-}