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

vim: Update anyquotes and anybrackets to behave like mini.ai plugin #24167

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

oca159
Copy link
Contributor

@oca159 oca159 commented Feb 4, 2025

Overview

This PR improves the existing mini.ai‐like text-object logic for both “AnyQuotes” (quotes) and “AnyBrackets” (brackets) by adding a multi‐line fallback. The first pass searches only the current line for a best match (cover or next); if none are found, we do a multi‐line pass. This preserves mini.ai's usual “line priority” while ensuring we can detect pairs that start on one line and end on another.

What Changed

  1. Brackets
  • Line-based pass uses gather_line_brackets(map, caret.row()) to find bracket pairs ((), [], {}, <>) on the caret’s line.
  • If that fails, we call gather_brackets_multiline(map) to single‐pass scan the entire buffer, collecting bracket pairs that might span multiple lines.
  • Finally, we apply the mini.ai “cover or next” logic (pick_best_range) to choose the best.
  1. Quotes
  • Similar line-based pass with gather_line_quotes(map, caret.row()).
  • If no local quotes found, we do a multi‐line fallback with gather_quotes_multiline(map), building a big string for the whole buffer and using naive regex for "...", '...', and ....
  • Also preserves “inner vs. outer” logic:
    • For inner (e.g. ciq), we skip bounding quotes or brackets if the range is at least 2 characters wide.
    • For outer (caq), we return the entire range.
  1. Shared “finalize” helpers
  • finalize_bracket_range and finalize_quote_range handle the “inner” skip‐chars vs. “outer” logic.
  • Both rely on the same “line first, then full fallback” approach.

Why This Matters

  • Old Behavior: If you had multi‐line brackets { ... } or multi‐line quotes spanning multiple lines, they weren’t found at all, since we only scanned line by line. That made text objects like ci{ or ciq fail in multi-line scenarios.
  • New Behavior: We still do a quick line pass (for user‐friendly “line priority”), but now if that fails, we do a single‐pass approach across the entire buffer. This detects multi‐line pairs and maintains mini.ai’s “cover‐or‐next” picking logic.

Example Use Cases

  • Curly braces: e.g., opening { on line 10, closing } on line 15 → previously missed; now recognized.
  • Multi‐line quotes: e.g., "'Line 1\nLine 2', no longer missed. We do gather_quotes_multiline with a naive regex matching across newlines.

Tests

  • Updated and expanded coverage in:
    • test_anyquotes_object:
      • Includes a multi-line '...' test case.
      • E.g. 'first' false\nstring 'second' → ensuring we detect multi‐line quotes.
    • test_anybrackets_object:
      • Verifies line‐based priority but also multi‐line bracket detection.
      • E.g., an open bracket ( on line 3, close ) on line 5, which used to fail.

Limitations / Future Enhancements

  • Escaping: The current approach for quotes is naive and doesn’t handle escape sequences (like ") or advanced parser logic. For deeper correctness, we’ll need more advanced logic, this is also not supported in the original mini.ai plugin so it is a known issue that won't be attended for now.

Important Notes

  • Fix for the bug: vim: ci' and ca' motions doesn't work when there is only one quoted string #23889 this PR addresses that bug specifically for the AnyQuotes text object. Note that the issue still remains in the built-in motions (ci', ci", ci`).
  • Caret Position Differences: The caret position now slightly deviates from Vim’s default behavior. This is intentional. I aim to closely mimic the mini.ai plugin. Because these text objects are optional (configurable via vim.json), this adjusted behavior is considered acceptable and in my opinion the new behavior is better and it should be the default in vim. Please review the new tests for details and context.
  • Improved Special Cases: I’ve also refined how “false strings” in the middle and certain curly-bracket scenarios are handled. The test suite reflects these improvements, resulting in a more seamless coding experience overall.

References:

Thank you for reviewing these changes!

Release Notes:

  • Improve logic of aq, iq, ab and ib motions to work more like mini.ai plugin

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Feb 4, 2025
@zed-industries-bot
Copy link

zed-industries-bot commented Feb 4, 2025

Messages
📖

This PR includes links to the following GitHub Issues: #23889
If this PR aims to close an issue, please include a Closes #ISSUE line at the top of the PR body.

Generated by 🚫 dangerJS against 500e525

@oca159
Copy link
Contributor Author

oca159 commented Feb 4, 2025

hi @ConradIrwin

I’ve been working on a new version of the AnyQuotes and AnyBrackets text objects to more closely match the behavior of the original plugin for both quotes and brackets. It took me longer than expected, but now it behaves exactly as the original mini.ai does in all test scenarios—especially the tricky ones involving “false” strings and multi-line brackets.

New Tests and Behaviors

  1. False string in the middle
    Previously, using cib in a situation like:

"first" false | string "second"

would improperly merge both quoted sections into something like:

"first"|"second"

Now, just like the real mini.ai plugin, the “false” string in the middle is correctly ignored, resulting in:

"first" false string "|"

This behavior aligns with what the original mini.ai does, avoiding accidental merging of unrelated quotes.

  1. Cover‐or‐Next Algorithm
    The plugin now uses the “cover‐or‐next” approach for brackets and quotes, first checking if the caret is covered by a bracket or quote on the same line and only then expanding the search to surrounding lines. For example, previously:
{
    |print("hello")
}

pressing cib would delete everything inside the curly braces, leaving:

{
    |
}

which could be quite frustrating. Now, consistent with the original mini.ai, it only targets the parentheses around "hello":

{
    print(|)
}

This is much more intuitive when working with nested brackets on multiple lines.

Summary

I’ve introduced new helper functions to achieve these behaviors and added test cases illustrating scenarios where the old implementation fell short. Please take a look at the new tests to see how each scenario is handled. I believe this new logic provides a much more seamless and predictable experience, matching the original mini.ai plugin’s design.

Let me know what you think or if you have any questions!

Thanks!

@maxdeviant maxdeviant changed the title feat: update anyquotes and anybrackets to behave like mini.ai plugin vim: Update anyquotes and anybrackets to behave like mini.ai plugin Feb 4, 2025
@oca159
Copy link
Contributor Author

oca159 commented Feb 4, 2025

I just want to take a moment to apologize for the multiple iterations regarding this feature. As a big fan of the mini.ai plugin in Neovim, I truly believe this feature will be loved by many developers, just as much as I do. That’s why I’m putting so much effort into making it happen, I’m really excited about its potential! Thank you for your patience and understanding :)

I highly recommend everyone give this feature a try, I’m confident it will significantly enhance your coding experience. You won’t regret it! :)

@ConradIrwin
Copy link
Member

@oca159 nice! Thanks for this, and I like the new behavior.

That said, we should not be doing anything O(n) in the size of the buffer if we can help it; and so I think we should use tree-sitter to do this.

Luckily, Zed treats quotes like brackets for the purposes of this, so I think we can use the same structure of method for both:

  • Use bracket_ranges to find any bracket ranges within the current line, filtered by quote-like ", ', "`" (or not for brackets)
  • If none, use innermost_enclosing_bracket_ranges to find anything multi-line we're inside.

Happy to pair with you on this: https://cal.com/conradirwin/pairing

@ConradIrwin ConradIrwin self-assigned this Feb 4, 2025
@oca159
Copy link
Contributor Author

oca159 commented Feb 4, 2025

Hi @ConradIrwin,

I’ve scheduled a meeting with you to discuss the Tree-sitter implementation, specifically focusing on the use of bracket_ranges and innermost_enclosing_bracket_ranges. I’ve been experimenting with these functions but haven’t had success yet. In the meantime, I’ll continue refactoring my code to better utilize them.

Looking forward to our discussion, and thanks for your help!

@oca159 oca159 force-pushed the ocordova/improve-any-brackets-and-any-quotes branch 2 times, most recently from 6c32b76 to a208dd9 Compare February 11, 2025 23:28
@oca159
Copy link
Contributor Author

oca159 commented Feb 11, 2025

Hi @ConradIrwin,

I’ve updated the PR. Could you please review it when you have a moment?

Additionally, I wanted to highlight that the tests for quotes are currently failing due to the bug I mentioned earlier in today’s meeting. It appears that brackets_range is not functioning correctly with single and back quotes. Let me know if you need further details or if there’s anything specific I should address.

Thanks!

@oca159
Copy link
Contributor Author

oca159 commented Feb 12, 2025

It seems that modifying the Tree-sitter queries in brackets.scm for TypeScript to the following resolves the issue:

("(" @open ")" @close)
("[" @open "]" @close)
("{" @open "}" @close)
("<" @open ">" @close)
("\"" @open "\"" @close)
("'" @open "'" @close)
("`" @open "`" @close)

With this change, it now works correctly for all types of quotes (double, single, and backticks).

Additionally, it was necessary to update the VimTestContext initialization to:

let mut cx = VimTestContext::new_typescript(cx).await;

To ensure the tests pass, the implementation of new_typescript should also be adjusted accordingly.

    pub async fn new_typescript(
        capabilities: lsp::ServerCapabilities,
        cx: &mut gpui::TestAppContext,
    ) -> EditorLspTestContext {
        let mut word_characters: HashSet<char> = Default::default();
        word_characters.insert('$');
        word_characters.insert('#');
        let language = Language::new(
            LanguageConfig {
                name: "Typescript".into(),
                matcher: LanguageMatcher {
                    path_suffixes: vec!["ts".to_string()],
                    ..Default::default()
                },
                brackets: language::BracketPairConfig {
                    pairs: vec![
                        language::BracketPair {
                            start: "{".to_string(),
                            end: "}".to_string(),
                            close: true,
                            surround: true,
                            newline: true,
                        },
                    ],
                    disabled_scopes_by_bracket_ix: Default::default(),
                },
                word_characters,
                ..Default::default()
            },
            Some(tree_sitter_typescript::LANGUAGE_TYPESCRIPT.into()),
        )
        .with_queries(LanguageQueries {
            brackets: Some(Cow::from(indoc! {r#"
                ("(" @open ")" @close)
                ("[" @open "]" @close)
                ("{" @open "}" @close)
                ("<" @open ">" @close)
                ("'" @open "'" @close)
                ("`" @open "`" @close)
                ("\"" @open "\"" @close)"#})),
            indents: Some(Cow::from(indoc! {r#"
                [
                    (call_expression)
                    (assignment_expression)
                    (member_expression)
                    (lexical_declaration)
                    (variable_declaration)
                    (assignment_expression)
                    (if_statement)
                    (for_statement)
                ] @indent

                (_ "[" "]" @end) @indent
                (_ "<" ">" @end) @indent
                (_ "{" "}" @end) @indent
                (_ "(" ")" @end) @indent
                "#})),
            ..Default::default()
        })
        .expect("Could not parse queries");

        Self::new(language, capabilities, cx).await
    }

After applying all the changes, I confirmed that the implementation now works correctly for all types of quotes. This means that the new Tree-sitter-based approach requires updating the brackets.scm queries for every language that needs to support this feature.

I’m not sure if there’s a more efficient way to handle this, so I’d appreciate your thoughts or suggestions.

Additionally, I noticed an issue with bracket_ranges when dealing with nested strings. For example:

Input:
This is a "nested 's|tring'"

Expected result after ciq:
This is a "nested '|'"

Actual result:
This is a "|"

It seems the current implementation doesn’t handle nested strings properly. Let me know if you’d like me to investigate this further or if you have any ideas on how to address it. That's all my concerns about the new implementation using Tree-Sitter

@oca159 oca159 force-pushed the ocordova/improve-any-brackets-and-any-quotes branch 4 times, most recently from fe6c640 to f71f116 Compare February 12, 2025 01:07
@@ -1972,9 +2063,36 @@ mod test {

#[gpui::test]
async fn test_anyquotes_object(cx: &mut gpui::TestAppContext) {
let mut cx = VimTestContext::new(cx, true).await;
let mut cx = VimTestContext::new_typescript(cx).await;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated to use new tree sitter queries that includes simple and back quotes

@@ -3,3 +3,5 @@
("{" @open "}" @close)
("<" @open ">" @close)
("\"" @open "\"" @close)
("'" @open "'" @close)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a better way to include any type of quotes as brackets for all languages?

Otherwise this code change should be included in all the brackets.scm file of all the languages

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly not that I know of.

@oca159 oca159 force-pushed the ocordova/improve-any-brackets-and-any-quotes branch 2 times, most recently from 191ae5a to b2ee789 Compare February 12, 2025 02:19
@@ -2359,13 +2102,13 @@ mod test {
(
"c i q",
"This is a \"simple 'qˇuote'\" example.",
"This is a \"simple 'ˇ'\" example.",
"This is a \"ˇ\" example.", // Not supported by tree sitter queries for now
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nested quotes are not supported by tree sitter queries for now, i need help to see if it's possible to support it using tree sitter queries

Mode::Insert,
),
(
"c a q",
"This is a \"simple 'qˇuote'\" example.",
"This is a \"simple ˇ\" example.", // same mini.ai plugin behavior
"This is a ˇ example.", // Not supported by tree sitter queries for now
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nested quotes are not supported by tree sitter queries for now, i need help to see if it's possible to support it using tree sitter queries

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is supportable with tree-sitter unfortunately :/.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(well, it is; but it would require changing the grammars for each language)

@oca159 oca159 force-pushed the ocordova/improve-any-brackets-and-any-quotes branch from b2ee789 to 0e83b78 Compare February 12, 2025 17:52
@oca159 oca159 force-pushed the ocordova/improve-any-brackets-and-any-quotes branch from 0e83b78 to a1af5ca Compare February 12, 2025 19:41
@oca159 oca159 force-pushed the ocordova/improve-any-brackets-and-any-quotes branch from ff0df2b to 38c0586 Compare February 12, 2025 23:24
// To support f-strings in python
if snapshot.chars_at(start_off).next() == Some('f') {
start_off += 1;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the mini.ai behavior is wrong in this case, and we should stick with start_off. Given an example like print(f"a|aa"), daq should give you print(|) not print(f|)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've noticed this is consistent with the behavior in the original mini.ai implementation. The system appears to ignore any characters that precede quotation marks. I understand your point about this limitation, and I'll investigate ways to improve this behavior in the code

around: bool,
) -> Option<Range<DisplayPoint>> {
find_any_delimiters(map, display_point, around, |buffer, start| {
matches!(buffer.chars_at(start).next(), Some('\'' | '"' | '`' | 'f'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use the last character of the query for now (so it works for f" in python, r" in rust, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense!

return Some(pair.start..pair.end);
} else {
let new_start = pair.start.column() + 1;
let new_end = pair.end.column() - 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bracket queries should give you a range for the start and end delimiter; so around becomes start_range.start..end_range.end and !around is start_range.end..end_range.start.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(it's also worth knowing that it's generally unsafe to + 1 on an offset into a string in rust, because string indexes must always align to utf8 bytes or string range queries panic; If you do need that in Zed, you can, but you need to clip the Point against the display map).

@ConradIrwin
Copy link
Member

Thanks for this! I've left a few comments inline. I also noticed that python docstrings weren't handled correctly by vaq, but I'm not sure why.

# for a docstring, `vaq` should select the whole string
«"""
  a
"""»

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants