Name clash in star imports should result in error when the name is us…

…ed (#5963)

## Description

Fixes #5940. 

This PR deals with the situation where the same name is imported by
multiple star imports, e.g.,
```
// a.sw
struct X = ...

// b.sw
struct X = ...

// main.sw
use a::*;
use b::*;
```
So far we have resolved this name clash by letting the latter import
shadow the former, which is incorrect.

The correct behavior is to allow the import without error, but to report
an error if the unqualified name (i.e., `X`) is used (qualified names
`a::X` and `b::X` are legal)`. This PR fixes this problem.

Note that if `X` is imported multiple times, but each time is bound to
the same entity (e.g., because it is imported both through
`core:;prelude` and `core::codec`), then this should not cause an error.

Note also that there is a problem with the implicit imports from the
core and std preludes, which in some cases causes the implementation to
think that a name is bound to multiple different entities, even though
the entities are actually the same. As a workaround anytime a name is
imported from either `std` or `core` when it is already imported from
`std` or `core`, the new binding replaces the old one instead of being
added as a new binding.

Unfortunately this workaround means that it is not currently possible to
redefine and use a name that exists in `std` or `core`, e.g., to create
a specialized version of `assert_eq` as is done in one of our tests. To
use such a redefinition one must use a qualified name for the entity,
which seems like an acceptable workaround. (Note that this only applies
if the redefined entity is imported using a star import - if imported
using an item import (`use a::X;`), then there is no issue).

Once #3487 is implemented this workaround should no longer be necessary.


## 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.

---------

Co-authored-by: Joshua Batty <[email protected]>
Co-authored-by: IGI-111 <[email protected]>

sway-core/src/language/call_path.rs

@@ -329,13 +329,17 @@ impl CallPath {
  .cloned()
  {
  Some(path)
- } else if let Some((path, _)) = m
+ } else if let Some(paths_and_decls) = m
  .current_items()
  .use_glob_synonyms
  .get(&self.suffix)
  .cloned()
  {
- Some(path)
+ if paths_and_decls.len() == 1 {
+ Some(paths_and_decls[0].0.clone())
+ } else {
+ None
+ }
  } else {
  None
  }

sway-core/src/language/ty/declaration/declaration.rs

@@ -127,6 +127,23 @@ impl PartialEqWithEngines for TyDecl {
  TyDecl::EnumDecl(EnumDecl { decl_id: lid, .. }),
  TyDecl::EnumDecl(EnumDecl { decl_id: rid, .. }),
  ) => decl_engine.get(lid).eq(&decl_engine.get(rid), ctx),
+ (
+ TyDecl::EnumVariantDecl(EnumVariantDecl {
+ enum_ref: l_enum,
+ variant_name: ln,
+ ..
+ }),
+ TyDecl::EnumVariantDecl(EnumVariantDecl {
+ enum_ref: r_enum,
+ variant_name: rn,
+ ..
+ }),
+ ) => {
+ ln == rn
+ && decl_engine
+ .get_enum(l_enum)
+ .eq(&decl_engine.get_enum(r_enum), ctx)
+ }
  (
  TyDecl::ImplTrait(ImplTrait { decl_id: lid, .. }),
  TyDecl::ImplTrait(ImplTrait { decl_id: rid, .. }),

sway-core/src/semantic_analysis/namespace/lexical_scope.rs

@@ -1,6 +1,6 @@
 use crate::{
  decl_engine::{parsed_id::ParsedDeclId, *},
- engine_threading::Engines,
+ engine_threading::{Engines, PartialEqWithEngines, PartialEqWithEnginesContext},
  language::{
  parsed::{Declaration, FunctionDeclaration},
  ty::{self, StructAccessInfo, TyDecl, TyStorageDecl},
@@ -38,7 +38,7 @@ impl ResolvedFunctionDecl {
 pub(super) type ParsedSymbolMap = im::OrdMap<Ident, Declaration>;
 pub(super) type SymbolMap = im::OrdMap<Ident, ty::TyDecl>;
 type SourceIdent = Ident;
-pub(super) type GlobSynonyms = im::HashMap<Ident, (ModulePathBuf, ty::TyDecl)>;
+pub(super) type GlobSynonyms = im::HashMap<Ident, Vec<(ModulePathBuf, ty::TyDecl)>>;
 pub(super) type ItemSynonyms = im::HashMap<Ident, (Option<SourceIdent>, ModulePathBuf, ty::TyDecl)>;
 
 /// Represents a lexical scope integer-based identifier, which can be used to reference
@@ -75,6 +75,12 @@ pub struct Items {
  /// path `foo::bar::Baz`.
  ///
  /// use_glob_synonyms contains symbols imported using star imports (`use foo::*`.).
+ ///
+ /// When star importing from multiple modules the same name may be imported more than once. This
+ /// is not an error, but it is an error to use the name without a module path. To represent
+ /// this, use_glob_synonyms maps identifiers to a vector of (module path, type declaration)
+ /// tuples.
+ ///
  /// 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
@@ -304,6 +310,59 @@ impl Items {
  Ok(())
  }
 
+ // Add a new binding into use_glob_synonyms. The symbol may already be bound by an earlier
+ // insertion, in which case the new binding is added as well so that multiple bindings exist.
+ //
+ // There are a few edge cases were a new binding will replace an old binding. These edge cases
+ // are a consequence of the prelude reexports not being implemented properly. See comments in
+ // the code for details.
+ pub(crate) fn insert_glob_use_symbol(
+ &mut self,
+ engines: &Engines,
+ symbol: Ident,
+ src_path: ModulePathBuf,
+ decl: &ty::TyDecl,
+ ) {
+ if let Some(cur_decls) = self.use_glob_synonyms.get_mut(&symbol) {
+ // Name already bound. Check if the decl is already imported
+ let ctx = PartialEqWithEnginesContext::new(engines);
+ match cur_decls.iter().position(|(cur_path, cur_decl)| {
+ cur_decl.eq(decl, &ctx)
+ // For some reason the equality check is not sufficient. In some cases items that
+ // are actually identical fail the eq check, so we have to add heuristics for these
+ // cases.
+ //
+ // These edge occur because core and std preludes are not reexported correctly. Once
+ // reexports are implemented we can handle the preludes correctly, and then these
+ // edge cases should go away.
+ // See https://github.com/FuelLabs/sway/issues/3113
+ //
+ // As a heuristic we replace any bindings from std and core if the new binding is
+ // also from std or core. This does not work if the user has declared an item with
+ // the same name as an item in one of the preludes, but this is an edge case that we
+ // will have to live with for now.
+ || ((cur_path[0].as_str() == "core" || cur_path[0].as_str() == "std")
+ && (src_path[0].as_str() == "core" || src_path[0].as_str() == "std"))
+ }) {
+ Some(index) => {
+ // The name is already bound to this decl, but
+ // we need to replace the binding to make the paths work out.
+ // This appears to be an issue with the core prelude, and will probably no
+ // longer be necessary once reexports are implemented:
+ // https://github.com/FuelLabs/sway/issues/3113
+ cur_decls[index] = (src_path.to_vec(), decl.clone());
+ }
+ None => {
+ // New decl for this name. Add it to the end
+ cur_decls.push((src_path.to_vec(), decl.clone()));
+ }
+ }
+ } else {
+ let new_vec = vec![(src_path.to_vec(), decl.clone())];
+ self.use_glob_synonyms.insert(symbol, new_vec);
+ }
+ }
+
  pub(crate) fn check_symbol(&self, name: &Ident) -> Result<ResolvedDeclaration, CompileError> {
  self.symbols
  .get(name)

sway-core/src/semantic_analysis/namespace/namespace.rs

@@ -278,13 +278,14 @@ impl Namespace {
  .cloned()
  .chain(Some(mod_name.clone()))
  .collect();
- let parent_mod_path = std::mem::replace(&mut self.mod_path, submod_path);
+ let parent_mod_path = std::mem::replace(&mut self.mod_path, submod_path.clone());
  // self.module() now refers to a different module, so refetch
  let new_module = self.module_mut(engines);
  new_module.name = Some(mod_name);
  new_module.span = Some(module_span);
  new_module.visibility = visibility;
  new_module.is_external = false;
+ new_module.mod_path = submod_path;
  SubmoduleNamespace {
  namespace: self,
  parent_mod_path,

sway-core/src/semantic_analysis/namespace/root.rs

@@ -81,12 +81,15 @@ impl Root {
  .current_items_mut()
  .implemented_traits
  .extend(implemented_traits, engines);
- for symbol_and_decl in symbols_and_decls {
- dst_mod
- .current_items_mut()
- .use_glob_synonyms
- .insert(symbol_and_decl.0, (src.to_vec(), symbol_and_decl.1));
- }
+
+ symbols_and_decls.iter().for_each(|(symbol, decl)| {
+ dst_mod.current_items_mut().insert_glob_use_symbol(
+ engines,
+ symbol.clone(),
+ src.to_vec(),
+ decl,
+ )
+ });
 
  Ok(())
  }
@@ -338,20 +341,22 @@ impl Root {
 
  for variant_decl in enum_decl.variants.iter() {
  let variant_name = &variant_decl.name;
+ let decl = TyDecl::EnumVariantDecl(ty::EnumVariantDecl {
+ enum_ref: enum_ref.clone(),
+ variant_name: variant_name.clone(),
+ variant_decl_span: variant_decl.span.clone(),
+ });
 
  // import it this way.
- let dst_mod = self.module.lookup_submodule_mut(handler, engines, dst)?;
- dst_mod.current_items_mut().use_glob_synonyms.insert(
- variant_name.clone(),
- (
+ self.module
+ .lookup_submodule_mut(handler, engines, dst)?
+ .current_items_mut()
+ .insert_glob_use_symbol(
+ engines,
+ 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(),
- }),
- ),
- );
+ &decl,
+ );
  }
  } else {
  return Err(handler.emit_err(CompileError::Internal(
@@ -396,8 +401,10 @@ impl Root {
 
  // 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, decls) in src_mod.current_items().use_glob_synonyms.iter() {
+ decls
+ .iter()
+ .for_each(|(_, decl)| 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()));
@@ -428,8 +435,14 @@ impl Root {
  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));
+ for (symbol, decls) in use_glob_synonyms {
+ decls.iter().for_each(|(mod_path, decl)| {
+ symbols_paths_and_decls.push((
+ symbol.clone(),
+ get_path(mod_path.clone()),
+ decl.clone(),
+ ))
+ });
  }
 
  let dst_mod = self.module.lookup_submodule_mut(handler, engines, dst)?;
@@ -441,16 +454,15 @@ impl Root {
  let mut try_add = |symbol, path, decl: ty::TyDecl| {
  dst_mod
  .current_items_mut()
- .use_glob_synonyms
- .insert(symbol, (path, decl));
+ .insert_glob_use_symbol(engines, symbol, path, &decl);
  };
 
  for (symbol, decl) in all_symbols_and_decls {
- try_add(symbol, src.to_vec(), decl);
+ try_add(symbol.clone(), src.to_vec(), decl);
  }
 
  for (symbol, path, decl) in symbols_paths_and_decls {
- try_add(symbol.clone(), path, decl.clone());
+ try_add(symbol.clone(), path, decl);
  }
 
  Ok(())
@@ -813,8 +825,35 @@ impl Root {
  return Ok(ResolvedDeclaration::Typed(decl.clone()));
  }
  // Check glob imports
- if let Some((_, decl)) = module.current_items().use_glob_synonyms.get(symbol) {
- return Ok(ResolvedDeclaration::Typed(decl.clone()));
+ if let Some(decls) = module.current_items().use_glob_synonyms.get(symbol) {
+ if decls.len() == 1 {
+ return Ok(ResolvedDeclaration::Typed(decls[0].1.clone()));
+ } else if decls.is_empty() {
+ return Err(handler.emit_err(CompileError::Internal(
+ "The name {symbol} was bound in a star import, but no corresponding module paths were found",
+ symbol.span(),
+ )));
+ } else {
+ // Symbol not found
+ return Err(handler.emit_err(CompileError::SymbolWithMultipleBindings {
+ name: symbol.clone(),
+ paths: decls
+ .iter()
+ .map(|(path, decl)| {
+ let mut path_strs = path.iter().map(|x| x.as_str()).collect::<Vec<_>>();
+ // Add the enum name to the path if the decl is an enum variant.
+ if let TyDecl::EnumVariantDecl(ty::EnumVariantDecl {
+ enum_ref, ..
+ }) = decl
+ {
+ path_strs.push(enum_ref.name().as_str())
+ };
+ path_strs.join("::")
+ })
+ .collect(),
+ span: symbol.span(),
+ }));
+ }
  }
  // Symbol not found
  Err(handler.emit_err(CompileError::SymbolNotFound {

sway-error/src/error.rs

@@ -426,6 +426,12 @@ pub enum CompileError {
  DeclIsNotATypeAlias { actually: String, span: Span },
  #[error("Could not find symbol \"{name}\" in this scope.")]
  SymbolNotFound { name: Ident, span: Span },
+ #[error("Found multiple bindings for \"{name}\" in this scope.")]
+ SymbolWithMultipleBindings {
+ name: Ident,
+ paths: Vec<String>,
+ span: Span,
+ },
  #[error("Symbol \"{name}\" is private.")]
  ImportPrivateSymbol { name: Ident, span: Span },
  #[error("Module \"{name}\" is private.")]
@@ -1019,6 +1025,7 @@ impl Spanned for CompileError {
  NotIndexable { span, .. } => span.clone(),
  FieldAccessOnNonStruct { span, .. } => span.clone(),
  SymbolNotFound { span, .. } => span.clone(),
+ SymbolWithMultipleBindings { span, .. } => span.clone(),
  ImportPrivateSymbol { span, .. } => span.clone(),
  ImportPrivateModule { span, .. } => span.clone(),
  NoElseBranch { span, .. } => span.clone(),
@@ -1907,6 +1914,16 @@ impl ToDiagnostic for CompileError {
  ]
  }
  },
+ SymbolWithMultipleBindings { name, paths, span } => Diagnostic {
+ reason: Some(Reason::new(code(1), "Multiple bindings for symbol in this scope".to_string())),
+ issue: Issue::error(
+ source_engine,
+ span.clone(),
+ format!("The following paths are all valid bindings for symbol \"{}\": {}", name, paths.iter().map(|path| format!("{path}::{name}")).collect::<Vec<_>>().join(", ")),
+ ),
+ hints: paths.iter().map(|path| Hint::info(source_engine, Span::dummy(), format!("{path}::{}", name.as_str()))).collect(),
+ help: vec![format!("Consider using a fully qualified name, e.g., {}::{}", paths[0], name.as_str())],
+ },
  StorageFieldDoesNotExist { field_name, available_fields, storage_decl_span } => Diagnostic {
  reason: Some(Reason::new(code(1), "Storage field does not exist".to_string())),
  issue: Issue::error(

test/src/e2e_vm_tests/test_programs/should_fail/language/import_star_name_clash/Forc.lock

@@ -0,0 +1,13 @@
+[[package]]
+name = "core"
+source = "path+from-root-95C7DDCD726BD0EA"
+
+[[package]]
+name = "import_star_name_clash"
+source = "member"
+dependencies = ["std"]
+
+[[package]]
+name = "std"
+source = "path+from-root-95C7DDCD726BD0EA"
+dependencies = ["core"]

test/src/e2e_vm_tests/test_programs/should_fail/language/import_star_name_clash/Forc.toml

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

test/src/e2e_vm_tests/test_programs/should_fail/language/import_star_name_clash/json_abi_oracle.json

@@ -0,0 +1,25 @@
+{
+ "configurables": [],
+ "functions": [
+ {
+ "attributes": null,
+ "inputs": [],
+ "name": "main",
+ "output": {
+ "name": "",
+ "type": 0,
+ "typeArguments": null
+ }
+ }
+ ],
+ "loggedTypes": [],
+ "messagesTypes": [],
+ "types": [
+ {
+ "components": [],
+ "type": "()",
+ "typeId": 0,
+ "typeParameters": null
+ }
+ ]
+}