Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/coffeescript/nodes.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion src/nodes.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -3255,7 +3255,7 @@ exports.Throw = class Throw extends Base
makeReturn: THIS

compileNode: (o) ->
fragments = @expression.compileToFragments o
Copy link
Collaborator

@connec connec Aug 25, 2017

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 ';'

Copy link
Collaborator

Choose a reason for hiding this comment

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

That would also fiw throw try ....

Copy link
Collaborator

Choose a reason for hiding this comment

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

Er, fix*.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.).

fragments = @expression.compileToFragments o, LEVEL_LIST
unshiftAfterComments fragments, @makeCode 'throw '
fragments.unshift @makeCode @tab
fragments.push @makeCode ';'
Expand Down
68 changes: 55 additions & 13 deletions test/exception_handling.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -89,31 +89,23 @@ test "try/catch with empty catch as last statement in a function body", ->
catch err
eq nonce, fn()


# Catch leads to broken scoping: #1595

test "try/catch with a reused variable name.", ->
test "#1595: try/catch with a reused variable name", ->
# `catch` shouldn’t lead to broken scoping.
do ->
try
inner = 5
catch inner
# nothing
eq typeof inner, 'undefined'


# Allowed to destructure exceptions: #2580

test "try/catch with destructuring the exception object", ->

test "#2580: try/catch with destructuring the exception object", ->
result = try
missing.object
catch {message}
message

eq message, 'missing is not defined'



test "Try catch finally as implicit arguments", ->
first = (x) -> x

Expand All @@ -130,8 +122,8 @@ test "Try catch finally as implicit arguments", ->
catch e
eq bar, yes

# Catch Should Not Require Param: #2900
test "parameter-less catch clause", ->
test "#2900: parameter-less catch clause", ->
# `catch` should not require a parameter.
try
throw new Error 'failed'
catch
Expand All @@ -140,3 +132,53 @@ test "parameter-less catch clause", ->
try throw new Error 'failed' catch finally ok true

ok try throw new Error 'failed' catch then true

test "#3709: throwing an if statement", ->
# `throw if` should return a closure around the `if` block, so that the
# output is valid JavaScript.
try
throw if no
new Error 'drat!'
else
new Error 'no escape!'
catch err
eq err.message, 'no escape!'

try
throw if yes then new Error 'huh?' else null
catch err
eq err.message, 'huh?'

test "#3709: throwing a switch statement", ->
i = 3
try
throw switch i
when 2
new Error 'not this one'
when 3
new Error 'oh no!'
catch err
eq err.message, 'oh no!'

test "#3709: throwing a for loop", ->
# `throw for` should return a closure around the `for` block, so that the
# output is valid JavaScript.
try
throw for i in [0..3]
i * 2
catch err
arrayEq err, [0, 2, 4, 6]

test "#3709: throwing a while loop", ->
i = 0
try
throw while i < 3
i++
catch err
eq i, 3

test "#3789: throwing a throw", ->
try
throw throw throw new Error 'whoa!'
catch err
eq err.message, 'whoa!'