Threading macro refactoring

[bo-tato]

I include a brief description of what threading macros are in case anyone hasn't seen them, as they aren't standard in common lisp, but are popular in clojure, and pipe operator which is the same thing is popular in other langs.
Threading macros let you write a series of functions reading from top to bottom instead of inside out. For example instead of:

(print (remove-if #'evenp (mapcar #'1+ '(1 2 3 4 5))))

you can write:

(->> '(1 2 3 4 5)
     (mapcar #'1+)
     (remove-if #'evenp)
     print)

->> substitutes each value as the last argument of each form in turn, while -> substitutes it as the first argument, so:

(uiop:read-file-lines (merge-pathnames (replace "foobar" "baz" :start1 3) "file.txt"))

(-> "foobar"
    (replace "baz" :start1 3)
    (merge-pathnames "file.txt")
    (uiop:read-file-lines))

This pull adds a series of refactoring commands that will automatically rewrite code into thread-first or thread-last style, or unwind a thread macro into "normal" code. It is 95% copied from clojure-mode which in turn got it from clj-refactor (as I note in authors section of the code), which are also like sly licensed in GPL so it shouldn't be a problem. This introduces 5 new interactive functions:
sly-unwind: unwind threading macro once, ie turn (-> a b c) into (-> (b a) c)
sly-unwind-all: fully unwind, ie turn (-> a b c) into (c (b a))
sly-thread-first-all: turn (+ (- (* (/ 1) 2) 3) 4) into:

(-> 1
    /
    (* 2)
    (- 3)
    (+ 4))

sly-thread-last-all: turn (print (remove-if #'evenp (mapcar #'1+ '(1 2 3 4 5)))) into:

(->> '(1 2 3 4 5)
     (mapcar #'1+)
     (remove-if #'evenp)
     print)

and sly-thread: which increases the threading by one, turning:

(->> (mapcar #'1+ input)
     (remove-if #'evenp)
     print)

into:

(->> input
     (mapcar #'1+)
     (remove-if #'evenp)
     print)

and introduces one new variable: sly-threading-macro which defaults to "->" or can be set to "~>" which is used by some CL threading macro libraries.
In the future more refactoring commands could be copied from clj-refactor (applied to CL), or original ones added.
I'm new at lisp and emacs, and never really done professional programming, this was my first time writing unit tests. so any feedback is welcome on how to make it high quality and mergeable

[joaotavora]

I'll have a better look at this later, but shouldn't be called sly-threading.el? I think "refactor" is too broad.

Some more questions:

• have you considered a third party contrib based on the sly-hello-world template?
• How do the threaded function-like pseudo-calls play with autodoc?

[bo-tato]

I'll have a better look at this later, but shouldn't be called sly-threading.el? I think "refactor" is too broad.

You're right threading is a more appropriate name, but I was feeling ambitious and thinking of copying more refactoring commands from clj-refactor or adding new ones, they share a lot of the same utility functions so it'd make since to put them all together. But I'll probably never get around to it as these thread and unwind, along with import symbol at point which sly already has, are probably the only refactor commands I'll use. So yea I should probably just call it sly-threading for now, and if at some point I add more refactoring commands that share common utility functions with this then make it a broader sly-refactoring.

have you considered a third party contrib based on the sly-hello-world template?

Nice, I didn't know about it. I just realized a couple more minor issues (that are also present in the clojure original):

• if you unwind on (-> f x) it turns into (-> (f x)), and then unwind again to get (f x), it should detect that (-> (f x)) is already fully unwound and remove the threading macro at that step
• if you have:

(-> f x)
(print 1)

and the cursor is on the open parenthesis of the print form, and you do unwind, it unwinds the preceding form with the threading macro when it should just do nothing
So yea it probably makes sense to just have it as an external contrib, and once I've used it for a while and run into any issues then consider bringing it into sly.

How do the threaded function-like pseudo-calls play with autodoc?

With autodoc the thread-first macro messes it up a little:

It doesn't know that the threading macro will insert an argument in the first position, so it bolds one argument before the one we're really on. I think it's kind of expected autodoc won't always work perfectly when macros can rewrite code, for example yesterday I was using lquery and within the lquery:$ macro autodoc was showing the documentation for the standard library map function although the map keyword means something different within the lquery:$ macro. Still it'd be fairly straightforward to add a check to autodoc, to look if the enclosing form is a thread first macro, and if so shift the argument position it bolds by one. What could get complicated though is there's no "standard" threading macro for common lisp, some libraries use "->" and some use "~>" but serapeum also uses "->" to mean declaim type. So how do we know if a "->" in the enclosing form is really a threading macro or it's something else? I guess it'd just be to use the sly-threading-macro config variable introduced with this. Do think this should also patch autodoc to understand threading macros?

[monnier]

With autodoc the thread-first macro messes it up a little:  It doesn't know that the threading macro will insert an argument in the first position, so it bolds one argument before the one we're really on. I think it's kind of expected autodoc won't always work perfectly when macros can rewrite code, for example yesterday I was using lquery and within

It would be neat if it worked by macro-expanding the code first :-) In ELisp I tried to do that for the completion of local variables, see `elisp--local-variables`. It's hackish (and dangerous), tho, so it doesn't really address the "won't always work perfectly".

[bo-tato]

Interesting, how would that work? How would you find where the current point(cursor) location corresponds to in the macroexpanded version?

[monnier]

Interesting, how would that work? How would you find where the current point(cursor) location corresponds to in the macroexpanded version?

As mentioned, you can check how I did it: `C-h f RET elisp--local-variables RET` and then follow the link to its source code. Basically I take the string between the start of the current sexp and point, add "elisp--witness--lisp" and the right number of closing parens, and then macro-expand the corresponding sexp and look for the "elisp--witness--lisp" symbol in the result. It can fail in a wonderful variety of ways, of course. Stefan

[joaotavora]

Interesting, how would that work?

As Stefan implied with macroexpand, in part by running "user code", i.e. by executing arbitrary user code that is under analysis. This strategy isn't new, and it's used by Elisp's flymake backend or in some select situations by SLY and SLIME too. But there's a conceptual difference between doing this macroexpansion/execution when the user specifically asks to and doing it in the background, while the user is innocently navigating code.

I've once deleted a full directory in my $HOME just because I had a miswritten macro with a delete call and I was using Elisp flymake. I hadn't even saved the file.

It can fail in a wonderful variety of ways, of course.

Hi @monnier In fact I think it's failing right now while editing lisp/progmodes/eglot.el :-)

Debugger entered--Lisp error: (wrong-number-of-arguments (2 . 2) 1)
  #f(compiled-function (arg1 arg2 &rest rest) "Destructure OBJECT, binding VARS in BODY.\nVARS is ([(INTERFACE)] SYMS...)\nHonor `eglot-strict-mode'." #<bytecode 0x1e46fc1e69c1011c>)(((TypeHierarchyItem) name elisp--witness--lisp))
  apply(#f(compiled-function (arg1 arg2 &rest rest) "Destructure OBJECT, binding VARS in BODY.\nVARS is ([(INTERFACE)] SYMS...)\nHonor `eglot-strict-mode'." #<bytecode 0x1e46fc1e69c1011c>) ((TypeHierarchyItem) name elisp--witness--lisp))
  macroexpand-1((eglot--dbind ((TypeHierarchyItem) name elisp--witness--lisp)) nil)
  macroexp-macroexpand((eglot--dbind ((TypeHierarchyItem) name elisp--witness--lisp)) nil)
  macroexp--expand-all((eglot--dbind ((TypeHierarchyItem) name elisp--witness--lisp)))
  macroexp--all-forms((lambda (n1 n2) (eglot--dbind ((TypeHierarchyItem) name elisp--witness--lisp))) 2)
  macroexp--expand-all((cl-function (lambda (n1 n2) (eglot--dbind ((TypeHierarchyItem) name elisp--witness--lisp)))))
  macroexp--all-forms((--cl-node-equal-- (cl-function (lambda (n1 n2) (eglot--dbind ((TypeHierarchyItem) name elisp--witness--lisp))))) 1)
  macroexp--all-clauses(((--cl-node-equal-- (cl-function (lambda (n1 n2) (eglot--dbind ((TypeHierarchyItem) name elisp--witness--lisp)))))) 1)
  macroexp--expand-all((cl-flet ((node-equal (n1 n2) (eglot--dbind ((TypeHierarchyItem) name elisp--witness--lisp))))))
  macroexp--all-forms(((cl-flet ((node-equal (n1 n2) (eglot--dbind ((TypeHierarchyItem) name elisp--witness--lisp)))))))
  macroexp--expand-all((let* ((srv (eglot-current-server)) (hierarchy (hierarchy-new)) (roots (jsonrpc-request srv preparer (eglot--TextDocumentPositionParams)))) (cl-flet ((node-equal (n1 n2) (eglot--dbind ((TypeHierarchyItem) name elisp--witness--lisp)))))))
  macroexp--all-forms((lambda (provider preparer methods) (eglot--server-capable-or-lose provider) (let* ((srv (eglot-current-server)) (hierarchy (hierarchy-new)) (roots (jsonrpc-request srv preparer (eglot--TextDocumentPositionParams)))) (cl-flet ((node-equal (n1 n2) (eglot--dbind (... name elisp--witness--lisp))))))) 2)
  macroexp--expand-all(#'(lambda (provider preparer methods) (eglot--server-capable-or-lose provider) (let* ((srv (eglot-current-server)) (hierarchy (hierarchy-new)) (roots (jsonrpc-request srv preparer (eglot--TextDocumentPositionParams)))) (cl-flet ((node-equal (n1 n2) (eglot--dbind ...)))))))
  macroexp--all-forms((defalias 'eglot--hierarchy-helper #'(lambda (provider preparer methods) (eglot--server-capable-or-lose provider) (let* ((srv (eglot-current-server)) (hierarchy (hierarchy-new)) (roots (jsonrpc-request srv preparer ...))) (cl-flet ((node-equal ... ...)))))) 1)
  #f(compiled-function (form func) #<bytecode -0xa784b5aded04feb>)(((defalias 'eglot--hierarchy-helper #'(lambda (provider preparer methods) (eglot--server-capable-or-lose provider) (let* ((srv ...) (hierarchy ...) (roots ...)) (cl-flet (...)))))) defalias)
  macroexp--expand-all((defun eglot--hierarchy-helper (provider preparer methods) (eglot--server-capable-or-lose provider) (let* ((srv (eglot-current-server)) (hierarchy (hierarchy-new)) (roots (jsonrpc-request srv preparer (eglot--TextDocumentPositionParams)))) (cl-flet ((node-equal (n1 n2) (eglot--dbind (... name elisp--witness--lisp))))))))
  macroexpand-all((defun eglot--hierarchy-helper (provider preparer methods) (eglot--server-capable-or-lose provider) (let* ((srv (eglot-current-server)) (hierarchy (hierarchy-new)) (roots (jsonrpc-request srv preparer (eglot--TextDocumentPositionParams)))) (cl-flet ((node-equal (n1 n2) (eglot--dbind (... name elisp--witness--lisp))))))))
  elisp--local-variables()

[monnier]

by SLY and SLIME too. But there's a conceptual difference between doing this macroexpansion/execution when the user specifically asks to and doing it in the background, while the user is innocently navigating code.

There's a difference in degree, maybe, but in both cases it's indeed quite dangerous :-(

I've once deleted a full directory in my $HOME just because I had a miswritten macro with a `delete` call and I was using Elisp flymake. I hadn't even saved the file.

Yup, we really need some way to confine the execution of the macros, and there's currently no way to do that :-(

> It can fail in a wonderful variety of ways, of course. Hi @monnier In fact I think it's failing right now while editing `lisp/progmodes/eglot.el` :-) ```elisp Debugger entered--Lisp error: (wrong-number-of-arguments (2 . 2) 1) #f(compiled-function (arg1 arg2 &rest rest) "Destructure OBJECT, binding

Hmm... looks like I pushed an incomplete patch, indeed. Should be fixed now. Stefan