bug#5950: defvaralias after defvar should be warned in runtime

[Noam Postavsky]

tags 5950 + patch
quit

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> Changing defvaralias to try and be more clever would definitely
> be wrong.  But changing it to output a warning about the problematic
> situation would be OK and changing the byte-compiler to output a warning
> in cases that make such a situation more likely is also perfectly good.

I see the byte-compiler side of this has been taken care of [1..4], but
I still managed to make this mistake when I made a cross-file alias
(Bug#31603).  Here's a patch for the runtime warning.  While writing it,
I found that invoking `display-warning' failed during bootstrap, so the
first commit works around that.  In the end it's not strictly necessary,
because I added a check to suppress the warning when the two variables
have `eq' values, so no warnings are generated during bootstrap anyway.

The bootstrap warnings were about cl-custom-print-functions,
mail-dont-reply-to-names, and several flymake-proc-* variables all of
which correctly have the alias before the definition.  This makes me
think I might be doing something wrong, but it seems to do the right
thing in my tests.

>From e039d38c653059716b68f64fdc7b395bf7593834 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Fri, 25 May 2018 21:37:17 -0400
Subject: [PATCH v1 1/2] Let display-warning work during bootstrap

* lisp/emacs-lisp/warnings.el (display-warning): Only call
`special-mode' and `newline' if they are `fbound'.
---
 lisp/emacs-lisp/warnings.el | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/lisp/emacs-lisp/warnings.el b/lisp/emacs-lisp/warnings.el
index 665733181c..c4d97ceab0 100644
--- a/lisp/emacs-lisp/warnings.el
+++ b/lisp/emacs-lisp/warnings.el
@@ -241,11 +241,15 @@ display-warning
               (old (get-buffer buffer-name))
               (buffer (or old (get-buffer-create buffer-name)))
               (level-info (assq level warning-levels))
+               ;; `newline' may be unbound during bootstrap.
+               (newline (if (fboundp 'newline) #'newline
+                          (lambda () (insert "\n"))))
               start end)
          (with-current-buffer buffer
            ;; If we created the buffer, disable undo.
            (unless old
-             (special-mode)
+             (when (fboundp 'special-mode) ; Undefined during bootstrap.
+                (special-mode))
              (setq buffer-read-only t)
              (setq buffer-undo-list t))
            (goto-char (point-max))
@@ -256,7 +260,7 @@ display-warning
                        (funcall warning-series)))))
            (let ((inhibit-read-only t))
              (unless (bolp)
-               (newline))
+               (funcall newline))
              (setq start (point))
              (if warning-prefix-function
                  (setq level-info (funcall warning-prefix-function
@@ -264,7 +268,7 @@ display-warning
              (insert (format (nth 1 level-info)
                              (format warning-type-format typename))
                      message)
-             (newline)
+              (funcall newline)
              (when (and warning-fill-prefix (not (string-match "\n" message)))
                (let ((fill-prefix warning-fill-prefix)
                      (fill-column 78))
-- 
2.11.0

>From 6fe5dee5f4ad0a6e18d73658d0392d4444ff1826 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Fri, 25 May 2018 08:40:55 -0400
Subject: [PATCH v1 2/2] Give warning if losing value to defvaralias (Bug#5950)

* src/eval.c (Fdefvaralias): Call `display-warning' if the alias
target has a non-eq value to the variable being aliased.
* test/src/eval-tests.el (defvaralias-overwrite-warning): New test.
---
 src/eval.c             | 10 ++++++++++
 test/src/eval-tests.el | 22 ++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/src/eval.c b/src/eval.c
index 90d8c33518..354ab2c2d1 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -623,6 +623,16 @@ DEFUN ("defvaralias", Fdefvaralias, Sdefvaralias, 2, 3, 0,
   if (NILP (Fboundp (base_variable)))
     set_internal (base_variable, find_symbol_value (new_alias),
                   Qnil, SET_INTERNAL_BIND);
+  else if (!NILP (Fboundp (new_alias))
+           && !EQ (find_symbol_value (new_alias),
+                   find_symbol_value (base_variable)))
+    call2 (intern ("display-warning"),
+           list3 (intern ("defvaralias"), intern ("losing-value"), new_alias),
+           CALLN (Fformat_message,
+                  build_string
+                  ("Overwriting value of `%s' by aliasing to `%s'"),
+                  new_alias, base_variable));
+
   {
     union specbinding *p;
 
diff --git a/test/src/eval-tests.el b/test/src/eval-tests.el
index 319dd91c86..281d959b53 100644
--- a/test/src/eval-tests.el
+++ b/test/src/eval-tests.el
@@ -26,6 +26,7 @@
 ;;; Code:
 
 (require 'ert)
+(eval-when-compile (require 'cl-lib))
 
 (ert-deftest eval-tests--bug24673 ()
   "Check that Bug#24673 has been fixed."
@@ -117,4 +118,25 @@ eval-tests--exceed-specbind-limit
   "Check that Bug#31072 is fixed."
   (should-error (eval '(defvar 1) t) :type 'wrong-type-argument))
 
+(ert-deftest defvaralias-overwrite-warning ()
+  "Test for Bug#5950."
+  (defvar eval-tests--foo)
+  (setq eval-tests--foo 2)
+  (defvar eval-tests--foo-alias)
+  (setq eval-tests--foo-alias 1)
+  (cl-letf (((symbol-function 'display-warning)
+             (lambda (type &rest _)
+               (throw 'got-warning type))))
+    ;; Warn if we lose a value through aliasing.
+    (should (equal
+             '(defvaralias losing-value eval-tests--foo-alias)
+             (catch 'got-warning
+               (defvaralias 'eval-tests--foo-alias 'eval-tests--foo))))
+    ;; Don't warn if we don't.
+    (makunbound 'eval-tests--foo-alias)
+    (should (eq 'no-warning
+                (catch 'got-warning
+                  (defvaralias 'eval-tests--foo-alias 'eval-tests--foo)
+                  'no-warning)))))
+
 ;;; eval-tests.el ends here
-- 
2.11.0


[1: 495963cfaf]: 2018-04-20 17:22:47 -0400
  * lisp/emacs-lisp/bytecomp.el (byte-compile-file-form-defvar-function):
  
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=495963cfaf535646350051f47c085b84319572f0

[2: 9c3eeba4db]: 2018-04-20 18:34:39 -0400
  The tedious game of whack-a-mole with compiler warnings continues
  
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=9c3eeba4db26ddaeead100beea7a96f9fa640918

[3: 18de2ada24]: 2018-04-20 18:55:04 -0400
  More alias-related tedium
  
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=18de2ada243653ece98b18044233e5d29eee5903

[4: 94e794c8d8]: 2018-04-20 19:02:16 -0400
  Tweak recent bytecomp defvaralias change
  
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=94e794c8d8b93a1d6813742da12135f2746ef80b