Implement pull-model documentDiagnostics #746

tristanlabelle · 2023-05-11T21:11:17Z

Implements LSP 3.17.0's pull-model documentDiagnostics request, which fix stale squiggly issues after fixing an error due to the relationship between code in two files. When using pull-model documentDiagnostics, we disable the push-model publishDiagnostics to avoid reporting them twice.

(this gif was using a temporary "pulldiag" source name to verify that it wasn't publish diagnostics in action)

Fixes #500

tristanlabelle · 2023-05-11T21:15:21Z

Sources/SourceKitLSP/Swift/SwiftLanguageServer.swift

+    // in addition to the existing push-based publish notifications.
+    // If the client supports pull diagnostics, we report the capability
+    // and we should disable the publish notifications to avoid double-reporting.
+    return clientCapabilities.textDocument?.diagnostic == nil


Should this rather be testing clientCapabilities.textDocument?.diagnostic?.dynamicRegistration != true? I'm not sure if we would receive the pull requests if dynamic registration is not supported by the client.

I think this is the right approach. Correct me if you think I’m wrong but checking for dynamicRegistration would only check if the client supports dynamic registration of the capability. If, for some reason, it supported registration of the capability at server initialization but not dynamically after the fact, checking for dynamicRegistration wouldn’t catch that.

We have the same understanding of dynamicRegistration. My read of SourceKitServer is that it doesn't report supporting pull diagnostics at initialization. Later, when it is asked about a swift file, it starts SwiftLanguageServer, initializes it, sees that it reports supporting pull diagnostics and at that point dynamically registers support for pull diagnostics to the LSP client. So, we depend on dynamicRegistration being supported.

Or maybe we should unconditionally report support for pull diagnostics during SourceKitServer's initialization, avoiding dynamic registration? If our underlying language server doesn't support the request, SourceKitServer will return no diagnostics so I think there's no harm done.

If you unconditionally supporting pull diagnostics are you then unconditionally disabling publish diagnostics? Given you are currently disabling publish diagnostics when pull diagnostics are enabled.

That shouldn't be a problem. SourceKitServer.initialize() can unconditionally report support for pull diagnostics, but SwiftLanguageServer's logic is based on the ClientCapabilities coming from the LSP Client, so it will only disable publish diagnostics if the LSP client supports pull diagnostics.

Honestly, what I really want, would be to see how other language servers handle this. But I haven’t found a single major LSP server that supports both publishDiagnsotics and textDocument/diagnostic. Also, what surprises me, is that I can’t find a PR or issue on /~https://github.com/microsoft/language-server-protocol that mentions textDocument/diagnostics, which kind of surprises me because I thought there would be some kind of discussion about these features.

Anyway, given the lack of precedence or clarification, I think we really need to make up our own and out of all the alternatives we have considered, I think not sending publishDiagnostics notifications if both the server and the client support “Pull Diagnostics” still seems like the cleanest design to me.

It adds some complexity for determining if we should or not publish diagnostics, and it requires dynamic capability registration.

What’s the added complexity here? I think passing down the CapabilityRegistry instead of ClientCapabilities as you do it in your PR works rather nicely. I haven’t looked at it in too much detail though.

I'm thinking a better approach would be to stop publishing diagnostics in the language-specific server as soon as we receive a first pull diagnostics request, so we have proof that the client is using them.

Hmm, I’m not a huge fan because this means that sending a request silently changes the entire behavior of SourceKit-LSP, which is very confusing IMO. I think whether publishDiagnostic notifications should be determined when the client and server agree on capabilities.

I also think it makes sense to unconditionally reporting pull diagnostics as supported in the SourceKitServer (independent of language-specific servers), rather than using dynamic registration when one language-specific server comes online and requires it. We have a well-defined response if we get a pull diagnostics request and the language-specific server doesn't support it (return an empty diagnostics array), and it won't stop the client from responding to publish diagnostics messages by language-specific servers who do not support pull diagnostics. Thoughts?

But wouldn’t that leave us with the following problem: clangd publishes its diagnostics using publishDiagnostics. Then the client requests diagnostics using textDocument/diagnostics and SourceKit-LSP returns an empty array, clearing any diagnostics displayed on the client.

Oh I think that your last point reveals our different assumptions. My assumption is that the namespacing of pull and publish diagnostics is separate and they don't interact because that would be hard to manage, but I'm still looking for some confirmation of this (specs or reference implementations).

Oh, yes. If they are indeed in different namespaces, that would certainly change things.

Let's see if this gets us more info: microsoft/language-server-protocol#1743

In the meanwhile, I finished and pushed your suggested implementation, so we can make some progress. It's ready to review now.

Sources/SourceKitLSP/Swift/SwiftLanguageServer.swift

tristanlabelle · 2023-05-11T21:18:30Z

@ahoppen Good news! I got this to work and it fixes some of the stale squiggly cases we knew of. Can you check the two comments I left on the PR as well? Thanks for the earlier pointers!

ahoppen · 2023-05-12T20:54:30Z

Sources/SourceKitLSP/Swift/SwiftLanguageServer.swift

+    // in addition to the existing push-based publish notifications.
+    // If the client supports pull diagnostics, we report the capability
+    // and we should disable the publish notifications to avoid double-reporting.
+    return clientCapabilities.textDocument?.diagnostic == nil


I think this is the right approach. Correct me if you think I’m wrong but checking for dynamicRegistration would only check if the client supports dynamic registration of the capability. If, for some reason, it supported registration of the capability at server initialization but not dynamically after the fact, checking for dynamicRegistration wouldn’t catch that.

ahoppen · 2023-05-12T20:55:23Z

Sources/SourceKitLSP/Swift/SwiftLanguageServer.swift

+    // Since LSP 3.17.0, diagnostics can be reported through pull-based requests,
+    // in addition to the existing push-based publish notifications.
+    // If the client supports pull diagnostics, we report the capability
+    // and we should disable the publish notifications to avoid double-reporting.


Is it documented somewhere that LSP servers shouldn’t publish diagnostics if the client supports pullDiagnostics? Or, if not, do you know if other LSP servers are also doing this as well or if VSCode is relying on it?

I think the two can be mix-and-matched if they use different provider names, but it wouldn't be desirable here because we would be reporting the same diagnostics twice, and the push-model ones would still have the same staleness issue being addressed by pull-model.

I’m just worried that some LSP client says that it supports pull diagnostics but for some reason still expects that diagnostics get published. That’s why I’d like to know what other LSP servers do…

I suggest a solution to this here: /~https://github.com/apple/sourcekit-lsp/pull/746/files#r1206839125

Sources/SourceKitLSP/Swift/SwiftLanguageServer.swift

ahoppen

Thank you for implementing support for the diagnostics request. The implementation is actually a lot shorter than I thought it would be, so great job! I’ve got a couple of questions, but overall, this looks great to me.

One request though: Could you also write a test case for diagnostic pulling?

tristanlabelle · 2023-05-15T13:36:35Z

@ahoppen

One request though: Could you also write a test case for diagnostic pulling?

I'll do my best. I was under the impression that the sourcekit-lsp tests weren't working on Windows and my macOS-fu is poor at best. :/

compnerd · 2023-05-15T13:54:29Z

@ahoppen

One request though: Could you also write a test case for diagnostic pulling?

I'll do my best. I was under the impression that the sourcekit-lsp tests weren't working on Windows and my macOS-fu is poor at best. :/

The tests worked (at one point) on Windows. They cannot be built currently due to the swift-syntax dependency and SPM miscompiling code. You should be able to roll back to prior to the swift-syntax adoption, port your code to that point and run the tests.

Alternatively, you can enumerate all test cases into a manifest, add CMake rules, and build the tests locally with CMake.

tristanlabelle · 2023-05-15T14:31:40Z

Or someone could ask swift-ci to build and test this PR for me? 😅 I have added a simple test mimicking the publish diagnostics test so there's a nonzero chance that it'll pass without me having to do much local build shenanigans... this time around.

compnerd · 2023-05-15T14:33:36Z

@swift-ci please test

ahoppen · 2023-05-22T18:11:25Z

Or someone could ask swift-ci to build and test this PR for me? 😅 I have added a simple test mimicking the publish diagnostics test so there's a nonzero chance that it'll pass without me having to do much local build shenanigans... this time around.

Sorry, I have meant to reply to this one and then forgot. I ran the test case locally and it had a few syntactic issues but I was able to fix them easily. Unfortunately, I forgot about the changes and discarded them. I can fix your test case again after you addressed #746 (comment).

tristanlabelle · 2023-05-22T20:35:54Z

@ahoppen On it! Sorry I was pulled into other things momentarily but I aim to get this all wrapped up and merged by EOW.

ahoppen · 2023-05-22T20:49:28Z

No worries, @tristanlabelle. Thanks for doing this work in the first place.

MaxDesiatov · 2023-05-31T12:38:15Z

@swift-ci test

MaxDesiatov · 2023-05-31T12:38:43Z

@swift-ci test windows

MaxDesiatov · 2023-05-31T14:44:38Z

@swift-ci test

ahoppen

Thanks. I think we have landed at a good design now and I’m curious to see if we’ll get a response from issue you filed to the LSP spec repo.

Also the refactoring of how we check for capabilities looks like a general improvement to me which usually shows that we’re on the right track. Thanks for your continued effort on this PR.

I’ve got two minor comments inline, otherwise, I think this is good to be merged.

ahoppen · 2023-05-31T17:30:35Z

Sources/SourceKitLSP/Swift/SwiftLanguageServer.swift

+    // FIXME: cancellation
+    _ = handle


I think you should be able to implement cancellation similar to here

/~https://github.com/apple/sourcekit-lsp/blob/b7d427069c869efa1ae8764879935e6b877dab59/Sources/SourceKitLSP/Swift/OpenInterface.swift#L97-L101

Could we leave this to a PR that fixes all cancellation at once? This was copied from many places

Sounds good to me as well

Tests/SourceKitLSPTests/PullDiagnosticsTests.swift

Co-authored-by: Alex Hoppen <alex@alexhoppen.de>

tristanlabelle · 2023-05-31T18:53:21Z

I would appreciate a ci kick. I'm still writing the unit test blindly due to current Windows build limitations. (@ahoppen if you get a chance to build locally that'd be super helpful)

MaxDesiatov · 2023-05-31T19:13:00Z

@swift-ci test

ahoppen · 2023-05-31T20:13:40Z

I just ran the test locally and if you apply the following diff, they pass

diff --git a/Tests/SourceKitLSPTests/PullDiagnosticsTests.swift b/Tests/SourceKitLSPTests/PullDiagnosticsTests.swift
index fa2b43f8..a28f23f4 100644
--- a/Tests/SourceKitLSPTests/PullDiagnosticsTests.swift
+++ b/Tests/SourceKitLSPTests/PullDiagnosticsTests.swift
@@ -16,6 +16,10 @@ import SKTestSupport
 import XCTest
 
 final class PullDiagnosticsTests: XCTestCase {
+  enum Error: Swift.Error {
+    case unexpectedDiagnsoticReport
+  }
+
   /// Connection and lifetime management for the service.
   var connection: TestSourceKitServer! = nil
 
@@ -61,8 +65,7 @@ final class PullDiagnosticsTests: XCTestCase {
     }
 
     guard case .full(let fullReport) = report else {
-      XCTFail("Unexpected diagnostics report type: \(report)")
-      return
+      throw Error.unexpectedDiagnsoticReport
     }
 
     return fullReport.items

tristanlabelle · 2023-05-31T20:20:40Z

Thank you, @ahoppen ! I applied your diff.

ahoppen · 2023-05-31T21:07:03Z

@swift-ci Please test

tristanlabelle · 2023-06-01T14:21:25Z

Seems like the Windows build went out to lunch, but the test now passes on Linux and macOS! 🎉

MaxDesiatov · 2023-06-01T14:25:39Z

@swift-ci test windows

MaxDesiatov · 2023-06-01T14:25:58Z

Windows builds need to be started separately, unfortunately.

tristanlabelle · 2023-06-01T16:32:59Z

Woo! All green :D

ahoppen · 2023-06-01T16:54:36Z

@tristanlabelle Can you also cherry-pick this PR to the release/5.9 branch? I think it would be useful to get this into the 5.9 release.

tristanlabelle · 2023-06-01T18:00:58Z

Cherry-picked to #751 (after requashing - this PR was completed by merging so there's a bit of my dirty laundry in the history now)

tristanlabelle commented May 11, 2023

View reviewed changes

tristanlabelle closed this May 11, 2023

tristanlabelle reopened this May 11, 2023

tristanlabelle marked this pull request as ready for review May 11, 2023 21:18

tristanlabelle requested review from benlangmuir and ahoppen as code owners May 11, 2023 21:18

ahoppen reviewed May 12, 2023

View reviewed changes

tristanlabelle added 8 commits May 26, 2023 10:35

Implement pull-model documentDiagnostics

542a29d

Remove testing code

e778ca2

Report diagnostics support in SourceKitServer.

bc44451

Add test for pul diagnostics

aae1058

Fix compilation errors in test.

abd19e1

Use CapabilityRegistry in ToolchainLanguageServer implementations

0da6291

Compilation fixes

22234a0

Fixed CodeCompletion build error

94a62c1

Remove reporting diagnostic support at SourceKitServer initialization

e91c83f

ahoppen reviewed May 31, 2023

View reviewed changes

Fix indentation

e9c069d

Co-authored-by: Alex Hoppen <alex@alexhoppen.de>

fix guard clause in test not returning

5e1f667

ahoppen approved these changes May 31, 2023

View reviewed changes

Fix unit test

ee043b5

ahoppen merged commit d66705c into swiftlang:main Jun 1, 2023

tristanlabelle deleted the implement-pull-document-diagnostics branch June 1, 2023 17:41

tristanlabelle mentioned this pull request Jun 1, 2023

Implement pull-model documentDiagnostics (cherry-pick from main) #751

Merged

tristanlabelle mentioned this pull request Jul 17, 2023

stale sourcekit-lsp problems in problem list swiftlang/vscode-swift#519

Closed

Implement pull-model documentDiagnostics #746

Implement pull-model documentDiagnostics #746

Conversation

tristanlabelle commented May 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tristanlabelle commented May 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahoppen left a comment

Choose a reason for hiding this comment

tristanlabelle commented May 15, 2023

compnerd commented May 15, 2023

tristanlabelle commented May 15, 2023

compnerd commented May 15, 2023

ahoppen commented May 22, 2023

tristanlabelle commented May 22, 2023

ahoppen commented May 22, 2023

MaxDesiatov commented May 31, 2023

MaxDesiatov commented May 31, 2023

MaxDesiatov commented May 31, 2023

ahoppen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tristanlabelle commented May 31, 2023

MaxDesiatov commented May 31, 2023

ahoppen commented May 31, 2023

tristanlabelle commented May 31, 2023

ahoppen commented May 31, 2023

tristanlabelle commented Jun 1, 2023

MaxDesiatov commented Jun 1, 2023

MaxDesiatov commented Jun 1, 2023 • edited Loading

tristanlabelle commented Jun 1, 2023

ahoppen commented Jun 1, 2023

tristanlabelle commented Jun 1, 2023

tristanlabelle commented May 11, 2023 •

edited

Loading

MaxDesiatov commented Jun 1, 2023 •

edited

Loading