Skip to content

Commit

Permalink
macros: simplify field ordering in diag derive
Browse files Browse the repository at this point in the history 
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 <david.wood@huawei.com>
  • Loading branch information
@davidtwco
davidtwco committed Oct 4, 2022
1 parent f9c6a5d commit 4d076b8
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 102 deletions.
5 changes: 4 additions & 1 deletion compiler/rustc_macros/src/diagnostics/diagnostic.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,10 @@ impl<'a> DiagnosticDerive<'a> {
}
};

let formatting_init = &builder.formatting_init;
quote! {
#init
#formatting_init
#preamble
#body
#diag
Expand Down Expand Up @@ -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
}
Expand Down
75 changes: 33 additions & 42 deletions compiler/rustc_macros/src/diagnostics/diagnostic_builder.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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,

Expand Down Expand Up @@ -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,
Expand All @@ -112,6 +103,7 @@ impl DiagnosticDeriveBuilder {
parent: &self,
span,
field_map: build_field_mapping(variant),
formatting_init: TokenStream::new(),
slug: None,
code: None,
};
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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) => {
Expand Down
60 changes: 1 addition & 59 deletions compiler/rustc_macros/src/diagnostics/utils.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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 })
}

0 comments on commit 4d076b8

Please sign in to comment.