Skip to content

[6.0] Prefix module name to test #1546

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 1 commit into from
Jul 3, 2024
Merged

[6.0] Prefix module name to test #1546

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
32 changes: 32 additions & 0 deletions Sources/SKCore/BuildSystemManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import BuildServerProtocol
import Dispatch
import LSPLogging
import LanguageServerProtocol
import SwiftExtensions

import struct TSCBasic.AbsolutePath

Expand Down Expand Up @@ -154,6 +155,37 @@ extension BuildSystemManager {
.first
}

/// Returns the target's module name as parsed from the `ConfiguredTarget`'s compiler arguments.
public func moduleName(for document: DocumentURI, in target: ConfiguredTarget) async -> String? {
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like something that should be provided by the build system, rather than found from the settings after the fact. We could have a default that does this though.

It's also the last argument that wins, so both the .first* below should be .last* instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its my understanding that the build settings already give us a standardized way of getting the compilerArguments across all the difference build systems, so pushing the implementation down into each individual BuildSystem would be largely duplicating the implementation of BuildSystem.buildSettings(for:in:language:). Let me know if this isn't what you meant.

Great catch on argument precedence, I'll put up a patch that fixes it on main and then roll it in to this cherry-pick.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've created #1549 to fix argument precedence

Copy link
Contributor

Choose a reason for hiding this comment

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

Its my understanding that the build settings already give us a standardized way of getting the compilerArguments across all the difference build systems

This is true, but doesn't necessarily mean we should go searching through build settings for the information we want (I would consider that a fallback) - the build system may already have the information needed. And like I said, we could always have a default implementation for that method that does as you are now.

guard let language = await self.defaultLanguage(for: document),
let buildSettings = await buildSettings(for: document, in: target, language: language)
else {
return nil
}

switch language {
case .swift:
// Module name is specified in the form -module-name MyLibrary
guard let moduleNameFlagIndex = buildSettings.compilerArguments.firstIndex(of: "-module-name") else {
return nil
}
return buildSettings.compilerArguments[safe: moduleNameFlagIndex + 1]
case .objective_c:
// Specified in the form -fmodule-name=MyLibrary
guard
let moduleNameArgument = buildSettings.compilerArguments.first(where: {
$0.starts(with: "-fmodule-name=")
}),
let moduleName = moduleNameArgument.split(separator: "=").last
else {
return nil
}
return String(moduleName)
default:
return nil
}
}

/// Returns the build settings for `document` from `buildSystem`.
///
/// Implementation detail of `buildSettings(for:language:)`.
Expand Down
27 changes: 26 additions & 1 deletion Sources/SourceKitLSP/TestDiscovery.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ extension SourceKitLSPServer {

func workspaceTests(_ req: WorkspaceTestsRequest) async throws -> [TestItem] {
return await self.workspaces
.concurrentMap { await self.tests(in: $0) }
.concurrentMap { await self.tests(in: $0).prefixTestsWithModuleName(workspace: $0) }
.flatMap { $0 }
.sorted { $0.testItem.location < $1.testItem.location }
.mergingTestsInExtensions()
Expand All @@ -272,6 +272,7 @@ extension SourceKitLSPServer {
languageService: LanguageService
) async throws -> [TestItem] {
return try await documentTestsWithoutMergingExtensions(req, workspace: workspace, languageService: languageService)
.prefixTestsWithModuleName(workspace: workspace)
.mergingTestsInExtensions()
}

Expand Down Expand Up @@ -476,6 +477,30 @@ fileprivate extension Array<AnnotatedTestItem> {
}
return result
}

func prefixTestsWithModuleName(workspace: Workspace) async -> Self {
return await self.asyncMap({
return AnnotatedTestItem(
testItem: await $0.testItem.prefixIDWithModuleName(workspace: workspace),
isExtension: $0.isExtension
)
})
}
}

extension TestItem {
fileprivate func prefixIDWithModuleName(workspace: Workspace) async -> TestItem {
guard let configuredTarget = await workspace.buildSystemManager.canonicalConfiguredTarget(for: self.location.uri),
let moduleName = await workspace.buildSystemManager.moduleName(for: self.location.uri, in: configuredTarget)
else {
return self
}

var newTest = self
newTest.id = "\(moduleName).\(newTest.id)"
newTest.children = await newTest.children.asyncMap({ await $0.prefixIDWithModuleName(workspace: workspace) })
return newTest
}
}

extension SwiftLanguageService {
Expand Down
19 changes: 19 additions & 0 deletions Sources/SwiftExtensions/Array+Safe.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
//===----------------------------------------------------------------------===//
//
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2014 - 2018 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
//
//===----------------------------------------------------------------------===//

extension Array {
/// Returns the element at the specified index if it is within the Array's
/// bounds, otherwise `nil`.
public subscript(safe index: Index) -> Element? {
return index >= 0 && index < count ? self[index] : nil
}
}
1 change: 1 addition & 0 deletions Sources/SwiftExtensions/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@

add_library(SwiftExtensions STATIC
Array+Safe.swift
AsyncQueue.swift
AsyncUtils.swift
Collection+Only.swift
Expand Down
Loading