ahrefs / atd

Static types for JSON APIs
Other
315 stars 52 forks source link

bucklescript: <ocaml repr="array"> unusable #156

Open ygrek opened 6 years ago

ygrek commented 6 years ago

atdgen -t generates reference to Atdgen_runtime module, but for bucklescript only Atdgen_codec_runtime is provided.

type t = int Atdgen_runtime.Util.ocaml_array
rgrinberg commented 5 years ago

Yeah, this is quite annoying and the problem isn't specific to just arrays. Validation is also slightly broken as a result of this. How about the following workaround for now:

I'll rename Atdgen_codec_runtime to Atdgen_runtime and try to port more parts of Atdgen_runtime to it.

A long term solution to this problem would be to use something like dune's variants. They would make it straight forward to vary the runtime for jsoo/native. That wouldn't solve the bucklescript issue unfortunately, but I think that's the right way to go to make the runtime more portable.

ygrek commented 5 years ago

Same module name for different backends looks like a workable solution.

rgrinberg commented 5 years ago

I gave this a try and it's a bit of a shame but it won't work right now because we can't have 2 libraries with the same in scope. I suppose that we should just wait for variants in dune to address this.

Until variants come, I can make the code generator spit out the module alias:

module Atdgen_runtime = Atdgen_codec_runtime
ELLIOTTCABLE commented 5 years ago

In dealing with a somewhat-similar problem, I ended up taking advantage of ignored_subdirs to keep some BuckleScript-specific stuff out of Dune's purview to avoid naming conflicts. Hope that helps!

ygrek commented 5 years ago

I am not sure I understand why there are two libraries with same module in scope.. Let me formulate the root of the problem to make sure we are on same page. Atdgen_runtime provides runtime for OCaml and comes packaged with atdgen. And bucklescript support can be provided by different libs, so it is not going to be hardcoded in atdgen itself, hence different name for runtime module - atdgen_codec_runtime. But _t files are generated equal for ml and bs currently.

So natural idea - maybe generate _bs_t modules specific for bucklescript?

Or from another angle - add separate option to atdgen to allow specify "module with runtime support" and default it to "Atdgen_runtime". And then let projects that share _t files with different backends to provide a module that will take care of it in both languages? (This will be probably less "out-of-box"-y then separate _t)

rgrinberg commented 5 years ago

Or from another angle - add separate option to atdgen to allow specify "module with runtime support" and default it to "Atdgen_runtime". And then let projects that share _t files with different backends to provide a module that will take care of it in both languages? (This will be probably less "out-of-box"-y then separate _t)

This is basically my idea. When I say runtime, I mean a library that is independent of the code generator but the code generator can rely on being present and freely refer to its modules.

Where we might have some confusion is the definition of scope. A scope in this case is a collection of libraries that dune says must not have overlapping names (in atd, we only have 1 scope). Here we'd like to have two runtime libraries with the same module name - because these two runtimes are mutually exclusive anyway. But dune doesn't let us do it because it couples library names to modules names. So

Same module name for different backends looks like a workable solution.

doesn't work because dune doesn't allow us to define (wrapped true) libraries with the same module names but different library names.

I think I've found another workaround however. I can simply use (wrapped false). This means that I will have to provide in 1 big module rather in pieces. But it's a good enough workaround for now.

struktured commented 5 years ago

I'll rename Atdgen_codec_runtime to Atdgen_runtime and try to port more parts of Atdgen_runtime to it.

@rgrinberg Do you mean you will rename Atdgen_codec_runtime to Atdgen_runtime in repo https://github.com/ahrefs/bs-atdgen-codec-runtime? If so, any reason this change hasn't taken place yet besides prioritization?

Edit: It seems modules would have to be renamed or moved in both atd and bs-atdgen-codec-runtime.

struktured commented 5 years ago

I worked around this inefficiently by representing the arrays as lists and using the atd wrap feature. For instance, here's int array:

(* int_array.atd *)
type t = int list wrap <ocaml module="Int_array_wrap">
(* int_array_wrap.ml *)
type t = int array

let unwrap x : int list = Array.to_list x

let wrap x : t = Array.of_list x

Side note: I don't think the wrap feature is working properly with parameterized types (I tried a generic version of the above and got a strange compiler error).

rgrinberg commented 5 years ago

Side note: I don't think the wrap feature is working properly with parameterized types (I tried a generic version of the above and got a strange compiler error).

Would you mind putting up a test case for this?

Do you mean you will rename Atdgen_codec_runtime to Atdgen_runtime in repo ahrefs/bs-atdgen-codec-runtime? If so, any reason this change hasn't taken place yet besides prioritization?

Because of module naming conflicts.

struktured commented 5 years ago

Side note: I don't think the wrap feature is working properly with parameterized types (I tried a generic version of the above and got a strange compiler error).

Would you mind putting up a test case for this?

See https://github.com/mjambon/atd/issues/180.