Theme loading and "editor.tokenColorCustomizations" support.

[seivan]

Fixes: Issue#1294

TODO:

• Load themes
• Load existing ralsp-prefixed overrides from "workbench.colorCustomizations".
• Load overrides from "editor.tokenColorCustomizations.textMateRules".
• Use RA tags to load vscode.DecorationRenderOptions (colors) from theme & overrides.
• Map RA tags to common TextMate scopes before loading colors.
• Add default scope mappings in extension.
• Cache mappings between settings updates.
• Add scope mapping configuration manifest in package.json
• Load configurable scope mappings from settings.
• Load JSON Scheme for text mate scope rules in settings.
• Update Readme.

Borrowed the theme loading (scopes.ts) from Tree Sitter with some modifications to reading "editor.tokenColorCustomizations" for merging with loaded themes and had to remove the async portions to be able to load it from settings updates.

Just a PoC and an idea I toyed around with a lot of room for improvement.
For starters, certain keywords aren't part of the standard TextMate grammar, so it still reads colors from the ralsp prefixed values in "workbench.colorCustomizations".

But I think there's more value making the extension work with existing themes by maping some of the decoration tags to existing key or keys.

These will merge with the default ones coming with the extension, so you don't have to implement all of them and works well with overrides defined in settings.

    "editor.tokenColorCustomizations": {
        "textMateRules": [
            {
                "scope": "keyword",
                "settings": {
                    "fontStyle": "bold",
                }
            },
        ]
    },

Edit: The idea is to work with 90% of the themes out there by working within existing scopes available that are generally styled. It's not to say I want to erase the custom Rust scopes - those should still remain and eventually worked into a custom grammar bundle for Rust specific themes that target those, I just want to make it work with generic themes offered on the market place for now.

A custom grammar bundle and themes for Rust specific scopes is out of... scope for this PR.
We'll make another round to tackle those issues.

Current fallbacks implemented

    [
        'comment',
        [
            'comment',
            'comment.block',
            'comment.line',
            'comment.block.documentation'
        ]
    ],
    ['string', ['string']],
    ['keyword', ['keyword']],
    ['keyword.control', ['keyword.control', 'keyword', 'keyword.other']],
    [
        'keyword.unsafe',
        ['storage.modifier', 'keyword.other', 'keyword.control', 'keyword']
    ],
    ['function', ['entity.name.function']],
    ['parameter', ['variable.parameter']],
    ['constant', ['constant', 'variable']],
    ['type', ['entity.name.type']],
    ['builtin', ['variable.language', 'support.type', 'support.type']],
    ['text', ['string', 'string.quoted', 'string.regexp']],
    ['attribute', ['keyword']],
    ['literal', ['string', 'string.quoted', 'string.regexp']],
    ['macro', ['support.other']],
    ['variable', ['variable']],
    ['variable.mut', ['variable', 'storage.modifier']],
    [
        'field',
        [
            'variable.object.property',
            'meta.field.declaration',
            'meta.definition.property',
            'variable.other'
        ]
    ],
    ['module', ['entity.name.section', 'entity.other']]

[matklad]

Hm, can with remove our own settings completely in favor of TextMate definitions?

I'd rather have one way to config the coloring, and I agree that theme files are better for this.

If the existing rust tm grammar does not contain tags that we need (like mutable variabels), should we ship our own .tm?

(Note: I haven't looked at the grammar files for VS Code at all, so I might be suggesting something stupid here)

[seivan]

@matklad Sure!

Tree Sitter ships their own TextMate grammar where they pattern match.
But honestly, very few themes will support custom Rust scopes.

My suggestion is to work within the existing scopes for those that are very Rust specific.

                decoration('keyword.unsafe'),
                decoration('builtin'),
                decoration('macro'),
                decoration('variable.mut', 'underline'),

Surely there's has to be a decent compromise for highlighting them with existing scopes.

I'm not 100% what Tree Sitter is doing here , but I know they also ship their own grammar files for each language they support.

[seivan]

@matklad I actually thought of a plan forward here so you can abandon the old settings.

So the issue is, some of the scopes aren't part of the general grammars. Not to mention many themes won't bother supplying styling for them.

Something like 'variable.mut' isn't going to be in a lot of user made themes not to mention isn't part of the regular grammars supplied.

So I was thinking of a mapper.
That apart from supplying a pre-configured default mapper, lets the user override the scopes.

"rust-analyzer.themingScopes" : {
  "keyword.unsafe" : "invalid.illegal", 
  "macro" : "meta", 
  "variable.mut" : "variable", 
  "builtin" : "support.type"
}

I am sure there are better scopes to map them to, but this is a start at least to get RA to work with most of the supplied themes on the VSCode extension marketplace.

[matklad]

LGTM!

Let's also add a short note here about how to configure colors now.

[seivan]

LGTM!

Let's also add a short note here about how to configure colors now.

Sure, but I wasn't... done. :)
It's still a work in progress, I just wanted some feedback on some of the ideas I had in mind to not.. half-ass it between the two current approaches.

Edit: Do'h, forgot about the readme. I thought I'd updated it once a decision has been made on how to move forward.

[matklad]

So I was thinking of a mapper.

Hm, I think this can backfire. Like, something like invalid.illegal sounds pretty inappropriate :) I think it's important that we preserve the ability to do custom things like variable.mut. It would be best if we can somehow define our own scopes, but, if that fails, just custom rust-specific override seettings seem ok

[seivan]

@matklad That was just an example. It doesn't have to backfire unless the user wants to.
I think to be safe, RA would pick sane standards.

variable -> variable
keyword.unsafe -> keyword.control

I agree that keeping the existing scopes is better, that's why I didn't suggest to change their names at all.
This is an intermediate step until a better solution comes up that allows custom tags/scopes to work with existing themes designed.

Edit: This together with "editor.tokenColorCustomizations" will allow you to actually decide what unsafe or mut looks like.
Apart from working out of the box with existing themes, let you actually set style that fits that context a little bit better.

[matklad]

The important thing here is that we support constructs that exist only in Rust, and not in other languages. In particular we should man if, while and firends to keyword.control, and for unsafe we really need a dedicated rust.unsafe tag (which, ideally, should fallback to keyword).

[seivan]

@matklad Yeah, and with the mapper, it can fallback to keyword out of the box.
If it's undefined, then it will be keyboard.unsafe and letting some theme pick it up.

But that's a bit more work with supplying a grammar bundle.
This is more intermediate that lets it work today.

[seivan]

So the plan I had in mind:

1. Let RA use existing scopes for the keywords defined by RA that matches both. [DONE]

2. Let RA allow mapping custom scopes to existing scopes without removing the custom ones from the code. [WIP]

3. Let RA supply a grammar bundle with custom scopes and pattern match the tokens that fit.

The last portion would let RA work with variable.mut and keyword.unsafe and is also what the tree-sitter does.

This reasoning comes from the fact that custom scopes are usually not implemented by themes. So what use is there to have a "variable.mut" if it remains unhighlighted?
This approach allows two things

Let the user define how a "variable.mut" looks like by either mapping to an existing scope OR defining the fontStyle & fontColor themselves.

Another PR later for custom grammars would let "variable.mut" be an actual scope and ready to be targeted by themes that are custom made for Rust.
But that's a poor solution on its own if it limits which themes you can use.

Letting the user decide which theme they want and allow them to tweak for Rust is a better approach.

[matklad]

SGTM, although I still have a feeling that it probably makes sense to just skip step 2.

[seivan]

@matklad Maybe I'm the one missing something here. But if you skip 2 you will be leaving keyword.unsafe and other custom scopes unmapped unless the theme supports it?
That's a big unless as my original plan to make it work with 90% of the themes out there in a nice compromise that lets the user fix those compromises if they so wsh without having to find a Rust-specific theme .

The important thing here is that we support constructs that exist only in Rust, and not in other languages. In particular we should man if, while and firends to keyword.control, and for unsafe we really need a dedicated rust.unsafe tag (which, ideally, should fallback to keyword).

This already happens now as the names are the same, so not really changing anything.

I wasn't talking about mapping everything - just the ones that were custom. This also makes the config portion easier to reason about from user PoV.

[matklad]

Hm, yeah, convinced that the mapping is a nice intermediate step! Sorry for the back and forth here :)

[lnicola]

Bit off-topic, but this tree-sitter demo is pretty cool: https://twitter.com/oni_vim/status/1167475446222479361.

[omerbenamram]

@seivan @matklad This looks really nice! Thanks for putting an effort into this.
I'm currently using Clion as my daily driver, and I really like their highlighting for Rust.

I was just playing with RA's syntax highlighting engine this weekend when I saw this PR!
I really like the flexibility of RA's syntax engine, I find it much nicer to play around with than the Textmate regexes VSCode has on by default.

Currently I have something like this (in a small fork I'm playing with), this is from the html based tests.

Just some thoughts - some of those are relevant to the highlighting engine in rust-analyzer and some to the vscode frontend this PR is modifying.

1. If we take https://macromates.com/manual/en/language_grammars as reference:

• #[derive] and other attibutes should be marked as entity
• stuff inside macro should be meta? I'm not entierly sure about that one, but it seems reasonable if we would want to implement a feature such as unsafe scopes (it's so good!) as meta.rust.unsafe or something to be able to decorate it.

2. It would be ideal if we could make highlighting scopes would be a bit more granular than available today.
   For example - Type aliases, generics, and lifetime annotations should be allowed to be colored differently, they could default to what's available today, although I find Clion's choices more tasteful.

If any help is necessary on the rust end of things (matching different categories for AST entities) I'd be happy to assist :)

[seivan]

1. It would be ideal if we could make highlighting scopes would be a bit more granular than available today.
   For example - Type aliases, generics, and lifetime annotations should be allowed to be colored differently, they could default to what's available today, although I find Clion's choices more tasteful.

The issue is working with existing themes that do not supply any theming for the Rust specific scopes.
Read more
A good compromise is to use the existing scopes as much as we can and also support any Rust "specific" theme that would want granular scopes.
This is yet to be implemented.
What exists as of today (with this pr) is:

• Work as much as we can with existing scopes that are shared.
  These include "keyword", "variable" and etc.

• Allow overriding scopes by defining editor.tokenColorCustomizations .
  This lets you add "fontStyle": "bold" or "foregroundColor": "#FF0000"

• Allow overriding Rust specific scopes with ralsp prefixed tags under workbench.colorCustomizations.
  This lets you target the custom scopes with your own colour, obviously this is theme independent and not the best approach .
  BUT it does let you have granular control over what unsafe or variable.mut looks like, which might be important to some. It's critical we don't remove the granular control while working with 90% of the themes out there.

If any help is necessary on the rust end of things (matching different categories for AST entities) I'd be happy to assist :)

I'd love some help on what good sane defaults could be mapped to the Rust specific scopes.

'keyword.unsafe'
'builtin'
'macro'
'variable.mut'

These are the scopes currently not targeted by either user supplied themes or editor.tokenColorCustomizations however you can still target them with statically defined colours under workbench.colorCustomizations.

So what's missing today is

1. Good sane default mapping to existing scopes
2. Code that takes care of said mapping

Eventually in a different PR also supply a grammar bundle for future Rust specific themes.

[bjorn3]

#[derive] and other attibutes should be marked as entity
stuff inside macro should be meta? I'm not entierly sure about that one, but it seems reasonable if we would want to implement a feature such as unsafe scopes (it's so good!) as meta.rust.unsafe or something to be able to decorate it.

From reading that document, I think you swapped the meaning of meta and entity:

meta — the meta scope is generally used to markup larger parts of the document. For example the entire line which declares a function would be meta.function and the subsets would be storage.type, entity.name.function, variable.parameter etc. and only the latter would be styled.

[omerbenamram]

@bjorn3 I believe my reasoning is correct.

so if we have:

unsafe {
    ffi_fn();
}

From the explanation -
For example the **entire** line which declares a function would be meta.function .

We would have keyword.unsafe followed by a meta scope of meta.unsafe.
Inside this scope you would have another function call etc..

The same reasoning goes for stuff that's inside macros and so forth.

[omerbenamram]

@seivan

IMO some sane defaults (unrelated to rust) would be:
unsafe -> storage.modifier
builtin -> variable.language
macro -> support.other.
variable.mut just a regular variable.

[seivan]

@lnicola Looks like latest master fixed the CI stuff.

[lnicola]

I prefer this, and since the Rust code follows the same pattern I thought it was appropriate, do you feel strongly about it?

In Rust it is indeed idiomatic to chain iterator methods (I'm not saying that it's not idiomatic in JS/TS, but it's the first time I see it), but it's used with collections. The moral equivalent would be vec![x].into_iter().map(f).for_each(g), which certainly isn't common. Also, Rust iterators are lazy, while I imagine JS makes an intermediate allocation on each of the steps (including the single element array). It might not matter during theme loading, but it's probably something we wouldn't want during tagging.

[lnicola]

I finally gave this a try with some of the default themes. There are some differences from the current highlighting, but nothing too bad -- most are on keywords. It didn't bite my cat or make my laptop catch fire. See the attached archive for some screenshots.

To summarize my previous feedback:

• this seems like a desirable change
• Code might get better semantic highlighting "soon"; we don't know how it will look like, but I wouldn't be surprised if it followed the TextMate scopes conventions
• there are some code idioms I'm not personally a fan of (and make the code a little convoluted)
• the TM scope mappings can be improved
• we might want a finer-grained highlighting support in RA

The last two points above can be done in a future PR, since this one has been idling here for a long time. I think we can merge this now (or soon-ish) and try to fine-tune it more in the future.

tm.zip

[matklad]

Thanks for the review, @lnicola !

Yeah, let's merge this then!

bors r=lnicola

[seivan]

@matklad @lnicola Just a moment, I need to address some of the issues brought up and push some fixes. But I am kinda short on time right now. Please don't merge just yet. :)

[matklad]

bors r_

…

On Wed, 4 Dec 2019 at 13:03, Seivan Heidari ***@***.***> wrote: @matklad <https://github.com/matklad> @lnicola <https://github.com/lnicola> Just a moment, I need to address some of the issues brought up. But I am kinda short on time right now. Please don't merge just yet. :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2061>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANB3M2HIY2OJEYRAV2VBATQW6MANANCNFSM4JEVXL6Q> .

[matklad]

bors r-

[bors]

Canceled

[seivan]

@matklad Sorry for the long delay, I've been out of commission but I'm back and merging in master and dealing with the last comments! Just wanted to update you.

[matklad]

Also see #604 (comment). Looks like there's some native support in the LSP for this already?

[seivan]

@matklad @lnicola Alright, done.
I won't be able to add any other changes myself now for some time, working on other things.
But as far as I know, I have nothing else to add.

[seivan]

@matklad I looked at that - unless I have misunderstood completely (pretty big chance) this means that scopes.ts functionality would be unnecessary;

• Loading active themes
• Creating decorations (inside highlighter.ts) based on the active theme rules

However it would still need scopes_mapper.ts to map Rust scopes to TM scopes - unless you want to rename them, which I advice against, I like your names more as they're easier to understand.

Either way, I am done with the PR - fixed the review comments with this exception
but I see value of that because it makes sure the scopes_mapper and scopes are not coupled. So you could get rid of the portion of the code that deals with VsCode hacks (e.g loading themes, etc) while keeping the code that maps rust scopes to TM scopes which will still be useful even with the LSP changes currently pending.

Merry Christmas btw.

[matklad]

Let's merge this and fix any fallout separatelly!

bors r+

[bors]

Build succeeded

• Rust
• TypeScript