bug#40562: [patch] Treat records as arrays in ert object comparisons and add support for cl-structs

bug#40562: [patch] Treat records as arrays in ert object comparisons and add support for cl-structs

[Clément Pit-Claudel]

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

Hi all,

With the introduction of native records ert lost the ability to peek inside structures when comparing unequal values:

(require 'ert)
(cl-defstruct xyz a b c)
(defvar xyz123 (make-xyz :a 1 :b 2 :c 3))
(defvar xyz143 (make-xyz :a 1 :b 4 :c 3))
(should (equal xyz123 xyz143))

Emacs 25 said this:

Test failed: ((should (equal xyz123 xyz143)) 
  :form (equal [cl-struct-xyz 1 2 3 nil] [cl-struct-xyz 1 4 3 nil]) 
  :value nil
  :explanation (array-elt 2 (different-atoms (2 "#x2" "?\x02") (4 "#x4" "?\x04"))))

Emacs 26 says this:

Test failed: ((should (equal xyz123 xyz143))
  :form (equal #s(xyz 1 2 3) #s(xyz 1 4 3))
  :value nil 
  :explanation (different-atoms #s(xyz 1 2 3) #s(xyz 1 4 3)))

The first attached patch fixes this for all records.  The second patch adds special support for cl-structs, to get this:

Test failed: ((should (equal xyz123 xyz143)) 
  :form (equal #s(xyz 1 2 3) #s(xyz 1 4 3))
  :value nil
  :explanation (struct-field c (different-atoms (2 "#x2" "?\x02") (4 "#x4" "?\x04"))))

Clément.

[-- Attachment #2: 0001-lisp-emacs-lisp-ert.el-ert-explain-equal-rec-Treat-r.patch --]
[-- Type: text/x-patch, Size: 942 bytes --]

From 1f9936f1f1f8428b9ddebec04a4a865719543cf4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Pit-Claudel?= <clement.pitclaudel@live.com>
Date: Sat, 11 Apr 2020 16:22:56 -0400
Subject: [PATCH 1/2] * lisp/emacs-lisp/ert.el (ert--explain-equal-rec): Treat
 records as arrays

---
 lisp/emacs-lisp/ert.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
index 764354b03b..74202183cc 100644
--- a/lisp/emacs-lisp/ert.el
+++ b/lisp/emacs-lisp/ert.el
@@ -515,7 +515,7 @@ ert--explain-equal-rec
                        `(cdr ,cdr-x)
                      (cl-assert (equal a b) t)
                      nil))))))))
-      ((pred arrayp)
+      ((or (pred arrayp) (pred recordp))
        ;; For mixed unibyte/multibyte string comparisons, make both multibyte.
        (when (and (stringp a)
                   (xor (multibyte-string-p a) (multibyte-string-p b)))
-- 
2.17.1


[-- Attachment #3: 0002-lisp-emacs-lisp-ert.el-ert-explain-equal-rec-Add-sup.patch --]
[-- Type: text/x-patch, Size: 1189 bytes --]

From 47eb414118ab41b32c613fcdbe86369b2efd4c56 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Pit-Claudel?= <clement.pitclaudel@live.com>
Date: Sat, 11 Apr 2020 16:23:47 -0400
Subject: [PATCH 2/2] * lisp/emacs-lisp/ert.el (ert--explain-equal-rec): Add
 support for cl-structs

---
 lisp/emacs-lisp/ert.el | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
index 74202183cc..e3666a84c0 100644
--- a/lisp/emacs-lisp/ert.el
+++ b/lisp/emacs-lisp/ert.el
@@ -515,6 +515,13 @@ ert--explain-equal-rec
                        `(cdr ,cdr-x)
                      (cl-assert (equal a b) t)
                      nil))))))))
+      ((pred cl-struct-p)
+       (cl-loop for slot in (cdr (cl-struct-slot-info (type-of a)))
+                for ai across a
+                for bi across b
+                for xf = (ert--explain-equal-rec ai bi)
+                do (when xf (cl-return `(struct-field ,(car slot) ,xf)))
+                finally (cl-assert (equal a b) t)))
       ((or (pred arrayp) (pred recordp))
        ;; For mixed unibyte/multibyte string comparisons, make both multibyte.
        (when (and (stringp a)
-- 
2.17.1

bug#40562: [patch] Treat records as arrays in ert object comparisons and add support for cl-structs

[Štěpán Němec]

On Sat, 11 Apr 2020 16:34:57 -0400
Clément Pit-Claudel wrote:

[...]

> (defvar xyz123 (make-xyz :a 1 :b 2 :c 3))
> (defvar xyz143 (make-xyz :a 1 :b 4 :c 3))

[...]

> The first attached patch fixes this for all records.

Seems like a nice addition.

> Test failed: ((should (equal xyz123 xyz143)) 
>   :form (equal #s(xyz 1 2 3) #s(xyz 1 4 3))
>   :value nil
>   :explanation (struct-field c (different-atoms (2 "#x2" "?") (4 "#x4" "?"))))
                               ^gotcha

>  lisp/emacs-lisp/ert.el | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
> index 74202183cc..e3666a84c0 100644
> --- a/lisp/emacs-lisp/ert.el
> +++ b/lisp/emacs-lisp/ert.el
> @@ -515,6 +515,13 @@ ert--explain-equal-rec
>                         `(cdr ,cdr-x)
>                       (cl-assert (equal a b) t)
>                       nil))))))))
> +      ((pred cl-struct-p)
> +       (cl-loop for slot in (cdr (cl-struct-slot-info (type-of a)))
> +                for ai across a
                   ^^^^^^^^^^^^^^^

This is incorrect, as witnessed by your very example (`c' instead of
`b'). Records are accessible with `aref', but the first slot is the type
descriptor, so you're making an off-by-one error here.

> +                for bi across b
> +                for xf = (ert--explain-equal-rec ai bi)
> +                do (when xf (cl-return `(struct-field ,(car slot) ,xf)))
> +                finally (cl-assert (equal a b) t)))
>        ((or (pred arrayp) (pred recordp))
>         ;; For mixed unibyte/multibyte string comparisons, make both multibyte.
>         (when (and (stringp a)

Thanks,

  Štěpán

bug#40562: [patch] Treat records as arrays in ert object comparisons and add support for cl-structs

[Clément Pit-Claudel]

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

On 11/04/2020 19.26, Štěpán Němec wrote:
> This is incorrect, as witnessed by your very example (`c' instead of
> `b'). Records are accessible with `aref', but the first slot is the type
> descriptor, so you're making an off-by-one error here.

Of course, it should be `for slot in (cl-struct-slot-info (type-of a))` not `for slot in (cdr (cl-struct-slot-info (type-of a)))`.  Updated patch attached.

Thanks for the review!

[-- Attachment #2: 0002-lisp-emacs-lisp-ert.el-ert-explain-equal-rec-Add-sup.patch --]
[-- Type: text/x-patch, Size: 1183 bytes --]

From 129ba8aeaa91b6ea44a325ec237f3df7e9400c9f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Pit-Claudel?= <clement.pitclaudel@live.com>
Date: Sat, 11 Apr 2020 16:23:47 -0400
Subject: [PATCH 2/2] * lisp/emacs-lisp/ert.el (ert--explain-equal-rec): Add
 support for cl-structs

---
 lisp/emacs-lisp/ert.el | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
index 74202183cc..241eece05b 100644
--- a/lisp/emacs-lisp/ert.el
+++ b/lisp/emacs-lisp/ert.el
@@ -515,6 +515,13 @@ ert--explain-equal-rec
                        `(cdr ,cdr-x)
                      (cl-assert (equal a b) t)
                      nil))))))))
+      ((pred cl-struct-p)
+       (cl-loop for slot in (cl-struct-slot-info (type-of a))
+                for ai across a
+                for bi across b
+                for xf = (ert--explain-equal-rec ai bi)
+                do (when xf (cl-return `(struct-field ,(car slot) ,xf)))
+                finally (cl-assert (equal a b) t)))
       ((or (pred arrayp) (pred recordp))
        ;; For mixed unibyte/multibyte string comparisons, make both multibyte.
        (when (and (stringp a)
-- 
2.17.1

bug#40562: [patch] Treat records as arrays in ert object comparisons and add support for cl-structs

[Štěpán Němec]

On Sat, 11 Apr 2020 23:06:47 -0400
Clément Pit-Claudel wrote:

> On 11/04/2020 19.26, Štěpán Němec wrote:
>> This is incorrect, as witnessed by your very example (`c' instead of
>> `b'). Records are accessible with `aref', but the first slot is the type
>> descriptor, so you're making an off-by-one error here.
>
> Of course, it should be `for slot in (cl-struct-slot-info (type-of
> a))` not `for slot in (cdr (cl-struct-slot-info (type-of a)))`.
> Updated patch attached.

Hm, except now you're duplicating the (eq (type-of a) (type-of b))
check. I'm not quite sure it's worth complicating the code to avoid
that, though, given how clumsy records currently seem to be to work with
(most sequence functions don't support them).

-- 
Štěpán

bug#40562: [patch] Treat records as arrays in ert object comparisons and add support for cl-structs

[Clément Pit-Claudel]

On 12/04/2020 05.24, Štěpán Němec wrote:
> On Sat, 11 Apr 2020 23:06:47 -0400
> Clément Pit-Claudel wrote:
> 
>> On 11/04/2020 19.26, Štěpán Němec wrote:
>>> This is incorrect, as witnessed by your very example (`c' instead of
>>> `b'). Records are accessible with `aref', but the first slot is the type
>>> descriptor, so you're making an off-by-one error here.
>>
>> Of course, it should be `for slot in (cl-struct-slot-info (type-of
>> a))` not `for slot in (cdr (cl-struct-slot-info (type-of a)))`.
>> Updated patch attached.
> 
> Hm, except now you're duplicating the (eq (type-of a) (type-of b))
> check.

Yup; I don't think that's a problem :)

bug#40562: [patch] Treat records as arrays in ert object comparisons and add support for cl-structs

[Clément Pit-Claudel]

On 12/04/2020 11.50, Clément Pit-Claudel wrote:
> On 12/04/2020 05.24, Štěpán Němec wrote:
>> On Sat, 11 Apr 2020 23:06:47 -0400
>> Clément Pit-Claudel wrote:
>>
>>> On 11/04/2020 19.26, Štěpán Němec wrote:
>>>> This is incorrect, as witnessed by your very example (`c' instead of
>>>> `b'). Records are accessible with `aref', but the first slot is the type
>>>> descriptor, so you're making an off-by-one error here.
>>>
>>> Of course, it should be `for slot in (cl-struct-slot-info (type-of
>>> a))` not `for slot in (cdr (cl-struct-slot-info (type-of a)))`.
>>> Updated patch attached.
>>
>> Hm, except now you're duplicating the (eq (type-of a) (type-of b))
>> check.
> 
> Yup; I don't think that's a problem :)

Is there anything else I should do before pushing this patch?

bug#40562: [patch] Treat records as arrays in ert object comparisons and add support for cl-structs

[Lars Ingebrigtsen]

Clément Pit-Claudel <cpitclaudel@gmail.com> writes:

> Is there anything else I should do before pushing this patch?

This was four months ago, so I guess you should go ahead and push the
patch.

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

bug#40562: [patch] Treat records as arrays in ert object comparisons and add support for cl-structs

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Clément Pit-Claudel <cpitclaudel@gmail.com> writes:
>
>> Is there anything else I should do before pushing this patch?
>
> This was four months ago, so I guess you should go ahead and push the
> patch.

That was a week ago, so I went ahead and pushed the patch to Emacs 28.

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