elixir-lsp / vscode-elixir-ls

Elixir language support and debugger for VS Code, powered by ElixirLS.
https://marketplace.visualstudio.com/items?itemName=JakeBecker.elixir-ls
MIT License
545 stars 105 forks source link

ElixirLS v0.17.1 in VSCode significantly changes token scopes, affecting syntax highlighting #385

Closed bceskavich closed 11 months ago

bceskavich commented 1 year ago

Environment

Current behavior

The latest release of ElixirLS for VSCode seems to remove a number of token scopes for Elixir, which in turn significantly changes the appearance of multiple syntax highlighting themes. The example below shows the following cases:

I am the author of the Dracula At Night VSCode theme, and the examples below use this theme. However, I've verified that this issue affects other popular themes as well (e.g. Cobalt2).

<= v0.17.0 v0.17.1
image image

Expected behavior

Consistent token scopes and syntax highlighting across versions.

lukaszsamson commented 1 year ago

Yes, there has been some changes aiming to correct issues in the grammar

Differentiation between module definition + module alias usage has disappeared

Should there be difference? In elixir AST both are the same. note {:__aliases__, [line: _], [:Other, :Asdf]} in

iex(4)> Code.string_to_quoted("""
...(4)> defmodule Other.Asdf do
...(4)>   def a do
...(4)>     Other.Asdf.asd()
...(4)>   end
...(4)> end
...(4)> """)
{:ok,
 {:defmodule, [line: 1],
  [
    {:__aliases__, [line: 1], [:Other, :Asdf]},
    [
      do: {:def, [line: 2],
       [
         {:a, [line: 2], nil},
         [
           do: {{:., [line: 3],
             [{:__aliases__, [line: 3], [:Other, :Asdf]}, :asd]}, [line: 3], []}
         ]
       ]}
    ]
  ]}}

anyway, the difference is still there usage in module is marked with scope meta.module.elixir

Screenshot 2023-10-17 at 16 23 37

other usages does not have that scope

Screenshot 2023-10-17 at 16 23 53

Identification of module attributes as distinct entities has disappeared

Screenshot 2023-10-17 at 16 27 08

Do you mean that the scope has changed from variable.other.readwrite.module.elixir to more standard variable.other.constant.elixir? There were no entity.name. scope on module attributes.

Expected behavior Consistent token scopes and syntax highlighting across versions.

I cannot evolve the grammar without making some changes

bceskavich commented 1 year ago

Thanks for the fast response @lukaszsamson!

Also, thanks for clarifying both of the items I highlighted. The decision to highlight module alias definitions vs usages differently is definitely a choice I've made with my theme, however one that is a common across other themes. Regarding the module attribute change, I seem to have misread the change in scopes and see that there's still a distinction there I can use to highlight those attributes as desired.

Lastly, while I understand the desire to make the grammar here more correct, to me these updates feel like a breaking change that went unannounced in a patch version release. At the very least it would be helpful in the future if such changes were documented and part of a minor (not patch) release.

In addition, I would advocate for considering significant changes in the token identification grammar as a breaking change in the future. There are thousands of users of this VSCode plugin who are suddenly experiencing a drastic change in appearance of Elixir code across multiple themes as a result of this update. And for those who aren't theme maintainers they may be searching in confusion to understand why this happened (i.e. is this the theme they use? is it their VSCode version? is it the LS plugin?).

lukaszsamson commented 1 year ago

The question now is if the current rules allow you to make your theme work as you wish or do you need some changes? Which areas could be improved?

Lastly, while I understand the desire to make the grammar here more correct, to me these updates feel like a breaking change that went unannounced in a patch version release. At the very least it would be helpful in the future if such changes were documented and part of a minor (not patch) release.

I understand that it can be frustrating, especially given that some of the changes was unintended (https://github.com/elixir-lsp/vscode-elixir-ls/issues/382). Grammar changes was mentioned in v0.17.0 and v0.17.1 release notes https://github.com/elixir-lsp/vscode-elixir-ls/blob/master/CHANGELOG.md. Besides 0.x in SemVer does not guarantee no breaking changes.

In addition, I would advocate for considering significant changes in the token identification grammar as a breaking change in the future

Thanks. I now know that more cere is needed here. I'm afraid there will be more changes in the future. Elixir is notoriously difficult to parse with textmate grammars and LSP symantic tokens seem to be the solution

bceskavich commented 1 year ago

Grammar changes was mentioned in v0.17.0 and v0.17.1 release notes

Ah, I'm sorry @lukaszsamson, I missed this and was looking in the wrong repo.

The question now is if the current rules allow you to make your theme work as you wish or do you need some changes? Which areas could be improved?

I haven't yet had time to update my theme package to work with the latest rules. I will let you know if I run into any challenges when I do so.

I now know that more cere is needed here. I'm afraid there will be more changes in the future. Elixir is notoriously difficult to parse with textmate grammars and LSP symantic tokens seem to be the solution

Noted, and understood!

jbosse commented 1 year ago

In addition, I would advocate for considering significant changes in the token identification grammar as a breaking change in the future. There are thousands of users of this VSCode plugin who are suddenly experiencing a drastic change in appearance of Elixir code across multiple themes as a result of this update. And for those who aren't theme maintainers they may be searching in confusion to understand why this happened (i.e. is this the theme they use? is it their VSCode version? is it the LS plugin?)

FWIW I would place myself in this user category. I was very confused as to what happened to my syntax highlighting. I honestly don't think there is anything @lukaszsamson could have done to avoid my confusion. My extensions auto-update and I rarely look are release notes. Even a note like "updated text mate grammar" would have gone over my head.

There are many developers like me who I would categorize "Line of Business" developers that don't really have a mental model on how our IDE does the magical things it does and don't know where to begin investigating when something odd happens.

On the bright side, this whole thing has taught me about how my editor themes work and how I can tweak them to make them even more to my taste.

seanhuggins1 commented 1 year ago

Yeah - folks at my work started noticing changes in our theme this week and we traced it to changes in this extension. Installing an older version is our current workaround. It's really wild how much differences in your theme affect readability!

lukaszsamson commented 11 months ago

Closing this for now. Please open new issues if you have concrete proposals. BTW I started work on semantic token provider. Probably it won't be ready for v0.18