Skip to content

Commit d229081

Browse files
committed
Make add-dependency idempotent
If a manifest already contains the exact dependency is being added via a `add-dependency` call, don't add it twice. Instead, do nothing. This ensures we don't end up with an invalid manifest with two of the same dependencies in it. If a manifest contains a dependency with the supplied URL but a different version qualifier then throw an error explaining that the supplied dependency has already been added. Issue: #8519
1 parent e952244 commit d229081

File tree

3 files changed

+284
-184
lines changed

3 files changed

+284
-184
lines changed

Sources/PackageModelSyntax/AddPackageDependency.swift

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,13 @@ public enum AddPackageDependency {
4444
throw ManifestEditError.cannotFindPackage
4545
}
4646

47+
guard try checkExistingDependency(
48+
dependency,
49+
in: packageCall
50+
) else {
51+
return PackageEditResult(manifestEdits: [])
52+
}
53+
4754
let newPackageCall = try addPackageDependencyLocal(
4855
dependency, to: packageCall
4956
)
@@ -55,6 +62,52 @@ public enum AddPackageDependency {
5562
)
5663
}
5764

65+
/// Check that the given package dependency doesn't already exist in the manifest.
66+
/// If the exact same dependency already exists, `false` is returned to indicate
67+
/// that no edits are needed. If a different dependency with the same id or url
68+
/// with different arguments exists, an error is thrown.
69+
private static func checkExistingDependency(
70+
_ dependency: MappablePackageDependency.Kind,
71+
in packageCall: FunctionCallExprSyntax
72+
) throws -> Bool {
73+
let dependencySyntax = dependency.asSyntax()
74+
guard let dependenctFnSyntax = dependencySyntax.as(FunctionCallExprSyntax.self) else {
75+
throw ManifestEditError.cannotFindPackage // TODO: Fix error
76+
}
77+
78+
guard let id = dependenctFnSyntax.arguments.first(where: {
79+
$0.label?.text == "url" || $0.label?.text == "id" || $0.label?.text == "path"
80+
}) else {
81+
throw InternalError("Missing id or url argument in dependency syntax")
82+
}
83+
84+
if let existingDependencies = packageCall.findArgument(labeled: "dependencies") {
85+
// If we have an existing dependencies array, we need to check if
86+
if let expr = existingDependencies.expression.as(ArrayExprSyntax.self) {
87+
// Iterate through existing dependencies and look for an argument that matches
88+
// either the `id` or `url` argument of the new dependency.
89+
let existingArgument = expr.elements.first { elem in
90+
if let funcExpr = elem.expression.as(FunctionCallExprSyntax.self) {
91+
return funcExpr.arguments.contains {
92+
$0.trimmedDescription == id.trimmedDescription
93+
}
94+
}
95+
return false
96+
}
97+
98+
if let existingArgument {
99+
let normalizedExistingArgument = existingArgument.detached.with(\.trailingComma, nil)
100+
// This exact dependency already exists, return false to indicate we should do nothing.
101+
if normalizedExistingArgument.trimmedDescription == dependencySyntax.trimmedDescription {
102+
return false
103+
}
104+
throw ManifestEditError.existingDependency(dependencyName: dependency.identifier)
105+
}
106+
}
107+
}
108+
return true
109+
}
110+
58111
/// Implementation of adding a package dependency to an existing call.
59112
static func addPackageDependencyLocal(
60113
_ dependency: MappablePackageDependency.Kind,
@@ -67,3 +120,16 @@ public enum AddPackageDependency {
67120
)
68121
}
69122
}
123+
124+
fileprivate extension MappablePackageDependency.Kind {
125+
var identifier: String {
126+
switch self {
127+
case .sourceControl(let name, let path, _):
128+
return name ?? path
129+
case .fileSystem(let name, let location):
130+
return name ?? location
131+
case .registry(let id, _):
132+
return id
133+
}
134+
}
135+
}

Sources/PackageModelSyntax/ManifestEditError.swift

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ package enum ManifestEditError: Error {
2323
case cannotFindArrayLiteralArgument(argumentName: String, node: Syntax)
2424
case oldManifest(ToolsVersion, expected: ToolsVersion)
2525
case cannotAddSettingsToPluginTarget
26+
case existingDependency(dependencyName: String)
2627
}
2728

2829
extension ToolsVersion {
@@ -46,6 +47,8 @@ extension ManifestEditError: CustomStringConvertible {
4647
"package manifest version \(version) is too old: please update to manifest version \(expectedVersion) or newer"
4748
case .cannotAddSettingsToPluginTarget:
4849
"plugin targets do not support settings"
50+
case .existingDependency(let name):
51+
"unable to add dependency '\(name)' because it already exists in the list of dependencies"
4952
}
5053
}
5154
}

0 commit comments

Comments
 (0)