Skip to content

Fix formatting performance problems with .formatter.exs in subdirectories #609

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Dec 4, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 24 additions & 10 deletions apps/language_server/lib/language_server/providers/formatting.ex
Original file line number Diff line number Diff line change
Expand Up @@ -53,21 +53,35 @@ defmodule ElixirLS.LanguageServer.Providers.Formatting do

defp can_format?(_uri, _project_dir), do: false

def should_format?(file_uri, project_dir, inputs) when is_list(inputs) do
defp should_format?(file_uri, project_dir, inputs) when is_list(inputs) do
file_path = file_uri |> SourceFile.abs_path_from_uri()
formatter_dir = find_formatter_dir(project_dir, Path.dirname(file_path))

inputs
|> Stream.flat_map(fn glob ->
[
Path.join([project_dir, glob]),
Path.join([project_dir, "apps", "*", glob])
]
Enum.any?(inputs, fn input_glob ->
glob = Path.join(formatter_dir, input_glob)
PathGlob.match?(file_path, glob, match_dot: true)
end)
|> Stream.flat_map(&Path.wildcard(&1, match_dot: true))
|> Enum.any?(&(file_path == &1))
end

def should_format?(_file_uri, _project_dir, _inputs), do: true
defp should_format?(_file_uri, _project_dir, _inputs), do: true

# Finds the deepest directory that contains file_path, that also contains a
# .formatter.exs. It's possible, though unlikely, that the .formatter.exs we
# find is not actually linked to the project_dir via the :subdirectories
# option in the top-level .formatter.exs. Currently, that edge case is
# glossed over.
defp find_formatter_dir(project_dir, dir) do
cond do
dir == project_dir ->
project_dir

Path.join(dir, ".formatter.exs") |> File.exists?() ->
dir

true ->
find_formatter_dir(project_dir, Path.dirname(dir))
end
end

defp myers_diff_to_text_edits(myers_diff) do
myers_diff_to_text_edits(myers_diff, {0, 0}, [])
Expand Down
3 changes: 2 additions & 1 deletion apps/language_server/mix.exs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ defmodule ElixirLS.LanguageServer.Mixfile do
{:erl2ex, github: "dazuma/erl2ex"},
{:dialyxir, "~> 1.0", runtime: false},
{:jason_vendored, github: "elixir-lsp/jason", branch: "vendored"},
{:stream_data, "~> 0.5", only: :test}
{:stream_data, "~> 0.5", only: :test},
{:path_glob, "~> 0.1.0"}
]
end

Expand Down
8 changes: 7 additions & 1 deletion apps/language_server/test/fixtures/formatter/.formatter.exs
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
[
line_length: 20,
subdirectories: ["lib"]
subdirectories: [
"lib",
"test",
"symlink",
"apps/*"
],
inputs: ["*.ex"]
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[
inputs: ["foo.ex"]
]
1 change: 1 addition & 0 deletions apps/language_server/test/fixtures/formatter/symlink
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[
inputs: ["*.exs"]
]
49 changes: 40 additions & 9 deletions apps/language_server/test/providers/formatting_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -474,18 +474,49 @@ defmodule ElixirLS.LanguageServer.Providers.FormattingTest do
end)
end

@tag :fixture
test "honors :inputs when deciding to format" do
file = __ENV__.file
uri = SourceFile.path_to_uri(file)
project_dir = Path.dirname(file)
in_fixture(Path.join(__DIR__, ".."), "formatter", fn ->
project_dir = Path.expand(".")

assert_formatted("file.ex", project_dir)

# test/.formatter.exs has [inputs: ["*.exs"]]
assert_formatted("test/file.exs", project_dir)
refute_formatted("test/file.ex", project_dir)

assert_formatted("symlink/file.exs", project_dir)
refute_formatted("symlink/file.ex", project_dir)

File.mkdir!("#{project_dir}/test/foo")
refute_formatted("test/foo/file.ex", project_dir)

# apps/foo/bar/.formatter.exs has [inputs: ["foo.ex"]]
assert_formatted("apps/foo/foo.ex", project_dir)
refute_formatted("apps/foo/bar.ex", project_dir)
refute_formatted("apps/foo.ex", project_dir)
end)
end

def assert_formatted(path, project_dir) do
assert match?({:ok, [%{}]}, format(path, project_dir)), "expected '#{path}' to be formatted"
end

def refute_formatted(path, project_dir) do
assert match?({:ok, []}, format(path, project_dir)), "expected '#{path}' not to be formatted"
end

opts = []
assert Formatting.should_format?(uri, project_dir, opts[:inputs])
defp format(path, project_dir) do
project_dir = maybe_convert_path_separators(project_dir)
path = maybe_convert_path_separators("#{project_dir}/#{path}")

opts = [inputs: ["*.exs"]]
assert Formatting.should_format?(uri, project_dir, opts[:inputs])
source_file = %SourceFile{
text: "",
version: 1,
dirty?: true
}

opts = [inputs: ["*.ex"]]
refute Formatting.should_format?(uri, project_dir, opts[:inputs])
File.write!(path, "")
Formatting.format(source_file, SourceFile.path_to_uri(path), project_dir)
end
end
4 changes: 4 additions & 0 deletions apps/language_server/test/server_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -899,6 +899,10 @@ defmodule ElixirLS.LanguageServer.ServerTest do
in_fixture(__DIR__, "formatter", fn ->
uri = Path.join([root_uri(), "file.ex"])

uri
|> SourceFile.abs_path_from_uri()
|> File.write!("")

code = """
defmodule MyModule do
def my_fn do
Expand Down
2 changes: 2 additions & 0 deletions mix.lock
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
"getopt": {:hex, :getopt, "1.0.1", "c73a9fa687b217f2ff79f68a3b637711bb1936e712b521d8ce466b29cbf7808a", [:rebar3], [], "hexpm", "53e1ab83b9ceb65c9672d3e7a35b8092e9bdc9b3ee80721471a161c10c59959c"},
"jason_vendored": {:git, "https://github.com/elixir-lsp/jason.git", "ee95ca80cd67b3a499a14f469536140935eb4483", [branch: "vendored"]},
"mix_task_archive_deps": {:git, "https://github.com/elixir-lsp/mix_task_archive_deps.git", "30fa76221def649286835685fec5d151be83c354", []},
"nimble_parsec": {:hex, :nimble_parsec, "1.1.0", "3a6fca1550363552e54c216debb6a9e95bd8d32348938e13de5eda962c0d7f89", [:mix], [], "hexpm", "08eb32d66b706e913ff748f11694b17981c0b04a33ef470e33e11b3d3ac8f54b"},
"path_glob": {:hex, :path_glob, "0.1.0", "55067de99f3e920b849dfc18d7895cf6750c4e182d96690c8fec4d9aa950ac5d", [:mix], [{:nimble_parsec, "~> 1.1.0", [hex: :nimble_parsec, repo: "hexpm", optional: false]}], "hexpm", "9784b48d2f0f9b948b559954578300da3cd3382a7ed326528c489a8bc486cd07"},
"providers": {:hex, :providers, "1.8.1", "70b4197869514344a8a60e2b2a4ef41ca03def43cfb1712ecf076a0f3c62f083", [:rebar3], [{:getopt, "1.0.1", [hex: :getopt, repo: "hexpm", optional: false]}], "hexpm", "e45745ade9c476a9a469ea0840e418ab19360dc44f01a233304e118a44486ba0"},
"stream_data": {:hex, :stream_data, "0.5.0", "b27641e58941685c75b353577dc602c9d2c12292dd84babf506c2033cd97893e", [:mix], [], "hexpm", "012bd2eec069ada4db3411f9115ccafa38540a3c78c4c0349f151fc761b9e271"},
}