From 722c95c00c120e976982dfa03c82bd7a4d86dc5d Mon Sep 17 00:00:00 2001 From: jjcnn <38888011+jjcnn@users.noreply.github.com> Date: Thu, 2 May 2024 11:43:20 +0200 Subject: [PATCH] Eliminate alias map from lexical scope (#5931) ## Description Fixes #5912, #5713 . Item imports with aliases `use x::y as z` have so far been represented using two maps: - A synonyms map mapping `y` to the type declaration of `x::y`. - An alias map mapping `z` to `y`. This is confusing, since `y` is not actually bound by this import, and unsurprisingly has lead to a bug because of a name capture (see #5713). This PR eliminates the alias mapping, and changes the synonyms map to map `z` to the type declaration of `x::y`. ## 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 | 29 +++-- .../namespace/lexical_scope.rs | 21 ++-- .../src/semantic_analysis/namespace/root.rs | 119 ++++++++---------- .../language/aliased_imports/src/main.sw | 21 +++- .../language/aliased_imports/src/wiz.sw | 8 ++ .../language/aliased_imports/test.toml | 4 +- 6 files changed, 111 insertions(+), 91 deletions(-) create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/language/aliased_imports/src/wiz.sw diff --git a/sway-core/src/language/call_path.rs b/sway-core/src/language/call_path.rs index 80c16005b27..8eff16d95b0 100644 --- a/sway-core/src/language/call_path.rs +++ b/sway-core/src/language/call_path.rs @@ -311,27 +311,30 @@ impl CallPath { let mut is_external = false; let mut is_absolute = false; - if let Some(use_synonym) = namespace.module_id(engines).read(engines, |m| { - if m.current_items() + if let Some(mod_path) = namespace.module_id(engines).read(engines, |m| { + if let Some((_, path, _)) = m + .current_items() .use_item_synonyms - .contains_key(&self.suffix) + .get(&self.suffix) + .cloned() { - m.current_items() - .use_item_synonyms - .get(&self.suffix) - .cloned() + Some(path) + } else if let Some((path, _)) = m + .current_items() + .use_glob_synonyms + .get(&self.suffix) + .cloned() + { + Some(path) } else { - m.current_items() - .use_glob_synonyms - .get(&self.suffix) - .cloned() + None } }) { - synonym_prefixes = use_synonym.0.clone(); + synonym_prefixes = mod_path.clone(); is_absolute = true; let submodule = namespace .module(engines) - .submodule(engines, &[use_synonym.0[0].clone()]); + .submodule(engines, &[mod_path[0].clone()]); if let Some(submodule) = submodule { is_external = submodule.read(engines, |m| m.is_external); } diff --git a/sway-core/src/semantic_analysis/namespace/lexical_scope.rs b/sway-core/src/semantic_analysis/namespace/lexical_scope.rs index e673c3b9284..acef75a9404 100644 --- a/sway-core/src/semantic_analysis/namespace/lexical_scope.rs +++ b/sway-core/src/semantic_analysis/namespace/lexical_scope.rs @@ -37,10 +37,9 @@ impl ResolvedFunctionDecl { pub(super) type ParsedSymbolMap = im::OrdMap; pub(super) type SymbolMap = im::OrdMap; -// The `Vec` path is absolute. -pub(super) type GlobSynonyms = im::HashMap, ty::TyDecl)>; -pub(super) type ItemSynonyms = im::HashMap, ty::TyDecl)>; -pub(super) type UseAliases = im::HashMap; +type SourceIdent = Ident; +pub(super) type GlobSynonyms = im::HashMap; +pub(super) type ItemSynonyms = im::HashMap, ModulePathBuf, ty::TyDecl)>; /// Represents a lexical scope integer-based identifier, which can be used to reference /// specific a lexical scope. @@ -77,13 +76,11 @@ pub struct Items { /// /// use_glob_synonyms contains symbols imported using star imports (`use foo::*`.). /// use_item_synonyms contains symbols imported using item imports (`use foo::bar`). + /// + /// For aliased item imports `use ::foo::bar::Baz as Wiz` the map key is `Wiz`. `Baz` is stored + /// as the optional source identifier for error reporting purposes. 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 - /// alias for `bar`. - pub(crate) use_aliases: UseAliases, /// If there is a storage declaration (which are only valid in contracts), store it here. pub(crate) declared_storage: Option, } @@ -287,12 +284,14 @@ impl Items { append_shadowing_error(ident, decl, false, false, &item, const_shadowing_mode); } - if let Some((ident, (_, decl))) = self.use_item_synonyms.get_key_value(&name) { + if let Some((ident, (imported_ident, _, decl))) = + self.use_item_synonyms.get_key_value(&name) + { append_shadowing_error( ident, decl, true, - self.use_aliases.get(&name.to_string()).is_some(), + imported_ident.is_some(), &item, const_shadowing_mode, ); diff --git a/sway-core/src/semantic_analysis/namespace/root.rs b/sway-core/src/semantic_analysis/namespace/root.rs index 7f5744a44df..917c46777d2 100644 --- a/sway-core/src/semantic_analysis/namespace/root.rs +++ b/sway-core/src/semantic_analysis/namespace/root.rs @@ -80,13 +80,12 @@ impl Root { dst_mod .current_items_mut() .implemented_traits - .extend(implemented_traits, engines); // TODO: No difference made between imported and declared items + .extend(implemented_traits, engines); for symbol_and_decl in symbols_and_decls { - 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(), symbol_and_decl.1), - ); + dst_mod + .current_items_mut() + .use_glob_synonyms + .insert(symbol_and_decl.0, (src.to_vec(), symbol_and_decl.1)); } Ok(()) @@ -161,25 +160,26 @@ 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((_, _)) = dst_mod.current_items().use_item_synonyms.get(name) { + let check_name_clash = |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_item_synonyms.insert( - // TODO: No difference made between imported and declared items - name.clone(), - (src.to_vec(), decl), - ); }; match alias { Some(alias) => { - add_synonym(&alias); + check_name_clash(&alias); + dst_mod + .current_items_mut() + .use_item_synonyms + .insert(alias.clone(), (Some(item.clone()), src.to_vec(), decl)) + } + None => { + check_name_clash(item); dst_mod .current_items_mut() - .use_aliases - .insert(alias.as_str().to_string(), item.clone()); // TODO: No difference made between imported and declared items + .use_item_synonyms + .insert(item.clone(), (None, src.to_vec(), decl)) } - None => add_synonym(item), }; } None => { @@ -194,7 +194,7 @@ impl Root { dst_mod .current_items_mut() .implemented_traits - .extend(impls_to_insert, engines); // TODO: No difference made between imported and declared items + .extend(impls_to_insert, engines); Ok(()) } @@ -245,37 +245,44 @@ 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((_, _)) = - dst_mod.current_items().use_item_synonyms.get(name) - { + let check_name_clash = |name| { + if dst_mod.current_items().use_item_synonyms.contains_key(name) { handler.emit_err(CompileError::ShadowsOtherSymbol { name: name.into(), }); } - dst_mod.current_items_mut().use_item_synonyms.insert( - // TODO: No difference made between imported and declared items - name.clone(), - ( - src.to_vec(), - TyDecl::EnumVariantDecl(ty::EnumVariantDecl { - enum_ref: enum_ref.clone(), - variant_name: variant_name.clone(), - variant_decl_span: variant_decl.span.clone(), - }), - ), - ); }; match alias { Some(alias) => { - add_synonym(&alias); - dst_mod - .current_items_mut() - .use_aliases - .insert(alias.as_str().to_string(), variant_name.clone()); - // TODO: No difference made between imported and declared items + check_name_clash(&alias); + dst_mod.current_items_mut().use_item_synonyms.insert( + alias.clone(), + ( + Some(variant_name.clone()), + src.to_vec(), + TyDecl::EnumVariantDecl(ty::EnumVariantDecl { + enum_ref: enum_ref.clone(), + variant_name: variant_name.clone(), + variant_decl_span: variant_decl.span.clone(), + }), + ), + ); + } + None => { + check_name_clash(variant_name); + dst_mod.current_items_mut().use_item_synonyms.insert( + variant_name.clone(), + ( + None, + src.to_vec(), + TyDecl::EnumVariantDecl(ty::EnumVariantDecl { + enum_ref: enum_ref.clone(), + variant_name: variant_name.clone(), + variant_decl_span: variant_decl.span.clone(), + }), + ), + ); } - None => add_synonym(variant_name), }; } else { return Err(handler.emit_err(CompileError::SymbolNotFound { @@ -345,7 +352,6 @@ impl Root { // import it this way. let dst_mod = self.module.lookup_submodule_mut(handler, engines, dst)?; dst_mod.current_items_mut().use_glob_synonyms.insert( - // TODO: No difference made between imported and declared items variant_name.clone(), ( src.to_vec(), @@ -403,7 +409,7 @@ impl Root { 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() { + 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() { @@ -429,7 +435,7 @@ impl Root { path }; - for (symbol, (mod_path, decl)) in use_item_synonyms { + 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 { @@ -440,7 +446,7 @@ impl Root { dst_mod .current_items_mut() .implemented_traits - .extend(implemented_traits, engines); // TODO: No difference made between imported and declared items + .extend(implemented_traits, engines); let mut try_add = |symbol, path, decl: ty::TyDecl| { dst_mod @@ -635,9 +641,7 @@ impl Root { current_mod_path.push(ident.clone()); } None => { - decl_opt = Some( - self.resolve_symbol_helper(handler, engines, ident, module, self_type)?, - ); + decl_opt = Some(self.resolve_symbol_helper(handler, ident, module)?); } } } @@ -651,8 +655,7 @@ impl Root { self.module .lookup_submodule(handler, engines, mod_path) .and_then(|module| { - let decl = - self.resolve_symbol_helper(handler, engines, symbol, module, self_type)?; + let decl = self.resolve_symbol_helper(handler, symbol, module)?; Ok((decl, mod_path.to_vec())) }) } @@ -802,29 +805,17 @@ impl Root { fn resolve_symbol_helper( &self, handler: &Handler, - engines: &Engines, symbol: &Ident, module: &Module, - self_type: Option, ) -> Result { - let true_symbol = module - .current_items() - .use_aliases - .get(symbol.as_str()) - .unwrap_or(symbol); // 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) { + if let Some(decl) = module.current_items().symbols.get(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) - { + if let Some((_, _, decl)) = 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())); diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/language/aliased_imports/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_pass/language/aliased_imports/src/main.sw index 54286017a31..8e102a37165 100644 --- a/test/src/e2e_vm_tests/test_programs/should_pass/language/aliased_imports/src/main.sw +++ b/test/src/e2e_vm_tests/test_programs/should_pass/language/aliased_imports/src/main.sw @@ -2,12 +2,31 @@ script; // This tests importing other files. mod foo; +mod wiz; use foo::Foo as MyFoo; +use wiz::Wiz as WizWiz; + +// This is fine - the imported Wiz name is aliased, so no name clash +struct Wiz { + local_wiz: bool +} + fn main() -> u64 { let foo = MyFoo { foo: 42, }; - foo.foo + let wiz = WizWiz { + wiz: 128 + }; + let local_wiz = Wiz { // This should resolve to the locally defined Wiz + local_wiz: true + }; + if local_wiz.local_wiz { + foo.foo + wiz.wiz + } + else { + 0 + } } diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/language/aliased_imports/src/wiz.sw b/test/src/e2e_vm_tests/test_programs/should_pass/language/aliased_imports/src/wiz.sw new file mode 100644 index 00000000000..bf1a83d00c9 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/language/aliased_imports/src/wiz.sw @@ -0,0 +1,8 @@ +library; + +// Dead code analysis disabled due to a bug. +// See https://github.com/FuelLabs/sway/issues/5902#issuecomment-2079212717 +#[allow(dead_code)] +pub struct Wiz { + pub wiz: u64, +} diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/language/aliased_imports/test.toml b/test/src/e2e_vm_tests/test_programs/should_pass/language/aliased_imports/test.toml index 057e34e97ef..76c143eda75 100644 --- a/test/src/e2e_vm_tests/test_programs/should_pass/language/aliased_imports/test.toml +++ b/test/src/e2e_vm_tests/test_programs/should_pass/language/aliased_imports/test.toml @@ -1,4 +1,4 @@ category = "run" -expected_result = { action = "return", value = 42 } -expected_result_new_encoding = { action = "return_data", value = "000000000000002A" } +expected_result = { action = "return", value = 170 } +expected_result_new_encoding = { action = "return_data", value = "00000000000000AA" } validate_abi = true