Fix issue with name clash on auto implementation of AbiEncode and Abi…

…Decode (#6044)

## 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ć <ironcev@hotmail.com>
Co-authored-by: João Matos <joao@tritao.eu>

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 {

sway-core/src/language/call_path.rs

@@ -329,6 +329,23 @@ impl CallPath {
             .collect::<Vec<_>>()
     }
 
+    /// 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)

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,

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 {

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(

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 {

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,

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();

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::<ModuleName, Module, BuildHasherDefault<FxHasher>>::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,

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() {

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"]

test/src/e2e_vm_tests/test_programs/should_pass/language/callpath_local_shadowing/Forc.toml

@@ -0,0 +1,9 @@
+[project]
+authors = ["Fuel Labs <contact@fuel.sh>"]
+license = "Apache-2.0"
+name = "callpath_local_shadowing"
+entry = "main.sw"
+implicit-std = false
+
+[dependencies]
+std = { path = "../../../../../../../sway-lib-std" }

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
+    }
+  ]
+}

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
+    }
+  ]
+}

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,
+}

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<T>(_x: T) { }

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

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