Skip to content

Commit

Permalink
Auto merge of #97121 - pvdrz:do-subdiagnostics-later, r=davidtwco
Browse files Browse the repository at this point in the history 
Avoid double binding of subdiagnostics inside `#[derive(SessionDiagnostic)]`

r? `@davidtwco`
  • Loading branch information
@bors
bors committed May 24, 2022
2 parents 43d9f38 + 8227e69 commit b2eba05
Show file tree
Hide file tree
Showing 4 changed files with 91 additions and 84 deletions.
155 changes: 83 additions & 72 deletions compiler/rustc_macros/src/diagnostics/diagnostic.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand Down Expand Up @@ -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())
});

quote! { #(#result);* }
});
// 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;

(!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);
Expand Down Expand Up @@ -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 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,
)?;
let inner_ty = FieldInnerTy::from_type(&field.ty);

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()
}
}

Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_macros/src/diagnostics/subdiagnostic.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
3 changes: 1 addition & 2 deletions compiler/rustc_macros/src/diagnostics/utils.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down Expand Up @@ -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,
Expand Down
16 changes: 7 additions & 9 deletions compiler/rustc_parse/src/parser/diagnostics.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
Expand Down Expand Up @@ -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);
Expand All @@ -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 },
Expand Down

0 comments on commit b2eba05

Please sign in to comment.