Allow legal const / uninitialized destructuring uses

[samwgoldman]

There are a couple of separate issues here:

1. Destructuring assignment used Env_js.set_var, which is intended for
   modification and prevents initializing const-bound destructured
   patterns. I fixed this by using the same destructuring helper, but
   using the appropriate Env_js.init_* function instead of set_var.
2. The parser rejects destructuring patterns without initialization in
   positions where it is legal. Specifically in for-of and for-in loops,
   where the initialization is implied. I fixed this by adding an
   implicit_init state to the parser.

For parsing, it's a bit complicated because we don't know if init is
implied until we reach the T_IN or T_OF token. I employed
backtracking here, so we first parse as if init were implied, but if it
turns out we are not in a for-in/for-of, we start over without implied
init, because it's a plain for loop. Is there a more efficient method?

Fixes #830 and #805

[gabelevi]

Using the Try module is a little dangerous for performance, so we need to be pretty judicious with its use. I'm not 100% sure we need it for this case. Can we do something like

1. expect T_FOR
2. expect T_LPAREN
3. parse init declaration
4. Decide which kind of loop to use
   • If the init declaration is initialized, assume for loop
   • else if the next token is T_OF assume for-of loop
   • else if the next token is T_IN assume for-in loop
   • else assume for loop

If we do need to use Try, it's important to make sure we try the most likely path first. I'm guessing that for is more popular than for-of and for-in so we probably would want to try that first. The pattern I'm starting to see around the Try module is

1. Try most likely path.
2. If there are errors, try less likely path
3. If there are errors, go back to most likely path, since those errors are more likely the ones we want to show.

[samwgoldman]

Sure, I can give that a shot. To add some more details to what you wrote, we should parse the init declaration with implicit_init, so we don't add errors for uninitialized const/destructuring. However, if it turns out that we're in a for loop, we need to re-add those errors by inspecting the parsed AST—that's some duplicate logic I was trying to avoid with backtracking, but it's probably worth it for improved performance.

[gabelevi]

From the parser's point of view, it can use the existence of an uninitialized init as evidence that this MUST be a for-of or a for-in. So if you have a pattern with uninitialized init, and then come across a ; (indicating a regular for loop) where you expect an in or of, the parser can throw an unexpected token.

If you want to give a better error, then yeah, you can do a little more work to keep track of where there is an uninitialized init.

Either way, you can probably just make variable_declaration_list return a list of the uninitialized init errors and decide later whether to throw them.

[samwgoldman]

@gabelevi OK, I changed up the implementation to collect the uninitialized errors and raise them from relevant call sites (ignored in for-of/for-in).

[gabelevi]

Sweet, this looks good to me! Thanks for bearing with me!

[gabelevi]

@facebook-github-bot shipit

[facebook-github-bot]

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/810188975768862/int_phab to review.