Merge pull request #1120 from ocaml/typer-caching-policy

Stricter typechecker caching policy

src/kernel/mpipeline.ml

@@ -22,11 +22,35 @@ let timed_lazy r x =
 module Cache = struct
   let cache = ref []
 
+  (* Values from configuration that are used as a key for the cache.
+     These values should:
+     - allow to maximize reuse; associating a single typechecker instance to a
+       filename and directory is natural, but keying also based on verbosity
+       makes no sense
+     - prevent reuse in different environments (if there is a change in
+       loadpath, a new typechecker should be produced).
+
+     It would be better to guarantee that the typechecker was well-behaved
+     when the loadpath changes (so that we can reusing the same instance, and
+     let the typechecker figure which part of its internal state should be
+     invalidated).
+     However we already had many bug related to that.  There are subtle changes
+     in the type checker behavior accross the different versions of OCaml.
+     It is simpler to create new instances upfront.
+  *)
+
+  let key config =
+    Mconfig.(
+      config.query.filename,
+      config.query.directory,
+      config.ocaml,
+      config.findlib,
+      {config.merlin with log_file = None; log_sections = []}
+    )
+
   let get config =
     let title = "pop_cache" in
-    let key =
-      Mconfig.(config.query.directory, config.query.filename, config.ocaml)
-    in
+    let key = key config in
     match List.assoc key !cache with
     | state ->
       cache := (key, state) :: List.remove_assoc key !cache;

tests/dune.inc

@@ -1124,6 +1124,21 @@
          (diff? %{t} %{t}.corrected)))))))
 (alias (name runtest) (deps (alias type-expr-test)))
 
+(alias
+ (name typer-cache-test)
+ (deps (:t ./test-dirs/typer-cache/test.t)
+       (source_tree ./test-dirs/typer-cache)
+       %{bin:ocamlmerlin}
+       %{bin:ocamlmerlin-server})
+ (action
+   (chdir ./test-dirs/typer-cache
+     (setenv MERLIN %{exe:merlin-wrapper}
+     (setenv OCAMLC %{ocamlc}
+       (progn
+         (run %{bin:mdx} test --syntax=cram %{t})
+         (diff? %{t} %{t}.corrected)))))))
+(alias (name runtest) (deps (alias typer-cache-test)))
+
 (alias
  (name warnings-backtrack)
  (deps (:t ./test-dirs/warnings/backtrack.t)

tests/test-dirs/typer-cache/test.ml

@@ -0,0 +1 @@
+open Dep

tests/test-dirs/typer-cache/test.t

@@ -0,0 +1,171 @@
+Instances of the typechecker are cached based on configuration 
+(values of type `Mconfig.t`).
+
+Older versions of Merlin ignored some components resulting in possible
+mismatches between the internal configuration of the typechecker (loadpath,
+global modules visible from the environment) and Merlin configuration.
+
+For instance, `-package` and `-cmi-path` were ignored.
+
+The server might already be running, we kill it to make sure we start from a
+clean slate:
+
+  $ $MERLIN server stop-server
+
+We build a dep which we will be revealed to Merlin later:
+
+  $ $OCAMLC -c sub/dep.ml
+
+First try with dep hidden:
+
+  $ $MERLIN server errors -filename test.ml < test.ml
+  {
+    "class": "return",
+    "value": [
+      {
+        "start": {
+          "line": 1,
+          "col": 5
+        },
+        "end": {
+          "line": 1,
+          "col": 8
+        },
+        "type": "typer",
+        "sub": [],
+        "valid": true,
+        "message": "Unbound module Dep"
+      }
+    ],
+    "notifications": []
+  }
+
+For reference, the answer in single mode:
+
+  $ $MERLIN single errors -filename test.ml < test.ml
+  {
+    "class": "return",
+    "value": [
+      {
+        "start": {
+          "line": 1,
+          "col": 5
+        },
+        "end": {
+          "line": 1,
+          "col": 8
+        },
+        "type": "typer",
+        "sub": [],
+        "valid": true,
+        "message": "Unbound module Dep"
+      }
+    ],
+    "notifications": []
+  }
+
+
+We try again after revealing the dependency:
+
+  $ $MERLIN server errors -filename test.ml -cmi-path sub < test.ml
+  {
+    "class": "return",
+    "value": [],
+    "notifications": []
+  }
+
+
+Reference:
+
+  $ $MERLIN single errors -filename test.ml -cmi-path sub < test.ml
+  {
+    "class": "return",
+    "value": [],
+    "notifications": []
+  }
+
+
+Well behaving versions of Merlin (>= 3.3.4) of should return the same answer as
+reference.
+
+We should check in the other direction too. Starting from a visible dep and
+hidding it.  Older versions of the typechecker (before the 4.08 revamp of Env)
+would accumulate dependencies and forget to flush the cache when a dependency
+disappeared.
+
+  $ $MERLIN server stop-server
+
+
+Visible:
+
+  $ $MERLIN server errors -filename test.ml -cmi-path sub < test.ml
+  {
+    "class": "return",
+    "value": [],
+    "notifications": []
+  }
+
+
+Reference:
+
+  $ $MERLIN single errors -filename test.ml -cmi-path sub < test.ml
+  {
+    "class": "return",
+    "value": [],
+    "notifications": []
+  }
+
+
+Hidden:
+
+  $ $MERLIN server errors -filename test.ml < test.ml
+  {
+    "class": "return",
+    "value": [
+      {
+        "start": {
+          "line": 1,
+          "col": 5
+        },
+        "end": {
+          "line": 1,
+          "col": 8
+        },
+        "type": "typer",
+        "sub": [],
+        "valid": true,
+        "message": "Unbound module Dep"
+      }
+    ],
+    "notifications": []
+  }
+
+
+Reference:
+
+  $ $MERLIN single errors -filename test.ml < test.ml
+  {
+    "class": "return",
+    "value": [
+      {
+        "start": {
+          "line": 1,
+          "col": 5
+        },
+        "end": {
+          "line": 1,
+          "col": 8
+        },
+        "type": "typer",
+        "sub": [],
+        "valid": true,
+        "message": "Unbound module Dep"
+      }
+    ],
+    "notifications": []
+  }
+
+
+Now some cleanup.
+
+  $ rm sub/dep.cm*