When using go-languageserver, also use code completion feature #1607

m90 · 2018-04-05T18:30:00Z

Instead of using go-code, use go-languageserver for providing
code completion results when enabled. Solves #1593

Instead of using go-code, use go-languageserver for providing code completion results when enabled. Solves #1593

m90 · 2018-04-05T18:31:39Z

I wonder if there is some way for the extension to know if the installed version of go-languageserver is recent enough to support the flag?

In case executing the binary with an unknown flag does not yield the -gocodecompletion flag in the help output, we assume the version installed is out of date and prompt for updating

m90 · 2018-04-06T10:44:15Z

src/goInstallTools.ts

 }
- return langServerAvailable;
+
+ // we execute the languageserver using -help so that it will fail and


This is pretty terrible, but it's the best I could come up with in case we want to make sure an up-to-date version of the language server is installed.

Alternatively we could call go-langserver -version, but I don't really know what do to with the output as it would currently just print sth like v2-dev: https://github.com/sourcegraph/go-langserver/blob/dd4c619b53ae9d18c94e372d23d8710e59f0766b/main.go#L41

…ture

OneOfOne · 2018-05-01T21:23:57Z

This doesn't seem to work at all, I can see go-langserver -mode=stdio -gocodecompletion running but there are no completions and nothing in the console.

OneOfOne · 2018-05-01T21:38:40Z

Ignore my previous comment, this is related to nsf/gocode#510, I opened a ticket sourcegraph/go-langserver#268.

I can confirm this works perfectly with go 1.10.

@ramya-rao-a ping

primalmotion · 2018-05-02T21:37:49Z

works like a charm for me

m90 · 2018-05-14T12:40:17Z

@ramya-rao-a Any input on this one?

ramya-rao-a · 2018-05-15T18:12:15Z

@m90 Do you know the reason why completions are not supported by default by the language server?
Is it because of the memory burden? The help for the -gocodecompletion flag has the comment enable completion (extra memory burden).

If this indeed is the case, then I'd prefer for users to opt-in to use the completions from the language server instead of us providing as a default.

cc @keegancsmith and @slimsag for input on why completions are not supported by default by the language server.

ramya-rao-a · 2018-05-15T18:16:11Z

src/goInstallTools.ts

+ vscode.window.showInformationMessage('Reload VS Code window after updating the Go language server');
+ }
+
+ return langserverSupportsCompletion;


Here, checkLanguageServer will return false for users with older version of the language server stopping them from using the language server altogether.

Instead, create a new function getLanguageServerFlags that returns all supported flags. Then in goMain, you can use the result to pass the appropriate flags. This will help in the future when we have other flags.

Instead, create a new function getLanguageServerFlags that returns all supported flags.

Just to double check: for getLanguageServerFlags you envision sth that reads the output from calling the naked command just above? Or do we already do sth similar elsewhere?

ramya-rao-a · 2018-05-15T18:17:25Z

src/goMain.ts

@@ -96,7 +96,7 @@ export function activate(ctx: vscode.ExtensionContext): void {
 'go-langserver',
 {
 command: getBinPath('go-langserver'),
- args: ['-mode=stdio', ...langServerFlags],
+ args: ['-mode=stdio', '-gocodecompletion', ...langServerFlags],


Pass the func-snippet-enabled flag as per the setting go.useCodeSnippetsOnFunctionSuggest

ramya-rao-a · 2018-05-15T18:21:16Z

src/goMain.ts

@@ -110,6 +110,7 @@ export function activate(ctx: vscode.ExtensionContext): void {

 ctx.subscriptions.push(c.start());


Since its possible for users to run the older versions of the language server where completions are not supported, add the below to register the completion provider from the extension

c.onReady().then(() => { if (c.initializeResult && c.initializeResult.capabilities && !c.initializeResult.capabilities.completionProvider) { ctx.subscriptions.push(vscode.languages.registerCompletionItemProvider(GO_MODE, new GoCompletionItemProvider(), '.', '\"')); } });

m90 · 2018-05-15T18:22:27Z

@ramya-rao-a That's a good question and a valid concern. I do have no idea why completion is behind a flag though. All I know is that when running locally it feels faster and more lightweight, but then again that's very likely to be very subjective.

Maybe the lovely Sourcegraph people know why it is the way it is?

… when unsupported

m90 · 2018-05-15T19:26:32Z

@ramya-rao-a I think this should reflect the requested changes now, feel free to check and let me know if you feel it'd be nicer to put this behind a preference.

ramya-rao-a · 2018-05-15T22:31:09Z

src/goMain.ts

+ ];
+
+ let applicableFlags = configuredLangServerFlags.filter(f => {
+ return supportedLangServerFlags.some(supported => f.startsWith(supported));


why startsWith and not === ?

Because for example the supported flags will contain -func-snippet-enabled but we also pass a value like -func-snippet-enabled=false.

Ah! Makes sense

ramya-rao-a · 2018-05-15T22:32:48Z

I've logged a proposal for the language server to use the initialziationOptions instead of flags to drive features. See sourcegraph/go-langserver#280

Let's wait on what our friends at sourcegraph have to say there before moving forward here

ramya-rao-a · 2018-05-22T15:04:04Z

@m90 Can you update this PR as per the below points?

Merge from master. There has been quite some changes there.
Use the InitializationOptions to enable the completion feature instead of the flags. See Use InitializationOptions instead of flags to drive capabilities sourcegraph/go-langserver#280
Include auto-complete as an experimental feature in the go.languageServerExperimentalFeatures setting such that users can disable this feature but still use the language server. This is helpful as completions for unimported packages and symbols from unimported packages are not yet supported by the language server

m90 · 2018-05-22T15:19:37Z

@ramya-rao-a Sure thing. When are you planning to do the next release of the extension? I'm a little swamped right now, so it might take until the weekend or so.

ramya-rao-a · 2018-05-22T15:46:49Z

@m90 In that case, no worries. I can make the changes in your branch and create a vsix file. You can then install that vsix and help with testing?

primalmotion · 2018-05-22T16:34:31Z

I can test if you like. I’m on that branch anyway

m90 · 2018-05-22T17:34:34Z

@ramya-rao-a Sounds good, thanks. Let me know when I can give things a test drive.

m90 · 2018-05-25T19:47:03Z

@ramya-rao-a So I have been able to unswamp myself and would be happy to still do the requested updates in the coming days. Does that sound like a plan to you or have you already started updating things?

ramya-rao-a · 2018-05-25T20:24:03Z

@m90 I havent started :) so go ahead make the updates

m90 · 2018-05-26T10:18:05Z

@ramya-rao-a I pushed the requested changes. Tbh I am not entirely sure how the entire graceful fallback mechanism works now when someone has an older version of go-langserver installed, so in case I missed something here, let me know and I will update accordingly. With an up-to-date language server it seems to work like a charm for me.

ramya-rao-a

Main comment in the PR is that we can revert the changes made to support the flag and assume that the user has the latest version of the language server if they have enabled the auto-complete feature.

Other than that, just small changes needed. Thats all.

Thanks for all your work on this!

ramya-rao-a · 2018-05-27T15:57:39Z

src/goInstallTools.ts

-export function checkLanguageServer(): boolean {
+// If langserver needs to be used, and is installed, this will return the list of supported flags
+// Returns null in all other cases
+export function checkLanguageServer(): string[] {


To keep things simple, lets go with the assumption that the auto-completion feature from language server is available only if the user has the latest language server i.e if the user's version of the language server respects the initalizationOptions sent.

Towards that end, we can revert the changes made to checkLanguageServer here

ramya-rao-a · 2018-05-27T15:59:14Z

src/goMain.ts

@@ -109,7 +117,7 @@ export function activate(ctx: vscode.ExtensionContext): void {
 revealOutputChannelOn: RevealOutputChannelOn.Never,
 middleware: {
 provideDocumentFormattingEdits: (document: vscode.TextDocument, options: FormattingOptions, token: vscode.CancellationToken, next: ProvideDocumentFormattingEditsSignature) => {
- if (languageServerExperimentalFeatures['format'] === true) {
+ if (languageServerExperimentalFeatures['format']) {


I specifically check for true here as user can provide anything in the settings and that value will make it through.

I wasn't allowed to use a non-boolean value when testing, which is why thought this was possible, but maybe that's only a warning? Undid this in any case.

ramya-rao-a · 2018-05-27T15:59:43Z

src/goMain.ts

@@ -118,6 +126,12 @@ export function activate(ctx: vscode.ExtensionContext): void {
 }
 );

+ c.onReady().then(() => {


There is already a c.onReady() below, can you combine this with that?

ramya-rao-a · 2018-05-27T16:03:17Z

src/goMain.ts

@@ -118,6 +126,12 @@ export function activate(ctx: vscode.ExtensionContext): void {
 }
 );

+ c.onReady().then(() => {
+ if (c.initializeResult && c.initializeResult.capabilities && !c.initializeResult.capabilities.completionProvider) {
+ ctx.subscriptions.push(vscode.languages.registerCompletionItemProvider(GO_MODE, new GoCompletionItemProvider(), '.', '\"'));


We would get inside this if block when the user had enabled the languageServerExperimentalFeatures['autoComplete'] setting, but the language server is old as well. In this case, we should notify the user that their version of the language server is old and does not support the auto-complete feature.

Same for the format feature.

m90 · 2018-05-27T16:58:22Z

src/goMain.ts

+ const capabilities = c.initializeResult && c.initializeResult.capabilities;
+ if (capabilities && !capabilities.completionProvider) {
+ ctx.subscriptions.push(vscode.languages.registerCompletionItemProvider(GO_MODE, new GoCompletionItemProvider(), '.', '\"'));
+ vscode.window.showInformationMessage('Your installed version of `go-langserver` is out of date and does not support code completion. Falling back to default behavior.');


@ramya-rao-a Is this an ok phrasing or would you also prompt the user to upgrade? It might also make sense to show a single generic message in case any fallback condition inside this function body was triggered.

@m90 I'll look into the update option later. I've pushed a commit to use generic message

m90 · 2018-05-27T17:03:08Z

src/goMain.ts

+ ctx.subscriptions.push(vscode.languages.registerCompletionItemProvider(GO_MODE, new GoCompletionItemProvider(), '.', '\"'));
+ vscode.window.showInformationMessage('Your installed version of `go-langserver` is out of date and does not support code completion. Falling back to default behavior.');
+ }
+
 if (!languageServerExperimentalFeatures['format'] ||


Should this check for !== true then, too?

Good point, I've pushed a commit to fix it

… message about outdated langserver version

When using go-languageserver, also use code completion

ad7b9f6

Instead of using go-code, use go-languageserver for providing code completion results when enabled. Solves #1593

Add naive feature detect for language server feature

6554ee6

In case executing the binary with an unknown flag does not yield the -gocodecompletion flag in the help output, we assume the version installed is out of date and prompt for updating

m90 commented Apr 6, 2018

View reviewed changes

Also consider case where -help might be added at some point in the fu…

55901d3

…ture

OneOfOne mentioned this pull request May 1, 2018

gocode is broken on current go git master #1645

Closed

ramya-rao-a suggested changes May 15, 2018

View reviewed changes

m90 added 2 commits May 15, 2018 20:34

update from master branch

f2d7910

Pass func snippet completion pref, fall back to non-server completion…

f901c06

… when unsupported

Check supported language server flags and filter before passing

4216a5a

ramya-rao-a reviewed May 15, 2018

View reviewed changes

Merge from upstream master

02ba529

Use initializationOptions for triggering langserver based features

00aae74

ramya-rao-a suggested changes May 27, 2018

View reviewed changes

m90 commented May 27, 2018

View reviewed changes

m90 and others added 3 commits May 27, 2018 19:20

Revert changes to langserver detection, merge onReady callbacks, show…

5bba166

… message about outdated langserver version

Refactoring

e59e7c1

stricter checks

c2df1f3

ramya-rao-a approved these changes May 27, 2018

View reviewed changes

ramya-rao-a merged commit 372b0fe into microsoft:master May 27, 2018

m90 deleted the server-completion branch May 27, 2018 18:02

		@@ -110,6 +110,7 @@ export function activate(ctx: vscode.ExtensionContext): void {

		ctx.subscriptions.push(c.start());

When using go-languageserver, also use code completion feature #1607

When using go-languageserver, also use code completion feature #1607

Conversation

m90 commented Apr 5, 2018

m90 commented Apr 5, 2018

Choose a reason for hiding this comment

OneOfOne commented May 1, 2018

OneOfOne commented May 1, 2018

primalmotion commented May 2, 2018

m90 commented May 14, 2018

ramya-rao-a commented May 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m90 commented May 15, 2018

m90 commented May 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramya-rao-a commented May 15, 2018

ramya-rao-a commented May 22, 2018 • edited Loading

m90 commented May 22, 2018

ramya-rao-a commented May 22, 2018

primalmotion commented May 22, 2018

m90 commented May 22, 2018

m90 commented May 25, 2018

ramya-rao-a commented May 25, 2018 • edited Loading

m90 commented May 26, 2018

ramya-rao-a left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m90 May 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramya-rao-a commented May 22, 2018 •

edited

Loading

ramya-rao-a commented May 25, 2018 •

edited

Loading

m90 May 27, 2018 •

edited

Loading