Skip to content

Commit 174cdc2

Browse files
authored
Make add-dependency idempotent (#8534)
### Motivation: If the same dependency is added twice with different version specifiers the manifest is no longer valid. If the same dependency is added twice with the same version specifier it remains valid, but less maintainable. ### Modifications: If a manifest already contains the exact dependency and version that is being added via a `add-dependency` call, don't add it twice. Instead, do nothing. 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. ### Result: A manifest that continues to parse correctly. Issue: #8519
1 parent 244f99c commit 174cdc2

File tree

3 files changed

+351
-183
lines changed

3 files changed

+351
-183
lines changed

Sources/PackageModelSyntax/AddPackageDependency.swift

Lines changed: 64 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 !dependencyAlreadyAdded(
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,50 @@ public enum AddPackageDependency {
5562
)
5663
}
5764

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

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)