Reproduce and fix issue #1647 with cache invalidation

CHANGES.md

@@ -6,6 +6,8 @@ unreleased
     - Omit module prefixes for constructors and record fields in the
       `construct` command (#1618).  Prefixes are still produced when
       warning 42 (disambiguated name) is active.
+    - Correctly invalidate PPX cache when pipeline ran partially (#1650,
+      fixes #1647)
   + editor modes
     - emacs: call merlin-client-logger with "interrupted" if the
       merlin binary itself is interrupted, not just the parsing of the

src/kernel/mpipeline.ml

@@ -81,7 +81,7 @@ module Reader = struct
   type t = {
     result : Mreader.result;
     config : Mconfig.t;
-    cache_was_hit : bool;
+    cache_version : int option;
   }
 end
 
@@ -144,10 +144,14 @@ module Reader_phase = struct
     config : Mconfig.t;
   }
 
-  type output = Mreader.result
+  type output = { result: Mreader.result; cache_version: int }
 
-  let f { source; for_completion; config } =
-    Mreader.parse ?for_completion config source
+  let f =
+    let cache_version = ref 0 in
+    fun { source; for_completion; config } ->
+    let result = Mreader.parse ?for_completion config source in
+    incr cache_version;
+    { result; cache_version = !cache_version }
 
   let title = "Reader phase"
 
@@ -162,10 +166,13 @@ end
 module Reader_with_cache = Phase_cache.With_cache (Reader_phase)
 
 module Ppx_phase = struct
-  type t = { parsetree : Mreader.parsetree; config : Mconfig.t }
+  type t = {
+    parsetree : Mreader.parsetree;
+    config : Mconfig.t;
+    reader_cache_version : int }
   type output = Mreader.parsetree
 
-  let f { parsetree; config } = Mppx.rewrite parsetree config
+  let f { parsetree; config; _ } = Mppx.rewrite parsetree config
   let title = "PPX phase"
 
   module Single_fingerprint = struct
@@ -185,9 +192,9 @@ module Ppx_phase = struct
   end
 
   module Fingerprint = struct
-    type t = Single_fingerprint.t list
+    type t = (Single_fingerprint.t list * int)
 
-    let make { config; _ } =
+    let make { config; reader_cache_version; _ } =
       let rec all_fingerprints acc = function
         | [] -> acc
         | { Std.workdir; workval } :: tl -> (
@@ -199,9 +206,12 @@ module Ppx_phase = struct
                     all_fingerprints (Result.map ~f:(List.cons fp) acc) tl)
                   (Single_fingerprint.make ~binary ~args ~workdir))
       in
-      all_fingerprints (Ok []) config.ocaml.ppx
+      Result.map (all_fingerprints (Ok []) config.ocaml.ppx)
+        ~f:(fun l -> (l, reader_cache_version))
 
-    let equal f1 f2 = List.equal ~eq:Single_fingerprint.equal f1 f2
+    let equal (f1, rcv1) (f2, rcv2) =
+      Int.equal rcv1 rcv2 &&
+      List.equal ~eq:Single_fingerprint.equal f1 f2
   end
 end
 
@@ -241,42 +251,45 @@ let process
         (let (lazy ((_, pp_result) as source)) = source in
           let config = Mconfig.normalize config in
           Mocaml.setup_reader_config config;
-          let { Reader_with_cache.output; cache_was_hit } =
-            let cache_disabling =
-              match (config.merlin.use_ppx_cache, pp_result) with
-              | false, _ -> Some "configuration"
-              | true, Some _ ->
-                (* The cache could be refined in the future to also act on the PP phase.
-                   For now, let's disable the whole cache when there's a PP. *)
-                Some "source preprocessor usage"
-              | true, None -> None
-            in
-            Reader_with_cache.apply ~cache_disabling ~force_invalidation:false
+          let cache_disabling =
+            match (config.merlin.use_ppx_cache, pp_result) with
+            | false, _ -> Some "configuration"
+            | true, Some _ ->
+              (* The cache could be refined in the future to also act on the
+                 PP phase. For now, let's disable the whole cache when there's
+                 a PP. *)
+              Some "source preprocessor usage"
+            | true, None -> None
+          in
+          let { Reader_with_cache.output = { result; cache_version }; _ } =
+            Reader_with_cache.apply ~cache_disabling
               { source; for_completion; config }
           in
-          { Reader.result = output; config; cache_was_hit }
+          let cache_version =
+            if Option.is_some cache_disabling then None else Some cache_version
+          in
+          { Reader.result; config; cache_version }
     )) in
   let ppx = timed_lazy ppx_time (lazy (
-      let (lazy
-          {
+      let (lazy {
             Reader.result = { Mreader.parsetree; _ };
             config;
-            cache_was_hit = source_is_unmodified;
-          }) =
-        reader
+            cache_version;
+          }) = reader
       in
       let caught = ref [] in
       Msupport.catch_errors Mconfig.(config.ocaml.warnings) caught @@ fun () ->
-      let {Ppx_with_cache.output = parsetree; cache_was_hit = _ } =
-        let cache_disabling =
-          if config.merlin.use_ppx_cache then None
-          else Some "configuration"
-        in
-        (* With this, the cache is invalidated even for source changes that don't
-           change the parsetree. To avoid that, we'd have to digest the parsetree
-           in the cache. *)
-        let force_invalidation = not source_is_unmodified in
-        Ppx_with_cache.apply ~cache_disabling ~force_invalidation {parsetree; config}
+      (* Currently the cache is invalidated even for source changes that don't
+          change the parsetree. To avoid that, we'd have to digest the
+          parsetree in the cache. *)
+      let cache_disabling, reader_cache_version =
+        match cache_version with
+        | Some v -> None, v
+        | None -> Some "reader cache is disabled", -1
+      in
+      let { Ppx_with_cache.output = parsetree; _ } =
+        Ppx_with_cache.apply ~cache_disabling
+          {parsetree; config; reader_cache_version}
       in
       { Ppx.config; parsetree; errors = !caught }
     )) in

src/kernel/phase_cache.ml

@@ -24,7 +24,7 @@ module With_cache (Phase : S) = struct
 
   let cache = ref None
 
-  let apply ~cache_disabling ~force_invalidation input =
+  let apply ?(cache_disabling = None) ?(force_invalidation = false) input =
     let title = Phase.title in
     match cache_disabling with
     | Some reason ->

src/kernel/phase_cache.mli

@@ -32,12 +32,12 @@ module With_cache (Phase : S) : sig
   type t = { output : Phase.output; cache_was_hit : bool }
 
   val apply :
-    cache_disabling:string option -> force_invalidation:bool -> Phase.t -> t
+    ?cache_disabling:string option -> ?force_invalidation:bool -> Phase.t -> t
   (** [apply ~cache_disabling ~force_invalidation phase_input] runs the phase
       computation [Phase.f phase_input], if there's some [cache_disabling].
       Otherwise, the phase computation is run with a cache mechanism. Whether
       the cache is invalidated depends on the outcome of a [Phase.Fingerprint]
       comparison between the current fingerprint and the last one. Additionally,
       the invalidation of the cache can be forced by setting the
       force_invalidation parameter to true.*)
-end
+end

tests/test-dirs/with-ppx/issue1647-cache-invalidation.t

@@ -0,0 +1,88 @@
+Make sure that this test doesn't depend on previous state
+  $ $MERLIN server stop-server
+
+  $ cat >dune-project <<EOF
+  > (lang dune 2.0)
+  > EOF
+
+  $ cat > my_ppx.ml <<EOF
+  > open Ppxlib
+  > let rule =
+  >    let ppx =
+  >      let ast_context = Extension.Context.expression in
+  >      let pl = Ast_pattern.(pstr nil) in
+  >      let expand_fn ~ctxt =
+  >        let loc = Expansion_context.Extension.extension_point_loc ctxt in
+  >        Ast_builder.Default.eint ~loc 0 in
+  >      Extension.V3.declare "just_zero" ast_context pl expand_fn
+  > in
+  > Ppxlib.Context_free.Rule.extension ppx
+  > let () = Driver.register_transformation ~rules:[ rule ] "just_zero"
+  > EOF
+
+  $ cat > main.ml <<EOF
+  > let () = print_int [%just_zero]
+  > EOF
+
+  $ cat >dune <<EOF
+  > (library
+  >  (name my_ppx)
+  >  (kind ppx_rewriter)
+  >  (modules my_ppx)
+  >  (libraries ppxlib))
+  > 
+  > (executable
+  >  (name main)
+  >  (modules main)
+  >  (preprocess
+  >   (pps my_ppx)))
+  > EOF
+
+  $ cat > .merlin <<EOF
+  > FLG -ppx '_build/default/.ppx/a56cb746ce56d7c281c7d9796f4166ed/ppx.exe -as-ppx
+  > USE_PPX_CACHE
+  > EOF
+
+  $ dune exec ./main.exe 2>/dev/null
+  0
+
+`errors` requires the typedtree and thus types the buffer
+  $ $MERLIN server errors -log-file merlin_logs \
+  > -filename main.ml 1> /dev/null <main.ml
+  $ cat merlin_logs | grep 'Phase cache' -A 1 | sed "s/[0-9]*//g"
+  # . Phase cache - Reader phase
+  Cache wasn't populated
+  --
+  # . Phase cache - PPX phase
+  Cache wasn't populated
+
+Now we edit the file. This should invalidate the caches
+  $ cat > main.ml <<EOF
+  > let () = print_string [%just_zero]
+  > EOF
+
+
+The `dump source` command only requires the parsetree and only invalidates the
+reader cache:
+  $ $MERLIN server dump -what source  -log-file merlin_logs \
+  > -filename main.ml <main.ml | tr -d '\n' | jq '.value'
+  "let () = print_string ([%just_zero ])"
+
+  $ cat merlin_logs | grep 'Phase cache' -A 1 | sed "s/[0-9]*//g"
+  # . Phase cache - Reader phase
+  Cache invalidation
+
+FIXME: The `dump ppxed-source` wrongly reuse the cached version since this
+behavior is based ont he reader cahce being hit or not
+  $ $MERLIN server dump -what ppxed-source  -log-file merlin_logs \
+  > -filename main.ml <main.ml | tr -d '\n' | jq '.value'
+  "let () = print_string 0"
+
+  $ cat merlin_logs | grep 'Phase cache' -A 1 | sed "s/[0-9]*//g"
+  # . Phase cache - Reader phase
+  Cache hit
+  # . Phase cache - PPX phase
+  Cache invalidation
+
+Stop server
+  $ $MERLIN server stop-server

tests/test-dirs/with-ppx/parsetree-cache.t/run.t

@@ -37,7 +37,7 @@ By default, the cache for the reader phase and the PPX phase are disabled
   Cache is disabled: configuration
   --
   # . Phase cache - PPX phase
-  Cache is disabled: configuration
+  Cache is disabled: reader cache is disabled
 
 The cache can be enabled via the USE_PPX_CACHE directive
   $ cat > .merlin <<EOF
@@ -242,7 +242,7 @@ And Merlin does the right thing.
   Cache is disabled: configuration
   --
   # . Phase cache - PPX phase
-  Cache is disabled: configuration
+  Cache is disabled: reader cache is disabled
 
 Again: when ppx_dep.txt is changed to contain a non-int, the AST contains an error.
   $ cat > ppx_dep.txt <<EOF
@@ -279,7 +279,7 @@ This time, since the PPX cache isn't enabled, Merlin does the right thing here a
   Cache is disabled: configuration
   --
   # . Phase cache - PPX phase
-  Cache is disabled: configuration
+  Cache is disabled: reader cache is disabled
 
 Let's clean up
   $ $MERLIN server stop-server