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(amazonq): implement basic server for standard LSP completion API #739

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

viktorsaws
Copy link
Contributor

@viktorsaws viktorsaws commented Jan 21, 2025

Problem

Q/codewhisperer does not support LSP standard "textDocument/completion" method #724

Solution

  • Added simple experimental AmazonQCompletionServer implementation to support textDocument/completion protocol. More docs available in server/aws-lsp-codewhisperer/src/experimental/README.md.
  • Added new testing app in app/aws-lsp-q-dev-completions, which includes AmazonQCompletionServer and device SSO token provider among other servers.

To test the server: fetch this PR or https://github.com/viktorsaws/language-servers/tree/experimental_lsp_completions branch and rebuild the project:

npm install && npm run package

Bundled language server is available at ./app/aws-lsp-q-dev-completions/build/amazon-q-completions-and-sso.js path.

6-fcf9be50f3

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@viktorsaws viktorsaws requested a review from a team as a code owner January 21, 2025 17:38
@@ -0,0 +1,26 @@
# AmazonQCompletionServer
Copy link
Contributor

Choose a reason for hiding this comment

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

given the layout of the rest of this project, the experimental/ directory seems unintuitive. how does it map to the existing pattern of the repo (which has client/ and language-server/ at the top level)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was not sure where to place it, given it's simple implementation. Moving to language-server would be more appropriate.

@@ -0,0 +1,41 @@
var path = require('path')

const baseConfig = {
Copy link
Contributor

Choose a reason for hiding this comment

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

why doesn't this inherit from some other part of this repo? does each "server" have its own webpack config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, every package like this in app directory could be built using it's own config. Moving it out to common shared config would be an optimization, I decided to keep it more standalone for now.

})
}

export const AmazonQCompletionServer =
Copy link
Contributor

Choose a reason for hiding this comment

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

why isn't this part of the existing codewhispererserver.ts ? can clients opt-in to the experiment by sending a settings flag on init?

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 wanted to avoid polluting already overloaded codewhispererserver.ts with this implementation. I'd look into toggling this completion behind a flag.

Copy link
Contributor

@justinmk3 justinmk3 Jan 24, 2025

Choose a reason for hiding this comment

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

I wanted to avoid polluting already overloaded codewhispererserver.ts

I'm more worried about the pollution of numerous duplicate modules, configs, sub-packages, which interact in a way that must be imagined, because code-navigation tools can't make sense of all these sub-packages and cross-configs. That's much harder to cognitively consume than 1 package with lots of code (which can use shared base-classes/modules).

But maybe I missed what you meant :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just support both approaches at the same time? Clients that send us textdocument/completion requests will get answers with code-with-references filtered out. Clients that ask for a richer response including references, will get the enhanced response. They could use exactly the same handler without any duplication. We only need a small additional filter and transformation to make sure the params and suggestions match the right shape.

Are we worried clients might be sending both at the same time? They shouldn't, at least not to the same LSP server. While we could try to avoid this behavior with turning capabilities on/off dynamically, or adding some additional settings, it seems to me that the LSP server is responsible for answering client's requests, not determining whether the client's request is valid or not.

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 (untested) code work instead?

        const onCompletionHandler = async (params: CompletionParams, token: CancellationToken) => {
            const inlineCompletionParams: InlineCompletionWithReferencesParams = {
                ...params,
                context: {
                    ...params.context,
                    triggerKind: InlineCompletionTriggerKind.Invoked,
                },
            }

            const inlineCompletionResult = await onInlineCompletionHandler(inlineCompletionParams, token)

            return {
                isIncomplete: false,
                items: inlineCompletionResult.items.filter(item => !item.references).map(item => {
                    return {
                        label: item.insertText.toString()
                    }
                }),
            }
        }

},
newText: item.insertText as string,
},
kind: CompletionItemKind.Text,
Copy link
Contributor

@justinmk3 justinmk3 Jan 27, 2025

Choose a reason for hiding this comment

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

Discussion with Neovim plugin authors leads me to believe that "snippet" is most appropriate for multiline completions.

I'm not sure if that means we should usekind: CompletionItemKind.Snippet here, but at least it means we should set insertTextFormat = InsertTextFormat.Snippet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually based on feedback from blink.nvim users, usekind: CompletionItemKind.Snippet seems to work better for them, while insertTextFormat: InsertTextFormat.Snippet risks activating snippet "placeholder" handling (snippet mode).

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.

3 participants