bug#61521: "default" is now the first item returned from (font-faces), breaking various code.

bug#61521: "default" is now the first item returned from (font-faces), breaking various code.

[Brennan Vincent]

Various code seems to expect "default" to be the /last/ item in the list
returned by that function, not the first. For example, this comment in faces.el:

  ;; The `reverse' is so that `default' goes first.
  (dolist (face (nreverse (face-list)))


Also, org-html-htmlize-generate-css does not work when default comes first in
the list (as it skips processing all fonts after default).

I am not sure why this was changed and if the change was intentional, but it can
be fixed by changing the "<" to a ">" in the last line of face-list, so I
suspect it might have been a mistake.

diff --git lisp/faces.el lisp/faces.el
index 4933b495a6c..e998dc504e5 100644
--- lisp/faces.el
+++ lisp/faces.el
@@ -199,7 +199,7 @@ face-list
     (maphash (lambda (face spec)
                (push `(,(car spec) . ,face) faces))
              face--new-frame-defaults)
-    (mapcar #'cdr (sort faces (lambda (f1 f2) (< (car f1) (car f2)))))))
+    (mapcar #'cdr (sort faces (lambda (f1 f2) (> (car f1) (car f2)))))))

 (defun make-face (face)
   "Define a new face with name FACE, a symbol.

bug#61521: "default" is now the first item returned from (font-faces), breaking various code.

[Brennan Vincent]

I suspect frame-face-alist needs to be changed in a similar way.

On 2023-02-14 19:31, Brennan Vincent wrote:
> Various code seems to expect "default" to be the /last/ item in the list
> returned by that function, not the first. For example, this comment in faces.el:
> 
>   ;; The `reverse' is so that `default' goes first.
>   (dolist (face (nreverse (face-list)))
> 
> 
> Also, org-html-htmlize-generate-css does not work when default comes first in
> the list (as it skips processing all fonts after default).
> 
> I am not sure why this was changed and if the change was intentional, but it can
> be fixed by changing the "<" to a ">" in the last line of face-list, so I
> suspect it might have been a mistake.
> 
> diff --git lisp/faces.el lisp/faces.el
> index 4933b495a6c..e998dc504e5 100644
> --- lisp/faces.el
> +++ lisp/faces.el
> @@ -199,7 +199,7 @@ face-list
>      (maphash (lambda (face spec)
>                 (push `(,(car spec) . ,face) faces))
>               face--new-frame-defaults)
> -    (mapcar #'cdr (sort faces (lambda (f1 f2) (< (car f1) (car f2)))))))
> +    (mapcar #'cdr (sort faces (lambda (f1 f2) (> (car f1) (car f2)))))))
> 
>  (defun make-face (face)
>    "Define a new face with name FACE, a symbol.

bug#61521: "default" is now the first item returned from (font-faces), breaking various code.

[Gregory Heytings]

>
> Various code seems to expect "default" to be the /last/ item in the list 
> returned by that function, not the first. For example, this comment in 
> faces.el:
>

Can you perhaps clarify what you mean by "that function"?  The subject 
line of your bug report mentions 'font-faces', but no such function exists 
in Emacs.

bug#61521: "default" is now the first item returned from (font-faces), breaking various code.

[Brennan Vincent]

On 2023-02-14 20:00, Gregory Heytings wrote:
> 
>>
>> Various code seems to expect "default" to be the /last/ item in the list
>> returned by that function, not the first. For example, this comment in faces.el:
>>
> 
> Can you perhaps clarify what you mean by "that function"?  The subject line of
> your bug report mentions 'font-faces', but no such function exists in Emacs.
> 

Apologies: I meant to write "face-list".

bug#61521: "default" is now the first item returned from (font-faces), breaking various code.

[Gregory Heytings]

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


>>> Various code seems to expect "default" to be the /last/ item in the 
>>> list returned by that function, not the first. For example, this 
>>> comment in faces.el:
>>
>> Can you perhaps clarify what you mean by "that function"?  The subject 
>> line of your bug report mentions 'font-faces', but no such function 
>> exists in Emacs.
>
> Apologies: I meant to write "face-list".
>

Thanks.  It seems the change you describe is not a recent one: the first 
element of the list returned by 'face-list' is 'default' in Emacs 27, 28, 
29 and 30.  (This is caused by e3b8ddd500, since which frame faces are 
stored in a hash table instead of an alist.)

Given this, and the fact that the docstring of 'face-list' does not 
specify the order in which the faces are returned, it's not clear to me 
that there is a bug here.  Code that assumes a given order should probably 
be fixed.

bug#61521: "default" is now the first item returned from (font-faces), breaking various code.

[Eli Zaretskii]

> Cc: 61521@debbugs.gnu.org
> Date: Wed, 15 Feb 2023 09:00:44 +0000
> From: Gregory Heytings <gregory@heytings.org>
> 
> Thanks.  It seems the change you describe is not a recent one: the first 
> element of the list returned by 'face-list' is 'default' in Emacs 27, 28, 
> 29 and 30.  (This is caused by e3b8ddd500, since which frame faces are 
> stored in a hash table instead of an alist.)

Right.  So I wonder whether we should remove the nreverse call in
face-set-after-frame-default.  WDYT?

> Given this, and the fact that the docstring of 'face-list' does not 
> specify the order in which the faces are returned, it's not clear to me 
> that there is a bug here.  Code that assumes a given order should probably 
> be fixed.

I agree.

bug#61521: "default" is now the first item returned from (font-faces), breaking various code.

[Gregory Heytings]

>> Thanks.  It seems the change you describe is not a recent one: the 
>> first element of the list returned by 'face-list' is 'default' in Emacs 
>> 27, 28, 29 and 30.  (This is caused by e3b8ddd500, since which frame 
>> faces are stored in a hash table instead of an alist.)
>
> Right.  So I wonder whether we should remove the nreverse call in 
> face-set-after-frame-default.  WDYT?
>

There are three occurrences of '(nreverse (face-list))': one in 
facemenu-complete-face-list, which seems to date from the 1990s, one in 
x-create-frame-with-faces, which was added by e3b8ddd500 "to handle subtle 
semantic change to how frame faces propagate, which otherwise breaks frame 
creation with reverse video enabled (bug#41200)", and the third one in 
'face-set-after-frame-default'.

The comment there is definitely outdated and should be removed, but given 
that '(nreverse (face-list))' is placed in a dolist whose body starts with 
'(face-spec-recalc face frame)', like in x-create-frame-with-faces, I'm 
not sure the nreverse can be removed without introducing a subtle bug. 
It is probably safer to leave the code unchanged.

bug#61521: "default" is now the first item returned from (font-faces), breaking various code.

[Kai Ma]

Gregory Heytings <gregory@heytings.org> writes:

>>> Thanks.  It seems the change you describe is not a recent one: the
>>> first element of the list returned by 'face-list' is 'default' in
>>> Emacs 27, 28, 29 and 30.  (This is caused by e3b8ddd500, since
>>> which frame faces are stored in a hash table instead of an alist.)
>>
>> Right.  So I wonder whether we should remove the nreverse call in
>> face-set-after-frame-default.  WDYT?
>>
>
> There are three occurrences of '(nreverse (face-list))': one in
> facemenu-complete-face-list, which seems to date from the 1990s, one
> in x-create-frame-with-faces, which was added by e3b8ddd500 "to handle
> subtle semantic change to how frame faces propagate, which otherwise
> breaks frame creation with reverse video enabled (bug#41200)", and the
> third one in 'face-set-after-frame-default'.
>
> The comment there is definitely outdated and should be removed, but
> given that '(nreverse (face-list))' is placed in a dolist whose body
> starts with '(face-spec-recalc face frame)', like in
> x-create-frame-with-faces, I'm not sure the nreverse can be removed
> without introducing a subtle bug. It is probably safer to leave the
> code unchanged.

My config becomes broken after pulling this change: the child frame of
vertico-posframe does not appear under certain themes, and signals
errors like:

  Error in post-command-hook (vertico--exhibit): (error "Invalid face" consult-separator)
  Error in post-command-hook (vertico--exhibit): (error "Invalid face" hl-todo)

I'm not sure if this is a downstream issue, but this problem can be
solved by either reverting this commit or removing the nreverse in
x-create-frame-with-faces:

diff --git a/lisp/faces.el b/lisp/faces.el
index 4933b495a6c..e91107e98cc 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -2226,7 +2226,7 @@ x-create-frame-with-faces
     (unwind-protect
 	(progn
 	  (x-setup-function-keys frame)
-	  (dolist (face (nreverse (face-list)))
+	  (dolist (face (face-list)))
 	    (face-spec-recalc face frame))
 	  (x-handle-reverse-video frame parameters)
 	  (frame-set-background-mode frame t)

This piece of code (w/ nreverse) was written as part of the hash table
rewrite, and at that time (face-list) did not sort its results.  I don't
know why nreverse is significant here though.

bug#61521: "default" is now the first item returned from (font-faces), breaking various code.

[Eli Zaretskii]

> From: Kai Ma <justksqsf@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  brennan@umanwizard.com,
>   61521@debbugs.gnu.org
> Date: Sat, 18 Feb 2023 06:17:25 +0800
> 
> My config becomes broken after pulling this change: the child frame of
> vertico-posframe does not appear under certain themes, and signals
> errors like:
> 
>   Error in post-command-hook (vertico--exhibit): (error "Invalid face" consult-separator)
>   Error in post-command-hook (vertico--exhibit): (error "Invalid face" hl-todo)
> 
> I'm not sure if this is a downstream issue, but this problem can be
> solved by either reverting this commit or removing the nreverse in
> x-create-frame-with-faces:
> 
> diff --git a/lisp/faces.el b/lisp/faces.el
> index 4933b495a6c..e91107e98cc 100644
> --- a/lisp/faces.el
> +++ b/lisp/faces.el
> @@ -2226,7 +2226,7 @@ x-create-frame-with-faces
>      (unwind-protect
>  	(progn
>  	  (x-setup-function-keys frame)
> -	  (dolist (face (nreverse (face-list)))
> +	  (dolist (face (face-list)))
>  	    (face-spec-recalc face frame))
>  	  (x-handle-reverse-video frame parameters)
>  	  (frame-set-background-mode frame t)
> 
> This piece of code (w/ nreverse) was written as part of the hash table
> rewrite, and at that time (face-list) did not sort its results.  I don't
> know why nreverse is significant here though.

Which change are you talking about?  Please clarify.

The original problem was solved recently on the emacs-29 branch by
reversing the order in which face-list sorts the faces, to make it
similar to what we had before the hash table rewrite.  So there should
be no reason to change any other code anywhere else.

Thanks.

bug#61521: "default" is now the first item returned from (font-faces), breaking various code.

[Kai Ma]

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



> On Feb 18, 2023, at 14:49, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Kai Ma <justksqsf@gmail.com <mailto:justksqsf@gmail.com>>
>> Cc: Eli Zaretskii <eliz@gnu.org <mailto:eliz@gnu.org>>,  brennan@umanwizard.com <mailto:brennan@umanwizard.com>,
>>  61521@debbugs.gnu.org <mailto:61521@debbugs.gnu.org>
>> Date: Sat, 18 Feb 2023 06:17:25 +0800
>> 
>> My config becomes broken after pulling this change: the child frame of
>> vertico-posframe does not appear under certain themes, and signals
>> errors like:
>> 
>>  Error in post-command-hook (vertico--exhibit): (error "Invalid face" consult-separator)
>>  Error in post-command-hook (vertico--exhibit): (error "Invalid face" hl-todo)
>> 
>> I'm not sure if this is a downstream issue, but this problem can be
>> solved by either reverting this commit or removing the nreverse in
>> x-create-frame-with-faces:
>> 
>> diff --git a/lisp/faces.el b/lisp/faces.el
>> index 4933b495a6c..e91107e98cc 100644
>> --- a/lisp/faces.el
>> +++ b/lisp/faces.el
>> @@ -2226,7 +2226,7 @@ x-create-frame-with-faces
>>     (unwind-protect
>> 	(progn
>> 	  (x-setup-function-keys frame)
>> -	  (dolist (face (nreverse (face-list)))
>> +	  (dolist (face (face-list)))
>> 	    (face-spec-recalc face frame))
>> 	  (x-handle-reverse-video frame parameters)
>> 	  (frame-set-background-mode frame t)
>> 
>> This piece of code (w/ nreverse) was written as part of the hash table
>> rewrite, and at that time (face-list) did not sort its results.  I don't
>> know why nreverse is significant here though.
> 
> Which change are you talking about?  Please clarify.

The fix of this bug, commit a555abc56d5 on branch emacs-29.

> The original problem was solved recently on the emacs-29 branch by
> reversing the order in which face-list sorts the faces, to make it
> similar to what we had before the hash table rewrite.  So there should
> be no reason to change any other code anywhere else.
> 
> Thanks.


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

bug#61521: "default" is now the first item returned from (font-faces), breaking various code.

[Eli Zaretskii]

> From: Kai Ma <justksqsf@gmail.com>
> Date: Sat, 18 Feb 2023 14:54:12 +0800
> Cc: gregory@heytings.org,
>  brennan@umanwizard.com,
>  61521@debbugs.gnu.org
> 
>  Which change are you talking about?  Please clarify.
> 
> The fix of this bug, commit a555abc56d5 on branch emacs-29.

Ah, thanks.  So I've now removed the call to nreverse from
x-create-frame-with-faces, as you suggested.  I agree that the call to
nreverse was probably added as part of the hash table rewrite due to
the wrong order returned by face-list back then.

bug#61521: "default" is now the first item returned from (font-faces), breaking various code.

[Eli Zaretskii]

> Date: Tue, 14 Feb 2023 19:31:30 -0500
> From: Brennan Vincent <brennan@umanwizard.com>
> 
> Various code seems to expect "default" to be the /last/ item in the list
> returned by that function, not the first. For example, this comment in faces.el:
> 
>   ;; The `reverse' is so that `default' goes first.
>   (dolist (face (nreverse (face-list)))

That comment is obsolete and needs to be changed (and the call to
nreverse should perhaps be removed).

> Also, org-html-htmlize-generate-css does not work when default comes first in
> the list (as it skips processing all fonts after default).

Isn't that a bug in that Org function?  It shouldn't rely on any
particular order.

bug#61521: "default" is now the first item returned from (font-faces), breaking various code.

[Brennan Vincent]

> On Feb 15, 2023, at 07:58, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> 
>> 
>> Date: Tue, 14 Feb 2023 19:31:30 -0500
>> From: Brennan Vincent <brennan@umanwizard.com>
>> 
>> Various code seems to expect "default" to be the /last/ item in the list
>> returned by that function, not the first. For example, this comment in faces.el:
>> 
>>  ;; The `reverse' is so that `default' goes first.
>>  (dolist (face (nreverse (face-list)))
> 
> That comment is obsolete and needs to be changed (and the call to
> nreverse should perhaps be removed).

If the order returned by face-list is not guaranteed, then why does it do sorting at all?

>> Also, org-html-htmlize-generate-css does not work when default comes first in
>> the list (as it skips processing all fonts after default).
> 
> Isn't that a bug in that Org function?  It shouldn't rely on any
> particular order.

In that case, we can consider this bug report to relate to the broken Org function, which is how I initially discovered the face-list issue.

bug#61521: "default" is now the first item returned from (font-faces), breaking various code.

[Gregory Heytings]

>> That comment is obsolete and needs to be changed (and the call to 
>> nreverse should perhaps be removed).
>
> If the order returned by face-list is not guaranteed, then why does it 
> do sorting at all?
>

Note that the sorting was added recently, in e3b8ddd500.  Before that 
there was no sorting, it just happened that 'default' was the last element 
of that alist.  It's not clear to me why the sorting is there, apparently 
it was added to fix bug#41200.

bug#61521: "default" is now the first item returned from (font-faces), breaking various code.

[Eli Zaretskii]

> From: Brennan Vincent <brennan@umanwizard.com>
> Date: Wed, 15 Feb 2023 09:01:31 -0500
> Cc: 61521@debbugs.gnu.org
> 
> > On Feb 15, 2023, at 07:58, Eli Zaretskii <eliz@gnu.org> wrote:
> > 
> > 
> >> 
> >> Date: Tue, 14 Feb 2023 19:31:30 -0500
> >> From: Brennan Vincent <brennan@umanwizard.com>
> >> 
> >> Various code seems to expect "default" to be the /last/ item in the list
> >> returned by that function, not the first. For example, this comment in faces.el:
> >> 
> >>  ;; The `reverse' is so that `default' goes first.
> >>  (dolist (face (nreverse (face-list)))
> > 
> > That comment is obsolete and needs to be changed (and the call to
> > nreverse should perhaps be removed).
> 
> If the order returned by face-list is not guaranteed, then why does it do sorting at all?

Good question.  AFAICT, the sorting was added when we switched from
storing faces in alists to storing them in hash tables.  It probably
sorted faces to be more compatible with what face-list returned before
the switch to hash table.  So I suspect the order we have now is
simply a bug, and we do need to change the order of sorting to get
back the original order.

Gregory, any counter-arguments?

bug#61521: "default" is now the first item returned from (font-faces), breaking various code.

[Brennan Vincent]

On 2023-02-15 09:24, Eli Zaretskii wrote:
>> From: Brennan Vincent <brennan@umanwizard.com>
>> Date: Wed, 15 Feb 2023 09:01:31 -0500
>> Cc: 61521@debbugs.gnu.org
>>
>>> On Feb 15, 2023, at 07:58, Eli Zaretskii <eliz@gnu.org> wrote:
>>>
>>> 
>>>>
>>>> Date: Tue, 14 Feb 2023 19:31:30 -0500
>>>> From: Brennan Vincent <brennan@umanwizard.com>
>>>>
>>>> Various code seems to expect "default" to be the /last/ item in the list
>>>> returned by that function, not the first. For example, this comment in faces.el:
>>>>
>>>>  ;; The `reverse' is so that `default' goes first.
>>>>  (dolist (face (nreverse (face-list)))
>>>
>>> That comment is obsolete and needs to be changed (and the call to
>>> nreverse should perhaps be removed).
>>
>> If the order returned by face-list is not guaranteed, then why does it do sorting at all?
> 
> Good question.  AFAICT, the sorting was added when we switched from
> storing faces in alists to storing them in hash tables.  It probably
> sorted faces to be more compatible with what face-list returned before
> the switch to hash table.  So I suspect the order we have now is
> simply a bug, and we do need to change the order of sorting to get
> back the original order.

I tend to agree. Sorry for not explaining this reasoning more fully in my
original message.

Here's what I suspect happened (not 100% sure, it's just a theory):

(1) Initially set of faces was stored as a list, so it was naturally maintained
in the inverse order that things were added to it (thus default would be at the
end).

(2) Now faces are stored in a hash table whose key is the face and whose value
contains various pieces of data, including the face ID.

(3) This face ID is allocated in increasing order (see e.g. this code in xfaces.c:
      Lisp_Object face_id = make_fixnum (next_lface_id);
      lface_id_to_name[next_lface_id] = face;
      Fput (face, Qface, face_id);
      ++next_lface_id;

(4) Thus, `face-list` and `frame-face-alist` sorted the faces by face ID in
order to maintain the old ordering behavior. However, the author accidentally
inverted the comparison when doing so.


> Gregory, any counter-arguments?

bug#61521: "default" is now the first item returned from (font-faces), breaking various code.

[Eli Zaretskii]

> Date: Wed, 15 Feb 2023 11:19:54 -0500
> Cc: 61521@debbugs.gnu.org
> From: Brennan Vincent <brennan@umanwizard.com>
> 
> > Good question.  AFAICT, the sorting was added when we switched from
> > storing faces in alists to storing them in hash tables.  It probably
> > sorted faces to be more compatible with what face-list returned before
> > the switch to hash table.  So I suspect the order we have now is
> > simply a bug, and we do need to change the order of sorting to get
> > back the original order.
> 
> I tend to agree. Sorry for not explaining this reasoning more fully in my
> original message.
> 
> Here's what I suspect happened (not 100% sure, it's just a theory):
> 
> (1) Initially set of faces was stored as a list, so it was naturally maintained
> in the inverse order that things were added to it (thus default would be at the
> end).
> 
> (2) Now faces are stored in a hash table whose key is the face and whose value
> contains various pieces of data, including the face ID.
> 
> (3) This face ID is allocated in increasing order (see e.g. this code in xfaces.c:
>       Lisp_Object face_id = make_fixnum (next_lface_id);
>       lface_id_to_name[next_lface_id] = face;
>       Fput (face, Qface, face_id);
>       ++next_lface_id;
> 
> (4) Thus, `face-list` and `frame-face-alist` sorted the faces by face ID in
> order to maintain the old ordering behavior. However, the author accidentally
> inverted the comparison when doing so.
> 
> 
> > Gregory, any counter-arguments?

No further comments, so I've now installed the proposed changes on the
emacs-29 branch, and I'm boldly closing this bug as done.