search

emacs-tree-sitter / elisp-tree-sitter

Emacs Lisp bindings for tree-sitter
https://emacs-tree-sitter.github.io
MIT License
827 stars 74 forks source link

csharp-mode: No highlighting after 547705d #84

Open theothornhill opened 3 years ago

theothornhill commented 3 years ago

Right now I'm using tree-sitter-hl-default-patterns inside csharp-mode to set the patterns. That seemed to work fine until latest commit, 547705d. I'm not completely sure why it stopped working, but now no highlighting shows. If I roll back one commit everything seems to be working again.

See: https://github.com/emacs-csharp/csharp-mode/blob/tree-sitter/csharp-mode.el#L335 for how the tentative, wip implementation is inside csharp-mode

ubolonton commented 3 years ago

When I first read this, I thought I knew what the issue was. But then I tried your code, and highlighting worked for me, so I don't know anymore 😅

Can you check the following variables, to see whether they have these expected buffer-local values:

theothornhill commented 3 years ago

My values, where some are different:

By the way, font locking in rust mode or some of the modes included here works. It is csharp-mode that isn't fontifying anymore.

I'll also add that I'm using emacs 27.1 on windows 10. My linux computer died, so cannot check there atm

theothornhill commented 3 years ago

And in 7875466db4fa02d0a2b8e847c7b31df2706a8935 I have the same values as you

ubolonton commented 3 years ago

Does setting font-lock-defaults to a non-nil value when csharp-mode is turned on fix it? Something similar to this:

(unless font-lock-defaults
  (setq font-lock-defaults '(nil)))

If that temporary fix works, I'll write down the explanation later.

theothornhill commented 3 years ago

This works! Explain! :D

ubolonton commented 3 years ago

font-lock relies on jit-lock for invalidation (on text changes and "viewport" changes).

As a "replacement" for font-lock, tree-sitter-hl should integrate with jit-lock as well. Instead of integrating with that directly, and replacing font-lock, tree-sitter-hl simply overrides font-lock functions. There are 2 reasons for this:

In other words, tree-sitter-hl works on top of font-lock, instead of disabling it outright.

font-lock has a "smart check" (see the end of font-lock-default-function) that, in certain situations, skips some of its initialization, including the jit-lock integration . Therefore, tree-sitter-hlused to have a hack that turns on this integration when font-lock skips it. This allowed tree-sitter-hl to work even without actually installing the corresponding major mode. However, that feature was removed because font-lock doesn't properly clean up this integration when it's explicitly disabled, causing #74.

The "certain conditions" above include checking whether any font-lock keywords were set. I have some global minor modes, so the keywords are always set for me. Without any minor modes adding font-lock keywords, one way for a major mode to trick font-lock is to specify an empty list of keywords: (setq font-lock-defaults '(nil)).

These fixes would probably be more reliable:

ubolonton commented 3 years ago

Anyway, I think csharp-mode may want to initially treat tree-sitter as an optional dependency, keeping its current font-lock integration (font-lock-defaults) for a (long) while. If that's the case, this should be a non-issue for csharp-mode.

theothornhill commented 3 years ago

Thank you - this is useful information!

I was thinking tree sitter should be optional, considering it doesn’t seem to support *BSD etc, while cc mode does.

jcs090218 commented 3 years ago 
(unless font-lock-defaults
 (setq font-lock-defaults '(nil)))

I have tried this but it seems to be not working in the latest version of tree-sitter and csharp-mode. Any idea?

theothornhill commented 3 years ago

What isn't working? Right now I'm using the latest of both, and everything seems fine to me. Could you elaborate?

jcs090218 commented 3 years ago

The highlighting isn't working. Following is my config for tree-sitter.

(use-package tree-sitter
  :config
  (require 'tree-sitter-langs)
  (add-hook 'tree-sitter-after-on-hook #'tree-sitter-hl-mode))

(global-tree-sitter-mode 1)

This works with other major mode but not csharp-mode. I have also tried emacs -q and it still not working. However, csharp-tree-sitter-mode did work.

jcs090218 commented 3 years ago

Okay, I think the issue is there are no c-sharp highlight queries file in the directory here.

https://github.com/ubolonton/emacs-tree-sitter/tree/master/langs/queries

theothornhill commented 3 years ago

Okay, I think the issue is there are no c-sharp highlight queries file in the directory here.

https://github.com/ubolonton/emacs-tree-sitter/tree/master/langs/queries

That's not the case. The reason is that you are trying to apply tree-sitter to normal csharp mode, and that is absolutely not a good idea, https://github.com/emacs-csharp/csharp-mode/issues/207 being the most important point. Please rather submit you additions to csharp-tree-sitter-mode instead :)

jcs090218 commented 3 years ago

The reason is that you are trying to apply tree-sitter to normal csharp mode, and that is absolutely not a good idea, emacs-csharp/csharp-mode#207 being the most important point.

Yes. That is why I was proposing to convert csharp-tree-sitter-mode to minor mode in the first place, see https://github.com/emacs-csharp/csharp-mode/pull/216. I do need tree-sitter runs in csharp-mode and not csharp-tree-sitter-mode because many packages use csharp-mode as an identifier.

Plus, tree-sitter is a minor mode so the user should customize it so tree-sitter does not run in csharp-mode (if they do not want tree-sitter-mode runs in csharp-mode). And there is no conflict with csharp-tree-sitter-mode since it is a major mode and NOT minor mode.

Please rather submit you additions to csharp-tree-sitter-mode instead :)

Like as said, I do need csharp-mode and not csharp-tree-sitter-mode as an identifier. I know the problem from https://github.com/emacs-csharp/csharp-mode/issues/207 but I can live with it. Furthermore, I run tree-sitter in c-mode, c++mode, java-mode, etc. They all rely on cc-mode, so basically saying I can not avoid the slowness from cc-mode. Why bother?

From my understanding, csharp-tree-sitter-mode is not going to replace csharp-mode? Then I don't see any conflicts by running tree-sitter inside csharp-mode. Especially, tree-sitter is a minor mode and it's very easy to enable/disable.

From the software architect's point of view, I see these two methods.

Contribute to upstream (my way) Contribute to csharp-mode
Image 3 Image 4

Hence, I have decided to make contribution to this and not csharp-mode.

Edit: Register charp-mode to tree-sitter-langs may be unnecessary but I would still prefer architect on the left.

theothornhill commented 3 years ago

Yes. That is why I was proposing to convert csharp-tree-sitter-mode to minor mode in the first place, see emacs-csharp/csharp-mode#216. I do need tree-sitter runs in csharp-mode and not csharp-tree-sitter-mode because many packages use csharp-mode as an identifier.

This is a valid concern, and I have stated that at some point we should indeed make the tree-sitter version the main one. Contributing to that mode will help that goal be reached. "Forking" like this will not.

Plus, tree-sitter is a minor mode so the user should customize it so tree-sitter does not run in csharp-mode (if they do not want tree-sitter-mode runs in csharp-mode). And there is no conflict with csharp-tree-sitter-mode since it is a major mode and NOT minor mode.

I don't understand what you are saying here. It is a minor mode precisely so that major-modes should use it.

Please rather submit you additions to csharp-tree-sitter-mode instead :)

Like as said, I do need csharp-mode and not csharp-tree-sitter-mode as an identifier. I know the problem from emacs-csharp/csharp-mode#207 but I can live with it. Furthermore, I run tree-sitter in c-mode, c++mode, java-mode, etc. They all rely on cc-mode, so basically saying I can not avoid the slowness from cc-mode. Why bother?

The only modes suffering from this is pike-mode and csharp-mode. So yes, the other modes are fine. This is because of the way cc-mode handles strings. If you don't care about performance, why not use the normal csharp-mode? Your issues are solved then. Performance is not the only reason either. CC Mode uses its own font locking mechanisms that break in subtle ways when combined with other ways for emacs to fontify/propertize. I've tried to explain this several times before.

From my understanding, csharp-tree-sitter-mode is not going to replace csharp-mode? Then I don't see any conflicts by running tree-sitter inside csharp-mode. Especially, tree-sitter is a minor mode and it's very easy to enable/disable.

See above

From the software architect's point of view, I see these two methods.

Contribute to upstream (my way) Contribute to csharp-mode Hence, I have decided to make contribution to this and not csharp-mode.

Frankly, this doesn't make any sense.

Edit: Register charp-mode to tree-sitter-langs may be unnecessary but I would still prefer architect on the left.

Well, from an "architect" point of view, I'd rather fix what's broken and not completely gloss over serious issues.

jcs090218 commented 3 years ago

This is a valid concern, and I have stated that at some point we should indeed make the tree-sitter version the main one.

Good, I will wait until that day.

Contributing to that mode will help that goal be reached. "Forking" like this will not.

I would not say this is a fork since tree-sitter-langs is package doing these kind of tasks. If you just want tree-sitter to work you probably should not use tree-sitter-langs since there is no point to use it.

I don't understand what you are saying here. It is a minor mode precisely so that major-modes should use it.

Easy, because you cannot have two major modes at a time.

Well, from an "architect" point of view, I'd rather fix what's broken and not completely gloss over serious issues.

True, so you have csharp-tree-sitter-mode and not csharp-mode (cc-mode) + tree-sitter. Good fix.

People who suffer from this issue SHOULD ALL USE csharp-tree-sitter-mode, AND NOT csharp-mode (From csharp-mode's README page, see https://github.com/emacs-csharp/csharp-mode#tree-sitter-support). I love your solution!

The only modes suffering from this is pike-mode and csharp-mode.

Okay, I was wrong. Sorry for the incorrect information.

If you don't care about performance, why not use the normal csharp-mode? Your issues are solved then.

Not solved. I am using csharp-mode but tree-sitter has a better highlighting. Maybe you can rewrite an entire highlighting from csharp-mode? I would love to use if you have a better highlighting from csharp-mode + tree-sitter.

Performance is not the only reason either. CC Mode uses its own font locking mechanisms that break in subtle ways when combined with other ways for emacs to fontify/propertize. I've tried to explain this several times before.

Well, if you can replace csharp-tree-sitter-mode to csharp-mode. Why not? I would love to use it. (Maybe don't rely on cc-mode and somehow rewrite it so [csharp-mode/#207]() doesn't occur)

Okay, so this is what I see.

What @theothornhill wants

  1. Solve issue csharp/#207 (not to rely on cc-mode)

What @jcs090218 wants

  1. I need csharp-mode as identifier
  2. I need tree-sitter in csharp-mode (same reason from Pt. 1)
jcs090218 commented 3 years ago

BTW, if you are going to make csharp-tree-sitter-mode officially the csharp-mode. Can you move highlight query to somewhere else (not in the code)? So we don't need to wait for melpa to update.

theothornhill commented 3 years ago

I think we want the sme thing. Open an issue with csharp-mode, and lets start working towards that goal. First goal should be to incorporate your additions to my code from your pr here.

There are some blockers, like M1 support missing etc, but we will get there quickly.

We will discuss the roadmap further over there. This issue is way off topic already :)

jcs090218 commented 3 years ago

Sorry that I was being a bit rude here.

TBH, I am not good at creating major mode which I am very appreciate your work on csharp-mode! I am proposing this PR #105 because this is the easiest thing for me to do. And I personally think giving highlights.scm file is good for decoupling code (so you don't need to wait for melpa to build). I would suggest focus on solving [csharp-mode/207]() and rely on tree-sitter since we already added tree-sitter as charp-mode's dependency, see https://github.com/emacs-csharp/csharp-mode/pull/224 and https://github.com/emacs-csharp/csharp-mode/commit/98a179ad9c8bb7c816986cdd5b9afbe2fcc77f78.

Unless you insist to have query file inside csharp-mode's repository. Well, it's easier for me to contribute in once place (here).

We will discuss the roadmap further over there. This issue is way off topic already :)

Of course, you can ping me whenever you want to discuss this. I am willing to help out if I could. :)