From 311e9b7fabe18ed3fad6392b29897d4c58579159 Mon Sep 17 00:00:00 2001 From: okamsn <28612288+okamsn@users.noreply.github.com> Date: Sun, 17 Sep 2023 14:45:22 +0000 Subject: [PATCH] Fix some bugs with `find` and nil values. (#171) - Fix `:on-failure` when the accumulated value is set to nil. Previously, a nil value was interpreted a failing the test, which meant that the value `:on-failure` would be used. - Fix detecting `:on-failure` when the keyword value is nil. Previously, if the value given by `:on-failure` was nil, then that would be interpreted as the keyword not being used. --- CHANGELOG.md | 25 +++++++++++ loopy-commands.el | 44 ++++++++++++------- tests/tests.el | 108 +++++++++++++++++++++++++++++++++++++--------- 3 files changed, 142 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ae332a9..a72be94e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,31 @@ This document describes the user-facing changes to Loopy. (reduce i #'*)) ``` +- Fix `find` when `:on-failure` is nil. Previously, `nil` was interpreted as + not passing `:on-failure`. + + ```emacs-lisp + ;; Previously erroneously returned 27: + ;; => nil + (loopy (with (val 27)) + (list i '(1 2 3)) + (find val nil (> i 10) :on-failure nil) + (finally-return val)) + ``` + +- Fix `find` when `EXPR` is nil and `:on-failure` is given. Previously, after + the test passed and `VAR` was set to `nil`, that `nil` was interpreted as not + passing the test, so that `VAR` then bound to the value passed for + `:on-failure`. + + ```emacs-lisp + ;; Previously erroneously returned 27: + ;; => nil + (loopy (list i '(1 2 3)) + (find nil (> i 1) :on-failure 27)) + ``` + + ### Breaking Changes - Make it an error to re-use iteration variables with multiple iteration diff --git a/loopy-commands.el b/loopy-commands.el index 6b87c1ab..252569fc 100644 --- a/loopy-commands.el +++ b/loopy-commands.el @@ -2244,35 +2244,49 @@ This function is called by `loopy--expand-optimized-accum'." (test-form (if (loopy--quoted-form-p test-arg) `(,(loopy--get-function-symbol test-arg) ,val) test-arg)) + (of-used (plist-member opts :on-failure)) (on-failure (plist-get opts :on-failure)) (tag-name (loopy--produce-non-returning-exit-tag-name - loopy--loop-name))) + loopy--loop-name)) + (found (gensym "found"))) `((loopy--non-returning-exit-used ,tag-name) (loopy--accumulation-vars (,var nil)) - (loopy--main-body (when ,test-form - (setq ,var ,val) - (throw (quote ,tag-name) t))) - ;; If VAR nil, bind to ON-FAILURE. - ,(when on-failure - `(loopy--vars-final-updates - (,var . (if ,var nil (setq ,var ,on-failure))))))) + ,@(if of-used + ;; If TEST always nil, bind to ON-FAILURE. + `((loopy--accumulation-vars (,found nil)) + (loopy--main-body (when ,test-form + (setq ,var ,val + ,found t) + (throw (quote ,tag-name) t))) + (loopy--vars-final-updates + (,var . (unless ,found (setq ,var ,on-failure))))) + `((loopy--main-body (when ,test-form + (setq ,var ,val) + (throw (quote ,tag-name) t))))))) :implicit (let* ((test-arg (cl-second args)) (test-form (if (loopy--quoted-form-p test-arg) `(,(loopy--get-function-symbol test-arg) ,val) test-arg)) + (of-used (plist-member opts :on-failure)) (on-failure (plist-get opts :on-failure)) + (found (gensym "found")) (tag-name (loopy--produce-non-returning-exit-tag-name loopy--loop-name))) `((loopy--non-returning-exit-used ,tag-name) (loopy--accumulation-vars (,var nil)) - (loopy--main-body (when ,test-form - (setq ,var ,val) - (throw (quote ,tag-name) t))) - ;; If VAR nil, bind to ON-FAILURE. - ,(when on-failure - `(loopy--vars-final-updates - (,var . (if ,var nil (setq ,var ,on-failure))))) + ,@(if of-used + ;; If TEST always nil, bind to ON-FAILURE. + `((loopy--accumulation-vars (,found nil)) + (loopy--main-body (when ,test-form + (setq ,var ,val + ,found t) + (throw (quote ,tag-name) t))) + (loopy--vars-final-updates + (,var . (unless ,found (setq ,var ,on-failure))))) + `((loopy--main-body (when ,test-form + (setq ,var ,val) + (throw (quote ,tag-name) t))))) (loopy--implicit-return ,var)))) ;;;;;;; Set Accum diff --git a/tests/tests.el b/tests/tests.el index ec3a2db6..43ffd899 100644 --- a/tests/tests.el +++ b/tests/tests.el @@ -4991,8 +4991,13 @@ Not multiple of 3: 7" ;;;;; finding (loopy-deftest find-pass-notest :result 3 - :body ((list i '(1 2 3)) - (_cmd i (> i 2))) + :multi-body t + :body [((list i '(1 2 3)) + (_cmd res i (> i 2)) + (finally-return res)) + + ((list i '(1 2 3)) + (_cmd i (> i 2)))] :repeat _cmd :loopy ((_cmd . (find finding))) :iter-keyword ((list . list) @@ -5002,8 +5007,13 @@ Not multiple of 3: 7" (loopy-deftest find-fail-notest :result nil - :body ((list i '(1 2 3)) - (find i (> i 4))) + :multi-body t + :body [((list i '(1 2 3)) + (find res i (> i 4)) + (finally-return res)) + + ((list i '(1 2 3)) + (find i (> i 4)))] :loopy t :iter-keyword (list find) :iter-bare ((list . listing) @@ -5011,8 +5021,13 @@ Not multiple of 3: 7" (loopy-deftest find-fail-onfail :result 0 - :body ((list i '(1 2 3)) - (find i (> i 4) :on-failure 0)) + :multi-body t + :body [((list i '(1 2 3)) + (find res i (> i 4) :on-failure 0) + (finally-return res)) + + ((list i '(1 2 3)) + (find i (> i 4) :on-failure 0))] :loopy t :iter-keyword (list find) :iter-bare ((list . listing) @@ -5020,8 +5035,30 @@ Not multiple of 3: 7" (loopy-deftest find-pass-onfail :result 2 - :body ((list i '(1 2 3)) - (find i (> i 1) :on-failure 0)) + :multi-body t + :body [((list i '(1 2 3)) + (find res i (> i 1) :on-failure 0) + (finally-return res)) + + ((list i '(1 2 3)) + (find i (> i 1) :on-failure 0))] + :loopy t + :iter-keyword (list find) + :iter-bare ((list . listing) + (find . finding))) + +(loopy-deftest find-expr-onfail-explicit-nil + :doc "Make sure a nil value will still be set on failure." + :result nil + :multi-body t + :body [((with (val 27)) + (list i '(1 2 3)) + (find val nil (> i 10) :on-failure nil) + (finally-return val)) + + ((with (loopy-result 27)) + (list i '(1 2 3)) + (find nil (> i 10) :on-failure nil))] :loopy t :iter-keyword (list find) :iter-bare ((list . listing) @@ -5029,9 +5066,13 @@ Not multiple of 3: 7" (loopy-deftest find-pass-var :result 2 - :body ((list i '(1 2 3)) - (find found i (= i 2)) - (finally-return found)) + :multi-body t + :body [((list i '(1 2 3)) + (find found i (= i 2)) + (finally-return found)) + + ((list i '(1 2 3)) + (find i (= i 2)))] :loopy t :iter-keyword (list find) :iter-bare ((list . listing) @@ -5039,9 +5080,13 @@ Not multiple of 3: 7" (loopy-deftest find-fail-var :result nil - :body ((list i '(1 2 3)) - (find found i (> i 3)) - (finally-return found)) + :multi-body t + :body [((list i '(1 2 3)) + (find found i (> i 3)) + (finally-return found)) + + ((list i '(1 2 3)) + (find i (> i 3)))] :loopy t :iter-keyword (list find) :iter-bare ((list . listing) @@ -5049,10 +5094,13 @@ Not multiple of 3: 7" (loopy-deftest find-fail-var-onfail :result "not found" - :body ((list i '(1 2 3)) - (find whether-found i (> i 4) - :on-failure "not found") - (finally-return whether-found)) + :multi-body t + :body [((list i '(1 2 3)) + (find whether-found i (> i 4) :on-failure "not found") + (finally-return whether-found)) + + ((list i '(1 2 3)) + (find i (> i 4) :on-failure "not found"))] :loopy t :iter-keyword (list find) :iter-bare ((list . listing) @@ -5060,10 +5108,30 @@ Not multiple of 3: 7" (loopy-deftest find-pass-var-onfail :result 2 - :body ((list i '(1 2 3)) - (find whether-found i (> i 1) + :multi-body t + :body [((list i '(1 2 3)) + (find whether-found i (> i 1) + :on-failure "not found") + (finally-return whether-found)) + + ((list i '(1 2 3)) + (find i (> i 1) :on-failure "not found"))] + :loopy t + :iter-keyword (list find) + :iter-bare ((list . listing) + (find . finding))) + +(loopy-deftest find-expr-is-nil-onfail + :doc "Make sure a nil value not assumed to be a failure." + :result nil + :multi-body t + :body [((list i '(1 2 3)) + (find whether-found nil (> i 1) :on-failure "not found") (finally-return whether-found)) + + ((list i '(1 2 3)) + (find nil (> i 1) :on-failure "not found"))] :loopy t :iter-keyword (list find) :iter-bare ((list . listing)