[CS2] Fix destructuring bugs #4673 and #4657

[helixbass]

@GeoffreyBooth in looking at #4673, it turned out to be related in a way to #4657 (which is a bug and should be open). So this should fix both

#4673 has a lot of other broken cases (really most non-identifier splat "variables" in an object spread destructure) eg {a.b...} = c, {{d}...} = e. Including corresponding broken cases for destructured object params eg ({@a...}) ->. I think all of these (including @connec's example in #4673) should compile based on what ES6 allows and what types of "variables" are allowed in other types of destructurings. But all are currently emitting crazy declarations for the "variable" eg var a.b

So it seemed like the cleanest way to get them all compiling correctly would be to fall back to Assign's understanding of how to compile assignments with more complex left-hand-sides (including when to declare a var). This meant getting rid of setScopeVar(), which was sort of the culprit for these crazy var declarations, and just allowing Assign to take responsibility for the declarations

It's possible @zdenko introduced setScopeVar() partly because of #4657, which is about missing declarations when compiling functions that can't target ES6 destructuring (eg a non-last splat in a destructured array param). So to get both of these to correctly declare destructured vars (even in the presence of outer vars of the same name) was mostly a question of making sure @param was set on the various generated Assigns

I then ran into top-level generated object destructurings no longer wanting to wrap themselves in parens, so I introduced a distinction between @param = yes and @param = 'alwaysDeclare' which I think corresponds to "this is actually a param" vs "this needs to always get declared even if there's an outer var of the same name". There may be a cleaner way to accomplish this and it's possible that not all of the alwaysDeclares that I introduced are actually necessary to cover all of the cases

But it looks like it does cover the different cases, I added tests for some of the various more complex object spread "variables" both in destructured assignment and as params as well as a test for the example from #4657

[GeoffreyBooth]

When I recompiled the compiler source as a result of this PR, I noticed lots of changes across every file. It turns out, as a result of this PR, function parameter default values were now getting redeclared inside functions:

fn = (options = {}) ->
  options.foo = yes

var fn;

fn = function(options = {}) {
  var options;
  return options.foo = true;
};

This doesn’t seem to have any practical effect—all the tests still pass—but it looks wrong.

[GeoffreyBooth]

In general, though, this PR is an improvement. If there was one bit of general feedback from the object destructuring work, it was that those PRs didn’t use enough of “the machinery,” e.g. the classes and methods we already have written for determining things like setting scope and handling the left hand sides of assignments. My feeling was that a lot of the object destructuring code would ultimately be temporary, as hopefully object destructuring will reach Stage 4 soon and we can just compile to it rather than into Object.assign, so I wasn’t too concerned; but it doesn’t hurt to start that refactoring work sooner if we can.

[helixbass]

@GeoffreyBooth fixed param redeclaration - it also had to do with the @param = yes vs @param = 'alwaysDeclare' distinction. Not sure why those changes didn't show up in lib/ when I'd compiled - I usually run git checkout lib && bin/cake build:except-parser but seems like for whatever reason this time bin/cake build:full was required

Agreed that some of this stuff will hopefully be pretty temporary but this was a lot of cases where the compiler was emitting broken JS (even if the use cases for object-spread-destructuring into a non-simple variable are slightly obscure) so worth fixing

[GeoffreyBooth]

That seemed to have done the trick. Looks good to me, but @connec should also review it.

[GeoffreyBooth]

@jashkenas / @lydell / @connec / @helixbass I’m thinking of merging this in (once it gets approval from more people than just me) and releasing it as part of 2.0.0. So basically 2.0.0 would be beta5 plus this PR, assuming no more bugs get reported. So there’s some risk with this plan, in that 2.0.0 wouldn’t have had a group review like beta5 is getting; but I think that’s preferable to either a beta6 or holding this PR for 2.0.1. What do you think?

And #4668 can be the first PR merged into 2.0.1, as it’s not a bugfix.

[helixbass]

@GeoffreyBooth yikes I don't want to be the guy who broke 2.0.0. But if @connec thinks it looks ok and you don't want to release another beta then I guess that makes sense. I do think it should get merged in pre-2.0.0 - it fixes a lot of cases (a la #4673) where the compiler is currently outputting invalid JS (not sure quite how obscure the broken cases are, basically any complex "variable" being used in an object spread destructuring) and #4657 does come up in real code (as evidenced by the naturally surfacing bug report)

[connec]

The code looks fine to me, the removal of setScopeVar is definitely the right thing to do. Unfortunately I'm 'low-tech' (phone+kindle - not the best development setup) for the next week so I can't really play with it, though the tests look good.

One thing I can't quite grok from the description+diff is the need for alwaysDeclare - is it just to handle 'params' that we also definitely need a var for (as opposed to most params, which we expect to be 'declared' in the function signature)? I have a memory of something in Scope already that forces a declaration, is that no use?

[GeoffreyBooth]

@connec Assuming @helixbass has a good answer to your question, do you think this would be okay to merge in? Or would you rather we wait until you can get back and test it properly?

[helixbass]

@connec yup that's exactly what I was trying to accomplish with alwaysDeclare. If there's a way to do that more gracefully with Scope I'm not too familiar with it but happy to take a closer look

[connec]

I think this looks fine then. I can't remember (or check) enough to know if there's a scope method that can do this. I'd say we merge it and enhance later if we find something.

[GeoffreyBooth]

@lydell or @jashkenas any thoughts?