GNU bug report logs - #25088
25.1; feature-unload and reload of cl-defstruct fails

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 25088 in the body.
You can then email your comments to 25088 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: bug-gnu-emacs <at> gnu.org
Subject: 25.1; feature-unload and reload of cl-defstruct fails
Date: Fri, 02 Dec 2016 00:24:04 -0500

[Message part 1 (text/plain, inline)]

tags: patch

Running

    emacs -Q -l bug-struct-reload.el --eval "(unload-feature 'bug-struct-reload)" -l bug-struct-reload.el

Where bug-struct-reload.el contains

    (eval-when-compile (require 'cl-lib))
    (cl-defstruct foo f1)
    (provide 'bug-struct-reload)

Shows in *Messages* the following error

    Unexpected element (define-type . foo) in load-history
    Compiler-macro error for foo-p: (void-function foo-p--cmacro) [2 times]

This is because cl-defstruct defines the field accessors before the
predicate.  After calling `feature-unload', the `macro-compiler' symbol
property remains on the predicate even though the function itself is
undefined.  Then when reloading, the compiler tries to call the
predicate's compiler-macro to inline it in the accessor function, and
fails to find the definition.

Since this is a regression in 25.1, I'd like to apply the following
patch to emacs-25, which simply puts the predicate definition before the
accessor functions.

[v1-0001-Define-struct-predicate-before-acccesors.patch (text/plain, inline)]

From d6285c44150fc9f6d0d3d6dadcd272bae3c498e5 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs <at> gmail.com>
Date: Fri, 2 Dec 2016 00:03:57 -0500
Subject: [PATCH v1] Define struct predicate before acccesors

The accessor functions use the predicate function, which causes problems
when reloading after unload-feature: the compiler-macro property is
still present on the predicate symbol, and the compiler fails to find
the definition when trying to inline it into the accessor function.

* lisp/emacs-lisp/cl-macs.el (cl-defstruct): Move predicate definition
before field accessor definitions.
---
 lisp/emacs-lisp/cl-macs.el | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index c51ed9d..b3a60b1 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -2687,6 +2687,14 @@ cl-defstruct
 				   (= safety 1))
 			      (cons 'and (cl-cdddr pred-form))
                             `(,predicate cl-x))))
+    (when pred-form
+      (push `(cl-defsubst ,predicate (cl-x)
+               (declare (side-effect-free error-free))
+               ,(if (eq (car pred-form) 'and)
+                    (append pred-form '(t))
+                  `(and ,pred-form t)))
+            forms)
+      (push `(put ',name 'cl-deftype-satisfies ',predicate) forms))
     (let ((pos 0) (descp descs))
       (while descp
 	(let* ((desc (pop descp))
@@ -2741,14 +2749,6 @@ cl-defstruct
 	(setq pos (1+ pos))))
     (setq slots (nreverse slots)
 	  defaults (nreverse defaults))
-    (when pred-form
-      (push `(cl-defsubst ,predicate (cl-x)
-               (declare (side-effect-free error-free))
-               ,(if (eq (car pred-form) 'and)
-                    (append pred-form '(t))
-                  `(and ,pred-form t)))
-            forms)
-      (push `(put ',name 'cl-deftype-satisfies ',predicate) forms))
     (and copier
          (push `(defalias ',copier #'copy-sequence) forms))
     (if constructor
-- 
2.9.3

Message #8 received at 25088 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: npostavs <at> users.sourceforge.net
Cc: 25088 <at> debbugs.gnu.org
Subject: Re: bug#25088: 25.1; feature-unload and reload of cl-defstruct fails
Date: Fri, 02 Dec 2016 10:13:44 +0200

> From: npostavs <at> users.sourceforge.net
> Date: Fri, 02 Dec 2016 00:24:04 -0500
> 
> Running
> 
>     emacs -Q -l bug-struct-reload.el --eval "(unload-feature 'bug-struct-reload)" -l bug-struct-reload.el
> 
> Where bug-struct-reload.el contains
> 
>     (eval-when-compile (require 'cl-lib))
>     (cl-defstruct foo f1)
>     (provide 'bug-struct-reload)
> 
> Shows in *Messages* the following error
> 
>     Unexpected element (define-type . foo) in load-history
>     Compiler-macro error for foo-p: (void-function foo-p--cmacro) [2 times]
> 
> This is because cl-defstruct defines the field accessors before the
> predicate.  After calling `feature-unload', the `macro-compiler' symbol
> property remains on the predicate even though the function itself is
> undefined.  Then when reloading, the compiler tries to call the
> predicate's compiler-macro to inline it in the accessor function, and
> fails to find the definition.
> 
> Since this is a regression in 25.1, I'd like to apply the following
> patch to emacs-25, which simply puts the predicate definition before the
> accessor functions.

How risky is this change?  cl-defstruct is a very widely used macro,
whereas unload-feature is a relatively obscure feature.  Is it really
worth fixing the (IMO minor) error and risking to break Emacs 25.2?

I don't have an intuition I can trust in these matters, so I need you
and others who do to offer their opinions, after carefully considering
the pros and cons.

Thanks.

Message #11 received at 25088 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 25088 <at> debbugs.gnu.org
Subject: Re: bug#25088: 25.1; feature-unload and reload of cl-defstruct fails
Date: Sat, 03 Dec 2016 17:38:36 -0500

Eli Zaretskii <eliz <at> gnu.org> writes:
>
> How risky is this change?  cl-defstruct is a very widely used macro,
> whereas unload-feature is a relatively obscure feature.  Is it really
> worth fixing the (IMO minor) error and risking to break Emacs 25.2?
>
> I don't have an intuition I can trust in these matters, so I need you
> and others who do to offer their opinions, after carefully considering
> the pros and cons.

I think it's safe, but I've been wrong before.  I agree that
unload-feature is sufficiently obscure that this can go to master
instead of emacs-25.

Perhaps the real bug is that unload-feature doesn't undo `put', thus the
compiler-macro still hangs around.

Message #14 received at 25088 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: npostavs <at> users.sourceforge.net
Cc: 25088 <at> debbugs.gnu.org
Subject: Re: bug#25088: 25.1; feature-unload and reload of cl-defstruct fails
Date: Sun, 04 Dec 2016 05:35:32 +0200

> From: npostavs <at> users.sourceforge.net
> Cc: 25088 <at> debbugs.gnu.org
> Date: Sat, 03 Dec 2016 17:38:36 -0500
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> >
> > How risky is this change?  cl-defstruct is a very widely used macro,
> > whereas unload-feature is a relatively obscure feature.  Is it really
> > worth fixing the (IMO minor) error and risking to break Emacs 25.2?
> >
> > I don't have an intuition I can trust in these matters, so I need you
> > and others who do to offer their opinions, after carefully considering
> > the pros and cons.
> 
> I think it's safe, but I've been wrong before.  I agree that
> unload-feature is sufficiently obscure that this can go to master
> instead of emacs-25.

Thanks.  Any other opinions?

I will leave it for a few days for others to chime in.

> Perhaps the real bug is that unload-feature doesn't undo `put', thus the
> compiler-macro still hangs around.

If this means there could be another, safer way of fixing this, please
show the details.

Message #17 received at 25088 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 25088 <at> debbugs.gnu.org
Subject: Re: bug#25088: 25.1; feature-unload and reload of cl-defstruct fails
Date: Fri, 09 Dec 2016 00:08:28 -0500

Eli Zaretskii <eliz <at> gnu.org> writes:
>> Perhaps the real bug is that unload-feature doesn't undo `put', thus the
>> compiler-macro still hangs around.
>
> If this means there could be another, safer way of fixing this, please
> show the details.

Not sure how much safer this is, I think we would have to record the
which symbol plists are being modified during `load' so that
`unload-feature' could find them in `load-history' and reverse them
along with functions definitions.  This would get rid of the
compiler-macro entries that were added to cl-defstruct accessor function
symbols, and so they would load successfully the second time round just
like the first (presumably).

Message #20 received at 25088 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: npostavs <at> users.sourceforge.net,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 25088 <at> debbugs.gnu.org
Subject: Re: bug#25088: 25.1; feature-unload and reload of cl-defstruct fails
Date: Fri, 09 Dec 2016 10:22:58 +0200

> From: npostavs <at> users.sourceforge.net
> Cc: 25088 <at> debbugs.gnu.org
> Date: Fri, 09 Dec 2016 00:08:28 -0500
> 
> > If this means there could be another, safer way of fixing this, please
> > show the details.
> 
> Not sure how much safer this is, I think we would have to record the
> which symbol plists are being modified during `load' so that
> `unload-feature' could find them in `load-history' and reverse them
> along with functions definitions.  This would get rid of the
> compiler-macro entries that were added to cl-defstruct accessor function
> symbols, and so they would load successfully the second time round just
> like the first (presumably).

It could be safer because it doesn't change cl-defstruct.  But it's
hard to tell without seeing an implementation.

What do others think?  Is the patch proposed by Noam safe enough for
the release branch?  Stefan?  Dmitry?

Thanks.

Message #23 received at 25088 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: npostavs <at> users.sourceforge.net
Cc: Eli Zaretskii <eliz <at> gnu.org>, 25088 <at> debbugs.gnu.org
Subject: Re: bug#25088: 25.1; feature-unload and reload of cl-defstruct fails
Date: Fri, 09 Dec 2016 11:27:29 -0500

> I think it's safe, but I've been wrong before.

I also think it's safe, and I've also been wrong before.

Another thing: the patch makes sense regardless of the change, since
it's good to define the things before we use them.

> Not sure how much safer this is, I think we would have to record the
> which symbol plists are being modified during `load' so that
> `unload-feature' could find them in `load-history' and reverse them
> along with functions definitions.  This would get rid of the
> compiler-macro entries that were added to cl-defstruct accessor function
> symbols, and so they would load successfully the second time round just
> like the first (presumably).

Yes, that would be good.  Currently unload-feature automatically un-does
defalias and defvar, more or less, but it would be good to extend this
to other top-level operations like `put`.


        Stefan

Message #26 received at 25088 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Eli Zaretskii <eliz <at> gnu.org>, npostavs <at> users.sourceforge.net,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 25088 <at> debbugs.gnu.org
Subject: Re: bug#25088: 25.1; feature-unload and reload of cl-defstruct fails
Date: Sat, 10 Dec 2016 01:33:57 +0200

On 09.12.2016 10:22, Eli Zaretskii wrote:

> What do others think?  Is the patch proposed by Noam safe enough for
> the release branch?  Stefan?  Dmitry?

Personally, the first option sounds safer for me (for the release 
branch), because the other one might affect many packages and not just 
this one.

Message #29 received at 25088 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: monnier <at> iro.umontreal.ca, 25088 <at> debbugs.gnu.org,
 npostavs <at> users.sourceforge.net
Subject: Re: bug#25088: 25.1; feature-unload and reload of cl-defstruct fails
Date: Sat, 10 Dec 2016 09:18:29 +0200

> Cc: 25088 <at> debbugs.gnu.org
> From: Dmitry Gutov <dgutov <at> yandex.ru>
> Date: Sat, 10 Dec 2016 01:33:57 +0200
> 
> On 09.12.2016 10:22, Eli Zaretskii wrote:
> 
> > What do others think?  Is the patch proposed by Noam safe enough for
> > the release branch?  Stefan?  Dmitry?
> 
> Personally, the first option sounds safer for me (for the release 
> branch), because the other one might affect many packages and not just 
> this one.

By "first option" do you mean the original patch posted by Noam?

If so, given a similar opinion from Stefan, let's install that on
emacs-25.

Message #32 received at 25088 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: monnier <at> iro.umontreal.ca, 25088 <at> debbugs.gnu.org,
 npostavs <at> users.sourceforge.net
Subject: Re: bug#25088: 25.1; feature-unload and reload of cl-defstruct fails
Date: Sat, 10 Dec 2016 11:49:42 +0200

On 10.12.2016 09:18, Eli Zaretskii wrote:

> By "first option" do you mean the original patch posted by Noam?

Yup!

Message #35 received at 25088 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: monnier <at> iro.umontreal.ca, 25088 <at> debbugs.gnu.org,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#25088: 25.1; feature-unload and reload of cl-defstruct fails
Date: Sat, 10 Dec 2016 16:05:04 -0500

tags 25088 fixed
close 25088 25.2
quit

Eli Zaretskii <eliz <at> gnu.org> writes:

>> Cc: 25088 <at> debbugs.gnu.org
>> From: Dmitry Gutov <dgutov <at> yandex.ru>
>> Date: Sat, 10 Dec 2016 01:33:57 +0200
>> 
>> On 09.12.2016 10:22, Eli Zaretskii wrote:
>> 
>> > What do others think?  Is the patch proposed by Noam safe enough for
>> > the release branch?  Stefan?  Dmitry?
>> 
>> Personally, the first option sounds safer for me (for the release 
>> branch), because the other one might affect many packages and not just 
>> this one.
>
> By "first option" do you mean the original patch posted by Noam?
>
> If so, given a similar opinion from Stefan, let's install that on
> emacs-25.

Okay, pushed as e4ac4507968b.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.