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

[RFC] Define a ROOT module with toplevel module #3865

Closed
bobot opened this issue Oct 16, 2020 · 22 comments
Closed

[RFC] Define a ROOT module with toplevel module #3865

bobot opened this issue Oct 16, 2020 · 22 comments
Assignees
Labels
proposal RFC's that are awaiting discussion to be accepted or rejected

Comments

@bobot
Copy link
Collaborator

bobot commented Oct 16, 2020

Desired Behavior

When discussing #3825 we realized that dune can help with the absence of a ROOT module which contains the toplevel module before any open by being able to generate one. However the name to use as always been a problem for adding this feature to the compiler. Dune has the hability through the dune-project file to activate the feature or allow to change the name. Also what are toplevel modules is not exactly defined. Dune by having a more highlevel view can better define them.

Sought advantage of having a ROOT module:

  • For ppx allow to be sure to access their runtime module.
  • For user allow to access library hidden by local modules

So the choice of what is a toplevel module is important:

  • toplevel module of wrapped libraries the library depend on: just add the main module of the libraries handled by dune and wrapped (what is in the name field of a library).
  • module for which the cmi are accessible in libraries the library depend on: add a binding for all the cmi that are in the included directories.

Finally we need a way to indicate the toplevel name to ppx. ppxlib can help by giving a simple API which use the ROOT module if available otherwise use directly the module.

Example

TODO

@bobot bobot added the proposal RFC's that are awaiting discussion to be accepted or rejected label Oct 16, 2020
@bobot bobot self-assigned this Oct 16, 2020
@rgrinberg
Copy link
Member

Will the ROOT module be written by the user or generated by dune?

If the module is user written, what is the difference between such a module and a module without any dependencies? E.g:

(library
 (name foo)
 (module_deps (root -> ()))

@rgrinberg
Copy link
Member

The incentive to solve the issue of ppx + runtime libraries seems nice, but I don't quite see how this proposal addresses it. As far as I see, the ppx needs to ask dune for a ROOT module that:

  • It can write the list of imports directly. E.g. module Ppx_runtime = Ppx_runtime to allow Root.Ppx_runtime.
  • Or the list of imports would be generated automatically by dune from the libraries used.

@bobot
Copy link
Collaborator Author

bobot commented Oct 20, 2020

The ROOT module would be generated by dune. The list of imports would be generated automatically by dune from the libraries used. The imports to generate are the toplevel modules I'm speaking about in the description of the issue. I'm editing the description to make clear that dune would generate the file.

@aalekseyev
Copy link
Collaborator

I don't see how this fixes the ppx runtime dependency problem.
It's equally easy for the user to write module ROOT = MY_ROOT as it is to shadow a ppx runtime dep.
In fact the former is probably easier.
And the ability to shadow things by open or include means that we can't get away with a per-project "forbidden" module name, either.

It does help with "renaming" library deps, though, so that part seems fine.

By the way, I couldn't understand the section where you say "So the choice of what is a toplevel module is important" followed by bullet-points. Maybe you could expand each bullet-point a bit?

@rgrinberg
Copy link
Member

It's equally easy for the user to write module ROOT = MY_ROOT as it is to shadow a ppx runtime dep.
In fact the former is probably easier.
And the ability to shadow things by open or include means that we can't get away with a per-project "forbidden" module name, either.

Perhaps just having a convention that this module cannot be shadowed is good enough? We could even call it something like Root_ to make it clearer that it isn't a normal module name. This isn't very elegant, but I like better than having nothing.

@bobot
Copy link
Collaborator Author

bobot commented Oct 21, 2020

If we want to be cautious we can write a ppx which check that we never have module Root_ =. Of course this check doesn't preserve against an include or open of a module defined outside the dune project which define the Root_ module (we can also look at the cmi but it starts to be crazy).

And at the end the feature in dune can be just an experiment for a futur feature in the compiler. For that direction we can fix the name of the root module: OCAML_ROOT_NAMESPACE or ROOT__.

Ok I'm expanding the bullet point.

@aalekseyev
Copy link
Collaborator

Oh, got it (I still had to re-read it, like, 3 times though).
So if we depend on a wrapped library that has:

a.cmi
a__X.cmi
a__Y.cmi

you're asking if we should only have module A = A, or to also include module A__X = A__X...

I think definitely the former. The only supported way to access wrapped libraries is through their public renaming module, so that's the only thing we should expose.

@bobot
Copy link
Collaborator Author

bobot commented Oct 21, 2020

I agree that it is better for wrapped library. But it means we do it only for wrapped dune libraries. Or for non-dune libraries we can do the latter. Sorry for the complicated explanation.

@aalekseyev
Copy link
Collaborator

I'm not very familiar with non-wrapped libraries, but I expect that for them we'll want to indeed have one alias per .cmi they expose.
Basically, whatever the public interface of the library is, we should create an alias for it.

@aalekseyev
Copy link
Collaborator

aalekseyev commented Oct 21, 2020

Here's another problem with having the ROOT module: the user can open it, or include it, or module type of it, and then we lose the property that adding a useless dependency is a no-op.

Consider code like this:

module A = struct 
  let x = 8
end
open ROOT
let () = printf "%d" A.x

What the code prints may depend on whether or not you depend on a library A. The problem becomes potentially more confusing if you consider how implicit_transitive_deps can make you depend on things you've never heard about.

@bobot
Copy link
Collaborator Author

bobot commented Oct 22, 2020

Interesting, forbidding open and include of the module should be also checked at least until no implicit_transitive_deps is the default.

@bobot
Copy link
Collaborator Author

bobot commented Oct 22, 2020

I'm moving to postponing this issue, except if it is found urgent by some.

@rgrinberg
Copy link
Member

I'd be interested in exploring this proposal in the short term to get an idea if #3825 is going to be necessary.

@bobot @aalekseyev I'd like to use #3825 or something similar in the short term, but I'm not against implementing this proposal or even (module_deps ..). Or do you think that #3825 would be harmless to have if this proposal was accepted later?

@bobot
Copy link
Collaborator Author

bobot commented Oct 22, 2020

#3825 can be put behind an experimental flag, and is harmless.

The problem with this ROOT proposal for me is choosing what is the set of toplevel libraries. If a ppx use Re, should the ppx use ROOT.Re or ROOT.MYRUNTIMELIBRARY.Re (the first one will not work in implicit_transitive_deps in a user code that doesn't depend on Re if we don't consider it to be a toplevel). For the ppx case I wonder if we should not aim at more: in a perfect world the ppx generated code would be able to access cmi that the developer code could not. The handling of cmi by the compiler is not very fine grain. Adding in the compiler an extension point that shortcut the lookup could be interesting for dune and ppxlib module FOO = {% ocaml.load_module ".../foo.cmi" %}.

In conclusion the ppx case shouldn't be considered in this proposal, as I did at first, I believe we should focus on the developer case.

@rgrinberg
Copy link
Member

I agree that we should shelve the ppx use case for now. How about I quickly turn #3825 into:

(library
 (root_module ROOT))

The machinery to do all of this is basically done in #3825. I think this is a little more elegant than my proposal.

I'll also mention that I discussed this proposal with Jeremie briefly, and he's fine with it, but strongly against introducing any reserved identifiers that aren't chosen explicitly by the user.

@bobot
Copy link
Collaborator Author

bobot commented Oct 28, 2020

I'm ok with that.

@rgrinberg
Copy link
Member

I'm afraid that root_module field is not sufficient on its own. The problem arises when we depend on an unwrapped library:

(library
 (libraries foo.bar) ;; foo.bar is unwrapped with modules A, B
 (root_module ROOT))

What name do we choose for foo.bar? It seems like we cannot get away from introducing individual names for the libraries. At which point, my original proposal seems simpler.

@bobot
Copy link
Collaborator Author

bobot commented Nov 6, 2020

Can we do it only for dune wrapped library? This possible constraint arose in the conversation.

@rgrinberg
Copy link
Member

We can, but I think that silently skipping unwrapped libraries is bad. At least the other approach can be adapted to support unwrapped libraries.

@bobot
Copy link
Collaborator Author

bobot commented Nov 9, 2020

In case of dune unwrapped library, can we just add the modules? So in your example A, B. In the unwrap case the module A and B should not hide other top-level module.

@rgrinberg
Copy link
Member

Yeah, I suppose that would be fine and not too surprising for the user.

@aalekseyev do you agree?

@rgrinberg
Copy link
Member

This was implemented

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal RFC's that are awaiting discussion to be accepted or rejected
Projects
None yet
Development

No branches or pull requests

3 participants