bug#39373: 27.0.50; [PATCH] mode-local-print-bindings broken with lexical-binding

["Mattias Engdegård" <mattiase@acm.org>]

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

1 feb. 2020 kl. 08.48 skrev Eli Zaretskii <eliz@gnu.org>:

> If that's the case, then why don't we say that loud and clear in the
> ELisp manual?  (The doc string of add-to-list has some vague
> recommendation, not sure if it really talks about this aspect, but
> that's definitely not enough for such a serious issue.)
> 
> And doesn't it mean we should audit all the gazillion uses of
> add-to-list in our sources, and do that urgently?

Yes, I agree, but it's not quite as simple as I thought at first. A mechanised scan of the source for uses of add-to-list on lexical variables revealed a handful:

lisp/cedet/mode-local.el:823:23: add-to-list to lexical variable mc
lisp/net/tramp-cache.el:376:45: add-to-list to lexical variable properties
lisp/net/tramp-cache.el:430:25: add-to-list to lexical variable result
lisp/net/zeroconf.el:259:40: add-to-list to lexical variable result
lisp/net/zeroconf.el:267:40: add-to-list to lexical variable result
lisp/net/zeroconf.el:281:23: add-to-list to lexical variable result
lisp/org/org.el:18685:49: add-to-list to lexical variable load-uncore
lisp/autoinsert.el:174:47: add-to-list to lexical variable modes
lisp/whitespace.el:1687:33: add-to-list to lexical variable style
test/lisp/emacs-lisp/map-tests.el:230:30: add-to-list to lexical variable result

However, add-to-list has a compiler macro which tries to make it work even for lexical variables, and it mostly does -- specifically, when the LIST-VAR parameter is on the form (quote VARIABLE), which is the case in all the above cases except the one i mode-local.el.

The macro (which apparently works in non-compiled code as well) attempts to warn about lexical variables but somehow this warning doesn't always trigger. I haven't researched this further.

It all seems rather fragile to me, and I'd rather add a note about not using add-to-list for lexical variables to its doc string and the manual, and fix all the calls listed above. However, this last point is not strictly necessary for correctness.

Two patches attached: a doc update, and a replacement of add-to-list in the cases listed above.


[-- Attachment #2: 0001-Clarify-add-to-list-documentation-bug-39373.patch --]
[-- Type: application/octet-stream, Size: 2039 bytes --]

From e7535d23b0855307a93ea6e8fa7cf7fb5910ac56 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Sat, 1 Feb 2020 20:11:11 +0100
Subject: [PATCH 1/2] Clarify add-to-list documentation (bug#39373)

While add-to-list often works with lexical variables, this is a hack
that isn't always effective; better tell the user not to try.

* doc/lispref/lists.texi (List Variables): Add a note about lexical
variables to the add-to-list description.  Fix the equivalent code.
* lisp/subr.el (add-to-list): Amend doc string.
---
 doc/lispref/lists.texi | 8 ++++++--
 lisp/subr.el           | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/doc/lispref/lists.texi b/doc/lispref/lists.texi
index 5ef21d0671..ce0d9a3c92 100644
--- a/doc/lispref/lists.texi
+++ b/doc/lispref/lists.texi
@@ -777,6 +777,9 @@ List Variables
 The argument @var{symbol} is not implicitly quoted; @code{add-to-list}
 is an ordinary function, like @code{set} and unlike @code{setq}.  Quote
 the argument yourself if that is what you want.
+
+Do not use this function when @var{symbol} refers to a lexical
+variable.
 @end defun
 
 Here's a scenario showing how to use @code{add-to-list}:
@@ -799,8 +802,9 @@ List Variables
 @var{value})} is this:
 
 @example
-(or (member @var{value} @var{var})
-    (setq @var{var} (cons @var{value} @var{var})))
+(if (member @var{value} @var{var})
+    @var{var}
+  (setq @var{var} (cons @var{value} @var{var})))
 @end example
 
 @defun add-to-ordered-list symbol element &optional order
diff --git a/lisp/subr.el b/lisp/subr.el
index a4fdc6bdfe..05fb82321e 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -1845,6 +1845,7 @@ add-to-list
 If ELEMENT is added, it is added at the beginning of the list,
 unless the optional argument APPEND is non-nil, in which case
 ELEMENT is added at the end.
+LIST-VAR should not refer to a lexical variable.
 
 The return value is the new value of LIST-VAR.
 
-- 
2.21.1 (Apple Git-122.3)


[-- Attachment #3: 0002-Replace-add-to-list-to-lexical-variable-with-push-bu.patch --]
[-- Type: application/octet-stream, Size: 7104 bytes --]

From 4501670056beed4c8e709674b3b80dc2a1160c0b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Sat, 1 Feb 2020 18:07:32 +0100
Subject: [PATCH 2/2] Replace add-to-list to lexical variable with push
 (bug#39373)

Since 'add-to-list', being a plain function, cannot access lexical
variables, such use must be rewritten for correctness.

* lisp/autoinsert.el (auto-insert-alist):
* lisp/cedet/mode-local.el (mode-local-print-bindings):
* lisp/net/tramp-cache.el (tramp-flush-connection-properties)
(tramp-list-connections):
* lisp/net/zeroconf.el (zeroconf-list-service-names)
(zeroconf-list-service-types, zeroconf-list-services):
* lisp/org/org.el (org-reload):
* lisp/whitespace.el (whitespace-report-region):
* test/lisp/emacs-lisp/map-tests.el (test-map-do):
Replace add-to-list with push.
---
 lisp/autoinsert.el                |  2 +-
 lisp/cedet/mode-local.el          | 14 ++++++--------
 lisp/net/tramp-cache.el           |  4 ++--
 lisp/net/zeroconf.el              | 12 ++++++------
 lisp/org/org.el                   |  5 +++--
 lisp/whitespace.el                |  2 +-
 test/lisp/emacs-lisp/map-tests.el |  2 +-
 7 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/lisp/autoinsert.el b/lisp/autoinsert.el
index 9bc3aad278..25961d4108 100644
--- a/lisp/autoinsert.el
+++ b/lisp/autoinsert.el
@@ -171,7 +171,7 @@ auto-insert-alist
                  (mapatoms (lambda (mode)
                              (let ((name (symbol-name mode)))
                                (when (string-match "-mode$" name)
-                                 (add-to-list 'modes name)))))
+                                 (push name modes)))))
                  (sort modes 'string<)))
      (completing-read "Local variables for mode: " v1 nil t)
      " . (("
diff --git a/lisp/cedet/mode-local.el b/lisp/cedet/mode-local.el
index a6e143cfcd..a1aea30c20 100644
--- a/lisp/cedet/mode-local.el
+++ b/lisp/cedet/mode-local.el
@@ -819,14 +819,12 @@ mode-local-print-bindings
         )
     ;; Order symbols by type
     (mapatoms
-     #'(lambda (s)
-         (add-to-list (cond
-                       ((get s 'mode-variable-flag)
-                        (if (get s 'constant-flag) 'mc 'mv))
-                       ((get s 'override-flag)
-                        (if (get s 'constant-flag) 'fo 'ov))
-                       ('us))
-                      s))
+     (lambda (s) (push s (cond
+                          ((get s 'mode-variable-flag)
+                           (if (get s 'constant-flag) mc mv))
+                          ((get s 'override-flag)
+                           (if (get s 'constant-flag) fo ov))
+                          (t us))))
      table)
     ;; Print symbols by type
     (when us
diff --git a/lisp/net/tramp-cache.el b/lisp/net/tramp-cache.el
index b81a1a23d5..62e25fa1f0 100644
--- a/lisp/net/tramp-cache.el
+++ b/lisp/net/tramp-cache.el
@@ -373,7 +373,7 @@ tramp-flush-connection-properties
    (let ((hash (gethash key tramp-cache-data))
 	 properties)
      (when (hash-table-p hash)
-       (maphash (lambda (x _y) (add-to-list 'properties x 'append)) hash))
+       (maphash (lambda (x _y) (push x properties)) hash))
      properties))
   (setq tramp-cache-data-changed t)
   (remhash key tramp-cache-data))
@@ -427,7 +427,7 @@ tramp-list-connections
 	 (when (and (tramp-file-name-p key)
 		    (null (tramp-file-name-localname key))
 		    (tramp-connection-property-p key "process-buffer"))
-	   (add-to-list 'result key)))
+	   (push key result)))
        tramp-cache-data)
       result))
 
diff --git a/lisp/net/zeroconf.el b/lisp/net/zeroconf.el
index b8becd712f..cb3c0f2a7e 100644
--- a/lisp/net/zeroconf.el
+++ b/lisp/net/zeroconf.el
@@ -256,17 +256,17 @@ zeroconf-list-service-names
   "Return all discovered Avahi service names as list."
   (let (result)
     (maphash
-     (lambda (_key value) (add-to-list 'result (zeroconf-service-name value)))
+     (lambda (_key value) (push (zeroconf-service-name value) result))
      zeroconf-services-hash)
-    result))
+    (delete-dups result)))
 
 (defun zeroconf-list-service-types ()
   "Return all discovered Avahi service types as list."
   (let (result)
     (maphash
-     (lambda (_key value) (add-to-list 'result (zeroconf-service-type value)))
+     (lambda (_key value) (push (zeroconf-service-type value) result))
      zeroconf-services-hash)
-    result))
+    (delete-dups result)))
 
 (defun zeroconf-list-services (type)
   "Return all discovered Avahi services for a given service type TYPE.
@@ -278,9 +278,9 @@ zeroconf-list-services
     (maphash
      (lambda (_key value)
        (when (equal type (zeroconf-service-type value))
-	 (add-to-list 'result value)))
+	 (push value result)))
      zeroconf-services-hash)
-    result))
+    (delete-dups result)))
 
 (defvar zeroconf-service-added-hooks-hash (make-hash-table :test 'equal)
   "Hash table of hooks for newly added services.
diff --git a/lisp/org/org.el b/lisp/org/org.el
index 5c8b02b9d1..568f5b9b87 100644
--- a/lisp/org/org.el
+++ b/lisp/org/org.el
@@ -18682,13 +18682,14 @@ org-reload
 			      (and (string= org-dir contrib-dir)
 				   (org-load-noerror-mustsuffix (concat contrib-dir f)))
 			      (and (org-load-noerror-mustsuffix (concat (org-find-library-dir f) f))
-				   (add-to-list 'load-uncore f 'append)
+				   (push f load-uncore)
 				   't)
 			      f))
 			lfeat)))
     (when load-uncore
       (message "The following feature%s found in load-path, please check if that's correct:\n%s"
-	       (if (> (length load-uncore) 1) "s were" " was") load-uncore))
+	       (if (> (length load-uncore) 1) "s were" " was")
+               (reverse load-uncore)))
     (if load-misses
 	(message "Some error occurred while reloading Org feature%s\n%s\nPlease check *Messages*!\n%s"
 		 (if (> (length load-misses) 1) "s" "") load-misses (org-version nil 'full))
diff --git a/lisp/whitespace.el b/lisp/whitespace.el
index 111b175263..db7c023324 100644
--- a/lisp/whitespace.el
+++ b/lisp/whitespace.el
@@ -1684,7 +1684,7 @@ whitespace-report-region
             (mapcar
              #'(lambda (option)
                  (when force
-                   (add-to-list 'style (car option)))
+                   (push (car option) style))
                  (goto-char rstart)
                  (let ((regexp
                         (cond
diff --git a/test/lisp/emacs-lisp/map-tests.el b/test/lisp/emacs-lisp/map-tests.el
index 06fd55faad..c52bb83fa3 100644
--- a/test/lisp/emacs-lisp/map-tests.el
+++ b/test/lisp/emacs-lisp/map-tests.el
@@ -227,7 +227,7 @@ test-map-do
   (with-maps-do map
     (let ((result nil))
       (map-do (lambda (k v)
-                (add-to-list 'result (list (int-to-string k) v)))
+                (push (list (int-to-string k) v) result))
               map)
       (should (equal result '(("2" 5) ("1" 4) ("0" 3)))))))
 
-- 
2.21.1 (Apple Git-122.3)