mdm not working on rightmost pair element

[rockboynton]

Summary

I have a rust import statement, use foo::{bar, baz};
If I place the cursor on the rightmost }, and try to do mdm it doesn't work (error says "Surround pair not found around all cursors")
But if I do mmmdm it works (delete the pair with the cursor on the leftmost {

Reproduction Steps

No response

Helix log

No response

Platform

Linux

Terminal Emulator

wezterm 20240203-110809-5046fc22

Installation Method

Nix Flake

Helix Version

helix 24.7, 748a9cf

[rockboynton]

It is not just rust files, it can be any file

[RoloEdits]

I've replicated this, but seems to be a bit more nuanced than I thought it would be. When deleting, it works if you select the first bracket open bracket, but trying to do this from the closing one leads to the weird behavior.

[rockboynton]

I've replicated this, but seems to be a bit more nuanced than I thought it would be. When deleting, it works if you select the first bracket open bracket, but trying to do this from the closing one leads to the weird behavior.

yeah that is what I experience as well, glad I'm not the only one. I swear this used to work properly

[RoloEdits]

Trying to look into this more, and found that when there is no sort, the behavior of the inner pair is reversed, where the opening pair main selection is what deletes the outer pair.

helix/helix-term/src/commands.rs

Lines 5691 to 5717 in 57ec3b7

	fn surround_delete(cx: &mut Context) {
	let count = cx.count();
	cx.on_next_key(move |cx, event| {
	let surround_ch = match event.char() {
	Some('m') => None, // m selects the closest surround pair
	Some(ch) => Some(ch),
	None => return,
	};
	let (view, doc) = current!(cx.editor);
	let text = doc.text().slice(..);
	let selection = doc.selection(view.id);
	let mut change_pos =
	match surround::get_surround_pos(doc.syntax(), text, selection, surround_ch, count) {
	Ok(c) => c,
	Err(err) => {
	cx.editor.set_error(err.to_string());
	return;
	}
	};
	change_pos.sort_unstable(); // the changeset has to be sorted to allow nested surrounds
	let transaction =
	Transaction::change(doc.text(), change_pos.into_iter().map(|p| (p, p + 1, None)));
	doc.apply(&transaction, view.id);
	exit_select_mode(cx);
	})
	}

The error is coming from this function:

helix/helix-core/src/surround.rs

Lines 92 to 155 in 57ec3b7

	fn find_nth_closest_pairs_plain(
	text: RopeSlice,
	range: Range,
	mut skip: usize,
	) -> Result<(usize, usize)> {
	let mut stack = Vec::with_capacity(2);
	let pos = range.from();
	let mut close_pos = pos.saturating_sub(1);
	for ch in text.chars_at(pos) {
	close_pos += 1;
	if is_open_bracket(ch) {
	// Track open pairs encountered so that we can step over
	// the corresponding close pairs that will come up further
	// down the loop. We want to find a lone close pair whose
	// open pair is before the cursor position.
	stack.push(ch);
	continue;
	}
	if !is_close_bracket(ch) {
	// We don't care if this character isn't a brace pair item,
	// so short circuit here.
	continue;
	}
	let (open, close) = get_pair(ch);
	if stack.last() == Some(&open) {
	// If we are encountering the closing pair for an opener
	// we just found while traversing, then its inside the
	// selection and should be skipped over.
	stack.pop();
	continue;
	}
	match find_nth_open_pair(text, open, close, close_pos, 1) {
	// Before we accept this pair, we want to ensure that the
	// pair encloses the range rather than just the cursor.
	Some(open_pos)
	if open_pos <= pos.saturating_add(1)
	&& close_pos >= range.to().saturating_sub(1) =>
	{
	// Since we have special conditions for when to
	// accept, we can't just pass the skip parameter on
	// through to the find_nth_*_pair methods, so we
	// track skips manually here.
	if skip > 1 {
	skip -= 1;
	continue;
	}
	return match range.direction() {
	Direction::Forward => Ok((open_pos, close_pos)),
	Direction::Backward => Ok((close_pos, open_pos)),
	};
	}
	_ => continue,
	}
	}
	Err(Error::PairNotFound)
	}

[rockboynton]

I actually started debugging it myself and it looks like the plaintext implementation works, just not the treesitter one. And the find_matching_bracket_fuzzy call in particular is where it returns from the function due to the ? operator. That's where I left off

[RoloEdits]

Interesting, I added this:

pub fn find_nth_closest_pairs_pos(
    syntax: Option<&Syntax>,
    text: RopeSlice,
    range: Range,
    skip: usize,
) -> Result<(usize, usize)> {
    match syntax {
        Some(syntax) => find_nth_closest_pairs_ts(syntax, text, range, skip).map_err(|err| {
            log::error!("Error with `find_nth_closest_pairs_ts`");
            err
        }),
        None => find_nth_closest_pairs_plain(text, range, skip).map_err(|err| {
            log::error!("Error with `find_nth_closest_pairs_plain`");
            err
        }),
    }
}

Which when replicating the behavior led to:

2024-10-02T21:55:17.193 helix_core::surround [ERROR] Error with `find_nth_closest_pairs_plain`

[woojiq]

Can you try #10611 ?

[RoloEdits]

I'm still seeing the same behavior as mentioned in this issue. As mentioned above, I seem to be triggering the error at the bottom of find_nth_closest_pairs_plain, but @rockboynton has found issue with the tree-sitter version. Would be great if we could nail down where the actual error is coming from. Then figure out if the logic is faulty or if the parameters being passed in are not not upholding some constraint.