drym-org / symex.el

An intuitive way to edit Lisp symbolic expressions ("symexes") structurally in Emacs
Other
271 stars 22 forks source link

Handle an edge case where going down to root would hang #51

Closed countvajhula closed 2 years ago

countvajhula commented 2 years ago

Summary of Changes

In a javascript file like this one:

window.BENCHMARK_DATA = {
  "lastUpdate": 1658432364487,
  "repoUrl": "https://github.com/countvajhula/qi",
}

(link to actual file)

... going "up" towards root while one level below it was causing it to hang.

The reason seems to be that there is a parent node without siblings encountered before the root and that was breaking an assumption somewhere. This PR changes the symex-ts--at-tree-root-p check to be consistent with the check for siblings used in actual movements, to fix the issue.

(Note: There is another issue in the same file where going down from root to a child node seems to skip a level in some cases. I plan to investigate that further but if I don't get to it soon I might do it as a follow-up PR.)

Public Domain Dedication

polaris64 commented 2 years ago

Hi @countvajhula,

I've managed to reproduce the issue you encountered using the test file together with js-mode.

Your patch seems to work fine in my initial test. Alternatively, the following should also work and does not involve any recursive calls: -

diff --git a/symex-ts.el b/symex-ts.el
index fbc1d8b..55106bc 100644
--- a/symex-ts.el
+++ b/symex-ts.el
@@ -221,7 +221,7 @@ Note that this does not consider global root to be a tree root."
   (let ((root (tsc-root-node tree-sitter-tree))
         (cur (symex-ts-get-current-node)))
     (let ((parent (tsc-get-parent cur)))
-      (and parent (tsc-node-eq parent root)))))
+      (or (not parent) (tsc-node-eq cur root)))))

 (defun symex-ts--at-first-p ()
   "Check if the current node is the first one at some level."

tsc-get-parent returns nil if NODE does not have a parent (i.e. is the root). So the predicate should return t if the current node does not have a parent or if the current node is equal to the root.

I haven't tested either solution in any great depth but hopefully I will be able to try later. Perhaps you could try both and see which offers the best runtime performance?

countvajhula commented 2 years ago

Ah I see, didn't realize that was the issue. And looks like this also fixes the other issue I was seeing (mentioned in the note in the description) where going up from window was going all the way to root instead of stopping at window.BENCHMARK_DATA first. Looks good in testing on my end 👌

One thing that's a little weird is that in either solution, the behavior of symex-ts--at-tree-root-p deviates from what the docstring says. In the original one, both window.BENCHMARK_DATA as well as the {...} expression return true as tree root, and the "true" root also returns true. In your solution, neither of those returns true, but the "true" root does return true. The docstring suggests that true root would not return true.

Personally, I'm happy to punt on understanding this discrepancy since the behavior at the moment is correct. We could revisit if we run into it again, and consider this a "note to self" until then. Wdyt?