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

Refactor queries for ecma based languages #7207

Conversation

yusuphgammut
Copy link
Contributor

@yusuphgammut yusuphgammut commented Jun 1, 2023

Intro

This PR builds on top of #3301, which created the "fake" ecma language to store queries that are common to both javascript and typescript.

The problem

Ecma-based languages share many traits, but it's not a linear and simple inheritance. Sometimes we want to inherit queries from one of those languages without inheriting the queries that that language inherits. The current state of inheritance is the following:

CURRENT INHERITANCE MODEL 
-----------------
javascript <--- ecma
jsx <---------- ecma
typescript <--- ecma
tsx <---------- jsx, typescript

There are three main examples why this current model generates limitations and bugs:

  • In tsx, the < and > symbols are being highlighted as operators in the context of generic types like MyGenericType<GenericArgument>. I wanted to fix this (change it to punctuation), but I realized that tsx was inheriting highlight queries from both jsx and typescript (check here), and both of them were inheriting ecma. The problem is that the queries in ecma for < and > (that assigned them to operator highlight, here) inherited by jsx where overwriting the queries for generic types in typescript (here). If I then changed the order and added typescript first, then those same operator queries in ecma inherited by typescript overwrite the < and > symbols for jsx elements (here), highlighting them as operators.
  • Aside from the particular problem above, ecma is being inherited twice in tsx, which is not ideal.
  • Javascript and typescript treesitter grammars have diverged in some parts, and that's specially evident in relation to parameters, where javascript defines formal_parameters and typescript adds required_parameter and optional_parameter. This affects the way queries should be written in particular for highlights and locals. We then should put those specific queries under javascript and typescript (not under ecma) and those queries should be inherited by jsx and tsx respectively. However, tsx also benefits from inheriting things from jsx, but it shouldn't inherit javascript specific queries, just jsx queries.

We can all agree that the javascript ecosystem is quite messy and that it presents some unique challenges for tooling developers.

So, as stated above, Ecma based languages share many traits, but it's not a linear and simple inheritance. Because of this we want to share as many queries as possible while avoiding nested inheritance in the queries that can make query behavior unpredictable due to unexpected precedence.

The solution

This PR proposes "public" and "private" versions for javascript, jsx, and typescript, that share the same name, but the "private" version name starts with an underscore (ecma already exists as a sort of base "private" language). This allows the "private" versions to host the specific queries of the language excluding any ; inherits statement, in order to make them safe to be inherited by the "public" version of the same language and other languages as well. This helps fixing the problems above. The tsx language doesn't have a "private" version given it doesn't need to be inherited by other languages.

INHERITANCE MODEL 
-----------------
javascript <--- _javascript, ecma
jsx <---------- _jsx, _javascript, ecma
typescript <--- _typescript, ecma
tsx <---------- _jsx, _typescript, ecma

In summary, I've...

  1. Created _javascript, _jsx, and _typescript languages (called "private" above).
  2. Moved existing queries to those new "private" versions.
  3. Changed query files in the javascript, jsx, typescript, and tsx languages (called "public" above) to only inherit from "private" languages.

In addition to this, I've adjusted some highlights and locals queries to take advantage of this inheritance and fix existing problems. I've put special attention to queries related to parameters, given it is a place where javascript and typescript treesitter grammars have diverged the most (you can use the following files to test that, just add the correct extensions:
test-js.txt, test-jsx.txt, test-ts.txt, test-tsx.txt)

Textobjects can be improved too, but I plan to address that in another PR. Let me know if this seems like a good strategy to tackle this issue.

@kirawi kirawi added the A-language-support Area: Support for programming/text languages label Jun 5, 2023
@yusuphgammut yusuphgammut force-pushed the ecma-ecosystem-queries-improvements branch from ca1634f to c381649 Compare June 6, 2023 21:36
@yusuphgammut
Copy link
Contributor Author

@the-mikedavis Sorry for tagging you directly. You were the author of #3301, so I think it'll be good to know your opinion on this proposal. I know it touches quite many files, but it's mostly moving existing queries to new files. Let me know your thoughts.

@yusuphgammut yusuphgammut force-pushed the ecma-ecosystem-queries-improvements branch from bdc639a to 62020e3 Compare June 19, 2023 18:05
@yusuphgammut
Copy link
Contributor Author

yusuphgammut commented Jun 19, 2023

This proposal is mostly finished. @kirawi I was wondering if you can add the "S-waiting-on-review" label to this PR so that anyone from the core team can take a look when they have time available.

@gabydd gabydd added the S-waiting-on-review Status: Awaiting review from a maintainer. label Jun 20, 2023
Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes sense. It fragments the queries a little but I don't see another way around it without undoing all of the inherits

runtime/queries/javascript/highlights.scm Outdated Show resolved Hide resolved
Ecma based languages share many traits. Because of this we want to share as many
queries as possible while avoiding nested inheritance that can make query
behavior unpredictable due to unexpected precedence. To achieve that, this PR
implements "public" and "private" versions for javascript, jsx, and typescript,
that share the same name, but the "private" version name starts with an
underscore (ecma already exists as sort of the base "private" language). This
allows the "private" versions to host the specific queries of the language
excluding any "inherits" statement, in order to make them safe to be inherited
by the "public" version of the same language and other languages as well.
The tsx language does not have a "private" version given it doesn't need to be
inherited by other languages.

In addition to this, I've adjusted some highlights and locals queries to take
advantage of this inheritance and fix existing problems. I've put special
attention to queries related to parameters, given it is a place where
javascript and typescript treesitter grammars have diverged.
@yusuphgammut yusuphgammut force-pushed the ecma-ecosystem-queries-improvements branch from 62020e3 to be1aba8 Compare June 28, 2023 02:00
@yusuphgammut
Copy link
Contributor Author

yusuphgammut commented Jun 28, 2023

I've rebased this branch with master so we have the latest version. If there are no additional comments, it is safe to merge this with master.

@pascalkuthe pascalkuthe merged commit 607b426 into helix-editor:master Jul 9, 2023
6 checks passed
mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request Jun 2, 2024
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-language-support Area: Support for programming/text languages S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants