bug#58312: Improve “Invalid face attribute” error message

bug#58312: Improve “Invalid face attribute” error message

[Felix Dietrich]

Package: emacs
Severity: wishlist

Could the log message “Invalid face attribute” created by
“xfaces.c:merge_face_ref” be extended to also include the buffer or
other identifying information about its origin?  This request is
motivated by a recent issue posted to the help-gnu-emacs list [1].

When Emacs encounters an anonymous face with an invalid face attribute
value in a buffer, it will write a log entry to the “*Messages*” buffer.
This message takes the form “Invalid face attribute ATTRIBUTE VALUE” and
lacks any information that would allow easy identification of the
invalid faceʼs place of origin.  The log entry is induced by the
function “xfaces.c:merge_face_ref”.  This function takes as its first
argument a “struct window” which could potentially be used to retrieve
the causing buffer using its “contents” member (note, though, that it
may be NULL).

Here is a snippet that will cause the “Invalid face attribute” message
to be written to the “*Messages*” buffer when evaluated:

#+begin_src emacs-lisp
  (let ((buf (generate-new-buffer "*test*"))
        (s (propertize "Hello World" 'face '(:foreground nil))))
    (with-current-buffer buf
      (insert s))
    (display-buffer buf))
#+end_src

#+begin_example
#<window 54 on *test*<2>>Invalid face attribute :foreground nil
Invalid face attribute :foreground nil
#+end_example

Two things I noticed exploring the issue:

1. the value I use in the above example for the :foreground attribute
nil is handled specially in “xfaces.c:internal-set-lisp-face-attribute”
(accompanied by a comment stating “Compatibility with 20.x.”): it is
changed to ‘unspecified’, but in “xfaces.c:merge_face_ref” nil is
considered an error;

2. there is a newline missing between the log entries for the result of
the evaluation and the error entries in the “*Messages*” buffer.  This
is bug #58311 <https://debbugs.gnu.org/cgi/bugreport.cgi?bug=58311>.


Footnotes:
[1]  From: Eric S Fraga
     Subject: how to track down "invalid face attribute" errors?
     Date: Sun, 25 Sep 2022 13:03:03 +0100
     Message-ID: <87k05rr79k.fsf@ucl.ac.uk>

     <https://lists.gnu.org/archive/html/help-gnu-emacs/2022-09/msg00415.html>

-- 
Felix Dietrich

bug#58312: Improve “Invalid face attribute” error message

[Lars Ingebrigtsen]

Felix Dietrich <felix.dietrich@sperrhaken.name> writes:

> Could the log message “Invalid face attribute” created by
> “xfaces.c:merge_face_ref” be extended to also include the buffer or
> other identifying information about its origin?

Perhaps include the window name?  That would be less ambiguous than the
buffer name.

bug#58312: Improve “Invalid face attribute” error message

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Felix Dietrich <felix.dietrich@sperrhaken.name> writes:
>
>> Could the log message “Invalid face attribute” created by
>> “xfaces.c:merge_face_ref” be extended to also include the buffer or
>> other identifying information about its origin?
>
> Perhaps include the window name?  That would be less ambiguous than the
> buffer name.

That makes sense to me.  Eli or Po Lu, WDYT about this change?

bug#58312: Improve “Invalid face attribute” error message

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Wed, 10 Jan 2024 02:50:46 -0800
> Cc: Felix Dietrich <felix.dietrich@sperrhaken.name>, 58312@debbugs.gnu.org, eliz@gnu.org, 
> 	Po Lu <luangruo@yahoo.com>
> 
> Lars Ingebrigtsen <larsi@gnus.org> writes:
> 
> > Felix Dietrich <felix.dietrich@sperrhaken.name> writes:
> >
> >> Could the log message “Invalid face attribute” created by
> >> “xfaces.c:merge_face_ref” be extended to also include the buffer or
> >> other identifying information about its origin?
> >
> > Perhaps include the window name?  That would be less ambiguous than the
> > buffer name.
> 
> That makes sense to me.  Eli or Po Lu, WDYT about this change?

Patches welcome, but please take into account the fact that the window
is not always known there: merge_face_ref can be called with its first
argument NULL.