Re: [BUG] org-capture autoload bug? [9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)]

[Ignacio Casso <ignaciocasso@hotmail.com>]

Hello,

The bug I reported about this to the Emacs devel mailing list
(https://debbugs.gnu.org/cgi/bugreport.cgi?bug=54399) has now been
closed, and some documentation has been updated in commit 071722e41120.

Basically, the problem is that in order for

  (let ((custom-variable local-value) ...)
      ...
      (defcustom custom-variable standard-value ...)
      ...)

to work as expected with dynamic binding, `defcustom' has to set the
variable with `set-default-toplevel-value'. This way, the code inside
the `let' form uses local-value and the default value is set to
standard-value. If `defcustom' uses `set-default' instead, it overwrites
the local value, so the let-binding is useless, and after the let form
is evaluated the variable is void.

The problem was that `set-default-toplevel-value' is used by default,
but the documentation said that the default was `set-default'. So either
whoever wrote the `org-capture-templates' setter was misguided by the
documentation and used `set-default', or more likely, that setter was
written before the introduction of `set-default-toplevel-value' in
custom.el, and was never updated.

The first thing we should do is finding all uses of `set-default' in the
org `defcustom' setters and replace them with
`set-default-toplevel-value'. This would fix this bug and similar ones
when using dynamic binding, and would not affect any other use case.

Then we should decide if we want to use autoload cookies for custom
variables to make this work also with lexical binding. Otherwise, code
like the snippet above would produce an error in Emacs 29, and in Emacs
27 the let binding would be ignored (although at least the custom setter
would work). I have no opinion regarding this last point since I don't
remember what were the disadvantages of using autoload cookies for
custom variables.

I've prepared a patch for the first point, which I attach at the end of
this email. All changes fall in one of the following cases:

- `set-default' -> `set-default-toplevel-value' (as explained)

- `set' -> `set-default-toplevel-value'

  The same, but in this cases there was another bug: If a buffer set the
  custom variable locally before the feature was autoloaded, the setter
  of the variable would not set the standard value as the default for
  other buffers, and would overwrite the buffer-local value.

- :set 'set-default -> nothing, since it would be already the default

I don't really know what most of the variables whose setter I have
changed do or whether it makes any sense to use them inside a let
binding, but I have made the change for all of them nevertheless, since
it can not harm and could potentially fix a bug. Feel free to restrict
those changes only to those variables where it makes sense, such as
`org-capture-templates', if you want to keep the patch small and simple.

Best regards,

Ignacio

[2. patch --- text/x-diff; 0001-using-set-default-toplevel-value-in-defcustom-setter.patch]
From b20e23013329fab574a4d05eb5be8cde1d43dad1 Mon Sep 17 00:00:00 2001
From: Ignacio Casso <ignaciocasso@hotmail.com>
Date: Fri, 10 Jun 2022 12:39:43 +0200
Subject: [PATCH] using `set-default-toplevel-value' in `defcustom' setters

---
 lisp/ob-lilypond.el  |  2 +-
 lisp/ob-shell.el     |  2 +-
 lisp/org-capture.el  |  2 +-
 lisp/org-clock.el    |  2 +-
 lisp/org-duration.el |  2 +-
 lisp/org-faces.el    |  2 +-
 lisp/org-footnote.el |  2 +-
 lisp/org-list.el     |  4 ++--
 lisp/org.el          | 23 +++++++++++------------
 lisp/ox-odt.el       |  2 +-
 10 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/lisp/ob-lilypond.el b/lisp/ob-lilypond.el
index df128441a..dc33ebc17 100644
--- a/lisp/ob-lilypond.el
+++ b/lisp/ob-lilypond.el
@@ -107,7 +107,7 @@ you can leave the string empty on this case."
   :package-version '(Org . "8.2.7")
   :set
   (lambda (symbol value)
-    (set symbol value)
+    (set-default-toplevel-value symbol value)
     (setq
      org-babel-lilypond-ly-command   (nth 0 value)
      org-babel-lilypond-pdf-command  (nth 1 value)
diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el
index c25941a44..4454e3b5d 100644
--- a/lisp/ob-shell.el
+++ b/lisp/ob-shell.el
@@ -68,7 +68,7 @@ outside the Customize interface."
   :group 'org-babel
   :type '(repeat (string :tag "Shell name: "))
   :set (lambda (symbol value)
-	 (set-default symbol value)
+	 (set-default-toplevel-value symbol value)
 	 (org-babel-shell-initialize)))
 
 (defcustom org-babel-shell-results-defaults-to-output t
diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index 773234967..948eb8bc6 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -376,7 +376,7 @@ When you need to insert a literal percent sign in the template,
 you can escape ambiguous cases with a backward slash, e.g., \\%i."
   :group 'org-capture
   :package-version '(Org . "9.5")
-  :set (lambda (s v) (set s (org-capture-upgrade-templates v)))
+  :set (lambda (s v) (set-default-toplevel-value s (org-capture-upgrade-templates v)))
   :type
   (let ((file-variants '(choice :tag "Filename       "
 				(file :tag "Literal")
diff --git a/lisp/org-clock.el b/lisp/org-clock.el
index e0c40ae23..b94c79baa 100644
--- a/lisp/org-clock.el
+++ b/lisp/org-clock.el
@@ -493,7 +493,7 @@ This variable only has effect if set with \\[customize]."
          (if value
              (add-hook 'kill-emacs-query-functions #'org-clock-kill-emacs-query)
            (remove-hook 'kill-emacs-query-functions #'org-clock-kill-emacs-query))
-         (set symbol value))
+         (set-default-toplevel-value symbol value))
   :type 'boolean
   :package-version '(Org . "9.5"))
 
diff --git a/lisp/org-duration.el b/lisp/org-duration.el
index b242a6f2c..338ea11a9 100644
--- a/lisp/org-duration.el
+++ b/lisp/org-duration.el
@@ -98,7 +98,7 @@ sure to call the following command:
   :version "26.1"
   :package-version '(Org . "9.1")
   :set (lambda (var val)
-         (set-default var val)
+         (set-default-toplevel-value var val)
          ;; Avoid recursive load at startup.
 	 (when (featurep 'org-duration)
            (org-duration-set-regexps)))
diff --git a/lisp/org-faces.el b/lisp/org-faces.el
index f919a6b47..5fb6c3e07 100644
--- a/lisp/org-faces.el
+++ b/lisp/org-faces.el
@@ -338,7 +338,7 @@ determines if it is a foreground or a background color."
 
 (defvar org-tags-special-faces-re nil)
 (defun org-set-tag-faces (var value)
-  (set var value)
+  (set-default-toplevel-value var value)
   (if (not value)
       (setq org-tags-special-faces-re nil)
     (setq org-tags-special-faces-re
diff --git a/lisp/org-footnote.el b/lisp/org-footnote.el
index 0a5f994a4..8e0ac0da2 100644
--- a/lisp/org-footnote.el
+++ b/lisp/org-footnote.el
@@ -110,7 +110,7 @@ you will need to run the following command after the change:
   :group 'org-footnote
   :initialize 'custom-initialize-default
   :set (lambda (var val)
-	 (set var val)
+	 (set-default-toplevel-value var val)
 	 (when (fboundp 'org-element-cache-reset)
 	   (org-element-cache-reset 'all)))
   :type '(choice
diff --git a/lisp/org-list.el b/lisp/org-list.el
index 515763036..af560cff4 100644
--- a/lisp/org-list.el
+++ b/lisp/org-list.el
@@ -235,7 +235,7 @@ interface or run the following code after updating it:
   :type '(choice (const :tag "dot like in \"2.\"" ?.)
 		 (const :tag "paren like in \"2)\"" ?\))
 		 (const :tag "both" t))
-  :set (lambda (var val) (set var val)
+  :set (lambda (var val) (set-default-toplevel-value var val)
 	 (when (featurep 'org-element) (org-element-update-syntax))))
 
 (defcustom org-list-allow-alphabetical nil
@@ -253,7 +253,7 @@ interface or run the following code after updating it:
   :group 'org-plain-lists
   :version "24.1"
   :type 'boolean
-  :set (lambda (var val) (set var val)
+  :set (lambda (var val) (set-default-toplevel-value var val)
 	 (when (featurep 'org-element) (org-element-update-syntax))))
 
 (defcustom org-list-two-spaces-after-bullet-regexp nil
diff --git a/lisp/org.el b/lisp/org.el
index ac94fb614..c57ccf319 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -231,7 +231,7 @@ Stars are put in group 1 and the trimmed body in group 2.")
 ;;;###autoload
 (defun org-babel-do-load-languages (sym value)
   "Load the languages defined in `org-babel-load-languages'."
-  (set-default sym value)
+  (set-default-toplevel-value sym value)
   (dolist (pair org-babel-load-languages)
     (let ((active (cdr pair)) (lang (symbol-name (car pair))))
       (if active
@@ -706,7 +706,7 @@ defined in org-duration.el.")
 
 (defun org-set-modules (var value)
   "Set VAR to VALUE and call `org-load-modules-maybe' with the force flag."
-  (set var value)
+  (set-default-toplevel-value var value)
   (when (featurep 'org)
     (org-load-modules-maybe 'force)
     (org-element-cache-reset 'all)))
@@ -837,7 +837,7 @@ depends on, if any."
   :package-version '(Org . "9.0")
   :initialize 'custom-initialize-set
   :set (lambda (var val)
-	 (if (not (featurep 'ox)) (set-default var val)
+	 (if (not (featurep 'ox)) (set-default-toplevel-value var val)
 	   ;; Any back-end not required anymore (not present in VAL and not
 	   ;; a parent of any back-end in the new value) is removed from the
 	   ;; list of registered back-ends.
@@ -862,7 +862,7 @@ depends on, if any."
 			  backend))
 		((not (memq backend new-list)) (push backend new-list))))
 	     ;; Set VAR to that list with fixed dependencies.
-	     (set-default var new-list))))
+	     (set-default-toplevel-value var new-list))))
   :type '(set :greedy t
 	      (const :tag "   ascii       Export buffer to ASCII format" ascii)
 	      (const :tag "   beamer      Export buffer to Beamer presentation" beamer)
@@ -1815,9 +1815,9 @@ are followed by a letter in parenthesis, like TODO(t)."
   :group 'org-todo
   :set (lambda (var val)
 	 (cond
-	  ((eq var t) (set var 'auto))
-	  ((eq var 'prefix) (set var nil))
-	  (t (set var val))))
+	  ((eq var t) (set-default-toplevel-value var 'auto))
+	  ((eq var 'prefix) (set-default-toplevel-value var nil))
+	  (t (set-default-toplevel-value var val))))
   :type '(choice
 	  (const :tag "Never" nil)
 	  (const :tag "Automatically, when key letter have been defined" auto)
@@ -1899,7 +1899,7 @@ be blocked if any prior sibling is not yet done.
 Finally, if the parent is blocked because of ordered siblings of its own,
 the child will also be blocked."
   :set (lambda (var val)
-	 (set var val)
+	 (set-default-toplevel-value var val)
 	 (if val
 	     (add-hook 'org-blocker-hook
 		       'org-block-todo-from-children-or-siblings-or-parent)
@@ -1917,7 +1917,7 @@ This variable needs to be set before org.el is loaded, and you need to
 restart Emacs after a change to make the change effective.  The only way
 to change it while Emacs is running is through the customize interface."
   :set (lambda (var val)
-	 (set var val)
+	 (set-default-toplevel-value var val)
 	 (if val
 	     (add-hook 'org-blocker-hook
 		       'org-block-todo-from-checkboxes)
@@ -2368,7 +2368,6 @@ The formats are defined through the variable `org-time-stamp-custom-formats'.
 To turn this on on a per-file basis, insert anywhere in the file:
    #+STARTUP: customtime"
   :group 'org-time
-  :set 'set-default
   :type 'sexp)
 (make-variable-buffer-local 'org-display-custom-times)
 
@@ -3275,7 +3274,7 @@ header, or they will be appended."
 
 (defun org-set-packages-alist (var val)
   "Set the packages alist and make sure it has 3 elements per entry."
-  (set var (mapcar (lambda (x)
+  (set-default-toplevel-value var (mapcar (lambda (x)
 		     (if (and (consp x) (= (length x) 2))
 			 (list (car x) (nth 1 x) t)
 		       x))
@@ -3548,7 +3547,7 @@ After a match, the match groups contain these elements:
 (defvar org-emphasis-alist) ; defined just below
 (defun org-set-emph-re (var val)
   "Set variable and compute the emphasis regular expression."
-  (set var val)
+  (set-default-toplevel-value var val)
   (when (and (boundp 'org-emphasis-alist)
 	     (boundp 'org-emphasis-regexp-components)
 	     org-emphasis-alist org-emphasis-regexp-components)
diff --git a/lisp/ox-odt.el b/lisp/ox-odt.el
index aa6e90122..9b46f15b5 100644
--- a/lisp/ox-odt.el
+++ b/lisp/ox-odt.el
@@ -404,7 +404,7 @@ with GNU ELPA tar or standard Emacs distribution."
     "Set `org-odt-schema-dir'.
 Also add it to `rng-schema-locating-files'."
     (let ((schema-dir value))
-      (set var
+      (set-default-toplevel-value var
 	   (if (and
 		(file-expand-wildcards
 		 (expand-file-name "od-manifest-schema*.rnc" schema-dir))
-- 
2.25.1