Skip to content

Commit 2073e2f

Browse files
hartbithartbit
authored
Merge pull request #2457 from hartbit/manifest-checks
Move duplicate target check to load time
2 parents 2ce530e + c1ea973 commit 2073e2f

File tree

10 files changed

+132
-111
lines changed

10 files changed

+132
-111
lines changed

‎Sources/PackageLoading/ManifestLoader.swift

Lines changed: 58 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,17 @@ public enum ManifestParseError: Swift.Error {
2222
/// The manifest was successfully loaded by swift interpreter but there were runtime issues.
2323
case runtimeManifestErrors([String])
2424

25+
/// The manifest contains dependencies with the same URL.
2526
case duplicateDependencyURLs([[PackageDependencyDescription]])
2627

27-
case targetDependencyUnknownPackage(targetName: String, packageName: String)
28-
28+
/// The manifest contains dependencies with the same name.
2929
case duplicateDependencyNames([[PackageDependencyDescription]])
30+
31+
/// The manifest contains targets with the same name.
32+
case duplicateTargetNames([String])
33+
34+
/// The manifest contains target product dependencies that reference an unknown package.
35+
case unknownTargetDependencyPackage(targetName: String, packageName: String)
3036
}
3137

3238
/// Resources required for manifest loading.
@@ -241,6 +247,12 @@ public final class ManifestLoader: ManifestLoaderProtocol {
241247
throw ManifestParseError.runtimeManifestErrors(manifestBuilder.errors)
242248
}
243249

250+
// Ensure no dupicate target definitions are found.
251+
let duplicateTargetNames: [String] = manifestBuilder.targets.map({ $0.name }).spm_findDuplicates()
252+
if !duplicateTargetNames.isEmpty {
253+
throw ManifestParseError.duplicateTargetNames(duplicateTargetNames)
254+
}
255+
244256
let manifest = Manifest(
245257
name: manifestBuilder.name,
246258
platforms: manifestBuilder.platforms,
@@ -265,6 +277,16 @@ public final class ManifestLoader: ManifestLoaderProtocol {
265277

266278
/// Validate the provided manifest.
267279
private func validate(_ manifest: Manifest, toolsVersion: ToolsVersion) throws {
280+
try validateDependencyURLs(manifest)
281+
282+
// Checks reserved for tools version 5.2 features
283+
if toolsVersion >= .v5_2 {
284+
try validateDependencyNames(manifest)
285+
try validateTargetDependencyReferences(manifest)
286+
}
287+
}
288+
289+
private func validateDependencyURLs(_ manifest: Manifest) throws {
268290
let duplicateDependenciesByURL = manifest.dependencies
269291
.lazy
270292
.map({ KeyedPair($0, key: PackageReference.computeIdentity(packageURL: $0.url)) })
@@ -273,43 +295,43 @@ public final class ManifestLoader: ManifestLoaderProtocol {
273295
let duplicates = duplicateDependenciesByURL.map({ $0.map({ $0.item }) })
274296
throw ManifestParseError.duplicateDependencyURLs(duplicates)
275297
}
298+
}
276299

277-
// Checks reserved for tools version 5.2 features
278-
if toolsVersion >= .v5_2 {
279-
// Make sure there are no dependencies with the same name.
280-
let duplicateDependenciesByName = manifest.dependencies
281-
.lazy
282-
.map({ KeyedPair($0, key: $0.name!) })
283-
.spm_findDuplicateElements()
284-
if !duplicateDependenciesByName.isEmpty {
285-
let duplicates = duplicateDependenciesByName.map({ $0.map({ $0.item }) })
286-
throw ManifestParseError.duplicateDependencyNames(duplicates)
287-
}
300+
/// Validates that all dependencies have different names.
301+
private func validateDependencyNames(_ manifest: Manifest) throws {
302+
let duplicateDependenciesByName = manifest.dependencies
303+
.lazy
304+
.map({ KeyedPair($0, key: $0.name!) })
305+
.spm_findDuplicateElements()
306+
if !duplicateDependenciesByName.isEmpty {
307+
let duplicates = duplicateDependenciesByName.map({ $0.map({ $0.item }) })
308+
throw ManifestParseError.duplicateDependencyNames(duplicates)
309+
}
310+
}
288311

289-
// Make sure all target package dependencies are valid.
290-
let targetNames = Set(manifest.targets.map({ $0.name }))
291-
for target in manifest.targets {
292-
for targetDependency in target.dependencies {
293-
// If this is a target dependency (or byName that references a target), we don't need to check.
294-
if case .target = targetDependency { continue }
295-
if case .byName(let name) = targetDependency, targetNames.contains(name) { continue }
296-
297-
// If we can't find the package dependency it references, the manifest is invalid.
298-
if manifest.packageDependency(referencedBy: targetDependency) == nil {
299-
let packageName: String
300-
switch targetDependency {
301-
case .product(_, package: let name?),
302-
.byName(let name):
303-
packageName = name
304-
default:
305-
fatalError("Invalid case: this shouldn't be a target, or a product with no name")
306-
}
307-
308-
throw ManifestParseError.targetDependencyUnknownPackage(
309-
targetName: target.name,
310-
packageName: packageName
311-
)
312+
/// Validates that product target dependencies reference an existing package.
313+
private func validateTargetDependencyReferences(_ manifest: Manifest) throws {
314+
for target in manifest.targets {
315+
for targetDependency in target.dependencies {
316+
// If this is a target dependency (or byName that references a target), we don't need to check.
317+
if case .target = targetDependency { continue }
318+
if case .byName(let name) = targetDependency, manifest.targetMap.keys.contains(name) { continue }
319+
320+
// If we can't find the package dependency it references, the manifest is invalid.
321+
if manifest.packageDependency(referencedBy: targetDependency) == nil {
322+
let packageName: String
323+
switch targetDependency {
324+
case .product(_, package: let name?),
325+
.byName(let name):
326+
packageName = name
327+
default:
328+
fatalError("Invalid case: this shouldn't be a target, or a product with no name")
312329
}
330+
331+
throw ManifestParseError.unknownTargetDependencyPackage(
332+
targetName: target.name,
333+
packageName: packageName
334+
)
313335
}
314336
}
315337
}

‎Sources/PackageLoading/PackageBuilder.swift

Lines changed: 5 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -368,23 +368,14 @@ public final class PackageBuilder {
368368
/// Private function that creates and returns a list of targets defined by a package.
369369
private func constructTargets() throws -> [Target] {
370370

371-
// Ensure no dupicate target definitions are found.
372-
let duplicateTargetNames: [String] = manifest.targets.map({ $0.name
373-
}).spm_findDuplicates()
374-
375-
if !duplicateTargetNames.isEmpty {
376-
throw Target.Error.duplicateTargets(duplicateTargetNames)
377-
}
378-
379371
// Check for a modulemap file, which indicates a system target.
380372
let moduleMapPath = packagePath.appending(component: moduleMapFilename)
381373
if fileSystem.isFile(moduleMapPath) {
382374

383375
// Warn about any declared targets.
384-
let targets = manifest.targets
385-
if !targets.isEmpty {
376+
if !manifest.targets.isEmpty {
386377
diagnostics.emit(
387-
.systemPackageDeclaresTargets(targets: targets.map{ $0.name }),
378+
.systemPackageDeclaresTargets(targets: Array(manifest.targets.map({ $0.name }))),
388379
location: diagnosticLocation()
389380
)
390381
}
@@ -518,12 +509,10 @@ public final class PackageBuilder {
518509
throw ModuleError.moduleNotFound(missingModule, potentialModules.first(where: { $0.name == missingModule })?.type ?? .regular)
519510
}
520511

521-
let targetItems = manifest.targets.map({ ($0.name, $0 as TargetDescription) })
522-
let targetMap = Dictionary(targetItems, uniquingKeysWith: { $1 })
523512
let potentialModuleMap = Dictionary(potentialModules.map({ ($0.name, $0) }), uniquingKeysWith: { $1 })
524513
let successors: (PotentialModule) -> [PotentialModule] = {
525514
// No reference of this target in manifest, i.e. it has no dependencies.
526-
guard let target = targetMap[$0.name] else { return [] }
515+
guard let target = self.manifest.targetMap[$0.name] else { return [] }
527516
return target.dependencies.compactMap({
528517
switch $0 {
529518
case .target(let name):
@@ -556,7 +545,7 @@ public final class PackageBuilder {
556545
// Validate the target name. This function will throw an error if it detects a problem.
557546
try validateModuleName(potentialModule.path, potentialModule.name, isTest: potentialModule.isTest)
558547
// Get the intra-package dependencies of this target.
559-
let deps: [Target] = targetMap[potentialModule.name].map({
548+
let deps: [Target] = manifest.targetMap[potentialModule.name].map({
560549
$0.dependencies.compactMap({
561550
switch $0 {
562551
case .target(let name):
@@ -575,7 +564,7 @@ public final class PackageBuilder {
575564
}) ?? []
576565

577566
// Get the target from the manifest.
578-
let manifestTarget = targetMap[potentialModule.name]
567+
let manifestTarget = manifest.targetMap[potentialModule.name]
579568

580569
// Figure out the product dependencies.
581570
let productDeps: [(String, String?)]

‎Sources/PackageModel/Manifest.swift

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,9 @@ public final class Manifest: ObjectIdentifierProtocol, CustomStringConvertible,
5151
/// The targets declared in the manifest.
5252
public let targets: [TargetDescription]
5353

54+
/// The targets declared in the manifest, keyed by their name.
55+
public let targetMap: [String: TargetDescription]
56+
5457
/// The products declared in the manifest.
5558
public let products: [ProductDescription]
5659

@@ -99,6 +102,7 @@ public final class Manifest: ObjectIdentifierProtocol, CustomStringConvertible,
99102
self.dependencies = dependencies
100103
self.products = products
101104
self.targets = targets
105+
self.targetMap = Dictionary(uniqueKeysWithValues: targets.lazy.map({ ($0.name, $0) }))
102106
}
103107

104108
public var description: String {

‎Sources/Workspace/Diagnostics.swift

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,14 @@ public struct ManifestDuplicateDependencyNamesDiagnostic: DiagnosticData {
7979
}
8080
}
8181

82+
public struct ManifestDuplicateTargetNamesDiagnostic: DiagnosticData {
83+
public let duplicates: [String]
84+
85+
public var description: String {
86+
"manifest parse error: duplicate target names: \(duplicates.joined(separator: ", "))\n"
87+
}
88+
}
89+
8290
extension ManifestParseError: DiagnosticDataConvertible {
8391
public var diagnosticData: DiagnosticData {
8492
switch self {
@@ -88,7 +96,9 @@ extension ManifestParseError: DiagnosticDataConvertible {
8896
return ManifestParseDiagnostic(errors, diagnosticFile: nil)
8997
case .duplicateDependencyURLs(let duplicates):
9098
return ManifestDuplicateDependencyURLsDiagnostic(duplicates: duplicates)
91-
case .targetDependencyUnknownPackage(let targetName, let packageName):
99+
case .duplicateTargetNames(let targetNames):
100+
return ManifestDuplicateTargetNamesDiagnostic(duplicates: targetNames)
101+
case .unknownTargetDependencyPackage(let targetName, let packageName):
92102
return ManifestTargetDependencyUnknownPackageDiagnostic(targetName: targetName, packageName: packageName)
93103
case .duplicateDependencyNames(let duplicates):
94104
return ManifestDuplicateDependencyNamesDiagnostic(duplicates: duplicates)

‎Tests/PackageLoadingTests/PD4LoadingTests.swift

Lines changed: 41 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -60,9 +60,7 @@ class PackageDescription4LoadingTests: PackageDescriptionLoadingTests {
6060

6161
loadManifest(stream.bytes) { manifest in
6262
XCTAssertEqual(manifest.name, "Trivial")
63-
let targets = Dictionary(uniqueKeysWithValues:
64-
manifest.targets.map({ ($0.name, $0 as TargetDescription ) }))
65-
let foo = targets["foo"]!
63+
let foo = manifest.targetMap["foo"]!
6664
XCTAssertEqual(foo.name, "foo")
6765
XCTAssertFalse(foo.isTest)
6866

@@ -75,7 +73,7 @@ class PackageDescription4LoadingTests: PackageDescriptionLoadingTests {
7573
]
7674
XCTAssertEqual(foo.dependencies, expectedDependencies)
7775

78-
let bar = targets["bar"]!
76+
let bar = manifest.targetMap["bar"]!
7977
XCTAssertEqual(bar.name, "bar")
8078
XCTAssertTrue(bar.isTest)
8179
XCTAssertEqual(bar.dependencies, ["foo"])
@@ -217,13 +215,10 @@ class PackageDescription4LoadingTests: PackageDescriptionLoadingTests {
217215
)
218216
"""
219217
loadManifest(stream.bytes) { manifest in
220-
let targets = Dictionary(uniqueKeysWithValues:
221-
manifest.targets.map({ ($0.name, $0 as TargetDescription ) }))
222-
223-
let foo = targets["Foo"]!
218+
let foo = manifest.targetMap["Foo"]!
224219
XCTAssertEqual(foo.publicHeadersPath, "inc")
225220

226-
let bar = targets["Bar"]!
221+
let bar = manifest.targetMap["Bar"]!
227222
XCTAssertEqual(bar.publicHeadersPath, nil)
228223
}
229224
}
@@ -247,16 +242,13 @@ class PackageDescription4LoadingTests: PackageDescriptionLoadingTests {
247242
)
248243
"""
249244
loadManifest(stream.bytes) { manifest in
250-
let targets = Dictionary(uniqueKeysWithValues:
251-
manifest.targets.map({ ($0.name, $0 as TargetDescription ) }))
252-
253-
let foo = targets["Foo"]!
245+
let foo = manifest.targetMap["Foo"]!
254246
XCTAssertEqual(foo.publicHeadersPath, "inc")
255247
XCTAssertEqual(foo.path, "foo/z")
256248
XCTAssertEqual(foo.exclude, ["bar"])
257249
XCTAssertEqual(foo.sources ?? [], ["bar.swift"])
258250

259-
let bar = targets["Bar"]!
251+
let bar = manifest.targetMap["Bar"]!
260252
XCTAssertEqual(bar.publicHeadersPath, nil)
261253
XCTAssertEqual(bar.path, nil)
262254
XCTAssertEqual(bar.exclude, [])
@@ -344,4 +336,39 @@ class PackageDescription4LoadingTests: PackageDescriptionLoadingTests {
344336
result.check(diagnostic: .contains("initialization of immutable value 'a' was never used"), behavior: .warning)
345337
}
346338
}
339+
340+
func testDuplicateTargets() throws {
341+
let fs = InMemoryFileSystem()
342+
let manifestPath = AbsolutePath.root.appending(component: Manifest.filename)
343+
let stream = BufferedOutputByteStream()
344+
345+
stream <<< """
346+
import PackageDescription
347+
348+
let package = Package(
349+
name: "Foo",
350+
targets: [
351+
.target(name: "A"),
352+
.target(name: "B"),
353+
.target(name: "A"),
354+
.target(name: "B"),
355+
]
356+
)
357+
"""
358+
359+
try fs.writeFileContents(manifestPath, bytes: stream.bytes)
360+
361+
do {
362+
let diagnostics = DiagnosticsEngine()
363+
_ = try manifestLoader.load(
364+
package: .root, baseURL: "/Foo",
365+
toolsVersion: .v4, fileSystem: fs,
366+
diagnostics: diagnostics
367+
)
368+
} catch ManifestParseError.duplicateTargetNames(let targetNames) {
369+
XCTAssertEqual(Set(targetNames), ["A", "B"])
370+
} catch {
371+
XCTFail(error.localizedDescription)
372+
}
373+
}
347374
}

‎Tests/PackageLoadingTests/PD4_2LoadingTests.swift

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -49,14 +49,12 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
4949
XCTAssertEqual(manifest.name, "Trivial")
5050

5151
// Check targets.
52-
let targets = Dictionary(uniqueKeysWithValues:
53-
manifest.targets.map({ ($0.name, $0 as TargetDescription ) }))
54-
let foo = targets["foo"]!
52+
let foo = manifest.targetMap["foo"]!
5553
XCTAssertEqual(foo.name, "foo")
5654
XCTAssertFalse(foo.isTest)
5755
XCTAssertEqual(foo.dependencies, ["dep1", .product(name: "product"), .target(name: "target")])
5856

59-
let bar = targets["bar"]!
57+
let bar = manifest.targetMap["bar"]!
6058
XCTAssertEqual(bar.name, "bar")
6159
XCTAssertTrue(bar.isTest)
6260
XCTAssertEqual(bar.dependencies, ["foo"])
@@ -304,15 +302,13 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
304302
)
305303
"""
306304
loadManifest(stream.bytes) { manifest in
307-
let targets = Dictionary(uniqueKeysWithValues:
308-
manifest.targets.map({ ($0.name, $0 as TargetDescription ) }))
309-
let foo = targets["foo"]!
305+
let foo = manifest.targetMap["foo"]!
310306
XCTAssertEqual(foo.name, "foo")
311307
XCTAssertFalse(foo.isTest)
312308
XCTAssertEqual(foo.type, .regular)
313309
XCTAssertEqual(foo.dependencies, ["bar"])
314310

315-
let bar = targets["bar"]!
311+
let bar = manifest.targetMap["bar"]!
316312
XCTAssertEqual(bar.name, "bar")
317313
XCTAssertEqual(bar.type, .system)
318314
XCTAssertEqual(bar.pkgConfig, "libbar")

‎Tests/PackageLoadingTests/PD5LoadingTests.swift

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -49,14 +49,12 @@ class PackageDescription5LoadingTests: PackageDescriptionLoadingTests {
4949
XCTAssertEqual(manifest.name, "Trivial")
5050

5151
// Check targets.
52-
let targets = Dictionary(uniqueKeysWithValues:
53-
manifest.targets.map({ ($0.name, $0 as TargetDescription ) }))
54-
let foo = targets["foo"]!
52+
let foo = manifest.targetMap["foo"]!
5553
XCTAssertEqual(foo.name, "foo")
5654
XCTAssertFalse(foo.isTest)
5755
XCTAssertEqual(foo.dependencies, ["dep1", .product(name: "product"), .target(name: "target")])
5856

59-
let bar = targets["bar"]!
57+
let bar = manifest.targetMap["bar"]!
6058
XCTAssertEqual(bar.name, "bar")
6159
XCTAssertTrue(bar.isTest)
6260
XCTAssertEqual(bar.dependencies, ["foo"])
@@ -453,14 +451,12 @@ class PackageDescription5LoadingTests: PackageDescriptionLoadingTests {
453451
XCTAssertEqual(manifest.name, "Foo")
454452

455453
// Check targets.
456-
let targets = Dictionary(uniqueKeysWithValues:
457-
manifest.targets.map({ ($0.name, $0 as TargetDescription ) }))
458-
let foo = targets["foo"]!
454+
let foo = manifest.targetMap["foo"]!
459455
XCTAssertEqual(foo.name, "foo")
460456
XCTAssertFalse(foo.isTest)
461457
XCTAssertEqual(foo.dependencies, [])
462458

463-
let settings = manifest.targets[0].settings
459+
let settings = foo.settings
464460
XCTAssertEqual(settings[0], .init(tool: .c, name: .define, value: ["LLVM_ON_WIN32"], condition: .init(platformNames: ["windows"])))
465461
}
466462
}

0 commit comments

Comments
 (0)