(root_module ..) field for libraries & executables

[rgrinberg]

There's an inconvenient situation that can occur when one is trying to define a
module that clashes with a dependency's top level name. The end result is that
the dependency becomes shadowed and unusable. For example:

(library
 (name foo)
 (modules re)
 (libraries re))

Now the library re is inaccessible

The current workaround is to rename the module used internally, and then modify the library
interface file to get back the desired public name:

(library
 (name foo)
 (modules re0 foo)
 (libraries re))

$ cat foo.ml
module Re = Re0

However, this is still problematic because:

• One has to use the crappy Re0 name everywhere
• Generated documentation will still refer to Re0.

I propose the following fix instead:

(library
 (name foo)
 (extra_aliases (re re_unshadow))
 (libraries re))

Now, we can us the re library via the re_unshadow name, and we don't need to
modify our own names.

This feature is pretty simple, so I prototyped it immediately. However, it's not
completely robust yet as it doesn't work for executables. If people like it, I
can polish this PR.

[aalekseyev]

I like the feature in principle. I thought the name extra_aliases is a bit opaque, thought. I wonder if something like this would be easier to understand:

(library
  (name foo)
  (libraries (re as re_unshadow))
)

This way the dependencies in scope are still controlled solely by the libraries field, so I feel the potential for confusion is smaller.

[bobot]

I like the as, but libraries could define more than one toplevel library, with names different from the name of the library.

For the overall need of the PR, I'm not against just sad, If ocaml would have a root directory we would do ROOT.Re. I think pxx and other code generator have the same need. I don't remember what is the current state in the OCaml typer. But perhaps Dune can propose something similar, we would define a module alias ROOT with all the toplevel symbols we can find. It seems hackish...

[aalekseyev]

I think we at jane street think of that as an anti-pattern, so I wouldn't be too sad if the syntax in that case would be verbose.
Something like

(libraries
  (foo (renaming (
    (foo_a foo_a_unshadow)
    (foo_b foo_b_unshadow)
   )))
)

I don't know if dune actually has enough information to check that a top-level module foo_a is indeed exposed by the library foo.
If it does, then I'm not even joking, the above seems fine to me. If not, then this seems bad because it will let users write some nonsensical stuff without any ability to check.

[rgrinberg]

I like the as, but libraries could define more than one toplevel library, with names different from the name of the library.

Yes, this is why a separate field is a little more flexible. However, I don't think it's very important to extend full support for unwrapped libraries. I don't know of many unwrapped libraries left in the wild and it's a poor practice to create new ones. So I think as is fine.

For the overall need of the PR, I'm not against just sad, If ocaml would have a root directory we would do ROOT.Re. I think pxx and other code generator have the same need. I don't remember what is the current state in the OCaml typer. But perhaps Dune can propose something similar, we would define a module alias ROOT with all the toplevel symbols we can find. It seems hackish

I think that's an excellent idea for a proposal. However, I wonder if we can use ROOT or any other valid name since it would break existing code. In any case, I'd be very interested to see such a proposal.

I don't know if dune actually has enough information to check that a top-level module foo_a is indeed exposed by the library foo.
If it does, then I'm not even joking, the above seems fine to me. If not, then this seems bad because it will let users write some nonsensical stuff without any ability to check.

It has enough information if foo is written in dune. Tbh, we could probably extend this checking to non dune libs with some ocamlobjinfo hacks, but it doesn't seem worth it. I don't think it's all that important to verify the names the user writes anyway, they will get a compilation if they mess it up regardless.

By the way, one practical issue with constructors is that it's a bit confusing to handle select with them. For example, how would you interpret:

(select bar.ml from
 (foo (renaming (
    (foo_a foo_a_unshadow)
    (foo_b foo_b_unshadow))) -> bar.a.ml)
 (!foo -> bar.b.ml))

I don't think we are going to handle conditional shadowing. It's a bit painful to implement. There's also going to be some awkward syntax with:

(re_export (foo (renaming (..)))

Which is regrettable.

[bobot]

Tbh, we could probably extend this checking to non dune libs with some ocamlobjinfo hacks, but it doesn't seem worth it

I agree but just wondering if just listing the .cmi is sufficent since it is just about typing not linking.

[rgrinberg]

I've discussed this feature with Jeremie and here are some quick suggestions:

• Using as reads well, but isn't uniform with the syntax. We need the first token to be a constructor name for consistency. So let's pick something like (rename re -> re_unshadow). Where rename is the constructor name, and I've replaced as with an arrow as we already use -> in select and (rename .. as ..) sounds unnatural.

• Supporting unwrapped modules is better done as follows: Consider an unwrapped library foo with modules bar and baz. Using this library with (rename foo -> foo_blah) will add the following aliases:

module Foo_blah = struct
  module Bar = Bar
  module Baz = Baz
end

We'll need to generate the list of modules for foo, but that's quite easy. In essence, this allows us to use unwrapped libraries as if they are wrapped. That seems quite good to me.

• There's no need to support rename and export simultaneously.

@bobot, @aalekseyev are you ok with these tweaks?

[aalekseyev]

Yeah, that sounds good.
Additionally, I think we should shadow the original names to make them unusable when this construction is used.

[rgrinberg]

I pushed a commit that implements the new proposal. For now I've left off the implementation for unwrapped libraries. It's a little tough to add, and I have no use it for it myself at the moment.

[emillon]

I think it's a neat feature and the limitation to wrapped libraries seems completely fair.

[rgrinberg]

Thanks. I'll give @bobot and @aalekseyev a chance to see if they are happy with the limitation and my implementation of this feature before merging.

[aalekseyev]

I added some new tests and CR comments to rename-deps.t.
Please have a look.

In particular I think we should be clear from the very beginning what the shadowing behavior of renaming is (what if you rename a library and thereby shadow another library? should renaming that library still work?).

[aalekseyev]

Why have this as a list if it's required to be a singleton list?

[rgrinberg]

When I wrote it, I expected to support unwrapped libraries. For such libraries, there are multiple toplevel modules. Since we don't support them yet, it doesn't need to be a list. Should we simplify the code for the use case that we support? Or keep the more general data type in anticipation?

[aalekseyev]

I don't anticipate that anybody will need it any time soon, and simplifying the code to work with a simpler data structure seems worth it to me. It will probably make it more clear that the downstream code is reasonable.

[aalekseyev]

Regarding the limitation to wrapped libraries only, that seems reasonable to me.

[rgrinberg]

@aalekseyev should be ready now

[rgrinberg]

Something like a user written Root would work if there was a way to specify that it has no module dependencies and we properly sandboxed module compilation. We'd have:

(library
  (module_deps
   (root -> ()))

and then a user written root could be used to do whatever renames are necessary. We'd just need to make sure that -I isn't polluted by other modules in the library.

[bobot]

I'm ok with the limitation to wrapped thing.

[rgrinberg]

@bobot @aalekseyev should be good now. I've switched to the simpler root_module proposal, with full support for unwrapped and non dune libraries. Leaving them unsupported seemed wrong in the end.

Just need to update the docs.

[rgrinberg]

@bobot @aalekseyev does one of you have time to review this PR?