diff --git a/third_party/move/move-compiler-v2/tests/checking-lang-v1/v1-typing/refer_other_constants.exp b/third_party/move/move-compiler-v2/tests/checking-lang-v1/v1-typing/refer_other_constants.exp new file mode 100644 index 00000000000000..f34833e81ac99c --- /dev/null +++ b/third_party/move/move-compiler-v2/tests/checking-lang-v1/v1-typing/refer_other_constants.exp @@ -0,0 +1,7 @@ + +Diagnostics: +error: not supported before language version `2.0`: referring to other constants + ┌─ tests/checking-lang-v1/v1-typing/refer_other_constants.move:3:5 + │ +3 │ const X: u64 = Y; + │ ^^^^^^^^^^^^^^^^^ diff --git a/third_party/move/move-compiler-v2/tests/checking-lang-v1/v1-typing/refer_other_constants.move b/third_party/move/move-compiler-v2/tests/checking-lang-v1/v1-typing/refer_other_constants.move new file mode 100644 index 00000000000000..1200772c203233 --- /dev/null +++ b/third_party/move/move-compiler-v2/tests/checking-lang-v1/v1-typing/refer_other_constants.move @@ -0,0 +1,14 @@ +address 0x42 { +module M { + const X: u64 = Y; + const Y: u64 = 0; + + public fun get_x(): u64 { + X + } + + public fun get_y(): u64 { + Y + } +} +} diff --git a/third_party/move/move-compiler-v2/tests/checking/typing/recursive_constant.exp b/third_party/move/move-compiler-v2/tests/checking/typing/recursive_constant.exp index 6ec265a00b8452..efee9764a1f42b 100644 --- a/third_party/move/move-compiler-v2/tests/checking/typing/recursive_constant.exp +++ b/third_party/move/move-compiler-v2/tests/checking/typing/recursive_constant.exp @@ -1,7 +1,51 @@ Diagnostics: -error: Found recursive definition of a constant - ┌─ tests/checking/typing/recursive_constant.move:4:5 +error: Found recursive definition of a constant X + ┌─ tests/checking/typing/recursive_constant.move:3:5 │ +3 │ const X: u64 = Y; + │ ^^^^^^^^^^^^^^^^^ + │ │ + │ X is defined 4 │ const Y: u64 = X; + │ ----------------- Y is defined + +error: Found recursive definition of a constant F + ┌─ tests/checking/typing/recursive_constant.move:6:5 + │ +6 │ const F: u64 = F; │ ^^^^^^^^^^^^^^^^^ + │ │ + │ F is defined + +error: Invalid expression in `const`. Constant folding failed due to incomplete evaluation + ┌─ tests/checking/typing/recursive_constant.move:8:8 + │ +8 │ Z + A + │ ^^^^^ + +error: Invalid expression in `const`. Constant folding failed due to incomplete evaluation + ┌─ tests/checking/typing/recursive_constant.move:10:20 + │ +10 │ const A: u64 = B + C; + │ ^^^^^ + +error: Found recursive definition of a constant A + ┌─ tests/checking/typing/recursive_constant.move:10:5 + │ + 7 │ ╭ const X1: u64 = { + 8 │ │ Z + A + 9 │ │ }; + │ ╰──────' X1 is defined +10 │ const A: u64 = B + C; + │ ^^^^^^^^^^^^^^^^^^^^^ + │ │ + │ A is defined +11 │ const B: u64 = X1; + │ ------------------ B is defined + +error: Invalid expression in `const`. Constant folding failed due to incomplete evaluation + ┌─ tests/checking/typing/recursive_constant.move:12:20 + │ +12 │ const C: u64 = Z + B; + │ ^^^^^ diff --git a/third_party/move/move-compiler-v2/tests/checking/typing/recursive_constant.move b/third_party/move/move-compiler-v2/tests/checking/typing/recursive_constant.move index bf342fcfa62826..5e2c6c587e8fff 100644 --- a/third_party/move/move-compiler-v2/tests/checking/typing/recursive_constant.move +++ b/third_party/move/move-compiler-v2/tests/checking/typing/recursive_constant.move @@ -3,6 +3,13 @@ module M { const X: u64 = Y; const Y: u64 = X; const Z: u64 = 0; + const F: u64 = F; + const X1: u64 = { + Z + A + }; + const A: u64 = B + C; + const B: u64 = X1; + const C: u64 = Z + B; public fun get_x(): u64 { X diff --git a/third_party/move/move-compiler/src/expansion/ast.rs b/third_party/move/move-compiler/src/expansion/ast.rs index 77b9018c7498fd..880234d6fc7158 100644 --- a/third_party/move/move-compiler/src/expansion/ast.rs +++ b/third_party/move/move-compiler/src/expansion/ast.rs @@ -21,7 +21,7 @@ use move_binary_format::file_format; use move_ir_types::location::*; use move_symbol_pool::Symbol; use std::{ - collections::{BTreeMap, BTreeSet, HashSet, VecDeque}, + collections::{BTreeMap, BTreeSet, VecDeque}, fmt, hash::Hash, }; @@ -382,7 +382,7 @@ pub enum ModuleAccess_ { } impl ModuleAccess_ { - fn get_name(&self) -> &Name { + pub fn get_name(&self) -> &Name { match self { ModuleAccess_::Name(n) | ModuleAccess_::ModuleAccess(_, n, _) => n, } @@ -548,44 +548,6 @@ pub enum Exp_ { } pub type Exp = Spanned; -impl Exp_ { - /// Get all names from an expression - /// only perform on expression supported in constant definition. - pub fn get_names_for_const_exp(&self) -> HashSet { - let mut names = HashSet::new(); - let mut add_names = |v: &Exp| { - let set = v.value.get_names_for_const_exp(); - for n in set.iter() { - names.insert(*n); - } - }; - match self { - Self::Name(access, _) => { - names.insert(*access.value.get_name()); - }, - Self::Call(_, _, _, exp_vec) | Self::Vector(_, _, exp_vec) => { - let _ = exp_vec.value.iter().map(&mut add_names); - }, - Self::UnaryExp(_, exp) => { - add_names(exp); - }, - Self::BinopExp(exp1, _, exp2) => { - add_names(exp1); - add_names(exp2); - }, - Self::Block(seq) => { - for s in seq.iter() { - if let SequenceItem_::Seq(exp) = &s.value { - add_names(exp); - } - } - }, - _ => {}, - } - names - } -} - pub type Sequence = VecDeque; #[derive(Debug, Clone, PartialEq)] pub enum SequenceItem_ { diff --git a/third_party/move/move-model/src/builder/module_builder.rs b/third_party/move/move-model/src/builder/module_builder.rs index a62d58577d6df3..d657722b250316 100644 --- a/third_party/move/move-model/src/builder/module_builder.rs +++ b/third_party/move/move-model/src/builder/module_builder.rs @@ -891,10 +891,51 @@ impl<'env, 'translator> ModuleBuilder<'env, 'translator> { &mut self, key: &PA::ConstantName, constant_map: &UniqueMap, - visiting: &mut HashSet, - visited: &mut HashSet, + visiting: &mut Vec<(PA::ConstantName, Loc)>, // constants that are being traversed during dfs + visited: &mut HashSet, // constants that are already visited during dfs compiled_module: &Option, ) { + // Get all names from an expression + // only recursively check on expression types supported in constant definition. + fn get_names_from_const_exp(exp: &EA::Exp_) -> BTreeSet { + let mut names = BTreeSet::new(); + let mut add_names = |v: &EA::Exp| { + let set = get_names_from_const_exp(&v.value); + for n in set.iter() { + names.insert(*n); + } + }; + match exp { + EA::Exp_::Name(access, _) => { + names.insert(*access.value.get_name()); + }, + EA::Exp_::Call(_, _, _, exp_vec) | EA::Exp_::Vector(_, _, exp_vec) => { + exp_vec.value.iter().for_each(&mut add_names); + }, + EA::Exp_::UnaryExp(_, exp) => { + add_names(exp); + }, + EA::Exp_::BinopExp(exp1, _, exp2) => { + add_names(exp1); + add_names(exp2); + }, + EA::Exp_::Block(seq) => { + for s in seq.iter() { + match &s.value { + EA::SequenceItem_::Seq(exp) | EA::SequenceItem_::Bind(_, exp) => { + add_names(exp); + }, + _ => {}, + } + } + }, + _ => {}, + } + names + } + if visited.contains(key) { + return; + } let qsym = self.qualified_by_module_from_name(&key.0); let loc = self .parent @@ -903,15 +944,22 @@ impl<'env, 'translator> ModuleBuilder<'env, 'translator> { .expect("constant declared") .loc .clone(); - if visited.contains(key) { - return; - } - if visiting.contains(key) { + if let Some(index) = visiting.iter().position(|r| r.0 == *key) { + self.parent.env.diag_with_labels( + Severity::Error, + &loc, + &format!("Found recursive definition of a constant {}", key), + visiting[index..] + .to_vec() + .iter() + .map(|(name, loc)| (loc.clone(), format!("{} is defined", name))) + .collect_vec(), + ); return; } - visiting.insert(*key); + visiting.push((*key, loc.clone())); if let Some(exp) = constant_map.get(key) { - let names = exp.value.value.get_names_for_const_exp(); + let names = get_names_from_const_exp(&exp.value.value); for name in names { let const_name = PA::ConstantName(name); let qsym = self.qualified_by_module_from_name(&name); @@ -920,17 +968,11 @@ impl<'env, 'translator> ModuleBuilder<'env, 'translator> { } self.check_language_version( &loc, - "Referring to other constants", + "referring to other constants", LanguageVersion::V2_0, ); if visited.contains(&const_name) { - return; - } - if visiting.contains(&const_name) { - self.parent - .env - .error(&loc, "Found recursive definition of a constant"); - return; + continue; } self.analyze_constant( &const_name, @@ -941,9 +983,9 @@ impl<'env, 'translator> ModuleBuilder<'env, 'translator> { ); } self.def_ana_constant(key, exp, compiled_module); - visited.insert(*key); - visiting.remove(key); } + visited.insert(*key); + visiting.pop(); } fn analyze_constants( @@ -952,7 +994,7 @@ impl<'env, 'translator> ModuleBuilder<'env, 'translator> { compiled_module: &Option, ) { let mut visited = HashSet::new(); - let mut visiting = HashSet::new(); + let mut visiting = vec![]; for (name, _) in module_def.constants.key_cloned_iter() { self.analyze_constant( &name, diff --git a/third_party/move/move-model/src/constant_folder.rs b/third_party/move/move-model/src/constant_folder.rs index 1be67699a5eb35..4422bebfcca020 100644 --- a/third_party/move/move-model/src/constant_folder.rs +++ b/third_party/move/move-model/src/constant_folder.rs @@ -328,9 +328,17 @@ impl<'env> ConstantFolder<'env> { O::Neq => val0 .equivalent(val1) .map(|equivalence| V(id, Bool(!equivalence)).into_exp()), - _ => self.constant_folding_error(id, |_| { - "Unknown binary expression in `const`".to_owned() - }), + _ => { + if oper.is_binop() { + self.constant_folding_error(id, |_| { + "Constant folding failed due to incomplete evaluation".to_owned() + }) + } else { + self.constant_folding_error(id, |_| { + "Unknown binary expression in `const`".to_owned() + }) + } + }, } } } else {