Support multi-arity functions in cljr-change-function-signature

CHANGELOG.md

@@ -2,7 +2,8 @@
 
 ## Unreleased
 
-- `cljr-change-function-signature` can now add and remove parameters, not just reorder and rename them. In the edit buffer, `a` adds a parameter (you're prompted for its name and a placeholder to insert at call sites) and `k`/`d` marks one for removal. Added parameters get their placeholder inserted at every call site; removals are never auto-deleted (an argument might have side effects) - the affected call sites are routed to the manual-intervention buffer for review. Still single-arity only.
+- `cljr-change-function-signature` can now add and remove parameters, not just reorder and rename them. In the edit buffer, `a` adds a parameter (you're prompted for its name and a placeholder to insert at call sites) and `k`/`d` marks one for removal. Added parameters get their placeholder inserted at every call site; removals are never auto-deleted (an argument might have side effects) - the affected call sites are routed to the manual-intervention buffer for review.
+- `cljr-change-function-signature` now handles multi-arity functions. It asks which arity to change, then updates only that arity's definition and the call sites whose argument count matches (call sites of other arities are left alone). Run it once per arity to change several.
 - Fix `cljr-change-function-signature` erroring with "Can't do work on functions of multiple arities" on ordinary single-arity functions. It parsed the middleware's `arglists-str` assuming an outer-paren format that current CIDER no longer sends, which had broken even the existing reorder/rename against a live REPL.
 - `cljr-clean-ns` degrades gracefully without a REPL. When the `clean-ns` middleware op isn't available it falls back to sorting the ns form with clojure-mode's `clojure-sort-ns` instead of erroring - pruning unused libspecs still needs the middleware and is skipped. Auto-sort after ns changes (`cljr-auto-sort-ns`) now works offline too.
 - Fix `cljr-slash` and `cljr-add-missing-libspec` erroring with "No linked CIDER sessions" when used without a connected REPL, instead of falling back to their offline behavior. `cljr--op-supported-p` now returns nil when disconnected rather than signaling.

clj-refactor.el

@@ -4085,19 +4085,22 @@ own element, matching how the rest of the machinery treats it."
                             (unless (string= inner "")
                               (split-string inner " " t)))))))
 
-(defun cljr--get-function-params (fn)
-  "Retrieve the parameters for FN.
-
-Only single-arity functions are supported; multi-arity functions signal
-an error."
+(defun cljr--get-function-arities (fn)
+  "Retrieve FN's parameter lists, one per arity."
   (let* ((info (cljr--var-info fn))
          (arglists-str (nrepl-dict-get info "arglists-str")))
     (unless arglists-str
       (error "Couldn't retrieve the parameter list for %s" fn))
-    (let ((arities (cljr--parse-arglists arglists-str)))
-      (when (> (length arities) 1)
-        (error "Can't do work on functions of multiple arities"))
-      (car arities))))
+    (cljr--parse-arglists arglists-str)))
+
+(defun cljr--choose-arity (arities)
+  "Return the parameter list to edit from ARITIES.
+Prompts when the function has more than one arity."
+  (if (= (length arities) 1)
+      (car arities)
+    (let* ((labels (seq-map (lambda (a) (format "[%s]" (string-join a " "))) arities))
+           (choice (completing-read "Change which arity? " labels nil t)))
+      (nth (seq-position labels choice) arities))))
 
 (defvar cljr--change-signature-mode-map
   (let ((keymap (make-sparse-keymap)))
@@ -4429,25 +4432,65 @@ parameters (by name), and drops removed ones."
                                   params)
       (cljr--maybe-wrap-form))))
 
-(defun cljr--goto-lambda-list ()
-  "Move into the lambda list of the function definition beginning at point.
+(defun cljr--count-lambda-list-params ()
+  "Return the number of parameters in the lambda list at point.
+
+Point must be at the opening bracket.  Schema type annotations (`:- T')
+are skipped, and a `&' rest marker counts as a parameter, so the count
+lines up with `cljr--parse-arglists'."
+  (save-excursion
+    (let ((end (cljr--point-after 'paredit-forward))
+          (count 0))
+      (paredit-forward-down)
+      (cljr--skip-past-whitespace-and-comments)
+      (while (< (point) (1- end))
+        (cljr--forward-parameter)
+        (setq count (1+ count)))
+      count)))
 
-E.g. move point from here:  |(defn foo [bar baz] ...)
+(defun cljr--goto-arity-lambda-list (arg-count)
+  "Move into the lambda list of the arity that has ARG-COUNT parameters.
+
+Point is assumed to be just prior to the function definition.  Handles
+both single-arity defns (a bare lambda list) and multi-arity defns (each
+arity wrapped in a list).  Signals an error if no matching arity exists.
+
+E.g. with ARG-COUNT 2, move point from here:  |(defn foo [bar baz] ...)
 to here:  (defn foo [|bar baz] ...)"
   (paredit-forward-down)
-  (cljr--skip-past-whitespace-and-comments)
-  (while (not (looking-at-p "\\["))
-    (paredit-forward)
-    (cljr--skip-past-whitespace-and-comments))
-  (paredit-forward-down))
+  (let ((defn-end (save-excursion (paredit-forward-up) (1- (point))))
+        (target nil))
+    (cljr--skip-past-whitespace-and-comments)
+    (while (and (not target) (< (point) defn-end))
+      (cond
+       ;; single-arity: a bare lambda list
+       ((looking-at-p "\\[")
+        (when (= (cljr--count-lambda-list-params) arg-count)
+          (setq target (point))))
+       ;; multi-arity: a clause like ([params] body ...)
+       ((looking-at-p "(")
+        (setq target (save-excursion
+                       (paredit-forward-down)
+                       (cljr--skip-past-whitespace-and-comments)
+                       (when (and (looking-at-p "\\[")
+                                  (= (cljr--count-lambda-list-params) arg-count))
+                         (point))))))
+      (unless target
+        (paredit-forward)
+        (cljr--skip-past-whitespace-and-comments)))
+    (unless target
+      (error "Couldn't find an arity with %d parameter(s)" arg-count))
+    (goto-char target)
+    (paredit-forward-down)))
 
 (defun cljr--update-function-signature (signature-changes)
   "Point is assumed to be just prior to the function definition we're about to update."
-  (cljr--goto-lambda-list)
-  (cljr--update-signature-names signature-changes)
-  (cljr--goto-toplevel)
-  (cljr--goto-lambda-list)
-  (cljr--update-signature-order signature-changes))
+  (let ((arg-count (cljr--old-arity signature-changes)))
+    (cljr--goto-arity-lambda-list arg-count)
+    (cljr--update-signature-names signature-changes)
+    (cljr--goto-toplevel)
+    (cljr--goto-arity-lambda-list arg-count)
+    (cljr--update-signature-order signature-changes)))
 
 (defun cljr--call-site-p (fn)
   "Is point at a call-site for FN?"
@@ -4457,6 +4500,25 @@ to here:  (defn foo [|bar baz] ...)"
       (paredit-forward-down)
       (string-suffix-p (cljr--symbol-suffix fn) (cider-symbol-at-point)))))
 
+(defun cljr--call-site-arg-count ()
+  "Return the number of arguments at the call site.
+Point is assumed to be at the function name being called.
+
+Counts sexps, so `#_'-ignored forms and `#?'-reader conditionals in the
+argument list are miscounted; such a call may not match its arity and be
+left unchanged."
+  (save-excursion
+    (paredit-backward-up)
+    (let ((end (cljr--point-after 'paredit-forward))
+          ;; start at -1 so the function name itself isn't counted
+          (count -1))
+      (paredit-forward-down)
+      (cljr--skip-past-whitespace-and-comments)
+      (while (< (point) (1- end))
+        (cljr--forward-parameter)
+        (setq count (1+ count)))
+      count)))
+
 (defun cljr--no-changes-to-parameter-order-p (signature-changes)
   (seq-every-p (lambda (e) (= (gethash :new-index e) (gethash :old-index e)))
                signature-changes))
@@ -4593,16 +4655,24 @@ Point is assumed to be at the function being called."
             (move-to-column (1- col-beg))
             (cond
              ((cljr--ignorable-occurrence-p) :do-nothing)
-             ;; Direct call site.  Removing a parameter is destructive - an
-             ;; argument might have side effects or be needed elsewhere - so
-             ;; never auto-delete it; route the call site to manual review.
-             ;; Likewise for variadic functions, whose call sites can't be
-             ;; rewritten positionally (the arg count varies).
+             ;; Direct call site.
              ((cljr--call-site-p name)
-              (if (or (cljr--signature-has-remove-p signature-changes)
-                      (cljr--signature-variadic-p signature-changes))
-                  (cljr--append-to-manual-intervention-buffer)
-                (cljr--update-call-site signature-changes)))
+              (cond
+               ;; Variadic functions can't be rewritten positionally (their
+               ;; call-site arg count varies), so leave them for review.
+               ((cljr--signature-variadic-p signature-changes)
+                (cljr--append-to-manual-intervention-buffer))
+               ;; A call to a different arity than the one being changed - the
+               ;; arg count doesn't match - is correct as is, so leave it.
+               ((/= (cljr--call-site-arg-count)
+                    (cljr--old-arity signature-changes))
+                :do-nothing)
+               ;; Removing a parameter is destructive - an argument might have
+               ;; side effects or be needed elsewhere - so never auto-delete
+               ;; it; route the call site to manual review.
+               ((cljr--signature-has-remove-p signature-changes)
+                (cljr--append-to-manual-intervention-buffer))
+               (t (cljr--update-call-site signature-changes))))
              ;; `apply'/`partial' sites can only be handled for pure reorders;
              ;; an added or removed arg would land in the wrong spot, so bail.
              ((cljr--partial-call-site-p)
@@ -4661,12 +4731,16 @@ Point is assumed to be at the function being called."
 (defun cljr-change-function-signature ()
   "Change the function signature of the function at point.
 
+For a multi-arity function you're asked which arity to change; only that
+arity's definition and the call sites with a matching number of
+arguments are updated.
+
 See: https://github.com/clojure-emacs/clj-refactor.el/wiki/cljr-change-function-signature"
   (interactive)
   (cljr--ensure-op-supported "find-symbol")
   (when (cljr--asts-y-or-n-p)
     (let* ((fn (cider-symbol-at-point))
-           (params (cljr--get-function-params fn))
+           (params (cljr--choose-arity (cljr--get-function-arities fn)))
            (var-info (cljr--var-info fn))
            (ns (nrepl-dict-get var-info "ns")))
       (setq cljr--occurrences (cljr--find-symbol-sync fn ns))

doc/design/change-function-signature.md

@@ -1,8 +1,18 @@
 # Design: extending `cljr-change-function-signature`
 
-Status: **P1 implemented** (single-arity add/remove). P2 (multi-arity) still
-proposed. Captures the plan for making `cljr-change-function-signature` add and
-remove parameters, and handle multi-arity functions.
+Status: **P1 and P2 implemented.** `cljr-change-function-signature` now adds and
+removes parameters and handles multi-arity functions. Captures the original plan.
+
+P2 (multi-arity) took the "one selected arity per run" route from the open
+questions below: for a multi-arity function the command prompts for which arity
+to change, then only that arity's definition lambda list and the call sites whose
+argument count matches are updated (via `cljr--goto-arity-lambda-list` and
+call-site arg-count matching in the classifier). Call sites of other arities are
+left untouched; variadic and `apply`/`partial` sites still route to manual. To
+change several arities, run the command once per arity. Validated live against a
+real refactor-nrepl connection (reorder of a chosen arity: the matching lambda
+list and all matching-arity call sites, including an internal recursive call,
+changed; the other arity and its call sites did not).
 
 P1 shipped with the tagged `signature-changes` model (`:keep`/`:add`/`:remove`),
 the edit-buffer keys `a` (add) and `k`/`d` (mark for removal), placeholder

features/step-definitions/clj-refactor-steps.el

@@ -436,6 +436,17 @@
          (cljr--change-function-signature (list (cljr--plist-to-hash (cl-second cljr--test-occurrences)))
                                           cljr--remove-baz)))
 
+(Given "I call the cljr--change-function-signature function directly with mockdata to swap foo and bar across mixed-arity call-sites"
+       (lambda ()
+         ;; Two calls: a 1-arg one (a different arity, must be left alone) and
+         ;; a 3-arg one (the arity being changed, must be swapped).
+         (cljr--change-function-signature
+          (list (cljr--plist-to-hash '(:line-beg 4 :line-end 4 :col-beg 5 :col-end 7
+                                       :name "core/tt" :file "core.clj" :match "(tt 1)"))
+                (cljr--plist-to-hash '(:line-beg 4 :line-end 4 :col-beg 12 :col-end 14
+                                       :name "core/tt" :file "core.clj" :match "(tt 1 2 3)")))
+          cljr--foo-bar-swapped)))
+
 (Given "I call the cljr--change-function-signature function directly with mockdata to swap foo and bar in a call-site on a defn line"
        (lambda ()
          ;; A call that happens to sit on a `(defn ...)' line must be treated

features/zzz-run-last-change-function-signature.feature

@@ -248,3 +248,20 @@ Feature: Change function signature
 
     (defn wrapper [x] (tt 2 x 3))
     """
+
+  Scenario: Only call-sites with a matching arity are changed
+    When I insert:
+    """
+    (ns core)
+
+    (defn caller []
+      [(tt 1) (tt 1 2 3)])
+    """
+    And I call the cljr--change-function-signature function directly with mockdata to swap foo and bar across mixed-arity call-sites
+    Then I should see:
+    """
+    (ns core)
+
+    (defn caller []
+      [(tt 1) (tt 2 1 3)])
+    """

tests/unit-test.el

@@ -813,3 +813,62 @@ str/"))))
     (expect (cljr--signature-variadic-p
              (list (cljr--test-keep 0 0 "a") (cljr--test-keep 1 1 "b")))
             :to-be nil)))
+
+(describe "cljr--count-lambda-list-params"
+  (it "counts params in a lambda list"
+    (cljr--with-clojure-temp-file "foo.clj"
+      (insert "[a b c]")
+      (goto-char (point-min))
+      (expect (cljr--count-lambda-list-params) :to-equal 3)))
+  (it "counts the rest marker as a param"
+    (cljr--with-clojure-temp-file "foo.clj"
+      (insert "[a & more]")
+      (goto-char (point-min))
+      (expect (cljr--count-lambda-list-params) :to-equal 3)))
+  (it "skips schema type annotations"
+    (cljr--with-clojure-temp-file "foo.clj"
+      (insert "[a :- s/Str b :- s/Int]")
+      (goto-char (point-min))
+      (expect (cljr--count-lambda-list-params) :to-equal 2))))
+
+(describe "cljr--call-site-arg-count"
+  (it "counts arguments at a call site"
+    (cljr--with-clojure-temp-file "foo.clj"
+      (insert "(foo 1 2 3)")
+      (goto-char (point-min))
+      (forward-char 1)
+      (expect (cljr--call-site-arg-count) :to-equal 3)))
+  (it "returns 0 for a no-arg call"
+    (cljr--with-clojure-temp-file "foo.clj"
+      (insert "(foo)")
+      (goto-char (point-min))
+      (forward-char 1)
+      (expect (cljr--call-site-arg-count) :to-equal 0))))
+
+(describe "cljr--choose-arity"
+  (it "returns the sole arity without prompting"
+    (expect (cljr--choose-arity '(("a" "b"))) :to-equal '("a" "b")))
+  (it "prompts and returns the chosen arity when there are several"
+    (spy-on 'completing-read :and-return-value "[x y]")
+    (expect (cljr--choose-arity '(("x") ("x" "y"))) :to-equal '("x" "y"))))
+
+(describe "cljr--update-function-signature (multi-arity)"
+  (it "reorders only the chosen arity's lambda list"
+    (cljr--with-clojure-temp-file "foo.clj"
+      (insert "(defn multi\n  ([x] (multi x 0))\n  ([x y] (+ x y)))")
+      (goto-char (point-min))
+      (cljr--update-function-signature
+       (list (cljr--test-keep 0 1 "x")
+             (cljr--test-keep 1 0 "y")))
+      (expect (buffer-string) :to-equal
+              "(defn multi\n  ([x] (multi x 0))\n  ([y x] (+ x y)))")))
+  (it "adds a parameter to the chosen arity only"
+    (cljr--with-clojure-temp-file "foo.clj"
+      (insert "(defn multi\n  ([x] x)\n  ([x y] (+ x y)))")
+      (goto-char (point-min))
+      (cljr--update-function-signature
+       (list (cljr--test-keep 0 0 "x")
+             (cljr--test-keep 1 1 "y")
+             (cljr--test-add 2 "z")))
+      (expect (buffer-string) :to-equal
+              "(defn multi\n  ([x] x)\n  ([x y z] (+ x y)))"))))