From fbf1bec48228a5c6c16073319cd4c9a54ec28c9f Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Mon, 8 Mar 2021 15:05:03 +0300 Subject: resolve/expand: Cache intermediate results of `#[derive]` expansion --- compiler/rustc_resolve/src/lib.rs | 15 +++++-- compiler/rustc_resolve/src/macros.rs | 87 ++++++++++++++++++++---------------- 2 files changed, 60 insertions(+), 42 deletions(-) (limited to 'compiler/rustc_resolve') diff --git a/compiler/rustc_resolve/src/lib.rs b/compiler/rustc_resolve/src/lib.rs index fe5078f26bd..9488ce14a54 100644 --- a/compiler/rustc_resolve/src/lib.rs +++ b/compiler/rustc_resolve/src/lib.rs @@ -37,7 +37,7 @@ use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexMap}; use rustc_data_structures::ptr_key::PtrKey; use rustc_data_structures::sync::Lrc; use rustc_errors::{struct_span_err, Applicability, DiagnosticBuilder}; -use rustc_expand::base::{SyntaxExtension, SyntaxExtensionKind}; +use rustc_expand::base::{DeriveResolutions, SyntaxExtension, SyntaxExtensionKind}; use rustc_hir::def::Namespace::*; use rustc_hir::def::{self, CtorOf, DefKind, NonMacroAttrKind, PartialRes}; use rustc_hir::def_id::{CrateNum, DefId, DefIdMap, LocalDefId, CRATE_DEF_INDEX}; @@ -851,6 +851,12 @@ enum BuiltinMacroState { AlreadySeen(Span), } +struct DeriveData { + resolutions: DeriveResolutions, + helper_attrs: Vec, + has_derive_copy: bool, +} + /// The main resolver class. /// /// This is the visitor that walks the whole crate. @@ -973,8 +979,9 @@ pub struct Resolver<'a> { output_macro_rules_scopes: FxHashMap>, /// Helper attributes that are in scope for the given expansion. helper_attrs: FxHashMap>, - /// Resolutions for paths inside the `#[derive(...)]` attribute with the given `ExpnId`. - derive_resolutions: FxHashMap, ast::Path)>>, + /// Ready or in-progress results of resolving paths inside the `#[derive(...)]` attribute + /// with the given `ExpnId`. + derive_data: FxHashMap, /// Avoid duplicated errors for "name already defined". name_already_seen: FxHashMap, @@ -1310,7 +1317,7 @@ impl<'a> Resolver<'a> { invocation_parent_scopes: Default::default(), output_macro_rules_scopes: Default::default(), helper_attrs: Default::default(), - derive_resolutions: Default::default(), + derive_data: Default::default(), local_macro_def_scopes: FxHashMap::default(), name_already_seen: FxHashMap::default(), potentially_unused_imports: Vec::new(), diff --git a/compiler/rustc_resolve/src/macros.rs b/compiler/rustc_resolve/src/macros.rs index d238f65c941..567a99e4abf 100644 --- a/compiler/rustc_resolve/src/macros.rs +++ b/compiler/rustc_resolve/src/macros.rs @@ -4,7 +4,7 @@ use crate::imports::ImportResolver; use crate::Namespace::*; use crate::{AmbiguityError, AmbiguityErrorMisc, AmbiguityKind, BuiltinMacroState, Determinacy}; -use crate::{CrateLint, ParentScope, ResolutionError, Resolver, Scope, ScopeSet, Weak}; +use crate::{CrateLint, DeriveData, ParentScope, ResolutionError, Resolver, Scope, ScopeSet, Weak}; use crate::{ModuleKind, ModuleOrUniformRoot, NameBinding, PathResult, Segment, ToNameBinding}; use rustc_ast::{self as ast, Inline, ItemKind, ModKind, NodeId}; use rustc_ast_lowering::ResolverAstLowering; @@ -14,8 +14,8 @@ use rustc_data_structures::fx::FxHashSet; use rustc_data_structures::ptr_key::PtrKey; use rustc_data_structures::sync::Lrc; use rustc_errors::struct_span_err; -use rustc_expand::base::Annotatable; -use rustc_expand::base::{Indeterminate, ResolverExpand, SyntaxExtension, SyntaxExtensionKind}; +use rustc_expand::base::{Annotatable, DeriveResolutions, Indeterminate, ResolverExpand}; +use rustc_expand::base::{SyntaxExtension, SyntaxExtensionKind}; use rustc_expand::compile_declarative_macro; use rustc_expand::expand::{AstFragment, Invocation, InvocationKind, SupportsMacroExpansion}; use rustc_feature::is_builtin_attr_name; @@ -359,8 +359,8 @@ impl<'a> ResolverExpand for Resolver<'a> { fn resolve_derives( &mut self, expn_id: ExpnId, - derives: Vec, force: bool, + derive_paths: &dyn Fn() -> DeriveResolutions, ) -> Result<(), Indeterminate> { // Block expansion of the container until we resolve all derives in it. // This is required for two reasons: @@ -368,49 +368,60 @@ impl<'a> ResolverExpand for Resolver<'a> { // is applied, so they have to be produced by the container's expansion rather // than by individual derives. // - Derives in the container need to know whether one of them is a built-in `Copy`. - // FIXME: Try to cache intermediate results to avoid resolving same derives multiple times. + // Temporarily take the data to avoid borrow checker conflicts. + let mut derive_data = mem::take(&mut self.derive_data); + let entry = derive_data.entry(expn_id).or_insert_with(|| DeriveData { + resolutions: derive_paths(), + helper_attrs: Vec::new(), + has_derive_copy: false, + }); let parent_scope = self.invocation_parent_scopes[&expn_id]; - let mut exts = Vec::new(); - let mut helper_attrs = Vec::new(); - let mut has_derive_copy = false; - for path in derives { - exts.push(( - match self.resolve_macro_path( - &path, - Some(MacroKind::Derive), - &parent_scope, - true, - force, - ) { - Ok((Some(ext), _)) => { - let span = - path.segments.last().unwrap().ident.span.normalize_to_macros_2_0(); - helper_attrs - .extend(ext.helper_attrs.iter().map(|name| Ident::new(*name, span))); - has_derive_copy |= ext.builtin_name == Some(sym::Copy); - ext - } - Ok(_) | Err(Determinacy::Determined) => self.dummy_ext(MacroKind::Derive), - Err(Determinacy::Undetermined) => return Err(Indeterminate), - }, - path, - )) + for (path, opt_ext) in &mut entry.resolutions { + if opt_ext.is_none() { + *opt_ext = Some( + match self.resolve_macro_path( + &path, + Some(MacroKind::Derive), + &parent_scope, + true, + force, + ) { + Ok((Some(ext), _)) => { + if !ext.helper_attrs.is_empty() { + let last_seg = path.segments.last().unwrap(); + let span = last_seg.ident.span.normalize_to_macros_2_0(); + entry.helper_attrs.extend( + ext.helper_attrs.iter().map(|name| Ident::new(*name, span)), + ); + } + entry.has_derive_copy |= ext.builtin_name == Some(sym::Copy); + ext + } + Ok(_) | Err(Determinacy::Determined) => self.dummy_ext(MacroKind::Derive), + Err(Determinacy::Undetermined) => { + assert!(self.derive_data.is_empty()); + self.derive_data = derive_data; + return Err(Indeterminate); + } + }, + ); + } } - self.derive_resolutions.insert(expn_id, exts); - self.helper_attrs.insert(expn_id, helper_attrs); + // If we get to here, then `derive_data` for the given `expn_id` will only be accessed by + // `take_derive_resolutions` later, so we can steal `helper_attrs` instead of cloning them. + self.helper_attrs.insert(expn_id, mem::take(&mut entry.helper_attrs)); // Mark this derive as having `Copy` either if it has `Copy` itself or if its parent derive // has `Copy`, to support cases like `#[derive(Clone, Copy)] #[derive(Debug)]`. - if has_derive_copy || self.has_derive_copy(parent_scope.expansion) { + if entry.has_derive_copy || self.has_derive_copy(parent_scope.expansion) { self.containers_deriving_copy.insert(expn_id); } + assert!(self.derive_data.is_empty()); + self.derive_data = derive_data; Ok(()) } - fn take_derive_resolutions( - &mut self, - expn_id: ExpnId, - ) -> Option, ast::Path)>> { - self.derive_resolutions.remove(&expn_id) + fn take_derive_resolutions(&mut self, expn_id: ExpnId) -> Option { + self.derive_data.remove(&expn_id).map(|data| data.resolutions) } // The function that implements the resolution logic of `#[cfg_accessible(path)]`. -- cgit 1.4.1-3-g733a5 From b96584485a43c561d90eb262651ecb87d1d98d2a Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Sun, 4 Apr 2021 17:51:31 +0300 Subject: resolve: Stable order for derive helper attributes --- compiler/rustc_resolve/src/lib.rs | 2 +- compiler/rustc_resolve/src/macros.rs | 13 ++++++++----- 2 files changed, 9 insertions(+), 6 deletions(-) (limited to 'compiler/rustc_resolve') diff --git a/compiler/rustc_resolve/src/lib.rs b/compiler/rustc_resolve/src/lib.rs index 9488ce14a54..d474e990211 100644 --- a/compiler/rustc_resolve/src/lib.rs +++ b/compiler/rustc_resolve/src/lib.rs @@ -853,7 +853,7 @@ enum BuiltinMacroState { struct DeriveData { resolutions: DeriveResolutions, - helper_attrs: Vec, + helper_attrs: Vec<(usize, Ident)>, has_derive_copy: bool, } diff --git a/compiler/rustc_resolve/src/macros.rs b/compiler/rustc_resolve/src/macros.rs index 567a99e4abf..10e27f33c29 100644 --- a/compiler/rustc_resolve/src/macros.rs +++ b/compiler/rustc_resolve/src/macros.rs @@ -376,7 +376,7 @@ impl<'a> ResolverExpand for Resolver<'a> { has_derive_copy: false, }); let parent_scope = self.invocation_parent_scopes[&expn_id]; - for (path, opt_ext) in &mut entry.resolutions { + for (i, (path, opt_ext)) in entry.resolutions.iter_mut().enumerate() { if opt_ext.is_none() { *opt_ext = Some( match self.resolve_macro_path( @@ -391,7 +391,9 @@ impl<'a> ResolverExpand for Resolver<'a> { let last_seg = path.segments.last().unwrap(); let span = last_seg.ident.span.normalize_to_macros_2_0(); entry.helper_attrs.extend( - ext.helper_attrs.iter().map(|name| Ident::new(*name, span)), + ext.helper_attrs + .iter() + .map(|name| (i, Ident::new(*name, span))), ); } entry.has_derive_copy |= ext.builtin_name == Some(sym::Copy); @@ -407,9 +409,10 @@ impl<'a> ResolverExpand for Resolver<'a> { ); } } - // If we get to here, then `derive_data` for the given `expn_id` will only be accessed by - // `take_derive_resolutions` later, so we can steal `helper_attrs` instead of cloning them. - self.helper_attrs.insert(expn_id, mem::take(&mut entry.helper_attrs)); + // Sort helpers in a stable way independent from the derive resolution order. + entry.helper_attrs.sort_by_key(|(i, _)| *i); + self.helper_attrs + .insert(expn_id, entry.helper_attrs.iter().map(|(_, ident)| *ident).collect()); // Mark this derive as having `Copy` either if it has `Copy` itself or if its parent derive // has `Copy`, to support cases like `#[derive(Clone, Copy)] #[derive(Debug)]`. if entry.has_derive_copy || self.has_derive_copy(parent_scope.expansion) { -- cgit 1.4.1-3-g733a5