bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Andrey Listopadov]

I've upgraded from elixir-mode to elixir-ts-mode and noticed that the
latter uses faces rather inconsistently.

For example, the =font-lock-keyword-face= is used for both keywords and
method calls, as well as for parentheses.  The
=font-lock-function-name-face= is used both for function definitions,
parameter names, and calls.  Some calls use the
=font-lock-keyword-face=, for example the call to `raise'.  The
=font-lock-type-face= is used both for types and =:symbols=.

All of that basically makes Elixir code mostly use 2 colors.
Additionally, it makes impossible selectively disabling highlighting, as
disabling the function call highlighting will disable the function
definition highlighitng an so on.

I believe, Emacs 29 introduced a lot of faces for all kinds of
situations possible in Tree-sitter generated AST, so perhaps the fix is
to use them more semantically, rather than for good looks.



In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.38, cairo version 1.17.8) of 2023-11-08 built on toolbox
Repository revision: bf9cbc2354124a1e9eb3327007468ba384ba2945
Repository branch: master
System Description: Fedora Linux 38 (Container Image)

Configured using:
 'configure --without-compress-install --with-native-compilation=aot
 --with-pgtk --with-mailutils --with-xwidgets
 --prefix=/var/home/alist/.local'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBXML2 MODULES NATIVE_COMP NOTIFY INOTIFY
PDUMPER PGTK PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS TREE_SITTER XIM XWIDGETS GTK3 ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: mu4e:main

Minor modes in effect:
  global-git-commit-mode: t
  magit-auto-revert-mode: t
  mu4e-search-minor-mode: t
  mu4e-update-minor-mode: t
  mu4e-context-minor-mode: t
  electric-pair-mode: t
  savehist-mode: t
  delete-selection-mode: t
  pixel-scroll-precision-mode: t
  global-auto-revert-mode: t
  repeat-mode: t
  vertico-mode: t
  marginalia-mode: t
  corfu-popupinfo-mode: t
  global-corfu-mode: t
  global-region-bindings-mode: t
  recentf-mode: t
  gcmh-mode: t
  override-global-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  context-menu-mode: t
  global-font-lock-mode: t
  blink-cursor-mode: t
  minibuffer-regexp-mode: t
  buffer-read-only: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  overwrite-mode: overwrite-mode-binary

Load-path shadows:
/var/home/alist/.config/emacs/elpa/transient-20231103.2312/transient hides /var/home/alist/.local/share/emacs/30.0.50/lisp/transient
/var/home/alist/.config/emacs/elpa/modus-themes-20231031.716/theme-loaddefs hides /var/home/alist/.local/share/emacs/30.0.50/lisp/theme-loaddefs

Features:
(shadow face-remap emacsbug mu4e mu4e-org ob-fennel fennel-proto-repl
fennel-mode inf-lisp xref magit-bookmark magit-submodule magit-blame
magit-stash magit-reflog magit-bisect magit-push magit-pull magit-fetch
magit-clone magit-remote magit-commit magit-sequence magit-notes
magit-worktree magit-tag magit-merge magit-branch magit-reset
magit-files magit-refs magit-status magit magit-repos magit-apply
magit-wip magit-log which-func imenu magit-diff smerge-mode diff
diff-mode git-commit log-edit pcvs-util add-log magit-core
magit-autorevert magit-margin magit-transient magit-process with-editor
magit-mode transient magit-git magit-base magit-section cursor-sensor
crm project ob-lua ob-shell shell org ob ob-tangle ob-ref ob-lob
ob-table ob-exp org-macro org-src ob-comint org-pcomplete pcomplete
org-list org-footnote org-faces org-entities ob-emacs-lisp ob-core
ob-eval org-cycle org-table org-keys oc org-loaddefs find-func mu4e-main
mu4e-view gnus-art mm-uu mml2015 mm-view mml-smime smime gnutls dig
gnus-sum gnus-group gnus-undo gnus-start gnus-dbus gnus-cloud nnimap
nnmail mail-source utf7 nnoo parse-time iso8601 gnus-spec gnus-int
gnus-range gnus-win gnus nnheader range cal-menu calendar cal-loaddefs
mu4e-headers mu4e-compose mu4e-draft mu4e-actions smtpmail mu4e-search
mu4e-lists mu4e-bookmarks mu4e-mark mu4e-message shr pixel-fill kinsoku
url-file svg dom flow-fill hl-line mu4e-contacts mu4e-update
mu4e-folders mu4e-server mu4e-context mu4e-vars mu4e-helpers mu4e-config
bookmark ido message sendmail yank-media puny dired dired-loaddefs
rfc822 mml mml-sec epa epg rfc6068 epg-config gnus-util time-date
mm-decode mm-bodies mm-encode mail-parse rfc2231 rfc2047 rfc2045 mm-util
ietf-drums mail-prsvr mailabbrev mail-utils gmm-utils mailheader
vertico-directory mule-util highlight-defined advice noutline outline
elec-pair isayt disp-table hideshow hl-todo savehist delsel pixel-scroll
cua-base autorevert filenotify repeat vertico marginalia corfu-popupinfo
cape corfu compat region-bindings recentf tree-widget gcmh init proxy
gsettings s gvariant parsec dash zig-compilation-mode
fennel-compilation-mode clojure-compilation-mode derived compile
text-property-search server ediff ediff-merg ediff-mult ediff-wind
ediff-diff ediff-help ediff-init ediff-util sql-indent sql view
thingatpt comint ansi-osc ring zig-mode reformatter ansi-color ol
org-fold org-fold-core org-compat org-version org-macs format-spec blog
use-package-delight formfeed modus-vivendi-theme modus-themes dbus xml
common-lisp-modes novice cus-edit pp cus-load wid-edit font mode-line
messages defaults edmacro kmacro functions use-package-bind-key bind-key
local-config delight comp comp-cstr warnings icons rx use-package-ensure
cl-extra help-mode use-package-core early-init finder-inf blog-autoloads
cape-autoloads clj-decompiler-autoloads clj-refactor-autoloads
cider-autoloads clojure-mode-autoloads common-lisp-modes-autoloads
consult-autoloads corfu-terminal-autoloads corfu-autoloads
csv-mode-autoloads delight-autoloads dumb-jump-autoloads eat-autoloads
elfeed-autoloads expand-region-autoloads fennel-mode-autoloads
gcmh-autoloads geiser-guile-autoloads geiser-autoloads gnugo-autoloads
ascii-art-to-unicode-autoloads gsettings-autoloads gvariant-autoloads
highlight-defined-autoloads hl-todo-autoloads inflections-autoloads
isayt-autoloads jdecomp-autoloads lsp-java-autoloads
lsp-metals-autoloads dap-mode-autoloads lsp-docker-autoloads
bui-autoloads lsp-treemacs-autoloads lsp-mode-autoloads f-autoloads
lua-mode-autoloads marginalia-autoloads markdown-mode-autoloads
message-view-patch-autoloads magit-autoloads pcase
magit-section-autoloads git-commit-autoloads modus-themes-autoloads
mu4e-alert-autoloads alert-autoloads log4e-autoloads gntp-autoloads
multiple-cursors-autoloads orderless-autoloads org-modern-autoloads
org-tree-slide-autoloads ox-hugo-autoloads
package-lint-flymake-autoloads package-lint-autoloads paredit-autoloads
parsec-autoloads parseedn-autoloads parseclj-autoloads
phi-search-autoloads popon-autoloads popup-autoloads puni-autoloads
easy-mmode racket-mode-autoloads region-bindings-autoloads
request-autoloads scala-mode-autoloads separedit-autoloads
edit-indirect-autoloads sesman-autoloads sly-autoloads spinner-autoloads
sql-indent-autoloads tomelr-autoloads transient-autoloads
treemacs-autoloads cfrs-autoloads posframe-autoloads ht-autoloads
hydra-autoloads lv-autoloads pfuture-autoloads ace-window-autoloads
avy-autoloads s-autoloads dash-autoloads vertico-autoloads
vundo-autoloads with-editor-autoloads info compat-autoloads
xpm-autoloads queue-autoloads yaml-autoloads yaml-mode-autoloads
yasnippet-autoloads zig-mode-autoloads reformatter-autoloads package
browse-url url url-proxy url-privacy url-expand url-methods url-history
url-cookie generate-lisp-file url-domsuf url-util mailcap url-handlers
url-parse auth-source cl-seq eieio eieio-core cl-macs password-cache
json subr-x map byte-opt gv bytecomp byte-compile url-vars cl-loaddefs
cl-lib rmc iso-transl tooltip cconv eldoc paren electric uniquify
ediff-hook vc-hooks lisp-float-type elisp-mode mwheel term/pgtk-win
pgtk-win term/common-win pgtk-dnd touch-screen tool-bar dnd fontset
image regexp-opt fringe tabulated-list replace newcomment text-mode
lisp-mode prog-mode register page tab-bar menu-bar rfn-eshadow isearch
easymenu timer select scroll-bar mouse jit-lock font-lock syntax
font-core term/tty-colors frame minibuffer nadvice seq simple cl-generic
indonesian philippine cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button loaddefs
theme-loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads xwidget-internal dbusbind
inotify dynamic-setting system-font-setting font-render-setting cairo
gtk pgtk lcms2 multi-tty move-toolbar make-network-process
native-compile emacs)

Memory information:
((conses 16 672577 585595) (symbols 48 39874 485)
 (strings 32 183934 60222) (string-bytes 1 5724204) (vectors 16 69084)
 (vector-slots 8 1236810 764574) (floats 8 489 1785)
 (intervals 56 649 178) (buffers 992 11))

-- 
Andrey Listopadov

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Dmitry Gutov]

On 17/11/2023 21:50, Andrey Listopadov wrote:
> 
> I've upgraded from elixir-mode to elixir-ts-mode and noticed that the
> latter uses faces rather inconsistently.
> 
> For example, the =font-lock-keyword-face= is used for both keywords and
> method calls, as well as for parentheses.  The
> =font-lock-function-name-face= is used both for function definitions,
> parameter names, and calls.  Some calls use the
> =font-lock-keyword-face=, for example the call to `raise'.  The
> =font-lock-type-face= is used both for types and =:symbols=.
> 
> All of that basically makes Elixir code mostly use 2 colors.
> Additionally, it makes impossible selectively disabling highlighting, as
> disabling the function call highlighting will disable the function
> definition highlighitng an so on.
> 
> I believe, Emacs 29 introduced a lot of faces for all kinds of
> situations possible in Tree-sitter generated AST, so perhaps the fix is
> to use them more semantically, rather than for good looks.

Thanks for the report. Wilhelm, could you look into this? If you have time.

Speaking of new faces, we added font-lock-function-call-face that can be 
used for function calls, while font-lock-function-name-face stays used 
on definitions.

For parens, if one wanted to highlight them at all, 
font-lock-bracket-face could be used. Though it's probably better to 
leave them to the 4th feature level (see js-ts-mode as an example). 
elixir-ts-mode currently defines 3 levels.

For levels, we've currently settled on the scheme that function calls 
and property lookups go to the 4th level of highlighting, whereas the 
default is 3. This is all tweakable by the individual user, but I think 
it's better to stay consistent between the modes.

Finally, I see that font-lock-function-name-face ends up being used for 
parameters (as Andrey mentioned) and local variables as well. That's not 
great; probably a query that ended up matching too widely. We prefer to 
use font-lock-variable-name-face (for parameter declarations) or 
font-lock-variable-use-face for such identifiers. Though it can be hard 
to reliably distinguish them in a dynamic language, so as far as I'm 
concerned, they could stay non-highlighted (the uses, that is; the 
declarations are usually easy to find using queries).

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Wilhelm Kirschbaum]

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

On Sat, Nov 18, 2023 at 3:36 AM Dmitry Gutov <dmitry@gutov.dev> wrote:

> On 17/11/2023 21:50, Andrey Listopadov wrote:
> >
> > I've upgraded from elixir-mode to elixir-ts-mode and noticed that the
> > latter uses faces rather inconsistently.
> >
> > For example, the =font-lock-keyword-face= is used for both keywords and
> > method calls, as well as for parentheses.  The
> > =font-lock-function-name-face= is used both for function definitions,
> > parameter names, and calls.  Some calls use the
> > =font-lock-keyword-face=, for example the call to `raise'.  The
> > =font-lock-type-face= is used both for types and =:symbols=.
> >
> > All of that basically makes Elixir code mostly use 2 colors.
> > Additionally, it makes impossible selectively disabling highlighting, as
> > disabling the function call highlighting will disable the function
> > definition highlighitng an so on.
> >
> > I believe, Emacs 29 introduced a lot of faces for all kinds of
> > situations possible in Tree-sitter generated AST, so perhaps the fix is
> > to use them more semantically, rather than for good looks.
>
> Thanks for the report. Wilhelm, could you look into this? If you have time.
>
> Speaking of new faces, we added font-lock-function-call-face that can be
> used for function calls, while font-lock-function-name-face stays used
> on definitions.
>
> For parens, if one wanted to highlight them at all,
> font-lock-bracket-face could be used. Though it's probably better to
> leave them to the 4th feature level (see js-ts-mode as an example).
> elixir-ts-mode currently defines 3 levels.
>
> For levels, we've currently settled on the scheme that function calls
> and property lookups go to the 4th level of highlighting, whereas the
> default is 3. This is all tweakable by the individual user, but I think
> it's better to stay consistent between the modes.
>
> Finally, I see that font-lock-function-name-face ends up being used for
> parameters (as Andrey mentioned) and local variables as well. That's not
> great; probably a query that ended up matching too widely. We prefer to
> use font-lock-variable-name-face (for parameter declarations) or
> font-lock-variable-use-face for such identifiers. Though it can be hard
> to reliably distinguish them in a dynamic language, so as far as I'm
> concerned, they could stay non-highlighted (the uses, that is; the
> declarations are usually easy to find using queries).
>

Thanks for the detailed explanation. It's unfortunate timing, because I
published a rework of the faces on MELPA so long and a few people are
trying it out. It is a total rework using the faces a bit better.  I can
submit the patch later today and start the conversation from there?

Wilhelm

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

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Dmitry Gutov]

On 18/11/2023 09:50, Wilhelm Kirschbaum wrote:
> 
> On Sat, Nov 18, 2023 at 3:36 AM Dmitry Gutov <dmitry@gutov.dev 
> <mailto:dmitry@gutov.dev>> wrote:
> 
>     On 17/11/2023 21:50, Andrey Listopadov wrote:
>      >
>      > I've upgraded from elixir-mode to elixir-ts-mode and noticed that the
>      > latter uses faces rather inconsistently.
>      >
>      > For example, the =font-lock-keyword-face= is used for both
>     keywords and
>      > method calls, as well as for parentheses.  The
>      > =font-lock-function-name-face= is used both for function definitions,
>      > parameter names, and calls.  Some calls use the
>      > =font-lock-keyword-face=, for example the call to `raise'.  The
>      > =font-lock-type-face= is used both for types and =:symbols=.
>      >
>      > All of that basically makes Elixir code mostly use 2 colors.
>      > Additionally, it makes impossible selectively disabling
>     highlighting, as
>      > disabling the function call highlighting will disable the function
>      > definition highlighitng an so on.
>      >
>      > I believe, Emacs 29 introduced a lot of faces for all kinds of
>      > situations possible in Tree-sitter generated AST, so perhaps the
>     fix is
>      > to use them more semantically, rather than for good looks.
> 
>     Thanks for the report. Wilhelm, could you look into this? If you
>     have time.
> 
>     Speaking of new faces, we added font-lock-function-call-face that
>     can be
>     used for function calls, while font-lock-function-name-face stays used
>     on definitions.
> 
>     For parens, if one wanted to highlight them at all,
>     font-lock-bracket-face could be used. Though it's probably better to
>     leave them to the 4th feature level (see js-ts-mode as an example).
>     elixir-ts-mode currently defines 3 levels.
> 
>     For levels, we've currently settled on the scheme that function calls
>     and property lookups go to the 4th level of highlighting, whereas the
>     default is 3. This is all tweakable by the individual user, but I think
>     it's better to stay consistent between the modes.
> 
>     Finally, I see that font-lock-function-name-face ends up being used for
>     parameters (as Andrey mentioned) and local variables as well. That's
>     not
>     great; probably a query that ended up matching too widely. We prefer to
>     use font-lock-variable-name-face (for parameter declarations) or
>     font-lock-variable-use-face for such identifiers. Though it can be hard
>     to reliably distinguish them in a dynamic language, so as far as I'm
>     concerned, they could stay non-highlighted (the uses, that is; the
>     declarations are usually easy to find using queries).
> 
> 
> Thanks for the detailed explanation. It's unfortunate timing, because I 
> published a rework of the faces on MELPA so long and a few people are 
> trying it out. It is a total rework using the faces a bit better.  I can 
> submit the patch later today and start the conversation from there?

I guess I expected that if the mode has been added to the core then the 
development is led here too. And modes maintained externally live more 
easily in ELPA. Anyway, we are where we are.

I see that the latest work 
(https://github.com/wkirschbaum/elixir-ts-mode/pull/36) anticipated at 
least some of my comments.

Remainder:

1) font-lock-variable-name-face is intended for declarations and perhaps 
initial assignments (where it's also an implicit declaration), but for 
other variable references it's better to use 
font-lock-variable-use-face. With my test file, I see examples to the 
contrary, even though some attempt to use the latter face had been made 
(inside the 'elixir-function-call' feature's query).

2) Moving highlighting of function calls and variable (and/or property) 
references to the 4th feature level. Looking at your font-lock rules, it 
seems like the elixir-function-call matches is more targeted than the 
elixir-variable one, but still the other built-in modes put both at 4, 
so it would be good for uniformity. Function definitions (and variable 
definitions/declarations, if they're highlighted separately) can remain 
in the 'declaration' or 'definition' feature which is put in a lower 
feature level (e.g. ruby/js/typescript modes have it on level 1).

Something else I would recommend:

3) Removing the '-face' suffix from the face names. This is the best 
practice across most modes, and it's something the manual entry for 
'defface' recommends:

   You should not quote the symbol face, and it should not end in 
‘-face’ (that would be redundant).

Face names inside font-lock.el are a historical exception (and we 
followed it when adding new faces recently), but if you search for 
'defface' inside the Emacs codebase, such names are in the minority.

I haven't done too much testing myself. Perhaps Andrey will take the 
upstream version for a spin. Or we'll wait for the changes to be merged 
here and continue.

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Andrey Listopadov]

Nov 20, 2023 04:50:27 Dmitry Gutov <dmitry@gutov.dev>:

> I guess I expected that if the mode has been added to the core then the 
> development is led here too. And modes maintained externally live more 
> easily in ELPA. Anyway, we are where we are.

I thought the same thing. I wonder if there are other modes that continue 
development on melpa first, and what is the proper way to set up the 
override in my init.el. I'll have to look into that, I guess.

> I haven't done too much testing myself. Perhaps Andrey will take the 
> upstream version for a spin. Or we'll wait for the changes to be merged 
> here and continue.

I have pulled the melpa package, and at level 2 the font locking now 
seems to be correct (I grew to expect syntax highlights to highlight 
important parts of the code, so level 2 seems the most appropriate to me 
personally).  Thanks for the changes!  However, I don't know how exactly 
tree sitter mode should be organizedm so I can't give a proper review on 
the changes - if there's a guide or a manual for that, I'd like to read 
it.

I guess when the patch for the core package will arive, someone who knows 
how font-locking is organized should give a proper review.

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Wilhelm Kirschbaum]

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

Sorry for the late response.

On Mon, Nov 20, 2023 at 12:00 PM Andrey Listopadov <andreyorst@gmail.com>
wrote:

> Nov 20, 2023 04:50:27 Dmitry Gutov <dmitry@gutov.dev>:
>
> > I guess I expected that if the mode has been added to the core then the
> > development is led here too. And modes maintained externally live more
> > easily in ELPA. Anyway, we are where we are.
>

I agree, but not sure how to deal with the users already using it on emacs
29.1 with the MELPA package.
When I wanted to submit the package, I remember there being an email saying
we should not add packages to ELPA yet, so now I am stuck with the github
repository and MELPA.
Once 30 drops it should be easier to redirect conversations here, not sure
how this works.  The packages are slightly different and I regret my
earlier decisions.


>
> I thought the same thing. I wonder if there are other modes that continue
> development on melpa first, and what is the proper way to set up the
> override in my init.el. I'll have to look into that, I guess.
>

There is no need to set an override if you use a conventional method of
loading the package as far as I know.


>
> > I haven't done too much testing myself. Perhaps Andrey will take the
> > upstream version for a spin. Or we'll wait for the changes to be merged
> > here and continue.
>
> I have pulled the melpa package, and at level 2 the font locking now
> seems to be correct (I grew to expect syntax highlights to highlight
> important parts of the code, so level 2 seems the most appropriate to me
> personally).  Thanks for the changes!  However, I don't know how exactly
> tree sitter mode should be organizedm so I can't give a proper review on
> the changes - if there's a guide or a manual for that, I'd like to read
> it.
>

Thanks for checking.  Any feedback will be very helpful, I will prep the
patch in the next day or two.

Wilhelm

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

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Dmitry Gutov]

On 24/11/2023 20:56, Wilhelm Kirschbaum wrote:
> On Mon, Nov 20, 2023 at 12:00 PM Andrey Listopadov <andreyorst@gmail.com 
> <mailto:andreyorst@gmail.com>> wrote:
> 
>     Nov 20, 2023 04:50:27 Dmitry Gutov <dmitry@gutov.dev
>     <mailto:dmitry@gutov.dev>>:
> 
>      > I guess I expected that if the mode has been added to the core
>     then the
>      > development is led here too. And modes maintained externally live
>     more
>      > easily in ELPA. Anyway, we are where we are.
> 
> 
> I agree, but not sure how to deal with the users already using it on 
> emacs 29.1 with the MELPA package.
> When I wanted to submit the package, I remember there being an email 
> saying we should not add packages to ELPA yet, so now I am stuck with 
> the github repository and MELPA.
> Once 30 drops it should be easier to redirect conversations here, not 
> sure how this works.  The packages are slightly different and I regret 
> my earlier decisions.

OTOH, once Emacs 30 drops, there will be no good way to undo the 
inclusion, if you became so inclined.

Which might be an option currently, since elixir-ts-mode hasn't been in 
an Emacs release yet.

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Wilhelm Kirschbaum]

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

On Fri, Nov 24, 2023 at 9:06 PM Dmitry Gutov <dmitry@gutov.dev> wrote:

> On 24/11/2023 20:56, Wilhelm Kirschbaum wrote:
> > On Mon, Nov 20, 2023 at 12:00 PM Andrey Listopadov <andreyorst@gmail.com
> > <mailto:andreyorst@gmail.com>> wrote:
> >
> >     Nov 20, 2023 04:50:27 Dmitry Gutov <dmitry@gutov.dev
> >     <mailto:dmitry@gutov.dev>>:
> >
> >      > I guess I expected that if the mode has been added to the core
> >     then the
> >      > development is led here too. And modes maintained externally live
> >     more
> >      > easily in ELPA. Anyway, we are where we are.
> >
> >
> > I agree, but not sure how to deal with the users already using it on
> > emacs 29.1 with the MELPA package.
> > When I wanted to submit the package, I remember there being an email
> > saying we should not add packages to ELPA yet, so now I am stuck with
> > the github repository and MELPA.
> > Once 30 drops it should be easier to redirect conversations here, not
> > sure how this works.  The packages are slightly different and I regret
> > my earlier decisions.
>
> OTOH, once Emacs 30 drops, there will be no good way to undo the
> inclusion, if you became so inclined.
>

> Which might be an option currently, since elixir-ts-mode hasn't been in
> an Emacs release yet.
>

I am not sure what you mean? Which option? I meant I regret pushing it to
MELPA first.

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

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Dmitry Gutov]

On 24/11/2023 21:23, Wilhelm Kirschbaum wrote:
> 
> On Fri, Nov 24, 2023 at 9:06 PM Dmitry Gutov <dmitry@gutov.dev 
> <mailto:dmitry@gutov.dev>> wrote:
> 
>     On 24/11/2023 20:56, Wilhelm Kirschbaum wrote:
>      > On Mon, Nov 20, 2023 at 12:00 PM Andrey Listopadov
>     <andreyorst@gmail.com <mailto:andreyorst@gmail.com>
>      > <mailto:andreyorst@gmail.com <mailto:andreyorst@gmail.com>>> wrote:
>      >
>      >     Nov 20, 2023 04:50:27 Dmitry Gutov <dmitry@gutov.dev
>     <mailto:dmitry@gutov.dev>
>      >     <mailto:dmitry@gutov.dev <mailto:dmitry@gutov.dev>>>:
>      >
>      >      > I guess I expected that if the mode has been added to the core
>      >     then the
>      >      > development is led here too. And modes maintained
>     externally live
>      >     more
>      >      > easily in ELPA. Anyway, we are where we are.
>      >
>      >
>      > I agree, but not sure how to deal with the users already using it on
>      > emacs 29.1 with the MELPA package.
>      > When I wanted to submit the package, I remember there being an email
>      > saying we should not add packages to ELPA yet, so now I am stuck
>     with
>      > the github repository and MELPA.
>      > Once 30 drops it should be easier to redirect conversations here,
>     not
>      > sure how this works.  The packages are slightly different and I
>     regret
>      > my earlier decisions.
> 
>     OTOH, once Emacs 30 drops, there will be no good way to undo the
>     inclusion, if you became so inclined.
> 
> 
>     Which might be an option currently, since elixir-ts-mode hasn't been in
>     an Emacs release yet.
> 
> 
> I am not sure what you mean? Which option? I meant I regret pushing it 
> to MELPA first.

Ah. If you want it to stay in Emacs core (and not in ELPA), then it 
should be easy enough to pull it from MELPA at any later point. Simply 
asking should suffice.

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Wilhelm Kirschbaum]

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

On Mon, Nov 20, 2023 at 3:50 AM Dmitry Gutov <dmitry@gutov.dev> wrote:

> On 18/11/2023 09:50, Wilhelm Kirschbaum wrote:
> >
> > On Sat, Nov 18, 2023 at 3:36 AM Dmitry Gutov <dmitry@gutov.dev
> > <mailto:dmitry@gutov.dev>> wrote:
> >
> >     On 17/11/2023 21:50, Andrey Listopadov wrote:
> >      >
> >      > I've upgraded from elixir-mode to elixir-ts-mode and noticed that
> the
> >      > latter uses faces rather inconsistently.
> >      >
> >      > For example, the =font-lock-keyword-face= is used for both
> >     keywords and
> >      > method calls, as well as for parentheses.  The
> >      > =font-lock-function-name-face= is used both for function
> definitions,
> >      > parameter names, and calls.  Some calls use the
> >      > =font-lock-keyword-face=, for example the call to `raise'.  The
> >      > =font-lock-type-face= is used both for types and =:symbols=.
> >      >
> >      > All of that basically makes Elixir code mostly use 2 colors.
> >      > Additionally, it makes impossible selectively disabling
> >     highlighting, as
> >      > disabling the function call highlighting will disable the function
> >      > definition highlighitng an so on.
> >      >
> >      > I believe, Emacs 29 introduced a lot of faces for all kinds of
> >      > situations possible in Tree-sitter generated AST, so perhaps the
> >     fix is
> >      > to use them more semantically, rather than for good looks.
> >
> >     Thanks for the report. Wilhelm, could you look into this? If you
> >     have time.
> >
> >     Speaking of new faces, we added font-lock-function-call-face that
> >     can be
> >     used for function calls, while font-lock-function-name-face stays
> used
> >     on definitions.
> >
> >     For parens, if one wanted to highlight them at all,
> >     font-lock-bracket-face could be used. Though it's probably better to
> >     leave them to the 4th feature level (see js-ts-mode as an example).
> >     elixir-ts-mode currently defines 3 levels.
> >
> >     For levels, we've currently settled on the scheme that function calls
> >     and property lookups go to the 4th level of highlighting, whereas the
> >     default is 3. This is all tweakable by the individual user, but I
> think
> >     it's better to stay consistent between the modes.
> >
> >     Finally, I see that font-lock-function-name-face ends up being used
> for
> >     parameters (as Andrey mentioned) and local variables as well. That's
> >     not
> >     great; probably a query that ended up matching too widely. We prefer
> to
> >     use font-lock-variable-name-face (for parameter declarations) or
> >     font-lock-variable-use-face for such identifiers. Though it can be
> hard
> >     to reliably distinguish them in a dynamic language, so as far as I'm
> >     concerned, they could stay non-highlighted (the uses, that is; the
> >     declarations are usually easy to find using queries).
> >
> >
> > Thanks for the detailed explanation. It's unfortunate timing, because I
> > published a rework of the faces on MELPA so long and a few people are
> > trying it out. It is a total rework using the faces a bit better.  I can
> > submit the patch later today and start the conversation from there?
>
> I guess I expected that if the mode has been added to the core then the
> development is led here too. And modes maintained externally live more
> easily in ELPA. Anyway, we are where we are.
>
> I see that the latest work
> (https://github.com/wkirschbaum/elixir-ts-mode/pull/36) anticipated at
> least some of my comments.
>
> Remainder:
>
> 1) font-lock-variable-name-face is intended for declarations and perhaps
> initial assignments (where it's also an implicit declaration), but for
> other variable references it's better to use
> font-lock-variable-use-face. With my test file, I see examples to the
> contrary, even though some attempt to use the latter face had been made
> (inside the 'elixir-function-call' feature's query).
>

Thanks, this has been fixed.


>
> 2) Moving highlighting of function calls and variable (and/or property)
> references to the 4th feature level. Looking at your font-lock rules, it
> seems like the elixir-function-call matches is more targeted than the
> elixir-variable one, but still the other built-in modes put both at 4,
> so it would be good for uniformity. Function definitions (and variable
> definitions/declarations, if they're highlighted separately) can remain
> in the 'declaration' or 'definition' feature which is put in a lower
> feature level (e.g. ruby/js/typescript modes have it on level 1).
>

Level 4 is strange to me, because the default is 3.  I read the docs and
tried to
follow it with the new changes, but was hesitant to remove much from the
highlighting as not many people might discover there is a 4th level.  Then
again, if there is a query it
is pretty easy just to communicate this.


>
> Something else I would recommend:
>
> 3) Removing the '-face' suffix from the face names. This is the best
> practice across most modes, and it's something the manual entry for
> 'defface' recommends:
>
>    You should not quote the symbol face, and it should not end in
> ‘-face’ (that would be redundant).
>
> Face names inside font-lock.el are a historical exception (and we
> followed it when adding new faces recently), but if you search for
> 'defface' inside the Emacs codebase, such names are in the minority.
>

Okay thanks, I had a look and it makes sense.  I see a new mode with the
same -face suffixes.
The defface docs does not mention this, so might be a good idea to add it
there?
I am not entirely sure what "you should not quote the symbol face" means
wrt to the changes, because
it does not look like I am quoting it.


>
> I haven't done too much testing myself. Perhaps Andrey will take the
> upstream version for a spin. Or we'll wait for the changes to be merged
> here and continue.
>

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

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Dmitry Gutov]

On 24/11/2023 21:47, Wilhelm Kirschbaum wrote:

>     I see that the latest work
>     (https://github.com/wkirschbaum/elixir-ts-mode/pull/36
>     <https://github.com/wkirschbaum/elixir-ts-mode/pull/36>) anticipated at
>     least some of my comments.
> 
>     Remainder:
> 
>     1) font-lock-variable-name-face is intended for declarations and
>     perhaps
>     initial assignments (where it's also an implicit declaration), but for
>     other variable references it's better to use
>     font-lock-variable-use-face. With my test file, I see examples to the
>     contrary, even though some attempt to use the latter face had been made
>     (inside the 'elixir-function-call' feature's query).
> 
> 
> Thanks, this has been fixed.

Sorry, is there a specific commit in the upstream repo I could look at?

>     2) Moving highlighting of function calls and variable (and/or property)
>     references to the 4th feature level. Looking at your font-lock
>     rules, it
>     seems like the elixir-function-call matches is more targeted than the
>     elixir-variable one, but still the other built-in modes put both at 4,
>     so it would be good for uniformity. Function definitions (and variable
>     definitions/declarations, if they're highlighted separately) can remain
>     in the 'declaration' or 'definition' feature which is put in a lower
>     feature level (e.g. ruby/js/typescript modes have it on level 1).
> 
> 
> Level 4 is strange to me, because the default is 3.  I read the docs and 
> tried to
> follow it with the new changes, but was hesitant to remove much from the
> highlighting as not many people might discover there is a 4th level.  
> Then again, if there is a query it
> is pretty easy just to communicate this.

The idea was to balance the new look between the "old" major modes and 
the newer, shinier ones. So that the overall style still somewhat 
appeals to the existing audience, just with added features and more 
precision. Here's a Reddit recent thread about the same sentiment: 
https://www.reddit.com/r/emacs/comments/18152qo/overcolorization_everything_is_purple/ 
It discusses a post written by Andrey, BTW.

One could basically say that a function call and a properly lookup are 
easy to distinguish from glancing at the text, there's not much need to 
highlight them. As opposed to e.g. implicit variable declaration or 
function declaration.

And here's another aspect: the default built-in theme doesn't 
distinguish many of the faces (and the same is true for many other 
built-in themes). E.g. it doesn't distinguish variable-name-face from 
variable-use-face or function-name-face from function-call-face.

So if the -use- and -call- are used freely, in the default setup they 
will get muddled with the function and variable declaration.

OTOH, if the user installs a theme which has this more advanced support 
for the new faces (or customize the faces manually to be distinct), they 
might as well set treesit-font-lock-level to 4, that's very little extra 
effort.

>     Something else I would recommend:
> 
>     3) Removing the '-face' suffix from the face names. This is the best
>     practice across most modes, and it's something the manual entry for
>     'defface' recommends:
> 
>         You should not quote the symbol face, and it should not end in
>     ‘-face’ (that would be redundant).
> 
>     Face names inside font-lock.el are a historical exception (and we
>     followed it when adding new faces recently), but if you search for
>     'defface' inside the Emacs codebase, such names are in the minority.
> 
> 
> Okay thanks, I had a look and it makes sense.  I see a new mode with the 
> same -face suffixes.
> The defface docs does not mention this, so might be a good idea to add 
> it there?

Probably. I rarely read the manual myself, and this is useful information.

> I am not entirely sure what "you should not quote the symbol face" means 
> wrt to the changes, because
> it does not look like I am quoting it.

Indeed you're not, I only put that in the quote so that the sentence is 
not cut off from the beginning. Sorry if that was confusing.

>     I haven't done too much testing myself. Perhaps Andrey will take the
>     upstream version for a spin. Or we'll wait for the changes to be merged
>     here and continue.
> 

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Andrey Listopadov]

Dmitry Gutov <dmitry@gutov.dev> writes:

> The idea was to balance the new look between the "old" major modes and
> the newer, shinier ones. So that the overall style still somewhat
> appeals to the existing audience, just with added features and more
> precision. Here's a Reddit recent thread about the same sentiment:
> https://www.reddit.com/r/emacs/comments/18152qo/overcolorization_everything_is_purple/
> It discusses a post written by Andrey, BTW.

Yes, I published this post after submitting the bug in order to raise
awareness among the community.

> One could basically say that a function call and a properly lookup are
> easy to distinguish from glancing at the text, there's not much need
> to highlight them. As opposed to e.g. implicit variable declaration or
> function declaration.

Yeah, as I said in my post, highlighting important parts of the code,
like macro calls, or dynamic/global variables tells you that you're
looking at something more intricate, that is otherwise syntactically
indistinguishable from regular code.

> And here's another aspect: the default built-in theme doesn't
> distinguish many of the faces (and the same is true for many other
> built-in themes). E.g. it doesn't distinguish variable-name-face from
> variable-use-face or function-name-face from function-call-face.

I'm wondering if font-lock.el needs a bit more generic faces, as
packages often define their own faces, that aren't supported by themes
in any way.  Again, the example with elixir-mode isn't to bash the
developers, but before 2019 elixir-mode (not elixir-ts-mode) defined a
few faces with explicit colors.  Here's a commit that fixed that
https://github.com/elixir-editors/emacs-elixir/commit/f101c676cc9485aa22ec088a71d8afc72cda3d58
but before it, `elixir-atom-face' and `elixir-attribute-face' were
`RoyalBlue4' and `MediumPurple4' no matter what theme you were using.
IIRC the CIDER package also defines some faces like that, so it's
somewhat common.

I can't come up with missing faces, and most modes I use define extra
faces in terms of inheritance to the inbuilt faces, but maybe
font-lock-symbol-face is worth including, as some languages may want to
distinguish these like elixir does right now with `elixir-ts-atom-face'.

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Dmitry Gutov]

On 25/11/2023 10:33, Andrey Listopadov wrote:

>> And here's another aspect: the default built-in theme doesn't
>> distinguish many of the faces (and the same is true for many other
>> built-in themes). E.g. it doesn't distinguish variable-name-face from
>> variable-use-face or function-name-face from function-call-face.
> 
> I'm wondering if font-lock.el needs a bit more generic faces, as
> packages often define their own faces, that aren't supported by themes
> in any way.  Again, the example with elixir-mode isn't to bash the
> developers, but before 2019 elixir-mode (not elixir-ts-mode) defined a
> few faces with explicit colors.  Here's a commit that fixed that
> https://github.com/elixir-editors/emacs-elixir/commit/f101c676cc9485aa22ec088a71d8afc72cda3d58
> but before it, `elixir-atom-face' and `elixir-attribute-face' were
> `RoyalBlue4' and `MediumPurple4' no matter what theme you were using.
> IIRC the CIDER package also defines some faces like that, so it's
> somewhat common.

As long as the faces are for unusual contexts and have some fallbacks 
(or preferably inherit from some of the core ones), that's fair practice.

> I can't come up with missing faces, and most modes I use define extra
> faces in terms of inheritance to the inbuilt faces,

Right.

> but maybe
> font-lock-symbol-face is worth including, as some languages may want to
> distinguish these like elixir does right now with `elixir-ts-atom-face'.

I agree we could add more. E.g. a face like that could automatically be 
used for "keywords" in Elisp (and Clojure, and other Lisps) and 
"symbols" in Elixir in Ruby.

What makes me pause is naming: the terminology is a mess here across 
languages. "symbols" usually mean something else in Emacs (and in Lisp 
languages in general), whereas "keywords" mean something else across 
most other languages. Using the name font-lock-symbol-face is bound to 
cause confusion at least across Lisp programmers. Luckily, 
'font-lock-keyword-face' is already taken, so we don't have to consider 
this alternative (which would puzzle the rest of the programming world).

The docstring of 'font-lock-constant-face' says "Face name to use for 
constant and label names", but a name 'font-lock-label-name' sounds 
pretty bland... OTOH, there are labels in C, but nothing with that 
particular name in Elixir, Ruby or Lisp (aside from one macro, I suppose).

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Wilhelm Kirschbaum]

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


Dmitry Gutov <dmitry@gutov.dev> writes:

> On 25/11/2023 10:33, Andrey Listopadov wrote:
>
>>> And here's another aspect: the default built-in theme doesn't
>>> distinguish many of the faces (and the same is true for many 
>>> other
>>> built-in themes). E.g. it doesn't distinguish 
>>> variable-name-face from
>>> variable-use-face or function-name-face from 
>>> function-call-face.
>> I'm wondering if font-lock.el needs a bit more generic faces, 
>> as
>> packages often define their own faces, that aren't supported by 
>> themes
>> in any way.  Again, the example with elixir-mode isn't to bash 
>> the
>> developers, but before 2019 elixir-mode (not elixir-ts-mode) 
>> defined a
>> few faces with explicit colors.  Here's a commit that fixed 
>> that
>> https://github.com/elixir-editors/emacs-elixir/commit/f101c676cc9485aa22ec088a71d8afc72cda3d58
>> but before it, `elixir-atom-face' and `elixir-attribute-face' 
>> were
>> `RoyalBlue4' and `MediumPurple4' no matter what theme you were 
>> using.
>> IIRC the CIDER package also defines some faces like that, so 
>> it's
>> somewhat common.
>
> As long as the faces are for unusual contexts and have some 
> fallbacks
> (or preferably inherit from some of the core ones), that's fair
> practice.
>
>> I can't come up with missing faces, and most modes I use define 
>> extra
>> faces in terms of inheritance to the inbuilt faces,
>
> Right.
>
>> but maybe
>> font-lock-symbol-face is worth including, as some languages may 
>> want to
>> distinguish these like elixir does right now with 
>> `elixir-ts-atom-face'.
>
> I agree we could add more. E.g. a face like that could 
> automatically
> be used for "keywords" in Elisp (and Clojure, and other Lisps) 
> and
> "symbols" in Elixir in Ruby.
>
> What makes me pause is naming: the terminology is a mess here 
> across
> languages. "symbols" usually mean something else in Emacs (and 
> in Lisp
> languages in general), whereas "keywords" mean something else 
> across
> most other languages. Using the name font-lock-symbol-face is 
> bound to
> cause confusion at least across Lisp programmers. Luckily,
> 'font-lock-keyword-face' is already taken, so we don't have to
> consider this alternative (which would puzzle the rest of the
> programming world).
>
> The docstring of 'font-lock-constant-face' says "Face name to 
> use for
> constant and label names", but a name 'font-lock-label-name' 
> sounds
> pretty bland... OTOH, there are labels in C, but nothing with 
> that
> particular name in Elixir, Ruby or Lisp (aside from one macro, I
> suppose).


Here is a patch to address numerous issues flagged on Elixir 
slack,
Github and in this thread.  It will not be perfect, but since the
changes are pretty large I want to get this in and then we can 
pick on
specific issues afterwards if that makes sense?

I am making the assumption that it is okay to rename custom faces 
as
elixir-ts-mode is only for 30.

One thing I tried to get right is to ensure that each level works
relatively well, which means a bit more brute forcing queries.  I 
have
not seen a major performance issue on massic Elixir files, so 
think its
fine.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Various-improvements-to-font-lock-settings-for-elixi.patch --]
[-- Type: text/x-patch, Size: 18119 bytes --]

From ef3e6b3106cabdcaa000503a9b2c227110be36f3 Mon Sep 17 00:00:00 2001
From: Wilhelm H Kirschbaum <wkirschbaum@gmail.com>
Date: Wed, 15 Nov 2023 20:41:08 +0200
Subject: [PATCH] Various improvements to font-lock-settings for elixir-ts-mode

Changes and made from conversations from the Elixir slack channel,
the github issue
https://github.com/wkirschbaum/elixir-ts-mode/issues/35 and bug#67246.

* lisp/progmodes/elixir-ts-mode.el
(elixir-ts--font-lock-settings): Update features.
(elixir-ts-mode): Update treesit-font-lock-feature-list.
(elixir-ts-font-comment-doc-identifier-face): Rename to
elixir-ts-comment-doc-identifier.
(elixir-ts-font-comment-doc-attribute-face): Rename to
elixir-ts-comment-doc-attribute.
(elixir-ts-font-sigil-name-face): Rename to elixir-ts-sigil-name.
(elixir-ts-atom-key-face)
(elixir-ts-keyword-key-face)
(elixir-ts-attribute-face): Add new custom face.
---
 lisp/progmodes/elixir-ts-mode.el | 323 ++++++++++++++++++-------------
 1 file changed, 191 insertions(+), 132 deletions(-)

diff --git a/lisp/progmodes/elixir-ts-mode.el b/lisp/progmodes/elixir-ts-mode.el
index c687ed9d06b..62429308d96 100644
--- a/lisp/progmodes/elixir-ts-mode.el
+++ b/lisp/progmodes/elixir-ts-mode.el
@@ -86,17 +86,35 @@ elixir-ts-mode-hook
   :group 'elixir-ts
   :version "30.1")
 
-(defface elixir-ts-font-comment-doc-identifier-face
+(defface elixir-ts-comment-doc-identifier
   '((t (:inherit font-lock-doc-face)))
-  "Face used for @comment.doc tags in Elixir files.")
+  "Face used for doc identifiers in Elixir files."
+  :group 'elixir-ts)
 
-(defface elixir-ts-font-comment-doc-attribute-face
+(defface elixir-ts-comment-doc-attribute
   '((t (:inherit font-lock-doc-face)))
-  "Face used for @comment.doc.__attribute__ tags in Elixir files.")
+  "Face used for doc attributes in Elixir files."
+  :group 'elixir-ts)
 
-(defface elixir-ts-font-sigil-name-face
+(defface elixir-ts-sigil-name
   '((t (:inherit font-lock-string-face)))
-  "Face used for @__name__ tags in Elixir files.")
+  "Face used for sigils in Elixir files."
+  :group 'elixir-ts)
+
+(defface elixir-ts-atom
+  '((t (:inherit font-lock-constant-face)))
+  "Face used for atoms in Elixir files."
+  :group 'elixir-ts)
+
+(defface elixir-ts-keyword-key
+  '((t (:inherit elixir-ts-atom)))
+  "Face used for keyword keys in Elixir files."
+  :group 'elixir-ts)
+
+(defface elixir-ts-attribute
+  '((t (:inherit font-lock-preprocessor-face)))
+  "Face used for attributes in Elixir files."
+  :group 'elixir-ts)
 
 (defconst elixir-ts--sexp-regexp
   (rx bol
@@ -114,7 +132,10 @@ elixir-ts--definition-keywords
     "defoverridable" "defp" "defprotocol" "defstruct"))
 
 (defconst elixir-ts--definition-keywords-re
-  (concat "^" (regexp-opt elixir-ts--definition-keywords) "$"))
+  (concat "^" (regexp-opt
+               (append elixir-ts--definition-keywords
+                       elixir-ts--test-definition-keywords))
+          "$"))
 
 (defconst elixir-ts--kernel-keywords
   '("alias" "case" "cond" "else" "for" "if" "import" "quote"
@@ -334,56 +355,73 @@ elixir-ts--indent-rules
                   (treesit-node-start
                    (treesit-node-parent
                     (treesit-node-at (point) 'elixir))))
-                  0)))))
+                0)))))
 
 (defvar elixir-ts--font-lock-settings
   (treesit-font-lock-rules
    :language 'elixir
-   :feature 'elixir-comment
-   '((comment) @font-lock-comment-face)
-
-   :language 'elixir
-   :feature 'elixir-string
-   :override t
-   '([(string) (charlist)] @font-lock-string-face)
-
+   :feature 'elixir-function-name
+   `((call target: (identifier) @target-identifier
+           (arguments (identifier) @font-lock-function-name-face)
+           (:match ,elixir-ts--definition-keywords-re @target-identifier))
+     (call target: (identifier) @target-identifier
+           (arguments
+            (call target: (identifier) @font-lock-function-name-face))
+           (:match ,elixir-ts--definition-keywords-re @target-identifier))
+     (call target: (identifier) @target-identifier
+           (arguments
+            (binary_operator
+             left: (call target: (identifier) @font-lock-function-name-face)))
+           (:match ,elixir-ts--definition-keywords-re @target-identifier))
+     (call target: (identifier) @target-identifier
+           (arguments (identifier) @font-lock-function-name-face)
+           (do_block)
+           (:match ,elixir-ts--definition-keywords-re @target-identifier))
+     (call target: (identifier) @target-identifier
+           (arguments
+            (call target: (identifier) @font-lock-function-name-face))
+           (do_block)
+           (:match ,elixir-ts--definition-keywords-re @target-identifier))
+     (call target: (identifier) @target-identifier
+           (arguments
+            (binary_operator
+             left: (call target: (identifier) @font-lock-function-name-face)))
+           (do_block)
+           (:match ,elixir-ts--definition-keywords-re @target-identifier))
+     (unary_operator
+      operator: "@"
+      (call (arguments
+             (binary_operator
+              left: (call target: (identifier) @font-lock-function-name-face))))))
+
+   ;; A function definition like "def _foo" is valid, but we should
+   ;; not apply the comment-face unless its a non-function identifier, so
+   ;; the comment matches has to be after the function matches.
    :language 'elixir
-   :feature 'elixir-string-interpolation
-   :override t
-   '((string
-      [
-       quoted_end: _ @font-lock-string-face
-       quoted_start: _ @font-lock-string-face
-       (quoted_content) @font-lock-string-face
-       (interpolation
-        "#{" @font-lock-regexp-grouping-backslash "}"
-        @font-lock-regexp-grouping-backslash)
-       ])
-     (charlist
-      [
-       quoted_end: _ @font-lock-string-face
-       quoted_start: _ @font-lock-string-face
-       (quoted_content) @font-lock-string-face
-       (interpolation
-        "#{" @font-lock-regexp-grouping-backslash "}"
-        @font-lock-regexp-grouping-backslash)
-       ]))
+   :feature 'elixir-comment
+   '((comment) @font-lock-comment-face
+     ((identifier) @font-lock-comment-face
+      (:match "^_[a-z]\\|^_$" @font-lock-comment-face)))
 
    :language 'elixir
-   :feature 'elixir-keyword
-   `(,elixir-ts--reserved-keywords-vector
-     @font-lock-keyword-face
-     (binary_operator
-      operator: _ @font-lock-keyword-face
-      (:match ,elixir-ts--reserved-keywords-re @font-lock-keyword-face)))
+   :feature 'elixir-variable
+   `((call target: (identifier)
+           (arguments
+            (binary_operator
+             (call target: (identifier)
+                   (arguments ((identifier) @font-lock-variable-use-face))))))
+     (call target: (identifier)
+           (arguments
+            (call target: (identifier)
+                  (arguments ((identifier)) @font-lock-variable-use-face))))
+     (dot left: (identifier) @font-lock-variable-use-face operator: "." ))
 
    :language 'elixir
    :feature 'elixir-doc
-   :override t
    `((unary_operator
-      operator: "@" @elixir-ts-font-comment-doc-attribute-face
+      operator: "@" @elixir-ts-comment-doc-attribute
       operand: (call
-                target: (identifier) @elixir-ts-font-comment-doc-identifier-face
+                target: (identifier) @elixir-ts-comment-doc-identifier
                 ;; Arguments can be optional, so adding another
                 ;; entry without arguments.
                 ;; If we don't handle then we don't apply font
@@ -395,109 +433,128 @@ elixir-ts--font-lock-settings
                   (charlist) @font-lock-doc-face
                   (sigil) @font-lock-doc-face
                   (boolean) @font-lock-doc-face
+                  (keywords) @font-lock-doc-face
                   ]))
       (:match ,elixir-ts--doc-keywords-re
-              @elixir-ts-font-comment-doc-identifier-face))
+              @elixir-ts-comment-doc-identifier))
      (unary_operator
-      operator: "@" @elixir-ts-font-comment-doc-attribute-face
+      operator: "@" @elixir-ts-comment-doc-attribute
       operand: (call
-                target: (identifier) @elixir-ts-font-comment-doc-identifier-face)
+                target: (identifier) @elixir-ts-comment-doc-identifier)
       (:match ,elixir-ts--doc-keywords-re
-              @elixir-ts-font-comment-doc-identifier-face)))
+              @elixir-ts-comment-doc-identifier)))
 
    :language 'elixir
-   :feature 'elixir-unary-operator
-   `((unary_operator operator: "@" @font-lock-preprocessor-face
-                     operand: [
-                               (identifier)  @font-lock-preprocessor-face
-                               (call target: (identifier)
-                                     @font-lock-preprocessor-face)
-                               (boolean)  @font-lock-preprocessor-face
-                               (nil)  @font-lock-preprocessor-face
-                               ])
+   :feature 'elixir-string
+   '((interpolation
+      "#{" @font-lock-escape-face
+      "}" @font-lock-escape-face)
+     (string (quoted_content) @font-lock-string-face)
+     (quoted_keyword (quoted_content) @font-lock-string-face)
+     (charlist (quoted_content) @font-lock-string-face)
+     ["\"" "'" "\"\"\""] @font-lock-string-face)
 
-     (unary_operator operator: "&") @font-lock-function-name-face
-     (operator_identifier) @font-lock-operator-face)
+   :language 'elixir
+   :feature 'elixir-sigil
+   `((sigil
+      (sigil_name) @elixir-ts-sigil-name
+      (quoted_content) @font-lock-string-face
+      ;; HEEx and Surface templates will handled by
+      ;; heex-ts-mode if its available.
+      (:match "^[^HF]$" @elixir-ts-sigil-name))
+     @font-lock-string-face
+     (sigil
+      (sigil_name) @font-lock-regexp-face
+      (:match "^[rR]$" @font-lock-regexp-face))
+     @font-lock-regexp-face
+     (sigil
+      "~" @font-lock-string-face
+      (sigil_name) @font-lock-string-face
+      quoted_start: _ @font-lock-string-face
+      quoted_end: _ @font-lock-string-face))
 
    :language 'elixir
    :feature 'elixir-operator
-   '((binary_operator operator: _ @font-lock-operator-face)
-     (dot operator: _ @font-lock-operator-face)
-     (stab_clause operator: _ @font-lock-operator-face)
-
-     [(boolean) (nil)] @font-lock-constant-face
-     [(integer) (float)] @font-lock-number-face
-     (alias) @font-lock-type-face
-     (call target: (dot left: (atom) @font-lock-type-face))
-     (char) @font-lock-constant-face
-     [(atom) (quoted_atom)] @font-lock-type-face
-     [(keyword) (quoted_keyword)] @font-lock-builtin-face)
+   `(["!"] @font-lock-negation-char-face
+     ["%"] @font-lock-bracket-face
+     ["," ";"] @font-lock-operator-face
+     ["(" ")" "[" "]" "{" "}" "<<" ">>"] @font-lock-bracket-face)
 
    :language 'elixir
-   :feature 'elixir-call
-   `((call
-      target: (identifier) @font-lock-keyword-face
-      (:match ,elixir-ts--definition-keywords-re @font-lock-keyword-face))
-     (call
-      target: (identifier) @font-lock-keyword-face
-      (:match ,elixir-ts--kernel-keywords-re @font-lock-keyword-face))
-     (call
-      target: [(identifier) @font-lock-function-name-face
-               (dot right: (identifier) @font-lock-keyword-face)])
+   :feature 'elixir-data-type
+   '([(atom) (alias)] @font-lock-type-face
+     (keywords (pair key: (keyword) @elixir-ts-keyword-key))
+     [(keyword) (quoted_keyword)] @elixir-ts-atom
+     [(boolean) (nil)] @elixir-ts-atom
+     (unary_operator operator: "@" @elixir-ts-attribute
+                     operand: [
+                               (identifier) @elixir-ts-attribute
+                               (call target: (identifier)
+                                     @elixir-ts-attribute)
+                               (boolean) @elixir-ts-attribute
+                               (nil) @elixir-ts-attribute
+                               ])
+     (operator_identifier) @font-lock-operator-face)
+
+   :language 'elixir
+   :feature 'elixir-keyword
+   `(,elixir-ts--reserved-keywords-vector
+     @font-lock-keyword-face
+     (binary_operator
+      operator: _ @font-lock-keyword-face
+      (:match ,elixir-ts--reserved-keywords-re @font-lock-keyword-face))
+     (binary_operator operator: _ @font-lock-operator-face)
      (call
       target: (identifier) @font-lock-keyword-face
-      (arguments
-       [
-        (identifier) @font-lock-keyword-face
-        (binary_operator
-         left: (identifier) @font-lock-keyword-face
-         operator: "when")
-        ])
       (:match ,elixir-ts--definition-keywords-re @font-lock-keyword-face))
      (call
       target: (identifier) @font-lock-keyword-face
-      (arguments
-       (binary_operator
-        operator: "|>"
-        right: (identifier)))
-      (:match ,elixir-ts--definition-keywords-re @font-lock-keyword-face)))
+      (:match ,elixir-ts--kernel-keywords-re @font-lock-keyword-face)))
 
    :language 'elixir
-   :feature 'elixir-constant
-   `((binary_operator operator: "|>" right: (identifier)
-                      @font-lock-function-name-face)
-     ((identifier) @font-lock-keyword-face
-      (:match ,elixir-ts--builtin-keywords-re
-              @font-lock-keyword-face))
-     ((identifier) @font-lock-comment-face
-      (:match "^_" @font-lock-comment-face))
-     (identifier) @font-lock-function-name-face
-     ["%"] @font-lock-keyward-face
-     ["," ";"] @font-lock-keyword-face
-     ["(" ")" "[" "]" "{" "}" "<<" ">>"] @font-lock-keyword-face)
+   :feature 'elixir-function-call
+   '((call target: (identifier) @font-lock-function-call-face)
+     (unary_operator operator: "&" @font-lock-operator-face
+                     operand: (binary_operator
+                               left: (identifier)
+                               @font-lock-function-call-face
+                               operator: "/" right: (integer)))
+     (call
+      target: (dot right: (identifier) @font-lock-function-call-face))
+     (unary_operator operator: "&" @font-lock-variable-name-face
+                     operand: (integer) @font-lock-variable-name-face)
+     (unary_operator operator: "&" @font-lock-operator-face
+                     operand: (list)))
 
    :language 'elixir
-   :feature 'elixir-sigil
+   :feature 'elixir-string-escape
    :override t
-   `((sigil
-      (sigil_name) @elixir-ts-font-sigil-name-face
-      (:match "^[^HF]$" @elixir-ts-font-sigil-name-face))
-     @font-lock-string-face
-     (sigil
-      (sigil_name) @font-lock-regexp-face
-      (:match "^[rR]$" @font-lock-regexp-face))
-     @font-lock-regexp-face
-     (sigil
-      "~" @font-lock-string-face
-      (sigil_name) @elixir-ts-font-sigil-name-face
-      quoted_start: _ @font-lock-string-face
-      quoted_end: _ @font-lock-string-face
-      (:match "^[HF]$" @elixir-ts-font-sigil-name-face)))
+   `((escape_sequence) @font-lock-escape-face)
 
    :language 'elixir
-   :feature 'elixir-string-escape
+   :feature 'elixir-number
+   '([(integer) (float)] @font-lock-number-face)
+
+   :language 'elixir
+   :feature 'elixir-variable
+   '((binary_operator left: (identifier) @font-lock-variable-name-face)
+     (binary_operator right: (identifier) @font-lock-variable-name-face)
+     (arguments ( (identifier) @font-lock-variable-name-face))
+     (tuple (identifier) @font-lock-variable-name-face)
+     (list (identifier) @font-lock-variable-name-face)
+     (pair value: (identifier) @font-lock-variable-name-face)
+     (body (identifier) @font-lock-variable-name-face)
+     (unary_operator operand: (identifier) @font-lock-variable-name-face)
+     (interpolation (identifier) @font-lock-variable-name-face)
+     (do_block (identifier) @font-lock-variable-name-face))
+
+   :language 'elixir
+   :feature 'elixir-builtin
    :override t
-   `((escape_sequence) @font-lock-regexp-grouping-backslash))
+   `(((identifier) @font-lock-builtin-face
+      (:match ,elixir-ts--builtin-keywords-re
+              @font-lock-builtin-face))))
+
   "Tree-sitter font-lock settings.")
 
 (defvar elixir-ts--treesit-range-rules
@@ -640,10 +697,12 @@ elixir-ts-mode
     ;; Font-lock.
     (setq-local treesit-font-lock-settings elixir-ts--font-lock-settings)
     (setq-local treesit-font-lock-feature-list
-                '(( elixir-comment elixir-constant elixir-doc )
-                  ( elixir-string elixir-keyword elixir-unary-operator
-                    elixir-call elixir-operator )
-                  ( elixir-sigil elixir-string-escape elixir-string-interpolation)))
+                '(( elixir-comment elixir-doc elixir-function-name)
+                  ( elixir-string elixir-keyword elixir-data-type)
+                  ( elixir-sigil elixir-variable elixir-builtin
+                    elixir-string-escape)
+                  ( elixir-function-call elixir-operator elixir-number )))
+
 
     ;; Imenu.
     (setq-local treesit-simple-imenu-settings
@@ -675,13 +734,13 @@ elixir-ts-mode
                           heex-ts--indent-rules))
 
       (setq-local treesit-font-lock-feature-list
-                  '(( elixir-comment elixir-constant elixir-doc
+                  '(( elixir-comment elixir-doc elixir-function-name
                       heex-comment heex-keyword heex-doctype )
-                    ( elixir-string elixir-keyword elixir-unary-operator
-                      elixir-call elixir-operator
-                      heex-component heex-tag heex-attribute heex-string)
-                    ( elixir-sigil elixir-string-escape
-                      elixir-string-interpolation ))))
+                    ( elixir-string elixir-keyword elixir-data-type
+                      heex-component heex-tag heex-attribute heex-string )
+                    ( elixir-sigil elixir-variable elixir-builtin
+                      elixir-string-escape)
+                    ( elixir-function-call elixir-operator elixir-number ))))
 
     (treesit-major-mode-setup)
     (setq-local syntax-propertize-function #'elixir-ts--syntax-propertize)))
-- 
2.43.0

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Dmitry Gutov]

On 27/11/2023 19:59, Wilhelm Kirschbaum wrote:
> Here is a patch to address numerous issues flagged on Elixir slack,
> Github and in this thread.  It will not be perfect, but since the
> changes are pretty large I want to get this in and then we can pick on
> specific issues afterwards if that makes sense?

Thank you. No problem, pushed to master.

> I am making the assumption that it is okay to rename custom faces as
> elixir-ts-mode is only for 30.

I think so.

> One thing I tried to get right is to ensure that each level works
> relatively well, which means a bit more brute forcing queries.  I have
> not seen a major performance issue on massic Elixir files, so think its
> fine.

One thing that jumped out at me is that arguments in method definitions 
(e.g. 'def build(parent, root_path, opts) do') are highlighted with the 
-use- face. Apparently, that's simply because the grammar parses these 
as nodes of type "call", just like it does for regular function calls. 
So that's unusual.

I suppose it's possible to separate them by matching on the call 
target's text? Which would be "def" or "defp".

Conversely, variable refs in expressions such as

   %{
     "start" => %{"line" => line, "character" => start_idx},
     "end" => %{"line" => line, "character" => start_idx + length}
   }

are highlighted with -name-, even though there's no destructuring here.

Anyway, good job, I can see that Elixir's grammar is one of the harder 
ones to work with.

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Andrey Listopadov]

Dmitry Gutov <dmitry@gutov.dev> writes:

> On 27/11/2023 19:59, Wilhelm Kirschbaum wrote:
>> Here is a patch to address numerous issues flagged on Elixir slack,
>> Github and in this thread.  It will not be perfect, but since the
>> changes are pretty large I want to get this in and then we can pick on
>> specific issues afterwards if that makes sense?
>
> Thank you. No problem, pushed to master.

Thanks! The code now seems to be properly highlighted. I've just tested
the update and noticed that putting the `do' keyword on a separate line
breaks indentation. Here's an example:

defmodule Foo
  do # case A
  @moduledoc """
  Test module.
  """

  defp a(x), do: a(x, 0)

  defp a(x, y),
       do: a(x, 0, 0) # case B

  defp a(x, y, z)
do # case C
  x + y + z
end
  end

I have intentionally introduced incorrect indentation before the first
`do' keyword (case A), but the matching `end' keyword was indented
automatically when I called `indent-region' on the whole buffer. The
case B seems to be indented incorrectly, the default formatter would
indent such `do:' by just two spaces after the parent `defp'.

The third case C is similar to case A, except the indentation was
provided by Emacs, meaning, after pressing the RET key before the `do'
keyword, Emacs had put the keyword at the BOL. If there are no `do'
after the closing parenthesis, the automatic indentation is correct.

I'm not sure if these problems were introduced by the changes, or were
present before, and if I should send a separeate bug report for them, as
this isn't strictly related to highlighting, just with the
tree-sitter-based indentation.


--
Andrey Listopadov

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Wilhelm Kirschbaum]

Dmitry Gutov <dmitry@gutov.dev> writes:

> On 27/11/2023 19:59, Wilhelm Kirschbaum wrote:
>> Here is a patch to address numerous issues flagged on Elixir 
>> slack,
>> Github and in this thread.  It will not be perfect, but since 
>> the
>> changes are pretty large I want to get this in and then we can 
>> pick on
>> specific issues afterwards if that makes sense?
>
> Thank you. No problem, pushed to master.
>

Thanks, I have one or two more change related to this issue 
coming.

>> I am making the assumption that it is okay to rename custom 
>> faces as
>> elixir-ts-mode is only for 30.
>
> I think so.
>
>> One thing I tried to get right is to ensure that each level 
>> works
>> relatively well, which means a bit more brute forcing queries.  
>> I have
>> not seen a major performance issue on massic Elixir files, so 
>> think its
>> fine.
>
> One thing that jumped out at me is that arguments in method
> definitions (e.g. 'def build(parent, root_path, opts) do') are
> highlighted with the -use- face. Apparently, that's simply 
> because the
> grammar parses these as nodes of type "call", just like it does 
> for
> regular function calls. So that's unusual.
>
> I suppose it's possible to separate them by matching on the call
> target's text? Which would be "def" or "defp".
>
> Conversely, variable refs in expressions such as
>
>   %{
>     "start" => %{"line" => line, "character" => start_idx},
>     "end" => %{"line" => line, "character" => start_idx + 
>     length}
>   }
>
> are highlighted with -name-, even though there's no 
> destructuring here.
>
> Anyway, good job, I can see that Elixir's grammar is one of the 
> harder
> ones to work with.

Thanks.  I am pretty busy atm, so will only get time for this in 
about a
week.  There are definitely some issues I spotted as well which 
needs to
be fixed.

Wilhelm

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Wilhelm Kirschbaum]

Andrey Listopadov <andreyorst@gmail.com> writes:

> Dmitry Gutov <dmitry@gutov.dev> writes:
>
>> On 27/11/2023 19:59, Wilhelm Kirschbaum wrote:
>>> Here is a patch to address numerous issues flagged on Elixir 
>>> slack,
>>> Github and in this thread.  It will not be perfect, but since 
>>> the
>>> changes are pretty large I want to get this in and then we can 
>>> pick on
>>> specific issues afterwards if that makes sense?
>>
>> Thank you. No problem, pushed to master.
>
> Thanks! The code now seems to be properly highlighted. I've just 
> tested
> the update and noticed that putting the `do' keyword on a 
> separate line
> breaks indentation. Here's an example:
>
> defmodule Foo
>   do # case A
>   @moduledoc """
>   Test module.
>   """
>
>   defp a(x), do: a(x, 0)
>
>   defp a(x, y),
>        do: a(x, 0, 0) # case B
>
>   defp a(x, y, z)
> do # case C
>   x + y + z
> end
>   end
>
> I have intentionally introduced incorrect indentation before the 
> first
> `do' keyword (case A), but the matching `end' keyword was 
> indented
> automatically when I called `indent-region' on the whole buffer. 
> The
> case B seems to be indented incorrectly, the default formatter 
> would
> indent such `do:' by just two spaces after the parent `defp'.
>
> The third case C is similar to case A, except the indentation 
> was
> provided by Emacs, meaning, after pressing the RET key before 
> the `do'
> keyword, Emacs had put the keyword at the BOL. If there are no 
> `do'
> after the closing parenthesis, the automatic indentation is 
> correct.
>
> I'm not sure if these problems were introduced by the changes, 
> or were
> present before, and if I should send a separeate bug report for 
> them, as
> this isn't strictly related to highlighting, just with the
> tree-sitter-based indentation.

The indention should be another issue imo. The changes in this 
thread
would not impact indentation.

Indentation is probably slightly more complicated than 
fontification, but
planning to take some time in a couple of weeks to fix some 
issues.

Wilhelm

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Stefan Kangas]

Wilhelm Kirschbaum <wkirschbaum@gmail.com> writes:

> Thanks, I have one or two more change related to this issue coming.

Any updates here?

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Wilhelm Kirschbaum]

[-- Attachment #1.1: Type: text/plain, Size: 469 bytes --]

On Wed, Jan 10, 2024 at 7:48 PM Stefan Kangas <stefankangas@gmail.com>
wrote:

> Wilhelm Kirschbaum <wkirschbaum@gmail.com> writes:
>
> > Thanks, I have one or two more change related to this issue coming.
>
> Any updates here?
>

This patch is the last of the fontification patches for now. Once these are
installed I think we can close this. I will work on some indentation
changes later, but don't think they need to be part of this issue.

Wilhelm

[-- Attachment #1.2: Type: text/html, Size: 851 bytes --]

[-- Attachment #2: 0001-Add-access_call-fontification-to-elixir-ts-mode.patch --]
[-- Type: text/x-patch, Size: 1222 bytes --]

From 3845d869211069f3361c9e02077c60ecefb7110a Mon Sep 17 00:00:00 2001
From: Wilhelm Kirschbaum <wkirschbaum@gmail.com>
Date: Fri, 29 Dec 2023 17:09:00 +0200
Subject: [PATCH] Add access_call fontification to elixir-ts-mode

* lisp/progmodes/elixir-ts-mode.el:
(elixir-ts--font-lock-settings):
Add access_call queries to the elixir-variable feature.
---
 lisp/progmodes/elixir-ts-mode.el | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lisp/progmodes/elixir-ts-mode.el b/lisp/progmodes/elixir-ts-mode.el
index b493195eedd..2c7323c318d 100644
--- a/lisp/progmodes/elixir-ts-mode.el
+++ b/lisp/progmodes/elixir-ts-mode.el
@@ -546,7 +546,9 @@ elixir-ts--font-lock-settings
      (body (identifier) @font-lock-variable-name-face)
      (unary_operator operand: (identifier) @font-lock-variable-name-face)
      (interpolation (identifier) @font-lock-variable-name-face)
-     (do_block (identifier) @font-lock-variable-name-face))
+     (do_block (identifier) @font-lock-variable-name-face)
+     (access_call target: (identifier) @font-lock-variable-name-face)
+     (access_call "[" key: (identifier) @font-lock-variable-name-face "]"))
 
    :language 'elixir
    :feature 'elixir-builtin
-- 
2.34.1

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Dmitry Gutov]

Hi!

On 13/01/2024 10:50, Wilhelm Kirschbaum wrote:
> + (access_call target: (identifier) @font-lock-variable-name-face) + 
> (access_call "[" key: (identifier) @font-lock-variable-name-face "]"))

This should use font-lock-variable-use-face. And all other "variable 
reference" highlights should use it too.

OTOH, the method parameters are still highlighted with 
font-lock-variable-use-face, which should be font-lock-variable-name-face.

This happens inside the first 'elixir-variable' highlight. Perhaps 
elixir-ts--definition-keywords-re could be used there to disambiguate as 
well.

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Dmitry Gutov]

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

On 29/01/2024 06:08, Dmitry Gutov wrote:
> Hi!
> 
> On 13/01/2024 10:50, Wilhelm Kirschbaum wrote:
>> + (access_call target: (identifier) @font-lock-variable-name-face) + 
>> (access_call "[" key: (identifier) @font-lock-variable-name-face "]"))
> 
> This should use font-lock-variable-use-face. And all other "variable 
> reference" highlights should use it too.
> 
> OTOH, the method parameters are still highlighted with 
> font-lock-variable-use-face, which should be font-lock-variable-name-face.
> 
> This happens inside the first 'elixir-variable' highlight. Perhaps 
> elixir-ts--definition-keywords-re could be used there to disambiguate as 
> well.

See this combined patch:

1. Your additions from the last attachment (access target highlighting).
2. All instances of font-lock-variable-name-face swapped for 
font-lock-variable-use-face (since most of those match variable references).
3. Added highlighting for method parameters with 
font-lock-variable-name-face.
4. Feature elixir-function-name renamed to elixir-definition since it 
now touches both function and variable (parameter) definitions.
5. Feature elixir-variable moved to the feature level 4, since that's 
where it is in other built-in ts modes.

Any objections to it?

[-- Attachment #2: elixir-ts-use-vs-name.diff --]
[-- Type: text/x-patch, Size: 5893 bytes --]

diff --git a/lisp/progmodes/elixir-ts-mode.el b/lisp/progmodes/elixir-ts-mode.el
index b493195eedd..57db211e881 100644
--- a/lisp/progmodes/elixir-ts-mode.el
+++ b/lisp/progmodes/elixir-ts-mode.el
@@ -360,13 +360,14 @@ elixir-ts--indent-rules
 (defvar elixir-ts--font-lock-settings
   (treesit-font-lock-rules
    :language 'elixir
-   :feature 'elixir-function-name
+   :feature 'elixir-definition
    `((call target: (identifier) @target-identifier
            (arguments (identifier) @font-lock-function-name-face)
            (:match ,elixir-ts--definition-keywords-re @target-identifier))
      (call target: (identifier) @target-identifier
            (arguments
-            (call target: (identifier) @font-lock-function-name-face))
+            (call target: (identifier) @font-lock-function-name-face
+                  (arguments ((identifier)) @font-lock-variable-name-face)))
            (:match ,elixir-ts--definition-keywords-re @target-identifier))
      (call target: (identifier) @target-identifier
            (arguments
@@ -379,13 +380,15 @@ elixir-ts--font-lock-settings
            (:match ,elixir-ts--definition-keywords-re @target-identifier))
      (call target: (identifier) @target-identifier
            (arguments
-            (call target: (identifier) @font-lock-function-name-face))
+            (call target: (identifier) @font-lock-function-name-face
+                  (arguments ((identifier)) @font-lock-variable-name-face)))
            (do_block)
            (:match ,elixir-ts--definition-keywords-re @target-identifier))
      (call target: (identifier) @target-identifier
            (arguments
             (binary_operator
-             left: (call target: (identifier) @font-lock-function-name-face)))
+             left: (call target: (identifier) @font-lock-function-name-face
+                         (arguments ((identifier)) @font-lock-variable-name-face))))
            (do_block)
            (:match ,elixir-ts--definition-keywords-re @target-identifier))
      (unary_operator
@@ -521,8 +524,8 @@ elixir-ts--font-lock-settings
                                operator: "/" right: (integer)))
      (call
       target: (dot right: (identifier) @font-lock-function-call-face))
-     (unary_operator operator: "&" @font-lock-variable-name-face
-                     operand: (integer) @font-lock-variable-name-face)
+     (unary_operator operator: "&" @font-lock-variable-use-face
+                     operand: (integer) @font-lock-variable-use-face)
      (unary_operator operator: "&" @font-lock-operator-face
                      operand: (list)))
 
@@ -537,16 +540,18 @@ elixir-ts--font-lock-settings
 
    :language 'elixir
    :feature 'elixir-variable
-   '((binary_operator left: (identifier) @font-lock-variable-name-face)
-     (binary_operator right: (identifier) @font-lock-variable-name-face)
-     (arguments ( (identifier) @font-lock-variable-name-face))
-     (tuple (identifier) @font-lock-variable-name-face)
-     (list (identifier) @font-lock-variable-name-face)
-     (pair value: (identifier) @font-lock-variable-name-face)
-     (body (identifier) @font-lock-variable-name-face)
-     (unary_operator operand: (identifier) @font-lock-variable-name-face)
-     (interpolation (identifier) @font-lock-variable-name-face)
-     (do_block (identifier) @font-lock-variable-name-face))
+   '((binary_operator left: (identifier) @font-lock-variable-use-face)
+     (binary_operator right: (identifier) @font-lock-variable-use-face)
+     (arguments ((identifier) @font-lock-variable-use-face))
+     (tuple (identifier) @font-lock-variable-use-face)
+     (list (identifier) @font-lock-variable-use-face)
+     (pair value: (identifier) @font-lock-variable-use-face)
+     (body (identifier) @font-lock-variable-use-face)
+     (unary_operator operand: (identifier) @font-lock-variable-use-face)
+     (interpolation (identifier) @font-lock-variable-use-face)
+     (do_block (identifier) @font-lock-variable-use-face)
+     (access_call target: (identifier) @font-lock-variable-use-face)
+     (access_call "[" key: (identifier) @font-lock-variable-use-face "]"))
 
    :language 'elixir
    :feature 'elixir-builtin
@@ -697,11 +702,10 @@ elixir-ts-mode
     ;; Font-lock.
     (setq-local treesit-font-lock-settings elixir-ts--font-lock-settings)
     (setq-local treesit-font-lock-feature-list
-                '(( elixir-comment elixir-doc elixir-function-name)
+                '(( elixir-comment elixir-doc elixir-definition)
                   ( elixir-string elixir-keyword elixir-data-type)
-                  ( elixir-sigil elixir-variable elixir-builtin
-                    elixir-string-escape)
-                  ( elixir-function-call elixir-operator elixir-number )))
+                  ( elixir-sigil elixir-builtin elixir-string-escape)
+                  ( elixir-function-call elixir-variable elixir-operator elixir-number )))
 
 
     ;; Imenu.
@@ -734,13 +738,12 @@ elixir-ts-mode
                           heex-ts--indent-rules))
 
       (setq-local treesit-font-lock-feature-list
-                  '(( elixir-comment elixir-doc elixir-function-name
+                  '(( elixir-comment elixir-doc elixir-definition
                       heex-comment heex-keyword heex-doctype )
                     ( elixir-string elixir-keyword elixir-data-type
                       heex-component heex-tag heex-attribute heex-string )
-                    ( elixir-sigil elixir-variable elixir-builtin
-                      elixir-string-escape)
-                    ( elixir-function-call elixir-operator elixir-number ))))
+                    ( elixir-sigil elixir-builtin elixir-string-escape)
+                    ( elixir-function-call elixir-variable elixir-operator elixir-number ))))
 
     (treesit-major-mode-setup)
     (setq-local syntax-propertize-function #'elixir-ts--syntax-propertize)))

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Wilhelm Kirschbaum]

On 2024/01/30 03:59, Dmitry Gutov wrote:

> On 29/01/2024 06:08, Dmitry Gutov wrote:
>> Hi!
>>
>> On 13/01/2024 10:50, Wilhelm Kirschbaum wrote:
>>> + (access_call target: (identifier) @font-lock-variable-name-face) + 
>>> (access_call "[" key: (identifier) @font-lock-variable-name-face "]"))
>>
>> This should use font-lock-variable-use-face. And all other "variable 
>> reference" highlights should use it too.
>>
>> OTOH, the method parameters are still highlighted with 
>> font-lock-variable-use-face, which should be 
>> font-lock-variable-name-face.
>>
>> This happens inside the first 'elixir-variable' highlight. Perhaps 
>> elixir-ts--definition-keywords-re could be used there to disambiguate 
>> as well.
>
> See this combined patch:
>
> 1. Your additions from the last attachment (access target highlighting).
> 2. All instances of font-lock-variable-name-face swapped for 
> font-lock-variable-use-face (since most of those match variable 
> references).
Thanks, this makes sense.
> 3. Added highlighting for method parameters with 
> font-lock-variable-name-face.
I had a look and think it covers most instances ( some should arguably 
not be highlighted as use-face, but can be debated ).
> 4. Feature elixir-function-name renamed to elixir-definition since it 
> now touches both function and variable (parameter) definitions.
Makes sense
> 5. Feature elixir-variable moved to the feature level 4, since that's 
> where it is in other built-in ts modes.
>
Agreed.
> Any objections to it?
Thanks for the effort and I have no objections.

There are however some more issues I spotted on the function-name and 
function-call matches which I will be looking into.


Wilhelm

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Wilhelm Kirschbaum]

On 2024/02/05 19:05, Wilhelm Kirschbaum wrote:
> On 2024/01/30 03:59, Dmitry Gutov wrote:
>
>> On 29/01/2024 06:08, Dmitry Gutov wrote:
>>> Hi!
>>>
>>> On 13/01/2024 10:50, Wilhelm Kirschbaum wrote:
>>>> + (access_call target: (identifier) @font-lock-variable-name-face) 
>>>> + (access_call "[" key: (identifier) @font-lock-variable-name-face 
>>>> "]"))
>>>
>>> This should use font-lock-variable-use-face. And all other "variable 
>>> reference" highlights should use it too.
>>>
>>> OTOH, the method parameters are still highlighted with 
>>> font-lock-variable-use-face, which should be 
>>> font-lock-variable-name-face.
>>>
>>> This happens inside the first 'elixir-variable' highlight. Perhaps 
>>> elixir-ts--definition-keywords-re could be used there to 
>>> disambiguate as well.
>>
>> See this combined patch:
>>
>> 1. Your additions from the last attachment (access target highlighting).
>> 2. All instances of font-lock-variable-name-face swapped for 
>> font-lock-variable-use-face (since most of those match variable 
>> references).
> Thanks, this makes sense.
>> 3. Added highlighting for method parameters with 
>> font-lock-variable-name-face.
> I had a look and think it covers most instances ( some should arguably 
> not be highlighted as use-face, but can be debated ).
>> 4. Feature elixir-function-name renamed to elixir-definition since it 
>> now touches both function and variable (parameter) definitions.
> Makes sense
>> 5. Feature elixir-variable moved to the feature level 4, since that's 
>> where it is in other built-in ts modes.
>>
> Agreed.
>> Any objections to it?
> Thanks for the effort and I have no objections.
>
> There are however some more issues I spotted on the function-name and 
> function-call matches which I will be looking into.

Adding this as the first item to :feature 'elixir-definition fixes the 
function-call/name issue:

@@ -360,13 +360,19 @@ elixir-ts--indent-rules
  (defvar elixir-ts--font-lock-settings
    (treesit-font-lock-rules
     :language 'elixir
-   :feature 'elixir-function-name
+   :feature 'elixir-definition
     `((call target: (identifier) @target-identifier
+           (arguments
+            (call target: (identifier) @font-lock-function-name-face
+                  (arguments)))
+           (:match ,elixir-ts--definition-keywords-re @target-identifier))
+     (call target: (identifier) @target-identifier
             (arguments (identifier) @font-lock-function-name-face)

I will be working in Elixir this week and will set different fonts for 
to test it during the week, but don't think it should hold up installing 
the suggested patch so long.

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Dmitry Gutov]

On 05/02/2024 19:34, Wilhelm Kirschbaum wrote:
> 
> Adding this as the first item to :feature 'elixir-definition fixes the 
> function-call/name issue:
> 
> @@ -360,13 +360,19 @@ elixir-ts--indent-rules
>   (defvar elixir-ts--font-lock-settings
>     (treesit-font-lock-rules
>      :language 'elixir
> -   :feature 'elixir-function-name
> +   :feature 'elixir-definition
>      `((call target: (identifier) @target-identifier
> +           (arguments
> +            (call target: (identifier) @font-lock-function-name-face
> +                  (arguments)))
> +           (:match ,elixir-ts--definition-keywords-re @target-identifier))
> +     (call target: (identifier) @target-identifier
>              (arguments (identifier) @font-lock-function-name-face)
> 
> I will be working in Elixir this week and will set different fonts for 
> to test it during the week, but don't think it should hold up installing 
> the suggested patch so long.

Thanks!

Could you give an example of problematic highlighting, though? So I can 
keep it mind when installing both changes.

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Wilhelm Kirschbaum]

On 2024/02/05 19:42, Dmitry Gutov wrote:

> On 05/02/2024 19:34, Wilhelm Kirschbaum wrote:
>>
>> Adding this as the first item to :feature 'elixir-definition fixes 
>> the function-call/name issue:
>>
>> @@ -360,13 +360,19 @@ elixir-ts--indent-rules
>>   (defvar elixir-ts--font-lock-settings
>>     (treesit-font-lock-rules
>>      :language 'elixir
>> -   :feature 'elixir-function-name
>> +   :feature 'elixir-definition
>>      `((call target: (identifier) @target-identifier
>> +           (arguments
>> +            (call target: (identifier) @font-lock-function-name-face
>> +                  (arguments)))
>> +           (:match ,elixir-ts--definition-keywords-re 
>> @target-identifier))
>> +     (call target: (identifier) @target-identifier
>>              (arguments (identifier) @font-lock-function-name-face)
>>
>> I will be working in Elixir this week and will set different fonts 
>> for to test it during the week, but don't think it should hold up 
>> installing the suggested patch so long.
>
> Thanks!
>
> Could you give an example of problematic highlighting, though? So I 
> can keep it mind when installing both changes.
Of course, sorry:

def boo(:one), do: :boo

highlighted boo as function-call-face, not function-name-face. Same with


def foo("boo") do

end


and variants.

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Dmitry Gutov]

On 05/02/2024 19:47, Wilhelm Kirschbaum wrote:
>> Could you give an example of problematic highlighting, though? So I 
>> can keep it mind when installing both changes.
> Of course, sorry:
> 
> def boo(:one), do: :boo
> 
> highlighted boo as function-call-face, not function-name-face. Same with
> 
> 
> def foo("boo") do
> 
> end
> 
> 
> and variants.

Thanks! Now I get it.

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Dmitry Gutov]

On 05/02/2024 22:51, Dmitry Gutov wrote:
> On 05/02/2024 19:47, Wilhelm Kirschbaum wrote:
>>> Could you give an example of problematic highlighting, though? So I 
>>> can keep it mind when installing both changes.
>> Of course, sorry:
>>
>> def boo(:one), do: :boo
>>
>> highlighted boo as function-call-face, not function-name-face. Same with
>>
>>
>> def foo("boo") do
>>
>> end
>>
>>
>> and variants.
> 
> Thanks! Now I get it.

I've now pushed the two your patches and one of mine.

Not closing the report yet because you said some problem with function 
calls remains, right?

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Dmitry Gutov]

On 05/02/2024 19:05, Wilhelm Kirschbaum wrote:
> I had a look and think it covers most instances ( some should arguably 
> not be highlighted as use-face, but can be debated ).

BTW, if you meant variable assignments here, these indeed could be 
discussed.

bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Wilhelm Kirschbaum]

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

> On 05/02/2024 22:51, Dmitry Gutov wrote:
> > On 05/02/2024 19:47, Wilhelm Kirschbaum wrote:
> >>> Could you give an example of problematic highlighting, though? So I
> >>> can keep it mind when installing both changes.
> >> Of course, sorry:
> >>
> >> def boo(:one), do: :boo
> >>
> >> highlighted boo as function-call-face, not function-name-face. Same with
> >>
> >>
> >> def foo("boo") do
> >>
> >> end
> >>
> >>
> >> and variants.
> >
> > Thanks! Now I get it.
>
> I've now pushed the two your patches and one of mine.
>

Thank you

>
> Not closing the report yet because you said some problem with function
> calls remains, right?
>

Yes, one more with function calls and one issue with else blocks, where the
wrong font gets applied. I will attempt a patch next week.

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