Skip to content

Remove assertion that we can't register for change notifications twice in SwiftPMWorkspace #949

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Sources/SKCore/BuildServerBuildSystem.swift
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ extension BuildServerBuildSystem: BuildSystem {
return buildSettings[document]
}

public func registerForChangeNotifications(for uri: DocumentURI, language: Language) {
public func registerForChangeNotifications(for uri: DocumentURI) {
let request = RegisterForChanges(uri: uri, action: .register)
_ = self.buildServer?.send(request) { result in
if let error = result.failure {
Expand Down
2 changes: 1 addition & 1 deletion Sources/SKCore/BuildSystem.swift
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public protocol BuildSystem: AnyObject {
/// IMPORTANT: When first receiving a register request, the `BuildSystem` MUST asynchronously
/// inform its delegate of any initial settings for the given file via the
/// `fileBuildSettingsChanged` method, even if unavailable.
func registerForChangeNotifications(for: DocumentURI, language: Language) async
func registerForChangeNotifications(for: DocumentURI) async

/// Unregister the given file for build-system level change notifications,
/// such as command line flag changes, dependency changes, etc.
Expand Down
2 changes: 1 addition & 1 deletion Sources/SKCore/BuildSystemManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ extension BuildSystemManager {
// system. That way, iff the main file changes, we will also notify the
// delegate about build setting changes of all header files that are based
// on that main file.
await buildSystem?.registerForChangeNotifications(for: mainFile, language: language)
await buildSystem?.registerForChangeNotifications(for: mainFile)
}

public func unregisterForChangeNotifications(for uri: DocumentURI) async {
Expand Down
14 changes: 5 additions & 9 deletions Sources/SKCore/CompilationDatabaseBuildSystem.swift
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public actor CompilationDatabaseBuildSystem {

/// The URIs for which the delegate has registered for change notifications,
/// mapped to the language the delegate specified when registering for change notifications.
var watchedFiles: [DocumentURI: Language] = [:]
var watchedFiles: Set<DocumentURI> = []

private var _indexStorePath: AbsolutePath?
public var indexStorePath: AbsolutePath? {
Expand Down Expand Up @@ -109,13 +109,13 @@ extension CompilationDatabaseBuildSystem: BuildSystem {
)
}

public func registerForChangeNotifications(for uri: DocumentURI, language: Language) async {
self.watchedFiles[uri] = language
public func registerForChangeNotifications(for uri: DocumentURI) async {
self.watchedFiles.insert(uri)
}

/// We don't support change watching.
public func unregisterForChangeNotifications(for uri: DocumentURI) {
self.watchedFiles[uri] = nil
self.watchedFiles.remove(uri)
}

private func database(for url: URL) -> CompilationDatabase? {
Expand Down Expand Up @@ -165,11 +165,7 @@ extension CompilationDatabaseBuildSystem: BuildSystem {
)

if let delegate = self.delegate {
var changedFiles = Set<DocumentURI>()
for (uri, _) in self.watchedFiles {
changedFiles.insert(uri)
}
await delegate.fileBuildSettingsChanged(changedFiles)
await delegate.fileBuildSettingsChanged(self.watchedFiles)
}
}

Expand Down
12 changes: 5 additions & 7 deletions Sources/SKSwiftPMWorkspace/SwiftPMWorkspace.swift
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ public actor SwiftPMWorkspace {

/// The URIs for which the delegate has registered for change notifications,
/// mapped to the language the delegate specified when registering for change notifications.
var watchedFiles: [DocumentURI: Language] = [:]
var watchedFiles: Set<DocumentURI> = []

/// This callback is informed when `reloadPackage` starts and ends executing.
var reloadPackageStatusCallback: (ReloadPackageStatus) async -> Void
Expand Down Expand Up @@ -245,8 +245,7 @@ extension SwiftPMWorkspace {
await reloadPackageStatusCallback(.end)
return
}
let changedFiles = Set<DocumentURI>(self.watchedFiles.keys)
await delegate.fileBuildSettingsChanged(changedFiles)
await delegate.fileBuildSettingsChanged(self.watchedFiles)
await delegate.fileHandlingCapabilityChanged()
await reloadPackageStatusCallback(.end)
}
Expand Down Expand Up @@ -300,15 +299,14 @@ extension SwiftPMWorkspace: SKCore.BuildSystem {
return nil
}

public func registerForChangeNotifications(for uri: DocumentURI, language: Language) async {
assert(self.watchedFiles[uri] == nil, "Registered twice for change notifications of the same URI")
self.watchedFiles[uri] = language
public func registerForChangeNotifications(for uri: DocumentURI) async {
self.watchedFiles.insert(uri)
}

/// Unregister the given file for build-system level change notifications, such as command
/// line flag changes, dependency changes, etc.
public func unregisterForChangeNotifications(for uri: DocumentURI) {
self.watchedFiles[uri] = nil
self.watchedFiles.remove(uri)
}

/// Returns the resolved target description for the given file, if one is known.
Expand Down
4 changes: 2 additions & 2 deletions Tests/SKCoreTests/BuildServerBuildSystemTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ final class BuildServerBuildSystemTests: XCTestCase {
_fixLifetime(buildSystemDelegate)
}
await buildSystem.setDelegate(buildSystemDelegate)
await buildSystem.registerForChangeNotifications(for: DocumentURI(fileUrl), language: .swift)
await buildSystem.registerForChangeNotifications(for: DocumentURI(fileUrl))

XCTAssertEqual(XCTWaiter.wait(for: [expectation], timeout: defaultTimeout), .completed)
}
Expand All @@ -76,7 +76,7 @@ final class BuildServerBuildSystemTests: XCTestCase {
_fixLifetime(buildSystemDelegate)
}
await buildSystem.setDelegate(buildSystemDelegate)
await buildSystem.registerForChangeNotifications(for: DocumentURI(fileUrl), language: .swift)
await buildSystem.registerForChangeNotifications(for: DocumentURI(fileUrl))

try await fulfillmentOfOrThrow([expectation])
}
Expand Down
2 changes: 1 addition & 1 deletion Tests/SKCoreTests/BuildSystemManagerTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -437,7 +437,7 @@ class ManualBuildSystem: BuildSystem {
return map[uri]
}

func registerForChangeNotifications(for uri: DocumentURI, language: Language) async {
func registerForChangeNotifications(for uri: DocumentURI) async {
}

func unregisterForChangeNotifications(for: DocumentURI) {
Expand Down
2 changes: 1 addition & 1 deletion Tests/SourceKitLSPTests/BuildSystemTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ final class TestBuildSystem: BuildSystem {
return buildSettingsByFile[document]
}

func registerForChangeNotifications(for uri: DocumentURI, language: Language) async {
func registerForChangeNotifications(for uri: DocumentURI) async {
watchedFiles.insert(uri)
}

Expand Down