From 76595ed3f3279a1b495bdee2de38f092a3cdb043 Mon Sep 17 00:00:00 2001 From: jjcnn <38888011+jjcnn@users.noreply.github.com> Date: Tue, 11 Jun 2024 18:29:34 +0200 Subject: [PATCH] Fix issue with name clash on auto implementation of AbiEncode and AbiDecode (#6044) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description Fixes #5500 . Also fixes the issue that caused one of @xunilrj 's tests to fail. The callpaths generated for struct declarations have so far been incorrect. The `shadowed_glob_imports` test case shows how this causes a compilation error when there is a name clash between a star imported struct and a locally declared struct. This causes a number of issues, in particular that the generated trait impls of `AbiEncode` and `AbiDecode` can refer to the wrong type or to a type that doesn't exist. The incorrect paths were the result of a combination of two bugs: 1. The `is_external` flag on the containing modules was often set incorrectly. In particular, `core` often had `is_external = false`, leading to the package name being added to the path of anything declared in `core`. 2. When typechecking a struct/enum declaration we would look up paths in the current environment before the name of the struct/enum was bound in the environment. If a type with the same name had already been imported this lookup would result in a path to the import source module rather than the path to current module (which is obviously where the struct/enum is located). This PR fixes both bugs: 1. When initializing the root namespace we now ensure that all modules that have already been added are marked with `is_external = true`. This happens in `Namespace::init_root()`. 2. When typechecking a type declaration we generate the callpath directly from the name of the type and the path of the current module. This happens in `CallPath::ident_to_fullpath()`. Step 1 unfortunately adds an extra cloning step in the `namespace` module, but I don't think it's avoidable at the moment. To avoid this extra cloning step we would need to have a better way of building the external module structure than we currently have. #5498 tracks this and other issues. The improved callpath generation should move us a step closer towards fixing #4700. ## 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). - [x] 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. --------- Co-authored-by: Igor Rončević Co-authored-by: João Matos --- forc-pkg/src/pkg.rs | 5 +-- sway-core/src/language/call_path.rs | 21 ++++++++- .../ast_node/declaration/abi.rs | 2 +- .../ast_node/declaration/enum.rs | 3 +- .../ast_node/declaration/impl_trait.rs | 17 +++---- .../ast_node/declaration/struct.rs | 3 +- .../ast_node/declaration/trait.rs | 12 +---- .../namespace/contract_helpers.rs | 1 - .../semantic_analysis/namespace/namespace.rs | 44 +++++++++++++++++-- .../src/semantic_analysis/namespace/root.rs | 1 - .../callpath_local_shadowing/Forc.lock | 13 ++++++ .../callpath_local_shadowing/Forc.toml | 9 ++++ .../json_abi_oracle.json | 26 +++++++++++ .../json_abi_oracle_new_encoding.json | 26 +++++++++++ .../callpath_local_shadowing/src/lib.sw | 5 +++ .../callpath_local_shadowing/src/main.sw | 17 +++++++ .../callpath_local_shadowing/test.toml | 3 ++ .../shadowing/shadowed_glob_imports/test.toml | 2 +- 18 files changed, 174 insertions(+), 36 deletions(-) create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/Forc.lock create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/Forc.toml create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/json_abi_oracle.json create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/json_abi_oracle_new_encoding.json create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/src/lib.sw create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/src/main.sw create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/test.toml diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index c21748f4ea4..e6c53cb13f8 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -1595,7 +1595,6 @@ pub fn dependency_namespace( }; root_module.write(engines, |root_module| { - root_module.is_external = true; root_module.name.clone_from(&name); root_module.visibility = Visibility::Public; }); @@ -1606,7 +1605,7 @@ pub fn dependency_namespace( let dep_node = edge.target(); let dep_name = kebab_to_snake_case(&edge.weight().name); let dep_edge = edge.weight(); - let dep_namespace = match dep_edge.kind { + let mut dep_namespace = match dep_edge.kind { DepKind::Library => lib_namespace_map .get(&dep_node) .cloned() @@ -1628,12 +1627,12 @@ pub fn dependency_namespace( contract_id_value, experimental, )?; - module.is_external = true; module.name = name; module.visibility = Visibility::Public; module } }; + dep_namespace.is_external = true; root_module.insert_submodule(dep_name, dep_namespace); let dep = &graph[dep_node]; if dep.name == CORE { diff --git a/sway-core/src/language/call_path.rs b/sway-core/src/language/call_path.rs index 52a8e41a04b..d0f993438b8 100644 --- a/sway-core/src/language/call_path.rs +++ b/sway-core/src/language/call_path.rs @@ -329,6 +329,23 @@ impl CallPath { .collect::>() } + /// Create a full [CallPath] from a given [Ident] and the [Namespace] in which the [Ident] is + /// declared. + /// + /// This function is intended to be used while typechecking the identifier declaration, i.e., + /// before the identifier is added to the environment. + pub fn ident_to_fullpath(suffix: Ident, namespace: &Namespace) -> CallPath { + let mut res: Self = suffix.clone().into(); + if let Some(ref pkg_name) = namespace.root_module().name { + res.prefixes.push(pkg_name.clone()) + }; + for mod_path in namespace.mod_path() { + res.prefixes.push(mod_path.clone()) + } + res.is_absolute = true; + res + } + /// Convert a given [CallPath] to a symbol to a full [CallPath] from the root of the project /// in which the symbol is declared. For example, given a path `pkga::SOME_CONST` where `pkga` /// is an _internal_ library of a package named `my_project`, the corresponding call path is @@ -350,7 +367,9 @@ impl CallPath { let mut is_absolute = false; if let Some(mod_path) = namespace.program_id(engines).read(engines, |m| { - if let Some((_, path, _)) = m + if m.current_items().symbols().contains_key(&self.suffix) { + None + } else if let Some((_, path, _)) = m .current_items() .use_item_synonyms .get(&self.suffix) diff --git a/sway-core/src/semantic_analysis/ast_node/declaration/abi.rs b/sway-core/src/semantic_analysis/ast_node/declaration/abi.rs index eee87c346ad..f6c1cc1e0d1 100644 --- a/sway-core/src/semantic_analysis/ast_node/declaration/abi.rs +++ b/sway-core/src/semantic_analysis/ast_node/declaration/abi.rs @@ -375,7 +375,7 @@ impl ty::TyAbiDecl { // from the same ABI later, during method application typechecking. let _ = ctx.insert_trait_implementation( &Handler::default(), - CallPath::from(self.name.clone()), + CallPath::ident_to_fullpath(self.name.clone(), ctx.namespace()), vec![], type_id, &all_items, diff --git a/sway-core/src/semantic_analysis/ast_node/declaration/enum.rs b/sway-core/src/semantic_analysis/ast_node/declaration/enum.rs index 574f14bcaf8..f07975ee2d7 100644 --- a/sway-core/src/semantic_analysis/ast_node/declaration/enum.rs +++ b/sway-core/src/semantic_analysis/ast_node/declaration/enum.rs @@ -42,8 +42,7 @@ impl ty::TyEnumDecl { ); } - let mut call_path: CallPath = name.into(); - call_path = call_path.to_fullpath(ctx.engines(), ctx.namespace()); + let call_path = CallPath::ident_to_fullpath(name, ctx.namespace()); // create the enum decl let decl = ty::TyEnumDecl { diff --git a/sway-core/src/semantic_analysis/ast_node/declaration/impl_trait.rs b/sway-core/src/semantic_analysis/ast_node/declaration/impl_trait.rs index 23072debd37..1f022a1b15b 100644 --- a/sway-core/src/semantic_analysis/ast_node/declaration/impl_trait.rs +++ b/sway-core/src/semantic_analysis/ast_node/declaration/impl_trait.rs @@ -305,17 +305,14 @@ impl TyImplTrait { let self_type_id = self_type_param.type_id; // create the trait name - let trait_name = CallPath { - prefixes: vec![], - suffix: match &&*type_engine.get(implementing_for.type_id) { - TypeInfo::Custom { - qualified_call_path: call_path, - .. - } => call_path.call_path.suffix.clone(), - _ => Ident::new_with_override("r#Self".into(), implementing_for.span()), - }, - is_absolute: false, + let suffix = match &&*type_engine.get(implementing_for.type_id) { + TypeInfo::Custom { + qualified_call_path: call_path, + .. + } => call_path.call_path.suffix.clone(), + _ => Ident::new_with_override("r#Self".into(), implementing_for.span()), }; + let trait_name = CallPath::ident_to_fullpath(suffix, ctx.namespace()); // Type check the type parameters. let new_impl_type_parameters = TypeParameter::type_check_type_params( diff --git a/sway-core/src/semantic_analysis/ast_node/declaration/struct.rs b/sway-core/src/semantic_analysis/ast_node/declaration/struct.rs index 361ac944ea9..a04efa0f274 100644 --- a/sway-core/src/semantic_analysis/ast_node/declaration/struct.rs +++ b/sway-core/src/semantic_analysis/ast_node/declaration/struct.rs @@ -37,8 +37,7 @@ impl ty::TyStructDecl { new_fields.push(ty::TyStructField::type_check(handler, ctx.by_ref(), field)?); } - let mut path: CallPath = name.into(); - path = path.to_fullpath(ctx.engines(), ctx.namespace()); + let path = CallPath::ident_to_fullpath(name, ctx.namespace()); // create the struct decl let decl = ty::TyStructDecl { diff --git a/sway-core/src/semantic_analysis/ast_node/declaration/trait.rs b/sway-core/src/semantic_analysis/ast_node/declaration/trait.rs index d5b3606f532..253b67ad5a4 100644 --- a/sway-core/src/semantic_analysis/ast_node/declaration/trait.rs +++ b/sway-core/src/semantic_analysis/ast_node/declaration/trait.rs @@ -112,11 +112,7 @@ impl TyTraitDecl { // to allow methods to use those functions ctx.insert_trait_implementation( handler, - CallPath { - prefixes: vec![], - suffix: name.clone(), - is_absolute: false, - }, + CallPath::ident_to_fullpath(name.clone(), ctx.namespace), new_type_parameters.iter().map(|x| x.into()).collect(), self_type, &dummy_interface_surface, @@ -180,11 +176,7 @@ impl TyTraitDecl { // to allow methods to use those functions ctx.insert_trait_implementation( handler, - CallPath { - prefixes: vec![], - suffix: name.clone(), - is_absolute: false, - }, + CallPath::ident_to_fullpath(name.clone(), ctx.namespace()), new_type_parameters.iter().map(|x| x.into()).collect(), self_type, &dummy_interface_surface, diff --git a/sway-core/src/semantic_analysis/namespace/contract_helpers.rs b/sway-core/src/semantic_analysis/namespace/contract_helpers.rs index 0e11482783a..0b1a5851e0c 100644 --- a/sway-core/src/semantic_analysis/namespace/contract_helpers.rs +++ b/sway-core/src/semantic_analysis/namespace/contract_helpers.rs @@ -99,7 +99,6 @@ fn default_with_contract_id_inner( let mut ns = Namespace::init_root(root); // This is pretty hacky but that's okay because of this code is being removed pretty soon ns.root.module.name = ns_name; - ns.root.module.is_external = true; ns.root.module.visibility = Visibility::Public; let type_check_ctx = TypeCheckContext::from_namespace(&mut ns, engines, experimental); let typed_node = TyAstNode::type_check(handler, type_check_ctx, &ast_node).unwrap(); diff --git a/sway-core/src/semantic_analysis/namespace/namespace.rs b/sway-core/src/semantic_analysis/namespace/namespace.rs index 97a12f3eb4e..cfe52d93d28 100644 --- a/sway-core/src/semantic_analysis/namespace/namespace.rs +++ b/sway-core/src/semantic_analysis/namespace/namespace.rs @@ -8,9 +8,11 @@ use super::{ root::{ResolvedDeclaration, Root}, submodule_namespace::SubmoduleNamespace, trait_map::ResolvedTraitImplItem, - ModulePath, ModulePathBuf, + ModuleName, ModulePath, ModulePathBuf, }; +use rustc_hash::FxHasher; +use std::hash::BuildHasherDefault; use sway_error::handler::{ErrorEmitted, Handler}; use sway_types::span::Span; @@ -55,11 +57,44 @@ impl Namespace { } /// Initialise the namespace at its root from the given initial namespace. + /// If the root module contains submodules these are now considered external. pub fn init_root(root: Root) -> Self { + assert!( + !root.module.is_external, + "The root module must not be external during compilation" + ); let mod_path = vec![]; + + // A copy of the root module is used to initialize every new submodule in the program. + // + // Every submodule that has been added before calling init_root is now considered + // external, which we have to enforce at this point. + fn clone_with_submodules_external(module: &Module) -> Module { + let mut new_submods = + im::HashMap::>::default(); + for (name, submod) in module.submodules.iter() { + let new_submod = clone_with_submodules_external(submod); + new_submods.insert(name.clone(), new_submod); + } + Module { + submodules: new_submods, + lexical_scopes: module.lexical_scopes.clone(), + current_lexical_scope_id: module.current_lexical_scope_id, + name: module.name.clone(), + visibility: module.visibility, + span: module.span.clone(), + is_external: true, + mod_path: module.mod_path.clone(), + } + } + + let mut init = clone_with_submodules_external(&root.module); + // The init module itself is not external + init.is_external = false; + Self { - init: root.module.clone(), - root, + init: init.clone(), + root: Root { module: init }, mod_path, } } @@ -268,6 +303,7 @@ impl Namespace { module_span: Span, ) -> SubmoduleNamespace { let init = self.init.clone(); + let is_external = self.module(engines).is_external; self.module_mut(engines) .submodules .entry(mod_name.to_string()) @@ -284,7 +320,7 @@ impl Namespace { new_module.name = Some(mod_name); new_module.span = Some(module_span); new_module.visibility = visibility; - new_module.is_external = false; + new_module.is_external = is_external; new_module.mod_path = submod_path; SubmoduleNamespace { namespace: self, diff --git a/sway-core/src/semantic_analysis/namespace/root.rs b/sway-core/src/semantic_analysis/namespace/root.rs index 3d30f541687..6008c7144c7 100644 --- a/sway-core/src/semantic_analysis/namespace/root.rs +++ b/sway-core/src/semantic_analysis/namespace/root.rs @@ -98,7 +98,6 @@ impl Root { self.check_module_privacy(handler, engines, src)?; let src_mod = self.module.lookup_submodule(handler, engines, src)?; - let implemented_traits = src_mod.current_items().implemented_traits.clone(); let mut symbols_and_decls = vec![]; for (symbol, decl) in src_mod.current_items().symbols.iter() { diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/Forc.lock b/test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/Forc.lock new file mode 100644 index 00000000000..30883f47562 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/Forc.lock @@ -0,0 +1,13 @@ +[[package]] +name = "callpath_local_shadowing" +source = "member" +dependencies = ["std"] + +[[package]] +name = "core" +source = "path+from-root-779BB769D43AD4B7" + +[[package]] +name = "std" +source = "path+from-root-779BB769D43AD4B7" +dependencies = ["core"] diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/Forc.toml b/test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/Forc.toml new file mode 100644 index 00000000000..22da11cfbac --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/Forc.toml @@ -0,0 +1,9 @@ +[project] +authors = ["Fuel Labs "] +license = "Apache-2.0" +name = "callpath_local_shadowing" +entry = "main.sw" +implicit-std = false + +[dependencies] +std = { path = "../../../../../../../sway-lib-std" } diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/json_abi_oracle.json b/test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/json_abi_oracle.json new file mode 100644 index 00000000000..e60dda965d4 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/json_abi_oracle.json @@ -0,0 +1,26 @@ +{ + "configurables": [], + "encoding": "1", + "functions": [ + { + "attributes": null, + "inputs": [], + "name": "main", + "output": { + "name": "", + "type": 0, + "typeArguments": null + } + } + ], + "loggedTypes": [], + "messagesTypes": [], + "types": [ + { + "components": [], + "type": "()", + "typeId": 0, + "typeParameters": null + } + ] +} \ No newline at end of file diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/json_abi_oracle_new_encoding.json b/test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/json_abi_oracle_new_encoding.json new file mode 100644 index 00000000000..e60dda965d4 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/json_abi_oracle_new_encoding.json @@ -0,0 +1,26 @@ +{ + "configurables": [], + "encoding": "1", + "functions": [ + { + "attributes": null, + "inputs": [], + "name": "main", + "output": { + "name": "", + "type": 0, + "typeArguments": null + } + } + ], + "loggedTypes": [], + "messagesTypes": [], + "types": [ + { + "components": [], + "type": "()", + "typeId": 0, + "typeParameters": null + } + ] +} \ No newline at end of file diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/src/lib.sw b/test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/src/lib.sw new file mode 100644 index 00000000000..28a98821301 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/src/lib.sw @@ -0,0 +1,5 @@ +library; + +pub struct TestStruct { + pub a: u64, +} diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/src/main.sw new file mode 100644 index 00000000000..f74a1a7a446 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/src/main.sw @@ -0,0 +1,17 @@ +script; + +mod lib; + +use lib::*; + +struct TestStruct { + pub x: u64, + pub y: u64, +} + +fn main() { + let ts = TestStruct { x: 0, y: 0 }; + poke(ts.x); +} + +fn poke(_x: T) { } diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/test.toml b/test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/test.toml new file mode 100644 index 00000000000..23f85066264 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/test.toml @@ -0,0 +1,3 @@ +category = "compile" +validate_abi = true +expected_warnings = 2 diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/language/shadowing/shadowed_glob_imports/test.toml b/test/src/e2e_vm_tests/test_programs/should_pass/language/shadowing/shadowed_glob_imports/test.toml index edf3a3eae21..75b5f2ef5a2 100644 --- a/test/src/e2e_vm_tests/test_programs/should_pass/language/shadowing/shadowed_glob_imports/test.toml +++ b/test/src/e2e_vm_tests/test_programs/should_pass/language/shadowing/shadowed_glob_imports/test.toml @@ -1,2 +1,2 @@ -category = "disabled" +category = "compile" expected_warnings = 27