From 55140ba8d534a805f9080b7fa4e9ba8fc99e8d31 Mon Sep 17 00:00:00 2001 From: jjcnn <38888011+jjcnn@users.noreply.github.com> Date: Wed, 24 Apr 2024 17:04:26 +0200 Subject: [PATCH] Refactor: Split use synonyms into separate maps for glob and item imports (#5914) ## Description Fixes #5911 . So far all imported items in a module have been collected in single map, with a flag in each value indicating whether it was the result of a glob/star import or an item import (this distinction matters because item imports shadow glob imports). This PR splits the map in two: One for glob imports and one for item imports. This simplifies name resolution, and eliminates the `GlobImport` flag. When another module imports all reexported items the logic is now slightly more complex, but this will be resolved once the excessive cloning of namespaces, modules and typed declarations are eliminated. ## Checklist - [x] I have linked to any relevant issues. - [x] I have commented my code, particularly in hard-to-understand areas. - [x] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [ ] If my change requires substantial documentation changes, I have [requested support from the DevRel team](https://github.com/FuelLabs/devrel-requests/issues/new/choose) - [x] I have added tests that prove my fix is effective or that my feature works. - [x] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [x] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [x] I have requested a review from the relevant team or maintainers. --- sway-core/src/language/call_path.rs | 15 +++- .../namespace/lexical_scope.rs | 18 +++-- .../src/semantic_analysis/namespace/module.rs | 44 ++++++------ .../src/semantic_analysis/namespace/root.rs | 68 ++++++++++--------- 4 files changed, 79 insertions(+), 66 deletions(-) diff --git a/sway-core/src/language/call_path.rs b/sway-core/src/language/call_path.rs index 6548e1b433d..80c16005b27 100644 --- a/sway-core/src/language/call_path.rs +++ b/sway-core/src/language/call_path.rs @@ -312,7 +312,20 @@ impl CallPath { let mut is_absolute = false; if let Some(use_synonym) = namespace.module_id(engines).read(engines, |m| { - m.current_items().use_synonyms.get(&self.suffix).cloned() + if m.current_items() + .use_item_synonyms + .contains_key(&self.suffix) + { + m.current_items() + .use_item_synonyms + .get(&self.suffix) + .cloned() + } else { + m.current_items() + .use_glob_synonyms + .get(&self.suffix) + .cloned() + } }) { synonym_prefixes = use_synonym.0.clone(); is_absolute = true; diff --git a/sway-core/src/semantic_analysis/namespace/lexical_scope.rs b/sway-core/src/semantic_analysis/namespace/lexical_scope.rs index f39f3401b91..eaf5e4093c2 100644 --- a/sway-core/src/semantic_analysis/namespace/lexical_scope.rs +++ b/sway-core/src/semantic_analysis/namespace/lexical_scope.rs @@ -21,13 +21,6 @@ use sway_types::{span::Span, Spanned}; use std::sync::Arc; -/// Is this a glob (`use foo::*;`) import? -#[derive(Clone, Copy, PartialEq, Debug)] -pub(crate) enum GlobImport { - Yes, - No, -} - pub enum ResolvedFunctionDecl { Parsed(ParsedDeclId), Typed(DeclRefFunction), @@ -45,7 +38,8 @@ impl ResolvedFunctionDecl { pub(super) type ParsedSymbolMap = im::OrdMap; pub(super) type SymbolMap = im::OrdMap; // The `Vec` path is absolute. -pub(super) type UseSynonyms = im::HashMap, GlobImport, ty::TyDecl)>; +pub(super) type GlobSynonyms = im::HashMap, ty::TyDecl)>; +pub(super) type ItemSynonyms = im::HashMap, ty::TyDecl)>; pub(super) type UseAliases = im::HashMap; /// Represents a lexical scope integer-based identifier, which can be used to reference @@ -80,7 +74,11 @@ pub struct Items { /// /// For example, in `use ::foo::bar::Baz;`, we store a mapping from the symbol `Baz` to its /// path `foo::bar::Baz`. - pub(crate) use_synonyms: UseSynonyms, + /// + /// use_glob_synonyms contains symbols imported using star imports (`use foo::*`.). + /// use_item_synonyms contains symbols imported using item imports (`use foo::bar`). + pub(crate) use_glob_synonyms: GlobSynonyms, + pub(crate) use_item_synonyms: ItemSynonyms, /// Represents an alternative name for an imported symbol. /// /// Aliases are introduced with syntax like `use foo::bar as baz;` syntax, where `baz` is an @@ -289,7 +287,7 @@ impl Items { append_shadowing_error(ident, decl, false, false, &item, const_shadowing_mode); } - if let Some((ident, (_, GlobImport::No, decl))) = self.use_synonyms.get_key_value(&name) { + if let Some((ident, (_, decl))) = self.use_item_synonyms.get_key_value(&name) { append_shadowing_error( ident, decl, diff --git a/sway-core/src/semantic_analysis/namespace/module.rs b/sway-core/src/semantic_analysis/namespace/module.rs index 8ef237c9c58..16cad9931e8 100644 --- a/sway-core/src/semantic_analysis/namespace/module.rs +++ b/sway-core/src/semantic_analysis/namespace/module.rs @@ -653,30 +653,28 @@ impl Module { .use_aliases .get(symbol.as_str()) .unwrap_or(symbol); - match module.current_items().use_synonyms.get(symbol) { - Some((_, _, decl @ ty::TyDecl::EnumVariantDecl { .. })) => { - Ok(ResolvedDeclaration::Typed(decl.clone())) - } - Some((src_path, _, _)) if mod_path != src_path => { - // If the symbol is imported, before resolving to it, - // we need to check if there is a local symbol withing the module with - // the same name, and if yes resolve to the local symbol, because it - // shadows the import. - // Note that we can have two situations here: - // - glob-import, in which case the local symbol simply shadows the glob-imported one. - // - non-glob import, in which case we will already have a name clash reported - // as an error, but still have to resolve to the local module symbol - // if it exists. - match module.current_items().symbols.get(true_symbol) { - Some(decl) => Ok(ResolvedDeclaration::Typed(decl.clone())), - None => self.resolve_symbol(handler, engines, src_path, true_symbol, self_type), - } - } - _ => module - .current_items() - .check_symbol(true_symbol) - .map_err(|e| handler.emit_err(e)), + // Check locally declared items. Any name clash with imports will have already been reported as an error. + if let Some(decl) = module.current_items().symbols.get(true_symbol) { + return Ok(ResolvedDeclaration::Typed(decl.clone())); + } + // Check item imports + if let Some((_, decl @ ty::TyDecl::EnumVariantDecl { .. })) = + module.current_items().use_item_synonyms.get(symbol) + { + return Ok(ResolvedDeclaration::Typed(decl.clone())); + } + if let Some((src_path, _)) = module.current_items().use_item_synonyms.get(symbol) { + return self.resolve_symbol(handler, engines, src_path, true_symbol, self_type); + } + // Check glob imports + if let Some((_, decl)) = module.current_items().use_glob_synonyms.get(symbol) { + return Ok(ResolvedDeclaration::Typed(decl.clone())); } + // Symbol not found + Err(handler.emit_err(CompileError::SymbolNotFound { + name: symbol.clone(), + span: symbol.span(), + })) } } diff --git a/sway-core/src/semantic_analysis/namespace/root.rs b/sway-core/src/semantic_analysis/namespace/root.rs index dc87fabab4c..b02835db5d8 100644 --- a/sway-core/src/semantic_analysis/namespace/root.rs +++ b/sway-core/src/semantic_analysis/namespace/root.rs @@ -1,6 +1,4 @@ -use super::{ - lexical_scope::GlobImport, module::Module, namespace::Namespace, trait_map::TraitMap, Ident, -}; +use super::{module::Module, namespace::Namespace, trait_map::TraitMap, Ident}; use crate::{ decl_engine::DeclRef, engine_threading::*, @@ -61,10 +59,10 @@ impl Root { .implemented_traits .extend(implemented_traits, engines); // TODO: No difference made between imported and declared items for symbol_and_decl in symbols_and_decls { - dst_mod.current_items_mut().use_synonyms.insert( + dst_mod.current_items_mut().use_glob_synonyms.insert( // TODO: No difference made between imported and declared items symbol_and_decl.0, - (src.to_vec(), GlobImport::Yes, symbol_and_decl.1), + (src.to_vec(), symbol_and_decl.1), ); } @@ -141,15 +139,13 @@ impl Root { // no matter what, import it this way though. let dst_mod = self.module.lookup_submodule_mut(handler, engines, dst)?; let add_synonym = |name| { - if let Some((_, GlobImport::No, _)) = - dst_mod.current_items().use_synonyms.get(name) - { + if let Some((_, _)) = dst_mod.current_items().use_item_synonyms.get(name) { handler.emit_err(CompileError::ShadowsOtherSymbol { name: name.into() }); } - dst_mod.current_items_mut().use_synonyms.insert( + dst_mod.current_items_mut().use_item_synonyms.insert( // TODO: No difference made between imported and declared items name.clone(), - (src.to_vec(), GlobImport::No, decl), + (src.to_vec(), decl), ); }; match alias { @@ -227,19 +223,18 @@ impl Root { // import it this way. let dst_mod = self.module.lookup_submodule_mut(handler, engines, dst)?; let mut add_synonym = |name| { - if let Some((_, GlobImport::No, _)) = - dst_mod.current_items().use_synonyms.get(name) + if let Some((_, _)) = + dst_mod.current_items().use_item_synonyms.get(name) { handler.emit_err(CompileError::ShadowsOtherSymbol { name: name.into(), }); } - dst_mod.current_items_mut().use_synonyms.insert( + dst_mod.current_items_mut().use_item_synonyms.insert( // TODO: No difference made between imported and declared items name.clone(), ( src.to_vec(), - GlobImport::No, TyDecl::EnumVariantDecl(ty::EnumVariantDecl { enum_ref: enum_ref.clone(), variant_name: variant_name.clone(), @@ -326,12 +321,11 @@ impl Root { // import it this way. let dst_mod = self.module.lookup_submodule_mut(handler, engines, dst)?; - dst_mod.current_items_mut().use_synonyms.insert( + dst_mod.current_items_mut().use_glob_synonyms.insert( // TODO: No difference made between imported and declared items variant_name.clone(), ( src.to_vec(), - GlobImport::Yes, TyDecl::EnumVariantDecl(ty::EnumVariantDecl { enum_ref: enum_ref.clone(), variant_name: variant_name.clone(), @@ -378,24 +372,27 @@ impl Root { let src_mod = self.module.lookup_submodule(handler, engines, src)?; let implemented_traits = src_mod.current_items().implemented_traits.clone(); - let use_synonyms = src_mod.current_items().use_synonyms.clone(); - let mut symbols_and_decls = src_mod - .current_items() - .use_synonyms - .iter() - .map(|(symbol, (_, _, decl))| (symbol.clone(), decl.clone())) - .collect::>(); + let use_item_synonyms = src_mod.current_items().use_item_synonyms.clone(); + let use_glob_synonyms = src_mod.current_items().use_glob_synonyms.clone(); + + // collect all declared and reexported symbols from the source module + let mut all_symbols_and_decls = vec![]; + for (symbol, (_, decl)) in src_mod.current_items().use_glob_synonyms.iter() { + all_symbols_and_decls.push((symbol.clone(), decl.clone())); + } + for (symbol, (_, decl)) in src_mod.current_items().use_item_synonyms.iter() { + all_symbols_and_decls.push((symbol.clone(), decl.clone())); + } for (symbol, decl) in src_mod.current_items().symbols.iter() { if is_ancestor(src, dst) || decl.visibility(decl_engine).is_public() { - symbols_and_decls.push((symbol.clone(), decl.clone())); + all_symbols_and_decls.push((symbol.clone(), decl.clone())); } } let mut symbols_paths_and_decls = vec![]; - for (symbol, (mod_path, _, decl)) in use_synonyms { + let get_path = |mod_path: Vec| { let mut is_external = false; - let submodule = src_mod.submodule(engines, &[mod_path[0].clone()]); - if let Some(submodule) = submodule { + if let Some(submodule) = src_mod.submodule(engines, &[mod_path[0].clone()]) { is_external = submodule.is_external }; @@ -406,7 +403,14 @@ impl Root { path.extend(mod_path); } - symbols_paths_and_decls.push((symbol, path, decl)); + path + }; + + for (symbol, (mod_path, decl)) in use_item_synonyms { + symbols_paths_and_decls.push((symbol, get_path(mod_path), decl)); + } + for (symbol, (mod_path, decl)) in use_glob_synonyms { + symbols_paths_and_decls.push((symbol, get_path(mod_path), decl)); } let dst_mod = self.module.lookup_submodule_mut(handler, engines, dst)?; @@ -418,16 +422,16 @@ impl Root { let mut try_add = |symbol, path, decl: ty::TyDecl| { dst_mod .current_items_mut() - .use_synonyms - .insert(symbol, (path, GlobImport::Yes, decl)); // TODO: No difference made between imported and declared items + .use_glob_synonyms + .insert(symbol, (path, decl)); }; - for (symbol, decl) in symbols_and_decls { + for (symbol, decl) in all_symbols_and_decls { try_add(symbol, src.to_vec(), decl); } for (symbol, path, decl) in symbols_paths_and_decls { - try_add(symbol, path, decl); + try_add(symbol.clone(), path, decl.clone()); } Ok(())