[merlin] Cleanup leftover merlin files (#4261)

* Make Build_system delete left-over `.merlin` files
* simplify code managing the Promoted_to_delete db
* Add a test for merlin files cleanup

Signed-off-by: Ulysse Gérard <[email protected]>
Co-authored-by: Arseniy Alekseyev <[email protected]>

src/dune_engine/build_system.ml

@@ -39,10 +39,21 @@ end = struct
     | Some path -> mkdir_p path
 end
 
+(* [Promoted_to_delete] is used mostly to implement [dune clean]. It is an
+   imperfect heuristic, in particular it can go wrong if:
+
+   - the user deletes .to-delete-in-source-tree file
+
+   - the user edits a previously promoted file with the intention of keeping it
+   in the source tree, or creates a new file with the same name *)
 module Promoted_to_delete : sig
   val add : Path.t -> unit
 
-  val load : unit -> Path.Set.t
+  val remove : Path.t -> unit
+
+  val mem : Path.t -> bool
+
+  val get_db : unit -> Path.Set.t
 end = struct
   module P = Dune_util.Persistent.Make (struct
     type t = Path.Set.t
@@ -52,30 +63,50 @@ end = struct
     let version = 1
   end)
 
-  let db = ref Path.Set.empty
-
   let fn = Path.relative Path.build_dir ".to-delete-in-source-tree"
 
+  (* [db] is used to accumulate promoted files from rules. *)
+  let db = lazy (ref (Option.value ~default:Path.Set.empty (P.load fn)))
+
+  let get_db () = !(Lazy.force db)
+
+  let set_db new_db = Lazy.force db := new_db
+
   let needs_dumping = ref false
 
+  let modify_db f =
+    match f (get_db ()) with
+    | None -> ()
+    | Some new_db ->
+      set_db new_db;
+      needs_dumping := true
+
   let add p =
-    if not (Path.Set.mem !db p) then (
-      needs_dumping := true;
-      db := Path.Set.add !db p
-    )
+    modify_db (fun db ->
+        if Path.Set.mem db p then
+          None
+        else
+          Some (Path.Set.add db p))
 
-  let load () = Option.value ~default:Path.Set.empty (P.load fn)
+  let remove p =
+    modify_db (fun db ->
+        if Path.Set.mem db p then
+          Some (Path.Set.remove db p)
+        else
+          None)
 
   let dump () =
     if !needs_dumping && Path.build_dir_exists () then (
       needs_dumping := false;
-      load () |> Path.Set.union !db |> P.dump fn
+      get_db () |> P.dump fn
     )
 
+  let mem p = Path.Set.mem !(Lazy.force db) p
+
   let () = Hooks.End_of_build.always dump
 end
 
-let files_in_source_tree_to_delete () = Promoted_to_delete.load ()
+let files_in_source_tree_to_delete () = Promoted_to_delete.get_db ()
 
 module Alias0 = struct
   include Alias
@@ -879,6 +910,40 @@ end = struct
         ~subdir:(Path.Build.basename dir)
   end
 
+  (* TODO: Delete this step after users of dune <2.8 are sufficiently rare. This
+     step is sketchy because it's using the [Promoted_to_delete] database and
+     that can get out of date (see a comment on [Promoted_to_delete]), so we
+     should not widen the scope of it too much. *)
+  let delete_stale_dot_merlin_file ~dir ~source_files_to_ignore =
+    (* If a [.merlin] file is present in the [Promoted_to_delete] set but not in
+       the [Source_files_to_ignore] that means the rule that ordered its
+       promotion is no more valid. This would happen when upgrading to Dune 2.8
+       from ealier version without and building uncleaned projects. We delete
+       these leftover files here. *)
+    let merlin_file = ".merlin" in
+    let source_dir = Path.Build.drop_build_context_exn dir in
+    let merlin_in_src = Path.Source.(relative source_dir merlin_file) in
+    let source_files_to_ignore =
+      if
+        Promoted_to_delete.mem (Path.source merlin_in_src)
+        && not (Path.Source.Set.mem source_files_to_ignore merlin_in_src)
+      then (
+        let path = Path.source merlin_in_src in
+        Log.info
+          [ Pp.textf "Deleting left-over Merlin file %s.\n"
+              (Path.to_string path)
+          ];
+        (* We remove the file from the promoted database *)
+        Promoted_to_delete.remove path;
+        Path.unlink_no_err path;
+        (* We need to keep ignoring the .merlin file for that build or Dune will
+           attempt to copy it and fail because it has been deleted *)
+        Path.Source.Set.add source_files_to_ignore merlin_in_src
+      ) else
+        source_files_to_ignore
+    in
+    source_files_to_ignore
+
   let load_dir_step2_exn t ~dir =
     let context_name, sub_dir =
       match Dpath.analyse_path dir with
@@ -947,6 +1012,9 @@ end = struct
       Path.Build.Set.to_list source_files_to_ignore
       |> Path.Source.Set.of_list_map ~f:Path.Build.drop_build_context_exn
     in
+    let source_files_to_ignore =
+      delete_stale_dot_merlin_file ~dir ~source_files_to_ignore
+    in
     (* Take into account the source files *)
     let to_copy, source_dirs =
       match context_name with

test/blackbox-tests/test-cases/promote/merlin-files.t/dune-project

@@ -0,0 +1 @@
+(lang dune 2.8)

test/blackbox-tests/test-cases/promote/merlin-files.t/run.t

@@ -0,0 +1,59 @@
+To mimic the behavior of Dune < 2.8 we add rules generating and promoting
+until-clean .merlin files in both source folders (root and subfolder subdir)
+We also add an other promotion that should not be impacted by these changes.
+  $ cat >dune <<EOF
+  > (executable
+  >  (name foo)
+  >  (promote (until-clean)))
+  > 
+  > (rule
+  >  (targets .merlin)
+  >  (action (with-stdout-to .merlin (echo "test")))
+  >  (mode (promote (until-clean))))
+  > EOF
+
+  $ mkdir subdir
+
+  $ cat >subdir/dune <<EOF
+  > (rule
+  >  (targets .merlin)
+  >  (action (with-stdout-to .merlin (echo "test")))
+  >  (mode (promote (until-clean))))
+  > EOF
+
+Building the project will promote .merlin files and foo.exe
+  $ dune build
+  $ ls -a | grep -i -e .merlin -e foo.exe
+  .merlin
+  foo.exe
+
+  $ ls -a subdir | grep -i -e .merlin -e foo.exe
+  .merlin
+
+Nothing happen on rebuild, rules are still in place, promoted files remain
+  $ dune build --verbose 2>&1 | grep "left-over"
+  [1]
+
+  $ ls -a | grep -i -e .merlin -e foo.exe
+  .merlin
+  foo.exe
+
+  $ ls -a subdir | grep -i -e .merlin -e foo.exe
+  .merlin
+
+Now we remove the rules with promotions
+  $ cat >dune <<EOF
+  > EOF
+  $ cat >subdir/dune <<EOF
+  > EOF
+
+Next build Dune will delete the leftover .merlin but not foo.exe
+  $ dune build --verbose 2>&1 | grep "left-over"
+  Deleting left-over Merlin file .merlin.
+  Deleting left-over Merlin file subdir/.merlin.
+
+  $ ls -a | grep -i -e .merlin -e foo.exe
+  foo.exe
+
+  $ ls -a subdir | grep -i -e .merlin -e foo.exe
+  [1]