[CS2] Fix #3709, #3789: ‘throw’ an ‘if’, ‘for’, ‘switch’, ‘while’

[GeoffreyBooth]

Fixes #3709 and #3789. If the expression to be thrown is of certain types (If, For, Switch, While or a nested Throw) wrap it in a closure, so that it becomes valid JavaScript.

I’m sure there are probably other expression types that should get wrapped, so please suggest any you can think of and I’ll add them to the list. But these should be the most likely ones that people would think to put after throw, (other than another throw, which is so unlikely it feels silly to even cover the case). And now we have a way to handle all similar cases.

I tried to find a way to deduce whether the expression should get wrapped in a closure, by looking at shouldCache or isStatement or the like, but I didn’t find a helper that was consistent. Ultimately I don’t think there are too many node types that make sense to put after throw, so just listing every possibility should hopefully be good enough.

[lydell]

Looks good to me! Might be worth leaving it open for a little bit though, in case somebody else can come up with more cases where a closure is needed.

[connec]

Is there any chance that changing this to @expression.compileToFragments o, LEVEL_LIST would work instead?

[connec]

Looks like the tests still pass with:

diff --git a/src/nodes.coffee b/src/nodes.coffee
index 677d4df0..f3b89902 100644
--- a/src/nodes.coffee
+++ b/src/nodes.coffee
@@ -3255,7 +3255,7 @@ exports.Throw = class Throw extends Base
   makeReturn: THIS
 
   compileNode: (o) ->
-    fragments = @expression.compileToFragments o
+    fragments = @expression.compileToFragments o, LEVEL_LIST
     unshiftAfterComments fragments, @makeCode 'throw '
     fragments.unshift @makeCode @tab
     fragments.push @makeCode ';'

[vendethiel]

That would also fiw throw try ....

[vendethiel]

Er, fix*.

[GeoffreyBooth]

@expression.compileToFragments o, LEVEL_LIST is interesting. Care to explain what that means? I've never fully understood the second parameter there.

[vendethiel]

Since CoffeeScript generates a JS string directly and not a JS ast, we carry around a notion of what context the node is gonna be compiled in, so we can generate expr/statement, surround with parentheses, etc not to mess with the precedence/... (like compiling {a:5} toplevel)

[connec]

That's a good way of putting it, re: there being no JS AST. It's exactly designed as a way of dealing with the situation in this PR (and #4644) - namely that a given JS expression (e.g. If, Obj, Op etc.) that's a child of another expression needs to be compiled in a certain way in order to be valid JS (e.g. wrapped in parens, closures, etc.).

[jashkenas]

Just to add some context -- this is what isStatement is supposed to be for!

Expressions are expressions, and their values can be used directly -- statements need to be closure-wrapped to be used as values.

There should be a way to use adjust the basic handling mechanism in compileToFragments to take care of this, instead of hardcoding a list of node types.

[GeoffreyBooth]

Well @expression.compileToFragments o, LEVEL_LIST seems to do the job. All the new tests I recently added pass, and simple code like throw new Error() aren’t wrapped in parentheses or closures. Thanks for the tip @connec!

[jashkenas]

Perfect!