Skip to content
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

URI - file system path conversion fixes #447

Merged
merged 11 commits into from
Jan 14, 2021
9 changes: 6 additions & 3 deletions .formatter.exs
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
[
inputs: [
"*.exs",
"config/**/*.exs",
"apps/*/{config,lib,test}/**/*.{ex,exs}",
"apps/*/mix.exs"
"config/**/*.exs"
],
subdirectories: [
"apps/elixir_ls_utils",
"apps/elixir_ls_debugger",
"apps/language_server"
]
]
6 changes: 6 additions & 0 deletions apps/elixir_ls_debugger/.formatter.exs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
[
inputs: [
"*.exs",
"{lib,test,config}/**/*.{ex,exs}"
]
]
8 changes: 8 additions & 0 deletions apps/elixir_ls_utils/.formatter.exs
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
[
inputs: [
"*.exs",
"{lib,config}/**/*.{ex,exs}",
"test/*.exs",
"test/support/**/*.ex"
]
]
6 changes: 6 additions & 0 deletions apps/language_server/.formatter.exs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
[
inputs: [
"*.exs",
"{lib,test,config}/**/*.{ex,exs}"
]
]
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ defmodule ElixirLS.LanguageServer.Providers.CodeLens.Test do

@run_test_command "elixir.lens.test.run"

def code_lens(uri, text) do
def code_lens(uri = "file:" <> _, text) do
with {:ok, buffer_file_metadata} <- parse_source(text) do
source_lines = SourceFile.lines(text)

Expand Down Expand Up @@ -48,6 +48,8 @@ defmodule ElixirLS.LanguageServer.Providers.CodeLens.Test do
end
end

def code_lens(_uri, _text), do: {:ok, []}

defp get_test_lenses(test_blocks, file_path) do
args = fn block ->
%{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,15 @@ defmodule ElixirLS.LanguageServer.Providers.Formatting do

# If in an umbrella project, the cwd might be set to a sub-app if it's being compiled. This is
# fine if the file we're trying to format is in that app. Otherwise, we return an error.
defp can_format?(file_uri, project_dir) do
defp can_format?(file_uri = "file:" <> _, project_dir) do
file_path = file_uri |> SourceFile.path_from_uri() |> Path.absname()

not String.starts_with?(file_path, project_dir) or
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The negation that was here seems wrong. @axelson do you have any idea why it was like this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it does seem backwards. I'd have to do some investigation but I would guess that it would be related to the fact that previously it wasn't doing a Path.absname before. But I'd have to do some more investigation to be more sure. If we do keep a check like it around, then I think we should refactor it to make it a bit more obvious exactly what it is checking (and add some relevant tests).

For reference this is the commit that introduced it: d143772

Copy link
Collaborator Author

@lukaszsamson lukaszsamson Jan 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, this module would use a refactor and better test coverage. Should I create a new issue? Edit: #459

String.starts_with?(file_path, Path.absname(project_dir)) or
String.starts_with?(file_path, File.cwd!())
end

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

def should_format?(file_uri, project_dir, inputs) when is_list(inputs) do
file_path = file_uri |> SourceFile.path_from_uri() |> Path.absname()

Expand Down
8 changes: 6 additions & 2 deletions apps/language_server/lib/language_server/server.ex
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ defmodule ElixirLS.LanguageServer.Server do
end

@impl GenServer
def handle_call({:suggest_contracts, uri}, from, state) do
def handle_call({:suggest_contracts, uri = "file:" <> _}, from, state) do
case state do
%{analysis_ready?: true, source_files: %{^uri => %{dirty?: false}}} ->
{:reply, Dialyzer.suggest_contracts([SourceFile.path_from_uri(uri)]), state}
Expand All @@ -130,6 +130,10 @@ defmodule ElixirLS.LanguageServer.Server do
end
end

def handle_call({:suggest_contracts, _uri}, _from, state) do
{:reply, [], state}
end

@impl GenServer
def handle_cast({:build_finished, {status, diagnostics}}, state)
when status in [:ok, :noop, :error] and is_list(diagnostics) do
Expand Down Expand Up @@ -400,7 +404,7 @@ defmodule ElixirLS.LanguageServer.Server do
# deleted file still open in editor, keep dirty flag
acc

%{"uri" => uri}, acc ->
%{"uri" => uri = "file:" <> _}, acc ->
# file created/updated - set dirty flag to false if file contents are equal
case acc[uri] do
%SourceFile{text: source_file_text, dirty?: true} = source_file ->
Expand Down
103 changes: 89 additions & 14 deletions apps/language_server/lib/language_server/source_file.ex
Original file line number Diff line number Diff line change
Expand Up @@ -62,28 +62,101 @@ defmodule ElixirLS.LanguageServer.SourceFile do
@doc """
Returns path from URI in a way that handles windows file:///c%3A/... URLs correctly
"""
def path_from_uri(uri) do
uri_path = URI.decode(URI.parse(uri).path)
def path_from_uri(%URI{scheme: "file", path: path, authority: authority}) do
uri_path =
cond do
path == nil ->
# treat no path as root path
"/"

authority not in ["", nil] and path not in ["", nil] ->
# UNC path
"//#{URI.decode(authority)}#{URI.decode(path)}"

true ->
decoded_path = URI.decode(path)

if match?({:win32, _}, :os.type()) and
String.match?(decoded_path, ~r/^\/[a-zA-Z]:/) do
# Windows drive letter path
# drop leading `/` and downcase drive letter
<<_, letter, path_rest::binary>> = decoded_path
<<downcase(letter), path_rest::binary>>
else
decoded_path
end
end

case :os.type() do
{:win32, _} -> String.trim_leading(uri_path, "/")
_ -> uri_path
{:win32, _} ->
# convert path separators from URI to Windows
String.replace(uri_path, ~r/\//, "\\")

_ ->
uri_path
end
end

def path_from_uri(%URI{scheme: scheme}) do
raise ArgumentError, message: "unexpected URI scheme #{inspect(scheme)}"
end

def path_from_uri(uri) do
uri |> URI.parse() |> path_from_uri
end

def path_to_uri(path) do
uri_path =
path
|> Path.expand()
|> URI.encode()
|> String.replace(":", "%3A")
path = Path.expand(path)

case :os.type() do
{:win32, _} -> "file:///" <> uri_path
_ -> "file://" <> uri_path
end
path =
case :os.type() do
{:win32, _} ->
# convert path separators from Windows to URI
String.replace(path, ~r/\\/, "/")

_ ->
path
end

{authority, path} =
case path do
"//" <> rest ->
# UNC path - extract authority
case String.split(rest, "/", parts: 2) do
[_] ->
# no path part, use root path
{rest, "/"}

[a, ""] ->
# empty path part, use root path
{a, "/"}

[a, p] ->
{a, "/" <> p}
end

"/" <> _rest ->
{"", path}

other ->
# treat as relative to root path
{"", "/" <> other}
end

%URI{
scheme: "file",
authority: authority |> URI.encode(),
# file system paths allow reserved URI characters that need to be escaped
# the exact rules are complicated but for simplicity we escape all reserved except `/`
# that's what https://github.com/microsoft/vscode-uri does
path: path |> URI.encode(&(&1 == ?/ or URI.char_unreserved?(&1)))
}
|> URI.to_string()
end

defp downcase(char) when char >= ?A and char <= ?Z, do: char + 32
defp downcase(char), do: char

def full_range(source_file) do
lines = lines(source_file)
last_line = List.last(lines)
Expand Down Expand Up @@ -244,7 +317,7 @@ defmodule ElixirLS.LanguageServer.SourceFile do
end

@spec formatter_opts(String.t()) :: {:ok, keyword()} | :error
def formatter_opts(uri) do
def formatter_opts(uri = "file:" <> _) do
path = path_from_uri(uri)

try do
Expand All @@ -263,6 +336,8 @@ defmodule ElixirLS.LanguageServer.SourceFile do
end
end

def formatter_opts(_), do: :error

defp format_code(code, opts) do
try do
{:ok, Code.format_string!(code, opts)}
Expand Down
55 changes: 36 additions & 19 deletions apps/language_server/test/providers/code_lens/test_test.exs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
defmodule ElixirLS.LanguageServer.Providers.CodeLens.TestTest do
use ExUnit.Case

import ElixirLS.LanguageServer.Test.PlatformTestHelpers
alias ElixirLS.LanguageServer.Providers.CodeLens

setup context do
Expand All @@ -16,7 +17,7 @@ defmodule ElixirLS.LanguageServer.Providers.CodeLens.TestTest do
end

test "returns all module code lenses" do
uri = "file://project/file.ex"
uri = "file:///project/file.ex"

text = """
defmodule MyModule do
Expand All @@ -32,13 +33,17 @@ defmodule ElixirLS.LanguageServer.Providers.CodeLens.TestTest do

assert lenses ==
[
build_code_lens(0, :module, "/file.ex", %{"module" => MyModule}),
build_code_lens(4, :module, "/file.ex", %{"module" => MyModule2})
build_code_lens(0, :module, maybe_convert_path_separators("/project/file.ex"), %{
"module" => MyModule
}),
build_code_lens(4, :module, maybe_convert_path_separators("/project/file.ex"), %{
"module" => MyModule2
})
]
end

test "returns all nested module code lenses" do
uri = "file://project/file.ex"
uri = "file:///project/file.ex"

text = """
defmodule MyModule do
Expand All @@ -54,13 +59,17 @@ defmodule ElixirLS.LanguageServer.Providers.CodeLens.TestTest do

assert lenses ==
[
build_code_lens(0, :module, "/file.ex", %{"module" => MyModule}),
build_code_lens(3, :module, "/file.ex", %{"module" => MyModule.MyModule2})
build_code_lens(0, :module, maybe_convert_path_separators("/project/file.ex"), %{
"module" => MyModule
}),
build_code_lens(3, :module, maybe_convert_path_separators("/project/file.ex"), %{
"module" => MyModule.MyModule2
})
]
end

test "does not return lenses for modules that don't import ExUnit.case" do
uri = "file://project/file.ex"
uri = "file:///project/file.ex"

text = """
defmodule MyModule do
Expand All @@ -73,7 +82,7 @@ defmodule ElixirLS.LanguageServer.Providers.CodeLens.TestTest do
end

test "returns lenses for all describe blocks" do
uri = "file://project/file.ex"
uri = "file:///project/file.ex"

text = """
defmodule MyModule do
Expand All @@ -91,17 +100,21 @@ defmodule ElixirLS.LanguageServer.Providers.CodeLens.TestTest do

assert Enum.member?(
lenses,
build_code_lens(3, :describe, "/file.ex", %{"describe" => "describe1"})
build_code_lens(3, :describe, maybe_convert_path_separators("/project/file.ex"), %{
"describe" => "describe1"
})
)

assert Enum.member?(
lenses,
build_code_lens(6, :describe, "/file.ex", %{"describe" => "describe2"})
build_code_lens(6, :describe, maybe_convert_path_separators("/project/file.ex"), %{
"describe" => "describe2"
})
)
end

test "returns lenses for all test blocks" do
uri = "file://project/file.ex"
uri = "file:///project/file.ex"

text = """
defmodule MyModule do
Expand All @@ -119,17 +132,21 @@ defmodule ElixirLS.LanguageServer.Providers.CodeLens.TestTest do

assert Enum.member?(
lenses,
build_code_lens(3, :test, "/file.ex", %{"testName" => "test1"})
build_code_lens(3, :test, maybe_convert_path_separators("/project/file.ex"), %{
"testName" => "test1"
})
)

assert Enum.member?(
lenses,
build_code_lens(6, :test, "/file.ex", %{"testName" => "test2"})
build_code_lens(6, :test, maybe_convert_path_separators("/project/file.ex"), %{
"testName" => "test2"
})
)
end

test "given test blocks inside describe blocks, should return code lenses with the test and describe name" do
uri = "file://project/file.ex"
uri = "file:///project/file.ex"

text = """
defmodule MyModule do
Expand All @@ -146,7 +163,7 @@ defmodule ElixirLS.LanguageServer.Providers.CodeLens.TestTest do

assert Enum.member?(
lenses,
build_code_lens(4, :test, "/file.ex", %{
build_code_lens(4, :test, maybe_convert_path_separators("/project/file.ex"), %{
"testName" => "test1",
"describe" => "describe1"
})
Expand Down Expand Up @@ -281,26 +298,26 @@ defmodule ElixirLS.LanguageServer.Providers.CodeLens.TestTest do
end

test "returns module lens on the module declaration line", %{text: text} do
uri = "file://project/file.ex"
uri = "file:///project/file.ex"

{:ok, lenses} = CodeLens.Test.code_lens(uri, text)

assert Enum.member?(
lenses,
build_code_lens(0, :module, "/file.ex", %{
build_code_lens(0, :module, maybe_convert_path_separators("/project/file.ex"), %{
"module" => ElixirLS.LanguageServer.DiagnosticsTest
})
)
end

test "returns test lenses with describe info", %{text: text} do
uri = "file://project/file.ex"
uri = "file:///project/file.ex"

{:ok, lenses} = CodeLens.Test.code_lens(uri, text)

assert Enum.member?(
lenses,
build_code_lens(5, :test, "/file.ex", %{
build_code_lens(5, :test, maybe_convert_path_separators("/project/file.ex"), %{
"testName" => "extract the stacktrace from the message and format it",
"describe" => "normalize/2"
})
Expand Down
Loading