From c9ae38c71e2838f1ac282803ddde47f21f4ca76e Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 24 Nov 2022 16:00:57 +1100 Subject: [PATCH] Avoid unnecessary `MetaItem`/`Attribute` conversions. `check_builtin_attribute` calls `parse_meta` to convert an `Attribute` to a `MetaItem`, which it then checks. However, many callers of `check_builtin_attribute` start with a `MetaItem`, and then convert it to an `Attribute` by calling `cx.attribute(meta_item)`. This `MetaItem` to `Attribute` to `MetaItem` conversion is silly. This commit adds a new function `check_builtin_meta_item`, which can be called instead from these call sites. `check_builtin_attribute` also now calls it. The commit also renames `check_meta` as `check_attr` to better match its arguments. --- .../rustc_ast_passes/src/ast_validation.rs | 2 +- .../src/cfg_accessible.rs | 6 +-- compiler/rustc_builtin_macros/src/derive.rs | 9 ++-- compiler/rustc_builtin_macros/src/util.rs | 11 +++-- compiler/rustc_expand/src/expand.rs | 2 +- compiler/rustc_parse/src/validate_attr.rs | 43 +++++++++++-------- 6 files changed, 44 insertions(+), 29 deletions(-) diff --git a/compiler/rustc_ast_passes/src/ast_validation.rs b/compiler/rustc_ast_passes/src/ast_validation.rs index acd7eb69ffc..29bf7948770 100644 --- a/compiler/rustc_ast_passes/src/ast_validation.rs +++ b/compiler/rustc_ast_passes/src/ast_validation.rs @@ -912,7 +912,7 @@ fn validate_generic_param_order( impl<'a> Visitor<'a> for AstValidator<'a> { fn visit_attribute(&mut self, attr: &Attribute) { - validate_attr::check_meta(&self.session.parse_sess, attr); + validate_attr::check_attr(&self.session.parse_sess, attr); } fn visit_expr(&mut self, expr: &'a Expr) { diff --git a/compiler/rustc_builtin_macros/src/cfg_accessible.rs b/compiler/rustc_builtin_macros/src/cfg_accessible.rs index 86df3c44eb3..4e4cafc7182 100644 --- a/compiler/rustc_builtin_macros/src/cfg_accessible.rs +++ b/compiler/rustc_builtin_macros/src/cfg_accessible.rs @@ -37,10 +37,10 @@ impl MultiItemModifier for Expander { _is_derive_const: bool, ) -> ExpandResult, Annotatable> { let template = AttributeTemplate { list: Some("path"), ..Default::default() }; - let attr = &ecx.attribute(meta_item.clone()); - validate_attr::check_builtin_attribute( + validate_attr::check_builtin_meta_item( &ecx.sess.parse_sess, - attr, + &meta_item, + ast::AttrStyle::Outer, sym::cfg_accessible, template, ); diff --git a/compiler/rustc_builtin_macros/src/derive.rs b/compiler/rustc_builtin_macros/src/derive.rs index c8a2fca00e8..0358c0b1b5f 100644 --- a/compiler/rustc_builtin_macros/src/derive.rs +++ b/compiler/rustc_builtin_macros/src/derive.rs @@ -33,14 +33,15 @@ impl MultiItemModifier for Expander { ecx.resolver.resolve_derives(ecx.current_expansion.id, ecx.force_mode, &|| { let template = AttributeTemplate { list: Some("Trait1, Trait2, ..."), ..Default::default() }; - let attr = - attr::mk_attr_outer(&sess.parse_sess.attr_id_generator, meta_item.clone()); - validate_attr::check_builtin_attribute( + validate_attr::check_builtin_meta_item( &sess.parse_sess, - &attr, + &meta_item, + ast::AttrStyle::Outer, sym::derive, template, ); + let attr = + attr::mk_attr_outer(&sess.parse_sess.attr_id_generator, meta_item.clone()); let mut resolutions: Vec<_> = attr .meta_item_list() diff --git a/compiler/rustc_builtin_macros/src/util.rs b/compiler/rustc_builtin_macros/src/util.rs index 527fe50eff0..83812631c2f 100644 --- a/compiler/rustc_builtin_macros/src/util.rs +++ b/compiler/rustc_builtin_macros/src/util.rs @@ -1,4 +1,4 @@ -use rustc_ast::{Attribute, MetaItem}; +use rustc_ast::{AttrStyle, Attribute, MetaItem}; use rustc_expand::base::{Annotatable, ExtCtxt}; use rustc_feature::AttributeTemplate; use rustc_lint_defs::builtin::DUPLICATE_MACRO_ATTRIBUTES; @@ -8,8 +8,13 @@ use rustc_span::Symbol; pub fn check_builtin_macro_attribute(ecx: &ExtCtxt<'_>, meta_item: &MetaItem, name: Symbol) { // All the built-in macro attributes are "words" at the moment. let template = AttributeTemplate { word: true, ..Default::default() }; - let attr = ecx.attribute(meta_item.clone()); - validate_attr::check_builtin_attribute(&ecx.sess.parse_sess, &attr, name, template); + validate_attr::check_builtin_meta_item( + &ecx.sess.parse_sess, + &meta_item, + AttrStyle::Outer, + name, + template, + ); } /// Emit a warning if the item is annotated with the given attribute. This is used to diagnose when diff --git a/compiler/rustc_expand/src/expand.rs b/compiler/rustc_expand/src/expand.rs index 3e98b024c73..e799fa404f6 100644 --- a/compiler/rustc_expand/src/expand.rs +++ b/compiler/rustc_expand/src/expand.rs @@ -1644,7 +1644,7 @@ impl<'a, 'b> InvocationCollector<'a, 'b> { let mut span: Option = None; while let Some(attr) = attrs.next() { rustc_ast_passes::feature_gate::check_attribute(attr, self.cx.sess, features); - validate_attr::check_meta(&self.cx.sess.parse_sess, attr); + validate_attr::check_attr(&self.cx.sess.parse_sess, attr); let current_span = if let Some(sp) = span { sp.to(attr.span) } else { attr.span }; span = Some(current_span); diff --git a/compiler/rustc_parse/src/validate_attr.rs b/compiler/rustc_parse/src/validate_attr.rs index 59e564114e5..72402a20090 100644 --- a/compiler/rustc_parse/src/validate_attr.rs +++ b/compiler/rustc_parse/src/validate_attr.rs @@ -10,9 +10,9 @@ use rustc_errors::{Applicability, FatalError, PResult}; use rustc_feature::{AttributeTemplate, BuiltinAttribute, BUILTIN_ATTRIBUTE_MAP}; use rustc_session::lint::builtin::ILL_FORMED_ATTRIBUTE_INPUT; use rustc_session::parse::ParseSess; -use rustc_span::{sym, Symbol}; +use rustc_span::{sym, Span, Symbol}; -pub fn check_meta(sess: &ParseSess, attr: &Attribute) { +pub fn check_attr(sess: &ParseSess, attr: &Attribute) { if attr.is_doc_comment() { return; } @@ -115,25 +115,34 @@ pub fn check_builtin_attribute( name: Symbol, template: AttributeTemplate, ) { - // Some special attributes like `cfg` must be checked - // before the generic check, so we skip them here. - let should_skip = |name| name == sym::cfg; - match parse_meta(sess, attr) { - Ok(meta) => { - if !should_skip(name) && !is_attr_template_compatible(&template, &meta.kind) { - emit_malformed_attribute(sess, attr, name, template); - } - } + Ok(meta) => check_builtin_meta_item(sess, &meta, attr.style, name, template), Err(mut err) => { err.emit(); } } } +pub fn check_builtin_meta_item( + sess: &ParseSess, + meta: &MetaItem, + style: ast::AttrStyle, + name: Symbol, + template: AttributeTemplate, +) { + // Some special attributes like `cfg` must be checked + // before the generic check, so we skip them here. + let should_skip = |name| name == sym::cfg; + + if !should_skip(name) && !is_attr_template_compatible(&template, &meta.kind) { + emit_malformed_attribute(sess, style, meta.span, name, template); + } +} + fn emit_malformed_attribute( sess: &ParseSess, - attr: &Attribute, + style: ast::AttrStyle, + span: Span, name: Symbol, template: AttributeTemplate, ) { @@ -147,7 +156,7 @@ fn emit_malformed_attribute( let mut msg = "attribute must be of the form ".to_owned(); let mut suggestions = vec![]; let mut first = true; - let inner = if attr.style == ast::AttrStyle::Inner { "!" } else { "" }; + let inner = if style == ast::AttrStyle::Inner { "!" } else { "" }; if template.word { first = false; let code = format!("#{}[{}]", inner, name); @@ -172,12 +181,12 @@ fn emit_malformed_attribute( suggestions.push(code); } if should_warn(name) { - sess.buffer_lint(&ILL_FORMED_ATTRIBUTE_INPUT, attr.span, ast::CRATE_NODE_ID, &msg); + sess.buffer_lint(&ILL_FORMED_ATTRIBUTE_INPUT, span, ast::CRATE_NODE_ID, &msg); } else { sess.span_diagnostic - .struct_span_err(attr.span, &error_msg) + .struct_span_err(span, &error_msg) .span_suggestions( - attr.span, + span, if suggestions.len() == 1 { "must be of the form" } else { @@ -196,7 +205,7 @@ pub fn emit_fatal_malformed_builtin_attribute( name: Symbol, ) -> ! { let template = BUILTIN_ATTRIBUTE_MAP.get(&name).expect("builtin attr defined").template; - emit_malformed_attribute(sess, attr, name, template); + emit_malformed_attribute(sess, attr.style, attr.span, name, template); // This is fatal, otherwise it will likely cause a cascade of other errors // (and an error here is expected to be very rare). FatalError.raise()