From 7aee2b332fb1a2b6c72893f425d650bd8c705bb6 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 14 Nov 2024 13:32:22 +1100 Subject: [PATCH 1/4] Make `configure_annotatable`/`flat_map_annotatable` infallible. They each have a single callsite, and the result is always unwrapped, so the `Option` return type is misleading. Also, the comment at the `configure_annotatable` call site is wrong, talking about a result vector, so this commit also removes that. --- compiler/rustc_builtin_macros/src/cfg_eval.rs | 46 +++++++++---------- 1 file changed, 22 insertions(+), 24 deletions(-) diff --git a/compiler/rustc_builtin_macros/src/cfg_eval.rs b/compiler/rustc_builtin_macros/src/cfg_eval.rs index b686a8cf935ca..2cb970aaa4d1e 100644 --- a/compiler/rustc_builtin_macros/src/cfg_eval.rs +++ b/compiler/rustc_builtin_macros/src/cfg_eval.rs @@ -39,46 +39,44 @@ pub(crate) fn cfg_eval( let features = Some(features); CfgEval(StripUnconfigured { sess, features, config_tokens: true, lint_node_id }) .configure_annotatable(annotatable) - // Since the item itself has already been configured by the `InvocationCollector`, - // we know that fold result vector will contain exactly one element. - .unwrap() } struct CfgEval<'a>(StripUnconfigured<'a>); -fn flat_map_annotatable( - vis: &mut impl MutVisitor, - annotatable: Annotatable, -) -> Option { +fn flat_map_annotatable(vis: &mut impl MutVisitor, annotatable: Annotatable) -> Annotatable { match annotatable { - Annotatable::Item(item) => vis.flat_map_item(item).pop().map(Annotatable::Item), + Annotatable::Item(item) => Annotatable::Item(vis.flat_map_item(item).pop().unwrap()), Annotatable::AssocItem(item, ctxt) => { - Some(Annotatable::AssocItem(vis.flat_map_assoc_item(item, ctxt).pop()?, ctxt)) + Annotatable::AssocItem(vis.flat_map_assoc_item(item, ctxt).pop().unwrap(), ctxt) } Annotatable::ForeignItem(item) => { - vis.flat_map_foreign_item(item).pop().map(Annotatable::ForeignItem) + Annotatable::ForeignItem(vis.flat_map_foreign_item(item).pop().unwrap()) } Annotatable::Stmt(stmt) => { - vis.flat_map_stmt(stmt.into_inner()).pop().map(P).map(Annotatable::Stmt) + Annotatable::Stmt(P(vis.flat_map_stmt(stmt.into_inner()).pop().unwrap())) } Annotatable::Expr(mut expr) => { vis.visit_expr(&mut expr); - Some(Annotatable::Expr(expr)) + Annotatable::Expr(expr) } - Annotatable::Arm(arm) => vis.flat_map_arm(arm).pop().map(Annotatable::Arm), + Annotatable::Arm(arm) => Annotatable::Arm(vis.flat_map_arm(arm).pop().unwrap()), Annotatable::ExprField(field) => { - vis.flat_map_expr_field(field).pop().map(Annotatable::ExprField) + Annotatable::ExprField(vis.flat_map_expr_field(field).pop().unwrap()) + } + Annotatable::PatField(fp) => { + Annotatable::PatField(vis.flat_map_pat_field(fp).pop().unwrap()) } - Annotatable::PatField(fp) => vis.flat_map_pat_field(fp).pop().map(Annotatable::PatField), Annotatable::GenericParam(param) => { - vis.flat_map_generic_param(param).pop().map(Annotatable::GenericParam) + Annotatable::GenericParam(vis.flat_map_generic_param(param).pop().unwrap()) + } + Annotatable::Param(param) => Annotatable::Param(vis.flat_map_param(param).pop().unwrap()), + Annotatable::FieldDef(sf) => { + Annotatable::FieldDef(vis.flat_map_field_def(sf).pop().unwrap()) } - Annotatable::Param(param) => vis.flat_map_param(param).pop().map(Annotatable::Param), - Annotatable::FieldDef(sf) => vis.flat_map_field_def(sf).pop().map(Annotatable::FieldDef), - Annotatable::Variant(v) => vis.flat_map_variant(v).pop().map(Annotatable::Variant), + Annotatable::Variant(v) => Annotatable::Variant(vis.flat_map_variant(v).pop().unwrap()), Annotatable::Crate(mut krate) => { vis.visit_crate(&mut krate); - Some(Annotatable::Crate(krate)) + Annotatable::Crate(krate) } } } @@ -123,11 +121,11 @@ impl CfgEval<'_> { self.0.configure(node) } - fn configure_annotatable(&mut self, mut annotatable: Annotatable) -> Option { + fn configure_annotatable(mut self, mut annotatable: Annotatable) -> Annotatable { // Tokenizing and re-parsing the `Annotatable` can have a significant // performance impact, so try to avoid it if possible if !has_cfg_or_cfg_attr(&annotatable) { - return Some(annotatable); + return annotatable; } // The majority of parsed attribute targets will never need to have early cfg-expansion @@ -197,13 +195,13 @@ impl CfgEval<'_> { Ok(a) => annotatable = a, Err(err) => { err.emit(); - return Some(annotatable); + return annotatable; } } // Now that we have our re-parsed `AttrTokenStream`, recursively configuring // our attribute target will correctly configure the tokens as well. - flat_map_annotatable(self, annotatable) + flat_map_annotatable(&mut self, annotatable) } } From b2b643c9d9d2a9807b5efe96e5902f99937c6c1e Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 14 Nov 2024 13:58:27 +1100 Subject: [PATCH 2/4] Inline and remove `flat_map_annotatable`. Important: we know from the `parse_annotatable_with` call above the call site that only some of the `Annotatable` variants are possible. The remaining cases can be replaced with `unreachable!`. --- compiler/rustc_builtin_macros/src/cfg_eval.rs | 56 ++++++------------- 1 file changed, 17 insertions(+), 39 deletions(-) diff --git a/compiler/rustc_builtin_macros/src/cfg_eval.rs b/compiler/rustc_builtin_macros/src/cfg_eval.rs index 2cb970aaa4d1e..f0f534fe2e719 100644 --- a/compiler/rustc_builtin_macros/src/cfg_eval.rs +++ b/compiler/rustc_builtin_macros/src/cfg_eval.rs @@ -43,44 +43,6 @@ pub(crate) fn cfg_eval( struct CfgEval<'a>(StripUnconfigured<'a>); -fn flat_map_annotatable(vis: &mut impl MutVisitor, annotatable: Annotatable) -> Annotatable { - match annotatable { - Annotatable::Item(item) => Annotatable::Item(vis.flat_map_item(item).pop().unwrap()), - Annotatable::AssocItem(item, ctxt) => { - Annotatable::AssocItem(vis.flat_map_assoc_item(item, ctxt).pop().unwrap(), ctxt) - } - Annotatable::ForeignItem(item) => { - Annotatable::ForeignItem(vis.flat_map_foreign_item(item).pop().unwrap()) - } - Annotatable::Stmt(stmt) => { - Annotatable::Stmt(P(vis.flat_map_stmt(stmt.into_inner()).pop().unwrap())) - } - Annotatable::Expr(mut expr) => { - vis.visit_expr(&mut expr); - Annotatable::Expr(expr) - } - Annotatable::Arm(arm) => Annotatable::Arm(vis.flat_map_arm(arm).pop().unwrap()), - Annotatable::ExprField(field) => { - Annotatable::ExprField(vis.flat_map_expr_field(field).pop().unwrap()) - } - Annotatable::PatField(fp) => { - Annotatable::PatField(vis.flat_map_pat_field(fp).pop().unwrap()) - } - Annotatable::GenericParam(param) => { - Annotatable::GenericParam(vis.flat_map_generic_param(param).pop().unwrap()) - } - Annotatable::Param(param) => Annotatable::Param(vis.flat_map_param(param).pop().unwrap()), - Annotatable::FieldDef(sf) => { - Annotatable::FieldDef(vis.flat_map_field_def(sf).pop().unwrap()) - } - Annotatable::Variant(v) => Annotatable::Variant(vis.flat_map_variant(v).pop().unwrap()), - Annotatable::Crate(mut krate) => { - vis.visit_crate(&mut krate); - Annotatable::Crate(krate) - } - } -} - fn has_cfg_or_cfg_attr(annotatable: &Annotatable) -> bool { struct CfgFinder; @@ -201,7 +163,23 @@ impl CfgEval<'_> { // Now that we have our re-parsed `AttrTokenStream`, recursively configuring // our attribute target will correctly configure the tokens as well. - flat_map_annotatable(&mut self, annotatable) + match annotatable { + Annotatable::Item(item) => Annotatable::Item(self.flat_map_item(item).pop().unwrap()), + Annotatable::AssocItem(item, ctxt) => { + Annotatable::AssocItem(self.flat_map_assoc_item(item, ctxt).pop().unwrap(), ctxt) + } + Annotatable::ForeignItem(item) => { + Annotatable::ForeignItem(self.flat_map_foreign_item(item).pop().unwrap()) + } + Annotatable::Stmt(stmt) => { + Annotatable::Stmt(P(self.flat_map_stmt(stmt.into_inner()).pop().unwrap())) + } + Annotatable::Expr(mut expr) => { + self.visit_expr(&mut expr); + Annotatable::Expr(expr) + } + _ => unreachable!(), + } } } From d55a5e5ddad362408fe93d17c8e137cbd35dbf19 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 14 Nov 2024 14:18:56 +1100 Subject: [PATCH 3/4] Merge matches in `configure_annotatable`. There are two matches: one in a closure, and one vanilla one. They can be combined and simplified by putting them in a `try` block. --- compiler/rustc_builtin_macros/src/cfg_eval.rs | 102 ++++++++---------- 1 file changed, 45 insertions(+), 57 deletions(-) diff --git a/compiler/rustc_builtin_macros/src/cfg_eval.rs b/compiler/rustc_builtin_macros/src/cfg_eval.rs index f0f534fe2e719..b8a477b2b3459 100644 --- a/compiler/rustc_builtin_macros/src/cfg_eval.rs +++ b/compiler/rustc_builtin_macros/src/cfg_eval.rs @@ -83,7 +83,7 @@ impl CfgEval<'_> { self.0.configure(node) } - fn configure_annotatable(mut self, mut annotatable: Annotatable) -> Annotatable { + fn configure_annotatable(mut self, annotatable: Annotatable) -> Annotatable { // Tokenizing and re-parsing the `Annotatable` can have a significant // performance impact, so try to avoid it if possible if !has_cfg_or_cfg_attr(&annotatable) { @@ -100,39 +100,6 @@ impl CfgEval<'_> { // the location of `#[cfg]` and `#[cfg_attr]` in the token stream. The tokenization // process is lossless, so this process is invisible to proc-macros. - let parse_annotatable_with: for<'a> fn(&mut Parser<'a>) -> PResult<'a, _> = - match annotatable { - Annotatable::Item(_) => { - |parser| Ok(Annotatable::Item(parser.parse_item(ForceCollect::Yes)?.unwrap())) - } - Annotatable::AssocItem(_, AssocCtxt::Trait) => |parser| { - Ok(Annotatable::AssocItem( - parser.parse_trait_item(ForceCollect::Yes)?.unwrap().unwrap(), - AssocCtxt::Trait, - )) - }, - Annotatable::AssocItem(_, AssocCtxt::Impl) => |parser| { - Ok(Annotatable::AssocItem( - parser.parse_impl_item(ForceCollect::Yes)?.unwrap().unwrap(), - AssocCtxt::Impl, - )) - }, - Annotatable::ForeignItem(_) => |parser| { - Ok(Annotatable::ForeignItem( - parser.parse_foreign_item(ForceCollect::Yes)?.unwrap().unwrap(), - )) - }, - Annotatable::Stmt(_) => |parser| { - Ok(Annotatable::Stmt(P(parser - .parse_stmt_without_recovery(false, ForceCollect::Yes)? - .unwrap()))) - }, - Annotatable::Expr(_) => { - |parser| Ok(Annotatable::Expr(parser.parse_expr_force_collect()?)) - } - _ => unreachable!(), - }; - // 'Flatten' all nonterminals (i.e. `TokenKind::Interpolated`) // to `None`-delimited groups containing the corresponding tokens. This // is normally delayed until the proc-macro server actually needs to @@ -151,34 +118,55 @@ impl CfgEval<'_> { // Re-parse the tokens, setting the `capture_cfg` flag to save extra information // to the captured `AttrTokenStream` (specifically, we capture // `AttrTokenTree::AttrsTarget` for all occurrences of `#[cfg]` and `#[cfg_attr]`) + // + // After that we have our re-parsed `AttrTokenStream`, recursively configuring + // our attribute target will correctly configure the tokens as well. let mut parser = Parser::new(&self.0.sess.psess, orig_tokens, None); parser.capture_cfg = true; - match parse_annotatable_with(&mut parser) { - Ok(a) => annotatable = a, - Err(err) => { - err.emit(); - return annotatable; + let res: PResult<'_, Annotatable> = try { + match annotatable { + Annotatable::Item(_) => { + let item = parser.parse_item(ForceCollect::Yes)?.unwrap(); + Annotatable::Item(self.flat_map_item(item).pop().unwrap()) + } + Annotatable::AssocItem(_, AssocCtxt::Trait) => { + let item = parser.parse_trait_item(ForceCollect::Yes)?.unwrap().unwrap(); + Annotatable::AssocItem( + self.flat_map_assoc_item(item, AssocCtxt::Trait).pop().unwrap(), + AssocCtxt::Trait, + ) + } + Annotatable::AssocItem(_, AssocCtxt::Impl) => { + let item = parser.parse_impl_item(ForceCollect::Yes)?.unwrap().unwrap(); + Annotatable::AssocItem( + self.flat_map_assoc_item(item, AssocCtxt::Impl).pop().unwrap(), + AssocCtxt::Impl, + ) + } + Annotatable::ForeignItem(_) => { + let item = parser.parse_foreign_item(ForceCollect::Yes)?.unwrap().unwrap(); + Annotatable::ForeignItem(self.flat_map_foreign_item(item).pop().unwrap()) + } + Annotatable::Stmt(_) => { + let stmt = + parser.parse_stmt_without_recovery(false, ForceCollect::Yes)?.unwrap(); + Annotatable::Stmt(P(self.flat_map_stmt(stmt).pop().unwrap())) + } + Annotatable::Expr(_) => { + let mut expr = parser.parse_expr_force_collect()?; + self.visit_expr(&mut expr); + Annotatable::Expr(expr) + } + _ => unreachable!(), } - } + }; - // Now that we have our re-parsed `AttrTokenStream`, recursively configuring - // our attribute target will correctly configure the tokens as well. - match annotatable { - Annotatable::Item(item) => Annotatable::Item(self.flat_map_item(item).pop().unwrap()), - Annotatable::AssocItem(item, ctxt) => { - Annotatable::AssocItem(self.flat_map_assoc_item(item, ctxt).pop().unwrap(), ctxt) - } - Annotatable::ForeignItem(item) => { - Annotatable::ForeignItem(self.flat_map_foreign_item(item).pop().unwrap()) - } - Annotatable::Stmt(stmt) => { - Annotatable::Stmt(P(self.flat_map_stmt(stmt.into_inner()).pop().unwrap())) - } - Annotatable::Expr(mut expr) => { - self.visit_expr(&mut expr); - Annotatable::Expr(expr) + match res { + Ok(ann) => ann, + Err(err) => { + err.emit(); + annotatable } - _ => unreachable!(), } } } From 7a96ed3dd0feb006dab9e4960204086dcac3dfb7 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 14 Nov 2024 14:57:52 +1100 Subject: [PATCH 4/4] Remove unreachable code in `has_cfg_or_cfg_attr`. --- compiler/rustc_builtin_macros/src/cfg_eval.rs | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/compiler/rustc_builtin_macros/src/cfg_eval.rs b/compiler/rustc_builtin_macros/src/cfg_eval.rs index b8a477b2b3459..f419c1e776b81 100644 --- a/compiler/rustc_builtin_macros/src/cfg_eval.rs +++ b/compiler/rustc_builtin_macros/src/cfg_eval.rs @@ -66,14 +66,7 @@ fn has_cfg_or_cfg_attr(annotatable: &Annotatable) -> bool { Annotatable::ForeignItem(item) => CfgFinder.visit_foreign_item(item), Annotatable::Stmt(stmt) => CfgFinder.visit_stmt(stmt), Annotatable::Expr(expr) => CfgFinder.visit_expr(expr), - Annotatable::Arm(arm) => CfgFinder.visit_arm(arm), - Annotatable::ExprField(field) => CfgFinder.visit_expr_field(field), - Annotatable::PatField(field) => CfgFinder.visit_pat_field(field), - Annotatable::GenericParam(param) => CfgFinder.visit_generic_param(param), - Annotatable::Param(param) => CfgFinder.visit_param(param), - Annotatable::FieldDef(field) => CfgFinder.visit_field_def(field), - Annotatable::Variant(variant) => CfgFinder.visit_variant(variant), - Annotatable::Crate(krate) => CfgFinder.visit_crate(krate), + _ => unreachable!(), }; res.is_break() }