check recursive in constant definition

aptos-labs · Sep 24, 2024 · 73e3bf6 · 73e3bf6
1 parent 8689b9a
commit 73e3bf6
Show file tree

Hide file tree

Showing 6 changed files with 144 additions and 23 deletions.
diff --git a/third_party/move/move-compiler-v2/tests/checking/typing/constant_all_valid_types.exp b/third_party/move/move-compiler-v2/tests/checking/typing/constant_all_valid_types.exp
@@ -24,6 +24,9 @@ module 0x42::M {
     private fun t8(): vector<address> {
         [Address(Numerical(0000000000000000000000000000000000000000000000000000000000000000)), Address(Numerical(0000000000000000000000000000000000000000000000000000000000000001))]
     }
+    private fun t9(): u8 {
+        0
+    }
 } // end 0x42::M
 module <SELF>_0 {
     private fun t() {

diff --git a/third_party/move/move-compiler-v2/tests/checking/typing/constant_all_valid_types.move b/third_party/move/move-compiler-v2/tests/checking/typing/constant_all_valid_types.move
@@ -1,5 +1,6 @@
 address 0x42 {
 module M {
+    const B: u8 = C1;
     const C1: u8 = 0;
     const C2: u64 = 0;
     const C3: u128 = 0;
@@ -17,6 +18,7 @@ module M {
     fun t6(): vector<u8> { C6 }
     fun t7(): vector<u8> { C7 }
     fun t8(): vector<address> { C8 }
+    fun t9(): u8 { B }
 }
 }
 

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
@@ -1,9 +1,7 @@
-// -- Model dump before bytecode pipeline
-module 0x42::M {
-    public fun get_x(): u64 {
-        false
-    }
-    public fun get_y(): u64 {
-        false
-    }
-} // end 0x42::M
+
+Diagnostics:
+error: Found recursive definition of a constant
+  ┌─ tests/checking/typing/recursive_constant.move:4:5
+  │
+4 │     const Y: u64 = X;
+  │     ^^^^^^^^^^^^^^^^^
diff --git 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, VecDeque},
+    collections::{BTreeMap, BTreeSet, HashSet, VecDeque},
     fmt,
     hash::Hash,
 };
@@ -548,6 +548,44 @@ pub enum Exp_ {
 }
 pub type Exp = Spanned<Exp_>;
 
+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<Name> {
+        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<SequenceItem>;
 #[derive(Debug, Clone, PartialEq)]
 pub enum SequenceItem_ {

diff --git a/third_party/move/move-model/src/builder/exp_builder.rs b/third_party/move/move-model/src/builder/exp_builder.rs
@@ -176,15 +176,8 @@ impl<'env, 'translator, 'module_translator> ExpTranslator<'env, 'translator, 'mo
     }
 
     pub fn check_language_version(&self, loc: &Loc, feature: &str, version_min: LanguageVersion) {
-        if !self.env().language_version().is_at_least(version_min) {
-            self.env().error(
-                loc,
-                &format!(
-                    "not supported before language version `{}`: {}",
-                    version_min, feature
-                ),
-            )
-        }
+        self.parent
+            .check_language_version(loc, feature, version_min);
     }
 
     pub fn set_spec_block_map(&mut self, map: BTreeMap<EA::SpecId, EA::SpecBlock>) {

diff --git a/third_party/move/move-model/src/builder/module_builder.rs b/third_party/move/move-model/src/builder/module_builder.rs
@@ -52,7 +52,7 @@ use move_ir_types::{ast::ConstantName, location::Spanned};
 use regex::Regex;
 use std::{
     cell::RefCell,
-    collections::{BTreeMap, BTreeSet},
+    collections::{BTreeMap, BTreeSet, HashSet},
     default::Default,
     fmt,
 };
@@ -875,6 +875,95 @@ impl<'env, 'translator> ModuleBuilder<'env, 'translator> {
 /// # Definition Analysis
 
 impl<'env, 'translator> ModuleBuilder<'env, 'translator> {
+    pub fn check_language_version(&self, loc: &Loc, feature: &str, version_min: LanguageVersion) {
+        if !self.parent.env.language_version().is_at_least(version_min) {
+            self.parent.env.error(
+                loc,
+                &format!(
+                    "not supported before language version `{}`: {}",
+                    version_min, feature
+                ),
+            )
+        }
+    }
+
+    fn analyze_constant(
+        &mut self,
+        key: &PA::ConstantName,
+        constant_map: &UniqueMap<PA::ConstantName, EA::Constant>,
+        visiting: &mut HashSet<PA::ConstantName>,
+        visited: &mut HashSet<PA::ConstantName>,
+        compiled_module: &Option<BytecodeModule>,
+    ) {
+        let qsym = self.qualified_by_module_from_name(&key.0);
+        let loc = self
+            .parent
+            .const_table
+            .get(&qsym)
+            .expect("constant declared")
+            .loc
+            .clone();
+        if visited.contains(key) {
+            return;
+        }
+        if visiting.contains(key) {
+            return;
+        }
+        visiting.insert(*key);
+        if let Some(exp) = constant_map.get(key) {
+            let names = exp.value.value.get_names_for_const_exp();
+            for name in names {
+                let const_name = PA::ConstantName(name);
+                let qsym = self.qualified_by_module_from_name(&name);
+                if !self.parent.const_table.contains_key(&qsym) {
+                    continue;
+                }
+                self.check_language_version(
+                    &loc,
+                    "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;
+                }
+                self.analyze_constant(
+                    &const_name,
+                    constant_map,
+                    visiting,
+                    visited,
+                    compiled_module,
+                );
+            }
+            self.def_ana_constant(key, exp, compiled_module);
+            visited.insert(*key);
+            visiting.remove(key);
+        }
+    }
+
+    fn analyze_constants(
+        &mut self,
+        module_def: &EA::ModuleDefinition,
+        compiled_module: &Option<BytecodeModule>,
+    ) {
+        let mut visited = HashSet::new();
+        let mut visiting = HashSet::new();
+        for (name, _) in module_def.constants.key_cloned_iter() {
+            self.analyze_constant(
+                &name,
+                &module_def.constants,
+                &mut visiting,
+                &mut visited,
+                compiled_module,
+            );
+        }
+    }
+
     fn def_ana(
         &mut self,
         module_def: &EA::ModuleDefinition,
@@ -886,9 +975,7 @@ impl<'env, 'translator> ModuleBuilder<'env, 'translator> {
         }
 
         // Analyze all constants.
-        for (name, def) in module_def.constants.key_cloned_iter() {
-            self.def_ana_constant(&name, def, compiled_module);
-        }
+        self.analyze_constants(module_def, compiled_module);
 
         // Analyze all schemas. This must be done before other things because schemas need to be
         // ready for inclusion. We also must do this recursively, so use a visited set to detect