x/tools/gopls: add go:embed support

[hamidfzm]

Is your feature request related to a problem? Please describe.
Go embed feature introduced in version 1.16. Currently, go-vscode treats text like //go:embed app as a simple comment.

Describe the solution you'd like
Provide text highlighting or navigation by CTRL+Clicking on these comments

Describe alternatives you've considered
Goland provides this feature in its recent versions:

Additional context
VS code view

It would be really nice if we have this feature in my favorite code editor.

[findleyr]

This could be implemented with a code action+associated command that invokes window/showDocument. It could also be implemented with jump-to-definition+hover. I am not sure which is best, but this seems to belong in the gopls issue tracker.

[hyangah]

For individual files, code action associated with window/showDocument or textDocument/documentLink would work. We still need a solution for handling folder type resources. For VSCode, I filed an issue microsoft/vscode#141564.

[ansaba]

List of improvements

• checks for the "embed" import
• Report it as semantic for highlighting
• The directive must immediately precede a line containing the declaration of a single variable. Only blank lines and ‘//’ line comments are permitted between the directive and the declaration.

EDIT (rfindley): cross out semantic highlighting, as I think we should punt on that for now

[findleyr]

We should also include a quick-fix to add the missing "embed" import.

[findleyr]

@vikblom expressed interest in picking this up.

I think the semantic highlighting support is a bit tricky to implement, so let's separate that bit out for now.

Viktor, if you are interested you could do the following:

• Update the embeddirective
  analyzer to check that the embed directive immediately precedes a line containing the declaration of a single variable. Only blank lines and ‘//’ line comments are permitted between the directive and the declaration.
• Update the analyzer to include a suggested fix to add the missing "embed" import.

I recommend doing this in two separate CLs. Adding the suggested fix is a little subtle (we should re-use the existing gopls.add_import custom command). So let's discuss the second item after the first is done.

I'll tentatively assign you. Let me know if you don't think you'll be able to work on this.

[findleyr]

Well, @vikblom it turns out I cannot assign you, so I assigned myself as your proxy :)

[vikblom]

@findleyr thanks, makes sense to isolate the changes.
I'll give the embeddirective analyzer a look and circle back if I get stuck or have a CL in mind.

[gopherbot]

Change https://go.dev/cl/504295 mentions this issue: internal/lsp/analysis: embed directive analyzer check following decl

[vikblom]

Hi @findleyr, with the CL above merged I've started looking at suggesting a fix from the analyzer.

analysis.Diagnostic has a field for SuggestedFixes which seems straight forward enough. You mentioned gopls.add_import, best I can tell that command is backed by source.AddImport. Am I aiming for the right things?

Are there any requirement on placement for the import? I'm used to placing blank imports like _ "embed" last.

Edit: I've also played around with a source.Analysis.Fix function instead to avoid the import cycle.

[findleyr]

Hi @vikblom yes, it sounds like you've got the right idea. There are two aproaches: either compute the edits inline as part of the analyzer, or defer them to later using the source.Analyzer.Fix indirection. The benefit of the latter is that you avoid the performance penalty of pre-computing the edits, and you have access to other facilities that aren't part of the analysis framework, such as source.AddImport. For that reason, I suggest you do the latter, and add a source.SuggestedFixFunc that calls AddImport.

I think there's a problem though, which is that the presence of source.Analyzer.Fix signals that all diagnostics produced by the analyzer can be fixed (which means every diagnostic will have a "quick fix" in VS Code). We don't want that, as the only diagnostics we can fix are those of the missing import. That means we'll have to invent a way to filter "fixable" diagnostics. Perhaps source.Analyzer can have an isFixable func(*source.Diagnostic) bool field. And then in cache.toSourceDiagnostic, build the source.Diagnostic first and pass to isFixable, to see if a quick-fix should be added.

[vikblom]

Thanks @findleyr that approach works great 👍

I've added a TestLSP testcase with a golden file verifying that the import is added. Do you have any suggestion or tips on how to test the negative case? (That the other kind of reports (incorrect decl) does not have a code action.)

edit: I found exiting regtest tests checking for false positive quickfixes and made one for this case.

[gopherbot]

Change https://go.dev/cl/509056 mentions this issue: gopls/internal/lsp: Add SuggestedFix for embeddirective Analyzer.

[vikblom]

I noticed that I haven't kept all the docs up to speed, for example analyzers.md. That should probably be corrected as part of this issue.

[findleyr]

@vikblom the analyzer doc is auto-generated, and there is a test to enforce this: cd gopls && go generate .

[vikblom]

@findleyr I see! Then it's the analyzers Doc field that (maybe) needs an update. I was thinking it should mention the new variable check and suggested fixes, do you agree?

[gopherbot]

Change https://go.dev/cl/513895 mentions this issue: gopls/internal/lsp/source: refresh embeddirective analyzer docs

[vikblom]

Hi @findleyr, is there a path forward with semantic highlighting here, given that the vscode ticket was closed by triage?

I tried out a naive textDocument/documentLink solution passing file:// URIs. Emacs could handle both files and directories. VSCode only accepted files. Does it makes sense to use that as a opt-in or base case (only files) feature?

You mentioned code action+associated command and jump-to-definition+hover above, which seems doable. However if VSCode requires specific (or new) mechanisms to handle directories, this feature might spread to codebases (vscode or vscode-go) I'm not familiar with

[findleyr]

Hi @vikblom, very sorry for the slow response on this.

Let's not worry about semantic tokens for now, I think that's a low priority. We can treat highlighting //go: directives as a separate project.

We discussed this in our triage meeting, and I think jump-to-definition+hover is the best way to implement this feature, as it will be supported by the greatest number of LSP clients. For directories, perhaps we should just pick an arbitrary file in the directory?

[vikblom]

An arbitrary file could be unexpected as a user. But I don't see any alternatives if this is a limitation of LSP clients. I agree it is a good place to start.

I'll try to put something together and post a CL.

[findleyr]

Great, thanks for sticking to this!

[gopherbot]

Change https://go.dev/cl/530195 mentions this issue: gopls/internal/lsp: hover over embed directives.

[gopherbot]

Change https://go.dev/cl/531775 mentions this issue: gopls/internal/lsp: go to definition from embed directive

[vikblom]

Hi @findleyr, what do you think is the next step? Would tackling semantic highlighting make sense?

[findleyr]

@vikblom if we're going to highlight go:embed, I think we should highlight all go: directives. Is that something you'd be interested in?

If so, I think we should perhaps make that a separate issue, and close this one.

[vikblom]

Sure, I'd be glad to take a look directive highlights!

Thanks again for all the help in this issue.

[findleyr]

Great! Opened https://go.dev/issue/63538.

Will close this then.