Flow: interface identifier should be declared in the scope

[JLHwung]

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

A follow up to #9493: Declare interface identifier in the scope so that it would not be an undefined export. Thanks @jj-choi for suggestions to the solution.

As I didn't find any specification for Flow, I have developed several test cases on Flow REPL. We revise the flowParseInterfaceish according to the following observations:

Declarations	Identifier Binding Type	Facts
Interface[Declare]	LEXICAL	interface I {}; interface I {}; will throw
Type[Declare]	LEXICAL	type A = string; type A = string; will throw
OpaqueType	LEXICAL	opaque type A = string; opaque type A = string' will throw
Class[+Declare]	VAR	declare class A {}; declare class A{}; does not throw but declare class A {}; class A{}; will throw
Variable[+Declare]	???	declare var A; declare var A; does not throw declare var A; declare var A; var A does not throw
Function[+Declare]	???	declare function A(): void; declare function A(): void; function A() {} does not throw

Since Class_[+Declare] implies a VAR type binding identifier, I have changed the scope type of DeclareModule to SCOPE_VAR, otherwise the following snippet will throw:

declare module M {
  declare class C{}
}
declare class C{}

Known issues

We haven't applied declareName for Variable_[+Declare] and Function_[+Declare] since they can share the identifier name with VariableDeclaration and FunctionDeclaration, which means the following snippet would fail

declare var A;
declare function B(): void;
export { A, B };

We may solve this issue by introducing flowVars to Scope and record them in an array other than scope.vars. However, I suggest we submit another PR to solve it before this one gets too big to review.

[babel-bot]

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

[babel-bot]

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

[existentialism]

/cc: @babel/flow

[tanhauhau]

I've a similar PR #10100, but I've no time to work on it now. Just FYI.

[JLHwung]

@tanhauhau Thank you. I should have found from the original issue that you are working on it. As our approaches are pretty similar, Nicolò will try to merge my test case to your implementation.

[JLHwung]

Update: I have merged #10100 add disabled some test related to Variable_[+Declare] and Function_[+Declare]

@tanhauhau This PR is almost same with #10100 except a61fa14#diff-c3d4a6608f6a39a6a14b0c4f474c922cL197

[fisker]

/cc @JLHwung status?

need this for prettier @babel/parser upgrade

[existentialism]

@fisker we've been waiting for someone on the Flow team to chime in

[fisker]

@existentialism that's OK, just checking.

FYI, this failure test is from flow project, and it has been broken since @bable/parser@7.4.0

[RReverser]

@existentialism Do you know who on the Flow team should be pinged to move this forward? This is currently the only issue/PR that is blocking Babel upgrade in Prettier.

[alexander-akait]

Yep, it is blocker for us (prettier)

/cc @nmote maybe you can help?

[nmote]

At its face this all sounds reasonable. I'm not an expert on the parser; if you need an in-depth review I'd try to get in touch with @gabelevi or @mroch. What sort of feedback are you looking for from the Flow team?

[nicolo-ribaudo]

Mostly if it is being declared in the scope correctly, or if it should follow some other rules (for example regarding to shadowing/redeclaration)

[nmote]

Unfortunately I'm not really familiar with that area of the parser. My process would basically be to look at how it behaves in practice, which @JLHwung has already done.

[nicolo-ribaudo]

Well, let's merge this!

[alexander-akait]

@nicolo-ribaudo any ETA for patch release?