Merge pull request #386 from benlangmuir/lifetimes-again

Fix lifetime issues in tests

Author: benlangmuir

Sources/SKCore/BuildSystemManager.swift

@@ -109,10 +109,10 @@ public final class BuildSystemManager {
   let fallbackBuildSystem: FallbackBuildSystem?
 
   /// Provider of file to main file mappings.
-  weak var mainFilesProvider: MainFilesProvider?
+  var _mainFilesProvider: MainFilesProvider?
 
   /// Build system delegate that will receive notifications about setting changes, etc.
-  weak var _delegate: BuildSystemDelegate?
+  var _delegate: BuildSystemDelegate?
 
   /// Create a BuildSystemManager that wraps the given build system. The new
   /// manager will modify the delegate of the underlying build system.
@@ -121,7 +121,7 @@ public final class BuildSystemManager {
     precondition(buildSystem?.delegate == nil)
     self.buildSystem = buildSystem
     self.fallbackBuildSystem = fallbackBuildSystem
-    self.mainFilesProvider = mainFilesProvider
+    self._mainFilesProvider = mainFilesProvider
     self.fallbackSettingsTimeout = fallbackSettingsTimeout
     self.buildSystem?.delegate = self
   }
@@ -138,6 +138,11 @@ extension BuildSystemManager: BuildSystem {
     set { queue.sync { _delegate = newValue } }
   }
 
+  public var mainFilesProvider: MainFilesProvider? {
+    get { queue.sync { _mainFilesProvider} }
+    set { queue.sync { _mainFilesProvider = newValue } }
+  }
+
   public func registerForChangeNotifications(for uri: DocumentURI, language: Language) {
     return queue.async {
       log("registerForChangeNotifications(\(uri.pseudoPath))")
@@ -146,7 +151,7 @@ extension BuildSystemManager: BuildSystem {
       if let watchedFile = self.watchedFiles[uri] {
         mainFile = watchedFile.mainFile
       } else {
-        let mainFiles = self.mainFilesProvider?.mainFilesContainingFile(uri)
+        let mainFiles = self._mainFilesProvider?.mainFilesContainingFile(uri)
         mainFile = chooseMainFile(for: uri, from: mainFiles ?? [])
         self.watchedFiles[uri] = (mainFile, language)
       }
@@ -396,7 +401,7 @@ extension BuildSystemManager: MainFilesDelegate {
       var buildSettingsChanges = [DocumentURI: FileBuildSettingsChange]()
 
       for (uri, state) in origWatched {
-        let mainFiles = self.mainFilesProvider?.mainFilesContainingFile(uri) ?? []
+        let mainFiles = self._mainFilesProvider?.mainFilesContainingFile(uri) ?? []
         let newMainFile = chooseMainFile(for: uri, previous: state.mainFile, from: mainFiles)
         let language = state.language
 

Sources/SourceKitLSP/SourceKitServer.swift

@@ -561,7 +561,11 @@ extension SourceKitServer {
     // pipe failure.
 
     // Close the index, which will flush to disk.
+    self.workspace?.buildSystemManager.mainFilesProvider = nil
     self.workspace?.index = nil
+
+    // Break retain cycle with the BSM.
+    self.workspace?.buildSystemManager.delegate = nil
   }
 
 

Tests/LanguageServerProtocolJSONRPCTests/ConnectionTests.swift

@@ -271,7 +271,9 @@ class ConnectionTests: XCTestCase {
 #endif
       conn.close()
 
-      waitForExpectations(timeout: 10)
+      withExtendedLifetime(conn) {
+        waitForExpectations(timeout: 10)
+      }
     }
   }
 }

Tests/SKCoreTests/BuildSystemManagerTests.swift

@@ -26,10 +26,6 @@ final class BuildSystemManagerTests: XCTestCase {
     let d = DocumentURI(string: "bsm:d")
 
     let mainFiles = ManualMainFilesProvider()
-    defer {
-      // BuildSystemManager has a weak reference to mainFiles. Keep it alive.
-      _fixLifetime(mainFiles)
-    }
     mainFiles.mainFiles = [
       a: Set([c]),
       b: Set([c, d]),
@@ -41,6 +37,7 @@ final class BuildSystemManagerTests: XCTestCase {
       buildSystem: FallbackBuildSystem(),
       fallbackBuildSystem: nil,
       mainFilesProvider: mainFiles)
+    defer { withExtendedLifetime(bsm) {} } // Keep BSM alive for callbacks.
 
     XCTAssertEqual(bsm._cachedMainFile(for: a), nil)
     XCTAssertEqual(bsm._cachedMainFile(for: b), nil)
@@ -95,16 +92,13 @@ final class BuildSystemManagerTests: XCTestCase {
   func testSettingsMainFile() {
     let a = DocumentURI(string: "bsm:a.swift")
     let mainFiles = ManualMainFilesProvider()
-    defer {
-      // BuildSystemManager has a weak reference to mainFiles. Keep it alive.
-      _fixLifetime(mainFiles)
-    }
     mainFiles.mainFiles = [a: Set([a])]
     let bs = ManualBuildSystem()
     let bsm = BuildSystemManager(
       buildSystem: bs,
       fallbackBuildSystem: nil,
       mainFilesProvider: mainFiles)
+    defer { withExtendedLifetime(bsm) {} } // Keep BSM alive for callbacks.
     let del = BSMDelegate(bsm)
 
     bs.map[a] = FileBuildSettings(compilerArguments: ["x"])
@@ -123,16 +117,13 @@ final class BuildSystemManagerTests: XCTestCase {
   func testSettingsMainFileInitialNil() {
     let a = DocumentURI(string: "bsm:a.swift")
     let mainFiles = ManualMainFilesProvider()
-    defer {
-      // BuildSystemManager has a weak reference to mainFiles. Keep it alive.
-      _fixLifetime(mainFiles)
-    }
     mainFiles.mainFiles = [a: Set([a])]
     let bs = ManualBuildSystem()
     let bsm = BuildSystemManager(
       buildSystem: bs,
       fallbackBuildSystem: nil,
       mainFilesProvider: mainFiles)
+    defer { withExtendedLifetime(bsm) {} } // Keep BSM alive for callbacks.
     let del = BSMDelegate(bsm)
     let initial = expectation(description: "initial settings")
     del.expected = [(a, nil, initial, #file, #line)]
@@ -149,17 +140,14 @@ final class BuildSystemManagerTests: XCTestCase {
   func testSettingsMainFileWithFallback() {
     let a = DocumentURI(string: "bsm:a.swift")
     let mainFiles = ManualMainFilesProvider()
-    defer {
-      // BuildSystemManager has a weak reference to mainFiles. Keep it alive.
-      _fixLifetime(mainFiles)
-    }
     mainFiles.mainFiles = [a: Set([a])]
     let bs = ManualBuildSystem()
     let fallback = FallbackBuildSystem()
     let bsm = BuildSystemManager(
       buildSystem: bs,
       fallbackBuildSystem: fallback,
       mainFilesProvider: mainFiles)
+    defer { withExtendedLifetime(bsm) {} } // Keep BSM alive for callbacks.
     let del = BSMDelegate(bsm)
     let fallbackSettings = fallback.settings(for: a, .swift)
     let initial = expectation(description: "initial fallback settings")
@@ -184,16 +172,13 @@ final class BuildSystemManagerTests: XCTestCase {
     let a = DocumentURI(string: "bsm:a.swift")
     let b = DocumentURI(string: "bsm:b.swift")
     let mainFiles = ManualMainFilesProvider()
-    defer {
-      // BuildSystemManager has a weak reference to mainFiles. Keep it alive.
-      _fixLifetime(mainFiles)
-    }
     mainFiles.mainFiles = [a: Set([a]), b: Set([b])]
     let bs = ManualBuildSystem()
     let bsm = BuildSystemManager(
       buildSystem: bs,
       fallbackBuildSystem: nil,
       mainFilesProvider: mainFiles)
+    defer { withExtendedLifetime(bsm) {} } // Keep BSM alive for callbacks.
     let del = BSMDelegate(bsm)
 
     bs.map[a] = FileBuildSettings(compilerArguments: ["x"])
@@ -234,16 +219,13 @@ final class BuildSystemManagerTests: XCTestCase {
     let a = DocumentURI(string: "bsm:a.swift")
     let b = DocumentURI(string: "bsm:b.swift")
     let mainFiles = ManualMainFilesProvider()
-    defer {
-      // BuildSystemManager has a weak reference to mainFiles. Keep it alive.
-      _fixLifetime(mainFiles)
-    }
     mainFiles.mainFiles = [a: Set([a]), b: Set([b])]
     let bs = ManualBuildSystem()
     let bsm = BuildSystemManager(
       buildSystem: bs,
       fallbackBuildSystem: nil,
       mainFilesProvider: mainFiles)
+    defer { withExtendedLifetime(bsm) {} } // Keep BSM alive for callbacks.
     let del = BSMDelegate(bsm)
 
     bs.map[a] = FileBuildSettings(compilerArguments: ["a"])
@@ -274,10 +256,6 @@ final class BuildSystemManagerTests: XCTestCase {
     let cpp1 = DocumentURI(string: "bsm:main.cpp")
     let cpp2 = DocumentURI(string: "bsm:other.cpp")
     let mainFiles = ManualMainFilesProvider()
-    defer {
-      // BuildSystemManager has a weak reference to mainFiles. Keep it alive.
-      _fixLifetime(mainFiles)
-    }
     mainFiles.mainFiles = [
       h: Set([cpp1]),
       cpp1: Set([cpp1]),
@@ -289,6 +267,7 @@ final class BuildSystemManagerTests: XCTestCase {
       buildSystem: bs,
       fallbackBuildSystem: nil,
       mainFilesProvider: mainFiles)
+    defer { withExtendedLifetime(bsm) {} } // Keep BSM alive for callbacks.
     let del = BSMDelegate(bsm)
 
     bs.map[cpp1] = FileBuildSettings(compilerArguments: ["C++ 1"])
@@ -333,10 +312,6 @@ final class BuildSystemManagerTests: XCTestCase {
     let h2 = DocumentURI(string: "bsm:header2.h")
     let cpp = DocumentURI(string: "bsm:main.cpp")
     let mainFiles = ManualMainFilesProvider()
-    defer {
-      // BuildSystemManager has a weak reference to mainFiles. Keep it alive.
-      _fixLifetime(mainFiles)
-    }
     mainFiles.mainFiles = [
       h1: Set([cpp]),
       h2: Set([cpp]),
@@ -347,6 +322,7 @@ final class BuildSystemManagerTests: XCTestCase {
       buildSystem: bs,
       fallbackBuildSystem: nil,
       mainFilesProvider: mainFiles)
+    defer { withExtendedLifetime(bsm) {} } // Keep BSM alive for callbacks.
     let del = BSMDelegate(bsm)
 
     bs.map[cpp] = FileBuildSettings(compilerArguments: ["C++ Main File"])
@@ -382,16 +358,13 @@ final class BuildSystemManagerTests: XCTestCase {
     let b = DocumentURI(string: "bsm:b.swift")
     let c = DocumentURI(string: "bsm:c.swift")
     let mainFiles = ManualMainFilesProvider()
-    defer {
-      // BuildSystemManager has a weak reference to mainFiles. Keep it alive.
-      _fixLifetime(mainFiles)
-    }
     mainFiles.mainFiles = [a: Set([a]), b: Set([b]), c: Set([c])]
     let bs = ManualBuildSystem()
     let bsm = BuildSystemManager(
       buildSystem: bs,
       fallbackBuildSystem: nil,
       mainFilesProvider: mainFiles)
+    defer { withExtendedLifetime(bsm) {} } // Keep BSM alive for callbacks.
     let del = BSMDelegate(bsm)
 
     bs.map[a] = FileBuildSettings(compilerArguments: ["a"])
@@ -436,10 +409,6 @@ final class BuildSystemManagerTests: XCTestCase {
   func testDependenciesUpdated() {
     let a = DocumentURI(string: "bsm:a.swift")
     let mainFiles = ManualMainFilesProvider()
-    defer {
-      // BuildSystemManager has a weak reference to mainFiles. Keep it alive.
-      _fixLifetime(mainFiles)
-    }
     mainFiles.mainFiles = [a: Set([a])]
 
     class DepUpdateDuringRegistrationBS: ManualBuildSystem {
@@ -454,6 +423,7 @@ final class BuildSystemManagerTests: XCTestCase {
       buildSystem: bs,
       fallbackBuildSystem: nil,
       mainFilesProvider: mainFiles)
+    defer { withExtendedLifetime(bsm) {} } // Keep BSM alive for callbacks.
     let del = BSMDelegate(bsm)
 
     bs.map[a] = FileBuildSettings(compilerArguments: ["x"])

Tests/SourceKitLSPTests/CodeActionTests.swift

@@ -182,9 +182,10 @@ final class CodeActionTests: XCTestCase {
     let textDocument = TextDocumentIdentifier(loc.url)
     let start = Position(line: 2, utf16index: 0)
     let request = CodeActionRequest(range: start..<start, context: .init(), textDocument: textDocument)
-    let result = try ws.sk.sendSync(request)
-
-    XCTAssertEqual(result, .codeActions([]))
+    try withExtendedLifetime(ws) {
+      let result = try ws.sk.sendSync(request)
+      XCTAssertEqual(result, .codeActions([]))
+    }
   }
 
   func testSemanticRefactorLocalRenameResult() throws {
@@ -194,8 +195,10 @@ final class CodeActionTests: XCTestCase {
 
     let textDocument = TextDocumentIdentifier(loc.url)
     let request = CodeActionRequest(range: loc.position..<loc.position, context: .init(), textDocument: textDocument)
-    let result = try ws.sk.sendSync(request)
-    XCTAssertEqual(result, .codeActions([]))
+    try withExtendedLifetime(ws) {
+      let result = try ws.sk.sendSync(request)
+      XCTAssertEqual(result, .codeActions([]))
+    }
   }
 
   func testSemanticRefactorLocationCodeActionResult() throws {
@@ -205,7 +208,7 @@ final class CodeActionTests: XCTestCase {
 
     let textDocument = TextDocumentIdentifier(loc.url)
     let request = CodeActionRequest(range: loc.position..<loc.position, context: .init(), textDocument: textDocument)
-    let result = try ws.sk.sendSync(request)
+    let result = try withExtendedLifetime(ws) { try ws.sk.sendSync(request) }
 
     let expectedCommandArgs: LSPAny = ["actionString": "source.refactoring.kind.localize.string", "positionRange": ["start": ["character": 43, "line": 1], "end": ["character": 43, "line": 1]], "title": "Localize String", "textDocument": ["uri": .string(loc.url.absoluteString)]]
 
@@ -230,7 +233,7 @@ final class CodeActionTests: XCTestCase {
 
     let textDocument = TextDocumentIdentifier(rangeStartLoc.url)
     let request = CodeActionRequest(range: rangeStartLoc.position..<rangeEndLoc.position, context: .init(), textDocument: textDocument)
-    let result = try ws.sk.sendSync(request)
+    let result = try withExtendedLifetime(ws) { try ws.sk.sendSync(request) }
 
     let expectedCommandArgs: LSPAny = ["actionString": "source.refactoring.kind.extract.function", "positionRange": ["start": ["character": 21, "line": 1], "end": ["character": 27, "line": 2]], "title": "Extract Method", "textDocument": ["uri": .string(rangeStartLoc.url.absoluteString)]]
     let metadataArguments: LSPAny = ["sourcekitlsp_textDocument": ["uri": .string(rangeStartLoc.url.absoluteString)]]

Tests/SourceKitLSPTests/ExecuteCommandTests.swift

@@ -65,7 +65,7 @@ final class ExecuteCommandTests: XCTestCase {
       req.reply(ApplyEditResponse(applied: true, failureReason: nil))
     }
 
-    let result = try ws.sk.sendSync(request)
+    let result = try withExtendedLifetime(ws) { try ws.sk.sendSync(request) }
 
     guard case .dictionary(let resultDict) = result else {
       XCTFail("Result is not a dictionary.")
@@ -108,7 +108,7 @@ final class ExecuteCommandTests: XCTestCase {
       req.reply(ApplyEditResponse(applied: true, failureReason: nil))
     }
 
-    let result = try ws.sk.sendSync(request)
+    let result = try withExtendedLifetime(ws) { try ws.sk.sendSync(request) }
 
     guard case .dictionary(let resultDict) = result else {
       XCTFail("Result is not a dictionary.")

Tests/SourceKitLSPTests/FoldingRangeTests.swift

@@ -36,7 +36,7 @@ final class FoldingRangeTests: XCTestCase {
     guard let (ws, uri) = try initializeWorkspace(withCapabilities: capabilities, testLoc: "fr:base") else { return }
 
     let request = FoldingRangeRequest(textDocument: TextDocumentIdentifier(uri))
-    let ranges = try ws.sk.sendSync(request)
+    let ranges = try withExtendedLifetime(ws) { try ws.sk.sendSync(request) }
 
     XCTAssertEqual(ranges, [
       FoldingRange(startLine: 0, startUTF16Index: 0, endLine: 2, endUTF16Index: 0, kind: .comment),
@@ -62,7 +62,7 @@ final class FoldingRangeTests: XCTestCase {
     guard let (ws, uri) = try initializeWorkspace(withCapabilities: capabilities, testLoc: "fr:base") else { return }
 
     let request = FoldingRangeRequest(textDocument: TextDocumentIdentifier(uri))
-    let ranges = try ws.sk.sendSync(request)
+    let ranges = try withExtendedLifetime(ws) { try ws.sk.sendSync(request) }
 
     XCTAssertEqual(ranges, [
       FoldingRange(startLine: 0, endLine: 1, kind: .comment),
@@ -83,7 +83,7 @@ final class FoldingRangeTests: XCTestCase {
       capabilities.rangeLimit = limit
       guard let (ws, url) = try initializeWorkspace(withCapabilities: capabilities, testLoc: "fr:base") else { return }
       let request = FoldingRangeRequest(textDocument: TextDocumentIdentifier(url))
-      let ranges = try ws.sk.sendSync(request)
+      let ranges = try withExtendedLifetime(ws) { try ws.sk.sendSync(request) }
       XCTAssertEqual(ranges?.count, expectedRanges, "Failed rangeLimit test at line \(line)")
     }
 
@@ -100,7 +100,7 @@ final class FoldingRangeTests: XCTestCase {
     guard let (ws, url) = try initializeWorkspace(withCapabilities: capabilities, testLoc: "fr:empty") else { return }
 
     let request = FoldingRangeRequest(textDocument: TextDocumentIdentifier(url))
-    let ranges = try ws.sk.sendSync(request)
+    let ranges = try withExtendedLifetime(ws) { try ws.sk.sendSync(request) }
 
     XCTAssertEqual(ranges?.count, 0)
   }

Tests/SourceKitLSPTests/LocalClangTests.swift

@@ -180,6 +180,8 @@ final class LocalClangTests: XCTestCase {
 
     try! ws.openDocument(loc.url, language: .objective_c)
 
-    waitForExpectations(timeout: 15)
+    withExtendedLifetime(ws) {
+      waitForExpectations(timeout: 15)
+    }
   }
 }