Skip to content

Convert ManualMainFilesProvider and BSMDelegate to be actors #917

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.

Sign up for GitHub

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 2 commits into from
Oct 25, 2023
Merged

Convert ManualMainFilesProvider and BSMDelegate to be actors #917

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f265857
Remove an unused queue from BSMDelegate
joehsieh Oct 19, 2023
ea445cb
Convert ManualMainFilesProvider to be an actor
joehsieh Oct 21, 2023
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
10 changes: 5 additions & 5 deletions Sources/SKCore/BuildSystemManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ extension BuildSystemManager {
for document: DocumentURI,
language: Language
) async -> FileBuildSettings? {
let mainFile = mainFile(for: document)
let mainFile = await mainFile(for: document)
guard var settings = await buildSettings(for: mainFile, language: language) else {
return nil
}
Expand Down Expand Up @@ -182,7 +182,7 @@ extension BuildSystemManager {

public func registerForChangeNotifications(for uri: DocumentURI, language: Language) async {
logger.debug("registerForChangeNotifications(\(uri.forLogging))")
let mainFile = mainFile(for: uri)
let mainFile = await mainFile(for: uri)
self.watchedFiles[uri] = (mainFile, language)

// Register for change notifications of the main file in the underlying build
Expand Down Expand Up @@ -274,7 +274,7 @@ extension BuildSystemManager: MainFilesDelegate {
public func mainFilesChanged() async {
var changedMainFileAssociations: Set<DocumentURI> = []
for (file, (oldMainFile, language)) in self.watchedFiles {
let newMainFile = self.mainFile(for: file, useCache: false)
let newMainFile = await self.mainFile(for: file, useCache: false)
if newMainFile != oldMainFile {
self.watchedFiles[file] = (newMainFile, language)
changedMainFileAssociations.insert(file)
Expand Down Expand Up @@ -303,7 +303,7 @@ extension BuildSystemManager: MainFilesDelegate {
/// For Swift or normal C files, this will be the file itself. For header
/// files, we pick a main file that includes the header since header files
/// don't have build settings by themselves.
private func mainFile(for uri: DocumentURI, useCache: Bool = true) -> DocumentURI {
private func mainFile(for uri: DocumentURI, useCache: Bool = true) async -> DocumentURI {
if useCache, let mainFile = self.watchedFiles[uri]?.mainFile {
// Performance optimization: We did already compute the main file and have
// it cached. We can just return it.
Expand All @@ -313,7 +313,7 @@ extension BuildSystemManager: MainFilesDelegate {
return uri
}

let mainFiles = mainFilesProvider.mainFilesContainingFile(uri)
let mainFiles = await mainFilesProvider.mainFilesContainingFile(uri)
if mainFiles.contains(uri) {
// If the main files contain the file itself, prefer to use that one
return uri
Expand Down
2 changes: 1 addition & 1 deletion Sources/SKCore/MainFilesProvider.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public protocol MainFilesProvider: AnyObject {
/// mainFilesContainingFile("foo.cpp") == Set(["foo.cpp"])
/// mainFilesContainingFile("foo.h") == Set(["foo.cpp", "bar.cpp"])
/// ```
func mainFilesContainingFile(_: DocumentURI) -> Set<DocumentURI>
func mainFilesContainingFile(_: DocumentURI) async -> Set<DocumentURI>
}

/// Delegate that responds to possible main file changes.
Expand Down
92 changes: 43 additions & 49 deletions Tests/SKCoreTests/BuildSystemManagerTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,14 @@ final class BuildSystemManagerTests: XCTestCase {
let c = DocumentURI(string: "bsm:c")
let d = DocumentURI(string: "bsm:d")

let mainFiles = ManualMainFilesProvider()
mainFiles.mainFiles = [
a: Set([c]),
b: Set([c, d]),
c: Set([c]),
d: Set([d]),
]
let mainFiles = ManualMainFilesProvider(
[
a: [c],
b: [c, d],
c: [c],
d: [d],
]
)

let bsm = await BuildSystemManager(
buildSystem: nil,
Expand All @@ -55,13 +56,9 @@ final class BuildSystemManagerTests: XCTestCase {
await assertEqual(bsm._cachedMainFile(for: c), c)
await assertEqual(bsm._cachedMainFile(for: d), d)

mainFiles.mainFiles = [
a: Set([a]),
b: Set([c, d, a]),
c: Set([c]),
d: Set([d]),
]

await mainFiles.updateMainFiles(for: a, to: [a])
await mainFiles.updateMainFiles(for: b, to: [c, d, a])

await assertEqual(bsm._cachedMainFile(for: a), c)
await assertEqual(bsm._cachedMainFile(for: b), bMain)
await assertEqual(bsm._cachedMainFile(for: c), c)
Expand Down Expand Up @@ -92,8 +89,7 @@ final class BuildSystemManagerTests: XCTestCase {

func testSettingsMainFile() async throws {
let a = DocumentURI(string: "bsm:a.swift")
let mainFiles = ManualMainFilesProvider()
mainFiles.mainFiles = [a: Set([a])]
let mainFiles = ManualMainFilesProvider([a: [a]])
let bs = ManualBuildSystem()
let bsm = await BuildSystemManager(
buildSystem: bs,
Expand All @@ -116,8 +112,7 @@ final class BuildSystemManagerTests: XCTestCase {

func testSettingsMainFileInitialNil() async throws {
let a = DocumentURI(string: "bsm:a.swift")
let mainFiles = ManualMainFilesProvider()
mainFiles.mainFiles = [a: Set([a])]
let mainFiles = ManualMainFilesProvider([a: [a]])
let bs = ManualBuildSystem()
let bsm = await BuildSystemManager(
buildSystem: bs,
Expand All @@ -138,8 +133,7 @@ final class BuildSystemManagerTests: XCTestCase {

func testSettingsMainFileWithFallback() async throws {
let a = DocumentURI(string: "bsm:a.swift")
let mainFiles = ManualMainFilesProvider()
mainFiles.mainFiles = [a: Set([a])]
let mainFiles = ManualMainFilesProvider([a: [a]])
let bs = ManualBuildSystem()
let fallback = FallbackBuildSystem(buildSetup: .default)
let bsm = await BuildSystemManager(
Expand Down Expand Up @@ -169,8 +163,7 @@ final class BuildSystemManagerTests: XCTestCase {
func testSettingsMainFileInitialIntersect() async throws {
let a = DocumentURI(string: "bsm:a.swift")
let b = DocumentURI(string: "bsm:b.swift")
let mainFiles = ManualMainFilesProvider()
mainFiles.mainFiles = [a: Set([a]), b: Set([b])]
let mainFiles = ManualMainFilesProvider([a: [a], b: [b]])
let bs = ManualBuildSystem()
let bsm = await BuildSystemManager(
buildSystem: bs,
Expand Down Expand Up @@ -210,8 +203,7 @@ final class BuildSystemManagerTests: XCTestCase {
func testSettingsMainFileUnchanged() async throws {
let a = DocumentURI(string: "bsm:a.swift")
let b = DocumentURI(string: "bsm:b.swift")
let mainFiles = ManualMainFilesProvider()
mainFiles.mainFiles = [a: Set([a]), b: Set([b])]
let mainFiles = ManualMainFilesProvider([a: [a], b: [b]])
let bs = ManualBuildSystem()
let bsm = await BuildSystemManager(
buildSystem: bs,
Expand Down Expand Up @@ -242,12 +234,13 @@ final class BuildSystemManagerTests: XCTestCase {
let h = DocumentURI(string: "bsm:header.h")
let cpp1 = DocumentURI(string: "bsm:main.cpp")
let cpp2 = DocumentURI(string: "bsm:other.cpp")
let mainFiles = ManualMainFilesProvider()
mainFiles.mainFiles = [
h: Set([cpp1]),
cpp1: Set([cpp1]),
cpp2: Set([cpp2]),
]
let mainFiles = ManualMainFilesProvider(
[
h: [cpp1],
cpp1: [cpp1],
cpp2: [cpp2],
]
)

let bs = ManualBuildSystem()
let bsm = await BuildSystemManager(
Expand All @@ -264,7 +257,7 @@ final class BuildSystemManagerTests: XCTestCase {
await bsm.registerForChangeNotifications(for: h, language: .c)
assertEqual(await bsm.buildSettingsInferredFromMainFile(for: h, language: .c), bs.map[cpp1]!)

mainFiles.mainFiles[h] = Set([cpp2])
await mainFiles.updateMainFiles(for: h, to: [cpp2])

let changed = expectation(description: "changed settings to cpp2")
await del.setExpected([(h, .c, bs.map[cpp2]!, changed, #file, #line)])
Expand All @@ -277,14 +270,14 @@ final class BuildSystemManagerTests: XCTestCase {
await bsm.mainFilesChanged()
try await fulfillmentOfOrThrow([changed2], timeout: 1)

mainFiles.mainFiles[h] = Set([cpp1, cpp2])
await mainFiles.updateMainFiles(for: h, to: [cpp1, cpp2])

let changed3 = expectation(description: "added lexicographically earlier main file")
await del.setExpected([(h, .c, bs.map[cpp1]!, changed3, #file, #line)])
await bsm.mainFilesChanged()
try await fulfillmentOfOrThrow([changed3], timeout: 1)

mainFiles.mainFiles[h] = Set([])
await mainFiles.updateMainFiles(for: h, to: [])

let changed4 = expectation(description: "changed settings to []")
await del.setExpected([(h, .c, nil, changed4, #file, #line)])
Expand All @@ -296,11 +289,12 @@ final class BuildSystemManagerTests: XCTestCase {
let h1 = DocumentURI(string: "bsm:header1.h")
let h2 = DocumentURI(string: "bsm:header2.h")
let cpp = DocumentURI(string: "bsm:main.cpp")
let mainFiles = ManualMainFilesProvider()
mainFiles.mainFiles = [
h1: Set([cpp]),
h2: Set([cpp]),
]
let mainFiles = ManualMainFilesProvider(
[
h1: [cpp],
h2: [cpp],
]
)

let bs = ManualBuildSystem()
let bsm = await BuildSystemManager(
Expand Down Expand Up @@ -342,8 +336,7 @@ final class BuildSystemManagerTests: XCTestCase {
let a = DocumentURI(string: "bsm:a.swift")
let b = DocumentURI(string: "bsm:b.swift")
let c = DocumentURI(string: "bsm:c.swift")
let mainFiles = ManualMainFilesProvider()
mainFiles.mainFiles = [a: Set([a]), b: Set([b]), c: Set([c])]
let mainFiles = ManualMainFilesProvider([a: [a], b: [b], c: [c]])
let bs = ManualBuildSystem()
let bsm = await BuildSystemManager(
buildSystem: bs,
Expand Down Expand Up @@ -384,8 +377,7 @@ final class BuildSystemManagerTests: XCTestCase {

func testDependenciesUpdated() async throws {
let a = DocumentURI(string: "bsm:a.swift")
let mainFiles = ManualMainFilesProvider()
mainFiles.mainFiles = [a: Set([a])]
let mainFiles = ManualMainFilesProvider([a: [a]])

let bs = ManualBuildSystem()
let bsm = await BuildSystemManager(
Expand All @@ -412,12 +404,15 @@ final class BuildSystemManagerTests: XCTestCase {
// MARK: Helper Classes for Testing

/// A simple `MainFilesProvider` that wraps a dictionary, for testing.
private final class ManualMainFilesProvider: MainFilesProvider {
let lock: DispatchQueue = DispatchQueue(label: "\(ManualMainFilesProvider.self)-lock")
private var _mainFiles: [DocumentURI: Set<DocumentURI>] = [:]
var mainFiles: [DocumentURI: Set<DocumentURI>] {
get { lock.sync { _mainFiles } }
set { lock.sync { _mainFiles = newValue } }
private final actor ManualMainFilesProvider: MainFilesProvider {
private var mainFiles: [DocumentURI: Set<DocumentURI>]

init(_ mainFiles: [DocumentURI: Set<DocumentURI>]) {
self.mainFiles = mainFiles
}

func updateMainFiles(for file: DocumentURI, to mainFiles: Set<DocumentURI>)async {
self.mainFiles[file] = mainFiles
}

func mainFilesContainingFile(_ file: DocumentURI) -> Set<DocumentURI> {
Expand Down Expand Up @@ -473,7 +468,6 @@ private actor BSMDelegate: BuildSystemDelegate {
uri: DocumentURI, expectation: XCTestExpectation, file: StaticString, line: UInt
)

let queue: DispatchQueue = DispatchQueue(label: "\(BSMDelegate.self)")
unowned let bsm: BuildSystemManager
var expected: [ExpectedBuildSettingChangedCall] = []

Expand Down