-
Notifications
You must be signed in to change notification settings - Fork 107
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
fix: remove the destructuring fields quantity check in interpreter #969
base: main
Are you sure you want to change the base?
Conversation
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 add the previously failing test case from the linked issue
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.
Regarding issue
@@ -1465,14 +1465,6 @@ export class Interpreter { | |||
ast.expression.loc, | |||
); | |||
} | |||
if (ast.identifiers.size !== Object.keys(val).length - 1) { |
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 think the solution is not to remove this code from the interpreter, but to add this number check in the typechecker (resolveStatements.ts
in the statement_destruct
case of the switch). Let me explain why.
The interpreter is only called when attempting to reduce an expression to a value. So, for example, in this code:
struct Two { first: Int; second: String }
fun discard(s: Two) {
let Two { first } = s; // A
}
contract TestContract {
get fun test1(): Int {
discard(Two {first: 5, second: "10"}); // B
return 0;
}
.............
Line B
will trigger the interpreter, because test1
is calling the discard
function. In other words, the interpreter will execute the code of discard
, detecting the error at line A
. If line B
is removed, the interpreter is never called, hence it will not detect the error at line A
in that case. Therefore, the error at line A
should be detected by the typechecker (i.e., the interpreter is just an optimization phase).
At compile-time, there is nothing in the code triggering a receive
, which means that the interpreter will never be called in the receive
example (again, this should be caught by the typechecker):
struct Two { first: Int; second: String }
message HasTwo { s: Two }
contract TestContract {
receive(msg: HasTwo) {
// Almost the same line as in the previous example
let Two { first } = msg.s;
// No errors before or here
dump(first);
}
Just to give an example that the interpreter does catch an example similar to the previous one, in the following example it says the correct error at line A
(just changed message HasTwo
to a struct):
struct Two { first: Int; second: String }
struct HasTwo { s: Two }
fun discard2(m: HasTwo) {
let Two { first } = m.s; // A
}
contract TestContract {
get fun test2(): Int {
discard2(HasTwo {s: Two {first: 5, second: "10"}});
return 0;
}
.............
Comming back to the code in resolveStatements.ts
, I see that the code at line 736 will only detect that the left side of a destructuring statement is contained in the right-hand side. But it will not detect the case when the right-hand side has more fields that the left-hand side, as in the examples above. The number check needs to be added before line 736.
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.
To add to your point — doesn't removing such check defeat the purpose of having field: _
syntax in the first place? Like, if there's no requirement to unpack all fields, the only purpose of _
is to temporarily stop binding the certain field to a certain variable name. But in this case, it would have been easier to just remove the field from the destructuring statement altogether, wouldn't it?
Because the type and number of elements in any Struct/Message are known at compile-time, I'm guessing it should be possible to add that check when resolving receive()
functions and alike too :)
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.
Yes. I think when the check is added to resolveStatements.ts
, it will handle all the cases, including those in receive
declarations.
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.
so do you suggest adding this check back (to typechecker)? @anton-trunov what do you think?
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 say we should make the user specify all the fields of a struct unless they explicitly say they do not want to do that. For instance, Rust has the ..
syntax for this case, so for the example above this will be let Two { first, .. } = struct;
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.
so should we also go with the ..
syntax?
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.
Yeah, let's go for it
@Gusarich Let's finish this PR asap, please |
72479d2
to
b4fafef
Compare
ea99e54
to
4b9e402
Compare
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.
LGTM, I've updated the docs to include ..
StatementDestruct = let typeId "{" ListOf<DestructItem, ","> ","? "}" "=" Expression (";" | &"}") --noRest | ||
| let typeId "{" ListOf<DestructItem, ","> "," ".." ","? "}" "=" Expression (";" | &"}") --withRest |
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'd say that allowing trailing comma after ..
makes little sense, since you cannot add more stuff later
StatementDestruct = let typeId "{" ListOf<DestructItem, ","> ","? "}" "=" Expression (";" | &"}") --noRest | |
| let typeId "{" ListOf<DestructItem, ","> "," ".." ","? "}" "=" Expression (";" | &"}") --withRest | |
StatementDestruct = let typeId "{" ListOf<DestructItem, ","> (","? | "," "..") }" "=" Expression (";" | &"}") |
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.
Better do ("," ".." | ","?)
, otherwise the ".." will always be omitted in the parse.
Or just ("," ".."?)?
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.
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 harder to understand
("," ".." | ","?)
-- this means the list of field patterns ends either ends with an optional trailing comma or the ignore unspecified fields pattern
@@ -326,6 +326,7 @@ export type AstStatementDestruct = { | |||
type: AstTypeId; | |||
/** field name -> [field id, local id] */ | |||
identifiers: Map<string, [AstId, AstId]>; | |||
rest: boolean; |
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.
rest: boolean; | |
ignoreUnspecifiedFields: boolean; |
let S { b: b4, c: c4 } = s; | ||
let S { a: a1, .. } = s; | ||
let S { b: b1, .. } = s; | ||
let S { c: c1, .., } = 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.
this should fail
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.
because of ..,
on line 17?
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.
yep
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.
Tests LGTM. 👍
Issue
Closes #968.
Checklist
[ ] I have added tests to demonstrate the contribution is correctly implemented: this usually includes both positive and negative tests, showing the happy path(s) and featuring intentionally broken cases