Skip to content

Create new syntax name for control flow keywords - #268

Open
vimpunk wants to merge 2 commits into
rust-lang:mainfrom
vimpunk:ctrl-flow-syntax
Open

vimpunk wants to merge 2 commits into
rust-lang:mainfrom
vimpunk:ctrl-flow-syntax

Conversation

@vimpunk

@vimpunk vimpunk commented Sep 21, 2018

Copy link
Copy Markdown

I'm writing a vim color scheme and I'd like to be able to differentiate return, break, continue, and similar keywords from "less important" keywords such as let or impl. Thus I added a new syntax type called rustCtrlFlow so color scheme authors can specifically target the control flow related keywords.

@rust-highfive

Copy link
Copy Markdown

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @chris-morgan (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@da-x

da-x commented Oct 16, 2018

Copy link
Copy Markdown
Member

Thanks! It's a good step, and it would be better even to bring us closer to what the docs say in :help group-name. We should assign keywords to their respective minor groups. if can be under Conditional, loop can be in the Repeat minor group as well, etc.

@vimpunk

vimpunk commented Oct 18, 2018

Copy link
Copy Markdown
Author

@da-x That's an even better idea. I can look into that, this weekend possibly, if that's fine!

@da-x

da-x commented Oct 18, 2018

Copy link
Copy Markdown
Member

@mandreyel Thanks!

@z2oh

z2oh commented Oct 26, 2018

Copy link
Copy Markdown
Contributor

@mandreyel Are you still planning on looking into this? If not, I'd be willing to take a stab at it.

@vimpunk

vimpunk commented Oct 26, 2018

Copy link
Copy Markdown
Author

I'm so sorry, lots happened recently and I never got around to this! @z2oh I'd still like to finish this, if you don't mind. But I'll let you know if I can't finish it!

@vimpunk

vimpunk commented Oct 26, 2018 •

Copy link
Copy Markdown
Author

@da-x Alright, I assigned loop keywords to Repeat.

I noticed that rustStructure and rustTypedef are also assigned to Keyword instead of their respective minor groups (Structure and Typedef, respectively). There are even comments saying that it would be more fitting to do so. Is there a reason for assigning them to Keyword instead of their minor groups?

@z2oh

z2oh commented Dec 3, 2018

Copy link
Copy Markdown
Contributor

@da-x sorry to bother you... do you have any sort of update on this? I am very much looking forward to improved syntax highlighting 😀

@chris-morgan

Copy link
Copy Markdown
Member

Apparently I never ended up actually posting this comment, and so it simply sat in the tab in Firefox for a month or two—

More precise groups exist, but consensus (between myself and whoever wrote the various parts in the first place) was that distinguishing them didn’t look as good; that’s why we left them, so that you can hi link rustStructure Structure, but that it doesn’t default to that.

(However, for reference, I belong to the school of thought that says colour schemes are generally too fancy. A few years ago I threw away my colour schemes and wrote a new one from scratch, which I called “bland”. Keywords are bold, strings are red, comments are italic green, numbers are blue, and that’s about it. I should therefore be considered biased.)

@da-x

da-x commented Dec 4, 2018

Copy link
Copy Markdown
Member

Hi, sorry for the late response.

I've done some analysis, and it's hard for me to find reasons to align myself with the previous consensus.

My rationale is the following: Being that it is up to the color scheme on whether to allow differentiation on keywords types, and then it is up to the user on whether to utilize it in his or her color scheme (as a side note example, my own color scheme does not (yet!) differentiate loop and conditional keywords from other keywords), combined with the fact that many syntax highlight plugins that come built-in with Vim do allow for this differentiation, then we should allow for this as well in rust.vim.

The main reason is consistency. For example, suppose that I was a using the trio of Lua, Haskell, and Rust in the same project, and have configured differentiation for the Structure subgroup in my color scheme, then I'd be surprised to find that Lua and Haskell do mark their 'data structure definition'-related keyword with the Structure keyword, but Rust does not.

$ grep -R Structure /usr/share/vim/vim81/syntax  | egrep 'haskell|lua|rust'
./haskell.vim:syn match hsStructure             "\<\(class\|data\|deriving\|instance\|default\|where\)\>"
./haskell.vim:hi def link hsModule                        hsStructure
./haskell.vim:hi def link hsStructure                     Structure
./rust.vim:syn keyword   rustStructure struct enum nextgroup=rustIdentifier skipwhite skipempty
./rust.vim:hi def link rustStructure     Keyword " More precise is Structure
./rust.vim:hi def link rustUnion         rustStructure
./lua.vim:hi def link luaTable          Structure

If the original intention for Structure was to target C-like structures, it was largely missed by plugin writers. Actually, I like what the other plugin writers did. So it's better to align ourselves to the conventions de-facto and not surprise the users (hence the PR). And on whether it looks good or not, depends largely on the color scheme and the taste of the user. Inconsistency looks worse, IMHO.

So let's continue with the changes?

@rustbot

rustbot commented Oct 27, 2026

Copy link
Copy Markdown

☔ The latest upstream changes (possibly #535) made this pull request unmergeable. Please resolve the merge conflicts.

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 this pull request may close these issues.

6 participants