diff --git a/CHANGES.md b/CHANGES.md index 9c25130cb..451412c43 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -8,6 +8,8 @@ ## Features - Display text of references in doc strings (#1166) +- Add mark/remove unused actions for open, types, for loop indexes, modules, + match cases, rec, and constructors (#1141) # 1.16.2 diff --git a/ocaml-lsp-server/src/code_actions/action_mark_remove_unused.ml b/ocaml-lsp-server/src/code_actions/action_mark_remove_unused.ml index aa9120206..03d23547a 100644 --- a/ocaml-lsp-server/src/code_actions/action_mark_remove_unused.ml +++ b/ocaml-lsp-server/src/code_actions/action_mark_remove_unused.ml @@ -1,6 +1,48 @@ open Import open Option.O +let diagnostic_regex, diagnostic_regex_marks = + let msgs = + ( Re.mark + (Re.alt + [ Re.str "Error (warning 26)" + ; Re.str "Error (warning 27)" + ; Re.str "unused value" + ]) + , `Value ) + :: ([ ("unused open", `Open) + ; ("unused open!", `Open_bang) + ; ("unused type", `Type) + ; ("unused constructor", `Constructor) + ; ("unused extension constructor", `Extension) + ; ("this match case is unused", `Case) + ; ("unused for-loop index", `For_loop_index) + ; ("unused rec flag", `Rec) + ; ("unused module", `Module) + ] + |> List.map ~f:(fun (msg, kind) -> (Re.mark (Re.str msg), kind))) + in + let regex = + Re.compile + (Re.seq [ Re.bol; Re.alt (List.map ~f:(fun ((_, r), _) -> r) msgs) ]) + in + let marks = List.map ~f:(fun ((m, _), k) -> (m, k)) msgs in + (regex, marks) + +let find_unused_diagnostic pos ds = + let open Option.O in + List.filter ds ~f:(fun (d : Diagnostic.t) -> + match Position.compare_inclusion pos d.range with + | `Outside _ -> false + | `Inside -> true) + |> List.find_map ~f:(fun (d : Diagnostic.t) -> + let* group = Re.exec_opt diagnostic_regex d.message in + let+ kind = + List.find_map diagnostic_regex_marks ~f:(fun (m, k) -> + if Re.Mark.test group m then Some k else None) + in + (kind, d)) + (* Return contexts enclosing `pos` in order from most specific to most general. *) let enclosing_pos pipeline pos = @@ -14,7 +56,6 @@ let enclosing_pos pipeline pos = (* `name` is an unused binding. `contexts` is a list of Mbrowse.t enclosing an unused definition of `name`, in order from most general to most specific. - Returns an edit that silences the 'unused value' warning. *) let rec mark_value_unused_edit name contexts = match contexts with @@ -64,11 +105,11 @@ let rec mark_value_unused_edit name contexts = | _ -> None let code_action_mark_value_unused pipeline doc (diagnostic : Diagnostic.t) = + let open Option.O in let* var_name = Document.substring doc diagnostic.range in let pos = diagnostic.range.start in let+ text_edit = - enclosing_pos pipeline pos - |> List.rev_map ~f:(fun (_, x) -> x) + enclosing_pos pipeline pos |> List.rev_map ~f:snd |> mark_value_unused_edit var_name in let edit = Document.edit doc [ text_edit ] in @@ -93,22 +134,23 @@ let enclosing_value_binding_range name = ; _ } ] - , body ) - ; exp_loc + , { exp_loc = { loc_start = let_end; _ }; _ } ) + ; exp_loc = { loc_start = let_start; _ } ; _ } when name = name' -> - let* start = Position.of_lexical_position exp_loc.loc_start in - let+ end_ = Position.of_lexical_position body.exp_loc.loc_start in + let* start = Position.of_lexical_position let_start in + let+ end_ = Position.of_lexical_position let_end in Range.create ~start ~end_ | _ -> None) (* Create a code action that removes [range] and refers to [diagnostic]. *) -let code_action_remove_range doc (diagnostic : Diagnostic.t) range = +let code_action_remove_range ?(title = "Remove unused") doc + (diagnostic : Diagnostic.t) range = let edit = Document.edit doc [ { range; newText = "" } ] in CodeAction.create ~diagnostics:[ diagnostic ] - ~title:"Remove unused" + ~title ~kind:CodeActionKind.QuickFix ~edit ~isPreferred:false @@ -121,25 +163,222 @@ let code_action_remove_value pipeline doc pos (diagnostic : Diagnostic.t) = |> enclosing_value_binding_range var_name |> Option.map ~f:(fun range -> code_action_remove_range doc diagnostic range) -let find_unused_diagnostic pos (ds : Diagnostic.t list) = - List.find ds ~f:(fun d -> - let in_range = - match Position.compare_inclusion pos d.range with - | `Outside _ -> false - | `Inside -> true - in - in_range && Diagnostic_util.is_unused_var_warning d.message) +(** [create_mark_action ~title doc pos d] creates a code action that resolves + the diagnostic [d] by inserting an underscore at [pos] in [doc]. *) +let create_mark_action ~title doc pos d = + let edit = + Document.edit + doc + [ { range = Range.create ~start:pos ~end_:pos; newText = "_" } ] + in + CodeAction.create + ~diagnostics:[ d ] + ~title + ~kind:CodeActionKind.QuickFix + ~edit + ~isPreferred:true + () + +let action_mark_type pipeline doc pos (d : Diagnostic.t) = + let open Option.O in + let m_name_loc_start = + enclosing_pos pipeline pos + |> List.find_map ~f:(fun (_, node) -> + match node with + | Browse_raw.Type_declaration + { typ_name = { loc = { loc_start; _ }; _ }; _ } -> Some loc_start + | _ -> None) + in + let* name_loc_start = m_name_loc_start in + let+ start = Position.of_lexical_position name_loc_start in + create_mark_action ~title:"Mark type as unused" doc start d -let code_action_mark pipeline doc (params : CodeActionParams.t) = - let pos = params.range.start in - let* d = find_unused_diagnostic pos params.context.diagnostics in - code_action_mark_value_unused pipeline doc d +let contains loc pos = + match Position.compare_inclusion pos (Range.of_loc loc) with + | `Outside _ -> false + | `Inside -> true + +let action_mark_for_loop_index pipeline doc pos (d : Diagnostic.t) = + let open Option.O in + let module I = Ocaml_parsing.Ast_iterator in + let exception Found of Warnings.loc in + let iterator = + let expr iter (e : Parsetree.expression) = + if contains e.pexp_loc pos then + match e.pexp_desc with + | Pexp_for ({ ppat_loc; _ }, _, _, _, _) when contains ppat_loc pos -> + raise_notrace (Found ppat_loc) + | _ -> I.default_iterator.expr iter e + in + let structure_item iter (item : Parsetree.structure_item) = + if contains item.pstr_loc pos then + I.default_iterator.structure_item iter item + in + { I.default_iterator with expr; structure_item } + in + let m_index_loc = + match Mpipeline.reader_parsetree pipeline with + | `Implementation parsetree -> ( + try + iterator.structure iterator parsetree; + None + with Found task -> Some task) + | `Interface _ -> None + in + let* (index_loc : Warnings.loc) = m_index_loc in + let+ start = Position.of_lexical_position index_loc.loc_start in + create_mark_action ~title:"Mark for-loop index as unused" doc start d -let code_action_remove pipeline doc (params : CodeActionParams.t) = - let pos = params.range.start in - let* d = find_unused_diagnostic pos params.context.diagnostics in - code_action_remove_value pipeline doc pos d +let action_mark_open doc (d : Diagnostic.t) = + let edit = + let pos = { d.range.start with character = d.range.start.character + 4 } in + let range = Range.create ~start:pos ~end_:pos in + Document.edit doc [ { range; newText = "!" } ] + in + CodeAction.create + ~diagnostics:[ d ] + ~title:"Replace with open!" + ~kind:CodeActionKind.QuickFix + ~edit + ~isPreferred:true + () -let mark = Code_action.batchable QuickFix code_action_mark +let rec_regex = + Re.compile + (Re.seq + [ Re.bos + ; Re.rep Re.any + ; Re.group (Re.str "rec") + ; Re.rep Re.space + ; Re.stop + ]) -let remove = Code_action.batchable QuickFix code_action_remove +let find_preceding doc pos regex = + let open Option.O in + let src = Document.source doc in + let (`Offset end_) = Msource.get_offset src @@ Position.logical pos in + let* groups = Re.exec_opt ~len:end_ regex (Msource.text src) in + let match_start, match_end = Re.Group.offset groups 1 in + let filename = Uri.to_path (Document.uri doc) in + let* start = + Msource.get_lexing_pos ~filename src (`Offset match_start) + |> Position.of_lexical_position + in + let+ end_ = + Msource.get_lexing_pos ~filename src (`Offset match_end) + |> Position.of_lexical_position + in + Range.create ~start ~end_ + +let action_remove_rec doc (d : Diagnostic.t) = + let open Option.O in + let+ rec_range = find_preceding doc d.range.start rec_regex in + code_action_remove_range ~title:"Remove unused rec" doc d rec_range + +let bar_regex = + Re.compile + (Re.seq + [ Re.bos + ; Re.rep Re.any + ; Re.group (Re.str "|") + ; Re.rep Re.space + ; Re.stop + ]) + +let action_remove_case pipeline doc (d : Diagnostic.t) = + let open Option.O in + let case_range = + enclosing_pos pipeline d.range.start + |> List.find_map ~f:(fun (_, node) -> + match node with + | Browse_raw.Case + { c_lhs = { pat_loc = { loc_start; _ }; _ } + ; c_rhs = { exp_loc = { loc_end; _ }; _ } + ; _ + } -> Some (loc_start, loc_end) + | _ -> None) + in + let* case_start, case_end = case_range in + let* start = Position.of_lexical_position case_start in + let* end_ = Position.of_lexical_position case_end in + let+ preceding_bar = find_preceding doc start bar_regex in + let edit = + Document.edit + doc + [ { range = Range.create ~start:preceding_bar.start ~end_; newText = "" } + ] + in + CodeAction.create + ~diagnostics:[ d ] + ~title:"Remove unused case" + ~kind:CodeActionKind.QuickFix + ~edit + ~isPreferred:true + () + +let action_remove_constructor pipeline doc (d : Diagnostic.t) = + let open Option.O in + let case_range = + enclosing_pos pipeline d.range.start + |> List.find_map ~f:(fun (_, node) -> + match node with + | Browse_raw.Constructor_declaration + { cd_loc = { loc_start; loc_end; _ }; _ } -> + Some (loc_start, loc_end) + | _ -> None) + in + let* case_start, case_end = case_range in + let* start = Position.of_lexical_position case_start in + let+ end_ = Position.of_lexical_position case_end in + let edit = + Document.edit doc [ { range = Range.create ~start ~end_; newText = "" } ] + in + CodeAction.create + ~diagnostics:[ d ] + ~title:"Remove unused constructor" + ~kind:CodeActionKind.QuickFix + ~edit + ~isPreferred:true + () + +let action_remove_simple kind doc (d : Diagnostic.t) = + code_action_remove_range ~title:("Remove unused " ^ kind) doc d d.range + +let mark = + let run pipeline doc (params : CodeActionParams.t) = + let open Option.O in + let pos = params.range.start in + let* diagnostic = find_unused_diagnostic pos params.context.diagnostics in + match diagnostic with + | `Value, d -> code_action_mark_value_unused pipeline doc d + | `Open, d -> Some (action_mark_open doc d) + | `Type, d -> action_mark_type pipeline doc pos d + | `For_loop_index, d -> action_mark_for_loop_index pipeline doc pos d + | (`Open_bang | `Constructor | `Extension | `Case | `Rec | `Module), _ -> + (* these diagnostics don't have a reasonable "mark as unused" action *) + None + in + Code_action.batchable QuickFix run + +let remove = + let run pipeline doc (params : CodeActionParams.t) = + let open Option.O in + let pos = params.range.start in + let* diagnostic = find_unused_diagnostic pos params.context.diagnostics in + match diagnostic with + | `Value, d -> code_action_remove_value pipeline doc pos d + | `Open, d -> Some (action_remove_simple "open" doc d) + | `Open_bang, d -> Some (action_remove_simple "open!" doc d) + | `Type, d -> Some (action_remove_simple "type" doc d) + | `Module, d -> Some (action_remove_simple "module" doc d) + | `Case, d -> action_remove_case pipeline doc d + | `Rec, d -> action_remove_rec doc d + | `Constructor, d -> action_remove_constructor pipeline doc d + | `Extension, _ -> + (* todo *) + None + | `For_loop_index, _ -> + (* these diagnostics don't have a reasonable "remove unused" action *) + None + in + Code_action.batchable QuickFix run diff --git a/ocaml-lsp-server/test/e2e-new/action_extract.ml b/ocaml-lsp-server/test/e2e-new/action_extract.ml index ff760153b..9360b72e7 100644 --- a/ocaml-lsp-server/test/e2e-new/action_extract.ml +++ b/ocaml-lsp-server/test/e2e-new/action_extract.ml @@ -1,8 +1,7 @@ -let extract_local_test src = - Code_actions.code_action_test ~title:"Extract local" ~source:src +let extract_local_test = Code_actions.code_action_test ~title:"Extract local" -let extract_function_test src = - Code_actions.code_action_test ~title:"Extract function" ~source:src +let extract_function_test = + Code_actions.code_action_test ~title:"Extract function" let%expect_test "extract local constant" = extract_local_test {| diff --git a/ocaml-lsp-server/test/e2e-new/action_inline.ml b/ocaml-lsp-server/test/e2e-new/action_inline.ml index 16120cef2..5b4350761 100644 --- a/ocaml-lsp-server/test/e2e-new/action_inline.ml +++ b/ocaml-lsp-server/test/e2e-new/action_inline.ml @@ -1,5 +1,4 @@ -let inline_test src = - Code_actions.code_action_test ~title:"Inline into uses" ~source:src +let inline_test = Code_actions.code_action_test ~title:"Inline into uses" let%expect_test "" = inline_test {| diff --git a/ocaml-lsp-server/test/e2e-new/action_mark_remove.ml b/ocaml-lsp-server/test/e2e-new/action_mark_remove.ml new file mode 100644 index 000000000..41f322e7c --- /dev/null +++ b/ocaml-lsp-server/test/e2e-new/action_mark_remove.ml @@ -0,0 +1,176 @@ +open Test.Import + +let run_test ~title ~message source = + let src, range = Code_actions.parse_selection source in + Option.iter + (Code_actions.apply_code_action + ~diagnostics:[ Diagnostic.create ~message ~range () ] + title + src + range) + ~f:print_string + +let mark_test = function + | `Value -> run_test ~title:"Mark as unused" ~message:"unused value" + | `Open -> run_test ~title:"Replace with open!" ~message:"unused open" + | `For_loop_index -> + run_test + ~title:"Mark for-loop index as unused" + ~message:"unused for-loop index" + +let remove_test = function + | `Value -> run_test ~title:"Remove unused" ~message:"unused value" + | `Open -> run_test ~title:"Remove unused open" ~message:"unused open" + | `Open_bang -> run_test ~title:"Remove unused open!" ~message:"unused open!" + | `Type -> run_test ~title:"Remove unused type" ~message:"unused type" + | `Module -> run_test ~title:"Remove unused module" ~message:"unused module" + | `Case -> + run_test ~title:"Remove unused case" ~message:"this match case is unused" + | `Rec -> run_test ~title:"Remove unused rec" ~message:"unused rec flag" + | `Constructor -> + run_test ~title:"Remove unused constructor" ~message:"unused constructor" + +let%expect_test "mark value in let" = + mark_test `Value {| +let f = + let $x$ = 1 in + 0 +|}; + [%expect {| + let f = + let _x = 1 in + 0 |}] + +(* todo *) +let%expect_test "mark value in top level let" = + mark_test `Value {| +let $f$ = + let x = 1 in + 0 +|} + +let%expect_test "mark value in match" = + mark_test `Value {| +let f = function + | $x$ -> 0 +|}; + [%expect {| + let f = function + | _x -> 0 |}] + +let%expect_test "remove value in let" = + remove_test `Value {| +let f = + let $x$ = 1 in + 0 +|}; + [%expect {| + let f = + 0 |}] + +(* todo *) +let%expect_test "remove value in top level let" = + remove_test `Value {| +let $f$ = + let x = 1 in + 0 +|} + +let%expect_test "mark open" = + mark_test `Open {| +$open M$ +|}; + [%expect {| open! M |}] + +let%expect_test "mark for loop index" = + mark_test `For_loop_index {| +let () = + for $i$ = 0 to 10 do + () + done +|}; + [%expect {| + let () = + for _i = 0 to 10 do + () + done |}] + +let%expect_test "remove open" = + remove_test `Open {| +open A +$open B$ +|}; + [%expect {| open A |}] + +let%expect_test "remove open!" = remove_test `Open_bang {| +open A +$open! B$ +|} + +let%expect_test "remove type" = + remove_test `Type {| +$type t = int$ +type s = bool +|}; + [%expect {| type s = bool |}] + +let%expect_test "remove module" = + remove_test `Module {| +$module A = struct end$ +module B = struct end +|}; + [%expect {| module B = struct end |}] + +let%expect_test "remove case" = + remove_test `Case {| +let f = function + | 0 -> 0 + | $0 -> 1$ +|}; + [%expect {| + let f = function + | 0 -> 0 |}] + +let%expect_test "remove rec flag" = + remove_test `Rec {| +let rec $f$ = 0 +|}; + [%expect {| let f = 0 |}] + +let%expect_test "remove constructor" = + remove_test `Constructor {| +type t = A $| B$ +|}; + [%expect {| type t = A |}] + +let%expect_test "remove constructor" = + remove_test `Constructor {| +type t = + | A + $| B$ +|}; + [%expect {| + type t = + | A |}] + +let%expect_test "remove constructor" = + remove_test `Constructor {| +type t = + $| A$ + | B +|}; + [%expect {| + type t = + + | B |}] + +let%expect_test "remove constructor" = + remove_test `Constructor {| +type t = + $A$ + | B +|}; + [%expect {| + type t = + + | B |}] diff --git a/ocaml-lsp-server/test/e2e-new/code_actions.ml b/ocaml-lsp-server/test/e2e-new/code_actions.ml index 90b6f599f..41789e39e 100644 --- a/ocaml-lsp-server/test/e2e-new/code_actions.ml +++ b/ocaml-lsp-server/test/e2e-new/code_actions.ml @@ -9,17 +9,17 @@ let openDocument ~client ~uri ~source = (TextDocumentDidOpen (DidOpenTextDocumentParams.create ~textDocument)) let iter_code_actions ?(prep = fun _ -> Fiber.return ()) ?(path = "foo.ml") - ~source range k = - let diagnostics = Fiber.Ivar.create () in + ?(diagnostics = []) ~source range k = + let got_diagnostics = Fiber.Ivar.create () in let handler = Client.Handler.make ~on_notification: (fun _ -> function | PublishDiagnostics _ -> ( - let* diag = Fiber.Ivar.peek diagnostics in + let* diag = Fiber.Ivar.peek got_diagnostics in match diag with | Some _ -> Fiber.return () - | None -> Fiber.Ivar.fill diagnostics ()) + | None -> Fiber.Ivar.fill got_diagnostics ()) | _ -> Fiber.return ()) () in @@ -42,7 +42,7 @@ let iter_code_actions ?(prep = fun _ -> Fiber.return ()) ?(path = "foo.ml") let* () = prep client in let* () = openDocument ~client ~uri ~source in let+ resp = - let context = CodeActionContext.create ~diagnostics:[] () in + let context = CodeActionContext.create ~diagnostics () in let request = let textDocument = TextDocumentIdentifier.create ~uri in CodeActionParams.create ~textDocument ~range ~context () @@ -52,7 +52,7 @@ let iter_code_actions ?(prep = fun _ -> Fiber.return ()) ?(path = "foo.ml") k resp in Fiber.fork_and_join_unit run_client (fun () -> - run >>> Fiber.Ivar.read diagnostics >>> Client.stop client) + run >>> Fiber.Ivar.read got_diagnostics >>> Client.stop client) let print_code_actions ?(prep = fun _ -> Fiber.return ()) ?(path = "foo.ml") ?(filter = fun _ -> true) source range = @@ -667,11 +667,12 @@ let apply_edits src edits = in apply src edits -let apply_code_action title source range = +let apply_code_action ?diagnostics title source range = let open Option.O in (* collect code action results *) let code_actions = ref None in - iter_code_actions ~source range (fun ca -> code_actions := Some ca); + iter_code_actions ?diagnostics ~source range (fun ca -> + code_actions := Some ca); let* m_code_actions = !code_actions in let* code_actions = m_code_actions in @@ -691,6 +692,6 @@ let apply_code_action title source range = | `CreateFile _ | `DeleteFile _ | `RenameFile _ -> []) |> apply_edits source -let code_action_test ~title ~source = +let code_action_test ~title source = let src, range = parse_selection source in Option.iter (apply_code_action title src range) ~f:print_string diff --git a/ocaml-lsp-server/test/e2e-new/code_actions.mli b/ocaml-lsp-server/test/e2e-new/code_actions.mli index c3e2b5dca..7e1c0a4ba 100644 --- a/ocaml-lsp-server/test/e2e-new/code_actions.mli +++ b/ocaml-lsp-server/test/e2e-new/code_actions.mli @@ -3,9 +3,17 @@ open Test.Import val iter_code_actions : ?prep:(unit Test.Import.Client.t -> unit Fiber.t) -> ?path:string + -> ?diagnostics:Diagnostic.t list -> source:string -> Range.t -> (CodeActionResult.t -> unit) -> unit -val code_action_test : title:string -> source:string -> unit +val parse_selection : string -> string * Range.t + +val apply_code_action : + ?diagnostics:Diagnostic.t list -> string -> string -> Range.t -> string option + +(** [code_action_test title source] runs the code action with title [title] and + prints the resulting source. *) +val code_action_test : title:string -> string -> unit diff --git a/ocaml-lsp-server/test/e2e-new/dune b/ocaml-lsp-server/test/e2e-new/dune index 1407334a3..8dcc1dd78 100644 --- a/ocaml-lsp-server/test/e2e-new/dune +++ b/ocaml-lsp-server/test/e2e-new/dune @@ -41,6 +41,7 @@ ((pps ppx_expect) action_extract action_inline + action_mark_remove code_actions doc_to_md document_flow