Fix #3727

[rgrinberg]

To fix this problem, I reuse the machinery in the deprecated library alias
stanza to inject a fake translation stanza for the private name. This is what
Jeremie recommended that I do.

The diff looks quite noisy and I'd appreciate some suggestions on how to
organize things a bit better here.

[nojb]

Is it clear that we don't want to deprecate and eventually forbid using private names across packages?

[aalekseyev]

I'm also unsure about what's desirable here. Naively, I would expect private names to be hidden. (or the "private" terminology changed)
In fact I'm very confused now: if the user can always refer to a library by its private name, why do we need public names at all?

[rgrinberg]

Is it clear that we don't want to deprecate and eventually forbid using private names across packages?

Do you see a reason to do it? Private and public names can be used transparently in dune everywhere else.

In fact I'm very confused now: if the user can always refer to a library by its private name, why do we need public names at all?

For wrapped modules, private names allow you to set the toplevel module name. That doesn't always match the public name.

[rgrinberg]

I'd also like to clarify that private names are going to be visible when there's a package field on private libraries. E.g.

(library
 (name foo)
 (package bar))

(library
 (public_name baz)
 (libraries foo))

So it seems more consistent to allow it everywhere.

[aalekseyev]

Ok, sounds like "private" is just not a good name for this concept.

Is it true that you have to use a private name to refer to the library in OCaml, or am I confused again?

Whatever the answer, I have no objection to this change. (but I haven't read the code in detail)

[rgrinberg]

I'm using the term "private" by analogy to public. I don't think we actually refer to name as private anywhere.

Yes the name field is what is used in OCaml code. However, as I said before it's only relevant for wrapped libraries.

[rgrinberg]

By the way, this PR also fixes the issue in #3447.

It seems like the PR is causes dune to segfault when running the test suite. That's quite strange. Investigating.

In any case, @nojb or @aalekseyev I'd appreciate a review of the code.

[aalekseyev]

So, I have done a test locally:

$ opam install core
...
$ touch dune-workspace
$ mkdir foo
$ cat > foo/foo.ml <<EOF
> include Nano_mutex
> EOF
$ cat > foo/dune <<EOF
> (library (name foo) (libraries nano_mutex))
> EOF
$ dune build
File "foo/dune", line 1, characters 31-41:
1 | (library (name foo) (libraries nano_mutex))
                                   ^^^^^^^^^^
Error: Library "nano_mutex" not found.
Hint: try: dune external-lib-deps --missing @@default
EXIT STATUS 1   

Replacing the reference with core.nano_mutex works.
Doesn't this mean that it's not correct to fix the way "-p" works? We'd need to change more than that, in particular how we find installed libraries.

And, presumably, we don't want to do that because it's slow to scan the entire library database?

[rgrinberg]

Replacing the reference with core.nano_mutex works.

This isn't quite right. You're referring to a private name outside the scope where it's defined. That is expected to fail. You should however freely refer to nano_mutex from inside the core project (where you defined nano_mutex).

[aalekseyev]

Oh, I see my mistake. I assumed a package was a unit of scope isolation, but it's not: a project is.
Thanks for clarifying.

[aalekseyev]

I thought this PR was pretty hard to understand, in part because very little is documented, and in part because it seems we're (ab-?)using a Deprecated_library_name for something that's not at all deprecated.

Do you think you can add clarifying comments and/or renamings that could explain why things are done this way?

In particular, the change in gen_rules.ml looks like dark magic and needs a justification in a comment.

Maybe Deprecated_library_name can be renamed to e.g. Library_name_forwarding so that it's clear that it has nothing to do with deprecation, at least in this case.

[rgrinberg]

@aalekseyev I've split the deprecation from the redirect stanza. Hopefully things are a little more clear now.

[aalekseyev]

Looked through the code and it looks good modulo a few comments.
Sorry for dragging my heels on this.

[aalekseyev]

Looks good!