Skip to content

Flow: interface identifier should be declared in the scope#10220

Merged
nicolo-ribaudo merged 10 commits intobabel:masterfrom
JLHwung:fix-10044
Oct 2, 2019
Merged

Flow: interface identifier should be declared in the scope#10220
nicolo-ribaudo merged 10 commits intobabel:masterfrom
JLHwung:fix-10044

Conversation

@JLHwung
Copy link
Copy Markdown
Contributor

@JLHwung JLHwung commented Jul 15, 2019 •
edited
Loading

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
Copy link
Copy Markdown
Collaborator

babel-bot commented Jul 16, 2019 •
edited
Loading

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

@babel-bot
Copy link
Copy Markdown
Collaborator

babel-bot commented Jul 16, 2019

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

@existentialism existentialism added area: flow PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Jul 16, 2019
@existentialism
Copy link
Copy Markdown
Member

existentialism commented Jul 16, 2019

/cc: @babel/flow

@tanhauhau
Copy link
Copy Markdown
Member

tanhauhau commented Jul 18, 2019

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

@JLHwung
Copy link
Copy Markdown
Contributor Author

JLHwung commented Jul 18, 2019

@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 and others added 10 commits July 23, 2019 22:31
@JLHwung
fix: typo
ded89e6
@tanhauhau @JLHwung
declare name for flow interface
e2888a4
@tanhauhau @JLHwung
add test case for export overload function, typescript
264446d
@JLHwung
test: add test
67ed024 
Fixes babel#10044
@JLHwung
test: update test
96ecd03
@JLHwung
test(flow): add multiple declarations regression test
274cded
@tanhauhau @JLHwung
re-enable flow test case
81f0c8b 
# Conflicts:
#	packages/babel-plugin-transform-flow-strip-types/test/fixtures/strip-types/def-site-variance/input.js
#	packages/babel-plugin-transform-flow-strip-types/test/fixtures/strip-types/strip-declare-statements/input.js
#	packages/babel-plugin-transform-flow-strip-types/test/fixtures/strip-types/strip-interfaces-module-and-script/input.js
#	packages/babel-plugin-transform-flow-strip-types/test/fixtures/strip-types/strip-iterator/input.js
@JLHwung
test: disable two flow dupl-decl test
ccc3975
@JLHwung
fix: do not declare name for declare function until we figure out a b…
a61fa14 
…etter way
@JLHwung
test: duplicate declare function and function would not throw
1bf302b
@JLHwung
Copy link
Copy Markdown
Contributor Author

JLHwung commented Jul 24, 2019

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 fisker mentioned this pull request Aug 12, 2019
Merged
@fisker
Copy link
Copy Markdown
Contributor

fisker commented Aug 13, 2019

/cc @JLHwung status?

need this for prettier @babel/parser upgrade

@existentialism
Copy link
Copy Markdown
Member

existentialism commented Aug 13, 2019

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

@fisker
Copy link
Copy Markdown
Contributor

fisker commented Aug 13, 2019 •
edited
Loading

@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

@JLHwung JLHwung mentioned this pull request Aug 14, 2019
Closed
@fisker fisker mentioned this pull request Sep 24, 2019
Merged
4 tasks
@RReverser
Copy link
Copy Markdown
Member

RReverser commented Sep 27, 2019

@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
Copy link
Copy Markdown
Contributor

alexander-akait commented Sep 30, 2019

Yep, it is blocker for us (prettier)

/cc @nmote maybe you can help?

@nmote
Copy link
Copy Markdown

nmote commented Sep 30, 2019

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
Copy link
Copy Markdown
Member

nicolo-ribaudo commented Sep 30, 2019

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
Copy link
Copy Markdown

nmote commented Sep 30, 2019

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
Copy link
Copy Markdown
Member

nicolo-ribaudo commented Oct 2, 2019

Well, let's merge this!

@nicolo-ribaudo nicolo-ribaudo merged commit fa5057f into babel:master Oct 2, 2019
@alexander-akait
Copy link
Copy Markdown
Contributor

alexander-akait commented Oct 3, 2019

@nicolo-ribaudo any ETA for patch release?

@JLHwung JLHwung deleted the fix-10044 branch October 3, 2019 13:18
This was referenced Oct 22, 2019
Merged
Open
@snyk-bot snyk-bot mentioned this pull request Nov 1, 2019
Open
This was referenced Nov 12, 2019
Open
Merged
This was referenced Nov 28, 2019
Open
Open
This was referenced Dec 5, 2019
Open
Merged
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jan 2, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@existentialism existentialism existentialism approved these changes

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo approved these changes

Assignees

No one assigned

Labels

area: flow outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Export is not defined

9 participants

@JLHwung @babel-bot @existentialism @tanhauhau @fisker @RReverser @alexander-akait @nmote @nicolo-ribaudo