Additions to the LSP module for `workspace/didChangeWatchedFiles` #376

DavidGoldman · 2021-03-14T17:06:35Z

We can then use this functionality to allow a BuildSystem to watch
files (e.g. via the client or even in-process file watching),
which would allow us to do things like detect new files and provide
accurate build settings for them.

Sources/LanguageServerProtocol/SupportTypes/FileSystemWatcher.swift

DavidGoldman · 2021-03-14T17:08:28Z

Sources/LanguageServerProtocol/SupportTypes/RegistrationOptions.swift

+
+import Foundation
+
+/// FIXME: Find a way to abstract this without deserializing.


I'm not sure of a better generalization here, we could do something like what we do for Notifications/Requests but to store it we'd need to type erase it/box it (e.g. an AnyRegistrationOptions)

There aren't that many of these - is there a reason not to just make encodeToLSPAny a protocol requirement and implement it concretely for each concrete type that implements the protocol?

DavidGoldman · 2021-03-14T17:16:58Z

@swift-ci please test

Sources/LanguageServerProtocol/SupportTypes/FileEvent.swift

Sources/LanguageServerProtocol/SupportTypes/FileSystemWatcher.swift

Sources/LanguageServerProtocol/SupportTypes/ServerCapabilities.swift

Sources/LanguageServerProtocol/Requests/RegisterCapabilityRequest.swift

benlangmuir · 2021-03-15T18:17:38Z

Sources/LanguageServerProtocol/SupportTypes/RegistrationOptions.swift

+
+import Foundation
+
+/// FIXME: Find a way to abstract this without deserializing.


There aren't that many of these - is there a reason not to just make encodeToLSPAny a protocol requirement and implement it concretely for each concrete type that implements the protocol?

We can then use this functionality to allow a `BuildSystem` to watch files (e.g. via the client or even in-process file watching), which would allow us to do things like detect new files and provide accurate build settings for them.

DavidGoldman · 2021-03-15T20:36:20Z

There aren't that many of these - is there a reason not to just make encodeToLSPAny a protocol requirement and implement it concretely for each concrete type that implements the protocol?

We could but then we'd probably want to implement it for all of the transitive types embedded there, also note that at the moment these really would only ever be encoded, not decoded since only the server sends them over.

benlangmuir · 2021-03-15T20:49:06Z

We could but then we'd probably want to implement it for all of the transitive types embedded there, also note that at the moment these really would only ever be encoded, not decoded since only the server sends them over.

This is fine for now as long as we don't have a use case, but we generally expect to be able to write both client and server using this code.

We could but then we'd probably want to implement it for all of the transitive types embedded there

Is there something I'm missing? The ones added so far seem straightforward/shallow in terms of object graph.

Tests/LanguageServerProtocolTests/LanguageServerProtocolTests.swift

DavidGoldman · 2021-03-15T20:54:00Z

We could but then we'd probably want to implement it for all of the transitive types embedded there, also note that at the moment these really would only ever be encoded, not decoded since only the server sends them over.

This is fine for now as long as we don't have a use case, but we generally expect to be able to write both client and server using this code.

We could but then we'd probably want to implement it for all of the transitive types embedded there

Is there something I'm missing? The ones added so far seem straightforward/shallow in terms of object graph.

Do we intend to keep LSPAny like it currently is? If so I wonder if it would be more useful to have a generalized encoder/decoder for it. Although that will be a bit more work it would save manually implementing all of these encoders/decoders. Also note that I didn't include all of the RegistrationOptions, there are more complex ones.

benlangmuir · 2021-03-15T21:04:52Z

Do we intend to keep LSPAny like it currently is?

Is there a good alternative? We're using it for places where the spec allows any. It would be nice if this was builtin to Foundation so that we could transparently encode/decode between string and LSPAny and between LSPAny and Codable types.

DavidGoldman · 2021-03-15T21:21:45Z

Do we intend to keep LSPAny like it currently is?

Is there a good alternative? We're using it for places where the spec allows any. It would be nice if this was builtin to Foundation so that we could transparently encode/decode between string and LSPAny and between LSPAny and Codable types.

Right, if we had a proper JSON type that had Encoder/Decoder we could use that instead of LSPAny and LSPAnyCodable, I guess I was asking if we planned to keep LSPAnyCodable. The simplest thing to do here would be to implement LSPAnyCodable on all of the referenced types, but a general fix would be proper coders

benlangmuir · 2021-03-15T21:33:09Z

I guess I was asking if we planned to keep LSPAnyCodable

LSPAnyCodable is a workaround for the lack of proper encoders/decoders, so from that perspective I'd be fine with getting rid of it if we had proper support. That being said, I'm not sure how big or complex the proper encoders/decoders would be to implement. If proper support reduced the number of lines of code by letting us throw out the LSPAnyCodable implementations, it would probably be an obvious win, but if it ends up being bigger than what we have it's harder to justify the maintenance.

DavidGoldman · 2021-03-16T13:38:18Z

LSPAnyCodable is a workaround for the lack of proper encoders/decoders, so from that perspective I'd be fine with getting rid of it if we had proper support. That being said, I'm not sure how big or complex the proper encoders/decoders would be to implement. If proper support reduced the number of lines of code by letting us throw out the LSPAnyCodable implementations, it would probably be an obvious win, but if it ends up being bigger than what we have it's harder to justify the maintenance.

After looking into the stdlib's JSONEncoder/JSONDecoder which is quite large and complicated, I agree that we should stick with LSPAnyCodable for now, I'll swap it over.

Also regenerate Linux main

DavidGoldman · 2021-03-16T16:29:18Z

@swift-ci please test

benlangmuir · 2021-03-16T17:14:30Z

Sources/LanguageServerProtocol/SupportTypes/LSPAny.swift

+
+  public func encodeToLSPAny() -> LSPAny {
+    guard let wrapped = self else { return .null }
+    return wrapped.encodeToLSPAny()


Do we actually use the null encoding anywhere, or is this just for completeness? If possible I strongly prefer skipping nil values instead of encoding them as null since that seems to play nicer with other tools and saves space.

Edit: to be clear, I'm fine with keeping this conformance of Optional: LSPAnyCodable, particularly so that we can decode it correctly, I'm just wondering if it's used concretely in our code anywhere.

Swapped over the place that used it

benlangmuir · 2021-03-16T17:20:12Z

Tests/SourceKitDTests/XCTestManifests.swift

@@ -23,6 +35,7 @@ extension SourceKitDTests {

 public func __allTests() -> [XCTestCaseEntry] {
    return [
+        testCase(CrashRecoveryTests.__allTests__CrashRecoveryTests),


Oops! Hope these are working on Linux...

Hmm, CI is failing but those errors link to

/home/buildnode/jenkins/workspace/swift-sourcekit-lsp-PR-Linux/tmp/TemporaryDirectory.YD3VQ4/pkg/Package.swift:3:5: error: type annotation missing in pattern 14:18:28 let pack

That seems like an unrelated issue that CI is picking up since it was present and enabled before in SwiftPMWorkspaceTests.testUnparsablePackage or perhaps something related to the diagnostics changed?

Hmm it might be related to the sourcekitd tests?

From the logs:

Test Suite 'Selected tests' started at 2021-03-16 13:18:18.663 Test Suite 'CrashRecoveryTests' started at 2021-03-16 13:18:18.666 Test Case 'CrashRecoveryTests.testSourcekitdCrashRecovery' started at 2021-03-16 13:18:18.666

But I don't see it ever report the status of that test...

It's failing in CrashRecoveryTests.testSourcekitdCrashRecovery, which makes some sense since sourcekitd is in-process on Linux, so that test doesn't really make sense. We should just skip it on non-Darwin platforms. Feel free to either do that in this patch (some kind of try XCTSkipIf(...)) or else remove the changes related to enabling these tests on Linux and let me or @ahoppen deal with it.

Gotcha, done

DavidGoldman · 2021-03-17T17:03:46Z

@swift-ci please test

DavidGoldman · 2021-03-17T17:36:27Z

@swift-ci please test

compnerd · 2021-03-18T02:26:34Z

This broke the Windows bootstrap :-(

Update the source file list for the new files. This repairs the bootstrap build on Windows.

build: update the CMakeLists.txt after #376

DavidGoldman · 2021-03-18T16:03:27Z

Is there any CI or pre-merge hooks for windows support? Or a script to run to update the CMake files or do you have to do it manually?

compnerd · 2021-03-18T19:47:27Z

There're nightly builds at https://dev.azure.com/compnerd/swift-build

The CMake files need to be updated manually.

DavidGoldman requested a review from benlangmuir as a code owner March 14, 2021 17:06

DavidGoldman commented Mar 14, 2021

View reviewed changes

Sources/LanguageServerProtocol/SupportTypes/FileSystemWatcher.swift Outdated Show resolved Hide resolved

DavidGoldman commented Mar 14, 2021

View reviewed changes

benlangmuir reviewed Mar 15, 2021

View reviewed changes

DavidGoldman force-pushed the filewatching branch from c382985 to d3ba9ef Compare March 15, 2021 20:33

benlangmuir reviewed Mar 15, 2021

View reviewed changes

Tests/LanguageServerProtocolTests/LanguageServerProtocolTests.swift Outdated Show resolved Hide resolved

Tests/LanguageServerProtocolTests/LanguageServerProtocolTests.swift Show resolved Hide resolved

DavidGoldman added 2 commits March 16, 2021 11:25

Improve #file and #line for LSP test errors

4c6d85c

Also regenerate Linux main

Add LSPAnyCodable protocol requirement to RegistrationOptions

35252e2

benlangmuir reviewed Mar 16, 2021

View reviewed changes

benlangmuir approved these changes Mar 16, 2021

View reviewed changes

Don't encode an optional documentSelector to null

eca97ff

Skip running testSourcekitdCrashRecovery when not on macOS

ad31fd0

DavidGoldman merged commit e6b0796 into swiftlang:main Mar 17, 2021

compnerd added a commit to compnerd/sourcekit-lsp that referenced this pull request Mar 18, 2021

build: update the CMakeLists.txt after swiftlang#376

fc34ed8

Update the source file list for the new files. This repairs the bootstrap build on Windows.

benlangmuir added a commit that referenced this pull request Mar 18, 2021

Merge pull request #378 from compnerd/cmake

cbfa1cf

build: update the CMakeLists.txt after #376

adam-fowler mentioned this pull request May 5, 2022

[SR-15633] SourceKit-LSP: New files are not registered and changes to Package.swift are not reflected #508

Closed


		import Foundation

		/// FIXME: Find a way to abstract this without deserializing.

Additions to the LSP module for workspace/didChangeWatchedFiles #376

Additions to the LSP module for workspace/didChangeWatchedFiles #376

Uh oh!

Conversation

DavidGoldman commented Mar 14, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidGoldman commented Mar 14, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidGoldman commented Mar 15, 2021

Uh oh!

benlangmuir commented Mar 15, 2021

Uh oh!

Uh oh!

Uh oh!

DavidGoldman commented Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benlangmuir commented Mar 15, 2021

Uh oh!

DavidGoldman commented Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benlangmuir commented Mar 15, 2021

Uh oh!

DavidGoldman commented Mar 16, 2021

Uh oh!

DavidGoldman commented Mar 16, 2021

Uh oh!

benlangmuir Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidGoldman commented Mar 17, 2021

Uh oh!

DavidGoldman commented Mar 17, 2021

Uh oh!

compnerd commented Mar 18, 2021

Uh oh!

DavidGoldman commented Mar 18, 2021

Uh oh!

compnerd commented Mar 18, 2021

Uh oh!

Uh oh!

Additions to the LSP module for `workspace/didChangeWatchedFiles` #376

Additions to the LSP module for `workspace/didChangeWatchedFiles` #376

DavidGoldman commented Mar 15, 2021 •

edited

Loading

DavidGoldman commented Mar 15, 2021 •

edited

Loading

benlangmuir Mar 16, 2021 •

edited

Loading