Publish diagnostics #253
Publish diagnostics #253
Conversation
|
sry for the delay. I'm aware of the build/test problems. Let's see if I can fix them in the next few days. |
langserver/integration_test.go
Outdated
| @@ -217,11 +337,74 @@ func integrationTest( | |||
| } | |||
| } | |||
|
|
|||
| ctx := context.Background() | |||
| cfg := NewDefaultConfig() | |||
| cfg.UseBinaryPkgCache = true | |||
There was a problem hiding this comment.
The TestIntegration_FileSystem test fails because I changed this to true.
I see the code why it does not work but I have another question: why does this language server even use the binary pkg cache? Nowhere in the language server is the code actually built (aka go install). So how is this supposed to work?
There was a problem hiding this comment.
Sorry I haven't looked at this yet! But cc @slimsag about UseBinaryPkgCache question.
There was a problem hiding this comment.
The idea with UseBinaryPkgCache was that you are already running go install on your own. This concept actually comes from https://github.com/rogpeppe/godef -- which makes the same assumption. If you are not running go install on your code, however, then this option will not work well for you (you'll either get no results or stale results). This feature exists primarily because go/loader does not allow for caching type-checking results.
In your case here, I think you should just not set UseBinaryPkgCache = true. That means it will not use the cache, and will instead use the other codepath that performs type-checking on-the-fly (which is slower, but ideal for a test such as this)
remove async publishing of diagnostics because it is really hard to assert published diagnostics then
|
I'm not sure if this PR still makes sense if we assume that diagnostics are served by another editor plugin... What's your opinion on that? |
|
We can have another editor plugin do it, but it does make sense for the langserver to properly support diagnostics. |
…ther integration tests this is actually a "trick". Because it helps the test to only send diagnostics for `textDocument/didSave` calls
|
@slimsag @keegancsmith your review is appreciated 😁 |
There was a problem hiding this comment.
Thanks! This is great, and the test suite is amazing. The main issue (mentioned inline) is that if we start running our own typechecker again, the memory usage balloons. This was a source of much frustration in go-langserver a while ago, which lead to us implementing a godef based approach which avoids the typechecker.
langserver/diagnostics.go
Outdated
| @@ -18,18 +19,37 @@ import ( | |||
|
|
|||
| type diagnostics map[string][]*lsp.Diagnostic // map of URI to diagnostics (for PublishDiagnosticParams) | |||
|
|
|||
| type diagnosticsCache struct { | |||
| cache diagnostics | |||
| lock sync.Mutex | |||
There was a problem hiding this comment.
nit: use style of a "mutex hat", so call rename lock to mu and have it above cache. https://dmitri.shuralyov.com/idiomatic-go#mutex-hat
langserver/diagnostics.go
Outdated
| lock sync.Mutex | ||
| } | ||
|
|
||
| func (p *diagnosticsCache) update(fn func(diagnostics) diagnostics) { |
There was a problem hiding this comment.
could you document this behaviour here? That we enforce that only one fn is running at a time.
langserver/handler.go
Outdated
| @@ -392,7 +397,7 @@ func (h *LangHandler) Handle(ctx context.Context, conn jsonrpc2.JSONRPC2, req *j | |||
| // a user is viewing this path, hint to add it to the cache | |||
| // (unless we're primarily using binary package cache .a | |||
| // files). | |||
| if !h.Config.UseBinaryPkgCache { | |||
| if !h.Config.UseBinaryPkgCache || req.Method == "textDocument/didSave" { | |||
There was a problem hiding this comment.
the main issue with enabling typechecking again, is that it uses a lot of memory. This might be a blocker for this PR / reason to make this opt-in. WDYT @slimsag
There was a problem hiding this comment.
I have added a simple command line flag --diagnostics to activate it
There was a problem hiding this comment.
you still need to make this conditional fail if diagnostics is not enabled.
langserver/handler.go
Outdated
| @@ -397,7 +397,7 @@ func (h *LangHandler) Handle(ctx context.Context, conn jsonrpc2.JSONRPC2, req *j | |||
| // a user is viewing this path, hint to add it to the cache | |||
| // (unless we're primarily using binary package cache .a | |||
| // files). | |||
| if !h.Config.UseBinaryPkgCache || req.Method == "textDocument/didSave" { | |||
| if h.Config.DiagnosticsEnabled && (!h.Config.UseBinaryPkgCache || req.Method == "textDocument/didSave") { | |||
There was a problem hiding this comment.
I'd suspect this matches the proper intent. We still want to run it if usebinarypkgcache is disabled
!h.Config.UseBinaryPkgCache || (h.Config.DiagnosticsEnabled && req.Method == "textDocument/didSave")There was a problem hiding this comment.
I have added the Config.DiagnosticsEnabled check in the publishDiagnostics. So there is no harm in doing the typecheck on save too, right?
There was a problem hiding this comment.
Anyway. I will change it to
if !h.Config.UseBinaryPkgCache || (h.Config.DiagnosticsEnabled && req.Method == "textDocument/didSave") {
|
Sorry one more inline comment then this is ready to merge :) |
|
great CI failure is on go tip so is unrelated to your change. Merging in. |
|
oh this is merging into |
This PR adds a very basic cache that holds diagnostics for each checked file. A file entry is removed once no more diagnostics exist for it.
An async
typechecknow also happens on every save requests.A simple unit test asserts the cache updating and an integration test modifies and saves files and then asserts certain diagnostics to be sent to the client.
Unfortunately it's currently based on #248 because it contains some changes that are necessary to work on my windows machine 😞
This is probably not 100% ready to be merged yet, but I think it is a good starting point 😁