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 does not create its own lexical environment #2422

Closed
michaelficarra opened this Issue Jul 5, 2012 · 11 comments

Comments

Projects
None yet
5 participants
@michaelficarra
Collaborator

michaelficarra commented Jul 5, 2012

... which causes us to leak globals:

$ coffee -bsp
try fn() catch e then
e = 0

try {
  fn();
} catch (e) {

}

e = 0;

e should be let-scoped to the catch block (see ES5 §12.14). The e outside should be seen as the first assignment to e and therefore auto-declared at the top of its closest containing scope.

@satyr

This comment has been minimized.

Show comment
Hide comment
@satyr

satyr Jul 5, 2012

Collaborator

#1476

Collaborator

satyr commented Jul 5, 2012

#1476

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Jul 5, 2012

Collaborator

Ah, I couldn't find that. @jashkenas: what should we do here? I say fuck JScript.

Collaborator

michaelficarra commented Jul 5, 2012

Ah, I couldn't find that. @jashkenas: what should we do here? I say fuck JScript.

@jashkenas

This comment has been minimized.

Show comment
Hide comment
@jashkenas

jashkenas Jul 5, 2012

Owner

What's the argument for which would be more useful?

Owner

jashkenas commented Jul 5, 2012

What's the argument for which would be more useful?

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Jul 5, 2012

Collaborator

It depends: either decision is a cognitive load we are forcing upon our users. If we keep the current semantics, CS is internally consistent, but inconsistent with standard JS. If we make the change, we're adopting the odd catch scoping semantics from JS, and forcing our users (assuming they all care about IE) to account for the JScript behaviour.

Potential errors with the current implementation: accidentally leaking globals. Potential errors with the proposed implementation: IE executions will potentially overwrite a variable that was assumed to be shadowed. I believe the catch variable may also leak to its containing scope if undeclared there, but I don't have a windows machine to test it on.

I say we go with my proposed solution. The negative behaviour with that solution will be much less commonly encountered.

Collaborator

michaelficarra commented Jul 5, 2012

It depends: either decision is a cognitive load we are forcing upon our users. If we keep the current semantics, CS is internally consistent, but inconsistent with standard JS. If we make the change, we're adopting the odd catch scoping semantics from JS, and forcing our users (assuming they all care about IE) to account for the JScript behaviour.

Potential errors with the current implementation: accidentally leaking globals. Potential errors with the proposed implementation: IE executions will potentially overwrite a variable that was assumed to be shadowed. I believe the catch variable may also leak to its containing scope if undeclared there, but I don't have a windows machine to test it on.

I say we go with my proposed solution. The negative behaviour with that solution will be much less commonly encountered.

@erisdev

This comment has been minimized.

Show comment
Hide comment
@erisdev

erisdev Jul 6, 2012

Would it be possible to mangle the variable name in the catch block to satisfy both conditions?

Better yet, maybe

try fn() catch e then
e = 0

should compile to

var e;

try {
  fn();
} catch (_1) {
  e = _1;
}

e = 0;

to maintain internal consistency, not break JScript, &c.

erisdev commented Jul 6, 2012

Would it be possible to mangle the variable name in the catch block to satisfy both conditions?

Better yet, maybe

try fn() catch e then
e = 0

should compile to

var e;

try {
  fn();
} catch (_1) {
  e = _1;
}

e = 0;

to maintain internal consistency, not break JScript, &c.

@satyr

This comment has been minimized.

Show comment
Hide comment
@satyr

satyr Jul 6, 2012

Collaborator
var e;
...
} catch (_1) {
  e = _1;

Yep, that would solve the conflict. It would further simplify our scope rules (function scope all the way), and might actually be useful as we often see the pattern catch e then err = e.

Collaborator

satyr commented Jul 6, 2012

var e;
...
} catch (_1) {
  e = _1;

Yep, that would solve the conflict. It would further simplify our scope rules (function scope all the way), and might actually be useful as we often see the pattern catch e then err = e.

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Jul 6, 2012

Collaborator

@erisdiscord: That won't scope e to the catch block, mimicking JS semantics. Though it would make CS scoping semantics more consistent.

edit: @satyr beat me to it.

Collaborator

michaelficarra commented Jul 6, 2012

@erisdiscord: That won't scope e to the catch block, mimicking JS semantics. Though it would make CS scoping semantics more consistent.

edit: @satyr beat me to it.

@paulmillr

This comment has been minimized.

Show comment
Hide comment
@paulmillr

paulmillr Jul 6, 2012

+1 for var, i'd ran into this issue already.

paulmillr commented Jul 6, 2012

+1 for var, i'd ran into this issue already.

@erisdev

This comment has been minimized.

Show comment
Hide comment
@erisdev

erisdev Jul 6, 2012

@michaelficarra yep, I think it's a worthwhile break from "correct" JavaScript semantics, both for consistency and for the sake of that common pattern @satyr mentioned.

erisdev commented Jul 6, 2012

@michaelficarra yep, I think it's a worthwhile break from "correct" JavaScript semantics, both for consistency and for the sake of that common pattern @satyr mentioned.

@jashkenas

This comment has been minimized.

Show comment
Hide comment
@jashkenas

jashkenas Jul 6, 2012

Owner

Sounds good to me.

Owner

jashkenas commented Jul 6, 2012

Sounds good to me.

@jashkenas

This comment has been minimized.

Show comment
Hide comment
@jashkenas

jashkenas Mar 5, 2013

Owner

Take a look at the above patch/diff, and let me know if that doesn't look alright.

Owner

jashkenas commented Mar 5, 2013

Take a look at the above patch/diff, and let me know if that doesn't look alright.

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