GNU bug report logs - #50214
28.0.50; cl-struct changes may affect user packages in the wild

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 50214 in the body.
You can then email your comments to 50214 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: Adam Porter <adam <at> alphapapa.net>
To: bug-gnu-emacs <at> gnu.org
Subject: 28.0.50; cl-struct changes may affect user packages in the wild
Date: Thu, 26 Aug 2021 11:47:09 -0500

Hi,

A helpful user of my ts.el package discovered a change in Emacs 28
that breaks it:

https://github.com/alphapapa/ts.el/issues/18

Specifically, this commit changes the internal struct constructor from
a plist to an alist:

https://github.com/emacs-mirror/emacs/commit/3788d2237d4c65b67b95e33d1aca8d8b41780429

For example:

;; Emacs 28.0.50
(nth 1 (cl-struct-slot-info 'ts))
;;  =>
;; (hour
;;  nil
;;  :type integer
;;  (:accessor-init string-to-number
;;   (format-time-string
;;    "%H"
;;    (ts-unix struct)))
;;  (:aliases H)
;;  (:constructor . "%H"))

;; Emacs 27.2.50
(nth 1 (cl-struct-slot-info 'ts))
;;  =>
;; (hour
;;  nil
;;  :type integer
;;  :accessor-init (string-to-number
;;                  (format-time-string
;;                   "%H"
;;                   (ts-unix struct)))
;;  :aliases (H)
;;  :constructor "%H")

Unfortunately this breaks how ts.el works.  Of course, that can be
worked around with a version check, but that means that users who
upgrade to Emacs 28 without upgrading ts.el will encounter failures.

I don't know if this is something you'd want to reconsider.  I guess
there was a good reason for the changes being made.  And maybe what
ts.el is doing is considered unsupported, which would seem like a
not-unreasonable position.

Anyway, I'm reporting this so the issue is officially documented and
any decisions can be made accordingly.

Thanks,
Adam

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

From: Arthur Miller <arthur.miller <at> live.com>
To: Adam Porter <adam <at> alphapapa.net>
Cc: 50214 <at> debbugs.gnu.org
Subject: Re: bug#50214: 28.0.50; cl-struct changes may affect user packages
 in the wild
Date: Thu, 26 Aug 2021 19:50:38 +0200

Adam Porter <adam <at> alphapapa.net> writes:

> Hi,
>
> A helpful user of my ts.el package discovered a change in Emacs 28
> that breaks it:
>
> https://github.com/alphapapa/ts.el/issues/18
>
> Specifically, this commit changes the internal struct constructor from
> a plist to an alist:
>
> https://github.com/emacs-mirror/emacs/commit/3788d2237d4c65b67b95e33d1aca8d8b41780429
>
> For example:
>
> ;; Emacs 28.0.50
> (nth 1 (cl-struct-slot-info 'ts))
> ;;  =>
> ;; (hour
> ;;  nil
> ;;  :type integer
> ;;  (:accessor-init string-to-number
> ;;   (format-time-string
> ;;    "%H"
> ;;    (ts-unix struct)))
> ;;  (:aliases H)
> ;;  (:constructor . "%H"))
>
> ;; Emacs 27.2.50
> (nth 1 (cl-struct-slot-info 'ts))
> ;;  =>
> ;; (hour
> ;;  nil
> ;;  :type integer
> ;;  :accessor-init (string-to-number
> ;;                  (format-time-string
> ;;                   "%H"
> ;;                   (ts-unix struct)))
> ;;  :aliases (H)
> ;;  :constructor "%H")
>
> Unfortunately this breaks how ts.el works.  Of course, that can be
> worked around with a version check, but that means that users who
> upgrade to Emacs 28 without upgrading ts.el will encounter failures.
>
> I don't know if this is something you'd want to reconsider.  I guess
> there was a good reason for the changes being made.  And maybe what
> ts.el is doing is considered unsupported, which would seem like a
> not-unreasonable position.
>
> Anyway, I'm reporting this so the issue is officially documented and
> any decisions can be made accordingly.
>
> Thanks,
> Adam

So what do we learn for the future? Don't use internal representation in your
apps, it is internal for a reason? :)

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Adam Porter <adam <at> alphapapa.net>
Cc: 50214 <at> debbugs.gnu.org
Subject: Re: bug#50214: 28.0.50; cl-struct changes may affect user packages
 in the wild
Date: Thu, 26 Aug 2021 21:37:03 +0200

Adam Porter <adam <at> alphapapa.net> writes:

> A helpful user of my ts.el package discovered a change in Emacs 28
> that breaks it:
>
> https://github.com/alphapapa/ts.el/issues/18
>
> Specifically, this commit changes the internal struct constructor from
> a plist to an alist:

[...]

> I don't know if this is something you'd want to reconsider.  I guess
> there was a good reason for the changes being made.  And maybe what
> ts.el is doing is considered unsupported, which would seem like a
> not-unreasonable position.

I'm not very familiar with the internals of cl-struct, but if I
understand correctly, I think this is...  well...  an internal thing
that package writers should expect to change, so they shouldn't rely on
things like this.

But I've added Stefan to the CCs; perhaps he has some comments.

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

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

From: Adam Porter <adam <at> alphapapa.net>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 50214 <at> debbugs.gnu.org
Subject: Re: bug#50214: 28.0.50; cl-struct changes may affect user packages in
 the wild
Date: Thu, 26 Aug 2021 15:52:30 -0500

On Thu, Aug 26, 2021 at 2:37 PM Lars Ingebrigtsen <larsi <at> gnus.org> wrote:
>
> I'm not very familiar with the internals of cl-struct, but if I
> understand correctly, I think this is...  well...  an internal thing
> that package writers should expect to change, so they shouldn't rely on
> things like this.

Unfortunately, I know of no other way to implement what ts.el does
without modifying the accessors, which requires accessing the internal
struct details after it is defined.  Unless I've missed something, or
something has changed, of course.  Regardless, the library's been
working well for the almost 3 years since I wrote it, and it's used in
various packages now, even including a few not my own.  :)

Anyway, if I have to add an Emacs-version check, that's not a big
deal.  I'll just have to answer the inevitable "I upgraded to Emacs 28
and your package doesn't work anymore" reports.

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

From: Philipp <p.stephani2 <at> gmail.com>
To: Adam Porter <adam <at> alphapapa.net>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 50214 <at> debbugs.gnu.org
Subject: Re: bug#50214: 28.0.50; cl-struct changes may affect user packages in
 the wild
Date: Sat, 4 Sep 2021 19:37:18 +0200

> Am 26.08.2021 um 22:52 schrieb Adam Porter <adam <at> alphapapa.net>:
> 
> On Thu, Aug 26, 2021 at 2:37 PM Lars Ingebrigtsen <larsi <at> gnus.org> wrote:
>> 
>> I'm not very familiar with the internals of cl-struct, but if I
>> understand correctly, I think this is...  well...  an internal thing
>> that package writers should expect to change, so they shouldn't rely on
>> things like this.
> 
> Unfortunately, I know of no other way to implement what ts.el does
> without modifying the accessors, which requires accessing the internal
> struct details after it is defined.

I haven't checked the code in detail, but AIUI ts.el tries to initialize structure members lazily?  Why not just use a wrapper function for that?

(defun ts-hour (ts)
  (or (ts--hour ts)
      (setf (ts--hour ts) (string-to-number (format-time-string "%H" (ts-unix ts)))))

Here ts--hour is the actual accessor, which is private and shouldn't be used outside of ts.el.

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Philipp <p.stephani2 <at> gmail.com>
Cc: Adam Porter <adam <at> alphapapa.net>, 50214 <at> debbugs.gnu.org
Subject: Re: bug#50214: 28.0.50; cl-struct changes may affect user packages
 in the wild
Date: Mon, 22 Aug 2022 16:35:30 +0200

Philipp <p.stephani2 <at> gmail.com> writes:

>> Unfortunately, I know of no other way to implement what ts.el does
>> without modifying the accessors, which requires accessing the internal
>> struct details after it is defined.
>
> I haven't checked the code in detail, but AIUI ts.el tries to initialize structure members lazily?  Why not just use a wrapper function for that?
>
> (defun ts-hour (ts)
>   (or (ts--hour ts)
>       (setf (ts--hour ts) (string-to-number (format-time-string "%H" (ts-unix ts)))))
>
> Here ts--hour is the actual accessor, which is private and shouldn't be used outside of ts.el.

Adam, does this suggestion work for you?

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

From: Adam Porter <adam <at> alphapapa.net>
To: Lars Ingebrigtsen <larsi <at> gnus.org>, Philipp <p.stephani2 <at> gmail.com>
Cc: 50214 <at> debbugs.gnu.org
Subject: Re: bug#50214: 28.0.50; cl-struct changes may affect user packages in
 the wild
Date: Mon, 22 Aug 2022 16:24:24 -0500

Hi Lars,

Thanks for following up on this report.

On 8/22/22 09:35, Lars Ingebrigtsen wrote:
> Philipp <p.stephani2 <at> gmail.com> writes:
> 
>>> Unfortunately, I know of no other way to implement what ts.el does
>>> without modifying the accessors, which requires accessing the internal
>>> struct details after it is defined.
>>
>> I haven't checked the code in detail, but AIUI ts.el tries to initialize structure members lazily?  Why not just use a wrapper function for that?
>>
>> (defun ts-hour (ts)
>>    (or (ts--hour ts)
>>        (setf (ts--hour ts) (string-to-number (format-time-string "%H" (ts-unix ts)))))
>>
>> Here ts--hour is the actual accessor, which is private and shouldn't be used outside of ts.el.
> 
> Adam, does this suggestion work for you?

That seems like a good idea to me.  I guess if I define it as an inline 
function, it should have a similar optimization as the current code. 
And the implementation would be much simpler and future-proof as well. 
In hindsight, I should probably have tried that first.  :)

I'll see if I can implement this in ts.el soon and release a new version 
accordingly.  In the meantime, I guess this bug report should be closed. 
 Thanks to all for your help.

Adam

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Adam Porter <adam <at> alphapapa.net>
Cc: Philipp <p.stephani2 <at> gmail.com>, 50214 <at> debbugs.gnu.org
Subject: Re: bug#50214: 28.0.50; cl-struct changes may affect user packages
 in the wild
Date: Tue, 23 Aug 2022 12:08:27 +0200

Adam Porter <adam <at> alphapapa.net> writes:

> I'll see if I can implement this in ts.el soon and release a new
> version accordingly.  In the meantime, I guess this bug report should
> be closed.   Thanks to all for your help.

No problem; closing this bug report, then.

Previous Next

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