From 422ce5d660d43e939f5800416a451675e6b67e26 Mon Sep 17 00:00:00 2001
From: Rudi Grinberg <me@rgrinberg.com>
Date: Sun, 22 Mar 2020 19:10:54 -0700
Subject: [PATCH] Fix #2896

Make sure that dependencies of implementations do not create a cycle
back to the virtual library.

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
---
 src/dune/lib.ml                               | 39 ++++++++++++++++++-
 .../virtual-libraries/github2896/run.t        |  6 +++
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/src/dune/lib.ml b/src/dune/lib.ml
index 4c94af369bb2..3fd1bb618f5a 100644
--- a/src/dune/lib.ml
+++ b/src/dune/lib.ml
@@ -861,6 +861,9 @@ module Vlib : sig
     -> linking:bool
     -> t list Or_exn.t
 
+  val validate_impl_closure :
+    lib list -> impl:lib -> vlib:lib -> (unit, exn) result
+
   module Unimplemented : sig
     (** set of unimplemented libraries*)
     type t
@@ -961,6 +964,12 @@ end = struct
 
     type t = lib Map.t
 
+    let filled_impls_only (impls : Partial.t) =
+      Map.filter_map impls ~f:(fun status ->
+          match status with
+          | Partial.No_impl _ -> None
+          | Impl (impl, _stack) -> Some impl)
+
     let make impls ~orig_stack : t Or_exn.t =
       let rec loop acc = function
         | [] -> Ok acc
@@ -1036,6 +1045,15 @@ end = struct
     let+ () = Result.List.iter ts ~f:loop in
     List.rev !res
 
+  let validate_impl_closure (closure : lib list) ~impl ~vlib =
+    let closure = List.map closure ~f:(fun x -> (x, Dep_stack.empty)) in
+    let* impls = Table.Partial.make closure ~orig_stack:Dep_stack.empty in
+    if Table.Partial.is_empty impls then
+      Ok ()
+    else
+      let impls = Table.filled_impls_only impls in
+      validate_closure ~vlib ~impl impls
+
   let associate closure ~orig_stack ~linking =
     let* impls = Table.Partial.make closure ~orig_stack in
     let closure = List.map closure ~f:fst in
@@ -1730,7 +1748,26 @@ module Compile = struct
         >>= Resolve.compile_closure_with_overlap_checks db
               ~stack:Dep_stack.empty ~forbidden_libraries:Map.empty )
     in
-    { direct_requires = t.requires
+    let valid_impl =
+      match t.implements with
+      | None
+      | Some (Error _) ->
+        Ok () (* we'll see the error later *)
+      | Some (Ok vlib) ->
+        (* we need this check to make sure *)
+        let* requires_link = Lazy.force requires_link in
+        Vlib.validate_impl_closure requires_link ~impl:t ~vlib
+    in
+    let direct_requires =
+      let* () = valid_impl in
+      t.requires
+    in
+    let requires_link =
+      match valid_impl with
+      | Ok _ -> requires_link
+      | Error e -> lazy (Error e)
+    in
+    { direct_requires
     ; requires_link
     ; resolved_selects = t.resolved_selects
     ; pps = t.pps
diff --git a/test/blackbox-tests/test-cases/virtual-libraries/github2896/run.t b/test/blackbox-tests/test-cases/virtual-libraries/github2896/run.t
index da6f5fa722bb..421a4f5e9f1f 100644
--- a/test/blackbox-tests/test-cases/virtual-libraries/github2896/run.t
+++ b/test/blackbox-tests/test-cases/virtual-libraries/github2896/run.t
@@ -19,6 +19,12 @@ 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: Implementation impl has a dependency that uses the virtual library
+  vlib
+     - impl at _build/default/impl
+  -> - lib at _build/default/lib
+  [1]
 
 The implementation impl was built, but it's not usable: