bug#6581: 24.0.50; `equal-including-properties' uses `eq' to compare property values

bug#6581: 24.0.50; `equal-including-properties' uses `eq' to compare property values

[Christian Ohler]

I just noticed that, in my current Emacs (24.0.50.1, built from trunk a 
few weeks ago):

(equal-including-properties #("a" 0 1 (k "v")) #("a" 0 1 (k "v")))
=> nil

This is because these two strings both have a property with key `k', but 
the values are two different strings "v", and 
`equal-including-properties' appears to use `eq' to compare property 
values (I didn't verify this in the code).  Is this a bug, or is it 
intentional?

For comparison,

(equal-including-properties #("a" 0 1 (k v)) #("a" 0 1 (k v)))
=> t

since `v' is an interned symbol.

My expectation was that, since `equal' is recursively defined in terms 
of itself, `equal-including-properties' should also call itself 
recursively on property values.

Christian.

bug#6581: 24.0.50; `equal-including-properties' uses `eq' to compare property values

[Dmitry Gutov]

I've just hit this bug today.  Half an hour wasted.

bug#6581: 24.0.50; `equal-including-properties' uses `eq' to compare property values

[Drew Adams]

> I've just hit this bug today.  Half an hour wasted.

There certainly is *at least* a doc bug.  Users of such
a function at least need to be told how the text property
values are compared.  That's a minimum for the doc of any
equality predicate: say just what equality means in this
case.

Both the doc string and the Elisp manual (node `Equality
Predicates') are deficient.

In addition, a reasonable enhancement might be (now) to
accept an optional comparison (equality) predicate,
especially to be able to work around the designed behavior.

But I'm guessing that the default behavior could/should
not be changed at this point, because existing code
could break.

Just call out the *actual behavior*, clearly and completely.
Even LOUDLY, as this is truly a gotcha.  (And one needs to
check C code to see what the function really does.)

This predicate was added after Emacs 20 (after 21 perhaps?).
It's a bit surprising that no one has thought of completing
the doc - at least.

bug#6581: 24.0.50; `equal-including-properties' uses `eq' to compare property values

[Dmitry Gutov]

On 01/13/2015 05:23 AM, Drew Adams wrote:

> There certainly is *at least* a doc bug.

Yup.

bug#6581: 24.0.50; `equal-including-properties' uses `eq' to compare property values

[Stefan Monnier]

> I've just hit this bug today.  Half an hour wasted.

The intention is to use `equal-including-properties' when
comparing the property values.


        Stefan

bug#6581: 24.0.50; `equal-including-properties' uses `eq' to compare property values

[Drew Adams]

> > I've just hit this bug today.  Half an hour wasted.
> 
> The intention is to use `equal-including-properties' when
> comparing the property values.

Whatever the intended use is, the behavior should be specified
in the predicate's doc.

bug#6581: 24.0.50; `equal-including-properties' uses `eq' to compare property values

[Dmitry Gutov]

On 01/13/2015 06:23 PM, Stefan Monnier wrote:

> The intention is to use `equal-including-properties' when
> comparing the property values.

So you agree the behavior should be fixed?

bug#6581: 24.0.50; `equal-including-properties' uses `eq' to compare property values

[Stefan Monnier]

>> The intention is to use `equal-including-properties' when
>> comparing the property values.
> So you agree the behavior should be fixed?

Yes.


        Stefan

bug#6581: 24.0.50; `equal-including-properties' uses `eq' to compare property values

[Stefan Kangas]

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

tags 6581 + patch
thanks

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> The intention is to use `equal-including-properties' when
>>> comparing the property values.
>> So you agree the behavior should be fixed?
>
> Yes.

The attached patch should fix it.

[-- Attachment #2: 0001-Fix-bug-with-string-values-in-equal-including-proper.patch --]
[-- Type: text/x-diff, Size: 4648 bytes --]

From 52cb8610cc4805be89ecf77285d9e6d1efaf9364 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Wed, 20 Oct 2021 14:16:07 +0200
Subject: [PATCH] Fix bug with string values in equal-including-properties

* src/intervals.c (intervals_equal_1): Factor out from
intervals_equal.  Use Fequal for comparison if third argument
use_equal is true.  This fixes a bug with string values in property
lists compared with 'equal-including-properties'.  (Bug#6581)
(intervals_equal): Update for the above.
(compare_string_intervals): Call intervals_equal1 with third
argument as true.
* src/intervals.h (intervals_equal_1): Declare.
* test/src/fns-tests.el (fns-tests-equal-including-properties)
(fns-tests-equal-including-properties/string-prop-vals): New tests.
---
 src/intervals.c       | 18 ++++++++++++++----
 src/intervals.h       |  1 +
 test/src/fns-tests.el | 19 +++++++++++++++++++
 3 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/src/intervals.c b/src/intervals.c
index f88a41f254..cef5d9a4d7 100644
--- a/src/intervals.c
+++ b/src/intervals.c
@@ -166,10 +166,11 @@ merge_properties (register INTERVAL source, register INTERVAL target)
     }
 }
 
-/* Return true if the two intervals have the same properties.  */
+/* Return true if the two intervals have the same properties.
+   If use_equal is true, use Fequal for comparisons instead of EQ.  */
 
 bool
-intervals_equal (INTERVAL i0, INTERVAL i1)
+intervals_equal_1 (INTERVAL i0, INTERVAL i1, bool use_equal)
 {
   Lisp_Object i0_cdr, i0_sym;
   Lisp_Object i1_cdr, i1_val;
@@ -204,7 +205,8 @@ intervals_equal (INTERVAL i0, INTERVAL i1)
       /* i0 and i1 both have sym, but it has different values in each.  */
       if (!CONSP (i1_val)
 	  || (i1_val = XCDR (i1_val), !CONSP (i1_val))
-	  || !EQ (XCAR (i1_val), XCAR (i0_cdr)))
+	  || (!use_equal && !EQ (XCAR (i1_val), XCAR (i0_cdr)))
+	  || (use_equal && NILP (Fequal (XCAR (i1_val), XCAR (i0_cdr)))))
 	return false;
 
       i0_cdr = XCDR (i0_cdr);
@@ -218,6 +220,14 @@ intervals_equal (INTERVAL i0, INTERVAL i1)
   /* Lengths of the two plists were equal.  */
   return (NILP (i0_cdr) && NILP (i1_cdr));
 }
+
+/* Return true if the two intervals have the same properties.  */
+
+bool
+intervals_equal (INTERVAL i0, INTERVAL i1)
+{
+  return intervals_equal_1 (i0, i1, false);
+}
 \f
 
 /* Traverse an interval tree TREE, performing FUNCTION on each node.
@@ -2291,7 +2301,7 @@ compare_string_intervals (Lisp_Object s1, Lisp_Object s2)
 
       /* If we ever find a mismatch between the strings,
 	 they differ.  */
-      if (! intervals_equal (i1, i2))
+      if (! intervals_equal_1 (i1, i2, true))
 	return 0;
 
       /* Advance POS till the end of the shorter interval,
diff --git a/src/intervals.h b/src/intervals.h
index c1b19345d2..4096dc02fd 100644
--- a/src/intervals.h
+++ b/src/intervals.h
@@ -243,6 +243,7 @@ #define TEXT_PROP_MEANS_INVISIBLE(prop)					\
 
 extern INTERVAL create_root_interval (Lisp_Object);
 extern void copy_properties (INTERVAL, INTERVAL);
+bool intervals_equal_1 (INTERVAL, INTERVAL, bool);
 extern bool intervals_equal (INTERVAL, INTERVAL);
 extern void traverse_intervals (INTERVAL, ptrdiff_t,
                                 void (*) (INTERVAL, Lisp_Object),
diff --git a/test/src/fns-tests.el b/test/src/fns-tests.el
index 3dc2e7b3ec..b4cb3c131a 100644
--- a/test/src/fns-tests.el
+++ b/test/src/fns-tests.el
@@ -57,6 +57,25 @@ fns-tests-equality-nan
       (puthash nan t h)
       (should (eq (funcall test nan -nan) (gethash -nan h))))))
 
+(ert-deftest fns-tests-equal-including-properties ()
+  (should (equal-including-properties "" ""))
+  (should (equal-including-properties "foobar" "foobar"))
+  (should (equal-including-properties #("a" 0 1 (k v))
+                                      #("a" 0 1 (k v))))
+  (should-not (equal-including-properties #("a" 0 1 (k v))
+                                          #("a" 0 1 (k x))))
+  (should-not (equal-including-properties #("a" 0 1 (k v))
+                                          #("b" 0 1 (k v)))))
+
+(ert-deftest fns-tests-equal-including-properties/string-prop-vals ()
+  "Handle string property values.  (Bug#6581)"
+  (should (equal-including-properties #("a" 0 1 (k "v"))
+                                      #("a" 0 1 (k "v"))))
+  (should-not (equal-including-properties #("a" 0 1 (k "v"))
+                                          #("a" 0 1 (k "x"))))
+  (should-not (equal-including-properties #("a" 0 1 (k "v"))
+                                          #("b" 0 1 (k "v")))))
+
 (ert-deftest fns-tests-reverse ()
   (should-error (reverse))
   (should-error (reverse 1))
-- 
2.30.2

bug#6581: 24.0.50; `equal-including-properties' uses `eq' to compare property values

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> The attached patch should fix it.

The patch looks OK tho I can't see why you add `intervals_equal_1` to
intervals.h (instead of marking it static).

> (compare_string_intervals): Call intervals_equal1 with third
> argument as true.

Rather than paraphrase the code change this could say what's the
intended effect of passing that arg.


        Stefan

bug#6581: 24.0.50; `equal-including-properties' uses `eq' to compare property values

[Stefan Kangas]

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

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> The attached patch should fix it.
>
> The patch looks OK tho I can't see why you add `intervals_equal_1` to
> intervals.h (instead of marking it static).

Me neither, fixed in the attached.  (It was an artifact of some earlier
experimentation that shouldn't have survived.)

>> (compare_string_intervals): Call intervals_equal1 with third
>> argument as true.
>
> Rather than paraphrase the code change this could say what's the
> intended effect of passing that arg.

I'm not a fan of the GNU ChangeLog format, as it seems to encourage bad
habits like not useful descriptions and massive patches (as opposed to
patch sets).  In any case, I've made an attempt to force a useful
description into this clunky format in the attached.

I also see that ert had an inefficient re-implementation of
'equal-including-properties' lying around as a workaround for this bug,
so I've fixed that up in a second patch.

I hope that Eli or Lars could say if they prefer the first patch (with
the bug fix) on emacs-28 or master.

The second patch is not urgent and should definitely go to master.

[-- Attachment #2: 0001-Fix-bug-with-string-values-in-equal-including-proper.patch --]
[-- Type: text/x-diff, Size: 6981 bytes --]

From 316ce9b0fbbf34c26cc4d7701c3780914adb505b Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Wed, 20 Oct 2021 14:16:07 +0200
Subject: [PATCH 1/2] Fix bug with string values in equal-including-properties

* src/intervals.c (intervals_equal_1): Factor out from
intervals_equal.  Optionally use Fequal for comparison of string
values in property lists.
(intervals_equal): Update for the above.
(compare_string_intervals): Use the above optional Fequal comparison
to fix a bug where 'equal-including-properties' compared strings with
eq, instead of equal.  (Bug#6581)
* test/src/fns-tests.el (fns-tests-equal-including-properties)
(fns-tests-equal-including-properties/string-prop-vals): New tests.

* test/lisp/emacs-lisp/ert-tests.el
(ert-test-equal-including-properties): Remove parts testing
'equal-including-properties'.
* lisp/emacs-lisp/ert.el (ert-equal-including-properties): Add
FIXME that this should be removed.
---
 lisp/emacs-lisp/ert.el            |  1 +
 src/intervals.c                   | 20 +++++++++++++++-----
 test/lisp/emacs-lisp/ert-tests.el | 14 --------------
 test/src/fns-tests.el             | 27 +++++++++++++++++++++++++++
 4 files changed, 43 insertions(+), 19 deletions(-)

diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
index 57655403c2..a5b5258c3e 100644
--- a/lisp/emacs-lisp/ert.el
+++ b/lisp/emacs-lisp/ert.el
@@ -92,6 +92,7 @@ ert-test-result-unexpected
 
 ;;; Copies/reimplementations of cl functions.
 
+;; FIXME: Bug#6581 is fixed, so this should be deleted.
 (defun ert-equal-including-properties (a b)
   "Return t if A and B have similar structure and contents.
 
diff --git a/src/intervals.c b/src/intervals.c
index f88a41f254..15bdc5b517 100644
--- a/src/intervals.c
+++ b/src/intervals.c
@@ -166,10 +166,11 @@ merge_properties (register INTERVAL source, register INTERVAL target)
     }
 }
 
-/* Return true if the two intervals have the same properties.  */
+/* Return true if the two intervals have the same properties.
+   If use_equal is true, use Fequal for comparisons instead of EQ.  */
 
-bool
-intervals_equal (INTERVAL i0, INTERVAL i1)
+static bool
+intervals_equal_1 (INTERVAL i0, INTERVAL i1, bool use_equal)
 {
   Lisp_Object i0_cdr, i0_sym;
   Lisp_Object i1_cdr, i1_val;
@@ -204,7 +205,8 @@ intervals_equal (INTERVAL i0, INTERVAL i1)
       /* i0 and i1 both have sym, but it has different values in each.  */
       if (!CONSP (i1_val)
 	  || (i1_val = XCDR (i1_val), !CONSP (i1_val))
-	  || !EQ (XCAR (i1_val), XCAR (i0_cdr)))
+	  || (!use_equal && !EQ (XCAR (i1_val), XCAR (i0_cdr)))
+	  || (use_equal && NILP (Fequal (XCAR (i1_val), XCAR (i0_cdr)))))
 	return false;
 
       i0_cdr = XCDR (i0_cdr);
@@ -218,6 +220,14 @@ intervals_equal (INTERVAL i0, INTERVAL i1)
   /* Lengths of the two plists were equal.  */
   return (NILP (i0_cdr) && NILP (i1_cdr));
 }
+
+/* Return true if the two intervals have the same properties.  */
+
+bool
+intervals_equal (INTERVAL i0, INTERVAL i1)
+{
+  return intervals_equal_1 (i0, i1, false);
+}
 \f
 
 /* Traverse an interval tree TREE, performing FUNCTION on each node.
@@ -2291,7 +2301,7 @@ compare_string_intervals (Lisp_Object s1, Lisp_Object s2)
 
       /* If we ever find a mismatch between the strings,
 	 they differ.  */
-      if (! intervals_equal (i1, i2))
+      if (! intervals_equal_1 (i1, i2, true))
 	return 0;
 
       /* Advance POS till the end of the shorter interval,
diff --git a/test/lisp/emacs-lisp/ert-tests.el b/test/lisp/emacs-lisp/ert-tests.el
index a18664bba3..39b7b47555 100644
--- a/test/lisp/emacs-lisp/ert-tests.el
+++ b/test/lisp/emacs-lisp/ert-tests.el
@@ -715,27 +715,13 @@ ert-test-explain-equal-string-properties
                  context-before "f" context-after "o"))))
 
 (ert-deftest ert-test-equal-including-properties ()
-  (should (equal-including-properties "foo" "foo"))
   (should (ert-equal-including-properties "foo" "foo"))
-
-  (should (equal-including-properties #("foo" 0 3 (a b))
-                                      (propertize "foo" 'a 'b)))
   (should (ert-equal-including-properties #("foo" 0 3 (a b))
                                           (propertize "foo" 'a 'b)))
-
-  (should (equal-including-properties #("foo" 0 3 (a b c d))
-                                      (propertize "foo" 'a 'b 'c 'd)))
   (should (ert-equal-including-properties #("foo" 0 3 (a b c d))
                                           (propertize "foo" 'a 'b 'c 'd)))
-
-  (should-not (equal-including-properties #("foo" 0 3 (a b c e))
-                                          (propertize "foo" 'a 'b 'c 'd)))
   (should-not (ert-equal-including-properties #("foo" 0 3 (a b c e))
                                               (propertize "foo" 'a 'b 'c 'd)))
-
-  ;; This is bug 6581.
-  (should-not (equal-including-properties #("foo" 0 3 (a (t)))
-                                          (propertize "foo" 'a (list t))))
   (should (ert-equal-including-properties #("foo" 0 3 (a (t)))
                                           (propertize "foo" 'a (list t)))))
 
diff --git a/test/src/fns-tests.el b/test/src/fns-tests.el
index 3dc2e7b3ec..bec5c03f9e 100644
--- a/test/src/fns-tests.el
+++ b/test/src/fns-tests.el
@@ -57,6 +57,33 @@ fns-tests-equality-nan
       (puthash nan t h)
       (should (eq (funcall test nan -nan) (gethash -nan h))))))
 
+(ert-deftest fns-tests-equal-including-properties ()
+  (should (equal-including-properties "" ""))
+  (should (equal-including-properties "foo" "foo"))
+  (should (equal-including-properties #("foo" 0 3 (a b))
+                                      (propertize "foo" 'a 'b)))
+  (should (equal-including-properties #("foo" 0 3 (a b c d))
+                                      (propertize "foo" 'a 'b 'c 'd)))
+  (should (equal-including-properties #("a" 0 1 (k v))
+                                      #("a" 0 1 (k v))))
+  (should-not (equal-including-properties #("a" 0 1 (k v))
+                                          #("a" 0 1 (k x))))
+  (should-not (equal-including-properties #("a" 0 1 (k v))
+                                          #("b" 0 1 (k v))))
+  (should-not (equal-including-properties #("foo" 0 3 (a b c e))
+                                          (propertize "foo" 'a 'b 'c 'd))))
+
+(ert-deftest fns-tests-equal-including-properties/string-prop-vals ()
+  "Handle string property values.  (Bug#6581)"
+  (should (equal-including-properties #("a" 0 1 (k "v"))
+                                      #("a" 0 1 (k "v"))))
+  (should (equal-including-properties #("foo" 0 3 (a (t)))
+                                      (propertize "foo" 'a (list t))))
+  (should-not (equal-including-properties #("a" 0 1 (k "v"))
+                                          #("a" 0 1 (k "x"))))
+  (should-not (equal-including-properties #("a" 0 1 (k "v"))
+                                          #("b" 0 1 (k "v")))))
+
 (ert-deftest fns-tests-reverse ()
   (should-error (reverse))
   (should-error (reverse 1))
-- 
2.30.2


[-- Attachment #3: 0002-Remove-workaround-for-fixed-Bug-6541-from-ert.patch --]
[-- Type: text/x-diff, Size: 16436 bytes --]

From 7af3861c4a18723abb04aada315e4515cd54ed18 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Thu, 21 Oct 2021 19:53:00 +0200
Subject: [PATCH 2/2] Remove workaround for fixed Bug#6541 from ert

* lisp/emacs-lisp/ert.el (ert-equal-including-properties): Make
into obsolete function alias for 'equal-including-properties'.
* test/src/editfns-tests.el (format-properties):
* test/lisp/emacs-lisp/ert-x-tests.el (ert-propertized-string)
(ert-test-run-tests-interactively-2): Don't use above obsolete
name.

(ert--explain-equal-including-properties-rec): New function.
(ert--explain-equal-including-properties): Use as an explainer for
'equal-including-properties' now that Bug#6581 is fixed.

* test/lisp/emacs-lisp/ert-tests.el
(ert-test-explain-equal-string-properties): Expand test.
(ert-test-equal-including-properties): Merge test into above
expanded test.
---
 lisp/emacs-lisp/ert.el              | 55 +++++++++++------------------
 test/lisp/emacs-lisp/ert-tests.el   | 55 ++++++++++++++++-------------
 test/lisp/emacs-lisp/ert-x-tests.el |  8 ++---
 test/src/editfns-tests.el           | 48 ++++++++++++-------------
 4 files changed, 78 insertions(+), 88 deletions(-)

diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
index a5b5258c3e..464680cd06 100644
--- a/lisp/emacs-lisp/ert.el
+++ b/lisp/emacs-lisp/ert.el
@@ -89,24 +89,6 @@ ert-test-result-unexpected
                                        :background "red3"))
   "Face used for unexpected results in the ERT results buffer.")
 
-
-;;; Copies/reimplementations of cl functions.
-
-;; FIXME: Bug#6581 is fixed, so this should be deleted.
-(defun ert-equal-including-properties (a b)
-  "Return t if A and B have similar structure and contents.
-
-This is like `equal-including-properties' except that it compares
-the property values of text properties structurally (by
-recursing) rather than with `eq'.  Perhaps this is what
-`equal-including-properties' should do in the first place; see
-Emacs bug 6581 at URL `https://debbugs.gnu.org/cgi/bugreport.cgi?bug=6581'."
-  ;; This implementation is inefficient.  Rather than making it
-  ;; efficient, let's hope bug 6581 gets fixed so that we can delete
-  ;; it altogether.
-  (not (ert--explain-equal-including-properties a b)))
-
-
 ;;; Defining and locating tests.
 
 ;; The data structure that represents a test case.
@@ -467,7 +449,7 @@ ert--explain-format-atom
 
 (defun ert--explain-equal-rec (a b)
   "Return a programmer-readable explanation of why A and B are not `equal'.
-Returns nil if they are."
+Return nil if they are."
   (if (not (eq (type-of a) (type-of b)))
       `(different-types ,a ,b)
     (pcase a
@@ -600,14 +582,9 @@ ert--abbreviate-string
           (t
            (substring s 0 len)))))
 
-;; TODO(ohler): Once bug 6581 is fixed, rename this to
-;; `ert--explain-equal-including-properties-rec' and add a fast-path
-;; wrapper like `ert--explain-equal'.
-(defun ert--explain-equal-including-properties (a b)
-  "Explainer function for `ert-equal-including-properties'.
-
-Returns a programmer-readable explanation of why A and B are not
-`ert-equal-including-properties', or nil if they are."
+(defun ert--explain-equal-including-properties-rec (a b)
+  "Return explanation of why A and B are not `equal-including-properties'.
+Return nil if they are."
   (if (not (equal a b))
       (ert--explain-equal a b)
     (cl-assert (stringp a) t)
@@ -629,15 +606,17 @@ ert--explain-equal-including-properties
                                     ,(ert--abbreviate-string
                                       (substring-no-properties a (1+ i))
                                       10 nil))))
-             ;; TODO(ohler): Get `equal-including-properties' fixed in
-             ;; Emacs, delete `ert-equal-including-properties', and
-             ;; re-enable this assertion.
-             ;;finally (cl-assert (equal-including-properties a b) t)
-             )))
-(put 'ert-equal-including-properties
-     'ert-explainer
-     'ert--explain-equal-including-properties)
+             finally (cl-assert (equal-including-properties a b) t))))
 
+(defun ert--explain-equal-including-properties (a b)
+  "Explainer function for `equal-including-properties'."
+  ;; Do a quick comparison in C to avoid running our expensive
+  ;; comparison when possible.
+  (if (equal-including-properties a b)
+      nil
+    (ert--explain-equal-including-properties-rec a b)))
+(put 'equal-including-properties 'ert-explainer
+     'ert--explain-equal-including-properties)
 
 ;;; Implementation of `ert-info'.
 
@@ -2786,6 +2765,12 @@ ert--erts-specifications
 (defvar ert-unload-hook ())
 (add-hook 'ert-unload-hook #'ert--unload-function)
 
+;;; Obsolete
+
+(define-obsolete-function-alias 'ert-equal-including-properties
+  #'equal-including-properties "29.1")
+(put 'ert-equal-including-properties 'ert-explainer
+     'ert--explain-equal-including-properties)
 
 (provide 'ert)
 
diff --git a/test/lisp/emacs-lisp/ert-tests.el b/test/lisp/emacs-lisp/ert-tests.el
index 39b7b47555..79576d2403 100644
--- a/test/lisp/emacs-lisp/ert-tests.el
+++ b/test/lisp/emacs-lisp/ert-tests.el
@@ -695,35 +695,40 @@ ert-test-abbreviate-string
   (should (equal (ert--abbreviate-string "bar" 0 t) "")))
 
 (ert-deftest ert-test-explain-equal-string-properties ()
-  (should
-   (equal (ert--explain-equal-including-properties #("foo" 0 1 (a b))
-                                                   "foo")
-          '(char 0 "f"
-                 (different-properties-for-key a (different-atoms b nil))
-                 context-before ""
-                 context-after "oo")))
-  (should (equal (ert--explain-equal-including-properties
+  (should-not (ert--explain-equal-including-properties-rec "foo" "foo"))
+  (should-not (ert--explain-equal-including-properties-rec
+               #("foo" 0 3 (a b))
+               (propertize "foo" 'a 'b)))
+  (should-not (ert--explain-equal-including-properties-rec
+               #("foo" 0 3 (a b c d))
+               (propertize "foo" 'a 'b 'c 'd)))
+  (should-not (ert--explain-equal-including-properties-rec
+               #("foo" 0 3 (a (t)))
+               (propertize "foo" 'a (list t))))
+
+  (should (equal (ert--explain-equal-including-properties-rec
+                  #("foo" 0 3 (a b c e))
+                  (propertize "foo" 'a 'b 'c 'd))
+                 '(char 0 "f" (different-properties-for-key c (different-atoms e d))
+                        context-before ""
+                        context-after "oo")))
+  (should (equal (ert--explain-equal-including-properties-rec
+                  #("foo" 0 1 (a b))
+                  "foo")
+                 '(char 0 "f"
+                        (different-properties-for-key a (different-atoms b nil))
+                        context-before ""
+                        context-after "oo")))
+  (should (equal (ert--explain-equal-including-properties-rec
                   #("foo" 1 3 (a b))
                   #("goo" 0 1 (c d)))
                  '(array-elt 0 (different-atoms (?f "#x66" "?f")
                                                 (?g "#x67" "?g")))))
-  (should
-   (equal (ert--explain-equal-including-properties
-           #("foo" 0 1 (a b c d) 1 3 (a b))
-           #("foo" 0 1 (c d a b) 1 2 (a foo)))
-          '(char 1 "o" (different-properties-for-key a (different-atoms b foo))
-                 context-before "f" context-after "o"))))
-
-(ert-deftest ert-test-equal-including-properties ()
-  (should (ert-equal-including-properties "foo" "foo"))
-  (should (ert-equal-including-properties #("foo" 0 3 (a b))
-                                          (propertize "foo" 'a 'b)))
-  (should (ert-equal-including-properties #("foo" 0 3 (a b c d))
-                                          (propertize "foo" 'a 'b 'c 'd)))
-  (should-not (ert-equal-including-properties #("foo" 0 3 (a b c e))
-                                              (propertize "foo" 'a 'b 'c 'd)))
-  (should (ert-equal-including-properties #("foo" 0 3 (a (t)))
-                                          (propertize "foo" 'a (list t)))))
+  (should (equal (ert--explain-equal-including-properties-rec
+                  #("foo" 0 1 (a b c d) 1 3 (a b))
+                  #("foo" 0 1 (c d a b) 1 2 (a foo)))
+                 '(char 1 "o" (different-properties-for-key a (different-atoms b foo))
+                        context-before "f" context-after "o"))))
 
 (ert-deftest ert-test-stats-set-test-and-result ()
   (let* ((test-1 (make-ert-test :name 'test-1
diff --git a/test/lisp/emacs-lisp/ert-x-tests.el b/test/lisp/emacs-lisp/ert-x-tests.el
index 9f40a18d34..1784934acb 100644
--- a/test/lisp/emacs-lisp/ert-x-tests.el
+++ b/test/lisp/emacs-lisp/ert-x-tests.el
@@ -90,10 +90,10 @@ ert-filter-string
                  "foo  baz")))
 
 (ert-deftest ert-propertized-string ()
-  (should (ert-equal-including-properties
+  (should (equal-including-properties
            (ert-propertized-string "a" '(a b) "b" '(c t) "cd")
            #("abcd" 1 2 (a b) 2 4 (c t))))
-  (should (ert-equal-including-properties
+  (should (equal-including-properties
            (ert-propertized-string "foo " '(face italic) "bar" " baz" nil
                                    " quux")
            #("foo bar baz quux" 4 11 (face italic)))))
@@ -166,7 +166,7 @@ ert-test-run-tests-interactively-2
 					  "1 skipped"))))
               (with-current-buffer buffer-name
                 (font-lock-mode 0)
-                (should (ert-equal-including-properties
+                (should (equal-including-properties
                          (ert-filter-string (buffer-string)
                                             '("Started at:\\(.*\\)$" 1)
                                             '("Finished at:\\(.*\\)$" 1))
@@ -175,7 +175,7 @@ ert-test-run-tests-interactively-2
                 ;; pretend we are.
                 (let ((noninteractive nil))
                   (font-lock-mode 1))
-                (should (ert-equal-including-properties
+                (should (equal-including-properties
                          (ert-filter-string (buffer-string)
                                             '("Started at:\\(.*\\)$" 1)
                                             '("Finished at:\\(.*\\)$" 1))
diff --git a/test/src/editfns-tests.el b/test/src/editfns-tests.el
index a731a95ccf..e83dd7c857 100644
--- a/test/src/editfns-tests.el
+++ b/test/src/editfns-tests.el
@@ -23,16 +23,16 @@
 
 (ert-deftest format-properties ()
   ;; Bug #23730
-  (should (ert-equal-including-properties
+  (should (equal-including-properties
            (format (propertize "%d" 'face '(:background "red")) 1)
            #("1" 0 1 (face (:background "red")))))
-  (should (ert-equal-including-properties
+  (should (equal-including-properties
            (format (propertize "%2d" 'face '(:background "red")) 1)
            #(" 1" 0 2 (face (:background "red")))))
-  (should (ert-equal-including-properties
+  (should (equal-including-properties
            (format (propertize "%02d" 'face '(:background "red")) 1)
            #("01" 0 2 (face (:background "red")))))
-  (should (ert-equal-including-properties
+  (should (equal-including-properties
            (format (concat (propertize "%2d" 'x 'X)
                            (propertize "a" 'a 'A)
                            (propertize "b" 'b 'B))
@@ -40,27 +40,27 @@ format-properties
            #(" 1ab" 0 2 (x X) 2 3 (a A) 3 4 (b B))))
 
   ;; Bug #5306
-  (should (ert-equal-including-properties
+  (should (equal-including-properties
            (format "%.10s"
                    (concat "1234567890aaaa"
                            (propertize "12345678901234567890" 'xxx 25)))
            "1234567890"))
-  (should (ert-equal-including-properties
+  (should (equal-including-properties
            (format "%.10s"
                    (concat "123456789"
                            (propertize "12345678901234567890" 'xxx 25)))
            #("1234567891" 9 10 (xxx 25))))
 
   ;; Bug #23859
-  (should (ert-equal-including-properties
+  (should (equal-including-properties
            (format "%4s" (propertize "hi" 'face 'bold))
            #("  hi" 2 4 (face bold))))
 
   ;; Bug #23897
-  (should (ert-equal-including-properties
+  (should (equal-including-properties
            (format "%s" (concat (propertize "01234" 'face 'bold) "56789"))
            #("0123456789" 0 5 (face bold))))
-  (should (ert-equal-including-properties
+  (should (equal-including-properties
            (format "%s" (concat (propertize "01" 'face 'bold)
                                 (propertize "23" 'face 'underline)
                                 "45"))
@@ -68,63 +68,63 @@ format-properties
   ;; The last property range is extended to include padding on the
   ;; right, but the first range is not extended to the left to include
   ;; padding on the left!
-  (should (ert-equal-including-properties
+  (should (equal-including-properties
            (format "%12s" (concat (propertize "01234" 'face 'bold) "56789"))
            #("  0123456789" 2 7 (face bold))))
-  (should (ert-equal-including-properties
+  (should (equal-including-properties
            (format "%-12s" (concat (propertize "01234" 'face 'bold) "56789"))
            #("0123456789  " 0 5 (face bold))))
-  (should (ert-equal-including-properties
+  (should (equal-including-properties
            (format "%10s" (concat (propertize "01" 'face 'bold)
                                   (propertize "23" 'face 'underline)
                                   "45"))
            #("    012345" 4 6 (face bold) 6 8 (face underline))))
-  (should (ert-equal-including-properties
+  (should (equal-including-properties
            (format "%-10s" (concat (propertize "01" 'face 'bold)
                                    (propertize "23" 'face 'underline)
                                    "45"))
            #("012345    " 0 2 (face bold) 2 4 (face underline))))
-  (should (ert-equal-including-properties
+  (should (equal-including-properties
            (format "%-10s" (concat (propertize "01" 'face 'bold)
                                    (propertize "23" 'face 'underline)
                                    (propertize "45" 'face 'italic)))
            #("012345    "
              0 2 (face bold) 2 4 (face underline) 4 10 (face italic))))
   ;; Bug #38191
-  (should (ert-equal-including-properties
+  (should (equal-including-properties
            (format (propertize "‘foo’ %s bar" 'face 'bold) "xxx")
            #("‘foo’ xxx bar" 0 13 (face bold))))
   ;; Bug #32404
-  (should (ert-equal-including-properties
+  (should (equal-including-properties
            (format (concat (propertize "%s" 'face 'bold)
                            ""
                            (propertize "%s" 'face 'error))
                    "foo" "bar")
            #("foobar" 0 3 (face bold) 3 6 (face error))))
-  (should (ert-equal-including-properties
+  (should (equal-including-properties
            (format (concat "%s" (propertize "%s" 'face 'error)) "foo" "bar")
            #("foobar" 3 6 (face error))))
-  (should (ert-equal-including-properties
+  (should (equal-including-properties
            (format (concat "%s " (propertize "%s" 'face 'error)) "foo" "bar")
            #("foo bar" 4 7 (face error))))
   ;; Bug #46317
   (let ((s (propertize "X" 'prop "val")))
-    (should (ert-equal-including-properties
+    (should (equal-including-properties
              (format (concat "%3s/" s) 12)
              #(" 12/X" 4 5 (prop "val"))))
-    (should (ert-equal-including-properties
+    (should (equal-including-properties
              (format (concat "%3S/" s) 12)
              #(" 12/X" 4 5 (prop "val"))))
-    (should (ert-equal-including-properties
+    (should (equal-including-properties
              (format (concat "%3d/" s) 12)
              #(" 12/X" 4 5 (prop "val"))))
-    (should (ert-equal-including-properties
+    (should (equal-including-properties
              (format (concat "%-3s/" s) 12)
              #("12 /X" 4 5 (prop "val"))))
-    (should (ert-equal-including-properties
+    (should (equal-including-properties
              (format (concat "%-3S/" s) 12)
              #("12 /X" 4 5 (prop "val"))))
-    (should (ert-equal-including-properties
+    (should (equal-including-properties
              (format (concat "%-3d/" s) 12)
              #("12 /X" 4 5 (prop "val"))))))
 
-- 
2.30.2

bug#6581: 24.0.50; `equal-including-properties' uses `eq' to compare property values

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Thu, 21 Oct 2021 11:05:57 -0700
> Cc: 6581@debbugs.gnu.org, Christian Ohler <ohler+emacs@fastmail.net>,
>  Dmitry Gutov <dgutov@yandex.ru>
> 
> I hope that Eli or Lars could say if they prefer the first patch (with
> the bug fix) on emacs-28 or master.

On master, please.  It's not really a bugfix, it's a kind-of
enhancement, and the change is not trivial.

Thanks.

bug#6581: 24.0.50; `equal-including-properties' uses `eq' to compare property values

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> On master, please.  It's not really a bugfix, it's a kind-of
> enhancement, and the change is not trivial.

That's fine by me, thank you.

bug#6581: 24.0.50; `equal-including-properties' uses `eq' to compare property values

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> +	  || (!use_equal && !EQ (XCAR (i1_val), XCAR (i0_cdr)))
> +	  || (use_equal && NILP (Fequal (XCAR (i1_val), XCAR (i0_cdr)))))

use_equal ? NILP (Fequal (XCAR (i1_val), XCAR (i0_cdr)))
          : !EQ (XCAR (i1_val), XCAR (i0_cdr))


-- Stefan

bug#6581: 24.0.50; `equal-including-properties' uses `eq' to compare property values

[Stefan Kangas]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> +	  || (!use_equal && !EQ (XCAR (i1_val), XCAR (i0_cdr)))
>> +	  || (use_equal && NILP (Fequal (XCAR (i1_val), XCAR (i0_cdr)))))
>
> use_equal ? NILP (Fequal (XCAR (i1_val), XCAR (i0_cdr)))
>           : !EQ (XCAR (i1_val), XCAR (i0_cdr))

Yes, that's better.

bug#6581: 24.0.50; `equal-including-properties' uses `eq' to compare property values

[Stefan Kangas]

close 6581 29.1
thanks

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Kangas <stefan@marxist.se>
>> Date: Thu, 21 Oct 2021 11:05:57 -0700
>> Cc: 6581@debbugs.gnu.org, Christian Ohler <ohler+emacs@fastmail.net>,
>>  Dmitry Gutov <dgutov@yandex.ru>
>>
>> I hope that Eli or Lars could say if they prefer the first patch (with
>> the bug fix) on emacs-28 or master.
>
> On master, please.  It's not really a bugfix, it's a kind-of
> enhancement, and the change is not trivial.

No further comments within almost 10 days, now pushed to master (commit
8227d1273e and 54b8ec4e6f).