From ec10833609aa63327437aabfaedfbe8a0edcc4d9 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 1 Apr 2025 14:49:58 +1100 Subject: [PATCH] Address review comments. --- .../rustc_ast_passes/src/ast_validation.rs | 16 ++-- .../src/proc_macro_harness.rs | 7 +- compiler/rustc_expand/src/expand.rs | 4 +- compiler/rustc_parse/src/parser/item.rs | 33 ++++--- .../rustc_resolve/src/build_reduced_graph.rs | 85 ++++++++++--------- compiler/rustc_resolve/src/def_collector.rs | 5 +- compiler/rustc_resolve/src/late.rs | 5 +- .../rustc_resolve/src/late/diagnostics.rs | 10 +-- .../clippy_lints/src/empty_line_after.rs | 15 ++-- .../clippy/clippy_utils/src/ast_utils/mod.rs | 14 ++- src/tools/rustfmt/src/visitor.rs | 2 +- tests/ui/custom_test_frameworks/full.rs | 7 +- 12 files changed, 104 insertions(+), 99 deletions(-) diff --git a/compiler/rustc_ast_passes/src/ast_validation.rs b/compiler/rustc_ast_passes/src/ast_validation.rs index 86661f3f359..839d5d3bb95 100644 --- a/compiler/rustc_ast_passes/src/ast_validation.rs +++ b/compiler/rustc_ast_passes/src/ast_validation.rs @@ -817,10 +817,10 @@ impl<'a> Visitor<'a> for AstValidator<'a> { self.has_proc_macro_decls = true; } - if attr::contains_name(&item.attrs, sym::no_mangle) { - if let Some(ident) = item.kind.ident() { - self.check_nomangle_item_asciionly(ident, item.span); - } + if let Some(ident) = item.kind.ident() + && attr::contains_name(&item.attrs, sym::no_mangle) + { + self.check_nomangle_item_asciionly(ident, item.span); } match &item.kind { @@ -1412,10 +1412,10 @@ impl<'a> Visitor<'a> for AstValidator<'a> { } fn visit_assoc_item(&mut self, item: &'a AssocItem, ctxt: AssocCtxt) { - if attr::contains_name(&item.attrs, sym::no_mangle) { - if let Some(ident) = item.kind.ident() { - self.check_nomangle_item_asciionly(ident, item.span); - } + if let Some(ident) = item.kind.ident() + && attr::contains_name(&item.attrs, sym::no_mangle) + { + self.check_nomangle_item_asciionly(ident, item.span); } if ctxt == AssocCtxt::Trait || self.outer_trait_or_trait_impl.is_none() { diff --git a/compiler/rustc_builtin_macros/src/proc_macro_harness.rs b/compiler/rustc_builtin_macros/src/proc_macro_harness.rs index 7c25f26895c..8862965c053 100644 --- a/compiler/rustc_builtin_macros/src/proc_macro_harness.rs +++ b/compiler/rustc_builtin_macros/src/proc_macro_harness.rs @@ -213,10 +213,8 @@ impl<'a> Visitor<'a> for CollectProcMacros<'a> { return; }; - // First up, make sure we're checking a bare function. If we're not then - // we're just not interested in this item. - // - // If we find one, try to locate a `#[proc_macro_derive]` attribute on it. + // Make sure we're checking a bare function. If we're not then we're + // just not interested any further in this item. let fn_ident = if let ast::ItemKind::Fn(fn_) = &item.kind { fn_.ident } else { @@ -243,6 +241,7 @@ impl<'a> Visitor<'a> for CollectProcMacros<'a> { return; } + // Try to locate a `#[proc_macro_derive]` attribute. if attr.has_name(sym::proc_macro_derive) { self.collect_custom_derive(item, fn_ident, attr); } else if attr.has_name(sym::proc_macro_attribute) { diff --git a/compiler/rustc_expand/src/expand.rs b/compiler/rustc_expand/src/expand.rs index d1dd454fa73..bca846d2ec4 100644 --- a/compiler/rustc_expand/src/expand.rs +++ b/compiler/rustc_expand/src/expand.rs @@ -1223,9 +1223,7 @@ impl InvocationCollectorNode for P { // Work around borrow checker not seeing through `P`'s deref. let (span, mut attrs) = (node.span, mem::take(&mut node.attrs)); - let ItemKind::Mod(_, ident, mod_kind) = &mut node.kind else { unreachable!() }; - let ident = *ident; - + let ItemKind::Mod(_, ident, ref mut mod_kind) = node.kind else { unreachable!() }; let ecx = &mut collector.cx; let (file_path, dir_path, dir_ownership) = match mod_kind { ModKind::Loaded(_, inline, _, _) => { diff --git a/compiler/rustc_parse/src/parser/item.rs b/compiler/rustc_parse/src/parser/item.rs index ed652285096..e93fb2473fb 100644 --- a/compiler/rustc_parse/src/parser/item.rs +++ b/compiler/rustc_parse/src/parser/item.rs @@ -243,8 +243,7 @@ impl<'a> Parser<'a> { // STATIC ITEM self.bump(); // `static` let mutability = self.parse_mutability(); - let item = self.parse_static_item(safety, mutability)?; - ItemKind::Static(Box::new(item)) + self.parse_static_item(safety, mutability)? } else if let Const::Yes(const_span) = self.parse_constness(Case::Sensitive) { // CONST ITEM if self.token.is_keyword(kw::Impl) { @@ -681,7 +680,7 @@ impl<'a> Parser<'a> { } None => (None, ty_first), // impl Type }; - let item_kind = ItemKind::Impl(Box::new(Impl { + Ok(ItemKind::Impl(Box::new(Impl { safety, polarity, defaultness, @@ -690,9 +689,7 @@ impl<'a> Parser<'a> { of_trait, self_ty, items: impl_items, - })); - - Ok(item_kind) + }))) } fn parse_item_delegation(&mut self) -> PResult<'a, ItemKind> { @@ -1222,13 +1219,12 @@ impl<'a> Parser<'a> { safety = Safety::Unsafe(self.token.span); let _ = self.eat_keyword(exp!(Unsafe)); } - let module = ast::ForeignMod { + Ok(ItemKind::ForeignMod(ast::ForeignMod { extern_span, safety, abi, items: self.parse_item_list(attrs, |p| p.parse_foreign_item(ForceCollect::No))?, - }; - Ok(ItemKind::ForeignMod(module)) + })) } /// Parses a foreign item (one in an `extern { ... }` block). @@ -1370,7 +1366,8 @@ impl<'a> Parser<'a> { Ok(item_kind) } - /// Parse a static item with the prefix `"static" "mut"?` already parsed and stored in `mutability`. + /// Parse a static item with the prefix `"static" "mut"?` already parsed and stored in + /// `mutability`. /// /// ```ebnf /// Static = "static" "mut"? $ident ":" $ty (= $expr)? ";" ; @@ -1379,7 +1376,7 @@ impl<'a> Parser<'a> { &mut self, safety: Safety, mutability: Mutability, - ) -> PResult<'a, StaticItem> { + ) -> PResult<'a, ItemKind> { let ident = self.parse_ident()?; if self.token == TokenKind::Lt && self.may_recover() { @@ -1391,7 +1388,8 @@ impl<'a> Parser<'a> { // FIXME: This could maybe benefit from `.may_recover()`? let ty = match (self.eat(exp!(Colon)), self.check(exp!(Eq)) | self.check(exp!(Semi))) { (true, false) => self.parse_ty()?, - // If there wasn't a `:` or the colon was followed by a `=` or `;`, recover a missing type. + // If there wasn't a `:` or the colon was followed by a `=` or `;`, recover a missing + // type. (colon, _) => self.recover_missing_global_item_type(colon, Some(mutability)), }; @@ -1399,7 +1397,8 @@ impl<'a> Parser<'a> { self.expect_semi()?; - Ok(StaticItem { ident, ty, safety, mutability, expr, define_opaque: None }) + let item = StaticItem { ident, ty, safety, mutability, expr, define_opaque: None }; + Ok(ItemKind::Static(Box::new(item))) } /// Parse a constant item with the prefix `"const"` already parsed. @@ -1537,7 +1536,7 @@ impl<'a> Parser<'a> { } let prev_span = self.prev_token.span; - let id = self.parse_ident()?; + let ident = self.parse_ident()?; let mut generics = self.parse_generics()?; generics.where_clause = self.parse_where_clause()?; @@ -1548,10 +1547,10 @@ impl<'a> Parser<'a> { (thin_vec![], Trailing::No) } else { self.parse_delim_comma_seq(exp!(OpenBrace), exp!(CloseBrace), |p| { - p.parse_enum_variant(id.span) + p.parse_enum_variant(ident.span) }) .map_err(|mut err| { - err.span_label(id.span, "while parsing this enum"); + err.span_label(ident.span, "while parsing this enum"); if self.token == token::Colon { let snapshot = self.create_snapshot_for_diagnostic(); self.bump(); @@ -1577,7 +1576,7 @@ impl<'a> Parser<'a> { }; let enum_definition = EnumDef { variants: variants.into_iter().flatten().collect() }; - Ok(ItemKind::Enum(id, enum_definition, generics)) + Ok(ItemKind::Enum(ident, enum_definition, generics)) } fn parse_enum_variant(&mut self, span: Span) -> PResult<'a, Option> { diff --git a/compiler/rustc_resolve/src/build_reduced_graph.rs b/compiler/rustc_resolve/src/build_reduced_graph.rs index 7f29f1a084e..4368f7882ff 100644 --- a/compiler/rustc_resolve/src/build_reduced_graph.rs +++ b/compiler/rustc_resolve/src/build_reduced_graph.rs @@ -10,8 +10,8 @@ use std::sync::Arc; use rustc_ast::visit::{self, AssocCtxt, Visitor, WalkItemKind}; use rustc_ast::{ - self as ast, AssocItem, AssocItemKind, Block, ConstItem, Delegation, ForeignItem, - ForeignItemKind, Impl, Item, ItemKind, MetaItemKind, NodeId, StaticItem, StmtKind, + self as ast, AssocItem, AssocItemKind, Block, ConstItem, Delegation, Fn, ForeignItem, + ForeignItemKind, Impl, Item, ItemKind, MetaItemKind, NodeId, StaticItem, StmtKind, TyAlias, }; use rustc_attr_parsing as attr; use rustc_expand::base::ResolverExpand; @@ -764,8 +764,8 @@ impl<'a, 'ra, 'tcx> BuildReducedGraphVisitor<'a, 'ra, 'tcx> { ItemKind::ExternCrate(orig_name, ident) => { self.build_reduced_graph_for_extern_crate( orig_name, - ident, item, + ident, local_def_id, vis, parent, @@ -797,7 +797,7 @@ impl<'a, 'ra, 'tcx> BuildReducedGraphVisitor<'a, 'ra, 'tcx> { | ItemKind::Static(box StaticItem { ident, .. }) => { self.r.define(parent, ident, ValueNS, (res, vis, sp, expansion)); } - ItemKind::Fn(box ast::Fn { ident, .. }) => { + ItemKind::Fn(box Fn { ident, .. }) => { self.r.define(parent, ident, ValueNS, (res, vis, sp, expansion)); // Functions introducing procedural macros reserve a slot @@ -806,7 +806,7 @@ impl<'a, 'ra, 'tcx> BuildReducedGraphVisitor<'a, 'ra, 'tcx> { } // These items live in the type namespace. - ItemKind::TyAlias(box ast::TyAlias { ident, .. }) | ItemKind::TraitAlias(ident, ..) => { + ItemKind::TyAlias(box TyAlias { ident, .. }) | ItemKind::TraitAlias(ident, ..) => { self.r.define(parent, ident, TypeNS, (res, vis, sp, expansion)); } @@ -900,8 +900,8 @@ impl<'a, 'ra, 'tcx> BuildReducedGraphVisitor<'a, 'ra, 'tcx> { fn build_reduced_graph_for_extern_crate( &mut self, orig_name: Option, - ident: Ident, item: &Item, + ident: Ident, local_def_id: LocalDefId, vis: ty::Visibility, parent: Module<'ra>, @@ -1362,14 +1362,16 @@ impl<'a, 'ra, 'tcx> Visitor<'a> for BuildReducedGraphVisitor<'a, 'ra, 'tcx> { } fn visit_foreign_item(&mut self, foreign_item: &'a ForeignItem) { - if let ForeignItemKind::MacCall(_) = foreign_item.kind { - self.visit_invoc_in_module(foreign_item.id); - return; - } + let ident = match foreign_item.kind { + ForeignItemKind::Static(box StaticItem { ident, .. }) + | ForeignItemKind::Fn(box Fn { ident, .. }) + | ForeignItemKind::TyAlias(box TyAlias { ident, .. }) => ident, + ForeignItemKind::MacCall(_) => { + self.visit_invoc_in_module(foreign_item.id); + return; + } + }; - // `unwrap` is safe because `MacCall` has been excluded, and other foreign item kinds have - // an ident. - let ident = foreign_item.kind.ident().unwrap(); self.build_reduced_graph_for_foreign_item(foreign_item, ident); visit::walk_item(self, foreign_item); } @@ -1384,26 +1386,35 @@ impl<'a, 'ra, 'tcx> Visitor<'a> for BuildReducedGraphVisitor<'a, 'ra, 'tcx> { } fn visit_assoc_item(&mut self, item: &'a AssocItem, ctxt: AssocCtxt) { - if let AssocItemKind::MacCall(_) = item.kind { - match ctxt { - AssocCtxt::Trait => { - self.visit_invoc_in_module(item.id); - } - AssocCtxt::Impl { .. } => { - let invoc_id = item.id.placeholder_to_expn_id(); - if !self.r.glob_delegation_invoc_ids.contains(&invoc_id) { - self.r - .impl_unexpanded_invocations - .entry(self.r.invocation_parent(invoc_id)) - .or_default() - .insert(invoc_id); - } - self.visit_invoc(item.id); - } - } - return; - } + let (ident, ns) = match item.kind { + AssocItemKind::Const(box ConstItem { ident, .. }) + | AssocItemKind::Fn(box Fn { ident, .. }) + | AssocItemKind::Delegation(box Delegation { ident, .. }) => (ident, ValueNS), + AssocItemKind::Type(box TyAlias { ident, .. }) => (ident, TypeNS), + + AssocItemKind::MacCall(_) => { + match ctxt { + AssocCtxt::Trait => { + self.visit_invoc_in_module(item.id); + } + AssocCtxt::Impl { .. } => { + let invoc_id = item.id.placeholder_to_expn_id(); + if !self.r.glob_delegation_invoc_ids.contains(&invoc_id) { + self.r + .impl_unexpanded_invocations + .entry(self.r.invocation_parent(invoc_id)) + .or_default() + .insert(invoc_id); + } + self.visit_invoc(item.id); + } + } + return; + } + + AssocItemKind::DelegationMac(..) => bug!(), + }; let vis = self.resolve_visibility(&item.vis); let feed = self.r.feed(item.id); let local_def_id = feed.key(); @@ -1418,16 +1429,6 @@ impl<'a, 'ra, 'tcx> Visitor<'a> for BuildReducedGraphVisitor<'a, 'ra, 'tcx> { self.r.feed_visibility(feed, vis); } - let ns = match item.kind { - AssocItemKind::Const(..) | AssocItemKind::Delegation(..) | AssocItemKind::Fn(..) => { - ValueNS - } - AssocItemKind::Type(..) => TypeNS, - AssocItemKind::MacCall(_) | AssocItemKind::DelegationMac(..) => bug!(), // handled above - }; - // `unwrap` is safe because `MacCall`/`DelegationMac` have been excluded, and other foreign - // item kinds have an ident. - let ident = item.kind.ident().unwrap(); if ctxt == AssocCtxt::Trait { let parent = self.parent_scope.module; let expansion = self.parent_scope.expansion; diff --git a/compiler/rustc_resolve/src/def_collector.rs b/compiler/rustc_resolve/src/def_collector.rs index 6ad056edbaf..13dfb59f27f 100644 --- a/compiler/rustc_resolve/src/def_collector.rs +++ b/compiler/rustc_resolve/src/def_collector.rs @@ -186,8 +186,11 @@ impl<'a, 'ra, 'tcx> visit::Visitor<'a> for DefCollector<'a, 'ra, 'tcx> { FnKind::Fn( _ctxt, _vis, - Fn { sig: FnSig { header, decl, span: _ }, generics, contract, body, .. }, + Fn { + sig: FnSig { header, decl, span: _ }, ident, generics, contract, body, .. + }, ) if let Some(coroutine_kind) = header.coroutine_kind => { + self.visit_ident(ident); self.visit_fn_header(header); self.visit_generics(generics); if let Some(contract) = contract { diff --git a/compiler/rustc_resolve/src/late.rs b/compiler/rustc_resolve/src/late.rs index d4dd0800452..20e19caf909 100644 --- a/compiler/rustc_resolve/src/late.rs +++ b/compiler/rustc_resolve/src/late.rs @@ -1025,9 +1025,10 @@ impl<'ra: 'ast, 'ast, 'tcx> Visitor<'ast> for LateResolutionVisitor<'_, 'ast, 'r match fn_kind { // Bail if the function is foreign, and thus cannot validly have // a body, or if there's no body for some other reason. - FnKind::Fn(FnCtxt::Foreign, _, Fn { sig, generics, .. }) - | FnKind::Fn(_, _, Fn { sig, generics, body: None, .. }) => { + FnKind::Fn(FnCtxt::Foreign, _, Fn { sig, ident, generics, .. }) + | FnKind::Fn(_, _, Fn { sig, ident, generics, body: None, .. }) => { self.visit_fn_header(&sig.header); + self.visit_ident(ident); self.visit_generics(generics); self.with_lifetime_rib( LifetimeRibKind::AnonymousCreateParameter { diff --git a/compiler/rustc_resolve/src/late/diagnostics.rs b/compiler/rustc_resolve/src/late/diagnostics.rs index 2a2b1ecef16..b62bc6c45e0 100644 --- a/compiler/rustc_resolve/src/late/diagnostics.rs +++ b/compiler/rustc_resolve/src/late/diagnostics.rs @@ -227,14 +227,10 @@ impl<'ast, 'ra: 'ast, 'tcx> LateResolutionVisitor<'_, 'ast, 'ra, 'tcx> { && let FnKind::Fn(_, _, ast::Fn { sig, .. }) = fn_kind && let Some(items) = self.diag_metadata.current_impl_items && let Some(item) = items.iter().find(|i| { - if let Some(ident) = i.kind.ident() - && ident.name == item_str.name - { + i.kind.ident().is_some_and(|ident| { // Don't suggest if the item is in Fn signature arguments (#112590). - !sig.span.contains(item_span) - } else { - false - } + ident.name == item_str.name && !sig.span.contains(item_span) + }) }) { let sp = item_span.shrink_to_lo(); diff --git a/src/tools/clippy/clippy_lints/src/empty_line_after.rs b/src/tools/clippy/clippy_lints/src/empty_line_after.rs index 899b5c59526..c67dcd3c82b 100644 --- a/src/tools/clippy/clippy_lints/src/empty_line_after.rs +++ b/src/tools/clippy/clippy_lints/src/empty_line_after.rs @@ -8,8 +8,7 @@ use rustc_errors::{Applicability, Diag, SuggestionStyle}; use rustc_lexer::TokenKind; use rustc_lint::{EarlyContext, EarlyLintPass, LintContext}; use rustc_session::impl_lint_pass; -use rustc_span::symbol::kw; -use rustc_span::{BytePos, ExpnKind, Ident, InnerSpan, Span, SpanData, Symbol, sym}; +use rustc_span::{BytePos, ExpnKind, Ident, InnerSpan, Span, SpanData, Symbol, kw}; declare_clippy_lint! { /// ### What it does @@ -375,14 +374,16 @@ impl EmptyLineAfter { &mut self, cx: &EarlyContext<'_>, kind: &ItemKind, - ident: &Option, + ident: Option, span: Span, attrs: &[Attribute], id: NodeId, ) { self.items.push(ItemInfo { kind: kind.descr(), - name: if let Some(ident) = ident { ident.name } else { sym::dummy }, + // FIXME: this `sym::empty` can be leaked, see + // https://github.com/rust-lang/rust/pull/138740#discussion_r2021979899 + name: if let Some(ident) = ident { ident.name } else { kw::Empty }, span: if let Some(ident) = ident { span.with_hi(ident.span.hi()) } else { @@ -471,7 +472,7 @@ impl EarlyLintPass for EmptyLineAfter { self.check_item_kind( cx, &item.kind.clone().into(), - &item.kind.ident(), + item.kind.ident(), item.span, &item.attrs, item.id, @@ -482,7 +483,7 @@ impl EarlyLintPass for EmptyLineAfter { self.check_item_kind( cx, &item.kind.clone().into(), - &item.kind.ident(), + item.kind.ident(), item.span, &item.attrs, item.id, @@ -490,6 +491,6 @@ impl EarlyLintPass for EmptyLineAfter { } fn check_item(&mut self, cx: &EarlyContext<'_>, item: &Item) { - self.check_item_kind(cx, &item.kind, &item.kind.ident(), item.span, &item.attrs, item.id); + self.check_item_kind(cx, &item.kind, item.kind.ident(), item.span, &item.attrs, item.id); } } diff --git a/src/tools/clippy/clippy_utils/src/ast_utils/mod.rs b/src/tools/clippy/clippy_utils/src/ast_utils/mod.rs index ff63eb505a5..eba576392eb 100644 --- a/src/tools/clippy/clippy_utils/src/ast_utils/mod.rs +++ b/src/tools/clippy/clippy_utils/src/ast_utils/mod.rs @@ -567,20 +567,23 @@ pub fn eq_foreign_item_kind(l: &ForeignItemKind, r: &ForeignItemKind) -> bool { ( TyAlias(box ast::TyAlias { defaultness: ld, + ident: li, generics: lg, + where_clauses: _, bounds: lb, ty: lt, - .. }), TyAlias(box ast::TyAlias { defaultness: rd, + ident: ri, generics: rg, + where_clauses: _, bounds: rb, ty: rt, - .. }), ) => { eq_defaultness(*ld, *rd) + && eq_id(*li, *ri) && eq_generics(lg, rg) && over(lb, rb, eq_generic_bound) && both(lt.as_ref(), rt.as_ref(), |l, r| eq_ty(l, r)) @@ -647,20 +650,23 @@ pub fn eq_assoc_item_kind(l: &AssocItemKind, r: &AssocItemKind) -> bool { ( Type(box TyAlias { defaultness: ld, + ident: li, generics: lg, + where_clauses: _, bounds: lb, ty: lt, - .. }), Type(box TyAlias { defaultness: rd, + ident: ri, generics: rg, + where_clauses: _, bounds: rb, ty: rt, - .. }), ) => { eq_defaultness(*ld, *rd) + && eq_id(*li, *ri) && eq_generics(lg, rg) && over(lb, rb, eq_generic_bound) && both(lt.as_ref(), rt.as_ref(), |l, r| eq_ty(l, r)) diff --git a/src/tools/rustfmt/src/visitor.rs b/src/tools/rustfmt/src/visitor.rs index 1dc0a906923..16d1f5105d5 100644 --- a/src/tools/rustfmt/src/visitor.rs +++ b/src/tools/rustfmt/src/visitor.rs @@ -623,13 +623,13 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { fn visit_assoc_item(&mut self, ai: &ast::AssocItem, visitor_kind: ItemVisitorKind) { use ItemVisitorKind::*; - // TODO(calebcartwright): Not sure the skip spans are correct let assoc_ctxt = match visitor_kind { AssocTraitItem => visit::AssocCtxt::Trait, // There is no difference between trait and inherent assoc item formatting AssocImplItem => visit::AssocCtxt::Impl { of_trait: false }, _ => unreachable!(), }; + // TODO(calebcartwright): Not sure the skip spans are correct let skip_span = ai.span; skip_out_of_file_lines_range_visitor!(self, ai.span); diff --git a/tests/ui/custom_test_frameworks/full.rs b/tests/ui/custom_test_frameworks/full.rs index 6be29f0418d..57b55e9437b 100644 --- a/tests/ui/custom_test_frameworks/full.rs +++ b/tests/ui/custom_test_frameworks/full.rs @@ -28,12 +28,13 @@ const TEST_1: IsFoo = IsFoo("hello"); static TEST_2: IsFoo = IsFoo("foo"); // FIXME: `test_case` is currently ignored on anything other than -// fn/const/static. Should this be a warning/error? +// fn/const/static. This should be an error. Compare this with `#[test]` and +// #[bench] whose expanders emit "error: expected a non-associated function, +// found […]" if applied to invalid items. #[test_case] struct _S; -// FIXME: `test_case` is currently ignored on anything other than -// fn/const/static. Should this be a warning/error? +// FIXME: as above. #[test_case] impl _S { fn _f() {}