Subject: Re: bug#68863: Add support for using setf with seq-subseq

From: Okamsn <[email-masked]>
Date: Tue, 14 May 2024 12:47:14 +0000

Michael Heerdegen wrote:
> Hello again,
> 
> There is a second point I want to hint at/ discuss:
> 
> Some time ago we have removed lots of or even the majority of the
> so far implemented generalized variables.
> 
> The main criteria were: are they useful, and is the semantics clear.  We
> should think about whether this is the case here.
> 
> For `seq-subseq' the semantics is not completely unambiguous.  For
> example, the setter could exchange a subsequence - but also simply
> insert an additional piece, creating a longer sequence.  Both operations
> can be useful.
> 
> Likewise, when an index (e.g. the starting position) is going beyond the
> end of a sequence, Emacs could raise an error, or provide functionality
> to append to the existing sequence.  Again, both kinds of behavior could
> make sense in different situations.
> 
> Finally, the question of whether the operation is destructive, or which
> of the involved sequences are reused, is not trivial.
> 
> Any thoughts about these points?  Is this generalized variable useful
> enough to make this acceptable?  Which kind of implementation do we
> prefer then?
> 
> 
> Michael.

Hello,

Here is what I have thought:

1. The setter should be consistent for different kinds of sequences. My 
understanding is that arrays cannot be extended, so I think that it 
should not be able to extend lists either. I think that something like a 
non-destructive `seq-replace` or a non-destructive `seq-splice` is a 
separate feature.

2. The setter should be consistent with how `seq-subseq` raises errors. 
`seq-subseq` raises an error if a given index is greater than the 
sequence's length, so the setter should too.

3. The setter should not modify the sequence containing the replacement 
values. The manual states: "All functions defined in this library are 
free of side-effects; i.e., they do not modify any sequence (list, 
vector, or string) that you pass as an argument." I think that it makes 
sense for the setter to modify the target sequence, because `setf` is 
used to set places to values, but I think that it would be unexpected to 
modify the sequence of replacement values and that it would be contrary 
to what the manual states and how the other features behave.

I will try to find uses of `cl-replace` and maybe uses of `append` and 
`nconc` for examples. Also, I have attached a version of the patch that 
does not compute the length of the list unless needed.

Thank you.

From 89c6800e4e788d6a121114bc16f47e1bbc2ee814 Mon Sep 17 00:00:00 2001
From: Earl Hyatt <[email-masked]>
Date: Sun, 28 Jan 2024 22:48:13 -0500
Subject: [PATCH v4] Add setf support for seq-subseq.

* lisp/emacs-lisp/seq.el (seq-subseq): Add a generic version of
calling setf on seq-subseq and add a specialized version for when the
modified sequence is a list.
* test/lisp/emacs-lisp/seq-tests.el (test-setf-seq-subseq)
(test-setf-seq-subseq-combinations): Add tests for the feature.

The feature will signal 'args-out-of-range' if the starting index or
ending index (if given) is outside of the range of values from 0
through the length of the sequence or from the negative length of the
sequence through negative 1.  If the starting index is equal to the
length of the sequence, then nothing is changed.  If the starting
index is equal to the ending index, then nothing is changed.  The
feature should signal an error in all cases where using 'seq-subseq'
would signal an error.
---
 lisp/emacs-lisp/seq.el            | 104 +++++++++++++++++++++
 test/lisp/emacs-lisp/seq-tests.el | 149 ++++++++++++++++++++++++++++++
 2 files changed, 253 insertions(+)

diff --git a/lisp/emacs-lisp/seq.el b/lisp/emacs-lisp/seq.el
index a20cff16982..95688a60645 100644
--- a/lisp/emacs-lisp/seq.el
+++ b/lisp/emacs-lisp/seq.el
@@ -193,6 +193,110 @@ seq-subseq
         (copy-sequence sequence))))
    (t (error "Unsupported sequence: %s" sequence))))
 
+(cl-defgeneric (setf seq-subseq) (store sequence start &optional end)
+  "Modify the elements of SEQUENCE from START to END to be those of STORE.
+END is exclusive.
+
+If END is omitted, it defaults to the length of the sequence.  If
+START or END is negative, it counts from the end.  Signal an
+error if START or END are outside of the sequence (i.e too large
+if positive or too small if negative).
+
+SEQUENCE is neither lengthened nor shortened."
+  (let* ((len (seq-length sequence))
+         (signal-fn (lambda ()
+                      (signal 'args-out-of-range
+                              (if end
+                                  (list sequence start end)
+                                (list sequence start)))))
+         (signal-or-val-fn (lambda (val)
+                             (cond
+                              ((> val len)
+                               (funcall signal-fn))
+                              ((< val 0)
+                               (let ((val2 (+ val len)))
+                                 (if (< val2 0)
+                                     (funcall signal-fn)
+                                   val2)))
+                              (t
+                               val))))
+         (idx (funcall signal-or-val-fn start))
+         (idx-end (if (null end)
+                      len
+                    (funcall signal-or-val-fn end)))
+         (tag (gensym)))
+    (if (> idx idx-end)
+        (funcall signal-fn)
+      (catch tag
+        (seq-do (lambda (v)
+                  (if (< idx idx-end)
+                      (setf (seq-elt sequence idx) v
+                            idx (1+ idx))
+                    (throw tag nil)))
+                store))))
+  store)
+
+(cl-defmethod (setf seq-subseq) (store (sequence list) start &optional end)
+  "Modify the elements of SEQUENCE from START to END to be those of STORE.
+END is exclusive.
+
+If END is omitted, it defaults to the length of the sequence.  If
+START or END is negative, it counts from the end.  Signal an
+error if START or END are outside of the sequence (i.e too large
+if positive or too small if negative).
+
+SEQUENCE is neither lengthened nor shortened."
+  (let* ((arg-start start)
+         (arg-end end)
+         (signal-fn (lambda ()
+                      (signal 'args-out-of-range
+                              (if arg-end
+                                  (list sequence arg-start arg-end)
+                                (list sequence arg-start)))))
+         (len)
+         (neg-start (< start 0))
+         (neg-end (and end (< end 0))))
+    ;; Avoid calculating the length of the list if we don't need too.
+    (when (or neg-start neg-end)
+      (setq len (length sequence))
+      (when neg-start
+        (setq start (+ len start))
+        (when (or (>= start len)
+                  (< start 0))
+          (funcall signal-fn)))
+      (when neg-end
+        (setq end (+ len end))
+        (when (or (> end len)
+                  (< end 0))
+          (funcall signal-fn))))
+    ;; If we already calculated the length, then we
+    ;; already checked START or END.  If not,
+    ;; then we check whether the nth cdr satisfies
+    ;; START and END.
+    (let ((replaced (nthcdr start sequence)))
+      (if (or (null replaced)
+              (and (null len)
+                   end
+                   (length< replaced (- end start))))
+          (funcall signal-fn)
+        (let ((tag (gensym)))
+          (catch tag
+            (seq-do (if end
+                        (let ((idx start))
+                          (lambda (v)
+                            (if (< idx end)
+                                (setf (car replaced) v
+                                      replaced (cdr replaced)
+                                      idx (1+ idx))
+                              (throw tag nil))))
+                      (lambda (v)
+                        (if replaced
+                            (setf (car replaced) v
+                                  replaced (cdr replaced))
+                          (throw tag nil))))
+                    store))))))
+  store)
+
 
 (cl-defgeneric seq-map (function sequence)
   "Return the result of applying FUNCTION to each element of SEQUENCE."
diff --git a/test/lisp/emacs-lisp/seq-tests.el b/test/lisp/emacs-lisp/seq-tests.el
index c06ceb00bdb..d3e46c32f99 100644
--- a/test/lisp/emacs-lisp/seq-tests.el
+++ b/test/lisp/emacs-lisp/seq-tests.el
@@ -312,6 +312,155 @@ test-seq-subseq
                   (:success
                    (should (equal (seq-subseq list start end) res))))))))))))
 
+(cl-defmacro test-setf-seq-subseq-combinations
+    (&key init-vals sub-vals result range error)
+  "Make a test for each combination of sequence type for `seq-subseq' using `setf'.
+
+- INIT-VALS is a list holding the initial elements.
+- RESULT is what the final value should be after substitution.
+- ERROR is whether the form should signal `args-out-of-range'.
+- SUB-VALS is a list holding the elements to be substituted in.
+- RANGE is a list of the `start' and `end' arguments of `seq-subseq'."
+  (let ((tests))
+    (dolist (type1 '(list vector string))
+      (dolist (type2 '(list vector string))
+        (push  (if error
+                   `(should-error (let ((seq (,type1 ,@init-vals)))
+                                    (setf (seq-subseq seq ,@range)
+                                          (,type2 ,@sub-vals))
+                                    seq)
+                                  :type (quote args-out-of-range))
+                 `(should (equal (,type1 ,@result)
+                                 (let ((seq (,type1 ,@init-vals)))
+                                   (setf (seq-subseq seq ,@range)
+                                         (,type2 ,@sub-vals))
+                                   seq))))
+               tests)))
+    `(progn ,@tests)))
+
+(ert-deftest test-setf-seq-subseq ()
+  "Test using `seq-subseq' with `setf'.
+
+Any combination of sequences should work.
+
+An error should be signalled if the inclusive starting index or
+the exclusive ending index is out of the range from 0 through the
+length of the sequence, or if the starting index is greater than
+the ending index.  If the starting index is equal to the ending
+index, then nothing is changed.  If the starting index is equal
+to the length of the sequence, then nothing is changed.  It
+should signal an error in all the cases that `seq-subseq' signals
+an error."
+  (test-setf-seq-subseq-combinations
+   :init-vals (0 1 2)
+   :sub-vals (10 11 12)
+   :range (0)
+   :result (10 11 12))
+
+  (test-setf-seq-subseq-combinations
+   :init-vals (0 1)
+   :sub-vals (10 11 12)
+   :range (0)
+   :result (10 11))
+
+  (test-setf-seq-subseq-combinations
+   :init-vals (0 1 2)
+   :sub-vals (10 11 12)
+   :range (1)
+   :result (0 10 11))
+
+  (test-setf-seq-subseq-combinations
+   :init-vals (0 1 2)
+   :sub-vals (10 11 12)
+   :range (1 3)
+   :result (0 10 11))
+
+  (test-setf-seq-subseq-combinations
+   :init-vals (0 1 2)
+   :sub-vals (10 11 12)
+   :range (3 1)
+   :error t)
+
+  (test-setf-seq-subseq-combinations
+   :init-vals (0 1 2)
+   :sub-vals (10 11 12)
+   :range (1 100)
+   :error t)
+
+  (test-setf-seq-subseq-combinations
+   :init-vals (0 1 2)
+   :sub-vals (10 11 12)
+   :range (7)
+   :error t)
+
+  (test-setf-seq-subseq-combinations
+   :init-vals (0 1 2 3 4)
+   :sub-vals (12 13 14 15)
+   :range (2 3)
+   :result (0 1 12 3 4))
+
+  (test-setf-seq-subseq-combinations
+   :init-vals (0 1 2 3 4)
+   :sub-vals (12 13 14 15)
+   :range (2 2)
+   :result (0 1 2 3 4))
+
+  (test-setf-seq-subseq-combinations
+   :init-vals (0 1 2 3 4)
+   :sub-vals (12 13 14 15)
+   :range (5)
+   :result (0 1 2 3 4))
+
+  (test-setf-seq-subseq-combinations
+   :init-vals (0 1 2 3 4)
+   :sub-vals (12 13 14 15)
+   :range (6)
+   :error t)
+
+  (test-setf-seq-subseq-combinations
+   :init-vals (0 1 2 3 4)
+   :sub-vals (12 13 14 15)
+   :range (5 6)
+   :error t)
+
+  (test-setf-seq-subseq-combinations
+   :init-vals (0 1 2 3 4 5 6 7 8 9)
+   :sub-vals (10 11 12 13 14)
+   :range (-2)
+   :result (0 1 2 3 4 5 6 7 10 11))
+
+  (test-setf-seq-subseq-combinations
+   :init-vals (0 1 2 3 4 5 6 7 8 9)
+   :sub-vals (10 11 12 13 14)
+   :range (-6 -3)
+   :result (0 1 2 3 10 11 12 7 8 9))
+
+  (test-setf-seq-subseq-combinations
+   :init-vals (0 1 2 3 4 5 6 7 8 9)
+   :sub-vals (10 11 12 13 14)
+   :range (-6 -10)
+   :error t)
+
+  ;; This range might make sense, but since it would signal an error
+  ;; in `seq-subseq', we also signal an error in the `setf' feature.
+  (test-setf-seq-subseq-combinations
+   :init-vals (0 1 2 3 4 5 6 7 8 9)
+   :sub-vals (10 11 12 13 14)
+   :range (-6 0)
+   :error t)
+
+  (test-setf-seq-subseq-combinations
+   :init-vals (0 1 2 3 4 5 6 7 8 9)
+   :sub-vals (10 11 12 13 14)
+   :range (100)
+   :error t)
+
+  (test-setf-seq-subseq-combinations
+   :init-vals (0 1 2 3 4 5 6 7 8 9)
+   :sub-vals (10 11 12 13 14)
+   :range (-100)
+   :error t))
+
 (ert-deftest test-seq-concatenate ()
   (with-test-sequences (seq '(2 4 6))
     (should (equal (seq-concatenate 'string seq [8]) (string 2 4 6 8)))
-- 
2.34.1