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

* Convert should_format? test to use format/3 and fixtures

This function is really an implementation detail, so let’s test the
public API.

The function also relies on interaction with the filesystem, so testing
without fixtures is a bad idea because it will lead to incorrect
results.

* Check .formatter.exs :inputs relative to its directory

When dealing with `.formatter.exs` files that are in subdirectories of
the main project, the behaviour was incorrect:
#361

And also caused performance problems:
#381

A good example is a project I currently work on. We have a top-level
`.formatter.exs`, as well as a `test/.formatter.exs`.

The one in `test/` has the `:inputs` option set to `["**/*.{ex,exs}"]`.
Before this commit, should_format?/3 would take this pattern and
concatenate it with the project directory. This would result in
Path.wildcard/2 globbing for all source files in the entire project,
rather than just under test/. This causes formatting to take about 5
seconds on our project.

This commit fixes the problem by taking the file to be formatted, and
finding the `.formatter.exs` file that exists in the closest parent
directory (so long as that directory is still within the project root).
We then perform the Path.wildcard/2 relative to that directory.

I’ve removed the hack that was in place to support umbrella projects,
because this is a more general solution to the problem.

On our project, I am now able to format a file in 100ms or so, which
isn’t a bad speed-up!

* Avoid costly Path.wildcard/2 call

The call to Path.wildcard/2 can cause use to traverse a large number of
files, which can be very slow depending on the project and the globs
used:

#381

I wrote a library that implements a glob parser and allows us to check
whether a glob matches a path without touching the filesystem:

https://github.com/jonleighton/path_glob

This commit switches out the Path.wildcard/2 call to use this library.

Co-authored-by: Jason Axelson <[email protected]>

Author: jonleighton

apps/language_server/lib/language_server/providers/formatting.ex

@@ -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}, [])

apps/language_server/mix.exs

@@ -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
 

apps/language_server/test/fixtures/formatter/.formatter.exs

@@ -1,4 +1,10 @@
 [
   line_length: 20,
-  subdirectories: ["lib"]
+  subdirectories: [
+    "lib",
+    "test",
+    "symlink",
+    "apps/*"
+  ],
+  inputs: ["*.ex"]
 ]

apps/language_server/test/fixtures/formatter/apps/foo/.formatter.exs

@@ -0,0 +1,3 @@
+[
+  inputs: ["foo.ex"]
+]

apps/language_server/test/fixtures/formatter/symlink

@@ -0,0 +1 @@
+test

apps/language_server/test/fixtures/formatter/test/.formatter.exs

@@ -0,0 +1,3 @@
+[
+  inputs: ["*.exs"]
+]

apps/language_server/test/providers/formatting_test.exs

@@ -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

apps/language_server/test/server_test.exs

@@ -917,6 +917,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

mix.lock

@@ -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"},
 }