Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

paredit fns that use internal global-find-by-node do not work after zipper updated #256

Closed
mrkam2 opened this issue Mar 15, 2024 · 3 comments · Fixed by #365
Closed

paredit fns that use internal global-find-by-node do not work after zipper updated #256

mrkam2 opened this issue Mar 15, 2024 · 3 comments · Fixed by #365

Comments

@mrkam2
Copy link

mrkam2 commented Mar 15, 2024

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.

@mrkam2 mrkam2 changed the title global-find-by-node works incorrectly paredit/kill and global-find-by-node works incorrectly Mar 15, 2024
@mrkam2 mrkam2 changed the title paredit/kill and global-find-by-node works incorrectly paredit/kill and global-find-by-node work incorrectly Mar 16, 2024
@lread
Copy link
Collaborator

lread commented Mar 16, 2024

Thanks @mrkam2!

I can take a peek sometime soon!

@lread
Copy link
Collaborator

lread commented Apr 27, 2024

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.

@lread lread added this to rewrite-clj Jul 3, 2024
@lread lread moved this to In Progress in rewrite-clj Jul 3, 2024
@lread lread changed the title paredit/kill and global-find-by-node work incorrectly paredit fns that use internal global-find-by-node do not work after zipper updated Feb 18, 2025
@lread
Copy link
Collaborator

lread commented Feb 18, 2025

As I fix other issues, I'll update what I've fixed for this issue here:

  • join
  • kill-one-at-pos
  • slurp-forward
  • slurp-forward-fully
  • slurp-backward
  • slurp-backward-fully
  • barf-forward
  • barf-backward
  • splice-killing-forward
  • splice-killing-backward
  • split-at-pos
  • split
  • kill
  • kill-at-pos

lread added a commit that referenced this issue Feb 18, 2025
- rewritten to not use internal `global-find-by-node`
(contributes to #256)
- now takes type of left sequence (closes #321)
- no longer removes comments between strings when joining 2
strings (closes #351)
lread added a commit that referenced this issue Feb 18, 2025
* paredit/join fn fixes

- rewritten to not use internal `global-find-by-node`
(contributes to #256)
- now takes type of left sequence (closes #321)
- no longer removes comments between strings when joining 2
strings (closes #351)

* docs: typos changelog [skip ci]

* docs: changelog typo [skip ci]
lread added a commit that referenced this issue Feb 18, 2025
In-string handling now:
- no-op if pos does not locate to word (i.e. whitespace)
- no longer has off-by-one error for word deletion

Closes #343

Also contributes to #256
lread added a commit that referenced this issue Feb 18, 2025
Because of #256, slurp fns had to be rewritten. During rewriting:

- address design flaw by deprecating existing `slurp` fns and adding
replacement `slurp-`*`-into` fns (closes #339)
- stop adding space char when preserving slurped newlines (closes #345)
- review ambiguous `-slurp-`*-`fully` fn behaviour (closes #341)
- when slurping, don't consider a node with `#_` uneval nodes
empty (closes #338)
- don't throw on rewrite-clj parseable but invalid clojure
`{:a}` (closes #336)
- slurping forward fully no longer throws when slurping into an empty
seq that is last item in a seq (closes #335)
- slurping backward at empty-seq at start of seq no longer
throws (closes #334)
- slurp forward now slurps when at empty-seq at end of seq (closes #333)
lread added a commit that referenced this issue Feb 18, 2025
Because of #256, slurp fns had to be rewritten. During rewriting:

- address design flaw by deprecating existing `slurp` fns and adding
replacement `slurp-`*`-into` fns (closes #339)
- stop adding space char when preserving slurped newlines (closes #345)
- review ambiguous `-slurp-`*-`fully` fn behaviour (closes #341)
- when slurping, don't consider a node with `#_` uneval nodes
empty (closes #338)
- don't throw on rewrite-clj parseable but invalid clojure
`{:a}` (closes #336)
- slurping forward fully no longer throws when slurping into an empty
seq that is last item in a seq (closes #335)
- slurping backward at empty-seq at start of seq no longer
throws (closes #334)
- slurp forward now slurps when at empty-seq at end of seq (closes #333)
lread added a commit that referenced this issue Feb 19, 2025
lread added a commit that referenced this issue Feb 19, 2025
lread added a commit that referenced this issue Feb 19, 2025
`barf-forward` and `barf-backward` rewritten to not use `global-find-by-node`.

Contributes to #256
lread added a commit that referenced this issue Feb 19, 2025
`barf-forward` and `barf-backward` rewritten to not use `global-find-by-node`.

Contributes to #256
lread added a commit that referenced this issue Feb 19, 2025
When `pos` is at end of string marker `"` or end of sequence
marker `]`, `)` etc, kill the node.

Closes #362

Rewrite of `kill-at-pos` contributes to #256
lread added a commit that referenced this issue Feb 19, 2025
When `pos` is at end of string marker `"` or end of sequence
marker `]`, `)` etc, kill the node.

Closes #362

Rewrite of `kill-at-pos` contributes to #256
lread added a commit that referenced this issue Feb 19, 2025
When `pos` is at end of string marker `"` or end of sequence
marker `]`, `)` etc, kill the node.

Closes #362

Rewrite of `kill-at-pos` contributes to #256
lread added a commit that referenced this issue Feb 19, 2025
lread added a commit that referenced this issue Feb 19, 2025
lread added a commit that referenced this issue Feb 20, 2025
Fixes for splice-killing-forward, splice-killing-backward, split-at-pos.

These are the final fns that relied on reader positional metadata,
add in a test to confirm paredit works on zipper without this metadata.

Closes #256
lread added a commit that referenced this issue Feb 20, 2025
Fixes for splice-killing-forward, splice-killing-backward, split-at-pos.

These are the final fns that relied on reader positional metadata,
add in a test to confirm paredit works on zipper without this metadata.

Closes #256
@github-project-automation github-project-automation bot moved this from In Progress to Done in rewrite-clj Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants