tc39 / proposal-do-expressions

Proposal for `do` expressions
MIT License
1.11k stars 14 forks source link

Completion value is complex 10 times harder when `break` is involved #75

Open Jack-Works opened 2 years ago

Jack-Works commented 2 years ago

As many issues have pointed out, completion values are hard to find out. Although the status quo disallows for loop, it's still not enough.

I was implementing the type checker for the do expression. It's an intuitive idea that I only need to recursively check the last statement and check every branch. But when break comes in, it becomes a mess.

expr = do {
a: {
    for (const a of [1, 2]) {
        if (a === 2) { a; break a; }
    }
    3
}
}

For example, the complete value of the expression above is 2. It needs much more work to analyze them correctly (not only for the type checker but also for developers).

Is it possible to ban LabelledStatement and SwitchStatement as the last statement?

SwitchStatement is useful before we have pattern matching, instead of ban it as a whole, maybe we can introduce a "well-formed" version like what we did for if (the last if must have else branch).

pitaj commented 2 years ago

Pretty sure the labeled block case you have there is already invalid.

From the readme:

More formally, the completion value of the StatementList can't rely on the completion value of a loop or declaration. See EndsInIterationOrDeclaration in the proposed specification for details.

bakkot commented 2 years ago

Yeah, I am undecided about these.

As you say, labelled break is particularly confusing, whereas switch can be straightforward if used in a particular way. The thing is, that is also true of those statements in any other context. I think the vast majority of JS programmers have not encountered labelled break (except when the label is on a loop), and I think that would continue to be the case with the introduction of do expressions. I'm reluctant to introduce a restriction which prohibits users from using a feature they probably weren't even aware of, because it's not obvious that would benefit anyone.

Jack-Works commented 2 years ago

From the readme:

It didn't. It relies on the completion value of LabelledStatement and SwitchStatement which can be breaked.

Jack-Works commented 2 years ago

Yeah, I am undecided about these.

As you say, labelled break is particularly confusing, whereas switch can be straightforward if used in a particular way. The thing is, that is also true of those statements in any other context. I think the vast majority of JS programmers have not encountered labelled break (except when the label is on a loop), and I think that would continue to be the case with the introduction of do expressions. I'm reluctant to introduce a restriction which prohibits users from using a feature they probably weren't even aware of, because it's not obvious that would benefit anyone.

Yes, I agree. I think those features are rarely used. But those edge cases increase the implementation cost (of a transpiler and a type checker) to keep track of all possible positions to yield a completion value. If you don't want to ban them at the language level, is it acceptable to treat this as a type error? (cc someone from TypeScript team maybe @orta or @rbuckton)

Jack-Works commented 2 years ago

Pretty sure the labeled block case you have there is already invalid.

From the readme:

More formally, the completion value of the StatementList can't rely on the completion value of a loop or declaration. See EndsInIterationOrDeclaration in the proposed specification for details.

Oh, as the code I give before, the completion value doesn't comes from the for...of statement.

expr = do {
a: {
    for (const a of [1, 2]) {
        if (a === 2) { a; break a; }
//                     ^ this "a" is where the completion value comes from which is an expression statement.
    }
    3
}
}
pitaj commented 2 years ago

Does it even need to be a loop inside?

let x = 5;
let y = do {
  a: {
    if (x === 5) {
      x;
      break a;
    }
    4
  }
};
55Cancri commented 2 years ago

Sorry if this is unrelated but what is taking do expressions so long to be released? Does anyone have an eta? The proposal is still in stage 1 and its been that way since like 2016, what gives?

bakkot commented 2 years ago

@55Cancri Currently the delay is to give us time to assess whether the behavior in this proposal (or aspects of it, like in the OP) is too confusing. I'm hopeful on that front, but it's going to take a bit more time to work through.

The delay from 2016 is mostly because the person who was originally championing it went to go do other things with his life. I only recently picked it up.

pitaj commented 2 years ago

I'm reluctant to introduce a restriction which prohibits users from using a feature they probably weren't even aware of, because it's not obvious that would benefit anyone.

On the other hand, adding restrictions later is not really possible, whereas it is possible to loosen restrictions later.

pitaj commented 2 years ago

@Jack-Works could you give an example why your first two switch restrictions are necessary? I'd think requiring a default case would be enough.

Jack-Works commented 2 years ago

The main idea of this issue is to keep the mind model simple.

In most cases, we can find "exits" of a do expression via recursively find the last meaningful statement.

For example,

const val = do {
    try {
        if (expr) expr2
        else expr3
    } catch (e) {
        expr4
    }
}

In this expression,

  1. In the do expression, the last statement that will produce completion value is TryStatement.
    1. In the try block, the last statement that will produce completion value is IfStatement.
      1. In the if block, the last statement that will produce completion value is ExpressionStatement (expr2). So this is an exit point.
      2. In the else block, the last statement that will produce completion value is ExpressionStatement (expr3). So this is an exit point.
    2. In the catch block, the last statement that will produce completion value is ExpressionStatement (expr4). So this is an exit point.
  2. Therefore, expr2, expr3 and expr4 is the exit point of this do expression.

This mental model is not how completion value works in JavaScript, but it's a good approximation. It's easier for programmers to understand how do expression works. If we can follow this convention.

The following cases break this analysis with the break statement.

switch (true) {
    case true:
         if (Math.random() > 0.5) { 1; break }
         2;
    case false:
}
expr = do {
    a: {
        if (Math.random() > 0.5) { 1; break  }
        2;
    }
}

I think we should try to maintain this mental model instead of allowing some subtle cases that leak how completion value actually works in JavaScript.

theScottyJam commented 2 years ago

Currently, right before the break of a labeled block, are you allowed to place bizarre things like if-without-else? Or is that statement still susceptible to the rules?

Jack-Works commented 2 years ago

I think we should try to maintain this mental model instead of allowing some subtle cases that leak how completion value actually works in JavaScript.

How do you think? I think it's good for developers and also for toolchain maintainers. @bakkot

2A5F commented 2 years ago

Explicit return can solve this

72

pitaj commented 2 years ago

Only mandatory explicit return, which is a drastic reduction of functionality. It's a trade-off of implementation simplicity for increased verbosity and less flexibility.

I know there is a very vocal minority who support explicit return, but you don't need to evangelize across every issue here.

ljharb commented 2 years ago

Regardless of one's feelings on both explicit and implicit return, imo the only thing worse than ${the one i like less} is users being able to arbitrarily pick one or the other. I think we should either force implicit completion, always, or force explicit completion, always.

Jack-Works commented 2 years ago

I think it's a problem more or less, so I implemented IDE support to solve this problem a little.