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: improve internal/lsp/cmd package #38085

Open
stamblerre opened this issue Mar 26, 2020 · 5 comments
Open

x/tools/gopls: improve internal/lsp/cmd package #38085

stamblerre opened this issue Mar 26, 2020 · 5 comments
Labels
Milestone

Comments

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Mar 26, 2020

I am really struggling to do code reviews for the cmd package. The logical flow is hard to reason about, and the way to set options, etc. is not obvious. We should spend some time restructuring it to be more clear.

@findleyr, @heschik: If any suggestions/ideas immediately come to mind, please let me know.

@stamblerre stamblerre added this to the gopls/v0.5.0 milestone Mar 26, 2020
@findleyr

This comment has been minimized.

Copy link

@findleyr findleyr commented Mar 26, 2020

Agree that it is hard to follow. I spent a long time staring at it for https://golang.org/cl/215742 -- and that CL simplified some things but made others more complicated. Also, feel free to send those reviews my way...

Can we list what we'd want to change? Off the top of my head, the following things are complex or confusing:

  1. the fact that commands interact with an in-process LSP server, and therefore have to translate async LSP into a synchronous command
  2. handling of the LSP connection: each subcommand connects and terminates the connection itself, duplicating logic and tightly coupling to the connection
  3. the internal@ hack/optimization for cmdtests
  4. special handling to default to the serve subcommand
  5. sharding of flags across the application and the subcommand
  6. handling of files, and translating to and from LSP file coordinates / URIs

Anything else come to mind?

Some of these probably can't/shouldn't be fixed, for example I think executing commands over LSP is The Right Thing To Do. Also, it's probably too late to stop defaulting to the serve subcommand (or is it?!).

As a starting point, I think we can improve (2) and (3): it would help to lift handling of the cmdClient / jsonrpc2.Conn out of .Run and up to the application layer, instead injecting in the connection to the subcommand. That might also simplify the internal@ optimization. I can take a stab at this.

Another straightforward improvement is just to add a bunch more documentation around the tricky parts.

WDYT?

@stamblerre

This comment has been minimized.

Copy link
Contributor Author

@stamblerre stamblerre commented Mar 26, 2020

Some of these probably can't/shouldn't be fixed, for example I think executing commands over LSP is The Right Thing To Do. Also, it's probably too late to stop defaulting to the serve subcommand (or is it?!).

Yes, I think is too late for that.

As a starting point, I think we can improve (2) and (3): it would help to lift handling of the cmdClient / jsonrpc2.Conn out of .Run and up to the application layer, instead injecting in the connection to the subcommand. That might also simplify the internal@ optimization. I can take a stab at this.

Agree with this plan - for me (3) is the hardest to reason about. Thanks!

Another straightforward improvement is just to add a bunch more documentation around the tricky parts.

Definitely. Even just writing down how the tests work would help immensely.

Another point of confusion to add to the list - we originally had a gopls query command for which the feature commands would be subcommands. Somehow this got lost along the way, but definition is still invoked as gopls query definition. We should make this consistent in one way or another. Maybe @ianthehat can shed light on what the benefits of query would have been?

@findleyr

This comment has been minimized.

Copy link

@findleyr findleyr commented Mar 26, 2020

Oh, another that occurred to me (perhaps that can't be fixed) is the fact that options is a func(*source.Options). That frequently confuses me -- especially since source.Options is huge.

@findleyr

This comment has been minimized.

Copy link

@findleyr findleyr commented Mar 26, 2020

Agree with this plan - for me (3) is the hardest to reason about. Thanks!

Do you think it's reasonable to expect that workspace initialization will ever be cacheable enough to delete the internal hack altogether? In other words, for cmdtests we initialize a gopls remote and just connect each cmd to a new session on the remote? That would be the ideal solution (and also would be an optimization for real-world usage of commands).

@ianthehat

This comment has been minimized.

Copy link

@ianthehat ianthehat commented Mar 26, 2020

I think the actual command line needs some thought.

  • I do not think it is too late to change it at all, but once we go beta it might be.
  • We should not default to the serve command
  • We should put some effort into separating automation verbs (and their flags) from user verbs. The former we should promise backwards compatibility but not worry about ugliness, the latter we should be happy to change/break for the benefit of clarity at any time.

I agree the internal hack should be vaporized, it ought to be possible (but maybe with a different cleaner hack added to the client server behavior)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.