fix: expose manipulate pipes command #521

polvalente · 2021-04-02T15:58:52Z

This PR also adds a test that shows a bug which seems to be in Macro.to_string itself :( I've opened an issue in the Elixir repo elixir-lang/elixir#10864 for discussion. Maybe this is expected and is something we have to deal with

polvalente · 2021-04-02T20:08:59Z

It seems that this fix is needed: elixir-lang/elixir#10864 (comment)
Let's wait for a response in the issue before merging this PR

polvalente · 2021-04-02T22:22:24Z

So, a commit was added changing this behavior in Elixir. Due to the fact that we need to be retrocompatible, I've added a function to treat added escapes whenever they are added. The main assumption is that from_pipe and to_pipe should never change the number of non-whitespace chars besides adding or removing |>, so if the number of \ changes, it means that we need to execute said replacement.

polvalente · 2021-04-03T19:17:21Z

apps/language_server/lib/language_server/providers/execute_command/manipulate_pipes/ast.ex

@@ -64,4 +64,17 @@ defmodule ElixirLS.LanguageServer.Providers.ExecuteCommand.ManipulatePipes.AST d
  defp do_to_pipe(node, acc) do
    {node, acc}
  end
+
+  defp fix_escape_chars(parsed, original) do


This fix doesn't work properly because sigil delimiters changed to " prior to 1.10

Some parsing of the AST will be needed to treat sigils

polvalente · 2021-04-03T19:41:12Z

apps/language_server/lib/language_server/providers/execute_command/manipulate_pipes/ast.ex

@@ -64,4 +64,77 @@ defmodule ElixirLS.LanguageServer.Providers.ExecuteCommand.ManipulatePipes.AST d
  defp do_to_pipe(node, acc) do
    {node, acc}
  end
+
+  def ast_to_string(ast) do


This is an attempt to handle escape characters as expected. Local tests passes successfully, but I'm not sure if this is the best way to approach the problem

axelson · 2021-04-03T19:50:08Z

I've been trying this out a bit with (https://github.com/elixir-lsp/vscode-elixir-ls/pull/182/files) and I'm running into some issues. It appears that sometimes the extracted text . Here's a partial failing test (partial because it should check the result):

    test "can handle multiple calls" do
      {:ok, _} =
        JsonRpcMock.start_link(success_reply: {:ok, %{"applied" => true}}, test_pid: self())

      uri = "file:/some_file.ex"

      text = """
      defmodule BasicEx do
        def add(a) do
          require Logger

          Logger.info(to_string(add_num(a, 12)))
        end

        def add_num(a, num), do: a + num
      end
      """

      assert {:ok, nil} =
               ManipulatePipes.execute(
                 ["fromPipe", uri, 4, 16],
                 %Server{
                   source_files: %{
                     uri => %SourceFile{
                       text: text
                     }
                   }
                 }
               )

That test results in:

code_string: "Logger.info(to_string(add_num(a, 12))"

  1) test execute/2 fromPipe can handle multiple calls (ElixirLS.LanguageServer.Providers.ExecuteCommand.ManipulatePipesTest)
     test/providers/execute_command/manipulate_pipes_test.exs:763
     ** (TokenMissingError) nofile:1: missing terminator: ) (for "(" starting at line 1)
     code: ManipulatePipes.execute(
     stacktrace:
       lib/language_server/providers/execute_command/manipulate_pipes/ast.ex:23: ElixirLS.LanguageServer.Providers.ExecuteCommand.ManipulatePipes.AST.from_pipe/1
       lib/language_server/providers/execute_command/manipulate_pipes.ex:122: ElixirLS.LanguageServer.Providers.ExecuteCommand.ManipulatePipes.from_pipe_at_cursor/3
       lib/language_server/providers/execute_command/manipulate_pipes.ex:31: ElixirLS.LanguageServer.Providers.ExecuteCommand.ManipulatePipes.execute/2
       test/providers/execute_command/manipulate_pipes_test.exs:782: (test)

Where I would expect the value of code_string to instead be more like Logger.info(to_string(add_num(a, 12))) (in actuality it is missing a paren). I think a good test would be to run the same command, but once with each character position and ensure that the same value is output each time (which is what I would expect as a user running extension.toPipe on a line without a ;)

polvalente · 2021-04-03T20:26:52Z

@axelson could you elaborate on "once with each character position"? Do you mean something like for column <- 0..line_length-1, do: ..., or just the "valid" columns for a given "to pipe"/"from pipe"?

The command is currently highly dependent on where the cursor is because there can be multiple function calls and multple pipes in a line. Anyway, your case is indeed a bug and I'll investigate on what's causing it

polvalente · 2021-04-03T22:23:30Z

apps/language_server/lib/language_server/providers/execute_command/manipulate_pipes.ex

@@ -230,7 +238,11 @@ defmodule ElixirLS.LanguageServer.Providers.ExecuteCommand.ManipulatePipes do
        col + tail_length
      end

-    {:ok, pipe_call, range(start_line, start_col, end_line, end_col)}
+    if String.contains?(pipe_call, "|>") do


@axelson I've added this because your test actually was calling "fromPipe", which shouldn't do anything if there aren't any pipes in the selected text

Another possible option which would require some rework is to scan until a pipe is found, but this could have unexpected results

polvalente · 2021-04-03T22:25:10Z

apps/language_server/lib/language_server/providers/execute_command/manipulate_pipes.ex

@@ -280,7 +292,7 @@ defmodule ElixirLS.LanguageServer.Providers.ExecuteCommand.ManipulatePipes do
    |> String.reverse()
    |> String.graphemes()
    |> Enum.reduce_while([], fn c, acc ->
-      if String.match?(c, ~r/\s/) do
+      if String.match?(c, ~r/[\s\(\[\{]/) do


This was needed if the suggested test is changed to "toPipe", because Logger.info(to_string(...) doesn't have any spaces, but the first ( actually breaks the identifier to_string

polvalente · 2021-04-03T22:27:06Z

apps/language_server/test/providers/execute_command/manipulate_pipes_test.exs

+        "start" => %{"character" => 16, "line" => 4}
+      }
+
+      expected_substitution = "to_string(a |> add_num(12))"


This one might bring up some discussion. The way that our macro-walking function is implemented, the code infers that the innermost function call is the one that should be piped.

Does this need to be reworked so the outermost one is the piped call?

I guess that would be preferred. On the other hand this is an ambiguous case.

@lukaszsamson I've fixed this in the latest commit. The issue was that there was a and t != [] clause which disallowed 1-arg functions from being piped. No other tests broke, so that's a good sign, I guess

axelson · 2021-04-04T05:04:35Z

@axelson could you elaborate on "once with each character position"? Do you mean something like for column <- 0..line_length-1, do: ..., or just the "valid" columns for a given "to pipe"/"from pipe"?

Yeah, for column <- 0..line_length-1, do: ... is exactly what I mean. But since the output may change based on where the cursor is, it would just check that no exception is raised.

The command is currently highly dependent on where the cursor is because there can be multiple function calls and multple pipes in a line. Anyway, your case is indeed a bug and I'll investigate on what's causing it

Yeah, makes sense 👍

polvalente · 2021-04-05T01:23:46Z

apps/language_server/test/providers/execute_command/manipulate_pipes_test.exs

@@ -760,4 +889,42 @@ defmodule ElixirLS.LanguageServer.Providers.ExecuteCommand.ManipulatePipesTest d
             ) == edited_text
    end
  end
+
+  defp assert_never_raises(text, uri, command) do


@axelson I've added this helper assertion that travels through the whole text calling the corresponding command ("toPipe" or "fromPipe"). This should ensure that no position in each test file raises in the code. Some bugs have been found and fixed this way already :)

lukaszsamson · 2021-04-06T14:23:36Z

apps/language_server/lib/language_server/providers/execute_command/manipulate_pipes/ast.ex

+  # fix which was introduced in
+  # https://github.com/elixir-lang/elixir/commit/88d82f059756ed1eb56a562fae7092f77d941de8
+  # is needed for proper treatment of sigils in our use-case.
+  defp sigil_call({sigil, meta, [{:<<>>, _, _} = parts, args]} = ast, fun)


add a TODO with info which part may be removed when we require elixir 1.13 (or 1.12 if the fix gets backported)

axelson

Okay, taking a deeper look at this. Sorry for the delay.

I have a high-level comment, that I wish I had flagged earlier. Given this snippet:

Ash.Changeset.for_create(Track, :create, data)
|> GenTracker.API.create()

Running toPipe on the first line I would expect to end up with

Track
|> Ash.Changeset.for_create(:create, data)
|> GenTracker.API.create()

But instead end up with

Track |> Ash.Changeset.for_create(:create, data)
|> GenTracker.API.create()

But I suppose this would be hard to handle for code that has multiple function calls on one line, so perhaps it's best handled as a future improvement.

Also (with the above code) I would expect the same result no matter where on the first line I called toPipe, but instead there's a bug where if you call toPipe at the end of the line you end up with:

Ash.Changeset.for_create(Track, :create, data)GenTracker.API.create()

Which is obviously wrong and does not compile.

to_pipe_at_cursor and from_pipe_at_cursor suggested changes:

For easier testing I think we should make them public functions and call them from the tests.
I think we should have those functions return a %ElixirLS.LanguageServer.Protocol.TextEdit{} (which needs to be created) instead a raw map
Then those two can be combined with ElixirLS.LanguageServer.Test.TestUtils.apply_text_edit/2 to greatly simplify the tests
I have some code related to this that I hope to post soon

polvalente · 2021-05-21T00:48:38Z

Okay, taking a deeper look at this. Sorry for the delay.

I have a high-level comment, that I wish I had flagged earlier. Given this snippet:
Ash.Changeset.for_create(Track, :create, data)
|> GenTracker.API.create()
Running toPipe on the first line I would expect to end up with
Track
|> Ash.Changeset.for_create(:create, data)
|> GenTracker.API.create()
But instead end up with
Track |> Ash.Changeset.for_create(:create, data)
|> GenTracker.API.create()
But I suppose this would be hard to handle for code that has multiple function calls on one line, so perhaps it's best handled as a future improvement.

Thanks for the review!

About changing multi-line to single-line pipes, I'd rather leave that as a future improvement, since most [citation needed] people that will be using this command will also be using "format on save" or similiar features.

Also (with the above code) I would expect the same result no matter where on the first line I called toPipe, but instead there's a bug where if you call toPipe at the end of the line you end up with:
Ash.Changeset.for_create(Track, :create, data)GenTracker.API.create()
Which is obviously wrong and does not compile.

Nice catch! I'll add a test and a fix for this as soon as I can get back to this PR.

to_pipe_at_cursor and from_pipe_at_cursor suggested changes:

For easier testing I think we should make them public functions and call them from the tests.

Which tests are you talking about? The integration ones, which receive the command itself?
I guess we could migrate them to a model where there is one test-case which calls it and the others only call these functions. WDYT?

I think we should have those functions return a %ElixirLS.LanguageServer.Protocol.TextEdit{} (which needs to be created) instead a raw map

Then those two can be combined with ElixirLS.LanguageServer.Test.TestUtils.apply_text_edit/2 to greatly simplify the tests

Sounds good!

I have some code related to this that I hope to post soon

Should I wait for said code?

Make `to_pipe_at_cursor/3` public to facilitate testing. I think we should still keep some integration-level testing, but I think the majority of the test cases can be simplified. I didn't implement making `from_pipe_at_cursor/3` public, but I think it would make sense as well. Introduce `ElixirLS.LanguageServer.Protocol.TextEdit` and return it from `to_pipe_at_cursor/3` and `from_pipe_at_cursor/3` Add an example of a working simpler test, and add a few examples of broken tests (as mentioned in elixir-lsp#521 (review))

…d-changes Proposed changes

polvalente · 2021-05-22T02:29:27Z

apps/language_server/lib/language_server/providers/execute_command/manipulate_pipes.ex

+  defp get_function_call(line, col, head, cur, original_tail) when cur in ["\n", "\r", "\r\n"] do
+    {head, new_cur} = String.split_at(head, -1)
+    get_function_call(line, col - 1, head, new_cur, cur <> original_tail)
+  end
+
+  defp get_function_call(line, col, head, ")", original_tail) do
+    {head, cur} = String.split_at(head, -1)
+    get_function_call(line, col - 1, head, cur, ")" <> original_tail)
+  end


@axelson here's what caused the tests you added to break: do to how do_get_function_call works, when we're at a closing paren, it will jump and try to pipe the next function call. The same for when we're already at the end of a line.

I'm thinking that maybe the correct fix is to track back the text until we reach a "(" character instead (updating indices properly).

I haven't tested, but this one probably breaks if the line ends as ") \n"

WDYT? Any other ideas?

Actually, tracing back blind yields other problems when the cursor is in the leading whitespace for the line :(

I"ve added a test which demonstrates the ")\s\n" case still breaking

do to how do_get_function_call works, when we're at a closing paren, it will jump and try to pipe the next function call. The same for when we're already at the end of a line.

Do we need to keep the "jumping forward" behavior? What is it buying us?

It's more of an artifact on how the "function call finding" code works. for instance on a multi-line function call like:

f( x, y, z)

Having the cursor at the beggining of the line containing x will still find the function correctly.

However, we can just for the cursor to be, for instance at a "(" character or a non-whitespace character, for instance

ah, I see.

FYI, I found another breaking case so I'm working on fixing that.

Changed the function call detection to not "jump forward", as part of this if the user requests an index that is not on a function call, then no action is performed. Also fix handling of piping function calls that span multiple lines by adjusting the returned line and column to point to the correct start location by counting backward.

axelson · 2021-06-02T07:29:06Z

@polvalente okay, I've pushed my proposed changes to a new PR: polvalente#2

This makes all the tests pass, but I want to do some more local testing as well.

Handle more cases for manipulating pipes Co-Authored-By: Jason Axelson <[email protected]>

axelson · 2021-06-02T17:35:27Z

@lukaszsamson Would you mind taking another look at the code as well? It's changed a bit since your review.

lukaszsamson · 2021-06-27T16:33:23Z

@axelson Looks good to me, LGTM

* fix: expose manipulate pipes command * test: add test for known bug * fix: make test retrocompatible * feat: treat escape chars after macro to_string * docs: add comment for treatment * feat: import code from Elixir source code to deal with sigils * fix: use ast_to_string instead of Macro.to_string * chore: remove redundant test * fix: various fixes due to new tests * test: make test retrocompatible * fix: do not raise for any position in the file * docs: add todo comment * fix: also pipe 1-arg functions * test: fix test assertion * Proposed changes Make `to_pipe_at_cursor/3` public to facilitate testing. I think we should still keep some integration-level testing, but I think the majority of the test cases can be simplified. I didn't implement making `from_pipe_at_cursor/3` public, but I think it would make sense as well. Introduce `ElixirLS.LanguageServer.Protocol.TextEdit` and return it from `to_pipe_at_cursor/3` and `from_pipe_at_cursor/3` Add an example of a working simpler test, and add a few examples of broken tests (as mentioned in elixir-lsp#521 (review)) * chore: unbreak tests * fix: make tests pass * test: add broken test * Handle more cases for manipulating pipes Changed the function call detection to not "jump forward", as part of this if the user requests an index that is not on a function call, then no action is performed. Also fix handling of piping function calls that span multiple lines by adjusting the returned line and column to point to the correct start location by counting backward. * chore: add suggestions from code review * chore: format code Co-authored-by: Jason Axelson <[email protected]>

polvalente added 3 commits April 2, 2021 12:22

fix: expose manipulate pipes command

9d8e189

test: add test for known bug

24cdfc9

fix: make test retrocompatible

c60a98f

polvalente mentioned this pull request Apr 2, 2021

feat: add toPipe and fromPipe commands elixir-lsp/vscode-elixir-ls#182

Merged

polvalente added 2 commits April 2, 2021 19:18

feat: treat escape chars after macro to_string

019bc5e

docs: add comment for treatment

c87c27d

polvalente commented Apr 3, 2021

View reviewed changes

polvalente added 2 commits April 3, 2021 16:37

feat: import code from Elixir source code to deal with sigils

9fc2285

fix: use ast_to_string instead of Macro.to_string

5a2ec92

polvalente commented Apr 3, 2021

View reviewed changes

chore: remove redundant test

38aca54

polvalente added 2 commits April 3, 2021 19:20

fix: various fixes due to new tests

e4523f8

test: make test retrocompatible

eafe746

polvalente commented Apr 3, 2021

View reviewed changes

fix: do not raise for any position in the file

19f8a4f

polvalente commented Apr 5, 2021

View reviewed changes

lukaszsamson reviewed Apr 6, 2021

View reviewed changes

polvalente added 2 commits April 8, 2021 01:16

docs: add todo comment

e1e65e1

fix: also pipe 1-arg functions

a20fa17

polvalente requested a review from lukaszsamson April 8, 2021 04:30

test: fix test assertion

3f80388

lukaszsamson approved these changes Apr 13, 2021

View reviewed changes

axelson requested changes May 20, 2021

View reviewed changes

axelson mentioned this pull request May 21, 2021

Proposed changes polvalente/elixir-ls#1

Merged

polvalente added 3 commits May 21, 2021 17:42

Merge pull request #1 from axelson/fix-expose-manipulate-pipes-comman…

65ee05d

…d-changes Proposed changes

chore: unbreak tests

195c928

fix: make tests pass

adcd0b1

polvalente commented May 22, 2021

View reviewed changes

test: add broken test

6507e72

axelson mentioned this pull request May 23, 2021

Feature Request: Add refactor functionality #436

Closed

polvalente and others added 2 commits June 2, 2021 09:43

chore: add suggestions from code review

7f6719e

Merge pull request #2 from axelson/manipulate-pipes-improvements-v2

6599fb8

Handle more cases for manipulating pipes Co-Authored-By: Jason Axelson <[email protected]>

polvalente force-pushed the fix/expose-manipulate-pipes-command branch from 37de054 to 6599fb8 Compare June 2, 2021 12:45

chore: format code

b70a95b

axelson approved these changes Jun 26, 2021

View reviewed changes

axelson merged commit f77999b into elixir-lsp:master Jun 27, 2021

polvalente deleted the fix/expose-manipulate-pipes-command branch June 28, 2021 12:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: expose manipulate pipes command #521

fix: expose manipulate pipes command #521

polvalente commented Apr 2, 2021

polvalente commented Apr 2, 2021

polvalente commented Apr 2, 2021

polvalente Apr 3, 2021

polvalente Apr 3, 2021

axelson commented Apr 3, 2021

polvalente commented Apr 3, 2021

polvalente Apr 3, 2021

polvalente Apr 3, 2021

polvalente Apr 3, 2021

polvalente Apr 3, 2021

lukaszsamson Apr 6, 2021

polvalente Apr 8, 2021

axelson commented Apr 4, 2021

polvalente Apr 5, 2021

lukaszsamson Apr 6, 2021

axelson left a comment

polvalente commented May 21, 2021

polvalente May 22, 2021

polvalente May 22, 2021

polvalente May 22, 2021

axelson May 22, 2021

polvalente May 22, 2021

axelson May 22, 2021

axelson commented Jun 2, 2021

axelson commented Jun 2, 2021

lukaszsamson commented Jun 27, 2021

fix: expose manipulate pipes command #521

fix: expose manipulate pipes command #521

Conversation

polvalente commented Apr 2, 2021

polvalente commented Apr 2, 2021

polvalente commented Apr 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axelson commented Apr 3, 2021

polvalente commented Apr 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axelson commented Apr 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axelson left a comment

Choose a reason for hiding this comment

polvalente commented May 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axelson commented Jun 2, 2021

axelson commented Jun 2, 2021

lukaszsamson commented Jun 27, 2021