From 99d02fb40fd339255ed08596ebeb41e9b8a09d45 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 7 Nov 2024 16:09:39 +1100 Subject: [PATCH 1/9] Optimize `check_keyword_case`. `to_lowercase` allocates, but `eq_ignore_ascii_case` doesn't. This path is hot enough that this makes a small but noticeable difference in benchmarking. --- compiler/rustc_parse/src/parser/mod.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_parse/src/parser/mod.rs b/compiler/rustc_parse/src/parser/mod.rs index 50a8b6542df..042ee96bbe8 100644 --- a/compiler/rustc_parse/src/parser/mod.rs +++ b/compiler/rustc_parse/src/parser/mod.rs @@ -641,9 +641,10 @@ impl<'a> Parser<'a> { return true; } + // Do an ASCII case-insensitive match, because all keywords are ASCII. if case == Case::Insensitive && let Some((ident, IdentIsRaw::No)) = self.token.ident() - && ident.as_str().to_lowercase() == kw.as_str().to_lowercase() + && ident.as_str().eq_ignore_ascii_case(kw.as_str()) { true } else { From 93f225856557d5930b64b329237a9a43e09637bc Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 7 Nov 2024 16:08:26 +1100 Subject: [PATCH 2/9] Use iteration instead of indexing to access ribs. This gives a small but noticeable performance improvement. --- compiler/rustc_resolve/src/ident.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_resolve/src/ident.rs b/compiler/rustc_resolve/src/ident.rs index b80e0e196ca..c8f7a12132c 100644 --- a/compiler/rustc_resolve/src/ident.rs +++ b/compiler/rustc_resolve/src/ident.rs @@ -311,13 +311,12 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { // Walk backwards up the ribs in scope. let mut module = self.graph_root; - for i in (0..ribs.len()).rev() { - debug!("walk rib\n{:?}", ribs[i].bindings); + for (i, rib) in ribs.iter().enumerate().rev() { + debug!("walk rib\n{:?}", rib.bindings); // Use the rib kind to determine whether we are resolving parameters // (macro 2.0 hygiene) or local variables (`macro_rules` hygiene). - let rib_ident = if ribs[i].kind.contains_params() { normalized_ident } else { ident }; - if let Some((original_rib_ident_def, res)) = ribs[i].bindings.get_key_value(&rib_ident) - { + let rib_ident = if rib.kind.contains_params() { normalized_ident } else { ident }; + if let Some((original_rib_ident_def, res)) = rib.bindings.get_key_value(&rib_ident) { // The ident resolves to a type parameter or local variable. return Some(LexicalScopeBinding::Res(self.validate_res_from_ribs( i, @@ -329,7 +328,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { ))); } - module = match ribs[i].kind { + module = match rib.kind { RibKind::Module(module) => module, RibKind::MacroDefinition(def) if def == self.macro_def(ident.span.ctxt()) => { // If an invocation of this macro created `ident`, give up on `ident` From d1d8be1d137404a52cdd28e78a9a9c59d38615b0 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 8 Nov 2024 08:50:51 +1100 Subject: [PATCH 3/9] Tweak a `resolutions` loop. In this case field access is more concise and easier to read than destructuring, and it matches how other similar loops are done elsewhere. --- compiler/rustc_resolve/src/lib.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_resolve/src/lib.rs b/compiler/rustc_resolve/src/lib.rs index 9abb3180388..cf37a4e3389 100644 --- a/compiler/rustc_resolve/src/lib.rs +++ b/compiler/rustc_resolve/src/lib.rs @@ -1809,12 +1809,11 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { assoc_item: Option<(Symbol, Namespace)>, ) -> bool { match (trait_module, assoc_item) { - (Some(trait_module), Some((name, ns))) => { - self.resolutions(trait_module).borrow().iter().any(|resolution| { - let (&BindingKey { ident: assoc_ident, ns: assoc_ns, .. }, _) = resolution; - assoc_ns == ns && assoc_ident.name == name - }) - } + (Some(trait_module), Some((name, ns))) => self + .resolutions(trait_module) + .borrow() + .iter() + .any(|(key, _name_resolution)| key.ns == ns && key.ident.name == name), _ => true, } } From d34f2823fdaeeee3bced7834d43589a3954bc62f Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 8 Nov 2024 09:00:39 +1100 Subject: [PATCH 4/9] Use `for_each_child` in a suitable place. `for_each_child` exists for this exact pattern. --- compiler/rustc_resolve/src/diagnostics.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_resolve/src/diagnostics.rs b/compiler/rustc_resolve/src/diagnostics.rs index 5437ca65935..5b78acd904a 100644 --- a/compiler/rustc_resolve/src/diagnostics.rs +++ b/compiler/rustc_resolve/src/diagnostics.rs @@ -535,14 +535,12 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { filter_fn: &impl Fn(Res) -> bool, ctxt: Option, ) { - for (key, resolution) in self.resolutions(module).borrow().iter() { - if let Some(binding) = resolution.borrow().binding { - let res = binding.res(); - if filter_fn(res) && ctxt.map_or(true, |ctxt| ctxt == key.ident.span.ctxt()) { - names.push(TypoSuggestion::typo_from_ident(key.ident, res)); - } + module.for_each_child(self, |_this, ident, _ns, binding| { + let res = binding.res(); + if filter_fn(res) && ctxt.map_or(true, |ctxt| ctxt == ident.span.ctxt()) { + names.push(TypoSuggestion::typo_from_ident(ident, res)); } - } + }); } /// Combines an error with provided span and emits it. From e7dffeedcffce81f709da8a076b8ff437b8d3060 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 8 Nov 2024 10:42:07 +1100 Subject: [PATCH 5/9] Use an atom comparison for a keyword check. Instead of a string comparison. --- compiler/rustc_resolve/src/ident.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_resolve/src/ident.rs b/compiler/rustc_resolve/src/ident.rs index c8f7a12132c..d8e1e296b22 100644 --- a/compiler/rustc_resolve/src/ident.rs +++ b/compiler/rustc_resolve/src/ident.rs @@ -1199,7 +1199,9 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { // Still doesn't deal with upvars if let Some(span) = finalize { let (span, resolution_error) = match item { - None if rib_ident.as_str() == "self" => (span, LowercaseSelf), + None if rib_ident.name == kw::SelfLower => { + (span, LowercaseSelf) + } None => { // If we have a `let name = expr;`, we have the span for // `name` and use that to see if it is followed by a type From d71c06d022b07518f97072ad36cbe2c8a25eee2d Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 8 Nov 2024 15:16:16 +1100 Subject: [PATCH 6/9] Remove two `_ext` methods. `resolve_ident_in_module` is a very thin wrapper around `resolve_ident_in_module_ext`, and `resolve_ident_in_module_unadjusted` is a very thin wrapper around `resolve_ident_in_module_unadjusted_ext`. The wrappers make the call sites slightly more concise, but I don't think that's worth the extra code and indirection. This commit removes the two wrappers and removes the `_ext` suffixes from the inner methods. --- compiler/rustc_resolve/src/ident.rs | 69 ++++++----------------------- 1 file changed, 13 insertions(+), 56 deletions(-) diff --git a/compiler/rustc_resolve/src/ident.rs b/compiler/rustc_resolve/src/ident.rs index d8e1e296b22..b8e07ef5402 100644 --- a/compiler/rustc_resolve/src/ident.rs +++ b/compiler/rustc_resolve/src/ident.rs @@ -349,6 +349,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { ident, ns, parent_scope, + false, finalize.map(|finalize| Finalize { used: Used::Scope, ..finalize }), ignore_binding, None, @@ -493,7 +494,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { Scope::CrateRoot => { let root_ident = Ident::new(kw::PathRoot, ident.span); let root_module = this.resolve_crate_root(root_ident); - let binding = this.resolve_ident_in_module_ext( + let binding = this.resolve_ident_in_module( ModuleOrUniformRoot::Module(root_module), ident, ns, @@ -515,7 +516,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { } Scope::Module(module, derive_fallback_lint_id) => { let adjusted_parent_scope = &ParentScope { module, ..*parent_scope }; - let binding = this.resolve_ident_in_module_unadjusted_ext( + let binding = this.resolve_ident_in_module_unadjusted( ModuleOrUniformRoot::Module(module), ident, ns, @@ -589,6 +590,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { ident, ns, parent_scope, + false, None, ignore_binding, ignore_import, @@ -747,35 +749,12 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { parent_scope: &ParentScope<'ra>, ignore_import: Option>, ) -> Result, Determinacy> { - self.resolve_ident_in_module_ext(module, ident, ns, parent_scope, None, None, ignore_import) + self.resolve_ident_in_module(module, ident, ns, parent_scope, None, None, ignore_import) .map_err(|(determinacy, _)| determinacy) } #[instrument(level = "debug", skip(self))] pub(crate) fn resolve_ident_in_module( - &mut self, - module: ModuleOrUniformRoot<'ra>, - ident: Ident, - ns: Namespace, - parent_scope: &ParentScope<'ra>, - finalize: Option, - ignore_binding: Option>, - ignore_import: Option>, - ) -> Result, Determinacy> { - self.resolve_ident_in_module_ext( - module, - ident, - ns, - parent_scope, - finalize, - ignore_binding, - ignore_import, - ) - .map_err(|(determinacy, _)| determinacy) - } - - #[instrument(level = "debug", skip(self))] - fn resolve_ident_in_module_ext( &mut self, module: ModuleOrUniformRoot<'ra>, mut ident: Ident, @@ -802,7 +781,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { // No adjustments } } - self.resolve_ident_in_module_unadjusted_ext( + self.resolve_ident_in_module_unadjusted( module, ident, ns, @@ -814,34 +793,10 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { ) } - #[instrument(level = "debug", skip(self))] - fn resolve_ident_in_module_unadjusted( - &mut self, - module: ModuleOrUniformRoot<'ra>, - ident: Ident, - ns: Namespace, - parent_scope: &ParentScope<'ra>, - finalize: Option, - ignore_binding: Option>, - ignore_import: Option>, - ) -> Result, Determinacy> { - self.resolve_ident_in_module_unadjusted_ext( - module, - ident, - ns, - parent_scope, - false, - finalize, - ignore_binding, - ignore_import, - ) - .map_err(|(determinacy, _)| determinacy) - } - /// Attempts to resolve `ident` in namespaces `ns` of `module`. /// Invariant: if `finalize` is `Some`, expansion and import resolution must be complete. #[instrument(level = "debug", skip(self))] - fn resolve_ident_in_module_unadjusted_ext( + fn resolve_ident_in_module_unadjusted( &mut self, module: ModuleOrUniformRoot<'ra>, ident: Ident, @@ -1046,13 +1001,13 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { ignore_binding, ignore_import, ) { - Err(Determined) => continue, + Err((Determined, _)) => continue, Ok(binding) if !self.is_accessible_from(binding.vis, single_import.parent_scope.module) => { continue; } - Ok(_) | Err(Undetermined) => return Err((Undetermined, Weak::No)), + Ok(_) | Err((Undetermined, _)) => return Err((Undetermined, Weak::No)), } } @@ -1121,19 +1076,20 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { ident, ns, adjusted_parent_scope, + false, None, ignore_binding, ignore_import, ); match result { - Err(Determined) => continue, + Err((Determined, _)) => continue, Ok(binding) if !self.is_accessible_from(binding.vis, glob_import.parent_scope.module) => { continue; } - Ok(_) | Err(Undetermined) => return Err((Undetermined, Weak::Yes)), + Ok(_) | Err((Undetermined, _)) => return Err((Undetermined, Weak::Yes)), } } @@ -1564,6 +1520,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { ignore_binding, ignore_import, ) + .map_err(|(determinacy, _)| determinacy) } else if let Some(ribs) = ribs && let Some(TypeNS | ValueNS) = opt_ns { From 6e0e9edbe034b8ce764f91abb5c12b7871e09e9c Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 8 Nov 2024 15:24:13 +1100 Subject: [PATCH 7/9] Move a call outside a loop. This path isn't hot enough for this to affect performance, but there's no point repeating the same computation multiple times. --- compiler/rustc_resolve/src/late.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_resolve/src/late.rs b/compiler/rustc_resolve/src/late.rs index f4a85c358e3..26b345f5941 100644 --- a/compiler/rustc_resolve/src/late.rs +++ b/compiler/rustc_resolve/src/late.rs @@ -1688,9 +1688,9 @@ impl<'a, 'ast, 'ra: 'ast, 'tcx> LateResolutionVisitor<'a, 'ast, 'ra, 'tcx> { } } + let normalized_ident = ident.normalize_to_macros_2_0(); let mut outer_res = None; for rib in lifetime_rib_iter { - let normalized_ident = ident.normalize_to_macros_2_0(); if let Some((&outer, _)) = rib.bindings.get_key_value(&normalized_ident) { outer_res = Some(outer); break; From 66cc7d6a05f2fadbe100e12ebc65d511fa690aea Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 14 Nov 2024 10:20:45 +1100 Subject: [PATCH 8/9] Replace the `restricted_shadowing` boolean argument with an enum. It makes the code clearer. --- compiler/rustc_resolve/src/ident.rs | 34 +++++++++++++++++++---------- 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/compiler/rustc_resolve/src/ident.rs b/compiler/rustc_resolve/src/ident.rs index b8e07ef5402..ad825d7813d 100644 --- a/compiler/rustc_resolve/src/ident.rs +++ b/compiler/rustc_resolve/src/ident.rs @@ -38,6 +38,12 @@ impl From for bool { } } +#[derive(Debug, PartialEq)] +enum Shadowing { + Restricted, + Unrestricted, +} + impl<'ra, 'tcx> Resolver<'ra, 'tcx> { /// A generic scope visitor. /// Visits scopes in order to resolve some identifier in them or perform other actions. @@ -349,7 +355,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { ident, ns, parent_scope, - false, + Shadowing::Unrestricted, finalize.map(|finalize| Finalize { used: Used::Scope, ..finalize }), ignore_binding, None, @@ -521,7 +527,11 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { ident, ns, adjusted_parent_scope, - !matches!(scope_set, ScopeSet::Late(..)), + if matches!(scope_set, ScopeSet::Late(..)) { + Shadowing::Unrestricted + } else { + Shadowing::Restricted + }, finalize.map(|finalize| Finalize { used: Used::Scope, ..finalize }), ignore_binding, ignore_import, @@ -590,7 +600,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { ident, ns, parent_scope, - false, + Shadowing::Unrestricted, None, ignore_binding, ignore_import, @@ -786,7 +796,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { ident, ns, adjusted_parent_scope, - false, + Shadowing::Unrestricted, finalize, ignore_binding, ignore_import, @@ -802,7 +812,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { ident: Ident, ns: Namespace, parent_scope: &ParentScope<'ra>, - restricted_shadowing: bool, + shadowing: Shadowing, finalize: Option, // This binding should be ignored during in-module resolution, so that we don't get // "self-confirming" import resolutions during import validation and checking. @@ -812,7 +822,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { let module = match module { ModuleOrUniformRoot::Module(module) => module, ModuleOrUniformRoot::CrateRootAndExternPrelude => { - assert!(!restricted_shadowing); + assert_eq!(shadowing, Shadowing::Unrestricted); let binding = self.early_resolve_ident_in_lexical_scope( ident, ScopeSet::AbsolutePath(ns), @@ -825,7 +835,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { return binding.map_err(|determinacy| (determinacy, Weak::No)); } ModuleOrUniformRoot::ExternPrelude => { - assert!(!restricted_shadowing); + assert_eq!(shadowing, Shadowing::Unrestricted); return if ns != TypeNS { Err((Determined, Weak::No)) } else if let Some(binding) = self.extern_prelude_get(ident, finalize.is_some()) { @@ -838,7 +848,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { }; } ModuleOrUniformRoot::CurrentScope => { - assert!(!restricted_shadowing); + assert_eq!(shadowing, Shadowing::Unrestricted); if ns == TypeNS { if ident.name == kw::Crate || ident.name == kw::DollarCrate { let module = self.resolve_crate_root(ident); @@ -897,7 +907,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { // Forbid expanded shadowing to avoid time travel. if let Some(shadowed_glob) = resolution.shadowed_glob - && restricted_shadowing + && shadowing == Shadowing::Restricted && binding.expansion != LocalExpnId::ROOT && binding.res() != shadowed_glob.res() { @@ -912,7 +922,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { }); } - if !restricted_shadowing + if shadowing == Shadowing::Unrestricted && binding.expansion != LocalExpnId::ROOT && let NameBindingKind::Import { import, .. } = binding.kind && matches!(import.kind, ImportKind::MacroExport) @@ -1024,7 +1034,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { // and prohibit access to macro-expanded `macro_export` macros instead (unless restricted // shadowing is enabled, see `macro_expanded_macro_export_errors`). if let Some(binding) = binding { - if binding.determined() || ns == MacroNS || restricted_shadowing { + if binding.determined() || ns == MacroNS || shadowing == Shadowing::Restricted { return check_usable(self, binding); } else { return Err((Undetermined, Weak::No)); @@ -1076,7 +1086,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { ident, ns, adjusted_parent_scope, - false, + Shadowing::Unrestricted, None, ignore_binding, ignore_import, From 12747f188a9dba7b13cf1ccb01fef74c6ce3294e Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 14 Nov 2024 11:34:49 +1100 Subject: [PATCH 9/9] Remove `Resolver::empty_disambiguator`. It was added in #115367 for anonymous ADTs. Those changes were then reverted in #131045, but `empty_disambiguator` was left behind, perhaps by mistake. It seems to be unnecessary. --- compiler/rustc_resolve/src/lib.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/compiler/rustc_resolve/src/lib.rs b/compiler/rustc_resolve/src/lib.rs index cf37a4e3389..e382295b8f6 100644 --- a/compiler/rustc_resolve/src/lib.rs +++ b/compiler/rustc_resolve/src/lib.rs @@ -1082,8 +1082,6 @@ pub struct Resolver<'ra, 'tcx> { binding_parent_modules: FxHashMap, Module<'ra>>, underscore_disambiguator: u32, - /// Disambiguator for anonymous adts. - empty_disambiguator: u32, /// Maps glob imports to the names of items actually imported. glob_map: FxHashMap>, @@ -1462,7 +1460,6 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { module_children: Default::default(), trait_map: NodeMap::default(), underscore_disambiguator: 0, - empty_disambiguator: 0, empty_module, module_map, block_map: Default::default(), @@ -1841,9 +1838,6 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { let disambiguator = if ident.name == kw::Underscore { self.underscore_disambiguator += 1; self.underscore_disambiguator - } else if ident.name == kw::Empty { - self.empty_disambiguator += 1; - self.empty_disambiguator } else { 0 };