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.

Sign up for GitHub

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

Jump to bottom

fix: remove the destructuring fields quantity check in interpreter #969

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

fix: remove the destructuring fields quantity check in interpreter #969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4e99d21
fix: remove the identifiers size check in interpreter
Gusarich Oct 16, 2024
8cf2173
chore: update changelog
Gusarich Oct 16, 2024
72ebd37
chore: add const eval testcase
Gusarich Oct 23, 2024
569de25
feat(grammar): introduce rest pattern
Gusarich Oct 24, 2024
d8f035d
feat(typechecker): check variables count
Gusarich Oct 24, 2024
7bb1ef4
feat: update prettyprinter
Gusarich Oct 24, 2024
4b9e402
chore: fix structs e2e test
Gusarich Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Docs: the `description` property to the frontmatter of the each page for better SEO: PR [#916](https://github.com/tact-lang/tact/pull/916)
- Docs: Google Analytics tags per every page: PR [#921](https://github.com/tact-lang/tact/pull/921)
- Ability to specify a compile-time method ID expression for getters: PR [#922](https://github.com/tact-lang/tact/pull/922) and PR [#932](https://github.com/tact-lang/tact/pull/932)
- Destructuring of structs and messages: PR [#856](https://github.com/tact-lang/tact/pull/856)
- Destructuring of structs and messages: PR [#856](https://github.com/tact-lang/tact/pull/856), PR [#969](https://github.com/tact-lang/tact/pull/969)

### Changed

Expand Down
32 changes: 20 additions & 12 deletions src/grammar/__snapshots__/grammar.spec.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8214,12 +8214,12 @@ exports[`grammar should parse stmt-destructuring 1`] = `
"loc": fun testFunc(): Int {
let s = S{ a: 1, b: 2, c: 3 };
let S { a, b, c } = s;
let S { a: a1 } = s;
let S { b: b1 } = s;
let S { c: c1 } = s;
let S { a: a2, b: b2 } = s;
let S { a: a3, c: c3 } = s;
let S { b: b4, c: c4 } = s;
let S { a: a1, .. } = s;
let S { b: b1, .. } = s;
let S { c: c1, .., } = s;
let S { a: a2, b: b2, .. } = s;
let S { a: a3, c: c3, .., } = s;
let S { b: b4, c: c4, .. } = s;

let m = M{ a: 1, b: 2 };
let M { a: a_m, b: b_m } = m;
Expand Down Expand Up @@ -8373,6 +8373,7 @@ exports[`grammar should parse stmt-destructuring 1`] = `
},
"kind": "statement_destruct",
"loc": let S { a, b, c } = s;,
"rest": false,
"type": {
"id": 35,
"kind": "type_id",
Expand Down Expand Up @@ -8405,7 +8406,8 @@ exports[`grammar should parse stmt-destructuring 1`] = `
],
},
"kind": "statement_destruct",
"loc": let S { a: a1 } = s;,
"loc": let S { a: a1, .. } = s;,
"rest": true,
"type": {
"id": 47,
"kind": "type_id",
Expand Down Expand Up @@ -8438,7 +8440,8 @@ exports[`grammar should parse stmt-destructuring 1`] = `
],
},
"kind": "statement_destruct",
"loc": let S { b: b1 } = s;,
"loc": let S { b: b1, .. } = s;,
"rest": true,
"type": {
"id": 53,
"kind": "type_id",
Expand Down Expand Up @@ -8471,7 +8474,8 @@ exports[`grammar should parse stmt-destructuring 1`] = `
],
},
"kind": "statement_destruct",
"loc": let S { c: c1 } = s;,
"loc": let S { c: c1, .., } = s;,
"rest": true,
"type": {
"id": 59,
"kind": "type_id",
Expand Down Expand Up @@ -8518,7 +8522,8 @@ exports[`grammar should parse stmt-destructuring 1`] = `
],
},
"kind": "statement_destruct",
"loc": let S { a: a2, b: b2 } = s;,
"loc": let S { a: a2, b: b2, .. } = s;,
"rest": true,
"type": {
"id": 65,
"kind": "type_id",
Expand Down Expand Up @@ -8565,7 +8570,8 @@ exports[`grammar should parse stmt-destructuring 1`] = `
],
},
"kind": "statement_destruct",
"loc": let S { a: a3, c: c3 } = s;,
"loc": let S { a: a3, c: c3, .., } = s;,
"rest": true,
"type": {
"id": 74,
"kind": "type_id",
Expand Down Expand Up @@ -8612,7 +8618,8 @@ exports[`grammar should parse stmt-destructuring 1`] = `
],
},
"kind": "statement_destruct",
"loc": let S { b: b4, c: c4 } = s;,
"loc": let S { b: b4, c: c4, .. } = s;,
"rest": true,
"type": {
"id": 83,
"kind": "type_id",
Expand Down Expand Up @@ -8721,6 +8728,7 @@ exports[`grammar should parse stmt-destructuring 1`] = `
},
"kind": "statement_destruct",
"loc": let M { a: a_m, b: b_m } = m;,
"rest": false,
"type": {
"id": 102,
"kind": "type_id",
Expand Down
1 change: 1 addition & 0 deletions src/grammar/ast.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,7 @@ export type AstStatementDestruct = {
type: AstTypeId;
/** field name -> [field id, local id] */
identifiers: Map<string, [AstId, AstId]>;
rest: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
rest: boolean;
ignoreUnspecifiedFields: boolean;

expression: AstExpression;
id: number;
loc: SrcInfo;
Expand Down
3 changes: 2 additions & 1 deletion src/grammar/grammar.ohm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,8 @@ Tact {

StatementForEach = foreach "(" id "," id "in" Expression ")" "{" Statement* "}"

StatementDestruct = let typeId "{" ListOf<DestructItem, ","> ","? "}" "=" Expression (";" | &"}")
StatementDestruct = let typeId "{" ListOf<DestructItem, ","> ","? "}" "=" Expression (";" | &"}") --noRest
| let typeId "{" ListOf<DestructItem, ","> "," ".." ","? "}" "=" Expression (";" | &"}") --withRest
Comment on lines +161 to +162
Copy link
Member

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

Suggested change
StatementDestruct = let typeId "{" ListOf<DestructItem, ","> ","? "}" "=" Expression (";" | &"}") --noRest
| let typeId "{" ListOf<DestructItem, ","> "," ".." ","? "}" "=" Expression (";" | &"}") --withRest
StatementDestruct = let typeId "{" ListOf<DestructItem, ","> (","? | "," "..") }" "=" Expression (";" | &"}")

Copy link
Member

@novusnota novusnota Oct 28, 2024 •
edited
Loading

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 ("," ".."?)?

Copy link
Member

Choose a reason for hiding this comment

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

image

https://ohmjs.org/editor/

Copy link
Member

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


DestructItem = id ":" id --regular
| id --punned
Expand Down
40 changes: 39 additions & 1 deletion src/grammar/grammar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1013,7 +1013,44 @@ semantics.addOperation<AstNode>("astOfStatement", {
loc: createRef(this),
});
},
StatementDestruct(
StatementDestruct_withRest(
_letKwd,
typeId,
_lparen,
identifiers,
_comma,
_rest,
_optTrailingComma,
_rparen,
_equals,
expression,
_semicolon,
) {
return createAstNode({
kind: "statement_destruct",
type: typeId.astOfType(),
identifiers: identifiers
.asIteration()
.children.reduce((map, item) => {
const destructItem = item.astOfExpression();
if (map.has(destructItem.field.text)) {
throwSyntaxError(
`Duplicate destructuring field: '${destructItem.field.text}'`,
destructItem.loc,
);
}
map.set(destructItem.field.text, [
destructItem.field,
destructItem.name,
]);
return map;
}, new Map<string, [AstId, AstId]>()),
rest: true,
expression: expression.astOfExpression(),
loc: createRef(this),
});
},
StatementDestruct_noRest(
_letKwd,
typeId,
_lparen,
Expand Down Expand Up @@ -1043,6 +1080,7 @@ semantics.addOperation<AstNode>("astOfStatement", {
]);
return map;
}, new Map<string, [AstId, AstId]>()),
rest: false,
expression: expression.astOfExpression(),
loc: createRef(this),
});
Expand Down
12 changes: 6 additions & 6 deletions src/grammar/test/stmt-destructuring.tact
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,12 @@ message M {
fun testFunc(): Int {
let s = S{ a: 1, b: 2, c: 3 };
let S { a, b, c } = s;
let S { a: a1 } = s;
let S { b: b1 } = s;
let S { c: c1 } = s;
let S { a: a2, b: b2 } = s;
let S { a: a3, c: c3 } = s;
let S { b: b4, c: c4 } = s;
let S { a: a1, .. } = s;
let S { b: b1, .. } = s;
let S { c: c1, .., } = s;
Copy link
Member

Choose a reason for hiding this comment

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

this should fail

Copy link
Member Author

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?

Copy link
Member

Choose a reason for hiding this comment

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

yep

let S { a: a2, b: b2, .. } = s;
let S { a: a3, c: c3, .., } = s;
let S { b: b4, c: c4, .. } = s;

let m = M{ a: 1, b: 2 };
let M { a: a_m, b: b_m } = m;
Expand Down
8 changes: 0 additions & 8 deletions src/interpreter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1465,14 +1465,6 @@ export class Interpreter {
ast.expression.loc,
);
}
if (ast.identifiers.size !== Object.keys(val).length - 1) {
Copy link
Contributor

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.

Copy link
Member

@novusnota novusnota Oct 17, 2024 •
edited
Loading

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 :)

Copy link
Contributor

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.

Copy link
Member Author

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?

Copy link
Member

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;

Copy link
Member Author

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?

Copy link
Member

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

throwErrorConstEval(
`destructuring assignment expected ${Object.keys(val).length - 1} fields, but got ${
ast.identifiers.size
}`,
ast.loc,
);
}

for (const [field, name] of ast.identifiers.values()) {
if (name.text === "_") {
Expand Down
4 changes: 3 additions & 1 deletion src/prettyPrinter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -697,7 +697,9 @@ export class PrettyPrinter {
acc.push(id);
return acc;
}, []);
return `${this.indent()}let ${this.ppAstTypeId(statement.type)} {${ids.join(", ")}} = ${this.ppAstExpression(statement.expression)};`;
const restPattern = statement.rest ? ", .." : "";
console.log(restPattern);
return `${this.indent()}let ${this.ppAstTypeId(statement.type)} {${ids.join(", ")}${restPattern}} = ${this.ppAstExpression(statement.expression)};`;
}
}

Expand Down
12 changes: 6 additions & 6 deletions src/test/contracts/case-destructuring.tact
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,12 @@ message M {
fun testFunc(): Int {
let s = S{a: 1, b: 2, c: 3};
let S {a, b, c} = s;
let S {a: a1} = s;
let S {b: b1} = s;
let S {c: c1} = s;
let S {a: a2, b: b2} = s;
let S {a: a3, c: c3} = s;
let S {b: b4, c: c4} = s;
let S {a: a1, ..} = s;
let S {b: b1, ..} = s;
let S {c: c1, ..} = s;
let S {a: a2, b: b2, ..} = s;
let S {a: a3, c: c3, ..} = s;
let S {b: b4, c: c4, ..} = s;
let m = M{a: 1, b: 2};
let M {a: a_m, b: b_m} = m;
return a + b + c + a1 + b1 + c1 + a2 + b2 + a3 + c3 + b4 + c4 + a_m + b_m;
Expand Down
12 changes: 6 additions & 6 deletions src/test/contracts/renamer-expected/case-destructuring.tact
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,12 @@ message message_decl_1 {
fun function_def_2(): Int {
let s = S{a: 1, b: 2, c: 3};
let S {a, b, c} = s;
let S {a: a1} = s;
let S {b: b1} = s;
let S {c: c1} = s;
let S {a: a2, b: b2} = s;
let S {a: a3, c: c3} = s;
let S {b: b4, c: c4} = s;
let S {a: a1, ..} = s;
let S {b: b1, ..} = s;
let S {c: c1, ..} = s;
let S {a: a2, b: b2, ..} = s;
let S {a: a3, c: c3, ..} = s;
let S {b: b4, c: c4, ..} = s;
let m = M{a: 1, b: 2};
let M {a: a_m, b: b_m} = m;
return a + b + c + a1 + b1 + c1 + a2 + b2 + a3 + c3 + b4 + c4 + a_m + b_m;
Expand Down
26 changes: 26 additions & 0 deletions src/test/e2e-emulated/contracts/structs.tact
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,17 @@ fun destructuringTest7(): S1 {
return S1 {a: e, b: b, c: a};
}

fun destructuringTest8(): Int {
let s = S {
a: true,
b: 42
};

let S {b, ..} = s;

return b;
}

contract StructsTester {
s1: S = S {a: false, b: 21 + 21};
s2: S;
Expand Down Expand Up @@ -1101,4 +1112,19 @@ contract StructsTester {
get fun destructuringTest7Const(): S1 {
return destructuringTest7();
}

get fun destructuringTest8(): Int {
let s = S {
a: true,
b: 42
};

let S {b, ..} = s;

return b;
}

get fun destructuringTest8Const(): Int {
return destructuringTest8();
}
}
2 changes: 2 additions & 0 deletions src/test/e2e-emulated/structs.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,5 +439,7 @@ describe("structs", () => {
b: 2n,
c: 1n,
});
expect(await contract.getDestructuringTest8()).toBe(42n);
expect(await contract.getDestructuringTest8Const()).toBe(42n);
});
});
44 changes: 32 additions & 12 deletions src/types/__snapshots__/resolveStatements.spec.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -939,12 +939,12 @@ Line 16, col 29:
`;

exports[`resolveStatements should fail statements for stmt-destructuring-fields-non-existing 1`] = `
"<unknown>:15:22: Field '"d"' not found in type 'S'
Line 15, col 22:
"<unknown>:15:19: Field '"d"' not found in type 'S'
Line 15, col 19:
14 | let s = S{ a: 1, b: 2, c: 3 };
> 15 | let S { a, b, c, d: e } = s;
^
16 | return a + b + c + e;
> 15 | let S { a, b, d: e } = s;
^
16 | return a + b + e;
"
`;

Expand All @@ -959,22 +959,22 @@ Line 15, col 16:
`;

exports[`resolveStatements should fail statements for stmt-destructuring-fields-non-existing-punned2 1`] = `
"<unknown>:15:22: Field '"d"' not found in type 'S'
Line 15, col 22:
"<unknown>:15:19: Field '"d"' not found in type 'S'
Line 15, col 19:
14 | let s = S{ a: 1, b: 2, c: 3 };
> 15 | let S { a, b, c, d } = s;
^
16 | return a + b + c + d;
> 15 | let S { a, b, d } = s;
^
16 | return a + b + d;
"
`;

exports[`resolveStatements should fail statements for stmt-destructuring-fields-non-existing-underscore 1`] = `
"<unknown>:15:12: Field '"_"' not found in type 'S'
Line 15, col 12:
14 | let s = S{ a: 1, b: 2, c: 3 };
> 15 | let S {_, b} = s;
> 15 | let S {_, b, c} = s;
^
16 | return b;
16 | return b + c;
"
`;

Expand All @@ -998,6 +998,26 @@ Line 16, col 16:
"
`;

exports[`resolveStatements should fail statements for stmt-destructuring-fields-wrong-count 1`] = `
"<unknown>:15:5: Expected 3 fields, but got 2
Line 15, col 5:
14 | let s = S{ a: 1, b: 2, c: 3 };
> 15 | let S { a, b } = s;
^~~~~~~~~~~~~~~~~~~
16 | return a + b;
"
`;

exports[`resolveStatements should fail statements for stmt-destructuring-fields-wrong-count2 1`] = `
"<unknown>:15:5: Expected 3 fields, but got 4
Line 15, col 5:
14 | let s = S{ a: 1, b: 2, c: 3 };
> 15 | let S { a, b, c, d } = s;
^~~~~~~~~~~~~~~~~~~~~~~~~
16 | return a + b + c + d;
"
`;

exports[`resolveStatements should fail statements for stmt-destructuring-fields-wrong-type 1`] = `
"<unknown>:18:21: Type mismatch: "S2" is not assignable to "S1"
Line 18, col 21:
Expand Down
8 changes: 8 additions & 0 deletions src/types/resolveStatements.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -717,6 +717,14 @@ function processStatements(
);
}

// Check variables count
if (!s.rest && s.identifiers.size !== ty.fields.length) {
throwCompilationError(
`Expected ${ty.fields.length} fields, but got ${s.identifiers.size}`,
s.loc,
);
}

// Compare type with the specified one
const typeRef = resolveTypeRef(ctx, s.type);
if (typeRef.kind !== "ref") {
Expand Down
Loading
Loading