Syntax inconsistencies using tree-sitter

[mikemcbride]

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

1. Enable experimental tree-sitter parsers
2. Open a JavaScript file
3. 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:

• const declarations are no longer highlighted. This one seems like it's probably an improvement in consistency since let and var are not highlighted orange.
• module keyword is no longer highlighted
• = sign is no longer highlighted (missing the syntax--assignment class in the markup that enables this)
• require is highlighted a different color. This one I could also see being an improvement, as function names in this color scheme are also blue, but with require being a special case, I wasn't sure.
• the => arrow function symbol is no longer highlighted
• the this keyword 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!

[Ben3eeE]

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]

Thanks for the feedback @mmcbride1007!

const declarations are no longer highlighted.

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 const and 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.

module keyword is no longer highlighted

require is highlighted a different color.

Yeah, in general I've gotten rid of special highlighting for globals that are defined in specific environments (e.g. node.js). Things like document and UIEvent no 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 called window or module and ideally, they would not have special highlighting.

= sign is no longer highlighted

the => arrow function symbol is no longer highlighted

These were not intentional! I will fix them shortly.

the this keyword is no longer highlighted

I agree, we should really highlight this some 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 this in 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]

Thanks for the response! Totally makes sense with the decisions for consistency on those items.

re: this (and other keywords, like super), 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.

[Zirro]

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]

Other than this, I'm really missing the highlighting for objects. The obj in obj.prop or obj.method() is no longer colored in tree-sitter.

[maxbrunsfeld]

@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:

let x = "hello";
console.log(x.length);

At no point is x an 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]

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]

Alright, we've addressed most of the inconsistencies. #587 adds distinct highlighting for module and require, 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 highlight b just 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.

• const declarations. We don't apply special highlighting to variables declared as const. 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.