Comments to the new tree sitter implementation

Comments to the new tree sitter implementation

[Theodor Thornhill]

[-- Attachment #1: Type: text/plain, Size: 6672 bytes --]

Hi there!

I think I finally understand most of the important bits of the tree
sitter implementation as seen from a major mode maintainer, and I want
to share my thoughts so that we can have something concrete to work on
when the feature branch hits.

1. New parent-beginning-of-line preset

For typescript-mode more often that not you want to find some close
parent, then go to beginning of line to calculate the offset.  A
suggested implementation of this is:

```elisp
(defun parent-beginning-of-line ()
  (lambda (node parent bol &rest _)
    (when-let ((parent (tree-sitter-node-parent node)))
      (save-excursion
        (goto-char (tree-sitter-node-start parent))
        (back-to-indentation)
        (tree-sitter-node-start
         (tree-sitter-node-at (point) (point) 'tree-sitter-tsx))))))
```

This works as following:

```typescript
function foo() {
  bar(() => ({
    baz
  }))
}
```

baz will know where to indent because it finds the closest {, then goes
to the start of 'bar' and returns that nodes position.  The same applies
to how 'bar' finds its closest '{', then goes to the first letter of
'function'.

To do all this you need a rule such as
```elisp
((node-is "}" (parent-beginning-of-line) 0)
```
and

```elisp
((parent-is "object_or_some_other_parent" (parent-beginning-of-line) 2)
```

inside of the 'simple-indent-rules'

2. Performance of query preset
The performance of the query preset is not good.  I finally understood
how the indentation engine worked by using that preset, but only after 4
or 5 queries indenting a file took several seconds.  Either it should be
marked with a caveat, or improved in some way.  I didn't get a
profiler-report before removing them.  Removing them and supplying
equivalent rules by using the 'parent-beginning-of-line' function made
indenting huge files instantaneous.

3. Performance of font-locking

The font-locking also can be flaky, and I'm not sure what happens here
either.  It may be the current implementation of typescript-mode, it
could be in tree-sitter.  Does the complexity increase proportionally to
the granularity of the queries, maybe?

Steps to reproduce:
  1. paste in some file, for example this:
    https://raw.githubusercontent.com/tree-sitter/tree-sitter-javascript/master/grammar.js
  2. start scrolling using 'C-n'
  3. observe enourmous lags when scrolling.

This time I got a profiler report:
```
       15588  97% - command-execute
       15588  97%  - call-interactively
       15417  96%   - funcall-interactively
       15417  96%    - previous-line
       15417  96%     - line-move
       13677  85%      - line-move-visual
       13655  85%       - vertical-motion
       13655  85%        - jit-lock-function
       13651  85%         - jit-lock-fontify-now
       13651  85%          - jit-lock--run-functions
       13651  85%           - run-hook-wrapped
       13651  85%            - #<compiled -0x156eafb94cdd4083>
       13651  85%             - font-lock-fontify-region
       13647  85%              - tree-sitter-font-lock-fontify-region
         762   4%               + tree-sitter-query-capture
         112   0%               + font-lock-default-fontify-region
           4   0%               + font-lock-unfontify-region
        1716  10%      - line-pixel-height
        1716  10%       - jit-lock-function
        1716  10%        - jit-lock-fontify-now
        1716  10%         - jit-lock--run-functions
        1716  10%          - run-hook-wrapped
        1716  10%           - #<compiled -0x156ecffbbac1d883>
        1716  10%            - font-lock-fontify-region
        1716  10%             - tree-sitter-font-lock-fontify-region
         108   0%              - tree-sitter-query-capture
         104   0%               - tree-sitter-expand-pattern
          36   0%                - tree-sitter-expand-pattern
           8   0%                   tree-sitter-expand-pattern
          12   0%              - font-lock-default-fontify-region
          12   0%               - font-lock-fontify-syntactically-region
          12   0%                  syntax-ppss
         171   1%   + byte-code
         156   0% + redisplay_internal (C function)
          89   0% + jsonrpc--process-filter
          63   0% + eldoc-pre-command-refresh-echo-area
          17   0% + timer-event-handler
           4   0%   internal-timer-start-idle
           0   0% + ...
```
As you can see most of the time is spent inside of
'tree-sitter-font-lock-fontify-region'.  I'm not sure how to get even
more granular information than this.

4. How do I make sure that the null node indents without the catch all
I want to be able to type this:

```typescript
const foo = () => {
|                       // This is the indentation I get
}
```

```typescript
const foo = () => {
  |                     // This is the indentation I want
}
```

In some cases the parser returns null, which I can match on using the
provided preset.  However, usually tree-sitter-tsx returns something
like:

```elisp
(some_parent (object))
```

where object is not a null node.  I don't want to match on this
specifically, for two reasons

  1. I have to make rules for every case where this happens
  2. I need to be mindful of the precedence of rules inside of the simple-indent-rules

This is because when I start to type something, the ast changes to:

```elisp
(some_parent (object (some_child)))
```

It is the 'some_child' I want to indent, not the object itself.  As such
a rule such as:

```elisp
((parent-is "object") parent 2)
```

will not work for this problem.  It will indent the 'some_child', but
not the cursor inside of the squigglies.

Is there a way to do this with a simple preset?

5. Bugs in error-handling during font-locking

When typing it seems tree-sitter-tsx returns ERROR (which is expected
during typing), but then emacs shifts around the font-locking and
returning broken font-locking.  Sometimes it recovers, sometimes it does
not.  Maybe we shouldn't font-lock when there are errors, or at least
don't change the font-locking from outside of the node where there is an
error?  Now it looks like the whole file errors out, but it is really
only one node, or maybe it bleeds into its sibling.

I attach the screenshots I've already sent to make documentation a
little easier.


Lastly I want to say that I'm really impressed by this, Yuan! Thank you,
and I'm looking forward to getting it mainlined.

I'll also add the link to the typescript-mode I'm making atm, so that
the context of what I'm talking about may be a little clearer:

https://github.com/emacs-typescript/typescript.el/blob/feature/tsx-support/typescript-ts.el

All the best,
Theodor


[-- Attachment #2: correct.png --]
[-- Type: image/png, Size: 66991 bytes --]

[-- Attachment #3: wrong.png --]
[-- Type: image/png, Size: 65645 bytes --]

Re: Comments to the new tree sitter implementation

[Stefan Monnier]

> 1. New parent-beginning-of-line preset
>
> For typescript-mode more often that not you want to find some close
> parent, then go to beginning of line to calculate the offset.  A
> suggested implementation of this is:

FWIW, I think this is a "quick hack" which gives fairly good results at
little effort but doesn't help in the long run because you'll have to
refine it sooner or later: it's never actually right, it's only ever
right by accident.

> ```typescript
> function foo() {
>   bar(() => ({
>     baz
>   }))
> }
> ```

What if there's another argument to `bar` after the
`() => ({ baz })` function, or if the user just prefers

    function foo() {
      bar(() => ({
            baz
          }))
    }

[ Or if the standard indentation rules prescribe something like this.  ]


        Stefan

Re: Comments to the new tree sitter implementation

[Theodor Thornhill]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> 1. New parent-beginning-of-line preset
>>
>> For typescript-mode more often that not you want to find some close
>> parent, then go to beginning of line to calculate the offset.  A
>> suggested implementation of this is:
>
> FWIW, I think this is a "quick hack" which gives fairly good results at
> little effort but doesn't help in the long run because you'll have to
> refine it sooner or later: it's never actually right, it's only ever
> right by accident.
>

Yes, absolutely.  Will indentation rules ever be perfect?  I'm thinking
a good approach is to provide something simple enough to not be too
controversial, and make sure a formatter takes care of the rest.  I
guess if we should cover all variants of the problems you outline I'd
have to provide a rule for all cases, don't I?  Or is there another way
you see?

>> ```typescript
>> function foo() {
>>   bar(() => ({
>>     baz
>>   }))
>> }
>> ```
>
> What if there's another argument to `bar` after the
> `() => ({ baz })` function, or if the user just prefers
>
>     function foo() {
>       bar(() => ({
>             baz
>           }))
>     }

In the first case you described my example function is surely wrong.  In
the second case I'm thinking either we provide some
'tree-sitter-simle-indent-override' defcustom that people can put their
own indentation rules into should they have some particular indentation
need

For typescript I'm now for the time being using the base 'prettier'
formatter just as a guidance tool for how to indent, as well as observe
how vscode behaves when typing.  Because the way Emacs handles
indentation is very different from how other editors do it (Emacs is
more of a formatter than the usual tab + shift-tab indenting method of
other editors), we have to make sure that both ways of indenting is
pleasant.  By this I mean that typing out a file from scratch should
indent as we type like how vscode or vim does it, while "C-x h TAB"
should work like a formatter.

With using prettiers default rules it would return

```typescript
function foo() {
  bar(
    () => ({
      baz,
    }),
    foo
  );
}
```

for the case where there's an extra arg where you specified.  This was
my reasoning for why this preset was ok enough.  It may not be for the
general case, though.  And fwiw, vscode indents it like this while
typing:

```typescript
function foo() {
   bar(() => ({
     baz
   }),
   foo
   )
}
```

This looks more like "I'm not even gonna try, just return 0 indent."

Re: Comments to the new tree sitter implementation

[Theodor Thornhill]

Theodor Thornhill <theo@thornhill.no> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> 1. New parent-beginning-of-line preset
>>>
>>> For typescript-mode more often that not you want to find some close
>>> parent, then go to beginning of line to calculate the offset.  A
>>> suggested implementation of this is:
>>
>> FWIW, I think this is a "quick hack" which gives fairly good results at
>> little effort but doesn't help in the long run because you'll have to
>> refine it sooner or later: it's never actually right, it's only ever
>> right by accident.
>>

BTW, the preset in an of itself is not a quick hack, as far as I can
tell.  It's just that it might not be the best guess in every case given
the example I provided.  But for the case where what you want is to find
the first parent, then move to beginning of indentation, I guess it's
fine?  Or am I wrong here as well?

I mean, for the try_statement, doing this check should be sufficent, I
believe.

Theo

Re: Comments to the new tree sitter implementation

[Stefan Monnier]

> the example I provided.  But for the case where what you want is to find
> the first parent, then move to beginning of indentation, I guess it's
> fine?

What I mean is that When this rule gives the right answer it's just by
lucky accident (usually because of other conventions that the writer
follows which causes the desired "anchor point" to be the node that's at
the beginning of the line).

A good indentation rule should try not to depend so arbitrarily on where
line-feeds were inserted in the code.  This is more important for code
that's not (fully) written by humans.  As a concrete example, the
auto-fill-mode support in SMIE behaves worse with such rules (because it
works by trying to insert a line-feed at different places on the line
and choose the spot that most reduced indentation, and the above rule
would lead to choosing poor places to insert the line-feeds because it
basically assumes that the line-feeds were placed with care).


        Stefan

Re: Comments to the new tree sitter implementation

[Yuan Fu]

> 
> 
> Lastly I want to say that I'm really impressed by this, Yuan! Thank you,
> and I'm looking forward to getting it mainlined.
> 
> I'll also add the link to the typescript-mode I'm making atm, so that
> the context of what I'm talking about may be a little clearer:
> 
> https://github.com/emacs-typescript/typescript.el/blob/feature/tsx-support/typescript-ts.el
> 
> All the best,
> Theodor
> 
> <correct.png><wrong.png>

Sorry that I can’t get back to you very soon. I’ve been incredibly busy recently. I’ll find time to look at your points and try to debug the problems you mention with typescript-ts.el. 

Eli, I was trying to push my branch on savannah but (embarrassingly) hasn’t succeed yet. I think if I create an account and add my ssh key and clone by user-name@git.savannah.gnu.org:/srv/git/emacs.git, I should be able to push a branch? I’m having some problem with authenticating with the key, hopefully that can be sorted out soon. Once I can push to my repo, how do I create a scratch branch under the main Emacs repo?

Yuan

Re: Comments to the new tree sitter implementation

[Eli Zaretskii]

> From: Yuan Fu <casouri@gmail.com>
> Date: Mon, 25 Apr 2022 11:52:53 -0700
> Cc: emacs-devel <emacs-devel@gnu.org>
> 
> Eli, I was trying to push my branch on savannah but (embarrassingly) hasn’t succeed yet. I think if I create an account and add my ssh key and clone by user-name@git.savannah.gnu.org:/srv/git/emacs.git, I should be able to push a branch? 

Yes.

> Once I can push to my repo, how do I create a scratch branch under the main Emacs repo?

Just create a local branch under feature/, make the changes there,
then push.  There's nothing special here, just Git commands.  For
example, "git push -u" should do the job and push the branch to
Savannah, AFAIU.

Re: Comments to the new tree sitter implementation

[Theodor Thornhill]

Hi, there!

>
> Sorry that I can’t get back to you very soon. I’ve been incredibly
> busy recently. I’ll find time to look at your points and try to debug
> the problems you mention with typescript-ts.el.
>

No worries.  I'll just note one more thing because I remember it now:
Why are you calling the font lock default function after tree sitter has
done its job?  That seems like the wrong thing for several reasons:

1. Shouldn't tree sitter be sufficient?
2. It makes the default function get higher precedence than tree-sitter,
causing some troublesome behavior.

Specifically, if you set single-quote as a string in the syntax table,
to make the major mode understand it as a string delimiter, it will mess
up comments.  This example snippet will make the rest of the file
fontlock as a string:

```typescript
function foo() {
  // don't mark the rest of the file as a string because of this:
  //    ^
  const dont = 3;
  const do = "":
  const it = {};
}
```

If the last two lines of `tree-sitter-font-lock-fontify-region` are
removed, things appear to work as normal.  It also works fine with
things such as eglot marking a variable as unused in the comment face
etc.

All the best,
Theodor

Re: Comments to the new tree sitter implementation

[Yuan Fu]

> 
> Hi, there!
> 
>> 
>> Sorry that I can’t get back to you very soon. I’ve been incredibly
>> busy recently. I’ll find time to look at your points and try to debug
>> the problems you mention with typescript-ts.el.
>> 
> 
> No worries.  I'll just note one more thing because I remember it now:
> Why are you calling the font lock default function after tree sitter has
> done its job?  That seems like the wrong thing for several reasons:
> 
> 1. Shouldn't tree sitter be sufficient?
> 2. It makes the default function get higher precedence than tree-sitter,
> causing some troublesome behavior.

That is intended. Maybe I should emphasize in the manual that major modes shouldn’t set regex font-locks if tree-sitter is enabled (added to todo list). I left regex font-lock enabled and to have higher precedence because many Emacs features uses regex font-lock for arbitrary highlighting. One example is highlight-regexp, but also highlights for outline, and packages like hl-todo, rainbow-mode, etc. It makes sense for their highlights to override the “base” highlight applied by tree-sitter.

> 
> Specifically, if you set single-quote as a string in the syntax table,
> to make the major mode understand it as a string delimiter, it will mess
> up comments.  This example snippet will make the rest of the file
> fontlock as a string:
> 
> ```typescript
> function foo() {
>  // don't mark the rest of the file as a string because of this:
>  //    ^
>  const dont = 3;
>  const do = "":
>  const it = {};
> }
> ```
> 
> If the last two lines of `tree-sitter-font-lock-fontify-region` are
> removed, things appear to work as normal.  It also works fine with
> things such as eglot marking a variable as unused in the comment face
> etc.

IIUC this can be solved by disabling the syntax-table-based font-locking by setting KEYWORDS-ONLY in font-lock-defaults.

Thanks,
Yuan

Re: Comments to the new tree sitter implementation

[Theodor Thornhill]

Yuan Fu <casouri@gmail.com> writes:

>
> That is intended. Maybe I should emphasize in the manual that major
> modes shouldn’t set regex font-locks if tree-sitter is enabled (added
> to todo list). I left regex font-lock enabled and to have higher
> precedence because many Emacs features uses regex font-lock for
> arbitrary highlighting. One example is highlight-regexp, but also
> highlights for outline, and packages like hl-todo, rainbow-mode,
> etc. It makes sense for their highlights to override the “base”
> highlight applied by tree-sitter.
>

Yeah, you're probably correct

>
> IIUC this can be solved by disabling the syntax-table-based font-locking by setting KEYWORDS-ONLY in font-lock-defaults.
>
Thanks!

Theodor

Re: Comments to the new tree sitter implementation

[Yuan Fu]

> On Apr 25, 2022, at 12:07 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Yuan Fu <casouri@gmail.com>
>> Date: Mon, 25 Apr 2022 11:52:53 -0700
>> Cc: emacs-devel <emacs-devel@gnu.org>
>> 
>> Eli, I was trying to push my branch on savannah but (embarrassingly) hasn’t succeed yet. I think if I create an account and add my ssh key and clone by user-name@git.savannah.gnu.org:/srv/git/emacs.git, I should be able to push a branch? 
> 
> Yes.
> 
>> Once I can push to my repo, how do I create a scratch branch under the main Emacs repo?
> 
> Just create a local branch under feature/, make the changes there,
> then push.  There's nothing special here, just Git commands.  For
> example, "git push -u" should do the job and push the branch to
> Savannah, AFAIU.

I finally figured out why I can’t access savannah with help from savannah folks: I’m not a member of the Emacs repository. I thought I don’t need membership to create a feature/scratch branch, and I thought I get a repository or my own and do things like pull request like on GitHub :-)

Yuan

Re: Comments to the new tree sitter implementation

[Eli Zaretskii]

> From: Yuan Fu <casouri@gmail.com>
> Date: Fri, 6 May 2022 23:49:27 -0700
> Cc: Theodor Thornhill <theo@thornhill.no>,
>  emacs-devel@gnu.org
> 
> I finally figured out why I can’t access savannah with help from savannah folks: I’m not a member of the Emacs repository. I thought I don’t need membership to create a feature/scratch branch, and I thought I get a repository or my own and do things like pull request like on GitHub :-)

You mean, you are not a member of the Emacs project, I guess.

Then create a Savannah account (if you don't have one already), and
apply for membership in the Emacs project.

Re: Comments to the new tree sitter implementation

[Yuan Fu]

> On May 7, 2022, at 12:11 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Yuan Fu <casouri@gmail.com>
>> Date: Fri, 6 May 2022 23:49:27 -0700
>> Cc: Theodor Thornhill <theo@thornhill.no>,
>> emacs-devel@gnu.org
>> 
>> I finally figured out why I can’t access savannah with help from savannah folks: I’m not a member of the Emacs repository. I thought I don’t need membership to create a feature/scratch branch, and I thought I get a repository or my own and do things like pull request like on GitHub :-)
> 
> You mean, you are not a member of the Emacs project, I guess.
> 
> Then create a Savannah account (if you don't have one already), and
> apply for membership in the Emacs project.

Cool. I did that.

Yuan