Skip to content

Fix background indexing behavior if a source file is included in two targets via a symlink #1858

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
Dec 10, 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
8 changes: 4 additions & 4 deletions Sources/BuildSystemIntegration/BuildSystemManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1033,11 +1033,11 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
return response.items
}

/// Returns all source files in the project that can be built.
/// Returns all source files in the project.
///
/// - SeeAlso: Comment in `sourceFilesAndDirectories` for a definition of what `buildable` means.
package func buildableSourceFiles() async throws -> [DocumentURI: SourceFileInfo] {
return try await sourceFilesAndDirectories(includeNonBuildableFiles: false).files
package func sourceFiles(includeNonBuildableFiles: Bool) async throws -> [DocumentURI: SourceFileInfo] {
return try await sourceFilesAndDirectories(includeNonBuildableFiles: includeNonBuildableFiles).files
}

/// Get all files and directories that are known to the build system, ie. that are returned by a `buildTarget/sources`
Expand Down Expand Up @@ -1093,7 +1093,7 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
}

package func testFiles() async throws -> [DocumentURI] {
return try await buildableSourceFiles().compactMap { (uri, info) -> DocumentURI? in
return try await sourceFiles(includeNonBuildableFiles: false).compactMap { (uri, info) -> DocumentURI? in
guard info.isPartOfRootProject, info.mayContainTests else {
return nil
}
Expand Down
2 changes: 1 addition & 1 deletion Sources/BuildSystemIntegration/SwiftPMBuildSystem.swift
Original file line number Diff line number Diff line change
Expand Up @@ -587,7 +587,7 @@ package actor SwiftPMBuildSystem: BuiltInBuildSystem {
SourceItem(
uri: DocumentURI($0),
kind: $0.isDirectory ? .directory : .file,
generated: false,
generated: false
)
}
result.append(SourcesItem(target: target, sources: sources))
Expand Down
4 changes: 3 additions & 1 deletion Sources/SKTestSupport/MultiFileTestProject.swift
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,9 @@ package class MultiFileTestProject {
/// File contents can also contain `$TEST_DIR`, which gets replaced by the temporary directory.
package init(
files: [RelativeFileLocation: String],
workspaces: (URL) async throws -> [WorkspaceFolder] = { [WorkspaceFolder(uri: DocumentURI($0))] },
workspaces: (_ scratchDirectory: URL) async throws -> [WorkspaceFolder] = {
[WorkspaceFolder(uri: DocumentURI($0))]
},
initializationOptions: LSPAny? = nil,
capabilities: ClientCapabilities = ClientCapabilities(),
options: SourceKitLSPOptions = .testDefault(),
Expand Down
4 changes: 3 additions & 1 deletion Sources/SKTestSupport/SwiftPMTestProject.swift
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,9 @@ package class SwiftPMTestProject: MultiFileTestProject {
package init(
files: [RelativeFileLocation: String],
manifest: String = SwiftPMTestProject.defaultPackageManifest,
workspaces: (URL) async throws -> [WorkspaceFolder] = { [WorkspaceFolder(uri: DocumentURI($0))] },
workspaces: (_ scratchDirectory: URL) async throws -> [WorkspaceFolder] = {
[WorkspaceFolder(uri: DocumentURI($0))]
},
initializationOptions: LSPAny? = nil,
capabilities: ClientCapabilities = ClientCapabilities(),
options: SourceKitLSPOptions = .testDefault(),
Expand Down
5 changes: 3 additions & 2 deletions Sources/SemanticIndex/SemanticIndexManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,8 @@ package final actor SemanticIndexManager {
filesToIndex
} else {
await orLog("Getting files to index") {
try await self.buildSystemManager.buildableSourceFiles().keys.sorted { $0.stringValue < $1.stringValue }
try await self.buildSystemManager.sourceFiles(includeNonBuildableFiles: false).keys
.sorted { $0.stringValue < $1.stringValue }
} ?? []
}
if !indexFilesWithUpToDateUnit {
Expand Down Expand Up @@ -408,7 +409,7 @@ package final actor SemanticIndexManager {
toCover files: some Collection<DocumentURI> & Sendable
) async -> [FileToIndex] {
let sourceFiles = await orLog("Getting source files in project") {
Set(try await buildSystemManager.buildableSourceFiles().keys)
Set(try await buildSystemManager.sourceFiles(includeNonBuildableFiles: false).keys)
}
guard let sourceFiles else {
return []
Expand Down
61 changes: 61 additions & 0 deletions Sources/SourceKitLSP/Workspace.swift
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,51 @@ fileprivate func firstNonNil<T>(
return try await defaultValue()
}

/// Actor that caches realpaths for `sourceFilesWithSameRealpath`.
fileprivate actor SourceFilesWithSameRealpathInferrer {
private let buildSystemManager: BuildSystemManager
private var realpathCache: [DocumentURI: DocumentURI] = [:]

init(buildSystemManager: BuildSystemManager) {
self.buildSystemManager = buildSystemManager
}

private func realpath(of uri: DocumentURI) -> DocumentURI {
if let cached = realpathCache[uri] {
return cached
}
let value = uri.symlinkTarget ?? uri
realpathCache[uri] = value
return value
}

/// Returns the URIs of all source files in the project that have the same realpath as a document in `documents` but
/// are not in `documents`.
///
/// This is useful in the following scenario: A project has target A containing A.swift an target B containing B.swift
/// B.swift is a symlink to A.swift. When A.swift is modified, both the dependencies of A and B need to be marked as
/// having an out-of-date preparation status, not just A.
package func sourceFilesWithSameRealpath(as documents: [DocumentURI]) async -> [DocumentURI] {
let realPaths = Set(documents.map { realpath(of: $0) })
return await orLog("Determining source files with same realpath") {
var result: [DocumentURI] = []
let filesAndDirectories = try await buildSystemManager.sourceFiles(includeNonBuildableFiles: true)
for file in filesAndDirectories.keys {
if realPaths.contains(realpath(of: file)) && !documents.contains(file) {
result.append(file)
}
}
return result
} ?? []
}

func filesDidChange(_ events: [FileEvent]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you check what event ends up being sent for changing a symlink? Is it a delete + add or just a change?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not. But it shouldn’t matter because we clear the realpath cache on any file event.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I was wondering if we could limit it to eg. just delete/add rather than on change

Copy link
Member Author

Choose a reason for hiding this comment

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

Even if I checked how it behaves on macOS, I don’t want to make inferences about how it behaves on various Linux distributions, so I think erroring on the side of caution is better here. Plus, recomputing a single realpath when a file is changed shouldn’t be that bad.

for event in events {
realpathCache[event.uri] = nil
}
}
}

/// Represents the configuration and state of a project or combination of projects being worked on
/// together.
///
Expand All @@ -86,6 +131,8 @@ package final class Workspace: Sendable, BuildSystemManagerDelegate {
/// The build system manager to use for documents in this workspace.
package let buildSystemManager: BuildSystemManager

private let sourceFilesWithSameRealpathInferrer: SourceFilesWithSameRealpathInferrer

let options: SourceKitLSPOptions

/// The source code index, if available.
Expand Down Expand Up @@ -126,6 +173,9 @@ package final class Workspace: Sendable, BuildSystemManagerDelegate {
self.options = options
self._uncheckedIndex = ThreadSafeBox(initialValue: uncheckedIndex)
self.buildSystemManager = buildSystemManager
self.sourceFilesWithSameRealpathInferrer = SourceFilesWithSameRealpathInferrer(
buildSystemManager: buildSystemManager
)
if options.backgroundIndexingOrDefault, let uncheckedIndex,
await buildSystemManager.initializationData?.prepareProvider ?? false
{
Expand Down Expand Up @@ -316,6 +366,17 @@ package final class Workspace: Sendable, BuildSystemManagerDelegate {
}

package func filesDidChange(_ events: [FileEvent]) async {
// First clear any cached realpaths in `sourceFilesWithSameRealpathInferrer`.
await sourceFilesWithSameRealpathInferrer.filesDidChange(events)

// Now infer any edits for source files that share the same realpath as one of the modified files.
var events = events
events +=
await sourceFilesWithSameRealpathInferrer
.sourceFilesWithSameRealpath(as: events.filter { $0.type == .changed }.map(\.uri))
.map { FileEvent(uri: $0, type: .changed) }

// Notify all clients about the reported and inferred edits.
await buildSystemManager.filesDidChange(events)
await syntacticTestIndex.filesDidChange(events)
await semanticIndexManager?.filesDidChange(events)
Expand Down
57 changes: 57 additions & 0 deletions Tests/SourceKitLSPTests/BackgroundIndexingTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1620,6 +1620,63 @@ final class BackgroundIndexingTests: XCTestCase {
return completionAfterEdit.items.map(\.label) == ["self", "test()"]
}
}

func testSymlinkedTargetReferringToSameSourceFile() async throws {
let project = try await SwiftPMTestProject(
files: [
"LibA/LibA.swift": """
public let myVar: String
""",
"Client/Client.swift": """
import LibASymlink

func test() {
print(1️⃣myVar)
}
""",
],
manifest: """
let package = Package(
name: "MyLibrary",
targets: [
.target(name: "LibA"),
.target(name: "LibASymlink"),
.target(name: "Client", dependencies: ["LibASymlink"]),
]
)
""",
workspaces: { scratchDirectory in
let sources = scratchDirectory.appendingPathComponent("Sources")
try FileManager.default.createSymbolicLink(
at: sources.appendingPathComponent("LibASymlink"),
withDestinationURL: sources.appendingPathComponent("LibA")
)
return [WorkspaceFolder(uri: DocumentURI(scratchDirectory))]
},
enableBackgroundIndexing: true
)

let (uri, positions) = try project.openDocument("Client.swift")
let preEditHover = try await project.testClient.send(
HoverRequest(textDocument: TextDocumentIdentifier(uri), position: positions["1️⃣"])
)
let preEditHoverContent = try XCTUnwrap(preEditHover?.contents.markupContent?.value)
XCTAssert(
preEditHoverContent.contains("String"),
"Pre edit hover content '\(preEditHoverContent)' does not contain 'String'"
)

let libAUri = try project.uri(for: "LibA.swift")
try "public let myVar: Int".write(to: try XCTUnwrap(libAUri.fileURL), atomically: true, encoding: .utf8)
project.testClient.send(DidChangeWatchedFilesNotification(changes: [FileEvent(uri: libAUri, type: .changed)]))

try await repeatUntilExpectedResult {
let postEditHover = try await project.testClient.send(
HoverRequest(textDocument: TextDocumentIdentifier(uri), position: positions["1️⃣"])
)
return try XCTUnwrap(postEditHover?.contents.markupContent?.value).contains("Int")
}
}
}

extension HoverResponseContents {
Expand Down