feat(core): add plaintext matching fallback to tree-sitter matching #4288

alevinval · 2022-10-15T09:57:31Z

This patch introduces bracket matching independent of tree-sitter grammar. This matching is introduced as a fallback in cases where the tree-sitter matcher does not match any bracket. The fallback should provide a better experience in cases like the ones reported in #3614

Relates to #3584 as well

alevinval · 2022-10-15T09:58:14Z

Let me know your thoughts about this.

helix-term/src/commands.rs

workingj · 2023-01-26T23:07:37Z

hi, any progress here possible?

alevinval · 2023-02-26T21:47:09Z

@archseer thoughts on this PR?

helix-core/src/match_brackets.rs

pascalkuthe · 2023-02-27T13:51:39Z

helix-term/src/commands.rs

+            if let Some(pos) = match_brackets::find_matching_bracket_fuzzy(syntax, text, pos)
+                .or_else(|| match_brackets::find_matching_bracket_current_line_plaintext(text, pos))


This doesn't seem to be quite what we want. This always perform the plaintext matching if the TS matching doesn't find a match. Meanwhile for plaintext files this won't work as in that case doc.syntax() is None (condition above).

I think we can do better for treesitter than the current implementation (which gets easily broken by comments for example). That solution might partially reuse your solution but would be constrained to not leave the current TS node. You can decide whether you want to do that in this PR or a followup PR, that would be the right fix for #3357

As it's implemented right now I think it's better to only us the plaintext matching for files without a TS grammar (so doc.syntax() would be None)

I see, this fallback will never run when there's no grammar. However, most of the users of this editor will be coding, and likely have an active TS grammar. My hope is that adding this fallback now already brings improvement, and eventually it will be useless if the TS logic improves.

Agreed, this makes the worst case (no matching bracket) a lot worse: first the TS lookup will fail, then this will scan backwards through the whole document (!!), failing too.

I think this is a great improvement for languages with no tree sitter grammar though.

I agree with Pascal:

find_matching_bracket_current_line_plaintext should only be used if there's no TS grammar available

If there's a grammar available, this scan should only happen inside the current TS node to limit how far we're attempting to scan.

That should settle it: adf0fbd

archseer · 2023-03-10T06:37:41Z

helix-term/src/commands.rs

+            if let Some(pos) = match_brackets::find_matching_bracket_fuzzy(syntax, text, pos)
+                .or_else(|| match_brackets::find_matching_bracket_current_line_plaintext(text, pos))


Agreed, this makes the worst case (no matching bracket) a lot worse: first the TS lookup will fail, then this will scan backwards through the whole document (!!), failing too.

I think this is a great improvement for languages with no tree sitter grammar though.

I agree with Pascal:

find_matching_bracket_current_line_plaintext should only be used if there's no TS grammar available

If there's a grammar available, this scan should only happen inside the current TS node to limit how far we're attempting to scan.

helix-core/src/match_brackets.rs

A-Walrus

A few style nits, other than that and what has already been said by @pascalkuthe and @archseer looks good!

helix-core/src/match_brackets.rs

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

- Introduce new variables only when needed - Avoid calling twice `is_valid_pair` on worst-case scenarios - Remove checking if we've closed all brackets on each iteration, this check only makes sense whenever we encouter a closing bracket.

Less overlapping variable names, now it should be even more clear to read. Also added some extra testing, specially to verify that this feature only works on the current line.

…rest Also fix some typos.

Move the decrement operation *after* checking if this is the last bracket to close. This avoids a useless operation. When `is_valid_pair` is entered, we already know we are closing a bracket, when there's only 1 bracket pending to be closed, we already know we're going to return immediately anyway, no need to decrement.

- Remove constraint to work only on current line - Improve iteration by properly using ropes

…id freezes

…sitter)

alevinval · 2023-06-01T14:56:31Z

Folks! I've been out of the loop for some time. I got back into this, I've rebased latest master to make sure everything kept working, the pending feedback that was requested has been applied in: adf0fbd 715b928 and f5cb382

pascalkuthe

One minor thing the rest lgtm

helix-term/src/commands.rs

pascalkuthe

LGTM 👍

dead10ck

Looks good to me, thanks for your contribution!

dead10ck · 2023-06-05T12:51:03Z

helix-core/src/match_brackets.rs

+
+    let mut open_cnt = 1;
+
+    for (i, candidate) in chars_iter.take(MAX_PLAINTEXT_SCAN).enumerate() {


One minor thing I'm noticing is that this doesn't limit the search to the current line only, as it advertises; it simply limits the scan to a max number of chars. I actually think this is fine, because limiting matching to the current line only seems unnecessarily restrictive to me, but we should probably update the function name and doc comment.

yeah it was changed to a fixed limit (from only matching on the current line) after feedback from @archseer and me. I missed that the doc comment wasn't updated good catch

Do you folks use something to embed a constant inside a Doc? I wanted to do this:

diff --git a/helix-core/src/match_brackets.rs b/helix-core/src/match_brackets.rs index 0ec040e5..07eb261a 100644 --- a/helix-core/src/match_brackets.rs +++ b/helix-core/src/match_brackets.rs @@ -73,9 +73,9 @@ fn find_pair(syntax: &Syntax, doc: &Rope, pos: usize, traverse_parents: bool) -> } } -/// Returns the position of the matching bracket under cursor, the search -/// is limited to the current line. This function works on plain text, it -/// ignores tree-sitter grammar. +/// Returns the position of the matching bracket under cursor. +/// This function works on plain text and ignores tree-sitter grammar. +/// The search is limited to [`MAX_PLAINTEXT_SCAN`] characters. /// /// If the cursor is on the opening bracket, the position of /// the closing bracket is returned. If the cursor on the closing

But that would require making the constant public for the docs to link to it correctly. If you are okay with that anyway (it works, but does not link) then I can push this

That seems fine to me 👍

Just remove the angle brackets SL that cargo xtask docgen doesn't complain. The link doesn't matter much. The docs are not build anyway.

…elix-editor#4288)

alevinval changed the title ~~bracket_matching: add plaintext matching fallback to tree-sitter matching~~ match_brackets: add plaintext matching fallback to tree-sitter matching Oct 15, 2022

mitsuhiko reviewed Oct 15, 2022

View reviewed changes

helix-term/src/commands.rs Outdated Show resolved Hide resolved

alevinval force-pushed the issue-3584 branch 2 times, most recently from d446f9c to 2f38d0b Compare October 15, 2022 12:15

the-mikedavis added A-core Area: Helix core improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Oct 15, 2022

alevinval changed the title ~~match_brackets: add plaintext matching fallback to tree-sitter matching~~ feat(core): add plaintext matching fallback to tree-sitter matching Oct 22, 2022

archseer self-requested a review February 27, 2023 01:49

pascalkuthe requested changes Feb 27, 2023

View reviewed changes

pascalkuthe added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 27, 2023

alevinval force-pushed the issue-3584 branch 2 times, most recently from 50e0366 to a1bf10f Compare February 27, 2023 22:17

archseer added this to the next milestone Mar 10, 2023

archseer mentioned this pull request Mar 10, 2023

Feat: Sticky Context #6118

Draft

38 tasks

archseer requested changes Mar 13, 2023

View reviewed changes

A-Walrus suggested changes Mar 14, 2023

View reviewed changes

helix-core/src/match_brackets.rs Outdated Show resolved Hide resolved

helix-core/src/match_brackets.rs Outdated Show resolved Hide resolved

pascalkuthe modified the milestones: 23.03, next Mar 27, 2023

alevinval added 8 commits June 1, 2023 15:35

feedback: address clippy complaints, and extract variables.

138fe38

self-review: better variable names, improved test coverage

9f34f13

Less overlapping variable names, now it should be even more clear to read. Also added some extra testing, specially to verify that this feature only works on the current line.

docs: make the documentation of the new function consistent with the …

9de7029

…rest Also fix some typos.

self-review: use or_else to make the fallback more concise

ac239af

Apply feedback

1e932a9

- Remove constraint to work only on current line - Improve iteration by properly using ropes

alevinval added 5 commits June 1, 2023 15:35

Ensure cargo fmt

3370b05

Remove unused assertion function

9ee32af

feedback: convert some of the comments to doc-comments

f5cb382

feedback: limit plain-text scanning to 10k characters at most, to avo…

715b928

…id freezes

feedback: add plain-text fallback only when there is no syntax (tree-…

adf0fbd

…sitter)

alevinval force-pushed the issue-3584 branch from aa1a794 to adf0fbd Compare June 1, 2023 14:51

archseer requested review from pascalkuthe and archseer June 2, 2023 08:01

pascalkuthe reviewed Jun 5, 2023

View reviewed changes

helix-term/src/commands.rs Outdated Show resolved Hide resolved

feedback: fix bug, use to ensure it only runs when there's no syntax

67099d1

pascalkuthe previously approved these changes Jun 5, 2023

View reviewed changes

pascalkuthe added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 5, 2023

dead10ck previously approved these changes Jun 5, 2023

View reviewed changes

dead10ck reviewed Jun 5, 2023

View reviewed changes

alevinval dismissed stale reviews from dead10ck and pascalkuthe via 5fefbe7 June 5, 2023 13:33

feedback: fix documentation

b745fb0

alevinval force-pushed the issue-3584 branch from 5fefbe7 to b745fb0 Compare June 5, 2023 13:36

archseer merged commit a2b8cfd into helix-editor:master Jun 5, 2023

alevinval mentioned this pull request Jun 5, 2023

tree-sitter independent bracket matching #3584

Closed

Triton171 pushed a commit to Triton171/helix that referenced this pull request Jun 18, 2023

feat(core): add plaintext matching fallback to tree-sitter matching (h…

d1fd181

…elix-editor#4288)

wes-adams pushed a commit to wes-adams/helix that referenced this pull request Jul 4, 2023

feat(core): add plaintext matching fallback to tree-sitter matching (h…

8f667a7

…elix-editor#4288)

pascalkuthe mentioned this pull request Jul 20, 2023

Bug: Matching brackets not working in plain text. #1108

Closed

AlexanderBrevig mentioned this pull request Mar 4, 2024

fix: close #9771 fix comments with ( and ) #9800

Merged

mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request Jun 2, 2024

feat(core): add plaintext matching fallback to tree-sitter matching (h…

81780f9

…elix-editor#4288)

smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024

feat(core): add plaintext matching fallback to tree-sitter matching (h…

9e7e9ea

…elix-editor#4288)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): add plaintext matching fallback to tree-sitter matching #4288

feat(core): add plaintext matching fallback to tree-sitter matching #4288

alevinval commented Oct 15, 2022 •

edited

Loading

alevinval commented Oct 15, 2022

workingj commented Jan 26, 2023

alevinval commented Feb 26, 2023

pascalkuthe Feb 27, 2023

alevinval Feb 27, 2023

archseer Mar 10, 2023

alevinval Jun 1, 2023

archseer Mar 10, 2023

A-Walrus left a comment

alevinval commented Jun 1, 2023

pascalkuthe left a comment

pascalkuthe left a comment

dead10ck left a comment

dead10ck Jun 5, 2023

pascalkuthe Jun 5, 2023 •

edited

Loading

alevinval Jun 5, 2023 •

edited

Loading

dead10ck Jun 5, 2023

pascalkuthe Jun 5, 2023

		if let Some(pos) = match_brackets::find_matching_bracket_fuzzy(syntax, text, pos)
		.or_else(\|\| match_brackets::find_matching_bracket_current_line_plaintext(text, pos))


		let mut open_cnt = 1;

		for (i, candidate) in chars_iter.take(MAX_PLAINTEXT_SCAN).enumerate() {

feat(core): add plaintext matching fallback to tree-sitter matching #4288

feat(core): add plaintext matching fallback to tree-sitter matching #4288

Conversation

alevinval commented Oct 15, 2022 • edited Loading

alevinval commented Oct 15, 2022

workingj commented Jan 26, 2023

alevinval commented Feb 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

A-Walrus left a comment

Choose a reason for hiding this comment

alevinval commented Jun 1, 2023

pascalkuthe left a comment

Choose a reason for hiding this comment

pascalkuthe left a comment

Choose a reason for hiding this comment

dead10ck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pascalkuthe Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

alevinval Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alevinval commented Oct 15, 2022 •

edited

Loading

pascalkuthe Jun 5, 2023 •

edited

Loading

alevinval Jun 5, 2023 •

edited

Loading