Repository navigation
Syntax inconsistencies using tree-sitter #558
Description
Prerequisites
- Put an X between the brackets on this line if you have done all of the following:
- Reproduced the problem in Safe Mode: http://flight-manual.atom.io/hacking-atom/sections/debugging/#using-safe-mode
- Followed all applicable steps in the debugging guide: http://flight-manual.atom.io/hacking-atom/sections/debugging/
- Checked the FAQs on the message board for common solutions: https://discuss.atom.io/c/faq
- Checked that your issue isn't already filed: https://github.com/issues?utf8=✓&q=is%3Aissue+user%3Aatom
- Checked that there is not already an Atom package that provides the described functionality: https://atom.io/packages
Description
When enabling the experimental tree-sitter parsing system, there are some inconsistencies in the syntax highlighting.
Steps to Reproduce
- Enable experimental tree-sitter parsers
- Open a JavaScript file
- Syntax highlighting is not consistent with old parser
Expected behavior: For the most part, I would expect consistency in the syntax highlighting.
Actual behavior: There are quite a few inconsistencies between the old and new.
Reproduces how often: 100%
Versions
Atom version: 1.25.0-beta0
APM version: 1.19.0
OS: macOS Sierra 10.12.6
Additional Information
Here's a screenshot, with the old on the left and the new on the right:
My apologies if this is actually expected behavior - I do realize that tree-sitter aims to generate a more accurate syntax tree and that would inevitably cause some highlighting differences. Some of these things do seem to provide extra consistency, but I'm unsure if others are intended behavior or missing features. More specifically, I am curious about the following:
constdeclarations are no longer highlighted. This one seems like it's probably an improvement in consistency sinceletandvarare not highlighted orange.modulekeyword is no longer highlighted=sign is no longer highlighted (missing thesyntax--assignmentclass in the markup that enables this)requireis highlighted a different color. This one I could also see being an improvement, as function names in this color scheme are also blue, but withrequirebeing a special case, I wasn't sure.- the
=>arrow function symbol is no longer highlighted - the
thiskeyword is no longer highlighted
Any insight on the above mentioned items is much appreciated. I am still familiarizing myself with tree-sitter, so my apologies if this is all intentional behavior. I also realize this is an experimental feature, and I am running on the Beta version, so there may be bugs. If this is indeed an issue and I can be of any assistance in troubleshooting, let me know, I'd be happy to help!
Activity
Ben3eeE commentedon Feb 15, 2018
Thank you for testing out tree-sitter 🙇♂️
@maxbrunsfeld Here is some great early feedback on tree-sitter and the differences to the old textmate grammar 👀
maxbrunsfeld commentedon Feb 15, 2018
Thanks for the feedback @mmcbride1007!
Yeah, this was something I intentionally ditched because I want to avoid as much as possible having a given entity appear in different colors in different places. It's not feasible for us to track which variables are
constand which aren't throughout a file, so if we highlight it in orange when it's declared, it's going to appear in different colors in other places.Yeah, in general I've gotten rid of special highlighting for globals that are defined in specific environments (e.g. node.js). Things like
documentandUIEventno longer have special highlighting either. We could possibly add back the ability to do things like this later, but I don't see it as a super high priority; it's hard to know when it's desirable. For example, I sometimes have local variables calledwindowormoduleand ideally, they would not have special highlighting.These were not intentional! I will fix them shortly.
I agree, we should really highlight
thissome color. The problem is that today, on most themes, it gets highlighted the same as object properties:☝️ That's a lotta red. I would want to highlight
thisin a different color, but I couldn't find a class that would look good with the built-in One themes. @simurai might have ideas here.mikemcbride commentedon Feb 15, 2018
Thanks for the response! Totally makes sense with the decisions for consistency on those items.
re:
this(and other keywords, likesuper), in one of the syntax themes I maintain, which I based heavily off One Dark Syntax, I was able to get different colored highlighting by targeting some classes that added higher specificity to give it that orange color you see in the screenshot above:https://github.com/mikemcbride/electron-highlighter-syntax/blob/master/styles/syntax/javascript.less#L18-L20
Not sure if that's too hacky, but it seemed to work pretty consistently in my experience. Could be a starting point for a discussion.
Fix highlighting for '=>' and some operators
Zirro commentedon Feb 18, 2018
Another difference is that the tree-sitter grammar currently supports highlighting JSX by default, while this previously has been rejected for this repository.
I can see the arguments in favour of doing so, but it must also be noted that the inclusion of one unofficial extension to the language sets a precedence for adding more of them, should another framework rise to equally great prominence in the future.
winstliu commentedon Feb 18, 2018
Other than
this, I'm really missing the highlighting for objects. Theobjinobj.proporobj.method()is no longer colored in tree-sitter.maxbrunsfeld commentedon Feb 18, 2018
@50Wliu That 'object' highlighting never really made sense to me. For one thing it didn't actually correctly identify objects (as opposed to other data types like numbers, strings or arrays). It also caused entities to appear in different colors in different places. For example:
At no point is
xan object. The highlighting would also make it look like two different things in the two places. I guess it depends on what you want out of syntax highlighting, but that one always seemed counterproductive to me.winstliu commentedon Feb 18, 2018
Good point, I've never thought about it like that. Something still seems "missing" to me though, I guess I'll think about it some more.
maxbrunsfeld commentedon Jul 30, 2018
Alright, we've addressed most of the inconsistencies. #587 adds distinct highlighting for
moduleandrequire, as well as const names that use the UPPER_CASE naming convention.There are certain differences with the TextMate grammar that we currently don't plan to address:
Assigning arrow functions to object properties like
a.b = () => c. We highlightbjust like any other object property whereas the TextMate grammar would (sometimes) highlight it as a function. This is doable with Tree-sitter but would require extending our AST-based highlighting system to support non-local relationships ("assignment expression where the RHS is a function") and I don't think it's worth it at the moment.constdeclarations. We don't apply special highlighting to variables declared asconst. This feature of the TextMate grammar is undesirable in my view because it causes variables to appear in different colors in different places. Until we are able to trace usages of variables back to their definition, and account for that in the highlighting, we're not going to be able to do this.qwelias commentedon Feb 18, 2019