[WIP] Let/const #404
[WIP] Let/const #404
Conversation
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."
samwgoldman
changed the title
|
cc @avikchaudhuri as well to follow along |
Doh.
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. I also noticed a really strange omission, in which block statements were not actually processed with statement_decl, just toplevels. I added that in this commit. There should be just one test failure here, in break.js, which I can't really understand. I'd really appreciate some more experienced eyes on this one to help me figure out what's going on. 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.
|
The test failure in that last commit is throwing me for a loop. I'll look again tomorrow, but if anyone has some insight, I'd appreciate it. |
This fixes a test failure for the break examples where refined environment was not properly reset after an abnormal exit. Since the non-Hoist blocks are new, clearing up to the Hoist block should restore the previous behavior of this function.
Also restore "basic" test for let and duplicate let tests for const.
|
OK! Figured that last error out. Some preliminary feedback on this approach would be really helpful. Also, if anyone has a real-life project with let/const, and is able to try out this branch on their project, I'd be happy to look into any issues. |
Also reorganize a bit: common.js has features that are relevant to both let and const.
Freebie from the parser!
|
Haven't gotten to the meatier portion of the PR yet, but wanted to send what I have so far before I run to lunch. Will come back to finish out the comments later today/tonight |
| @@ -507,7 +507,16 @@ type block_entry = { | |||
| def_loc: Spider_monkey_ast.Loc.t option; | |||
| for_type: bool; | |||
| } | |||
| type block = block_entry SMap.t ref | |||
|
|
|||
| type block_scope = | |||
There was a problem hiding this comment.
Let's call this "scope_kind" (i.e. odd that the name seems to imply one of the variants but not the other -- in fact the issue really is that "block" means two different things now, but more on that in the next comment...)
|
I completely agree about the renames, and it's encouraging that your suggestions are so in line with what I was thinking. The only reason I hesitated to rename now is that I didn't want to cause merge difficulties down the road, but I think a bit of upfront pain is better than wonky names. And hey, it's not my problem. ;) |
| let block = { scope = Block; entries } in | ||
| Env_js.push_env block; | ||
|
|
||
| List.iter (statement_decl cx) body; |
There was a problem hiding this comment.
Wan to call out this bit specifically. Is it wrong to jump back into the statement_decl codepath from the statement codepath like this? If this is wrong, then that somewhat invalidates my approach here.
There was a problem hiding this comment.
I think this is ok -- it's essentially what functions currently do (except way over in the mk_body function)
|
Heh. Well if it's not much harder for you, making the renames in a separate PR (that this one could be considered "stacked" on top of) might make my life a little easier :) We can just make sure they get merged in proper order |
|
Yeah, I think it makes sense to do a separate PR that renames |
|
not sure where you guys ended up on the naming of the two different types of scopes ( |
|
I can get behind that naming, for sure. |
|
I've always found "lexically scoped" pretty confusing (both are "lexical"). I could live with it if someone feels strongly, but I'd be even happier with "VarScope" and "LetScope" |
|
yeah I find it non-optimal too, but since we're following a spec I thought it might be easier if they matched so that you don't have to wonder about/remember the translation between the code and the spec. if we name one |
|
Ok, will leave it up to you @samwgoldman -- I can live with either I guess |
|
We're talking about these definitions, correct? It makes sense to me that Since it makes sense to me, and I like the idea of naming things close to the spec, I'll try it out and we can see how it feels. |
|
I was actually looking at these: http://people.mozilla.org/~jorendorff/es6-draft.html#sec-block-static-semantics-lexicallyscopeddeclarations (and similar sections all over section 13) |
|
Closing in favor of rebased #431 |
I've just started digging in my heels on this one. I'm keeping this in an open PR to get feedback and ensure I don't step on anyone's toes.
TODO:
Overview:
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.