Add _ to unused variable code action not working properly

[bigardone]

Environment

• Elixir & Erlang versions (elixir --version):

Erlang/OTP 25 [erts-13.0] [source] [64-bit] [smp:12:12] [ds:12:12:10] [async-threads:1] [jit:ns]

Elixir 1.14.0 (compiled with Erlang/OTP 23)

• Elixir Language Server version:

06259977458a48bd106402f6a6d6e1f222991337

• Operating system:

macOs Monterey

• Editor or IDE name (e.g. Emacs/VSCode):

NeoVim

• Editor Plugin/LSP Client name and version:

CoC

Current behavior

Where there is an unused in a function signature, and I try to apply the Add '_' to unused variable code action, the _ gets added in the wrong position depending on how the function is formatted.

With oneliner function definitions, it adds the _ to the function's name:

With maps:

Apparently, it adds the _ to the beginning of the line rather than to the beginning of the unused variable.

## Log of coc.nvim

2022-10-24T07:25:03.692 INFO (pid:4514) [services] - registered service "languageserver.elixirLS"
2022-10-24T07:25:03.694 INFO (pid:4514) [services] - registered service "languageserver.elmLS"
2022-10-24T07:25:03.694 INFO (pid:4514) [services] - registered service "languageserver.golangLS"
2022-10-24T07:25:03.723 INFO (pid:4514) [services] - registered service "highlight"
2022-10-24T07:25:03.767 INFO (pid:4514) [plugin] - coc.nvim initialized with node: v14.16.1 after 155ms
2022-10-24T07:25:03.833 INFO (pid:4514) [services] - LanguageClient highlight server state change: starting => running
2022-10-24T07:25:05.891 INFO (pid:4514) [services] - LanguageClient elixirLS state change: stopped => starting
2022-10-24T07:25:05.905 INFO (pid:4514) [language-client-index] - Language server "languageserver.elixirLS" started with 4578
2022-10-24T07:25:07.170 INFO (pid:4514) [services] - LanguageClient elixirLS state change: starting => running
2022-10-24T07:25:07.172 INFO (pid:4514) [services] - service languageserver.elixirLS started
2022-10-24T07:25:54.488 INFO (pid:4514) [attach] - receive notification: codeAction [ 'currline' ]
2022-10-24T07:25:56.193 WARN (pid:4514) [plugin] - Slow action "codeAction" cost 1704ms
2022-10-24T07:26:35.132 INFO (pid:4514) [attach] - receive notification: codeAction [ 'currline' ]
2022-10-24T07:26:36.485 WARN (pid:4514) [plugin] - Slow action "codeAction" cost 1352ms
2022-10-24T07:28:19.679 INFO (pid:4514) [attach] - receive notification: codeAction [ 'currline' ]
2022-10-24T07:28:21.907 WARN (pid:4514) [plugin] - Slow action "codeAction" cost 2228ms
2022-10-24T07:30:11.503 INFO (pid:4514) [attach] - receive notification: codeAction [ 'currline' ]
2022-10-24T07:30:42.012 WARN (pid:4514) [plugin] - Slow action "codeAction" cost 30509ms
2022-10-24T07:32:23.733 WARN (pid:4514) [completion] - Suggest not triggered with input "", minimal trigger input length: 1
2022-10-24T07:32:29.781 WARN (pid:4514) [completion] - Suggest not triggered with input "", minimal trigger input length: 1
2022-10-24T07:32:30.789 INFO (pid:4514) [attach] - Request action: doKeymap [ 'iPg==0' ]
2022-10-24T07:32:30.821 WARN (pid:4514) [completion] - Suggest not triggered with input "", minimal trigger input length: 1
2022-10-24T07:32:32.514 WARN (pid:4514) [completion] - Suggest not triggered with input "", minimal trigger input length: 1
2022-10-24T07:32:38.138 WARN (pid:4514) [completion] - Suggest not triggered with input "", minimal trigger input length: 1
2022-10-24T07:32:38.618 INFO (pid:4514) [attach] - Request action: doKeymap [ 'iPg==0' ]
2022-10-24T07:32:38.635 WARN (pid:4514) [completion] - Suggest not triggered with input "", minimal trigger input length: 1
2022-10-24T07:32:53.829 INFO (pid:4514) [attach] - receive notification: codeAction [ 'currline' ]
2022-10-24T07:32:54.951 WARN (pid:4514) [plugin] - Slow action "codeAction" cost 1121ms
2022-10-24T07:33:38.884 INFO (pid:4514) [attach] - receive notification: codeAction [ 'currline' ]
2022-10-24T07:33:56.425 WARN (pid:4514) [plugin] - Slow action "codeAction" cost 17541ms
2022-10-24T07:35:31.360 INFO (pid:4514) [attach] - receive notification: codeAction [ 'currline' ]
2022-10-24T07:35:34.172 WARN (pid:4514) [plugin] - Slow action "codeAction" cost 2811ms
2022-10-24T07:35:44.376 INFO (pid:4514) [attach] - receive notification: showInfo []

Expected behavior

I should add the _ to the beginning of the unused variable name.

[scohen]

@lukaszsamson I looked into this, the code action cannot work as written; it uses the diagnostic start and end as the basis for the edit, and the diagnostic is just for pointing the user's attention to a specific line. The character can and is often 0. Furthermore, the action as written only accounts for the simplest variables of the form foo = bar. It fails for parameters (it leaves a comma), it fails for pattern matches like %User{} = steve (it puts the comma at the beginning of the match, rather than at the end.

The current implementation is fatally flawed and cannot work, nor can it be made to work without a complete rewrite that actually looks at the syntax of the expression in question. I suggest removing the commits.

[lukaszsamson]

Code actions was never high on my priority list so haven't tested this too much. I wouldn't immediately revert it as this was highly asked for. Can you take a look @lucacervello? To make this work more reliably we need to make the edits on AST.

I will need to take a closer look on positions returned from elixir compiler. Some fixes may be neede upstream

[scohen]

I wouldn't immediately revert it as this was highly asked for.

I would because it simply doesn't work in all but the simplest cases and breaks code.

I've added a replacement code action that uses rewriting to handle the other cases here, you can see the different approach and the unit tests.

https://github.com/elixir-lsp/elixir-ls/pull/773/files#diff-f6861fbf287eade6a55a23d8b5a2d9e0222aa140d08aea5fb54d6ef4a55f06d6

I will need to take a closer look on positions returned from elixir compiler. Some fixes may be needed upstream

The compiler should only ever be a guide for where problems occur, Even if we had proper lines and columns, the approach above is still flawed and will remain flawed. Code modification cannot ever be text-based, there's just too much variability in elixir code. Personally, I don't think the compiler needs to change in this circumstance.

[sheldak]

It turns out I was working on the fix parallel to you. I've created a simple solution that you may consider (#778). It should work in most real-life cases and not break the code in the other ones.

[lucacervello]

@lukaszsamson In my PR I pointed out that this code action wasn't working well, I didn't find a good solution to edit code in elixir-ls.
Anyway I would keep the code actions support which enable us to add code action.
If we want to remove the prefix_with_underscore code action is fine with me.

[scohen]

@lukaszsamson
This is a fantastic time to draw a line in the sand and say "here's the future direction of the project". The options before us are to use the code_mod approach in the experimental branch here, or continue with the text based approach to code modification as outlined in the two prior code action PRs.

I believe that continuing down the current path will be a poor technical decision for the project in general, and represents a serious problem for any refactoring that I have already done actually landing in the project.

I see the following technical problems with the current implementation of code actions:

• It is harder to guarantee that text-based code modifications do what they intend to do. The current examples are about as simple as code modification gets, but each one has had serious bugs. Can you imagine what "organize aliases" would look like under a text-based system?
• Adding to that, the present system is massively harder to test (for example, look at the paucity of tests in the initial PR, and compare this to the number of lines of code it took to write them. Now contrast the wealth of tests in the new way, and how little code it takes to make each happen.. Also contrast how easy it is to verify if the tests are indeed correct in the new way vs. the old way (are you sure the edit begins on character 19? How do you know?). Now imagine this being multiplied over the number of things elixir_ls needs to do that modify code.
• The present system freely mixes concerns around the protocol with concerns around code modification, while the new system keeps them separated. This means that tests for code modification test just that part and the rest of the system handles the protocol and conversions of ranges and positions.
• It's extremely easy for the present system to produce incorrect results. In fact, I'd go so far as to say the current method produces errors with multibyte characters by default, and none of the code in the code actions PRs will produce valid ranges for a line with a multibyte character. In order to fix this, every single range and position needs to be audited and changed by hand, on every PR you review.
• Even with all of the above, the current system takes many more lines of code to do the same thing when compared to the new system. The code modification of adding underscores is 20 lines of can't fail AST rewriting in the new system.
• The new system leverages reusable modules (like SourceFile and Diff) that not only aid the developer in writing such tooling, but perform better and are better tested, and prevent needless allocations and iteration.
• People emulate behavior they see, and more people write code against this approach, pushing the project further down the path of code that's difficult to change, maintain and test. This will perennially relegate the experimental branch to playing catch up while feature development continues in the main branch.
• The code actions module will be come increasingly bloated as more and more are added.

In contrast, saying "all future code modification happens in the experimental server" means that

• There will be a reason for people to enable and test it
• Developers of elixir_ls will have an easier time building and testing things that modify code.
• Having effective, simple to understand patterns for doing code modification means developers will not be scared away from adding things, and reviewers will have an easier time ensuring the code is valid.
• Code modification will be able to do so much more because it will be able to share the tools that have been built to make it happen. It's worth noting that there are no reusable modules introduced in the current code_actions code.
• You will be able to release code with more confidence because so many of these concerns are handled by the system and not placed in the hands of contributors.

To sum up, what I'm asking is to remove code actions in their current form, to land the 3 PRs that have been sitting open for weeks (and don't affect the rest of the system) and direct people that would like to add functionality to elixir_ls to do it under the experimental server. Then, we can have it take over things (like code actions, which it already handles) and people who want the new functionality can then enable the new code via a config option.

I'd be curious to see what @sheldak thinks of the new approach too, he's been expending a lot of effort to write and test code actions. I'm always impressed when people jump through hoops to scratch an itch they have.

[lukaszsamson]

@scohen You don't have to convince me that AST transformations are the right way to go. I simply wanted to see if the original author or some other contributor is willing to take up the mantle and implement it correctly.
Regarding your implementation, we'll need to make sure we use Code.string_to_quoted_with_comments. Otherwise comment nodes are stripped from AST. Unfortunately that's elixir 1.13+

[scohen]

@lukaszsamson I think the long comment obscured the point i was trying to make, only part of which is that AST transforms are the way forward here.

So let me clarify. I think that keeping string based modifications in the project at all is the mistake. Leaving them in will encourage more people to adopt that methodology, because people extend what's already there, as it's the path of least resistance. This will prevent the eventual AST based modifications from being tested or merged. You can see that we've had two additions to code actions since they were merged, and both follow the approach already taken.

My take, and please correct me if I'm wrong, is that there's no way to implement this correctly using string-based source code modification in the current project without it being a massive performance problem. While @sheldak's bug fix indeed fixes some of the bugs present in the initial implementation, it doesn't account for multibyte characters on the line and will break in subtle ways if they're encountered.

What I'm trying to prevent here is a pile-on of features that use dead-end techniques.

[sheldak]

@scohen I agree that working on AST should be the future of code actions. I created two PRs just before realizing there is all that work on refactoring code actions and rewriting strings in these particular cases seemed like a decent solution then.

Maybe I will also add some background from me: I'm working in Qarma and we would like to have a few more code action features, so I'll be working part-time on elixir-ls for some time. Hopefully, we will be able to incorporate these features into the official project if @lukaszsamson finds them useful. I can surely work on top of @scohen's code actions framework. I just need to know where can I extend code actions safely (if anywhere).

[scohen]

@sheldak I'm trying to do a couple of things here. The first is to outline a hopefully better approach to code modification. The second is to funnel some work to the experimental branches so more people try them out so we can improve the developer experience and get more eyes on the experimental server. Lastly, I'm trying to prevent work from happening on a technology that won't be used going forward.

I think it's the last thing that I'm having a hard time getting across. I'm of the opinion that text-based code-modification is actively harmful to the project's long term health, and I think they should be removed from it right now. They represent an incorrect way to solve the problem, have serious bugs, and are causing real-world pain right now. I'm pretty surprised that they haven't been removed yet and a point-release added to address them. I'm pretty active on the elixir discord, and we have had pretty consistent complaints about elixir-ls since 0.12. I've been working with it and it burns me multiple times every day (And I have fixed code actions!). I don't like to be critical in public, but this is not a good developer experience. We need to address this now.

[lukaszsamson]

Closing this as the broken implementation has been removed