Skip to content

Commit

Permalink
Refactor: Split use synonyms into separate maps for glob and item imp…
Browse files Browse the repository at this point in the history 
…orts (#5914)

## Description

Fixes #5911 .

So far all imported items in a module have been collected in single map,
with a flag in each value indicating whether it was the result of a
glob/star import or an item import (this distinction matters because
item imports shadow glob imports).

This PR splits the map in two: One for glob imports and one for item
imports. This simplifies name resolution, and eliminates the
`GlobImport` flag.

When another module imports all reexported items the logic is now
slightly more complex, but this will be resolved once the excessive
cloning of namespaces, modules and typed declarations are eliminated.

## 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 Apr 24, 2024
1 parent 7d28830 commit 55140ba
Show file tree
Hide file tree
Showing 4 changed files with 79 additions and 66 deletions.
15 changes: 14 additions & 1 deletion sway-core/src/language/call_path.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,20 @@ impl CallPath {
let mut is_absolute = false;

if let Some(use_synonym) = namespace.module_id(engines).read(engines, |m| {
m.current_items().use_synonyms.get(&self.suffix).cloned()
if m.current_items()
.use_item_synonyms
.contains_key(&self.suffix)
{
m.current_items()
.use_item_synonyms
.get(&self.suffix)
.cloned()
} else {
m.current_items()
.use_glob_synonyms
.get(&self.suffix)
.cloned()
}
}) {
synonym_prefixes = use_synonym.0.clone();
is_absolute = true;
Expand Down
18 changes: 8 additions & 10 deletions sway-core/src/semantic_analysis/namespace/lexical_scope.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,6 @@ use sway_types::{span::Span, Spanned};

use std::sync::Arc;

/// Is this a glob (`use foo::*;`) import?
#[derive(Clone, Copy, PartialEq, Debug)]
pub(crate) enum GlobImport {
Yes,
No,
}

pub enum ResolvedFunctionDecl {
Parsed(ParsedDeclId<FunctionDeclaration>),
Typed(DeclRefFunction),
Expand All @@ -45,7 +38,8 @@ 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 UseSynonyms = im::HashMap<Ident, (Vec<Ident>, GlobImport, ty::TyDecl)>;
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>;

/// Represents a lexical scope integer-based identifier, which can be used to reference
Expand Down Expand Up @@ -80,7 +74,11 @@ pub struct Items {
///
/// For example, in `use ::foo::bar::Baz;`, we store a mapping from the symbol `Baz` to its
/// path `foo::bar::Baz`.
pub(crate) use_synonyms: UseSynonyms,
///
/// use_glob_synonyms contains symbols imported using star imports (`use foo::*`.).
/// use_item_synonyms contains symbols imported using item imports (`use foo::bar`).
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
Expand Down Expand Up @@ -289,7 +287,7 @@ impl Items {
append_shadowing_error(ident, decl, false, false, &item, const_shadowing_mode);
}

if let Some((ident, (_, GlobImport::No, decl))) = self.use_synonyms.get_key_value(&name) {
if let Some((ident, (_, decl))) = self.use_item_synonyms.get_key_value(&name) {
append_shadowing_error(
ident,
decl,
Expand Down
44 changes: 21 additions & 23 deletions sway-core/src/semantic_analysis/namespace/module.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -653,30 +653,28 @@ impl Module {
.use_aliases
.get(symbol.as_str())
.unwrap_or(symbol);
match module.current_items().use_synonyms.get(symbol) {
Some((_, _, decl @ ty::TyDecl::EnumVariantDecl { .. })) => {
Ok(ResolvedDeclaration::Typed(decl.clone()))
}
Some((src_path, _, _)) if mod_path != src_path => {
// If the symbol is imported, before resolving to it,
// we need to check if there is a local symbol withing the module with
// the same name, and if yes resolve to the local symbol, because it
// shadows the import.
// Note that we can have two situations here:
// - glob-import, in which case the local symbol simply shadows the glob-imported one.
// - non-glob import, in which case we will already have a name clash reported
// as an error, but still have to resolve to the local module symbol
// if it exists.
match module.current_items().symbols.get(true_symbol) {
Some(decl) => Ok(ResolvedDeclaration::Typed(decl.clone())),
None => self.resolve_symbol(handler, engines, src_path, true_symbol, self_type),
}
}
_ => module
.current_items()
.check_symbol(true_symbol)
.map_err(|e| handler.emit_err(e)),
// 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) {
return Ok(ResolvedDeclaration::Typed(decl.clone()));
}
// Check item imports
if let Some((_, decl @ ty::TyDecl::EnumVariantDecl { .. })) =
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()));
}
// Symbol not found
Err(handler.emit_err(CompileError::SymbolNotFound {
name: symbol.clone(),
span: symbol.span(),
}))
}
}

Expand Down
68 changes: 36 additions & 32 deletions sway-core/src/semantic_analysis/namespace/root.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
use super::{
lexical_scope::GlobImport, module::Module, namespace::Namespace, trait_map::TraitMap, Ident,
};
use super::{module::Module, namespace::Namespace, trait_map::TraitMap, Ident};
use crate::{
decl_engine::DeclRef,
engine_threading::*,
Expand Down Expand Up @@ -61,10 +59,10 @@ impl Root {
.implemented_traits
.extend(implemented_traits, engines); // TODO: No difference made between imported and declared items
for symbol_and_decl in symbols_and_decls {
dst_mod.current_items_mut().use_synonyms.insert(
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(), GlobImport::Yes, symbol_and_decl.1),
(src.to_vec(), symbol_and_decl.1),
);
}

Expand Down Expand Up @@ -141,15 +139,13 @@ 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((_, GlobImport::No, _)) =
dst_mod.current_items().use_synonyms.get(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_synonyms.insert(
dst_mod.current_items_mut().use_item_synonyms.insert(
// TODO: No difference made between imported and declared items
name.clone(),
(src.to_vec(), GlobImport::No, decl),
(src.to_vec(), decl),
);
};
match alias {
Expand Down Expand Up @@ -227,19 +223,18 @@ 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((_, GlobImport::No, _)) =
dst_mod.current_items().use_synonyms.get(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_synonyms.insert(
dst_mod.current_items_mut().use_item_synonyms.insert(
// TODO: No difference made between imported and declared items
name.clone(),
(
src.to_vec(),
GlobImport::No,
TyDecl::EnumVariantDecl(ty::EnumVariantDecl {
enum_ref: enum_ref.clone(),
variant_name: variant_name.clone(),
Expand Down Expand Up @@ -326,12 +321,11 @@ impl Root {

// import it this way.
let dst_mod = self.module.lookup_submodule_mut(handler, engines, dst)?;
dst_mod.current_items_mut().use_synonyms.insert(
dst_mod.current_items_mut().use_glob_synonyms.insert(
// TODO: No difference made between imported and declared items
variant_name.clone(),
(
src.to_vec(),
GlobImport::Yes,
TyDecl::EnumVariantDecl(ty::EnumVariantDecl {
enum_ref: enum_ref.clone(),
variant_name: variant_name.clone(),
Expand Down Expand Up @@ -378,24 +372,27 @@ impl Root {
let src_mod = self.module.lookup_submodule(handler, engines, src)?;

let implemented_traits = src_mod.current_items().implemented_traits.clone();
let use_synonyms = src_mod.current_items().use_synonyms.clone();
let mut symbols_and_decls = src_mod
.current_items()
.use_synonyms
.iter()
.map(|(symbol, (_, _, decl))| (symbol.clone(), decl.clone()))
.collect::<Vec<_>>();
let use_item_synonyms = src_mod.current_items().use_item_synonyms.clone();
let use_glob_synonyms = src_mod.current_items().use_glob_synonyms.clone();

// 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, (_, 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() {
if is_ancestor(src, dst) || decl.visibility(decl_engine).is_public() {
symbols_and_decls.push((symbol.clone(), decl.clone()));
all_symbols_and_decls.push((symbol.clone(), decl.clone()));
}
}

let mut symbols_paths_and_decls = vec![];
for (symbol, (mod_path, _, decl)) in use_synonyms {
let get_path = |mod_path: Vec<Ident>| {
let mut is_external = false;
let submodule = src_mod.submodule(engines, &[mod_path[0].clone()]);
if let Some(submodule) = submodule {
if let Some(submodule) = src_mod.submodule(engines, &[mod_path[0].clone()]) {
is_external = submodule.is_external
};

Expand All @@ -406,7 +403,14 @@ impl Root {
path.extend(mod_path);
}

symbols_paths_and_decls.push((symbol, path, decl));
path
};

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

let dst_mod = self.module.lookup_submodule_mut(handler, engines, dst)?;
Expand All @@ -418,16 +422,16 @@ impl Root {
let mut try_add = |symbol, path, decl: ty::TyDecl| {
dst_mod
.current_items_mut()
.use_synonyms
.insert(symbol, (path, GlobImport::Yes, decl)); // TODO: No difference made between imported and declared items
.use_glob_synonyms
.insert(symbol, (path, decl));
};

for (symbol, decl) in symbols_and_decls {
for (symbol, decl) in all_symbols_and_decls {
try_add(symbol, src.to_vec(), decl);
}

for (symbol, path, decl) in symbols_paths_and_decls {
try_add(symbol, path, decl);
try_add(symbol.clone(), path, decl.clone());
}

Ok(())
Expand Down

0 comments on commit 55140ba

Please sign in to comment.