Average-user-facing interface for tree-sitter

Average-user-facing interface for tree-sitter

[Yuan Fu]

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

From the suggestions I collected from the old thread, here is my proposal:

We define a custom option treesit-settings (we can discuss the name later), which controls whether to enable/disable tree-sitter for each major mode, and the default preference, like this:

(defcustom treesit-settings '((t nil nil))
  "Tree-sitter toggles for major modes.

A list of (MODE ENABLE INHERIT).  MODE is a major mode, ENABLE
can be one of the following:

  demand => Demand the use of tree-sitter, warn if can't enable.
  t => Enable if available
  nil => Don't enable

If INHERIT is nil, the setting does't apply to derived modes of
MODE, if t, the setting does apply.

If MODE is t, the settings applies to all the modes which don't
have any setting.  INHERIT doesn't matter for this special
default setting."
  :type …)

I added the INHERIT flag because I had bad experience trying to enable eglot for python-mode but not for sage-mode, which derives from python-mode.

Then a major mode can just use (treesit-enable-p ‘python-mode ‘python) to check whether to use tree-sitter, this function checks that:
- tree-sitter is built with Emacs
- buffer size is within limit
- language definition is available
- tree-sitter is enabled for this mode according to treesit-settings

This function raises warnings w/ explanation if the user sets ‘demand for the mode and we can’t enable tree-sitter.

I attached a screenshot of treesit-settings in (my) Customize. This should be easy to use and understand for any average user (right?)

Yuan



[-- Attachment #2.1: Type: text/html, Size: 2595 bytes --]

[-- Attachment #2.2: Screen Shot 2022-10-11 at 11.00.40 PM.png --]
[-- Type: image/png, Size: 63217 bytes --]

Re: [SPAM UNSURE] Average-user-facing interface for tree-sitter

[Stephen Leake]

Yuan Fu <casouri@gmail.com> writes:

> From the suggestions I collected from the old thread, here is my proposal:
>
> We define a custom option treesit-settings (we can discuss the name
> later), which controls whether to enable/disable tree-sitter for each
> major mode, and the default preference, like this:
>
> (defcustom treesit-settings '((t nil nil))
>   "Tree-sitter toggles for major modes.
>
> A list of (MODE ENABLE INHERIT).  MODE is a major mode, ENABLE
> can be one of the following:
>
>   demand => Demand the use of tree-sitter, warn if can't enable.
>   t => Enable if available
>   nil => Don't enable
>
> If INHERIT is nil, the setting does't apply to derived modes of
> MODE, if t, the setting does apply.
>
> If MODE is t, the settings applies to all the modes which don't
> have any setting.  INHERIT doesn't matter for this special
> default setting."
>   :type …)

This is reasonable, but I think we will need finer control. LSP has
dozens of features that can be individually supported or not by a given
server. I don't think Emacs needs to go that far (although eglot does
support that). I think separate control for face, indent, xref, and
maybe imenu would be reasonable.

In the above, perhaps ENABLE could be a symbol if it controls all
features for a mode, or a plist to specify features separately:

(mode (:face ENABLE :indent ENABLE ...) INHERIT).

I'm not sure if INHERIT needs to be similarly split.

-- 
-- Stephe

Re: Average-user-facing interface for tree-sitter

[Lars Ingebrigtsen]

Yuan Fu <casouri@gmail.com> writes:

> From the suggestions I collected from the old thread, here is my proposal:
>
> We define a custom option treesit-settings (we can discuss the name
> later), which controls whether to enable/disable tree-sitter for each
> major mode, and the default preference, like this:
>
> (defcustom treesit-settings '((t nil nil))
>   "Tree-sitter toggles for major modes.

Hm...  well, there's also modes that are "pure treesit", and there are
(or will be) alternate modes with and without tree-sitter.

I think users basically fall into two camps: The ones that want to have
tree-sitter in all modes, and ones that want to enable it in specific
modes (and ones that don't want it at all).  (Note Computer
Science-mandated off-by-one error.)

This suggests to me that we should just use the normal minor mode
machinery that we have for these things.

That is, people that want treesit in python-mode will say:

(add-hook 'python-mode 'treesit-mode)

And people that want it everywhere will say:

(global-treesit-mode)

This mode will, in addition to switching `treesit-mode' on everywhere,
also set up `major-mode-remap-alist', so that `typescript-mode' is
mapped to `ts-mode', and `c-mode' is mapped to `treesit-c-mode' (which
I'm sure somebody is going to write in a couple of days), etc.

Re: [SPAM UNSURE] Average-user-facing interface for tree-sitter

[Stephen Leake]

Stephen Leake <stephen_leake@stephe-leake.org> writes:

> Yuan Fu <casouri@gmail.com> writes:
>
>> From the suggestions I collected from the old thread, here is my proposal:
>>
>> We define a custom option treesit-settings (we can discuss the name
>> later), which controls whether to enable/disable tree-sitter for each
>> major mode, and the default preference, like this:
>>
>> (defcustom treesit-settings '((t nil nil))
>>   "Tree-sitter toggles for major modes.
>>
>> A list of (MODE ENABLE INHERIT).  MODE is a major mode, ENABLE
>> can be one of the following:
>>
>>   demand => Demand the use of tree-sitter, warn if can't enable.
>>   t => Enable if available
>>   nil => Don't enable
>>
>> If INHERIT is nil, the setting does't apply to derived modes of
>> MODE, if t, the setting does apply.
>>
>> If MODE is t, the settings applies to all the modes which don't
>> have any setting.  INHERIT doesn't matter for this special
>> default setting."
>>   :type …)
>
> This is reasonable, but I think we will need finer control. LSP has
> dozens of features that can be individually supported or not by a given
> server. I don't think Emacs needs to go that far (although eglot does
> support that). I think separate control for face, indent, xref, and
> maybe imenu would be reasonable.
>
> In the above, perhaps ENABLE could be a symbol if it controls all
> features for a mode, or a plist to specify features separately:
>
> (mode (:face ENABLE :indent ENABLE ...) INHERIT).
>
> I'm not sure if INHERIT needs to be similarly split.

As I mentioned in my other email, also signature-help, completion,
diagnostics.

-- 
-- Stephe

Re: Average-user-facing interface for tree-sitter

[Robert Pluim]

>>>>> On Thu, 13 Oct 2022 08:22:53 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:

    Lars> I think users basically fall into two camps: The ones that want to have
    Lars> tree-sitter in all modes, and ones that want to enable it in specific
    Lars> modes (and ones that don't want it at all).  (Note Computer
    Lars> Science-mandated off-by-one error.)

off-by-two: thereʼs also the "I want syntax highlighting and
indentation to work out of the box, donʼt bother me with
implementation details" camp, which kind of implies that we should
turn on tree-sitter by default if itʼs available.

Robert
-- 

Re: Average-user-facing interface for tree-sitter

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

> off-by-two: thereʼs also the "I want syntax highlighting and
> indentation to work out of the box, donʼt bother me with
> implementation details" camp, which kind of implies that we should
> turn on tree-sitter by default if itʼs available.

I'm assuming Emacs will default `global-treesit-mode' on by default at
some point (but not in Emacs 29.1, obviously) if things work out well.

Re: Average-user-facing interface for tree-sitter

[Po Lu]

Robert Pluim <rpluim@gmail.com> writes:

> off-by-two: thereʼs also the "I want syntax highlighting and
> indentation to work out of the box, donʼt bother me with
> implementation details" camp, which kind of implies that we should
> turn on tree-sitter by default if itʼs available.

That's camp 1.

I fall somewhere in between.  My hope is that Emacs and major modes will
still support being run without tree-sitter.

Re: Average-user-facing interface for tree-sitter

[Robert Pluim]

>>>>> On Thu, 13 Oct 2022 17:32:33 +0800, Po Lu <luangruo@yahoo.com> said:

    Po Lu> Robert Pluim <rpluim@gmail.com> writes:
    >> off-by-two: thereʼs also the "I want syntax highlighting and
    >> indentation to work out of the box, donʼt bother me with
    >> implementation details" camp, which kind of implies that we should
    >> turn on tree-sitter by default if itʼs available.

    Po Lu> That's camp 1.

How can they know the want tree-sitter everywhere if they donʼt want
to be bothered with how syntax highlighting and indentation are
achieved? Anyway, the taxonomy of potential tree-sitter users is not
important

    Po Lu> I fall somewhere in between.  My hope is that Emacs and major modes will
    Po Lu> still support being run without tree-sitter.

I think that will be true, but suspect the non-tree-sitter experience
will not keep up.

Robert
-- 

Re: Average-user-facing interface for tree-sitter

[Daniel Martín]

Robert Pluim <rpluim@gmail.com> writes:

> off-by-two: thereʼs also the "I want syntax highlighting and
> indentation to work out of the box, donʼt bother me with
> implementation details" camp, which kind of implies that we should
> turn on tree-sitter by default if itʼs available.
>

I think it's still early to enable Tree-sitter by default.  We're not
sure about the trade-offs compared to the current implementation.  I'd
leave it off by default, at least for Emacs 29, even if the Tree-sitter
library is installed.

I agree with you that we should make it as transparent to users as
possible.  Enabling something like global-treesit-mode should
automatically make major modes and some minor modes (code folding?) use
Tree-sitter if possible (and they should fallback to the
non-Tree-sitter-based logic if the Tree-sitter-based one is not
implemented yet).

For advanced users, we can offer the buffer-local minor mode, and even
offer the "per feature" toggles that were suggested in this thread.

Re: [SPAM UNSURE] Re: Average-user-facing interface for tree-sitter

[Stephen Leake]

Robert Pluim <rpluim@gmail.com> writes:

>>>>>> On Thu, 13 Oct 2022 08:22:53 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:
>
>     Lars> I think users basically fall into two camps: The ones that want to have
>     Lars> tree-sitter in all modes, and ones that want to enable it in specific
>     Lars> modes (and ones that don't want it at all).  (Note Computer
>     Lars> Science-mandated off-by-one error.)
>
> off-by-two: thereʼs also the "I want syntax highlighting and
> indentation to work out of the box, donʼt bother me with
> implementation details" camp, which kind of implies that we should
> turn on tree-sitter by default if itʼs available.

Depends on who "we" is here; the mode author may be in the best
position to know, or the "correct" choice may also depend on user
preference.

Which is why ada-mode 8.0 supports user choice of eglot or wisi for each
feature; the choice depends on exactly what the user is doing with
ada-mode, and how hard they want to work to install it.

-- 
-- Stephe

Re: Average-user-facing interface for tree-sitter

[Stefan Kangas]

Robert Pluim <rpluim@gmail.com> writes:

>     Po Lu> I fall somewhere in between.  My hope is that Emacs and major modes will
>     Po Lu> still support being run without tree-sitter.
>
> I think that will be true, but suspect the non-tree-sitter experience
> will not keep up.

Yes, there should be even less motivation to maintain the old code once
we have tree-sitter support.  Which sounds to me like a good thing, as
it frees up time to work on other stuff.

Re: Average-user-facing interface for tree-sitter

[Stefan Monnier]

> (defcustom treesit-settings '((t nil nil))
>   "Tree-sitter toggles for major modes.
>
> A list of (MODE ENABLE INHERIT).  MODE is a major mode, ENABLE
> can be one of the following:

In my world, user options that map modes to some information are
a code smell.  Better use a buffer-local variable set via mode hooks.


        Stefan

Re: Average-user-facing interface for tree-sitter

[Jostein Kjønigsen]

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


> Yuan Fu<casouri@gmail.com>  writes:
>
>>  From the suggestions I collected from the old thread, here is my proposal:
>>
>> We define a custom option treesit-settings (we can discuss the name
>> later), which controls whether to enable/disable tree-sitter for each
>> major mode, and the default preference, like this:
>>
>> (defcustom treesit-settings '((t nil nil))
>>    "Tree-sitter toggles for major modes.
> Hm...  well, there's also modes that are "pure treesit", and there are
> (or will be) alternate modes with and without tree-sitter.
>
> I think users basically fall into two camps: The ones that want to have
> tree-sitter in all modes, and ones that want to enable it in specific
> modes (and ones that don't want it at all).  (Note Computer
> Science-mandated off-by-one error.)
>
> This suggests to me that we should just use the normal minor mode
> machinery that we have for these things.
>
> That is, people that want treesit in python-mode will say:
>
> (add-hook 'python-mode 'treesit-mode)
>
> And people that want it everywhere will say:
>
> (global-treesit-mode)
>
> This mode will, in addition to switching `treesit-mode' on everywhere,
> also set up `major-mode-remap-alist', so that `typescript-mode' is
> mapped to `ts-mode', and `c-mode' is mapped to `treesit-c-mode' (which
> I'm sure somebody is going to write in a couple of days), etc.
>
>
Hey there.

I'd like to come up with a somewhat contrarian point of view, if nothing 
else to provide a point of reference.

Right now we have a single (that is: one, 1) implementation for c-mode 
and c++-mode. They are based on cc-mode.

There's no c-mode where you enable usage of cc-mode through a 
minor-mode, nor is there a global-cc-mode toggle. And there's a really 
simple reason for that. That's because c-mode is implemented using 
cc-mode as a foundation, and there's no other implementations.

Now if we are starting to implement some new major-modes using 
treesitter as a foundation (for instance ts-mode or c#-mode), such a 
toggle for tree-sitter-mode will be equally pointless. There is no 
ts-mode without treesitter. And what should we do if people activate 
ts-mode without these bespoke minor-modes being active at the same time? 
I think it's a bit fuzzy and doesn't really sound very .. well defined.

Existing users of typescript-mode can be guided to the new 
implementation through a variety of ways, but to me the flow of 
activating typescript-mode major-mode, then treesit-minor-mode, and then 
magically switching to ts-mode makes absolutely no sense. And the 
performance of that is probably terrible too.

For those users I think a simple deprecation warning which informs about 
the new built-in ts-mode should be more than sufficient, for the users 
to clean out the old (buggy) typescript.el from their conigs.

The one place where it gets slightly complicated is for major-modes 
where one decides to have two or more canonical implementations, that is 
to add a new treesitter backend alongside the existing elisp-one.

For an established built in-mode, like c-mode, switching over night is 
probable not feasable nor desirable. So in practice you will have a few 
modes where you might have multiple backends, ideally only in a 
transitional phase, but in worst case scenario permanently.

So how can we deal with /that/ problem, where we have it, and not 
everywhere else?

I've had a similar "problem" for LSP backend for some programming 
languages. The solution I landed on there was to have a defcustom per 
language/major-mode where there were different LSP-backends which I may 
want to use.

Examples below:

- (defcustom csharp-lsp-backend) ;; 'lsp-mode 'omnisharp

- (defcustom typescript-lsp-backend) ;; 'lsp-mode 'tide

Then I in my major-mode hooks checked for which of the possible backends 
are chosen and dynamically dispatch based on that.

For major-modes ending up with multiple backends, I think this is an 
approach which scales better (supports more than 1 backend, but only 1 
at a time!), and will perform better (because it means avoiding 
triggering code for more than one single backend when activated).

It will also better support the flow if we eventually end up with a 
single backend again which then becomes the default. In that case, we 
don't need to maintain empty/obsolete minor-modes just to avoid breaking 
people's major-mode hooks. Then they will just have a defcustom setting 
which will be ignored.

Personally I think that would work much better, and support the 
transitional period we may be entering, without causing lasting "cruft" 
in the code-base we rather would not have.

Just my 2 cents.

--
Jostein



[-- Attachment #2: Type: text/html, Size: 5649 bytes --]

Re: Average-user-facing interface for tree-sitter

[Eli Zaretskii]

> Date: Thu, 13 Oct 2022 16:32:37 +0200
> Cc: emacs-devel <emacs-devel@gnu.org>
> From: Jostein Kjønigsen <jostein@secure.kjonigsen.net>
> 
> I'd like to come up with a somewhat contrarian point of view, if nothing else to provide a point of reference.

Thanks, but what you describe is not contrary to what we intend to do,
it's almost 100% identical to our intentions.

Re: Average-user-facing interface for tree-sitter

[Lars Ingebrigtsen]

Jostein Kjønigsen <jostein@secure.kjonigsen.net> writes:

> Existing users of typescript-mode can be guided to the new
> implementation through a variety of ways, but to me the flow of
> activating typescript-mode major-mode, then treesit-minor-mode, and
> then magically switching to ts-mode makes absolutely no sense. And the
> performance of that is probably terrible too.

That is not how `major-mode-remap-alist' works.

Re: Average-user-facing interface for tree-sitter

[Yuan Fu]

> On Oct 12, 2022, at 11:22 PM, Lars Ingebrigtsen <larsi@gnus.org> wrote:
> 
> Yuan Fu <casouri@gmail.com> writes:
> 
>> From the suggestions I collected from the old thread, here is my proposal:
>> 
>> We define a custom option treesit-settings (we can discuss the name
>> later), which controls whether to enable/disable tree-sitter for each
>> major mode, and the default preference, like this:
>> 
>> (defcustom treesit-settings '((t nil nil))
>>  "Tree-sitter toggles for major modes.
> 
> Hm...  well, there's also modes that are "pure treesit", and there are
> (or will be) alternate modes with and without tree-sitter.
> 
> I think users basically fall into two camps: The ones that want to have
> tree-sitter in all modes, and ones that want to enable it in specific
> modes (and ones that don't want it at all).  (Note Computer
> Science-mandated off-by-one error.)
> 
> This suggests to me that we should just use the normal minor mode
> machinery that we have for these things.
> 
> That is, people that want treesit in python-mode will say:
> 
> (add-hook 'python-mode 'treesit-mode)
> 
> And people that want it everywhere will say:
> 
> (global-treesit-mode)
> 
> This mode will, in addition to switching `treesit-mode' on everywhere,
> also set up `major-mode-remap-alist', so that `typescript-mode' is
> mapped to `ts-mode', and `c-mode' is mapped to `treesit-c-mode' (which
> I'm sure somebody is going to write in a couple of days), etc.

Ok, so to make it concrete, in addition to treesit-mode and global-treesit-mode, we would have

treesit-major-mode-remap-alist: same as major-mode-remap-alist,
appended to major-mode-remap-alist when global-treesit-mode turns on.

treesit-toggle-function: A function treesit-mode calls to enable
tree-sitter support, major modes set this function.

I have yet to come up a way to combine the minor-mode machinery with the ability to specify backend for each feature (imenu, xref, font-lock, etc).

Maybe we can have something like

feature-provider-alist: An buffer-local alist of (FEATURE .
PROVIDERS), FEATURE could be indent, font-lock, imenu, xref, etc,
PROVIDERS is a list of treesit, eglot, emacs, etc.

But I failed to see how to integrate it with the rest.

I wonder if we can have a major mode hook that runs before the body runs, like xxx-mode-before-hook. Then user can set feature-provider-alist in that hook. This way major modes can decide what backend (provider) to use for each features.

Yuan

Re: Average-user-facing interface for tree-sitter

[Lars Ingebrigtsen]

Yuan Fu <casouri@gmail.com> writes:

> Ok, so to make it concrete, in addition to treesit-mode and
> global-treesit-mode, we would have
>
> treesit-major-mode-remap-alist: same as major-mode-remap-alist,
> appended to major-mode-remap-alist when global-treesit-mode turns on.

Yup.

> treesit-toggle-function: A function treesit-mode calls to enable
> tree-sitter support, major modes set this function.

Hm...  I'm not sure.  A mode like python-mode which has both treesit and
non-treesit code paths would just be

  (if treesit-mode
      (do-one-thing)
    (do-the-old-thing))

I think?  On the other hand, that wouldn't allow dynamic switching
between treesit and non, so perhaps we should have something like that;
yes.

> I have yet to come up a way to combine the minor-mode machinery with
> the ability to specify backend for each feature (imenu, xref,
> font-lock, etc).
>
> Maybe we can have something like
>
> feature-provider-alist: An buffer-local alist of (FEATURE .
> PROVIDERS), FEATURE could be indent, font-lock, imenu, xref, etc,
> PROVIDERS is a list of treesit, eglot, emacs, etc.
>
> But I failed to see how to integrate it with the rest.
>
> I wonder if we can have a major mode hook that runs before the body
> runs, like xxx-mode-before-hook. Then user can set
> feature-provider-alist in that hook. This way major modes can decide
> what backend (provider) to use for each features.

Hm...

Re: Average-user-facing interface for tree-sitter

[Stephen Leake]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Yuan Fu <casouri@gmail.com> writes:
>
>> I wonder if we can have a major mode hook that runs before the body
>> runs, like xxx-mode-before-hook. Then user can set
>> feature-provider-alist in that hook. This way major modes can decide
>> what backend (provider) to use for each features.

+1

I needed something similar in ada-mode; I implemented it by moving have
of the major mode function into a post-local-vars hook; that runs
after the major mode function.

-- 
-- Stephe

Re: Average-user-facing interface for tree-sitter

[Yuan Fu]

> On Oct 14, 2022, at 4:22 AM, Stephen Leake <stephen_leake@stephe-leake.org> wrote:
> 
> Lars Ingebrigtsen <larsi@gnus.org> writes:
> 
>> Yuan Fu <casouri@gmail.com> writes:
>> 
>>> I wonder if we can have a major mode hook that runs before the body
>>> runs, like xxx-mode-before-hook. Then user can set
>>> feature-provider-alist in that hook. This way major modes can decide
>>> what backend (provider) to use for each features.
> 
> +1
> 
> I needed something similar in ada-mode; I implemented it by moving have
> of the major mode function into a post-local-vars hook; that runs
> after the major mode function.

I think the best way to allow users to alter major mode’s setup is to have such a hook run after clearing local variable but before major mode setup. Currently since major mode hook run after the setup, setting local variable in the hook doesn’t do anything. Example:

(defvar-local treesit-activate nil)

;; Enable for python-mode, TBH this looks a bit awkward.
(add-hook 'python-mode-before-hook
          (lambda () (setq treesit-activate t)))

(define-minor-mode global-treesit-mode ""
  :global t
  (if global-treesit-mode
      (setq-default treesit-activate t)
    (setq-default treesit-activate nil)))

;; Not sure about minor mode yet.

Yuan

Re: Average-user-facing interface for tree-sitter

[Yuan Fu]

> On Oct 14, 2022, at 4:02 AM, Lars Ingebrigtsen <larsi@gnus.org> wrote:
> 
> Yuan Fu <casouri@gmail.com> writes:
> 
>> Ok, so to make it concrete, in addition to treesit-mode and
>> global-treesit-mode, we would have
>> 
>> treesit-major-mode-remap-alist: same as major-mode-remap-alist,
>> appended to major-mode-remap-alist when global-treesit-mode turns on.
> 
> Yup.

Thinking more about it, there are some problems need to be resolved:
- when does xxx-mode.el set treesit-major-mode-remap-alist? Require-time? That’ seems too late: M-x xxx-mode (autoloader) -> (require ‘xxx-mode) -> added to treesit-major-mode-remap-alist -> ?
- If global-treesit-mode is already on, changing treesit-major-mode-remap-alist doesn’t do anything, you need to toggle global-treesit-mode for it to take effect.

> 
>> treesit-toggle-function: A function treesit-mode calls to enable
>> tree-sitter support, major modes set this function.
> 
> Hm...  I'm not sure.  A mode like python-mode which has both treesit and
> non-treesit code paths would just be
> 
>  (if treesit-mode
>      (do-one-thing)
>    (do-the-old-thing))
> 
> I think?  On the other hand, that wouldn't allow dynamic switching
> between treesit and non, so perhaps we should have something like that;
> yes.

As I said in the previous message, setting treesit-mode locally in major mode hook doesn’t do anything, because the hook runs after all the setup.

> 
>> I have yet to come up a way to combine the minor-mode machinery with
>> the ability to specify backend for each feature (imenu, xref,
>> font-lock, etc).
>> 
>> Maybe we can have something like
>> 
>> feature-provider-alist: An buffer-local alist of (FEATURE .
>> PROVIDERS), FEATURE could be indent, font-lock, imenu, xref, etc,
>> PROVIDERS is a list of treesit, eglot, emacs, etc.
>> 
>> But I failed to see how to integrate it with the rest.
>> 
>> I wonder if we can have a major mode hook that runs before the body
>> runs, like xxx-mode-before-hook. Then user can set
>> feature-provider-alist in that hook. This way major modes can decide
>> what backend (provider) to use for each features.
> 
> Hm...

Emm…

Yuan

Re: Average-user-facing interface for tree-sitter

[Stefan Monnier]

> I wonder if we can have a major mode hook that runs before the body runs,
> like xxx-mode-before-hook.

Given the way things are currently setup, it's not really feasible in
a clean and reliable way, no.

What we have instead is:
- the `:after-hook`, i.e. part of the major-mode's
  code that is run after the major-mode hook.
- The `hack-local-variables-hook` which the major-mode can set
  buffer-locally.  It's similar to the `after-hook:`, except it's run
  after setting the file-local variables, so it's more often what
  we want.


        Stefan

Re: Average-user-facing interface for tree-sitter

[Yuan Fu]

> On Oct 14, 2022, at 1:49 PM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> 
>> I wonder if we can have a major mode hook that runs before the body runs,
>> like xxx-mode-before-hook.
> 
> Given the way things are currently setup, it's not really feasible in
> a clean and reliable way, no.
> 
> What we have instead is:
> - the `:after-hook`, i.e. part of the major-mode's
>  code that is run after the major-mode hook.
> - The `hack-local-variables-hook` which the major-mode can set
>  buffer-locally.  It's similar to the `after-hook:`, except it's run
>  after setting the file-local variables, so it's more often what
>  we want.
> 

I dunno, it literally has “hack” in its name. If we can’t do before-hook, maybe its best to let major mode setup normally and locally bind treesit-enable-function, which treesit-mode calls should user add it to major mode hook.

And if a user wants more control (eg, use tree-sitter for indent only), he/she assigns the tree-sitter function to indent-line-function, etc. No hand-holding from us.

Buuut, if a user wants tree-sitter for parent mode but not derived mode, they need to write

(add-hook 'xxx-mode-hook
          (lambda () (when (eq major-mode 'xxx-mode)
                       (treesit-mode))))

Which again is a bit awkward.

So maybe a central variable isn’t that bad an idea. Could you layout why mapping modes to some information is bad? Can we fix it somehow?

Yuan

Re: Average-user-facing interface for tree-sitter

[Stefan Monnier]

> (add-hook 'xxx-mode-hook
>           (lambda () (when (eq major-mode 'xxx-mode)
>                        (treesit-mode))))
>
> Which again is a bit awkward.

Awkward but possible.  Another approach can be:

    (add-hook 'xxx-mode-hook #'treesit-mode)
    (add-hook 'yyy-mode-hook (lambda () (treesit-mode -1)))

> So maybe a central variable isn’t that bad an idea.  Could you layout
> why mapping modes to some information is bad?

Because it needs to be able to say "for this mode and all its derived
modes" but also "for this mode only" as well as various combinations,
and then you need to document how it interacts with the major mode's
hook, ...

Maybe we should devise a way to "centrally" control the value of some
vars depending on major modes, but if so we should carefully design
a thing specifically for that, make sure it's sufficiently flexible, and
then use it for several (any?) variable.

All the vars I've seen so far which do that do it quite naively, which
works OK for simple cases but breaks down one way or another when you
start taking derived modes into account.  Hence my considering it a code
smell (just like most uses of the `major-mode` variable).


        Stefan

Re: Average-user-facing interface for tree-sitter

[Yuan Fu]

> On Oct 14, 2022, at 8:26 PM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> 
>> (add-hook 'xxx-mode-hook
>>          (lambda () (when (eq major-mode 'xxx-mode)
>>                       (treesit-mode))))
>> 
>> Which again is a bit awkward.
> 
> Awkward but possible.  Another approach can be:
> 
>    (add-hook 'xxx-mode-hook #'treesit-mode)
>    (add-hook 'yyy-mode-hook (lambda () (treesit-mode -1)))

With the central variable approach, you can do (push ‘(xxx-mode t nil) treesit-settings), where t means enable, nil means don’t inherit.

> 
>> So maybe a central variable isn’t that bad an idea.  Could you layout
>> why mapping modes to some information is bad?
> 
> Because it needs to be able to say "for this mode and all its derived
> modes" but also "for this mode only" as well as various combinations,
> and then you need to document how it interacts with the major mode's
> hook, ...

Do you mind elaborate on: What are the other combinations? What are the ways a central variable can interact with major mode hooks? 

> Maybe we should devise a way to "centrally" control the value of some
> vars depending on major modes, but if so we should carefully design
> a thing specifically for that, make sure it's sufficiently flexible, and
> then use it for several (any?) variable.
> 
> All the vars I've seen so far which do that do it quite naively, which
> works OK for simple cases but breaks down one way or another when you
> start taking derived modes into account.  Hence my considering it a code
> smell (just like most uses of the `major-mode` variable).

How about a function mode-specific-value that takes any variable with the following shape:

((MODE VALUE INHERIT)…)

And returns the right VALUE for the current mode? INHERIT decides whether VALUE is inherited by MODE’s derived modes. Something like:

(defun mode-specific-value (central-var)
  (cl-loop for setting in central-var
           for mode = (nth 0 setting)
           for value = (nth 1 setting)
           for inherit = (nth 2 setting)
           if (and (derived-mode-p mode) inherit)
           return value
           finally return
           (if-let ((default (alist-get t central-var))) ; t sets the default value for all mode.
               (car default)
             nil)))

This function would handle derived mode ok, but I don’t know what are the other problems you are think of, maybe you can tell me what this function falls short for. And we can go from there.

Yuan

Re: Average-user-facing interface for tree-sitter

[Yuan Fu]

> On Oct 14, 2022, at 10:05 PM, Yuan Fu <casouri@gmail.com> wrote:
> 
> 
> 
>> On Oct 14, 2022, at 8:26 PM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>> 
>>> (add-hook 'xxx-mode-hook
>>>         (lambda () (when (eq major-mode 'xxx-mode)
>>>                      (treesit-mode))))
>>> 
>>> Which again is a bit awkward.
>> 
>> Awkward but possible.  Another approach can be:
>> 
>>   (add-hook 'xxx-mode-hook #'treesit-mode)
>>   (add-hook 'yyy-mode-hook (lambda () (treesit-mode -1)))
> 
> With the central variable approach, you can do (push ‘(xxx-mode t nil) treesit-settings), where t means enable, nil means don’t inherit.
> 
>> 
>>> So maybe a central variable isn’t that bad an idea.  Could you layout
>>> why mapping modes to some information is bad?
>> 
>> Because it needs to be able to say "for this mode and all its derived
>> modes" but also "for this mode only" as well as various combinations,
>> and then you need to document how it interacts with the major mode's
>> hook, ...
> 
> Do you mind elaborate on: What are the other combinations? What are the ways a central variable can interact with major mode hooks? 
> 
>> Maybe we should devise a way to "centrally" control the value of some
>> vars depending on major modes, but if so we should carefully design
>> a thing specifically for that, make sure it's sufficiently flexible, and
>> then use it for several (any?) variable.
>> 
>> All the vars I've seen so far which do that do it quite naively, which
>> works OK for simple cases but breaks down one way or another when you
>> start taking derived modes into account.  Hence my considering it a code
>> smell (just like most uses of the `major-mode` variable).
> 
> How about a function mode-specific-value that takes any variable with the following shape:
> 
> ((MODE VALUE INHERIT)…)
> 
> And returns the right VALUE for the current mode? INHERIT decides whether VALUE is inherited by MODE’s derived modes. Something like:
> 
> (defun mode-specific-value (central-var)
>  (cl-loop for setting in central-var
>           for mode = (nth 0 setting)
>           for value = (nth 1 setting)
>           for inherit = (nth 2 setting)
>           if (and (derived-mode-p mode) inherit)
>           return value
>           finally return
>           (if-let ((default (alist-get t central-var))) ; t sets the default value for all mode.
>               (car default)
>             nil)))
> 
> This function would handle derived mode ok, but I don’t know what are the other problems you are think of, maybe you can tell me what this function falls short for. And we can go from there.

If no one has opinions on this, I’m going to implement the central-variable approach, since IMO that one is most fit.

Yuan

Re: Average-user-facing interface for tree-sitter

[Lars Ingebrigtsen]

Yuan Fu <casouri@gmail.com> writes:

> If no one has opinions on this, I’m going to implement the
> central-variable approach, since IMO that one is most fit.

Well, my opinion is that modes is the right approach, since that's what
we use for most of these things in Emacs, so people are used to them.
(We've been moving a whole bunch of stuff that used to be variables to
modes over the years.)

Re: Average-user-facing interface for tree-sitter

[Stefan Monnier]

> How about a function mode-specific-value that takes any variable with the following shape:
>
> ((MODE VALUE INHERIT)…)
>
> And returns the right VALUE for the current mode? INHERIT decides
> whether VALUE is inherited by MODE’s derived modes.

FWIW, `inherit` should be the default, IMO, so the third part should
specify DONT-INHERIT.

> This function would handle derived mode ok, but I don’t know what are
> the other problems you are think of, maybe you can tell me what this
> function falls short for. And we can go from there.

The function is one thing.  Another question is when it is run, and
hence how it interacts with other ways to enable/set/control the thing
you want to control (in this case enabling `treesit-mode`, IIUC).

I recommend you focus on making the `treesit-mode` minor mode work
regardless when it's called.  So it can be controlled by the usual
(add-hook foo-mode-hook #'treesit-mode).

Then we can worry about how `global-treesit-mode` should be controlled
(and worry about harmonizing it with `global-font-lock-mode`, for
example, which relies on `font-lock-global-modes` which is also such
a centralized way to specify the behavior per-mode (and a poor one at
that if you ask me)).


        Stefan

Re: Average-user-facing interface for tree-sitter

[Yuan Fu]

> On Oct 17, 2022, at 2:15 AM, Lars Ingebrigtsen <larsi@gnus.org> wrote:
> 
> Yuan Fu <casouri@gmail.com> writes:
> 
>> If no one has opinions on this, I’m going to implement the
>> central-variable approach, since IMO that one is most fit.
> 
> Well, my opinion is that modes is the right approach, since that's what
> we use for most of these things in Emacs, so people are used to them.
> (We've been moving a whole bunch of stuff that used to be variables to
> modes over the years.)

Ok, I installed the toggle system. Now we have treesit-mode and global-treesit-mode. Major modes set major-mode-backend-function and treesit-mode uses that function to turn on/off tree-sitter features.

mode-backend-function is pretty straightforward for major modes that supports both tree-sitter and no-tree-sitter. They just need to set local variables.

For separated major modes, they need to do a bit of a dance. Besides switching to the new major mode, they need  to add (mode . new-mode) to major-mode-remap-alist if global-treesit-mode is on (so next time we go straight to new-mode as long as global-treesit-mode is on). They also need to add (mode . new-mode) to treesit-remapped-major-mode-alist so global-treesit-mode can remove these entries when it turns off.

Yuan

Re: Average-user-facing interface for tree-sitter

[Yuan Fu]

> On Oct 18, 2022, at 1:49 PM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> 
>> How about a function mode-specific-value that takes any variable with the following shape:
>> 
>> ((MODE VALUE INHERIT)…)
>> 
>> And returns the right VALUE for the current mode? INHERIT decides
>> whether VALUE is inherited by MODE’s derived modes.
> 
> FWIW, `inherit` should be the default, IMO, so the third part should
> specify DONT-INHERIT.
> 
>> This function would handle derived mode ok, but I don’t know what are
>> the other problems you are think of, maybe you can tell me what this
>> function falls short for. And we can go from there.
> 
> The function is one thing.  Another question is when it is run, and
> hence how it interacts with other ways to enable/set/control the thing
> you want to control (in this case enabling `treesit-mode`, IIUC).
> 
> I recommend you focus on making the `treesit-mode` minor mode work
> regardless when it's called.  So it can be controlled by the usual
> (add-hook foo-mode-hook #'treesit-mode).
> 
> Then we can worry about how `global-treesit-mode` should be controlled
> (and worry about harmonizing it with `global-font-lock-mode`, for
> example, which relies on `font-lock-global-modes` which is also such
> a centralized way to specify the behavior per-mode (and a poor one at
> that if you ask me)).

I installed treesit-mode and global-treesit-mode. Errr global-treesit-mode uses define-globalized-minor-mode so it has a global-treesit-modes, sooo...

Yuan

Re: Average-user-facing interface for tree-sitter

[Stefan Monnier]

> For separated major modes, they need to do a bit of a dance.

I don't think we should even try and document what they should do:
they're evil and we should spend our efforts merging them into one instead.

> Besides switching to the new major mode, they need  to add (mode
> . new-mode) to major-mode-remap-alist if global-treesit-mode is on (so
> next time we go straight to new-mode as long as global-treesit-mode is
> on).  They also need to add (mode . new-mode) to
> treesit-remapped-major-mode-alist so global-treesit-mode can remove
> these entries when it turns off.

Too much magic, IMO.  Let each case handle it manually, since I suspect
that every case will be different and the best thing to do will vary (it
depends on whether the two modes are still actively maintained, whether
by the same authors or not, the relative age and popularity of the
respective modes, ...).


        Stefan

Re: Average-user-facing interface for tree-sitter

[Yuan Fu]

> On Oct 18, 2022, at 2:48 PM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> 
>> For separated major modes, they need to do a bit of a dance.
> 
> I don't think we should even try and document what they should do:
> they're evil and we should spend our efforts merging them into one instead.
> 
>> Besides switching to the new major mode, they need  to add (mode
>> . new-mode) to major-mode-remap-alist if global-treesit-mode is on (so
>> next time we go straight to new-mode as long as global-treesit-mode is
>> on).  They also need to add (mode . new-mode) to
>> treesit-remapped-major-mode-alist so global-treesit-mode can remove
>> these entries when it turns off.
> 
> Too much magic, IMO.  Let each case handle it manually, since I suspect
> that every case will be different and the best thing to do will vary (it
> depends on whether the two modes are still actively maintained, whether
> by the same authors or not, the relative age and popularity of the
> respective modes, …)

So we don’t do anything in global-treesit-mode? Ie,

(define-globalized-minor-mode global-treesit-mode treesit-mode
  global-treesit-mode--turn-on
  :version "29.1"
  :group 'treesit
  :predicate t
  (when (not global-treesit-mode)
    (dolist (map treesit-remapped-major-mode-alist)
      (setq major-mode-remap-alist
            (remove map major-mode-remap-alist)))))

to

(define-globalized-minor-mode global-treesit-mode treesit-mode
  global-treesit-mode--turn-on
  :version "29.1"
  :group 'treesit
  :predicate t
  )

And remove treesit-remapped-major-mode-alist?

Yuan

Re: Average-user-facing interface for tree-sitter

[Stefan Monnier]

>> Too much magic, IMO.  Let each case handle it manually, since I suspect
>> that every case will be different and the best thing to do will vary (it
>> depends on whether the two modes are still actively maintained, whether
>> by the same authors or not, the relative age and popularity of the
>> respective modes, …)
>
> So we don’t do anything in global-treesit-mode? Ie,
>
> (define-globalized-minor-mode global-treesit-mode treesit-mode
>   global-treesit-mode--turn-on
>   :version "29.1"
>   :group 'treesit
>   :predicate t
>   (when (not global-treesit-mode)
>     (dolist (map treesit-remapped-major-mode-alist)
>       (setq major-mode-remap-alist
>             (remove map major-mode-remap-alist)))))
>
> to
>
> (define-globalized-minor-mode global-treesit-mode treesit-mode
>   global-treesit-mode--turn-on
>   :version "29.1"
>   :group 'treesit
>   :predicate t
>   )
>
> And remove treesit-remapped-major-mode-alist?

That's I'd do, yes.

It's much easier to add features later (if they prove to satisfy
a common need) than to remove unused features.

BTW, I see that `global-treesit-mode--turn-on` is currently a (non
official) alias of `treesit-mode`, but I think it should only call
`treesit-mode` in those buffers where there is some indication that the
major mode provides some treesit-mode support (e.g., currently that
could be if `major-mode-backend-function` is set).

And then maybe `treesit-mode` should signal an error (and turn itself
off) if it's enabled in a buffer whose major mode does not provide any
support for `treesit-mode`.


        Stefan

Re: Average-user-facing interface for tree-sitter

[Yuan Fu]

> On Oct 18, 2022, at 3:31 PM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> 
>>> Too much magic, IMO.  Let each case handle it manually, since I suspect
>>> that every case will be different and the best thing to do will vary (it
>>> depends on whether the two modes are still actively maintained, whether
>>> by the same authors or not, the relative age and popularity of the
>>> respective modes, …)
>> 
>> So we don’t do anything in global-treesit-mode? Ie,
>> 
>> (define-globalized-minor-mode global-treesit-mode treesit-mode
>>  global-treesit-mode--turn-on
>>  :version "29.1"
>>  :group 'treesit
>>  :predicate t
>>  (when (not global-treesit-mode)
>>    (dolist (map treesit-remapped-major-mode-alist)
>>      (setq major-mode-remap-alist
>>            (remove map major-mode-remap-alist)))))
>> 
>> to
>> 
>> (define-globalized-minor-mode global-treesit-mode treesit-mode
>>  global-treesit-mode--turn-on
>>  :version "29.1"
>>  :group 'treesit
>>  :predicate t
>>  )
>> 
>> And remove treesit-remapped-major-mode-alist?
> 
> That's I'd do, yes.
> 
> It's much easier to add features later (if they prove to satisfy
> a common need) than to remove unused features.
> 
> BTW, I see that `global-treesit-mode--turn-on` is currently a (non
> official) alias of `treesit-mode`, but I think it should only call
> `treesit-mode` in those buffers where there is some indication that the
> major mode provides some treesit-mode support (e.g., currently that
> could be if `major-mode-backend-function` is set).

I’m changing it to what you described in another email. (So major-mode-backend-function is goner). I can use treesit-font-lock-settings as the minimum criterion.

> 
> And then maybe `treesit-mode` should signal an error (and turn itself
> off) if it's enabled in a buffer whose major mode does not provide any
> support for `treesit-mode`.

So currently treesit-mode signals a warning if it can’t activate tree-sitter and (eq this-command ’treesit-mode), but if it is called in find-file-hook, etc set up by global-treesit-mode, it doesn’t signal a warning. The idea is that if a user actively tries to turn on treesit-mode (by M-x treesit-mode), they should know if it didn’t work, but if they just turns on global-treesit-mode, they shouldn’t be bothered with warnings-in-the-face. And if one wants to know if tree-sitter is working for a buffer, they can use M-x treesit-mode to get a clear answer.

Yuan

Re: Average-user-facing interface for tree-sitter

[Stefan Monnier]

>> BTW, I see that `global-treesit-mode--turn-on` is currently a (non
>> official) alias of `treesit-mode`, but I think it should only call
>> `treesit-mode` in those buffers where there is some indication that the
>> major mode provides some treesit-mode support (e.g., currently that
>> could be if `major-mode-backend-function` is set).
>
> I’m changing it to what you described in another email. (So
> major-mode-backend-function is goner). I can use treesit-font-lock-settings
> as the minimum criterion.
>
>> 
>> And then maybe `treesit-mode` should signal an error (and turn itself
>> off) if it's enabled in a buffer whose major mode does not provide any
>> support for `treesit-mode`.
>
> So currently treesit-mode signals a warning if it can’t activate tree-sitter
> and (eq this-command ’treesit-mode), but if it is called in find-file-hook,
> etc set up by global-treesit-mode, it doesn’t signal a warning.

That's why define-globalized-minor-mode has a TURN-ON function
argument separately from the MODE argument: the TURN-ON function has
to determine whether to enable the minor mode or not.  It shouldn't
blindly call `treesit-mode` but first check whether `treesit-mode`
is applicable.

This way, `treesit-mode` can always signal an error when called in
a buffer where it's not applicable: it's always the responsibility of
the caller to check beforehand.


        Stefan

RE: [External] : Re: Average-user-facing interface for tree-sitter

[Drew Adams]

> That's why define-globalized-minor-mode has a TURN-ON function
> argument separately from the MODE argument: the TURN-ON function has
> to determine whether to enable the minor mode or not.  It shouldn't
> blindly call `treesit-mode` but first check whether `treesit-mode`
> is applicable.
> 
> This way, `treesit-mode` can always signal an error when called in
> a buffer where it's not applicable: it's always the responsibility of
> the caller to check beforehand.

FWIW, I think it wouldn't hurt to add this info to
the `define-globalized-minor-mode` doc.  It makes
perfect sense when you hear it, but otherwise it's
not necessarily obvious.

We do say it: "[TURN-ON] should try to turn MODE on
if applicable for that buffer".  Still, it's clearer
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
the way you just pointed it out, somehow.

Re: Average-user-facing interface for tree-sitter

[Theodor Thornhill]

Hi Yuan!

Yuan Fu <casouri@gmail.com> writes:
>>> 
>>> And remove treesit-remapped-major-mode-alist?
>> 
>> That's I'd do, yes.
>> 
>> It's much easier to add features later (if they prove to satisfy
>> a common need) than to remove unused features.
>> 
>> BTW, I see that `global-treesit-mode--turn-on` is currently a (non
>> official) alias of `treesit-mode`, but I think it should only call
>> `treesit-mode` in those buffers where there is some indication that the
>> major mode provides some treesit-mode support (e.g., currently that
>> could be if `major-mode-backend-function` is set).
>
> I’m changing it to what you described in another email. (So major-mode-backend-function is goner). I can use treesit-font-lock-settings as the minimum criterion.
>

It seems we've had some regressions _after_
851a8f65e9a6b00b51f6a41f4c8f2ec2a797862b, I presume with the major mode
toggles, so font-locking has ceased to work properly.  One thing I see
is that the "warn" isn't a symbol in the new toggle mechanism, so it
errors out.  But even after that has been fixed it still doesn't work.

Why would we want to init the whole js-mode if we already know that we
don't want to use it?

All the best,
Theo

Re: Average-user-facing interface for tree-sitter

[Yuan Fu]

> On Oct 18, 2022, at 7:52 PM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> 
>>> BTW, I see that `global-treesit-mode--turn-on` is currently a (non
>>> official) alias of `treesit-mode`, but I think it should only call
>>> `treesit-mode` in those buffers where there is some indication that the
>>> major mode provides some treesit-mode support (e.g., currently that
>>> could be if `major-mode-backend-function` is set).
>> 
>> I’m changing it to what you described in another email. (So
>> major-mode-backend-function is goner). I can use treesit-font-lock-settings
>> as the minimum criterion.

Ends up using treesit-mode-supported.

>> 
>>> 
>>> And then maybe `treesit-mode` should signal an error (and turn itself
>>> off) if it's enabled in a buffer whose major mode does not provide any
>>> support for `treesit-mode`.
>> 
>> So currently treesit-mode signals a warning if it can’t activate tree-sitter
>> and (eq this-command ’treesit-mode), but if it is called in find-file-hook,
>> etc set up by global-treesit-mode, it doesn’t signal a warning.
> 
> That's why define-globalized-minor-mode has a TURN-ON function
> argument separately from the MODE argument: the TURN-ON function has
> to determine whether to enable the minor mode or not.  It shouldn't
> blindly call `treesit-mode` but first check whether `treesit-mode`
> is applicable.
> 
> This way, `treesit-mode` can always signal an error when called in
> a buffer where it's not applicable: it's always the responsibility of
> the caller to check beforehand.

Ok, I pushed the new system. Now major modes sets a range of tree-sitter specific variables, and treesit-mode takes care of activating them. For example, python-mode sets these:

  (setq-local treesit-mode-supported t)
  (setq-local treesit-required-languages '(python))
  (setq-local treesit-font-lock-feature-list
              '((basic) (moderate) (elaborate)))
  (setq-local treesit-font-lock-settings python--treesit-settings)
  (setq-local treesit-imenu-function
              #'python-imenu-treesit-create-index)

Js-mode sets these:

  (setq-local treesit-mode-supported t)
  (setq-local treesit-required-languages '(javascript))
  (setq-local treesit-simple-indent-rules js--treesit-indent-rules)
  (setq-local treesit-defun-type-regexp
              (rx (or "class_declaration"
                      "method_definition"
                      "function_declaration"
                      "lexical_declaration")))
  (setq-local treesit-font-lock-settings js--treesit-font-lock-settings)
  (setq-local treesit-font-lock-feature-list '((minimal) (moderate) (full)))

treesit-mode-supported could seem a bit redundant, but no harm in being explicit I guess?

Now global-treesit-mode never signals warning, and treesit-mode always signals warning, when treesit can’t activate in a buffer.

Yuan

Re: Average-user-facing interface for tree-sitter

[Yuan Fu]

> On Oct 18, 2022, at 10:35 PM, Theodor Thornhill <theo@thornhill.no> wrote:
> 
> Hi Yuan!
> 
> Yuan Fu <casouri@gmail.com> writes:
>>>> 
>>>> And remove treesit-remapped-major-mode-alist?
>>> 
>>> That's I'd do, yes.
>>> 
>>> It's much easier to add features later (if they prove to satisfy
>>> a common need) than to remove unused features.
>>> 
>>> BTW, I see that `global-treesit-mode--turn-on` is currently a (non
>>> official) alias of `treesit-mode`, but I think it should only call
>>> `treesit-mode` in those buffers where there is some indication that the
>>> major mode provides some treesit-mode support (e.g., currently that
>>> could be if `major-mode-backend-function` is set).
>> 
>> I’m changing it to what you described in another email. (So major-mode-backend-function is goner). I can use treesit-font-lock-settings as the minimum criterion.
>> 
> 
> It seems we've had some regressions _after_
> 851a8f65e9a6b00b51f6a41f4c8f2ec2a797862b, I presume with the major mode
> toggles, so font-locking has ceased to work properly.  One thing I see
> is that the "warn" isn't a symbol in the new toggle mechanism, so it
> errors out.  But even after that has been fixed it still doesn't work.
> 
> Why would we want to init the whole js-mode if we already know that we
> don't want to use it?

Yes, sorry, I made some further changes to js-mode. Could you have a look and see if it makes sense?

Thanks,
Yuan

Re: Average-user-facing interface for tree-sitter

[Theodor Thornhill]

Yuan Fu <casouri@gmail.com> writes:
>> 
>> It seems we've had some regressions _after_
>> 851a8f65e9a6b00b51f6a41f4c8f2ec2a797862b, I presume with the major mode
>> toggles, so font-locking has ceased to work properly.  One thing I see
>> is that the "warn" isn't a symbol in the new toggle mechanism, so it
>> errors out.  But even after that has been fixed it still doesn't work.
>> 
>> Why would we want to init the whole js-mode if we already know that we
>> don't want to use it?
>
> Yes, sorry, I made some further changes to js-mode. Could you have a look and see if it makes sense?
>
> Thanks,
> Yuan

I'm not sure I really like the new changes.  Now we init everything in
js-mode in addition to everything treesitter related.  So now stuff like

```
  (setq-local font-lock-defaults
              (list js--font-lock-keywords nil nil nil nil
                    '(font-lock-syntactic-face-function
                      . js-font-lock-syntactic-face-function)))
  (setq-local syntax-propertize-function #'js-syntax-propertize)
  (add-hook 'syntax-propertize-extend-region-functions
            #'syntax-propertize-multiline 'append 'local)
  (add-hook 'syntax-propertize-extend-region-functions
            #'js--syntax-propertize-extend-region 'append 'local)
  (setq-local prettify-symbols-alist js--prettify-symbols-alist)

  (setq-local parse-sexp-ignore-comments t)

```
And many before/after change functions are enabled.  I don't see any
reason why they should be, and in many cases this will _absolutely_
be difficult to reason about.  That's why I did the 'avoid cc-mode
altogether' approach earlier.  I'm interested to know why this is
perceived as better than the 'js-use-treesitter' thing we had earlier.
Why not just check for 'global-treesit-mode' or something and then just
jump over everything else than treesit inits?


Also see suggested diff to get ts-mode to work again, as it couldn't
activate in the current revision.  This means that the only reason that
js-mode works is because we set the font-lock-defaults earlier.
Wouldn't that mean it works only by accident?


diff --git a/lisp/progmodes/ts-mode.el b/lisp/progmodes/ts-mode.el
index c23f2bec05..670e103eec 100644
--- a/lisp/progmodes/ts-mode.el
+++ b/lisp/progmodes/ts-mode.el
@@ -276,6 +276,8 @@ ts-mode
                       "function_declaration"
                       "lexical_declaration")))
   ;; Font-lock.
+  (unless font-lock-defaults
+    (setq font-lock-defaults '(nil t)))
   (setq-local treesit-font-lock-settings ts-mode--font-lock-settings)
   (setq-local treesit-font-lock-feature-list '((minimal) (moderate) (full)))
 


Thanks,
Theo

Re: Average-user-facing interface for tree-sitter

[Stefan Monnier]

>> Yes, sorry, I made some further changes to js-mode. Could you have a look
>> and see if it makes sense?
>>
> I'm not sure I really like the new changes.

[ Context: I'm largely to blame, I proposed the change.  ]

> Now we init everything in
> js-mode in addition to everything treesitter related.  So now stuff like
>
> ```
>   (setq-local font-lock-defaults
>               (list js--font-lock-keywords nil nil nil nil
>                     '(font-lock-syntactic-face-function
>                       . js-font-lock-syntactic-face-function)))
>   (setq-local syntax-propertize-function #'js-syntax-propertize)
>   (add-hook 'syntax-propertize-extend-region-functions
>             #'syntax-propertize-multiline 'append 'local)
>   (add-hook 'syntax-propertize-extend-region-functions
>             #'js--syntax-propertize-extend-region 'append 'local)
>   (setq-local prettify-symbols-alist js--prettify-symbols-alist)
>
>   (setq-local parse-sexp-ignore-comments t)
> ```

Hmm... yeah, undoing those add-hook settings of various other variables
which treesit-mode doesn't use is a problem.  We need to find
a better way.


        Stefan

Re: Average-user-facing interface for tree-sitter

[Theodor Thornhill]

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

>>> Yes, sorry, I made some further changes to js-mode. Could you have a look
>>> and see if it makes sense?
>>>
>> I'm not sure I really like the new changes.
>
> [ Context: I'm largely to blame, I proposed the change.  ]
>

No worries :-)

>> Now we init everything in
>> js-mode in addition to everything treesitter related.  So now stuff like
>>
>> ```
>>   (setq-local font-lock-defaults
>>               (list js--font-lock-keywords nil nil nil nil
>>                     '(font-lock-syntactic-face-function
>>                       . js-font-lock-syntactic-face-function)))
>>   (setq-local syntax-propertize-function #'js-syntax-propertize)
>>   (add-hook 'syntax-propertize-extend-region-functions
>>             #'syntax-propertize-multiline 'append 'local)
>>   (add-hook 'syntax-propertize-extend-region-functions
>>             #'js--syntax-propertize-extend-region 'append 'local)
>>   (setq-local prettify-symbols-alist js--prettify-symbols-alist)
>>
>>   (setq-local parse-sexp-ignore-comments t)
>> ```
>
> Hmm... yeah, undoing those add-hook settings of various other variables
> which treesit-mode doesn't use is a problem.  We need to find
> a better way.
>
>

Yeah.  Shouldn't it be possible to just have a global var instead of a
mode?  That way we can just look for that variable when enabling the
mode, and avoid calling anything other than what we want.  At least for
the foreseeable future, enabling these per mode in the init file
shouldn't really be too much of a problem, IMO.  When more users
actually get to try this we can get a feel for how the init should best
be handled.

To me the '*-use-tree-siter' defcustoms was beautiful :)

Theo

Re: Average-user-facing interface for tree-sitter

[Theodor Thornhill]

> be handled.
>
> To me the '*-use-tree-siter' defcustoms was beautiful :)

At least with such a pretty typo...

Re: Average-user-facing interface for tree-sitter

[Yuan Fu]

> On Oct 20, 2022, at 11:10 AM, Theodor Thornhill <theo@thornhill.no> wrote:
> 
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
> 
>>>> Yes, sorry, I made some further changes to js-mode. Could you have a look
>>>> and see if it makes sense?
>>>> 
>>> I'm not sure I really like the new changes.
>> 
>> [ Context: I'm largely to blame, I proposed the change.  ]
>> 
> 
> No worries :-)
> 
>>> Now we init everything in
>>> js-mode in addition to everything treesitter related.  So now stuff like
>>> 
>>> ```
>>>  (setq-local font-lock-defaults
>>>              (list js--font-lock-keywords nil nil nil nil
>>>                    '(font-lock-syntactic-face-function
>>>                      . js-font-lock-syntactic-face-function)))
>>>  (setq-local syntax-propertize-function #'js-syntax-propertize)
>>>  (add-hook 'syntax-propertize-extend-region-functions
>>>            #'syntax-propertize-multiline 'append 'local)
>>>  (add-hook 'syntax-propertize-extend-region-functions
>>>            #'js--syntax-propertize-extend-region 'append 'local)
>>>  (setq-local prettify-symbols-alist js--prettify-symbols-alist)
>>> 
>>>  (setq-local parse-sexp-ignore-comments t)
>>> ```
>> 
>> Hmm... yeah, undoing those add-hook settings of various other variables
>> which treesit-mode doesn't use is a problem.  We need to find
>> a better way.
>> 
>> 
> 
> Yeah.  Shouldn't it be possible to just have a global var instead of a
> mode?  That way we can just look for that variable when enabling the
> mode, and avoid calling anything other than what we want.  At least for
> the foreseeable future, enabling these per mode in the init file
> shouldn't really be too much of a problem, IMO.  When more users
> actually get to try this we can get a feel for how the init should best
> be handled.
> 
> To me the '*-use-tree-siter' defcustoms was beautiful :)

Back to centralized variable, perhaps?

Yuan

Re: Average-user-facing interface for tree-sitter

[Yuan Fu]

>> 
>> Yeah.  Shouldn't it be possible to just have a global var instead of a
>> mode?  That way we can just look for that variable when enabling the
>> mode, and avoid calling anything other than what we want.  At least for
>> the foreseeable future, enabling these per mode in the init file
>> shouldn't really be too much of a problem, IMO.  When more users
>> actually get to try this we can get a feel for how the init should best
>> be handled.
>> 
>> To me the '*-use-tree-siter' defcustoms was beautiful :)
> 
> Back to centralized variable, perhaps?

I’ve thought really hard but didn’t come up with any brilliant ideas, so I’m going with centralized variable. Now, should I throw away recent commits and create a new branch so we don’t have so many changes back and forth on js.el and python.el? Plus feature/tree-sitter wouldn’t be used for long anyway since it’s merging into master soon.

Yuan

Re: Average-user-facing interface for tree-sitter

[Stefan Monnier]

>>> Yeah.  Shouldn't it be possible to just have a global var instead of a
>>> mode?  That way we can just look for that variable when enabling the
>>> mode, and avoid calling anything other than what we want.  At least for
>>> the foreseeable future, enabling these per mode in the init file
>>> shouldn't really be too much of a problem, IMO.  When more users
>>> actually get to try this we can get a feel for how the init should best
>>> be handled.
>>> 
>>> To me the '*-use-tree-siter' defcustoms was beautiful :)
>> 
>> Back to centralized variable, perhaps?
>
> I’ve thought really hard but didn’t come up with any brilliant ideas, so I’m
> going with centralized variable. Now, should I throw away recent commits and
> create a new branch so we don’t have so many changes back and forth on js.el
> and python.el? Plus feature/tree-sitter wouldn’t be used for long anyway
> since it’s merging into master soon.

I'd wait a bit more to see if some other ideas come up first.


        Stefan

Re: Average-user-facing interface for tree-sitter

[Fu Yuan]

> 
>> 
>>>> Yeah.  Shouldn't it be possible to just have a global var instead of a
>>>> mode?  That way we can just look for that variable when enabling the
>>>> mode, and avoid calling anything other than what we want.  At least for
>>>> the foreseeable future, enabling these per mode in the init file
>>>> shouldn't really be too much of a problem, IMO.  When more users
>>>> actually get to try this we can get a feel for how the init should best
>>>> be handled.
>>>> 
>>>> To me the '*-use-tree-siter' defcustoms was beautiful :)
>>> 
>>> Back to centralized variable, perhaps?
>> 
>> I’ve thought really hard but didn’t come up with any brilliant ideas, so I’m
>> going with centralized variable. Now, should I throw away recent commits and
>> create a new branch so we don’t have so many changes back and forth on js.el
>> and python.el? Plus feature/tree-sitter wouldn’t be used for long anyway
>> since it’s merging into master soon.
> 
> I'd wait a bit more to see if some other ideas come up first.

Here’s my thought (that didn’t go anywhere): since major modes sets a plethora of local hooks and variables, only the major mode itself knows how to reverse them. The cleanest way is probably to clear all the local variables and hooks and re-run the major mode setup, which suggests we should let major mode branch on whether to enable tree-sitter during initialization. I wonder if minor modes can somehow work with this model?

It would be also nice to leave room for inclusion of other “backends” besides elisp and tree-sitter in the future.

Yuan

Re: Average-user-facing interface for tree-sitter

[Theodor Thornhill]

On 23 October 2022 03:59:35 CEST, Fu Yuan <casouri@gmail.com> wrote:
>
>
>> 
>>> 
>>>>> Yeah.  Shouldn't it be possible to just have a global var instead of a
>>>>> mode?  That way we can just look for that variable when enabling the
>>>>> mode, and avoid calling anything other than what we want.  At least for
>>>>> the foreseeable future, enabling these per mode in the init file
>>>>> shouldn't really be too much of a problem, IMO.  When more users
>>>>> actually get to try this we can get a feel for how the init should best
>>>>> be handled.
>>>>> 
>>>>> To me the '*-use-tree-siter' defcustoms was beautiful :)
>>>> 
>>>> Back to centralized variable, perhaps?
>>> 
>>> I’ve thought really hard but didn’t come up with any brilliant ideas, so I’m
>>> going with centralized variable. Now, should I throw away recent commits and
>>> create a new branch so we don’t have so many changes back and forth on js.el
>>> and python.el? Plus feature/tree-sitter wouldn’t be used for long anyway
>>> since it’s merging into master soon.
>> 
>> I'd wait a bit more to see if some other ideas come up first.
>
>Here’s my thought (that didn’t go anywhere): since major modes sets a plethora of local hooks and variables, only the major mode itself knows how to reverse them. The cleanest way is probably to clear all the local variables and hooks and re-run the major mode setup, which suggests we should let major mode branch on whether to enable tree-sitter during initialization. I wonder if minor modes can somehow work with this model?

Yeah, that's what I was thinking too. We can just separate the inits into its own function and use a coed. Adding other variants will then be trivial. 

>
>It would be also nice to leave room for inclusion of other “backends” besides elisp and tree-sitter in the future.
>
>Yuan


Theo

Re: Average-user-facing interface for tree-sitter

[Stefan Monnier]

> Here’s my thought (that didn’t go anywhere): since major modes sets
> a plethora of local hooks and variables, only the major mode itself knows
> how to reverse them. The cleanest way is probably to clear all the local
> variables and hooks and re-run the major mode setup, which suggests we
> should let major mode branch on whether to enable tree-sitter during
> initialization. I wonder if minor modes can somehow work with this model?

Re-running is fairly problematic.  Not only because it risks repeating
side effects but also because it starts by killing all buffer-local
vars, so we'd need extra hacks to try and preserve the treesit-mode's
own information (making it permanent-local is one way, but that can
cause further breakage when the user really wants to change to another
mode, so it tends to be hackish).

> It would be also nice to leave room for inclusion of other “backends”
> besides elisp and tree-sitter in the future.

I'm not comfortable with this notion of "backend", because each one of
those "backends" (elisp, treesit, eglot, ...) tends to support
a different set of features, so in practice, I'd expect that in the
common case many major modes will use a mix of those backends.

A simple solution, tho not as elegant as I'd like, is to keep the code
we have (where the major mode sets all vars upfront) but add to the
major mode something like:

   (add-hook 'treesit-mode-hook #'js--treesit-mode-hook nil t)
   (js--treesit-mode-hook)

where `js--treesit-mode-hook` is in charge of removing those settings
that don't apply when `treesit-mode` is enabled` (and to re-instate
them when `treesit-mode` is disabled, which is why I call it right away
in the example above, so we don't duplicate the code between the major
mode's body and the `js--treesit-mode-hook`).

Sample completely untested patch below.

We could try and help write this code by providing a helper function
that relies on some buffer-local var containing a list of vars to be set
(along with their values), a list of hooks to add (and remove), ...
so we don't need to duplicate the list into a "set" and an "unset"
branch like I had to do in the patch.

Note that it's very similar to a "backend" function.  But it's only
meant to choose between "treesit activated" and "treesit not activated".


        Stefan


diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
index 52160fbb5ee..94295da5167 100644
--- a/lisp/progmodes/js.el
+++ b/lisp/progmodes/js.el
@@ -3617,14 +3617,8 @@ js-mode
               (list js--font-lock-keywords nil nil nil nil
                     '(font-lock-syntactic-face-function
                       . js-font-lock-syntactic-face-function)))
-  (setq-local syntax-propertize-function #'js-syntax-propertize)
-  (add-hook 'syntax-propertize-extend-region-functions
-            #'syntax-propertize-multiline 'append 'local)
-  (add-hook 'syntax-propertize-extend-region-functions
-            #'js--syntax-propertize-extend-region 'append 'local)
   (setq-local prettify-symbols-alist js--prettify-symbols-alist)
 
-  (setq-local parse-sexp-ignore-comments t)
   (setq-local which-func-imenu-joiner-function #'js--which-func-joiner)
 
   ;; Comments
@@ -3634,25 +3628,11 @@ js-mode
   (setq-local fill-paragraph-function #'js-fill-paragraph)
   (setq-local normal-auto-fill-function #'js-do-auto-fill)
 
-  ;; Parse cache
-  (add-hook 'before-change-functions #'js--flush-caches t t)
-
-  ;; Frameworks
-  (js--update-quick-match-re)
-
-  ;; Syntax extensions
-  (unless (js-jsx--detect-and-enable)
-    (add-hook 'after-change-functions #'js-jsx--detect-after-change nil t))
-  (js-use-syntactic-mode-name)
-
   ;; Imenu
   (setq imenu-case-fold-search nil)
   (setq imenu-create-index-function #'js--imenu-create-index)
 
   ;; for filling, pretend we're cc-mode
-  (c-foreign-init-lit-pos-cache)
-  (add-hook 'before-change-functions #'c-foreign-truncate-lit-pos-cache nil t)
-  (setq-local comment-line-break-function #'c-indent-new-comment-line)
   (setq-local comment-multi-line t)
   (setq-local electric-indent-chars
 	      (append "{}():;," electric-indent-chars)) ;FIXME: js2-mode adds "[]*".
@@ -3698,7 +3678,51 @@ js-mode
                       "function_declaration"
                       "lexical_declaration")))
   (setq-local treesit-font-lock-settings js--treesit-font-lock-settings)
-  (setq-local treesit-font-lock-feature-list '((minimal) (moderate) (full))))
+  (setq-local treesit-font-lock-feature-list '((minimal) (moderate) (full)))
+
+  (add-hook 'treesit-mode-hook #'js--treesit-mode-hook nil t)
+  (js--treesit-mode-hook))
+
+(defun js--treesit-mode-hook ()
+  (cond
+   (treesit-mode
+    (kill-local-variable 'syntax-propertize-function)
+    (remove-hook 'syntax-propertize-extend-region-functions
+                 #'syntax-propertize-multiline 'local)
+    (remove-hook 'syntax-propertize-extend-region-functions
+                 #'js--syntax-propertize-extend-region 'local)
+    (kill-local-variable 'parse-sexp-ignore-comments)
+    (remove-hook 'before-change-functions #'js--flush-caches t)
+
+    ;; Syntax extensions
+    (remove-hook 'after-change-functions #'js-jsx--detect-after-change t)
+    (js-use-syntactic-mode-name)        ;FIXME?
+
+    (remove-hook 'before-change-functions #'c-foreign-truncate-lit-pos-cache t)
+    (kill-local-variable 'comment-line-break-function)
+    )
+   (t
+    ;; Parse cache
+    (setq-local syntax-propertize-function #'js-syntax-propertize)
+    (add-hook 'syntax-propertize-extend-region-functions
+              #'syntax-propertize-multiline 'append 'local)
+    (add-hook 'syntax-propertize-extend-region-functions
+              #'js--syntax-propertize-extend-region 'append 'local)
+    (setq-local parse-sexp-ignore-comments t)
+    (add-hook 'before-change-functions #'js--flush-caches t t)
+
+    ;; Frameworks
+    (js--update-quick-match-re)
+
+    ;; Syntax extensions
+    (unless (js-jsx--detect-and-enable)
+      (add-hook 'after-change-functions #'js-jsx--detect-after-change nil t))
+    (js-use-syntactic-mode-name)
+
+    (c-foreign-init-lit-pos-cache)
+    (add-hook 'before-change-functions #'c-foreign-truncate-lit-pos-cache nil t)
+    (setq-local comment-line-break-function #'c-indent-new-comment-line)
+    )))
 
 (defvar js-json--treesit-font-lock-settings
   (treesit-font-lock-rules

Re: Average-user-facing interface for tree-sitter

[Stephen Leake]

Fu Yuan <casouri@gmail.com> writes:

>> 
>>> 
>>>>> Yeah.  Shouldn't it be possible to just have a global var instead of a
>>>>> mode?  That way we can just look for that variable when enabling the
>>>>> mode, and avoid calling anything other than what we want.  At least for
>>>>> the foreseeable future, enabling these per mode in the init file
>>>>> shouldn't really be too much of a problem, IMO.  When more users
>>>>> actually get to try this we can get a feel for how the init should best
>>>>> be handled.
>>>>> 
>>>>> To me the '*-use-tree-siter' defcustoms was beautiful :)
>>>> 
>>>> Back to centralized variable, perhaps?
>>> 
>>> I’ve thought really hard but didn’t come up with any brilliant ideas, so I’m
>>> going with centralized variable. Now, should I throw away recent commits and
>>> create a new branch so we don’t have so many changes back and forth on js.el
>>> and python.el? Plus feature/tree-sitter wouldn’t be used for long anyway
>>> since it’s merging into master soon.
>> 
>> I'd wait a bit more to see if some other ideas come up first.
>
> Here’s my thought (that didn’t go anywhere): since major modes sets a
> plethora of local hooks and variables, only the major mode itself
> knows how to reverse them. The cleanest way is probably to clear all
> the local variables and hooks and re-run the major mode setup, which
> suggests we should let major mode branch on whether to enable
> tree-sitter during initialization. 

That's what I ended up with in ada-mode; it decides on xref, indent,
face backend in the major-mode function based on ada-*-backend
variables. Actually, in a post-local-vars function; we need to allow
dir-local and file-local hooks to set those vars.

> I wonder if minor modes can somehow work with this model?

It seems to me minor modes are only needed if you want to have different
menus and keybindings for each minor mode. In ada-mode, I just
enable menu entries based on the current backends; I left keybindings
alone for now, which means many of them are undefined or non-functional.
Not the ideal solution.

Is there something written on why Emacs prefers minor-modes for choices
like this?

> It would be also nice to leave room for inclusion of other “backends”
> besides elisp and tree-sitter in the future.

Right. wisi is one, available now.

-- 
-- Stephe

Re: Average-user-facing interface for tree-sitter

[Stephen Leake]

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

>> Here’s my thought (that didn’t go anywhere): since major modes sets
>> a plethora of local hooks and variables, only the major mode itself knows
>> how to reverse them. The cleanest way is probably to clear all the local
>> variables and hooks and re-run the major mode setup, which suggests we
>> should let major mode branch on whether to enable tree-sitter during
>> initialization. I wonder if minor modes can somehow work with this model?
>
> Re-running is fairly problematic.  Not only because it risks repeating
> side effects but also because it starts by killing all buffer-local
> vars, so we'd need extra hacks to try and preserve the treesit-mode's
> own information (making it permanent-local is one way, but that can
> cause further breakage when the user really wants to change to another
> mode, so it tends to be hackish).
>
>> It would be also nice to leave room for inclusion of other “backends”
>> besides elisp and tree-sitter in the future.
>
> I'm not comfortable with this notion of "backend", because each one of
> those "backends" (elisp, treesit, eglot, ...) tends to support
> a different set of features, so in practice, I'd expect that in the
> common case many major modes will use a mix of those backends.

Yes, one backend choice for each feature (most backends will provide
more than one feature). There cannot be two backends for indent, but
there can be different backends for indent and face.

> A simple solution, tho not as elegant as I'd like, is to keep the code
> we have (where the major mode sets all vars upfront) but add to the
> major mode something like:
>
>    (add-hook 'treesit-mode-hook #'js--treesit-mode-hook nil t)
>    (js--treesit-mode-hook)
>
> where `js--treesit-mode-hook` is in charge of removing those settings
> that don't apply when `treesit-mode` is enabled` (and to re-instate
> them when `treesit-mode` is disabled, which is why I call it right away
> in the example above, so we don't duplicate the code between the major
> mode's body and the `js--treesit-mode-hook`).

Since js--treesit-mode-hook is provided by the major-mode, how is that
better than simply including that code in js-mode, in a cond or cl-ecase
on the desired backend for each feature?

Ah; js--treesit-mode-hook (I object to the name; it's _not_ a hook
variable! It might as well be js-treesit-minor-mode) also does unset.

But what calls it to do the unset? I don't see the need for that, except
possibly when the user is still experimenting to determine which backend
is best. In that case, I always prefer restarting Emacs; that's the only
way to ensure the previous mode is fully unset (ie, not even loaded).

I set the backend choices in my .emacs, and change them rarely (next
time I change one will be when ada_language_server gains
SemanticToken support, allowing face via eglot).

I suppose there could be a situation where one xref backend is good at
one task (say finding references in system libraries), while another is
good at something else (maybe finding all refs withing one file); then
you might want to swap backends on the fly. The wisi and
ada_language_server xref facilities may actually be like that; I need to
experiment some more. Or this could argue for splitting the xref feature
in two, for global and local xref. It will take some experience to
settle on a good common set of features.

Another place where the backend matters; refactoring. For example wisi
and ada_language_server offer disjoint refactoring operations, all
useful. So to allow using all of them in ada-mode, each refactoring
function will use whichever backend provides that function; there will
be no ada-refactoring-backend setting.

> We could try and help write this code by providing a helper function
> that relies on some buffer-local var containing a list of vars to be set
> (along with their values), a list of hooks to add (and remove), ...
> so we don't need to duplicate the list into a "set" and an "unset"
> branch like I had to do in the patch.

That would be good.

> Note that it's very similar to a "backend" function.  But it's only
> meant to choose between "treesit activated" and "treesit not
> activated".

We should also allow for eglot, wisi, and other future backends.

-- 
-- Stephe

Re: Average-user-facing interface for tree-sitter

[Yuan Fu]

> On Oct 24, 2022, at 5:57 AM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> 
>> Here’s my thought (that didn’t go anywhere): since major modes sets
>> a plethora of local hooks and variables, only the major mode itself knows
>> how to reverse them. The cleanest way is probably to clear all the local
>> variables and hooks and re-run the major mode setup, which suggests we
>> should let major mode branch on whether to enable tree-sitter during
>> initialization. I wonder if minor modes can somehow work with this model?
> 
> Re-running is fairly problematic.  Not only because it risks repeating
> side effects but also because it starts by killing all buffer-local
> vars, so we'd need extra hacks to try and preserve the treesit-mode's
> own information (making it permanent-local is one way, but that can
> cause further breakage when the user really wants to change to another
> mode, so it tends to be hackish).

I agree, tho I think the risk of side effect is a bit exaggerated. 

> 
>> It would be also nice to leave room for inclusion of other “backends”
>> besides elisp and tree-sitter in the future.
> 
> I'm not comfortable with this notion of "backend", because each one of
> those "backends" (elisp, treesit, eglot, ...) tends to support
> a different set of features, so in practice, I'd expect that in the
> common case many major modes will use a mix of those backends.
> 
> A simple solution, tho not as elegant as I'd like, is to keep the code
> we have (where the major mode sets all vars upfront) but add to the
> major mode something like:
> 
>   (add-hook 'treesit-mode-hook #'js--treesit-mode-hook nil t)
>   (js--treesit-mode-hook)
> 
> where `js--treesit-mode-hook` is in charge of removing those settings
> that don't apply when `treesit-mode` is enabled` (and to re-instate
> them when `treesit-mode` is disabled, which is why I call it right away
> in the example above, so we don't duplicate the code between the major
> mode's body and the `js--treesit-mode-hook`).
> 
> Sample completely untested patch below.
> 
> We could try and help write this code by providing a helper function
> that relies on some buffer-local var containing a list of vars to be set
> (along with their values), a list of hooks to add (and remove), ...
> so we don't need to duplicate the list into a "set" and an "unset"
> branch like I had to do in the patch.
> 
> Note that it's very similar to a "backend" function.  But it's only
> meant to choose between "treesit activated" and "treesit not activated”.

Now the minor mode scheme is getting more and more complicated that I wonder if the benefit is worth it. Maybe we should revisit the central variable scheme, which I still think is pretty clean and convenient:
> 
>> 
>> This function would handle derived mode ok, but I don’t know what are
>> the other problems you are think of, maybe you can tell me what this
>> function falls short for. And we can go from there.
> 
> The function is one thing.  Another question is when it is run, and
> hence how it interacts with other ways to enable/set/control the thing
> you want to control (in this case enabling `treesit-mode`, IIUC).

There will not be treesit-mode and global-treesit-mode, only the central variable treesit-settings. Enabling tree-sitter only requires adding (mode t) to treesit-setttings, and M-x revert-buffer, or reopen the file, or rerun the major mode. Actually, 99% of the time people would just set that setting, enabling tree-sitter for a mode (or all supported modes) and be done with it. Ie, there is not a lot of toggling that calls for a minor mode. Minor mode are still nice since one can add it to the major mode hook, but we’ve seen that it brings more trouble than benefit.

Letting major modes to branch on a global variable is also cleaner and easier to write than adding code to enable/disable a bunch of setups, IMO.

At this point, I think we see how minor modes clearly are a bit awkward to control major mode behavior with. And AFAIK the central variable doesn’t have any visible issues. So I think central variable is better.

Yuan

Re: Average-user-facing interface for tree-sitter

[Stefan Monnier]

>> I'm not comfortable with this notion of "backend", because each one of
>> those "backends" (elisp, treesit, eglot, ...) tends to support
>> a different set of features, so in practice, I'd expect that in the
>> common case many major modes will use a mix of those backends.
> Yes, one backend choice for each feature (most backends will provide
> more than one feature). There cannot be two backends for indent, but
> there can be different backends for indent and face.

Individual major modes can indeed offer more fine-grained control, but
I'm not convinced we want/need that complexity in the generic code.
Especially since existing "ELisp backends" (CC-mode, SMIE, and ad-hoc
ones) were not written with such piecemeal use in mind, so there's
a good chance we'd bump into more bugs.

I'd rather start small and later add such refined control if/when this
proves to be often necessary, at which point we'll know better what are
the pitfalls.

>>    (add-hook 'treesit-mode-hook #'js--treesit-mode-hook nil t)
>>    (js--treesit-mode-hook)
[...]
> Since js--treesit-mode-hook is provided by the major-mode, how is that
> better than simply including that code in js-mode, in a cond or cl-ecase
> on the desired backend for each feature?

We can't test directly from the major mode function because we don't
know yet if `treesit-mode` will be enabled.  It'd have to be postponed
to `hack-local-variables` anyway.
[ IIUC you do that for ada-mode, right?  ]

> Ah; js--treesit-mode-hook (I object to the name;

[ I object too, but I was in a hurry.  Now that I have more time to
  think about it, `js--post-treesit-setup` seems better.  ]

> But what calls it to do the unset?

Minor mode hooks are called both when enabling and disabling the mode.

> I set the backend choices in my .emacs, and change them rarely (next

Toggling `treesit-mode` dynamically is expected to be rare, indeed.
But you might enable `treesit-mode` in a mode-hook and then want to
disable it in a child mode's hook.

>> We could try and help write this code by providing a helper function
>> that relies on some buffer-local var containing a list of vars to be set
>> (along with their values), a list of hooks to add (and remove), ...
>> so we don't need to duplicate the list into a "set" and an "unset"
>> branch like I had to do in the patch.
> That would be good.
>> Note that it's very similar to a "backend" function.  But it's only
>> meant to choose between "treesit activated" and "treesit not
>> activated".
> We should also allow for eglot, wisi, and other future backends.

My hope is that for a given major mode we'll usually know which
functionality should better be offered via `treesit-mode` or via
`eglot-mode`.  We'll definitely want to allow the user to refine this
choice, but I don't think we have enough experience yet with it to know
how important it is and what it should look like.


        Stefan

Re: Average-user-facing interface for tree-sitter

[Stefan Monnier]

> There will not be treesit-mode and global-treesit-mode, only the central
> variable treesit-settings. Enabling tree-sitter only requires adding (mode
> t) to treesit-setttings, and M-x revert-buffer, or reopen the file, or rerun
> the major mode.

Yeah, I guess it's good enough for now.  We can still keep all the
treesit-<foo> variables and then a `treesit-enable` function which the
major mode has to call, right?  Oh and probably something like
`treesit-enable-p` function (or maybe `treesit-enable` can return
whether it enabled the treesit support).

So the major mode code could look like in the patch below?


        Stefan


diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
index 52160fbb5ee..4cea9f4ec1a 100644
--- a/lisp/progmodes/js.el
+++ b/lisp/progmodes/js.el
@@ -3617,14 +3617,8 @@ js-mode
               (list js--font-lock-keywords nil nil nil nil
                     '(font-lock-syntactic-face-function
                       . js-font-lock-syntactic-face-function)))
-  (setq-local syntax-propertize-function #'js-syntax-propertize)
-  (add-hook 'syntax-propertize-extend-region-functions
-            #'syntax-propertize-multiline 'append 'local)
-  (add-hook 'syntax-propertize-extend-region-functions
-            #'js--syntax-propertize-extend-region 'append 'local)
   (setq-local prettify-symbols-alist js--prettify-symbols-alist)
 
-  (setq-local parse-sexp-ignore-comments t)
   (setq-local which-func-imenu-joiner-function #'js--which-func-joiner)
 
   ;; Comments
@@ -3634,25 +3628,11 @@ js-mode
   (setq-local fill-paragraph-function #'js-fill-paragraph)
   (setq-local normal-auto-fill-function #'js-do-auto-fill)
 
-  ;; Parse cache
-  (add-hook 'before-change-functions #'js--flush-caches t t)
-
-  ;; Frameworks
-  (js--update-quick-match-re)
-
-  ;; Syntax extensions
-  (unless (js-jsx--detect-and-enable)
-    (add-hook 'after-change-functions #'js-jsx--detect-after-change nil t))
-  (js-use-syntactic-mode-name)
-
   ;; Imenu
   (setq imenu-case-fold-search nil)
   (setq imenu-create-index-function #'js--imenu-create-index)
 
   ;; for filling, pretend we're cc-mode
-  (c-foreign-init-lit-pos-cache)
-  (add-hook 'before-change-functions #'c-foreign-truncate-lit-pos-cache nil t)
-  (setq-local comment-line-break-function #'c-indent-new-comment-line)
   (setq-local comment-multi-line t)
   (setq-local electric-indent-chars
 	      (append "{}():;," electric-indent-chars)) ;FIXME: js2-mode adds "[]*".
@@ -3698,7 +3678,29 @@ js-mode
                       "function_declaration"
                       "lexical_declaration")))
   (setq-local treesit-font-lock-settings js--treesit-font-lock-settings)
-  (setq-local treesit-font-lock-feature-list '((minimal) (moderate) (full))))
+  (setq-local treesit-font-lock-feature-list '((minimal) (moderate) (full)))
+
+  (unless (treesit-enable)
+    ;; Parse cache
+    (setq-local syntax-propertize-function #'js-syntax-propertize)
+    (add-hook 'syntax-propertize-extend-region-functions
+              #'syntax-propertize-multiline 'append 'local)
+    (add-hook 'syntax-propertize-extend-region-functions
+              #'js--syntax-propertize-extend-region 'append 'local)
+    (setq-local parse-sexp-ignore-comments t)
+    (add-hook 'before-change-functions #'js--flush-caches t t)
+
+    ;; Frameworks
+    (js--update-quick-match-re)
+
+    ;; Syntax extensions
+    (unless (js-jsx--detect-and-enable)
+      (add-hook 'after-change-functions #'js-jsx--detect-after-change nil t))
+    (js-use-syntactic-mode-name)
+
+    (c-foreign-init-lit-pos-cache)
+    (add-hook 'before-change-functions #'c-foreign-truncate-lit-pos-cache nil t)
+    (setq-local comment-line-break-function #'c-indent-new-comment-line)))
 
 (defvar js-json--treesit-font-lock-settings
   (treesit-font-lock-rules

Re: Average-user-facing interface for tree-sitter

[Yuan Fu]

> On Oct 24, 2022, at 4:55 PM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> 
>> There will not be treesit-mode and global-treesit-mode, only the central
>> variable treesit-settings. Enabling tree-sitter only requires adding (mode
>> t) to treesit-setttings, and M-x revert-buffer, or reopen the file, or rerun
>> the major mode.
> 
> Yeah, I guess it's good enough for now.  We can still keep all the
> treesit-<foo> variables and then a `treesit-enable` function which the
> major mode has to call, right?  Oh and probably something like
> `treesit-enable-p` function (or maybe `treesit-enable` can return
> whether it enabled the treesit support).
> 
> So the major mode code could look like in the patch below?

Ok, I pushed the change. I didn’t move as many setup in js-mode as you did in the patch, because I think some of them are still needed? For example, filling. Js-mode’s tree-sitter code doesn’t provide filling right now, so we should setup filling regardless of tree-sitter.

Yuan

Re: Average-user-facing interface for tree-sitter

[Stefan Monnier]

> Ok, I pushed the change. I didn’t move as many setup in js-mode as you did
> in the patch, because I think some of them are still needed?  For example,
> filling.  Js-mode’s tree-sitter code doesn’t provide filling right now, so we
> should setup filling regardless of tree-sitter.

Yes, there are indeed some non-trivial tradeoffs here.


        Stefan

Re: Average-user-facing interface for tree-sitter

[Yuan Fu]

> On Oct 25, 2022, at 3:49 PM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> 
>> Ok, I pushed the change. I didn’t move as many setup in js-mode as you did
>> in the patch, because I think some of them are still needed?  For example,
>> filling.  Js-mode’s tree-sitter code doesn’t provide filling right now, so we
>> should setup filling regardless of tree-sitter.
> 
> Yes, there are indeed some non-trivial tradeoffs here.

Reading some old bug report made me realize another problem: what if a derived-mode of js-mode, A-mode expects cc-mode stuff to be setup by js-mode, but js-mode doesn’t setup cc-mode because it’s using tree-sitter?

But if A-mode uses tree-sitter too, then it wouldn’t want js-mode to setup cc-mode. So it seems A-mode needs a way to control whether it wants its parent mode(s) to setup tree-sitter or not.

One way would be adding another flag in treesit-settings, which controls whether this mode’s parent should setup tree-sitter.

Yuan

Re: Average-user-facing interface for tree-sitter

[Stefan Monnier]

>> Yes, there are indeed some non-trivial tradeoffs here.
>
> Reading some old bug report made me realize another problem: what if
> a derived-mode of js-mode, A-mode expects cc-mode stuff to be setup by
> js-mode, but js-mode doesn’t setup cc-mode because it’s using tree-sitter?
>
> But if A-mode uses tree-sitter too, then it wouldn’t want js-mode to setup
> cc-mode. So it seems A-mode needs a way to control whether it wants its
> parent mode(s) to setup tree-sitter or not.
>
> One way would be adding another flag in treesit-settings, which controls
> whether this mode’s parent should setup tree-sitter.

Sounds too hypothetical to make a good decision on what should be done
about it.  Let's cross that bridge when we get there.  In the mean time
I think it's perfectly acceptable to say "don't do that if it hurts".


        Stefan

Re: Average-user-facing interface for tree-sitter

[Dmitry Gutov]

On 27.10.2022 18:21, Stefan Monnier wrote:
>> Yes, there are indeed some non-trivial tradeoffs here.
> Reading some old bug report made me realize another problem: what if
> a derived-mode of js-mode, A-mode expects cc-mode stuff to be setup by
> js-mode, but js-mode doesn’t setup cc-mode because it’s using tree-sitter?

js2-mode also derives from js-mode, and I think it might be rather 
surprised to encounter TreeSitter integration in its buffers.

Not sure how much it's going to conflict in practice, but using two 
different parsers at the same time sounds problematic.

Re: Average-user-facing interface for tree-sitter

[Yuan Fu]

> On Oct 27, 2022, at 8:29 AM, Dmitry Gutov <dgutov@yandex.ru> wrote:
> 
> On 27.10.2022 18:21, Stefan Monnier wrote:
>>> Yes, there are indeed some non-trivial tradeoffs here.
>> Reading some old bug report made me realize another problem: what if
>> a derived-mode of js-mode, A-mode expects cc-mode stuff to be setup by
>> js-mode, but js-mode doesn’t setup cc-mode because it’s using tree-sitter?
> 
> js2-mode also derives from js-mode, and I think it might be rather surprised to encounter TreeSitter integration in its buffers.
> 
> Not sure how much it's going to conflict in practice, but using two different parsers at the same time sounds problematic.

Yes, I think there are roughly two types of derived modes, one that only add minor change or even personal settings to the parent mode, like python-mode w/ my-python-mode; the other derives from a mode that is essentially another language, like ts-mode w/ js-mode, or json-mode with js-mode.

The first kind probably wants parent mode’s tree-sitter setup (if it is enabled by the user), but the second one probably don’t, and they might need non-tree-sitter setup made by the parent mode.

I think derived major modes should have a way to opt-in to parent mode’s tree-sitter setups. It doesn’t need to be a user-facing option, but something a major mode configures upon definition.

Yuan