Skip to content
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.

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

bracket matching works incorrectly sometimes #3357

Closed
A-Walrus opened this issue Aug 8, 2022 · 10 comments · Fixed by #7242
Closed

bracket matching works incorrectly sometimes #3357

A-Walrus opened this issue Aug 8, 2022 · 10 comments · Fixed by #7242
Labels
A-helix-term Area: Helix term improvements C-bug Category: This is a bug

Comments

@A-Walrus
Copy link
Contributor

A-Walrus commented Aug 8, 2022

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 A-Walrus added the C-bug Category: This is a bug label Aug 8, 2022
@the-mikedavis the-mikedavis added the A-helix-term Area: Helix term improvements label Aug 8, 2022
@A-Walrus
Copy link
Contributor Author

A-Walrus commented Aug 9, 2022

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?

@the-mikedavis
Copy link
Member

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.

@A-Walrus
Copy link
Contributor Author

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).

@the-mikedavis
Copy link
Member

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 👍

@eightfilms
Copy link

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 }.

@LeoniePhiline
Copy link
Contributor

LeoniePhiline commented Jan 31, 2023

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

@antoyo
Copy link
Contributor

antoyo commented Apr 11, 2023

Same issue in C++ with macros:

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

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

@pascalkuthe
Copy link
Member

#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

@alevinval
Copy link
Contributor

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: #7238 I see it more as a trade-off to improve experience to most users with minimal required work.

@pascalkuthe
Copy link
Member

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements C-bug Category: This is a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants