Swift package manifest refactoring actions #2904
Conversation
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.
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
|
@swift-ci please test Windows |
…Refactor This manifest editing action introduces a new dependency to an existing target
…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.
DougGregor
force-pushed
the
package-manifest-refactor
branch
from
f371a2c to
4061163
Compare
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
|
@swift-ci please test |
|
@swift-ci please test Windows |
This removes a conflict with the SwiftPM version.
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
|
@swift-ci please test Linux |
|
@swift-ci please test macOS |
| /// Syntactic wrapper type that describes an absolute path for refactoring | ||
| /// purposes but does not interpret its contents. | ||
| public struct AbsolutePath: CustomStringConvertible, Equatable, Hashable, Sendable { | ||
| public private(set) var description: String |
There was a problem hiding this comment.
Doesn’t look like you ever modify it so could be
| public private(set) var description: String | |
| public let description: String |
Same for the other wrapper types.
There was a problem hiding this comment.
Since you’re adding new public declarations, could you add a release notes entry?
|
|
||
| let separateParen: String = arguments.count > 1 ? "\n" : "" | ||
| let argumentsSyntax = LabeledExprListSyntax(arguments) | ||
| return ".\(raw: functionName)(\(argumentsSyntax)\(raw: separateParen))" |
There was a problem hiding this comment.
Maybe I’m missing something but this reads to me as if we put all the arguments on the same line as the function name and then the closing parenthesis on a new line. Am I missing something?
| // or productFilter yet. | ||
| switch location { | ||
| case .local: | ||
| fatalError() |
There was a problem hiding this comment.
Is there a way we can avoid the fatalError because this would take down SourceKit-LSP when hit, which we really want to avoid. Maybe return an optional or error and fail the refactoring? Or implement this case.
|
|
||
| /// Produce trivia from the last newline to the end, dropping anything | ||
| /// prior to that. | ||
| func onlyLastLine() -> Trivia { |
There was a problem hiding this comment.
I think a computed property would fit better here.
| /// The set of argument labels that can occur after the "dependencies" | ||
| /// argument in the various target initializers. | ||
| /// | ||
| /// TODO: Could we generate this from the the PackageDescription module, so |
There was a problem hiding this comment.
If this is no longer defined in SwiftPM, I don’t think we’ll be able to generate it from the PackageDescription module, so I’m not sure if this comment is still applicable anymore.
Same in the other refactorings.
| public init( | ||
| testHarness: TestHarness = .default, | ||
| swiftSyntaxVersion: SemanticVersion = SemanticVersion("600.0.0-latest"), | ||
| swiftTestingVersion: SemanticVersion = SemanticVersion("0.8.0") | ||
| ) { |
There was a problem hiding this comment.
I’m quite worried about this because we will forget to update these (they are already out-of-date).
| target.dependencies.append(contentsOf: macroTargetDependencies) | ||
|
|
||
| default: | ||
| break; |
| } | ||
| } | ||
|
|
||
| default: break; |
There was a problem hiding this comment.
| default: break; | |
| default: break |
| ) | ||
| } | ||
|
|
||
| func XCTAssertThrows<T: Swift.Error, Ignore>( |
There was a problem hiding this comment.
Why doesn’t XCTAssertThrowsError from XCTest work here? If it doesn’t work, I would name this assertThrows because we don’t want to define local functions with the XCT prefix. Also, I would make this fileprivate to avoid it leaking into other tests.
|
|
||
| /// Syntactic wrapper type that describes an absolute path for refactoring | ||
| /// purposes but does not interpret its contents. | ||
| public struct AbsolutePath: CustomStringConvertible, Equatable, Hashable, Sendable { |
There was a problem hiding this comment.
I'd prefer not introducing so many fairly generic public types here (AbsolutePath, PackageIdentity, RelativePath, SemanticVersion, SourceControlURL). How much do you prefer this over just strings for these cases? Another alternative would be to namespace them in an enum, but I believe @ahoppen is generally not a fan of that.
Also, could we make Target -> PackageTarget?
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:SwiftRefactormodule is where we are collecting various refactoring.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
ManifestEditRefactoringProviderprotocol that unifies them all is new.Note that we end up bringing over syntactic versions of various SwiftPM types like
TargetDescriptionandSourceControlURL, 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.