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

feat(treesitter): add more default groups to highlight map #17835

Merged
merged 1 commit into from Mar 30, 2022

Conversation

gpanders
Copy link
Member

This covers more default groups listed in :h group-name.

@clason clason requested a review from theHamsta March 24, 2022 08:26
@clason
Copy link
Member

clason commented Mar 24, 2022

Where do these capture names come from? AFAIK these are not standard.

@gpanders
Copy link
Member Author

Are there standard capture group names? I wasn’t aware if so. I just used the lower case version of the highlight group.

@clason
Copy link
Member

clason commented Mar 24, 2022

There are in nvim-treesitter (https://github.com/nvim-treesitter/nvim-treesitter/blob/master/CONTRIBUTING.md#highlights), which are enforced by CI.

I think there should be some objective guideline here; otherwise there's no limit to the number of group names people will request.

@gpanders
Copy link
Member Author

otherwise there's no limit to the number of group names people will request.

People can modify hl_map themselves, which is exactly what nvim-treesitter does. So I don't think we need to worry about this :)

Of course, I could do the same thing (and I am, for now), but I thought it'd be useful to at least have coverage over all of the "builtin" highlight groups listed in :h group-name.

That can be our objective guideline too: in core we only include capture groups for the builtin highlight groups. Anything beyond that can be done externally by plugins.

@clason
Copy link
Member

clason commented Mar 24, 2022

Of course, I could do the same thing (and I am, for now), but I thought it'd be useful to at least have coverage over all of the "builtin" highlight groups listed in :h group-name.

Yes, but the captures for it are still arbitrary -- why these and not others? Are these ever used (outside your own dotfiles, presumably?)

To be clear, I am not against adding stuff, but I'd like some reasoned guideline about what gets added (on both side of the mapping).

@gpanders
Copy link
Member Author

Yes, but the captures for it are still arbitrary -- why these and not others? Are these ever used (outside your own dotfiles, presumably?)

I see your point. I can integrate these with capture groups used by nvim-treesitter to the extent possible (e.g. text.underline rather than underlined). But some are not used by nvim-treesitter (typedef and storageclass, for instance). The capture groups are "arbitrary" in a sense, but not entirely since they reflect the underlying highlight group.

I am also happy to introduce the new capture groups to nvim-treesitter as well with a PR to help propagate them into the broader ecosystem.

This covers some default groups listed in :h group-name.
@clason
Copy link
Member

clason commented Mar 24, 2022

Yeah, that's my point: I'd like to use this opportunity to come up with a sort of strategy that we can follow across the wider ecosystem and in the future.

That's why I also want to hear from @theHamsta here, who was the strongest advocate for cross-language standardization so far.

@clason
Copy link
Member

clason commented Mar 25, 2022

@theHamsta ping

@clason
Copy link
Member

clason commented Mar 30, 2022

Guess no objections :)

@clason clason merged commit 6d648f5 into neovim:master Mar 30, 2022
@gpanders gpanders deleted the ts-hl-map branch March 30, 2022 20:38
@theHamsta
Copy link
Member

theHamsta commented Mar 31, 2022

Can we please revert this? I don't think we should try to imitate the Vim highlighting groups especially since they can be referred to directly already now.

  • Just because there is a highlight group isn't a reason to add a cross-language category. There should be a strong reason to add a new one. It must be clear how a Vim highlight can fit into the already dense Atom namespace and whether a subcategory which is now supported would make more sense (easier to understand purpose, more flexible to configure (top-level vs specifications), compatibility with Atom/Emacs)
  • If a new capture is added it (best one per PR to go through all the implication), it should be applied to all languages. Otherwise, we will have again duplicates, unused, misdocumented or fuzzy captures that only get added to some languages or get added differently because every. When a new capture gets added, everybody should be clear what they mean and where they apply
  • We removed some built-in highlights like @structure (applies to enum/struct in C and not to types that are struct) before because they were very confusing. Also because there was no documentation for the original Vim highlight groups which made them used also quite flexibly for different syntaxes
  • The "confusing" part was mostly that those hl-groups only make sense for "C" and there is not much meaning when not read in the context of a syntax file where each part of the syntax tree for a specific language gets finer and finer categorized but not as general categories.

About the categories:

  • @storageclass might make sense, might be more explicit as @keyword.storageclass
  • @character.special is super-specific. No big issue as a sub-category. But examples on where this should a apply and why it is necessary would be good
  • @todo might make sense but should maybe go into the @text.X
  • @debug and @type.attribute are totally unclear what they should be. Documentation "debug is debug statement" does not really help. Maybe FIXME etc are meant but then I don't get why those are statements. @type.attribute might refer to something similar like @storageclass but it might be difficult to find a clear distinction
  • @type.definition is the only semantic category. This should be handled more generally and in conjunction with how we use #is? in future or any other no, @type.definition is just confusing because it refers to C typedef but everyone things of something different
  • @preproc collides with @macro and distinction only makes sense for C, similar @define @type,definition. Should not require a documentation if just a specialization for C like @macro.define. @define/TSTypeDefinition is missing in CONTRIB.

@gpanders
Copy link
Member Author

Thanks for your feedback.

@storageclass might make sense, might be more explicit as @Keyword.storageclass
@todo might make sense but should maybe go into the @text.X

I can make these changes.

no, @type.definition is just confusing because it refers to C typedef but everyone things of something different

Not necessarily. What about a newtype in Haskell or Rust? Or in Zig one can do something like

const MyType = u32;

which is similar to a C typedef in that it "aliases" a type.

@debug and @type.attribute are totally unclear what they should be
@preproc collides with @macro and distinction only makes sense for C, similar @define @type,definition

No arguments here, I don't have any issue reverting these.

@theHamsta
Copy link
Member

I'm not categorically against this. But I would prefer actually introducing the highlight groups one by one and actually fixating them in the highlights. I assume there won't be many rogue PRs using the new groups so we can also do this discussion for each hl-group when it's used in the query instead of inverting. It's generally preferable to have the actual usage before adding documentation. The usage generally shows ambiguities in the understanding of the hl-groups.

@clason
Copy link
Member

clason commented Mar 31, 2022

Just because there is a highlight group isn't a reason to add a cross-language category. There should be a strong reason to add a new one. It must be clear how a Vim highlight can fit into the already dense Atom namespace and whether a subcategory which is now supported would make more sense (easier to understand purpose, more flexible to configure (top-level vs specifications), compatibility with Atom/Emacs)

I strongly disagree with this reasoning. Our concern is with Neovim highlighting, not Atom/Emacs. (It would be different if we could use upstream queries as-is, but that ship has already sailed, so we might as well go whole hog.) In this sense, it makes complete sense to align with legacy highlight groups which are widely used and form the basis for user expectations.

I also think that cross-language homogenization is a fool's errand -- languages are just too different, and trying to force them into a common corset will make it fit none.

Finally, the main issue here is the lack of documentation (on the Neovim side; nvim-treesitter is a different project and discussion) -- I really want some sort of documented guidance here, both on the general philosophy and the specific captures/highlights. (I had hoped this -- open -- PR would spark that, but there seemed to be no interest.) Also, I wanted these changes in for 0.7, which has a feature freeze tomorrow.

@theHamsta As you have the strongest opinions on this, please open a new issue outlining your proposed policy.

@theHamsta
Copy link
Member

theHamsta commented Mar 31, 2022

I strongly disagree with this reasoning. Our concern is with Neovim highlighting, not Atom/Emacs. (It would be different if we could use upstream queries as-is, but that ship has already sailed, so we might as well go whole hog.) In this sense, it makes complete sense to align with legacy highlight groups which are widely used and form the basis for user expectations.

I don't think a general discussion brings any progress. If something is missing or should it should be discussed with concrete example and all the changes it would imply.

I also think that cross-language homogenization is a fool's errand -- languages are just too different, and trying to force them into a common corset will make it fit none.

Vim has a very flat organization of the highlight groups basically each syntax construct in every has a name. They link to a chain of default groups but the links are not immediately obvious when looking at the syntax files. The Atom uses a fixed set of captures allowing to use a hierarchy to make them more specific. We happen to use the system of Atom. This system already covers full languages. I think Vim specifics can be easier learned, configured and adopted when they integrate in the current system and make Atom ones we using more specific (e.g. @keyword.XXX). New contributors can port existing queries and we only tell where something needs to be specialized further. I propose to for language specifics to use this kind specialization and keep the general documentation slim. There is no need to document a certain query in the global namespace when it's understood best in the context of that language. The problem with the argument "my language is special" is that no reason is provided why the captures are chosen in a specific way (mostly "I like this color")

The steps forward for me would be to add those new captures one by one to the queries (each for all the languages at once) and refine docs and their naming if needed. CI needs to be adapted to allow @language.specific.specializations when the prefix is documented (e.g. language.specific).

For upstream, it needs to be decided whether the idea is to follow nvim-treesitter or to maintain own built-in syntax files. In any case, I'd recommend to use the actual groups in hlmap in the queries and by that assure that they are tested in practice and necessary.

@clason
Copy link
Member

clason commented Mar 31, 2022

New contributors can port existing queries and we only tell where something needs to be specialized further.

That can be very frustrating, as it feels like you are made to play a game where you have to guess the correct groups ("Highlightle")... Especially where there are no existing queries yet. It makes it much easier for new contributors (and hence more likely for them to do it) if they can be pointed to some document and only finetuning is needed.

I strongly believe that we do need a general documentation, in particular if we switch from the current flat Vim approach to a hierarchical Atom approach (which has its charms, but a) we should do it right or not at all, and b) we need to make that switch explicit). We need some consensus for that, and I don't see how we can get either without a discussion involving all stakeholders.

(And I would hope that there can be some agreement so that nvim-treesitter can remain the staging/testing ground for an eventual upstream support.)

@theHamsta
Copy link
Member

That can be very frustrating, as it feels like you are made to play a game where you have to guess the correct groups ("Highlightle")... Especially where there are no existing queries yet. It makes it much easier for new contributors (and hence more likely for them to do it) if they can be pointed to some document and only finetuning is needed.

But we have the documentation for it. We shouldn't make it too complicated by adding too many overlapping captures. Most additional highlights have already namespaced in someway.

I strongly believe that we do need a general documentation, in particular if we switch from the current flat Vim approach to a hierarchical Atom approach (which has its charms, but a) we should do it right or not at all, and b) we need to make that switch explicit). We need some consensus for that, and I don't see how we can get either without a discussion involving all stakeholders.

We currently have mostly the hierarchical Atom approach now. There are only some exceptions for keywords which are not too bad at the moment.

@clason
Copy link
Member

clason commented Mar 31, 2022

OK, maybe (definitely) I wasn't clear enough here -- I should have been much more explicit in distinguishing between captures and highlights.

  1. Captures clearly follow the Atom model, and that is good. (There's also little Neovim can do about it since they're set by nvim-treesitter, at least for the foreseeable future.) What I would like here is some more explanation about these -- the general structure (for example, the fact that Atom-style is followed as far as possible/reasonable). More guidance/examples on the individual captures wouldn't hurt, either. (I'm speaking from experience here -- if I may be blunt between two Germans: right now, it often feels like the reason for using one specific capture is "because @theHamsta said so", which doesn't much help when selecting the next capture... And the fact that @gpanders picked the "wrong" captures is also an indication that more guidance would be useful.)

  2. Highlight groups currently follow the Vim model, with a TS slapped in front. There is no real reason for this to be the case -- we could just as well set these up "bottom-up" by mimicking the actual captures (like TSKeywordReturn for @keyword.return). That would also send a signal. When I was speaking about a discussion and decision, this is what I was talking about.

@theHamsta
Copy link
Member

(I'm speaking from experience here -- if I may be blunt between two Germans: right now, it often feels like the reason for using one specific capture is "because @theHamsta said so",

Right now often I'm just reminding what the results earlier discussions was. You're right that it would be good to write the results of those issue down in a documentation. The current capture system was mostly designed by @vigoux. He was mostly against adding more captures at some point so every new one was longer discussion and we also used them directly. Before there were large tracking issues for which languages a certain feature was already added and which are using a capture in a different way (e.g. ternary operator, what are tags and attributes). I really want to avoid falling back into times where documentation and certain group of languages diverged and rather have compact changes on a specific topic which is easiest when we fill the added hl-group one by one with live by using them in the queries. A general philosophy description PR or a re-grouping of the names of the captures would also be a targeted topic.

(like TSKeywordReturn for @keyword.return). That would also send a signal. When I was speaking about a discussion and decision, this is what I was talking about.

Sounds good. The sub-spacing was mostly applied as a compromise in cases where there was no strong opinion on whether a new capture was really necessary. I have no opinion whether existing also @storageclass would be fine for me.

@clason
Copy link
Member

clason commented Mar 31, 2022

And for what it's worth, I too find the original Vim highlight groups needlessly C-centric, and would love to use the opportunity to re-think the new TS highlight groups. (They would still link to the old groups, of course, for backwards compatibility -- but I hope that we'll extend the default colorschemes to handle TS groups directly sooner rather than later...)

@stsewd
Copy link
Contributor

stsewd commented Apr 1, 2022

If it's worth, I agree with @theHamsta, we should be conservative about adding new captures/highlights, since once introduced other themes may be using them already. Also, the PR on the nvim-treesitter repo introduced those captures, but it didn't make use of them in any language.

dmitmel pushed a commit to dmitmel/neovim that referenced this pull request Apr 16, 2022
)

This covers some default groups listed in :h group-name.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants