Skip to content
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
Closed

catch does not create its own lexical environment #2422

michaelficarra opened this issue Jul 5, 2012 · 11 comments

Comments

@michaelficarra
Copy link
Collaborator

... 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
Copy link
Collaborator

satyr commented Jul 5, 2012

#1476

@michaelficarra
Copy link
Collaborator Author

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

@jashkenas
Copy link
Owner

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

@michaelficarra
Copy link
Collaborator Author

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
Copy link

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
Copy link
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
Copy link
Collaborator Author

@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
Copy link

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

@erisdev
Copy link

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
Copy link
Owner

Sounds good to me.

@jashkenas
Copy link
Owner

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
Projects
None yet
Development

No branches or pull requests

5 participants