bug#53636: 29.0.50; face-remapping broken on master

[Eli Zaretskii]

> Date: Wed, 09 Feb 2022 15:57:14 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 53636@debbugs.gnu.org, tsdh@gnu.org
> 
> > From: Lars Ingebrigtsen <larsi@gnus.org>
> > Cc: 53636@debbugs.gnu.org,  tsdh@gnu.org
> > Date: Wed, 09 Feb 2022 09:01:27 +0100
> > 
> > Eli Zaretskii <eliz@gnu.org> writes:
> > 
> > > So: in the scenario you've shown, do we want *scratch* to have its
> > > mode-line remapped on the new frame, or don't we?  IOW, I agree that
> > > the result ideally shouldn't depend on the buffer where make-frame is
> > > called, but I need to know what is the desired behavior in order to
> > > find code which produces the undesired results.
> > 
> > Yes, I don't think that the results in the new frame should depend on
> > the current buffer when `make-frame' is called.
> 
> OK, I think I know where this happens.  Let me dig a bit.

After digging into this, I'm sorry to say that this is unworkable: if
a basic face inherits from some other face, and that other face is
remapped via a buffer-local value of face-remapping-alist, the result
will be unpredictable.  For a simple demonstration of the
unpredictable nature of the result, try this simple variation of your
recipe, which just adds the last step:

  . emacs -Q
  . M-: (progn (face-remap-add-relative 'mode-line 'link-visited) (make-frame)) 
RET
  . C-x C-f src/xdisp.c RET

Now all the buffers on the new frame have the mode line rendered with
the default attributes, whereas all the buffers on the old frame
suddenly get the mode line rendered with the link-visited face.  IOW,
just visiting a file on the new frame causes the mode line to be
displayed differently than it was before you visit that file.

This is due to how we handle the basic faces.  There's "face
realization" and "face look up".  The former means we recompute all
the face's attributes and load the necessary GUI resources (colors,
fonts, etc.) from scratch; the latter means we just look up in the
frame's face cache the attributes that were already computed.  Face
realization is obviously much more expensive than look up, so we only
realize the basic faces when the frame's face cache is empty.  A new
frame has its cache empty, but there are other situations when the
face cache could become empty: for example, when attributes of a named
face change.  So fundamentally, whether using a basic face will go
through "face realization" or just "face look up" is unpredictable and
can happen at any moment.

Here is the crucial point: face inheritance is only considered in
"face realization", not in "face look up".  (That is why we empty the
face cache when a named face is modified in the first place.)  So if a
basic face inherits from another face, and that other face is
remapped, the effect of the remapping will make the result of "face
realization" different from "face look up", although no face has
changed the attributes.  And since it is unpredictable when Emacs will
use "face realization" and when it will settle with "face look up",
you get unpredictable results.  Moreover, Emacs expects a remapped
face to have a different face ID internally (the ID is just the index
of the remapped face in the frame's face cache).  But "face
realization" when a parent face is remapped doesn't yield a new face
ID, it just influences the attributes recorded under the original face
ID.  The bottom line is that if "face realization" is called when the
current buffer happens to be one with non-nil face-remapping-alist,
the default face ID will use the attributes of the remapped face, but
Emacs will not know that the face was effectively remapped.  And that
is the root cause of what you see in your recipes.

Contrast this with a perfectly correct and expected behavior with
these slightly modified recipes:

  (progn
    (face-remap-add-relative 'mode-line-active 'link-visited)
    (make-frame))

  (progn
    (face-remap-add-relative 'mode-line-active 'link-visited)
    (switch-to-buffer "*Messages*")
    (make-frame))

Now the only mode line that is affected is the one of the *scratch*
buffer (when it is the current buffer), no matter how many other files
you visit after creating the new frame, and no matter which frame
displays what buffer.

So I can just reiterate what I said up-thread: basic faces cannot
inherit from other faces, if we want to support remapping of those
parent faces.  Our internal infrastructure for handling the basic
faces simply doesn't support this.