bug#59347: 29.0.50; `:family` face setting ignored

[Gregory Heytings <gregory@heytings.org>]

Amazing.  I thought this exhausting discussion was over, but no, Po Lu 
came and "improved" the code that was agreed upon only a couple of hours 
after it was pushed, disregarding this entire discussion, the docstring of 
the variable that the commit introduced, the commit message, and without 
asking any questions.  How can that be right, how can that be acceptable?

Po Lu's commit said "there is no reason any user should have to think 
about bitmasks" even though the docstring said "there is no reason to 
change that value except for debugging purposes."  (Of course, that 
sentence was also removed from the "improved" version of the docstring.) 
It is on purpose that that variable was a bitmask and not a list, it is on 
purpose that that variable was used in a macro and not in a function: 
there is no reason that each face realization in each Emacs instance 
should spend unnecessary CPU cycles (a function call and traversing a 
list) on a variable that should never (or hardly ever) be changed, and 
that was introduced only to help debugging other potential problems in 
that subtle area of Emacs' code.

The name of the variable was changed, and the docstring was "improved", 
and became completely wrong.  It is simply untrue that this is a list of 
attributes that Emacs will "ignore when realizing a face", or in fact that 
this changes anything fundamental in the way Emacs realizes faces.

Here is again, in every possible detail, but this time in a single post, 
the analysis of this subtle bug, and the rationale of the patch that was 
agreed upon.  I explain this bug with an concrete example, with the 
'variable-pitch' face and a font with a 'medium' weight.  That example is 
only meant to illustrate the bug: the same reasoning applies for other 
faces and other font attributes (slant or width).

(1) The first (and perhaps most important) thing to note is that, before 
or after this bug fix,

(set-face-attribute 'variable-pitch nil :weight 'medium)

does _not_ change the font that is used for the 'variable-pitch' face, 
unless the font that Emacs would have selected for the 'variable-pitch' 
face without that call to set-face-attribute happens too have a 'medium' 
variant.  Fonts with an explicit 'medium' variant are rare, so most likely 
(that is, on most computers) after this call to set-face-attribute, the 
weight of the font for the 'variable-pitch' face will still be 'regular' 
(or 'normal').  IOW, when the weight/slant/width attribute of a face is 
set, Emacs tries to find a good match among the available fonts, and does 
not consider (and never considered) these attributes as strict 
requirements, _even if they are explicitly specified in the face itself_. 
Clearly, that should remain the case when these attributes are 
_unspecified_ in a face.

(2) Now suppose that a user has, in their init file, the following line:

(set-face-attribute 'default nil :font "Source Code Pro" :weight 'medium)

That "Source Code Pro" font has more variants than most fonts, and in 
particular it has a variant with a 'medium' weight, which is slightly 
thicker than the 'regular' weight (the difference is probably only visible 
on HiDPI screens).  That apparently anecdotal detail is important: it 
implies that the weight of the default face is set to 'medium' (see 
below).

The 'variable-pitch' face is defined as follows in emacs -Q:

Family: Sans Serif
Foundry: unspecified
Width: unspecified
Height: unspecified
Weight: unspecified
Slant: unspecified

Note that, apart from the family, all other attributes are unspecified in 
that face (which means implicitly that it should be similar, with respect 
to its width/height/weight/slant, to the default face).

What happens when a buffer with a text with the 'variable-pitch' face is 
displayed is this:

(2.1) face_at_buffer_position is called, and finds (with 
Fget_text_property) that the face is 'variable-pitch'.  A few lines below, 
the following code:

/* Begin with attributes from the default face.  */
memcpy (attrs, default_face->lface, sizeof(attrs));

copies the attributes of the default face into the Lisp_Object attrs. The 
last statement of that function is:

return lookup_face (f, attrs);

which means that lookup_face is called with 'attrs' set the attributes of 
the default face, merged with the attributes of the 'variable-pitch' face 
which are, except for the family, unspecified. At that point, 'attrs' also 
contains, in its 'font' slot, a font-spec corresponding to the default 
face.  Therefore lookup_face is called with attrs[weight] = 'medium' and 
attrs[font][weight] = 'medium'. (Note that we now have _two_ copies of the 
same information ("the weight of the default face is 'medium'") in the 
same Lisp_Object.)

(2.2) lookup_face calls realize_face without changing its 'attr' 
parameter, which means that it calls realize_face with attr[weight] = 
'medium' and attrs[font][weight] = 'medium'.

(2.3) realize_face calls realize_gui_face without changing its 'attrs' 
parameter, which means that it calls realize_gui_face with attrs[weight] = 
'medium' and attrs[font][weight] = 'medium'.

(2.4) In realize_gui_face, the conditional 'if (! FONT_OBJECT_P 
(attrs[LFACE_FONT_INDEX]))' is taken, because the 'font' slot of 'attrs' 
contains a font-spec, not a font object (see 2.1).

(2.5) realize_gui_face calls copy_font_spec, which copies the font-spec 
from the 'font' slot of 'attrs' into the Lisp_Object 'spec'. Therefore 
spec[weight] = 'medium'.  I'm explaining the bug, so for now I suppose 
that the code that unsets weight in 'spec' isn't present.

(2.6) realize_gui_face calls font_load_for_lface, with 'attrs' (in which 
attrs[weight] = 'medium' and attrs[font][weight] = 'medium'), and with 
'spec' (in which spec[weight] = 'medium').  (Yes, there are now _three_ 
copies of the same information.)

(2.7) font_load_for_lface calls font_find_for_lface, with 'attrs' and 
'spec' unmodified.  Therefore attrs[weight] = 'medium', 
attrs[font][weight] = 'medium', and spec[weight] = 'medium'.

(2.8) font_find_for_lface calls copy_font_spec, and puts a copy of the 
font-spec 'spec' into the Lisp_Object 'work'.  The weight slot of 'work' 
is not modified in font_find_for_lface.

(2.9) font_find_for_lface calls (in the final loop) font_list_entities 
with 'work'.  Therefore font_list_entities is called with work[weight] = 
'medium'.  And, given that the 'variable-pitch' face is being realized, we 
also have work[family] = Sans Serif.

(2.10) font_list_entities checks the weight slot of its 'spec' parameter, 
it is set to 'medium', therefore need_filtering is set to true. 
font_list_entities also populates the Lisp_Object 'scratch_font_spec' with 
some of the attributes coming from its 'spec' parameter, in particular the 
family.  Note also that the weight slot of 'scratch_font_spec' is set to 
nil.  Therefore the 'list' function of the font driver is called with 
scratch_font_spec[family] = Sans Serif and scratch_font_spec[weight] = 
nil.  This returns a list of candidate fonts from the Sans Serif family, 
of any weight (given that scratch_font_spec[weight] = nil).

(2.11) needs_filtering is true, therefore font_delete_unmatched is called 
with 'spec' (remember that spec[weight] = 'medium').  This removes all 
fonts, from the list returned by the 'list' function, that do not have an 
explicit 'medium' variant.  As noted above, fonts with a 'medium' variant 
are comparatively rare.  On computers on which there are no fonts in the 
Sans Serif family with an explicit 'medium' variant, font_delete_unmatch 
returns an _empty_ list.  Therefore font_list_entities returns an _empty_ 
list, even if there are fonts on the computer with a weight that is close 
to the 'medium' weight, such as a 'regular' weight (see weight_table).

(2.12) Therefore, in font_find_for_lface, the call to font_select_entity, 
which is supposed to "select the best match for PIXEL_SIZE and attributes 
in ATTRS if there are several candidates", is bypassed.  Remember that 
attrs[weight] = 'medium', so if font_list_entities had for example 
returned a list with one or more fonts in the Sans Serif family with a 
'regular' weight, one of them would have been selected as the best match, 
because attrs[weight] = 'medium'.

(2.13) The fact that attrs[weight] = 'medium' (and attrs[font][weight] = 
'medium') in font_find_for_lface shows clearly that the 'medium' weight is 
not at all "ignored" by Emacs when realizing the 'variable-pitch' face. 
It is (or rather, should be) used to find a font that is similar, with 
respect to its width/height/weight/slant, to the font of the default face. 
Note that this is exactly what happens in case (1) above: when the weight 
of the font of the default face is not 'medium', setting the weight of the 
'variable-pitch' face to 'medium' does _not_ change the font of the 
'variable-pitch' face, instead Emacs selects the best possible match among 
the available fonts.

(2.14) Therefore font_find_for_lface tries to relax the 
family/foundry/registry/adstyle attributes in 'work', until it finds a 
font with a weight = 'medium'.  All other fonts are rejected, which is 
clearly wrong: the 'variable-pitch' face that is being realized does _not_ 
specify any weight.  It is only a coincidence that the 'default' face has 
a 'medium' variant, and that none of the available fonts in the Sans Serif 
family have a 'medium' variant.

(2.15) In the case that triggered this bug, a font that is not in the Sans 
Serif family but that happens to have a 'medium' variant is selected by 
Emacs, such as a non-antialiased font, or a monospace font.  In the worst 
of the worst cases, even after trying to relax all 
family/foundry/registry/adstyle attributes, no fonts with a 'medium' 
variant has been found, and the 'variable-pitch' face becomes identical to 
the default face.

With the bug fix, in (2.5), spec[weight] is set to nil.  Therefore in 
(2.10), need_filtering remains false, in (2.11) font_delete_unmatch is not 
called and font_list_entities does not return an empty list, but instead 
returns (as it should) a list of candidate fonts in the Sans Serif family, 
and in (2.12) font_select_entity does the job it is supposed to do, and 
selects the best candidate among those fonts and selects the font that is 
as close as possible to attrs[weight] = 'medium'.  Which is again exactly 
what happens in case (1) above, when the height of the default face does 
not happen to be 'medium'.