bug#59328: 29.0.50; `seq-keep' implementation only valid for lists

bug#59328: 29.0.50; `seq-keep' implementation only valid for lists

[Michael Heerdegen]

Hello,

  [FWIW I tried to reopen 58278 but it seemed to...complicated for me]

The current implementation of the (non-generic!) function `seq-keep':

#+begin_src emacs-lisp
(defun seq-keep (function sequence)
  (delq nil (seq-map function sequence)))
;; ^^^^
#+end_src

obviously only works when `seq-map' returns a list.  This is the case
for the default implementation of the generic function `seq-map' but not
necessarily for other implementations of `seq-map'.

We need to filter out the `nil' elements with a way appropriate for any
sequence type supported by "seq.el" (i.e. with a generic function
defined in this lib), e.g.

#+begin_src emacs-lisp
(defun seq-keep (function sequence)
  (seq-filter #'identity (seq-map function sequence)))
#+end_src


BTW, is the name a good one?  Why "keep"?  It returns a sequence of
potentially all completely different elements.  And is the function that
useful and a good abstraction at all (I don't have thought about it
too long...)?


TIA,

Michael.

bug#59328: 29.0.50; `seq-keep' implementation only valid for lists

[Eli Zaretskii]

> Cc: Lars Ingebrigtsen <larsi@gnus.org>, Jonas Bernoulli <jonas@bernoul.li>
> From: Michael Heerdegen <michael_heerdegen@web.de>
> Date: Thu, 17 Nov 2022 03:17:46 +0100
> 
> The current implementation of the (non-generic!) function `seq-keep':
> 
> #+begin_src emacs-lisp
> (defun seq-keep (function sequence)
>   (delq nil (seq-map function sequence)))
> ;; ^^^^
> #+end_src
> 
> obviously only works when `seq-map' returns a list.  This is the case
> for the default implementation of the generic function `seq-map' but not
> necessarily for other implementations of `seq-map'.
> 
> We need to filter out the `nil' elements with a way appropriate for any
> sequence type supported by "seq.el" (i.e. with a generic function
> defined in this lib), e.g.
> 
> #+begin_src emacs-lisp
> (defun seq-keep (function sequence)
>   (seq-filter #'identity (seq-map function sequence)))
> #+end_src

This makes sense to me, so please go ahead and install, preferably
with a test for non-list cases.

> BTW, is the name a good one?  Why "keep"?  It returns a sequence of
> potentially all completely different elements.  And is the function that
> useful and a good abstraction at all (I don't have thought about it
> too long...)?

FWIW, "keep" doesn't sound problematic to me.

Thanks.

bug#59328: 29.0.50; `seq-keep' implementation only valid for lists

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> > #+begin_src emacs-lisp
> > (defun seq-keep (function sequence)
> >   (seq-filter #'identity (seq-map function sequence)))
> > #+end_src
>
> This makes sense to me, so please go ahead and install, preferably
> with a test for non-list cases.

I don't think adding a test case to Emacs is possible - all currently
existing counterexamples I found (stream.el, myers.el) are in Elpa or
somewhere else, but not in the Emacs repo.  So I guess it's ok to go
without a test?

Michael.

bug#59328: 29.0.50; `seq-keep' implementation only valid for lists

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: 59328@debbugs.gnu.org,  larsi@gnus.org,  jonas@bernoul.li
> Date: Thu, 24 Nov 2022 14:04:27 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > > #+begin_src emacs-lisp
> > > (defun seq-keep (function sequence)
> > >   (seq-filter #'identity (seq-map function sequence)))
> > > #+end_src
> >
> > This makes sense to me, so please go ahead and install, preferably
> > with a test for non-list cases.
> 
> I don't think adding a test case to Emacs is possible - all currently
> existing counterexamples I found (stream.el, myers.el) are in Elpa or
> somewhere else, but not in the Emacs repo.

I don't understand: why cannot we add a test to Emacs, even though other
tests are elsewhere?

> So I guess it's ok to go without a test?

It's okay, but I'd prefer to have a test.

bug#59328: 29.0.50; `seq-keep' implementation only valid for lists

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> I don't understand: why cannot we add a test to Emacs, even though
> other tests are elsewhere?

Every kind of sequence (in the sense of seq.el) that exists in Emacs is
not affected by the bug.  A test makes only sense for types that are,
but those are defined in libraries that are not in Gnu Emacs, only in
Gnu Elpa.  But I can't make the test depend on them.

Michael.

bug#59328: 29.0.50; `seq-keep' implementation only valid for lists

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: 59328@debbugs.gnu.org,  larsi@gnus.org,  jonas@bernoul.li
> Date: Thu, 24 Nov 2022 15:26:01 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I don't understand: why cannot we add a test to Emacs, even though
> > other tests are elsewhere?
> 
> Every kind of sequence (in the sense of seq.el) that exists in Emacs is
> not affected by the bug.  A test makes only sense for types that are,
> but those are defined in libraries that are not in Gnu Emacs, only in
> Gnu Elpa.  But I can't make the test depend on them.

I see I was confused by the Subject of your bug report.

OK, then just install this, though now I wonder why we need this change...

bug#59328: 29.0.50; `seq-keep' implementation only valid for lists

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> > Every kind of sequence (in the sense of seq.el) that exists in Emacs is
> > not affected by the bug.  A test makes only sense for types that are,
> > but those are defined in libraries that are not in Gnu Emacs, only in
> > Gnu Elpa.  But I can't make the test depend on them.
>
> I see I was confused by the Subject of your bug report.
>
> OK, then just install this, though now I wonder why we need this change...

Without that change `seq-keep' would error for sequence types like
streams.  Try for example

#+begin_src emacs-lisp
(require 'stream)
(seq-keep
 (lambda (x) (and (<= 0 x) x))
 (stream (list -1 2 -3 4)))
#+end_src

That errors without the proposed change but it's a legitimate call (and
it should return a stream of course).

Michael.

bug#59328: 29.0.50; `seq-keep' implementation only valid for lists

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: 59328@debbugs.gnu.org,  larsi@gnus.org,  jonas@bernoul.li
> Date: Thu, 24 Nov 2022 16:09:32 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > OK, then just install this, though now I wonder why we need this change...
> 
> Without that change `seq-keep' would error for sequence types like
> streams.  Try for example
> 
> #+begin_src emacs-lisp
> (require 'stream)
> (seq-keep
>  (lambda (x) (and (<= 0 x) x))
>  (stream (list -1 2 -3 4)))
> #+end_src

Didn't you just say that 'stream' is not in Emacs?  If I try the above, the
debugger kicks in right on the 'require' line.

bug#59328: 29.0.50; `seq-keep' implementation only valid for lists

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> > Without that change `seq-keep' would error for sequence types like
> > streams.  Try for example
> >
> > #+begin_src emacs-lisp
> > (require 'stream)
> > (seq-keep
> >  (lambda (x) (and (<= 0 x) x))
> >  (stream (list -1 2 -3 4)))
> > #+end_src
>
> Didn't you just say that 'stream' is not in Emacs?  If I try the above, the
> debugger kicks in right on the 'require' line.

Yes.

But seq-keep is not a generic function, so it would be broken for
sequence types defined elsewhere, and there is no way for those other
sequence types to fix this.  Defining a generic interface for sequences
would not make much sense if it then only supports lists (and maybe
vectors).

So we want to support cases like streams.  Since seq-keep can be
expressed and is semantically equivalent a simple concetanation of
existing sequence operations there is probably no need to define it as
generic function.  We just need to implement it correctly to support any
otherwise supported type of sequences.

Michael.

bug#59328: 29.0.50; `seq-keep' implementation only valid for lists

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: 59328@debbugs.gnu.org,  larsi@gnus.org,  jonas@bernoul.li
> Date: Thu, 24 Nov 2022 16:25:44 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > > Without that change `seq-keep' would error for sequence types like
> > > streams.  Try for example
> > >
> > > #+begin_src emacs-lisp
> > > (require 'stream)
> > > (seq-keep
> > >  (lambda (x) (and (<= 0 x) x))
> > >  (stream (list -1 2 -3 4)))
> > > #+end_src
> >
> > Didn't you just say that 'stream' is not in Emacs?  If I try the above, the
> > debugger kicks in right on the 'require' line.
> 
> Yes.
> 
> But seq-keep is not a generic function, so it would be broken for
> sequence types defined elsewhere, and there is no way for those other
> sequence types to fix this.  Defining a generic interface for sequences
> would not make much sense if it then only supports lists (and maybe
> vectors).
> 
> So we want to support cases like streams.

Can tests for this be written in a way that they are only run if the
relevant packages are available on the user's system?  If so, I'd prefer to
have that than no tests at all.

Your call.

Thanks.

bug#59328: 29.0.50; `seq-keep' implementation only valid for lists

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> Can tests for this be written in a way that they are only run if the
> relevant packages are available on the user's system?  If so, I'd
> prefer to have that than no tests at all.

I don't know.

Alternatively we could implement `seq-map' for an ad-hoc defined
sequence type and test using that type, e.g. this expression:

#+begin_src emacs-lisp
(progn
  (defvar gensym)
  (let ((gensym (make-symbol "foo")))
    (eval `(cl-defmethod seq-map (function (thing (head ,gensym)))
             (append (list (car thing) (cadr thing)) (seq-map function (cddr thing))))
          t)
    (equal (list gensym nil 4 46)
           (seq-keep (lambda (x) (and (integerp x) (* 2 x)))
                     (list gensym nil 2 'x gensym 23)))))
#+end_src

returns t with my patch installed and nil else and works without relying
on something external.  I'm not sure if defining methods (for seq-map in
this case) that are globally visible is allowed in tests, so I
implemented the example above in a way that the change of the generic
function is not visible from the outside (thus the "secret" gensym).

Would something like that be acceptable?

Sorry for my ignorance, I didn't write much tests before.


TIA,

Michael.

bug#59328: 29.0.50; `seq-keep' implementation only valid for lists

[Stefan Kangas]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> #+begin_src emacs-lisp
> (progn
>   (defvar gensym)
>   (let ((gensym (make-symbol "foo")))
>     (eval `(cl-defmethod seq-map (function (thing (head ,gensym)))
>              (append (list (car thing) (cadr thing)) (seq-map function (cddr thing))))
>           t)
>     (equal (list gensym nil 4 46)
>            (seq-keep (lambda (x) (and (integerp x) (* 2 x)))
>                      (list gensym nil 2 'x gensym 23)))))
> #+end_src
>
> returns t with my patch installed and nil else and works without relying
> on something external. I'm not sure if defining methods (for seq-map in
> this case) that are globally visible is allowed in tests, so I
> implemented the example above in a way that the change of the generic
> function is not visible from the outside (thus the "secret" gensym).

We allow things that are globally visible, yes, as that will only
affect testing sessions.  So I think dropping the gensym will be an
improvement.

> Would something like that be acceptable?

Makes sense to me.  Perhaps we should do something similar for other
seq.el tests.

bug#59328: 29.0.50; `seq-keep' implementation only valid for lists

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: 59328@debbugs.gnu.org,  larsi@gnus.org,  jonas@bernoul.li
> Date: Fri, 25 Nov 2022 10:47:31 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Can tests for this be written in a way that they are only run if the
> > relevant packages are available on the user's system?  If so, I'd
> > prefer to have that than no tests at all.
> 
> I don't know.

AFAIK, 'require' can return nil if asked not to error out.

> Alternatively we could implement `seq-map' for an ad-hoc defined
> sequence type and test using that type, e.g. this expression:
> 
> #+begin_src emacs-lisp
> (progn
>   (defvar gensym)
>   (let ((gensym (make-symbol "foo")))
>     (eval `(cl-defmethod seq-map (function (thing (head ,gensym)))
>              (append (list (car thing) (cadr thing)) (seq-map function (cddr thing))))
>           t)
>     (equal (list gensym nil 4 46)
>            (seq-keep (lambda (x) (and (integerp x) (* 2 x)))
>                      (list gensym nil 2 'x gensym 23)))))
> #+end_src
> 
> returns t with my patch installed and nil else and works without relying
> on something external.  I'm not sure if defining methods (for seq-map in
> this case) that are globally visible is allowed in tests, so I
> implemented the example above in a way that the change of the generic
> function is not visible from the outside (thus the "secret" gensym).
> 
> Would something like that be acceptable?
> 
> Sorry for my ignorance, I didn't write much tests before.

Sounds like over-engineering to me.

Like I said: it's your call.  If you see too many complications to adding a
test, and my suggestions don't convince you, I won't object to installing
your original proposal without a test.

Thanks.