behaviour change in cl-subseq

behaviour change in cl-subseq

[Phillip Lord]

I've just had a bug report for pabbrev. The root cause is a change in
the implementation of cl-subseq. 

In Emacs 24:

(cl-subseq '() 10 10) ;; nil

While in Emacs 25

In Emacs 25 `cl-subseq' has been redefined in terms of seq-subseq.

(seq-subseq '() 10 10) ;; errors
(cl-subseq '() 10 10) ;; errors

Which is a reasonably substantial change in the interface of cl-subseq.
The actual (exceptional) behaviour of cl-subseq is not documented, so
it's reasonable, but perhaps not sensible. It's easy to fix in pabbrev
(and anywhere) in a way which does not require me to probe for Emacs
versions.

I offer no opinions, just wanted to check whether this was intended.

Phil

Re: behaviour change in cl-subseq

[Leo Liu]

On 2015-08-05 20:14 +0800, Phillip Lord wrote:
> (cl-subseq '() 10 10) ;; errors

What did you get when you run it in Common Lisp?

Leo

Re: behaviour change in cl-subseq

[Tassilo Horn]

Leo Liu <sdl.web@gmail.com> writes:

>> (cl-subseq '() 10 10) ;; errors
>
> What did you get when you run it in Common Lisp?

In SBCL 1.2.12 I get an error:


* (subseq '() 10 10)

debugger invoked on a SB-KERNEL:BOUNDING-INDICES-BAD-ERROR in thread
#<THREAD "main thread" RUNNING {1002D76633}>:
  The bounding indices 10 and 10 are bad for a sequence of length 0.
See also:
  The ANSI Standard, Glossary entry for "bounding index designator"
  The ANSI Standard, writeup for Issue SUBSEQ-OUT-OF-BOUNDS:IS-AN-ERROR

So I guess the new behavior is in compliance with Common Lisp.  Or well,
not with the current ANSI standard but with a future one.

Bye,
Tassilo

Re: behaviour change in cl-subseq

[Phillip Lord]

Leo Liu <sdl.web@gmail.com> writes:

> On 2015-08-05 20:14 +0800, Phillip Lord wrote:
>> (cl-subseq '() 10 10) ;; errors
>
> What did you get when you run it in Common Lisp?

Fortunately, Tassilo has answered this; I've never used CL.

I agree that this change is entirely justifiable. I'm generally not a
huge fan of silently ignoring inaccurate arguments. If you are happy
with the changed interface, then I have no major problem, although it
may break packages besides mine.

Do you mind if I patch the docstrings, though?

Phil

Re: behaviour change in cl-subseq

[Leo Liu]

On 2015-08-06 17:34 +0800, Phillip Lord wrote:
> Fortunately, Tassilo has answered this; I've never used CL.

I answered it in a hurry before going to work :(

> I agree that this change is entirely justifiable. I'm generally not a
> huge fan of silently ignoring inaccurate arguments. If you are happy
> with the changed interface, then I have no major problem, although it
> may break packages besides mine.

The old behaviour was odd and inconsistent (try it on []).

> Do you mind if I patch the docstrings, though?

Not at all.

Leo

Re: behaviour change in cl-subseq

[Phillip Lord]

Leo Liu <sdl.web@gmail.com> writes:

> On 2015-08-06 17:34 +0800, Phillip Lord wrote:
>> Fortunately, Tassilo has answered this; I've never used CL.
>
> I answered it in a hurry before going to work :(
>
>> I agree that this change is entirely justifiable. I'm generally not a
>> huge fan of silently ignoring inaccurate arguments. If you are happy
>> with the changed interface, then I have no major problem, although it
>> may break packages besides mine.
>
> The old behaviour was odd and inconsistent (try it on []).
>
>> Do you mind if I patch the docstrings, though?
>
> Not at all.


Was working through this, and found this behaviour.

;; error
(seq-subseq '() 10 10)

;; nil
(seq-subseq '() -10 -10)

So, a bounding box error when the index is too large, but it is silently
ignored when too low.

Not convinced this makes sense.

Phil

Re: behaviour change in cl-subseq

[Nicolas Petton]

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

Phillip Lord <phillip.lord@newcastle.ac.uk> writes:

> Was working through this, and found this behaviour.
>
> ;; error
> (seq-subseq '() 10 10)
>
> ;; nil
> (seq-subseq '() -10 -10)
>
> So, a bounding box error when the index is too large, but it is silently
> ignored when too low.
>
> Not convinced this makes sense.

Indeed, it doesn't.  Do you want to push a patch for it?

Nico
-- 
Nicolas Petton
http://nicolas-petton.fr

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: behaviour change in cl-subseq

[Phillip Lord]

Nicolas Petton <nicolas@petton.fr> writes:

> Phillip Lord <phillip.lord@newcastle.ac.uk> writes:
>
>> Was working through this, and found this behaviour.
>>
>> ;; error
>> (seq-subseq '() 10 10)
>>
>> ;; nil
>> (seq-subseq '() -10 -10)
>>
>> So, a bounding box error when the index is too large, but it is silently
>> ignored when too low.
>>
>> Not convinced this makes sense.
>
> Indeed, it doesn't.  Do you want to push a patch for it?


Yes, will do. My expectation is this:

;; error
(seq-subseq '() 10 10)

;; (same) error
(seq-subseq '() -10 -10)


You agree?

Phil

Re: behaviour change in cl-subseq

[Nicolas Petton]

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

Phillip Lord <phillip.lord@newcastle.ac.uk> writes:

> Yes, will do. My expectation is this:
>
> ;; error
> (seq-subseq '() 10 10)
>
> ;; (same) error
> (seq-subseq '() -10 -10)
>
>
> You agree?

Yes, I do.

Nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: behaviour change in cl-subseq

[Phillip Lord]

Nicolas Petton <nicolas@petton.fr> writes:

> Phillip Lord <phillip.lord@newcastle.ac.uk> writes:
>
>> Yes, will do. My expectation is this:
>>
>> ;; error
>> (seq-subseq '() 10 10)
>>
>> ;; (same) error
>> (seq-subseq '() -10 -10)
>>
>>
>> You agree?
>
> Yes, I do.


Added as fix/subsequence-error-with-negative-sequences branching from
current trunk.

Would you like to check that I have got everything in place? Please feel
free to merge/rebase to trunk iff you are happy. If not, let me know and
I will improve.

Phil

Re: behaviour change in cl-subseq

[Nicolas Petton]

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

Phillip Lord <phillip.lord@newcastle.ac.uk> writes:

> Added as fix/subsequence-error-with-negative-sequences branching from
> current trunk.
>
> Would you like to check that I have got everything in place? Please feel
> free to merge/rebase to trunk iff you are happy. If not, let me know and
> I will improve.

It looks great, and thanks for adding regression tests!
I merged your branch into master.

Cheers,
Nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: behaviour change in cl-subseq

[Phillip Lord]

Nico

I'm wondering about this function.


(defun seq-contains-p (seq elt &optional testfn)
  "Return the first element in SEQ that equals to ELT.
Equality is defined by TESTFN if non-nil or by `equal' if nil."
  (seq-some-p (lambda (e)
                (funcall (or testfn #'equal) elt e))
              seq))

Two issues. Implementation wise, you are depending on behaviour outside
the interface of seq-some-p which says....

"Return any element for which (PRED element) is non-nil in SEQ, nil
  otherwise."

which is "any" not "first". Minor point, and technically an
implementation detail.

But the more serious problem is this...

;; => nil
(seq-contains-p '(1 2 3) nil)
;; => nil
(seq-contains-p '(1 2 nil) nil)

Which is correct by the documentation of seq-contains-p but not actually
any use. Why not have it return nil or t?

;; => nil
(seq-contains-p '(1 2 3) nil)
;; => t
(seq-contains-p '(1 2 nil) nil)

As it happens, that forces the implementation issue as well (because you
can use `seq-some-p' any more for the same reason).

The other option is to add a `seq-contains-nil-p' function.

Personally, I'd change seq-contains-p. Better to break the interface of
seq-contains-p earlier rather than later.

Phil

Re: behaviour change in cl-subseq

[Artur Malabarba]

> (defun seq-contains-p (seq elt &optional testfn)
>   "Return the first element in SEQ that equals to ELT.
> Equality is defined by TESTFN if non-nil or by `equal' if nil."
>   (seq-some-p (lambda (e)
>                 (funcall (or testfn #'equal) elt e))
>               seq))
>
> Two issues. Implementation wise, you are depending on behaviour outside
> the interface of seq-some-p which says....
>
> "Return any element for which (PRED element) is non-nil in SEQ, nil
>   otherwise."
>
> The other option is to add a `seq-contains-nil-p' function.
>
> Personally, I'd change seq-contains-p. Better to break the interface of
> seq-contains-p earlier rather than later.

I think both functions need to change behavior. They are named as
`-p', but "return an element" is not the behavior of a boolean
predicate (precisely because of the situation you describe).

IMO, the current behavior of `seq-some-p' should be called `seq-some'.
And the same for `seq-contains'.

Re: behaviour change in cl-subseq

[Nicolas Petton]

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

Phillip Lord <phillip.lord@newcastle.ac.uk> writes:

> I've just had a bug report for pabbrev. The root cause is a change in
> the implementation of cl-subseq. 
>
> In Emacs 24:
>
> (cl-subseq '() 10 10) ;; nil
>
> While in Emacs 25
>
> In Emacs 25 `cl-subseq' has been redefined in terms of seq-subseq.
>
> (seq-subseq '() 10 10) ;; errors
> (cl-subseq '() 10 10) ;; errors
>
> Which is a reasonably substantial change in the interface of cl-subseq.
> The actual (exceptional) behaviour of cl-subseq is not documented, so
> it's reasonable, but perhaps not sensible. It's easy to fix in pabbrev
> (and anywhere) in a way which does not require me to probe for Emacs
> versions.
>
> I offer no opinions, just wanted to check whether this was intended.

Hi,

The change was apparently intended, and was done in commit
253d44bd27b7d90b614b6b968a3b125eeb0a48f2

Cheers,
Nico
-- 
Nicolas Petton
http://nicolas-petton.fr

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 512 bytes --]