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

Autocomplete: truncate multiline completions by the next new sibling #1709

Merged
merged 4 commits into from
Nov 13, 2023

Conversation

valerybugakov
Copy link
Member

Context

Test plan

  1. Tested locally by generating multiple completions.
  2. Multiline truncation unit tests are ✅.

@valerybugakov valerybugakov requested a review from a team November 10, 2023 05:44
@valerybugakov valerybugakov marked this pull request as ready for review November 10, 2023 05:44
Copy link
Contributor

@philipp-spiess philipp-spiess left a comment

Choose a reason for hiding this comment

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

Let's gooo!

Comment on lines +118 to +121
// Remove the characters that are being replaced by the completion to avoid having
// them in the parse tree. It breaks the multiline truncation logic which looks for
// the increased number of children in the tree.
const textWithCompletion = prefix + insertText + suffix.slice(matchingSuffixLength)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this work in cases where the insertion contains multiple insert blocks and the matchingSuffixLength is not following right after another or am I misunderstanding something? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you elaborate a bit more or share an example?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking of a document state like this:

function bubbleSort(){

and a completion like this:

function bubbleSort(items: any[]): any[] {

But since suffix will be the suffix inside the document, it doesn't matter actually and it should work as expected.

// function test(one, two) {...}
//
// In this case, the logic will find the parameter list as a node with the updated number
// of siblings. We get the expected result if we start from the "{" node.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I understand the intend here but I don't think it's guaranteed that the start of the block will be in the first line?

function test()
function test (
  one,
  two,
) {
 //...
}

Is the worst case here that the truncation will stop with the multiple argument lines?

function test (
  one,
  two,
)

or how can I imagine this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is the worst case here that the truncation will stop with the multiple argument lines?

Yes, in the case of the multiline argument list, we will stop at it.

Comment on lines +48 to +51
const parsedFirstLine = parseCompletionFirstLineMemoized(firstLine, {
document,
docContext,
})
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand under which situations this is called multiple lines? Is this for when a multi-line completion has three examples but all of them start with the same first line?

Copy link
Member Author

Choose a reason for hiding this comment

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

It will be called multiple times from canUsePartialCompletion with the tree-sitter-powered truncation. So, you are correct that it's not called numerous times in the current implementation. Memozation will be used in the follow-up PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense!

@valerybugakov valerybugakov merged commit 360c4e1 into main Nov 13, 2023
14 checks passed
@valerybugakov valerybugakov deleted the vb/truncate-by-new-sibling branch November 13, 2023 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tree-sitter: implement the next-new-sibling multiline truncation approach
2 participants