Skip to content

Commit fb3d21d

Browse files
tnowackitnowacki
authored
[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.
1 parent 746b7f0 commit fb3d21d

File tree

59 files changed

+319
-174
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

59 files changed

+319
-174
lines changed

‎move-analyzer/src/symbols.rs

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -350,8 +350,14 @@ fn type_to_ide_string(sp!(_, t): &Type) -> String {
350350

351351
fn addr_to_ide_string(addr: &Address) -> String {
352352
match addr {
353-
Address::Numerical(None, sp!(_, bytes)) => format!("{}", bytes),
354-
Address::Numerical(Some(name), _) => format!("{}", name),
353+
Address::Numerical {
354+
name: None,
355+
value: sp!(_, bytes),
356+
..
357+
} => format!("{}", bytes),
358+
Address::Numerical {
359+
name: Some(name), ..
360+
} => format!("{}", name),
355361
Address::NamedUnassigned(name) => format!("{}", name),
356362
}
357363
}

‎move-compiler/src/compiled_unit.rs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,8 +98,11 @@ pub type AnnotatedCompiledUnit = CompiledUnitEnum<AnnotatedCompiledModule, Annot
9898
impl AnnotatedCompiledModule {
9999
pub fn module_ident(&self) -> ModuleIdent {
100100
use crate::expansion::ast::Address;
101-
let address =
102-
Address::Numerical(self.address_name, sp(self.loc, self.named_module.address));
101+
let address = Address::Numerical {
102+
name: self.address_name,
103+
value: sp(self.loc, self.named_module.address),
104+
name_conflict: false,
105+
};
103106
sp(
104107
self.loc,
105108
ModuleIdent_::new(

‎move-compiler/src/expansion/ast.rs

Lines changed: 41 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,12 @@ pub struct Script {
142142

143143
#[derive(Clone, Copy)]
144144
pub enum Address {
145-
Numerical(Option<Name>, Spanned<NumericalAddress>),
145+
Numerical {
146+
name: Option<Name>,
147+
value: Spanned<NumericalAddress>,
148+
// set to true when the same name is used across multiple packages
149+
name_conflict: bool,
150+
},
146151
NamedUnassigned(Name),
147152
}
148153
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
@@ -565,7 +570,7 @@ impl fmt::Debug for Address {
565570
impl PartialEq for Address {
566571
fn eq(&self, other: &Self) -> bool {
567572
match (self, other) {
568-
(Self::Numerical(_, l), Self::Numerical(_, r)) => l == r,
573+
(Self::Numerical { value: l, .. }, Self::Numerical { value: r, .. }) => l == r,
569574
(Self::NamedUnassigned(l), Self::NamedUnassigned(r)) => l == r,
570575
_ => false,
571576
}
@@ -585,10 +590,10 @@ impl Ord for Address {
585590
use std::cmp::Ordering;
586591

587592
match (self, other) {
588-
(Self::Numerical(_, _), Self::NamedUnassigned(_)) => Ordering::Less,
589-
(Self::NamedUnassigned(_), Self::Numerical(_, _)) => Ordering::Greater,
593+
(Self::Numerical { .. }, Self::NamedUnassigned(_)) => Ordering::Less,
594+
(Self::NamedUnassigned(_), Self::Numerical { .. }) => Ordering::Greater,
590595

591-
(Self::Numerical(_, l), Self::Numerical(_, r)) => l.cmp(r),
596+
(Self::Numerical { value: l, .. }, Self::Numerical { value: r, .. }) => l.cmp(r),
592597
(Self::NamedUnassigned(l), Self::NamedUnassigned(r)) => l.cmp(r),
593598
}
594599
}
@@ -597,7 +602,10 @@ impl Ord for Address {
597602
impl Hash for Address {
598603
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
599604
match self {
600-
Self::Numerical(_, sp!(_, bytes)) => bytes.hash(state),
605+
Self::Numerical {
606+
value: sp!(_, bytes),
607+
..
608+
} => bytes.hash(state),
601609
Self::NamedUnassigned(name) => name.hash(state),
602610
}
603611
}
@@ -623,22 +631,29 @@ impl UseFuns {
623631

624632
impl Address {
625633
pub const fn anonymous(loc: Loc, address: NumericalAddress) -> Self {
626-
Self::Numerical(None, sp(loc, address))
634+
Self::Numerical {
635+
name: None,
636+
value: sp(loc, address),
637+
name_conflict: false,
638+
}
627639
}
628640

629641
pub fn into_addr_bytes(self) -> NumericalAddress {
630642
match self {
631-
Self::Numerical(_, sp!(_, bytes)) => bytes,
643+
Self::Numerical {
644+
value: sp!(_, bytes),
645+
..
646+
} => bytes,
632647
Self::NamedUnassigned(_) => NumericalAddress::DEFAULT_ERROR_ADDRESS,
633648
}
634649
}
635650

636651
pub fn is(&self, address: impl AsRef<str>) -> bool {
637652
match self {
638-
Self::Numerical(Some(n), _) | Self::NamedUnassigned(n) => {
653+
Self::Numerical { name: Some(n), .. } | Self::NamedUnassigned(n) => {
639654
n.value.as_str() == address.as_ref()
640655
}
641-
Self::Numerical(None, _) => false,
656+
Self::Numerical { name: None, .. } => false,
642657
}
643658
}
644659
}
@@ -843,9 +858,22 @@ impl IntoIterator for AbilitySet {
843858
impl fmt::Display for Address {
844859
fn fmt(&self, f: &mut fmt::Formatter) -> std::fmt::Result {
845860
match self {
846-
Self::Numerical(None, sp!(_, bytes)) => write!(f, "{}", bytes),
847-
Self::Numerical(Some(name), sp!(_, bytes)) => write!(f, "({}={})", name, bytes),
848-
Self::NamedUnassigned(name) => write!(f, "{}", name),
861+
Self::Numerical {
862+
name: None,
863+
value: sp!(_, bytes),
864+
..
865+
} => write!(f, "{}", bytes),
866+
Self::Numerical {
867+
name: Some(name),
868+
value: sp!(_, bytes),
869+
name_conflict: true,
870+
} => write!(f, "({}={})", name, bytes),
871+
Self::Numerical {
872+
name: Some(name),
873+
value: _,
874+
name_conflict: false,
875+
}
876+
| Self::NamedUnassigned(name) => write!(f, "{}", name),
849877
}
850878
}
851879
}

‎move-compiler/src/expansion/translate.rs

Lines changed: 58 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ use crate::{
1818
FullyCompiledProgram,
1919
};
2020
use move_command_line_common::parser::{parse_u16, parse_u256, parse_u32};
21+
use move_core_types::account_address::AccountAddress;
2122
use move_ir_types::location::*;
2223
use move_symbol_pool::Symbol;
2324
use std::{
@@ -36,6 +37,7 @@ type ModuleMembers = BTreeMap<Name, ModuleMemberKind>;
3637
struct Context<'env, 'map> {
3738
module_members: UniqueMap<ModuleIdent, ModuleMembers>,
3839
named_address_mapping: Option<&'map NamedAddressMap>,
40+
address_conflicts: BTreeSet<Symbol>,
3941
address: Option<Address>,
4042
aliases: AliasMap,
4143
is_source_definition: bool,
@@ -52,6 +54,7 @@ impl<'env, 'map> Context<'env, 'map> {
5254
fn new(
5355
compilation_env: &'env mut CompilationEnv,
5456
module_members: UniqueMap<ModuleIdent, ModuleMembers>,
57+
address_conflicts: BTreeSet<Symbol>,
5558
) -> Self {
5659
let mut all_filter_alls = WarningFilters::new_for_dependency();
5760
for allow in compilation_env.filter_attributes() {
@@ -63,6 +66,7 @@ impl<'env, 'map> Context<'env, 'map> {
6366
module_members,
6467
env: compilation_env,
6568
named_address_mapping: None,
69+
address_conflicts,
6670
address: None,
6771
aliases: AliasMap::new(),
6872
is_source_definition: false,
@@ -122,6 +126,40 @@ impl<'env, 'map> Context<'env, 'map> {
122126
}
123127
}
124128

129+
/// We mark named addresses as having a conflict if there is not a bidirectional mapping between
130+
/// the name and its value
131+
fn compute_address_conflicts(
132+
pre_compiled_lib: Option<&FullyCompiledProgram>,
133+
prog: &P::Program,
134+
) -> BTreeSet<Symbol> {
135+
let mut name_to_addr: BTreeMap<Symbol, BTreeSet<AccountAddress>> = BTreeMap::new();
136+
let mut addr_to_name: BTreeMap<AccountAddress, BTreeSet<Symbol>> = BTreeMap::new();
137+
let all_addrs = prog.named_address_maps.all().iter().chain(
138+
pre_compiled_lib
139+
.iter()
140+
.flat_map(|pre| pre.parser.named_address_maps.all()),
141+
);
142+
for map in all_addrs {
143+
for (n, addr) in map {
144+
let n = *n;
145+
let addr = addr.into_inner();
146+
name_to_addr.entry(n).or_default().insert(addr);
147+
addr_to_name.entry(addr).or_default().insert(n);
148+
}
149+
}
150+
let name_to_addr_conflicts = name_to_addr
151+
.into_iter()
152+
.filter(|(_, addrs)| addrs.len() > 1)
153+
.map(|(n, _)| n);
154+
let addr_to_name_conflicts = addr_to_name
155+
.into_iter()
156+
.filter(|(_, addrs)| addrs.len() > 1)
157+
.flat_map(|(_, ns)| ns.into_iter());
158+
name_to_addr_conflicts
159+
.chain(addr_to_name_conflicts)
160+
.collect()
161+
}
162+
125163
//**************************************************************************************************
126164
// Entry
127165
//**************************************************************************************************
@@ -131,17 +169,20 @@ pub fn program(
131169
pre_compiled_lib: Option<&FullyCompiledProgram>,
132170
prog: P::Program,
133171
) -> E::Program {
172+
let address_conflicts = compute_address_conflicts(pre_compiled_lib, &prog);
134173
let module_members = {
135174
let mut members = UniqueMap::new();
136175
all_module_members(
137176
compilation_env,
177+
&address_conflicts,
138178
&prog.named_address_maps,
139179
&mut members,
140180
true,
141181
&prog.source_definitions,
142182
);
143183
all_module_members(
144184
compilation_env,
185+
&address_conflicts,
145186
&prog.named_address_maps,
146187
&mut members,
147188
true,
@@ -151,6 +192,7 @@ pub fn program(
151192
assert!(pre_compiled.parser.lib_definitions.is_empty());
152193
all_module_members(
153194
compilation_env,
195+
&address_conflicts,
154196
&pre_compiled.parser.named_address_maps,
155197
&mut members,
156198
false,
@@ -160,7 +202,7 @@ pub fn program(
160202
members
161203
};
162204

163-
let mut context = Context::new(compilation_env, module_members);
205+
let mut context = Context::new(compilation_env, module_members, address_conflicts);
164206

165207
let mut source_module_map = UniqueMap::new();
166208
let mut lib_module_map = UniqueMap::new();
@@ -296,6 +338,7 @@ fn address_without_value_error(suggest_declaration: bool, loc: Loc, n: &Name) ->
296338
fn address(context: &mut Context, suggest_declaration: bool, ln: P::LeadingNameAccess) -> Address {
297339
address_(
298340
context.env,
341+
&context.address_conflicts,
299342
context.named_address_mapping.as_ref().unwrap(),
300343
suggest_declaration,
301344
ln,
@@ -304,6 +347,7 @@ fn address(context: &mut Context, suggest_declaration: bool, ln: P::LeadingNameA
304347

305348
fn address_(
306349
compilation_env: &mut CompilationEnv,
350+
address_conflicts: &BTreeSet<Symbol>,
307351
named_address_mapping: &NamedAddressMap,
308352
suggest_declaration: bool,
309353
ln: P::LeadingNameAccess,
@@ -312,11 +356,15 @@ fn address_(
312356
let sp!(loc, ln_) = ln;
313357
match ln_ {
314358
P::LeadingNameAccess_::AnonymousAddress(bytes) => {
315-
debug_assert!(name_res.is_ok()); //
316-
Address::Numerical(None, sp(loc, bytes))
359+
debug_assert!(name_res.is_ok());
360+
Address::anonymous(loc, bytes)
317361
}
318362
P::LeadingNameAccess_::Name(n) => match named_address_mapping.get(&n.value).copied() {
319-
Some(addr) => Address::Numerical(Some(n), sp(loc, addr)),
363+
Some(addr) => Address::Numerical {
364+
name: Some(n),
365+
value: sp(loc, addr),
366+
name_conflict: address_conflicts.contains(&n.value),
367+
},
320368
None => {
321369
if name_res.is_ok() {
322370
compilation_env.add_diag(address_without_value_error(
@@ -416,7 +464,7 @@ fn set_sender_address(
416464
context
417465
.env
418466
.add_diag(diag!(Declarations::InvalidModule, (loc, msg)));
419-
Address::Numerical(None, sp(loc, NumericalAddress::DEFAULT_ERROR_ADDRESS))
467+
Address::anonymous(loc, NumericalAddress::DEFAULT_ERROR_ADDRESS)
420468
}
421469
})
422470
}
@@ -801,7 +849,7 @@ fn attribute_value(
801849
match avalue_ {
802850
PV::Value(v) => EV::Value(value(context, v)?),
803851
PV::ModuleAccess(sp!(ident_loc, PN::Two(sp!(aloc, LN::AnonymousAddress(a)), n))) => {
804-
let addr = Address::Numerical(None, sp(aloc, a));
852+
let addr = Address::anonymous(aloc, a);
805853
let mident = sp(ident_loc, ModuleIdent_::new(addr, ModuleName(n)));
806854
if context.module_members.get(&mident).is_none() {
807855
context.env.add_diag(diag!(
@@ -940,6 +988,7 @@ fn warning_filter(
940988

941989
fn all_module_members<'a>(
942990
compilation_env: &mut CompilationEnv,
991+
address_conflicts: &BTreeSet<Symbol>,
943992
named_addr_maps: &NamedAddressMaps,
944993
members: &mut UniqueMap<ModuleIdent, ModuleMembers>,
945994
always_add: bool,
@@ -958,21 +1007,21 @@ fn all_module_members<'a>(
9581007
Some(a) => {
9591008
address_(
9601009
compilation_env,
1010+
address_conflicts,
9611011
named_addr_map,
9621012
/* suggest_declaration */ true,
9631013
*a,
9641014
)
9651015
}
9661016
// Error will be handled when the module is compiled
967-
None => {
968-
Address::Numerical(None, sp(m.loc, NumericalAddress::DEFAULT_ERROR_ADDRESS))
969-
}
1017+
None => Address::anonymous(m.loc, NumericalAddress::DEFAULT_ERROR_ADDRESS),
9701018
};
9711019
module_members(members, always_add, addr, m)
9721020
}
9731021
P::Definition::Address(addr_def) => {
9741022
let addr = address_(
9751023
compilation_env,
1024+
address_conflicts,
9761025
named_addr_map,
9771026
/* suggest_declaration */ false,
9781027
addr_def.addr,

‎move-compiler/src/naming/fake_natives.rs

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55
//! as 'native`, but do not appear in the compiled module. For developer sanity, they must be marked
66
//! with the `FAKE_NATIVE_ATTR`
77
8-
use move_core_types::account_address::AccountAddress;
98
use std::convert::TryInto;
109

1110
use crate::{
@@ -90,12 +89,13 @@ pub fn resolve_builtin(
9089
let sp!(_, ModuleIdent_ { address, module }) = module;
9190
// Only resolve if either (a) the address is named "std" or (b) its value is 0x1
9291
match address {
93-
Address::Numerical(Some(sp!(_, n)), _) | Address::NamedUnassigned(sp!(_, n))
94-
if *n == symbol!("std") => {}
95-
Address::Numerical(_, sp!(_, a)) if a.into_inner() == AccountAddress::ONE => {}
96-
_ => {
97-
return None;
92+
Address::Numerical {
93+
name: Some(sp!(_, n)),
94+
..
9895
}
96+
| Address::NamedUnassigned(sp!(_, n))
97+
if *n == symbol!("std") => {}
98+
_ => return None,
9999
};
100100
Some(match (module.value().as_str(), function.value().as_str()) {
101101
("vector", "empty") => |tys| IR::Bytecode_::VecPack(expect_one_ty_arg(tys), 0),

‎move-compiler/src/shared/mod.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,10 @@ impl NamedAddressMaps {
178178
pub fn get(&self, idx: NamedAddressMapIndex) -> &NamedAddressMap {
179179
&self.0[idx.0]
180180
}
181+
182+
pub fn all(&self) -> &[NamedAddressMap] {
183+
&self.0
184+
}
181185
}
182186

183187
#[derive(Clone, Debug, Eq, PartialEq)]

‎move-compiler/src/to_bytecode/context.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,9 @@ impl<'a> Context<'a> {
208208

209209
fn ir_module_alias(sp!(_, ModuleIdent_ { address, module }): &ModuleIdent) -> IR::ModuleName {
210210
let s = match address {
211-
Address::Numerical(_, sp!(_, a_)) => format!("{:X}::{}", a_, module),
211+
Address::Numerical {
212+
value: sp!(_, a_), ..
213+
} => format!("{:X}::{}", a_, module),
212214
Address::NamedUnassigned(name) => format!("{}::{}", name, module),
213215
};
214216
IR::ModuleName(s.into())

0 commit comments

Comments
 (0)