Migrate some duplicate binding tests to traverse

[danez]

Q	A
Fixed Issues?	Fixes #1, Fixes #2
Patch: Bug Fix?	y
Major: Breaking Change?	?
Minor: New Feature?	n
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?
License	MIT

The main goal here was to transfer all the tests which test duplicate bindings. They were in different transforms, although the code was in @babel/traverse and the transforms have no impact on that behavior, and imho also shouldn't.

While doing this I discovered, that on case wasn't throwing an error:

let foo;
function foo(){};

see inline comment.

[danez]

For some reason we have this exception in the code for the case mentioned in the PR. I couldn't come up with a reason why we do this and the commit that introduced it doesn't help either 8a4296a
Simply removing it also doesn't break any of the tests.
Do you think it is safe to remove? Anyone has an idea why this code was there and which case it covers?

To avoid confusion: kind === "hoisted" are function declarations. Should be renamed I think, but that would be definitely breaking.

[nicolo-ribaudo]

Maybe it was this? It is an Annex B thing:

let a;
{ function a() {} }

[danez]

right but even with removing these lines, this case still works. will add a test.

[babel-bot]

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/10156/

[babel-bot]

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/10154/

[nicolo-ribaudo]

Maybe it's worth using .toThrowErrorMatchingInlineSnapshot instead of toThrowErrorMatchingSnapshot, since the error messages are small?

[danez]

Oh I have never seen this. makes sense.

[danez]

Nevermind this does not work obviously with loops and generated testcases.

[nicolo-ribaudo]

We should also test class/function and class/var (and maybe function/var to be sure that it doesn't throw)

[danez]

Okay I added a more generic way to define tests.
Turns out we have some problems, unless I made a mistake in the tests.

var foo = 1;
class foo {};

and

class foo {};
var foo = 1;

Class and var never throw.

also does not throw:

function foo() {}
const foo = 1;

var foo = 1;
const foo = 1;

Thats why I disabled the reverse matching in the test, because swapping the two statements does throw.

[danez]

It seems the scope tracking in traverse has some flaws. I guess we should have a look separately from this. I can maybe work on this once done with the scope tracking in the parser.
I was also already thinking that maybe we can make a shared module that handles scopes correctly and share it between the parser and traverse. That would be nice, but not sure what requirements the scope tracking in traverse has.

[nicolo-ribaudo]

Yeah let's just update the tests for now.

I was also already thinking that maybe we can make a shared module that handles scopes correctly and share it between the parser and traverse

traverse's scope tracker needs to be kept updated while parser's scope describes a static AST; I don't know how feasible it would be.

[danez]

traverse's scope tracker needs to be kept updated while parser's scope describes a static AST; I don't know how feasible it would be.

But in the end both implementations (parser and traverse) work on nodes, have a stack of scopes, add/remove bindings and check for duplicates. But yes there might be more stuff going on in traverse which I'm not yet aware of.