SCC & topological sorting fails for types contained in declare module

[Lupus]

I'm trying to generate bindings for stream.d.ts from @types/node and do not see sufficient amount of recursive modules in output both in default and in all modes for --rec-modules. Only top level modules seem to be recursed, but not the nested ones. Strongly connected components and topological sorting should be applied to each level in module hierarchy to govern the resulting order and bundling of modules, or I'm missing something?

Command that I'm using:

$ ./node_modules/@ocsigen/ts2ocaml/dist/ts2ocaml.js jsoo \
     -o node \
     --preset=full \
     --readable-names \
     --follow-relative-references \
     --rec-modules all \
     node_modules/@types/node/stream.d.ts

$ grep ' rec ' node/node__stream.mli
module rec Node_stream : sig

ts2ocaml 1.4.2

[cannorin]

It seems stream.d.ts just happens not to contain any recursive definitions at all.

You can see the SCC & topological sorting working: https://gist.github.com/cannorin/456a9e235dda3665da974c94c3fc7f07

I didn't see all of the stream.d.ts. Can you create a minimal failing example it you think there is an issue?
I believe it's very unlikely since the CI is testing typescript.d.ts which contains a lot of recursive definitions, though.

[cannorin]

Also, the correct option to make all the modules recursive is --rec-module=naive, not --rec-modules=all.

It seems it doesn't error if you specify an unknown option, which is an issue.

[Lupus]

May be I'm missing something, but I clearly see recursive stuff in the output, that is not organized as recusrive modules. Let's take the following command for a try:

$ ./node_modules/@ocsigen/ts2ocaml/dist/ts2ocaml.js jsoo -o output --preset=full --readable-names --follow-relative-references --rec-modules optimized node_modules/@types/node/stream.d.ts

--rec-module optimized is a valid combination of command line flags according to --help. This is the trimmed down piece of output:

module rec Node_stream : sig
end

and[@js.scope "stream"] Stream : sig
  module[@js.scope "internal"] Internal : sig
    module WritableOptions : sig
      val cast_to_StreamOptions: t -> Writable.t_0 StreamOptions.t_1 [@@js.cast]
    end
    
    (** @since v0.9.4 *)
    module[@js.scope "Writable"] Writable : sig
      val create: ?opts:WritableOptions.t_0 -> unit -> 'tags this [@@js.create]
      val addListener'''': 'tags this -> event:([`L_s7_pipe[@js "pipe"]] [@js.enum]) -> listener:(Readable.t_0 -> unit) -> 'tags this [@@js.call "addListener"]
      val on'''': 'tags this -> event:([`L_s7_pipe[@js "pipe"]] [@js.enum]) -> listener:(Readable.t_0 -> unit) -> 'tags this [@@js.call "on"]
      val cast_to_Stream: t -> Stream.t_0 [@@js.cast]
    end
    module StreamOptions : sig
    end
    module[@js.scope "Stream"] Stream : sig
      val create: ?opts:ReadableOptions.t_0 -> unit -> 'tags this [@@js.create]
    end
    module ReadableOptions : sig
      val read: 'tags this -> this:Readable.t_0 -> size:float -> unit [@@js.call "read"]
      val cast_to_StreamOptions: t -> Readable.t_0 StreamOptions.t_1 [@@js.cast]
    end
    
    (** @since v0.9.4 *)
    module[@js.scope "Readable"] Readable : sig
      val create: ?opts:ReadableOptions.t_0 -> unit -> 'tags this [@@js.create]
      val cast_to_Stream: t -> Stream.t_0 [@@js.cast]
    end
    module DuplexOptions : sig
      val construct: 'tags this -> this:Duplex.t_0 -> callback:(?error:Error.t_0 option -> unit -> unit) -> unit [@@js.call "construct"]
      val cast_to_ReadableOptions: t -> ReadableOptions.t_0 [@@js.cast]
      val cast_to_WritableOptions: t -> WritableOptions.t_0 [@@js.cast]
    end
    
    module[@js.scope "Duplex"] Duplex : sig
      val create: ?opts:DuplexOptions.t_0 -> unit -> 'tags this [@@js.create]
      val cast_to_Readable: t -> Readable.t_0 [@@js.cast]
      val cast_to_Writable: t -> Writable.t_0 [@@js.cast]
    end
    type t = [`Stream_internal] intf [@@js.custom { of_js=Obj.magic; to_js=Obj.magic }]
  end
end

Dependency graph between these modules definitely includes some cycles:

[cannorin]

It seems that ts2ocaml is failing to relate the types correctly because TypeScript API reports the fullname of something without the enclosing module (e.g. internal.Stream, but not stream.internal.Stream).

It may take some time to fix this. I'm heavily refactoring the typer in #32, so the fix would be after that PR merged.

In the meantime, you can workaround this by either

• forcing all modules to be recursive by --rec-module=naive, or
• not using recursive modules by --rec-module=off.

There is no s after the --rec-module. If you find it confusing that some options have s and some don't, please create a separate issue.

[Lupus]

Argh, ignoring invalid options is the most frustrating thing that can happen to the user lol :D

In my attempts to solve the automated typescript bindings generator I took the path of sorting and bundling recursive modules after all the stuff was already assigned OCaml module names, that worked well - generated code was always sound in terms of recursive modules and ordering of modules. Might not be applicable to ts2ocaml, just sharing my experience :)

I've already recursed the modules by hand while cleaning the definitions emitted by ts2ocaml, not a big deal and still much better than doing everything by hand! Keep up the great work! 👍

[cannorin]

Argh, ignoring invalid options is the most frustrating thing that can happen to the user lol :D

I'm releasing 1.4.3 to properly ignore invalid options 😅 , it should be live within few minutes.

just sharing my experience :)

ts2ocaml used to do this, but at some point I stopped resolving full names on my own and started using TypeScript API (which is mostly undocumented) instead. It seems I ended up shooting myself in the foot...

Keep up the great work! 👍

Thanks❤️