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

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

[Adam Porter]

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

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

[Arthur Miller]

Adam Porter <adam@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? :)

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

[Lars Ingebrigtsen]

Adam Porter <adam@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

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

[Adam Porter]

On Thu, Aug 26, 2021 at 2:37 PM Lars Ingebrigtsen <larsi@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.

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

[Philipp]

> Am 26.08.2021 um 22:52 schrieb Adam Porter <adam@alphapapa.net>:
> 
> On Thu, Aug 26, 2021 at 2:37 PM Lars Ingebrigtsen <larsi@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.

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

[Lars Ingebrigtsen]

Philipp <p.stephani2@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?

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

[Adam Porter]

Hi Lars,

Thanks for following up on this report.

On 8/22/22 09:35, Lars Ingebrigtsen wrote:
> Philipp <p.stephani2@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

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

[Lars Ingebrigtsen]

Adam Porter <adam@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.