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

Jump to bottom

x/tools/gopls: gopls.gc_details back doesn't clear all gc details #42198

Closed
hyangah opened this issue Oct 25, 2020 · 9 comments
Closed

x/tools/gopls: gopls.gc_details back doesn't clear all gc details #42198

hyangah opened this issue Oct 25, 2020 · 9 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
vscode-go: gopls by default
Milestone
gopls/v0.6.0

Comments

@hyangah
Copy link
Contributor

hyangah commented Oct 25, 2020

How to reproduce (using vscode)

  • From tip, open any go file (I tried one of the files in src/net/http in the example.
  • Run "Go: Toggle gc details" command
    Note: gopls is reporting gc details not only on the package net/http but also other packages it depends on.
  • Run "Go: Toggle gc details" command again.
    Note: gopls is reporting updated diagnostics on net/http, but this time, it doesn't send updated diagnostics on other packages.
First `gopls.gc_details` with `net/http/server_test.go` 
[Trace - 10:45:31.841 AM] Sending request 'workspace/executeCommand - (54)'.
Params: {"command":"gopls.gc_details","arguments":["file:///Users/hakim/go_tip/go/src/net/http/server_test.go"]}
...
[Trace - 10:45:32.546 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///Users/hakim/go_tip/go/src/net/dial.go","diagnostics":[{"range":{"start":{"line":368,"character":5},"end":{"line":368,"character":5}},"severity":3,"source":"go compiler","message":"canInlineFunction(cost: 76)"},{"range":{"start":{"line":368,"character":5},"end":{"line":368,"character":5}},"severity":3,"source":"go compiler","message":"cannotInlineCall(net.(*Dialer).DialContext cannot be inlined)"},{"range":{"start":{"line":368,"character":29},"end":{"line":368,"character":29}},"severity":3,"source":"go compiler","message":"leak(parameter net.ctx leaks to {heap} with derefs=0)","relatedInformation":[{"location":{"uri":"file:///Users/hakim/go_tip/go/src/net/dial.go","range":{"start":{"line":368,"character":5},"end":{"line":368,"character":5}}},"message":"escflow:    flow: {heap} = net.ctx:"},{"location":{"uri":"file:///Users/hakim/go_tip/go/src/net/dial.go","range":{"start":{"line":368,"character":5},"end":{"line":368,"character":5}}},"message":"escflow:      from .this.DialContext(net.ctx, net.network, net.address) (call parameter)"}]},{"range":{"start":{"line":368,"character":50},"end":{"line":368,"character":50}},"severity":3,"source":"go compiler","message":"leak(parameter net.network leaks to {heap} with derefs=0)","relatedInformation":[{"location":{"uri":"file:///Users/hakim/go_tip/go/src/net/dial.go","range":{"start":{"line":368,"character":5},"end":{"line":368,"character":5}}},"message":"escflow:    flow: {heap} = net.network:"},{"location":{"uri":"file:///Users/hakim/go_tip/go/src/net/dial.go","range":{"start":{"line":368,"character":5},"end":{"line":368,"character":5}}},"message":"escflow:      from .this.DialContext(net.ctx, net.network, net.address) (call parameter)"}]},{"range":{"start":{"line":368,"character":59},"end":{"line":368,"character":59}},"severity":3,"source":"go compiler","message":"leak(parameter net.address leaks to {heap} with derefs=0)","relatedInformation":[{"location":{"uri":"file:///Users/hakim/go_tip/go/src/net/dial.go","range":{"start":{"line":368,"character":5},"end":{"line":368,"character":5}}},"message":"escflow:    flow: {heap} = net.address:"},{"location":{"uri":"file:///Users/hakim/go_tip/go/src/net/dial.go","range":{"start":{"line":368,"character":5},"end":{"line":368,"character":5}}},"message":"escflow:      from .this.DialContext(net.ctx, net.network, net.address) (call parameter)"}]}]}
...
Second `gopls.gc_details` with `net/http/server_test.go` 
[Trace - 10:47:03.289 AM] Sending request 'workspace/executeCommand - (58)'.
Params: {"command":"gopls.gc_details","arguments":["file:///Users/hakim/go_tip/go/src/net/http/server_test.go"]}

[Trace - 10:47:03.289 AM] Received request 'window/workDoneProgress/create - (11)'.
Params: {"token":"1443635317331776148"}

[Trace - 10:47:03.290 AM] Sending response 'window/workDoneProgress/create - (11)' in 1ms.
Result: 

[Trace - 10:47:03.291 AM] Received notification '$/progress'.
Params: {"token":"1443635317331776148","value":{"kind":"begin","title":"Toggle gc_details","cancellable":true,"message":"Running..."}}

[Trace - 10:47:03.291 AM] Received response 'workspace/executeCommand - (58)' in 2ms.
Result: null

[Trace - 10:47:03.294 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///Users/hakim/go_tip/go/src/net/http/fs.go","diagnostics":[]}

[Trace - 10:47:03.294 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///Users/hakim/go_tip/go/src/net/http/transfer.go","diagnostics":[]}

[Trace - 10:47:03.294 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///Users/hakim/go_tip/go/src/net/http/header.go","diagnostics":[]}

[Trace - 10:47:03.294 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///Users/hakim/go_tip/go/src/net/http/transport.go","diagnostics":[]}

[Trace - 10:47:03.295 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///Users/hakim/go_tip/go/src/net/http/client.go","diagnostics":[]}

[Trace - 10:47:03.295 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///Users/hakim/go_tip/go/src/net/http/request.go","diagnostics":[]}

[Trace - 10:47:03.295 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///Users/hakim/go_tip/go/src/net/http/response.go","diagnostics":[]}

[Trace - 10:47:03.295 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///Users/hakim/go_tip/go/src/net/http/clone.go","diagnostics":[]}

[Trace - 10:47:03.295 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///Users/hakim/go_tip/go/src/net/http/socks_bundle.go","diagnostics":[]}

[Trace - 10:47:03.295 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///Users/hakim/go_tip/go/src/net/http/sniff.go","diagnostics":[]}

[Trace - 10:47:03.295 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///Users/hakim/go_tip/go/src/net/http/filetransport.go","diagnostics":[]}

[Trace - 10:47:03.295 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///Users/hakim/go_tip/go/src/net/http/h2_bundle.go","diagnostics":[]}

[Trace - 10:47:03.295 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///Users/hakim/go_tip/go/src/net/http/roundtrip.go","diagnostics":[]}

[Trace - 10:47:03.295 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///Users/hakim/go_tip/go/src/net/http/http.go","diagnostics":[]}

[Trace - 10:47:03.295 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///Users/hakim/go_tip/go/src/net/http/cookie.go","diagnostics":[]}

[Trace - 10:47:03.295 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///Users/hakim/go_tip/go/src/net/http/jar.go","diagnostics":[]}

[Trace - 10:47:03.295 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///Users/hakim/go_tip/go/src/net/http/server.go","diagnostics":[]}

[Trace - 10:47:03.295 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///Users/hakim/go_tip/go/src/net/http/status.go","diagnostics":[]}

What you expected
All gc details reports are cleared

What you see instead
All diagnostics from the package that I have a file open is gone, but everal gc details messages about other packages are still left over.
Screen Shot 2020-10-25 at 10 39 02 AM

As soon as I opened the file by clicking one of the left-over gc details diagnostic messages, the left-over gc details on the package disappeared.

Extra info

go version: tip
gopls: v0.5.2-pre.1
@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 Oct 25, 2020
@gopherbot gopherbot added this to the Unreleased milestone Oct 25, 2020
@hyangah
Copy link
Contributor Author

hyangah commented Oct 26, 2020 •
edited

cc @pjweinb

@pjweinb
Copy link

pjweinb commented Oct 26, 2020

I have reproduced this:
workspace is the src directory in the go source distribution
open net/http/server_test.go
toggle gc_details, there are 386 informational diagnostics reported, of which 5 are in dial.go
still in net/http/server_test.go, toggle gc_details again
most of the informational diagnostics go away, but there are still 5 details shown in dial.go, although if one clicks on them, they go away.
(this differs from the report above in that the erroneously retained diagnostics are in the same package)

I'm using the latest gopls and the latest vscod-go and the latest code-insiders.

@pjweinb
Copy link

pjweinb commented Oct 27, 2020

There are two ways of computing diagnostics, controlled by a boolean 'withAnalysis'. If a file is open in the editor, its (package's) diagnostics are computed withAnalysis true. So in this case, diagnostics for net/http are computed withAnalysis true. The details are computed for the directory net/http, but include details for net/dial.go, but as all the diagnostics for net/http are packaged together, these count as having withAnalysis true also.

Then when gc_details is turned off, the diagnostics are recomputed, and there are none for net/dial.go. But gopls sees that the previous diagnostics for net/dial.go were done withAnalysis true, so it believes the new diagnostics are less precise, so doesn't send them.

That is, the diagnostics for net/dial.go were sent originally as part of diagnostics on a different package (http, which has a visible file) and withAnalysis true, and the new diagnostics are on package net where withAnalysis is false.

If this is right, the problem occurs when gc_detaills is turned off. There seem to be two approaches:
A. In command.go, where diagnoseSnapshot is called, pass it an extra parameter ['useLatest bool'] to tell gopls to send all diagnostics (and not privilege those diagnostics saved having withAnalysis==true)
B. In command.go, look at all the diagnostics (at least for files not in the package) and invalidate the saved diagnostics if they contain gc_details diagnostics. [there's a lock for the saved diagnostics, so that's not trivially racy]

@pjweinb
Copy link

pjweinb commented Oct 29, 2020

Alas, it is harder than that. B doesn't work easily as we need to get empty diagnostics sent. Changing the remembered diagnostics doesn't help enough. And it's even worse: in the current source distribution gc_details also creates a diagnostic for vendor/golang.org/x/net/http/httproxy/proxy.go, and the regular diagnostics never produce anything for that, not even empty diagnostics, so there's no way to turn them off in vscode.

@gopherbot
Copy link

Change https://golang.org/cl/266342 mentions this issue: internal/lsp: elide details for non-package files

@pjweinb
Copy link

pjweinb commented Nov 9, 2020

there are other cases where the details don't go away.

@pjweinb pjweinb reopened this Nov 9, 2020
@gopherbot
Copy link

Change https://golang.org/cl/268464 mentions this issue: internal/lsp: make sure gc_details go away when toggled off (another case)

@stamblerre stamblerre added this to Needs Triage in vscode-go: gopls by default Nov 10, 2020
@stamblerre stamblerre moved this from Needs Triage to Critical in vscode-go: gopls by default Nov 10, 2020
@stamblerre stamblerre moved this from Critical to In progress in vscode-go: gopls by default Nov 10, 2020
@gopherbot
Copy link

Change https://golang.org/cl/269320 mentions this issue: internal/lsp: fix circumstances where toggling off gc_details left some

@gopherbot
Copy link

Change https://golang.org/cl/269677 mentions this issue: internal/lsp: track diagnostics by reporting source

vscode-go: gopls by default automation moved this from In progress to Done Nov 24, 2020
@golang golang locked and limited conversation to collaborators Nov 24, 2021
@rsc rsc unassigned pjweinb Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
No open projects
vscode-go: gopls by default
  
Done
Milestone
gopls/v0.6.0
Development

No branches or pull requests
4 participants
@hyangah @pjweinb @stamblerre @gopherbot