bug#25088: 25.1; feature-unload and reload of cl-defstruct fails

bug#25088: 25.1; feature-unload and reload of cl-defstruct fails

[npostavs]

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

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.


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 2027 bytes --]

From d6285c44150fc9f6d0d3d6dadcd272bae3c498e5 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@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

bug#25088: 25.1; feature-unload and reload of cl-defstruct fails

[Eli Zaretskii]

> From: npostavs@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.

bug#25088: 25.1; feature-unload and reload of cl-defstruct fails

[npostavs]

Eli Zaretskii <eliz@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.

bug#25088: 25.1; feature-unload and reload of cl-defstruct fails

[Eli Zaretskii]

> From: npostavs@users.sourceforge.net
> Cc: 25088@debbugs.gnu.org
> Date: Sat, 03 Dec 2016 17:38:36 -0500
> 
> Eli Zaretskii <eliz@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.

bug#25088: 25.1; feature-unload and reload of cl-defstruct fails

[npostavs]

Eli Zaretskii <eliz@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).

bug#25088: 25.1; feature-unload and reload of cl-defstruct fails

[Eli Zaretskii]

> From: npostavs@users.sourceforge.net
> Cc: 25088@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.

bug#25088: 25.1; feature-unload and reload of cl-defstruct fails

[Stefan Monnier]

> 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

bug#25088: 25.1; feature-unload and reload of cl-defstruct fails

[Dmitry Gutov]

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.

bug#25088: 25.1; feature-unload and reload of cl-defstruct fails

[Eli Zaretskii]

> Cc: 25088@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@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.

bug#25088: 25.1; feature-unload and reload of cl-defstruct fails

[Dmitry Gutov]

On 10.12.2016 09:18, Eli Zaretskii wrote:

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

Yup!

bug#25088: 25.1; feature-unload and reload of cl-defstruct fails

[npostavs]

tags 25088 fixed
close 25088 25.2
quit

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 25088@debbugs.gnu.org
>> From: Dmitry Gutov <dgutov@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.