feat: find closest pairs using tree-sitter

[woojiq]

Closes #8325
closes #3615
closes #3432
closes #4475
closes #7012

Finding the closest surround pairs (mim, mam) was done without being aware of the syntax. This PR adds a tree-sitter implementation that may be faster (untested), but most important it's more correct.
If the syntax distinct strings than mim and mam will work inside strings (didn't work with plaintext impl), matching should also work inside injections.

OLD

`find_nth_closest_pairs_pos` produces unsorted tuple. The tuple must be sorted before passing it to the change Transaction. Otherwise, this statement will fail: https://github.com/helix-editor/helix/blob/13d4463e4146473391e37b7f75e99b731aa55878/helix-core/src/transaction.rs#L582-L585

MRE:

1. hx
2. repeat exact keys (two spaces after "("): ( <esc>hvhmdm. Make sure auto-pairs is turn on.

[pascalkuthe]

I haven't revied this too much yet but it strikes me as a bit odd that mdm actually uses the codepath for mi<x> instead of the codepath of mim/mam (which uses TS).

Perhpahs instead of fixing the broken code here it might be more worthwhile to change that? Thaughts @the-mikedavis

[the-mikedavis]

I think it makes sense historically: surround_* functions were all introduced together before textobjects: #320 vs #728, but I think it makes more sense to use tree-sitter if possible (ideally the same code path as mm?)

[woojiq]

mim/mam (which uses TS).

Under the hood, both commands use

helix/helix-term/src/commands.rs

Line 5163 in 2284bce

'm' => textobject::textobject_pair_surround_closest(

which uses function that doesn't use TS

helix/helix-core/src/textobject.rs

Line 230 in 2284bce

None => surround::find_nth_closest_pairs_pos(slice, range, count),

Though, as @the-mikedavis mentioned, mm uses TS to find the matching brackets

helix/helix-core/src/match_brackets.rs

Line 54 in 2284bce

fn find_pair(

I think we need to rewrite find_nth_closest_pairs_pos to use/reuse something like find_pair (but n times) when TS is present and fall back to the current implementation otherwise. Is it a good one?

[woojiq]

I think I'm done. I've created a new function find_nth_closest_pairs_ts which under the hood uses find_matching_bracket_fuzzy - a tree-sitter version of finding nearest closing bracket. I added some tests:

• surround_delete checks for crashes (linked issue) on mdm.
• surround_replace_ts tests a new version of finding nearest pairs using tree-sitter.

Now "mim" or "mrm" seems more natural than before. If you have a selection that crosses multiple brackets, a pair of brackets will be found that completely covers the selection, which was not the case with the plain implementation (which assumed that the selection was just the cursor head).

[pascalkuthe]

I still need to do some testing but from reading trough the changes this looks great.

[pascalkuthe]

Changes look go to me. I will do some testing in my daily driver branch (I use mim/mam quite a bit)

[woojiq]

I found one discrepancy. Since I combined the pairs into one list, quotes are now considered as pairs too. But if you use a file without the tree-sitter syntax, it will still skip mim|mam for quotes:

Try mim on this text { " cursor here " }

because current impl of nearest brackets first checks if the char is open and continues to look for a closed char if it is true.

helix/helix-core/src/surround.rs

Line 70 in 9f6e9a1

if is_open_pair(ch) {

The code used to work correctly because we had a different set of pairs (without the quotes). But now that the pairs are merged into one, I believe it should also accept quotes as pairs so that all pairs from PAIRS are treated the same (they do with tree-sitter, this "bug" only applies to non-tree-sitter files). The solution is simple - give the check of the closing element a higher priority. Do I need to do this?

[pascalkuthe]

Hmm good qustion we might wnat to just not support quotes for plaintext files since its not clear whether we are currently "inside" the pair or not. We do the same for match_bracket.

[woojiq]

we might wnat to just not support quotes for plaintext

If so then it works as expected, but for clarity I might add something like if is_quote(ch) to the plain implementation to make it more obvious to readers what's going on.

[pascalkuthe]

we might wnat to just not support quotes for plaintext

If so then it works as expected, but for clarity I might add something like if is_quote(ch) to the plain implementation to make it more obvious to readers what's going on.

I think a comment would be good 👍

[woojiq]

So I took a closer look at the behavior of the code with gdb and found that this is not true:

We do the same for match_bracket.

On the master (this is not a side effect of my PR🙂), it's still going through all the text trying to find closing quote. But it just never finds it (redundant cpu cycles), because there is no opposite state for quotes. I don't want to do it in this PR, but I'll probably make another one when I have some free time to clean up match_brackets.rs and surround.rs a bit.
A few things need to be done:

1. Name functions and variables accordingly. Some have "bracket" in the name but actually check all pairs, etc.
2. Distinguish between quotes and brackets so there is no hidden flows when matching on plaintext.

[pascalkuthe]

hmm I think you are right reading the code again. I think that is an oversight tough the original PR stated it worked that way.

I think I assumed from the naming of is_valid_bracket that it would exclude quotes (and therefor bail here:

helix/helix-core/src/match_brackets.rs

Line 145 in 9f6e9a1

if !is_valid_bracket(bracket) {

) during review so it would be good to clean that up.

It doesn't actually work tough if you test it (pressing mm on quotes in plaintext files does nothing).

[pascalkuthe]

would really love to see this land and think its almost there. Are you interested in finishing this @woojiq?

[woojiq]

would really love to see this land and think its almost there. Are you interested in finishing this @woojiq?

Sure, afair this pr is almost ready, I'm going to look again and rebase in 1-2 days.

[woojiq]

I've rebased and changed this PR a bit. To me it looks much better than before. I wrote few tests for mam.
Let me know if I need to explain more in the description or commit messages what this PR does (as it looks pretty obvious to me because I implemented this😀).

[pascalkuthe]

thanks for finishing that this will be great to have!

[the-mikedavis]

I agree, looks great! Could you rebase / merge master to resolve the conflicts?

[thomasaarholt]

@woojiq, I found that this doesn't work for quotation symbols (") in Python, and only on the opening quotation mark in Rust (for let foo = "hello world", mim only works with the cursor positioned on the first ", not the last one).

If you have the time and energy, would you mind taking a look?
See my comment here for more details.

[pascalkuthe]

#4475 (comment)

Only the rudt case is abug either this PR, the pythpm case is an existing bug with mm that I wanted to fix for a while and you don't need to look into.