Refactor internal code action repr

Summary: The current encoding which uses a combination of constraints, polymorphic variants and GADTs doesn't seem to be buying us a lot and makes adding additional 'things' which will become code actions a little tricky. This diff backs out the more complex encoding in favor of plain old ADTs and a factored common type. Reviewed By: mheiber Differential Revision: D68443044 fbshipit-source-id: c44ec8cb9f077e6972ffd6fdad2de5725f48ef4f
facebook · Jan 23, 2025 · 68751c7 · 68751c7
1 parent ab5ce55
commit 68751c7
Show file tree

Hide file tree

Showing 18 changed files with 179 additions and 180 deletions.
diff --git a/.../hack/src/client/ide_service/code_actions_services/code_action_types/code_action_types.ml b/.../hack/src/client/ide_service/code_actions_services/code_action_types/code_action_types.ml
@@ -14,19 +14,20 @@ type edit = {
 
 type edits = edit list Relative_path.Map.t
 
-type +'kind t = {
+type edit_data = {
   title: string;
   edits: edits Lazy.t;
   selection: Pos.t option;
   trigger_inline_suggest: bool;
-  kind: [< `Refactor | `Quickfix ] as 'kind;
 }
 
-type refactor = [ `Refactor ] t
+type refactor = Refactor of edit_data [@@ocaml.unboxed]
 
-type quickfix = [ `Quickfix ] t
+type quickfix = Quickfix of edit_data [@@ocaml.unboxed]
 
-type any = [ `Refactor | `Quickfix ] t
+type t =
+  | Refactor_action of edit_data
+  | Quickfix_action of edit_data
 
 type find_refactor =
   entry:Provider_context.entry -> Pos.t -> Provider_context.t -> refactor list

diff --git a/...hack/src/client/ide_service/code_actions_services/code_action_types/code_action_types.mli b/...hack/src/client/ide_service/code_actions_services/code_action_types/code_action_types.mli
@@ -13,10 +13,8 @@ type edit = {
 
 type edits = edit list Relative_path.Map.t
 
-(** Internal representation for code actions in our language server,
- * distinct from Lsp.CodeAction and from quickfixex in Quickfixes.ml
- *)
-type +'kind t = {
+(** Data common to [Refactor] and [Quickfix] kind code actions  *)
+type edit_data = {
   title: string;  (** Title of the code action, as displayed by VSCode *)
   edits: edits Lazy.t;  (** Series of edits that will be applied *)
   selection: Pos.t option;
@@ -25,14 +23,18 @@ type +'kind t = {
   trigger_inline_suggest: bool;
       (** Whether or not to trigger the inline-suggest functionality in VSCode
           after inserting the edits and (optionally) changing the selection. *)
-  kind: [< `Refactor | `Quickfix ] as 'kind;
 }
 
-type refactor = [ `Refactor ] t
+type refactor = Refactor of edit_data [@@ocaml.unboxed]
 
-type quickfix = [ `Quickfix ] t
+type quickfix = Quickfix of edit_data [@@ocaml.unboxed]
 
-type any = [ `Refactor | `Quickfix ] t
+(** Internal representation for code actions in our language server,
+ * distinct from Lsp.CodeAction and from quickfixex in Quickfixes.ml
+ *)
+type t =
+  | Refactor_action of edit_data
+  | Quickfix_action of edit_data
 
 type find_refactor =
   entry:Provider_context.entry -> Pos.t -> Provider_context.t -> refactor list

diff --git a/hphp/hack/src/client/ide_service/code_actions_services/code_actions_quickfixes.ml b/hphp/hack/src/client/ide_service/code_actions_services/code_actions_quickfixes.ml
@@ -40,14 +40,14 @@ let convert_quickfix
          (text_edits classish_positions quickfix))
   in
 
-  Code_action_types.
-    {
-      title = Quickfix.get_title quickfix;
-      edits;
-      kind = `Quickfix;
-      selection = None;
-      trigger_inline_suggest = false;
-    }
+  Code_action_types.(
+    Quickfix
+      {
+        title = Quickfix.get_title quickfix;
+        edits;
+        selection = None;
+        trigger_inline_suggest = false;
+      })
 
 let quickfix_positions_for_error
     (classish_positions : Pos.t Classish_positions.t) (error : Errors.error) :

diff --git a/hphp/hack/src/client/ide_service/code_actions_services/code_actions_services.ml b/hphp/hack/src/client/ide_service/code_actions_services/code_actions_services.ml
@@ -70,8 +70,8 @@ let workspace_edit_of_code_action_edits
   in
   Lsp.WorkspaceEdit.{ changes }
 
-let to_action
-    Code_action_types.{ title; edits; kind; selection; trigger_inline_suggest }
+let edit_to_code_action
+    Code_action_types.{ title; edits; selection; trigger_inline_suggest } ~kind
     =
   let workspace_edit = Lazy.map edits ~f:workspace_edit_of_code_action_edits in
   let trigger_inline_suggest_command =
@@ -102,25 +102,27 @@ let to_action
       lazy
         (Lsp.CodeAction.BothEditThenCommand (Lazy.force workspace_edit, command))
   in
-  let lsp_kind =
-    match kind with
-    | `Refactor -> Lsp.CodeActionKind.refactor
-    | `Quickfix -> Lsp.CodeActionKind.quickfix
-  in
   Lsp.CodeAction.Action
     {
       Lsp.CodeAction.title;
-      kind = lsp_kind;
+      kind;
       diagnostics = [];
       action = Lsp.CodeAction.UnresolvedEdit action;
       isAI = None;
     }
 
+let to_action code_action =
+  match code_action with
+  | Code_action_types.Refactor_action edit ->
+    edit_to_code_action edit ~kind:Lsp.CodeActionKind.refactor
+  | Code_action_types.Quickfix_action edit ->
+    edit_to_code_action edit ~kind:Lsp.CodeActionKind.quickfix
+
 let find
     ~(ctx : Provider_context.t)
     ~error_filter
     ~(entry : Provider_context.entry)
-    ~(range : Lsp.range) : [ `Refactor | `Quickfix ] Code_action_types.t list =
+    ~(range : Lsp.range) =
   let pos =
     let source_text = Ast_provider.compute_source_text ~entry in
     let line_to_offset line =
@@ -130,18 +132,25 @@ let find
     Lsp_helpers.lsp_range_to_pos ~line_to_offset path range
   in
   let quickfixes = Code_actions_quickfixes.find ~entry pos ctx ~error_filter in
-  let quickfix_titles =
-    SSet.of_list @@ List.map quickfixes ~f:(fun q -> q.Code_action_types.title)
+  let (quickfix_titles, quickfixes) =
+    List.fold_map
+      quickfixes
+      ~init:SSet.empty
+      ~f:(fun acc (Code_action_types.Quickfix edit) ->
+        ( SSet.add edit.Code_action_types.title acc,
+          Code_action_types.Quickfix_action edit ))
   in
-  let refactors =
-    Code_actions_refactors.find ~entry pos ctx
-    (* Ensure no duplicates with quickfixes generated from Quickfixes_to_refactors_config. *)
-    |> List.filter ~f:(fun Code_action_types.{ title; _ } ->
-           not (SSet.mem title quickfix_titles))
-  in
-  let quickfixes :> Code_action_types.any list = quickfixes in
-  let refactors :> Code_action_types.any list = refactors in
-  quickfixes @ refactors
+  (* Accumulate refactors then reverse the entire list so that quickfixes come first *)
+  List.rev
+  @@ List.fold_left
+       (Code_actions_refactors.find ~entry pos ctx)
+       ~init:quickfixes
+       ~f:(fun acc Code_action_types.(Refactor edit) ->
+         (* Ensure no duplicates with quickfixes generated from Quickfixes_to_refactors_config. *)
+         if SSet.mem edit.Code_action_types.title quickfix_titles then
+           acc
+         else
+           Code_action_types.Refactor_action edit :: acc)
 
 let map_edit_and_or_command ~f :
     Lsp.CodeAction.resolved_marker Lsp.CodeAction.edit_and_or_command ->
@@ -192,6 +201,13 @@ the same title, so cannot resolve the code action.
       data = None;
     }
 
+let code_action_title t =
+  let open Code_action_types in
+  match t with
+  | Refactor_action { title; _ }
+  | Quickfix_action { title; _ } ->
+    title
+
 let resolve
     ~(ctx : Provider_context.t)
     ~error_filter
@@ -212,7 +228,7 @@ let resolve
   in
   find ~ctx ~entry ~range:(lsp_range_of_ide_range range) ~error_filter
   |> List.find ~f:(fun code_action ->
-         String.equal code_action.Code_action_types.title resolve_title)
+         String.equal (code_action_title code_action) resolve_title)
   (* When we can't find a matching code action, ContentModified is the right error
      per https://github.com/microsoft/language-server-protocol/issues/1738 *)
   |> Result.of_option ~error:content_modified

diff --git a/hphp/hack/src/client/ide_service/code_actions_services/quickfixes_from_refactors.ml b/hphp/hack/src/client/ide_service/code_actions_services/quickfixes_from_refactors.ml
@@ -40,27 +40,13 @@ let select_refactoring (e : Errors.error) :
   | Some fn -> Some fn
   | None -> select_refactoring_from_type_error e
 
+let quickfix_from_refactor Code_action_types.(Refactor edit_data) =
+  Code_action_types.(Quickfix edit_data)
+
 let find ctx entry (e : Errors.error) =
   select_refactoring e
   |> Option.map ~f:(fun find_refactors ->
          let e_pos = User_error.get_pos e in
          let refactors = find_refactors ~entry e_pos ctx in
-         List.map
-           refactors
-           ~f:
-             Code_action_types.(
-               fun {
-                     title;
-                     edits;
-                     kind = `Refactor;
-                     selection;
-                     trigger_inline_suggest;
-                   } ->
-                 {
-                   title;
-                   edits;
-                   kind = `Quickfix;
-                   selection;
-                   trigger_inline_suggest;
-                 }))
+         List.map refactors ~f:quickfix_from_refactor)
   |> Option.value ~default:[]
diff --git a/hphp/hack/src/client/ide_service/code_actions_services/refactors/add_doc_comment.ml b/hphp/hack/src/client/ide_service/code_actions_services/refactors/add_doc_comment.ml
@@ -25,14 +25,14 @@ let edits_of_candidate ~path ~line_to_offset candidate : Code_action_types.edits
 
 let to_refactor ~line_to_offset ~path candidate =
   let edits = lazy (edits_of_candidate ~path ~line_to_offset candidate) in
-  Code_action_types.
-    {
-      title = "Add doc comment";
-      edits;
-      kind = `Refactor;
-      selection = None;
-      trigger_inline_suggest = false;
-    }
+  Code_action_types.(
+    Refactor
+      {
+        title = "Add doc comment";
+        edits;
+        selection = None;
+        trigger_inline_suggest = false;
+      })
 
 let find_candidate pos source_text positioned_tree =
   let root = PositionedTree.root positioned_tree in

diff --git a/hphp/hack/src/client/ide_service/code_actions_services/refactors/add_local_type_hint.ml b/hphp/hack/src/client/ide_service/code_actions_services/refactors/add_local_type_hint.ml
@@ -88,14 +88,8 @@ let edits_of_candidate ~path { lhs_var; lhs_type_string; lhs_pos } :
 let to_refactor ~path candidate =
   let edits = lazy (edits_of_candidate ~path candidate) in
   let title = Printf.sprintf "Add local type hint for %s" candidate.lhs_var in
-  Code_action_types.
-    {
-      title;
-      edits;
-      kind = `Refactor;
-      selection = None;
-      trigger_inline_suggest = false;
-    }
+  Code_action_types.(
+    Refactor { title; edits; selection = None; trigger_inline_suggest = false })
 
 let has_typed_local_variables_enabled root_node =
   let open Full_fidelity_positioned_syntax in

diff --git a/hphp/hack/src/client/ide_service/code_actions_services/refactors/await_expression.ml b/hphp/hack/src/client/ide_service/code_actions_services/refactors/await_expression.ml
@@ -372,14 +372,14 @@ let refactor_of_candidate ctx entry candidate =
          entry.Provider_context.path
          (edits_of_candidate ctx entry candidate))
   in
-  Code_action_types.
-    {
-      title = "await expression";
-      edits;
-      kind = `Refactor;
-      selection = None;
-      trigger_inline_suggest = false;
-    }
+  Code_action_types.(
+    Refactor
+      {
+        title = "await expression";
+        edits;
+        selection = None;
+        trigger_inline_suggest = false;
+      })
 
 let find ~entry selection ctx =
   if Pos.length selection <> 0 then

diff --git a/...service/code_actions_services/refactors/extract_classish/extract_classish_to_refactors.ml b/...service/code_actions_services/refactors/extract_classish/extract_classish_to_refactors.ml
@@ -105,14 +105,14 @@ let edits_of_candidate source_text path candidate : Code_action_types.edits =
 
 let to_refactor source_text path candidate : Code_action_types.refactor =
   let edits = lazy (edits_of_candidate source_text path candidate) in
-  Code_action_types.
-    {
-      title = "Extract interface";
-      edits;
-      kind = `Refactor;
-      selection = None;
-      trigger_inline_suggest = false;
-    }
+  Code_action_types.(
+    Refactor
+      {
+        title = "Extract interface";
+        edits;
+        selection = None;
+        trigger_inline_suggest = false;
+      })
 
 let to_refactors (source_text : Full_fidelity_source_text.t) path candidate :
     Code_action_types.refactor list =

diff --git a/.../ide_service/code_actions_services/refactors/extract_method/extract_method_to_refactor.ml b/.../ide_service/code_actions_services/refactors/extract_method/extract_method_to_refactor.ml
@@ -290,11 +290,11 @@ let edits_of_candidate
 
 let of_candidate ~source_text ~path candidate =
   let edits = lazy (edits_of_candidate ~source_text ~path candidate) in
-  Code_action_types.
-    {
-      title = "Extract into method";
-      edits;
-      kind = `Refactor;
-      selection = None;
-      trigger_inline_suggest = false;
-    }
+  Code_action_types.(
+    Refactor
+      {
+        title = "Extract into method";
+        edits;
+        selection = None;
+        trigger_inline_suggest = false;
+      })
diff --git a/hphp/hack/src/client/ide_service/code_actions_services/refactors/extract_shape_type.ml b/hphp/hack/src/client/ide_service/code_actions_services/refactors/extract_shape_type.ml
@@ -151,14 +151,14 @@ let edits_of_candidate source_text ~path candidate : Code_action_types.edits =
 
 let to_refactor source_text ~path candidate =
   let edits = lazy (edits_of_candidate source_text ~path candidate) in
-  Code_action_types.
-    {
-      title = title_of_candidate candidate;
-      edits;
-      kind = `Refactor;
-      selection = None;
-      trigger_inline_suggest = false;
-    }
+  Code_action_types.(
+    Refactor
+      {
+        title = title_of_candidate candidate;
+        edits;
+        selection = None;
+        trigger_inline_suggest = false;
+      })
 
 let find ~entry selection ctx =
   let source_text = Ast_provider.compute_source_text ~entry in

diff --git a/hphp/hack/src/client/ide_service/code_actions_services/refactors/extract_variable.ml b/hphp/hack/src/client/ide_service/code_actions_services/refactors/extract_variable.ml
@@ -206,14 +206,14 @@ let refactor_of_candidate ctx entry path candidate =
          path
          (edits_of_candidate ctx entry candidate))
   in
-  Code_action_types.
-    {
-      title = "Extract into variable";
-      edits;
-      kind = `Refactor;
-      selection = None;
-      trigger_inline_suggest = false;
-    }
+  Code_action_types.(
+    Refactor
+      {
+        title = "Extract into variable";
+        edits;
+        selection = None;
+        trigger_inline_suggest = false;
+      })
 
 let find ~entry selection ctx =
   let path = entry.Provider_context.path in

diff --git a/hphp/hack/src/client/ide_service/code_actions_services/refactors/flip_around_comma.ml b/hphp/hack/src/client/ide_service/code_actions_services/refactors/flip_around_comma.ml
@@ -199,14 +199,14 @@ let edit_of_candidate
 
 let refactor_of_candidate ~path ~source_text candidate =
   let edits = lazy (edit_of_candidate ~path ~source_text candidate) in
-  Code_action_types.
-    {
-      title = "Flip around comma";
-      edits;
-      kind = `Refactor;
-      selection = None;
-      trigger_inline_suggest = false;
-    }
+  Code_action_types.(
+    Refactor
+      {
+        title = "Flip around comma";
+        edits;
+        selection = None;
+        trigger_inline_suggest = false;
+      })
 
 let find ~entry pos ctx =
   if Pos.length pos = 0 then