`IndexStoreDB` moves its index to the `saved` directory when it is deallocated. Because `IndexStoreDB` is primarily owned by `UncheckedIndex`, we rely on deallocating this object to save the index store. This is fairly brittle because various parts of the codebase may hold transient references to that object as reported in https://github.com/swiftlang/sourcekit-lsp/issues/2455#issuecomment-3873561003.
Explicitly remove the reference from `UncheckedIndex` to `IndexStoreDB`. While this still isn’t perfect because other parts of the code base may hold references to `IndexStoreDB` but those should be a lot rarer, resulting in a more consistent closing of the index.
Replace usages of `Optional.map` and `Optional.flatMap` by if expressions or other expressions.
I personally find `Optional.map` to be hard to read because `map` implies mapping a collection to me. Usually the alternative constructs seem clearer to me.
The reason why I am making the first change is because in a separate PR in
swiftlang I am fixing a bug that caused certain captured parameters to be
treated as sending parameters incorrectly. This allowed for parameters to
incorrectly be allowed to be sent from one isolation domain to another.
The specific problem here can be seen with the following swift code:
```swift
actor B {
init(callback: @escaping @Sendable () -> Void) async {}
}
actor A {
private func poke() {}
func schedule() async {
_ = await B(
callback: { [weak self] in // closure 1
Task.detached { // closure 2
await self?.poke()
}
})
}
}
```
When we capture the weak self from closure 1 in closure 2, we are not actually
capturing self directly. Instead we are capturing the var box which contains the
weak self. The box (unlike self) is actually non-Sendable. Since closure 2 is
not call(once), the compiler must assume semantically that the closure can be
invoked potentially multiple times meaning that it cannot allow for self to be
used in Task.detached. The fix for this is to perform an inner [weak self]
capture. As follows:
```swift
actor A {
private func poke() {}
func schedule() async {
_ = await B(
callback: { [weak self] in // closure 1
Task.detached { [weak self] // closure 2
await self?.poke()
}
})
}
}
```
The reason why this works is that when we form the second weak self binding, we
perform a load from the outer weak self giving us an Optional<A>. Then we store
that optional value back into a new weak box. Since Optional<A> is Sendable, we
know that the two non-Sendable weak var boxes are completely unrelated, so we
can send that new var box into the new Task.detached safely.
The second `[weak self]` is just something I noticed later in the function. The
`[weak self]` just makes the detached function safer.
Until we have better measurements that would motivate a different batching strategy, copying the driver’s batch size seems like the most reasonable thing to do.
According to https://developer.apple.com/documentation/foundation/processinfo/activeprocessorcount
> Whereas the processorCount property reports the number of advertised processing cores, the activeProcessorCount property reflects the actual number of active processing cores on the system. There are a number of different factors that may cause a core to not be active, including boot arguments, thermal throttling, or a manufacturing defect.
For short-lived workloads like `concurrentMap` we want to parallelize across the number of cores that are currently active, so use `activeProcessorCount` instead. The only case where we want to continue using `processorCount` is the computation of concurrent tasks for `TaskScheduler` because the value is stored for the lifetime of the SourceKit-LSP process and we don’t want to limit parallelism if SourceKit-LSP was launched during a time of thermal throttling.
I stumbled across this while working on #2302
We should not take the number of files in an `UpdateIndexStoreTaskDescription` as an indication on how important the task is. If we do need this functionality, eg. because we want to update the index of files with syntactic matches for a rename term, this should be communicated using a specific purpose similar to `TargetPreparationPurpose`. Since the only reason we update the index store for a file right now is background indexing, such a check is not needed.
This way we can guarantee that all files passed to `UpdateIndexStoreTaskDescription` belong to the same target, which will simplify multi-file indexing.
When I added the log structure to `build/logMessage` in #2022 I must have assumed that the entire BSP notifciation was an extension defined by SourceKit-LSP and didn’t realized that this was actually a change that made the notification non-compliant with BSP. Change it up a little bit to make it compliant again.
The term *build system* predated our wide-spread adoption of BSP for communicating between SourceKit-LSP to the build system and was never really the correct term anyway – ie. a `JSONCompilationDatabaseBuildSystem` never really sounded right. We now have a correct term for the communication layer between SourceKit-LSP: A build server. Rename most occurrences of *build system* to *build server* to reflect this. There are unfortunately a couple lingering instances of *build system* that we can’t change, most notably: `fallbackBuildSystem` in the config file, the `workspace/waitForBuildSystemUpdates` BSP extension request and the `synchronize-for-build-system-updates` experimental feature.
This is what `shutDown()` is documented to do. I also remember having this check before, it might have gotten lost during a rebase when I was working on https://github.com/swiftlang/sourcekit-lsp/pull/2081.
I noticed this while investigating https://github.com/swiftlang/sourcekit-lsp/pull/2152: In this case `buildTarget/prepare` was cancelled because the SourceKit-LSP server was shut down but indexing of a file was still started after the shutdown now that preparation had finished (because it was cancelled).
A queued task might have been cancelled after the execution ask was started but before the task was yielded to `executionTaskCreatedContinuation`. In that case the result task will simply cancel the await on the `executionTaskCreatedStream` and hence not call `valuePropagatingCancellation` on the execution task. This means that the queued task cancellation wouldn't be propagated to the execution task. To address this, check if `resultTaskCancelled` was set and, if so, explicitly cancel the execution task here.
Fixes an issue I saw in CI during PR testing.
When SourceKit-LSP is shut down, we should make sure that we don’t leave behind child processes, which will become orphans after SourceKit-LSP has terminated. What’s worse, when SourceKit-LSP has exited, these processes might not have any process to read their stdout/stderr, which can lead to them running indefinitely.
This change does not cover the termination of subprocess trees. For example, if we launch `swift build` and need to kill it because it doesn’t honor SIGINT, its child processes will still live on. Similarly, if we kill a BSP server, its child processes might live on. Fixing this is a drastically bigger endeavor, likely requiring changes to Foundation and/or TSC. I filed https://github.com/swiftlang/sourcekit-lsp/issues/2080 for it.
This wasn’t an issue when we were always waiting for an up-to-date build graph before indexing files but now we could still have an old build graph that contains the deleted files and thus start an indexing process for a file that doesn’t exist anymore on disk. And it’s cleaner anyway.
Since we re-index source files if the build server sends us a `buildTarget/didChange` notification, we no longer need to wait for an up-to-date build graph when a file is modified. This resolves an issue that causes a `Scheduling tasks` progress to appear in the status bar whenever a file in the project is changed. Before https://github.com/swiftlang/sourcekit-lsp/pull/1973, this happened fairly frequently during the initial indexing when a header file was written into the build directory. After that PR the `Scheduling Indexing` status appears a lot more frequently, namely every time the index’s database is modified, which happens quite all the time during indexing...
