Allow defining libs with same name in multiple contexts

[jchavarri]

Continuation of #10220. Supersedes #9839.

While #10220 fixed name collisions in exes and emits declared in separate contexts through enabled_if, this PR implements the same improvement for the library stanzas.

Changes in invariants

• before this PR, there was an invariant (implemented in the two DB creation functions in Scope) that prevented having two libraries sharing either private names or public names.

• after this PR, we need to relax this invariant to allow two or more libraries with the same private name or public name, because they might exist in different contexts, and at library DB creation time we can't resolve the enabled_if conditions as we don't have the expander available yet.

Implementation

• The core change is in the way lib databases are created in Scope. While before the duplication checking logic was performed over there, and the functions to resolve libraries from their names would return a single value, now they can return multiple ones. This is done through new variants Many and Multiple_results.
• Another change is performed on the way library rules are created. Before, these rules would be created regardless the state of the library enabled_if field. Now we check for it and only create the rules if the library is actually enabled.
• Besides the above, another change was necessary to make sure we can detect duplication of public libraries under the same folder. The reason is that the public lib database is indexed by public names, but collisions can occur if the private names of the libraries coincide. For this reason, before creating the rules of each library, we perform a lookup in the database of the library private name to trigger any errors in those cases.

[jchavarri]

@rgrinberg I am trying to find a solution for this issue: whenever I define two libraries in the same folder in 2 separate contexts using enabled_if I get this error:

dune/src/dune_rules/ml_sources.ml

Line 66 in 91f6b39

"Library %S appears for the second time in this directory"

(see eif-library-name-collision-same-folder.t for an example).

The reason is that when Dune creates the Ml_sources.t for a given folder, it doesn't take into account any context-related information, it just gets all libraries available in that folder through this call:

dune/src/dune_rules/dir_contents.ml

Line 272 in 91f6b39

let libs = Scope.DB.find_by_dir dir >>| Scope.libs in

Afaiu, all libraries DBs are created early in the build process, at a time where we don't know yet under which context the DB will be queried from.

I know we previously discussed about having some new "library id" that includes the build path etc, but I think that approach would be useless, because at the time were Ml_sources.make is called, we can know under which build context we are building, but at the DB level we can't know if a given library is defined for that context or not.

One solution could be to defer the DB creation (Scope.DB.all) until later in the process, at a point where we actually know which build context we're in, and so we can either only add the libraries that are enabled in that context to the DB, or at least track under which context they're enabled. But that sounds like a big change. Wdyt?

[rgrinberg]

The reason is that when Dune creates the Ml_sources.t for a given folder, it doesn't take into account any context-related information, it just gets all libraries available in that folder through this call:

It doesn't take into account any context information because the scopes are already per context. Also, the library database is only needed to resolve the virtual libraries of implementations. So it can be used to fetch all the libraries in the entire scope, rather than the directory.

Afaiu, all libraries DBs are created early in the build process, at a time where we don't know yet under which context the DB will be queried from.

I think you've mentioned this a couple of times, but it's incorrect. Every library database belongs to a particular context. It might help if you explain why you think they're context independent, so that we could point out the source of the confusion.

I know we #9839 (comment) about having some new "library id" that includes the build path etc, but I think that approach would be useless, because at the time were Ml_sources.make is called, we can know under which build context we are building, but at the DB level we can't know if a given library is defined for that context or not.

Let me clarify the approach a bit. Your modifications to the library database are fine. For Ml_sources, you can do the following:

1. For a particular library in your directory, check if the library is enabled using the library stanza you have on hand. There's no need to touch the library database at all

2. For enabled libraries, the code should do exactly what it did before.

3. For disabled libraries, you just need to record that they're disabled. So that calls like Ml_sources.modules fail appropriately. This step will need to modify Ml_sources.t to make it possible to answer this.

Thinking about this issue a little more, I'm actually not sure if we support two executables in the same directory either. E.g.:

(executable
 (name foo)
 (enabled_if false))
(executable
 (name foo)
 (enabled_if true))

I would imagine that we'd get the same duplicate executable error. I think it would be helpful to solve the issue for executables and melange.emit first. It should be a bit simpler, and it would demonstrate what would be necessary to make this work.

[jchavarri]

I think you've mentioned this a couple of times, but it's incorrect. Every library database belongs to a particular context. It might help if you explain why you think they're context independent, so that we could point out the source of the confusion.

Yes, they are created by context, but what I mean is that the database doesn't reflect any context-related info that the library might use in enabled_if. Iow, we blindly add all libs available to the DB, regardless what their enabled_if status is under the context of the DB.

3. For disabled libraries, you just need to record that they're disabled. So that calls like Ml_sources.modules fail appropriately.

This is the tricky part. The way Ml_sources exposes its data to other modules is through the for_ type. But this type only tracks the its data (libs, exes, or melange.emits) by name, or target name in the case of melange.emit.

In the case of having two libraries with the same name in the same folder, I don't see how Ml_sources can differentiate between them, because just by their name they are the same, and are located in the same folder. The only option that I can think about is that instead of using Lib_name.t as key of the map that Ml_sources stores, we use some combination of Lib_name.t and enabled status (e.g. foo,true and foo,false), so that collisions between enabled and disabled libs can be skipped, but we still can track "real" ones when two enabled libs with the same name are used in the same folder. Does that make sense?

I think it would be helpful to solve the issue for executables and melange.emit first. It should be a bit simpler, and it would demonstrate what would be necessary to make this work.

That makes sense, thanks for the idea, will work on this now then.

[jchavarri]

The way Ml_sources exposes its data to other modules is through the for_ type. But this type only tracks the its data (libs, exes, or melange.emits) by name, or target name in the case of melange.emit.

(Note to self) This is the main reason why "cutting off early" (i.e. just avoid adding the rules of a library in Gen_rules under a given context if that library enabled_if field resolves to false under that context) doesn't work. Because in the case where two libs with the same name exist under the same folder and enabled by context conditionally, when Ml_modules.ocaml goes to resolve the modules of the enabled lib, it would lead into the "appears for the second time" error.

[jchavarri]

@rgrinberg @anmonteiro I'm having some trouble while trying to maintain the behavior in this test case. Writing here my stream of thoughts in case you see some obvious path forward.

The current behavior in main branch is driven by these lines:

dune/src/dune_rules/scope.ml

Lines 73 to 74 in a18c530

	| Redirect (loc1, lib1), Redirect (loc2, lib2) ->
	if Lib_name.equal lib1 lib2 then Ok v1 else Error (loc1, loc2)

In the case of two public libs with names baz.foo and bar.foo the code above would run through the error branch as the private names are equal but the public ones are not. Afaics, there's some logic in Lib_name that transforms public names into private ones (by removing the dot) to use as keys of the map, so that's why this collision can be detected.

But in this PR, we have to defer this error until a later point when the current context is available, so the case above just adds the two libraries to the list set as payload of Redirect and then keeps on. Eventually rules are added for both public libraries and the "Multiple rules" error is triggered.

What I can't figure out is that all the queries for lib information are done against the public libs DB using the public names baz.foo and bar.foo, so there's no way to detect the duplication after the fact. Maybe we have to track duplicates and keep them stored in the Project variant, so that we can error out later on when the current build context is defined?

dune/src/dune_rules/scope.ml

Lines 139 to 140 in a18c530

	| Library (_, { project; visibility = Public p; _ }) ->
	Some (Public_lib.name p, Project project)

[jchavarri]

Still pretty blocked 😞 There seems to be a database for all libraries, but then a separate DB for public libs. This one is the one used to resolve public libraries apparently, and it's indexed by the public libs public names.

So when there are only public libraries that conflict, the collision detection won't work because the name resolution will go through the Project branch:

dune/src/dune_rules/scope.ml

Lines 119 to 121 in 40ea41f

	| Some (Project project) ->
	let scope = find_by_project (Fdecl.get t) project in
	Lib.DB.Resolve_result.redirect scope.db (Loc.none, name)

At that point, all the potential duplicates are lost, so the rules creation proceeds and the conflictive cmxs rules are created.

@rgrinberg do you have any pointers? Should we somehow keep 2 indexes in public_libs, one keyed by public names and another with private? Thanks.

[jchavarri]

I attempted a fix in b970f3c.

[jchavarri]

The idea is to perform a query to the DB using the libraries private names before adding their rules in Gen_rules.of_stanza. Then check the result of this query for Invalid values, and proceed accordingly to show the user error.

[anmonteiro]

After taking a brief look at the code, I would have thought:

• we create a DB with all the public libraries in the project from the stanzas
  • this DB has ~parent:(Some installed_libraries)
• when we create the DB with all the public libraries in the project, we could change the map that we generate:

- Found_or_redirect.t Lib_name.Map.t
+ Found_or_redirect.t list Lib_name.Map.t

Given this change, I think you could perform your check like you did for the private libraries?

[jchavarri]

The Found_or_redirect.t Lib_name.Map.t type is the one used to create the "private" database, the public database type is redirect_to Lib_name.Map.t.

I tried changing the former type as suggested, the result can be seen in jchavarri#11 (draft PR). This change doesn't affect behavior, but I think it makes the pattern matching a bit more noisy.

The main problem, as far as I understand, is that the public DB is indexed by public lib names, but the name collisions happen when the private names are used (i.e. in the private DB). Because public lib installation doesn't query the private DB but the public one, then the conflicts are never detected. That's why I had to add some code that tries to resolve the library private names:

dune/src/dune_rules/gen_rules.ml

Lines 115 to 118 in 531e4d3

	(* This check surfaces conflicts between private names of public libraries,
	without it the user might get duplicated rules errors for cmxs
	when the libraries are defined in the same folder and have the same private name *)
	let* res = Lib.DB.find_invalid db (Library.private_name lib) in

[anmonteiro]

I see. The code in Gen_rules doesn't match my intuition about this change, but I'm not intimately familiar with Lib and Scope apart from my cross-compilation changes.

Since you have a significant amount of context about this now, could you summarize the invariants/requirements we're trying to achieve? I'll take a stab at an incomplete list:

• requirement: resolving a library by its private name should return all libraries matching that name

  • we filter them based on the context they belong to, expect to find only 1, error if that's not the case
  • invariant: resolving a private library returns 1 library per context, or errors out if libs_in_current_context > 1

• requirement: resolving a public library by its public name should return all libraries matching that name

  • however, for public libraries, there could exist public libraries a and b with a common private name priv, e.g.

  (library
   (name priv)
   (public_name a)) 
  (library
   (name priv)
   (public_name b))

  • In this case, we must go look for the private library name to check whether there's another conflicting library (which could be both public or private -- we might need a test with a public library that has the same private name as a private lib?)
  • invariant: the transitive closure of libraries for a module group must not contain multiple public libraries with the same private name

I tried to summarize my understanding of the current requirements to help me review the PR better. Did I get anything wrong and/or, is there something I missed?

[jchavarri]

To be honest, I don't think I understand everything that is going on to the level that I can explain the invariants in too much detail. At the (very) basic level, I could say that:

• before this PR, there was an invariant (implemented in the two DB creation functions) that prevented having two libraries sharing either private names or public names.
• after this PR, we need to relax this invariant to allow two or more libraries with the same private name or public name, because they might exist in different contexts, and at library DB creation time we can't resolve the enabled_if conditions as we don't have the expander available yet.

Maybe one important piece of information (that I recently understood) is that the "redirects" in the private DB are redirecting from the private names to the public names, which is a bit counterintuitive. I would have expected redirects to go from public names to private ones, because (I assume) all libs have a private name, but not all have a public name. But I assume there are good reasons for that.

This means that the private DB can contain Found values with either public names, or private names. This can be seen on the usage of Library.best_name conf when Found values are added into it:

dune/src/dune_rules/scope.ml

Line 69 in 171c231

Library.best_name conf, Found_or_redirect.found info)

As an example of the above, if we have a public library with private name foo and public name bar.foo, there will be two entries in the private DB map:

• a Redirect value, indexed with key foo
• a Found value, indexed with key bar.foo

On cases where the conflict exists on the private names of public libraries (like the one tracked in lib-collision-public-same-folder.t), the conflict is detected if we ever have to resolve the private names. But for installing public libs, that's never the case, we will just look for a library called bar.foo, get a Found value back, and then continue (until we get an error because both libs are trying to install cmxs under the same folder, because for that installation the private names are used).

[jchavarri]

The code in Gen_rules doesn't match my intuition about this change

Maybe the call to Lib.DB.find_invalid could be added somewhere else, it doesn't necessarily need to be in Gen_rules, I just didn't know where to place it.

[jchavarri]

I just realized I was missing a few cases from the tests, mostly mixing public and private libs that collision. These cases are not covered by the current code.

So while I thought the PR was ready to review, I am currently looking into what should be modified to cover those cases as well, and I don't know how much code will be changing.

[jchavarri]

This is ready to review. The case with a mix of public and private library is now covered.

The error messages can be improved, but at least the behavior is there already. I included some inline comments for extra context. Thanks.

[jchavarri]

I understood that "available" means rather "exists". For inexistent libraries, enabled_in_context might return true surprisingly, so we have to keep both conditions in the check.

[jchavarri]

Adding a new variant to distinguish between a regular redirect (public libs) and deprecated libs. Treating them both the same way leads to all the tests in test/blackbox-tests/test-cases/deprecated-library-name/features.t failing because of duplicated errors. See related PR #10231 (those changes were added directly into this PR).

[jchavarri]

If we don't properly distinguish between libraries with different info, the paths will be broken (when getting the info for library foo in folder b, we would get the lib foo in folder a).

[jchavarri]

This is roughly the same treatment for each variant as the one in the "non-multiple" case, but we filter out disabled libs in the Found branch.

[jchavarri]

This is not as exhaustive as the check it had originally (see #9964), but it seems to get the job done, for the purposes of memoization.

[rgrinberg]

From my brief look at the PR, I would have expected the following changes:

The redirect constructor is now only active if it's corresponding enabled_if was turned on. So it would be:

type t =
  | Redirect of (Loc.t * Lib_name.t) * Toggle.t Memo.t

Crucially, we only follow the redirect if we resolve the Toggle.t to be enabled.

find_invalid shouldn't be needed at all if we improve how the library database is constructed. Previously, it was the case that every library database had a unique set of names. So one could go from Library.t to Lib.t just by looking it up using the name. With this PR, such a look up is no longer reliable. The database should support the following API instead:

module Id : sig
  type t
  val of_stanza : Library.t -> t
end
val find : Lib_name.t -> Id.t option Memo.t
val resolve : Id.t -> Lib.t Resolve.t Memo.t

And to summarize the main uses of this API:

• To check if a library is enabled when generating the rules, use Id.of_stanza and then check if it resolves into something valid.
• To resolve names in (libraries ..) fields, percent forms, etc, compose find and resolve to get back the old behavior.

The API above is meant to be simple to explain, but I'm sure it can be tweaked so that minimal downstream code changes are needed.

[anmonteiro]

I tried implementing Rudi's proposal in jchavarri#12, but there's no Id module and I couldn't understand how it would fit.

There are changes in the cram test snapshots that lead me to believe we're doing less evaluation (fewer errors in some tests), but I'm not sure if I got the entire implementation right.

[jchavarri]

Continues in #10307.