-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[Compiler-V2] [Feature] Positional Fields #14000
Conversation
06df31f
to
2131fe6
Compare
2131fe6
to
485fe79
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14000 +/- ##
===========================================
- Coverage 70.8% 59.0% -11.9%
===========================================
Files 2324 824 -1500
Lines 459502 198347 -261155
===========================================
- Hits 325634 117075 -208559
+ Misses 133868 81272 -52596 ☔ View full report in Codecov by Sentry. |
094a397
to
b2919c5
Compare
b2919c5
to
e02216a
Compare
Please add a test case for "assign anonymous field", e.g., perhaps:
Or even one with the modified var as a parameter:
|
Also tests of accesses into nested structs, both anonymous and not, e.g.:
|
struct S(u8); | ||
|
||
fun arity_mismatch0(): S { | ||
S() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a similar test but with S having 2 fields and you providing 1. Also for an assignment. 1/0 might be a special case, test enough for confidence that both corner cases and typical cases will work. A single test case isn't enough to see a rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
b14be48
to
86e5f61
Compare
done. |
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have at least a few transactional tests. Maybe something based on the assignment tests.
} | ||
} | ||
|
||
pub fn is_struct_or_schema_name(&self) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep the function name same as the original here, despite being wordy. It's more precise, and is_valid...
implies that the form is ok but it might not be a struct or schema.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -18,6 +18,12 @@ error: variants not allowed in this context | |||
22 │ 0x815::m::missplaced_variant::Red(); | |||
│ ^^^ | |||
|
|||
error: the function takes 1 argument but 0 were provided |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have a new error here? Get rid of it, please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More general comments to be addressed:
- We need several transactional tests showcasing end-to-end functionality. I tried adding a simple one, and it seems to fail with a compiler "bug" because
0
is not a identifier (so there may be restrictions for even later in the compiler process that disallows certain field names). - Following are some transactional tests that would be nice:
- Showcases both generics (I don't think there were cases with generic structs with positional fields instantiated) and the tuple swap bugs we have had before:
struct Tup<T, U>(T, U);
fun baz(x: u64, y: u64): u64 {
let Tup(y, x) = Tup(x, y);
y - x
}
- A case where we have
enum Blah
with multiple variants using positional fields, and a "cross-variant common access" (likex.0
, wherex: Foo
).
enum Blah {
A(u8),
B(u8),
};
fun foo(x: Blah): u8 {
x.0
}
@@ -0,0 +1,65 @@ | |||
module 0x42::test { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's please rename this directory as positional_fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
x: u8, | ||
} | ||
|
||
struct S1(u64, bool); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a test where one of the positional fields is a reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done (in decl_invalid.move
)
@@ -0,0 +1,31 @@ | |||
|
|||
Diagnostics: | |||
error: missing field `0` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit.
Would something like below be clearer to users who may not know the internal representation?
missing field 0
-> positional fields arity mismatch: expected 1, actual 0.
field 1
not declared -> positional fields arity mismatch: expected 1, actual 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done in my next PR for ".." syntax.
@@ -0,0 +1,7 @@ | |||
|
|||
Diagnostics: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it is a bit misleading to name this file decl_ok
when it actually contains compile-time invalid code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -0,0 +1,7 @@ | |||
|
|||
Diagnostics: | |||
error: expected struct constructor `test::E1{ ... }` for struct constructor `test::E1` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error message is misleading and confusing: test::E1{ ... }
is not a valid constructor, test::E1::V2 {...}
is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -2796,6 +2868,28 @@ fn parse_field_annot(context: &mut Context) -> Result<(Field, Type), Box<Diagnos | |||
Ok((f, st)) | |||
} | |||
|
|||
/// Parse a comma list of types surrounded by parenthesis into a vector of `(Field, Type)` pairs | |||
/// where the fields are named "0", "1", ... with location the location of the type in the second field |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicate "location" term in sentence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -2796,6 +2868,28 @@ fn parse_field_annot(context: &mut Context) -> Result<(Field, Type), Box<Diagnos | |||
Ok((f, st)) | |||
} | |||
|
|||
/// Parse a comma list of types surrounded by parenthesis into a vector of `(Field, Type)` pairs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to also give the pseudo-grammar, keeping up with the tradition of the rest of file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
)) | ||
.expect("unique field names"); | ||
let unpack_ = EA::LValue_::Unpack(maccess.clone(), generics.clone(), fields); | ||
let unpack = Spanned::new(lv.loc.clone(), unpack_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let unpack = Spanned::new(lv.loc.clone(), unpack_); | |
let unpack = Spanned::new(lv.loc, unpack_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
)?; | ||
let field_decls = field_decls.clone(); | ||
if is_positional_constructor != expected_positional_constructor { | ||
let struct_name_display = struct_name.display(self.env()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are ignoring the variant information here (when available), which is part of the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_positional_constructor
is computed from the struct and also the variant, if this is what you are asking?
@@ -133,7 +133,7 @@ pub(crate) struct StructEntry { | |||
|
|||
#[derive(Debug, Clone)] | |||
pub(crate) enum StructLayout { | |||
Singleton(BTreeMap<Symbol, FieldData>), | |||
Singleton(BTreeMap<Symbol, FieldData>, bool), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Describe what the bool
is for in a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider whether it would be clearer to create a distinct variant here, perhaps Positional(...)
to avoid the confusing bool
. You could even potentially change the BTreeMap
to a Vec
but maybe being able to share code with the Singleton
case would be useful.
6b0c1b4
to
09a63bc
Compare
a26a886
to
3ace642
Compare
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
Description
This PR introduces positional fields to Move 2. Example usage:
Implementation
Declarations such as
is desugared into
Similarly,
S(42)
is desugared intoS {0 : 42 }
,let S(x) = y;
intolet S { 0: x } = y;
and so on. This happens on the parser AST, making minimal changes to the compiler.We also need to resolve call expressions such as
S(42)
to pack expressions when building the build.Type of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
third_party/move/move-compiler-v2/tests/checking/anonymous_fields
Checklist