advance line counter by 1 on /r/n line ending (#489)

previous version wrongly advanced it by 2

Fixes #484

Author: lukaszsamson

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

@@ -61,8 +61,8 @@ defmodule ElixirLS.LanguageServer.Providers.Formatting do
 
   def should_format?(_file_uri, _project_dir, _inputs), do: true
 
-  defp myers_diff_to_text_edits(myers_diff, starting_pos \\ {0, 0}) do
-    myers_diff_to_text_edits(myers_diff, starting_pos, [])
+  defp myers_diff_to_text_edits(myers_diff) do
+    myers_diff_to_text_edits(myers_diff, {0, 0}, [])
   end
 
   defp myers_diff_to_text_edits([], _pos, edits) do
@@ -92,7 +92,7 @@ defmodule ElixirLS.LanguageServer.Providers.Formatting do
 
   defp advance_pos({line, col}, str) do
     Enum.reduce(String.split(str, "", trim: true), {line, col}, fn char, {line, col} ->
-      if char in ["\n", "\r"] do
+      if char in ["\r\n", "\n", "\r"] do
         {line + 1, 0}
       else
         # LSP contentChanges positions are based on UTF-16 string representation

apps/language_server/test/providers/formatting_test.exs

@@ -6,7 +6,7 @@ defmodule ElixirLS.LanguageServer.Providers.FormattingTest do
   alias ElixirLS.LanguageServer.Test.FixtureHelpers
 
   @tag :fixture
-  test "Formats a file" do
+  test "Formats a file with LF line endings" do
     in_fixture(Path.join(__DIR__, ".."), "formatter", fn ->
       path = "lib/file.ex"
       uri = SourceFile.path_to_uri(path)
@@ -55,6 +55,176 @@ defmodule ElixirLS.LanguageServer.Providers.FormattingTest do
     end)
   end
 
+  @tag :fixture
+  test "Formats a file with CRLF line endings" do
+    in_fixture(Path.join(__DIR__, ".."), "formatter", fn ->
+      path = "lib/file.ex"
+      uri = SourceFile.path_to_uri(path)
+
+      text = """
+      defmodule MyModule do
+        require Logger
+
+        def dummy_function() do
+          Logger.info "dummy"
+        end
+      end
+      """
+
+      text = text |> String.replace("\n", "\r\n")
+
+      source_file = %SourceFile{
+        text: text,
+        version: 1,
+        dirty?: true
+      }
+
+      project_dir = maybe_convert_path_separators(FixtureHelpers.get_path("formatter"))
+
+      assert {:ok, changes} = Formatting.format(source_file, uri, project_dir)
+
+      assert changes == [
+               %{
+                 "newText" => "\n",
+                 "range" => %{
+                   "end" => %{"character" => 0, "line" => 7},
+                   "start" => %{"character" => 3, "line" => 6}
+                 }
+               },
+               %{
+                 "newText" => "\n",
+                 "range" => %{
+                   "end" => %{"character" => 0, "line" => 6},
+                   "start" => %{"character" => 5, "line" => 5}
+                 }
+               },
+               %{
+                 "newText" => ")\n",
+                 "range" => %{
+                   "end" => %{"character" => 0, "line" => 5},
+                   "start" => %{"character" => 23, "line" => 4}
+                 }
+               },
+               %{
+                 "newText" => "(",
+                 "range" => %{
+                   "end" => %{"character" => 16, "line" => 4},
+                   "start" => %{"character" => 15, "line" => 4}
+                 }
+               },
+               %{
+                 "newText" => "\n",
+                 "range" => %{
+                   "end" => %{"character" => 0, "line" => 4},
+                   "start" => %{"character" => 25, "line" => 3}
+                 }
+               },
+               %{
+                 "newText" => "\n\n",
+                 "range" => %{
+                   "end" => %{"character" => 0, "line" => 3},
+                   "start" => %{"character" => 16, "line" => 1}
+                 }
+               },
+               %{
+                 "newText" => "\n",
+                 "range" => %{
+                   "end" => %{"character" => 0, "line" => 1},
+                   "start" => %{"character" => 21, "line" => 0}
+                 }
+               }
+             ]
+
+      assert Enum.all?(changes, fn change ->
+               assert_position_type(change["range"]["end"]) and
+                 assert_position_type(change["range"]["start"])
+             end)
+    end)
+  end
+
+  @tag :fixture
+  test "elixir formatter does not support CR line endings" do
+    in_fixture(Path.join(__DIR__, ".."), "formatter", fn ->
+      path = "lib/file.ex"
+      uri = SourceFile.path_to_uri(path)
+
+      text = """
+      defmodule MyModule do
+        require Logger
+
+        def dummy_function() do
+          Logger.info "dummy"
+        end
+      end
+      """
+
+      text = text |> String.replace("\n", "\r")
+
+      source_file = %SourceFile{
+        text: text,
+        version: 1,
+        dirty?: true
+      }
+
+      project_dir = maybe_convert_path_separators(FixtureHelpers.get_path("formatter"))
+
+      assert {:error, :internal_error, msg} = Formatting.format(source_file, uri, project_dir)
+      assert String.contains?(msg, "Unable to format")
+    end)
+  end
+
+  @tag :fixture
+  test "formatting preserves line indings inside a string" do
+    in_fixture(Path.join(__DIR__, ".."), "formatter", fn ->
+      path = "lib/file.ex"
+      uri = SourceFile.path_to_uri(path)
+
+      text = """
+      defmodule MyModule do
+        require Logger
+
+        def dummy_function() do
+          Logger.info "dummy"
+        end
+      end
+      """
+
+      text = text |> String.replace("\"dummy\"", "    \"du\nm\rm\r\ny\"")
+
+      source_file = %SourceFile{
+        text: text,
+        version: 1,
+        dirty?: true
+      }
+
+      project_dir = maybe_convert_path_separators(FixtureHelpers.get_path("formatter"))
+
+      assert {:ok, changes} = Formatting.format(source_file, uri, project_dir)
+
+      assert changes == [
+               %{
+                 "newText" => ")",
+                 "range" => %{
+                   "end" => %{"character" => 2, "line" => 7},
+                   "start" => %{"character" => 2, "line" => 7}
+                 }
+               },
+               %{
+                 "newText" => "(",
+                 "range" => %{
+                   "end" => %{"character" => 20, "line" => 4},
+                   "start" => %{"character" => 15, "line" => 4}
+                 }
+               }
+             ]
+
+      assert Enum.all?(changes, fn change ->
+               assert_position_type(change["range"]["end"]) and
+                 assert_position_type(change["range"]["start"])
+             end)
+    end)
+  end
+
   defp assert_position_type(%{"character" => ch, "line" => line}),
     do: is_integer(ch) and is_integer(line)