From 462c1c846b5488024c2e5fec6e1ed4700642b49e Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Tue, 17 May 2022 13:10:15 -0500 Subject: [PATCH 1/6] generate code for `subdiagnostic` fields in the second `match` --- .../src/diagnostics/diagnostic.rs | 74 ++++++++++++++----- .../rustc_parse/src/parser/diagnostics.rs | 16 ++-- 2 files changed, 63 insertions(+), 27 deletions(-) diff --git a/compiler/rustc_macros/src/diagnostics/diagnostic.rs b/compiler/rustc_macros/src/diagnostics/diagnostic.rs index dac3e986e7a..adb25e1fdef 100644 --- a/compiler/rustc_macros/src/diagnostics/diagnostic.rs +++ b/compiler/rustc_macros/src/diagnostics/diagnostic.rs @@ -71,30 +71,45 @@ impl<'a> SessionDiagnosticDerive<'a> { } }; + // Keep track of which fields are subdiagnostics + let mut subdiagnostics = 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()) - }); + let attrs = structure + .clone() + // Remove the fields that have a `subdiagnostic` attribute. + .filter(|field_binding| { + field_binding.ast().attrs.iter().all(|attr| { + "subdiagnostic" != attr.path.segments.last().unwrap().ident.to_string() + || { + subdiagnostics.insert(field_binding.binding.clone()); + false + } + }) + }) + .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);* } - }); + quote! { #(#result);* } + }); // 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 @@ -107,7 +122,7 @@ impl<'a> SessionDiagnosticDerive<'a> { // 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 tokens = if field.attrs.is_empty() { let diag = &builder.diag; let ident = field_binding.ast().ident.as_ref().unwrap(); quote! { @@ -118,6 +133,27 @@ impl<'a> SessionDiagnosticDerive<'a> { } } else { quote! {} + }; + // If this field had a subdiagnostic attribute, we generate the code here to + // avoid binding it twice. + if subdiagnostics.contains(&field_binding.binding) { + 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);* #tokens } + } else { + tokens } }); @@ -359,6 +395,8 @@ impl SessionDiagnosticDeriveBuilder { let (binding, needs_destructure) = match (name.as_str(), &inner_ty) { // `primary_span` can accept a `Vec` 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), }; diff --git a/compiler/rustc_parse/src/parser/diagnostics.rs b/compiler/rustc_parse/src/parser/diagnostics.rs index 3b2ce0de509..0c20bf49c4f 100644 --- a/compiler/rustc_parse/src/parser/diagnostics.rs +++ b/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 }, From 19e1f73085c1e74f06512074805c5960db51891f Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Wed, 18 May 2022 10:50:59 -0500 Subject: [PATCH 2/6] generate `set_arg` code inside `generate_field_attrs_code` --- .../src/diagnostics/diagnostic.rs | 196 +++++++++--------- 1 file changed, 99 insertions(+), 97 deletions(-) diff --git a/compiler/rustc_macros/src/diagnostics/diagnostic.rs b/compiler/rustc_macros/src/diagnostics/diagnostic.rs index adb25e1fdef..25cba2df7f1 100644 --- a/compiler/rustc_macros/src/diagnostics/diagnostic.rs +++ b/compiler/rustc_macros/src/diagnostics/diagnostic.rs @@ -71,91 +71,72 @@ impl<'a> SessionDiagnosticDerive<'a> { } }; - // Keep track of which fields are subdiagnostics - let mut subdiagnostics = std::collections::HashSet::new(); + // 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. + // 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() - // Remove the fields that have a `subdiagnostic` attribute. .filter(|field_binding| { - field_binding.ast().attrs.iter().all(|attr| { - "subdiagnostic" != attr.path.segments.last().unwrap().ident.to_string() - || { - subdiagnostics.insert(field_binding.binding.clone()); - false - } - }) + 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| { 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()) - }); + let field_span = field.span(); - quote! { #(#result);* } + builder.generate_field_attrs_code( + &field.attrs, + FieldInfo { + vis: &field.vis, + binding: field_binding, + ty: &field.ty, + span: &field_span, + }, + ) }); - // 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). + // 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). 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. - let tokens = 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! {} - }; - // If this field had a subdiagnostic attribute, we generate the code here to - // avoid binding it twice. - if subdiagnostics.contains(&field_binding.binding) { - 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()) - }); + // 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| { + let field = field_binding.ast(); + let field_span = field.span(); - quote! { #(#result);* #tokens } - } else { - tokens - } - }); + builder.generate_field_attrs_code( + &field.attrs, + FieldInfo { + vis: &field.vis, + binding: field_binding, + ty: &field.ty, + span: &field_span, + }, + ) + }); let span = ast.span().unwrap(); let (diag, sess) = (&builder.diag, &builder.sess); @@ -383,38 +364,59 @@ impl SessionDiagnosticDeriveBuilder { Ok(tokens.drain(..).collect()) } - fn generate_field_attr_code( - &mut self, - attr: &syn::Attribute, - info: FieldInfo<'_>, - ) -> Result { + fn generate_field_attrs_code<'a>( + &'a mut self, + attrs: &'a [syn::Attribute], + info: FieldInfo<'a>, + ) -> TokenStream { let field_binding = &info.binding.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` 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 { - 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)) + if attrs.is_empty() { + let diag = &self.diag; + let ident = info.binding.ast().ident.as_ref().unwrap(); + quote! { + #diag.set_arg( + stringify!(#ident), + #field_binding + ); + } } else { - Ok(generated_code) + 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` 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 { + vis: info.vis, + binding: info.binding, + ty: inner_ty.inner_type().unwrap_or(&info.ty), + span: info.span, + }, + binding, + ) + .unwrap_or_else(|v| v.to_compile_error()); + + if needs_destructure { + inner_ty.with(field_binding, generated_code) + } else { + generated_code + } + }) + .collect() } } From 952121933e817cd10409a52620dab11c013b3e84 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Wed, 18 May 2022 10:53:48 -0500 Subject: [PATCH 3/6] move misplaced comment --- compiler/rustc_macros/src/diagnostics/diagnostic.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_macros/src/diagnostics/diagnostic.rs b/compiler/rustc_macros/src/diagnostics/diagnostic.rs index 25cba2df7f1..8ba2fcf6ec2 100644 --- a/compiler/rustc_macros/src/diagnostics/diagnostic.rs +++ b/compiler/rustc_macros/src/diagnostics/diagnostic.rs @@ -110,10 +110,6 @@ impl<'a> SessionDiagnosticDerive<'a> { ) }); - // 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). structure.bind_with(|_| synstructure::BindStyle::Move); // 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 @@ -373,6 +369,10 @@ impl SessionDiagnosticDeriveBuilder { let inner_ty = FieldInnerTy::from_type(&info.ty); + // 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 attrs.is_empty() { let diag = &self.diag; let ident = info.binding.ast().ident.as_ref().unwrap(); From 7e9be9240cf1f3c2fc48fffbb202b249821c61b2 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Wed, 18 May 2022 10:55:35 -0500 Subject: [PATCH 4/6] remove unnecessary generics --- compiler/rustc_macros/src/diagnostics/diagnostic.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_macros/src/diagnostics/diagnostic.rs b/compiler/rustc_macros/src/diagnostics/diagnostic.rs index 8ba2fcf6ec2..c24b45862eb 100644 --- a/compiler/rustc_macros/src/diagnostics/diagnostic.rs +++ b/compiler/rustc_macros/src/diagnostics/diagnostic.rs @@ -360,10 +360,10 @@ impl SessionDiagnosticDeriveBuilder { Ok(tokens.drain(..).collect()) } - fn generate_field_attrs_code<'a>( - &'a mut self, - attrs: &'a [syn::Attribute], - info: FieldInfo<'a>, + fn generate_field_attrs_code( + &mut self, + attrs: &[syn::Attribute], + info: FieldInfo<'_>, ) -> TokenStream { let field_binding = &info.binding.binding; From d839d2ecc274f4633bbb621d169d84347ffc2ba9 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Thu, 19 May 2022 00:47:45 -0500 Subject: [PATCH 5/6] let `generate_field_attrs_code` create `FieldInfo` this simplifies the code inside the `structure.each` closure argument and allows to remove the `vis` field from `FieldInfo`. --- .../src/diagnostics/diagnostic.rs | 56 +++++-------------- .../src/diagnostics/subdiagnostic.rs | 1 - .../rustc_macros/src/diagnostics/utils.rs | 3 +- 3 files changed, 14 insertions(+), 46 deletions(-) diff --git a/compiler/rustc_macros/src/diagnostics/diagnostic.rs b/compiler/rustc_macros/src/diagnostics/diagnostic.rs index c24b45862eb..8c5b64fab5f 100644 --- a/compiler/rustc_macros/src/diagnostics/diagnostic.rs +++ b/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> { @@ -95,20 +95,7 @@ impl<'a> SessionDiagnosticDerive<'a> { false } }) - .each(|field_binding| { - let field = field_binding.ast(); - let field_span = field.span(); - - builder.generate_field_attrs_code( - &field.attrs, - FieldInfo { - vis: &field.vis, - binding: field_binding, - ty: &field.ty, - span: &field_span, - }, - ) - }); + .each(|field_binding| builder.generate_field_attrs_code(field_binding)); structure.bind_with(|_| synstructure::BindStyle::Move); // When a field has attributes like `#[label]` or `#[note]` then it doesn't @@ -119,20 +106,7 @@ impl<'a> SessionDiagnosticDerive<'a> { .filter(|field_binding| { subdiagnostics_or_empty.contains(&field_binding.binding) }) - .each(|field_binding| { - let field = field_binding.ast(); - let field_span = field.span(); - - builder.generate_field_attrs_code( - &field.attrs, - FieldInfo { - vis: &field.vis, - binding: field_binding, - ty: &field.ty, - span: &field_span, - }, - ) - }); + .each(|field_binding| builder.generate_field_attrs_code(field_binding)); let span = ast.span().unwrap(); let (diag, sess) = (&builder.diag, &builder.sess); @@ -360,22 +334,19 @@ impl SessionDiagnosticDeriveBuilder { Ok(tokens.drain(..).collect()) } - fn generate_field_attrs_code( - &mut self, - attrs: &[syn::Attribute], - info: FieldInfo<'_>, - ) -> TokenStream { - 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 inner_ty = FieldInnerTy::from_type(&field.ty); // 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 attrs.is_empty() { + if field.attrs.is_empty() { let diag = &self.diag; - let ident = info.binding.ast().ident.as_ref().unwrap(); + let ident = field.ident.as_ref().unwrap(); quote! { #diag.set_arg( stringify!(#ident), @@ -383,7 +354,7 @@ impl SessionDiagnosticDeriveBuilder { ); } } else { - attrs + field.attrs .iter() .map(move |attr| { let name = attr.path.segments.last().unwrap().ident.to_string(); @@ -401,10 +372,9 @@ impl SessionDiagnosticDeriveBuilder { .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: binding_info, + ty: inner_ty.inner_type().unwrap_or(&field.ty), + span: &field.span(), }, binding, ) diff --git a/compiler/rustc_macros/src/diagnostics/subdiagnostic.rs b/compiler/rustc_macros/src/diagnostics/subdiagnostic.rs index ae5b9dbd9ba..df01419c82a 100644 --- a/compiler/rustc_macros/src/diagnostics/subdiagnostic.rs +++ b/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(), diff --git a/compiler/rustc_macros/src/diagnostics/utils.rs b/compiler/rustc_macros/src/diagnostics/utils.rs index af5a30880e0..636bcf1f7b1 100644 --- a/compiler/rustc_macros/src/diagnostics/utils.rs +++ b/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, From 8227e69018279353bd17ee3ca227e22ce6c03783 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Thu, 19 May 2022 09:02:50 -0500 Subject: [PATCH 6/6] formatting --- compiler/rustc_macros/src/diagnostics/diagnostic.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_macros/src/diagnostics/diagnostic.rs b/compiler/rustc_macros/src/diagnostics/diagnostic.rs index 8c5b64fab5f..d7daee64d79 100644 --- a/compiler/rustc_macros/src/diagnostics/diagnostic.rs +++ b/compiler/rustc_macros/src/diagnostics/diagnostic.rs @@ -354,7 +354,8 @@ impl SessionDiagnosticDeriveBuilder { ); } } else { - field.attrs + field + .attrs .iter() .map(move |attr| { let name = attr.path.segments.last().unwrap().ident.to_string();