Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-add support for local Flow bindings (TypeAlias, OpaqueTypeAlias and Interface) #7900

Merged
merged 1 commit into from May 17, 2018

Conversation

@rubennorte
Copy link
Contributor

rubennorte commented May 10, 2018

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

Flow binding support was completely removed in #6528. While the reason behind that change was totally valid:

The reason behind this change is that declare var foo doesn't introduce a new local binding, but it represents a global one.

I think it should only be applied to global Flow declarations. Local Flow declarations (like TypeAlias, OpaqueTypeAlias and Interface) create local type bindings that exist in the same namespace as value bindings. E.g.:

const T = 'some constant';

function foo() {
  type T = string;
  const str: T = 'fooStr';
  const baz: str = T;
}

When using Flow, T referenced by str is bound to the type defined in foo instead of the const defined outside. Same for the value used by baz, which causes a Flow error because it references a type in that scope.

This PR adds support for those local Flow bindings.

@rubennorte rubennorte force-pushed the rubennorte:add-local-flow-bindings branch from 3016216 to d8a8d15 May 10, 2018
@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented May 10, 2018

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

@hzoo hzoo requested review from gabelevi and samwgoldman May 10, 2018
Copy link
Contributor

gabelevi left a comment

This makes sense to me

@gabelevi

This comment has been minimized.

Copy link
Contributor

gabelevi commented May 10, 2018

The TL;DR for bindings & Flow is

  • Values and types share a namespace. For example, you can't have class Foo {} and type Foo = ....
  • value bindings can be used as identifiers in expressions, like var x = y. They can also be used in typeof types. Like var x: typeof y
  • type bindings can be used as type identifiers in types. Like var x: MyType
  • Things that introduce a value binding also introduce a type binding. For example, class MyClass introduces a MyClass value binding and a MyClass type binding
  • Things like type and interface only introduce a type binding
@loganfsmyth

This comment has been minimized.

Copy link
Member

loganfsmyth commented May 10, 2018

Could one of you verify how exactly flow differentiates "global" and "module" in cases like Babel? I was under the impression that something like declare class Foo {} or declare var foo: number;, when run in the contexts of a module, would be scoped to the module, is an incorrect assumption?

@samwgoldman

This comment has been minimized.

Copy link
Contributor

samwgoldman commented May 10, 2018

when run in the contexts of a module, would be scoped to the module

This is correct.

It's unusual to use declare ... in a module, though. We support it, and when used in a module the scoping is exactly the same as if you had not used declare.

The intention is that declare ... is used in library definitions. The "module-level" scope of every libdef file is merged into a single scope, which is the global scope. That might be the source of confusion here?

@loganfsmyth

This comment has been minimized.

Copy link
Member

loganfsmyth commented May 10, 2018

Does Flow ever treat .js files as non-ESM/CommonJS, or is it safe to assume that it'd only every be files loaded by [libs] where the top-level declarations would be global?

In my mind, part of the reason we just removed this is that it wasn't clear that it worked right anyway. None of our transforms really expect Flowtypes to exist, so they usually end up corrupting the type definitions. That or we end up with issues like #5395 where people expect top-level flow declarations to be global. I think it's all made more complicated because there's no clear specification anywhere defining what Flow expects for this kind of thing.

If I declare var global in a module, what binding should that be resolving? The global one, or some non-existant module-level declaration? Should you only be allowed to declare something in a module if there actually is a JS declaration for the same binding?

@samwgoldman

This comment has been minimized.

Copy link
Contributor

samwgoldman commented May 10, 2018

or is it safe to assume that it'd only every be files loaded by [libs] where the top-level declarations would be global

Yes, this is correct. Only files loaded by [libs] have this behavior where top-level declarations are global. All such files are analyzed first to establish the global environment.

That or we end up with issues like #5395 where people expect top-level flow declarations to be global

I'm not sure I understand that issue entirely, but it's not correct to assume that a top-level declare var (or declare <whatever>) introduces a global.

there's no clear specification anywhere defining what Flow expects for this kind of thing

Sorry the specification is unclear. Hopefully this is clear: declare <whatever> is the same as <whatever>.

If I declare var global in a module, what binding should that be resolving?

It's declaring a new binding, which shadows the global binding in the global scope, not resolving anything. It's just as if you had written var global = e.

@loganfsmyth

This comment has been minimized.

Copy link
Member

loganfsmyth commented May 10, 2018

I'm not sure I understand that issue entirely, but it's not correct to assume that a top-level declare var (or declare ) introduces a global.

Alright, that's good to know, so the user's usecase is not one that Flow actually supports/expects. In that case, the user was using a module-level declare to define the type of a global variable. Since the declare is stripped out, the variable actually accesses a global variable at runtime.

Would it make sense for Flow to error in cases like this? If you assign to something declared with declare, but not actually declared in JS itself? Otherwise you're allowing people to use declare as if it were defining the type of a global variable, whether that is the intention or not.

@samwgoldman

This comment has been minimized.

Copy link
Contributor

samwgoldman commented May 10, 2018

Would it make sense for Flow to error in cases like this?

IMO we should just forbid declare ... in implementation files altogether to avoid this confusion.

@loganfsmyth

This comment has been minimized.

Copy link
Member

loganfsmyth commented May 10, 2018

Fair enough!

@Jessidhia

This comment has been minimized.

Copy link
Member

Jessidhia commented May 10, 2018

I write this as a TypeScript user, but there are times I do want to use declare var do declare a global but have the declaration only be visible within this one module.

I usually do this with modules that import global data into local state; everything else should refer to the local state.

@rubennorte

This comment has been minimized.

Copy link
Contributor Author

rubennorte commented May 13, 2018

@hzoo can we please merge this?

@loganfsmyth

This comment has been minimized.

Copy link
Member

loganfsmyth commented May 13, 2018

Given the conversation above, do we actually want the "Global" distinction included here? It's not super clear to me what distinction that is trying to draw.

@samwgoldman

This comment has been minimized.

Copy link
Contributor

samwgoldman commented May 14, 2018

I agree that the Global distinction is unclear, and the original FlowDeclaration name better describes how Flow understand the pre-transform code. Otherwise, this is fine with me.

@rubennorte

This comment has been minimized.

Copy link
Contributor Author

rubennorte commented May 17, 2018

@loganfsmyth I made the distinction because that's why the Flow bindings were removed in the first place. I can update the PR to remove the distinction so it matches what Flow actually interprets in code files (vs library files), if you're OK with that.

Edit: updated to remove the distinction.

@rubennorte rubennorte force-pushed the rubennorte:add-local-flow-bindings branch 3 times, most recently from 4853c4a to cfd75b3 May 17, 2018
@rubennorte rubennorte force-pushed the rubennorte:add-local-flow-bindings branch from cfd75b3 to 82dd23e May 17, 2018
@loganfsmyth loganfsmyth merged commit bc6f0f9 into babel:master May 17, 2018
4 checks passed
4 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 80.8% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lock lock bot added the outdated label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants
You can’t perform that action at this time.