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

[WIP] Generate flow annotations #780

Closed
wants to merge 10 commits into from
Closed

Conversation

pvolok
Copy link

@pvolok pvolok commented Sep 17, 2016

We have two options how to annotate: inline annotations vs .js.flow files. IMO the latter is the way to go. Its easier to implement and also more performant on the flow side.

Right now I'm working with a Types.signature in Lam_compile_group.lambda_as_module. Signature seems to have most of information needed except for function argument names.

Things to be implemented:

  • "Opaque types" like records

Related: #671

@@ -0,0 +1,117 @@
open Types

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a minor style issue, in general, open is not encouraged, especially for modules like Types which is an internal module

@pvolok
Copy link
Author

pvolok commented Oct 8, 2016

I finally got more time, and want to finish this.

But I can't figure out one thing. I'm not sure how this thing is called, maybe type variable scope. Check the following example. Is there a way to know where type variable should be added?

function fn1<T>(id: (x: T) => T) {
  id(1);
  id('2'); // ERROR: string is incompatible with number
}

function fn2(id: <T>(x: T) => T) {
  id(1);
  id('2');
}

In the Types.signature tree we can encounter Types.Tvar types and need to know where to attach them.

@bobzhang
Copy link
Member

bobzhang commented Oct 9, 2016

hi @pvolok does flow support higher rank polymorhism? here you might hit type system mismatch: some types are not expressible in OCaml, some are not expressible in Flow?
Ocaml only support it via record/objects

@pvolok
Copy link
Author

pvolok commented Oct 9, 2016

If I understand correctly what higher rank polymorphism is, then yes, flow supports it. I read more about it and it looks like the following is not possible in ocaml:

let fn (same : 'a . 'a -> 'a -> unit) =
  same 1 2;     (* OK *)
  same 'a' 'b'; (* OK *)
  same 1 '2';   (* ERROR: int and char are incompatible *)

So I attach poly types to the most outer function. Like this:
OCaml

val fn: 'a -> ('b -> 'b) -> unit

Flow

declare var fn: <A, B>(p1: A, p2: (p: B) => B) => void;

@bobzhang
Copy link
Member

you are right, let fn (same : 'a . 'a -> 'a -> unit) = is not possible
in ocaml, the best you can do is wrap same in a record then you have higher rank polymorphism, with 4.04, there will be no performance penalty thanks to unboxed
reply@reply.github.com At: 10/09/16 15:05:26" data-digest="From: reply@reply.github.com At: 10/09/16 15:05:26" style="">
From: reply@reply.github.com At: 10/09/16 15:05:26
To: bucklescript@noreply.github.com
Cc: HONGBO ZHANG (BLOOMBERG/ 731 LEX), comment@noreply.github.com
Subject: Re: [bloomberg/bucklescript] [WIP] Generate flow annotations (#780)

If I understand correctly what higher rank polymorphism is, then yes, flow supports it. I read more about it and it looks like the following is not possible in ocaml:
let fn (same : 'a . 'a -> 'a -> unit) =
same 1 2; (* OK )
same 'a' 'b'; (
OK )
same 1 '2'; (
ERROR: int and char are incompatible *)
So I attach poly types to the most outer function. Like this:
OCaml
val fn: 'a -> ('b -> 'b) -> unit
Flow
declare var fn: <A, B>(p1: A, p2: (p: B) => B) => void;

You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

@pvolok
Copy link
Author

pvolok commented Oct 11, 2016

@bobzhang I think I finished with the types that are representable in flow. Can you review when you have time? If you need any explanation ping me in gitter/discord.

There are some types that are not supposed to be worked with in js (like records, lists, etc). They are anys for now. I'd prefer to finish this PR without those types.

@bobzhang
Copy link
Member

@pvolok it seems that .cmi -> .js.flow does not need any info from buckle side, is it okay to make it as a independent tool?

@pvolok
Copy link
Author

pvolok commented Oct 12, 2016

I think it's possible to make it as a separate tool. But some things are (in my opinion) in favor of the build-in flow support:

  • The way types are represented in flow depends on how bucklescript compiles to js. If bs changes how certain types are represented in js, the flow types generation should be updated accordingly. So this tool doesn't make sense without bs.
  • In order to generate .flow.js files, you need to discover a separate tool, add the same output settings as to bsc, and run it every time bsc is being run. So the integration will not be as seamless.

On the other hand, it will add a burden of maintaining additional code. What do you think?

@bobzhang
Copy link
Member

The way types are represented in flow depends on how bucklescript compiles to js.
Can you be a bit more specific? so far we did not touch the type checker yet

About the integration, I think it depends on how well the build tool(rebel or whatever tool) works out, my concern is that if there is a hard dependency on buckle(like it needs some information from the buckle compiler) then we should include it, if there is no real dependency then maybe it is better to develop it as a separate tool (and you can also move as fast as you want)

@pvolok
Copy link
Author

pvolok commented Oct 13, 2016

@bobzhang I was talking about how ocaml types are compiled to js types. While generating flow declarations we use the knowledge of how bs converts types from ocaml to js. For example:

int -> number
string array -> Array<string>
bool -> 0 or 1

string list -> shouldn't be directly worked with in js?
record -> shouldn't be directly worked with in js

So this flow types generator only makes sense with bs. Sorry if I'm still being cryptic.

I will continue to work on this as a separate tool. If you change your mind, we can always merge it.

@pvolok pvolok closed this Oct 13, 2016
@bobzhang
Copy link
Member

@pvolok I understand your comments now. we can revisit this issue later

@pvolok
Copy link
Author

pvolok commented Oct 18, 2016

Hey @bobzhang

I've been working on making flow types generator as a separate tool and encountered a couple of problems:

  • I'm using the env (Env.t) to resolve external types. But once the signature is saved into a .cmi file the env is not available anymore. So what I have to do is to recreate the env by resolving module deps (including stdlib modules).
  • In order to load a cmi file I use Cmi_format.read_cmi. So both bucklescript and this tool have to be compiled using the same version of ocaml. Otherwise the format of the cmi file may have changed. I'm not sure if this issue is relevant since all packages are compiled for the active ocaml version if I understand correctly.

Maybe you have ideas of how to solve these problems (especially first) or want to reevaluate your decision of keeping flow generator out of the bs src?

@bobzhang
Copy link
Member

@pvolok Cmi_format.read_cmi is fine, our compiler is ABI compatible with OCaml 4.02.3 with regard to cmi.
About one, can you be more specific, in the compiler there are APIs to read persistent signatures, actually we have plans to make BuckleScript not rely on Env.t to make our changes less intrusive.
Another approach is to read cmt file, but typedtree is quite a complex data structure, in the compiler you can get typedtree and untype it to parsetree (with type annotations), and consume parsetree is much easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants