We made quite a few fixes recently to make sure that path handling works correctly using `URL` on Windows. Use `URL` in most places to have a single type that represents file paths instead of sometimes using `AbsolutePath`.
While doing so, also remove usages of `TSCBasic.FileSystem` an `InMemoryFileSystem`. The pattern of using `InMemoryFileSystem` for tests was never consistently used and it was a little confusing that some types took a `FileSystem` parameter while other always assumed to work on the local file system.
Change a l public declarations to the `package` access level, accept for:
- The `LanguageServerProtocol` module
- The `BuildServerProtocol` module
- `InProcessClient.InProcessSourceKitLSPClient`
- `LanguageServerProtocolJSONRPC` (I would like to create a more ergonomic API for this like `InProcessSourceKitLSPClient` in the future, but for now, we’ll leave it public)
Unfortunately, our pattern of marking functions as `@_spi(Testing) public` no longer works with the `package` access level because declarations at the `package` access level cannot be marked as SPI. I have decided to just mark these functions as `package`. Alternatives would be:
- Add an underscore to these functions, like we did for functions exposed for testing before the introduction of `SPI`
- Use `@testable` import in the test targets and mark the methods as `internal`
Resolves#1315
rdar://128295618
VS Code does not cancel semantic tokens requests. If a source file gets into a state where an AST build takes very long, this can cause us to wait for the semantic tokens from sourcekitd for a few minutes, effectively blocking all other semantic functionality in that file.
To circumvent this problem (or any other problem where an editor might not be cancelling requests they are no longer interested in) add a maximum request duration for SourceKitD requests, defaulting to 2 minutes.
rdar://130948453
Since the `Atomic*` types can not be marked as `Sendable` (because they aren’t C structs), we can change the variables to constants and can remove `nonisolated(unsafe)`.
We used C atomics but these were allocated as Swift variables. Even thought they were atomic, concurrent accesses to them could violate Swift’s exclusivity laws, raising thread sanitizer errors.
Allocate the C atomics using malloc to fix this problem.
rdar://129170128
Turns out that sourcekitd test hooks were a bad idea because of the following comment that I wrote:
```
`testHooks` are only considered when an instance is being created. If a sourcekitd instance at the given path already exists, its test hooks will be used.
```
During test execution in Xcode, we generate a bunch of `SourceKitServer` instances in the same process that all call `DynamicallyLoadedSourceKitD.getOrCreate`. Now, if `testDontReturnEmptyDiagnosticsIfDiagnosticRequestIsCancelled` is not the first test being executed in the process (which usually it is not), the test hooks in it won’t get used.
Switch back to using the preparation hooks, essentially reverting https://github.com/apple/sourcekit-lsp/pull/1412 and keeping the following snippet to fix the underlying issue
```swift
// Poll until the `CancelRequestNotification` has been propagated to the request handling.
for _ in 0..<Int(defaultTimeout * 100) {
if Task.isCancelled {
break
}
usleep(10_000)
}
```
I saw a few non-deterministic test failures. I think the issue was that handling of the `CancelRequestNotification` is done asynchronously, which left a short window in which the sourcekitd diagnostics request could run and return results instead of being cancelled. Wait for the diagnostic request to actually be cancelled before running it for real.
While doing this, also introduce proper sourcekitd test hooks instead of relying on the preparation test hooks, which just got run as a side effect.
I saw a failure in CI where a sourcekitd diagnostic request got cancelled without an LSP cancellation request. I am suspecting that there is some implicit cancellation going on in sourcekitd despite `key.cancel_builds: 0`, which doesn’t make sense to me. Adding some explicit logging to sourcekit-lsp to check if we cancel the request for some reason.
Package manifests don’t have an associated target to prepare and are represented by a `ConfiguredTarget` with an empty target ID. We were mistakingly running `swift build` with an empty target name to prepare them, which failed. There is nothing to prepare.
Now that all the types that model test projects are suffixed `Project` instead of `Workspace`, the `ws` abbreviation doesn‘t make sense. It also never really fit with the new style of avoiding abbreviations. Rename all occurrences to `project`.
rdar://124727401
The naming was quite inconsistent here. Let’s rename these to `LanguageService` to highlight that they belong together.
- ToolchainLanguageServer -> LanguageService
- SwiftLanguageServer -> SwiftLanguageService
- ClangLanguageServerShim -> ClangLanguageService
Rename all the classes that write files to disk to create a test project that we can open in sourcekit-lsp to end with `TestProject`. This is better than the old `Workspace` suffix because it avoids ambiguities with the `Workspace` type inside sourcekit-lsp.
- IndexedSingleSwiftFileWorkspace -> IndexedSingleSwiftFileTestProject
- MultiFileTestWorkspace -> MultiFileTestProject
- SwiftPMTestWorkspace -> SwiftPMTestProject
This allows us to prompt the user to file an issue. It’s also useful for developers of sourcekit-lsp to indicate why semantic functionality is limited.
rdar://123391260
Currently, all tests send publish diagnostics notifications, which is noise in the logs for most tests. Change the tests to use the pull diagnostics model by default and make the push diagnostic model opt-in.
rdar://123241539
Naming types in sourcekitd_functions.h `sourcekit_api_` instead of `sourcekitd_` indicates that these are types to be used with dynamically loaded sourcekitd libraries. It avoids confusion if sourcekitd is also linked, which adds the `sourcekitd_` symbols.
Adding nullability annotations to it is also just nice.
And some improved formatting never hurts.
Up until now we have skipped tests on a very ad-hoc basis and some of these skips have lingered around much longer than they were needed.
Add infrastructure to check if a given host toolchain has a feature required by a test. The key ideas here are:
- The skip check specifies a Swift version in which the required feature was introduced. Any newer Swift versions are always considered to support this feature. This allows us to remove the checks once the minimum required version to test sourcekit-lsp exceeds the specified version.
- If the toolchain version is the same as the Swift version that introduces the feature, we execute a closure that performs a fine-grained check to see if the feature is indeed supported. This differentiates toolchain snapshots which contain the feature from those that don’t.
- Tests are never skipped in Swift CI. This ensures that we don’t get passing tests because we hit the skip condition even though we are not expecting to hit it in Swift CI, which should have built an up-to-date toolchain.
- Use this new infrastructure to skip tests that aren’t passing using older Swift versions.
rdar://121911039
Global state is never a good thing and we needed to modify it in tests. The design becomes a lot cleaner if we explicitly pass the toolchain registry around.
