Skip to content

Add a general notion of experimental features to sourcekit-lsp #1389

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
merged 1 commit into from
Jun 2, 2024
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/Diagnose/IndexCommand.swift
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ public struct IndexCommand: AsyncParsableCommand {

public func run() async throws {
var serverOptions = SourceKitLSPServer.Options()
serverOptions.indexOptions.enableBackgroundIndexing = true
serverOptions.experimentalFeatures.append(.backgroundIndexing)

let installPath =
if let toolchainOverride, let toolchain = Toolchain(try AbsolutePath(validating: toolchainOverride)) {
Expand Down
4 changes: 3 additions & 1 deletion Sources/SKTestSupport/TestSourceKitLSPClient.swift
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,9 @@ public final class TestSourceKitLSPClient: MessageHandler {
if let moduleCache {
serverOptions.buildSetup.flags.swiftCompilerFlags += ["-module-cache-path", moduleCache.path]
}
serverOptions.indexOptions.enableBackgroundIndexing = enableBackgroundIndexing
if enableBackgroundIndexing {
serverOptions.experimentalFeatures.append(.backgroundIndexing)
}

var notificationYielder: AsyncStream<any NotificationType>.Continuation!
self.notifications = AsyncStream { continuation in
Expand Down
1 change: 1 addition & 0 deletions Sources/SourceKitLSP/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ add_library(SourceKitLSP STATIC
CreateBuildSystem.swift
DocumentManager.swift
DocumentSnapshot+FromFileContents.swift
ExperimentalFeatures.swift
IndexProgressManager.swift
IndexStoreDB+MainFilesProvider.swift
LanguageServerType.swift
Expand Down
2 changes: 1 addition & 1 deletion Sources/SourceKitLSP/CreateBuildSystem.swift
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func createBuildSystem(
url: rootUrl,
toolchainRegistry: toolchainRegistry,
buildSetup: options.buildSetup,
isForIndexBuild: options.indexOptions.enableBackgroundIndexing,
isForIndexBuild: options.experimentalFeatures.contains(.backgroundIndexing),
reloadPackageStatusCallback: reloadPackageStatusCallback
)
}
Expand Down
26 changes: 26 additions & 0 deletions Sources/SourceKitLSP/ExperimentalFeatures.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
//===----------------------------------------------------------------------===//
//
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2014 - 2024 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See https://swift.org/LICENSE.txt for license information
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
//
//===----------------------------------------------------------------------===//

/// An experimental feature that can be enabled by passing `--experimental-feature` to `sourcekit-lsp` on the command
/// line. The raw value of this feature is how it is named on the command line.
public enum ExperimentalFeature: String, Codable, Sendable, CaseIterable {
/// Enable background indexing.
case backgroundIndexing = "background-indexing"

/// Show the files that are currently being indexed / the targets that are currently being prepared in the work done
/// progress.
///
/// This is an option because VS Code tries to render a multi-line work done progress into a single line text field in
/// the status bar, which looks broken. But at the same time, it is very useful to get a feeling about what's
/// currently happening indexing-wise.
case showActivePreparationTasksInProgress = "show-active-preparation-tasks-in-progress"
}
2 changes: 1 addition & 1 deletion Sources/SourceKitLSP/IndexProgressManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ actor IndexProgressManager {
// Clip the finished tasks to 0 because showing a negative number there looks stupid.
let finishedTasks = max(queuedIndexTasks - indexTasks.count, 0)
message = "\(finishedTasks) / \(queuedIndexTasks)"
if await sourceKitLSPServer.options.indexOptions.showActivePreparationTasksInProgress {
if await sourceKitLSPServer.options.experimentalFeatures.contains(.showActivePreparationTasksInProgress) {
var inProgressTasks: [String] = []
inProgressTasks += preparationTasks.filter { $0.value == .executing }
.map { "- Preparing \($0.key.targetID)" }
Expand Down
5 changes: 5 additions & 0 deletions Sources/SourceKitLSP/SourceKitLSPServer+Options.swift
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ extension SourceKitLSPServer {
/// notification when running unit tests.
public var swiftPublishDiagnosticsDebounceDuration: TimeInterval

/// Experimental features that are enabled.
public var experimentalFeatures: [ExperimentalFeature]
Comment on lines +52 to +53
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to make a Set?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My assumption was that you usually pass less than 3 experimental features (honestly, most likely you pass 0) and I think a linear scan through an array is quicker than a set lookup in that case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I wasn't really thinking of performance, more just semantically it feels like Set better fits. But I don't feel strongly at all 🤷

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither do I. It’s in as an array now, let’s just leave it.


public var indexTestHooks: IndexTestHooks

public init(
Expand All @@ -59,6 +62,7 @@ extension SourceKitLSPServer {
completionOptions: SKCompletionOptions = .init(),
generatedInterfacesPath: AbsolutePath = defaultDirectoryForGeneratedInterfaces,
swiftPublishDiagnosticsDebounceDuration: TimeInterval = 2, /* 2s */
experimentalFeatures: [ExperimentalFeature] = [],
indexTestHooks: IndexTestHooks = IndexTestHooks()
) {
self.buildSetup = buildSetup
Expand All @@ -68,6 +72,7 @@ extension SourceKitLSPServer {
self.completionOptions = completionOptions
self.generatedInterfacesPath = generatedInterfacesPath
self.swiftPublishDiagnosticsDebounceDuration = swiftPublishDiagnosticsDebounceDuration
self.experimentalFeatures = experimentalFeatures
self.indexTestHooks = indexTestHooks
}
}
Expand Down
2 changes: 1 addition & 1 deletion Sources/SourceKitLSP/SourceKitLSPServer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -941,7 +941,7 @@ extension SourceKitLSPServer {
self?.indexProgressManager.indexProgressStatusDidChange()
}
)
if let workspace, options.indexOptions.enableBackgroundIndexing, workspace.semanticIndexManager == nil,
if let workspace, options.experimentalFeatures.contains(.backgroundIndexing), workspace.semanticIndexManager == nil,
!self.didSendBackgroundIndexingNotSupportedNotification
{
self.sendNotificationToClient(
Expand Down
19 changes: 2 additions & 17 deletions Sources/SourceKitLSP/Workspace.swift
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ public final class Workspace: Sendable {
mainFilesProvider: uncheckedIndex,
toolchainRegistry: toolchainRegistry
)
if options.indexOptions.enableBackgroundIndexing,
if options.experimentalFeatures.contains(.backgroundIndexing),
let uncheckedIndex,
await buildSystemManager.supportsPreparation
{
Expand Down Expand Up @@ -247,37 +247,22 @@ public struct IndexOptions: Sendable {
/// explicit calls to pollForUnitChangesAndWait().
public var listenToUnitEvents: Bool

/// Whether background indexing should be enabled.
public var enableBackgroundIndexing: Bool

/// The percentage of the machine's cores that should at most be used for background indexing.
///
/// Setting this to a value < 1 ensures that background indexing doesn't use all CPU resources.
public var maxCoresPercentageToUseForBackgroundIndexing: Double

/// Whether to show the files that are currently being indexed / the targets that are currently being prepared in the
/// work done progress.
///
/// This is an option because VS Code tries to render a multi-line work done progress into a single line text field in
/// the status bar, which looks broken. But at the same time, it is very useful to get a feeling about what's
/// currently happening indexing-wise.
public var showActivePreparationTasksInProgress: Bool

public init(
indexStorePath: AbsolutePath? = nil,
indexDatabasePath: AbsolutePath? = nil,
indexPrefixMappings: [PathPrefixMapping]? = nil,
listenToUnitEvents: Bool = true,
enableBackgroundIndexing: Bool = false,
maxCoresPercentageToUseForBackgroundIndexing: Double = 1,
showActivePreparationTasksInProgress: Bool = false
maxCoresPercentageToUseForBackgroundIndexing: Double = 1
) {
self.indexStorePath = indexStorePath
self.indexDatabasePath = indexDatabasePath
self.indexPrefixMappings = indexPrefixMappings
self.listenToUnitEvents = listenToUnitEvents
self.enableBackgroundIndexing = enableBackgroundIndexing
self.maxCoresPercentageToUseForBackgroundIndexing = maxCoresPercentageToUseForBackgroundIndexing
self.showActivePreparationTasksInProgress = showActivePreparationTasksInProgress
}
}
22 changes: 11 additions & 11 deletions Sources/sourcekit-lsp/SourceKitLSP.swift
Original file line number Diff line number Diff line change
Expand Up @@ -201,18 +201,14 @@ struct SourceKitLSP: AsyncParsableCommand {
)
var completionMaxResults = 200

@Flag(
help: "Enable background indexing. This feature is still under active development and may be incomplete."
)
var experimentalEnableBackgroundIndexing = false

@Flag(
@Option(
name: .customLong("experimental-feature"),
help: """
When reporting index progress, show the currently running index tasks in addition to the task's count. \
This produces a multi-line work done progress, which might render incorrectly, depending on the editor.
Enable an experimental sourcekit-lsp feature.
Available features are: \(ExperimentalFeature.allCases.map(\.rawValue).joined(separator: ", "))
"""
)
var experimentalShowActivePreparationTasksInProgress = false
var experimentalFeatures: [ExperimentalFeature]

func mapOptions() -> SourceKitLSPServer.Options {
var serverOptions = SourceKitLSPServer.Options()
Expand All @@ -229,8 +225,6 @@ struct SourceKitLSP: AsyncParsableCommand {
serverOptions.indexOptions.indexStorePath = indexStorePath
serverOptions.indexOptions.indexDatabasePath = indexDatabasePath
serverOptions.indexOptions.indexPrefixMappings = indexPrefixMappings
serverOptions.indexOptions.enableBackgroundIndexing = experimentalEnableBackgroundIndexing
serverOptions.indexOptions.showActivePreparationTasksInProgress = experimentalShowActivePreparationTasksInProgress
serverOptions.completionOptions.maxResults = completionMaxResults
serverOptions.generatedInterfacesPath = generatedInterfacesPath

Expand Down Expand Up @@ -285,3 +279,9 @@ struct SourceKitLSP: AsyncParsableCommand {
try await Task.sleep(for: .seconds(60 * 60 * 24 * 365 * 10))
}
}

#if compiler(>=6)
extension ExperimentalFeature: @retroactive ExpressibleByArgument {}
#else
extension ExperimentalFeature: ExpressibleByArgument {}
#endif