[move-compiler] Make addresses in error messages more readable (#14049)

## Description 

- About two years ago, we made named addresses transparent, instead of
opaque, with respect to their numerical value.
- To help prevent confusion, the compiler always expanded the named
address.
- Now, the compiler will only do so when the named address is not in a
1:1 mapping with its value

## Test Plan 

- Added tests, updated tests 

---
If your changes are not user-facing and not a breaking change, you can
skip the following section. Otherwise, please indicate what changed, and
then add to the Release Notes section as highlighted during the release
process.

### Type of Change (Check all that apply)

- [ ] protocol change
- [X] user-visible impact
- [ ] breaking change for a client SDKs
- [ ] breaking change for FNs (FN binary must upgrade)
- [ ] breaking change for validators or node operators (must upgrade
binaries)
- [ ] breaking change for on-chain data layout
- [ ] necessitate either a data wipe or data migration

### Release notes

The Move compiler will now try to make addresses in errors more
readable, expanding them to their numerical value only when it is
potentially ambiguous.

Author: tnowacki

move-analyzer/src/symbols.rs

@@ -350,8 +350,14 @@ fn type_to_ide_string(sp!(_, t): &Type) -> String {
 
 fn addr_to_ide_string(addr: &Address) -> String {
     match addr {
-        Address::Numerical(None, sp!(_, bytes)) => format!("{}", bytes),
-        Address::Numerical(Some(name), _) => format!("{}", name),
+        Address::Numerical {
+            name: None,
+            value: sp!(_, bytes),
+            ..
+        } => format!("{}", bytes),
+        Address::Numerical {
+            name: Some(name), ..
+        } => format!("{}", name),
         Address::NamedUnassigned(name) => format!("{}", name),
     }
 }

move-compiler/src/compiled_unit.rs

@@ -98,8 +98,11 @@ pub type AnnotatedCompiledUnit = CompiledUnitEnum<AnnotatedCompiledModule, Annot
 impl AnnotatedCompiledModule {
     pub fn module_ident(&self) -> ModuleIdent {
         use crate::expansion::ast::Address;
-        let address =
-            Address::Numerical(self.address_name, sp(self.loc, self.named_module.address));
+        let address = Address::Numerical {
+            name: self.address_name,
+            value: sp(self.loc, self.named_module.address),
+            name_conflict: false,
+        };
         sp(
             self.loc,
             ModuleIdent_::new(

move-compiler/src/expansion/ast.rs

@@ -142,7 +142,12 @@ pub struct Script {
 
 #[derive(Clone, Copy)]
 pub enum Address {
-    Numerical(Option<Name>, Spanned<NumericalAddress>),
+    Numerical {
+        name: Option<Name>,
+        value: Spanned<NumericalAddress>,
+        // set to true when the same name is used across multiple packages
+        name_conflict: bool,
+    },
     NamedUnassigned(Name),
 }
 #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
@@ -565,7 +570,7 @@ impl fmt::Debug for Address {
 impl PartialEq for Address {
     fn eq(&self, other: &Self) -> bool {
         match (self, other) {
-            (Self::Numerical(_, l), Self::Numerical(_, r)) => l == r,
+            (Self::Numerical { value: l, .. }, Self::Numerical { value: r, .. }) => l == r,
             (Self::NamedUnassigned(l), Self::NamedUnassigned(r)) => l == r,
             _ => false,
         }
@@ -585,10 +590,10 @@ impl Ord for Address {
         use std::cmp::Ordering;
 
         match (self, other) {
-            (Self::Numerical(_, _), Self::NamedUnassigned(_)) => Ordering::Less,
-            (Self::NamedUnassigned(_), Self::Numerical(_, _)) => Ordering::Greater,
+            (Self::Numerical { .. }, Self::NamedUnassigned(_)) => Ordering::Less,
+            (Self::NamedUnassigned(_), Self::Numerical { .. }) => Ordering::Greater,
 
-            (Self::Numerical(_, l), Self::Numerical(_, r)) => l.cmp(r),
+            (Self::Numerical { value: l, .. }, Self::Numerical { value: r, .. }) => l.cmp(r),
             (Self::NamedUnassigned(l), Self::NamedUnassigned(r)) => l.cmp(r),
         }
     }
@@ -597,7 +602,10 @@ impl Ord for Address {
 impl Hash for Address {
     fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
         match self {
-            Self::Numerical(_, sp!(_, bytes)) => bytes.hash(state),
+            Self::Numerical {
+                value: sp!(_, bytes),
+                ..
+            } => bytes.hash(state),
             Self::NamedUnassigned(name) => name.hash(state),
         }
     }
@@ -623,22 +631,29 @@ impl UseFuns {
 
 impl Address {
     pub const fn anonymous(loc: Loc, address: NumericalAddress) -> Self {
-        Self::Numerical(None, sp(loc, address))
+        Self::Numerical {
+            name: None,
+            value: sp(loc, address),
+            name_conflict: false,
+        }
     }
 
     pub fn into_addr_bytes(self) -> NumericalAddress {
         match self {
-            Self::Numerical(_, sp!(_, bytes)) => bytes,
+            Self::Numerical {
+                value: sp!(_, bytes),
+                ..
+            } => bytes,
             Self::NamedUnassigned(_) => NumericalAddress::DEFAULT_ERROR_ADDRESS,
         }
     }
 
     pub fn is(&self, address: impl AsRef<str>) -> bool {
         match self {
-            Self::Numerical(Some(n), _) | Self::NamedUnassigned(n) => {
+            Self::Numerical { name: Some(n), .. } | Self::NamedUnassigned(n) => {
                 n.value.as_str() == address.as_ref()
             }
-            Self::Numerical(None, _) => false,
+            Self::Numerical { name: None, .. } => false,
         }
     }
 }
@@ -843,9 +858,22 @@ impl IntoIterator for AbilitySet {
 impl fmt::Display for Address {
     fn fmt(&self, f: &mut fmt::Formatter) -> std::fmt::Result {
         match self {
-            Self::Numerical(None, sp!(_, bytes)) => write!(f, "{}", bytes),
-            Self::Numerical(Some(name), sp!(_, bytes)) => write!(f, "({}={})", name, bytes),
-            Self::NamedUnassigned(name) => write!(f, "{}", name),
+            Self::Numerical {
+                name: None,
+                value: sp!(_, bytes),
+                ..
+            } => write!(f, "{}", bytes),
+            Self::Numerical {
+                name: Some(name),
+                value: sp!(_, bytes),
+                name_conflict: true,
+            } => write!(f, "({}={})", name, bytes),
+            Self::Numerical {
+                name: Some(name),
+                value: _,
+                name_conflict: false,
+            }
+            | Self::NamedUnassigned(name) => write!(f, "{}", name),
         }
     }
 }

move-compiler/src/expansion/translate.rs

@@ -18,6 +18,7 @@ use crate::{
     FullyCompiledProgram,
 };
 use move_command_line_common::parser::{parse_u16, parse_u256, parse_u32};
+use move_core_types::account_address::AccountAddress;
 use move_ir_types::location::*;
 use move_symbol_pool::Symbol;
 use std::{
@@ -36,6 +37,7 @@ type ModuleMembers = BTreeMap<Name, ModuleMemberKind>;
 struct Context<'env, 'map> {
     module_members: UniqueMap<ModuleIdent, ModuleMembers>,
     named_address_mapping: Option<&'map NamedAddressMap>,
+    address_conflicts: BTreeSet<Symbol>,
     address: Option<Address>,
     aliases: AliasMap,
     is_source_definition: bool,
@@ -52,6 +54,7 @@ impl<'env, 'map> Context<'env, 'map> {
     fn new(
         compilation_env: &'env mut CompilationEnv,
         module_members: UniqueMap<ModuleIdent, ModuleMembers>,
+        address_conflicts: BTreeSet<Symbol>,
     ) -> Self {
         let mut all_filter_alls = WarningFilters::new_for_dependency();
         for allow in compilation_env.filter_attributes() {
@@ -63,6 +66,7 @@ impl<'env, 'map> Context<'env, 'map> {
             module_members,
             env: compilation_env,
             named_address_mapping: None,
+            address_conflicts,
             address: None,
             aliases: AliasMap::new(),
             is_source_definition: false,
@@ -122,6 +126,40 @@ impl<'env, 'map> Context<'env, 'map> {
     }
 }
 
+/// We mark named addresses as having a conflict if there is not a bidirectional mapping between
+/// the name and its value
+fn compute_address_conflicts(
+    pre_compiled_lib: Option<&FullyCompiledProgram>,
+    prog: &P::Program,
+) -> BTreeSet<Symbol> {
+    let mut name_to_addr: BTreeMap<Symbol, BTreeSet<AccountAddress>> = BTreeMap::new();
+    let mut addr_to_name: BTreeMap<AccountAddress, BTreeSet<Symbol>> = BTreeMap::new();
+    let all_addrs = prog.named_address_maps.all().iter().chain(
+        pre_compiled_lib
+            .iter()
+            .flat_map(|pre| pre.parser.named_address_maps.all()),
+    );
+    for map in all_addrs {
+        for (n, addr) in map {
+            let n = *n;
+            let addr = addr.into_inner();
+            name_to_addr.entry(n).or_default().insert(addr);
+            addr_to_name.entry(addr).or_default().insert(n);
+        }
+    }
+    let name_to_addr_conflicts = name_to_addr
+        .into_iter()
+        .filter(|(_, addrs)| addrs.len() > 1)
+        .map(|(n, _)| n);
+    let addr_to_name_conflicts = addr_to_name
+        .into_iter()
+        .filter(|(_, addrs)| addrs.len() > 1)
+        .flat_map(|(_, ns)| ns.into_iter());
+    name_to_addr_conflicts
+        .chain(addr_to_name_conflicts)
+        .collect()
+}
+
 //**************************************************************************************************
 // Entry
 //**************************************************************************************************
@@ -131,17 +169,20 @@ pub fn program(
     pre_compiled_lib: Option<&FullyCompiledProgram>,
     prog: P::Program,
 ) -> E::Program {
+    let address_conflicts = compute_address_conflicts(pre_compiled_lib, &prog);
     let module_members = {
         let mut members = UniqueMap::new();
         all_module_members(
             compilation_env,
+            &address_conflicts,
             &prog.named_address_maps,
             &mut members,
             true,
             &prog.source_definitions,
         );
         all_module_members(
             compilation_env,
+            &address_conflicts,
             &prog.named_address_maps,
             &mut members,
             true,
@@ -151,6 +192,7 @@ pub fn program(
             assert!(pre_compiled.parser.lib_definitions.is_empty());
             all_module_members(
                 compilation_env,
+                &address_conflicts,
                 &pre_compiled.parser.named_address_maps,
                 &mut members,
                 false,
@@ -160,7 +202,7 @@ pub fn program(
         members
     };
 
-    let mut context = Context::new(compilation_env, module_members);
+    let mut context = Context::new(compilation_env, module_members, address_conflicts);
 
     let mut source_module_map = UniqueMap::new();
     let mut lib_module_map = UniqueMap::new();
@@ -296,6 +338,7 @@ fn address_without_value_error(suggest_declaration: bool, loc: Loc, n: &Name) ->
 fn address(context: &mut Context, suggest_declaration: bool, ln: P::LeadingNameAccess) -> Address {
     address_(
         context.env,
+        &context.address_conflicts,
         context.named_address_mapping.as_ref().unwrap(),
         suggest_declaration,
         ln,
@@ -304,6 +347,7 @@ fn address(context: &mut Context, suggest_declaration: bool, ln: P::LeadingNameA
 
 fn address_(
     compilation_env: &mut CompilationEnv,
+    address_conflicts: &BTreeSet<Symbol>,
     named_address_mapping: &NamedAddressMap,
     suggest_declaration: bool,
     ln: P::LeadingNameAccess,
@@ -312,11 +356,15 @@ fn address_(
     let sp!(loc, ln_) = ln;
     match ln_ {
         P::LeadingNameAccess_::AnonymousAddress(bytes) => {
-            debug_assert!(name_res.is_ok()); //
-            Address::Numerical(None, sp(loc, bytes))
+            debug_assert!(name_res.is_ok());
+            Address::anonymous(loc, bytes)
         }
         P::LeadingNameAccess_::Name(n) => match named_address_mapping.get(&n.value).copied() {
-            Some(addr) => Address::Numerical(Some(n), sp(loc, addr)),
+            Some(addr) => Address::Numerical {
+                name: Some(n),
+                value: sp(loc, addr),
+                name_conflict: address_conflicts.contains(&n.value),
+            },
             None => {
                 if name_res.is_ok() {
                     compilation_env.add_diag(address_without_value_error(
@@ -416,7 +464,7 @@ fn set_sender_address(
             context
                 .env
                 .add_diag(diag!(Declarations::InvalidModule, (loc, msg)));
-            Address::Numerical(None, sp(loc, NumericalAddress::DEFAULT_ERROR_ADDRESS))
+            Address::anonymous(loc, NumericalAddress::DEFAULT_ERROR_ADDRESS)
         }
     })
 }
@@ -801,7 +849,7 @@ fn attribute_value(
         match avalue_ {
             PV::Value(v) => EV::Value(value(context, v)?),
             PV::ModuleAccess(sp!(ident_loc, PN::Two(sp!(aloc, LN::AnonymousAddress(a)), n))) => {
-                let addr = Address::Numerical(None, sp(aloc, a));
+                let addr = Address::anonymous(aloc, a);
                 let mident = sp(ident_loc, ModuleIdent_::new(addr, ModuleName(n)));
                 if context.module_members.get(&mident).is_none() {
                     context.env.add_diag(diag!(
@@ -940,6 +988,7 @@ fn warning_filter(
 
 fn all_module_members<'a>(
     compilation_env: &mut CompilationEnv,
+    address_conflicts: &BTreeSet<Symbol>,
     named_addr_maps: &NamedAddressMaps,
     members: &mut UniqueMap<ModuleIdent, ModuleMembers>,
     always_add: bool,
@@ -958,21 +1007,21 @@ fn all_module_members<'a>(
                     Some(a) => {
                         address_(
                             compilation_env,
+                            address_conflicts,
                             named_addr_map,
                             /* suggest_declaration */ true,
                             *a,
                         )
                     }
                     // Error will be handled when the module is compiled
-                    None => {
-                        Address::Numerical(None, sp(m.loc, NumericalAddress::DEFAULT_ERROR_ADDRESS))
-                    }
+                    None => Address::anonymous(m.loc, NumericalAddress::DEFAULT_ERROR_ADDRESS),
                 };
                 module_members(members, always_add, addr, m)
             }
             P::Definition::Address(addr_def) => {
                 let addr = address_(
                     compilation_env,
+                    address_conflicts,
                     named_addr_map,
                     /* suggest_declaration */ false,
                     addr_def.addr,

move-compiler/src/naming/fake_natives.rs

@@ -5,7 +5,6 @@
 //! as 'native`, but do not appear in the compiled module. For developer sanity, they must be marked
 //! with the `FAKE_NATIVE_ATTR`
 
-use move_core_types::account_address::AccountAddress;
 use std::convert::TryInto;
 
 use crate::{
@@ -90,12 +89,13 @@ pub fn resolve_builtin(
     let sp!(_, ModuleIdent_ { address, module }) = module;
     // Only resolve if either (a) the address is named "std" or (b) its value is 0x1
     match address {
-        Address::Numerical(Some(sp!(_, n)), _) | Address::NamedUnassigned(sp!(_, n))
-            if *n == symbol!("std") => {}
-        Address::Numerical(_, sp!(_, a)) if a.into_inner() == AccountAddress::ONE => {}
-        _ => {
-            return None;
+        Address::Numerical {
+            name: Some(sp!(_, n)),
+            ..
         }
+        | Address::NamedUnassigned(sp!(_, n))
+            if *n == symbol!("std") => {}
+        _ => return None,
     };
     Some(match (module.value().as_str(), function.value().as_str()) {
         ("vector", "empty") => |tys| IR::Bytecode_::VecPack(expect_one_ty_arg(tys), 0),

move-compiler/src/shared/mod.rs

@@ -178,6 +178,10 @@ impl NamedAddressMaps {
     pub fn get(&self, idx: NamedAddressMapIndex) -> &NamedAddressMap {
         &self.0[idx.0]
     }
+
+    pub fn all(&self) -> &[NamedAddressMap] {
+        &self.0
+    }
 }
 
 #[derive(Clone, Debug, Eq, PartialEq)]

move-compiler/src/to_bytecode/context.rs

@@ -208,7 +208,9 @@ impl<'a> Context<'a> {
 
     fn ir_module_alias(sp!(_, ModuleIdent_ { address, module }): &ModuleIdent) -> IR::ModuleName {
         let s = match address {
-            Address::Numerical(_, sp!(_, a_)) => format!("{:X}::{}", a_, module),
+            Address::Numerical {
+                value: sp!(_, a_), ..
+            } => format!("{:X}::{}", a_, module),
             Address::NamedUnassigned(name) => format!("{}::{}", name, module),
         };
         IR::ModuleName(s.into())