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.

Sign up for GitHub

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

Jump to bottom

x/tools/gopls: gopls api-json references "internal/protocol.CodeLensSource" type #68057

Closed
hyangah opened this issue Jun 18, 2024 · 7 comments
Closed

x/tools/gopls: gopls api-json references "internal/protocol.CodeLensSource" type #68057

hyangah opened this issue Jun 18, 2024 · 7 comments
Assignees
@adonovan
Labels
gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@hyangah
Copy link
Contributor

hyangah commented Jun 18, 2024 •
edited
Loading

v0.16.0-pre.1 and pre.2

$ gopls api-json

...
   {
     "Name": "codelenses",
     "Type": "map[golang.org/x/tools/gopls/internal/protocol.CodeLensSource]bool",
				 ...

Previously, it was map[string]bool, but now we need to check the internal package.
It would be nice if we can stick with the basic types like string here.

Assuming that only vscode-go's tool was affected and we can work around it, I don't think it is a release blocker.
@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Jun 18, 2024
@gopherbot gopherbot added this to the Unreleased milestone Jun 18, 2024
@adonovan
Copy link
Member

adonovan commented Jun 19, 2024 •
edited
Loading

The right place to fix this is in loadOptions in gopls/doc/generate/generate.go, which uses a very simple heuristic to turn the go/types representation of each settings.Option field into JSON-like type names.

The complete set of types that it encounters today is given by this grammar:

T = string | int | bool
    | []T
    | map[string]T
    | any
    | time.Duration
    | enum
    | settings.Annotation | protocol.CodeLensSource

Notes:

  • any is only used for linksInHover, whose JSON type is actually the sum true + false + "gopls".
  • time.Duration is a Go integer type, but the JSON value must be a string parseable by time.ParseDuration.
  • enum is used to indicate any of several known enum types; but the type and its values are not specified
  • settings.Annotation and protocol.CodeLensSource should both be covered by the enum case but it looks like there's been an oversight.

There's definite room for improvement here. Perhaps each setting.Options struct field could provide an optional tag that gives its JSON type for documentation purposes if and only if it is not derivable in the obvious way (i.e. the last four lines of the grammar above).

@adonovan adonovan self-assigned this Jun 19, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/593615 mentions this issue: gopls/internal/settings: move CodeLensSource from protocol

@hyangah
Copy link
Contributor Author

hyangah commented Jun 19, 2024

Thanks.

Re: any as the type. (linksInHover).

{
                                "Name": "linksInHover",
                                "Type": "any",
                                "Doc": "linksInHover controls the presence of documentation links\nin hover markdown.\n\nIts legal values are:\n- `false`, for no links;\n- `true`, for links to the `linkTarget` domain; or\n- `\"gopls\"`, for links to gopls' internal documentation viewer.\n",
                                "EnumKeys": {
                                        "ValueType": "",
                                        "Keys": null
                                },
                                "EnumValues": null,
                                "Default": "true",
                                "Status": "",
                                "Hierarchy": "ui.documentation"
                        },

What vscode setting generator does is to map this api-json settings to the vscode configuration as explained in https://code.visualstudio.com/api/references/contribution-points#Configuration-property-schema.

For linksInHover acceptable values can be either boolean or gopls (string).
I think mapping this to enum works best for users.

"ui.documentation.linksInHover": {
            ...
              "enum": [
                true,
                false,
                "gopls"
              ],
                    ...
 }

Screenshot 2024-06-19 at 4 28 26 PM

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/593676 mentions this issue: [gopls-release-branch.0.16] gopls/internal/settings: move CodeLensSource from protocol

gopherbot pushed a commit to golang/tools that referenced this issue Jun 20, 2024
@adonovan @gopherbot
[gopls-release-branch.0.16] gopls/internal/settings: move CodeLensSou…
cb3016b 
…rce from protocol

This enum is properly a setting, not a protocol feature.

The .md and api.json generator script assumes that all enums
are in the settings package, and consequently emitted incorrect
names for this type.

Generator changes in detail:
- remove the package.Load of both "settings" and "protocol",
  reverting part of an early change.
- remove loadAPI special case for "codelenses" not being an
  enum-keyed map, as it is one. (As a consequence, the enum
  values appear in api.json in alphabetical, not declaration,
  order. Also, the title portion of the codelend doc string
  is no longer discarded.)
- add lots of missing commentary.

This may seem like a large change at the 11th hour, but note:
- the only change to the production code is a renaming;
- the effects of the generator changes are entirely confined
  to settings.md and api.json.

Fixes golang/go#68057

Change-Id: I097f0a9b2e34b8f9a3438112b55efb2081b4acb2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/593615
Reviewed-by: Robert Findley <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
(cherry picked from commit 1cd1a4f08f940bc68597469ffb33630f33ef104f)
Reviewed-on: https://go-review.googlesource.com/c/tools/+/593676
Auto-Submit: Alan Donovan <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/593656 mentions this issue: gopls/doc/generate: treat LinksInHover as an enum

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/593678 mentions this issue: extension/tools/goplssetting: update to work with gopls v0.16.0 settings

gopherbot pushed a commit to golang/tools that referenced this issue Jun 20, 2024
@adonovan
gopls/doc/generate: treat LinksInHover as an enum
2d104ec 
This CL defines a type for the true|false|"gopls" type used
by linksInHover, and adds a special case to the doc+api generator
to treat it as an enum, so that VS Code will present a better
value-chooser UI for it.

Also, document the type grammar used in the docs.

Updates golang/go#68057

Change-Id: I9e334fbc94dcbdc70657d8e64f67fb807e69cbf8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/593656
Reviewed-by: Robert Findley <[email protected]>
Reviewed-by: Hyang-Ah Hana Kim <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>
gopherbot pushed a commit to golang/vscode-go that referenced this issue Jun 21, 2024
@hyangah
extension/tools/goplssetting: update to work with gopls v0.16.0 settings
5584351 
Temporarily map "any" to "boolean" until CL 593656 is released.

For golang/go#68057

Change-Id: Id134337269b26b9a9ddb546c63f645996d3c3c83
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/593678
Reviewed-by: Alan Donovan <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
Commit-Queue: Hyang-Ah Hana Kim <[email protected]>
kokoro-CI: kokoro <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/616679 mentions this issue: extension/tools/goplssetting: handle enum types

hyangah added a commit to hyangah/vscode-go that referenced this issue Oct 10, 2024
@hyangah
extension/tools/goplssetting: handle enum types
f19b641 
gopls v0.17 will have LinksInHover that is the true|false|"gopls"
enum type. Previously, we assumed enum type is always string. That
is no longer true. Handle this sum-type enum.

For golang/go#68057

Change-Id: I9eb2e8376191d997895f8998bfffb2662fbfb92e
gopherbot pushed a commit to golang/vscode-go that referenced this issue Oct 16, 2024
@hyangah
extension/tools/goplssetting: handle enum types
bea7919 
gopls v0.17 will have LinksInHover that is the true|false|"gopls"
enum type. Previously, we assumed enum type is always string. That
is no longer true. Handle this sum-type enum.

For golang/go#68057

Change-Id: I9eb2e8376191d997895f8998bfffb2662fbfb92e
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/616679
Commit-Queue: Hyang-Ah Hana Kim <[email protected]>
kokoro-CI: kokoro <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@adonovan adonovan

Labels
gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
3 participants
@hyangah @adonovan @gopherbot