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

Goto matching bracket not working in yaml #3614

Closed
icholy opened this issue Aug 31, 2022 · 5 comments
Closed

Goto matching bracket not working in yaml #3614

icholy opened this issue Aug 31, 2022 · 5 comments
Labels
C-bug Category: This is a bug

Comments

@icholy
Copy link

icholy commented Aug 31, 2022

Summary

The Goto matching bracket not working in yaml.

Reproduction Steps

I tried this:

  1. Place cursor on either { or } and press mm.

image

I expected the cursor to move to the matching bracket, but it didn't move.

Helix log

No response

Platform

Linux

Terminal Emulator

alacritty 0.9.0

Helix Version

helix 22.05 (83f177d)

@icholy icholy added the C-bug Category: This is a bug label Aug 31, 2022
@icholy icholy changed the title Goto matching bracket no working in yaml Goto matching bracket not working in yaml Aug 31, 2022
@alevinval
Copy link
Contributor

alevinval commented Oct 13, 2022

I've been looking at that issue. I'm using the following YAML document

key: ${value}

I've added some debug traces, now I run mm on line 1, which yields:

2022-10-13T23:28:27.687 helix_core::match_brackets [DEBUG] >>>>>> FIND PAIR CALL <<<<<<
2022-10-13T23:28:27.687 helix_core::match_brackets [DEBUG] {Node string_scalar (0, 5) - (0, 13)}
2022-10-13T23:28:27.687 helix_core::match_brackets [DEBUG] pos=6
2022-10-13T23:28:27.687 helix_core::match_brackets [DEBUG] start_byte=5 end_byte=12
2022-10-13T23:28:27.687 helix_core::match_brackets [DEBUG] start_char=5 end_char=12
2022-10-13T23:28:27.687 helix_core::match_brackets [DEBUG] start=$
2022-10-13T23:28:27.688 helix_core::match_brackets [DEBUG] end=}
2022-10-13T23:28:27.688 helix_core::match_brackets [DEBUG] not valid pair...

It will never match because it considers the start character to be the $ which will never match the }.
I also notice the find_pair is called up to 24 times, for that single action.

I notice that the find_matching_bracket_fuzzy is not running the sanity check that find_matching_bracket does, which first checks that the start character is a valid bracket in the first place. Probably because it simply tries to match based on the current node that is selected (in your case, a string scalar).

To be honest, I think this is not really an issue, because even if you put ${something} this is basically a string scalar that begins with $ and ends with }. The fact that you are using something that does string interpolation that's already outside the YAML spec, it's fine because it's a string scalar, but there's no way to know you are trying to interpolate a variable.

@alevinval
Copy link
Contributor

Surely we could implement a hacky workaround like this one:

diff --git a/helix-core/src/match_brackets.rs b/helix-core/src/match_brackets.rs
index 0189dedd..a2aae514 100644
--- a/helix-core/src/match_brackets.rs
+++ b/helix-core/src/match_brackets.rs
@@ -51,7 +51,11 @@ fn find_pair(syntax: &Syntax, doc: &Rope, pos: usize, traverse_parents: bool) ->

     loop {
         let (start_byte, end_byte) = surrounding_bytes(doc, &node)?;
-        let (start_char, end_char) = (doc.byte_to_char(start_byte), doc.byte_to_char(end_byte));
+        let (mut start_char, end_char) = (doc.byte_to_char(start_byte), doc.byte_to_char(end_byte));
+
+        if doc.char(start_char) == '$' && end_char > start_char {
+            start_char += 1
+        }

         if is_valid_pair(doc, start_char, end_char) {
             if end_byte == pos {

This should be relatively safe, because the $ char is not a valid matching pair. If we ever encounter it, we could try skipping to the next character, which worst-case scenario is not a pair either, and best-case scenario we find someone doing variable interpolation with ${...} and then we would match correctly. While I feel this is strictly working as intended, it's pretty common to have YAMLs with string interpolation, so this small change could indeed improve the user experience, and the expectations when running mm on a YAML file.

@alevinval
Copy link
Contributor

alevinval commented Oct 13, 2022

I've opened a draft PR in case that we're interested in adding such things. I feel this is somewhat ugly, yet at the same time may provide a better experience in some cases. But this may also introduce odd behavior in other cases or languages.

(I was interested in trying to contribute to Helix because it's an amazing editor, so I just looked for some raised issues trying to see if I can help you folks, either closing issues or contributing useful stuff)

@the-mikedavis
Copy link
Member

Let's close this in favor of #3357 which is the same problem

@alevinval
Copy link
Contributor

@the-mikedavis I've found some other users bothered with the same behaviour (on plain text files) #1108, then I've found this one #3584 which seems to me like a good long-term solution.

alevinval added a commit to alevinval/helix that referenced this issue Oct 15, 2022
…tching when nothing is matched

This patch introduces bracket matching independent of tree-sitter grammar.
For the initial iteration of this feature, only match on the current line.
This matching is introduced as a fallback in cases that tree-sitter matcher
does not match any bracket.

This fallback should provide a better experience to users that are editing
documents without tree-sitter grammer, but also provides a better experience
in cases like the ones reported in helix-editor#3614

If we find that this feature works well, we could consider extending it
for multi-line matching, but I wanted to keep it small for the first iteration
and gather thoughts beforehand.
alevinval added a commit to alevinval/helix that referenced this issue Oct 15, 2022
…hing

This patch introduces bracket matching independent of tree-sitter grammar.
For the initial iteration of this feature, only match on the current line.
This matching is introduced as a fallback in cases that tree-sitter matcher
does not match any bracket.

This fallback should provide a better experience to users that are editing
documents without tree-sitter grammer, but also provides a better experience
in cases like the ones reported in helix-editor#3614

If we find that this feature works well, we could consider extending it
for multi-line matching, but I wanted to keep it small for the first iteration
and gather thoughts beforehand.
alevinval added a commit to alevinval/helix that referenced this issue Oct 15, 2022
…hing

This patch introduces bracket matching independent of tree-sitter grammar.
For the initial iteration of this feature, only match on the current line.
This matching is introduced as a fallback in cases where the tree-sitter
matcher does not match any bracket.

This fallback should provide a better experience to users that are editing
documents without tree-sitter grammar, but also provides a better experience
in cases like the ones reported in helix-editor#3614

If we find that this feature works well, we could consider extending it
for multi-line matching, but I wanted to keep it small for the first iteration
and gather thoughts beforehand.
alevinval added a commit to alevinval/helix that referenced this issue Oct 15, 2022
…hing

This patch introduces bracket matching independent of tree-sitter grammar.
For the initial iteration of this feature, only match on the current line.
This matching is introduced as a fallback in cases where the tree-sitter
matcher does not match any bracket.

This fallback should provide a better experience to users that are editing
documents without tree-sitter grammar, but also provides a better experience
in cases like the ones reported in helix-editor#3614

If we find that this feature works well, we could consider extending it
for multi-line matching, but I wanted to keep it small for the first iteration
and gather thoughts beforehand.
alevinval added a commit to alevinval/helix that referenced this issue Feb 27, 2023
…hing

This patch introduces bracket matching independent of tree-sitter grammar.
For the initial iteration of this feature, only match on the current line.
This matching is introduced as a fallback in cases where the tree-sitter
matcher does not match any bracket.

This fallback should provide a better experience to users that are editing
documents without tree-sitter grammar, but also provides a better experience
in cases like the ones reported in helix-editor#3614

If we find that this feature works well, we could consider extending it
for multi-line matching, but I wanted to keep it small for the first iteration
and gather thoughts beforehand.
alevinval added a commit to alevinval/helix that referenced this issue Jun 1, 2023
…hing

This patch introduces bracket matching independent of tree-sitter grammar.
For the initial iteration of this feature, only match on the current line.
This matching is introduced as a fallback in cases where the tree-sitter
matcher does not match any bracket.

This fallback should provide a better experience to users that are editing
documents without tree-sitter grammar, but also provides a better experience
in cases like the ones reported in helix-editor#3614

If we find that this feature works well, we could consider extending it
for multi-line matching, but I wanted to keep it small for the first iteration
and gather thoughts beforehand.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug
Projects
None yet
Development

No branches or pull requests

3 participants