SourceKit-LSP not registering new files

Development SourceKit-LSP
1

This has been a long standing issue and one which frustrates many people. I have a local copy of the SSWG swift extension which sends workspace/didChangeWatchedFiles to the server but this is still not enough for this to be resolved. So I added the bug [SR-15633] SourceKit-LSP: New files are not registered and changes to Package.swift are not reflected · Issue #508 · apple/sourcekit-lsp · GitHub to track this issue.

I have a couple of questions related to this

  1. The solution at the moment is to close/re-open the project. This is not ideal so I am looking to add a trigger to restart the LSP server when ever a file is created or deleted in VSCode. Does anyone see any issue with doing this apart from it being a bit of overkill just to get a file registered with the server. This won't catch new files added via other methods, like updating from git.
  2. When a solution is implemented in SourceKit-LSP will I still be expected to send workspace/didChangeWatchedFiles messages or will SourceKit-LSP have its own file system watcher.
3 Likes
2

Hi Adam,

I just put together a rudimentary implementation of file watching in SourceKit-LSP apple/sourcekit-lsp#443, which reloads the entire package when a new file gets added. It’s still sub-optimal but I’d rather handle it on the SourceKit-LSP side than have the VSCode plugin restart the LSP server. Maybe we’ll be able to improve from there.

Regarding file watching, I found the following in the LSP spec:

Servers are allowed to run their own file system watching mechanism and not rely on clients to provide file system events. However this is not recommended due to the following reasons:

  • to our experience getting file system watching on disk right is challenging, especially if it needs to be supported across multiple OSes.
  • file system watching is not for free especially if the implementation uses some sort of polling and keeps a file system tree in memory to compare time stamps (as for example some node modules do)
  • a client usually starts more than one server. If every server runs its own file system watching it can become a CPU or memory problem.
  • in general there are more server than client implementations. So this problem is better solved on the client side

Since I don’t fancy implementing file watching in SourceKit-LSP, I’d like to stick with the LSP spec’s recommendation of relying on the client for file watching. If I understand the spec correctly, the client is also supposed to send change notifications for files added e.g. by git.

3 Likes
3

There's actually a file watcher in TSCUtility already: https://github.com/apple/swift-tools-support-core/blob/main/Sources/TSCUtility/FSWatch.swift so at least we wouldn't need to own the low-level implementation.

Ideally, I think we would use the LSP file change notifications when they are supported by our client, but fall back to watching the files ourselves otherwise.

4

Using this file watcher extensively in https://carton.dev, I personally found it quite buggy. It can't watch separate files, only directories, and I think there also some issues with how it behaves on latest Big Sur and maybe even Monterey? Still have to thoroughly investigate this breakage. But any attempts to watch a specific file with it, like Package.swift, failed for me, I only had limit success with watching the whole Sources directory with subdirectories in a given Swift project.

Still better than nothing I suppose. Having this LSP bug fixed would unblock me from using Swift outside of Xcode, since the requirement to restart the whole IDE every time a file is added or removed is, to put it mildly, outrageously annoying.

5

That's great Alex. I'm happy to send the didChangedWatchedFiles messages so you shouldn't need a file system watcher. The only issue I see here is that every editor client that uses sourcekit-lsp will have to send these messages. VSCode makes it fairly easy. I don't know what it is like for other IDEs though.

More questions

  • Is there a server capability I can test for, that will indicate the didChangedWatchedFiles are being processed? I would like to fall back to the restart server on new file if these messages aren't being processed.
  • @blangmuir combined the new file bug with the Package.swift changes not being propagated bug. Does your PR also resolve this.

One minor thing to note as well. If I have a workspace with multiple folders and I move between files which are in different folders I am restarting the server to point to the folder for the new file. Does sourcekit-lsp support multiple folders, so I can avoid this?

6

Ok Alex I built a version of sourcekit-lsp from your PR and tested it. Here is what I found

VSCode does all the client work. I don't need to send any didChangedWatchedFiles messages because it does this for me as long as the server returns workspace/didChangedWatchedFiles as a capability, which it is. This answer my question above about recognising a server which supports this.

You request to only watch for adding of files. You will need to watch for removal of files as well.

You are watching for all file changes ** including changes to the .git folder, and build files in the .build folder. Perhaps you should limit your watch to **/*.swift.

Finally it doesn't seem to work. The client is sending the correct messages but I still don't get file completion in a new file.

7

We probably want to ensure that we search for all file types that SwiftPM supports, so .c, .h etc as well

8

Nice :tada:

It doesn’t seem to be strictly necessary at the moment, but it’s goodness to watch for deleted files as well. Changed it (see https://github.com/apple/sourcekit-lsp/pull/443#discussion_r780438117). Thanks for noticing it. :+1:

It’s a little tricky to figure out which file extensions to watch for because SwiftPM packages can also include clang files with all sorts of extensions like c, cpp, c++ etc. I’m trying to implement something a little more clever than what I did in my initial PR here: Reload Swift Package when new file creation is indicated by `DidChangeWatchedFileNotification` by ahoppen · Pull Request #443 · apple/sourcekit-lsp · GitHub.

That’s interesting. It is working for me. Here’s the setup I use:

  • VSCode with the “old” SourceKit-LSP plugin, because I couldn’t figure out how to change the sourcekit-lsp path in the Swift plugin you wrote
  • I open a more or less blank executable package that has the following class defined in main.swift.
class Foo {
    init(myVar: Int) {}
}
  • I create a new file a new file in the sources folder and check that I get completion for myVar: Int after typing Foo(.
9

Sorry my bad it is working. I was using a call in the vscode language client to catch whether workspace/didChangeWorkspaceFiles was being registered as a capability. In doing this I managed to disable the feature on the client side.

Allowing the client to pass every file added/deleted can produce a lot messages to the server in a large project. Maybe it isn't an issue, I guess time will tell.

1 Like
10

Oh also I realised I made a mistake when removing the source kit-lsp path config. It'll be back in the next version.

1 Like
11

Awesome. Let’s try and get the existing changes a little polished and merged and we can always improve from there.

2 Likes
12

Hey Alex, what release of Swift do you think this will make it into?

13

When the PR into main has been merged, I think it would make sense to also cherry-pick it to 5.6. But I don’t want to make any promises.

14

@ahoppen that’d be great if that was the case.

15

The sourcekit-lsp.serverPath configuration value has been resurrected in v0.2.0 of the swift extension.

2 Likes
16

Hey Alex,

I noticed your PR has been merged :tada: . I hope it can make it into 5.6. On bugs.swift.org this bug was merged with the updates to Package.swift not being propagated bug. As I understand it your PR doesn't currently resolve that issue. Will there be another PR to resolve that issue?

cheers

17

Hey Adam,

The change also just made it into 5.6 :tada:

I didn’t have the time to listen for changes to Package.swift yet, even though it should just be about writing a test case and verifying that VSCode actually sends the change notifications when we expect them. I hope, I’ll get to it in the next week or so. Unfortunately, with the Swift 5.6 release winding down, I think that change won’t make it into the release anymore, though. If you want to hot fix this in the VSCode extensions, I would suggest that you send a file created notification followed by a file deleted notification for some arbitrary file whenever you detect changes to Package.swift.

2 Likes
18

Thanks for this fix, it's amazing to see it in included in 5.6! Would it require a corresponding update to the VS Code extension for things to fully work?

19

No update to the extension is required. VSCode does all the work in the background, if the LSP server says it supports the workspace/didChangeWatchedFiles message then VSCode will send the relevant messages.

1 Like
20

@ahoppen I know you weren't keen on the restarting of the LSP server, but do you have any problems with the extension doing this if the swift version is < 5.6