clj-commons / rewrite-clj

Rewrite Clojure code and edn
https://cljdoc.org/d/rewrite-clj
MIT License
592 stars 55 forks source link

`paredit/kill` and `global-find-by-node` work incorrectly #256

Open mrkam2 opened 8 months ago

mrkam2 commented 8 months ago

Version rewrite-clj/rewrite-clj {:mvn/version "1.1.47"}

Platform Clojure version: 1.11.1

Symptom Two issues:

  1. When using rewrite-clj.paredit/kill incorrectly positions the zipper after performing the operation.
  2. Docstring says that it should be removing nodes to the right from the current node. But in fact it deletes nodes starting from the current node.

Reproduction

(-> (z/of-string "[1 2 3 4]")
    z/down
    (z/insert-left (n/keyword-node :wrong-pos))
    z/next
    (z/insert-left (n/keyword-node :nil-meta))
    pe/kill
    z/print)
:wrong-pos

Actual behavior

  1. Positions zipper on the :wrong-pos.
  2. End result is [:wrong-pos 1 :nil-meta ].

Expected behavior

  1. Expected to position on 2.
  2. Expected end result is [:wrong-pos 1 :nil-meta 2]

Diagnosis

  1. Incorrect positioning is caused by global-find-by-node search that compares nodes meta not taking into account that new nodes have nil meta.
  2. Either docstring or the behavior needs to be fixed to match each other.

Action Let me know if a PR is preferred.

lread commented 8 months ago

Thanks @mrkam2!

I can take a peek sometime soon!

lread commented 7 months ago

I'm starting to take a look at this one!

  1. I confirm that the kill docstring is wrong and the behavior is correct, barring the new node bug you found.
  2. I confirm that kill behavior is broken when new nodes are involved.
  3. And yes, anything that uses paredit API's internal global-find-by-node will be broken for newly added nodes.

Thanks again for reporting, @mrkam2. Folks seem to be starting to use the paredit API a bit, and you've uncovered some very interesting flaws.