bug#21695: 25.0.50; Change most occurrences of `setq' in Emacs manual to `customize-set-variable'? Really?

[Gregory Heytings <gregory@heytings.org>]

[-- Attachment #1: Type: text/plain, Size: 1886 bytes --]


>> +** 'setq' displays a warning when 'customize-set-variable' should have been used.
>> +Some custom variables need to be set with 'customize-set-variable', because
>> +they were designed to be set through the Customization interface and have a
>> +:set lambda form which does other things after they have been set.
>
> I thought the conclusion was that most variables with :set can be safely 
> set by setq, isn't that so?  If so, these warnings will mostly annoy.
>

No, that wasn't the conclusion.  Most variable with :set can safely be set 
by setq *before* the file in which they are declared is loaded.  And no 
warnings are displayed in that case.  But all variables with :set cannot 
be safely be set by setq *after* the file in which they are declared has 
been loaded.  And warnings are displayed in that case.

>
> What will happen if setq is in the user's init file?  We generally delay 
> warnings until after the startup in those cases.
>

With the following .emacs:

(require 'allout)
(setq allout-auto-activation t)

a warning is displayed, but I don't know if this is during of after the 
startup.  At least it is visible when startup has completed.

>
> Also, warnings.el is not preloaded, so this call could barf in some 
> valid cases.
>

That's not what I see:

;;;###autoload
(defun display-warning (type message &optional level buffer-name)

>
> OTOH, setq is a primitive written in C, so ther should be no need to 
> call intern for it.
>

Okay, updated patch attached.

>
> And finally, do we really want to slow down each setq by calling intern 
> and Fget?  setq is many times used inside tight loops.  I'm not sure the 
> resulting run-time penalty is justified.  Did you measure the effect of 
> this on performance?
>

With the updated patch, on my laptop, the execution of setq takes ~48 
nanoseconds instead of ~40 nanoseconds.  Which seems reasonable.

[-- Attachment #2: Type: text/x-diff, Size: 3231 bytes --]

From 723e83b20f077ee36e8784b7101b4e20ace648f9 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Sun, 12 Sep 2021 09:22:13 +0000
Subject: [PATCH] Warn when custom variable is wrongly set.

* src/eval.c (Fsetq): Display warning when a custom variable with a :set
property is set with setq.  Warnings are displayed only for custom variables
whose files have been loaded and that have a :set property.  No warnings are
displayed for custom variables whose files have merely been preloaded,
for custom variables that do not have a :set property, and for non-custom
variables.  See bug#21695.
(syms_of_eval): Three new symbols.

* etc/NEWS: Document the warning.

* doc/emacs/custom.texi: Mention the warning.
---
 doc/emacs/custom.texi |  3 ++-
 etc/NEWS              |  6 ++++++
 src/eval.c            | 12 +++++++++++-
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/doc/emacs/custom.texi b/doc/emacs/custom.texi
index 9220a2078f..1b7e7d9361 100644
--- a/doc/emacs/custom.texi
+++ b/doc/emacs/custom.texi
@@ -2378,7 +2378,8 @@ Init Syntax
 command.  Finally, a few customizable user options are initialized in
 complex ways, and these have to be set either via the customize
 interface (@pxref{Customization}) or by using
-@code{customize-set-variable} (@pxref{Examining}).
+@code{customize-set-variable} (@pxref{Examining}).  If such options
+are inadvertently set with @code{setq}, a warning is displayed.
 
   The second argument to @code{setq} is an expression for the new
 value of the variable.  This can be a constant, a variable, or a
diff --git a/etc/NEWS b/etc/NEWS
index ca269aabaa..dfdd925ec5 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -3401,6 +3401,12 @@ truncating precision field, such as "%.2a".
 Such mixes are always signs that the outer lexical binding was an
 error and should have used dynamic binding instead.
 
+--
+** 'setq' displays a warning when 'customize-set-variable' should have been used.
+Some custom variables need to be set with 'customize-set-variable', because
+they were designed to be set through the Customization interface and have a
+:set lambda form which does other things after they have been set.
+
 ---
 ** New variable 'inhibit-mouse-event-check'.
 If bound to non-nil, a command with '(interactive "e")' doesn't signal
diff --git a/src/eval.c b/src/eval.c
index 48104bd0f4..84500f7c1a 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -525,8 +525,15 @@ DEFUN ("setq", Fsetq, Ssetq, 0, UNEVALLED, 0,
 	   : Qnil);
       if (!NILP (lex_binding))
 	XSETCDR (lex_binding, val); /* SYM is lexically bound.  */
-      else
+      else {
+	if (!NILP (Fget (sym, Qcustom_set)))
+	  call2 (Qdisplay_warning, Qsetq,
+		 CALLN (Fformat,
+			build_string
+			("`%s' should be set with `customize-set-variable'"),
+			sym));
 	Fset (sym, val);	/* SYM is dynamically bound.  */
+      }
     }
 
   return val;
@@ -4556,4 +4563,7 @@ syms_of_eval (void)
   defsubr (&Sbacktrace__locals);
   defsubr (&Sspecial_variable_p);
   defsubr (&Sfunctionp);
+  DEFSYM (Qcustom_set, "custom-set");
+  DEFSYM (Qdisplay_warning, "display-warning");
+  DEFSYM (Qsetq, "setq");
 }
-- 
2.33.0