Let/const #431

samwgoldman · 2015-04-29T22:51:57Z

Rebased #404 on top of the block→scope rename.

Left for future work

very sophisticated TDZ
let blocks
let expressions
let/const in lib declarations

In order to support let/const, we need to know what kind of scope each block is. Currently, the only blocks we have are what I am calling "Hoist" blocks. Whenever an entry which should be hoisted is added to the env, it should be added to the closest "Hoist" block. Let/const identifiers should be added to the nearest "Block" block. Currently we only create blocks corresponding to hoist points. Subsequent commits will need to create a great many more blocks to support block-level scope. NB: We should probably rename "block" to "scope."

The big change here is that I changed `init_env` to take an extra boolean parameter, which when true indicates that the nearest "Hoist" block should be extended. Obviously, there is a lot of logic missing here. I suspect I'll need to add block scope information to the block entries themselves, but so far the only roadblock I hit I don't quite understand, so I'm reaching out.

Also restore "basic" test for let and duplicate let tests for const.

Also reorganize a bit: common.js has features that are relevant to both let and const.

Freebie from the parser!

I extracted variable_declaration from outside of statement_decl. I changed the name to variable_decl to mirror statement_decl, but would be happy to revert that. I don't totally understand the change I made to widen_env, so please take a close look there. This is very similar to a change I made in clear_env, where the modification happens in all scopes up to the closest VarScope. Once I understand this better, and if it is clear that is the correct thing to do, that pattern should be extracted out into a function.

jeffmo · 2015-04-30T14:35:15Z

src/typing/constraint_js.ml

-type scope = scope_entry SMap.t ref
+
+type scope_kind =
+  | VarScope (* var, class, functions hoisted up to this point *)


FYI: Classes are actually let-scoped

samwgoldman · 2015-04-30T19:56:15Z

To handle some TDZ cases and const reassignment, I'll need to track more information that I do now about the scope entries. I have an idea for how I can proceed with that, but if anyone has a preference for how I should proceed, I appreciate all input.

samwgoldman · 2015-04-30T20:02:42Z

Regarding TDZ, I'm not sure it's possible to (in general) prevent errors like this:

function bar() {
  console.log(foo);
}

bar();

let foo = "foo";

because the following is valid:

function bar() {
  console.log(foo);
}

let foo = "foo";

bar();

Thoughts?

jeffmo · 2015-04-30T21:00:50Z

Right, some TDZ errors for let are definitely going to be harder (and sometimes impossible) to identify statically. We should try do best-effort here, but maybe that kind of granularity could be a added in more detail a follow-up PR?

For now, same-env TDZ errors seems like a good start

1. function scopes (VarScope) are lexical, too 2. vars hoist outside of non-var scopes

samwgoldman · 2015-04-30T23:01:56Z

src/typing/type_inference_js.ml

@@ -870,7 +865,7 @@ and statement_decl cx = Ast.Statement.(
      let _, { Ast.Identifier.name; _ } = id in
      let r = mk_reason (spf "type %s" name) loc in
      let tvar = Flow_js.mk_tvar cx r in
-      Env_js.init_env cx name (create_env_entry ~for_type:true tvar tvar (Some loc))
+      Env_js.init_env cx name (create_env_entry ~for_type:true tvar tvar (Some loc)) VarScope (* TODO: hoist behavior *)


@jeffmo What do you think the scoping behavior for type aliases should be?

Definitely var-scoped -- type aliases need recursion

This is important because type aliases can be mutually recursive. I added a test for this case, even though it currently passes with a non-var lexical scope, as we haven't implemented any TDZ yet.

samwgoldman · 2015-05-01T01:08:16Z

src/typing/env_js.ml

@@ -327,7 +401,7 @@ let string_of_env cx ctx =
 let install_refinement cx x xtypes =
  if exists_in_scope x (peek_env ()) = None then
    let t = SMap.find_unsafe x xtypes in
-    init_env cx x (create_env_entry t t None)
+    init_env cx x (create_env_entry t t None) VarScope (* TODO: verify correct hoist behavior *)


I'm pretty sure that the scope of the refinement depends on the type of entry corresponding to x. Furthermore, code that relies on exists_in_scope/peek_env instead of exists_in_env/scope_kind is generally suspect.

Unfortunately, we don't generally know the scope kind of the entry here, and I'm not even sure we can thread it through. The refinement logic is still a bit opaque to me, but I'll try to come up with some test cases to stress this code. Help here would be appreciated.

OK, so I think I have two tests that show the refinement scope needs to depend on the scope of the thing being refined.

Here is why just VarScope doesn't work:

function scope() { { let x: {p:?string} = {p:"xxx"}; if (x.p == null) { return; } } x.p // should fail, but $REFI x.p is hoisted to here, so there is no warning }

Here is why just LexicalScope doesn't work:

function scope() { var x: {p:?string} = {p:"xxx"}; { if (x.p == null) { return; } } var y: string = x.p // should pass, but refinement doesn't live long enough }

@gabelevi, you helped me understand this refinement stuff in the first place (thanks!). Do you see any problem with my reasoning here? It seems like the scope_kind of the refinement needs to depend on the scope_kind of the thing being refined (x in this case).

If that's true, we currently don't store that information beyond init_env, so I believe I'd need to add scope_kind to the scope_entry struct. (If true, this isn't so bad, because I think we need to add this kind of information to support TDZ use cases anyway.)

I've implemented this in 4659a41. Mostly sure it's correct :)

Only `declare var` syntax is implemented. There is no `declare let` or `declare const`.

Modules and imports can only ever be at the top level, and imports should be hoisted.

nmn · 2015-06-29T11:33:52Z

While work on Let/Const continues, is there a way to tell flow to treat Let and Const as simple vars and continue to work.

Many of us have been using Babel for our code, and use Let and Const instead of var everywhere. The fact that const isn't re-assigned is checked by Babel.

But flow completely fails on such code. Having it treat Let and Const the same as var for now would be a great temporary fix. I tried to make a change in the source itself, but I don't know enough OCaml. :(

winkler1 · 2015-06-29T16:55:34Z

Voting for @nmn's suggestion. I'm on an es6 codebase, want to get Flow going. Choices are to change lets/const -> var (and incur the political capital of selling that to team), or not using Flow. Would much rather use Flow sooner than later. An interim flag to treat let/const as "var" would be ideal.

samwgoldman · 2015-07-02T20:31:16Z

@nmn @winkler1 Treating let and const as var isn't a great option because it isn't safe. It's easy to create an example that has runtime errors if you treat all variable declarations as var declarations, but flow would happily accept:

function bad1() {
  if (true) {
    let foo = "local var";
  }
  return foo; // out of scope, but flow doesn't know
}

So, unfortunately, you'll have to sit tight a bit longer while we get this right. Sorry for the delay!

pluma · 2015-07-02T21:44:12Z

Flow might not catch it but eslint (with babel-eslint) sure does. Flow is primarily a type checker, so I'd think most people interested in reporting like that also use a linter, if only as a backup.

+1 for doing the wrong thing before doing the right thing.

nmn · 2015-07-03T00:50:02Z

Doing the wrong thing in the mean time was only a suggestion. I'm not sure how much sense it makes from a time commitment point of view.

But while I understand the fact that it would not catch errors, it's still good to be able to at least flow. As of now, it's more of a all or nothing approach. The Let/Const pull request has been around for a while, and I've been watching it patiently for a while. If it's close to being merged than this whole discussion is pointless anyway!

samwgoldman · 2015-07-03T01:11:25Z

If you are truly intent on treating let/const as var, it's not hard to make that happen in a fork. Just change the Const and Let branches in the variable_declaration function to be the same as the Var branch. I don't think such a change will be merged upstream, but it will be very simple to keep that change up-to-date with master.

Another possibility: You might be able to run babel with a limited number of transforms using the "whitelist" option. If you just whitelisted the "es6.blockScoping" transform, flow could process the post-transformed source, and you wouldn't run into the pitfalls of using flow on fully transformed source files.

Lastly, this feature branch is actually feature complete—but it's pending some internal refactoring. I haven't bothered to keep this up-to-date with master, so it's a bit behind on other features. If you want to go ahead and use this branch, please do let me know if you run into any issues.

nmn · 2015-07-05T03:54:19Z

@samwgoldman thanks for pointing me to the right place. I am finally able to use flow on my code!

bsr203 · 2015-07-06T22:04:43Z

+1 Thank for working on supporting let/const and hope it gets merged soon

fredericksilva · 2015-07-06T22:05:55Z

👍

nmn · 2015-07-07T08:58:46Z

@bsr203 @fredericksilva If you are using babel to transpile your code, you may be happy enough with my fork of flow: github.com/nmn/flow
I've just made a couple of small edits that makes flow treat let/const the same way as var. It won't catch scoping mistakes, and may still complain about scoping incorrectly, but in the predominant case it will be just fine. Plus you can actually start using flow now if let/const is a blocker for you.

bsr203 · 2015-07-07T12:22:15Z

thanks @nmn your fork let me use flow now !

bhosmer · 2015-07-14T17:46:17Z

Quick interim update - unf. delay will last a little longer, it's due to some refactoring I need to land in front of Sam's awesome work. I'm back on the grid end of this week, will update status here.

fredericksilva · 2015-07-14T18:17:58Z

Thanks @nmn, let's see how your work flow. 👊

midinastasurazz · 2015-07-27T16:38:40Z

@bhosmer Any updates on this?

matklad · 2015-08-05T15:27:19Z

Is const + destructing in scope of this PR? At the moment, code like const [x] = [92] fails with "Assignment to constant variable x"

samwgoldman · 2015-08-05T21:54:59Z

@matklad Thanks for the report! Looks like I mishandled destructuring, so I'm throwing an unnecessary error there. I will add a test case for that once the refactoring lands and I pick this up again.

mroch · 2015-08-14T02:19:57Z

i'm not keeping up here, so apologies if this is already handled, but this is seriously wtf so hopefully we can detect it properly: http://es-discourse.com/t/why-typeof-is-no-longer-safe/15

samwgoldman · 2015-08-14T02:30:13Z

@mroch We do handle a number of TDZ errors, although it's worth adding an explicit test case for typeof, in case that code goes through a different code path. Here's the code that checks, although it probably (hopefully) won't survive the upcoming refactor.

samwgoldman · 2015-08-26T19:10:28Z

All right. Looks like the refactoring landed (@bhosmer, let me know if there's more in the pipeline), so now I'm the one blocking this.

bhosmer · 2015-08-27T20:56:57Z

@samwgoldman nothing imminent - the one nontrivial remaining loose end is far less important than your stuff and can wait. Definitely happy to walk through the new stuff to whatever extent you want BTW.

meagar · 2015-09-03T19:40:54Z

+1

samwgoldman · 2015-09-06T05:26:34Z

superseded by #784

samwgoldman added 8 commits April 29, 2015 10:34

Allow const variable declarations

2b27342

Also restore "basic" test for let and duplicate let tests for const.

Add outstanding let/const features as TODO tests

309d0da

Also reorganize a bit: common.js has features that are relevant to both let and const.

Const variables must be initialized

760c1e9

Freebie from the parser!

Use scope_kind variant instead of hoist boolean

d07d944

Simplify flat_env inner loop definition

87a59ff

Add pending test for class declaration scope

8fb878c

samwgoldman mentioned this pull request Apr 29, 2015

[WIP] Let/const #404

Closed

12 tasks

facebook-github-bot added the CLA Signed label Apr 29, 2015

jeffmo reviewed Apr 30, 2015
View reviewed changes

Fix inaccurate comment

222d411

samwgoldman added 3 commits April 30, 2015 11:32

Add explicit tests for hoisting edge cases

6642876

1. function scopes (VarScope) are lexical, too 2. vars hoist outside of non-var scopes

Create new lexical scope let-bound variables in for-in loops

5e063da

Classes are scoped lexically

c6ddef1

samwgoldman reviewed Apr 30, 2015
View reviewed changes

samwgoldman added 3 commits April 30, 2015 13:08

Functions are indeed var-scoped

6ed936c

Type aliases should be var-scoped

03c8a4b

This is important because type aliases can be mutually recursive. I added a test for this case, even though it currently passes with a non-var lexical scope, as we haven't implemented any TDZ yet.

Classes and interfaces are declared in a lexical scope

40fca43

samwgoldman reviewed May 1, 2015
View reviewed changes

samwgoldman added 2 commits April 30, 2015 15:13

Declared variables and functions are var-scoped

d1d3b32

Only `declare var` syntax is implemented. There is no `declare let` or `declare const`.

Modules and imports are OK as VarScope

99e4262

Modules and imports can only ever be at the top level, and imports should be hoisted.

yanivtal mentioned this pull request Jul 22, 2015

Remove Flow types for now reduxjs/redux#299

Merged

jeffmo added the do-not-reap label Jul 24, 2015

friday mentioned this pull request Jul 27, 2015

Support for ES6 features #62

Closed

vslinko mentioned this pull request Jul 28, 2015

Use block scoped variables everywhere reactjs/react-future#38

Closed

willheslam mentioned this pull request Aug 30, 2015

Babel moves comment position in function arguments babel/babel#2291

Closed

paulshen mentioned this pull request Sep 5, 2015

Allow const let #783

Closed

samwgoldman closed this Sep 6, 2015

goodmind added the Superseded PRs that solve the same issue as a "superseding" PR that already landed label Jul 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let/const #431

Let/const #431

samwgoldman commented Apr 29, 2015

jeffmo Apr 30, 2015

samwgoldman commented Apr 30, 2015

samwgoldman commented Apr 30, 2015

jeffmo commented Apr 30, 2015

samwgoldman Apr 30, 2015

jeffmo May 1, 2015

samwgoldman May 1, 2015

samwgoldman May 10, 2015

samwgoldman May 20, 2015

nmn commented Jun 29, 2015

winkler1 commented Jun 29, 2015

samwgoldman commented Jul 2, 2015

pluma commented Jul 2, 2015

nmn commented Jul 3, 2015

samwgoldman commented Jul 3, 2015

nmn commented Jul 5, 2015

bsr203 commented Jul 6, 2015

fredericksilva commented Jul 6, 2015

nmn commented Jul 7, 2015

bsr203 commented Jul 7, 2015

bhosmer commented Jul 14, 2015

fredericksilva commented Jul 14, 2015

midinastasurazz commented Jul 27, 2015

matklad commented Aug 5, 2015

samwgoldman commented Aug 5, 2015

mroch commented Aug 14, 2015

samwgoldman commented Aug 14, 2015

samwgoldman commented Aug 26, 2015

bhosmer commented Aug 27, 2015

meagar commented Sep 3, 2015

samwgoldman commented Sep 6, 2015

Let/const #431

Let/const #431

Conversation

samwgoldman commented Apr 29, 2015

jeffmo Apr 30, 2015

Choose a reason for hiding this comment

samwgoldman commented Apr 30, 2015

samwgoldman commented Apr 30, 2015

jeffmo commented Apr 30, 2015

samwgoldman Apr 30, 2015

Choose a reason for hiding this comment

jeffmo May 1, 2015

Choose a reason for hiding this comment

samwgoldman May 1, 2015

Choose a reason for hiding this comment

samwgoldman May 10, 2015

Choose a reason for hiding this comment

samwgoldman May 20, 2015

Choose a reason for hiding this comment

nmn commented Jun 29, 2015

winkler1 commented Jun 29, 2015

samwgoldman commented Jul 2, 2015

pluma commented Jul 2, 2015

nmn commented Jul 3, 2015

samwgoldman commented Jul 3, 2015

nmn commented Jul 5, 2015

bsr203 commented Jul 6, 2015

fredericksilva commented Jul 6, 2015

nmn commented Jul 7, 2015

bsr203 commented Jul 7, 2015

bhosmer commented Jul 14, 2015

fredericksilva commented Jul 14, 2015

midinastasurazz commented Jul 27, 2015

matklad commented Aug 5, 2015

samwgoldman commented Aug 5, 2015

mroch commented Aug 14, 2015

samwgoldman commented Aug 14, 2015

samwgoldman commented Aug 26, 2015

bhosmer commented Aug 27, 2015

meagar commented Sep 3, 2015

samwgoldman commented Sep 6, 2015