From 37ffbefbb2f5f381238ed1a22937aa2042cf21d0 Mon Sep 17 00:00:00 2001 From: Manos Emmanouilidis Date: Fri, 25 Mar 2022 14:56:41 +0200 Subject: [PATCH 1/5] Suggest an appropriate module name when auto-completing the 'defmodule' snippet --- .../language_server/providers/completion.ex | 55 ++++++++++- .../lib/language_server/server.ex | 3 +- .../test/providers/completion_test.exs | 97 +++++++++++++++++++ 3 files changed, 153 insertions(+), 2 deletions(-) diff --git a/apps/language_server/lib/language_server/providers/completion.ex b/apps/language_server/lib/language_server/providers/completion.ex index b50f3c1d6..3955b3e29 100644 --- a/apps/language_server/lib/language_server/providers/completion.ex +++ b/apps/language_server/lib/language_server/providers/completion.ex @@ -565,7 +565,9 @@ defmodule ElixirLS.LanguageServer.Providers.Completion do completion end - if snippet = snippet_for({origin, name}, context) do + uri = Keyword.get(options, :uri) + + if snippet = snippet_for({origin, name}, Map.put(context, :uri, uri)) do %{completion | insert_text: snippet, kind: :snippet, label: name} else completion @@ -576,6 +578,12 @@ defmodule ElixirLS.LanguageServer.Providers.Completion do nil end + defp snippet_for({"Kernel", "defmodule"}, %{uri: uri}) when is_binary(uri) do + # In a mix project the uri can be something like "/some/code/path/project/lib/project/sub_path/myfile.ex" + # so we'll try to guess the appropriate module name from the path + "defmodule #{suggest_module_name(uri)}$1 do\n\t$0\nend" + end + defp snippet_for(key, %{pipe_before?: true}) do # Get pipe-friendly version of snippet if available, otherwise fallback to standard Map.get(@pipe_func_snippets, key) || Map.get(@func_snippets, key) @@ -593,6 +601,51 @@ defmodule ElixirLS.LanguageServer.Providers.Completion do end end + def suggest_module_name(file_path) when is_binary(file_path) do + file_path + |> Path.split() + |> Enum.reverse() + |> do_suggest_module_name() + end + + defp do_suggest_module_name([]), do: nil + + defp do_suggest_module_name([filename | reversed_path]) do + filename + |> String.split(".") + |> case do + [file, "ex"] -> + do_suggest_module_name(reversed_path, [file], topmost_parent: "lib") + + [file, "exs"] -> + if String.ends_with?(file, "_test") do + do_suggest_module_name(reversed_path, [file], topmost_parent: "test") + else + nil + end + + _otherwise -> + nil + end + end + + defp do_suggest_module_name([dir | _rest], module_name_acc, topmost_parent: topmost) + when dir == topmost do + module_name_acc + |> Enum.map(&Macro.camelize/1) + |> Enum.join(".") + end + + defp do_suggest_module_name([dir_name | rest], module_name_acc, opts) do + do_suggest_module_name(rest, [dir_name | module_name_acc], opts) + end + + defp do_suggest_module_name([], _module_name_acc, _opts) do + # we went all the way up without ever encountering a 'lib' or a 'test' folder + # so we ignore the accumulated module name because it's probably wrong/useless + nil + end + def function_snippet(name, args, arity, opts) do snippets_supported? = Keyword.get(opts, :snippets_supported, false) trigger_signature? = Keyword.get(opts, :trigger_signature?, false) diff --git a/apps/language_server/lib/language_server/server.ex b/apps/language_server/lib/language_server/server.ex index 0f2d4c09b..e239da09a 100644 --- a/apps/language_server/lib/language_server/server.ex +++ b/apps/language_server/lib/language_server/server.ex @@ -685,7 +685,8 @@ defmodule ElixirLS.LanguageServer.Server do tags_supported: tags_supported, signature_help_supported: signature_help_supported, locals_without_parens: locals_without_parens, - signature_after_complete: signature_after_complete + signature_after_complete: signature_after_complete, + uri: uri ) end diff --git a/apps/language_server/test/providers/completion_test.exs b/apps/language_server/test/providers/completion_test.exs index 3d55f5340..a0d889a2b 100644 --- a/apps/language_server/test/providers/completion_test.exs +++ b/apps/language_server/test/providers/completion_test.exs @@ -908,6 +908,62 @@ defmodule ElixirLS.LanguageServer.Providers.CompletionTest do """ } end + + test "will suggest defmodule with module_name snippet when file path matches **/lib/**/*.ex" do + text = """ + defmod + # ^ + """ + + {line, char} = {0, 6} + + TestUtils.assert_has_cursor_char(text, line, char) + + assert {:ok, %{"items" => [first | _] = _items}} = + Completion.completion( + text, + line, + char, + @supports + |> Keyword.put( + :uri, + "file://some/path/my_project/lib/my_project/sub_folder/my_file.ex" + ) + ) + + assert %{ + "label" => "defmodule", + "insertText" => "defmodule MyProject.SubFolder.MyFile$1 do\n\t$0\nend" + } = first + end + + test "will suggest defmodule without module_name snippet when file path does not match expected patterns" do + text = """ + defmod + # ^ + """ + + {line, char} = {0, 6} + + TestUtils.assert_has_cursor_char(text, line, char) + + assert {:ok, %{"items" => [first | _] = _items}} = + Completion.completion( + text, + line, + char, + @supports + |> Keyword.put( + :uri, + "file://some/path/my_project/lib/my_project/sub_folder/my_file.heex" + ) + ) + + assert %{ + "label" => "defmodule", + "insertText" => "defmodule $1 do\n\t$0\nend" + } = first + end end describe "generic suggestions" do @@ -1053,4 +1109,45 @@ defmodule ElixirLS.LanguageServer.Providers.CompletionTest do assert insert_text =~ "if do\n\t" end end + + describe "suggest_module_name/1" do + import Completion, only: [suggest_module_name: 1] + + test "returns nil if current file uri is empty" do + assert nil == suggest_module_name("") + end + + test "returns nil if current file is not an .ex file" do + assert nil == suggest_module_name("some/path/lib/dir/file.heex") + end + + test "returns nil if current file is an .ex file but no lib folder exists in path" do + assert nil == suggest_module_name("some/path/not_lib/dir/file.ex") + end + + test "returns nil if current file is an *_test.exs file but not test folder exists in path" do + assert nil == suggest_module_name("some/path/not_test/dir/file_test.exs") + end + + test "returns an appropriate suggestion if file directly under lib" do + assert "MyProject" == suggest_module_name("some/path/my_project/lib/my_project.ex") + end + + test "returns an appropriate suggestion if file arbitrarily nested under lib/" do + assert "MyProject.Foo.Bar.Baz.MyFile" = + suggest_module_name("some/path/my_project/lib/my_project/foo/bar/baz/my_file.ex") + end + + test "returns an appropriate suggestion if file directly under test/" do + assert "MyProjectTest" == + suggest_module_name("some/path/my_project/test/my_project_test.exs") + end + + test "returns an appropriate suggestion if file arbitrarily nested under test" do + assert "MyProject.Foo.Bar.Baz.MyFileTest" == + suggest_module_name( + "some/path/my_project/test/my_project/foo/bar/baz/my_file_test.exs" + ) + end + end end From 3935a16c1e1d4dcffd606dec8fb1391714c30e91 Mon Sep 17 00:00:00 2001 From: Manos Emmanouilidis Date: Fri, 1 Apr 2022 15:10:17 +0300 Subject: [PATCH 2/5] use processed file_path instead of raw file:// uri for determining appropriate module_names --- .../lib/language_server/providers/completion.ex | 10 +++++----- .../lib/language_server/server.ex | 2 +- .../test/providers/completion_test.exs | 16 +++++++++------- 3 files changed, 15 insertions(+), 13 deletions(-) diff --git a/apps/language_server/lib/language_server/providers/completion.ex b/apps/language_server/lib/language_server/providers/completion.ex index 3955b3e29..ce5ef4918 100644 --- a/apps/language_server/lib/language_server/providers/completion.ex +++ b/apps/language_server/lib/language_server/providers/completion.ex @@ -565,9 +565,9 @@ defmodule ElixirLS.LanguageServer.Providers.Completion do completion end - uri = Keyword.get(options, :uri) + file_path = Keyword.get(options, :file_path) - if snippet = snippet_for({origin, name}, Map.put(context, :uri, uri)) do + if snippet = snippet_for({origin, name}, Map.put(context, :file_path, file_path)) do %{completion | insert_text: snippet, kind: :snippet, label: name} else completion @@ -578,10 +578,10 @@ defmodule ElixirLS.LanguageServer.Providers.Completion do nil end - defp snippet_for({"Kernel", "defmodule"}, %{uri: uri}) when is_binary(uri) do - # In a mix project the uri can be something like "/some/code/path/project/lib/project/sub_path/myfile.ex" + defp snippet_for({"Kernel", "defmodule"}, %{file_path: file_path}) when is_binary(file_path) do + # In a mix project the file_path can be something like "/some/code/path/project/lib/project/sub_path/my_file.ex" # so we'll try to guess the appropriate module name from the path - "defmodule #{suggest_module_name(uri)}$1 do\n\t$0\nend" + "defmodule #{suggest_module_name(file_path)}$1 do\n\t$0\nend" end defp snippet_for(key, %{pipe_before?: true}) do diff --git a/apps/language_server/lib/language_server/server.ex b/apps/language_server/lib/language_server/server.ex index e239da09a..e5e9d27c7 100644 --- a/apps/language_server/lib/language_server/server.ex +++ b/apps/language_server/lib/language_server/server.ex @@ -686,7 +686,7 @@ defmodule ElixirLS.LanguageServer.Server do signature_help_supported: signature_help_supported, locals_without_parens: locals_without_parens, signature_after_complete: signature_after_complete, - uri: uri + file_path: SourceFile.path_from_uri(uri) ) end diff --git a/apps/language_server/test/providers/completion_test.exs b/apps/language_server/test/providers/completion_test.exs index a0d889a2b..9a97db9c4 100644 --- a/apps/language_server/test/providers/completion_test.exs +++ b/apps/language_server/test/providers/completion_test.exs @@ -926,8 +926,8 @@ defmodule ElixirLS.LanguageServer.Providers.CompletionTest do char, @supports |> Keyword.put( - :uri, - "file://some/path/my_project/lib/my_project/sub_folder/my_file.ex" + :file_path, + "/some/path/my_project/lib/my_project/sub_folder/my_file.ex" ) ) @@ -954,8 +954,8 @@ defmodule ElixirLS.LanguageServer.Providers.CompletionTest do char, @supports |> Keyword.put( - :uri, - "file://some/path/my_project/lib/my_project/sub_folder/my_file.heex" + :file_path, + "/some/path/my_project/lib/my_project/sub_folder/my_file.heex" ) ) @@ -1030,7 +1030,7 @@ defmodule ElixirLS.LanguageServer.Providers.CompletionTest do text = """ defmodule MyModule do def hello do - Date.today() |> + Date.today() |> # ^ end end @@ -1042,6 +1042,8 @@ defmodule ElixirLS.LanguageServer.Providers.CompletionTest do assert {:ok, %{"items" => items}} = Completion.completion(text, line, char, @supports) + IO.inspect(Enum.filter(items, fn i -> i["label"] == "make_ref/0" end)) + refute Enum.any?(items, fn i -> i["label"] == "make_ref/0" end) end end @@ -1113,7 +1115,7 @@ defmodule ElixirLS.LanguageServer.Providers.CompletionTest do describe "suggest_module_name/1" do import Completion, only: [suggest_module_name: 1] - test "returns nil if current file uri is empty" do + test "returns nil if current file_path is empty" do assert nil == suggest_module_name("") end @@ -1125,7 +1127,7 @@ defmodule ElixirLS.LanguageServer.Providers.CompletionTest do assert nil == suggest_module_name("some/path/not_lib/dir/file.ex") end - test "returns nil if current file is an *_test.exs file but not test folder exists in path" do + test "returns nil if current file is an *_test.exs file but no test folder exists in path" do assert nil == suggest_module_name("some/path/not_test/dir/file_test.exs") end From 036901c5991f54406ca962c2f48f57188894a03d Mon Sep 17 00:00:00 2001 From: Manos Emmanouilidis Date: Fri, 1 Apr 2022 15:23:09 +0300 Subject: [PATCH 3/5] add umbrella_app test to guard against future regressions --- apps/language_server/test/providers/completion_test.exs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/apps/language_server/test/providers/completion_test.exs b/apps/language_server/test/providers/completion_test.exs index 9a97db9c4..af97b7078 100644 --- a/apps/language_server/test/providers/completion_test.exs +++ b/apps/language_server/test/providers/completion_test.exs @@ -1151,5 +1151,12 @@ defmodule ElixirLS.LanguageServer.Providers.CompletionTest do "some/path/my_project/test/my_project/foo/bar/baz/my_file_test.exs" ) end + + test "returns an appropriate suggestion if file is part of an umbrella project" do + assert "MySubApp.Foo.Bar.Baz" == + suggest_module_name( + "some/path/my_umbrella_project/apps/my_sub_app/lib/my_sub_app/foo/bar/baz.ex" + ) + end end end From dd6c0547c7420bddb7c9103d0991e111ba2e81d0 Mon Sep 17 00:00:00 2001 From: Manos Emmanouilidis Date: Fri, 1 Apr 2022 18:33:29 +0300 Subject: [PATCH 4/5] special case common Phoenix folders when suggesting module names --- .../language_server/providers/completion.ex | 24 +++++++++++++++++ .../test/providers/completion_test.exs | 27 +++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/apps/language_server/lib/language_server/providers/completion.ex b/apps/language_server/lib/language_server/providers/completion.ex index ce5ef4918..611a3508d 100644 --- a/apps/language_server/lib/language_server/providers/completion.ex +++ b/apps/language_server/lib/language_server/providers/completion.ex @@ -636,6 +636,30 @@ defmodule ElixirLS.LanguageServer.Providers.Completion do |> Enum.join(".") end + defp do_suggest_module_name( + [probable_phoenix_dir | [project_web_dir | _] = rest], + module_name_acc, + opts + ) + when probable_phoenix_dir in [ + "controllers", + "views", + "channels", + "plugs", + "endpoints", + "sockets" + ] do + if String.ends_with?(project_web_dir, "_web") do + # by convention Phoenix doesn't use these folders as part of the module names + # for modules located inside them, so we'll try to do the same + do_suggest_module_name(rest, module_name_acc, opts) + else + # when not directly under the *_web folder however then we should make the folder + # part of the module's name + do_suggest_module_name(rest, [probable_phoenix_dir | module_name_acc], opts) + end + end + defp do_suggest_module_name([dir_name | rest], module_name_acc, opts) do do_suggest_module_name(rest, [dir_name | module_name_acc], opts) end diff --git a/apps/language_server/test/providers/completion_test.exs b/apps/language_server/test/providers/completion_test.exs index af97b7078..5c4532a3c 100644 --- a/apps/language_server/test/providers/completion_test.exs +++ b/apps/language_server/test/providers/completion_test.exs @@ -1158,5 +1158,32 @@ defmodule ElixirLS.LanguageServer.Providers.CompletionTest do "some/path/my_umbrella_project/apps/my_sub_app/lib/my_sub_app/foo/bar/baz.ex" ) end + + test "returns appropriate suggestions for modules nested under known phoenix dirs" do + [ + {"MyProjectWeb.MyController", "controllers/my_controller.ex"}, + {"MyProjectWeb.MyPlug", "plugs/my_plug.ex"}, + {"MyProjectWeb.MyView", "views/my_view.ex"}, + {"MyProjectWeb.MyChannel", "channels/my_channel.ex"}, + {"MyProjectWeb.MyEndpoint", "endpoints/my_endpoint.ex"}, + {"MyProjectWeb.MySocket", "sockets/my_socket.ex"} + ] + |> Enum.each(fn {expected_module_name, partial_path} -> + path = "some/path/my_project/lib/my_project_web/#{partial_path}" + assert expected_module_name == suggest_module_name(path) + end) + end + + test "uses known Phoenix dirs as part of a module's name if these are not located directly beneath the *_web folder" do + assert "MyProject.Controllers.MyController" == + suggest_module_name( + "some/path/my_project/lib/my_project/controllers/my_controller.ex" + ) + + assert "MyProjectWeb.SomeNestedDir.Controllers.MyController" == + suggest_module_name( + "some/path/my_project/lib/my_project_web/some_nested_dir/controllers/my_controller.ex" + ) + end end end From d3b624fb2ecb4ead1fe061bca82d783848cb8af9 Mon Sep 17 00:00:00 2001 From: Manos Emmanouilidis Date: Fri, 15 Apr 2022 14:10:53 +0300 Subject: [PATCH 5/5] fix broken test Some plugin or setting in my editor trimmed extra spaces from the lines but in this case it cause a test to break by changing the cursor position of the auto-completion trigger --- apps/language_server/test/providers/completion_test.exs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/apps/language_server/test/providers/completion_test.exs b/apps/language_server/test/providers/completion_test.exs index 5c4532a3c..f60b923de 100644 --- a/apps/language_server/test/providers/completion_test.exs +++ b/apps/language_server/test/providers/completion_test.exs @@ -1030,7 +1030,7 @@ defmodule ElixirLS.LanguageServer.Providers.CompletionTest do text = """ defmodule MyModule do def hello do - Date.today() |> + Date.today() |> # ^ end end @@ -1042,8 +1042,6 @@ defmodule ElixirLS.LanguageServer.Providers.CompletionTest do assert {:ok, %{"items" => items}} = Completion.completion(text, line, char, @supports) - IO.inspect(Enum.filter(items, fn i -> i["label"] == "make_ref/0" end)) - refute Enum.any?(items, fn i -> i["label"] == "make_ref/0" end) end end