helix icon indicating copy to clipboard operation
helix copied to clipboard

bracket matching works incorrectly sometimes

Open A-Walrus opened this issue 2 years ago • 5 comments

Summary

For example in this example, with the cursor on either one of the parentheses in Some(thing), the matching one is not highlighted and hitting mm doesn't take you to the matching one, instead it takes you to the bracket on line 6.

fn main() {
    let opt: Option<()> = None;
    match opt {
        Some(thing) => {},
        None => {}
    }
}

Reproduction Steps

asciinema

I tried this:

  1. hx
  2. open the rust file with the example code from above, make sure the language is set to rust
  3. put the cursor on either one of the parentheses in Some(thing)
  4. hit mm I expected this to happen: The matching parenthesis is highlighted in step 3, and moved to in step 4

Instead, this happened: Nothing is highlighted in step 3, and the bracket on line 6 is gone to in step 4

Helix log

~/.cache/helix/helix.log
please provide a copy of `~/.cache/helix/helix.log` here if possible, you may need to redact some of the lines

Platform

Linux

Terminal Emulator

Kitty

Helix Version

22.05-376-g6b84344e

A-Walrus avatar Aug 08 '22 17:08 A-Walrus

This appears to be because the bracket matching expects matching brackets to be the border of a tree sitter node, but in this case Some, (, thing and ) are all children of the same node, with no separate parent for (thing). This is unlike the case of

let a = Some(5);

where (5) are grouped separately under an arguments node.

Is this an issue/bug in tree-sitter-rust (that they should group the (thing) in the above example), or should we change the matching bracket code on our end?

A-Walrus avatar Aug 09 '22 12:08 A-Walrus

If matching brackets within a node is possible without behavior regressions, that'd be preferrable. tree-sitter-rust actually already exposes a node just for the (5) part and the parens are children of that

(call_expression
  function: (identifier)   ; "Some"
  arguments:
    (arguments             ; "(5)"
       (integer_literal))) ; "5"

So I'm not sure there's a change in tree-sitter-rust we can make to improve the situation.

the-mikedavis avatar Aug 10 '22 01:08 the-mikedavis

tree-sitter-rust actually already exposes a node just for the (5) part and the parens are children of that

That's correct, but the problem is that in the case of match arms: Some, (, thing and ), are all siblings, with no node for just (thing). I do think tree-sitter rust could be changed, but it's also possible to fix this within helix (I'll work on this).

A-Walrus avatar Aug 10 '22 10:08 A-Walrus

Oh yeah in matchs they are all under one node. We could fork tree-sitter-rust and ensure that tuple_struct_pattern uses an arguments node as a child to fix this case. I'm worried that other grammars are also inconsistent about having named nodes for their pairs. So I think it's best to improve this within helix if we can 👍

the-mikedavis avatar Aug 10 '22 12:08 the-mikedavis

Also facing this issue with Zig:

const std = @import("std");

const StringHashMap = std.StringHashMap;

pub fn MyStruct() type {
    return struct {
        const Self = @This();

        map: StringHashMap,

        pub fn init() Self {
            return Self{
                .map = StringHashMap([]const u8).init(),
            };
        }
    }; // <-- 'mm' here
} // <-- jumps here

The 2nd last } jumps to the last }.

eightfilms avatar Jan 11 '23 16:01 eightfilms

Should this be a separate bug report?

I get wrong bracket matching with the following :lang html documents:

<!-- () -->
<!-- ) -->
<!-- ( -->

The comments may contain more content. (Makes no difference.) This is a minimal reproduction.

In each instance, all matching brackets to ( or ) are mistakenly found to be >.

Here is how tokyonight highlights > as matching bracket to ( and ):

grafik

grafik

Note that the matching bracket to > is (correctly) <:

grafik

LeoniePhiline avatar Jan 31 '23 23:01 LeoniePhiline

Same issue in C++ with macros:

#define TEST(A, B) ((A) + (B))

None of the parenthesis in ((A) + (B)) are highlighted.

antoyo avatar Apr 11 '23 17:04 antoyo

#define TEST(A, B) ((A) + (B))

I think that's a separate issue. ma) works for these cases just mm and bracket highlights don't

pascalkuthe avatar Jun 05 '23 14:06 pascalkuthe

I know this is not the perfect solution, but it would alleviate glaring cases like the one presented in this issue, where you are on top of the bracket: https://github.com/helix-editor/helix/pull/7238 I see it more as a trade-off to improve experience to most users with minimal required work.

alevinval avatar Jun 05 '23 15:06 alevinval

Same issue in C++ with macros:

#define TEST(A, B) ((A) + (B))

None of the parenthesis in ((A) + (B)) are highlighted.

this is a pretty big edge-case. ) was able to handle everything else through TS bet C preprocessor tokens are just plaintext blobs. I handled that case with a super restrictive plaintext fallback. That case could be handled with TS too by injecting c into c preprocessor nodes the way nvim does but that feels hacky...

pascalkuthe avatar Jun 06 '23 00:06 pascalkuthe