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

Completion: fix bad trailing } completion #378

Merged
merged 6 commits into from
Jul 27, 2023

Conversation

vovakulikov
Copy link
Contributor

Fixes bad completion case https://github.com/sourcegraph/cody/discussions/358#discussioncomment-6531096

After hours of catching corner cases, I think we can't rely on regexp-based shouldIncludeClosingLine logic here. Counting brackets doesn't provide much value when we don't know about the structure of the expression where we try to insert completions. This PR just tries to handle the simple case with an empty completion line with a closed bracket symbol and cut off this completion.

@philipp-spiess I don't think I fully understand how this is possible, but unit tests tell me that completion should be cut off, but vscode manual test shows that this problem is still there. Let me know if you have a minute for a quick pair on this.

Test plan

  • Try to force } completion (see original GitHub discussion thread for the example)
  • Check that there is no } compleation

@philipp-spiess
Copy link
Contributor

The shouldIncludeClosingLine is only relevant for the multi-line completion truncation, where we find out if we should include one line below the current indentation. I would not base the solution to this problem in that code (because when we later use tree sitter to parse the response, we'd want it to emit everything for the current function scope and that would also include the closing tags) and we then do a follow-up step to try and merge it into the existing code (which is what the suffix matching tries to do now).

So instead, I'd see if we can fix this in the suffix matching logic only. WDYT?

My initial idea here is that this should 100% be a case of "the completion matches the next line". The only way it doesn't is because the completion can not delete code and we always append the same line prefix whitespace (even though Claude doesn't know about).

My two ideas are:

  • We handle the case of potential "dedent" from Claude specifically for the next line logic. That would mean that if you compare the first line of the completion with the suffix, we do not match on the exact indentation anymore.
  • We try to be smart and make more sense of Claude's response. I think we can't really rely on the whitespace that's why I’m not sure this works but if the completion that Claude sends us + the prompt we send it does not have the same indentation as our current prefix, we should maybe discard the completion? This would also fix https://github.com/sourcegraph/cody/discussions/358#discussioncomment-6519515 where Claude is not emitting a \n in the completion but the current suffix already has a \n and we just trim it for the Claude prompt.

@vovakulikov vovakulikov marked this pull request as ready for review July 26, 2023 18:35
@vovakulikov vovakulikov requested a review from a team July 26, 2023 18:35
@vovakulikov
Copy link
Contributor Author

As we discussed with @philipp-spiess, this PR doesn't try to do anything about bracket counting and parsing and just aims to catch some narrow cases with single close bracket completion.

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.

LGTM! Just one comment inline :)

vscode/src/completions/text-processing.ts Outdated Show resolved Hide resolved
@vovakulikov vovakulikov merged commit d07fa9b into main Jul 27, 2023
7 checks passed
@vovakulikov vovakulikov deleted the vk/fix-trailing-bracket-case branch July 27, 2023 15:24
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