Skip to content

Destructuring breaks program order #32305

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

Closed
Validark opened this issue Jul 9, 2019 · 9 comments · Fixed by #34806
Closed

Destructuring breaks program order #32305

Validark opened this issue Jul 9, 2019 · 9 comments · Fixed by #34806
Assignees
Labels
Bug A bug in TypeScript

Comments

@Validark
Copy link

Validark commented Jul 9, 2019

I was wondering in what order destructuring is supposed to execute expressions, so I wrote the following test. I tried it in Chrome's V8 and Node, and they both gave me the same answer. I tried it in ts-node/TS, and it gave me a different answer.

TypeScript Version: any

Search Terms: destructuring breaks program order

Code

function f() {
    console.log("f");
    return { x: 1 };
}
function g() {
    console.log("g");
    return "x" as "x";
}
const { [g()]: x } = f();

Expected behavior: f g

Actual behavior: g f

Playground Link: https://www.typescriptlang.org/play/?target=1#code/GYVwdgxgLglg9mABMAFASkQbwFCL4iBAZzgBsBTAOlLgHMUAiYBtAbl3wCdyoROlMiAB4AuRAEZEAX3ZTsoSLASJ6GHPgLEyVGvQa0W7Dd179EDIQ0QBDIucuzshMEShZEAbVUBdMUOmIALzI6KxAA

Code 2
This also means that the program state can be contaminated (with bad practices?)

let i = 0;

function f() {
  console.log(i);
  return { 0: "a", 1: "b", 2: "c" } as { [key: number]: string };
}

const { [++i]: x } = f();

Expected behavior: 0

Actual behavior: 1

Playground Link:
https://www.typescriptlang.org/play/?target=1#code/DYUwLgBAlhC8EAYDcAoFAzArgOwMZigHtsJ0AKASggG8UBIXYgZ0NADphCBzMqC1CIIgAncJmElqiAFwQARAEM5AGggBGWXIBGKiACZNuORAC+EBUxoBtANYgAntOyYAtlpDCAutKZhhUbC4TVBM0RmxfGggrAGoYqG8IAA9TOFJKVHCWdk4eJIogA

Related Issues: #31469

Babel seems to compile it as f g as well: https://babeljs.io/repl#?babili=false&browsers=chrome%2068&build=&builtIns=usage&spec=false&loose=false&code_lz=GYVwdgxgLglg9mABMAFASkQbwFCL4iBAZzgBsBTAOlLgHMUAiYBtAbl3wCdyoROlMiAB4AuRAEZEAX3ZTsoSLASJ6GHPgLEyVGvQa0W7Dd179EDIQ1nZCYIlCyIA2qoC6YodMQBeZOlZAA&debug=false&forceAllTransforms=false&shippedProposals=false&circleciRepo=&evaluate=false&fileSize=false&timeTravel=false&sourceType=module&lineWrap=false&presets=es2015%2Cflow&prettier=false&targets=&version=7.5.2&externalPlugins=

I don't understand what ECMAScript is saying about what it should be, but I believe this is relevant: https://tc39.es/ecma262/#sec-runtime-semantics-destructuringassignmentevaluation

In my opinion, TS's compilation makes the most sense. I don't know who is correct. Maybe TS is correct and everyone else is wrong. Again, I don't understand the ECMAScript document. But I can definitively say that TS differs from everyone else in this way.

@RyanCavanaugh
Copy link
Member

Clarification: f g is the V8 behavior, g f is the current TypeScript downlevel behavior.

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Jul 9, 2019
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.7.0 milestone Jul 9, 2019
@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Jul 9, 2019

I haven't combed through the spec to find the exact specification, but it's safe to assume V8 is getting it right and we're not.

The last line should be something like

var _b = f(), _a = g(), x = _b[_a];

instead of

var _a = g(), x = f()[_a];

@fatcerberus
Copy link

https://tc39.es/ecma262/#sec-runtime-semantics-keyeddestructuringassignmentevaluation

This is a bit hard to follow but it looks like the RHS gets evaluated before the LHS meaning f g (V8) is indeed the correct behavior.

@Validark
Copy link
Author

Also, note that while this is "only downlevel behavior", this still does have significant implications. I am guessing most people aren't targetting such an old version of JS, but ts-node relies on this part of TS compiling properly. I am not exactly sure how ts-node works under the hood (I am guessing it compiles to JS temp files and feeds directly into node?), but I can tell you that this bug makes ts-node run differently than compiling with tsc and executing with node.

@RyanCavanaugh
Copy link
Member

We intend to fix the bug.

ts-node should probably start targeting ES6 instead of ES5.

@Validark
Copy link
Author

Don't forget about #31469 either! Making ts-node target ES6 will fix this issue for ts-node but won't make #31469 any better.

@RyanCavanaugh
Copy link
Member

@Validark if something is tagged Bug and is in a milestone, it's going to get fixed

@RyanCavanaugh
Copy link
Member

Also I would love to know what non-pathological code you're writing that ends up depending on this?

@Validark
Copy link
Author

Actually, @RyanCavanaugh, I am one of the primary developers of a TypeScript-to-Lua program for the Roblox platform. View our project page here. I have been actively trying to think of ways it could break, and I have looked to the TypeScript compiler to see how it compiles certain things and handles certain cases. We still have yet to make it to v1, and there are a few issues on my to-do list, but I am proud of some of the things we have accomplished thus far with the project. It would be cool if you checked out this playground we made! Keep in mind that in roblox-ts, the type checker is law and lying to it is sin: We do type-based emits because that's the most efficient way for our use-case.

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

Successfully merging a pull request may close this issue.

4 participants