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

fEst1ck · 2024-06-13T02:13:58Z

Description

Implements the public(package) visibility modifier for functions, making the functions visible to the modules with the same address in the current package. Underlying, the package visibility public(package) is transformed into friend visibility public(friend). Then friend declarations are inferred when and only when necessary. Specifically, when module A uses a function with package visibility in module B, we declare module A as a friend in module B. We cannot simply declare a module containing a public(package) function as a friend to all other modules in the package. Because then whenever we have two or more modules with public(package) functions, a friend cycle would result.

As an example,

module 0x42::A {
    public(package) fun foo() {
        0x42::B::foo();
    }
}

module 0x42::B {
    public(package) fun bar() {}
}

is transformed into

module 0x42::A {
    public(friend) fun foo() {
        0x42::B::foo();
    }
}

module 0x42::B {
    friend 0x42::A;
    public(friend) fun bar() {}
}

Note that this transformation disallows the use of both friend and package visibility in the same module. Say we have

module 0x42::A {
friend 0x42::B
public(friend) foo;
public(package) bar;
}

module 0x42::B { ... }

module 0x42::C { ... }

if we transform it into

module 0x42::A {
friend 0x42::B;
friend 0x42::C;
public(friend) foo;
public(friend) bar;
}

module 0x42::B { ... }

module 0x42::C { ... }

This would be wrong because now foo is visible in module C.

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Existing tests and new tests under visibility-checker, third_party/move/tools/move-package/tests/test_sources/compilation/call_package_fun_from_other_package.

trunk-io · 2024-06-13T02:14:02Z

⏱️ 7h 24m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
rust-targeted-unit-tests	2h 7m	⬜ 🟥 🟥 ⬜ 🟥 (+3 more)
rust-move-tests	1h 11m	⬜ 🟩 🟩 🟩 🟩 (+4 more)
test-fuzzers	1h 9m	🟩 🟩
rust-move-unit-coverage	1h 8m	⬜ 🟩 🟩 🟩 🟩 (+3 more)
run-tests-main-branch	37m	🟩 🟩 🟩 🟩 🟩 (+3 more)
rust-lints	34m	⬜ 🟩 🟥 🟥 🟥 (+3 more)
general-lints	15m	🟩 🟩 🟩 🟩 🟩 (+3 more)
check-dynamic-deps	11m	🟩 🟩 🟩 🟩 🟩 (+4 more)
permission-check	6m	🟩 🟩 🟩 🟩 🟩 (+4 more)
semgrep/ci	4m	🟩 🟩 🟩 🟩 🟩 (+4 more)
file_change_determinator	2m	🟩 🟩 🟩 🟩 🟩 (+4 more)
file_change_determinator	1m	🟩 🟩 🟩 🟩 🟩 (+4 more)
permission-check	32s	🟩 🟩 🟩 🟩 🟩 (+4 more)
permission-check	28s	🟩 🟩 🟩 🟩 🟩 (+4 more)
permission-check	25s	🟩 🟩 🟩 🟩 🟩 (+4 more)

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

fEst1ck · 2024-06-13T02:59:32Z

third_party/move/move-model/src/builder/module_builder.rs

@@ -471,6 +482,23 @@ impl<'env, 'translator> ModuleBuilder<'env, 'translator> {
            )
        }
        let fun_id = FunId::new(qsym.symbol);
+        let visibility = match def.visibility {


Moved here to avoid borrowing issues.

third_party/move/move-compiler-v2/tests/visibility-checker/package_visibility_diff_addr.exp

@@ -0,0 +1,10 @@
+
+Diagnostics:
+error: friend modules of `0x42::A` must have the same address, but the declared friend module `0x43::B` has a different address


rahxephon89 · 2024-06-13T18:53:15Z

@fEst1ck, could you add some tests to the V1 compiler crate to show error messages that will be generated by V1 when compiling the code with this new feature?

wrwg

Some first comments

wrwg · 2024-06-17T16:34:19Z

third_party/move/move-compiler/src/parser/keywords.rs

@@ -22,6 +22,7 @@ pub const KEYWORDS: &[&str] = &[
    "module",
    "move",
    "native",
+    "package",


Please do not make this a keyword, we do not have breaking changes in Move 2.

Instead use a 'soft' keyword, that is interpreting an identifier in a special way in a given context. There are multiple examples for this already in syntax.rs, search e.g. for "schema" or check "enum" in the enum PR.

wrwg · 2024-06-17T16:36:54Z

third_party/move/move-compiler/src/to_bytecode/translate.rs

@@ -618,6 +618,7 @@ fn visibility(v: Visibility) -> IR::FunctionVisibility {
        Visibility::Public(_) => IR::FunctionVisibility::Public,
        Visibility::Friend(_) => IR::FunctionVisibility::Friend,
        Visibility::Internal => IR::FunctionVisibility::Internal,
+        _ => unreachable!(),


Please panic with an error message 'unexpected visibility'. unreachable! should only be used if its clearly never failing, e.g. if you made a check just before in the local code (if x.is_some() { let Some(y) = x else { unreachable!() }})

fEst1ck · 2024-06-18T08:42:12Z

third_party/move/move-model/src/model.rs

@@ -2634,6 +2633,13 @@ impl<'env> ModuleEnv<'env> {
        self.env.file_id_is_primary_target.contains(&file_id)
    }

+    /// Returns true if both modules are defined in the same package.
+    pub fn in_same_package(&self, other: &'env Self) -> bool {


Submitted issue #13745 for this.

As said above, I think we only need to distinguish whether a module is part of the currently compiled package or not. And this is defined by is_primary_target (is_target was recently renamed as such, which was an unfortunate decision because it already created a lot of confusion).

fEst1ck · 2024-06-20T17:30:19Z

@fEst1ck, could you add some tests to the V1 compiler crate to show error messages that will be generated by V1 when compiling the code with this new feature?

done.

brmataptos · 2024-06-26T21:37:54Z

aptos-move/framework/aptos-framework/doc/code.md

    // 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


Not sure if this was a manual change or auto-generated, but "need" should be changed to "needs" to have the grammar be correct.

The problem is that we seem to identify package as a kind of keyword. This isn't good as it is potentially breaking.

There shouldn't be any changes for generated documentation.

package is should in boldface because it's now a contextual keyword.

github-actions · 2024-07-03T04:17:24Z

✅ Forge suite `compat` success on `1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5` ==> `ee3499c79f069c56aaceb58cbb0a90a84f50f5a0`

Compatibility test results for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> ee3499c79f069c56aaceb58cbb0a90a84f50f5a0 (PR)
1. Check liveness of validators at old version: 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5
compatibility::simple-validator-upgrade::liveness-check : committed: 9876.693646968446 txn/s, latency: 2975.96932770948 ms, (p50: 2600 ms, p90: 3300 ms, p99: 11100 ms), latency samples: 388820
2. Upgrading first Validator to new version: ee3499c79f069c56aaceb58cbb0a90a84f50f5a0
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 3158.2836516355783 txn/s, latency: 8280.53706448258 ms, (p50: 9100 ms, p90: 11400 ms, p99: 11800 ms), latency samples: 76920
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 3265.168027969696 txn/s, latency: 9514.563593363411 ms, (p50: 9400 ms, p90: 14800 ms, p99: 15100 ms), latency samples: 137420
3. Upgrading rest of first batch to new version: ee3499c79f069c56aaceb58cbb0a90a84f50f5a0
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 3126.7384938455907 txn/s, latency: 8331.173365712786 ms, (p50: 9200 ms, p90: 10200 ms, p99: 10900 ms), latency samples: 76180
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 3218.9178234425067 txn/s, latency: 9603.695330428467 ms, (p50: 9500 ms, p90: 14800 ms, p99: 15300 ms), latency samples: 137700
4. upgrading second batch to new version: ee3499c79f069c56aaceb58cbb0a90a84f50f5a0
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 453.3835606870242 txn/s, submitted: 730.05759921735 txn/s, failed submission: 71.46784428521765 txn/s, expired: 276.67403853032573 txn/s, latency: 33803.22008646491 ms, (p50: 38900 ms, p90: 57200 ms, p99: 58200 ms), latency samples: 30070
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 5960.2903571409315 txn/s, latency: 5160.274971878515 ms, (p50: 5100 ms, p90: 6400 ms, p99: 8400 ms), latency samples: 231140
5. check swarm health
Compatibility test for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> ee3499c79f069c56aaceb58cbb0a90a84f50f5a0 passed
Test Ok

github-actions · 2024-07-03T04:17:54Z

✅ Forge suite `realistic_env_max_load` success on `ee3499c79f069c56aaceb58cbb0a90a84f50f5a0`

two traffics test: inner traffic : committed: 8654.675868007393 txn/s, latency: 4517.672176202008 ms, (p50: 3900 ms, p90: 6600 ms, p99: 13100 ms), latency samples: 3749560
two traffics test : committed: 99.96009309825766 txn/s, latency: 1943.8677777777777 ms, (p50: 1900 ms, p90: 2200 ms, p99: 4400 ms), latency samples: 1800
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.227, avg: 0.216", "QsPosToProposal: max: 0.112, avg: 0.106", "ConsensusProposalToOrdered: max: 0.318, avg: 0.291", "ConsensusOrderedToCommit: max: 0.404, avg: 0.388", "ConsensusProposalToCommit: max: 0.696, avg: 0.679"]
Max round gap was 1 [limit 4] at version 1872442. Max no progress secs was 5.077172 [limit 15] at version 1872442.
Test Ok

github-actions · 2024-07-03T04:18:40Z

✅ Forge suite `framework_upgrade` success on `1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5` ==> `ee3499c79f069c56aaceb58cbb0a90a84f50f5a0`

Compatibility test results for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> ee3499c79f069c56aaceb58cbb0a90a84f50f5a0 (PR)
Upgrade the nodes to version: ee3499c79f069c56aaceb58cbb0a90a84f50f5a0
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1218.3694808484738 txn/s, submitted: 1220.8633521341706 txn/s, failed submission: 2.4938712856965415 txn/s, expired: 2.4938712856965415 txn/s, latency: 2506.037532564198 ms, (p50: 1800 ms, p90: 4500 ms, p99: 8700 ms), latency samples: 107480
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1158.0972215324773 txn/s, submitted: 1160.3031210020629 txn/s, failed submission: 2.205899469585671 txn/s, expired: 2.205899469585671 txn/s, latency: 2547.903152380952 ms, (p50: 1800 ms, p90: 4800 ms, p99: 9300 ms), latency samples: 105000
5. check swarm health
Compatibility test for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> ee3499c79f069c56aaceb58cbb0a90a84f50f5a0 passed
Upgrade the remaining nodes to version: ee3499c79f069c56aaceb58cbb0a90a84f50f5a0
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1190.7905650289324 txn/s, submitted: 1193.7448164307218 txn/s, failed submission: 2.954251401789336 txn/s, expired: 2.954251401789336 txn/s, latency: 2585.578673664122 ms, (p50: 1800 ms, p90: 4700 ms, p99: 9300 ms), latency samples: 104800
Test Ok

fEst1ck force-pushed the package-visibility branch from e818b84 to b719d56 Compare June 13, 2024 02:53

fEst1ck commented Jun 13, 2024

View reviewed changes

fEst1ck self-assigned this Jun 13, 2024

fEst1ck added the compiler-v2 label Jun 13, 2024

fEst1ck requested review from vineethk, brmataptos and rahxephon89 June 13, 2024 03:15

fEst1ck marked this pull request as ready for review June 13, 2024 03:16

fEst1ck requested review from davidiw, movekevin and wrwg as code owners June 13, 2024 03:16

fEst1ck commented Jun 13, 2024

View reviewed changes

fEst1ck marked this pull request as draft June 14, 2024 16:53

wrwg reviewed Jun 17, 2024

View reviewed changes

fEst1ck force-pushed the package-visibility branch from 5847d04 to d2d9e70 Compare June 18, 2024 08:28

fEst1ck requested review from movekevin and removed request for davidiw and movekevin June 18, 2024 08:29

fEst1ck commented Jun 18, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

fEst1ck force-pushed the package-visibility branch from a88ac82 to 28e99fb Compare June 20, 2024 07:11

fEst1ck marked this pull request as ready for review June 20, 2024 07:12

fEst1ck force-pushed the package-visibility branch from bf3fd5d to 451e4be Compare June 20, 2024 17:25

brmataptos approved these changes Jun 26, 2024

View reviewed changes

sausagee added stale-exempt Prevents issues from being automatically marked and closed as stale and removed stale-exempt Prevents issues from being automatically marked and closed as stale compiler-v2 labels Jun 27, 2024

Zekun Wang added 12 commits July 2, 2024 20:54

update tests

3c2ef8b

revert

154c0fa

update doc

dc50c0b

change is_target to is_primary target and impl of in_same_package

b237dc1

remove file

232dc9a

revert to is_target & bug fix & update tests

1841ad7

linter

b4a8e33

add tests

8756217

switch to is_primary_target

3683571

renaming

a3f1914

format

b4620c7

remove in_same_package

5307aa2

fEst1ck force-pushed the package-visibility branch from 8f5cced to 5307aa2 Compare July 3, 2024 00:54

format

8dcd6f6

fEst1ck requested a review from wrwg July 3, 2024 00:55

fEst1ck enabled auto-merge (squash) July 3, 2024 00:56

This comment has been minimized.

Sign in to view

Merge branch 'main' into package-visibility

ee3499c

This comment has been minimized.

Sign in to view

fEst1ck merged commit e599987 into aptos-labs:main Jul 3, 2024
44 of 47 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

fEst1ck commented Jun 13, 2024 •

edited

Loading

trunk-io bot commented Jun 13, 2024 •

edited

Loading

fEst1ck Jun 13, 2024

This comment was marked as outdated.

rahxephon89 commented Jun 13, 2024

wrwg left a comment

wrwg Jun 17, 2024

fEst1ck Jun 18, 2024

wrwg Jun 17, 2024

fEst1ck Jun 18, 2024

fEst1ck Jun 18, 2024

wrwg Jun 29, 2024

This comment was marked as outdated.

fEst1ck commented Jun 20, 2024

brmataptos Jun 26, 2024

wrwg Jun 29, 2024

fEst1ck Jul 2, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jul 3, 2024

github-actions bot commented Jul 3, 2024

github-actions bot commented Jul 3, 2024

		@@ -0,0 +1,10 @@

		Diagnostics:
		error: friend modules of `0x42::A` must have the same address, but the declared friend module `0x43::B` has a different address

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

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

Conversation

fEst1ck commented Jun 13, 2024 • edited Loading

Description

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

trunk-io bot commented Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

This comment was marked as outdated.

rahxephon89 commented Jun 13, 2024

wrwg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

fEst1ck commented Jun 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jul 3, 2024

✅ Forge suite compat success on 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> ee3499c79f069c56aaceb58cbb0a90a84f50f5a0

github-actions bot commented Jul 3, 2024

✅ Forge suite realistic_env_max_load success on ee3499c79f069c56aaceb58cbb0a90a84f50f5a0

github-actions bot commented Jul 3, 2024

✅ Forge suite framework_upgrade success on 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> ee3499c79f069c56aaceb58cbb0a90a84f50f5a0

fEst1ck commented Jun 13, 2024 •

edited

Loading

trunk-io bot commented Jun 13, 2024 •

edited

Loading

✅ Forge suite `compat` success on `1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5` ==> `ee3499c79f069c56aaceb58cbb0a90a84f50f5a0`

✅ Forge suite `realistic_env_max_load` success on `ee3499c79f069c56aaceb58cbb0a90a84f50f5a0`

✅ Forge suite `framework_upgrade` success on `1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5` ==> `ee3499c79f069c56aaceb58cbb0a90a84f50f5a0`