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

[Wilhelm Kirschbaum <wkirschbaum@gmail.com>]

[-- 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 --]