[move-compiler] add support for public(package) visibility.
#13408
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
tnowacki
requested review from
amnn,
rvantonder,
awelc,
tnowacki,
tzakian and
dariorussi
tnowacki
requested changes
Aug 15, 2023
There was a problem hiding this comment.
Awesome stuff! Thanks for working out the test generation too!
Requesting changes for:
- The implementation of
check_visibility_modifiersand the missing sub/secondary labels - Fixing the friend list generation
- A test that runs the code. This might require some edition support in the transactional test framework. We could maybe make it an argument to
init - A test that tests named packages, you will need to add move-cli tests for that
...nal-crates/move/move-compiler/tests/move_check/typing/module_public_package_featuregate.move
Outdated
Show resolved
Hide resolved
...l-crates/move/move-compiler/tests/move_2024/typing/module_call_visibility_mixed_invalid.move
Outdated
Show resolved
Hide resolved
external-crates/move/move-compiler/tests/move_2024/typing/module_call_visibility_friend.move
Outdated
Show resolved
Hide resolved
cgswords
force-pushed
the
cswords/public_package
branch
from
de1e5ff to
edaaa8f
Compare
cgswords
force-pushed
the
cswords/public_package
branch
from
edaaa8f to
86955ad
Compare
cgswords
force-pushed
the
cswords/public_package
branch
from
86955ad to
e08ff7e
Compare
tzakian
reviewed
Aug 21, 2023
external-crates/move/move-compiler/tests/move_check/parser/function_visibility_empty.exp
Outdated
Show resolved
Hide resolved
cgswords
force-pushed
the
cswords/public_package
branch
from
88563a5 to
dc717ee
Compare
cgswords
force-pushed
the
cswords/public_package
branch
from
dc717ee to
df7f9fc
Compare
This adds support for `public(package)` visibility, including feature gating it between Move 2024 edition. It also includes tests to ensure visibility and feature gate checking. Add new tests to the compiler, including expected results. [ ] protocol change [x] user-visible impact [ ] breaking change for a client SDKs [ ] breaking change for FNs (FN binary must upgrade) [ ] breaking change for validators or node operators (must upgrade binaries) [ ] breaking change for on-chain data layout [ ] necessitate either a data wipe or data migration
Update wording Co-authored-by: Tim Zakian <2895723+tzakian@users.noreply.github.com>
cgswords
force-pushed
the
cswords/public_package
branch
from
df7f9fc to
817d15b
Compare
tnowacki
reviewed
Aug 29, 2023
external-crates/move/testing-infra/transactional-test-runner/src/framework.rs
Show resolved
Hide resolved
external-crates/move/move-compiler/transactional-tests/tests/dependencies/public_package.move
Show resolved
Hide resolved
external-crates/move/move-compiler/transactional-tests/tests/dependencies/public_package.move
Outdated
Show resolved
Hide resolved
...e/move-compiler/transactional-tests/tests/dependencies/public_package_different_packages.exp
Show resolved
Hide resolved
.../move-compiler/transactional-tests/tests/dependencies/public_package_different_packages.move
Show resolved
Hide resolved
cgswords
force-pushed
the
cswords/public_package
branch
from
1aa5a1a to
fc20243
Compare
tnowacki
requested changes
Aug 29, 2023
There was a problem hiding this comment.
I'm fairly certain the collect::<UniqueMap<_,_>> added will crash the compiler, so let's add the test to be sure.
But as a style thing, I'd really rather not add the hidden unwrap, as I think it will just cause issues.
For the transactional test that feels unexpected, this is really more of an issue with how I setup package definitions, and I will fix it in another PR (so don't worry about it)
.../move-compiler/transactional-tests/tests/dependencies/public_package_different_packages.move
Show resolved
Hide resolved
tnowacki
approved these changes
Sep 7, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This adds support for
public(package)visibility, including feature gating it between Move 2024 edition. It also includes tests to ensure visibility and feature gate checking.Test Plan
Add new tests to the compiler, including expected results.
Type of Change (Check all that apply)
Release notes
Adds initial
public(package)support to Move 2024.alpha. Instead of explicitly listing friend modules, you can now make function definitions public across the entire package. They are publicly available for any modules in that package, but you still cannot call them outside of that package.