Reload Swift Package when new file creation is indicated by `DidChangeWatchedFileNotification` #443

ahoppen · 2022-01-07T17:48:49Z

Implement rudementary support for DidChangeWatchedFileNotification for SwiftPM projects: When a file is added, reload the Swift package to compute build settings for the new file.

This enables proper semantic functionality added to the project after the LSP server was started.

Resolves rdar://87149128 SR-15633.

ahoppen · 2022-01-07T17:49:51Z

@swift-ci Please test

benlangmuir

reloadPackages needs to be taught to notify the build system delegate of the settings changes using delegate.fileBuildSettingsChanged for all the registered URLs (and we'll need to start keeping a list of the registered URLs in the SwiftPM build system). That will fix the settings in files that were already open before the change.

Excited to see this happening!

Sources/SKSwiftPMWorkspace/SwiftPMWorkspace.swift

Sources/SourceKitLSP/SourceKitServer.swift

benlangmuir · 2022-01-07T18:23:09Z

Sources/SKCore/BuildSystemManager.swift

@@ -125,6 +125,11 @@ public final class BuildSystemManager {
    self.fallbackSettingsTimeout = fallbackSettingsTimeout
    self.buildSystem?.delegate = self
  }
+
+  public func filesDidChange(_ events: [FileEvent]) {
+    buildSystem?.filesDidChange(events)


This should probably be wrapped in queue.async. Otherwise we are doing settings queries on the build system manager queue, but triggering file changes on the main server queue, which is racy. Alternatively we could make the SwiftPM build system itself thread-safe, but that seems like more boilerplate. Hopefully we can switch to swift concurrency and it won't be so annoying.

Also running the BuildSystem.filesDidChange code async is beneficial I think so we don’t block the queue that receives the LSP messages.

benlangmuir · 2022-01-07T18:39:56Z

Also, don't forget to remove this from the README:

SwiftPM build settings are not updated automatically after files are added/removed.

Workaround: close and reopen the project after adding/removing files

😸

Sources/SourceKitLSP/CapabilityRegistry.swift

Sources/SourceKitLSP/SourceKitServer.swift

Sources/SKSwiftPMWorkspace/SwiftPMWorkspace.swift

DavidGoldman · 2022-01-07T19:57:07Z

Tests/SourceKitLSPTests/SwiftPMIntegration.swift

+
+    let newFile = ws.testLoc("newFile:call")
+
+    // Check that we don't get cross-file code completion befor we sent a `DidChangeWatchedFilesNotification` to make sure we didn't include the file in the initial retrieval of build settings.


before we send

DavidGoldman · 2022-01-07T20:23:03Z

reloadPackages needs to be taught to notify the build system delegate of the settings changes using delegate.fileBuildSettingsChanged for all the registered URLs (and we'll need to start keeping a list of the registered URLs in the SwiftPM build system). That will fix the settings in files that were already open before the change.

Excited to see this happening!

Yep, this is great! :D Thanks for working on it

ahoppen · 2022-01-10T17:49:55Z

Thanks for the feedback @benlangmuir and @DavidGoldman. I addressed the inline comments by squashing the first commit and introduced notification of the build system delegate on package reloading in a second commit.

The points still open are:

Should we also listen for deleted files? (Reload Swift Package when new file creation is indicated by DidChangeWatchedFileNotification #443 (comment))
Watch for changes to Package.swift, which I’ll add in a follow-up PR (Reload Swift Package when new file creation is indicated by DidChangeWatchedFileNotification #443 (comment))

ahoppen · 2022-01-10T18:29:42Z

@swift-ci Please test

ahoppen · 2022-01-10T20:41:46Z

Sources/SKSwiftPMWorkspace/SwiftPMWorkspace.swift

+  private func isFileEventRelevant(event: FileEvent) -> Bool {
+    // FIXME: fileRules should also include self.workspace.additionalFileRules and toolsVersion should be self.workspace.currentToolsVersion
+    let fileRules = FileRuleDescription.builtinRules
+    let toolsVersion = ToolsVersion.currentToolsVersion
+
+    guard let fileURL = event.uri.fileURL else {
+      return false
+    }
+    let filePath = AbsolutePath(fileURL.path)
+    switch event.type {
+    case .created, .deleted:
+      return fileRules.contains(where: { $0.match(path: filePath, toolsVersion: toolsVersion) })
+    case .changed:
+      return false
+    default: // Unknown file change type
+      return false
+    }
+  }


@neonichu Could you take a look whether this code makes sense? The intention is to match only files which might impact the build settings computed by SwiftPM. If you think this is correct, I’ll open a PR on SwiftPM to make Workspace.additionalFileRules and Workspace.currentToolsVersion public.

I think it might be better to find a way to refactor TargetSourcesBuilder in SwiftPM to provide this kind of information instead of interacting with file rules directly.

Do you have any particular path in mind how Workspace should expose the TargetSourcesBuilder? AFAICT it only creates an ad-hoc PackageBuilder, which in turn creates an ad-hoc TargetSourcesBuilder.

That's a good question, do you have an existing package graph at this point from which you could get a TargetDescription to pass down? If yes, maybe just a new function on Workspace that gets package identity etc and the target description and returns a configured TargetSourcesBuilder? Additionally, I think target sources builder would need a function that would decide whether a singular file path is part of the given package or not.

I do, but I don’t seem to be able to get a TargetDescription from a PackageGraph. AFAICT it only stores ResolvedTargets and Targets but there’s no way to go back to a TargetDescription. Also, I couldn’t really figure out where TargetDescriptions are usually getting created. The places I found where the TargetDescription constructor is being called were the following and none of these seemed to match general-purpose target creation:

ManifestJSONParser.parseTarget (I don’t know why target creation should go through JSON serialization)

ManifestLoader.loadFile (deals with legacy system packages)

PackageBuilder.createSnippetTargets (I don’t exactly know what snippets are, but they don’t seem to be the general target creation mechanism either)

Ah you're right, I wonder whether we could change TargetSourcesBuilder to take a ResolvedTarget instead. This is turning out to be a bit of a bigger change than I originally thought.

I guess another option could be that we add a new function to Workspace to which you pass all the info you have and for now it would just do a simple check using file rules, but we could later refine it to be more accurate. That way we can decouple your PR from making bigger changes to SwiftPM.

BTW, ManifestJSONParser.parseTarget is the general purpose target creation, because it is just done as part of manifest loading. We run the compiled manifest, it produces JSON and then we load that JSON.

After some offline discussion with @neonichu, we have decided to only watch files matching extensions specified in FileRuleDescription.builtinRules in the client. Additionally, I’m adding a function to SwiftPM that will allow more granular filtering based on e.g. the directory structure of a package in the future: swiftlang/swift-package-manager#4016

DavidGoldman · 2022-01-11T00:42:50Z

Sources/SKSwiftPMWorkspace/SwiftPMWorkspace.swift

+    if events.contains(where: { isFileEventRelevant(event: $0) }) {
+      // TODO: It should not be necessary to reload the entire package just to get build settings for one file.
+      orLog {
+        try self.reloadPackage()


Do we need to do this on self.queue as well?

It doesn’t need to be, because BuildSystemManager is already guarding against race conditions in BuildSystemManager.filesDidChange by informing the build systems about file changes serially on its own queue.

I think it does since registerForChangeNotifications (on queue) could be called right before filesDidChange, and then reloadPackage (off queue) could mutate packageGraph/fileToTarget/sourceDirToTarget while registerForChangeNotifications is reading it on the queue, right? So we need some way to synchronize that.

Oh, yes, we also need to guard packageGraph, fileToTarget and sourceDirToTarget against race conditions. I only thought of watchedFiles and delegate before. Added the guards in 2e3b783.

ahoppen · 2022-01-13T14:43:09Z

swiftlang/swift-package-manager#4016

@swift-ci Please test

benlangmuir · 2022-01-13T16:12:45Z

Sources/SKSwiftPMWorkspace/SwiftPMWorkspace.swift

+  /// - `packageGraph`
+  /// - `fileToTarget`
+  /// - `sourceDirToTarget`
+  let queue: DispatchQueue = .init(label: "SwiftPMWorkspace.queue", qos: .background)


Just noticed this: we probably want utility not background. Settings queries block language features, but they're done asyncrhonously so there is no qos donation.

I wonder how much of a difference it makes in practice. But yes, the description of .utility does seem to match more closely to what we are doing.

benlangmuir · 2022-01-13T16:16:54Z

Tests/SourceKitLSPTests/SwiftPMIntegration.swift

@@ -56,4 +56,88 @@ final class SwiftPMIntegrationTests: XCTestCase {
        deprecated: false),
    ])
  }
+
+  func testAddFile() throws {


Need to update Linux tests. Incidentally, I wonder if we could switch to automatic test discovery on Linux at this point - it's been around for a while now.

Thanks for catching this. I’ll try to switch to automatic test discovery in a follow-up PR.

ahoppen · 2022-01-14T12:26:33Z

swiftlang/swift-package-manager#4016

@swift-ci Please test

benlangmuir · 2022-01-14T17:46:48Z

Sources/SourceKitLSP/SourceKitServer.swift

@@ -21,6 +21,7 @@ import SKSupport
 import TSCBasic
 import TSCLibc
 import TSCUtility
+import PackageLoading


It would be nice if we could contain all imports of SwiftPM modules to the SwiftPM build system file to keep things contained. No need to change it in this PR though.

…WatchedFileNotification Implement rudementary support for `DidChangeWatchedFileNotification` for SwiftPM projects: When a file is added, reload the Swift package to compute build settings for it. This enables proper semantic functionality added to the project after the LSP server was started. Resolves SR-15633

…kage is reloaded

…nges

…space by queue These properties might be modified by `reloadPackage` (which can be called asynchronously) and thus need to be guareded by a queue.

ahoppen · 2022-01-19T10:20:30Z

swiftlang/swift-package-manager#4016

@swift-ci Please test

ahoppen requested review from DavidGoldman and benlangmuir January 7, 2022 17:48

benlangmuir reviewed Jan 7, 2022

View reviewed changes

DavidGoldman reviewed Jan 7, 2022

View reviewed changes

ahoppen force-pushed the pr/didchangewatchedfilesnotification branch from 8503a3f to ee5dbf4 Compare January 10, 2022 17:47

ahoppen force-pushed the pr/didchangewatchedfilesnotification branch from ee5dbf4 to b156d6f Compare January 10, 2022 18:29

ahoppen commented Jan 10, 2022

View reviewed changes

DavidGoldman reviewed Jan 11, 2022

View reviewed changes

ahoppen mentioned this pull request Jan 13, 2022

Add function to check whether a file affects swift or clang build settings swiftlang/swift-package-manager#4016

Merged

ahoppen force-pushed the pr/didchangewatchedfilesnotification branch 2 times, most recently from 533d3b3 to 49b2518 Compare January 13, 2022 14:43

benlangmuir reviewed Jan 13, 2022

View reviewed changes

ahoppen force-pushed the pr/didchangewatchedfilesnotification branch from 49b2518 to 7d957e6 Compare January 14, 2022 10:55

adam-fowler mentioned this pull request Jan 14, 2022

Check adding new file doesn't require a restart swiftlang/vscode-swift#97

Closed

benlangmuir approved these changes Jan 14, 2022

View reviewed changes

ahoppen added 4 commits January 19, 2022 11:20

Notify SwiftPMWorkspace’s delegate of build settings changes if pac…

a5defc6

…kage is reloaded

Only reload SwiftPM package if a file that is relevant to SwiftPM cha…

fda6326

…nges

Guard packageGraph, fileToTarget and sourceDirToTarget on SwiftPMWork…

9fb40da

…space by queue These properties might be modified by `reloadPackage` (which can be called asynchronously) and thus need to be guareded by a queue.

ahoppen force-pushed the pr/didchangewatchedfilesnotification branch from 7d957e6 to 9fb40da Compare January 19, 2022 10:20

ahoppen merged commit 0f51262 into swiftlang:main Jan 24, 2022

ahoppen deleted the pr/didchangewatchedfilesnotification branch January 24, 2022 18:58

This was referenced Jan 24, 2022

[5.6] Reload Swift Package when new file creation is indicated by DidChangeWatchedFileNotification #448

Merged

[5.6] Set didChangedWatchedFiles glob to **/*.ext #453

Merged

ahoppen mentioned this pull request Jan 7, 2022

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reload Swift Package when new file creation is indicated by `DidChangeWatchedFileNotification` #443

Reload Swift Package when new file creation is indicated by `DidChangeWatchedFileNotification` #443

ahoppen commented Jan 7, 2022 •

edited

Loading

ahoppen commented Jan 7, 2022

benlangmuir left a comment

benlangmuir Jan 7, 2022

ahoppen Jan 10, 2022

benlangmuir commented Jan 7, 2022

DavidGoldman Jan 7, 2022

DavidGoldman commented Jan 7, 2022

ahoppen commented Jan 10, 2022

ahoppen commented Jan 10, 2022

ahoppen Jan 10, 2022

neonichu Jan 10, 2022

ahoppen Jan 10, 2022

neonichu Jan 10, 2022

ahoppen Jan 11, 2022

neonichu Jan 11, 2022

ahoppen Jan 13, 2022

DavidGoldman Jan 11, 2022

ahoppen Jan 11, 2022

DavidGoldman Jan 12, 2022

ahoppen Jan 12, 2022

ahoppen commented Jan 13, 2022

benlangmuir Jan 13, 2022

ahoppen Jan 14, 2022

benlangmuir Jan 13, 2022

ahoppen Jan 14, 2022

ahoppen commented Jan 14, 2022

benlangmuir Jan 14, 2022

ahoppen commented Jan 19, 2022


		let newFile = ws.testLoc("newFile:call")

		// Check that we don't get cross-file code completion befor we sent a `DidChangeWatchedFilesNotification` to make sure we didn't include the file in the initial retrieval of build settings.

Reload Swift Package when new file creation is indicated by DidChangeWatchedFileNotification #443

Reload Swift Package when new file creation is indicated by DidChangeWatchedFileNotification #443

Conversation

ahoppen commented Jan 7, 2022 • edited Loading

ahoppen commented Jan 7, 2022

benlangmuir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benlangmuir commented Jan 7, 2022

Choose a reason for hiding this comment

DavidGoldman commented Jan 7, 2022

ahoppen commented Jan 10, 2022

ahoppen commented Jan 10, 2022

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

Choose a reason for hiding this comment

ahoppen commented Jan 13, 2022

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 commented Jan 14, 2022

Choose a reason for hiding this comment

ahoppen commented Jan 19, 2022

Reload Swift Package when new file creation is indicated by `DidChangeWatchedFileNotification` #443

Reload Swift Package when new file creation is indicated by `DidChangeWatchedFileNotification` #443

ahoppen commented Jan 7, 2022 •

edited

Loading