bug#27016: possible bug in `defsetf'

[npostavs]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Is it intended to add an :gv-expanders entry to
> byte-compile-macro-environment more than once?

Hmm, not entirely consciously, I was following the same pattern I had
used for cl-symbol-macrolet, but in that case we're establishing a
let-binding so it's important to be able to pop back to the old binding.
It's not needed here though.

Also, I had a typo in gv-get (:gv-expands instead of :gv-expanders),
because I didn't actually test the positive case.  Here's a working(?)
version, with tests:

>From b48af25eb5c18cb45d9e431076df767718efa0ec Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Mon, 10 Jul 2017 21:42:05 -0400
Subject: [PATCH v2] Don't define gv expanders in compiler session (Bug#27016)

This prevents definitions being compiled from leaking into the current
Emacs doing the compilation.
* lisp/emacs-lisp/gv.el (gv-define-expander): Push the expander
definition into `byte-compile-macro-environment' instead of evaluating
at compile time.
(gv-get): Check `byte-compile-macro-environment' for gv-expander
definitions.
* test/lisp/emacs-lisp/gv-tests.el: New tests.
---
 lisp/emacs-lisp/gv.el            |  23 ++++++---
 test/lisp/emacs-lisp/gv-tests.el | 103 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 119 insertions(+), 7 deletions(-)
 create mode 100644 test/lisp/emacs-lisp/gv-tests.el

diff --git a/lisp/emacs-lisp/gv.el b/lisp/emacs-lisp/gv.el
index c5c12a6414..fa8ae27e1f 100644
--- a/lisp/emacs-lisp/gv.el
+++ b/lisp/emacs-lisp/gv.el
@@ -91,7 +91,10 @@ (defun gv-get (place do)
    ((not (consp place)) (signal 'gv-invalid-place (list place)))
    (t
     (let* ((head (car place))
-           (gf (function-get head 'gv-expander 'autoload)))
+           (gf (or (alist-get head (alist-get :gv-expanders
+                                              (bound-and-true-p
+                                               
byte-compile-macro-environment)))
+                   (function-get head 'gv-expander 'autoload))))
       (if gf (apply gf do (cdr place))
         (let ((me (macroexpand-1 place
                                  ;; (append macroexpand-all-environment
@@ -146,12 +149,18 @@ (defmacro gv-define-expander (name handler)
 HANDLER is a function which takes an argument DO followed by the same
 arguments as NAME.  DO is a function as defined in `gv-get'."
   (declare (indent 1) (debug (sexp form)))
-  ;; Use eval-and-compile so the method can be used in the same file as it
-  ;; is defined.
-  ;; FIXME: Just like byte-compile-macro-environment, we should have something
-  ;; like byte-compile-symbolprop-environment so as to handle these things
-  ;; cleanly without affecting the running Emacs.
-  `(eval-and-compile (put ',name 'gv-expander ,handler)))
+  ;; Push onto `byte-compile-macro-environment' so the method can be
+  ;; used in the same file as it is defined.
+  (when (boundp 'byte-compile-macro-environment)
+    (let* ((expanders (assq :gv-expanders byte-compile-macro-environment))
+           (expander (assq name (cdr expanders)))
+           (new-expander (cons name handler)))
+      (cond (expander (setcdr expander handler))
+            (expanders (setcdr expanders (cons new-expander (cdr expanders))))
+            (t (setq byte-compile-macro-environment
+                     (cons (cons :gv-expanders (list new-expander))
+                           byte-compile-macro-environment))))))
+  `(put ',name 'gv-expander ,handler))
 
 ;;;###autoload
 (defun gv--defun-declaration (symbol name args handler &optional fix)
diff --git a/test/lisp/emacs-lisp/gv-tests.el b/test/lisp/emacs-lisp/gv-tests.el
new file mode 100644
index 0000000000..affc7ce455
--- /dev/null
+++ b/test/lisp/emacs-lisp/gv-tests.el
@@ -0,0 +1,103 @@
+;;; gv-tests.el --- tests for gv.el  -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2017 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Code:
+
+(require 'ert)
+(eval-when-compile (require 'cl-lib))
+
+(cl-defmacro gv-tests--in-temp-dir ((elvar elcvar)
+                                    (&rest filebody)
+                                    &rest body)
+  (declare (indent 2))
+  `(let ((default-directory (make-temp-file "gv-test" t)))
+     (unwind-protect
+         (let ((,elvar "gv-test-deffoo.el")
+               (,elcvar "gv-test-deffoo.elc"))
+           (with-temp-file ,elvar
+             (insert ";; -*- lexical-binding: t; -*-\n")
+             (dolist (form ',filebody)
+               (pp form (current-buffer))))
+           ,@body)
+       (delete-directory default-directory t))))
+
+(ert-deftest gv-define-expander-in-file ()
+  (gv-tests--in-temp-dir (el elc)
+      ((gv-define-setter gv-test-foo (newval cons)
+         `(setcar ,cons ,newval))
+       (defvar gv-test-pair (cons 1 2))
+       (setf (gv-test-foo gv-test-pair) 99)
+       (message "%d" (car gv-test-pair)))
+    (with-temp-buffer
+      (call-process (concat invocation-directory invocation-name)
+                    nil '(t t) nil
+                    "-Q" "-batch" "--eval" (prin1-to-string 
`(byte-compile-file ,el))
+                    "-l" elc)
+      (should (equal (buffer-string) "99\n")))))
+
+(ert-deftest gv-dont-define-expander-in-file ()
+  ;; The expander is defined while we are compiling the file, even
+  ;; though it's inside (when nil ...).
+  :expected-result :failed
+  (gv-tests--in-temp-dir (el elc)
+      ((when nil (gv-define-setter gv-test-foo (newval cons)
+                   `(setcar ,cons ,newval)))
+       (defvar gv-test-pair (cons 1 2))
+       (setf (gv-test-foo gv-test-pair) 99)
+       (message "%d" (car gv-test-pair)))
+    (with-temp-buffer
+      (call-process (concat invocation-directory invocation-name)
+                    nil '(t t) nil
+                    "-Q" "-batch" "--eval" (prin1-to-string 
`(byte-compile-file ,el))
+                    "-l" elc)
+      (should (equal (buffer-string)
+                     "Symbol's function definition is void: \\(setf\\ 
gv-test-foo\\)\n")))))
+
+(ert-deftest gv-define-expander-out-of-file ()
+  (gv-tests--in-temp-dir (el elc)
+      ((gv-define-setter gv-test-foo (newval cons)
+         `(setcar ,cons ,newval))
+       (defvar gv-test-pair (cons 1 2)))
+    (with-temp-buffer
+      (call-process (concat invocation-directory invocation-name)
+                    nil '(t t) nil
+                    "-Q" "-batch" "--eval" (prin1-to-string 
`(byte-compile-file ,el))
+                    "-l" elc
+                    "--eval"
+                    (prin1-to-string '(progn (setf (gv-test-foo gv-test-pair) 
99)
+                                             (message "%d" (car 
gv-test-pair)))))
+      (should (equal (buffer-string) "99\n")))))
+
+(ert-deftest gv-dont-define-expander-other-file ()
+  (gv-tests--in-temp-dir (el elc)
+      ((if nil (gv-define-setter gv-test-foo (newval cons)
+                 `(setcar ,cons ,newval)))
+       (defvar gv-test-pair (cons 1 2)))
+    (with-temp-buffer
+      (call-process (concat invocation-directory invocation-name)
+                    nil '(t t) nil
+                    "-Q" "-batch" "--eval" (prin1-to-string 
`(byte-compile-file ,el))
+                    "-l" elc
+                    "--eval"
+                    (prin1-to-string '(progn (setf (gv-test-foo gv-test-pair) 
99)
+                                             (message "%d" (car 
gv-test-pair)))))
+      (should (equal (buffer-string)
+                     "Symbol's function definition is void: \\(setf\\ 
gv-test-foo\\)\n")))))
+
+;;; gv-tests.el ends here
-- 
2.11.1