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

Try new Rust grammar #108254

Merged
merged 3 commits into from
Nov 2, 2020
Merged

Try new Rust grammar #108254

merged 3 commits into from
Nov 2, 2020

Conversation

alexr00
Copy link
Member

@alexr00 alexr00 commented Oct 7, 2020

Related to #64488

@@ -210,7 +342,29 @@
},
{
"c": "let",
Copy link
Member Author

Choose a reason for hiding this comment

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

In other languages, let is usually scoped as storage.type. Should that be the case for Rust?

Copy link

Choose a reason for hiding this comment

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

I think so. Even if not, keyword.control seems wrong as it doesn't affect control flow.

Copy link

@cynecx cynecx Nov 7, 2020

Choose a reason for hiding this comment

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

The new grammer has an issue I am seeing right now:

fn and use should not be highlighted as keyword.control. In my opinion fn does not not necessarily signify control-flow. The same applies to use, there is no control-flow.

@alexr00 alexr00 self-assigned this Oct 7, 2020
@@ -154,19 +242,30 @@
}
},
{
"c": "println!",
"t": "source.rust support.function.std.rust",
"c": "println",
Copy link
Member Author

Choose a reason for hiding this comment

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

In other languages (and in the old grammar for Rust), print statements are support.function should that be the case for Rust too?

Copy link

Choose a reason for hiding this comment

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

In Rust println! is a macro and not a function. I also think that println! maybe should be considered a single token and not two tokens (println and !)

}
},
{
"c": "mut",
Copy link
Member Author

Choose a reason for hiding this comment

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

In the old grammar mut was storage.modifier, is keyword more correct?

Copy link

@bjorn3 bjorn3 Oct 8, 2020

Choose a reason for hiding this comment

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

https://macromates.com/manual/en/language_grammars

  • storage — things relating to “storage”.
    • modifier — a storage modifier like static, final, abstract, etc.

Should be storage.modifier I think.

@@ -1,13 +1,13 @@
[
{
"c": "impl",
"t": "source.rust storage.type.rust",
"t": "source.rust keyword.control.rust",
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what scope this should have, but the new grammar does change the scope it gets.

},
{
"c": "struct",
"t": "source.rust keyword.control.rust",
Copy link
Member Author

Choose a reason for hiding this comment

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

In other languages, struct is scoped as storage.type, and in the old grammar it was also storate.type. Should it change?

Copy link

@bjorn3 bjorn3 Oct 8, 2020

Choose a reason for hiding this comment

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

https://macromates.com/manual/en/language_grammars

  • storage — things relating to “storage”.
    • type — the type of something, class, function, int, var, etc.

Yeah, it should be storage.type I think.

}
},
{
"c": "trait",
Copy link
Member Author

Choose a reason for hiding this comment

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

Should this stay storage.type?

@alexr00
Copy link
Member Author

alexr00 commented Oct 7, 2020

@dustypomerleau I have tried adopting your rust grammar in this PR. We have colorization tests that compare the color and scope of tokens in sample language files. The language files for rust are here:
https://github.com/Microsoft/vscode/blob/5bb9d0b0987c9db4335535e7b339a26d20dda473/extensions/rust/test/colorize-fixtures/test-6611.rs#L1-L1
https://github.com/Microsoft/vscode/blob/8509d4e47a8e85355353500d0b0636d3fcbd0462/extensions/rust/test/colorize-fixtures/test.rs#L1-L1

Your grammar definitely colors more tokens and colors some tokens more specifically, which is great. However, there are some tokens that I'm not confident have the correct scope. I left some comments in the output of the colorization tests where the scope doesn't match what I'd expect.

As you probably know already, the reason we have some standard scopes that we expect various tokens to be is so that one theme can look good on any language. I am not a rust user, so I'm not 100% certain that the comments I have are relevant, but please take a look and convince me that the scopes are correct (or update your grammar and I'll pull the change into this PR).

Once we've addressed the scope differences, I'll bring this grammar into insiders for a month or two. Given the low user count for this grammar currently, I want to make sure that it has plenty of time in insiders so users and find any issues.

@dustypomerleau
Copy link
Contributor

@alexr00 The reason I chose keyword for many of these scopes is because that's how they are classified by the Rust lexer. Strict keywords listed in The Rust Reference include most of the tokens caught by your test:

  • impl
  • let
  • mut
  • struct
  • trait

I'm happy to move some of the keywords into storage, but which ones? Anything that allocates on the heap?

As for println!, @bjorn3 has already mentioned that this is a macro in rust. It is one of the macros provided by the standard library, so I can see how it might be viewed as support. My only opinion here is that it should be possible to theme macros that use the macro_bang! syntax differently from plain functions. I personally don't feel a need to have different scopes for standard library macros versus user-defined macros, but if others felt strongly I could separate them. As for the ! itself, I can provide a separate scope for that (distinct from it's use as NOT), and let theme designers decide whether to match it to the rest of the macro or let it stand out.

@alexr00
Copy link
Member Author

alexr00 commented Oct 8, 2020

@dustypomerleau since many of the tokens that get scoped as storage in other languages are also keywords in their respective languages, I think something like the guideline that @bjorn3 quoted is a good rule of thumb for when to use storage: #108254 (comment)

What you said about macros makes sense to me.

Please ping here when you've made the changes you want to make and I'll update again!

@dustypomerleau
Copy link
Contributor

@alexr00 Changes have been pushed. Appreciate your help.

@dustypomerleau
Copy link
Contributor

dustypomerleau commented Oct 16, 2020

@alexr00 Pushed an additional commit to fix a highlighting error for raw strings rust-lang/rust-analyzer#6042.

@alexr00
Copy link
Member Author

alexr00 commented Oct 20, 2020

@dustypomerleau I'll try to get to this this week. Thanks for the updates!

@alexr00 alexr00 added this to the November 2020 milestone Oct 30, 2020
@alexr00
Copy link
Member Author

alexr00 commented Oct 30, 2020

I was not able to review this again for October. Will address in November.

@alexr00
Copy link
Member Author

alexr00 commented Nov 2, 2020

@dustypomerleau I am happy with the changes, thanks! I will merge the grammar from https://github.com/dustypomerleau/rust-syntax/ into VS Code and we will see what users think for a month.

@dustypomerleau
Copy link
Contributor

Thanks for your help, @alexr00! For patch updates, should I ping someone or submit my own PR with updated commit hash? We are still finding occasional edge cases since I merged into Rust Analyzer.

@alexr00
Copy link
Member Author

alexr00 commented Nov 3, 2020

@dustypomerleau if there's an urgent fix in the grammar, please just open an issue and ping me on it. Otherwise, I pick up grammar changes with a script about once per month at the beginning of our milestones.

@alexr00
Copy link
Member Author

alexr00 commented Nov 3, 2020

and @bjorn3 thanks for the additional feedback!

@cynecx
Copy link

cynecx commented Nov 7, 2020

The grammar in its current state has some issues, I'd like to get it fixed:

The new grammer has an issue I am seeing right now:

fn and use as should not be highlighted as keyword.control. In my opinion fn does not not necessarily signify control-flow. The same applies to use, there is no control-flow.

Is it preferred to open a PR here or upstream (https://github.com/dustypomerleau/rust-syntax/)? Also the current situation is quite weird, because we've got this same grammar in multiple places vscode, upstream extension and in rust-analyzer.

@bjorn3
Copy link

bjorn3 commented Nov 7, 2020

I think vscode doesn't accept any changes to grammars. I think they only periodically pull in updates from the upstream version, which is https://github.com/dustypomerleau/rust-syntax/ in this case.

@alexr00
Copy link
Member Author

alexr00 commented Nov 9, 2020

@bjorn3 is correct.

@alexr00
Copy link
Member Author

alexr00 commented Dec 7, 2020

There has been no feedback about this grammar having problems. I will leave it in for the release of 1.52.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants