Clojure formatting problems with `srefactor-lisp-format-buffer`

[tsdh]

Hi @tuhdo,

because of your posts on the Clojure and Emacs mailinglists accompanied by the nice demo gifs, I had to give srefactor-lisp-format-buffer a try (version 20150408.2109 from MELPA). While it seems to be really good for initially formatting a completely unformatted buffer, it introduces several formatting issues when being applied to a buffer which is already formatted perfectly according to clojure-mode, i.e., indent-region with the whole buffer being marked wouldn't change anything.

Here's are some diff hunks of stuff which I consider wrong or at least suboptimal:

 (ns funnyqt.in-place
-  "In-place transformation stuff."
-  (:require [clojure.tools.macro   :as m]
-            [funnyqt.generic       :as g]
-            [funnyqt.visualization :as viz]
-            [funnyqt.utils         :as u]
-            [funnyqt.query         :as q]
-            [funnyqt.pmatch        :as pm]
-            [funnyqt.tg            :as tg])
-  (:import
-   (javax.swing JDialog JButton AbstractAction WindowConstants BoxLayout
-                JPanel JLabel JScrollPane JComboBox Action JCheckBox
-                Box BorderFactory ImageIcon JComponent)
-   (java.awt.event ActionEvent ItemEvent ItemListener)
-   (java.awt Color GridLayout GridBagLayout GridBagConstraints)))
+    "In-place transformation stuff."
+    (:require [clojure.tools.macro :as m]
+         [funnyqt.generic :as g]
+         [funnyqt.visualization :as viz]
+         [funnyqt.utils :as u]
+         [funnyqt.query :as q]
+         [funnyqt.pmatch :as pm]
+         [funnyqt.tg :as tg])
+    (:import (javax.swing JDialog JButton AbstractAction WindowConstants BoxLayout JPanel JLabel JScrollPane JComboBox Action JCheckBox Box BorderFactory ImageIcon JComponent)
+        (java.awt.event ActionEvent ItemEvent ItemListener)
+        (java.awt Color GridLayout GridBagLayout GridBagConstraints)))

1. Before, the :as clauses lined up nicely, now they don't anymore.
2. The :require vectors also used to line up and don't anymore.
3. Before, the imported classes wrapped at col 80, now they are all on one long line.

 (def ^{:dynamic true
-       :doc "Only for internal use.  See `as-pattern' macro."}
-  *as-pattern* false)
+     :doc "Only for internal use.  See `as-pattern' macro."
+     } *as-pattern* false)

Putting the closing } of a map onto a new line seems as unidiomatic to me as putting closing parens on separate lines.

-(defmacro as-pattern
-  "Performs the given rule application `rule-app` as a pattern.
+(defmacro as-pattern "Performs the given rule application `rule-app` as a pattern.

A macro's docstring should be on a separate line IMO.

   `(binding [*as-pattern* true]
-     ~rule-app))
+       ~rule-app))

binding should have the same indentation of its body as let. This might be caused by srefactor-lisp-format-buffer introducing TABs whereas clojure-mode sets indent-tabs-mode buffer-locally to nil.

 (defn ^:private unrecur
   "Replaces (recur ...) forms with (fnname ...) forms where *as-test* is bound to false.
   Existing (fnname ...) forms are also wrapped by bindings of *as-test* to
   false.  Doesn't replace in nested `loop` or `fn` forms."
   [fnname form]
-  (u/prewalk (fn [el]
-               (if (and (seq? el)
-                        (or (= (first el) 'recur)
-                            (= (first el) fnname)))
-                 `(binding [*as-test* false]
-                    (~fnname ~@(next el)))
-                 el))
-             (fn [el]
-               (and (seq? el)
-                    (let [x (first el)]
-                      (or (= x `clojure.core/loop)
-                          (= x `clojure.core/fn)))))
-             form))
+  (u/prewalk
+   (fn [el]
+       (if (and (seq? el)
+       (or (= (first el) 'recur)
+           (= (first el) fnname)))
+      `(binding
+        [*as-test* false]
+        (~fnname ~@
+             (next el)))
+    el))
+   (fn [el]
+       (and (seq? el)
+       (let [x (first el)]
+         (or (= x `clojure.core/loop)
+         (= x `clojure.core/fn)))))
+   form))

The inner functions' bodies are indented wrongly. The first body form should start below the n in fn. (That the other contents look wrong here is again just the TABs issue.)

-      (let [pattern-vector (first more)
-            bf          (@#'pm/transform-pattern-spec name pattern-vector args)
-            matchsyms   (pm/bindings-to-argvec bf)
-            body        (next more)
-            pattern     (gensym "pattern")
-            matches     (gensym "matches")
-            action-fn   (gensym "action-fn")
-            match       (gensym "match")
-            recheck-pattern (gensym "recheck-pattern")]
+   (let [pattern-vector (first more)
+                bf
+                (@ #'pm/transform-pattern-spec name pattern-vector args)
+                matchsyms
+                (pm/bindings-to-argvec bf)
+                body
+                (next more)
+                pattern
+                (gensym "pattern")
+                matches
+                (gensym "matches")
+                action-fn
+                (gensym "action-fn")
+                match
+                (gensym "match")
+                recheck-pattern
+                (gensym "recheck-pattern")]

One usually wants to have each variable expression pair on one line.

-(defmacro letrule
-  "Establishes local rules just like `letfn` establishes local fns.
+(defmacro letrule "Establishes local rules just like `letfn` establishes local fns.
   Also see `rule` and `defrule`."
-  {:arglists '([[rspecs] & body])}
+  {:arglists '
+  ([[rspecs]
+    &
+    body])
+  }

That it adds a linebreak after the quote is strange. And usually, one prefers to have each key value pair in a map on the same line if there's enough space. Also, I don't see a reason why the argument list is split into three lines.

-    `(letfn [~@(map (fn [[n & more]]
+   `(letfn [~@ (map (fn

The space between the ~@ and the following form is wrong. The same happens if you have ~(stuff) forms.

-  (fn all-rule-fn [& args]
-    (loop [rs rules, rets [], one-was-applied false]
-      (if (seq rs)
-        (let [r (apply (first rs) args)]
-          (recur (rest rs) (conj rets r) (or one-was-applied r)))
-        (if one-was-applied rets false)))))
+  (fn all-rule-fn
+      [& args]
+      (loop [rs rules
+       ,rets
+       []
+       ,one-was-applied
+       false]
+       (if (seq rs)
+       (let [r (apply (first rs)
+                  args)]
+         (recur (rest rs)
+            (conj rets r)
+            (or one-was-applied r)))
+         (if one-was-applied rets false)))))

Again, in a loop you want to have at least every variable expression pair on the same line. If space permits it, I like having multiple pairs separated by comma on the same line, too. And there it's strange that the commas are placed on a new line.

+           (try (f)
+            (catch Exception
+              e
+              (.printStackTrace e))))))

The variable to which the caught exception is bound should be on the same line as the catch.

I think most other changes I have are just duplicates of the issues above.

[tuhdo]

Thanks for the detailed report. Could you give the sample file above?

[tuhdo]

I've fixed the indenting and indent-tab-mode issues. Please check again when it's ready on MELPA (or you can just copy the file from there and eval again).

[tsdh]

Hi @tuhdo,

the file I used for testing is https://github.com/jgralab/funnyqt/blob/master/src/funnyqt/in_place.clj. I use aggressive-indent-mode so all files in that project should be indented exactly as clojure-mode does it (with the exception of custom macros where I have some define-clojure-indent configs in my ~/.emacs).

I've tested the latest version, and the indent-tabs-mode commit indeed already fixes many issues from above.

[tuhdo]

Meanwhile waiting for it to be fixed, you can use srefactor-lisp-format-sexp to format current sexp at point when there is a let in your function body.

[tuhdo]

These are solved so far:

 (ns funnyqt.in-place
-  "In-place transformation stuff."
-  (:require [clojure.tools.macro   :as m]
-            [funnyqt.generic       :as g]
-            [funnyqt.visualization :as viz]
-            [funnyqt.utils         :as u]
-            [funnyqt.query         :as q]
-            [funnyqt.pmatch        :as pm]
-            [funnyqt.tg            :as tg])
-  (:import
-   (javax.swing JDialog JButton AbstractAction WindowConstants BoxLayout
-                JPanel JLabel JScrollPane JComboBox Action JCheckBox
-                Box BorderFactory ImageIcon JComponent)
-   (java.awt.event ActionEvent ItemEvent ItemListener)
-   (java.awt Color GridLayout GridBagLayout GridBagConstraints)))
+    "In-place transformation stuff."
+    (:require [clojure.tools.macro :as m]
+         [funnyqt.generic :as g]
+         [funnyqt.visualization :as viz]
+         [funnyqt.utils :as u]
+         [funnyqt.query :as q]
+         [funnyqt.pmatch :as pm]
+         [funnyqt.tg :as tg])
+    (:import (javax.swing JDialog JButton AbstractAction WindowConstants BoxLayout JPanel JLabel JScrollPane JComboBox Action JCheckBox Box BorderFactory ImageIcon JComponent)
+        (java.awt.event ActionEvent ItemEvent ItemListener)
+        (java.awt Color GridLayout GridBagLayout GridBagConstraints)))

 (def ^{:dynamic true
-       :doc "Only for internal use.  See `as-pattern' macro."}
-  *as-pattern* false)
+     :doc "Only for internal use.  See `as-pattern' macro."
+     } *as-pattern* false)

-(defmacro as-pattern
-  "Performs the given rule application `rule-app` as a pattern.
+(defmacro as-pattern "Performs the given rule application `rule-app` as a pattern.

-(defmacro letrule
-  "Establishes local rules just like `letfn` establishes local fns.
+(defmacro letrule "Establishes local rules just like `letfn` establishes local fns.
   Also see `rule` and `defrule`."
-  {:arglists '([[rspecs] & body])}
+  {:arglists '
+  ([[rspecs]
+    &
+    body])
+  }

   `(binding [*as-pattern* true]
-     ~rule-app))
+       ~rule-app))

It works well enough for Emacs Lisp, Common Lisp and Scheme. Clojure syntax is more complicated though so it's more difficult to handle. But good nevertheless, since if it can handle Clojure code it should handle Lisps fine. The most difficult one is the let, I will leave it last.

[tsdh]

Good progress so far. I guess the main problem with Clojure is that its binding forms (let, binding) pair the variables and expressions without explicitly wrapping them in parenthesis, right?

[tuhdo]

That's right. To solve this problem, I will add another custom variable, each element is a list of a head symbol (ilke let), the position of sexp inside the sexp of the head symbol to be specially formatted, and a formatter function for that particular sexp instead of current generic sexp formatter.

[tuhdo]

@tsdh Now these are solved:

 (defn ^:private unrecur
   "Replaces (recur ...) forms with (fnname ...) forms where *as-test* is bound to false.
   Existing (fnname ...) forms are also wrapped by bindings of *as-test* to
   false.  Doesn't replace in nested `loop` or `fn` forms."
   [fnname form]
-  (u/prewalk (fn [el]
-               (if (and (seq? el)
-                        (or (= (first el) 'recur)
-                            (= (first el) fnname)))
-                 `(binding [*as-test* false]
-                    (~fnname ~@(next el)))
-                 el))
-             (fn [el]
-               (and (seq? el)
-                    (let [x (first el)]
-                      (or (= x `clojure.core/loop)
-                          (= x `clojure.core/fn)))))
-             form))
+  (u/prewalk
+   (fn [el]
+       (if (and (seq? el)
+       (or (= (first el) 'recur)
+           (= (first el) fnname)))
+      `(binding
+        [*as-test* false]
+        (~fnname ~@
+             (next el)))
+    el))
+   (fn [el]
+       (and (seq? el)
+       (let [x (first el)]
+         (or (= x `clojure.core/loop)
+         (= x `clojure.core/fn)))))
+   form))

+           (try (f)
+            (catch Exception
+              e
+              (.printStackTrace e))))))

The above erroneous case of catch above is easy to handle. All you need to do is add it to the skip symbol list like in this commit: https://github.com/tuhdo/semantic-refactor/commit/7e235f748267809abd1f958f943d7eafdeb248df. 2 means that Srefactor skipos two symbols to the right of the head symbols before it inserts a newline. If you have more cases like this, add it and PR is welcome.

I think it's pretty good to use at a small scale like format an sexp tree, i.e. when you delete and add sexp and the layout becomes a bit messy, just run srefactor-lisp-format-sexp.

[tuhdo]

Now only the let and the handling of closing } left.

[tuhdo]

@tsdh what is this called in Clojure?

{1 2, 3 4, 5 6, 7 8}

[tsdh]

Tu Do [email protected] writes:

@tsdh what is this called in Clojure?

{1 2, 3 4, 5 6, 7 8}

It's a map literal, i.e., a hash-map where the keys are 1, 3, 5, and 7 and the associated values are 2, 4, 6, and 8.

[tuhdo]

@tsdh the trailing bracket:

+     :doc "Only for internal use.  See `as-pattern' macro."
+     } *as-pattern* false)

is solved. Now only the let formatting left.

[tsdh]

:+1:

[tuhdo]

The formatting is getting better over the past week. However, to implement this last case, it will take a while because now I don't have much time anymore. In addition, I want to make it right: instead of manually hard coded the form of sexp, I let user define and customize it like this, for example, the let form:

(srefactor-deftemplate :major-mode clojure-mode
                       :form (let (:newline-gap 2 :align-regexp ":")
                               ))

which means that the second sexp of the form let will insert a newline every 2 sexp (either a symbol or a list) and align according to regex ":" if there's one.

[tsdh]

That looks like a good idea. (I don't get why there should be a ":" in a let-binding form, though.)

And just to give you another challenge, this is also a valid let-form:

(let [x 1
      y 2 #_(this is a form not read by the reader)
      z 17]
  ...)

Any form in clojure can be "commented out" by prefixing it with #_. So such forms have to be completely ignored when inserting a newline every two sexps. However, such a #_sexp form may be arbitrarily complex and should be subject to formatting itself. And of course #_ forms may be nested. :-)

[tuhdo]

That looks like a good idea. (I don't get why there should be a ":" in a let-binding form, though.)

So that it looks like keyword arguments and it's easier to quickly separate keywords and value with a glimpse.

And thanks for the input.

[tsdh]

@tuhdo Sorry, I still don't get it. In a clojure let binding-vector there cannot be any colons to which one could align. The value forms can contain colons, of course, but I don't see why those would provide a sensible point for alignment.

Can you simply provide a clojure code snippet showing what you have in mind?

[tuhdo]

@tsdh the template is not the actual code but formatting spec for a specific sexp in the form. Let's look at this again:

(srefactor-deftemplate :major-mode clojure-mode
                       :form (let [:newline-gap 2 :align-regexp ":"]
                               ))

The form [:newline-gap 2 :align-regexp ":"] you are seeing is not actual Clojure form but merely a format specification to tell Srefactor to apply such formatting to the second form after the let keyword. :newline-gap 2 means that for every 2 sexp in that form, a newline is inserted. Let's have a look at another example:

(srefactor-deftemplate :major-mode common-lisp-mode
                       :form (cond
                              ((:newline-gap 1))
                               ))

It means that the conditional form of the cond will always be inserted with newline after the first sexp, no matter what.

However, I've also thought of another style of formatting to handle cases like loop macro in Common Lisp. For example:

(srefactor-deftemplate :major-mode common-lisp-mode
                       :form (loop
                              for :s in :s :nl
                              for :s from :s to :s :nl
                              do :s
))

When Srefactor see :s, replace it with the actual sexp in the token stream. When Srefactor see :nl, it inserts a newline. For every other symbols in the template, just insert it as is. Of course loop macro has many variants so we must have different templates. The let binding can be also rewritten in this format:

(srefactor-deftemplate :major-mode clojure-mode
                       :form (let [:s :s :nl :align-regexp ":"]
                               ))

It has the same meaning as above, and :align-regexp tells Srefactor to align according to ":".

[tsdh]

@tuhdo Yes, I understand that this is a template for srefactor and no clojure form. What I'm asking for is a sample clojure let-form where the :align-regexp ":" would have an effect (and what the effect would be). ;-)

[tuhdo]

Ah I see. Proper example should be:

(srefactor-deftemplate :major-mode clojure-mode
                       :form (:require [:s :s :nl :align-regexp ":"]
                               ))

[tsdh]

@tuhdo Hehe, you still didn't get me. The above template says that the :require form of a clojure ns form should be aligned at :. So I guess I get an indentation like

(ns my.ns
  (:require [foo.bar.baz :as baz]
             [quux.bla   :as bla]))

so the :as keywords are aligned. That's very cool!

BUT: you also specified :align-regexp ":" in the template for the clojure let form. But in a let form there is usually no point in aligning at a :. Usually, there are no colons at all.

If you wanted to align a let binding form, then you'd align it to the start of the value-expression of the variable with the longest name, e.g.,

(let [x      19
      foo    (do-it :x x)
      z      (some-other-expr)
      foobar 1000]
  ...)

However, I don't know if that's a good idea as default behavior. It's only nice if the let isn't indented very deeply, the variable names are reasonable short, and so are the value expressions.

[allentiak]

Hi, @tuhdo ,

I guess the best would be for the behavior to be based on the clojure style guide.

Best, Leandro