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

Block scoping temporal dead zones #563

Closed
tolmasky opened this Issue Jan 21, 2015 · 17 comments

Comments

Projects
None yet
4 participants
@tolmasky
Contributor

tolmasky commented Jan 21, 2015

Basically, in ES6 lets don't hoist, but with 6to5, they do. This leads to this innocent snippet:

    i
    let i;

Working fine in 6to5, but actually being a ticking time bomb when you move to a real ES6 environment where it throws a "Reference Error not defined" and takes down the entire script because lets don't hoist. This seems particularly bad because its not a feature not working that then does work, but quite the opposite, a feature working that then crashes.

I haven't thought about it deeply yet, but I think its actually syntactically apparent when the variable is in the "temporal dead zone", and could simply be syntactically replaced with a throw:

    i
    let i;

--->

    throw "ReferenceError"
    var i;
@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Jan 21, 2015

Member

TDZ is hard. You basically have to implement complete runtime evaluation. Consider the following:

function foo() {
  x; // this is legal
}

let x;
function foo() {
  x; // this is illegal
}

foo();

let x;

It's impossible to discern between the two without actually evaluating the code.

Member

kittens commented Jan 21, 2015

TDZ is hard. You basically have to implement complete runtime evaluation. Consider the following:

function foo() {
  x; // this is legal
}

let x;
function foo() {
  x; // this is illegal
}

foo();

let x;

It's impossible to discern between the two without actually evaluating the code.

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Jan 21, 2015

Member

There's the optional blockScopingTDZ transformer but it's extremely naive and overzealous and doesn't take into consideration my above examples, it simply checks whether or not a reference appears before the declaration, which is subsequently why it's optional in the first place.

Member

kittens commented Jan 21, 2015

There's the optional blockScopingTDZ transformer but it's extremely naive and overzealous and doesn't take into consideration my above examples, it simply checks whether or not a reference appears before the declaration, which is subsequently why it's optional in the first place.

@kittens kittens changed the title from Deviation in let hoisting behavior can cause crashes when moving to real ES6 to Block scoping temporal dead zones Jan 21, 2015

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Jan 21, 2015

Member

I'll leave this open as I think this kind of discussion is important to have and I'd like a reference to point people towards if this question come up again.

Member

kittens commented Jan 21, 2015

I'll leave this open as I think this kind of discussion is important to have and I'd like a reference to point people towards if this question come up again.

@appden

This comment has been minimized.

Show comment
Hide comment
@appden

appden Jan 21, 2015

Contributor

Determining a TDZ violation at compile time is probably impossible (like the halting problem), but compiling these cases into something that throws a ReferenceError is possible, though would incur a runtime (and code size) cost that probably isn't worth doing, at least by default.

Contributor

appden commented Jan 21, 2015

Determining a TDZ violation at compile time is probably impossible (like the halting problem), but compiling these cases into something that throws a ReferenceError is possible, though would incur a runtime (and code size) cost that probably isn't worth doing, at least by default.

@tolmasky

This comment has been minimized.

Show comment
Hide comment
@tolmasky

tolmasky Jan 21, 2015

Contributor

Yeah I was just looking and it seems that the traceur people have come up with a (possibly non-performant) strategy: google/traceur-compiler#1382

But yeah, given the difficulty of this, perhaps just a mention in caveats would be enough (or perhaps just the failing let test is already enough)

Contributor

tolmasky commented Jan 21, 2015

Yeah I was just looking and it seems that the traceur people have come up with a (possibly non-performant) strategy: google/traceur-compiler#1382

But yeah, given the difficulty of this, perhaps just a mention in caveats would be enough (or perhaps just the failing let test is already enough)

@appden

This comment has been minimized.

Show comment
Hide comment
@appden

appden Jan 21, 2015

Contributor

That's exactly the approach I was thinking of. Fancy seeing you here @tolmasky. :)

Contributor

appden commented Jan 21, 2015

That's exactly the approach I was thinking of. Fancy seeing you here @tolmasky. :)

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Jan 21, 2015

Member

@tolmasky If it's disabled by default and non-performant then noone will use it.

Member

kittens commented Jan 21, 2015

@tolmasky If it's disabled by default and non-performant then noone will use it.

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Jan 21, 2015

Member

I guess I can implement that Traceur solution if users really want it. I'm just not willing to implement and maintain a feature that noone will use when my time can be better spent maturing the platform.

Although the tradeoff may be worth it for completeness.

Member

kittens commented Jan 21, 2015

I guess I can implement that Traceur solution if users really want it. I'm just not willing to implement and maintain a feature that noone will use when my time can be better spent maturing the platform.

Although the tradeoff may be worth it for completeness.

@appden

This comment has been minimized.

Show comment
Hide comment
@appden

appden Jan 21, 2015

Contributor

For the record, I'm not asking for it. I think your time is better spent on other issues. However, if it were available, then I'd add that optional transform to development builds.

Contributor

appden commented Jan 21, 2015

For the record, I'm not asking for it. I think your time is better spent on other issues. However, if it were available, then I'd add that optional transform to development builds.

@kittens kittens removed the cantfix label Jan 21, 2015

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Jan 21, 2015

Member

@appden Development builds are actually a good use-case that I hadn't considered. This shouldn't be too hard to implement so I'll try and get around to it sometime today.

Member

kittens commented Jan 21, 2015

@appden Development builds are actually a good use-case that I hadn't considered. This shouldn't be too hard to implement so I'll try and get around to it sometime today.

@tolmasky

This comment has been minimized.

Show comment
Hide comment
@tolmasky

tolmasky Jan 21, 2015

Contributor

I have kind of two competing feelings on this. On the one hand I agree that if its not default it probably won't get used (especially with an issue that is subtle, as opposed to turning on some crazy cool feature).

My other feeling is that this unfortunately is the intersection of "very easy error to make", and "potentially catastrophic when moving to ES6/iojs/whatever". That is to say, everyone is so used to hoisting that its totally reasonable to use this like a var, so given the promise that this will definitely get you ready to switch to ES6 when its ready, it would be very frustrating to have your node app then crash immediately on launch.

Contributor

tolmasky commented Jan 21, 2015

I have kind of two competing feelings on this. On the one hand I agree that if its not default it probably won't get used (especially with an issue that is subtle, as opposed to turning on some crazy cool feature).

My other feeling is that this unfortunately is the intersection of "very easy error to make", and "potentially catastrophic when moving to ES6/iojs/whatever". That is to say, everyone is so used to hoisting that its totally reasonable to use this like a var, so given the promise that this will definitely get you ready to switch to ES6 when its ready, it would be very frustrating to have your node app then crash immediately on launch.

@kittens kittens added this to the 3.0.0 milestone Jan 21, 2015

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Jan 22, 2015

Member

For complete support it'll have to explode

  • AssignmentExpression
  • UnaryExpression
  • UpdateExpression
  • Static references

It'll also need to support all operators and prefixes associated, as well as retain evaluation order.

Member

kittens commented Jan 22, 2015

For complete support it'll have to explode

  • AssignmentExpression
  • UnaryExpression
  • UpdateExpression
  • Static references

It'll also need to support all operators and prefixes associated, as well as retain evaluation order.

@appden

This comment has been minimized.

Show comment
Hide comment
@appden

appden Jan 22, 2015

Contributor

I was thinking every reference to a let x variable be transformed to (x == UNINITIALIZED ? throw new ReferenceError() : x). Essentially, that's what a ES6 engine would be doing under the hood (with many optimizations of course). There are so many edge cases that I don't think it's worth trying to be smarter than that considering this would be an optional, development-only (for sane people) transform.

Contributor

appden commented Jan 22, 2015

I was thinking every reference to a let x variable be transformed to (x == UNINITIALIZED ? throw new ReferenceError() : x). Essentially, that's what a ES6 engine would be doing under the hood (with many optimizations of course). There are so many edge cases that I don't think it's worth trying to be smarter than that considering this would be an optional, development-only (for sane people) transform.

@kittens kittens modified the milestones: 4.0.0, 3.0.0 Jan 26, 2015

@kittens kittens closed this in eb14f1d Feb 7, 2015

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Feb 7, 2015

Member

Implementation is a little hacky but it works great. References should be supported in all the contexts as mentioned in my previous comment.

Member

kittens commented Feb 7, 2015

Implementation is a little hacky but it works great. References should be supported in all the contexts as mentioned in my previous comment.

@pigulla

This comment has been minimized.

Show comment
Hide comment
@pigulla

pigulla Mar 5, 2015

Should this be working?

// test.js
i;
let i;
$ node_modules/.bin/babel-node -V
4.6.6

$ node_modules/.bin/babel-node test.js 
undefined
$ node_modules/.bin/babel test.js 
"use strict";

i;
var i = undefined;

Am I missing something?

pigulla commented Mar 5, 2015

Should this be working?

// test.js
i;
let i;
$ node_modules/.bin/babel-node -V
4.6.6

$ node_modules/.bin/babel-node test.js 
undefined
$ node_modules/.bin/babel test.js 
"use strict";

i;
var i = undefined;

Am I missing something?

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Mar 5, 2015

Member

@pigulla

$ babel-node --optional es6.blockScopingTDZ test.js 
Member

kittens commented Mar 5, 2015

@pigulla

$ babel-node --optional es6.blockScopingTDZ test.js 
@pigulla

This comment has been minimized.

Show comment
Hide comment
@pigulla

pigulla Mar 5, 2015

Ah, so I did miss something. Thanks!

pigulla commented Mar 5, 2015

Ah, so I did miss something. Thanks!

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