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: add support for provided WorkDoneToken in ExecuteCommand #40527

Closed
leitzler opened this issue Aug 1, 2020 · 5 comments
Closed

x/tools/gopls: add support for provided WorkDoneToken in ExecuteCommand #40527

leitzler opened this issue Aug 1, 2020 · 5 comments
Assignees
Labels
Milestone

Comments

@leitzler
Copy link
Contributor

@leitzler leitzler commented Aug 1, 2020

What version of Go are you using (go version)?

$ go version
-
x/tools@644d4167

What did you do?

Called ExecuteCommand with a provided WorkDoneToken:

&protocol.ExecuteCommandParams{
     Command:   "generate",
     Arguments: {
         {0x22, 0x66, 0x69, 0x6c, 0x65, 0x3a, 0x2f, 0x2f, 0x2f, 0x70, 0x72, 0x69, 0x76, 0x61, 0x74, 0x65, 0x2f, 0x76, 0x61, 0x72, 0x2f, 0x66, 0x6f, 0x6c, 0x64, 0x65, 0x72, 0x73, 0x2f, 0x78, 0x6d, 0x2f, 0x6c, 0x73, 0x32, 0x30, 0x38, 0x78, 0x64, 0x31, 0x37, 0x34, 0x76, 0x39, 0x35, 0x70, 0x67, 0x64, 0x32, 0x30, 0x72, 0x6e, 0x5f, 0x71, 0x62, 0x68, 0x30, 0x30, 0x30, 0x30, 0x67, 0x6e, 0x2f, 0x54, 0x2f, 0x74, 0x6d, 0x70, 0x2e, 0x5a, 0x53, 0x69, 0x38, 0x48, 0x67, 0x49, 0x47, 0x22},
         {0x66, 0x61, 0x6c, 0x73, 0x65},
     },
     WorkDoneProgressParams: protocol.WorkDoneProgressParams{
         WorkDoneToken: int(1001),
     },
}

When gopls starts to execute go generate, it sends a ShowMessageRequest to the client so the client can present a dialog to the user. That dialog includes an action, Cancel, to let the user cancel go generate before it's done.

Using a WorkDoneToken in the ExecuteCommand request is the only way I found to tie the dialog to a specific execution. If it can't be tied to that specific execution there is no way to know when the dialog should be automatically closed (i.e. when go generate is done and Cancel isn't a valid action anymore).

What did you expect to see?

The WorkDoneToken used during progress reporting.

What did you see instead?

A new token created for me:

WorkDoneProgressCreate callback: &protocol.WorkDoneProgressCreateParams{
    Token: "8674665223082153551",
}

That is used to report progress:

Progress callback: &protocol.ProgressParams{
    Token: "8674665223082153551",
    Value: map[string]interface {}{
        "cancellable": bool(true),
        "kind":        "begin",
        "message":     "running go generate",
        "title":       "generate",
    },
}
Progress callback: &protocol.ProgressParams{
    Token: "8674665223082153551",
    Value: map[string]interface {}{
        "cancellable": bool(true),
        "kind":        "report",
        "message":     "sleep 10\n",
    },
}
Progress callback: &protocol.ProgressParams{
    Token: "8674665223082153551",
    Value: map[string]interface {}{
        "kind":    "end",
        "message": "finished",
    },
}
@gopherbot gopherbot added this to the Unreleased milestone Aug 1, 2020
@leitzler
Copy link
Contributor Author

@leitzler leitzler commented Aug 2, 2020

ShowMessageRequest isn't sent when WorkDoneProgress is supported by the client, so this isn't as urgent as I initially thought. The entire lifetime can be traced using progress callbacks only, even though they can't be linked to a specific command execution.

@stamblerre stamblerre modified the milestones: Unreleased, gopls/v1.0.0 Aug 4, 2020
@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Aug 7, 2020

I don't think we actually ever use the WorkProgressToken passed in by any request, though we should. @findleyr has the most experience with this code, and maybe have some thoughts here?

@findleyr
Copy link
Contributor

@findleyr findleyr commented Aug 7, 2020

Yeah, this is a bug. I'll fix it.

@findleyr findleyr self-assigned this Aug 7, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 7, 2020

Change https://golang.org/cl/247321 mentions this issue: internal/lsp: use the token supplied by the client for progress

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 7, 2020

Change https://golang.org/cl/247407 mentions this issue: internal/lsp/progress: refactor progress reporting

gopherbot pushed a commit to golang/tools that referenced this issue Aug 10, 2020
Our WorkDone reporting was generating a random token for each unit of
work, even if a token was supplied by the client.  Change this to use
the client token if it is non-empty, and skip the
workDoneProgress/create request.

After this change we can no longer rely on tokens being a string.
Update our progress tracking accordingly.

For golang/go#40527

Change-Id: I702f739c466efb613b69303aaf07005addd3b5e2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/247321
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@stamblerre stamblerre added this to the gopls/v.0.4.5 milestone Aug 10, 2020
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.