unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
From: "Mattias Engdegård" <mattiase@acm.org>
To: Eli Zaretskii <eliz@gnu.org>
Cc: 39373@debbugs.gnu.org
Subject: bug#39373: 27.0.50; [PATCH] mode-local-print-bindings broken with lexical-binding
Date: Sat, 1 Feb 2020 20:24:56 +0100	[thread overview]
Message-ID: <3FB58E0B-E1DB-4709-98AB-92A45508486A@acm.org> (raw)
In-Reply-To: <83eevekat8.fsf@gnu.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)


  parent reply	other threads:[~2020-02-01 19:24 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-31 17:08 bug#39373: 27.0.50; [PATCH] mode-local-print-bindings broken with lexical-binding Mattias Engdegård
2020-01-31 19:27 ` Eli Zaretskii
2020-01-31 19:38   ` Mattias Engdegård
2020-01-31 20:25     ` Eli Zaretskii
2020-01-31 20:51       ` Mattias Engdegård
2020-02-01  7:48         ` Eli Zaretskii
2020-02-01 15:53           ` Drew Adams
2020-02-01 19:24           ` Mattias Engdegård [this message]
2020-02-01 19:28             ` Eli Zaretskii
2020-02-01 20:15             ` Stefan Monnier
2020-02-01 21:40               ` Mattias Engdegård
2020-02-02  3:31                 ` Eli Zaretskii
2020-02-02 11:58                   ` Mattias Engdegård
2020-02-02 13:55                     ` Stefan Monnier
2020-02-02 14:14                       ` Mattias Engdegård
2020-08-09 11:39                         ` Lars Ingebrigtsen
2020-08-09 13:28                           ` Mattias Engdegård
2020-08-09 19:42                             ` Lars Ingebrigtsen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/emacs/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3FB58E0B-E1DB-4709-98AB-92A45508486A@acm.org \
    --to=mattiase@acm.org \
    --cc=39373@debbugs.gnu.org \
    --cc=eliz@gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).