Add refactoring command for converting #() shorthand

[yuhan0]

M-x clojure-convert-shorthand-fn turns the closest #() before point into a regular (fn ...) form. If there are any % arguments it prompts interactively for the arg names.
Useful for fixing nested #() forms and general refactoring for readability. (I believe the reverse refactoring is much less necessary in practice)

Keybindings:

Not implemented yet, but I think "convert function" ( prefix + cf / fc) makes sense in the clj-refactor package, as well as here in the default clojure-refactor-map as the single key bindings # f and c are all taken.

---

Before submitting a PR mark the checkboxes for the items you've done (if you
think a checkbox does not apply, then leave it unchecked):

• The commits are consistent with our contribution guidelines.
• You've added tests (if possible) to cover your change(s). Bugfix, indentation, and font-lock tests are extremely important!
• You've run M-x checkdoc and fixed any warnings in the code you've written.
• You've updated the changelog (if adding/changing user-visible functionality).
• You've updated the readme (if adding/changing user-visible functionality).

Thanks!

[bbatsov]

Sorry for forgetting about this PR! The code looks great and we only need to decide on a keybinding (where I'm fine with your suggestions) and add this to the menu, so it's easier to discover.

I agree that a conversion in the other direction doesn't make as much sense, although we can add it for the sake of uniformity (this will also allow us to have some "toggle" command that alternates between the two). That's totally optional, though.

[magnars]

We have this feature already tho, it's cljr-promote-function :)

[yuhan0]

Oh! That's embarrassing... I should familiarize myself more with the cljr refactorings, unfortunately the ones requiring full project AST analysis don't work on our codebase.

I guess one advantage of this PR's command is that it's entirely textual and doesn't require a Cider REPL, whereas the cljr-promote-function always requires full AST evaluation(?) due to the fn -> defn feature.

https://github.com/clojure-emacs/clj-refactor.el/wiki/cljr-promote-function
https://github.com/clojure-emacs/clj-refactor.el/blob/7bec40518af94893d7067ec71d774d1a7346e2d8/clj-refactor.el#L2501

It also doesn't appear to support %& syntax and throws an error if the #() form is at top level (unlikely in actual code but still the first bug I encountered trying out the command)

There was a push to move all non-repl commands in clj-refactor to the clojure-mode repo, perhaps we could rename my code to clojure-promote-function-literal and reorganize things in cljr so the #() case delegates to it?

[bbatsov]

I like this idea. I guess cljr can use the function from clojure-mode in the absence of a REPL connection - win-win for the end users.

It also doesn't appear to support %& syntax and throws an error if the #() form is at top level (unlikely in actual code but still the first bug I encountered trying out the command)

Please, report those issues to the clj-refactor repo. We'll have to address them at some point.

[yuhan0]

Ok, I've renamed the functions ("promote" does convey the intention better than "convert") and bound it to "C-c C-r P" ("p" was taken by clojure-cycle-privacy).

Will submit a PR to clj-refactor repo if this gets merged :)

[vemv]

whereas the cljr-promote-function always requires full AST evaluation

No, not at all. https://github.com/clojure-emacs/clj-refactor.el/wiki marks with * the ops requiring a repl

Accordingly I'd try to move this impl to clj-refactor entirely - sounds simpler to only maintain one impl.

unfortunately the ones requiring full project AST analysis don't work on our codebase.

would appreciate a report, we keep improving things in this front

(the only thing that hasn't seen an immediate fix is performance - scales linearly with codebase size. Think 8m for 100KLOC)

Cheers - V

[vemv]

btw, now that rewrite-clj is so advanced (and cross-platform) it would make sense to eventually implement this feature server-side.

Else it's pretty plausible that the impl will always have bugs - who really wants to implement a full-blown Clojure refactoring engine in elisp? 😀

The good thing is that rewrite-clj is embedded in both refactor-nrepl and clj-kondo/lsp so this approach accomodates all kinds of preferences.

[bbatsov]

Else it's pretty plausible that the impl will always have bugs - who really wants to implement a full-blown Clojure refactoring engine in elisp? 😀

The point is that many people don't want to install several packages, so it's a good idea to have some simple commands that mostly get the job done in clojure-mode. Some people value power, other value a simple setup. We aim to cater to the needs of both groups.

[vemv]

I'd be comfortable with moving the impl from clj-refactor to clojure-mode + merging impls / discarding older bits.

Maintaining two impls (presumably each with its own set of small bugs) seems a bit disproportionate.

[bbatsov]

I'd be comfortable with moving the impl from clj-refactor to clojure-mode + merging impls / discarding older bits.

That'd be fine by me. I agree that having 2 implementations that don't rely on any server-side code is an overkill.

[yuhan0]

@vemv

No, not at all. https://github.com/clojure-emacs/clj-refactor.el/wiki marks with * the ops requiring a repl

Perhaps the wiki is outdated? The function body is clearly wrapped in a (when (cljr--asts-y-or-n-p)) block, and I can't execute it without a repl:
https://github.com/clojure-emacs/clj-refactor.el/blob/7bec40518af94893d7067ec71d774d1a7346e2d8/clj-refactor.el#L2501

Although I got around it by invoking the private (cljr--promote-function-literal) with M-x eval-expression.

There's surely lots of buggy regex-based clojure parsing and code duplication going on in both codebases, but as @bbatsov pointed out, it's good enough for a lot of usecases.

Another case in point - clj-refactor's use of multiple-cursors and interactive overlays often interact in unpredictable ways with Evil and/or undo-fu, corrupting the undo history. I ran into this many times while testing out cljr-promote-function, where interrupting the command would not record the edit and create confusing results on undo.

That's not necessarily the fault of either package, but just one of my reasons to favour less "sophisticated" impls.

I agree that delegating all refactoring ops to rewrite-clj would be the most elegant solution but that would itself be quite a significant refactoring(heh).

[vemv]

Perhaps the wiki is outdated?

Updated, thanks! I see, it's because it calls cljr--call-middleware-to-find-used-locals. It uses https://github.com/clojure-emacs/refactor-nrepl/blob/master/src/refactor_nrepl/find/find_locals.clj which while uses tools.analyzer:

• doesn't need the whole project to be analysed
  • only one ns, i.e. fast and much less likely to fail
• said ns analysis is subject to caching, as usual

So I'd expect a sub-second (but real) latency associated to tools.analyzer. Of course, one is trading it for something, namely accuracy.

Another case in point - clj-refactor's use of multiple-cursors and interactive overlays often interact in unpredictable ways with Evil and/or undo-fu

There's defcustom cljr-use-multiple-cursors. Similarly we could trivially implement a defcustom for overlays.

I think it would add objectivity to the discussion that you tried to use clj-refactor's impl and created an issue for every observed nuance.

My thinking is that simply:

• we shouldn't NIH each other across projects
• within reason, we should try to emphasize the CIDER way
  • i.e. runtime-powered things that are smart/accurate

I think this still leaves the room open for #601 (comment) . The 'locals' could be an argument. It would default to nothing when using bare clojure-mode, and to something when wrapping it via clj-refactor.el.

That way we still have a single impl, while accomodating different approaches/tradeoffs.

[vemv]

btw the 'locals' could also be provided by even more impls, e.g. clj-kondo. So one has 3 approaches for the price of one

[yuhan0]

Perhaps there is some misunderstanding here - cljr-promote-function is in fact two different refactorings, dispatched on context

• #() -> fn
• fn -> defn

This PR only implements the first case, "promote-fn-literal" and does not consider the second case. It's only the latter which requires locals analysis and runtime powered smarts, while the former case even in clj-refactor impl is a purely textual transformation (being after all a read-time macro).

My proposal was something like (pseudocode):

;; replace current impl
- (defun cljr--promote-function-literal 
-   ... )

(defun cljr-promote-function ()
  (if (looking-at "#(")
+   (clojure-promote-fn-literal)  ; <- this PR's impl, not REPL gated

    (cljr--ensure-op-supported "find-used-locals")
    (when (cljr--asts-y-or-n-p)
      (cljr--promote-function)))  ; <- retain the fn->defn runtime supported logic

I'm sorry if this PR comes across as NIH, I was unaware of the existing impl and wrote one from scratch. After the fact I found several edge cases with the cljr impl as reported in clojure-emacs/clj-refactor.el#505, which are correctly handled by my code.

[bbatsov]

I'm sorry if this PR comes across as NIH, I was unaware of the existing impl and wrote one from scratch. After the fact I found several edge cases with the cljr impl as reported in clojure-emacs/clj-refactor.el#505, which are correctly handled by my code.

Don't worry at all! We can really be talking about NIH, as for me all the projects here are just different parts of one project. :-) In light of everything so far, I still feel it's a good idea to include this in clojure-mode, so that more people can make use of it. We have to remember that many people don't use CIDER and by extension cljr at all (e.g. some people use inf-clojure, some rely solely on clojure-lsp and don't use a REPL at all).

In this case it also seems that cljr benefits from the proposal as well, so it's a win-win in my book. We just have to document this well for posterity's sake.

[bbatsov]

Don't forget to add a menu entry as well.

[bbatsov]

We also need a changelog entry.

[yuhan0]

Rebased on master and added changes

[bbatsov]

Thanks!

[vemv]

Released as 5.14.0