Emacs 28 cl-struct change breaks ts-fill

[tpeacock19]

Hello,

I have noticed that I'm no longer able to use any org-ql-view that relies on evaluating a timestamp.

You should be able to replicate this with the following

emacs -Q -l

(setq user-emacs-directory "~/.config/emacs/")
(require 'package)
(add-to-list 'package-archives
             '("melpa" . "https://melpa.org/packages/") t)
(package-initialize)
(package-install 'use-package)
(package-install 'quelpa-use-package)
(quelpa
 '(quelpa-use-package
   :fetcher git
   :url "https://github.com/quelpa/quelpa-use-package.git"))
(require 'quelpa-use-package)
(use-package org-ql
  :quelpa (org-ql :fetcher github :repo "alphapapa/org-ql"
            :files (:defaults (:exclude "helm-org-ql.el"))))
(toggle-debug-on-error)
(org-ql-view-recent-items :num-days 10)

I think I've narrowed it down to the fact that ts-fill does not seem to fill the relevant slots based on the unix timestamp. Perhaps this is better suited to be an issue in ts.el but I figured you are more active here.

(ts-now)
;;  =>
;;  #s(ts nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil 1625426631.6235018)

(ts-fill (ts-now))
;;  =>
;;  #s(ts nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil 1625426636.7569551)

I'm using
GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.24, cairo version 1.16.0)

Specifically the flatwhatson's pgtk-native-comp branch

emacs-pgtk-native-comp 28.0.50-197.e288348

[alphapapa]

Thanks for reporting. It's not practical for me to test that specific build of Emacs 28; can you reproduce it on Emacs 27.2 or on a master-branch build of Emacs 28.0?

[tpeacock19]

I'm able to reproduce with the above recipe on the master branch of Emacs 28 but it does not happen in the latest build of Emacs 27.2.50

[tpeacock19]

This is more of a generic question, but how do you go about debugging ts-fill? I can't use edebug because it is defined at runtime I think.

[alphapapa]

Before we go any further, it's possible this is a macro-expansion issue due to struct-related changes in different Emacs versions. Are you sharing any config between these different versions? I'd recommend using my emacs-sandbox.sh script to test each Emacs version with a clean configuration, installing ts into each one from scratch.

[alphapapa]

If the problem does occur with clean configurations, then it's probably happening due to some kind of internal struct-related changes in Emacs 28. ts-define-fill is a macro that defines the ts-fill function at load time depending on the slots defined in the ts struct.

To see what's happening, you could edebug the macro and step through it, or just expand the macro call with macroexpand and see what it expands to (especially the value-conversions part).

If that doesn't help, I can add a debug declaration to make it work with edebug, which should probably be done anyway.

[tpeacock19]

some further observations. It seems like the ts struct is slightly different between the two versions. The :constructor is made into an

;; 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")

which I've tracked to this commit on master emacs-mirror/emacs@3788d22. With the introduction of cl--plist-to-alist it's changed how ts-fill needs to lookup slots/constructor.

After way too much time due to my lack of familiarity, I believe this works with a clean version of Emacs 28.0.50 after the above commit:

diff --git a/ts.el b/ts.el
index c36443f..43f3a6e 100644
--- a/ts.el
+++ b/ts.el
@@ -398,15 +398,15 @@ to `make-ts'."
 (defmacro ts-define-fill ()
   "Define `ts-fill' function that fills all applicable slots of `ts' object from its `unix' slot."
   (let* ((slots (->> (cl-struct-slot-info 'ts)
-                     (--select (and (not (member (car it) '(unix internal cl-tag-slot)))
-                                    (plist-get (cddr it) :constructor)))
+                  (--select (and (not (member (car it) '(unix internal cl-tag-slot)))
+                                 (alist-get :constructor (cddr it))))
 
-                     (--map (list (intern (concat ":" (symbol-name (car it))))
-                                  (cddr it)))))
+                  (--map (list (intern (concat ":" (symbol-name (car it))))
+                               (cddr it)))))
          (keywords (-map #'car slots))
          (constructors (->> slots
-                            (--map (plist-get (cadr it) :constructor))
-                            -non-nil))
+                         (--map (alist-get :constructor (cadr it)))
+                         -non-nil))
          (types (--map (plist-get (cadr it) :type) slots))
          (format-string (s-join "\f" constructors))
          (value-conversions (cl-loop for type in types

I'm assuming something that would check the emacs version and using plist-get or alist-get as necessary would be the right move.

EDIT: sorry for the change in formatting/indentation above. I'm not sure why mine would differ from yours.

[tpeacock19]

I'd recommend using my emacs-sandbox.sh script to test each Emacs version with a clean configuration, installing ts into each one from scratch.

I actually built two clean versions of emacs after you had mentioned it. I ended up just changing the respective user-emacs-directory to separate locations. But going forward I think i'll use emacs-sandbox.sh as it looks simpler

[alphapapa]

Wow, you're fast. :) Tracking it down to the commit is great, and that is recent, indeed.

I wish this kind of change wasn't made, but I don't know if the way I'm using internal struct details is, well, allowed. I'm guessing Stefan might frown on it, and he's probably making this change for a good reason.

OTOH, I guess that this will break existing Emacs configs, as it broke yours, so it would require users to upgrade ts to a newer version. I can imagine receiving bug reports just like yours, and having to answer them all the same way. And he's surely not aware of this library, so he may not expect that change to have broken anything in the wild.

So it might be worth asking him about it, since the change hasn't made it to a released Emacs version yet. What do you think?

Thanks for all your work on this. I don't usually run Emacs master builds, so this could have gone unnoticed until it released, which would have been an unpleasant surprise.

[alphapapa]

(I'm going to try this newfangled--to me--transfer feature, to try to move this issue to the ts repo...)

[alphapapa]

...Seems to have worked...

[alphapapa]

Added GitHub Actions CI, which will test with Emacs 28: 099ec36

[alphapapa]

The test for ts-fill revealed an oversight, in that it should have always had a ZONE argument passed on to format-time-string. I added that in a new 0.3-pre version.

[alphapapa]

And running the tests on CI reveals that ts-format also needs a ZONE argument, otherwise the test results depend on the time zone of the machine they're run on (I think this is also mentioned on another issue somewhere...yes, #6).

[meedstrom]

Another thing related to the Emacs 28 cl struct. I get a lot of these near-identical warnings on compiling my package. Doesn't happen on Emacs 27.

In eva--pending-p:
eva.el:949:24: Warning: value returned from (memq (type-of last-called)
    cl-struct-ts-tags) is unused
eva.el:949:24: Warning: value returned from (memq (type-of struct)
    cl-struct-ts-tags) is unused

In eva--init:
eva.el:1376:8: Warning: value returned from (memq (type-of online)
    cl-struct-ts-tags) is unused
eva.el:1376:8: Warning: value returned from (memq (type-of chatted)
    cl-struct-ts-tags) is unused

And so on ad infinitum. If it matters, I use ts-fill simply for inspectability, no programmatic purpose.

[alphapapa]

@meedstrom Yes, I've seen those when I've tested on Emacs 28 at times recently. I hope it's fixable, but I haven't had time to look into it yet.

[alphapapa]

I reported this as https://debbugs.gnu.org/cgi/bugreport.cgi?bug=50214. I doubt they'll be willing to back out the change, but at least we'll have it documented.

[alphapapa]

Today Lars followed up on that report and made me aware of Philip Stefani's suggestion, which would be simpler and more future-proof than the current implementation:

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.

I think that should work, and if I define it as an inline function, it should be similarly optimized. So that is the current plan.