From c7df450ab0a719ecf215aa9b0627281b08a3be0e Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Wed, 7 Feb 2024 15:19:13 -0600 Subject: [PATCH 1/9] Refactor ModResolver.directory Make it so the field is only modified through a with_directory function so that it is easier to follow. --- src/modules.rs | 71 +++++++++++++++++++++++++++++++------------------- 1 file changed, 44 insertions(+), 27 deletions(-) diff --git a/src/modules.rs b/src/modules.rs index af9a154a6ae..9639737fa89 100644 --- a/src/modules.rs +++ b/src/modules.rs @@ -225,13 +225,11 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { item: &'c ast::Item, sub_mod: Module<'ast>, ) -> Result<(), ModuleResolutionError> { - let old_directory = self.directory.clone(); let sub_mod_kind = self.peek_sub_mod(item, &sub_mod)?; if let Some(sub_mod_kind) = sub_mod_kind { self.insert_sub_mod(sub_mod_kind.clone())?; self.visit_sub_mod_inner(sub_mod, sub_mod_kind)?; } - self.directory = old_directory; Ok(()) } @@ -288,11 +286,15 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { path: mod_path.parent().unwrap().to_path_buf(), ownership: directory_ownership, }; - self.visit_sub_mod_after_directory_update(sub_mod, Some(directory)) + self.with_directory(directory, |this| { + this.visit_sub_mod_after_directory_update(sub_mod) + })?; } SubModKind::Internal(item) => { - self.push_inline_mod_directory(item.ident, &item.attrs); - self.visit_sub_mod_after_directory_update(sub_mod, None) + let directory = self.inline_mod_directory(item.ident, &item.attrs); + self.with_directory(directory, |this| { + this.visit_sub_mod_after_directory_update(sub_mod) + })?; } SubModKind::MultiExternal(mods) => { for (mod_path, directory_ownership, sub_mod) in mods { @@ -300,21 +302,19 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { path: mod_path.parent().unwrap().to_path_buf(), ownership: directory_ownership, }; - self.visit_sub_mod_after_directory_update(sub_mod, Some(directory))?; + self.with_directory(directory, |this| { + this.visit_sub_mod_after_directory_update(sub_mod) + })?; } - Ok(()) } } + Ok(()) } fn visit_sub_mod_after_directory_update( &mut self, sub_mod: Module<'ast>, - directory: Option, ) -> Result<(), ModuleResolutionError> { - if let Some(directory) = directory { - self.directory = directory; - } match (sub_mod.ast_mod_kind, sub_mod.items) { (Some(Cow::Borrowed(ast::ModKind::Loaded(items, _, _))), _) => { self.visit_mod_from_ast(items) @@ -470,10 +470,12 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { } } - fn push_inline_mod_directory(&mut self, id: symbol::Ident, attrs: &[ast::Attribute]) { + fn inline_mod_directory(&mut self, id: symbol::Ident, attrs: &[ast::Attribute]) -> Directory { if let Some(path) = find_path_value(attrs) { - self.directory.path.push(path.as_str()); - self.directory.ownership = DirectoryOwnership::Owned { relative: None }; + Directory { + path: self.directory.path.join(path.as_str()), + ownership: DirectoryOwnership::Owned { relative: None }, + } } else { let id = id.as_str(); // We have to push on the current module name in the case of relative @@ -482,19 +484,28 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { // // For example, a `mod z { ... }` inside `x/y.rs` should set the current // directory path to `/x/y/z`, not `/x/z` with a relative offset of `y`. - if let DirectoryOwnership::Owned { relative } = &mut self.directory.ownership { - if let Some(ident) = relative.take() { - // remove the relative offset - self.directory.path.push(ident.as_str()); - - // In the case where there is an x.rs and an ./x directory we want - // to prevent adding x twice. For example, ./x/x - if self.directory.path.exists() && !self.directory.path.join(id).exists() { - return; - } + let new_path = if let DirectoryOwnership::Owned { + relative: Some(ident), + } = self.directory.ownership + { + // remove the relative offset + let relative = self.directory.path.join(ident.as_str()); + let nested = relative.join(id); + + // In the case where there is an x.rs and an ./x directory we want + // to prevent adding x twice. For example, ./x/x + if relative.exists() && !nested.exists() { + relative + } else { + nested } + } else { + self.directory.path.join(id) + }; + Directory { + path: new_path, + ownership: self.directory.ownership, } - self.directory.path.push(id); } } @@ -512,8 +523,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { } let mut result = vec![]; for path in path_visitor.paths() { - let mut actual_path = self.directory.path.clone(); - actual_path.push(&path); + let actual_path = self.directory.path.join(path); if !actual_path.exists() { continue; } @@ -546,6 +556,13 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { } result } + + fn with_directory(&mut self, directory: Directory, f: impl FnOnce(&mut Self) -> T) -> T { + let old = std::mem::replace(&mut self.directory, directory); + let out = f(self); + self.directory = old; + out + } } fn path_value(attr: &ast::Attribute) -> Option { From 6c6c3386b412d27b61bee9d90e1fa88208f8ce5a Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Fri, 9 Feb 2024 16:40:36 -0600 Subject: [PATCH 2/9] Remove needless attrs reference It's a little tricky to follow, but if you trace the code paths for inline modules and for external modules, this value is never actually used. This makes two functions more similar so that they can be deduped in a later commit. --- src/modules.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/modules.rs b/src/modules.rs index 9639737fa89..8cb4ff251e6 100644 --- a/src/modules.rs +++ b/src/modules.rs @@ -212,7 +212,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { span, Some(Cow::Borrowed(sub_mod_kind)), Cow::Owned(ThinVec::new()), - Cow::Borrowed(&item.attrs), + Cow::Owned(ast::AttrVec::new()), ), )?; } From 1224f422708c193c0e3af125676a62cd91859e9c Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Mon, 5 Feb 2024 17:01:24 -0600 Subject: [PATCH 3/9] Butcher some wandering Cows --- src/modules.rs | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/modules.rs b/src/modules.rs index 8cb4ff251e6..e6039144a7e 100644 --- a/src/modules.rs +++ b/src/modules.rs @@ -36,7 +36,7 @@ impl<'a> Module<'a> { mod_span: Span, ast_mod_kind: Option>, mod_items: Cow<'a, ThinVec>>, - mod_attrs: Cow<'a, ast::AttrVec>, + mod_attrs: &[ast::Attribute], ) -> Self { let inner_attr = mod_attrs .iter() @@ -141,16 +141,16 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { mk_sp(snippet_provider.start_pos(), snippet_provider.end_pos()), None, Cow::Borrowed(&krate.items), - Cow::Borrowed(&krate.attrs), + &krate.attrs, ), ); Ok(self.file_map) } /// Visit `cfg_if` macro and look for module declarations. - fn visit_cfg_if(&mut self, item: Cow<'ast, ast::Item>) -> Result<(), ModuleResolutionError> { + fn visit_cfg_if(&mut self, item: &ast::Item) -> Result<(), ModuleResolutionError> { let mut visitor = visitor::CfgIfVisitor::new(self.parse_sess); - visitor.visit_item(&item); + visitor.visit_item(item); for module_item in visitor.mods() { if let ast::ItemKind::Mod(_, ref sub_mod_kind) = module_item.item.kind { self.visit_sub_mod( @@ -159,7 +159,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { module_item.item.span, Some(Cow::Owned(sub_mod_kind.clone())), Cow::Owned(ThinVec::new()), - Cow::Owned(ast::AttrVec::new()), + &[], ), )?; } @@ -170,11 +170,11 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { /// Visit modules defined inside macro calls. fn visit_mod_outside_ast( &mut self, - items: ThinVec>, + items: &[rustc_ast::ptr::P], ) -> Result<(), ModuleResolutionError> { for item in items { - if is_cfg_if(&item) { - self.visit_cfg_if(Cow::Owned(item.into_inner()))?; + if is_cfg_if(item) { + self.visit_cfg_if(item)?; continue; } @@ -186,7 +186,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { span, Some(Cow::Owned(sub_mod_kind.clone())), Cow::Owned(ThinVec::new()), - Cow::Owned(ast::AttrVec::new()), + &[], ), )?; } @@ -201,7 +201,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { ) -> Result<(), ModuleResolutionError> { for item in items { if is_cfg_if(item) { - self.visit_cfg_if(Cow::Borrowed(item))?; + self.visit_cfg_if(item)?; } if let ast::ItemKind::Mod(_, ref sub_mod_kind) = item.kind { @@ -212,7 +212,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { span, Some(Cow::Borrowed(sub_mod_kind)), Cow::Owned(ThinVec::new()), - Cow::Owned(ast::AttrVec::new()), + &[], ), )?; } @@ -320,7 +320,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { self.visit_mod_from_ast(items) } (Some(Cow::Owned(ast::ModKind::Loaded(items, _, _))), _) | (_, Cow::Owned(items)) => { - self.visit_mod_outside_ast(items) + self.visit_mod_outside_ast(&items) } (_, _) => Ok(()), } @@ -350,7 +350,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { span, Some(Cow::Owned(ast::ModKind::Unloaded)), Cow::Owned(items), - Cow::Owned(attrs), + &attrs, ), ))), Err(ParserError::ParseError) => Err(ModuleResolutionError { @@ -400,7 +400,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { span, Some(Cow::Owned(ast::ModKind::Unloaded)), Cow::Owned(items), - Cow::Owned(attrs), + &attrs, ), ))) } @@ -412,7 +412,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { span, Some(Cow::Owned(ast::ModKind::Unloaded)), Cow::Owned(items), - Cow::Owned(attrs), + &attrs, ), )); if should_insert { @@ -550,7 +550,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { span, Some(Cow::Owned(ast::ModKind::Unloaded)), Cow::Owned(items), - Cow::Owned(attrs), + &attrs, ), )) } From aca8f9c4dc30f83130b364210a0fc05448ba78c9 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Wed, 7 Feb 2024 16:21:18 -0600 Subject: [PATCH 4/9] Make cfg_if DRYer --- src/modules.rs | 14 +------------- src/modules/visitor.rs | 16 ++++------------ src/parse/macros/cfg_if.rs | 7 ++++--- 3 files changed, 9 insertions(+), 28 deletions(-) diff --git a/src/modules.rs b/src/modules.rs index e6039144a7e..319265bd0b5 100644 --- a/src/modules.rs +++ b/src/modules.rs @@ -151,19 +151,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { fn visit_cfg_if(&mut self, item: &ast::Item) -> Result<(), ModuleResolutionError> { let mut visitor = visitor::CfgIfVisitor::new(self.parse_sess); visitor.visit_item(item); - for module_item in visitor.mods() { - if let ast::ItemKind::Mod(_, ref sub_mod_kind) = module_item.item.kind { - self.visit_sub_mod( - &module_item.item, - Module::new( - module_item.item.span, - Some(Cow::Owned(sub_mod_kind.clone())), - Cow::Owned(ThinVec::new()), - &[], - ), - )?; - } - } + self.visit_mod_outside_ast(&visitor.items)?; Ok(()) } diff --git a/src/modules/visitor.rs b/src/modules/visitor.rs index 48431693332..a98047975a6 100644 --- a/src/modules/visitor.rs +++ b/src/modules/visitor.rs @@ -1,4 +1,5 @@ use rustc_ast::ast; +use rustc_ast::ptr::P; use rustc_ast::visit::Visitor; use rustc_span::Symbol; @@ -6,27 +7,19 @@ use crate::attr::MetaVisitor; use crate::parse::macros::cfg_if::parse_cfg_if; use crate::parse::session::ParseSess; -pub(crate) struct ModItem { - pub(crate) item: ast::Item, -} - /// Traverse `cfg_if!` macro and fetch modules. pub(crate) struct CfgIfVisitor<'a> { parse_sess: &'a ParseSess, - mods: Vec, + pub(crate) items: Vec>, } impl<'a> CfgIfVisitor<'a> { pub(crate) fn new(parse_sess: &'a ParseSess) -> CfgIfVisitor<'a> { CfgIfVisitor { - mods: vec![], parse_sess, + items: Vec::new(), } } - - pub(crate) fn mods(self) -> Vec { - self.mods - } } impl<'a, 'ast: 'a> Visitor<'ast> for CfgIfVisitor<'a> { @@ -63,8 +56,7 @@ impl<'a, 'ast: 'a> CfgIfVisitor<'a> { }; let items = parse_cfg_if(self.parse_sess, mac)?; - self.mods - .append(&mut items.into_iter().map(|item| ModItem { item }).collect()); + self.items.extend(items); Ok(()) } diff --git a/src/parse/macros/cfg_if.rs b/src/parse/macros/cfg_if.rs index bafef7b0f46..e0c18f606c8 100644 --- a/src/parse/macros/cfg_if.rs +++ b/src/parse/macros/cfg_if.rs @@ -1,6 +1,7 @@ use std::panic::{catch_unwind, AssertUnwindSafe}; use rustc_ast::ast; +use rustc_ast::ptr::P; use rustc_ast::token::{Delimiter, TokenKind}; use rustc_parse::parser::ForceCollect; use rustc_span::symbol::kw; @@ -11,7 +12,7 @@ use crate::parse::session::ParseSess; pub(crate) fn parse_cfg_if<'a>( sess: &'a ParseSess, mac: &'a ast::MacCall, -) -> Result, &'static str> { +) -> Result>, &'static str> { match catch_unwind(AssertUnwindSafe(|| parse_cfg_if_inner(sess, mac))) { Ok(Ok(items)) => Ok(items), Ok(err @ Err(_)) => err, @@ -22,7 +23,7 @@ pub(crate) fn parse_cfg_if<'a>( fn parse_cfg_if_inner<'a>( sess: &'a ParseSess, mac: &'a ast::MacCall, -) -> Result, &'static str> { +) -> Result>, &'static str> { let ts = mac.args.tokens.clone(); let mut parser = build_stream_parser(sess.inner(), ts); @@ -63,7 +64,7 @@ fn parse_cfg_if_inner<'a>( && parser.token.kind != TokenKind::Eof { let item = match parser.parse_item(ForceCollect::No) { - Ok(Some(item_ptr)) => item_ptr.into_inner(), + Ok(Some(item_ptr)) => item_ptr, Ok(None) => continue, Err(err) => { err.cancel(); From b4b9b7de6c56c4064022d708e3830266e419851c Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Fri, 23 Feb 2024 21:56:17 -0600 Subject: [PATCH 5/9] Factor out SubModKind::Internal This variant can be factored out fairly easily. It makes things simpler overall and opens the door for later refactors. --- src/modules.rs | 53 +++++++++++++++++--------------------------------- 1 file changed, 18 insertions(+), 35 deletions(-) diff --git a/src/modules.rs b/src/modules.rs index 319265bd0b5..f5c4070e005 100644 --- a/src/modules.rs +++ b/src/modules.rs @@ -90,16 +90,14 @@ pub(crate) enum ModuleResolutionErrorKind { } #[derive(Clone)] -enum SubModKind<'a, 'ast> { +enum SubModKind<'ast> { /// `mod foo;` External(PathBuf, DirectoryOwnership, Module<'ast>), /// `mod foo;` with multiple sources. MultiExternal(Vec<(PathBuf, DirectoryOwnership, Module<'ast>)>), - /// `mod foo {}` - Internal(&'a ast::Item), } -impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { +impl<'ast, 'sess> ModResolver<'ast, 'sess> { /// Creates a new `ModResolver`. pub(crate) fn new( parse_sess: &'sess ParseSess, @@ -210,40 +208,33 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { fn visit_sub_mod( &mut self, - item: &'c ast::Item, + item: &ast::Item, sub_mod: Module<'ast>, ) -> Result<(), ModuleResolutionError> { - let sub_mod_kind = self.peek_sub_mod(item, &sub_mod)?; - if let Some(sub_mod_kind) = sub_mod_kind { - self.insert_sub_mod(sub_mod_kind.clone())?; - self.visit_sub_mod_inner(sub_mod, sub_mod_kind)?; - } - Ok(()) - } - - /// Inspect the given sub-module which we are about to visit and returns its kind. - fn peek_sub_mod( - &self, - item: &'c ast::Item, - sub_mod: &Module<'ast>, - ) -> Result>, ModuleResolutionError> { if contains_skip(&item.attrs) { - return Ok(None); + return Ok(()); } - if is_mod_decl(item) { // mod foo; // Look for an extern file. - self.find_external_module(item.ident, &item.attrs, sub_mod) + let Some(kind) = self.find_external_module(item.ident, &item.attrs, &sub_mod)? else { + return Ok(()); + }; + self.insert_sub_mod(kind.clone())?; + self.visit_sub_mod_inner(kind)?; } else { // An internal module (`mod foo { /* ... */ }`); - Ok(Some(SubModKind::Internal(item))) - } + let directory = self.inline_mod_directory(item.ident, &item.attrs); + self.with_directory(directory, |this| { + this.visit_sub_mod_after_directory_update(sub_mod) + })?; + }; + Ok(()) } fn insert_sub_mod( &mut self, - sub_mod_kind: SubModKind<'c, 'ast>, + sub_mod_kind: SubModKind<'ast>, ) -> Result<(), ModuleResolutionError> { match sub_mod_kind { SubModKind::External(mod_path, _, sub_mod) => { @@ -258,15 +249,13 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { .or_insert(sub_mod); } } - _ => (), } Ok(()) } fn visit_sub_mod_inner( &mut self, - sub_mod: Module<'ast>, - sub_mod_kind: SubModKind<'c, 'ast>, + sub_mod_kind: SubModKind<'ast>, ) -> Result<(), ModuleResolutionError> { match sub_mod_kind { SubModKind::External(mod_path, directory_ownership, sub_mod) => { @@ -278,12 +267,6 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { this.visit_sub_mod_after_directory_update(sub_mod) })?; } - SubModKind::Internal(item) => { - let directory = self.inline_mod_directory(item.ident, &item.attrs); - self.with_directory(directory, |this| { - this.visit_sub_mod_after_directory_update(sub_mod) - })?; - } SubModKind::MultiExternal(mods) => { for (mod_path, directory_ownership, sub_mod) in mods { let directory = Directory { @@ -320,7 +303,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { mod_name: symbol::Ident, attrs: &[ast::Attribute], sub_mod: &Module<'ast>, - ) -> Result>, ModuleResolutionError> { + ) -> Result>, ModuleResolutionError> { let relative = match self.directory.ownership { DirectoryOwnership::Owned { relative } => relative, DirectoryOwnership::UnownedViaBlock => None, From dbd7c1d9cb447078e6065c12885d65c3302394f8 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Fri, 23 Feb 2024 16:30:57 -0600 Subject: [PATCH 6/9] Use arenas for AST This makes some code DRYer since we don't need to use Cows and have different code paths for items with different lifetimes. --- src/formatting.rs | 5 +++ src/lib.rs | 1 + src/modules.rs | 100 +++++++++++++++------------------------------- 3 files changed, 38 insertions(+), 68 deletions(-) diff --git a/src/formatting.rs b/src/formatting.rs index 60361505a3f..1ee5b656c9f 100644 --- a/src/formatting.rs +++ b/src/formatting.rs @@ -4,6 +4,7 @@ use std::collections::HashMap; use std::io::{self, Write}; use std::time::{Duration, Instant}; +use rustc_arena::TypedArena; use rustc_ast::ast; use rustc_span::Span; @@ -131,7 +132,11 @@ fn format_project( }; let mut context = FormatContext::new(&krate, report, parse_session, config, handler); + let item_arena = TypedArena::default(); + let mod_kind_arena = TypedArena::default(); let files = modules::ModResolver::new( + &item_arena, + &mod_kind_arena, &context.parse_session, directory_ownership.unwrap_or(DirectoryOwnership::UnownedViaBlock), !input_is_stdin && !config.skip_children(), diff --git a/src/lib.rs b/src/lib.rs index a67adb1478f..d57527427ce 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -11,6 +11,7 @@ extern crate lazy_static; extern crate tracing; // N.B. these crates are loaded from the sysroot, so they need extern crate. +extern crate rustc_arena; extern crate rustc_ast; extern crate rustc_ast_pretty; extern crate rustc_builtin_macros; diff --git a/src/modules.rs b/src/modules.rs index f5c4070e005..06a4a0b23ee 100644 --- a/src/modules.rs +++ b/src/modules.rs @@ -1,12 +1,12 @@ -use std::borrow::Cow; use std::collections::BTreeMap; use std::path::{Path, PathBuf}; +use rustc_arena::TypedArena; use rustc_ast::ast; +use rustc_ast::ptr::P; use rustc_ast::visit::Visitor; use rustc_span::symbol::{self, sym, Symbol}; use rustc_span::Span; -use thin_vec::ThinVec; use thiserror::Error; use crate::attr::MetaVisitor; @@ -25,8 +25,8 @@ type FileModMap<'ast> = BTreeMap>; /// Represents module with its inner attributes. #[derive(Debug, Clone)] pub(crate) struct Module<'a> { - ast_mod_kind: Option>, - pub(crate) items: Cow<'a, ThinVec>>, + ast_mod_kind: Option<&'a ast::ModKind>, + pub(crate) items: &'a [rustc_ast::ptr::P], inner_attr: ast::AttrVec, pub(crate) span: Span, } @@ -34,8 +34,8 @@ pub(crate) struct Module<'a> { impl<'a> Module<'a> { pub(crate) fn new( mod_span: Span, - ast_mod_kind: Option>, - mod_items: Cow<'a, ThinVec>>, + ast_mod_kind: Option<&'a ast::ModKind>, + mod_items: &'a [rustc_ast::ptr::P], mod_attrs: &[ast::Attribute], ) -> Self { let inner_attr = mod_attrs @@ -58,6 +58,8 @@ impl<'a> Module<'a> { /// Maps each module to the corresponding file. pub(crate) struct ModResolver<'ast, 'sess> { + item_arena: &'ast TypedArena>, + mod_kind_arena: &'ast TypedArena, parse_sess: &'sess ParseSess, directory: Directory, file_map: FileModMap<'ast>, @@ -100,11 +102,15 @@ enum SubModKind<'ast> { impl<'ast, 'sess> ModResolver<'ast, 'sess> { /// Creates a new `ModResolver`. pub(crate) fn new( + item_arena: &'ast TypedArena>, + mod_kind_arena: &'ast TypedArena, parse_sess: &'sess ParseSess, directory_ownership: DirectoryOwnership, recursive: bool, ) -> Self { ModResolver { + item_arena, + mod_kind_arena, directory: Directory { path: PathBuf::new(), ownership: directory_ownership, @@ -128,7 +134,7 @@ impl<'ast, 'sess> ModResolver<'ast, 'sess> { // Skip visiting sub modules when the input is from stdin. if self.recursive { - self.visit_mod_from_ast(&krate.items)?; + self.visit_items(&krate.items)?; } let snippet_provider = self.parse_sess.snippet_provider(krate.spans.inner_span); @@ -138,7 +144,7 @@ impl<'ast, 'sess> ModResolver<'ast, 'sess> { Module::new( mk_sp(snippet_provider.start_pos(), snippet_provider.end_pos()), None, - Cow::Borrowed(&krate.items), + &krate.items, &krate.attrs, ), ); @@ -149,58 +155,20 @@ impl<'ast, 'sess> ModResolver<'ast, 'sess> { fn visit_cfg_if(&mut self, item: &ast::Item) -> Result<(), ModuleResolutionError> { let mut visitor = visitor::CfgIfVisitor::new(self.parse_sess); visitor.visit_item(item); - self.visit_mod_outside_ast(&visitor.items)?; + let items = self.item_arena.alloc_from_iter(visitor.items); + self.visit_items(&*items)?; Ok(()) } - /// Visit modules defined inside macro calls. - fn visit_mod_outside_ast( - &mut self, - items: &[rustc_ast::ptr::P], - ) -> Result<(), ModuleResolutionError> { - for item in items { - if is_cfg_if(item) { - self.visit_cfg_if(item)?; - continue; - } - - if let ast::ItemKind::Mod(_, ref sub_mod_kind) = item.kind { - let span = item.span; - self.visit_sub_mod( - &item, - Module::new( - span, - Some(Cow::Owned(sub_mod_kind.clone())), - Cow::Owned(ThinVec::new()), - &[], - ), - )?; - } - } - Ok(()) - } - - /// Visit modules from AST. - fn visit_mod_from_ast( + fn visit_items( &mut self, items: &'ast [rustc_ast::ptr::P], ) -> Result<(), ModuleResolutionError> { for item in items { if is_cfg_if(item) { self.visit_cfg_if(item)?; - } - - if let ast::ItemKind::Mod(_, ref sub_mod_kind) = item.kind { - let span = item.span; - self.visit_sub_mod( - item, - Module::new( - span, - Some(Cow::Borrowed(sub_mod_kind)), - Cow::Owned(ThinVec::new()), - &[], - ), - )?; + } else if let ast::ItemKind::Mod(_, sub_mod_kind) = &item.kind { + self.visit_sub_mod(&item, Module::new(item.span, Some(sub_mod_kind), &[], &[]))?; } } Ok(()) @@ -208,7 +176,7 @@ impl<'ast, 'sess> ModResolver<'ast, 'sess> { fn visit_sub_mod( &mut self, - item: &ast::Item, + item: &'ast ast::Item, sub_mod: Module<'ast>, ) -> Result<(), ModuleResolutionError> { if contains_skip(&item.attrs) { @@ -286,14 +254,10 @@ impl<'ast, 'sess> ModResolver<'ast, 'sess> { &mut self, sub_mod: Module<'ast>, ) -> Result<(), ModuleResolutionError> { - match (sub_mod.ast_mod_kind, sub_mod.items) { - (Some(Cow::Borrowed(ast::ModKind::Loaded(items, _, _))), _) => { - self.visit_mod_from_ast(items) - } - (Some(Cow::Owned(ast::ModKind::Loaded(items, _, _))), _) | (_, Cow::Owned(items)) => { - self.visit_mod_outside_ast(&items) - } - (_, _) => Ok(()), + if let Some(ast::ModKind::Loaded(items, _, _)) = sub_mod.ast_mod_kind { + self.visit_items(items) + } else { + self.visit_items(&sub_mod.items) } } @@ -319,8 +283,8 @@ impl<'ast, 'sess> ModResolver<'ast, 'sess> { DirectoryOwnership::Owned { relative: None }, Module::new( span, - Some(Cow::Owned(ast::ModKind::Unloaded)), - Cow::Owned(items), + Some(self.mod_kind_arena.alloc(ast::ModKind::Unloaded)), + self.item_arena.alloc_from_iter(items), &attrs, ), ))), @@ -369,8 +333,8 @@ impl<'ast, 'sess> ModResolver<'ast, 'sess> { dir_ownership, Module::new( span, - Some(Cow::Owned(ast::ModKind::Unloaded)), - Cow::Owned(items), + Some(self.mod_kind_arena.alloc(ast::ModKind::Unloaded)), + self.item_arena.alloc_from_iter(items), &attrs, ), ))) @@ -381,8 +345,8 @@ impl<'ast, 'sess> ModResolver<'ast, 'sess> { dir_ownership, Module::new( span, - Some(Cow::Owned(ast::ModKind::Unloaded)), - Cow::Owned(items), + Some(self.mod_kind_arena.alloc(ast::ModKind::Unloaded)), + self.item_arena.alloc_from_iter(items), &attrs, ), )); @@ -519,8 +483,8 @@ impl<'ast, 'sess> ModResolver<'ast, 'sess> { DirectoryOwnership::Owned { relative: None }, Module::new( span, - Some(Cow::Owned(ast::ModKind::Unloaded)), - Cow::Owned(items), + Some(self.mod_kind_arena.alloc(ast::ModKind::Unloaded)), + self.item_arena.alloc_from_iter(items), &attrs, ), )) From f0b1898fc4de2f75a0fd3db31a3eaca4ee905daf Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Fri, 23 Feb 2024 19:06:15 -0600 Subject: [PATCH 7/9] Simplify find_external_module arguments Defer creating a Module until needed. This is possible now that we are using arenas, so ast Items have the 'ast lifetime. --- src/modules.rs | 54 +++++++++++++++++++++++++++++++++++--------------- 1 file changed, 38 insertions(+), 16 deletions(-) diff --git a/src/modules.rs b/src/modules.rs index 06a4a0b23ee..ac55d479f1a 100644 --- a/src/modules.rs +++ b/src/modules.rs @@ -51,6 +51,18 @@ impl<'a> Module<'a> { } } + pub(crate) fn from_item(item: &'a ast::Item) -> Module<'a> { + let mod_kind = match &item.kind { + ast::ItemKind::Mod(_, mod_kind) => Some(mod_kind), + _ => None, + }; + let items = match &item.kind { + ast::ItemKind::Mod(_, ast::ModKind::Loaded(items, ..)) => &**items, + _ => &[], + }; + Module::new(item.span, mod_kind, items, &item.attrs) + } + pub(crate) fn attrs(&self) -> &[ast::Attribute] { &self.inner_attr } @@ -185,7 +197,7 @@ impl<'ast, 'sess> ModResolver<'ast, 'sess> { if is_mod_decl(item) { // mod foo; // Look for an extern file. - let Some(kind) = self.find_external_module(item.ident, &item.attrs, &sub_mod)? else { + let Some(kind) = self.find_external_module(item)? else { return Ok(()); }; self.insert_sub_mod(kind.clone())?; @@ -264,19 +276,18 @@ impl<'ast, 'sess> ModResolver<'ast, 'sess> { /// Find a file path in the filesystem which corresponds to the given module. fn find_external_module( &self, - mod_name: symbol::Ident, - attrs: &[ast::Attribute], - sub_mod: &Module<'ast>, + item: &'ast ast::Item, ) -> Result>, ModuleResolutionError> { + let mod_name = item.ident; let relative = match self.directory.ownership { DirectoryOwnership::Owned { relative } => relative, DirectoryOwnership::UnownedViaBlock => None, }; - if let Some(path) = Parser::submod_path_from_attr(attrs, &self.directory.path) { + if let Some(path) = Parser::submod_path_from_attr(&item.attrs, &self.directory.path) { if self.parse_sess.is_file_parsed(&path) { return Ok(None); } - return match Parser::parse_file_as_module(self.parse_sess, &path, sub_mod.span) { + return match Parser::parse_file_as_module(self.parse_sess, &path, item.span) { Ok((ref attrs, _, _)) if contains_skip(attrs) => Ok(None), Ok((attrs, items, span)) => Ok(Some(SubModKind::External( path, @@ -300,7 +311,7 @@ impl<'ast, 'sess> ModResolver<'ast, 'sess> { } // Look for nested path, like `#[cfg_attr(feature = "foo", path = "bar.rs")]`. - let mut mods_outside_ast = self.find_mods_outside_of_ast(attrs, sub_mod); + let mut mods_outside_ast = self.find_mods_outside_of_ast(item); match self .parse_sess @@ -320,12 +331,16 @@ impl<'ast, 'sess> ModResolver<'ast, 'sess> { return Ok(None); } else { if should_insert { - mods_outside_ast.push((file_path, dir_ownership, sub_mod.clone())); + mods_outside_ast.push(( + file_path, + dir_ownership, + Module::from_item(item), + )); } return Ok(Some(SubModKind::MultiExternal(mods_outside_ast))); } } - match Parser::parse_file_as_module(self.parse_sess, &file_path, sub_mod.span) { + match Parser::parse_file_as_module(self.parse_sess, &file_path, item.span) { Ok((ref attrs, _, _)) if contains_skip(attrs) => Ok(None), Ok((attrs, items, span)) if outside_mods_empty => { Ok(Some(SubModKind::External( @@ -351,7 +366,11 @@ impl<'ast, 'sess> ModResolver<'ast, 'sess> { ), )); if should_insert { - mods_outside_ast.push((file_path, dir_ownership, sub_mod.clone())); + mods_outside_ast.push(( + file_path, + dir_ownership, + Module::from_item(item), + )); } Ok(Some(SubModKind::MultiExternal(mods_outside_ast))) } @@ -365,7 +384,11 @@ impl<'ast, 'sess> ModResolver<'ast, 'sess> { }), Err(..) => { if should_insert { - mods_outside_ast.push((file_path, dir_ownership, sub_mod.clone())); + mods_outside_ast.push(( + file_path, + dir_ownership, + Module::from_item(item), + )); } Ok(Some(SubModKind::MultiExternal(mods_outside_ast))) } @@ -446,12 +469,11 @@ impl<'ast, 'sess> ModResolver<'ast, 'sess> { fn find_mods_outside_of_ast( &self, - attrs: &[ast::Attribute], - sub_mod: &Module<'ast>, + item: &'ast ast::Item, ) -> Vec<(PathBuf, DirectoryOwnership, Module<'ast>)> { // Filter nested path, like `#[cfg_attr(feature = "foo", path = "bar.rs")]`. let mut path_visitor = visitor::PathVisitor::default(); - for attr in attrs.iter() { + for attr in item.attrs.iter() { if let Some(meta) = attr.meta() { path_visitor.visit_meta_item(&meta) } @@ -467,12 +489,12 @@ impl<'ast, 'sess> ModResolver<'ast, 'sess> { result.push(( actual_path, DirectoryOwnership::Owned { relative: None }, - sub_mod.clone(), + Module::from_item(item), )); continue; } let (attrs, items, span) = - match Parser::parse_file_as_module(self.parse_sess, &actual_path, sub_mod.span) { + match Parser::parse_file_as_module(self.parse_sess, &actual_path, item.span) { Ok((ref attrs, _, _)) if contains_skip(attrs) => continue, Ok(m) => m, Err(..) => continue, From 3a5b7946ff97941a85149fa9edc19a8f991c0c67 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Sat, 24 Feb 2024 10:37:56 -0600 Subject: [PATCH 8/9] Rename visit_mod and simplify arguments --- src/modules.rs | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/src/modules.rs b/src/modules.rs index ac55d479f1a..dc4a89a5fc0 100644 --- a/src/modules.rs +++ b/src/modules.rs @@ -11,7 +11,6 @@ use thiserror::Error; use crate::attr::MetaVisitor; use crate::config::FileName; -use crate::items::is_mod_decl; use crate::parse::parser::{ Directory, DirectoryOwnership, ModError, ModulePathSuccess, Parser, ParserError, }; @@ -179,36 +178,37 @@ impl<'ast, 'sess> ModResolver<'ast, 'sess> { for item in items { if is_cfg_if(item) { self.visit_cfg_if(item)?; - } else if let ast::ItemKind::Mod(_, sub_mod_kind) = &item.kind { - self.visit_sub_mod(&item, Module::new(item.span, Some(sub_mod_kind), &[], &[]))?; + } else if let ast::ItemKind::Mod(_, mod_kind) = &item.kind { + self.visit_mod(&item, mod_kind)?; } } Ok(()) } - fn visit_sub_mod( + fn visit_mod( &mut self, item: &'ast ast::Item, - sub_mod: Module<'ast>, + mod_kind: &'ast ast::ModKind, ) -> Result<(), ModuleResolutionError> { if contains_skip(&item.attrs) { return Ok(()); } - if is_mod_decl(item) { - // mod foo; - // Look for an extern file. - let Some(kind) = self.find_external_module(item)? else { - return Ok(()); - }; - self.insert_sub_mod(kind.clone())?; - self.visit_sub_mod_inner(kind)?; - } else { - // An internal module (`mod foo { /* ... */ }`); - let directory = self.inline_mod_directory(item.ident, &item.attrs); - self.with_directory(directory, |this| { - this.visit_sub_mod_after_directory_update(sub_mod) - })?; - }; + match mod_kind { + ast::ModKind::Loaded(items, ast::Inline::Yes, _) => { + // An internal module (`mod foo { /* ... */ }`); + let directory = self.inline_mod_directory(item.ident, &item.attrs); + self.with_directory(directory, |this| this.visit_items(items))?; + } + _ => { + // mod foo; + // Look for an extern file. + let Some(kind) = self.find_external_module(item)? else { + return Ok(()); + }; + self.insert_sub_mod(kind.clone())?; + self.visit_sub_mod_inner(kind)?; + } + } Ok(()) } From ba2f38db5eee2dc1ec94c5f6009dbece727d2a9d Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Sat, 24 Feb 2024 10:40:56 -0600 Subject: [PATCH 9/9] Factor out Module.ast_mod_kind At this point it is just redundant with Module.items. --- src/formatting.rs | 2 -- src/modules.rs | 60 ++++++----------------------------------------- 2 files changed, 7 insertions(+), 55 deletions(-) diff --git a/src/formatting.rs b/src/formatting.rs index 1ee5b656c9f..40c4d59c50e 100644 --- a/src/formatting.rs +++ b/src/formatting.rs @@ -133,10 +133,8 @@ fn format_project( let mut context = FormatContext::new(&krate, report, parse_session, config, handler); let item_arena = TypedArena::default(); - let mod_kind_arena = TypedArena::default(); let files = modules::ModResolver::new( &item_arena, - &mod_kind_arena, &context.parse_session, directory_ownership.unwrap_or(DirectoryOwnership::UnownedViaBlock), !input_is_stdin && !config.skip_children(), diff --git a/src/modules.rs b/src/modules.rs index dc4a89a5fc0..e043cb5cb51 100644 --- a/src/modules.rs +++ b/src/modules.rs @@ -24,7 +24,6 @@ type FileModMap<'ast> = BTreeMap>; /// Represents module with its inner attributes. #[derive(Debug, Clone)] pub(crate) struct Module<'a> { - ast_mod_kind: Option<&'a ast::ModKind>, pub(crate) items: &'a [rustc_ast::ptr::P], inner_attr: ast::AttrVec, pub(crate) span: Span, @@ -33,7 +32,6 @@ pub(crate) struct Module<'a> { impl<'a> Module<'a> { pub(crate) fn new( mod_span: Span, - ast_mod_kind: Option<&'a ast::ModKind>, mod_items: &'a [rustc_ast::ptr::P], mod_attrs: &[ast::Attribute], ) -> Self { @@ -46,20 +44,15 @@ impl<'a> Module<'a> { items: mod_items, inner_attr, span: mod_span, - ast_mod_kind, } } pub(crate) fn from_item(item: &'a ast::Item) -> Module<'a> { - let mod_kind = match &item.kind { - ast::ItemKind::Mod(_, mod_kind) => Some(mod_kind), - _ => None, - }; let items = match &item.kind { ast::ItemKind::Mod(_, ast::ModKind::Loaded(items, ..)) => &**items, _ => &[], }; - Module::new(item.span, mod_kind, items, &item.attrs) + Module::new(item.span, items, &item.attrs) } pub(crate) fn attrs(&self) -> &[ast::Attribute] { @@ -70,7 +63,6 @@ impl<'a> Module<'a> { /// Maps each module to the corresponding file. pub(crate) struct ModResolver<'ast, 'sess> { item_arena: &'ast TypedArena>, - mod_kind_arena: &'ast TypedArena, parse_sess: &'sess ParseSess, directory: Directory, file_map: FileModMap<'ast>, @@ -114,14 +106,12 @@ impl<'ast, 'sess> ModResolver<'ast, 'sess> { /// Creates a new `ModResolver`. pub(crate) fn new( item_arena: &'ast TypedArena>, - mod_kind_arena: &'ast TypedArena, parse_sess: &'sess ParseSess, directory_ownership: DirectoryOwnership, recursive: bool, ) -> Self { ModResolver { item_arena, - mod_kind_arena, directory: Directory { path: PathBuf::new(), ownership: directory_ownership, @@ -154,7 +144,6 @@ impl<'ast, 'sess> ModResolver<'ast, 'sess> { root_filename, Module::new( mk_sp(snippet_provider.start_pos(), snippet_provider.end_pos()), - None, &krate.items, &krate.attrs, ), @@ -243,9 +232,7 @@ impl<'ast, 'sess> ModResolver<'ast, 'sess> { path: mod_path.parent().unwrap().to_path_buf(), ownership: directory_ownership, }; - self.with_directory(directory, |this| { - this.visit_sub_mod_after_directory_update(sub_mod) - })?; + self.with_directory(directory, |this| this.visit_items(&sub_mod.items))?; } SubModKind::MultiExternal(mods) => { for (mod_path, directory_ownership, sub_mod) in mods { @@ -253,26 +240,13 @@ impl<'ast, 'sess> ModResolver<'ast, 'sess> { path: mod_path.parent().unwrap().to_path_buf(), ownership: directory_ownership, }; - self.with_directory(directory, |this| { - this.visit_sub_mod_after_directory_update(sub_mod) - })?; + self.with_directory(directory, |this| this.visit_items(&sub_mod.items))?; } } } Ok(()) } - fn visit_sub_mod_after_directory_update( - &mut self, - sub_mod: Module<'ast>, - ) -> Result<(), ModuleResolutionError> { - if let Some(ast::ModKind::Loaded(items, _, _)) = sub_mod.ast_mod_kind { - self.visit_items(items) - } else { - self.visit_items(&sub_mod.items) - } - } - /// Find a file path in the filesystem which corresponds to the given module. fn find_external_module( &self, @@ -292,12 +266,7 @@ impl<'ast, 'sess> ModResolver<'ast, 'sess> { Ok((attrs, items, span)) => Ok(Some(SubModKind::External( path, DirectoryOwnership::Owned { relative: None }, - Module::new( - span, - Some(self.mod_kind_arena.alloc(ast::ModKind::Unloaded)), - self.item_arena.alloc_from_iter(items), - &attrs, - ), + Module::new(span, self.item_arena.alloc_from_iter(items), &attrs), ))), Err(ParserError::ParseError) => Err(ModuleResolutionError { module: mod_name.to_string(), @@ -346,24 +315,14 @@ impl<'ast, 'sess> ModResolver<'ast, 'sess> { Ok(Some(SubModKind::External( file_path, dir_ownership, - Module::new( - span, - Some(self.mod_kind_arena.alloc(ast::ModKind::Unloaded)), - self.item_arena.alloc_from_iter(items), - &attrs, - ), + Module::new(span, self.item_arena.alloc_from_iter(items), &attrs), ))) } Ok((attrs, items, span)) => { mods_outside_ast.push(( file_path.clone(), dir_ownership, - Module::new( - span, - Some(self.mod_kind_arena.alloc(ast::ModKind::Unloaded)), - self.item_arena.alloc_from_iter(items), - &attrs, - ), + Module::new(span, self.item_arena.alloc_from_iter(items), &attrs), )); if should_insert { mods_outside_ast.push(( @@ -503,12 +462,7 @@ impl<'ast, 'sess> ModResolver<'ast, 'sess> { result.push(( actual_path, DirectoryOwnership::Owned { relative: None }, - Module::new( - span, - Some(self.mod_kind_arena.alloc(ast::ModKind::Unloaded)), - self.item_arena.alloc_from_iter(items), - &attrs, - ), + Module::new(span, self.item_arena.alloc_from_iter(items), &attrs), )) } result