matching_brace function not correct when jumping from open brace to close brace

[xcaptain]

https://github.com/rust-analyzer/rust-analyzer/blob/31f22d85491d9e7eaadf5fd4f9754c83fc0f3ea6/crates/ra_ide_api/src/matching_brace.rs#L41

swap the order of do_check parameters the makes the test case failed

do_check("struct Foo <|>{ a: i32, }", "struct Foo { a: i32, }<|>");

this matching behavior should be symmetrical

https://github.com/rust-analyzer/rust-analyzer/blob/31f22d85491d9e7eaadf5fd4f9754c83fc0f3ea6/crates/ra_ide_api/src/matching_brace.rs#L18-L19

if the matching node is a left brace, we use start(), if the matching node is a right brace, we should use end()

[matklad]

Great observation! I think we maybe should take cursor position into account as well? That is, cursor should stay inside or outside the braces. A good flea would be to check what IntelliJ does in this case. The fix itself shouldn’t be too hard

…

On Wednesday, 2 October 2019, Joey ***@***.***> wrote: https://github.com/rust-analyzer/rust-analyzer/blob/ 31f22d8/crates/ra_ide_api/ src/matching_brace.rs#L41 swap the order of do_check parameters the makes the test case failed do_check("struct Foo <|>{ a: i32, }", "struct Foo { a: i32, }<|>"); this matching behavior should be symmetrical https://github.com/rust-analyzer/rust-analyzer/blob/ 31f22d8/crates/ra_ide_api/ src/matching_brace.rs#L18-L19 if the matching node is a left brace, we use start(), if the matching node is a right brace, we should use end() — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1942>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANB3M2SHFDOQXXCUHP4KZLQMRCABANCNFSM4I4SUEAA> .

[flodiebold]

That is, cursor should stay inside or outside the braces.

I think there are two problems with that:

1. In a situation like (<|>( we have to choose one brace.
2. In a vim-like editor in normal mode or any terminal editor, the cursor isn't actually "between" the characters, but "on" them. So if I have <|>(, I'd expect to go to <|>).

[xcaptain]

I just happened to run into this problem, feel free to close it if it's not a bug :)

[matklad]

@xcaptain no, I think this is a reasonable thing to do. That is, current impl does something, but it wasn't really well thought out. I'll be happy to accept any PR which makes this more consistent, with both bar and block cursor models.

[matklad]

Hm, I've realized why I haven't noticed this before: I am using a block caret, and, with a block caret, current behavior is pretty reasonable: cursor is always "on" the brace.

But, for a line caret, the behavior is indeed strange, because the cursor visually jumps from outside to inside of the block.

I've checked what IntelliJ does, and it always uses start/end ranges like this issue proposes, which works perfectly for a line cursor (cursor is always outside of parenthesis). For a block cursor, IntelliJ places it on the opening brace, but after the closing brace.

The build-in VS Code brace matching does the opposite, it "favors" block cursor.

I'd like to say that both VS Code and IntelliJ are not perfect, and the behavior really should be depending on the current cursor visual. However, that means that the effect of editor's macros depends on UI settings, and that's wrong.

So I think what we need here is:

• treat the results from the server as logical positions of the braces (ie, always use .start range, like we already do)
• for each client, have a setting to tweak how cursor moves
• pick some default for VS Code client. I think that the default should be the current behavior, as it matches the built-in command. It can be argued that the default for built-in command is wrong, as line carets are more common, and for them IntelliJ behavior makes more sense, but that is really an issue for VS Code.

In other words, action item here is to implement this is an op-in tweak to the TS side of the plugin, here. Sorry for not digging to the root of it from the start: I thought that the correct behavior hear is "obvious", but apparently it isn't.

[flodiebold]

Yeah, the difference between block/line caret was what I was getting at :)

[codgician]

Hi, I'd like to try working on this during my company's hackathon week. 😄

I think maybe we could tweak the behavior according to vscode's editor.cursorStyle setting instead of creating a new setting for users to set.

[codgician]

I also observed a related matching issue with the following Rust code:

let x = (1 + (2 + 3)) * 4;

In a situation like <|>(1 + (2 + 3)) * 4, it will go to (1 + (2 + 3)<|>) * 4, and if user tries to query matching bracket again it will go to (1 + <|>(2 + 3)) * 4 while logically the expected result should be <|>(1 + (2 + 3)) * 4. This behavior exists in both line cursor style and block cursor style.

---

Probably because when matching brace is called at (1 + (2 + 3)<|>) * 4, the parenthesis to the left is returned at

https://github.com/rust-analyzer/rust-analyzer/blob/92abc56bc928fb2a11b8f5b2a37e3c9ee31102d7/crates/ide/src/matching_brace.rs#L22

because find_map returns the first successful result. So when there are multiple consecutive braces, the brace to cursor's left would be returned when VSCode actually prefers to highlight the brace to the right. I am not sure if this should be viewed as a bug but it can be confusing.