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

Salvage code actions from experimental server #1057

Closed

Conversation

sheldak
Copy link
Contributor

@sheldak sheldak commented Jan 25, 2024

@lukaszsamson As we have discussed in #891, I recovered code actions with some helper functions that I found useful from the experimental server.

This PR introduces two code actions.

Replacing an unused variable with an underscore

e.g.

def foo(x), do: 42

x is a variable that is not used, so would get an action:

Rename to _x

that will replace the code with

def foo(_x), do: 42

Replacing unknown remote function

e.g.

def foo, do: Enum.counts([2, 3])

contains an unknown function: counts/1. After this change, we would get 2 possible actions:

Rename to concat
Rename to count

that rewrite our code to:

def foo, do: Enum.concat([2, 3])

and

def foo, do: Enum.count([2, 3])

respectively.

To find the best functions for replacement, I use String.jaro_distance/2 with threshold 0.77 and show at most 5 suggestions (similar to did you mean functionality).

Comment on lines +18 to +23
def from(text) when is_binary(text) do
case ElixirSense.string_to_quoted(text, {1, 1}) do
{:ok, ast} -> {:ok, ast}
_ -> :error
end
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The following test of unused variables would not pass:

test "applied to a branch in a case" do
  {:ok, result} =
	~q[
	  case my_thing do
		{:ok, unused} -> :ok
		_ -> :error
	  end
	]
	|> modify(line: 1)

  expected = ~q[
	case my_thing do
	  {:ok, _unused} -> :ok
	  _ -> :error
	end
  ]

  assert result == expected
end

because ElixirSense.string_to_quoted(" {:ok, unused} -> :ok", {1, 1}) returns

{:ok, {:__atom_elixir_marker_1__, [closing: [line: 1, column: 27], line: 1, column: 2], []}}

that is not an AST of that line we can use to add underscore. It seems like ElixirSense issue. Now, in that case, we are just not producing any code action.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You cannot use ElixirSense here. Better stick with Code.string_to_quoted_with_comments as it preserves more info and do not attempt to fix errors

Copy link
Contributor Author

@sheldak sheldak Feb 27, 2024

Choose a reason for hiding this comment

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

I see. But it seems that Code.string_to_quoted_with_comments was added in Elixir 1.13 and ElixirLS supports 1.12. Will that be a problem?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not really. For now we can limit this feature to 1.13+ and 1.12 goes out of support in few months anyway

Comment on lines +24 to +28
# We're dealing with a single error on a single line.
# If the line doesn't compile (like it has a do with no end), ElixirSense
# adds additional lines to documents with errors. Also, in case of a one-line do,
# ElixirSense creates do with end from the AST.
|> maybe_recover_one_line_do(updated_ast)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the original line has one-line do, after conversion to AST and conversion back to string, the output line has do end. Here I made a workaround that preserves one-line do. It is possible because the ASTs of the cases differ - just Macro.to_string/1 creates the same line out of both.

@lukaszsamson
Copy link
Collaborator

Awesome @sheldak. I plan to play around with it after I'm done with v0.20 release

@lukaszsamson
Copy link
Collaborator

I rebased to master and merged it in 43b656d. Thanks

@sheldak
Copy link
Contributor Author

sheldak commented Mar 10, 2024

I rebased to master and merged it in 43b656d. Thanks

@lukaszsamson That's great! But didn't you prefer to use Code.string_to_quoted_with_comments instead of ElixirSense? I'm actually working on that change

@lukaszsamson
Copy link
Collaborator

Yes, let’s bring that in following PRs. I didn’t want this one to be open too long

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants