paredit does not handle "decorated" nodes

[openvest]

Version
version 1.1.48

Symptom
Cannot slurp or barf, forward or backward in a syntax quoted element
| indicating the cursor, see:

[:left '[foo| bar]  :right]

Related is trying to slurp forward from:

@(def | x) 42

Actual behavior
assertion error is thrown

Diagnosis
assertion error is inside QuoteNode creation

Action
I know that there has been discussion in a separate thread about asserts; but here it looks to be harmful to the cause.

BTW: Thanks for the great library. It is adding paredit functionality to repl-balance (an update of rebel-readine)

[lread]

Thanks for the issue @openvest, I'm happy to see that folks are starting to use the paredit API more.

Here's my repro of your issue:

(require '[rewrite-clj.paredit :as par]
         '[rewrite-clj.zip :as z])

;; navigate to foo and slurp forward
(def zloc (-> "[:left '[foo bar] :right]"
              z/of-string
              z/down
              z/right
              z/down
              z/down))

(z/string zloc)
;; => "foo"

(par/slurp-forward zloc)
;; => Execution error (AssertionError) at rewrite-clj.node.protocols/assert-sexpr-count (protocols.cljc:177).
;;    Assert failed: can only contain 1 non-whitespace form.
;;    (= (count (without-whitespace nodes)) c)

Very good, I can reproduce!

Let's retry without a quoted node to see what happens:

;; repeat with non-quoted
;; navigate to foo
(def zloc (-> "[:left [foo bar] :right]"
              z/of-string
              z/down
              z/right
              z/down))

(z/string zloc)
;; => "foo"

(-> zloc 
    par/slurp-forward
    z/root-string)
;; => "[:left [foo bar :right]]"

Good, no probs without quoted node.

I had launched into reviewing the paredit API with various fixes, but got distracted by other projects. This one is new to me, so thanks for sharing!

[lread]

Ah. Ok. Interesting. The paredit slurp (and other paredit fns) code assumes that if a node can have children, we can directly add a child node.

But in the case of a quote node (and others I'm sure), this is not correct.

A quote node is, more abstractly, a node that can have a single child but also optionally have whitespace. In the case above, the single child is the vector. And it is the vector we want to be slurping into, but paredit is trying to slurp into the quote node.

Exploring in the REPL:

We can add whitespace to a quote node:

(-> "'[foo bar]"
    z/of-string
    (z/insert-child (n/spaces 1))
    ((juxt z/tag z/string z/root-string)))
;; => [:quote "'  [foo bar]" "'  [foo bar]"]

But we cannot add a non-whitespace node:

(-> "'[foo bar]"
    z/of-string
    (z/insert-child :boop))
;; => Execution error (AssertionError) at rewrite-clj.node.protocols/assert-sexpr-count (protocols.cljc:177).
;;    Assert failed: can only contain 1 non-whitespace form.
;;    (= (count (without-whitespace nodes)) c)

But we can add a non-whitespace element to the quoted vector

(-> "'[foo bar]"
    z/of-string
    z/down
    (z/insert-child :boop)
    ((juxt z/tag z/string z/root-string)))
;; => [:vector "[:boop foo bar]" "'[:boop foo bar]"]

I'll see how I can adapt paredit to handle these types of nodes.

[lread]

I'm thinking through this.

Some interesting cases:

Should slurp baz into vector: (same scenario as reported in issue but stresses the possibility of double quoting)

• ''[|foo bar] baz

These should all slurp baz into vector:

• [foo |''bar] baz
• [foo '|'bar] baz
• [foo ''|bar] baz

When slurping into the current node these should all slurp baz into [bar]:

• [foo |''[bar] baz]
• [foo '|'[bar] baz]
• [foo ''|[bar] baz]

Barfing should barf ''bar out of vector for:

• [foo |''bar] baz
• [foo '|'bar] baz
• [foo ''|bar] baz

Other Nodes

This, of course, is not limited to quote ' nodes.
It applies to all nodes that really only hold one child node (with optional whitespace nodes).
I'm currently calling these "decorator" nodes.

We have:

• quote-node 'foo
• deref-node @foo
• eval-node #=foo
• unquote-node ~foo
• unquote-splicing-node ~@foo
• var-node #'foo
• uneval-node #_ foo can be a little different, ex: #_#_#_ foo bar baz

To think about (I think these all fall under decorators too, but the decoration includes user specified content?):

• meta-node ^:foo [1] ^{:foo 42} [1]
• raw-meta-node #^foo [1] #^foo [1]
• namespaced-map-node #:foo{:a 1}
• reader-macro-node #my-macro 42 or even #?(:clj foo :cljs bar)

And

• fn-node #(foo 42 %) - This one might be technically similar to a list/vector/map/set. Will have to see.

Other paredit operations

This issue is not limited to slurping and barfing.

We'll have to evaluate each paredit fn in the context of decorator nodes.

• what does join mean for '''[foo bar] |(baz)? Probably '''[foo bar |baz]
• raise on '''[foo |bar baz] is likely '''bar
• kill will kill the node and its decorators.
• move-to-prev will have to be a bit smarter to skip decorator nodes.
• should split include decorators? '''[foo |bar baz] maybe becomes '''[|foo] '''[bar baz] or maybe without decorations '''[|foo] [bar baz]

[lread]

Known Limitations

Rewrite-clj enforces, in some cases, its idea of valid clojure.
This means that some paredit operations might not make sense.

Discard (uneval) node

A slurp of #_ [|foo] b could be ok, and result in #_ [|foo b].

But what about #_ #_ [|foo] b? If we slurp b into [foo], our 2nd #_ is now invalid.
I think making any automatic compensating adjustments would be more confusing than helpful.

Soo... for now, no-op any paredit ops on uneval nodes that would result in invalid nodes.

Metadata node

A slurp ^{:foobar "baz"} [|1 2 3] 4 could be ok and result in ^{:foobar "baz"} [|1 2 3 4].

But... if slurping from a metadata map, things stop making sense to rewrite-clj.

So... for now, no-op on paredit ops on a metadata map that would result in invalid nodes.

??

I'll add more as I discover them.

[lread]

I may revisit my algorithm for updating the location in the zipper after a paredit operation.

Rewrite-cljs used an internal global-find-by-node assuming that the unique id of a node could be the tools reader populated row/col metadata. This was flawed because nodes can be created dynamically and, in this case, the row/col metadata is absent. #256

When fixing paredit issue, I decided to track navigation so that it could later be restored. I felt searching through the entire zipper for a location after a paredit operation was a bit much and tracking/restoring was relatively straightforward.

But... while looking at this issue, I'm finding that tracking navigation is adding considerable complexity to the paredit code.

I may (and may not, not sure yet) revert to finding the location in the zipper after an op. The difference from global-find-by-node being the unique id will be added to a node explicitly as metadata. This would not be an optimization degradation from the current released paredit API.
If we find it is a performance issue, we can revisit.

[lread]

I'm now calling "decorator" nodes "restricted" nodes. Instead of blathering about them here I'm thinking things through in a design doc.