Signal a warning for incompatible accumulation initializations.

- Rename error `loopy-incompatible-accumulations` to `loopy-incompatible-accumulation-types`. - Create the error `loopy-incompatible-accumulation-initializations`, but don't use it yet. - Signal a warning an there are multiple expected starting values for an accumulation variable. In the future, this will signal an error. See also this PR #203 and issue #169.
okamsn · Aug 20, 2024 · 97fbdec · 97fbdec
1 parent f261b1a
commit 97fbdec
Show file tree

Hide file tree

Showing 7 changed files with 119 additions and 48 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,22 @@
 
 This document describes the user-facing changes to Loopy.
 
+## Unreleased
+
+### Breaking Changes
+
+- Conflicting starting values for accumulation variables, such as from using
+  `sum` (value: 0) and `multiply` (value: 1), now signal an error ([#169],
+  [#203]).  In the future, they will signal an error.
+
+  For now, Loopy continues the behavior of using the first found starting value
+  from the macro arguments.  To silence this warning and to avoid this future
+  error, use the `with` special macro argument to explicitly state a starting
+  value for the accumulation variable.
+
+[#169]: https://github.com/okamsn/loopy/issues/169
+[#203]: https://github.com/okamsn/loopy/pull/203
+
 ## 0.13.0
 
 ### Breaking Changes

diff --git a/README.org b/README.org
@@ -30,6 +30,9 @@ please let me know.
 -----
 
  _Recent breaking changes:_
+ - Unreleased:
+   - Conflicting initialization values for accumulation variables now signal
+     a warning.  In the future, they will signal an error.
  - Version 0.13.0:
    - The deprecated =:init= keyword argument has been removed.  Use the =with=
      special macro argument instead.

diff --git a/doc/loopy-doc.org b/doc/loopy-doc.org
@@ -2558,37 +2558,6 @@ Keep in mind that it is an error to modify accumulation variables outside of
 accumulation commands.  This restriction allows accumulations to be much faster.
 #+end_quote
 
-#+cindex: accumulation initial values
-Like with other loop commands, variables created by accumulation commands (such
-as ~coll~ in the above example) are initialized to ~nil~ unless stated
-otherwise.  When otherwise, such as for the commands =sum= and =multiply=, the
-initial value of a variable depends on the first accumulation command using that
-variable in the arguments given to the macro.  Remember that a variable's
-initial value can be controlled using the =with= special macro argument.
-
-#+begin_src emacs-lisp
-  ;; => 27
-  (loopy (numbers i :from 1 :to 3)
-         (sum my-accum i) ; Starts at 0.
-         (multiply my-accum i)
-         (finally-return my-accum))
-
-  ;; => 21
-  (loopy (numbers i :from 1 :to 3)
-         (multiply my-accum i) ; Starts at 1.
-         (sum my-accum i)
-         (finally-return my-accum))
-
-  ;; Using `with':
-  ;;
-  ;; => 87
-  (loopy (with (my-accum 10))
-         (numbers i :from 1 :to 3)
-         (sum my-accum i) ; Starts at 0.
-         (multiply my-accum i)
-         (finally-return my-accum))
-#+end_src
-
 #+cindex: accumulation destructuring
 Similar to iteration commands, accumulation commands can also use destructuring.
 In accumulation commands, the values resulting from destructuring are
@@ -2703,11 +2672,46 @@ other hand, =sum= and =multiply= are compatible, since they both act on numbers.
 
   ;; Compatible commands:
   ;; => 27
-  (loopy (numbers i :from 1 :to 3)
+  (loopy (with (loopy-result 0))
+         (numbers i :from 1 :to 3)
          (sum i)
          (multiply i))
 #+end_src
 
+#+cindex: accumulation initial values
+Each accumulation command has a default initialization value for the
+accumulation variable.  For most commands, this is ~nil~.  This documentation
+tries to note when it is not ~nil~.  For example, the default starting value for
+the =sum= command is ~0~ and the default starting value for the =multiply=
+command is ~1~.  The default initialization value used by an accumulation
+command can be overridden using the =with= special macro argument.
+
+#+attr_texinfo: :tag Warning
+#+begin_quote
+Currently, a warning is raised when the default initial values of accumulation
+commands conflict.  In the future, this will be an error.  To resolve this
+conflict, use the =with= special macro argument, as noted above.
+#+end_quote
+
+#+begin_src emacs-lisp
+  ;; Raises a warning. Will raise an error in the future.
+  ;;
+  ;; => 27
+  (loopy (numbers i :from 1 :to 3)
+         (sum my-accum i) ; Starts at 0.
+         (multiply my-accum i)
+         (finally-return my-accum))
+
+  ;; No warning because using `with':
+  ;;
+  ;; => 87
+  (loopy (with (my-accum 10))
+         (numbers i :from 1 :to 3)
+         (sum my-accum i) ; Starts at 0.
+         (multiply my-accum i)
+         (finally-return my-accum))
+#+end_src
+
 By default, one must specify separate accumulation variables to be able to
 accumulate into separate values.  This can make accumulation slower, because
 ~loopy~ ensures that named accumulation variables (excluding the previously
@@ -2866,7 +2870,7 @@ all described below.
   #+begin_quote
   This argument is similar to the =:test= argument used by =cl-lib=, but is
   closer to the optional =testfn= argument used by =seq= (for example, in
-  ~seq-contains-p~).  This are two important differences:
+  ~seq-contains-p~).  There are two important differences:
   1. Tests default to ~equal~, like in other Emacs Lisp libraries, not ~eql~.
   2. The first argument is the existing value or sequence and the second
      argument is the tested value.  This is the /opposite/ of the order used by
@@ -2888,7 +2892,7 @@ all described below.
   #+end_src
 
 #+cindex: accumulation keyword key
-- =key= :: A one-argument function that transforms the _both_ tested value and
+- =key= :: A one-argument function that transforms _both_ the tested value and
   the value from sequence used by the =test= keyword.
 
   The keyword =key= is useful to avoid applying a transforming function to the

diff --git a/loopy-commands.el b/loopy-commands.el
@@ -1479,7 +1479,7 @@ command.
         (cl-destructuring-bind (existing-category existing-command)
             existing-description
           (unless (eq category existing-category)
-            (signal 'loopy-incompatible-accumulations
+            (signal 'loopy-incompatible-accumulation-types
                     (list existing-command
                           command))))
       (push (cons key (list category command))

diff --git a/loopy-misc.el b/loopy-misc.el
@@ -71,12 +71,16 @@
               '(loopy-error loopy-bad-command-arguments))
 
 ;;;;; Errors on Accumulations
-(define-error 'loopy-incompatible-accumulations
-              "Loopy: Incompatible accumulations"
+(define-error 'loopy-incompatible-accumulation-types
+              "Loopy: Incompatible accumulation types"
+              'loopy-error)
+
+(define-error 'loopy-incompatible-accumulation-initializations
+              "Loopy: Incompatible initial values for accumulations"
               'loopy-error)
 
 (define-error 'loopy-incompatible-accumulation-final-updates
-              "Loopy: Incompatible accumulations"
+              "Loopy: Incompatible expectations for accumulations"
               'loopy-error)
 
 (define-error 'loopy-missing-accum-counters

diff --git a/loopy.el b/loopy.el
@@ -648,9 +648,24 @@ macro `loopy' itself."
 
       (loopy--accumulation-vars
        (loopy--validate-binding instruction-value)
-       ;; Don't want to accidentally rebind variables to `nil'.
-       (unless (loopy--bound-p (cl-first instruction-value))
-         (push instruction-value loopy--accumulation-vars)))
+       ;; Don't want to accidentally rebind variables to `nil'
+       ;; or to accidentally mis-use commands that need
+       ;; different initial values.
+       (loopy--pcase-let-workaround (var new-val)
+         (pcase-let ((`(,var ,new-val) instruction-value))
+           (pcase var
+             ((pred loopy--with-bound-p) nil)
+             ((and (app loopy--command-bound-p `(,_place . ,old-val))
+                   (guard (not (equal new-val old-val))))
+              ;; TODO: Switch from raising a warning to raising an error.
+              ;; (signal 'loopy-incompatible-accumulation-initializations
+              ;;         (list :in place :var var :old old-val :new new-val))
+              (display-warning
+               'loopy
+               (format "loopy: Conflicting accumulation starting values: `%s', %s, %s\nThis will be an error in the future.  To resolve this error, use `with' to explicitly specify a starting value."
+                       var old-val new-val)
+               :warning))
+             (_ (push instruction-value loopy--accumulation-vars))))))
 
       (loopy--other-vars
        (loopy--validate-binding instruction-value)

diff --git a/tests/tests.el b/tests/tests.el
@@ -629,7 +629,7 @@ SYMS-STR are the string names of symbols from `loopy-iter-bare-commands'."
   :iter-keyword (array loopy))
 
 (loopy-deftest at-disagreeing-accum-types
-  :error loopy-incompatible-accumulations
+  :error loopy-incompatible-accumulation-types
   :macroexpand t
   :multi-body t
   :body ((outer
@@ -3137,14 +3137,43 @@ expansion time."
               (nunion . nunioning)
               (nconc . nconcing)))
 
+;; TODO: Enable these tests in a future version.
+;;
+;; (loopy-deftest accumulation-compatibility-different-inits-1
+;;   :doc "Check that accumulation commands with different initial values raise an error.
+;; This should apply even when they're compatible types."
+;;   :error loopy-incompatible-accumulation-initializations
+;;   :macroexpand t
+;;   :body ((list i '(1 2 3 4 5))
+;;          (sum i)
+;;          (multiply i))
+;;   :loopy t
+;;   :iter-keyword (list sum multiply)
+;;   :iter-bare ((list . listing)
+;;               (sum . summing)
+;;               (multiply . multiplying)))
+;;
+;; (loopy-deftest accumulation-compatibility-different-inits-2
+;;   :doc "Check that `with' on the variable (see test 1) avoids the error."
+;;   :result 27
+;;   :body ((with (loopy-result 0))
+;;          (list i '(1 2 3))
+;;          (sum i)
+;;          (multiply i))
+;;   :loopy t
+;;   :iter-keyword (list sum multiply)
+;;   :iter-bare ((list . listing)
+;;               (sum . summing)
+;;               (multiply . multiplying)))
+
 (loopy-deftest accumulation-compatibility-different-types
   :doc "Check that commands with different accumulation types should raise error."
-  :error loopy-incompatible-accumulations
+  :error loopy-incompatible-accumulation-types
   :macroexpand t
   :multi-body t
-  :body [((list i '((1 2) (3 4) (5 6)))
-          (collect i)
-          (concat i))
+  :body [(loopy (list i '((1 2) (3 4) (5 6)))
+                (collect i)
+                (concat i))
 
          ((list i '((1 2) (3 4) (5 6)))
           (append i)
@@ -5518,7 +5547,7 @@ Not multiple of 3: 7"
               (collect . collecting)))
 
 (loopy-deftest thereis-always-same-var
-  :error loopy-incompatible-accumulations
+  :error loopy-incompatible-accumulation-types
   :multi-body t
   :body [((list i '(1 2 3))
           (always i)
@@ -5534,7 +5563,7 @@ Not multiple of 3: 7"
   :iter-bare ((list . listing)))
 
 (loopy-deftest thereis-never-same-var
-  :error loopy-incompatible-accumulations
+  :error loopy-incompatible-accumulation-types
   :multi-body t
   :body  [((list i '(1 2 3))
            (never i)