Skip to content

Fix that ViFindBrace doesn't search for brace when current char is not a brace #4862

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.

Sign up for GitHub

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

Open
sharpchen wants to merge 2 commits into
base: master
Choose a base branch
from
+81 −4
Open

Fix that ViFindBrace doesn't search for brace when current char is not a brace #4862

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
277b071
Fix that ViFindBrace doesn't search for brace when current char is no…
sharpchen Jul 22, 2025
85ec2c2
Fix `ViGotoBrace` tests
andyleejordan Apr 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion PSReadLine/Movement.vi.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,28 @@ private int ViFindBrace(int i)
case ')':
return ViFindBackward(i, '(', withoutPassing: ')');
default:
return i;
ReadOnlySpan<char> parenthese = stackalloc char[] { '{', '}', '(', ')', '[', ']' };
int nextParen = i;
// find next of any kind of paren
for (; nextParen < _buffer.Length; nextParen++)
for (int idx = 0; idx < parenthese.Length; idx++)
Comment on lines +260 to +261
Copy link
Copy Markdown
Member

@daxian-dbw daxian-dbw Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can find the next parenthesis by the code below, instead of using nested loop here:

ReadOnlySpan<char> parens = stackalloc char[] { '{', '}', '(', ')', '[', ']' };

// Find next parenthesis of any type
int idx = _buffer.AsSpan(i).IndexOfAny(parens);
if (idx < 0)
{
    // No parenthesis, so nothing to match
    return i;
}

int nextParen = i + rel;
int match = _buffer[nextParen] switch
{
    ...
}

return match == nextParen ? i : match;

if (parenthese[idx] == _buffer[nextParen]) goto Outer;
Copy link
Copy Markdown
Member

@andyleejordan andyleejordan Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rare great use of goto

Comment on lines +257 to +262
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variable name parenthese appears to be a typo (should be parentheses or similar). Renaming would improve readability and avoid spreading the misspelling.

Suggested change
ReadOnlySpan<char> parenthese = stackalloc char[] { '{', '}', '(', ')', '[', ']' };
int nextParen = i;
// find next of any kind of paren
for (; nextParen < _buffer.Length; nextParen++)
for (int idx = 0; idx < parenthese.Length; idx++)
if (parenthese[idx] == _buffer[nextParen]) goto Outer;
ReadOnlySpan<char> parentheses = stackalloc char[] { '{', '}', '(', ')', '[', ']' };
int nextParen = i;
// find next of any kind of paren
for (; nextParen < _buffer.Length; nextParen++)
for (int idx = 0; idx < parentheses.Length; idx++)
if (parentheses[idx] == _buffer[nextParen]) goto Outer;

Copilot uses AI. Check for mistakes.

Outer:
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the default case, if no brace/paren/bracket is found from the current position to the end of the buffer, nextParen will end up equal to _buffer.Length, and _buffer[nextParen] will throw IndexOutOfRangeException. Add an explicit check after the scan (e.g., if nextParen >= _buffer.Length return i) before indexing into _buffer.

Suggested change
Outer:
Outer:
if (nextParen >= _buffer.Length)
{
return i;
}

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

@daxian-dbw daxian-dbw Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the default case, if no brace/paren/bracket is found from the current position to the end of the buffer, nextParen will end up equal to _buffer.Length, and _buffer[nextParen] will throw IndexOutOfRangeException

This is a real problem. I prefer to not using the Outer label here. For the goto statement, we should avoid using it unless it's really needed.

int match = _buffer[nextParen] switch
{
// if next is opening, find forward
'{' => ViFindForward(nextParen, '}', withoutPassing: '{'),
'[' => ViFindForward(nextParen, ']', withoutPassing: '['),
'(' => ViFindForward(nextParen, ')', withoutPassing: '('),
// if next is closing, find backward
'}' => ViFindBackward(nextParen, '{', withoutPassing: '}'),
']' => ViFindBackward(nextParen, '[', withoutPassing: ']'),
')' => ViFindBackward(nextParen, '(', withoutPassing: ')'),
_ => nextParen
};

return match == nextParen ? i : match;
}
}

Expand Down
62 changes: 59 additions & 3 deletions test/MovementTest.VI.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using Xunit;
using System.Collections.Generic;
using Xunit;
Comment on lines +1 to +2
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using System.Collections.Generic; appears unused in this file (no List, Dictionary, etc.). Please remove it to avoid compiler warnings and keep usings minimal.

Suggested change
using System.Collections.Generic;
using Xunit;
using Xunit;

Copilot uses AI. Check for mistakes.

namespace Test
{
Expand Down Expand Up @@ -370,7 +371,7 @@ public void ViGlobMovement_EmptyBuffer_Defect1195()
TestSetup(KeyMode.Vi);

TestMustDing("", Keys(
_.Escape, "W"
_.Escape, "W"
));
}

Expand Down Expand Up @@ -423,6 +424,11 @@ public void ViCursorMovement()
[SkippableFact]
public void ViGotoBrace()
{
// NOTE: When the input has unmatched braces, in order to avoid an
// exception caused by AcceptLineImpl waiting for incomplete input,
// the test needs to end with the Vi command "ddi" and assert that
// the result is an empty string.

TestSetup(KeyMode.Vi);

Test("0[2(4{6]8)a}c", Keys(
Expand Down Expand Up @@ -450,10 +456,60 @@ public void ViGotoBrace()
CheckThat(() => AssertCursorLeftIs(4)),
_.Percent,
CheckThat(() => AssertCursorLeftIs(4)),
"ddi"
"ddi" // Unmatched brace
));
}

// Tests when the cursor is not on any paren
foreach (var (opening, closing) in new[] { ('(', ')'), ('{', '}'), ('[', ']') })
{
// Closing paren with backward match
string input1 = $"0{opening}2{opening}4foo{closing}";
Test("", Keys(
input1,
CheckThat(() => AssertCursorLeftIs(9)),
_.Escape, CheckThat(() => AssertCursorLeftIs(8)),
"0ff", CheckThat(() => AssertCursorLeftIs(5)),
_.Percent, CheckThat(() => AssertCursorLeftIs(3)),
_.Percent, CheckThat(() => AssertCursorLeftIs(8)),
"ddi" // Unmatched closing brace
));

// Closing paren without backward match
string input2 = $"0]2)4foo{closing}";
Test(input2, Keys(
input2,
CheckThat(() => AssertCursorLeftIs(9)),
_.Escape, CheckThat(() => AssertCursorLeftIs(8)),
"0ff", CheckThat(() => AssertCursorLeftIs(5)),
_.Percent, CheckThat(() => AssertCursorLeftIs(5)), // stay still
_.Percent, CheckThat(() => AssertCursorLeftIs(5))
));
Comment on lines +478 to +487
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the "Closing paren without backward match" case, % should still trigger a ding (consistent with other unmatched-brace cases in this test and with ViGotoBrace dinging when ViFindBrace returns the current position). Consider using TestMustDing here so the test actually asserts the expected ding behavior, not just that the cursor stays still.

Copilot uses AI. Check for mistakes.

// Opening paren with forward match
string input3 = $"0{opening}2foo6{closing}";
Test(input3, Keys(
input3,
CheckThat(() => AssertCursorLeftIs(8)),
_.Escape, CheckThat(() => AssertCursorLeftIs(7)),
"0ff", CheckThat(() => AssertCursorLeftIs(3)),
_.Percent, CheckThat(() => AssertCursorLeftIs(1)),
_.Percent, CheckThat(() => AssertCursorLeftIs(7))
));

// Opening paren without forward match
string input4 = $"0)2]4foo{opening}(";
TestMustDing("", Keys(
input4,
CheckThat(() => AssertCursorLeftIs(10)),
_.Escape, CheckThat(() => AssertCursorLeftIs(9)),
"0ff", CheckThat(() => AssertCursorLeftIs(5)),
_.Percent, CheckThat(() => AssertCursorLeftIs(5)), // stay still
_.Percent, CheckThat(() => AssertCursorLeftIs(5)),
"ddi" // Unmatched brace
));
}

// <%> with empty text buffer should work fine.
Test("", Keys(
_.Escape, _.Percent,
Expand Down