From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Wilhelm Kirschbaum Newsgroups: gmane.emacs.bugs Subject: bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently Date: Fri, 24 Nov 2023 21:47:32 +0200 Message-ID: References: <87y1ewgnn7.fsf@gmail.com> Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="000000000000cee100060aeb39bc" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="2746"; mail-complaints-to="usenet@ciao.gmane.io" Cc: Andrey Listopadov , 67246@debbugs.gnu.org To: Dmitry Gutov Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Fri Nov 24 20:48:37 2023 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1r6cAG-0000Vu-4T for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 24 Nov 2023 20:48:36 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1r6c9f-00055T-9m; Fri, 24 Nov 2023 14:47:59 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1r6c9d-00053o-Rw for bug-gnu-emacs@gnu.org; Fri, 24 Nov 2023 14:47:57 -0500 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1r6c9d-0005F7-KZ for bug-gnu-emacs@gnu.org; Fri, 24 Nov 2023 14:47:57 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1r6c9h-0003GG-Uk for bug-gnu-emacs@gnu.org; Fri, 24 Nov 2023 14:48:01 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Wilhelm Kirschbaum Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Fri, 24 Nov 2023 19:48:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 67246 X-GNU-PR-Package: emacs Original-Received: via spool by 67246-submit@debbugs.gnu.org id=B67246.170085528012522 (code B ref 67246); Fri, 24 Nov 2023 19:48:01 +0000 Original-Received: (at 67246) by debbugs.gnu.org; 24 Nov 2023 19:48:00 +0000 Original-Received: from localhost ([127.0.0.1]:37318 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1r6c9f-0003Fn-Nb for submit@debbugs.gnu.org; Fri, 24 Nov 2023 14:48:00 -0500 Original-Received: from mail-qt1-x82f.google.com ([2607:f8b0:4864:20::82f]:44441) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1r6c9a-0003Ee-3v for 67246@debbugs.gnu.org; Fri, 24 Nov 2023 14:47:58 -0500 Original-Received: by mail-qt1-x82f.google.com with SMTP id d75a77b69052e-41cd4cc515fso11839291cf.1 for <67246@debbugs.gnu.org>; Fri, 24 Nov 2023 11:47:49 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1700855264; x=1701460064; darn=debbugs.gnu.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=O+l4IrU5iMU3dqz73/u7FugvVULv8gX04lMRTXL8R4o=; b=FrlabXhlPLjuCt1j+BYkNdSTPFGB9q6HwpNivRgzIdNVhHDDs0fx/egMy2RIFfRRh7 SJZIybLgDvLSjfobaC10WSE7FzVWCQGa/ktFAj4El8R60x8VBuRF/+fnprLuHL5wrHHS kZTOhKV7avnLfqUiCvDy4hzy0mJPxIBdLcSfumnhfZDYE7Phu3yLLeIGhCEFdmrGLq0s dqLnMeAifzTVOJ3r9Izp/lhJhvrjpL9NtJm86TJA5OXbHnN5m/SxCwufave/YC5mI9W3 KBVOMkEiwHDpr+dpo0ZzDt1ZZ8lm2IZ5R65P+gXO1jjZXl5iJessksKGh78nhYrvznW5 F7ug== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1700855264; x=1701460064; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=O+l4IrU5iMU3dqz73/u7FugvVULv8gX04lMRTXL8R4o=; b=g2e5AtAVNoRvsaxKebVlkFUtwgdgDamFCWMHa3hmL7Cgj7pwk8QAUWIaMrZXkRiGMz 3uoF1p1f0Vu2pP68qJ7qYZ1Kz6Tkxn4nDVevhzuI4rHN7FbKSDxt+nUMbzJ2q84Zil5s /BhitLsMoMIq9/lW1lbSvF11hK/g+mpNouiAhOyx9ZE6No2Ns8hQavZDcQpWqMNeH5+b Ha/K/BSE1jh73xge00PMKzhO2u8tkxUr7qwuGfDqCjWQHiSi27YCKnTM3XwZWLviSDnl 5eOc6OHVtPgQ8KqGqcn6xg6gNHUw+aWV7HXOGspgUMtS0naFGwSeiKjKtFPcK8/2vQU8 vmAQ== X-Gm-Message-State: AOJu0Yxil4d9ixeYmtBZsyD+X5mOIMA0qETeHXUMzrxFIvJQ6tzCIAga ViIcLJ5Ew50qnevsLaBkiROPVS35cicyC9O5cEQ= X-Google-Smtp-Source: AGHT+IHYGMlzbQTn0uSlzmla/Qi+5uNr7E+xPlvp+yueiTigMqfCX0JgrmZiTCaJ8zEyJlPF/0HdzX07tfDGACr8ig8= X-Received: by 2002:a05:622a:a13:b0:423:98be:e73e with SMTP id bv19-20020a05622a0a1300b0042398bee73emr4592630qtb.66.1700855263713; Fri, 24 Nov 2023 11:47:43 -0800 (PST) In-Reply-To: X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:274888 Archived-At: --000000000000cee100060aeb39bc Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, Nov 20, 2023 at 3:50=E2=80=AFAM Dmitry Gutov wro= te: > On 18/11/2023 09:50, Wilhelm Kirschbaum wrote: > > > > On Sat, Nov 18, 2023 at 3:36=E2=80=AFAM Dmitry Gutov > > wrote: > > > > On 17/11/2023 21:50, Andrey Listopadov wrote: > > > > > > I've upgraded from elixir-mode to elixir-ts-mode and noticed tha= t > the > > > latter uses faces rather inconsistently. > > > > > > For example, the =3Dfont-lock-keyword-face=3D is used for both > > keywords and > > > method calls, as well as for parentheses. The > > > =3Dfont-lock-function-name-face=3D is used both for function > definitions, > > > parameter names, and calls. Some calls use the > > > =3Dfont-lock-keyword-face=3D, for example the call to `raise'. = The > > > =3Dfont-lock-type-face=3D is used both for types and =3D:symbols= =3D. > > > > > > 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 functi= on > > > 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 t= o > > 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 cal= ls > > and property lookups go to the 4th level of highlighting, whereas t= he > > 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 prefe= r > 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 ca= n > > 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 > =E2=80=98-face=E2=80=99 (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. > --000000000000cee100060aeb39bc Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Mon, Nov 20, 2023 at 3:50=E2=80=AF= AM Dmitry Gutov <dmitry@gutov.dev> wrote:
On = 18/11/2023 09:50, Wilhelm Kirschbaum wrote:
>
> On Sat, Nov 18, 2023 at 3:36=E2=80=AFAM Dmitry Gutov <dmitry@gutov.dev
> <mailto:dmitr= y@gutov.dev>> wrote:
>
>=C2=A0 =C2=A0 =C2=A0On 17/11/2023 21:50, Andrey Listopadov wrote:
>=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 > I've upgraded from elixir-mode to elixir-= ts-mode and noticed that the
>=C2=A0 =C2=A0 =C2=A0 > latter uses faces rather inconsistently.
>=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 > For example, the =3Dfont-lock-keyword-face=3D= is used for both
>=C2=A0 =C2=A0 =C2=A0keywords and
>=C2=A0 =C2=A0 =C2=A0 > method calls, as well as for parentheses.=C2= =A0 The
>=C2=A0 =C2=A0 =C2=A0 > =3Dfont-lock-function-name-face=3D is used bo= th for function definitions,
>=C2=A0 =C2=A0 =C2=A0 > parameter names, and calls.=C2=A0 Some calls = use the
>=C2=A0 =C2=A0 =C2=A0 > =3Dfont-lock-keyword-face=3D, for example the= call to `raise'.=C2=A0 The
>=C2=A0 =C2=A0 =C2=A0 > =3Dfont-lock-type-face=3D is used both for ty= pes and =3D:symbols=3D.
>=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 > All of that basically makes Elixir code mostl= y use 2 colors.
>=C2=A0 =C2=A0 =C2=A0 > Additionally, it makes impossible selectively= disabling
>=C2=A0 =C2=A0 =C2=A0highlighting, as
>=C2=A0 =C2=A0 =C2=A0 > disabling the function call highlighting will= disable the function
>=C2=A0 =C2=A0 =C2=A0 > definition highlighitng an so on.
>=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 > I believe, Emacs 29 introduced a lot of faces= for all kinds of
>=C2=A0 =C2=A0 =C2=A0 > situations possible in Tree-sitter generated = AST, so perhaps the
>=C2=A0 =C2=A0 =C2=A0fix is
>=C2=A0 =C2=A0 =C2=A0 > to use them more semantically, rather than fo= r good looks.
>
>=C2=A0 =C2=A0 =C2=A0Thanks for the report. Wilhelm, could you look into= this? If you
>=C2=A0 =C2=A0 =C2=A0have time.
>
>=C2=A0 =C2=A0 =C2=A0Speaking of new faces, we added font-lock-function-= call-face that
>=C2=A0 =C2=A0 =C2=A0can be
>=C2=A0 =C2=A0 =C2=A0used for function calls, while font-lock-function-n= ame-face stays used
>=C2=A0 =C2=A0 =C2=A0on definitions.
>
>=C2=A0 =C2=A0 =C2=A0For parens, if one wanted to highlight them at all,=
>=C2=A0 =C2=A0 =C2=A0font-lock-bracket-face could be used. Though it'= ;s probably better to
>=C2=A0 =C2=A0 =C2=A0leave them to the 4th feature level (see js-ts-mode= as an example).
>=C2=A0 =C2=A0 =C2=A0elixir-ts-mode currently defines 3 levels.
>
>=C2=A0 =C2=A0 =C2=A0For levels, we've currently settled on the sche= me that function calls
>=C2=A0 =C2=A0 =C2=A0and property lookups go to the 4th level of highlig= hting, whereas the
>=C2=A0 =C2=A0 =C2=A0default is 3. This is all tweakable by the individu= al user, but I think
>=C2=A0 =C2=A0 =C2=A0it's better to stay consistent between the mode= s.
>
>=C2=A0 =C2=A0 =C2=A0Finally, I see that font-lock-function-name-face en= ds up being used for
>=C2=A0 =C2=A0 =C2=A0parameters (as Andrey mentioned) and local variable= s as well. That's
>=C2=A0 =C2=A0 =C2=A0not
>=C2=A0 =C2=A0 =C2=A0great; probably a query that ended up matching too = widely. We prefer to
>=C2=A0 =C2=A0 =C2=A0use font-lock-variable-name-face (for parameter dec= larations) or
>=C2=A0 =C2=A0 =C2=A0font-lock-variable-use-face for such identifiers. T= hough it can be hard
>=C2=A0 =C2=A0 =C2=A0to reliably distinguish them in a dynamic language,= so as far as I'm
>=C2=A0 =C2=A0 =C2=A0concerned, they could stay non-highlighted (the use= s, that is; the
>=C2=A0 =C2=A0 =C2=A0declarations are usually easy to find using queries= ).
>
>
> Thanks for the detailed explanation. It's unfortunate timing, beca= use 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.=C2= =A0 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.
=C2= =A0

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 i= n 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.=C2=A0 I read the docs and tried to=C2=A0
follow it with the n= ew changes, but was hesitant to remove much from the=C2=A0
highli= ghting as not many people might discover there is a 4th level.=C2=A0 Then a= gain, if there is a query it
is pretty easy just to communicate t= his.
=C2=A0

Something else I would recommend:

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

=C2=A0 =C2=A0You should not quote the symbol face, and it should not end in=
=E2=80=98-face=E2=80=99 (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 make= s sense.=C2=A0 I see a new mode with the same -face suffixes.=C2=A0
The defface docs does not mention this, so might be a good idea to = add it there?=C2=A0=C2=A0
I am not entirely sure what "you s= hould not quote the symbol face" means wrt to the changes, because=C2= =A0
it does not look like I am quoting it.
=C2=A0<= /div>

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.
--000000000000cee100060aeb39bc--