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

[Gregory Heytings <gregory@heytings.org>]

>> 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;