bug#39169: 28.0.50; Confusing obsolete variable warnings in eieio-defclass-autoload

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> > diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
> > index 966990bac9..418070fabe 100644
> > --- a/lisp/emacs-lisp/bytecomp.el
> > +++ b/lisp/emacs-lisp/bytecomp.el
> > @@ -3357,6 +3357,7 @@ byte-compile-check-variable
> >             (and od
> >                  (not (memq var byte-compile-not-obsolete-vars))
> >                  (not (memq var byte-compile-global-not-obsolete-vars))
> > +                (not (memq var byte-compile-lexical-variables))
> >                  (or (pcase (nth 1 od)
> >                        ('set (not (eq access-type 'reference)))
> >                        ('get (eq access-type 'reference))
>
> Yes!

Coming back to this now... Ok, then let's do that!

> > BTW, while I looked at this, I found this spurious lookup in
> > `byte-compile-lexical-variables':
> >
> > #+begin_src emacs-lisp
> > (defun byte-compile-form (form &optional for-effect)
> >   (let ((byte-compile--for-effect for-effect))
> >     (cond
> >      ((not (consp form))..10..)
> >      ((symbolp (car form))
> >       (let* ((fn (car form))..4..)
> >         (when (memq fn '(set symbol-value run-hooks..4..)
> >           (pcase (cdr form)
> >             (`(',var . ,_)
> >              (when (assq var byte-compile-lexical-variables) ;; <--- here
> >                (byte-compile-report-error
> >                 (format-message "%s cannot use lexical var `%s'" fn var)))
> >              ...)))))))))
> > #+end_src
> >
> > shouldn't the `assq' be `memq'?
>
> I think you're right.

Seems this has been treated meanwhile: Bug#44980 and commit ace6eba036
"Fix byte-compiler warning for failed uses of lexical vars", Stefan
Kangas.

> Both changes should ideally come with corresponding regression tests.

I tried to add an appropriate test for the remaining case, please check
the following patch (my first addition to the test suite) - is it what
you had in mind? - where I also try to give a less confusing (but
longer) compiler warning.  Does it look alright?

From 63f3420d91273fecb51095c20c81d9fd0508ee4c Mon Sep 17 00:00:00 2001
From: Michael Heerdegen <michael_heerdegen@web.de>
Date: Tue, 22 Dec 2020 05:44:47 +0100
Subject: [PATCH] Fix obsolete variable warnings about class names

* lisp/emacs-lisp/eieio-core.el (eieio-defclass-autoload): Try to make
the wording of the warning about the obsoleted variable less confusing.
* lisp/emacs-lisp/bytecomp.el (byte-compile-check-variable): Don't
warn for lexical variables (Bug#39169).  Fix spurious `or'.
* test/lisp/emacs-lisp/bytecomp-tests.el
(bytecomp/warn-obsolete-variable-bound\.el): New test.
* test/lisp/emacs-lisp/bytecomp-resources/warn-obsolete-variable-bound.el:
New file.
---
 lisp/emacs-lisp/bytecomp.el                              | 9 +++++----
 lisp/emacs-lisp/eieio-core.el                            | 3 ++-
 .../bytecomp-resources/warn-obsolete-variable-bound.el   | 7 +++++++
 test/lisp/emacs-lisp/bytecomp-tests.el                   | 3 +++
 4 files changed, 17 insertions(+), 5 deletions(-)
 create mode 100644 
test/lisp/emacs-lisp/bytecomp-resources/warn-obsolete-variable-bound.el

diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index f14ad93d2e..aa531c2558 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -3441,10 +3441,11 @@ byte-compile-check-variable
            (and od
                 (not (memq var byte-compile-not-obsolete-vars))
                 (not (memq var byte-compile-global-not-obsolete-vars))
-                (or (pcase (nth 1 od)
-                      ('set (not (eq access-type 'reference)))
-                      ('get (eq access-type 'reference))
-                      (_ t)))))
+                (not (memq var byte-compile-lexical-variables))
+                (pcase (nth 1 od)
+                  ('set (not (eq access-type 'reference)))
+                  ('get (eq access-type 'reference))
+                  (_ t))))
         (byte-compile-warn-obsolete var))))

 (defsubst byte-compile-dynamic-variable-op (base-op var)
diff --git a/lisp/emacs-lisp/eieio-core.el b/lisp/emacs-lisp/eieio-core.el
index 3bc65d0d4c..010ff54d4d 100644
--- a/lisp/emacs-lisp/eieio-core.el
+++ b/lisp/emacs-lisp/eieio-core.el
@@ -215,7 +215,8 @@ eieio-defclass-autoload
       ;; turn this into a usable self-pointing symbol
       (when eieio-backward-compatibility
         (set cname cname)
-        (make-obsolete-variable cname (format "use \\='%s instead" cname)
+        (make-obsolete-variable cname (format "\
+use \\='%s or turn off `eieio-backward-compatibility' instead" cname)
                                 "25.1"))

       (setf (cl--find-class cname) newc)
diff --git 
a/test/lisp/emacs-lisp/bytecomp-resources/warn-obsolete-variable-bound.el 
b/test/lisp/emacs-lisp/bytecomp-resources/warn-obsolete-variable-bound.el
new file mode 100644
index 0000000000..e65a541e6e
--- /dev/null
+++ b/test/lisp/emacs-lisp/bytecomp-resources/warn-obsolete-variable-bound.el
@@ -0,0 +1,7 @@
+;;; -*- lexical-binding: t -*-
+
+(make-obsolete-variable 'bytecomp--tests-obsolete-var-2 nil "99.99")
+
+(defun foo ()
+  (let ((bytecomp--tests-obsolete-var-2 2))
+    bytecomp--tests-obsolete-var-2))
diff --git a/test/lisp/emacs-lisp/bytecomp-tests.el 
b/test/lisp/emacs-lisp/bytecomp-tests.el
index 47aab563f6..c4a27b9ef8 100644
--- a/test/lisp/emacs-lisp/bytecomp-tests.el
+++ b/test/lisp/emacs-lisp/bytecomp-tests.el
@@ -625,6 +625,9 @@ "warn-obsolete-variable-same-file.el"
 (bytecomp--define-warning-file-test "warn-obsolete-variable.el"
                             "bytecomp--tests-obs.*obsolete.*99.99")

+(bytecomp--define-warning-file-test "warn-obsolete-variable-bound.el"
+                            "bytecomp--tests-obs.*obsolete.*99.99" t)
+
 (bytecomp--define-warning-file-test "warn-redefine-defun-as-macro.el"
                             "as both function and macro")

--
2.29.2


TIA,

Michael.