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

x/tools/gopls: support selection range request #36679

Closed
stamblerre opened this issue Jan 21, 2020 · 18 comments
Closed

x/tools/gopls: support selection range request #36679

stamblerre opened this issue Jan 21, 2020 · 18 comments
Assignees
Labels
FeatureRequest FrozenDueToAge gopls Issues related to the Go language server, gopls. help wanted Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@stamblerre
Copy link
Contributor

stamblerre commented Jan 21, 2020

Forked from microsoft/vscode-go#2984.

See https://microsoft.github.io/language-server-protocol/specifications/specification-current/#textDocument_selectionRange.

@gopherbot gopherbot added this to the Unreleased milestone Jan 21, 2020
@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 Jan 21, 2020
@stamblerre stamblerre changed the title x/tools/gopls: support selectionRange x/tools/gopls: support selection range request Jan 21, 2020
@stamblerre stamblerre modified the milestones: Unreleased, gopls/v1.0.0 Jan 21, 2020
@ridersofrohan ridersofrohan self-assigned this Feb 5, 2020
@ridersofrohan ridersofrohan removed their assignment Feb 20, 2020
@stamblerre stamblerre removed this from the Unreleased milestone Jul 23, 2020
@stamblerre stamblerre added this to the gopls/unplanned milestone Oct 21, 2020
@rneatherway
Copy link

I'm interested in having a look at this one. Would you suggest that I follow the same advice as over here (#47558 (comment)) to get started?

@findleyr
Copy link
Contributor

Hi @rneatherway, sorry for the slow response. Sure this is a good starter project, and a good way to get started would be to solve the problem outside of gopls using go/ast: given a position and an ast.File, can you find interesting selection ranges?

Then we can help you wire this into gopls' request handling.

@rneatherway
Copy link

No problem at all, thanks for the guidance 👍

@rneatherway
Copy link

👋 I've had a go at this and put together a function of this signature:

type Selection struct {
	StartLine int
	StartCol  int
	EndLine   int
	EndCol    int
}

func expandSelection(
	fileSet *token.FileSet,
	file *ast.File,
	s Selection,
) Selection

It's up at https://github.com/rneatherway/expand along with some tests/examples.

Note that it also requires a token.FileSet to get the location of AST nodes. It works by doing simple descent of the tree ignoring any nodes that do not include the current selection, returning the last node that is a strict superset of the current selection. I noticed that the column numbers displayed in VSCode count a tab as multiple characters according to the configured tab width, but I'd guess that's already accounted for by other gopls features or the language server API.

@findleyr
Copy link
Contributor

Thanks! We'll take a look.

@rneatherway
Copy link

Sorry to prod, but if you have any advice on wiring this up I'd be interested to try that too.

@findleyr
Copy link
Contributor

@rneatherway so sorry that this fell through the cracks. Entirely my fault, and thank you for following up.

What you've done looks good! I think there is a utility that may be helpful, which operates similarly to your algorithm:
https://pkg.go.dev/golang.org/x/tools@v0.2.0/go/ast/astutil#PathEnclosingInterval

Using this traversal, you can start from the innermost enclosing interval, and work outwards until you find an interesting node, then compute its range.

One note: for simplicity I suggest working with a token.Pos rather than token.Position. Language servers use UTF-16 column positions anyway, so we will always have to do a conversion before returning to the client.

@rneatherway
Copy link

What you've done looks good! I think there is a utility that may be helpful, which operates similarly to your algorithm:
https://pkg.go.dev/golang.org/x/tools@v0.2.0/go/ast/astutil#PathEnclosingInterval

Oh nice, thanks! I've simplified the code in my repository using that helper method. The input and output are now in terms of offsets so hopefully this is ready to try integrating. It's hard to go much further without knowing what the language server has available when this will be invoked.

@findleyr
Copy link
Contributor

Yep, let's start integrating it.

The entry point for this handler is x/tools/gopls/internal/lsp/server_gen.go. In this case, you'll be writing a handler that needs what we call the "ParsedGoFile" corresponding to the request URI. A ParsedGoFile is just a wrapper around *ast.File that bundles additional related informatoin.

A similar type of request is DocumentSymbols, which returns information about symbols in a given file, using the AST. You can look at how that handler is structured: get a snapshot using Server.beginFileRequest, then use that snapshot to parse. Want to give it a shot and see how far you get? Now that you're working in x/tools, the simplest way to collaborate will be to send me a CL (even if it is a work in progress) to comment on.

@findleyr
Copy link
Contributor

(aside: converting to int does not convert token.Pos to offsets, as token.Pos is relative to some file base position. You must use token.File.Offset to convert to an offset)

rneatherway added a commit to rneatherway/tools that referenced this issue Nov 19, 2022
rneatherway added a commit to rneatherway/tools that referenced this issue Nov 19, 2022
@rneatherway
Copy link

rneatherway commented Nov 19, 2022

@findleyr I've got a branch visible at https://github.com/golang/tools/compare/master...rneatherway:tools:selectionrange?expand=1 including extending the tests to support this new case. A few things came up while working on it that I'd appreciate your opinion on:

  • I found a few minor typos and other issues in READMEs and the code generation helper doesn't work on Windows. Do you mind me including those in this change or should I submit them separately?

  • The tests for my standalone version that I wrote over at https://github.com/rneatherway/expand/blob/master/expand_test.go include multiline cases, which seem to be necessary for proper testing of this feature. Does the annotation testing support marking spans over multiple lines right now? Update: the inputs are positions so multiline annotations are not necessary.

  • I wasn't sure if it is necessary or desirable to implement command-line support for this feature. I also claim that it doesn't need "source" level testing, but I'm not sure what that means.

  • My implementation interprets position[0] and position[1] as specifying a range, as I had in mind the idea of extending a single selection by one step. The LSP documentation states:

    A selection range in the return array is for the position in the provided parameters at the same index. Therefore positions[i] must be contained in result[i].range. To allow for results where some positions have selection ranges and others do not, result[i].range is allowed to be the empty range at positions[i].

    Some background reading at Feature request: "document/extendSelection" for semantic selection microsoft/language-server-protocol#613 helped to clarify that this is intended for multiple cursors and to return the entire path to the root of the AST in order to avoid round-trips. So I can probably pretty much return astutil.PathEnclosingInterval(...) for each position. I'll update accordingly and try to think about how to re-phrase the tests. Update: I have made these updates and changed the tests so they output numeric and textual representations of the ranges to a golden file.

@gopherbot
Copy link

Change https://go.dev/cl/452315 mentions this issue: gopls/internal/lsp: add selection range request

@rneatherway
Copy link

rneatherway commented Nov 20, 2022

I got far enough that I decided to open the pull request: golang/tools#416

I guess my last question would be whether you had any advice on how to test this change with VSCode "live". So far I have tried go installing from my branch and invoking the "expand selection" command in a subsequently launched VSCode, but it doesn't behave any differently.

@findleyr findleyr removed their assignment Dec 8, 2022
@findleyr
Copy link
Contributor

findleyr commented Dec 9, 2022

@hyangah do we need to register a new selection range provider? It may be that this needs a vscode-go change as well.

@rneatherway
Copy link

@hyangah do we need to register a new selection range provider? It may be that this needs a vscode-go change as well.

I've got a feeling that we need to do something like https://github.com/elm-tooling/elm-language-server/blob/c7e21144c0833690a58bbf5f5e6b44b123c1fc9f/src/providers/selectionRangeProvider.ts, but looking in the vscode-go repository a lot of the providers are in a legacy directory, so I am not sure if I should be adding new similar code or if there is a new way of doing things.

@gopherbot
Copy link

Change https://go.dev/cl/458199 mentions this issue: gopls/internal/lsp: announce selectionRangeProvider capability

@hyangah
Copy link
Contributor

hyangah commented Dec 16, 2022

Thanks for implementing the feature. That's neat.
VSCode Go uses LSP so the server needs to announce this new capability.

gopherbot pushed a commit to golang/tools that referenced this issue Dec 16, 2022
The server needs to tell the client it's supporting this feature.

Updates golang/go#36679

Change-Id: Ia6fdb35ea90fac81367d4b45721928c606344afc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/458199
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
@rneatherway
Copy link

Thanks for implementing the feature. That's neat.
VSCode Go uses LSP so the server needs to announce this new capability.

Ah thanks! I wasn't sure how the final wiring up should look. Is there anything more we need to do in vscode-go?

@golang golang locked and limited conversation to collaborators Dec 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FeatureRequest FrozenDueToAge gopls Issues related to the Go language server, gopls. help wanted Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants