Skip to content

Commit

Permalink
Eliminate alias map from lexical scope (#5931)
Browse files Browse the repository at this point in the history 
## 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.
  • Loading branch information
@jjcnn
jjcnn authored May 2, 2024
1 parent 2c20592 commit 722c95c
Show file tree
Hide file tree
Showing 6 changed files with 111 additions and 91 deletions.
29 changes: 16 additions & 13 deletions sway-core/src/language/call_path.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
21 changes: 10 additions & 11 deletions sway-core/src/semantic_analysis/namespace/lexical_scope.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,9 @@ impl ResolvedFunctionDecl {

pub(super) type ParsedSymbolMap = im::OrdMap<Ident, Declaration>;
pub(super) type SymbolMap = im::OrdMap<Ident, ty::TyDecl>;
// The `Vec<Ident>` path is absolute.
pub(super) type GlobSynonyms = im::HashMap<Ident, (Vec<Ident>, ty::TyDecl)>;
pub(super) type ItemSynonyms = im::HashMap<Ident, (Vec<Ident>, ty::TyDecl)>;
pub(super) type UseAliases = im::HashMap<String, Ident>;
type SourceIdent = Ident;
pub(super) type GlobSynonyms = im::HashMap<Ident, (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
/// specific a lexical scope.
Expand Down Expand Up @@ -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<DeclRefStorage>,
}
Expand Down Expand Up @@ -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,
);
Expand Down
119 changes: 55 additions & 64 deletions sway-core/src/semantic_analysis/namespace/root.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(())
Expand Down Expand Up @@ -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 => {
Expand All @@ -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(())
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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() {
Expand All @@ -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 {
Expand All @@ -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
Expand Down Expand Up @@ -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)?);
}
}
}
Expand All @@ -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()))
})
}
Expand Down Expand Up @@ -802,29 +805,17 @@ impl Root {
fn resolve_symbol_helper(
&self,
handler: &Handler,
engines: &Engines,
symbol: &Ident,
module: &Module,
self_type: Option<TypeId>,
) -> Result<ResolvedDeclaration, ErrorEmitted> {
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()));
Expand Down
21 changes: 20 additions & 1 deletion test/src/e2e_vm_tests/test_programs/should_pass/language/aliased_imports/src/main.sw
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
8 changes: 8 additions & 0 deletions test/src/e2e_vm_tests/test_programs/should_pass/language/aliased_imports/src/wiz.sw
View file
Original file line number Diff line number Diff line change
@@ -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,
}
4 changes: 2 additions & 2 deletions test/src/e2e_vm_tests/test_programs/should_pass/language/aliased_imports/test.toml
View file
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 722c95c

Please sign in to comment.