bug#57499: Documentation bug in the docstring of set-face-attribute?

[Gregory Heytings]

> > So this call is already included in the previous one.  Why should we 
> > tell users to add such a redundant call in their code?
>
> The new text doesn't say the call with FRAME = t should be an additional 
> call.

It doesn't say so explicitly indeed, but without reading the code of 
set-face-attribute everyone understands that it should be an additional 
call.

> > As far as I understand, the actual and real problem here is some users 
> > use nil when they should use 'unspecified, because they are not aware 
> > that nil and 'unspecified are subtly different.  The subtle difference 
> > is that using nil works for frame = #<frame-1> ... #<frame-n>, but does 
> > not work with frame = t.
>
> That is a backward-compatibility feature that I don't want to mention in 
> the doc string.  Lisp programs should only use valid values that are 
> documented in the doc string.

I wasn't suggesting to mention this.  I was suggesting to add a mention 
that the symbol 'unspecified should be used for the value `unspecified', 
which might be clear to you and me but is evidently not clear for users.

> I've installed the last text I proposed, and I'm closing this bug with 
> that.

Would the patch below be okay?  This would be another way to clarify the 
matter.

diff --git a/src/xfaces.c b/src/xfaces.c
index 70d5cbeb4c..2dfba51f74 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -3390,6 +3390,8 @@ DEFUN ("internal-set-lisp-face-attribute", 
Finternal_set_lisp_face_attribute,
     }
   else if (EQ (attr, QCforeground))
     {
+      if (NILP (value) && EQ (frame, Qt))
+       signal_error ("Invalid foreground face attribute value", value);
       /* Compatibility with 20.x.  */
       if (NILP (value))
        value = Qunspecified;
@@ -3410,6 +3412,8 @@ DEFUN ("internal-set-lisp-face-attribute", 
Finternal_set_lisp_face_attribute,
   else if (EQ (attr, QCdistant_foreground))
     {
       /* Compatibility with 20.x.  */
+      if (NILP (value) && EQ (frame, Qt))
+       signal_error ("Invalid distant-foreground face attribute value", value);
       if (NILP (value))
        value = Qunspecified;
       if (!UNSPECIFIEDP (value)
@@ -3428,6 +3432,8 @@ DEFUN ("internal-set-lisp-face-attribute", 
Finternal_set_lisp_face_attribute,
     }
   else if (EQ (attr, QCbackground))
     {
+      if (NILP (value) && EQ (frame, Qt))
+       signal_error ("Invalid background face attribute value", value);
       /* Compatibility with 20.x.  */
       if (NILP (value))
        value = Qunspecified;