Skip to content

Proposal: color attributes using regular syntax coloring #4430

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

Closed
georgewfraser opened this issue May 12, 2020 · 9 comments · Fixed by #4474
Closed

Proposal: color attributes using regular syntax coloring #4430

georgewfraser opened this issue May 12, 2020 · 9 comments · Fixed by #4474

Comments

@georgewfraser
Copy link
Contributor

Following up on #4400. Right now, Rust attributes are left as the default color. Both the TextMate grammar and the semantic coloring rules specify meta.attribute.rust, which has no effect. In the default Dark+ theme, they show up as plain white:

Screen Shot 2020-05-11 at 5 23 39 PM

To me, this seems like more of an oversight than an intentional choice. Now that semantic coloring is available, rust-analyzer is "taking ownership" of syntax coloring, so it makes sense to fix this here. What do other languages do?

C# attributes are colored with normal syntax coloring, with the surrounding brackets punctuation.squarebracket.open.cs, which is the default color in Dark+:

Screen Shot 2020-05-11 at 5 04 07 PM

Typescript decorators are colored with normal syntax coloring, with the @ symbol punctuation.decorator.ts, which is the default color in Dark+:

Screen Shot 2020-05-11 at 5 14 20 PM

Java annotations are colored with normal syntax coloring, with the @ symbol punctuation.definition.annotation.java, which is the default color in Dark+:

Screen Shot 2020-05-11 at 5 09 05 PM

Based on these precedents, I propose that we color the inner parts of Rust attributes with the normal syntax coloring rules, and the surrounding #[] as punctuation.attribute.rust, which will render as the default color in the builtin themes (as it does now).

LMK what you think @matklad

@aloucks FYI

@georgewfraser
Copy link
Contributor Author

Proof of concept of what this might look like:

Screen Shot 2020-05-11 at 7 32 16 PM

@aloucks
Copy link
Contributor

aloucks commented May 12, 2020

I use an older version of atom one dark and currently have semantic coloring turned off until it gets sorted out a bit more.

I suspect that the changes proposed here might break existing themes that already have support for rust. I'm curious what would happen if you added the additional scopes but do not remove any of the existing ones.

image

image

image

@georgewfraser
Copy link
Contributor Author

@aloucks The screenshot you show should look the same right now with semantic coloring on. #4397 should have undone the things that made semantic coloring different than TextMate coloring, except that semantic coloring is more accurate.

@Aloso
Copy link

Aloso commented May 12, 2020

In my opinion, the traits in #[derive] attributes should be highlighted like types, and the name of the attributes the same as normal macros:

vscode attributes

An alternative is to highlight built-in attributes like keywords and proc-macro attributes like normal macros:

vscode attributes alt

@aloucks
Copy link
Contributor

aloucks commented May 12, 2020

IntelliJs highlighting does something similar (note that Debug is highlighted as a trait):

image

@georgewfraser
Copy link
Contributor Author

@Aloso Rust attributes are strange---they're like a built-in mini-language of their own. I am still learning about them myself, but here is my understanding so far: the identifier Clone in #[derive(Clone)] isn't exactly a reference to the type std::clone::Clone---it's a constant/macro of the same name, defined by rustc. The derive attribute knows to implement the Clone trait when it sees the identifier Clone, but there's nothing in principle preventing it from doing something else. I'm not disagreeing with you, I'm just saying we should think carefully about "what do the identifiers in attributes really represent?"

@georgewfraser
Copy link
Contributor Author

The builtin attributes appear to be defined here:

https://github.com/rust-lang/rust/blob/master/src/librustc_feature/builtin_attrs.rs

@georgewfraser
Copy link
Contributor Author

Custom attributes can be defined as described here: https://doc.rust-lang.org/reference/procedural-macros.html

It seems the outermost layer of symbols in an attribute are always functions. The arguments to these functions are not parsed---they are passed to the functions as a stream of tokens. So they could represent anything, and there's not really any general mechanism to understand the meaning of these tokens. Having said that, hard-coded rules for builtin attributes are probably not the worst idea. For example, it would be nice if #[derive(Clone)] showed Clone as a type.

@Aloso
Copy link

Aloso commented May 13, 2020

Derive macros usually implement a trait of the same name. I know that this is only a convention, and might not always be true, but it's true for the built-in derives and for many crates with derives, such as serde, diesel, thiserror and many more.

Providing syntax highlighting and "Go to definition" based on this assumption is useful, with almost no downsides IMO.

You are correct about procedural macros. That is the reason why both IntelliJ and rust-analyzer don't attempt to highlight the token tree in a procedural macro, except for literals (numbers, bools, strings and chars) and keywords I believe.

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

Successfully merging a pull request may close this issue.

3 participants