let/const #784
let/const #784
Conversation
This commit allows for the creation of let/const bindings and ensures all block-scoped bindings are installed into the appropriate scope. This involves creating new LexScopes, as outlined in `test/bindings/scope.js`. Before this change, the environment stack, i.e., frames, changesets, and scopes, would change in lockstep. Now, lex scopes change separately from frames and changesets via `push_lex` and `pop_lex`. When adding heap refinements, we may traverse over lex scopes to find the scope where the base object was bound. In unusual cases, like try/catch, envs may have an uneven number of scopes when merging. The uneven scopes will always be lex. I have updated `merge_env` to accept this imbalance. Note that this is not a complete implementation of let/const: * switch statements should forbid rebinding lexicals between cases * functions, amazingly, are block scoped *and* hoisted in ES2015 * numerous TDZ cases, surely, which I have not yet investigated * var bindings can not shadow a lexical binding, ever * more tests
Also fix comment typos / reword
Wrote the comment first, thinking what I was going to write. Then I wrote it differently, which turned out to be the correct way. Updating comment to match.
I'm referring to the `statement_decl` phase as phase 1 and the `toplevels` phase as phase 2 in this description. Before this change, rebinding errors were ignored in `Env_js.bind__entry`, but caught in `Env_js.init_value_entry`. This made it difficult to catch rebinding errors in switch statements, because phase 2 does some sophisticated env and changeset management, which makes it difficult to know when one case rebinds a name from a previous case. After this change, a simple, straightforward modification to `statement_decl` exposes rebinding errors in switch statements. There was a comment that indicated that the error messages were better if we deferred rebinding error handling. However, after this change, I saw no change in the error messages. Also, now that binding errors are captured in phase 1, it became necessary to add a suite of tests for inference behavior in phase 2, to ensure scopes are managed properly in both phases. I added tests around two inference behaviors that are affected by scoping: refinments and type widening. The added test cases also forced some real errors in `Env_js.merge_env` and `Env_js.copy_env`. I took this opportunity to tighten up the logic there.
For compatibility reasons, functions can shadow other vars/functions at the top level (global scope or function scope) only.
Before, we only forbade var/lex shadowing when they were both found in the same scope. However, a lexical may be in a nested scope.
Certain update expressions need to be forbidden, but are semantically different from assignment.
|
All right. This is about as far as I can get right now. Feedback on what's here is appreciated. I may have missed something, but TDZ should be the only part outstanding here. @bhosmer, did you have an approach in mind for handling that? I noticed you want to kill two birds with one stone here, and allow declaring non-maybe vars without an initializer. |
|
I have only briefly tried this out, but so far it seems to work as advertised! Great work! :) Will let you know if I run into any issues :) |
| which AST traversal takes place, these lookups may legitimately violate | ||
| rule #2, hence the need for a special mode. | ||
| *) | ||
| module LookupMode = struct | ||
| type t = ForValue | ForType | ForTypeof | ||
| type t = ForValue | ForUpdate | ForType | ForTypeof |
There was a problem hiding this comment.
Per IRC, if you take the modes path, I'd suggest
type t = ForValue of value_op | ForType | ForTypeof
and value_op = Read | Write
The important thing is to keep the modes nonoverlapping, and pushing the read/write distinction down into ForValue will also spare you the occasional mode = ForValueRead || mode = ForValueWrite down below.
There was a problem hiding this comment.
OK, so I went down this path and things got pretty ugly. Happy to go over it in more depth, but I think the alternative approach of defining a set_var-like entrypoint for Update/PlusAssign. As far as a name for that entrypoint... any ideas? To be honest, I don't really see the problem with ForUpdate, so maybe this is a better change for you to make separately/subsequently?
|
Is TDZ compliance possible to statically check for in general? Babel only supports using runtime checks because its earlier attempt at static checking had problems. For what it's worth, Flow already misses TDZ-like issues with vars: /* @flow */
console.log(s.length);
var s = "abc";/* @flow */
f();
var s = "abc";
function f() {
console.log(s.length);
} |
| match get_entry name scope with | ||
| (* nested scope var shadows a lex-scoped lexical *) | ||
| | Some prev | ||
| when is_lex scope && Entry.is_lex prev -> |
There was a problem hiding this comment.
is_lex scope is unnecessary here...
The previous logic was pretty twisted--a side effect of my tacking on some extra logic hastily. The key thing here is that we need to actually check for existing entries in a lex scope, even if we're trying to add a var-scoped binding, because any shadowing in a lex scope is bad. Also, it turns out that I didn't handle types shadowing lexicals, so bully for that.
This resolves an issue which I misdiagnosed, causing me to add unnecessary cases to Env_js.merge_env, which could mask other real errors. What's happening here: We enter a catch clause, which pushes a new lex scope. Before we can pop the lex scope, we hit an abnormal and jump away. Then, we merge the env that has a dangling lex scope on top. In general, we run this risk whenever we push a lex scope and then process statements that might raise. I have convinced myself that no other code that calls merge_env will hit this specific case, but a generalized approach would be nice, too.
We needed these before, when we had code that referenced the whole list, but that code is gone now.
An earlier commit removed similar code from merge_env, which was caused by a separate bug.
The added case isn't strictly required, but @bhosmer likes this more, and that's enough for me. Also, it's unnecessary to check if the Entry.is_lex, because lex scopes imply that.
|
👏 👏 👏 👏 👏 |
|
Huzzah! Great work! 🎆 💥 👏 ✋ 👍 👌 🍻 |
|
🎉 🎉 🎉 |
|
Here’s a quick jscodeshift script you can use to transform all your http://felix-kling.de/esprima_ast_explorer/#/tGblyJN52T/6 Flow and unit tests caught the two issues my current project codebase had post-transform. Both are documented in the transform. I’ll update it again after #831 lands. |
This commit allows for the creation of let/const bindings and ensures
all block-scoped bindings are installed into the appropriate scope. This
involves creating new LexScopes, as outlined in
test/bindings/scope.js.Before this change, the environment stack, i.e., frames, changesets, and
scopes, would change in lockstep. Now, lex scopes change separately from
frames and changesets via
push_lexandpop_lex.When adding heap refinements, we may traverse over lex scopes to find
the scope where the base object was bound.
In unusual cases, like try/catch, envs may have an uneven number of
scopes when merging. The uneven scopes will always be lex. I have
updated
merge_envto accept this imbalance.Note that this is not a complete implementation of let/const: