fix: better cycle detection for virtual libraries

CHANGES.md

@@ -1,6 +1,9 @@
 Unreleased
 ----------
 
+- Report cycles between virtual libraries and their implementation (#5050,
+  fixes #2896, @rgrinberg)
+
 - Allow users to specify dynamic dependencies in rules. For example `(deps
   %{read:foo.gen})` (#4662, fixes #4089, @jeremiedimino)
 

src/dune_rules/lib.ml

@@ -1101,6 +1101,21 @@ end = struct
       | Public (_, _) -> From_same_project
     in
     let resolve name = resolve_dep db name ~private_deps in
+    let* resolved =
+      let open Resolve.Build.O in
+      let* pps =
+        let instrumentation_backend =
+          instrumentation_backend db.instrument_with resolve
+        in
+        Lib_info.preprocess info
+        |> Preprocess.Per_module.with_instrumentation ~instrumentation_backend
+        >>| Preprocess.Per_module.pps
+      in
+      let dune_version = Lib_info.dune_version info in
+      Lib_info.requires info
+      |> resolve_deps_and_add_runtime_deps db ~private_deps ~dune_version ~pps
+      |> Memo.Build.map ~f:Resolve.return
+    in
     let* implements =
       match Lib_info.implements info with
       | None -> Memo.Build.return None
@@ -1115,6 +1130,29 @@ end = struct
         in
         Memo.Build.map res ~f:Option.some
     in
+    let* requires =
+      let requires =
+        let open Resolve.O in
+        let* resolved = resolved in
+        resolved.requires
+      in
+      match implements with
+      | None -> Memo.Build.return requires
+      | Some vlib ->
+        let open Resolve.Build.O in
+        let* (_ : lib list) =
+          let* vlib = Memo.Build.return vlib in
+          let* requires_for_closure_check =
+            Memo.Build.return
+              (let open Resolve.O in
+              let+ requires = requires in
+              List.filter requires ~f:(fun lib -> not (equal lib vlib)))
+          in
+          linking_closure_with_overlap_checks None requires_for_closure_check
+            ~forbidden_libraries:(Map.singleton vlib Loc.none)
+        in
+        Memo.Build.return requires
+    in
     let resolve_impl impl_name =
       let open Resolve.Build.O in
       let* impl = resolve impl_name in
@@ -1161,25 +1199,10 @@ end = struct
                              (Package.Name.to_string p')
                          ])))
     in
-    let* resolved =
-      let open Resolve.Build.O in
-      let* pps =
-        let instrumentation_backend =
-          instrumentation_backend db.instrument_with resolve
-        in
-        Lib_info.preprocess info
-        |> Preprocess.Per_module.with_instrumentation ~instrumentation_backend
-        >>| Preprocess.Per_module.pps
-      in
-      let dune_version = Lib_info.dune_version info in
-      Lib_info.requires info
-      |> resolve_deps_and_add_runtime_deps db ~private_deps ~dune_version ~pps
-      |> Memo.Build.map ~f:Resolve.return
-    in
     let* requires =
       Memo.Build.return
         (let open Resolve.O in
-        let* requires = resolved >>= fun r -> r.requires in
+        let* requires = requires in
         match implements with
         | None -> Resolve.return requires
         | Some impl ->

test/blackbox-tests/test-cases/virtual-libraries/github2896.t/run.t

@@ -19,13 +19,23 @@ where vlib is a virtual library, and impl implements this library.
   $ cat >impl/dune <<EOF
   > (library (name impl) (implements vlib) (libraries lib))
   > EOF
+  $ dune build @all
+  Error: Library "vlib" was pulled in.
+  -> required by library "lib" in _build/default/lib
+  -> required by library "impl" in _build/default/impl
+  -> required by _build/default/impl/.impl.objs/byte/vlib.cmo
+  -> required by _build/default/impl/impl.cma
+  -> required by alias impl/all
+  [1]
 
 The implementation impl was built, but it's not usable:
 
   $ echo 'Vlib.run ()' > foo.ml
   $ echo "(executable (name foo) (libraries impl))" > dune
   $ dune exec ./foo.exe
-  File "_none_", line 1:
-  Error: No implementations provided for the following modules:
-           Vlib referenced from lib/lib.cmxa(Lib)
+  Error: Library "vlib" was pulled in.
+  -> required by library "lib" in _build/default/lib
+  -> required by library "impl" in _build/default/impl
+  -> required by executable foo in dune:1
+  -> required by _build/default/foo.exe
   [1]