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

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

[Michael Heerdegen]

Hello,

I'm currently developing a Gnu Elpa package that makes use of
`defclass'.  These lines in `eieio-defclass-autoload':

#+begin_src emacs-lisp
      ;; turn this into a usable self-pointing symbol
      (when eieio-backward-compatibility
        (set cname cname)
        (make-obsolete-variable cname (format "use \\='%s instead" cname)
                                "25.1"))
#+end_src

(and eieio-backward-compatibility defaults to t) lead to the following
situation: when I have any class, for example, named `buffer-note', and
I have the generated autoloads loaded, whenever I use a variable with
the name `buffer-note' (which is a quite natural name for objects of
that class), I get tons of warnings saying:

| buffer-note.el:136:11:Warning: `buffer-note' is an obsolete
|     variable (as of 25.1); use 'buffer-note

The purpose of these warnings is a backward compatibility one, but it
shoots way over target: these warnings prevent me from using the class
name as a variable name - I keep renaming variables to prevent these
annoying warnings all the time.  They are obviously very confusing if
you don't know the background internals, unless you really have hit the
addressed case (and of course following the instruction is not
expedient).  And it's hard to get rid of them: because the
`make-obsolete-variable' are in the autoloads, not even a file-local
eieio-backward-compatibility setting helps.

Please reconsider how these warnings are implemented.  If no better
solution can be found, the warning message should at least be made more
meaningful and tell which case it addresses, and there should be a
(discoverable!) way to turn off these warnings (file locally).


TIA,

Michael.


In GNU Emacs 28.0.50 (build 23, x86_64-pc-linux-gnu, GTK+ Version 3.24.13, cairo version 1.16.0)
 of 2020-01-17 built on drachen
Repository revision: 4d3ac4cddbe1960f5227d14bd0d357a7b19c1296
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12006000
System Description: Debian GNU/Linux bullseye/sid

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

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> I'm currently developing a Gnu Elpa package that makes use of
> `defclass'.  These lines in `eieio-defclass-autoload':
>
> #+begin_src emacs-lisp
>       ;; turn this into a usable self-pointing symbol
>       (when eieio-backward-compatibility
>         (set cname cname)
>         (make-obsolete-variable cname (format "use \\='%s instead" cname)
>                                 "25.1"))
> #+end_src
>
> (and eieio-backward-compatibility defaults to t) lead to the following
> situation: when I have any class, for example, named `buffer-note', and
> I have the generated autoloads loaded, whenever I use a variable with
> the name `buffer-note' (which is a quite natural name for objects of
> that class), I get tons of warnings saying:
>
> | buffer-note.el:136:11:Warning: `buffer-note' is an obsolete
> |     variable (as of 25.1); use 'buffer-note
>
> The purpose of these warnings is a backward compatibility one, but it
> shoots way over target: these warnings prevent me from using the class
> name as a variable name - I keep renaming variables to prevent these
> annoying warnings all the time.

Yes, that does sound annoying.  eieio got a lot of warnings during the
conversion to more normal cl-* functions the other year, so it made
sense to add warnings like this during the rewrite.  But that's done
now, so perhaps it makes sense to remove these over-enthusiastic
warnings now?  Stefan?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Stefan Monnier]

>> I'm currently developing a Gnu Elpa package that makes use of
>> `defclass'.  These lines in `eieio-defclass-autoload':
>>
>> #+begin_src emacs-lisp
>>       ;; turn this into a usable self-pointing symbol
>>       (when eieio-backward-compatibility
>>         (set cname cname)
>>         (make-obsolete-variable cname (format "use \\='%s instead" cname)
>>                                 "25.1"))
>> #+end_src
>>
>> (and eieio-backward-compatibility defaults to t) lead to the following
>> situation: when I have any class, for example, named `buffer-note', and
>> I have the generated autoloads loaded, whenever I use a variable with
>> the name `buffer-note' (which is a quite natural name for objects of
>> that class), I get tons of warnings saying:
>>
>> | buffer-note.el:136:11:Warning: `buffer-note' is an obsolete
>> |     variable (as of 25.1); use 'buffer-note
>>
>> The purpose of these warnings is a backward compatibility one, but it
>> shoots way over target: these warnings prevent me from using the class
>> name as a variable name - I keep renaming variables to prevent these
>> annoying warnings all the time.

Indeed that happens when you re-use the identifier `buffer-note` which has
a global binding and our obsolescence warning doesn't know how to
distinguish local bindings from global bindings.

But you could set `eieio-backward-compatibility` to nil.

> Yes, that does sound annoying.  eieio got a lot of warnings during the
> conversion to more normal cl-* functions the other year, so it made
> sense to add warnings like this during the rewrite.  But that's done
> now, so perhaps it makes sense to remove these over-enthusiastic
> warnings now?  Stefan?

You mean we should change the default of `eieio-backward-compatibility`
to nil?

Maybe you're right.

We're currently removing things that were made obsolete in Emacs-23,
whereas those were made obsolete in Emacs-25, but setting that var to
nil is not as drastic as removing those vars and functions, so it's
probably OK to do it already.


        Stefan

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

[Lars Ingebrigtsen]

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

> You mean we should change the default of `eieio-backward-compatibility`
> to nil?

No, I meant removing the warning, since we no longer have any in-tree
usage of this stuff (I think).

> Maybe you're right.
>
> We're currently removing things that were made obsolete in Emacs-23,
> whereas those were made obsolete in Emacs-25, but setting that var to
> nil is not as drastic as removing those vars and functions, so it's
> probably OK to do it already.

Hm...  it does sound a bit drastic...  But now I'm wondering why Michael
isn't just setting/binding eieio-backward-compatibility to nil?  Then
the warnings would go away, and he probably aren't using the compat
symbols, anyway?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Hm...  it does sound a bit drastic...  But now I'm wondering why Michael
> isn't just setting/binding eieio-backward-compatibility to nil?  Then
> the warnings would go away, and he probably aren't using the compat
> symbols, anyway?                              ^
                                                |
Insert grammer here -----------------------------

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> But now I'm wondering why Michael isn't just setting/binding
> eieio-backward-compatibility to nil?  Then the warnings would go away,
> and he probably aren't using the compat symbols, anyway?

AFAIR, the situation is: I can do that file locally in my library, yes.

But any user of the library will still see those warnings.  It's likely
that this user uses the name "buffer-note" for a buffer-note, and the
warning text is meaningless for anybody who doesn't know what's going
on.

Michael.

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

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> But now I'm wondering why Michael isn't just setting/binding
>> eieio-backward-compatibility to nil?  Then the warnings would go away,
>> and he probably aren't using the compat symbols, anyway?
>
> AFAIR, the situation is: I can do that file locally in my library, yes.
>
> But any user of the library will still see those warnings.  It's likely
> that this user uses the name "buffer-note" for a buffer-note, and the
> warning text is meaningless for anybody who doesn't know what's going
> on.

OK, but then I think the right thing here is just to punt further, to
that user of the library.  :-)  If they don't want warnings about these
obsolete compat variables, then they are the ones that should set
eieio-backward-compatibility to nil?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> OK, but then I think the right thing here is just to punt further, to
> that user of the library.  :-)  If they don't want warnings about these
> obsolete compat variables, then they are the ones that should set
> eieio-backward-compatibility to nil?

Fine for me - in principle.  Note the word "confusing" in my bug report
title.  How long do you think would a average user (not every programmer
knows the internals of eieio) derive from the warning

  Warning: `buffer-note' is an obsolete variable (as of 25.1); use
  'buffer-note

that emacs wants him to add a file local binding
eieio-backward-compatibility -> nil?


Michael.

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

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> that emacs wants him to add a file local binding
> eieio-backward-compatibility -> nil?

Note that I didn't check whether this actually (and always) works: The
problematic obsolete variable declaration is performed in the function
`eieio-defclass-autoload'.  If the value of
`eieio-backward-compatibility' is checked when loading autoload
definitions, will a file local binding in the source library be
considered at all?

Oh, and let me add another important aspect: why does using an obsolete
name as the name of a _lexical_ variable trigger the "variable is
obsolete" warning at all?  If that would not be the case (and I don't
think it is useful) then in source files using lexical binding mode we
would not see the problem.

Michael.

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

[Michael Heerdegen]

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

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Oh, and let me add another important aspect: why does using an obsolete
> name as the name of a _lexical_ variable trigger the "variable is
> obsolete" warning at all?  If that would not be the case (and I don't
> think it is useful) then in source files using lexical binding mode we
> would not see the problem.

Would doing something like this make sense?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: bug.patch --]
[-- Type: text/x-diff, Size: 608 bytes --]

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))

[-- Attachment #3: Type: text/plain, Size: 709 bytes --]



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'?


Michael.

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

[Stefan Monnier]

> Note that I didn't check whether this actually (and always) works: The
> problematic obsolete variable declaration is performed in the function
> `eieio-defclass-autoload'.  If the value of
> `eieio-backward-compatibility' is checked when loading autoload
> definitions, will a file local binding in the source library be
> considered at all?

Good question.  I did not pay attention to non-global values of
`eieio-backward-compatibility` when writing that code, so I don't know
whether it would work well (or at all).

>> Oh, and let me add another important aspect: why does using an obsolete
>> name as the name of a _lexical_ variable trigger the "variable is
>> obsolete" warning at all?  If that would not be the case (and I don't
>> think it is useful) then in source files using lexical binding mode we
>> would not see the problem.
>
> Would doing something like this make sense?
>
> 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!

> 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.

Both changes should ideally come with corresponding regression tests.


        Stefan

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

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Fine for me - in principle.  Note the word "confusing" in my bug report
> title.  How long do you think would a average user (not every programmer
> knows the internals of eieio) derive from the warning
>
>   Warning: `buffer-note' is an obsolete variable (as of 25.1); use
>   'buffer-note
>
> that emacs wants him to add a file local binding
> eieio-backward-compatibility -> nil?

Yeah, that's true.  Could we add that to the obsolete warning (instead
of the confusing "'buffer-note")?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Michael Heerdegen]

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

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?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-obsolete-variable-warnings-about-class-names.patch --]
[-- Type: text/x-diff, Size: 3954 bytes --]

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


[-- Attachment #3: Type: text/plain, Size: 17 bytes --]



TIA,

Michael.

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

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> 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?

Installed.

Then we should be done here, and I'm closing this report.

Thanks to all,

Michael.