Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[move-compiler] Add public struct type support to the parser #13917

Merged
merged 1 commit into from
Sep 27, 2023

Conversation

tzakian
Copy link
Contributor

@tzakian tzakian commented Sep 21, 2023

Description

Add support for public struct declarations in the Move 2024 edition.

To my knowledge this is the first backwards incompatible change, so we need to split off a separate version of the move-stdlib that tracks the new 2024 edition as well.

The bottom commit of this PR does this, and also refactors the test runner so we aren't passing so many fields all the time in the test runner.

Test Plan

Added additional tests.


If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process.

Type of Change (Check all that apply)

  • protocol change
  • 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

Release notes

Adds initial public struct type visibility support to Move 2024.alpha. When using the Move 2024.alpha edition you will be required to write public in front of the struct type which will keep the same semantics as struct with no visibility modifiers today (i.e., in the legacy edition), this is to allow for future support of private struct types.

@vercel
Copy link

vercel bot commented Sep 21, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
mysten-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 27, 2023 10:25pm
sui-typescript-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 27, 2023 10:25pm
3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
explorer ⬜️ Ignored (Inspect) Visit Preview Sep 27, 2023 10:25pm
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Sep 27, 2023 10:25pm
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Sep 27, 2023 10:25pm

Copy link
Contributor

@tnowacki tnowacki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To my knowledge this is the first backwards incompatible change, so we need to split off a separate version of the move-stdlib that tracks the new 2024 edition as well.

It is indeed neat to do so, but we don't have to split it off, and it might be worth not doing so to make sure we aren't breaking the compiler. Thoughts?

We would just need to change the tests to stick the stdlib stuff in its own package with its own package config settings

Comment on lines -72 to -74
false
} else {
true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧠

Comment on lines 2180 to 2193
let current_package = context.package_name;
match visibility {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, maybe check the feature gate at the top level here, as I think this is a bit strange as is

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking behind this was to only mention the feature gate if there's a public struct but not if, e.g., someone wrote public(package) struct or something like that. But, happy to move it up to the top level as well :)

external-crates/move/move-compiler/src/parser/syntax.rs Outdated Show resolved Hide resolved
external-crates/move/move-compiler/src/parser/syntax.rs Outdated Show resolved Hide resolved
external-crates/move/move-compiler/src/parser/syntax.rs Outdated Show resolved Hide resolved
@@ -0,0 +1,10 @@
[package]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, neat! This should be fun :)

@tzakian
Copy link
Contributor Author

tzakian commented Sep 25, 2023

It is indeed neat to do so, but we don't have to split it off, and it might be worth not doing so to make sure we aren't breaking the compiler. Thoughts?

We would just need to change the tests to stick the stdlib stuff in its own package with its own package config settings

I like this idea! Let me see about updating it so we compile the stdlib in legacy mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants