Add new semantic token modifiers in gopls 0.17

[xzbdmw]

Is your feature request related to a problem? Please describe.
https://github.com/golang/tools/blob/master/gopls/doc/release/v0.17.0.md#semantic-token-modifiers-of-top-level-constructor-of-types

Describe the solution you'd like
Add those as default capabilities for vscode

interface, struct, signature, pointer, array, map, slice, chan, string, number, bool, and invalid

[findleyr]

CC @h9jiang

[hyangah]

@h9jiang fyi https://github.com/rust-lang/rust-analyzer/blob/d3bb15ecec7c1386767c45776fab497e8a22a30f/editors/code/package.json#L3060

[h9jiang]

A CL will be sent out for review so the user will be able to customize what kind to modifiers and types they want gopls to report. The result will looks like screenshot above.

[xzbdmw]

customize what kind to modifiers and types they want gopls to report

I feel this is not needed, because vscode users need to define the color of those "not in spec" modifiers, otherwise there are no visual differences.

[findleyr]

@xzbdmw that may the case, but there is no guarantee that other clients behave well in the presence of unexpected semantic tokens. The spec is (IMO) unclear about how servers are supposed to behave if clients do not advertise a particular token capability.

Furthermore, we already have some ad-hoc configuration to customize the behavior of semantic tokens, to address https://go.dev/issue/45753
https://github.com/golang/tools/blob/master/gopls/doc/settings.md#nosemanticstring-bool

I think it makes sense to deprecate the somewhat ad-hoc "noSemanticString" and "noSemanticNumber" settings, and replace them with a map[string]bool that turns token types on or off. Same for modifiers.

[gopherbot]

Change https://go.dev/cl/642077 mentions this issue: gopls/internal/golang: customize semantic token types and modifiers

[xzbdmw]

The spec is (IMO) unclear about how servers are supposed to behave if clients do not advertise a particular token capability

I do hope gopls sends them all by default ( I think that's what rust-analyzer does, I get tons of modifiers with a default setup), because right now, unless people take attention to release notes, and manually advertise these token capabilities, they never know those "out-of-spec" modifiers even exists.

[findleyr]

@xzbdmw @h9jiang OK, let's do this: let's add the new configuration map, but send all by default. If any clients have trouble with the new token types or modifiers, they can use the configuration map to disable the unexpected types.

Sound good?

[h9jiang]

By default, with only semanticTokens enabled in vscode, gopls will report all token types including string and number ..., will report all token modifiers including struct and pointer ....

Users can add additional configuration under ui.semanticTokenModifiers and ui.semanticTokenTypes to disable some of them without restarting the gopls or reload the vscode window.

Disable struct and pointer modifiers, then move the cursor to a different place in the same file and move back. The struct modifier will be taken out.

Disable number and string types, then move the cursor around.. The number and string will be taken out.

[h9jiang]

But there is a small issue with noSemanticNumber & noSemanticString. I feel like that is a more general question and should not be considered as blocker for this issue. See golang/go#71227.

I will proceed with my CL and discussing further in golang/go#71227.

[gopherbot]

Change https://go.dev/cl/642416 mentions this issue: DNR extension/package.json: introduce new options for semantic legend

[gopherbot]

Change https://go.dev/cl/642423 mentions this issue: docs/docs.go: add empty go file under docs module

[h9jiang]

This FR is now implemented and will be shipped with Gopls in v0.18.0 and configuration will be available in vscode-go v0.46.0.

[findleyr]

@h9jiang let's also be sure to deprecate noSemanticNumber and noSemanticString. We can associate that change with this CL. We should also document this configuration change in gopls/doc/release/v0.18.0.md.

[gopherbot]

Change https://go.dev/cl/642998 mentions this issue: gopls/internal/settins: include deprecation message in api-json

[h9jiang]

As mentioned in the CL, noSemanticString & noSemanticNumber will be deprecated in favor of semanticTokenType["string"] = false, semanticTokenType["number"] = false. Users can use semanticTokenType map to disable more types based on their preference. (Same rule applies to semantic token modifiers)

Gopls will still be able to interpret and understand noSemanticString & noSemanticNumber for the next release.

[xzbdmw]

Regarding noSemanticString, should we report semantic tokens for format string directives now since we already done this for DocumentHighlight?

[gopherbot]

Change https://go.dev/cl/643056 mentions this issue: extension/tools/goplssetting: interpret deprecation message from apijson

[gopherbot]

Change https://go.dev/cl/643057 mentions this issue: DNR: extension/package.json: update based on gopls api-json

[h9jiang]

xzbdmw@ you mean, return type: directives, modifiers: [] or return type: string, modifiers: [directive]

I would be very careful when choosing a new type for string directives since vscode theme does not understand this type and have no idea how to render them.

I'm personally ok with having an extra modifiers.

@adonovan might be able to offer a decision here.

[adonovan]

Regarding noSemanticString, should we report semantic tokens for format string directives now since we already done this for DocumentHighlight?

I'm not sure how format strings relate to noSemanticString. I think you must be asking: when the server returns "string" token types, should it also return a "formatString" modifier for the subset that appear to be format strings?

I don't know the answer (I don't use semantic tokens). What would a client do with this information? Would it not be redundant with the information provided by DocumentHighlight? And let's not forget that we're only using a heuristic to classify format strings.

[xzbdmw]

What would a client do with this information?

It will look something like this (nvim + treesitter-printf):

It has limitations that you have to specify the names of these methods in advance for it to parse.

Would it not be redundant with the information provided by DocumentHighlight

SemanticHighlight helps to understand the directives without cursor being on it.

let's not forget that we're only using a heuristic to classify format strings

For vscode users, they can set noSemanticString: true and use textmate rule to highlight those %s (that's why this setting exists), and it's a more simple heuristic, as it will highlight any string which has a %, for example, in this image it interprets double % as placeholders for a string literal:

[findleyr]

Elsewhere, we break up strings into multiple tokens, for example to highlight the imported package name in import specs:
https://cs.opensource.google/go/x/tools/+/master:gopls/internal/golang/semtok.go;l=822;drc=4403100389e1e2f337e097f0b7b27293c6f05c91

I'm doubtful that we should be doing that here, as it is less precise than imports (and I'm not entirely sold on sub-string highlighting in imports as well). However, @xzbdmw please feel free to open that as a separate gopls issue, which we can consider independently.

[gopherbot]

Change https://go.dev/cl/643497 mentions this issue: gopls/doc/release: add semantic token config change