[move-compiler] Add positional struct fields #14073

tzakian · 2023-10-03T17:26:53Z

Description

Adds support for positional struct fields in Move 2024.

This affects the parser, expansion, and naming (and a minor update is needed in to_bytecode).

Parser

Updated to support positional struct declarations
Updated dotted field accesses to support valid struct indices
Updated binding parsing to support positional struct destructuring

Expansion

Added support for conversion of a Call to Unpack if it's in an LValue position (similar to what we do in expansion for Pack to Unpack in LValue position).

Naming

Positional fields are removed at this point
Calls are disambiguated into Packs or remain as Calls based on the name references an in-scope struct type.
We enforce that named packs/unpacks and positional packs/unpacks correspond to a named/positional struct declaration.

Test Plan

Added a number of tests for each phase.

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 positional struct declaration support to Move 2024.alpha. When using the 2024 edition you'll be able to declare structs with positional fields (e.g., public struct Positional(u64, bool)), access positional struct fields via their index (e.g., x.0 ,x.1) and unpack/pack them positionally (e.g., let Positional(value, is_tf) = x;, let x = Positional(0, true)).

vercel · 2023-10-03T17:27:02Z

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	Oct 5, 2023 6:54pm
sui-typescript-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 5, 2023 6:54pm

3 Ignored Deployments

Name	Status	Preview	Updated (UTC)
explorer	⬜️ Ignored (Inspect)	Visit Preview	Oct 5, 2023 6:54pm
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	Oct 5, 2023 6:54pm
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	Oct 5, 2023 6:54pm

tnowacki

Not sure if I missed them, but are there tests with explicit type args? e.g. Cup<u64>(0)

My usual rundown for those would be

Pack
Unpack
Pack with space (should fail), e.g. Cup <u64>(0)
Unpack with space (should fail), e.g. Cup <u64>(0)

tnowacki · 2023-10-03T21:11:46Z

external-crates/move/move-compiler/src/parser/ast.rs

+                    st.ast_debug(w);
+                });
+            }),
+            StructFields::Native(_) => unreachable!(),


This shouldn't be unreachable?

Ah good point -- should just do nothing. Will fix

tnowacki · 2023-10-03T21:20:05Z

external-crates/move/move-compiler/src/parser/syntax.rs

+        if next_tok != Tok::LBrace
+            && next_tok != Tok::Less
+            && next_tok != Tok::ColonColon
+            && next_tok != Tok::LParen


Suggested change

if next_tok != Tok::LBrace

&& next_tok != Tok::Less

&& next_tok != Tok::ColonColon

&& next_tok != Tok::LParen

if !matches!(next_tok, Tok::LBrace | Tok::Less | Tok::ColonColon | Tok::LParen) {

tnowacki · 2023-10-03T21:22:13Z

external-crates/move/move-compiler/src/parser/syntax.rs

+                        ) =>
+                    {
+                        let contents = context.tokens.content();
+                        if parse_u8(contents).is_err() {


I'm assuming this is the current max number of fields?

I don't think we have a hard limit in the compiler, but the bytecode verifier will cap these at 200, so bounding it to 255 seems like a nice bound.

tnowacki · 2023-10-03T21:22:55Z

external-crates/move/move-compiler/src/parser/syntax.rs

+                        let field_access = Name::new(loc, contents.into());
+                        Exp_::Dot(Box::new(lhs), field_access)
+                    }
+                    _ => {


Could do matches above if you want to unindent one level. This is fine too though :)

tnowacki · 2023-10-03T21:27:59Z

external-crates/move/move-compiler/src/expansion/translate.rs

+                    E::FieldBindings::Named(fields)
+                }
+                FieldBindings::Positional(positional_bindings) => {
+                    let fields: Option<Vec<E::LValue>> = positional_bindings


This is good for now, Just wondering if we should maybe replace Nones on each inner binding with some sort of UnresolvedError binding

tnowacki · 2023-10-03T21:38:31Z

external-crates/move/move-compiler/src/to_bytecode/translate.rs

@@ -787,7 +787,13 @@ fn var(v: Var) -> IR::Var {
 }

 fn field(f: Field) -> IR::Field {
-    sp(f.0.loc, IR::Field_(f.0.value))
+    // If it's a positional field, lower it into `pos{field_idx}` so they're a valid identifier
+    let field_ident = if f.0.value.parse::<u8>().is_ok() {


Is f.0.value.starts_with('0'..'9') not sufficient?

This seems fine... just something about parsing it scares me lol

tnowacki · 2023-10-03T21:41:59Z

external-crates/move/move-compiler/tests/move_check/parser/positional_struct_declaration.exp

@@ -0,0 +1,8 @@
+error[E13001]: feature is not supported in specified edition


Do we need all of these parser tests duplicated for legacy?

Nope. Will remove

tnowacki · 2023-10-03T21:51:01Z

external-crates/move/move-compiler/tests/move_2024/naming/positional_struct_lhs_unpack.exp

+28 │         let Foo() = x;
+   │             ^^^^^ Missing binding for field '0' in '0x42::M::Foo'


These errors are mildly hilarious, but good enough for now

tnowacki · 2023-10-03T21:55:26Z

...-crates/move/move-compiler/tests/move_2024/parser/positional_field_access_no_annotations.exp

+6 │         y.0_u8
+  │           ^^^^ Invalid field access. Expected a number less than or equal to 255
+  │
+  = Positional fields must be in the range [0 .. 255] and not be typed, e.g. `0_u8`


Maybe this should be , e.g. 0but not0_u8`?

tnowacki · 2023-10-03T21:56:01Z

...-crates/move/move-compiler/tests/move_2024/parser/positional_field_access_no_annotations.exp

+error[E03010]: unbound field
+  ┌─ tests/move_2024/parser/positional_field_access_no_annotations.move:6:9
+  │
+6 │         y.0_u8


Do we have a test for y.1_0 instead of y.10?
What about leading zeros? y.0001?

We do not. But I will add them. They should "just work" since the field parsing for these is the same function that we use to parse numbers normally.

Should we support this though? It feels a bit ... strange? Or maybe we just say fuck it and have the autoformatter replace them to something sane?

tnowacki

Thanks for doing this! And glad by the looks of it turned out to be relatively easy :)

tzakian · 2023-10-04T18:15:01Z

Thanks for the comments! Should be updated now. Added tests for the explicit type arguments.

Note however, that for unpacks things like let Foo <u64>(x) = ... are accepted, but this is in-line with what we currently accept for named unpacks today -- let Bar <u64> { x } = ... is also accepted.

tnowacki · 2023-10-04T18:17:07Z

Thanks for the comments! Should be updated now. Added tests for the explicit type arguments.

Note however, that for unpacks things like let Foo <u64>(x) = ... are accepted, but this is in-line with what we currently accept for named unpacks today -- let Bar <u64> { x } = ... is also accepted.

But not accepted at unpack assignment? Foo <u64>(x) = e?

tnowacki · 2023-10-04T18:17:32Z

Oh? But I removed that nonsense I think (It was a thing long ago)

tnowacki

🎉
This has been on my todo list since the first version of the compiler. So thanks!

tnowacki · 2023-10-04T18:30:51Z

We could also maybe check that if all fields are pos0...posn with no gaps, that we convert back to positional, when doing the interface generator. But also fine if we don't do that :)

tzakian · 2023-10-05T15:11:42Z

I think it's fine if we don't do it for interface generation (at least until we have public struct fields) since those fields aren't publicly visible so I'll leave it as-is :)

tzakian requested review from cgswords and tnowacki October 3, 2023 17:26

tnowacki reviewed Oct 3, 2023

View reviewed changes

tnowacki mentioned this pull request Oct 3, 2023

Move Edition 2024: New Language Features! #14062

Open

tzakian force-pushed the tzakian/positional-structs branch from 708c748 to db37b4a Compare October 4, 2023 18:13

vercel bot deployed to Preview – sui-typescript-docs October 4, 2023 18:14 View deployment

vercel bot temporarily deployed to Preview – mysten-ui October 4, 2023 18:14 Inactive

tnowacki approved these changes Oct 4, 2023

View reviewed changes

[move-compiler] Add positional struct fields

4130991

tzakian enabled auto-merge (squash) October 5, 2023 15:17

tzakian force-pushed the tzakian/positional-structs branch from db37b4a to 91a9944 Compare October 5, 2023 15:26

Update based on comments

241a88f

tzakian force-pushed the tzakian/positional-structs branch from 91a9944 to 241a88f Compare October 5, 2023 18:53

vercel bot deployed to Preview – sui-typescript-docs October 5, 2023 18:54 View deployment

vercel bot temporarily deployed to Preview – mysten-ui October 5, 2023 18:54 Inactive

tzakian merged commit ff37443 into main Oct 5, 2023

tzakian deleted the tzakian/positional-structs branch October 5, 2023 19:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[move-compiler] Add positional struct fields #14073

[move-compiler] Add positional struct fields #14073

tzakian commented Oct 3, 2023

vercel bot commented Oct 3, 2023 •

edited

Loading

tnowacki left a comment

tnowacki Oct 3, 2023

tzakian Oct 4, 2023

tnowacki Oct 3, 2023

tnowacki Oct 3, 2023

tzakian Oct 4, 2023

tnowacki Oct 3, 2023

tnowacki Oct 3, 2023

tnowacki Oct 3, 2023

tnowacki Oct 3, 2023

tzakian Oct 4, 2023

tnowacki Oct 3, 2023

tnowacki Oct 3, 2023

tnowacki Oct 3, 2023

tzakian Oct 4, 2023

tnowacki Oct 4, 2023

tnowacki left a comment

tzakian commented Oct 4, 2023 •

edited

Loading

tnowacki commented Oct 4, 2023

tnowacki commented Oct 4, 2023

tnowacki left a comment

tnowacki commented Oct 4, 2023

tzakian commented Oct 5, 2023 •

edited

Loading

		@@ -0,0 +1,8 @@
		error[E13001]: feature is not supported in specified edition

		28 │ let Foo() = x;
		│ ^^^^^ Missing binding for field '0' in '0x42::M::Foo'

[move-compiler] Add positional struct fields #14073

[move-compiler] Add positional struct fields #14073

Conversation

tzakian commented Oct 3, 2023

Description

Parser

Expansion

Naming

Test Plan

Type of Change (Check all that apply)

Release notes

vercel bot commented Oct 3, 2023 • edited Loading

tnowacki 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnowacki left a comment

Choose a reason for hiding this comment

tzakian commented Oct 4, 2023 • edited Loading

tnowacki commented Oct 4, 2023

tnowacki commented Oct 4, 2023

tnowacki left a comment

Choose a reason for hiding this comment

tnowacki commented Oct 4, 2023

tzakian commented Oct 5, 2023 • edited Loading

vercel bot commented Oct 3, 2023 •

edited

Loading

tzakian commented Oct 4, 2023 •

edited

Loading

tzakian commented Oct 5, 2023 •

edited

Loading