Skip to content

Conversation

tanhauhau
Copy link
Member

@tanhauhau tanhauhau commented Jun 9, 2019

Rebased #6975, fixed PR review issues

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

@babel-bot
Copy link
Collaborator

babel-bot commented Jun 9, 2019

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

@babel-bot
Copy link
Collaborator

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

@nicolo-ribaudo nicolo-ribaudo added PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Do Expressions labels Jun 13, 2019
breakStatement.replaceWith(switchCase.scope.buildUndefinedNode());
paths = addCompletionRecords(breakStatement, paths);
}
} else if (consequent.length > 0 && isDefaultCase) {
Copy link
Member

Choose a reason for hiding this comment

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

default cases have nothing special, they should be handled exactly like case:

This should evaluate to 1:

const a = do {
  switch (2) {
    default: 0;
    case 1: 1; break;
  }
};

This should evaluate to 1:

const a = do {
  switch (1) {
    case 1: 1;
  }
};

The check should probably be more like if (isLastStatement) {

Copy link
Member

Choose a reason for hiding this comment

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

Also this should evaluate to 1:

const a = do {
  switch (1) {
    case 1: 1;
    case 2:
  }
};

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 can I say that the lastStatement is an expression statement , then no fall through, else will fall through say:

let a = 1
const b = do {
switch (1) {
   case 1: a = 2;
   case 2: a * 10;
}

So b = ?

Copy link
Member

Choose a reason for hiding this comment

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

10, because the last statement is a*10

Copy link
Member Author

Choose a reason for hiding this comment

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

ok. interesting. is this documented somewhere within https://github.com/tc39/proposal-do-expressions? or where do I find the spec for this?

Copy link
Member

Choose a reason for hiding this comment

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

  1. Switch expressions evaluate to the result of their block: https://tc39.es/ecma262/#sec-switch-statement-runtime-semantics-evaluation steps 7 and 9
  2. It delegates to https://tc39.es/ecma262/#sec-runtime-semantics-caseblockevaluation, in the CaseBlock: { CaseClauses } section.
    i. At the first iteration of step 4 of that algorithm, V is set to 2 (the result of a = 2).
    ii. Since case 1 isn't followed by a break/return/throw statement, R is a NormalCompletion and not an AbruptCompletion: for this reason the loop continues running to the following case.
    iii. At the second iteration, V is set to 20 (the result of a * 10).
    iv. The loop ends, because there are no more cases, and it returns 20

Copy link
Member

Choose a reason for hiding this comment

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

🤦‍♂️ 🤦‍♂️ 🤦‍♂️ 🤦‍♂️
I just noticed I wrote 10 instead of 20

Copy link
Member Author

Choose a reason for hiding this comment

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

I realised reading the ECMA262 Spec is giving me more confusion 🤦‍♂ 🤦‍♂ 🤦‍♂, but I guess I roughly know what you trying to point out. And I think we should document more in the do-expression proposal docs

Copy link
Member

Choose a reason for hiding this comment

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

Btw, you can already "test" do expression in any browser.

do { ANYTHING }

is

eval(`{
  ANYTHING
}`);

"b";
case 3:
{}
{ break; }
Copy link
Member Author

Choose a reason for hiding this comment

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

@nicolo-ribaudo i need some help over here, i've figured this as a plausible test case, but I not sure how i should transform this.

Previously, what I did is to find the BreakStatement, and turn the statement before BreakStatement as a completionPath, which would then be transformed into a return statement.

Problem I've created right now is that the previous statement could be anything from BlockStatement to nested BlockStatements, and I need to find the "last statement" which I am kind stuck on how I should go for this...
not entirely sure I am in the right direction, that's why I am seeking some help over here

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about how to solve it, but it isn't a problem just for switchs. It's a problem for everything which uses break.

e.g.

let a = do {
  while (1) {
    1;
    break;
    2;
  }
};

I think that we should try to fix it separately since it is a really hard problem and this PR is already a big improvement.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK I guess I shall disable that test case then

@tanhauhau
Copy link
Member Author

@nicolo-ribaudo any update on this?

breakStatement.replaceWith(switchCase.scope.buildUndefinedNode());
paths = addCompletionRecords(breakStatement, paths);
}
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest check isLastCaseWithConsequent early so that it may skip the whole hasConsequent setup when it is false.

Copy link
Member Author

Choose a reason for hiding this comment

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

@JLHwung updated!

paths = addCompletionRecords(prevSibling, paths);
breakStatement.remove();
} else {
breakStatement.replaceWith(switchCase.scope.buildUndefinedNode());
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think it should be breakStatement.scope.buildUndefinedNode().

Copy link
Member Author

Choose a reason for hiding this comment

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

@JLHwung sure!

Copy link
Contributor

Choose a reason for hiding this comment

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

After #10243 they will be logically equivalent: buildUndefinedNode does not depend on scope any more. It is by reviewing this PR that I find the room for improvements in buildUndefinedNode.

@tanhauhau tanhauhau force-pushed the tanhauhau/rajasekarm/bug-3872 branch from 2bc5df7 to 39be06f Compare July 20, 2019 15:19
@tanhauhau
Copy link
Member Author

@nicolo-ribaudo bump

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

I don't think that switch/break support is perfect yet. For example, it doesn't work with this code:

let a = do {
  switch (11) {
    case 1:
      { 2; { break } }
  }
};

That said, it is a general problem with nested breaks:

let a = do {
  if (1) block: { 2; { break block } }
};

This PR already does a good job by special-casing case x: { ... }, which is a common way of using switch statements. In the future, we shouldn't special case it and properly fix break support.

@nicolo-ribaudo nicolo-ribaudo merged commit 3e4a9d5 into babel:master Aug 1, 2019
@tanhauhau tanhauhau deleted the tanhauhau/rajasekarm/bug-3872 branch August 1, 2019 13:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Do Expressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do expressions transform incorrectly for switch statements (T6822)
5 participants