clean up reported location for let-bound functions

report the entire value binding when not in the lsp-compat regime

also move all the lsp-compat tests to a separate file to group them
together

Signed-off-by: David Vulakh <[email protected]>

Author: dvulakh

src/analysis/stack_or_heap_enclosing.ml

@@ -33,8 +33,15 @@ let from_nodes ~lsp_compat ~pos ~path =
     | ( Pattern { pat_desc = Tpat_var _; _ },
         Some
           (Value_binding
-            { vb_expr = { exp_desc = Texp_function { alloc_mode; _ }; _ }; _ })
-      ) -> ret (Alloc_mode alloc_mode.mode)
+            { vb_expr = { exp_desc = Texp_function { alloc_mode; _ }; _ };
+              vb_loc;
+              _
+            }) ) ->
+      (* The location that most sensibly corresponds to the "allocation" is the entire
+         value binding. However, the LSP hover at this point will describe just the
+         pattern, so we don't override the location in the [lsp_compat] regime. *)
+      let loc = if lsp_compat then None else Some vb_loc in
+      ret ?loc (Alloc_mode alloc_mode.mode)
     | Expression { exp_desc; _ }, _ -> (
       match exp_desc with
       | Texp_function { alloc_mode; body; _ } -> (
@@ -73,6 +80,10 @@ let from_nodes ~lsp_compat ~pos ~path =
           ({ loc; txt = _lident }, { cstr_repr; _ }, args, maybe_alloc_mode)
         -> (
         let loc =
+          (* The location of the "allocation" here is the entire expression, but the LSP
+             hover for a constructor reports information just for the constructor (not the
+             entire [Texp_construct] expression), so we override the location in the
+             [lsp_compat] regime. *)
           if lsp_compat && cursor_is_inside loc then Some loc else None
         in
         match maybe_alloc_mode with

tests/test-dirs/stack-or-heap.t/closures.ml

@@ -57,35 +57,3 @@ let f = function
   | Some _ -> 1
          (* ^ *)
 
-(* Pattern of a [let]-bound function (we treat this case specially to improve LSP
-   compatibility) *)
-
-let f g x y =
- (* ^ *)
-  let z = x + y in
-  exclave_ Some (g z)
-and h g x y =
- (* ^ *)
-  let z = x + y in
-  exclave_ Some (g z)
-;;
-
-let ignore (local_ _) = ()
-
-let () =
-  let f g x y =
-   (* ^ *)
-    let z = x + y in
-    exclave_ Some (g z)
-  and h g x y =
-   (* ^ *)
-    let z = x + y in
-    exclave_ Some (g z)
-  in
-  ignore f;
-  ignore h
-
-(* Ensure other [let]-bound patterns aren't treated this way *)
-
-let x = Some 5
- (* ^ *)

tests/test-dirs/stack-or-heap.t/constructors.ml

@@ -18,27 +18,6 @@ let f g x y =
   y
 ;;
 
-(* Cursor on the constructor itself (we treat this case specially to improve LSP
-   compatibility) *)
-
-let f g x y =
-  let z = x + y in
-  Some (g z)
- (* ^ *)
-;;
-
-let f g x y =
-  let z = x + y in
-  exclave_ Some (g z)
-          (* ^ *)
-;;
-
-let f g x y =
-  let z = Some (g x) in
-         (* ^ *)
-  y
-;;
-
 (* Constructors with no arguments *)
 
 let f g x y =

tests/test-dirs/stack-or-heap.t/lsp_compat.ml

@@ -0,0 +1,53 @@
+(* Cursor on the constructor itself (we treat this case specially to improve LSP
+   compatibility) *)
+
+let f g x y =
+  let z = x + y in
+  Some (g z)
+ (* ^ *)
+;;
+
+let f g x y =
+  let z = x + y in
+  exclave_ Some (g z)
+          (* ^ *)
+;;
+
+let f g x y =
+  let z = Some (g x) in
+         (* ^ *)
+  y
+;;
+
+(* Pattern of a [let]-bound function (we treat this case specially to improve LSP
+   compatibility) *)
+
+let f g x y =
+ (* ^ *)
+  let z = x + y in
+  exclave_ Some (g z)
+and h g x y =
+ (* ^ *)
+  let z = x + y in
+  exclave_ Some (g z)
+;;
+
+let ignore (local_ _) = ()
+
+let () =
+  let f g x y =
+   (* ^ *)
+    let z = x + y in
+    exclave_ Some (g z)
+  and h g x y =
+   (* ^ *)
+    let z = x + y in
+    exclave_ Some (g z)
+  in
+  ignore f;
+  ignore h
+
+(* Ensure other [let]-bound patterns aren't treated this way *)
+
+let x = Some 5
+ (* ^ *)