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

feat: request autocompletion without typing a letter #310

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

Conversation

juleswritescode
Copy link
Collaborator

@juleswritescode juleswritescode commented Apr 4, 2025

So far, we were relying on complete tree-sitter trees for autocompletion.

This works fine if you type the first letter of a node, say:
select * from u|
Now, the u is considered a table node, and we can look for tables.

But this PR makes it such that we can also request completions if we haven't typed the u yet.

It does so by checking whether the tree is valid and, if not, injecting a token to fix the tree-sitter tree.

Additionally, in the case of select * from |, the workspace would consider the cursor to be outside the statement. I added a little offset so the cursor still matches a statement if it's 2 characters after the last token.

@juleswritescode juleswritescode changed the title so far… improve auto completions Apr 4, 2025
@juleswritescode juleswritescode marked this pull request as ready for review April 11, 2025 10:49
@juleswritescode juleswritescode changed the title improve auto completions feat: request autocompletion without typing a letter Apr 11, 2025
Copy link
Collaborator

@psteinroe psteinroe left a comment

Choose a reason for hiding this comment

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

looks good to me overall. just would like to double check if we really need to instantiate the tree sitter parser with every request 😢. can't we just mutate the tree?

also: we will have a bunch of conflicts, and a few things will need to be handled differently. but no issues, just that we should merge yours first so I can fix the merge conflicts with mine.

sql.push(c);
}

let mut parser = tree_sitter::Parser::new();
Copy link
Collaborator

Choose a reason for hiding this comment

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

isnt there a way to modify the tree sitter tree instead of reparsing the string?

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we can pass the passer down to here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think editing the node isn to possible, so maybe we should try to pass the existing tree sitter parser down.

@juleswritescode
Copy link
Collaborator Author

juleswritescode commented Apr 11, 2025

looks good to me overall. just would like to double check if we really need to instantiate the tree sitter parser with every request 😢. can't we just mutate the tree?

I think I would shy away from that – we would mutate the tree that is also used for other features… seems like a source of very annoying bugs :/

Also, keep in mind that:

  1. the statement is likely pretty small and the parsing should happen quickly and
  2. that we don't need to create a tree and will reuse the existing one if the user has already written a letter.

I added some benchmarks – on my machine, reusing the old treesitter takes about 500 nanoseconds, and creating a new one about 5-150 microseconds depending on the size of the sql.

Screenshot 2025-04-11 at 18 32 12

I mean, I can play a Counterspell when I see a hakbal in about that time, but I wouldn't mind waiting that long for completions

@juleswritescode
Copy link
Collaborator Author

juleswritescode commented Apr 12, 2025

@psteinroe Curious about your opinion: I tried moving the GetCompletionsMapper into the completions crate, but then I couldn't access the parser.cst_db because that's private. But it's weird to write feature-functions and comments into the parsed_document I think :/

We could make the parser.cst_db public, or provide a parser.get_tree() method, or leave the mappers in the crate… what do you think?

@psteinroe
Copy link
Collaborator

psteinroe commented Apr 12, 2025

ah sorry, i didn't mean to move the mapper there. I was thinking to clone or copy the tree sitter parser to skip the initialisation costs.

Will look at the pr later, thanks for trying! 🙏🏼

Copy link
Collaborator

@psteinroe psteinroe left a comment

Choose a reason for hiding this comment

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

just one detail, rest lgtm! I would say we go with constructing a new parser with every request for now and then check it later if we get problems.

@@ -24,3 +28,167 @@ impl IntoIterator for CompletionsResult {
self.items.into_iter()
}
}

pub(crate) fn get_statement_for_completions<'a>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

why can't this be implemented as a Filter in parsed_document?

Copy link
Collaborator Author

@juleswritescode juleswritescode Apr 12, 2025

Choose a reason for hiding this comment

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

because I need to peek into the next statement for the overlap check :/
If you have other ideas, let me know!

Copy link
Collaborator

Choose a reason for hiding this comment

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

makes sense. maybe we can find a good abstraction for this but for now lets roll with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants