handle comments

aptos-labs · Apr 10, 2024 · 541bc7b · 541bc7b
1 parent 7164b6a
commit 541bc7b
Show file tree

Hide file tree

Showing 15 changed files with 248 additions and 100 deletions.
diff --git a/third_party/move/move-compiler-v2/src/function_checker.rs b/third_party/move/move-compiler-v2/src/function_checker.rs
@@ -8,7 +8,7 @@ use codespan_reporting::diagnostic::Severity;
 use move_binary_format::file_format::Visibility;
 use move_model::{
     ast::{ExpData, Operation, Pattern},
-    model::{FunId, FunctionEnv, GlobalEnv, Loc, ModuleId, NodeId, QualifiedId},
+    model::{FunId, FunctionEnv, GlobalEnv, Loc, ModuleEnv, NodeId, QualifiedId},
     ty::Type,
 };
 use std::{collections::BTreeSet, iter::Iterator, vec::Vec};
@@ -57,16 +57,29 @@ pub fn check_for_function_typed_parameters(env: &mut GlobalEnv) {
     }
 }
 
-fn access_error(env: &GlobalEnv, fun_loc: &Loc, id: &NodeId, oper: &str, msg: String) {
+fn access_error(
+    env: &GlobalEnv,
+    fun_loc: &Loc,
+    id: &NodeId,
+    oper: &str,
+    msg: String,
+    module_env: &ModuleEnv,
+) {
     let call_details: Vec<_> = [*id]
         .iter()
         .map(|node_id| (env.get_node_loc(*node_id), format!("{} here", oper)))
         .collect();
+    let msg = format!(
+        "Invalid operation: {} can only be done within the defining module `{}`",
+        msg,
+        module_env.get_full_name_str()
+    );
     env.diag_with_labels(Severity::Error, fun_loc, &msg, call_details);
 }
 
-/// check a struct can only be accessed within the module that defines it.
-fn check_access_and_use_of_structs(env: &GlobalEnv, fun_env: &FunctionEnv) {
+/// check privileged operations on a struct such as storage operation, pack/unpack and field accesses
+/// can only be performed within the module that defines it.
+fn check_privileged_operations_on_structs(env: &GlobalEnv, fun_env: &FunctionEnv) {
     if let Some(fun_body) = fun_env.get_def() {
         let caller_module_id = fun_env.module_env.get_id();
         fun_body.visit_pre_order(&mut |exp: &ExpData| {
@@ -75,24 +88,27 @@ fn check_access_and_use_of_structs(env: &GlobalEnv, fun_env: &FunctionEnv) {
                     Operation::Exists(_)
                     | Operation::BorrowGlobal(_)
                     | Operation::MoveFrom
-                    | Operation::MoveTo
-                        if env.get_node_instantiation(*id)[0].get_struct(env).is_some() =>
-                    {
+                    | Operation::MoveTo => {
                         let inst = env.get_node_instantiation(*id);
-                        let (mid, sid, _) = inst[0].require_struct();
-                        if mid != caller_module_id {
-                            let qualified_struct_id = mid.qualified(sid);
-                            let struct_env = env.get_struct(qualified_struct_id);
-                            access_error(
-                                env,
-                                &fun_env.get_id_loc(),
-                                id,
-                                "called",
-                                format!(
-                                    "Invalid storage operation on external type `{}`",
-                                    struct_env.get_full_name_str()
-                                ),
-                            );
+                        debug_assert!(!inst.is_empty());
+                        if let Some((struct_env, _)) = inst[0].get_struct(env) {
+                            let mid = struct_env.module_env.get_id();
+                            let sid = struct_env.get_id();
+                            if mid != caller_module_id {
+                                let qualified_struct_id = mid.qualified(sid);
+                                let struct_env = env.get_struct(qualified_struct_id);
+                                access_error(
+                                    env,
+                                    &fun_env.get_id_loc(),
+                                    id,
+                                    "called",
+                                    format!(
+                                        "storage operation on type `{}`",
+                                        struct_env.get_full_name_str(),
+                                    ),
+                                    &struct_env.module_env,
+                                );
+                            }
                         }
                     },
                     Operation::Select(mid, sid, fid) if *mid != caller_module_id => {
@@ -104,10 +120,11 @@ fn check_access_and_use_of_structs(env: &GlobalEnv, fun_env: &FunctionEnv) {
                             id,
                             "accessed",
                             format!(
-                                "Invalid access of field `{}` on external type `{}`",
+                                "access of the field `{}` on type `{}`",
                                 fid.symbol().display(struct_env.symbol_pool()),
-                                struct_env.get_full_name_str()
+                                struct_env.get_full_name_str(),
                             ),
+                            &struct_env.module_env,
                         );
                     },
                     Operation::Pack(mid, sid) => {
@@ -119,22 +136,32 @@ fn check_access_and_use_of_structs(env: &GlobalEnv, fun_env: &FunctionEnv) {
                                 &fun_env.get_id_loc(),
                                 id,
                                 "packed",
-                                format!(
-                                    "Invalid pack operation on external type {}",
-                                    struct_env.get_full_name_str()
-                                ),
+                                format!("pack of `{}`", struct_env.get_full_name_str(),),
+                                &struct_env.module_env,
                             );
                         }
                     },
                     _ => {},
                 },
-                ExpData::Assign(_, pat, _) | ExpData::Block(_, pat, _, _) => {
-                    check_access_and_use_of_structs_in_pattern(
-                        env,
-                        &fun_env.get_id_loc(),
-                        &caller_module_id,
-                        pat,
-                    );
+                ExpData::Assign(_, pat, _)
+                | ExpData::Block(_, pat, _, _)
+                | ExpData::Lambda(_, pat, _) => {
+                    pat.visit_pre_post(&mut |_, pat| {
+                        if let Pattern::Struct(id, str, _) = pat {
+                            let module_id = str.module_id;
+                            if module_id != caller_module_id {
+                                let struct_env = env.get_struct(str.to_qualified_id());
+                                access_error(
+                                    env,
+                                    &fun_env.get_id_loc(),
+                                    id,
+                                    "unpacked",
+                                    format!("unpack of `{}`", struct_env.get_full_name_str(),),
+                                    &struct_env.module_env,
+                                );
+                            }
+                        }
+                    });
                 },
                 // access in specs is not restricted
                 ExpData::SpecBlock(_, _) => {
@@ -147,49 +174,14 @@ fn check_access_and_use_of_structs(env: &GlobalEnv, fun_env: &FunctionEnv) {
     }
 }
 
-fn check_access_and_use_of_structs_in_pattern(
-    env: &GlobalEnv,
-    fun_loc: &Loc,
-    mid: &ModuleId,
-    pat: &Pattern,
-) {
-    match pat {
-        Pattern::Struct(id, str, args) => {
-            let module_id = str.module_id;
-            if module_id != *mid {
-                let struct_env = env.get_struct(str.to_qualified_id());
-                access_error(
-                    env,
-                    fun_loc,
-                    id,
-                    "unpacked",
-                    format!(
-                        "Invalid unpack operation on external type {}",
-                        struct_env.get_full_name_str()
-                    ),
-                );
-            }
-            for pat in args {
-                check_access_and_use_of_structs_in_pattern(env, fun_loc, mid, pat);
-            }
-        },
-        Pattern::Tuple(_, pats) => {
-            for pat in pats {
-                check_access_and_use_of_structs_in_pattern(env, fun_loc, mid, pat);
-            }
-        },
-        _ => {},
-    }
-}
-
 /// For all function in target modules:
 ///
 /// If `before_inlining`, then
 /// - check that all function calls involving inline functions are accessible;
 /// - warn about unused private functions
 /// Otherwise  (`!before_inlining`):
 /// - check that all function calls *not* involving inline functions are accessible.
-/// - check structs are not accessed across the module boundary.
+/// - check privileged operations on structs cannot be done across module boundary
 pub fn check_access_and_use(env: &mut GlobalEnv, before_inlining: bool) {
     // For each function seen, we record whether it has an accessible caller.
     let mut functions_with_callers: BTreeSet<QualifiedFunId> = BTreeSet::new();
@@ -205,7 +197,7 @@ pub fn check_access_and_use(env: &mut GlobalEnv, before_inlining: bool) {
             let caller_module_is_script = caller_module.get_name().is_script();
             for caller_func in caller_module.get_functions() {
                 if !before_inlining {
-                    check_access_and_use_of_structs(env, &caller_func);
+                    check_privileged_operations_on_structs(env, &caller_func);
                 }
                 let caller_qfid = caller_func.get_qualified_id();
 

diff --git a/third_party/move/move-compiler-v2/tests/checking/inlining/resources_invalid.exp b/third_party/move/move-compiler-v2/tests/checking/inlining/resources_invalid.exp
@@ -1,6 +1,6 @@
 
 Diagnostics:
-error: Invalid access of field `addr` on external type `objects::ReaderRef`
+error: Invalid operation: access of the field `addr` on type `objects::ReaderRef` can only be done within the defining module `0x42::objects`
    ┌─ tests/checking/inlining/resources_invalid.move:17:16
    │
  8 │         borrow_global<T>(ref.addr)

diff --git a/third_party/move/move-compiler-v2/tests/checking/typing/borrow_field_internal.exp b/third_party/move/move-compiler-v2/tests/checking/typing/borrow_field_internal.exp
@@ -1,14 +1,14 @@
 
 Diagnostics:
-error: Invalid access of field `f` on external type `X::S`
+error: Invalid operation: access of the field `f` on type `X::S` can only be done within the defining module `0x2::X`
    ┌─ tests/checking/typing/borrow_field_internal.move:12:9
    │
 12 │     fun t0() {
    │         ^^
 13 │         (&X::s().f: &u64);
    │           -------- accessed here
 
-error: Invalid access of field `f` on external type `X::S`
+error: Invalid operation: access of the field `f` on type `X::S` can only be done within the defining module `0x2::X`
    ┌─ tests/checking/typing/borrow_field_internal.move:12:9
    │
 12 │     fun t0() {

diff --git a/third_party/move/move-compiler-v2/tests/checking/typing/global_builtins_script.exp b/third_party/move/move-compiler-v2/tests/checking/typing/global_builtins_script.exp
@@ -1,6 +1,6 @@
 
 Diagnostics:
-error: Invalid storage operation on external type `M::R`
+error: Invalid operation: storage operation on type `M::R` can only be done within the defining module `0x42::M`
    ┌─ tests/checking/typing/global_builtins_script.move:14:5
    │
 14 │ fun test<Token>(account: signer) {
@@ -9,7 +9,7 @@ error: Invalid storage operation on external type `M::R`
 16 │     borrow_global<M::R>(@0x1);
    │     ------------------------- called here
 
-error: Invalid storage operation on external type `M::R`
+error: Invalid operation: storage operation on type `M::R` can only be done within the defining module `0x42::M`
    ┌─ tests/checking/typing/global_builtins_script.move:14:5
    │
 14 │ fun test<Token>(account: signer) {

diff --git a/...arty/move/move-compiler-v2/tests/checking/typing/implicit_deref_borrow_field_internal.exp b/...arty/move/move-compiler-v2/tests/checking/typing/implicit_deref_borrow_field_internal.exp
@@ -1,14 +1,14 @@
 
 Diagnostics:
-error: Invalid access of field `f` on external type `X::S`
+error: Invalid operation: access of the field `f` on type `X::S` can only be done within the defining module `0x2::X`
    ┌─ tests/checking/typing/implicit_deref_borrow_field_internal.move:12:9
    │
 12 │     fun t0() {
    │         ^^
 13 │         (X::s().f: u64);
    │          -------- accessed here
 
-error: Invalid access of field `f` on external type `X::S`
+error: Invalid operation: access of the field `f` on type `X::S` can only be done within the defining module `0x2::X`
    ┌─ tests/checking/typing/implicit_deref_borrow_field_internal.move:12:9
    │
 12 │     fun t0() {

diff --git a/third_party/move/move-compiler-v2/tests/checking/typing/mutate_field_internal.exp b/third_party/move/move-compiler-v2/tests/checking/typing/mutate_field_internal.exp
@@ -1,14 +1,14 @@
 
 Diagnostics:
-error: Invalid access of field `f` on external type `X::S`
+error: Invalid operation: access of the field `f` on type `X::S` can only be done within the defining module `0x2::X`
    ┌─ tests/checking/typing/mutate_field_internal.move:12:9
    │
 12 │     fun t0() {
    │         ^^
 13 │         X::s().f = 0;
    │         -------- accessed here
 
-error: Invalid access of field `f` on external type `X::S`
+error: Invalid operation: access of the field `f` on type `X::S` can only be done within the defining module `0x2::X`
    ┌─ tests/checking/typing/mutate_field_internal.move:12:9
    │
 12 │     fun t0() {

diff --git a/third_party/move/move-compiler-v2/tests/testsuite.rs b/third_party/move/move-compiler-v2/tests/testsuite.rs
@@ -333,6 +333,7 @@ const TEST_CONFIGS: Lazy<BTreeMap<&str, TestConfig>> = Lazy::new(|| {
             include: vec!["/acquires-checker/"],
             exclude: vec![],
             exp_suffix: None,
+            // Skip access check to avoid error message change in the acquires-checker
             options: opts.clone().set_experiment(Experiment::ACCESS_CHECK, false),
             // Run the full compiler pipeline to double-check the result.
             stop_after: StopAfter::FileFormat,

diff --git a/third_party/move/move-compiler-v2/tests/visibility-checker/global_operator.exp b/third_party/move/move-compiler-v2/tests/visibility-checker/global_operator.exp
@@ -1,6 +1,6 @@
 
 Diagnostics:
-error: Invalid storage operation on external type `M::R`
+error: Invalid operation: storage operation on type `M::R` can only be done within the defining module `0x42::M`
    ┌─ tests/visibility-checker/global_operator.move:10:9
    │
 10 │     fun test<Token>(account: signer) {
@@ -9,7 +9,7 @@ error: Invalid storage operation on external type `M::R`
 12 │         borrow_global<M::R>(@0x1);
    │         ------------------------- called here
 
-error: Invalid storage operation on external type `M::R`
+error: Invalid operation: storage operation on type `M::R` can only be done within the defining module `0x42::M`
    ┌─ tests/visibility-checker/global_operator.move:10:9
    │
 10 │     fun test<Token>(account: signer) {
@@ -18,7 +18,7 @@ error: Invalid storage operation on external type `M::R`
 13 │         borrow_global_mut<M::R>(@0x1);
    │         ----------------------------- called here
 
-error: Invalid storage operation on external type `M::R`
+error: Invalid operation: storage operation on type `M::R` can only be done within the defining module `0x42::M`
    ┌─ tests/visibility-checker/global_operator.move:10:9
    │
 10 │     fun test<Token>(account: signer) {
@@ -27,7 +27,7 @@ error: Invalid storage operation on external type `M::R`
 14 │         exists<M::R>(@0x1);
    │         ------------------ called here
 
-error: Invalid storage operation on external type `M::R`
+error: Invalid operation: storage operation on type `M::R` can only be done within the defining module `0x42::M`
    ┌─ tests/visibility-checker/global_operator.move:10:9
    │
 10 │     fun test<Token>(account: signer) {
@@ -36,7 +36,7 @@ error: Invalid storage operation on external type `M::R`
 15 │         move_to(&account, r);
    │         -------------------- called here
 
-error: Invalid storage operation on external type `M::R`
+error: Invalid operation: storage operation on type `M::R` can only be done within the defining module `0x42::M`
    ┌─ tests/visibility-checker/global_operator.move:24:9
    │
 24 │     fun test<Token>(account: signer) {
@@ -45,7 +45,7 @@ error: Invalid storage operation on external type `M::R`
 26 │         borrow_global<M::R>(@0x1);
    │         ------------------------- called here
 
-error: Invalid storage operation on external type `M::R`
+error: Invalid operation: storage operation on type `M::R` can only be done within the defining module `0x42::M`
    ┌─ tests/visibility-checker/global_operator.move:24:9
    │
 24 │     fun test<Token>(account: signer) {
@@ -54,7 +54,7 @@ error: Invalid storage operation on external type `M::R`
 27 │         borrow_global_mut<M::R>(@0x1);
    │         ----------------------------- called here
 
-error: Invalid storage operation on external type `M::R`
+error: Invalid operation: storage operation on type `M::R` can only be done within the defining module `0x42::M`
    ┌─ tests/visibility-checker/global_operator.move:24:9
    │
 24 │     fun test<Token>(account: signer) {
@@ -63,7 +63,7 @@ error: Invalid storage operation on external type `M::R`
 28 │         exists<M::R>(@0x1);
    │         ------------------ called here
 
-error: Invalid storage operation on external type `M::R`
+error: Invalid operation: storage operation on type `M::R` can only be done within the defining module `0x42::M`
    ┌─ tests/visibility-checker/global_operator.move:24:9
    │
 24 │     fun test<Token>(account: signer) {