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

Latest nightly seems to ignore semantic highlighting settings #6267

Closed
aloucks opened this issue Oct 17, 2020 · 33 comments · Fixed by #6984
Closed

Latest nightly seems to ignore semantic highlighting settings #6267

aloucks opened this issue Oct 17, 2020 · 33 comments · Fixed by #6984
Labels
S-actionable Someone could pick this issue up and work on it right now

Comments

@aloucks
Copy link
Contributor

aloucks commented Oct 17, 2020

I have semantic highlight disabled via the configuration below. The latest nightly seems to ignore it now and always applies semantic highlighting. I've also tried to disable it via the global Editor -> Semantic Highlighting: Enabled setting.

Disabling of semantic highlighting worked as expected prior to rust-analyzer version: nightly (f0412da).

   "editor.semanticTokenColorCustomizations": {
        "[Atom One Dark]": {
            "enabled": false,
        }
    },

EDIT: Per conversation below, the highlighting differences are attributed to a newly introduced grammar.

@bjorn3
Copy link
Member

bjorn3 commented Oct 17, 2020

Try "editor.semanticHighlighting.enabled": false.

@aloucks
Copy link
Contributor Author

aloucks commented Oct 17, 2020

Try "editor.semanticHighlighting.enabled": false.

I've tried that but it has no effect :(

As mentioned above, this all worked as expected prior to today (2020-10-17).

@autumnontape
Copy link

With the new update, I'm having the same problem. I've had "editor.semanticHighlighting.enabled": false in my settings.json to disable semantic highlighting for a while now, and today semantic highlighting unexpectedly turned back on. Adding editor.semanticTokenColorCustomizations has no effect.

@autumnontape
Copy link

It's possible that what I'm observing is on account of the new TextMate grammar (49d824a) rather than semantic highlighting being turned on. I don't really know what difference semantic highlighting makes, I just had it turned off because I didn't like having so much of my code colored like functions.

@Veykril
Copy link
Member

Veykril commented Oct 20, 2020

I would believe this is due to the new textmate grammar if anything as using "editor.semanticHighlighting.enabled": false at least on current nightly properly disables semantic highlighting for me. So my guess is that the new textmate grammar has some scopes that are somewhat different to the previous one.

@autumnontape
Copy link

Is there any way I can use vscode's built-in Rust grammar instead of the one that comes with this extension?

@Veykril
Copy link
Member

Veykril commented Oct 20, 2020

Also one way to check whether semantic highlighting is truly active or not is to use the Developr: Inspect Editor Tokens and Scopes action in VSCode. If it only shows textmate scopes for items like function names and the like it is definitely disabled.
With semantic highlighting enabled:
Code_VV295ByJZR
And with it disabled:
Code_whpk2B4eA9

@autumnontape
Copy link

@Veykril When I do that, I don't see any semantic token type listing. So it looks like my editor, at least, doesn't have semantic highlighting enabled. The new grammar just makes it look similar to the way it looks when it is enabled.

@Veykril
Copy link
Member

Veykril commented Oct 20, 2020

I don't think there is a way to specifically disable/replace a textmate grammar given by an extension unfortunately, I at least don't see/find a way.

What exactly is the problem with the new grammar though or rather what is dissatisfying? I use semantic highlighting so I dont know what changed but judging from this Issue here I suppose it's not using the scopes people would expect.

@autumnontape
Copy link

@Veykril Here's an example of what a couple pieces of the same file look like in my editor:

With the old rust-analyzer grammar

screenshot of code

screenshot of code

With the new rust-analyzer grammar

screenshot of code

screenshot of code

I like some of the changes. A lot of punctuation went from the default color to being colored like & or +, which is very helpful, especially with dollar signs in macro definitions.

The biggest problem for me is that so much stuff is green now!! I'm used to green being used in a few places, mostly for functions, but now it's used for modules (most of the time), enum variants (including Some/None/Ok/Err), type parameters, all type names, and macro_rules. This is way too much, I want the most common color to be the default color. I also don't like having imports colored differently from each other, except idents with special meaning in use declarations, like self.

Highlighting of attributes has also changed in a way that doesn't make sense to me. #[derive(Copy, Clone)] used to have derive colored green. Now derive is the default color, and Copy and Clone are green. The path is the most important part of an attribute, so I appreciated having only that part colored differently.

And this is probably my most subjective issue, but I liked having different colors for identifiers from the prelude, like From, Some, None, etc.

There are other things that make the change jarring (let being a different color from mut is the main one), but they're understandable differences that I wouldn't mind getting used to.

@bjorn3
Copy link
Member

bjorn3 commented Oct 20, 2020

let being a different color from mut is the main one

let is storage.type.rust while mut is storage.modifier.mut.rust. These are the correct textmate scopes.

Highlighting of attributes has also changed in a way that doesn't make sense to me. #[derive(Copy, Clone)] used to have derive colored green. Now derive is the default color, and Copy and Clone are green.

derive is missing. Copy and Clone get interprwted as types and thus become entity.name.type.rust.

The biggest problem for me is that so much stuff is green now!! I'm used to green being used in a few places, mostly for functions, but now it's used for modules (most of the time), enum variants (including Some/None/Ok/Err), type parameters, all type names, and macro_rules. This is way too much, I want the most common color to be the default color.

That is something the theme decides, not the grammar. The mere task of the grammar is to categorize tokens. It is the theme that decides how it should then look.

I also don't like having imports colored differently from each other, except idents with special meaning in use declarations, like self.

Names starting with a capital letter are assumed to be types and thus get entity.name.type.rust for use statements lowercase names seem to get no scope from looking at the textmate grammar. (oversight?)

@autumnontape
Copy link

All I see is what colors the editor chooses, and all of the problems I have seem to be similar in all of the built-in color themes. The only difference is that some themes distinguish between "types" and "functions," causing enum variants to look different depending on what kind of fields they have, which is even worse!

I'm sure the new scopes would be very helpful for a theme built specifically for Rust, but I'd rather use the default grammar because of how it looks.

@bjorn3
Copy link
Member

bjorn3 commented Oct 20, 2020

Enum variants should always start with a capital first letter. (There is a lint for that) This means they should always get the type textmate scope.

@autumnontape
Copy link

Tuple variants get the function scope.

@bjorn3
Copy link
Member

bjorn3 commented Oct 20, 2020

Interesting. It seems that the enum rule only matches enum Name and interprets the part between { and } as if it isn't an enum declaration. This means that tuple variants get interpreted as function calls.

@benesch
Copy link

benesch commented Oct 21, 2020

I tried to write an extension to override this (https://marketplace.visualstudio.com/items?itemName=benesch.legacy-rust-syntax) but unfortunately it seems to be impossible to override the textmate grammar that ships with rust-analyzer. At least, I don't know how to do it.

I'd like to propose that rust-analyzer not ship a Rust grammar at all, besides the semantic one. That would allow folks to choose the grammar they want by either installing or not installing @dustypomerleau's Rust Syntax extension. Upstream VSCode seems likely to adopt that as the standard Rust grammar, too. (And if they do, then the extension I created should work to undo that.)

benesch added a commit to benesch/rust-analyzer that referenced this issue Oct 21, 2020
This allows folks to install a different extension to supply the Rust
grammar of their choice. Upstream VSCode seems likely to incorporate
the grammar that is currently bundled with rust-analzyer as the default
grammar anyway.

Fix rust-lang#6267.
@benesch
Copy link

benesch commented Oct 21, 2020

Whipped that proposal up as a PR here: #6318

@dustypomerleau
Copy link
Contributor

  • I am happy to remove the grammar from Rust Analyzer if the community prefers the old grammar and/or the core team is finding it burdensome to have these grammar-related issues on the repo. I don't want it to be a distraction. In my original PR, shipping no grammar was one of the considered options. I'm getting a lot of useful feedback from shipping it here, which will make it better if VS Code adopts it, but again, that adoption should only happen if the community wants it.

  • As Bjorn has already pointed out, there is a big difference between not liking the color of a token, and not liking the scope. Feedback on the scopes is something I can action. For example, some themes use a separate color for storage and keyword, but others use the same color. The colors are easily overridden, but the scopes are not.

  • Some of the edge cases we're finding are also broken in the old grammar, so would need to be fixed in either case. For example, we fixed run-on raw strings in the new grammar, but in the old grammar they still look like this:

Screen Shot 2020-10-22 at 18 59 14

  • Attributes are a good example of something the old grammar treated as a single scope (allowing only strings). For example, all of:

    #[derive(Debug, Clone, Logos)]

receives meta.attribute.rust in the old grammar. Neither the function scope or the trait scopes are allowed inside the attribute. I made the decision to allow the trait scopes through, but if there is broad agreement that it would be better to allow the function scope and not the trait scope, these are easy things to change. I think it's visually helpful to have some scopes visible in attributes

  • Some of the issues on the new grammar are related to scoping things that had no scope at all in the old grammar. For example, raw identifiers are simply treated as source.rust, and no theming can be applied. The new grammar caught the keywords, which made the raw identifiers look incorrect, but also brought them to my attention, so that we can add scopes. Whether that's helpful or just annoying depends on whether you have an interest in theming them, I suppose.

  • Some of the issues in both the old grammar and the new grammar are just limitations of regex-based systems, which is why we have semantic highlighting. For example, tuple structs can be easily themed at the time of definition, but when you create an instance it's nearly impossible to distinguish this from a function call, because some functions start with capital letters:

Old grammar:

Screen Shot 2020-10-22 at 19 18 25

New grammar:

Screen Shot 2020-10-22 at 19 19 00

I can do things like extending the meta scope all the way to the closing } of a struct, but this only helps with the scoping of fields inside the declaration. If you refer to the fields outside the struct it won't necessarily be possible to theme them the same.

In any event, I'm flexible on whether to keep it in the repo. I appreciate all the feedback either way. @matklad @bjorn3 @lnicola

@matklad
Copy link
Member

matklad commented Oct 22, 2020

@dustypomerleau as long as you have desire and capacity for fixing the issues as they come up and coordinating with upstream (VS Code) regarding the inclusion of the grammar as the standard one, I think it will be highly beneficial to ship this grammar with rust-analyzer.

We are seeing quite a few reports about it, but this is a feature: it's much better to debug grammar while it is in rust-analyzer with a weekly release cycle, than when it is in VS Code.

When VS Code ships, it would make sense to remove grammar from rust-analyzer, but not before that.

Let me grant you triage rights and r+ on this repo, so that it's easier for you to make grammar changes and fix issues... Done!

@lnicola
Copy link
Member

lnicola commented Oct 22, 2020

I agree. Letting your grammar bake in rust-analyzer seems to have uncovered a lot of issues, so I think it's been really beneficial.

Even if it gets merged in Code, we could keep it as a more up-to-date version (if you think it's still possible to improve it further). As for the issue of not being able to override it, IIRC matklad used to have a very bare-bones grammar, so maybe there's still a way?

@matklad
Copy link
Member

matklad commented Oct 22, 2020

Long-term, I think it would be better to ship nothing -- if we ship a grammar, a user can't override it with a custom one.

So, sadly, my favorite Rust textmate grammar doesn't work with current ra :)

@lnicola
Copy link
Member

lnicola commented Oct 22, 2020

I think it's safe to remove the attribution comment 😄.

@autumnontape
Copy link

If you keep a grammar in the extension, you should probably communicate to users in some way that there's a custom grammar included and no known way to override it. I only came here because I thought what I was seeing was related to semantic highlighting.

@benesch
Copy link

benesch commented Oct 22, 2020

if we ship a grammar, a user can't override it with a custom one.

Yeah, unfortunately I just can't use rust-analyzer until this grammar is removed. I know I'm probably weirdly particular, but I can't deal with the new grammar, and no amount of tweaks will change that.

Are there any hacks we can use in the meantime to override the grammar? I mean, there must be some sort of extension load order that determines what extension gets to provide the Rust grammar, right?

@autumnontape
Copy link

@benesch You could fiddle with your editor.tokenColorCustomizations setting until it looks the way you want it again. That's what I'm planning to do.

@matklad
Copy link
Member

matklad commented Oct 22, 2020

@benesch there are several options:

  • you can revert to an older version of the extension
  • you can build the extension from source with grammar removed cargo xtask install --client=code
  • rust-analyzer also explicitly encourages forks, so you are free to publish an alternative extension to crates.io

@aloucks
Copy link
Contributor Author

aloucks commented Oct 22, 2020

Could the new grammar be moved to a separate extension so that it can be iterated on independently?

@matklad
Copy link
Member

matklad commented Oct 22, 2020

Yes, it could be moved to a separate extension, but I believe that would be less efficient way to arrive at the worse grammar.

@benesch
Copy link

benesch commented Oct 22, 2020

@benesch there are several options:

  • you can revert to an older version of the extension
  • you can build the extension from source with grammar removed cargo xtask install --client=code
  • rust-analyzer also explicitly encourages forks, so you are free to publish an alternative extension to crates.io

Thanks for the suggestions. Those are all reasonable, but I'm on the hunt for a solution that allows me to continue to use the latest and greatest rust-analyzer without too much work on my end.

So here's the hack that seems to be working for me:

curl https://raw.githubusercontent.com/microsoft/vscode/28b6143cb20edb22712acb45eee32c910ea45172/extensions/rust/syntaxes/rust.tmLanguage.json > ~/.vscode/extensions/matklad.rust-analyzer-0.2.368/rust.tmGrammar.json 

If you're using VSCode Remote like I am, you can tweak the command slightly like so and run it on the remote machine:

curl https://raw.githubusercontent.com/microsoft/vscode/28b6143cb20edb22712acb45eee32c910ea45172/extensions/rust/syntaxes/rust.tmLanguage.json > ~/.vscode-server/extensions/matklad.rust-analyzer-0.2.368/rust.tmGrammar.json 

That just in-place overwrites the grammar that ships with rust-analyzer with the old grammar that ships with VSCode today. I imagine I'll have to run that command whenever rust-analyzer is auto-updated to a new version, but I can live with that.

Also, I'm sorry to be a downer! I'm sure this new grammar is great for most folks, and it's awesome that you've spent so much time improving it, @dustypomerleau. I just have my development environment set up just so and I hate change, I guess.

@benesch
Copy link

benesch commented Nov 2, 2020

For those following along, microsoft/vscode#108254 was just merged. The November edition of VSCode is likely to include the new syntax highlighting.

@flodiebold
Copy link
Member

Is there anything remaining to do on this issue? @matklad

@lnicola
Copy link
Member

lnicola commented Dec 21, 2020

I think we can still remove the TextMate grammar (to use the upstream one instead).

@flodiebold flodiebold added the S-actionable Someone could pick this issue up and work on it right now label Dec 21, 2020
@bors bors bot closed this as completed in 61711d9 Dec 22, 2020
@benesch
Copy link

benesch commented Dec 28, 2020

Since rust-analyzer no longer ships a grammar, it's now possible for another extension to supply one! I've released "Legacy Rust Syntax" (https://marketplace.visualstudio.com/items?itemName=benesch.legacy-rust-syntax) for any other weirdos like me who want to stick with the old sort-of broken grammar. It should just work now, if you're on the most recent versions of VSCode and rust-analyzer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-actionable Someone could pick this issue up and work on it right now
Projects
None yet
9 participants