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.