minad / cape

🦸cape.el - Completion At Point Extensions
GNU General Public License v3.0
603 stars 22 forks source link

pick up treesitter faces with cape-wrap-faces #109

Closed LemonBreezes closed 7 months ago

LemonBreezes commented 7 months ago

This is probably the most subtle bug ever. Cape doesn't pick up the tree-sitter faces unless I use get-pos-property instead of get-text-property.

minad commented 7 months ago

Thanks. Can you please explain why this is necessary? I assume the problem is that the face is not yet there because at the time you are typing, the syntax highlighting hasn't been applied yet? get-char-property (and also get-pos-property) are not really what we want otherwise since they also take overlays into account. Do you have a simple example starting from emacs -Q which demonstrates the problem?

minad commented 7 months ago

I've also seen that you reported get-pos-property related bugs on emacs-devel. It seems this change would make Cape less robust, so I am a little bit worried to merge this.

LemonBreezes commented 7 months ago

Thanks. Can you please explain why this is necessary? I assume the problem is that the face is not yet there because at the time you are typing, the syntax highlighting hasn't been applied yet? get-char-property (and also get-pos-property) are not really what we want otherwise since they also take overlays into account. Do you have a simple example starting from emacs -Q which demonstrates the problem?

Here is a recipe to reproduce the issue:

;; Bootstrap straight
(defvar bootstrap-version)

(let ((bootstrap-file
       (expand-file-name "straight/repos/straight.el/bootstrap.el" user-emacs-directory))
      (bootstrap-version 5))
  (unless (file-exists-p bootstrap-file)
    (with-current-buffer
        (url-retrieve-synchronously "https://raw.githubusercontent.com/raxod502/straight.el/develop/install.el"
                                    'silent 'inhibit-cookies)
      (goto-char (point-max))
      (eval-print-last-sexp)))
  (load bootstrap-file nil 'nomessage))

(straight-use-package 'tree-sitter)
(straight-use-package 'tree-sitter-langs)

(require 'cc-mode)
(scratch-buffer)
(insert "int main() { return 0; // comment")
(c-mode)
(tree-sitter-hl-mode)
(tree-sitter-hl--highlight-region (point-min) (point-max))
(message "get-text-property: %s | get-pos-property: %s"
         (get-text-property (point-max) 'face)
         (get-pos-property (point-max) 'face))

True. The issue there was that (window-buffer) was not equal to (current-buffer). That can happen after some commands. We can consider adding (set-buffer (window-buffer)).

minad commented 7 months ago

StrawberryTea @.***> writes:

True. The issue there was that (window-buffer) was not equal to (current-buffer). That can happen after some commands. We can consider adding (set-buffer (window-buffer)).

Thanks! Do I understand correctly that this is a workaround for a bug in the tree-sitter package? We are not talking about Emacs 29 tree-sitter, as I had originally assumed? Is it possible to fix this in the tree-sitter package directly?

LemonBreezes commented 7 months ago

StrawberryTea @.***> writes: True. The issue there was that (window-buffer) was not equal to (current-buffer). That can happen after some commands. We can consider adding (set-buffer (window-buffer)). Thanks! Do I understand correctly that this is a workaround for a bug in the tree-sitter package? We are not talking about Emacs 29 tree-sitter, as I had originally assumed? Is it possible to fix this in the tree-sitter package directly?

Ok, so I was digging and I'm not sure if this is a bug or if it's just the way it is. But basically this is caused by:

(scratch-buffer)
(insert "hello world")
(font-lock-mode -1)
(put-text-property (point-min) (point-max) 'face '(:foreground "red"))
(message "get-text-property: %s, get-pos-property: %s"
         (get-text-property (point-max) 'face)
         (get-pos-property (point-max) 'face))

I get the following message: get-text-property: nil, get-pos-property: (:foreground red)

So the right endpoint is not seen by get-text-property. And I feel that most of the time we are using a Cape backend, we are at the right endpoint because we are typing.

minad commented 7 months ago

Okay, this explains it. The problem is that at the end of a string or a comment get-text-property does not see the face.

// comment|
          ^
          Problem: Face is not seen here

However get-pos-property also takes overlays into account so we cannot use it.

// comment|
          ^ 
          Problem 1: face could be hl-line

"string"|
        ^
        Problem 2: Point is not *inside* the string.

Therefore I am afraid we cannot use get-pos-property.

LemonBreezes commented 7 months ago

Okay, this explains it. The problem is that at the end of a string or a comment get-text-property does not see the face.

// comment|
          ^
          Problem: Face is not seen here

However get-pos-property also takes overlays into account so we cannot use it.

// comment|
          ^ 
          Problem 1: face could be hl-line

"string"|
        ^
        Problem 2: Point is not *inside* the string.

Therefore I am afraid we cannot use get-pos-property.

I ended up just modifying the code in my Doom Emacs Corfu module PR to use a predicate with (nth 4 (syntax-ppss)) along with the faces.

minad commented 7 months ago

I ended up just modifying the code in my Doom Emacs Corfu module PR to use a predicate with (nth 4 (syntax-ppss)) and along with the faces.

Makes sense. If you need a more complex predicate there is nothing wrong with implementing your own Capf wrapper.