[Feature] [Compiler-V2] Package Visibility (#13680)

* extend parser * impl package visibility * bug fix * trivial: remove commented code * doc * doc * doc * rename * doc * doc * refactor * rename * linter * add tests * bug fix * update tests * refactor * remove package keyword * make package a contextual keyword * unreachable! to panic! * linter * err msg * typo * refactor * add tests * add tests * remove outdated exp file * format * linter * gate public(package) * add v1 exp * update test * update tests * revert * update doc * change is_target to is_primary target and impl of in_same_package * remove file * revert to is_target & bug fix & update tests * linter * add tests * switch to is_primary_target * renaming * format * remove in_same_package * format --------- Co-authored-by: Zekun Wang <[email protected]>
aptos-labs · Jul 3, 2024 · e599987 · e599987
1 parent 00f5930
commit e599987
Show file tree

Hide file tree

Showing 52 changed files with 699 additions and 40 deletions.
diff --git a/aptos-move/framework/aptos-framework/doc/code.md b/aptos-move/framework/aptos-framework/doc/code.md
@@ -612,9 +612,9 @@ package.
     // Checks for valid dependencies <b>to</b> other packages
     <b>let</b> allowed_deps = <a href="code.md#0x1_code_check_dependencies">check_dependencies</a>(addr, &pack);
 
-    // Check package against conflicts
+    // Check <b>package</b> against conflicts
     // To avoid prover compiler <a href="../../aptos-stdlib/../move-stdlib/doc/error.md#0x1_error">error</a> on <b>spec</b>
-    // the package need <b>to</b> be an immutable variable
+    // the <b>package</b> need <b>to</b> be an immutable variable
     <b>let</b> module_names = <a href="code.md#0x1_code_get_module_names">get_module_names</a>(&pack);
     <b>let</b> package_immutable = &<b>borrow_global</b>&lt;<a href="code.md#0x1_code_PackageRegistry">PackageRegistry</a>&gt;(addr).packages;
     <b>let</b> len = <a href="../../aptos-stdlib/../move-stdlib/doc/vector.md#0x1_vector_length">vector::length</a>(package_immutable);
@@ -688,8 +688,8 @@ package.
 
     <b>let</b> registry = <b>borrow_global_mut</b>&lt;<a href="code.md#0x1_code_PackageRegistry">PackageRegistry</a>&gt;(code_object_addr);
     <a href="../../aptos-stdlib/../move-stdlib/doc/vector.md#0x1_vector_for_each_mut">vector::for_each_mut</a>&lt;<a href="code.md#0x1_code_PackageMetadata">PackageMetadata</a>&gt;(&<b>mut</b> registry.packages, |pack| {
-        <b>let</b> package: &<b>mut</b> <a href="code.md#0x1_code_PackageMetadata">PackageMetadata</a> = pack;
-        package.upgrade_policy = <a href="code.md#0x1_code_upgrade_policy_immutable">upgrade_policy_immutable</a>();
+        <b>let</b> <b>package</b>: &<b>mut</b> <a href="code.md#0x1_code_PackageMetadata">PackageMetadata</a> = pack;
+        <b>package</b>.upgrade_policy = <a href="code.md#0x1_code_upgrade_policy_immutable">upgrade_policy_immutable</a>();
     });
 }
 </code></pre>
@@ -779,7 +779,7 @@ Checks whether a new package with given names can co-exist with old package.
 
 
 <pre><code><b>fun</b> <a href="code.md#0x1_code_check_coexistence">check_coexistence</a>(old_pack: &<a href="code.md#0x1_code_PackageMetadata">PackageMetadata</a>, new_modules: &<a href="../../aptos-stdlib/../move-stdlib/doc/vector.md#0x1_vector">vector</a>&lt;String&gt;) {
-    // The modules introduced by each package must not overlap <b>with</b> `names`.
+    // The modules introduced by each <b>package</b> must not overlap <b>with</b> `names`.
     <a href="../../aptos-stdlib/../move-stdlib/doc/vector.md#0x1_vector_for_each_ref">vector::for_each_ref</a>(&old_pack.modules, |old_mod| {
         <b>let</b> old_mod: &<a href="code.md#0x1_code_ModuleMetadata">ModuleMetadata</a> = old_mod;
         <b>let</b> j = 0;

diff --git a/third_party/move/move-compiler-v2/src/env_pipeline/lambda_lifter.rs b/third_party/move/move-compiler-v2/src/env_pipeline/lambda_lifter.rs
@@ -112,6 +112,7 @@ pub fn lift_lambdas(options: LambdaLiftingOptions, env: &mut GlobalEnv) {
                 fun_id.symbol(),
                 loc,
                 Visibility::Private,
+                false,
                 type_params,
                 params,
                 result_type,

diff --git a/third_party/move/move-compiler-v2/src/function_checker.rs b/third_party/move/move-compiler-v2/src/function_checker.rs
@@ -18,7 +18,7 @@ type QualifiedFunId = QualifiedId<FunId>;
 /// check that non-inline function parameters do not have function type.
 pub fn check_for_function_typed_parameters(env: &mut GlobalEnv) {
     for caller_module in env.get_modules() {
-        if caller_module.is_target() {
+        if caller_module.is_primary_target() {
             for caller_func in caller_module.get_functions() {
                 // Check that non-inline function parameters don't have function type
                 if !caller_func.is_inline() {
@@ -191,7 +191,7 @@ pub fn check_access_and_use(env: &mut GlobalEnv, before_inlining: bool) {
     let mut private_funcs: BTreeSet<QualifiedFunId> = BTreeSet::new();
 
     for caller_module in env.get_modules() {
-        if caller_module.is_target() {
+        if caller_module.is_primary_target() {
             let caller_module_id = caller_module.get_id();
             let caller_module_has_friends = !caller_module.has_no_friends();
             let caller_module_is_script = caller_module.get_name().is_script();
@@ -253,6 +253,36 @@ pub fn check_access_and_use(env: &mut GlobalEnv, before_inlining: bool) {
                                 Visibility::Friend => {
                                     if callee_func.module_env.has_friend(&caller_module_id) {
                                         true
+                                    } else if callee_func.has_package_visibility() {
+                                        if callee_func.module_env.self_address()
+                                            == caller_func.module_env.self_address()
+                                        {
+                                            // if callee is also a primary target, then they are in the same package
+                                            if callee_func.module_env.is_primary_target() {
+                                                // we should've inferred the friend declaration
+                                                panic!(
+                                                    "{} should have friend {}",
+                                                    callee_func.module_env.get_full_name_str(),
+                                                    caller_func.module_env.get_full_name_str()
+                                                );
+                                            } else {
+                                                call_package_fun_from_diff_package_error(
+                                                    env,
+                                                    sites,
+                                                    &caller_func,
+                                                    &callee_func,
+                                                );
+                                                false
+                                            }
+                                        } else {
+                                            call_package_fun_from_diff_addr_error(
+                                                env,
+                                                sites,
+                                                &caller_func,
+                                                &callee_func,
+                                            );
+                                            false
+                                        }
                                     } else {
                                         not_a_friend_error(env, sites, &caller_func, &callee_func);
                                         false
@@ -336,7 +366,13 @@ fn generic_error(
     let msg = format!(
         "{}function `{}` cannot be called from {}\
          because {}",
-        if callee.is_inline() { "inline " } else { "" },
+        if callee.is_inline() {
+            "inline "
+        } else if callee.has_package_visibility() {
+            "public(package) "
+        } else {
+            ""
+        },
         callee_name,
         called_from,
         why,
@@ -391,3 +427,23 @@ fn not_a_friend_error(
     );
     cannot_call_error(env, &why, sites, caller, callee);
 }
+
+fn call_package_fun_from_diff_package_error(
+    env: &GlobalEnv,
+    sites: &BTreeSet<NodeId>,
+    caller: &FunctionEnv,
+    callee: &FunctionEnv,
+) {
+    let why = "they are from different packages";
+    cannot_call_error(env, why, sites, caller, callee);
+}
+
+fn call_package_fun_from_diff_addr_error(
+    env: &GlobalEnv,
+    sites: &BTreeSet<NodeId>,
+    caller: &FunctionEnv,
+    callee: &FunctionEnv,
+) {
+    let why = "they are from different addresses";
+    cannot_call_error(env, why, sites, caller, callee);
+}
diff --git a/..._party/move/move-compiler-v2/tests/checking/visibility-checker/parser_package_keyword.exp b/..._party/move/move-compiler-v2/tests/checking/visibility-checker/parser_package_keyword.exp
@@ -0,0 +1,9 @@
+// -- Model dump before bytecode pipeline
+module 0x42::package {
+    friend fun package(package: u8): u8 {
+        {
+          let package: u8 = Add<u8>(package, 1);
+          package
+        }
+    }
+} // end 0x42::package
diff --git a/...party/move/move-compiler-v2/tests/checking/visibility-checker/parser_package_keyword.move b/...party/move/move-compiler-v2/tests/checking/visibility-checker/parser_package_keyword.move
@@ -0,0 +1,6 @@
+module 0x42::package {
+    public(package) fun package(package: u8): u8 {
+        let package = package + 1;
+        package
+    }
+}
diff --git a/.../move/move-compiler-v2/tests/visibility-checker/mix_friend_package_visibility_invalid.exp b/.../move/move-compiler-v2/tests/visibility-checker/mix_friend_package_visibility_invalid.exp
@@ -0,0 +1,23 @@
+
+Diagnostics:
+error: Cannot use both package and friend visibility in the same module
+  ┌─ tests/visibility-checker/mix_friend_package_visibility_invalid.move:7:3
+  │
+3 │         public(package) fun foo() {
+  │         --------------- package visibility declared here
+  ·
+7 │         public(friend) fun bar() {
+  │         ^^^^^^^^^^^^^^
+  │         │
+  │         friend visibility declared here
+
+error: Cannot use both package and friend visibility in the same module
+   ┌─ tests/visibility-checker/mix_friend_package_visibility_invalid.move:15:3
+   │
+15 │         public(friend) fun foo() {
+   │         ^^^^^^^^^^^^^^
+   │         │
+   │         friend visibility declared here
+   ·
+19 │         public(package) fun bar() {
+   │         --------------- package visibility declared here
diff --git a/...move/move-compiler-v2/tests/visibility-checker/mix_friend_package_visibility_invalid.move b/...move/move-compiler-v2/tests/visibility-checker/mix_friend_package_visibility_invalid.move
@@ -0,0 +1,23 @@
+address 0x42 {
+	module A {
+		public(package) fun foo() {
+
+		}
+
+		public(friend) fun bar() {
+
+		}
+	}
+}
+
+address 0x43 {
+	module A {
+		public(friend) fun foo() {
+
+		}
+
+		public(package) fun bar() {
+
+		}
+	}
+}
diff --git a/...ty/move/move-compiler-v2/tests/visibility-checker/mix_friend_package_visibility_valid.exp b/...ty/move/move-compiler-v2/tests/visibility-checker/mix_friend_package_visibility_valid.exp
@@ -0,0 +1,12 @@
+// -- Model dump before bytecode pipeline
+module 0x42::B {
+    friend fun foo() {
+        Tuple()
+    }
+} // end 0x42::B
+module 0x42::A {
+    friend fun foo() {
+        B::foo();
+        Tuple()
+    }
+} // end 0x42::A
diff --git a/...y/move/move-compiler-v2/tests/visibility-checker/mix_friend_package_visibility_valid.move b/...y/move/move-compiler-v2/tests/visibility-checker/mix_friend_package_visibility_valid.move
@@ -0,0 +1,13 @@
+// It's ok to use (public)friend and public(package) in different modules
+
+module 0x42::A {
+    public(friend) fun foo() {
+        0x42::B::foo();
+    }
+}
+
+module 0x42::B {
+    public(package) fun foo() {
+
+    }
+}
diff --git a/third_party/move/move-compiler-v2/tests/visibility-checker/package_visibility.exp b/third_party/move/move-compiler-v2/tests/visibility-checker/package_visibility.exp
@@ -0,0 +1,33 @@
+// -- Model dump before bytecode pipeline
+module 0x42::A {
+    friend fun bar() {
+        Tuple()
+    }
+    private fun foo() {
+        Tuple()
+    }
+} // end 0x42::A
+module 0x42::B {
+    use 0x42::A; // resolved as: 0x42::A
+    public fun bar() {
+        A::bar()
+    }
+    friend fun foo() {
+        A::bar()
+    }
+    private fun baz() {
+        A::bar()
+    }
+} // end 0x42::B
+module 0x42::C {
+    use 0x42::B; // resolved as: 0x42::B
+    public fun bar() {
+        B::foo()
+    }
+    friend fun foo() {
+        B::foo()
+    }
+    private fun baz() {
+        B::foo()
+    }
+} // end 0x42::C
diff --git a/third_party/move/move-compiler-v2/tests/visibility-checker/package_visibility.move b/third_party/move/move-compiler-v2/tests/visibility-checker/package_visibility.move
@@ -0,0 +1,36 @@
+module 0x42::A {
+    fun foo() {}
+    public(package) fun bar() {}
+}
+
+module 0x42::B {
+    use 0x42::A;
+
+    public(package) fun foo() {
+        A::bar()
+    }
+
+    public fun bar() {
+        A::bar()
+    }
+
+    fun baz() {
+        A::bar()
+    }
+}
+
+module 0x42::C {
+    use 0x42::B;
+
+    public(package) fun foo() {
+        B::foo()
+    }
+
+    public fun bar() {
+        B::foo()
+    }
+
+    fun baz() {
+        B::foo()
+    }
+}
diff --git a/...arty/move/move-compiler-v2/tests/visibility-checker/package_visibility_already_friend.exp b/...arty/move/move-compiler-v2/tests/visibility-checker/package_visibility_already_friend.exp
@@ -0,0 +1,29 @@
+// -- Model dump before bytecode pipeline
+module 0x42::C {
+    friend fun bar() {
+        Tuple()
+    }
+    friend fun foo() {
+        C::bar()
+    }
+} // end 0x42::C
+module 0x42::A {
+    friend fun bar() {
+        Tuple()
+    }
+    private fun foo() {
+        Tuple()
+    }
+} // end 0x42::A
+module 0x42::B {
+    use 0x42::A; // resolved as: 0x42::A
+    public fun bar() {
+        A::bar()
+    }
+    friend fun foo() {
+        A::bar()
+    }
+    private fun baz() {
+        A::bar()
+    }
+} // end 0x42::B
diff --git a/...rty/move/move-compiler-v2/tests/visibility-checker/package_visibility_already_friend.move b/...rty/move/move-compiler-v2/tests/visibility-checker/package_visibility_already_friend.move
@@ -0,0 +1,33 @@
+module 0x42::A {
+    // check we don't add duplicate `friend 0x42::B;`
+    // during the transformation
+    friend 0x42::B;
+    fun foo() {}
+    public(package) fun bar() {}
+}
+
+module 0x42::B {
+    use 0x42::A;
+
+    public(package) fun foo() {
+        A::bar()
+    }
+
+    public fun bar() {
+        A::bar()
+    }
+
+    fun baz() {
+        A::bar()
+    }
+}
+
+module 0x42::C {
+    public(package) fun foo() {
+        bar()
+    }
+
+    public(package) fun bar() {
+
+    }
+}
diff --git a/third_party/move/move-compiler-v2/tests/visibility-checker/package_visibility_cycle.exp b/third_party/move/move-compiler-v2/tests/visibility-checker/package_visibility_cycle.exp
@@ -0,0 +1,10 @@
+
+Diagnostics:
+error: invalid 'module' declaration
+  ┌─ tests/visibility-checker/package_visibility_cycle.move:3:9
+  │
+3 │         0x42::B::foo()
+  │         ^^^^^^^^^^^^ '0x42::B' uses '0x42::A'. This 'use' relationship creates a dependency cycle.
+  ·
+9 │         0x42::A::foo()
+  │         ------------ '0x42::A' uses '0x42::B'
diff --git a/third_party/move/move-compiler-v2/tests/visibility-checker/package_visibility_cycle.move b/third_party/move/move-compiler-v2/tests/visibility-checker/package_visibility_cycle.move
@@ -0,0 +1,11 @@
+module 0x42::A {
+    public(package) fun foo() {
+        0x42::B::foo()
+    }
+}
+
+module 0x42::B {
+    public(package) fun foo() {
+        0x42::A::foo()
+    }
+}