Change CheckedIndex to use DocumentURI instead of URL

Author: ahoppen

Sources/LanguageServerProtocol/SupportTypes/DocumentURI.swift

@@ -75,6 +75,10 @@ public struct DocumentURI: Codable, Hashable, Sendable {
     assert(self.storage.scheme != nil, "Received invalid URI without a scheme '\(self.storage.absoluteString)'")
   }
 
+  public init(filePath: String, isDirectory: Bool) {
+    self.init(URL(fileURLWithPath: filePath, isDirectory: isDirectory))
+  }
+
   public init(from decoder: Decoder) throws {
     try self.init(string: decoder.singleValueContainer().decode(String.self))
   }

Sources/SKTestSupport/SkipUnless.swift

@@ -70,12 +70,14 @@ public actor SkipUnless {
         )
       } else if toolchainSwiftVersion == requiredSwiftVersion {
         logger.info("Checking if feature '\(featureName)' is supported")
+        defer {
+          logger.info("Done checking if feature '\(featureName)' is supported")
+        }
         if try await !featureCheck() {
           return .featureUnsupported(skipMessage: "Skipping because toolchain doesn't contain \(featureName)")
         } else {
           return .featureSupported
         }
-        logger.info("Done checking if feature '\(featureName)' is supported")
       } else {
         return .featureSupported
       }

Sources/SemanticIndex/CheckedIndex.swift

@@ -19,9 +19,9 @@ import LanguageServerProtocol
 ///
 /// Protocol is needed because the `SemanticIndex` module is lower-level than the `SourceKitLSP` module.
 public protocol InMemoryDocumentManager {
-  /// Returns true if the file at the given URL has a different content in the document manager than on-disk. This is
+  /// Returns true if the file at the given URI has a different content in the document manager than on-disk. This is
   /// the case if the user made edits to the file but didn't save them yet.
-  func fileHasInMemoryModifications(_ url: URL) -> Bool
+  func fileHasInMemoryModifications(_ uri: DocumentURI) -> Bool
 }
 
 public enum IndexCheckLevel {
@@ -105,7 +105,7 @@ public final class CheckedIndex {
   }
 
   public func symbols(inFilePath path: String) -> [Symbol] {
-    guard self.hasUpToDateUnit(for: URL(fileURLWithPath: path, isDirectory: false)) else {
+    guard self.hasUpToDateUnit(for: DocumentURI(filePath: path, isDirectory: false)) else {
       return []
     }
     return index.symbols(inFilePath: path)
@@ -121,11 +121,11 @@ public final class CheckedIndex {
   /// If `crossLanguage` is set to `true`, Swift files that import a header through a module will also be reported.
   public func mainFilesContainingFile(uri: DocumentURI, crossLanguage: Bool = false) -> [DocumentURI] {
     return index.mainFilesContainingFile(path: uri.pseudoPath, crossLanguage: crossLanguage).compactMap {
-      let url = URL(fileURLWithPath: $0)
-      guard checker.indexHasUpToDateUnit(for: url, mainFile: nil, index: self.index) else {
+      let uri = DocumentURI(filePath: $0, isDirectory: false)
+      guard checker.indexHasUpToDateUnit(for: uri, mainFile: nil, index: self.index) else {
         return nil
       }
-      return DocumentURI(url)
+      return uri
     }
   }
 
@@ -141,16 +141,16 @@ public final class CheckedIndex {
   /// If `mainFile` is passed, then `url` is a header file that won't have a unit associated with it. `mainFile` is
   /// assumed to be a file that imports `url`. To check that `url` has an up-to-date unit, check that the latest unit
   /// for `mainFile` is newer than the mtime of the header file at `url`.
-  public func hasUpToDateUnit(for url: URL, mainFile: URL? = nil) -> Bool {
-    return checker.indexHasUpToDateUnit(for: url, mainFile: mainFile, index: index)
+  public func hasUpToDateUnit(for uri: DocumentURI, mainFile: DocumentURI? = nil) -> Bool {
+    return checker.indexHasUpToDateUnit(for: uri, mainFile: mainFile, index: index)
   }
 
-  /// Returns true if the file at the given URL has a different content in the document manager than on-disk. This is
+  /// Returns true if the file at the given URI has a different content in the document manager than on-disk. This is
   /// the case if the user made edits to the file but didn't save them yet.
   ///
   /// - Important: This must only be called on a `CheckedIndex` with a `checkLevel` of `inMemoryModifiedFiles`
-  public func fileHasInMemoryModifications(_ url: URL) -> Bool {
-    return checker.fileHasInMemoryModifications(url)
+  public func fileHasInMemoryModifications(_ uri: DocumentURI) -> Bool {
+    return checker.fileHasInMemoryModifications(uri)
   }
 }
 
@@ -212,14 +212,14 @@ private struct IndexOutOfDateChecker {
     }
   }
 
-  /// Caches whether a file URL has modifications in `documentManager` that haven't been saved to disk yet.
-  private var fileHasInMemoryModificationsCache: [URL: Bool] = [:]
+  /// Caches whether a document has modifications in `documentManager` that haven't been saved to disk yet.
+  private var fileHasInMemoryModificationsCache: [DocumentURI: Bool] = [:]
 
-  /// File URLs to modification times that have already been computed.
-  private var modTimeCache: [URL: ModificationTime] = [:]
+  /// Document URIs to modification times that have already been computed.
+  private var modTimeCache: [DocumentURI: ModificationTime] = [:]
 
-  /// File URLs to whether they exist on the file system
-  private var fileExistsCache: [URL: Bool] = [:]
+  /// Document URIs to whether they exist on the file system
+  private var fileExistsCache: [DocumentURI: Bool] = [:]
 
   init(checkLevel: IndexCheckLevel) {
     self.checkLevel = checkLevel
@@ -230,16 +230,16 @@ private struct IndexOutOfDateChecker {
   /// Returns `true` if the source file for the given symbol location exists and has not been modified after it has been
   /// indexed.
   mutating func isUpToDate(_ symbolLocation: SymbolLocation) -> Bool {
-    let url = URL(fileURLWithPath: symbolLocation.path, isDirectory: false)
+    let uri = DocumentURI(filePath: symbolLocation.path, isDirectory: false)
     switch checkLevel {
     case .inMemoryModifiedFiles(let documentManager):
-      if fileHasInMemoryModifications(url, documentManager: documentManager) {
+      if fileHasInMemoryModifications(uri, documentManager: documentManager) {
         return false
       }
       fallthrough
     case .modifiedFiles:
       do {
-        let sourceFileModificationDate = try modificationDate(of: url)
+        let sourceFileModificationDate = try modificationDate(of: uri)
         switch sourceFileModificationDate {
         case .fileDoesNotExist:
           return false
@@ -251,7 +251,7 @@ private struct IndexOutOfDateChecker {
         return true
       }
     case .deletedFiles:
-      return fileExists(at: url)
+      return fileExists(at: uri)
     }
   }
 
@@ -262,7 +262,7 @@ private struct IndexOutOfDateChecker {
   /// If `mainFile` is passed, then `filePath` is a header file that won't have a unit associated with it. `mainFile` is
   /// assumed to be a file that imports `url`. To check that `url` has an up-to-date unit, check that the latest unit
   /// for `mainFile` is newer than the mtime of the header file at `url`.
-  mutating func indexHasUpToDateUnit(for filePath: URL, mainFile: URL?, index: IndexStoreDB) -> Bool {
+  mutating func indexHasUpToDateUnit(for filePath: DocumentURI, mainFile: DocumentURI?, index: IndexStoreDB) -> Bool {
     switch checkLevel {
     case .inMemoryModifiedFiles(let documentManager):
       if fileHasInMemoryModifications(filePath, documentManager: documentManager) {
@@ -273,7 +273,9 @@ private struct IndexOutOfDateChecker {
       // If there are no in-memory modifications check if there are on-disk modifications.
       fallthrough
     case .modifiedFiles:
-      guard let lastUnitDate = index.dateOfLatestUnitFor(filePath: (mainFile ?? filePath).path) else {
+      guard let fileURL = (mainFile ?? filePath).fileURL,
+        let lastUnitDate = index.dateOfLatestUnitFor(filePath: fileURL.path)
+      else {
         return false
       }
       do {
@@ -300,23 +302,25 @@ private struct IndexOutOfDateChecker {
   /// `documentManager` must always be the same between calls to `hasFileInMemoryModifications` since it is not part of
   /// the cache key. This is fine because we always assume the `documentManager` to come from the associated value of
   /// `CheckLevel.imMemoryModifiedFiles`, which is constant.
-  private mutating func fileHasInMemoryModifications(_ url: URL, documentManager: InMemoryDocumentManager) -> Bool {
-    if let cached = fileHasInMemoryModificationsCache[url] {
+  private mutating func fileHasInMemoryModifications(_ uri: DocumentURI, documentManager: InMemoryDocumentManager)
+    -> Bool
+  {
+    if let cached = fileHasInMemoryModificationsCache[uri] {
       return cached
     }
-    let hasInMemoryModifications = documentManager.fileHasInMemoryModifications(url)
-    fileHasInMemoryModificationsCache[url] = hasInMemoryModifications
+    let hasInMemoryModifications = documentManager.fileHasInMemoryModifications(uri)
+    fileHasInMemoryModificationsCache[uri] = hasInMemoryModifications
     return hasInMemoryModifications
   }
 
-  /// Returns true if the file at the given URL has a different content in the document manager than on-disk. This is
+  /// Returns true if the file at the given URI has a different content in the document manager than on-disk. This is
   /// the case if the user made edits to the file but didn't save them yet.
   ///
   /// - Important: This must only be called on an `IndexOutOfDateChecker` with a `checkLevel` of `inMemoryModifiedFiles`
-  mutating func fileHasInMemoryModifications(_ url: URL) -> Bool {
+  mutating func fileHasInMemoryModifications(_ uri: DocumentURI) -> Bool {
     switch checkLevel {
     case .inMemoryModifiedFiles(let documentManager):
-      return fileHasInMemoryModifications(url, documentManager: documentManager)
+      return fileHasInMemoryModifications(uri, documentManager: documentManager)
     case .modifiedFiles, .deletedFiles:
       logger.fault(
         "fileHasInMemoryModifications(at:) must only be called on an `IndexOutOfDateChecker` with check level .inMemoryModifiedFiles"
@@ -325,9 +329,12 @@ private struct IndexOutOfDateChecker {
     }
   }
 
-  private func modificationDateUncached(of url: URL) throws -> ModificationTime {
+  private func modificationDateUncached(of uri: DocumentURI) throws -> ModificationTime {
     do {
-      let attributes = try FileManager.default.attributesOfItem(atPath: url.resolvingSymlinksInPath().path)
+      guard let fileURL = uri.fileURL else {
+        return .fileDoesNotExist
+      }
+      let attributes = try FileManager.default.attributesOfItem(atPath: fileURL.resolvingSymlinksInPath().path)
       guard let modificationDate = attributes[FileAttributeKey.modificationDate] as? Date else {
         throw Error.fileAttributesDontHaveModificationDate
       }
@@ -337,21 +344,26 @@ private struct IndexOutOfDateChecker {
     }
   }
 
-  private mutating func modificationDate(of url: URL) throws -> ModificationTime {
-    if let cached = modTimeCache[url] {
+  private mutating func modificationDate(of uri: DocumentURI) throws -> ModificationTime {
+    if let cached = modTimeCache[uri] {
       return cached
     }
-    let modTime = try modificationDateUncached(of: url)
-    modTimeCache[url] = modTime
+    let modTime = try modificationDateUncached(of: uri)
+    modTimeCache[uri] = modTime
     return modTime
   }
 
-  private mutating func fileExists(at url: URL) -> Bool {
-    if let cached = fileExistsCache[url] {
+  private mutating func fileExists(at uri: DocumentURI) -> Bool {
+    if let cached = fileExistsCache[uri] {
       return cached
     }
-    let fileExists = FileManager.default.fileExists(atPath: url.path)
-    fileExistsCache[url] = fileExists
+    let fileExists =
+      if let fileUrl = uri.fileURL {
+        FileManager.default.fileExists(atPath: fileUrl.path)
+      } else {
+        false
+      }
+    fileExistsCache[uri] = fileExists
     return fileExists
   }
 }

Sources/SemanticIndex/SemanticIndexManager.swift

@@ -260,13 +260,7 @@ public final actor SemanticIndexManager {
         await testHooks.buildGraphGenerationDidFinish?()
         let index = index.checked(for: .modifiedFiles)
         let filesToIndex = await self.buildSystemManager.sourceFiles().lazy.map(\.uri)
-          .filter { uri in
-            guard let url = uri.fileURL else {
-              // The URI is not a file, so there's nothing we can index.
-              return false
-            }
-            return !index.hasUpToDateUnit(for: url)
-          }
+          .filter { !index.hasUpToDateUnit(for: $0) }
         await scheduleBackgroundIndex(files: filesToIndex)
         generateBuildGraphTask = nil
       }

Sources/SemanticIndex/UpdateIndexStoreTaskDescription.swift

@@ -206,11 +206,7 @@ public struct UpdateIndexStoreTaskDescription: IndexTaskDescription {
       // If we know that the file is up-to-date without having ot hit the index, do that because it's fastest.
       return
     }
-    guard let sourceFileUrl = file.sourceFile.fileURL else {
-      // The URI is not a file, so there's nothing we can index.
-      return
-    }
-    guard !index.checked(for: .modifiedFiles).hasUpToDateUnit(for: sourceFileUrl, mainFile: file.mainFile.fileURL)
+    guard !index.checked(for: .modifiedFiles).hasUpToDateUnit(for: file.sourceFile, mainFile: file.mainFile)
     else {
       logger.debug("Not indexing \(file.forLogging) because index has an up-to-date unit")
       // We consider a file's index up-to-date if we have any up-to-date unit. Changing build settings does not

Sources/SourceKitLSP/DocumentManager.swift

@@ -191,12 +191,12 @@ public final class DocumentManager: InMemoryDocumentManager, Sendable {
     }
   }
 
-  public func fileHasInMemoryModifications(_ url: URL) -> Bool {
-    guard let document = try? latestSnapshot(DocumentURI(url)) else {
+  public func fileHasInMemoryModifications(_ uri: DocumentURI) -> Bool {
+    guard let document = try? latestSnapshot(uri), let fileURL = uri.fileURL else {
       return false
     }
 
-    guard let onDiskFileContents = try? String(contentsOf: url, encoding: .utf8) else {
+    guard let onDiskFileContents = try? String(contentsOf: fileURL, encoding: .utf8) else {
       // If we can't read the file on disk, it can't match any on-disk state, so it's in-memory state
       return true
     }

Sources/SourceKitLSP/TestDiscovery.swift

@@ -204,12 +204,9 @@ extension SourceKitLSPServer {
     let index = workspace.index(checkedFor: .inMemoryModifiedFiles(documentManager))
 
     let filesWithInMemoryState = documentManager.documents.keys.filter { uri in
-      guard let url = uri.fileURL else {
-        return true
-      }
       // Use the index to check for in-memory modifications so we can re-use its cache. If no index exits, ask the
       // document manager directly.
-      return index?.fileHasInMemoryModifications(url) ?? documentManager.fileHasInMemoryModifications(url)
+      return index?.fileHasInMemoryModifications(uri) ?? documentManager.fileHasInMemoryModifications(uri)
     }
 
     let testsFromFilesWithInMemoryState = await filesWithInMemoryState.concurrentMap { (uri) -> [AnnotatedTestItem] in
@@ -254,7 +251,7 @@ extension SourceKitLSPServer {
           // up-to-date. Include the tests from `testsFromFilesWithInMemoryState`.
           return nil
         }
-        if let fileUrl = testItem.location.uri.fileURL, index?.hasUpToDateUnit(for: fileUrl) ?? false {
+        if index?.hasUpToDateUnit(for: testItem.location.uri) ?? false {
           // We don't have a test for this file in the semantic index but an up-to-date unit file. This means that the
           // index is up-to-date and has more knowledge that identifies a `TestItem` as not actually being a test, eg.
           // because it starts with `test` but doesn't appear in a class inheriting from `XCTestCase`.
@@ -341,7 +338,7 @@ extension SourceKitLSPServer {
           }
         ) + syntacticSwiftTestingTests
       }
-      if let fileURL = mainFileUri.fileURL, index.hasUpToDateUnit(for: fileURL) {
+      if index.hasUpToDateUnit(for: mainFileUri) {
         // The semantic index is up-to-date and doesn't contain any tests. We don't need to do a syntactic fallback for
         // XCTest. We do still need to return swift-testing tests which don't have a semantic index.
         return syntacticSwiftTestingTests