Skip to content

When a file is changed, only mark targets that depend on it as out-of-date #1313

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
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
4 changes: 4 additions & 0 deletions Sources/SKCore/BuildServerBuildSystem.swift
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,10 @@ extension BuildServerBuildSystem: BuildSystem {
return nil
}

public func targets(dependingOn targets: [ConfiguredTarget]) -> [ConfiguredTarget]? {
return nil
}

public func prepare(targets: [ConfiguredTarget]) async throws {
throw PrepareNotSupportedError()
}
Expand Down
9 changes: 9 additions & 0 deletions Sources/SKCore/BuildSystem.swift
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,15 @@ public protocol BuildSystem: AnyObject, Sendable {
/// `nil` if the build system doesn't support topological sorting of targets.
func topologicalSort(of targets: [ConfiguredTarget]) async -> [ConfiguredTarget]?

/// Returns the list of targets that might depend on the given target and that need to be re-prepared when a file in
/// `target` is modified.
///
/// The returned list can be an over-approximation, in which case the indexer will perform more work than strictly
/// necessary by scheduling re-preparation of a target where it isn't necessary.
///
/// Returning `nil` indicates that all targets should be considered depending on the given target.
func targets(dependingOn targets: [ConfiguredTarget]) async -> [ConfiguredTarget]?
Copy link
Contributor

Choose a reason for hiding this comment

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

Would taking Target make more sense here? We don't really care about the possibly large set of configured targets as input. I'd argue that's even the case for output, but we do need the configured target eventually since that's what we track preparation of.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, Alex brought up a very valid point here. It's entirely possible that while the concept of a "target" may contain the file, the underlying target + platform may not - consider settings for including/excluding files per platform. So ConfiguredTarget really is the right abstraction here, thanks for remembering that one :).

Copy link
Member Author

Choose a reason for hiding this comment

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

Added the following comment to SemanticIndexManager.filesDidChange

    // Note that configured targets are the right abstraction layer here (instead of a non-configured target) because a
    // build system might have targets that include different source files. Hence a source file might be in target T
    // configured for macOS but not in target T configured for iOS.


/// Prepare the given targets for indexing and semantic functionality. This should build all swift modules of target
/// dependencies.
func prepare(targets: [ConfiguredTarget]) async throws
Expand Down
11 changes: 10 additions & 1 deletion Sources/SKCore/BuildSystemManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -126,12 +126,17 @@ extension BuildSystemManager {
}
}

/// Returns all the `ConfiguredTarget`s that the document is part of.
public func configuredTargets(for document: DocumentURI) async -> [ConfiguredTarget] {
return await buildSystem?.configuredTargets(for: document) ?? []
}

/// Returns the `ConfiguredTarget` that should be used for semantic functionality of the given document.
public func canonicalConfiguredTarget(for document: DocumentURI) async -> ConfiguredTarget? {
// Sort the configured targets to deterministically pick the same `ConfiguredTarget` every time.
// We could allow the user to specify a preference of one target over another. For now this is not necessary because
// no build system currently returns multiple targets for a source file.
return await buildSystem?.configuredTargets(for: document)
return await configuredTargets(for: document)
.sorted { ($0.targetID, $0.runDestinationID) < ($1.targetID, $1.runDestinationID) }
.first
}
Expand Down Expand Up @@ -220,6 +225,10 @@ extension BuildSystemManager {
return await buildSystem?.topologicalSort(of: targets)
}

public func targets(dependingOn targets: [ConfiguredTarget]) async -> [ConfiguredTarget]? {
return await buildSystem?.targets(dependingOn: targets)
}

public func prepare(targets: [ConfiguredTarget]) async throws {
try await buildSystem?.prepare(targets: targets)
}
Expand Down
4 changes: 4 additions & 0 deletions Sources/SKCore/CompilationDatabaseBuildSystem.swift
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,10 @@ extension CompilationDatabaseBuildSystem: BuildSystem {
return nil
}

public func targets(dependingOn targets: [ConfiguredTarget]) -> [ConfiguredTarget]? {
return nil
}

public func registerForChangeNotifications(for uri: DocumentURI) async {
self.watchedFiles.insert(uri)
}
Expand Down
55 changes: 42 additions & 13 deletions Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,14 @@ private func getDefaultToolchain(_ toolchainRegistry: ToolchainRegistry) async -
return await toolchainRegistry.default
}

fileprivate extension ConfiguredTarget {
init(_ buildTarget: any SwiftBuildTarget) {
self.init(targetID: buildTarget.name, runDestinationID: "dummy")
}

static let forPackageManifest = ConfiguredTarget(targetID: "", runDestinationID: "")
}

/// Swift Package Manager build system and workspace support.
///
/// This class implements the `BuildSystem` interface to provide the build settings for a Swift
Expand Down Expand Up @@ -101,10 +109,10 @@ public actor SwiftPMBuildSystem {
var fileToTarget: [AbsolutePath: SwiftBuildTarget] = [:]
var sourceDirToTarget: [AbsolutePath: SwiftBuildTarget] = [:]

/// Maps target ids (aka. `ConfiguredTarget.targetID`) to their SwiftPM build target as well as an index in their
/// topological sorting. Targets with lower index are more low level, ie. targets with higher indices depend on
/// targets with lower indices.
var targets: [String: (index: Int, buildTarget: SwiftBuildTarget)] = [:]
/// Maps configured targets ids to their SwiftPM build target as well as an index in their topological sorting.
///
/// Targets with lower index are more low level, ie. targets with higher indices depend on targets with lower indices.
var targets: [ConfiguredTarget: (index: Int, buildTarget: SwiftBuildTarget)] = [:]

/// The URIs for which the delegate has registered for change notifications,
/// mapped to the language the delegate specified when registering for change notifications.
Expand Down Expand Up @@ -289,7 +297,7 @@ extension SwiftPMBuildSystem {

self.targets = Dictionary(
try buildDescription.allTargetsInTopologicalOrder(in: modulesGraph).enumerated().map { (index, target) in
return (key: target.name, (index, target))
return (key: ConfiguredTarget(target), (index, target))
},
uniquingKeysWith: { first, second in
logger.fault("Found two targets with the same name \(first.buildTarget.name)")
Expand Down Expand Up @@ -362,11 +370,11 @@ extension SwiftPMBuildSystem: SKCore.BuildSystem {
return nil
}

if configuredTarget.targetID == "" {
if configuredTarget == .forPackageManifest {
return try settings(forPackageManifest: path)
}

guard let buildTarget = self.targets[configuredTarget.targetID]?.buildTarget else {
guard let buildTarget = self.targets[configuredTarget]?.buildTarget else {
logger.error("Did not find target with name \(configuredTarget.targetID)")
return nil
}
Expand Down Expand Up @@ -397,13 +405,13 @@ extension SwiftPMBuildSystem: SKCore.BuildSystem {
}

if let target = try? buildTarget(for: path) {
return [ConfiguredTarget(targetID: target.name, runDestinationID: "dummy")]
return [ConfiguredTarget(target)]
}

if path.basename == "Package.swift" {
// We use an empty target name to represent the package manifest since an empty target name is not valid for any
// user-defined target.
return [ConfiguredTarget(targetID: "", runDestinationID: "dummy")]
return [ConfiguredTarget.forPackageManifest]
}

if url.pathExtension == "h", let target = try? target(forHeader: path) {
Expand All @@ -415,19 +423,40 @@ extension SwiftPMBuildSystem: SKCore.BuildSystem {

public func topologicalSort(of targets: [ConfiguredTarget]) -> [ConfiguredTarget]? {
return targets.sorted { (lhs: ConfiguredTarget, rhs: ConfiguredTarget) -> Bool in
let lhsIndex = self.targets[lhs.targetID]?.index ?? self.targets.count
let rhsIndex = self.targets[lhs.targetID]?.index ?? self.targets.count
let lhsIndex = self.targets[lhs]?.index ?? self.targets.count
let rhsIndex = self.targets[lhs]?.index ?? self.targets.count
return lhsIndex < rhsIndex
}
}

public func targets(dependingOn targets: [ConfiguredTarget]) -> [ConfiguredTarget]? {
let targetIndices = targets.compactMap { self.targets[$0]?.index }
let minimumTargetIndex: Int?
if targetIndices.count == targets.count {
minimumTargetIndex = targetIndices.min()
} else {
// One of the targets didn't have an entry in self.targets. We don't know what might depend on it.
minimumTargetIndex = nil
}

// Files that occur before the target in the topological sorting don't depend on it.
// Ideally, we should consult the dependency graph here for more accurate dependency analysis instead of relying on
// a flattened list (https://github.com/apple/sourcekit-lsp/issues/1312).
return self.targets.compactMap { (configuredTarget, value) -> ConfiguredTarget? in
if let minimumTargetIndex, value.index <= minimumTargetIndex {
return nil
}
return configuredTarget
}
}

public func prepare(targets: [ConfiguredTarget]) async throws {
// TODO (indexing): Support preparation of multiple targets at once.
// https://github.com/apple/sourcekit-lsp/issues/1262
for target in targets {
try await prepare(singleTarget: target)
}
let filesInPreparedTargets = targets.flatMap { self.targets[$0.targetID]?.buildTarget.sources ?? [] }
let filesInPreparedTargets = targets.flatMap { self.targets[$0]?.buildTarget.sources ?? [] }
await fileDependenciesUpdatedDebouncer.scheduleCall(Set(filesInPreparedTargets.map(DocumentURI.init)))
}

Expand Down Expand Up @@ -620,7 +649,7 @@ extension SwiftPMBuildSystem {
var dir = path.parentDirectory
while !dir.isRoot {
if let buildTarget = sourceDirToTarget[dir] {
return ConfiguredTarget(targetID: buildTarget.name, runDestinationID: "dummy")
return ConfiguredTarget(buildTarget)
}
dir = dir.parentDirectory
}
Expand Down
18 changes: 13 additions & 5 deletions Sources/SemanticIndex/SemanticIndexManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -209,11 +209,19 @@ public final actor SemanticIndexManager {
for uri in changedFiles {
indexStatus[uri] = nil
}
// Clear the preparation status so that we re-prepare them. If the target hasn't been affected by the file changes,
// we rely on a null build during preparation to fast re-preparation.
// Ideally, we would have more fine-grained dependency management here and only mark those targets out-of-date that
// might be affected by the changed files.
preparationStatus.removeAll()
// Note that configured targets are the right abstraction layer here (instead of a non-configured target) because a
// build system might have targets that include different source files. Hence a source file might be in target T
// configured for macOS but not in target T configured for iOS.
let targets = await changedFiles.asyncMap { await buildSystemManager.configuredTargets(for: $0) }.flatMap { $0 }
if let dependentTargets = await buildSystemManager.targets(dependingOn: targets) {
for dependentTarget in dependentTargets {
preparationStatus[dependentTarget] = nil
}
} else {
// We couldn't determine which targets depend on the modified targets. Be conservative and assume all of them do.
preparationStatus = [:]
Comment on lines +221 to +222
Copy link
Contributor

Choose a reason for hiding this comment

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

We could not remove targets in this case, not that it's really expected to happen anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

The only case where we’d be able to not remove the target itself is if there is only a single target. Say you have a project two targets A and B and you change one file in A and one file in B then you don’t know if they depend on each other and you need to assume that they do. So you need to invalidate both.

I don’t think it’s worth optimizing for the single module case. Really, the build system should provide more accurate information here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Single target is the main case though 😅 I agree the buildsystem should, but asking for preparation on every edit is really pretty bad.

Copy link
Member Author

Choose a reason for hiding this comment

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

In one of my follow-up PRs (not up yet), I’m re-implementing the entire up-to-date tracking of targets and that allows us to early-exit if we know that the target is up-to-date without any build system interaction.

}

await scheduleBackgroundIndex(files: changedFiles)
}

Expand Down
4 changes: 4 additions & 0 deletions Tests/SKCoreTests/BuildSystemManagerTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,10 @@ class ManualBuildSystem: BuildSystem {
return nil
}

public func targets(dependingOn targets: [ConfiguredTarget]) -> [ConfiguredTarget]? {
return nil
}

func registerForChangeNotifications(for uri: DocumentURI) async {
}

Expand Down
4 changes: 4 additions & 0 deletions Tests/SourceKitLSPTests/BuildSystemTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,10 @@ actor TestBuildSystem: BuildSystem {
return nil
}

public func targets(dependingOn targets: [ConfiguredTarget]) -> [ConfiguredTarget]? {
return nil
}

func registerForChangeNotifications(for uri: DocumentURI) async {
watchedFiles.insert(uri)
}
Expand Down