Swift package manifest refactoring actions #2904

DougGregor · 2024-12-01T06:10:49Z

Port the package manifest editing refactoring actions over to SwiftRefactor. These refactoring operations come from the swift-package-manager repository, and are being moved here for several reasons:

They are shared by SwiftPM and SourceKit-LSP already, and are likely to also get used in the compiler, so swift-syntax is a good place for them to be to avoid code duplication.
The SwiftRefactor module is where we are collecting various refactoring.
Putting them here completely separates them from the semantic package model, so it's clear that we are just dealing with syntactic transforms.

This ports all of the package manifest refactoring actions over from swift-package-manager (add package dependency, add target, add product, add target dependency) and introduces a new one that will be used by swift-package-manager in the future, "add plugin usage". The code itself is little changed from the SwiftPM version, although the ManifestEditRefactoringProvider protocol that unifies them all is new.

Note that we end up bringing over syntactic versions of various SwiftPM types like TargetDescription, but in a string-backed, simplified form that is purely syntactic. These are needed to community with the refactoring actions, and can easily be constructed by clients.

DougGregor · 2024-12-01T06:10:55Z

@swift-ci please test

DougGregor · 2024-12-01T07:32:49Z

@swift-ci please test

DougGregor · 2024-12-01T07:33:04Z

@swift-ci please test Windows

DougGregor · 2024-12-01T16:57:16Z

@swift-ci please test

DougGregor · 2024-12-01T16:58:15Z

@swift-ci please test

DougGregor · 2024-12-01T19:08:44Z

@swift-ci please test

DougGregor · 2024-12-01T19:09:05Z

@swift-ci please test Windows

DougGregor · 2024-12-02T07:12:40Z

@swift-ci please test

DougGregor · 2024-12-02T07:13:18Z

@swift-ci please test

DougGregor · 2024-12-02T16:46:08Z

@swift-ci please test Linux

DougGregor · 2024-12-02T16:46:12Z

@swift-ci please test macOS

ahoppen

Thanks for moving the code, Doug.

Sources/SwiftRefactor/PackageManifest/AbsolutePath.swift

Sources/SwiftRefactor/PackageManifest/ProductDescription.swift

Sources/SwiftRefactor/PackageManifest/PackageDependency.swift

Sources/SwiftRefactor/PackageManifest/SyntaxEditUtils.swift

Sources/SwiftRefactor/PackageManifest/AddTargetDependency.swift

Sources/SwiftRefactor/PackageManifest/AddPackageTarget.swift

Tests/SwiftRefactorTest/ManifestEditTests.swift

Sources/SwiftRefactor/PackageManifest/AbsolutePath.swift

Start porting the package manifest editing operations from the Swift Package Manager package over here, so that all of the syntactic refactorings are together in one common place. These refactorings are needed by a number of tools, including SwiftPM, SourceKit-LSP, and (soon) the Swift compiler itself, which can all depend on swift-syntax. The implementation here stubs out the various types used to describe package syntax, using simple string-backed types in place of some of the semantic types that are part of SwiftPM itself, such as SemanticVersion or SourceControlURL. I've also introduced the notion of a ManifestEditRefactoringProvider to generalize over all of the package manifest editing operations. This commit ports over the "Add Package Dependency" command and its tests.

…Refactor This manifest editing action introduces a new dependency to an existing target

…factor

…for a target This allows one to add a plugin usage to a given target, given the target name and description of how the plugin should be used.

This removes a conflict with the SwiftPM version.

…sError` Removes `XCTAssertThrows` in favor of XCTest standard way of testing throwing functions.

…ents When inserting a new element to an array or new argument to a call let's only copy the indentation of the previous element instead of whole leading trivia that includes comments.

…ose Windows failures

…String` All of these types are simple placeholders already with one exception `RelativePath` which used to differentiate path separators on Windows and other platforms, which is not strictly necessary any longer.

…ring` This removes final stub type that could simply be a string and instead of hard-coding a version of swift-syntax library for new dependencies, let's use a placeholder.

… `AddPackageDependency` This was introduced by swiftlang/swift-package-manager#8534

…ft settings Allows to add `enable{Upcoming, Experimental}Feature`, `swiftLanguageMode`, `strictMemorySafety` as well as custom ones.

…onger applicable

…always accept a string This matches `PackageDescription` which explicitly states that all source control dependencies expect a URL.

xedin · 2025-08-07T21:16:18Z

@swift-ci please test

xedin · 2025-08-07T22:13:16Z

Sources/SwiftRefactor/PackageManifest/ManifestEditRefactoringProvider.swift

+public protocol ManifestEditRefactoringProvider: EditRefactoringProvider
+where Self.Input == SourceFileSyntax {
+
+  static func manifestRefactor(syntax: SourceFileSyntax, in context: Context) throws -> PackageEdit


Note: I'm planning to follow-up to this PR and remove this, I think all we need to do is change result type of textRefactor to not only produce edits but also auxiliary files.

Sources/SwiftRefactor/PackageManifest/AddPackageDependency.swift

bnbarham · 2025-08-08T04:08:09Z

Sources/SwiftRefactor/PackageManifest/AddPackageDependency.swift

+/// Add a package dependency to a package manifest's source code.
+public struct AddPackageDependency: ManifestEditRefactoringProvider {
+  public struct Context {
+    public var dependency: PackageDependency


I'm tempted to say this should be optional and we could add placeholder if not given?

I'd think that the whole point of the refactoring is to actually add a dependency that's why it's non-optional at the moment, I'm not sure how we'd placeholder that efficiently because different dependency kinds have different arguments...

I'm not sure how we'd placeholder that efficiently

We'd just pick a reasonable default, ie. SourceControl and from:

.package(url: <#url#>, from: <#version#>)

(though those placeholders could also be typed)

Maybe that is the way to go here but instead of having it optional we can make it a defaulted parameter on initializer in both cases.

Sources/SwiftRefactor/PackageManifest/PackageDependency.swift

Sources/SwiftRefactor/PackageManifest/AddPackageTarget.swift

bnbarham · 2025-08-08T04:14:44Z

Sources/SwiftRefactor/PackageManifest/AddPackageTarget.swift

+/// Add a target to a manifest's source code.
+public struct AddPackageTarget: ManifestEditRefactoringProvider {
+  public struct Context {
+    public let target: PackageTarget


Same comment as adding a package dependency, I could see it being useful to add a placeholder target. Although maybe I'm wrong and you could just use completion for that. Thoughts?

Sources/SwiftRefactor/PackageManifest/AddPluginUsage.swift

…d `SourceControl` Both types are identified by their path and location respectively.

…inUsage`

…ageTarget.Context`

ahoppen

I reviewed this from a swift-syntax standpoint, not from a SwiftPM correctness point of view.

A couple high-level comments:

This PR is quite large and I think we might need at least one more round of reviews. Would it be possible to split it up, eg. by adding only one refactoring action in this one.
I am sceptical about adding SwiftPM-specific types to swift-syntax (or generally the amount of public API surface that this PR adds). Should we consider making these types SPI instead?
For the TODO/FIXMEs in the codebase, could you file issues if you’re not planning to address them in this PR? In my experience they just get lost somehow and some of them probably make for good first issues.
Should these refactorings live in a separte module (eg. SwiftPackageRefactor)? I think that would be a good separation of general Swift language refactorings and refactorings that have knowledge about SwiftPM package manifests. Ideally, we could consider moving that module to a separate repo at some point in the future. I’m not really a fan of adding SwiftPM-specific types to swift-syntax. I think in a perfect world, these types would live in some repository that depends on swift-syntax and that both SourceKit-LSP and SwiftPM can depend on. Would it make sense to mark these types as SPI for now?

ahoppen · 2025-08-08T08:43:23Z

Sources/SwiftRefactor/PackageManifest/ManifestEditRefactoringProvider.swift

+
+import SwiftSyntax
+
+public protocol ManifestEditRefactoringProvider: EditRefactoringProvider


Does this need to be a public protocol? I think it would be better of EditRefactoringProvider would gain the ability to modify auxiliary files. At that point a package refactoring wouldn’t be anything special, it would just be a normal refactoring.

See my comment about this - #2904 (comment)

ahoppen · 2025-08-08T08:44:14Z

Sources/SwiftRefactor/PackageManifest/ManifestEditRefactoringProvider.swift

+  static func manifestRefactor(syntax: SourceFileSyntax, in context: Context) throws -> PackageEdit
+}
+
+extension EditRefactoringProvider where Self: ManifestEditRefactoringProvider {


Couldn’t this be simplified to the following? Or doesn’t that work?

Suggested change

extension EditRefactoringProvider where Self: ManifestEditRefactoringProvider {

extension ManifestEditRefactoringProvider {

✅ You are right, it doesn't require Self: could be placed directly.

ahoppen · 2025-08-08T08:48:39Z

Sources/SwiftRefactor/PackageManifest/PackageDependency.swift

+  case registry(Registry)
+
+  public struct FileSystem: Sendable {
+    public let nameForTargetDependencyResolutionOnly: String?


That name is quite a mouthful. If this is the best name, could you add a comment that describes what it does? Maybe an example would be the best way to convey what both of these members do – also for SourceControl and Registry.

Honestly seems like this could just be name, I don't think the "for target dependency resolution" part really matters

I removed this completely since we don't really need that for refactorings.

ahoppen · 2025-08-08T08:48:58Z

Sources/SwiftRefactor/PackageManifest/PackageDependency.swift

+}
+
+extension PackageDependency: ManifestSyntaxRepresentable {
+  func asSyntax() -> ExprSyntax {


Would this make more sense as a computed property?

ahoppen · 2025-08-08T08:49:49Z

Sources/SwiftRefactor/PackageManifest/PackageDependency.swift

+
+extension PackageDependency.SourceControl: ManifestSyntaxRepresentable {
+  func asSyntax() -> ExprSyntax {
+    // TODO: Not handling identity, nameForTargetDependencyResolutionOnly,


Could you file an issue for this if you’re not planning to address it in this PR. My experience is that these TODOs will just be forgotten. And this would probably also make for a nice good first issue.

We should handle name now and remove the TODO

✅ This TODO was outdated and I removed it, nameFor* is SwiftPM concept that shouldn't be involved here and source control could be identified by its location.

ahoppen · 2025-08-08T09:45:24Z

Sources/SwiftRefactor/PackageManifest/SyntaxEditUtils.swift

+
+fileprivate extension SyntaxProtocol {
+  /// Replace the given child with a new child node.
+  func replacingChild(_ childNode: Syntax, with newChildNode: Syntax) -> Self {


Do we ever replace nodes with nodes of a different type? If so, this would fail the cast(Self.self) if self == childNode. If we don’t need to replace nodes of different types, this could be

Suggested change

func replacingChild(_ childNode: Syntax, with newChildNode: Syntax) -> Self {

func replacingChild<T: SyntaxProtocol>(_ childNode: T, with newChildNode: T) -> Self {

ahoppen · 2025-08-08T09:47:02Z

Sources/SwiftRefactor/PackageManifest/SyntaxEditUtils.swift

+
+      // Format the element appropriately for the context.
+      let indentation = Trivia(
+        pieces: arg.leadingTrivia.filter(\.isSpaceOrTab)


Should we be using indentaitonOfLine or something like that to cover if arg is not on a new line?

ahoppen · 2025-08-08T09:54:08Z

Sources/SwiftRefactor/PackageManifest/AddPackageTarget.swift

+    }
+
+    let importDecls = importModuleNames.lazy.sorted().map { name in
+      DeclSyntax("import \(raw: name)").with(\.trailingTrivia, .newline)


Can’t this just be

Suggested change

DeclSyntax("import \(raw: name)").with(\.trailingTrivia, .newline)

DeclSyntax("import \(raw: name)\n")

ahoppen · 2025-08-08T09:56:05Z

Sources/SwiftRefactor/PackageManifest/AddPackageTarget.swift

+    let sourceFileText: SourceFileSyntax
+    switch target.type {
+    case .binary, .plugin, .system:
+      fatalError("should have exited above")


Where is above? I don’t see the check where this should have exited.

ahoppen · 2025-08-08T09:57:26Z

Sources/SwiftRefactor/PackageManifest/AddPackageTarget.swift

+}
+
+fileprivate extension String {
+  func localizedFirstWordCapitalized() -> String { prefix(1).uppercased() + dropFirst() }


What’s the localize part of this function name?

…porting types as SPI Avoid increasing public API surface of the swift-syntax for now until the factorings are fully evaluated.

xedin · 2025-08-08T17:57:12Z

We have discussed this and decided to SPI new refactorings for now.

xedin · 2025-08-08T18:02:01Z

@swift-ci please test

xedin · 2025-08-08T18:04:11Z

@swift-ci please test Windows platform

ahoppen

Good with me, assuming all declarations are at most SPI (ie. no public declarations).

DougGregor requested review from ahoppen and bnbarham as code owners December 1, 2024 06:10

DougGregor force-pushed the package-manifest-refactor branch from f371a2c to 4061163 Compare December 1, 2024 16:57

ahoppen reviewed Dec 3, 2024

View reviewed changes

bnbarham reviewed Dec 4, 2024

View reviewed changes

Sources/SwiftRefactor/PackageManifest/AbsolutePath.swift Outdated Show resolved Hide resolved

DougGregor added 13 commits August 4, 2025 17:15

FIXUP for adding package dependency

4a6e7bd

Port "Add target dependency" package manifest editing action to Swift…

86581a0

…Refactor This manifest editing action introduces a new dependency to an existing target

Port "Add target" from the Swift Package Manager code base to SwiftRe…

1b624fc

…factor

Port "Add Product" manifest edit refactor over from SwiftPM

d308aca

Add a package manifest edit refactor to introduce a new plugin usage …

11649fd

…for a target This allows one to add a plugin usage to a given target, given the target name and description of how the plugin should be used.

Implement a better "contains string literal" check for a source file

1aa14e6

Remove more uses of switch expressions

89c2854

Remove yet more uses of switch expressions

ba0ac8a

Rename PackageEditResult -> PackageEdit

18c9f0e

This removes a conflict with the SwiftPM version.

Rename TargetDescription -> Target

4b2073f

Sink ProductType into ProductDescription

7293a79

Rename AddTarget -> AddPackageTarget

e6bd577

xedin force-pushed the package-manifest-refactor branch from ede1bea to 6af939f Compare August 5, 2025 07:20

xedin added 11 commits August 7, 2025 11:38

[SwiftRefactor] Replace custom XCTAssertThrows with `XCTAssertThrow…

9370e43

…sError` Removes `XCTAssertThrows` in favor of XCTest standard way of testing throwing functions.

[SwiftRefactor] Fix handling of leading trivia for new elements/argum…

902b53e

…ents When inserting a new element to an array or new argument to a call let's only copy the indentation of the previous element instead of whole leading trivia that includes comments.

[Tests] Add more information to unexpected aux files failure to diagn…

95190dc

…ose Windows failures

[SwiftRefactor] PackageManifest: Rename Target to PackageTarget

de9afa3

[SwiftRefactor] PackageManifest: Replace custom types with a simple `…

cd7758e

…String` All of these types are simple placeholders already with one exception `RelativePath` which used to differentiate path separators on Windows and other platforms, which is not strictly necessary any longer.

[SwiftRefactor] PackageManifest: Replace SemanticVersion with a `St…

b44a72f

…ring` This removes final stub type that could simply be a string and instead of hard-coding a version of swift-syntax library for new dependencies, let's use a placeholder.

[SwiftRefactor] PackageManifest: Fix formatting for license headers

58a01d3

[SwiftRefactor] PackageManifest: Add duplicate dependency checking to…

621ae13

… `AddPackageDependency` This was introduced by swiftlang/swift-package-manager#8534

[SwiftRefactor] PackageManifest: Add refactoring to add introduce swi…

fb6939c

…ft settings Allows to add `enable{Upcoming, Experimental}Feature`, `swiftLanguageMode`, `strictMemorySafety` as well as custom ones.

[SwiftRefactor] PackageManifest: Remove an outdated TODO that is no l…

68432ea

…onger applicable

[SwiftRefactor] PackageManifest: Change source control dependency to …

b896109

…always accept a string This matches `PackageDescription` which explicitly states that all source control dependencies expect a URL.

xedin force-pushed the package-manifest-refactor branch from a0d3cfe to b896109 Compare August 7, 2025 20:28

xedin requested review from bnbarham and ahoppen August 7, 2025 20:32

xedin reviewed Aug 7, 2025

View reviewed changes

bnbarham reviewed Aug 8, 2025

View reviewed changes

xedin added 3 commits August 7, 2025 22:56

[SwiftRefactor] PackageManifest: Remove identity from FileSystem an…

966e0d4

…d `SourceControl` Both types are identified by their path and location respectively.

[SwiftRefactor] PackageManifest: Remove unused property from `AddPlug…

db2b78f

…inUsage`

[SwiftRefactor] PackageManifest: Inline Configuration into `AddPack…

6a98663

…ageTarget.Context`

ahoppen reviewed Aug 8, 2025

View reviewed changes

xedin force-pushed the package-manifest-refactor branch from f72f9d9 to 22bbcde Compare August 8, 2025 17:45

xedin added 2 commits August 8, 2025 10:48

[SwiftRefactor] PackageManifest: Address stylistic review feedback

9e08670

[SwiftRefactor] PackageManifest: Mark all of the refactorings and sup…

2f6937d

…porting types as SPI Avoid increasing public API surface of the swift-syntax for now until the factorings are fully evaluated.

xedin force-pushed the package-manifest-refactor branch from 22bbcde to 2f6937d Compare August 8, 2025 17:48

ahoppen approved these changes Aug 8, 2025

View reviewed changes

xedin merged commit c4002f2 into swiftlang:main Aug 8, 2025
30 checks passed


		import SwiftSyntax

		public protocol ManifestEditRefactoringProvider: EditRefactoringProvider

	extension EditRefactoringProvider where Self: ManifestEditRefactoringProvider {
	extension ManifestEditRefactoringProvider {

	func replacingChild(_ childNode: Syntax, with newChildNode: Syntax) -> Self {
	func replacingChild<T: SyntaxProtocol>(_ childNode: T, with newChildNode: T) -> Self {

	DeclSyntax("import \(raw: name)").with(\.trailingTrivia, .newline)
	DeclSyntax("import \(raw: name)\n")

Swift package manifest refactoring actions #2904

Swift package manifest refactoring actions #2904

Conversation

DougGregor commented Dec 1, 2024 • edited by xedin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DougGregor commented Dec 1, 2024

Uh oh!

DougGregor commented Dec 1, 2024

Uh oh!

DougGregor commented Dec 1, 2024

Uh oh!

DougGregor commented Dec 1, 2024

Uh oh!

DougGregor commented Dec 1, 2024

Uh oh!

DougGregor commented Dec 1, 2024

Uh oh!

DougGregor commented Dec 1, 2024

Uh oh!

DougGregor commented Dec 2, 2024

Uh oh!

DougGregor commented Dec 2, 2024

Uh oh!

DougGregor commented Dec 2, 2024

Uh oh!

DougGregor commented Dec 2, 2024

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xedin commented Aug 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

DougGregor commented Dec 1, 2024 •

edited by xedin

Loading