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

add a new TextMate grammar #6137

Merged
merged 1 commit into from
Oct 12, 2020
Merged

add a new TextMate grammar #6137

merged 1 commit into from
Oct 12, 2020

Conversation

dustypomerleau
Copy link
Contributor

Thanks to everyone working hard on Rust Analyzer - my impression is that it's quickly becoming the community default.

I think it would be helpful to have a more robust TextMate grammar to fall back on, for those who wish to disable semantic highlighting for any reason. It should allow theming of punctuation, and provide scopes for all tokens on the page. This can be done at zero cost to those who enable semantic highlighting, as the TextMate scopes will be invisible to those users.

I can see a couple ways of accomplishing this:

  1. Ship a new grammar by merging this PR.
  2. Ship no TextMate grammar at all (like the Rust extension), and allow users to install a separate extension that provides the grammar of their choice (I have released this one as Rust Syntax). If no grammar were installed, they would simply fall back to the default grammar provided by their editor. In the case of VS Code, the default grammar already matches what is currently being shipped, so users who choose not to override it would see no difference.

I have tried to choose sensible default scopes, in the hopes that a wider variety of themes would work out of the box with Rust, even if those themes do not yet supply scopes for semantic highlighting. There is definitely some interest in using this grammar with Rust Analyzer, as this was the very first issue after the syntax extension was shipped: dustypomerleau/rust-syntax#1.

I considered simply using an alternative grammar alongside Rust Analyzer, but this doesn't seem possible. When RA starts, any existing grammar/extension is overridden, and I haven't been able to find a workaround.

]
}
}
"$schema": "https://raw.githubusercontent.com/martinring/tmlanguage/master/tmlanguage.json",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use 4 space indentation everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be avoided by having an .editorconfig, wdyt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it matters. Since this grammar is maintained externally by @dustypomerleau, we could bundle it as-is instead of reformatting the JSON.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm flexible on this. I am maintaining the grammar as YAML and generating the JSON.

@matklad
Copy link
Member

matklad commented Oct 6, 2020

cc @georgewfraser

@lnicola lnicola added the hacktoberfest-accepted Hacktoberfest accepted PR label Oct 6, 2020
@dustypomerleau
Copy link
Contributor Author

Will push a few changes this weekend based on feedback @bjorn3 gave in microsoft/vscode#108254.

@matklad
Copy link
Member

matklad commented Oct 12, 2020

bors r+

Let's merge this as is. However, if the upstream indeed ships this as a baseline grammar for Rust, I'll be inclined to just remove our bundled grammar altogether, as it indeed makes it easier to override by a different extension, and because I would prefer to minimize the number of not server-specific bits in this repo.

@bors
Copy link
Contributor

bors bot commented Oct 12, 2020

@georgewfraser
Copy link
Contributor

@matklad Sorry I haven't been on top of my GH notifications and I'm just seeing this now.

@dustypomerleau I am supportive of your goal of making the textmate grammar better.

However I think your approach, may have created some unintended consequences. The scope names that are used by the textmate grammar need to be kept in sync with the scope names in the semanticTokenScopes section of package.json:

https://github.com/rust-analyzer/rust-analyzer/blob/7c94f1cb5e86739864871dc56fcbd7a6de116315/editors/code/package.json#L938

Additionally, when you know that there's going to be a "second pass" of more accurate syntax coloring from the language server, it's important to make sure that the textmate coloring is either conservative (when in doubt, don't color) or that whatever mistakes it makes are explicitly fixed in the second pass.

The grammar I created when I submitted the PR that "fixed up" semantic coloring was designed with all this in mind: https://github.com/rust-analyzer/rust-analyzer/pull/4397/files

Unfortunately, it seems that in this PR you just ignored my grammar and started over. I'm not offended, I just assume you have semantic highlighting turned off for whatever reason and so you didn't consider this when you made this PR.

@matklad I agree that @dustypomerleau textmate grammar should be removed now that it's been upstreamed in VSCode, but we may need to do some follow-up work to make sure what's in package.json matches what's in the new grammar.

@dustypomerleau
Copy link
Contributor Author

dustypomerleau commented Nov 6, 2020

Hi, @georgewfraser, thanks for commenting. My position on all of this is pretty simple: I don't want to ship anything (here, VS Code, my own extension) that's going to break Rust Analyzer in some way. Since RA is the future default for many IDEs, my grammar has little value if it conflicts with RA, or if the community that uses RA doesn't prefer it. You're going to need to educate me a bit about the need to sync the scopes. Since RA has 100% token coverage with semantic highlighting, I didn't think the underlying Textmate scopes would have any effect on the semantic scopes applied. If my scopes differ from those you linked in package.json, what is the consequence?

@georgewfraser
Copy link
Contributor

@dustypomerleau There's a delay between when VSCode renders using the textmate scope, so if there's a mismatch between your theme and the semantic scopes, the user will see a "pop" where the colors change every time they open a file.

Specifically, we need to match up the textmate scopes in your theme with the scopes in package.json and the builtin defaults for VSCode, either by editing your theme or editing package.json.

The additional complication of this is that the existing color themes contain a ton of redundancy where they map multiple scopes to the same color. So even if your TextMate scopes don't match, the default color themes may "fix" the problem by mapping them back to the same colors. But a user with a different theme may still have a problem.

It's also just good hygiene to try to reduce the number of different TextMate scopes we use in VScode. It makes life easier for color theme developers when they have a smaller number of scopes to target. The ones I picked for my "minimalist" theme were intended to be the most commonly used themes for each concept.

@georgewfraser
Copy link
Contributor

georgewfraser commented Nov 7, 2020

The default fallback scopes are much more comprehensive than when I last looked at them, I think we should align on that since it represents guidance from VSCode. We may be able to actually remove the fallbacks in package.json now.

Edit: I was able to remove only a few fallback scopes, I made a PR: #6496

@georgewfraser
Copy link
Contributor

@dustypomerleau You've also made a bunch of choices in your grammar that contradict the generally accepted meaning of the scopes. For example, you've made fn, use, and const control keywords, they should be regular keywords.

@cynecx
Copy link
Contributor

cynecx commented Nov 7, 2020

Using an upstream grammar has a downside where shipping changes may take a while to upstream because of the monthly release schedule of vscode. Whereas shipping it with the rust-analyzer extension give us more control overall.

@georgewfraser
Copy link
Contributor

@cynecx that is true but I don't think @matklad really wants to control the textmate grammar. @dustypomerleau has gotten VSCode to accept his grammar, and I think he's the ideal maintainer since he he's demonstrated the passion for chasing down all the little corner cases of textmate grammars. We just need to make sure his grammar is coordinated with what we're doing here with semantic coloring.

@cynecx
Copy link
Contributor

cynecx commented Nov 7, 2020

JFYI: Some of the non control-flow keywords should be fixed by #6497.

@dustypomerleau
Copy link
Contributor Author

I'll have a look at the fallback scopes, and see whether alignment can be improved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Hacktoberfest accepted PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants