Subject: bug#34852: 26.1; seq-intersection ignores nil as element

From: "Basil L. Contovounesios" <[email-masked]>
Date: Thu, 14 Mar 2019 12:22:40 +0000

>From afddf78a703b73729ec7b5562747ebeb5077966d Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <[email-masked]>
Date: Thu, 14 Mar 2019 12:10:28 +0000
Subject: [PATCH] Do not use seq-contains as a predicate (bug#34852)

* doc/lispref/sequences.texi (Sequence Functions):
* lisp/emacs-lisp/seq.el (seq-contains): Document unsuitability of
seq-contains as a predicate.
(seq-position): Document return value when needle not found.
(seq-set-equal-p, seq-uniq, seq-intersection, seq-difference): Use
seq-position instead of seq-contains as a predicate.
* test/lisp/emacs-lisp/seq-tests.el
(test-seq-contains-should-return-the-elt):
(test-seq-position): Test case when needle is nil.
---
 doc/lispref/sequences.texi        |  7 +++++++
 lisp/emacs-lisp/seq.el            | 21 ++++++++++++---------
 test/lisp/emacs-lisp/seq-tests.el | 14 +++++++++-----
 3 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/doc/lispref/sequences.texi b/doc/lispref/sequences.texi
index 0c3c4e3b28..e33944a2dd 100644
--- a/doc/lispref/sequences.texi
+++ b/doc/lispref/sequences.texi
@@ -787,6 +787,9 @@ Sequence Functions
 @var{elt}.  If the optional argument @var{function} is non-@code{nil},
 it is a function of two arguments to use instead of the default @code{equal}.
 
+Note that this function is not suitable as a predicate since its value
+depends on that of @var{elt}, which may be @code{nil}.
+
 @example
 @group
 (seq-contains '(symbol1 symbol2) 'symbol1)
@@ -796,6 +799,10 @@ Sequence Functions
 (seq-contains '(symbol1 symbol2) 'symbol3)
 @result{} nil
 @end group
+@group
+(seq-contains '(symbol1 nil) nil)
+@result{} nil
+@end group
 @end example
 
 @end defun
diff --git a/lisp/emacs-lisp/seq.el b/lisp/emacs-lisp/seq.el
index 4a811d7895..de8456eb79 100644
--- a/lisp/emacs-lisp/seq.el
+++ b/lisp/emacs-lisp/seq.el
@@ -357,7 +357,9 @@ seq-sort-by
 
 (cl-defgeneric seq-contains (sequence elt &optional testfn)
   "Return the first element in SEQUENCE that is equal to ELT.
-Equality is defined by TESTFN if non-nil or by `equal' if nil."
+Equality is defined by TESTFN if non-nil or by `equal' if nil.
+Note that this function is not suitable as a predicate since its
+value depends on that of ELT, which may be nil."
   (seq-some (lambda (e)
               (when (funcall (or testfn #'equal) elt e)
                 e))
@@ -366,12 +368,13 @@ seq-sort-by
 (cl-defgeneric seq-set-equal-p (sequence1 sequence2 &optional testfn)
   "Return non-nil if SEQUENCE1 and SEQUENCE2 contain the same elements, regardless of order.
 Equality is defined by TESTFN if non-nil or by `equal' if nil."
-  (and (seq-every-p (lambda (item1) (seq-contains sequence2 item1 testfn)) sequence1)
-       (seq-every-p (lambda (item2) (seq-contains sequence1 item2 testfn)) sequence2)))
+  (and (seq-every-p (lambda (item1) (seq-position sequence2 item1 testfn)) sequence1)
+       (seq-every-p (lambda (item2) (seq-position sequence1 item2 testfn)) sequence2)))
 
 (cl-defgeneric seq-position (sequence elt &optional testfn)
   "Return the index of the first element in SEQUENCE that is equal to ELT.
-Equality is defined by TESTFN if non-nil or by `equal' if nil."
+Return nil if no such element is found.  Equality is defined by
+TESTFN if non-nil or by `equal' if nil."
   (let ((index 0))
     (catch 'seq--break
       (seq-doseq (e sequence)
@@ -385,7 +388,7 @@ seq-sort-by
 TESTFN is used to compare elements, or `equal' if TESTFN is nil."
   (let ((result '()))
     (seq-doseq (elt sequence)
-      (unless (seq-contains result elt testfn)
+      (unless (seq-position result elt testfn)
         (setq result (cons elt result))))
     (nreverse result)))
 
@@ -410,7 +413,7 @@ seq-sort-by
   "Return a list of the elements that appear in both SEQUENCE1 and SEQUENCE2.
 Equality is defined by TESTFN if non-nil or by `equal' if nil."
   (seq-reduce (lambda (acc elt)
-                (if (seq-contains sequence2 elt testfn)
+                (if (seq-position sequence2 elt testfn)
                     (cons elt acc)
                   acc))
               (seq-reverse sequence1)
@@ -420,9 +423,9 @@ seq-sort-by
   "Return a list of the elements that appear in SEQUENCE1 but not in SEQUENCE2.
 Equality is defined by TESTFN if non-nil or by `equal' if nil."
   (seq-reduce (lambda (acc elt)
-                (if (not (seq-contains sequence2 elt testfn))
-                    (cons elt acc)
-                  acc))
+                (if (seq-position sequence2 elt testfn)
+                    acc
+                  (cons elt acc)))
               (seq-reverse sequence1)
               '()))
 
diff --git a/test/lisp/emacs-lisp/seq-tests.el b/test/lisp/emacs-lisp/seq-tests.el
index d8f00cfea4..417606d9d7 100644
--- a/test/lisp/emacs-lisp/seq-tests.el
+++ b/test/lisp/emacs-lisp/seq-tests.el
@@ -183,7 +183,9 @@ test-sequences-oddp
 
 (ert-deftest test-seq-contains-should-return-the-elt ()
   (with-test-sequences (seq '(3 4 5 6))
-    (should (= 5 (seq-contains seq 5)))))
+    (should (= 5 (seq-contains seq 5))))
+  (should-not (seq-contains '(1 2 nil) nil))
+  (should-not (seq-contains [1 2 nil] nil)))
 
 (ert-deftest test-seq-every-p ()
   (with-test-sequences (seq '(43 54 22 1))
@@ -384,12 +386,14 @@ test-sequences-oddp
 
 (ert-deftest test-seq-position ()
   (with-test-sequences (seq '(2 4 6))
-    (should (null (seq-position seq 1)))
+    (should-not (seq-position seq 1))
+    (should-not (seq-position seq nil))
     (should (= (seq-position seq 4) 1)))
-  (let ((seq '(a b c)))
-    (should (null (seq-position seq 'd #'eq)))
+  (let ((seq '(a b c nil)))
+    (should-not (seq-position seq 'd #'eq))
     (should (= (seq-position seq 'a #'eq) 0))
-    (should (null (seq-position seq (make-symbol "a") #'eq)))))
+    (should (= (seq-position seq nil #'eq) 3))
+    (should-not (seq-position seq (make-symbol "a") #'eq))))
 
 (ert-deftest test-seq-sort-by ()
   (let ((seq ["x" "xx" "xxx"]))
-- 
2.20.1

>From 7b8d5672db306e3bb52196b04c74eb7098ec32a3 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <[email-masked]>
Date: Thu, 14 Mar 2019 11:19:55 +0000
Subject: [PATCH] Return boolean instead of element in seq-contains

* lisp/emacs-lisp/seq.el: Bump major version.
(seq-contains): Return non-nil instead of needle if it is found in
the sequence. (bug#34852)
* test/lisp/emacs-lisp/seq-tests.el (test-seq-contains): Add check
for nil as an element.
(test-seq-contains-should-return-the-elt): Remove, replacing with...
(test-seq-contains-return-non-nil): ...this new test.
* doc/lispref/sequences.texi (Sequence Functions):
* etc/NEWS: Document change to seq-contains.
---
 doc/lispref/sequences.texi        | 9 +++++----
 etc/NEWS                          | 3 +++
 lisp/emacs-lisp/seq.el            | 7 +++----
 test/lisp/emacs-lisp/seq-tests.el | 9 +++++----
 4 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/doc/lispref/sequences.texi b/doc/lispref/sequences.texi
index 0c3c4e3b28..b0c65fe526 100644
--- a/doc/lispref/sequences.texi
+++ b/doc/lispref/sequences.texi
@@ -783,14 +783,15 @@ Sequence Functions

 @defun seq-contains sequence elt &optional function
-  This function returns the first element in @var{sequence} that is equal to
-@var{elt}.  If the optional argument @var{function} is non-@code{nil},
-it is a function of two arguments to use instead of the default @code{equal}.
+  This function returns non-@code{nil} if an element in @var{sequence}
+is equal to @var{elt}.  If the optional argument @var{function} is
+non-@code{nil}, it is a function of two arguments to use instead of
+the default @code{equal}.
 
 @example
 @group
 (seq-contains '(symbol1 symbol2) 'symbol1)
-@result{} symbol1
+@result{} t
 @end group
 @group
 (seq-contains '(symbol1 symbol2) 'symbol3)
diff --git a/etc/NEWS b/etc/NEWS
index 410c1821ae..b41ff89613 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -383,6 +383,9 @@ buffers.
 New convenience functions 'seq-first' and 'seq-rest' give easy access
 to respectively the first and all but the first elements of sequences.
 
++++
+*** 'seq-contains' now returns a boolean rather than the element found.
+
 ---
 ** Follow mode
 In the current follow group of windows, "ghost" cursors are no longer
diff --git a/lisp/emacs-lisp/seq.el b/lisp/emacs-lisp/seq.el
index 4a811d7895..73cb985b85 100644
--- a/lisp/emacs-lisp/seq.el
+++ b/lisp/emacs-lisp/seq.el
@@ -4,7 +4,7 @@
 
 ;; Author: Nicolas Petton <[email-masked]>
 ;; Keywords: sequences
-;; Version: 2.21
+;; Version: 3.0
 ;; Package: seq
 
 ;; Maintainer: [email-masked]
@@ -356,11 +356,10 @@ seq-sort-by
     count))
 
 (cl-defgeneric seq-contains (sequence elt &optional testfn)
-  "Return the first element in SEQUENCE that is equal to ELT.
+  "Return non-nil if an element in SEQUENCE is equal to ELT.
 Equality is defined by TESTFN if non-nil or by `equal' if nil."
   (seq-some (lambda (e)
-              (when (funcall (or testfn #'equal) elt e)
-                e))
+              (funcall (or testfn #'equal) elt e))
             sequence))
 
 (cl-defgeneric seq-set-equal-p (sequence1 sequence2 &optional testfn)
diff --git a/test/lisp/emacs-lisp/seq-tests.el b/test/lisp/emacs-lisp/seq-tests.el
index d8f00cfea4..fee31e7f8f 100644
--- a/test/lisp/emacs-lisp/seq-tests.el
+++ b/test/lisp/emacs-lisp/seq-tests.el
@@ -176,14 +176,15 @@ test-sequences-oddp
 (ert-deftest test-seq-contains ()
   (with-test-sequences (seq '(3 4 5 6))
     (should (seq-contains seq 3))
-    (should-not (seq-contains seq 7)))
+    (should-not (seq-contains seq 7))
+    (should-not (seq-contains seq nil)))
   (with-test-sequences (seq '())
     (should-not (seq-contains seq 3))
     (should-not (seq-contains seq nil))))
 
-(ert-deftest test-seq-contains-should-return-the-elt ()
-  (with-test-sequences (seq '(3 4 5 6))
-    (should (= 5 (seq-contains seq 5)))))
+(ert-deftest test-seq-contains-return-non-nil ()
+  (should (seq-contains '(1 2 nil) nil))
+  (should (seq-contains [1 2 nil] nil)))
 
 (ert-deftest test-seq-every-p ()
   (with-test-sequences (seq '(43 54 22 1))
-- 
2.20.1

"Miguel V. S. Frasson" <[email-masked]> writes:

> seq-intersection skips nil as common element, so returns wrong result.
>
> Reproducing from emacs -Q:
>
> In *scratch* buffer, eval the expressions
>
> (progn
>   (require 'seq)
>   (seq-intersection '(1 2 nil) '(1 nil) 'eq))
>
> -> (1)
>
> (seq-intersection '(1 2 nil) '(1 nil) 'equal)
>
> -> (1)
>
> Expected result in both cases: (1 nil)

This is actually due to seq-contains returning the element found, rather
than a boolean indicating whether the element was found:

(seq-contains '(nil) nil) ; => nil

The nature of seq-contains as a predicate-ish has been discussed in the
past[1], and Stefan recently fixed a similar problem with
map-contains-key[2].

[1]: https://lists.gnu.org/archive/html/emacs-devel/2016-07/msg01256.html
[2]: * lisp/emacs-lisp/map.el: Make the functions generic
  2018-12-11 17:54:13 -0500
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=1691a51094d35ac4b2c311fa407c6b77eea7a105

One solution is to leave seq-contains as it is, and switch to using
seq-position (or some new predicate) as a predicate instead.  Another is
to make seq-contains return a boolean instead of the needle found, which
would be a backward-incompatible change similar to that for
map-contains-key.  I attach a patch for each of these respective
solutions; WDYT?

-- 
Basil