Skip to content

Commit 74df495

Browse files
DavidGoldmanDavidGoldman
committed
Swap BuildSystem to remove build settings method
Instead the `BuildSystem` should inform its delegate of changes to build settings. Change-Id: I0040e360825cf12281e1867e1b365a21c9b35a48
1 parent c051601 commit 74df495

18 files changed

+170
-94
lines changed

‎Sources/SKCore/BuildServerBuildSystem.swift

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -130,16 +130,22 @@ final class BuildServerHandler: LanguageServerEndpoint {
130130
}
131131

132132
func handleFileOptionsChanged(_ notification: Notification<FileOptionsChangedNotification>) {
133-
// TODO: add delegate method to include the changed settings directly
134-
self.delegate?.fileBuildSettingsChanged([notification.params.uri])
133+
let params = notification.params
134+
let options = params.updatedOptions
135+
let fileBuildSettings = FileBuildSettings(
136+
compilerArguments: options.options, workingDirectory: options.workingDirectory)
137+
self.delegate?.fileBuildSettingsChanged([params.uri: .modified(fileBuildSettings)])
135138
}
136139
}
137140

138141
extension BuildServerBuildSystem: BuildSystem {
139142

140143
/// Register the given file for build-system level change notifications, such as command
141144
/// line flag changes, dependency changes, etc.
142-
public func registerForChangeNotifications(for url: LanguageServerProtocol.URL) {
145+
public func registerForChangeNotifications(
146+
for url: LanguageServerProtocol.URL,
147+
language: Language)
148+
{
143149
let request = RegisterForChanges(uri: url, action: .register)
144150
_ = self.buildServer?.send(request, queue: requestQueue, reply: { result in
145151
if let error = result.failure {
@@ -163,6 +169,11 @@ extension BuildServerBuildSystem: BuildSystem {
163169
return buildFolder?.appending(components: "index", "db")
164170
}
165171

172+
public func cachedSettings(for: URL, _ language: Language) -> FileBuildSettings? {
173+
// TODO: Implement caching once the change notifications are supported.
174+
return nil
175+
}
176+
166177
public func settings(for url: URL, _ language: Language) -> FileBuildSettings? {
167178
if let response = try? self.buildServer?.sendSync(SourceKitOptions(uri: url)) {
168179
return FileBuildSettings(compilerArguments: response.options, workingDirectory: response.workingDirectory)

‎Sources/SKCore/BuildSystem.swift

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,9 +31,6 @@ public protocol BuildSystem: AnyObject {
3131
/// The path to put the index database, if any.
3232
var indexDatabasePath: AbsolutePath? { get }
3333

34-
/// Returns the settings for the given url and language mode, if known.
35-
func settings(for: URL, _ language: Language) -> FileBuildSettings?
36-
3734
/// Returns the toolchain to use to compile this file
3835
func toolchain(for: URL, _ language: Language) -> Toolchain?
3936

@@ -43,7 +40,10 @@ public protocol BuildSystem: AnyObject {
4340

4441
/// Register the given file for build-system level change notifications, such
4542
/// as command line flag changes, dependency changes, etc.
46-
func registerForChangeNotifications(for: URL)
43+
///
44+
/// If the `BuildSystem` has cached build settings information, it is recommended to
45+
/// immediately notify its delegate, as an optimization.
46+
func registerForChangeNotifications(for: URL, language: Language)
4747

4848
/// Unregister the given file for build-system level change notifications,
4949
/// such as command line flag changes, dependency changes, etc.

‎Sources/SKCore/BuildSystemDelegate.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,5 +19,5 @@ public protocol BuildSystemDelegate: AnyObject {
1919
/// Notify the delegate that the given files' build settings have changed.
2020
///
2121
/// The callee should request new build settings for any of the given files that they are interested in.
22-
func fileBuildSettingsChanged(_ changedFiles: Set<URL>)
22+
func fileBuildSettingsChanged(_ changes: [URL: FileBuildSettingsChange])
2323
}

‎Sources/SKCore/BuildSystemList.swift

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -36,20 +36,11 @@ extension BuildSystemList: BuildSystem {
3636

3737
public var indexDatabasePath: AbsolutePath? { return providers.first?.indexDatabasePath }
3838

39-
public func settings(for url: URL, _ language: Language) -> FileBuildSettings? {
40-
for provider in providers {
41-
if let settings = provider.settings(for: url, language) {
42-
return settings
43-
}
44-
}
45-
return nil
46-
}
47-
4839
/// Register the given file for build-system level change notifications, such as command
4940
/// line flag changes, dependency changes, etc.
50-
public func registerForChangeNotifications(for url: URL) {
41+
public func registerForChangeNotifications(for url: URL, language: Language) {
5142
// Only register with the primary build system, since we only use its delegate.
52-
providers.first?.registerForChangeNotifications(for: url)
43+
providers.first?.registerForChangeNotifications(for: url, language: language)
5344
}
5445

5546
/// Unregister the given file for build-system level change notifications, such as command

‎Sources/SKCore/CompilationDatabaseBuildSystem.swift

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,12 @@ extension CompilationDatabaseBuildSystem: BuildSystem {
5454

5555
public func toolchain(for: URL, _ language: Language) -> Toolchain? { return nil }
5656

57-
/// We don't support change watching.
58-
public func registerForChangeNotifications(for: URL) {}
57+
/// We don't support change watching, so we only notify our `delegate` of the settings here.
58+
public func registerForChangeNotifications(for url: URL, language: Language) {
59+
if let settings = self.settings(for: url, language) {
60+
self.delegate?.fileBuildSettingsChanged([url: .modified(settings)])
61+
}
62+
}
5963

6064
/// We don't support change watching.
6165
public func unregisterForChangeNotifications(for: URL) {}

‎Sources/SKCore/FallbackBuildSystem.swift

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,12 @@ public final class FallbackBuildSystem: BuildSystem {
5454
}
5555
}
5656

57-
/// We don't support change watching.
58-
public func registerForChangeNotifications(for: URL) {}
57+
/// We don't support change watching, so we only notify our `delegate` of the settings here.
58+
public func registerForChangeNotifications(for url: URL, language: Language) {
59+
if let settings = self.settings(for: url, language) {
60+
self.delegate?.fileBuildSettingsChanged([url: .modified(settings)])
61+
}
62+
}
5963

6064
/// We don't support change watching.
6165
public func unregisterForChangeNotifications(for: URL) {}

‎Sources/SKCore/FileBuildSettingsChange.swift

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
//===----------------------------------------------------------------------===//
2+
//
3+
// This source file is part of the Swift.org open source project
4+
//
5+
// Copyright (c) 2014 - 2019 Apple Inc. and the Swift project authors
6+
// Licensed under Apache License v2.0 with Runtime Library Exception
7+
//
8+
// See https://swift.org/LICENSE.txt for license information
9+
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
10+
//
11+
//===----------------------------------------------------------------------===//
12+
13+
/// Denotes a change in build settings for a single file.
14+
public enum FileBuildSettingsChange {
15+
16+
/// The `BuildSystem` no longer has `FileBuildSettings` for the file.
17+
case removed
18+
19+
/// The `FileBuildSettings` have been modified.
20+
case modified(FileBuildSettings)
21+
}
22+
23+
public extension FileBuildSettingsChange {
24+
var newFileBuildSettings: FileBuildSettings? {
25+
switch self {
26+
case .removed:
27+
return nil
28+
case .modified(let settings):
29+
return settings
30+
}
31+
}
32+
}

‎Sources/SKSwiftPMWorkspace/SwiftPMWorkspace.swift

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -224,8 +224,14 @@ extension SwiftPMWorkspace: BuildSystem {
224224

225225
/// Register the given file for build-system level change notifications, such as command
226226
/// line flag changes, dependency changes, etc.
227-
public func registerForChangeNotifications(for url: LanguageServerProtocol.URL) {
227+
public func registerForChangeNotifications(
228+
for url: LanguageServerProtocol.URL,
229+
language: Language)
230+
{
228231
// TODO: Support for change detection (via file watching)
232+
if let settings = self.settings(for: url, language) {
233+
self.delegate?.fileBuildSettingsChanged([url: .modified(settings)])
234+
}
229235
}
230236

231237
/// Unregister the given file for build-system level change notifications, such as command

‎Sources/SKTestSupport/SKSwiftPMTestWorkspace.swift

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,17 @@ extension SKSwiftPMTestWorkspace {
125125
version: 1,
126126
text: try sources.sourceCache.get(url))))
127127
}
128+
129+
public func initialize(clientCapabilities: ClientCapabilities) throws {
130+
_ = try sk.sendSync(InitializeRequest(
131+
processId: nil,
132+
rootPath: nil,
133+
rootURL: nil,
134+
initializationOptions: nil,
135+
capabilities: clientCapabilities,
136+
trace: .off,
137+
workspaceFolders: nil))
138+
}
128139
}
129140

130141
extension XCTestCase {

‎Sources/SourceKit/SourceKitServer.swift

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -238,21 +238,21 @@ public final class SourceKitServer: LanguageServer {
238238
// MARK: - Build System Delegate
239239

240240
extension SourceKitServer: BuildSystemDelegate {
241-
public func fileBuildSettingsChanged(_ changedFiles: Set<URL>) {
241+
public func fileBuildSettingsChanged(_ changes: [URL : FileBuildSettingsChange]) {
242242
guard let workspace = self.workspace else {
243243
return
244244
}
245245
let documentManager = workspace.documentManager
246246
let openDocuments = documentManager.openDocuments
247-
for url in changedFiles {
247+
for (url, change) in changes {
248248
guard openDocuments.contains(url) else {
249249
continue
250250
}
251251

252252
log("Build settings changed for opened file \(url)")
253253
if let snapshot = documentManager.latestSnapshot(url),
254254
let service = languageService(for: url, snapshot.document.language, in: workspace) {
255-
service.documentUpdatedBuildSettings(url, language: snapshot.document.language)
255+
service.documentChangedBuildSettings(url, change)
256256
}
257257
}
258258
}
@@ -360,11 +360,16 @@ extension SourceKitServer {
360360
workspace.documentManager.open(note.params)
361361

362362
let textDocument = note.params.textDocument
363-
workspace.buildSettings.registerForChangeNotifications(for: textDocument.url)
363+
let url = textDocument.url
364+
let language = textDocument.language
364365

365-
if let service = languageService(for: textDocument.url, textDocument.language, in: workspace) {
366-
service.openDocument(note.params)
367-
}
366+
// Create the `service` before registering for change notifications in case the `BuildSystem`
367+
// immediately callbacks to us (its delegate) with `FileBuildSettings` so we notify this
368+
// `service` (which might have been newly created).
369+
let service = languageService(for: url, language, in: workspace)
370+
371+
workspace.buildSettings.registerForChangeNotifications(for: url, language: language)
372+
service?.openDocument(note.params)
368373
}
369374

370375
func closeDocument(_ note: Notification<DidCloseTextDocument>, workspace: Workspace) {
@@ -708,11 +713,11 @@ public func languageService(
708713

709714
case .c, .cpp, .objective_c, .objective_cpp:
710715
guard toolchain.clangd != nil else { return nil }
711-
return try makeJSONRPCClangServer(client: client, toolchain: toolchain, buildSettings: (client as? SourceKitServer)?.workspace?.buildSettings, clangdOptions: options.clangdOptions)
716+
return try makeJSONRPCClangServer(client: client, toolchain: toolchain, clangdOptions: options.clangdOptions)
712717

713718
case .swift:
714719
guard let sourcekitd = toolchain.sourcekitd else { return nil }
715-
return try makeLocalSwiftServer(client: client, sourcekitd: sourcekitd, buildSettings: (client as? SourceKitServer)?.workspace?.buildSettings, clientCapabilities: (client as? SourceKitServer)?.workspace?.clientCapabilities)
720+
return try makeLocalSwiftServer(client: client, sourcekitd: sourcekitd, clientCapabilities: (client as? SourceKitServer)?.workspace?.clientCapabilities)
716721

717722
default:
718723
return nil

‎Sources/SourceKit/ToolchainLanguageServer.swift

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212

1313
import Foundation
1414
import LanguageServerProtocol
15+
import SKCore
1516

1617
/// A `LanguageServer` that exists within the context of the current process.
1718
public protocol ToolchainLanguageServer: AnyObject {
@@ -28,7 +29,11 @@ public protocol ToolchainLanguageServer: AnyObject {
2829
func changeDocument(_ note: DidChangeTextDocument)
2930
func willSaveDocument(_ note: WillSaveTextDocument)
3031
func didSaveDocument(_ note: DidSaveTextDocument)
31-
func documentUpdatedBuildSettings(_ url: URL, language: Language)
32+
33+
// MARK: - Build System Interactions
34+
35+
/// Note that this may be called before a respective `openDocument`.
36+
func documentChangedBuildSettings(_ url: URL, _ change: FileBuildSettingsChange)
3237

3338
// MARK: - Text Document
3439

‎Sources/SourceKit/clangd/ClangLanguageServer.swift

Lines changed: 20 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -27,15 +27,11 @@ final class ClangLanguageServerShim: ToolchainLanguageServer {
2727

2828
var capabilities: ServerCapabilities? = nil
2929

30-
let buildSystem: BuildSystem
31-
3230
let clang: AbsolutePath?
3331

3432
/// Creates a language server for the given client using the sourcekitd dylib at the specified path.
35-
public init(client: Connection, clangd: Connection, buildSystem: BuildSystem,
36-
clang: AbsolutePath?) throws {
33+
public init(client: Connection, clangd: Connection, clang: AbsolutePath?) throws {
3734
self.clangd = clangd
38-
self.buildSystem = buildSystem
3935
self.clang = clang
4036
}
4137

@@ -71,6 +67,8 @@ final class ClangLanguageServerShim: ToolchainLanguageServer {
7167

7268
extension ClangLanguageServerShim {
7369

70+
// MARK: - Lifetime
71+
7472
func initializeSync(_ initialize: InitializeRequest) throws -> InitializeResult {
7573
let result = try clangd.sendSync(initialize)
7674
self.capabilities = result.capabilities
@@ -81,11 +79,26 @@ extension ClangLanguageServerShim {
8179
clangd.send(initialized)
8280
}
8381

82+
// MARK: - Build System Interactions
83+
84+
public func documentChangedBuildSettings(_ url: URL, _ change: FileBuildSettingsChange) {
85+
let buildSettings = change.newFileBuildSettings
86+
87+
logAsync(level: buildSettings == nil ? .warning : .debug) { _ in
88+
let settingsStr = buildSettings == nil ? "nil" : buildSettings!.compilerArguments.description
89+
return "settings for \(url): \(settingsStr)"
90+
}
91+
92+
if let settings = buildSettings {
93+
clangd.send(DidChangeConfiguration(settings: .clangd(
94+
ClangWorkspaceSettings(
95+
compilationDatabaseChanges: [url.path: ClangCompileCommand(settings, clang: clang)]))))
96+
}
97+
}
98+
8499
// MARK: - Text synchronization
85100

86101
public func openDocument(_ note: DidOpenTextDocument) {
87-
let textDocument = note.textDocument
88-
documentUpdatedBuildSettings(textDocument.url, language: textDocument.language)
89102
clangd.send(note)
90103
}
91104

@@ -105,21 +118,6 @@ extension ClangLanguageServerShim {
105118

106119
}
107120

108-
public func documentUpdatedBuildSettings(_ url: URL, language: Language) {
109-
let settings = buildSystem.settings(for: url, language)
110-
111-
logAsync(level: settings == nil ? .warning : .debug) { _ in
112-
let settingsStr = settings == nil ? "nil" : settings!.compilerArguments.description
113-
return "settings for \(url): \(settingsStr)"
114-
}
115-
116-
if let settings = settings {
117-
clangd.send(DidChangeConfiguration(settings: .clangd(
118-
ClangWorkspaceSettings(
119-
compilationDatabaseChanges: [url.path: ClangCompileCommand(settings, clang: clang)]))))
120-
}
121-
}
122-
123121
// MARK: - Text Document
124122

125123
func completion(_ req: Request<CompletionRequest>) {
@@ -173,7 +171,6 @@ extension ClangLanguageServerShim {
173171
func makeJSONRPCClangServer(
174172
client: MessageHandler,
175173
toolchain: Toolchain,
176-
buildSettings: BuildSystem?,
177174
clangdOptions: [String]
178175
) throws -> ToolchainLanguageServer {
179176
guard let clangd = toolchain.clangd else {
@@ -194,7 +191,6 @@ func makeJSONRPCClangServer(
194191
let shim = try ClangLanguageServerShim(
195192
client: connectionToClient,
196193
clangd: connection,
197-
buildSystem: buildSettings ?? BuildSystemList(),
198194
clang: toolchain.clang)
199195

200196
connectionToClient.start(handler: client)

0 commit comments

Comments
 (0)