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

Code.Fragment.surround_context does not match when cursor is on end of alias #13150

Closed
lukaszsamson opened this issue Nov 28, 2023 · 5 comments
Closed

Comments

@lukaszsamson
Copy link
Contributor

Elixir and Erlang/OTP versions

Erlang/OTP 26 [erts-14.1.1] [source] [64-bit] [smp:12:12] [ds:12:12:10] [async-threads:1] [jit]
Elixir 1.16.0-rc.0 (d716bc2) (compiled with Erlang/OTP 26)

also on 1.15.7

Operating system

any

Current behavior

When Code.Fragment.surround_context is called on elixir alias and the cursor is on the last position it returns inconsistent results leading to confusing behaviour. See original report in elixir-lsp/elixir-ls#1027

Here are all cases found by @jaminthorns

Naked alias: all good

iex(1)> Code.Fragment.surround_context("Test", {1, 1})
%{context: {:alias, ~c"Test"}, begin: {1, 1}, end: {1, 5}}
iex(2)> Code.Fragment.surround_context("Test", {1, 5})
%{context: {:alias, ~c"Test"}, begin: {1, 1}, end: {1, 5}}

Remote call - inconsistent

iex(1)> Code.Fragment.surround_context("Test.test()", {1, 1})
%{context: {:alias, ~c"Test"}, begin: {1, 1}, end: {1, 5}}
iex(2)> Code.Fragment.surround_context("Test.test()", {1, 5})
%{context: {:dot, {:alias, ~c"Test"}, ~c"test"}, begin: {1, 1}, end: {1, 10}}

Capture - inconsistent

iex(2)> Code.Fragment.surround_context("&Test.test/0", {1, 2})
%{context: {:alias, ~c"Test"}, begin: {1, 2}, end: {1, 6}}
iex(4)> Code.Fragment.surround_context("&Test.test/0", {1, 6})
%{context: {:dot, {:alias, ~c"Test"}, ~c"test"}, begin: {1, 2}, end: {1, 11}}

Followed by comma - inconsistent

iex(3)> Code.Fragment.surround_context("[Test, :abc]", {1, 2})
%{context: {:alias, ~c"Test"}, begin: {1, 2}, end: {1, 6}}
iex(4)> Code.Fragment.surround_context("[Test, :abc]", {1, 6})
:none

Followed by ) - inconsistent

iex(6)> Code.Fragment.surround_context("List.wrap(Test)", {1, 11})
%{context: {:alias, ~c"Test"}, begin: {1, 11}, end: {1, 15}}
iex(7)> Code.Fragment.surround_context("List.wrap(Test)", {1, 15})
:none

It seems a65dae9 released in 1.15.1 fixed the problem only in a few cases

Expected behavior

Consistently returning module on last character position

@josevalim
Copy link
Member

The question is: when the alias finishes and when the dot starts? I am pretty sure I checked how intellisense/mouse over works in editors at the time and, if you had the cursor just before the dot (which is after the alias), then it was considered part of the dot. The commit you linked has a comment about this.

So I think we can improve the last two cases. I am not sure that's true for the dot ones.

@lukaszsamson
Copy link
Contributor Author

The question is: when the alias finishes and when the dot starts?

I know that it's blurry. I'd say if the cursor is on the left side of the dot then the left side should get the focus. Especially if calling surround_context shifted to the left will return a range containing the current position.
When I call it on {1, 4}, I get a range {1, 1} - {1, 4}, then I expect that on {1, 5} I'll get the same range.

Alternatively, I can workaround this issue by calling surround_context twice - first for original {1, 5} and then {1, 4} and choosing more suitable result.

@josevalim
Copy link
Member

When I checked, if we adopt the behavior you propose, then midway through the dot is still going to show the alias, and I don’t think that’s accurate.

The word Test has four columns, 1 2 3 and 4. I am ok with adding a fifth column if there is nothing meaningful after but I don’t think it should glob the dot. I am almost thinking that reverting the earlier commit may even be better.

@lukaszsamson
Copy link
Contributor Author

I am almost thinking that reverting the earlier commit may even be better

Agreed. Either we make it consistent or bring back the previous behavior.

@josevalim
Copy link
Member

The counter argument to reverting is that we should consider opening parens or brackets part of the identifier. So we either revert the behavior for spaces and commas or make aliases consistent in behavior and commas and brackets.

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

No branches or pull requests

2 participants