Typescript: Disallow comma operator with side-effect-free left operands

[RyanCavanaugh]

It's easy to write code like this (#10802)

// This code does not do what it appears to!
let arr = [];
switch(arr.length) {
  case 0, 1:
    return 'zero or one';
  default:
    return 'more than one';
}

Or this

let x = Math.pow((3, 5)); // x = NaN, wat?

Or this:

let a = [(3 + 4), ((1 + 1, 8) * 4)]; // oops

We should disallow left comma operands when they are _side-effect free_. An expression is side-effect free (SEF) if it is:

• A literal
• An identifier
• A unary operator whose operand is SEF
• A binary operator, other than the assignment operators, whose operands are both SEF
• A ternary operator whose operands are all SEF
• A parenthesized expression which is SEF
• A function expression

[yortus]

Tweaking what is/isn't SEF:

• A class expression is also SEF
• An identifier _may not_ be SEF if it's a getter defined on the global or window object.
• ++ and -- unary operators obviously aren't SEF

[RyanCavanaugh]

Thinking about this more, I think we should define SEF to be slightly more prescriptive than an actual runtime description. For example, an array-literal is known to be SEF if its elements are SEF, but we still don't want to allow this clearly-wrong (or at least clearly-wat) code:

let a = ([x++], 4);

Same for

let b = (x * ++y, 4);

Basically, if an expression's _top-level form_ isn't one with potential side effects, we should error, because even if there's a nested non-SEF expression, the parent expression is doing something that doesn't go anywhere, which doesn't pass the smell test.

[yortus]

@RyanCavanaugh aren't these two separate issues? One is the OP issue with comma expressions, the other is about expressions whose top-level result is not used.

If TypeScript disallowed let b = (x * ++y, 4); on the basis that the result of x * ++y is not used, then perhaps it should also disallow x * ++y; as an ExpressionStatement.

(I'm assuming the guiding principle here is "statically identify constructs that are likely to be errors.")

[yortus]

After looking at the PR, it seems like it's just pragmatic to only check the top-level is SEF since it simplifies SEF checking of comma expressions, has the desired effect, and may catch a few additional programmer errors as a bonus.

I'd still argue for performing similar SEF checks on ExpressionStatements for consistency sake. I think you'd catch more 'constructs likely to be errors' of this kind in ExpressionStatetments than in rarely-used comma expressions. Of course "use strict"; would have to be allowed still.

[RyanCavanaugh]

I agree in principle that x * ++y; should be disallowed as well. A "conflict of interest" is that we use those kinds of expression statements _everywhere_ in our testbed to gather their types for testing purposes and it'll be a lot more work to fix all those.

The comma operator specifically was just really compelling because of the "looks like it works but really doesn't" aspect of case labels as well as messing up paren balancing in function calls / array literals. Accidently making a SEF ExpressionStatement _seems_ to be slightly harder but I could be convinced otherwise.

[yortus]

Right, it's really convenient to use naked SEF ExpressionStatements to check type inference/narrowing in test code and code under development. It would be very annoying if all of these expressions were compiler errors.

Your PR uses the allowUnreachableCode flag to suppress SEF checks, would that work for ExpressionStatements as well?

[yortus]

One example of a SEF ExpressionStatement I have come across is a (possibly very long) IIFE that doesn't actually get executed. Something like this:

(function () {

    /* IIFE code... */

}) // oops forgot to call it

[RyanCavanaugh]

@mhegazy just showed me a great hit from our partner suite :laughing:

   if (...) {
      throw ("Unexpected thing {0}, expected {1}", actual, expected);
   }

[RReverser]

Could this be allowed for things like (0, object.func)()? It's a common pattern to call function with no this context without extracting it into a temporary variable.

[RReverser]

Just saw https://github.com/Microsoft/TypeScript/issues/12978#issuecomment-274654571, guess that's fine as a workaround.