WIP - Do expressions transform for switch statements by rajasekarm · Pull Request #6975 · babel/babel

rajasekarm · 2017-12-05T04:29:09Z

Q	A
Fixed Issues?	#3872
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	Yes
Documentation PR
Any Dependency Changes?
License	MIT

babel-bot · 2017-12-05T04:38:44Z

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/6170/

xtuc

Great job 👍

I was wondering why we needed to change babel-traverse for that but it's because the do-expression uses our internal

babel/packages/babel-traverse/src/path/replacement.js

Line 213 in 830c527

export function replaceExpressionWithStatements(nodes: Array<Object>) {

Andarist · 2017-12-05T15:30:24Z

packages/babel-traverse/src/path/family.js

+      );
+    } else {
+      const finalStatement = cases[i].get("consequent")[consequentLength - 1];
+      const isBreakStatement =


hasBreakStatement would suit here better imho

Andarist · 2017-12-05T15:31:32Z

packages/babel-traverse/src/path/family.js

+        paths,
+      );
+    } else {
+      const finalStatement = cases[i].get("consequent")[consequentLength - 1];


maybe something like lastCaseStatement?

Andarist · 2017-12-05T15:33:19Z

packages/babel-traverse/src/path/family.js

+        paths,
+      );
+    } else {
+      const finalStatement = cases[i].get("consequent")[consequentLength - 1];


we should also check if consequentLength - 1 is a valid index, accessing -1 may cause deoptimizations in v8

Andarist · 2017-12-05T15:36:55Z

packages/babel-traverse/src/path/family.js

+function completionRecordForSwitch(cases, paths) {
+  for (let i = 0, caseLen = cases.length; i < caseLen; i++) {
+    const consequentLength = cases[i].get("consequent").length;
+    const isDefaultStatement = !cases[i].get("test").type;


a more explicit check of cases[i].get("test").type === null could be used

also isDefaultCase would be more descriptive

Andarist · 2017-12-05T15:38:01Z

packages/babel-traverse/src/path/family.js

 }

+function completionRecordForSwitch(cases, paths) {
+  for (let i = 0, caseLen = cases.length; i < caseLen; i++) {


is there any reason to cache caseLen here?

I would prefer to cache the array length always. Your views please.

This should be a for (const case of cases) {} loop. You make no use of the loop variables other than cases[i].

changed to for of

nicolo-ribaudo

This example is broken: https://babeljs.io/repl/build/6104/#?babili=false&browsers=&build=&builtIns=false&code_lz=G4QwTgBCELwQJgewgbwFAQgZwO4EsAXAYwAsAKABgEpUNMIiQsBTCCgLggCIAzRRLgG46mRiwgBGTgCMwzEAGthmAL5oVEAD6bu08FzRpQkabATJ0mXIVKUal-mNYcIs-UpEMmrKdz4DBVzlFZQg1DW1dfUMQABtmMAIyaABqbnS06SpBIA&debug=false&circleciRepo=&evaluate=true&lineWrap=true&presets=es2015%2Ces2016%2Ces2017%2Cstage-0&prettier=false&targets=Node-8.9&version=7.0.0-beta.34%2Bpr.6975

var a = do {
  switch(0) {
    case 0: "foo";
    case 1: break;
  }
} || "bar"

var b = do {
  switch(0) {
    case 0: break;
    case 1: "foo"; break;
  }
} || "bar"

alert(a + " " + b);

Expected output: foo bar
Actual output: bar foo

rajasekarm · 2017-12-06T00:08:31Z

@nicolo-ribaudo @Andarist I'll update the PR and keep posted here.

Andarist · 2017-12-07T00:53:45Z

packages/babel-traverse/src/path/family.js

+    const isDefaultCase = switchcase.get("test").type === null;
+    if (isDefaultCase) {
+      paths = addCompletionRecords(
+        switchcase.get("consequent")[consequentLength - 1],


this code path is not guarded against accessing -1 index

updated the code to break the loop, if it has zero element

Andarist · 2017-12-07T01:02:11Z

packages/babel-traverse/src/path/family.js

+      const lastCaseStatement = switchcase.get("consequent")[
+        consequentLength - 1
+      ];
+      const hasBreakStatement =


as latter IfStatements are both checking against hasBreakStatement you could just continue here for !hasBreakStatement and remove those checks below

also the mentioned IfStatements should form if/else imho

Andarist · 2017-12-07T11:14:34Z

@rajzshkr

I forgot about something @nicolo-ribaudo's example should become an exec.js test. In fact each of your test could get exec.js version. It would help prevent accidental regressions in the future.

rajasekarm · 2017-12-08T02:35:25Z

I've few doubts, after @nicolo-ribaudo comment with the below example,

Example 1:

var a = do {
  switch(0) {
    case 0: "foo";
    case 1: break;
  }
} || "bar"

Example 2:

var b = do {
  switch(0) {
    case 0: break;
    case 1: "foo"; break;
  }
} || "bar"

alert(a + " " + b);

Expected output: foo bar
Actual output: bar foo

I read the the spec of switch case,, so as per spec the code should be transformed as below to get the proper evaluated output.

var a = function() {
  switch(0) {
    case 0: "foo";
    case 1: 
       return "foo"; // last evaluated output should be return if `caseclause` encounter a break.
  }
}() || "bar"

but it create confusion if there is change in structure of source code. Whats is the possible/right solution?

nicolo-ribaudo · 2017-12-08T08:01:04Z

That transformation wouldn't work, since switch(1)... should return undefined. It should become like this.

var a = function() {
  switch(0) {
    case 0: return "foo";
    case 1: return;
  }
}() || "bar"

If a case clause's first statement is break, it should be replaced with undefined, which must be marked as a completion value. Then, if the last statement of the record before isn't break, it should also be marked as a completion value.

I think you may need to loop over the case declarations from last to first, otherwise the break statement of the case before would already have been removed.

xtuc · 2017-12-08T09:58:22Z

@nicolo-ribaudo @rajzshkr

Your example

I simplified a bit your example: REPL (removed the break because it was causing confusing I think).

Actual: abar foo
Expected: abar foo

Note that:

var a = function () {
  switch (0) {
    case 0:
      "foo";
  }
}();

returns undefined since there's not break or default case. I don't know what the spec says for this case but since it cannot fall through to another case it should return the value?

rajasekarm · 2017-12-12T17:10:36Z

@xtuc but as per ECMA spec, last expression in the case block should be returned if there is no break statement.

nicolo-ribaudo · 2017-12-12T19:01:38Z

packages/babel-traverse/src/path/family.js

+      const hasBreakStatement =
+        lastCaseStatement && lastCaseStatement.isBreakStatement();
+
+      if (hasBreakStatement) {


All these checks are the same of lines 62-80?

nicolo-ribaudo · 2017-12-12T19:04:03Z

packages/babel-traverse/src/path/family.js

+        );
+      }
+
+      if (!hasBreakStatement && consequentLength) {


Only if the first statement of the following case is break.

e.g.

let a = do { switch(0) { case 0: "foo"; case 1: "bar"; break; } };

Should be "bar".

xtuc · 2017-12-13T07:16:11Z

@rajzshkr so the behavior I explained here #6975 (comment) is a bug, do we agree?

rajasekarm · 2017-12-13T10:42:13Z

@xtuc Yup. The example in the comment should output as foo instead of undefined

yavorsky

Please, consider:
https://babeljs.io/repl/build/6104/#?babili=false&browsers=&build=&builtIns=false&code_lz=G4QwTgBCELwQJgewgbwFAQgZwO4EsAXAYwAsAKABgEpUNMIiQsBTCCgLlQjvrYG4e9AEZhmIANYDeAX0GMWEAIycARADNEiFXwgixkurNloQAG2ZgCZEFR0B6OxADKJRAFdT8XawpA&debug=false&circleciRepo=&evaluate=true&lineWrap=true&presets=es2015%2Ces2016%2Ces2017%2Cstage-0&prettier=false&targets=Node-8.9&version=7.0.0-beta.34%2Bpr.6975

Consequent could be not just ExpressionStatement, but BlockStatement. Inside this block, we can have break, which should return the value above.

  switch(0) {
    case 0: { 
      0;
      break;
    }
    case 1: "foo"; break;
  }

for now, will return undefined, because we didn't override break for this case and output is:

case 0: {
   0;
   break;
}

rajasekarm · 2017-12-23T07:38:24Z

@yavorsky I've noted this issue also, I'll update the PR.

linonetwo · 2018-08-08T12:25:43Z

@rajzshkr Will you update this recently?

hackwaly · 2019-05-06T16:52:27Z

Any one can pick his progress?

lint fix

724b9ed

rajasekarm changed the title ~~lint fix~~ Do expressions transform incorrectly for switch statements Dec 5, 2017

rajasekarm changed the title ~~Do expressions transform incorrectly for switch statements~~ Do expressions transform for switch statements Dec 5, 2017

rajasekarm changed the title ~~Do expressions transform for switch statements~~ WIP - Do expressions transform for switch statements Dec 5, 2017

switch inside do expression

7d63ee7

xtuc approved these changes Dec 5, 2017

View reviewed changes

xtuc added pkg: traverse PR: Spec Compliance 👓 A type of pull request used for our changelog categories Spec: Do Expressions labels Dec 5, 2017

Andarist reviewed Dec 5, 2017

View reviewed changes

nicolo-ribaudo requested changes Dec 5, 2017

View reviewed changes

rajasekarm added 3 commits December 6, 2017 06:58

review comments

f93f8c6

review comments

56a7c44

make min diff

6d93e6c

Andarist reviewed Dec 7, 2017

View reviewed changes

adding more test

cb18e0f

Andarist reviewed Dec 7, 2017

View reviewed changes

review changes

9cdfadb

Andarist approved these changes Dec 7, 2017

View reviewed changes

xtuc approved these changes Dec 7, 2017

View reviewed changes

review changes

0d886f8

nicolo-ribaudo reviewed Dec 12, 2017

View reviewed changes

nicolo-ribaudo mentioned this pull request Dec 15, 2017

example of do expression with switch/case tc39/proposal-do-expressions#8

Open

yavorsky requested changes Dec 20, 2017

View reviewed changes

tanhauhau mentioned this pull request Jun 9, 2019

Do expressions transform for switch statements #10070

Merged

nicolo-ribaudo closed this in #10070 Aug 1, 2019

rajasekarm deleted the bug-3872 branch August 1, 2019 08:20

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 31, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 31, 2019

Uh oh!

Conversation

rajasekarm commented Dec 5, 2017

Uh oh!

babel-bot commented Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xtuc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andarist Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajasekarm Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajasekarm commented Dec 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andarist commented Dec 7, 2017

Uh oh!

rajasekarm commented Dec 8, 2017

Uh oh!

nicolo-ribaudo commented Dec 8, 2017

Uh oh!

xtuc commented Dec 8, 2017

Your example

Note that:

Uh oh!

rajasekarm commented Dec 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xtuc commented Dec 13, 2017

Uh oh!

rajasekarm commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yavorsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajasekarm commented Dec 23, 2017

Uh oh!

linonetwo commented Aug 8, 2018

Uh oh!

hackwaly commented May 6, 2019

Uh oh!

Reviewers

Assignees

Labels

babel-bot commented Dec 5, 2017 •

edited

Loading

Andarist Dec 5, 2017 •

edited

Loading

rajasekarm Dec 7, 2017 •

edited

Loading

nicolo-ribaudo left a comment •

edited

Loading

rajasekarm commented Dec 13, 2017 •

edited

Loading

yavorsky left a comment •

edited

Loading