New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Catch creates its own shared scope. #1476

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@taku0
Contributor

taku0 commented Jun 29, 2011

"catch e" should create its own shared scope with e in it.
Current implementation simply add e to current scope.
However, in compiled JavaScript, e is local to catch clause.
This mismatch breaks lexical scope of CoffeeScript.

Neither simply adding e to current scope, nor creating non-shared scope passes the following test:

test "catch statements should create shared scope with their argument", ->
  g = ->
    try
    catch e
    e = 2 # e should local to g
    try
      throw "error"
    catch e
      e = 3 # e should local to catch clause
      x = 1 # x should local to g, not catch clause

    ok e is 2
    ok x is 1

  e = 1 # this e should be different to e in g
  g()
  ok e is 1
Catch creates its own shared scope.
"catch e" create its own shared scope with e in it.
Neither simply adding e to current scope instead of creating new socpe,
nor creating non-shared scope pass the following test:

test "catch statements should create shared scope with their argument", ->
  g = ->
    try
    catch e
    e = 2 # e should local to g
    try
      throw "error"
    catch e
      e = 3 # e should local to catch clause
      x = 1 # x should local to g, not catch clause

    ok e is 2
    ok x is 1

  e = 1 # this e should be different to e in g
  g()
  ok e is 1
@satyr

This comment has been minimized.

Show comment
Hide comment
@satyr

satyr Jun 29, 2011

Collaborator

This might backfire due to JScript (which we must support) bug:

> cat 1476.js
e = 0
try { throw 1 } catch (e) { e = 2 }
WSH.echo(e)

> cscript 1476.js
2

@zimothy's fix is about as good as we can get.

Collaborator

satyr commented Jun 29, 2011

This might backfire due to JScript (which we must support) bug:

> cat 1476.js
e = 0
try { throw 1 } catch (e) { e = 2 }
WSH.echo(e)

> cscript 1476.js
2

@zimothy's fix is about as good as we can get.

@taku0

This comment has been minimized.

Show comment
Hide comment
@taku0

taku0 Jul 1, 2011

Contributor

I see @zimothy's fix matches JScript's behavior.
Using 'var' with scope.add instead of 'param' may be better, but there is no perfect solution since JScript's bug contradicts ECMAScript specification.

My patch doesn't do well with JScript after all. I close the request. Thanks.

Contributor

taku0 commented Jul 1, 2011

I see @zimothy's fix matches JScript's behavior.
Using 'var' with scope.add instead of 'param' may be better, but there is no perfect solution since JScript's bug contradicts ECMAScript specification.

My patch doesn't do well with JScript after all. I close the request. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment