all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* bug#25088: 25.1; feature-unload and reload of cl-defstruct fails
@ 2016-12-02  5:24 npostavs
  2016-12-02  8:13 ` Eli Zaretskii
  0 siblings, 1 reply; 11+ messages in thread
From: npostavs @ 2016-12-02  5:24 UTC (permalink / raw)
  To: 25088

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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* bug#25088: 25.1; feature-unload and reload of cl-defstruct fails
  2016-12-02  5:24 bug#25088: 25.1; feature-unload and reload of cl-defstruct fails npostavs
@ 2016-12-02  8:13 ` Eli Zaretskii
  2016-12-03 22:38   ` npostavs
  0 siblings, 1 reply; 11+ messages in thread
From: Eli Zaretskii @ 2016-12-02  8:13 UTC (permalink / raw)
  To: npostavs; +Cc: 25088

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





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#25088: 25.1; feature-unload and reload of cl-defstruct fails
  2016-12-02  8:13 ` Eli Zaretskii
@ 2016-12-03 22:38   ` npostavs
  2016-12-04  3:35     ` Eli Zaretskii
  2016-12-09 16:27     ` Stefan Monnier
  0 siblings, 2 replies; 11+ messages in thread
From: npostavs @ 2016-12-03 22:38 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 25088

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.





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#25088: 25.1; feature-unload and reload of cl-defstruct fails
  2016-12-03 22:38   ` npostavs
@ 2016-12-04  3:35     ` Eli Zaretskii
  2016-12-09  5:08       ` npostavs
  2016-12-09 16:27     ` Stefan Monnier
  1 sibling, 1 reply; 11+ messages in thread
From: Eli Zaretskii @ 2016-12-04  3:35 UTC (permalink / raw)
  To: npostavs; +Cc: 25088

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





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#25088: 25.1; feature-unload and reload of cl-defstruct fails
  2016-12-04  3:35     ` Eli Zaretskii
@ 2016-12-09  5:08       ` npostavs
  2016-12-09  8:22         ` Eli Zaretskii
  0 siblings, 1 reply; 11+ messages in thread
From: npostavs @ 2016-12-09  5:08 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 25088

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





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#25088: 25.1; feature-unload and reload of cl-defstruct fails
  2016-12-09  5:08       ` npostavs
@ 2016-12-09  8:22         ` Eli Zaretskii
  2016-12-09 23:33           ` Dmitry Gutov
  0 siblings, 1 reply; 11+ messages in thread
From: Eli Zaretskii @ 2016-12-09  8:22 UTC (permalink / raw)
  To: npostavs, Stefan Monnier, Dmitry Gutov; +Cc: 25088

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





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#25088: 25.1; feature-unload and reload of cl-defstruct fails
  2016-12-03 22:38   ` npostavs
  2016-12-04  3:35     ` Eli Zaretskii
@ 2016-12-09 16:27     ` Stefan Monnier
  1 sibling, 0 replies; 11+ messages in thread
From: Stefan Monnier @ 2016-12-09 16:27 UTC (permalink / raw)
  To: npostavs; +Cc: 25088

> 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





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#25088: 25.1; feature-unload and reload of cl-defstruct fails
  2016-12-09  8:22         ` Eli Zaretskii
@ 2016-12-09 23:33           ` Dmitry Gutov
  2016-12-10  7:18             ` Eli Zaretskii
  0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Gutov @ 2016-12-09 23:33 UTC (permalink / raw)
  To: Eli Zaretskii, npostavs, Stefan Monnier; +Cc: 25088

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.





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#25088: 25.1; feature-unload and reload of cl-defstruct fails
  2016-12-09 23:33           ` Dmitry Gutov
@ 2016-12-10  7:18             ` Eli Zaretskii
  2016-12-10  9:49               ` Dmitry Gutov
  2016-12-10 21:05               ` npostavs
  0 siblings, 2 replies; 11+ messages in thread
From: Eli Zaretskii @ 2016-12-10  7:18 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: monnier, 25088, npostavs

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





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#25088: 25.1; feature-unload and reload of cl-defstruct fails
  2016-12-10  7:18             ` Eli Zaretskii
@ 2016-12-10  9:49               ` Dmitry Gutov
  2016-12-10 21:05               ` npostavs
  1 sibling, 0 replies; 11+ messages in thread
From: Dmitry Gutov @ 2016-12-10  9:49 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: monnier, 25088, npostavs

On 10.12.2016 09:18, Eli Zaretskii wrote:

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

Yup!





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#25088: 25.1; feature-unload and reload of cl-defstruct fails
  2016-12-10  7:18             ` Eli Zaretskii
  2016-12-10  9:49               ` Dmitry Gutov
@ 2016-12-10 21:05               ` npostavs
  1 sibling, 0 replies; 11+ messages in thread
From: npostavs @ 2016-12-10 21:05 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: monnier, 25088, Dmitry Gutov

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.





^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2016-12-10 21:05 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-12-02  5:24 bug#25088: 25.1; feature-unload and reload of cl-defstruct fails npostavs
2016-12-02  8:13 ` Eli Zaretskii
2016-12-03 22:38   ` npostavs
2016-12-04  3:35     ` Eli Zaretskii
2016-12-09  5:08       ` npostavs
2016-12-09  8:22         ` Eli Zaretskii
2016-12-09 23:33           ` Dmitry Gutov
2016-12-10  7:18             ` Eli Zaretskii
2016-12-10  9:49               ` Dmitry Gutov
2016-12-10 21:05               ` npostavs
2016-12-09 16:27     ` Stefan Monnier

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.