Refactor and clean up plugin communication and wire messages (#4155)

Refactor the plugin machinery for better maintainability, consolidation of wire protocol struct definitions, and groundwork for extending the plugin functionality (such as adding new capabilities or new plugin messages such as dynamic querying of capabilities).

Changes:

- factor out the Codable declarations of enums and struct for the plugin wire protocol into a PluginMessage file that can be shared between the plugin and plugin host; this gets us compile-time checking so that the host and plugin are always in sync
- separate out the functionality of DefaultPluginScriptRunner to only handle the mechanics to talking to the plugin and not be concerned with what the messages are (this is factored back down into PluginInvocation)
- hoist the different kinds of plugin action messages up to top-level messages, allowing each to have a separate set of parameters
- factor the plugin input context serializer and deserializer out into their own source files and better separating from the semantics
- better separation of the structures for the plugin request structures (for building, testing, etc) to not reference any part of the plugin API; this will allow separate representations when/if necessary
- pull the package parameter into the plugin action, to support actions that don't operate on packages (such as querying the plugin for information, etc)

No functional changes.

Author: abertelrud

Sources/Commands/SwiftPackageTool.swift

@@ -1064,8 +1064,7 @@ extension SwiftPackageTool {
             // Run the command plugin.
             let buildEnvironment = try swiftTool.buildParameters().buildEnvironment
             let _ = try tsc_await { plugin.invoke(
-                action: .performCommand(arguments: arguments),
-                package: package,
+                action: .performCommand(package: package, arguments: arguments),
                 buildEnvironment: buildEnvironment,
                 scriptRunner: pluginScriptRunner,
                 workingDirectory: swiftTool.originalWorkingDirectory,

Sources/PackagePlugin/CMakeLists.txt

@@ -16,7 +16,8 @@ add_library(PackagePlugin
   PackageModel.swift
   Path.swift
   Plugin.swift
-  PluginInput.swift
+  PluginContextDeserializer.swift
+  PluginMessages.swift
   Protocols.swift
   Utilities.swift)
 

Sources/PackagePlugin/PackageManagerProxy.swift

@@ -1,7 +1,7 @@
 /*
  This source file is part of the Swift.org open source project
 
- Copyright (c) 2021 Apple Inc. and the Swift project authors
+ Copyright (c) 2021-2022 Apple Inc. and the Swift project authors
  Licensed under Apache License v2.0 with Runtime Library Exception
 
  See http://swift.org/LICENSE.txt for license information
@@ -29,14 +29,14 @@ public struct PackageManager {
     ) throws -> BuildResult {
         // Ask the plugin host to build the specified products and targets, and wait for a response.
         // FIXME: We'll want to make this asynchronous when there is back deployment support for it.
-        return try sendMessageAndWaitForReply(.buildOperationRequest(subset: subset, parameters: parameters)) {
+        return try sendMessageAndWaitForReply(.buildOperationRequest(subset: .init(subset), parameters: .init(parameters))) {
             guard case .buildOperationResponse(let result) = $0 else { return nil }
-            return result
+            return .init(result)
         }
     }
 
     /// Specifies a subset of products and targets of a package to build.
-    public enum BuildSubset: Encodable {
+    public enum BuildSubset {
         /// Represents the subset consisting of all products and of either all
         /// targets or (if `includingTests` is false) just non-test targets.
         case all(includingTests: Bool)
@@ -49,7 +49,7 @@ public struct PackageManager {
     }
 
     /// Parameters and options to apply during a build.
-    public struct BuildParameters: Encodable {
+    public struct BuildParameters {
         /// Whether to build for debug or release.
         public var configuration: BuildConfiguration
 
@@ -76,17 +76,17 @@ public struct PackageManager {
 
     /// Represents an overall purpose of the build, which affects such things
     /// asoptimization and generation of debug symbols.
-    public enum BuildConfiguration: String, Encodable {
+    public enum BuildConfiguration: String {
         case debug, release
     }
 
     /// Represents the amount of detail in a build log.
-    public enum BuildLogVerbosity: String, Encodable {
+    public enum BuildLogVerbosity: String {
         case concise, verbose, debug
     }
 
     /// Represents the results of running a build.
-    public struct BuildResult: Decodable {
+    public struct BuildResult {
         /// Whether the build succeeded or failed.
         public var succeeded: Bool
 
@@ -98,7 +98,7 @@ public struct PackageManager {
         public var builtArtifacts: [BuiltArtifact]
 
         /// Represents a single artifact produced during a build.
-        public struct BuiltArtifact: Decodable {
+        public struct BuiltArtifact {
             /// Full path of the built artifact in the local file system.
             public var path: Path
 
@@ -108,7 +108,7 @@ public struct PackageManager {
             /// Represents the kind of artifact that was built. The specific file
             /// formats may vary from platform to platform — for example, on macOS
             /// a dynamic library may in fact be built as a framework.
-            public enum Kind: String, Decodable {
+            public enum Kind: String {
                 case executable, dynamicLibrary, staticLibrary
             }
         }
@@ -129,14 +129,14 @@ public struct PackageManager {
     ) throws -> TestResult {
         // Ask the plugin host to run the specified tests, and wait for a response.
         // FIXME: We'll want to make this asynchronous when there is back deployment support for it.
-        return try sendMessageAndWaitForReply(.testOperationRequest(subset: subset, parameters: parameters)) {
+        return try sendMessageAndWaitForReply(.testOperationRequest(subset: .init(subset), parameters: .init(parameters))) {
             guard case .testOperationResponse(let result) = $0 else { return nil }
-            return result
+            return .init(result)
         }
     }
 
     /// Specifies what tests in a package to run.
-    public enum TestSubset: Encodable {
+    public enum TestSubset {
         /// Represents all tests in the package.
         case all
 
@@ -147,7 +147,7 @@ public struct PackageManager {
     }
 
     /// Parameters that control how the tests are run.
-    public struct TestParameters: Encodable {
+    public struct TestParameters {
         /// Whether to collect code coverage information while running the tests.
         public var enableCodeCoverage: Bool
 
@@ -157,7 +157,7 @@ public struct PackageManager {
     }
 
     /// Represents the result of running unit tests.
-    public struct TestResult: Decodable {
+    public struct TestResult {
         /// Whether the test run succeeded or failed.
         public var succeeded: Bool
 
@@ -171,24 +171,24 @@ public struct PackageManager {
 
         /// Represents the results of running some or all of the tests in a
         /// single test target.
-        public struct TestTarget: Decodable {
+        public struct TestTarget {
             public var name: String
             public var testCases: [TestCase]
 
             /// Represents the results of running some or all of the tests in
             /// a single test case.
-            public struct TestCase: Decodable {
+            public struct TestCase {
                 public var name: String
                 public var tests: [Test]
 
                 /// Represents the results of running a single test.
-                public struct Test: Decodable {
+                public struct Test {
                     public var name: String
                     public var result: Result
                     public var duration: Double
 
                     /// Represents the result of running a single test.
-                    public enum Result: String, Decodable {
+                    public enum Result: String {
                         case succeeded, skipped, failed
                     }
                 }
@@ -206,19 +206,19 @@ public struct PackageManager {
     ) throws -> SymbolGraphResult {
         // Ask the plugin host for symbol graph information for the target, and wait for a response.
         // FIXME: We'll want to make this asynchronous when there is back deployment support for it.
-        return try sendMessageAndWaitForReply(.symbolGraphRequest(targetName: target.name, options: options)) {
+        return try sendMessageAndWaitForReply(.symbolGraphRequest(targetName: target.name, options: .init(options))) {
             guard case .symbolGraphResponse(let result) = $0 else { return nil }
-            return result
+            return .init(result)
         }
     }
 
     /// Represents options for symbol graph generation.
-    public struct SymbolGraphOptions: Encodable {
+    public struct SymbolGraphOptions {
         /// The symbol graph will include symbols at this access level and higher.
         public var minimumAccessLevel: AccessLevel
 
         /// Represents a Swift access level.
-        public enum AccessLevel: String, CaseIterable, Encodable {
+        public enum AccessLevel: String, CaseIterable {
             case `private`, `fileprivate`, `internal`, `public`, `open`
         }
 
@@ -236,13 +236,15 @@ public struct PackageManager {
     }
 
     /// Represents the result of symbol graph generation.
-    public struct SymbolGraphResult: Decodable {
+    public struct SymbolGraphResult {
         /// The directory that contains the symbol graph files for the target.
         public var directoryPath: Path
     }
-    
+}
+
+fileprivate extension PackageManager {
     /// Private helper function that sends a message to the host and waits for a reply. The reply handler should return nil for any reply message it doesn't recognize.
-    fileprivate func sendMessageAndWaitForReply<T>(_ message: PluginToHostMessage, replyHandler: (HostToPluginMessage) -> T?) throws -> T {
+    func sendMessageAndWaitForReply<T>(_ message: PluginToHostMessage, replyHandler: (HostToPluginMessage) -> T?) throws -> T {
         try pluginHostConnection.sendMessage(message)
         guard let reply = try pluginHostConnection.waitForNextMessage() else {
             throw PackageManagerProxyError.unspecified("internal error: unexpected lack of response message")
@@ -264,3 +266,170 @@ public enum PackageManagerProxyError: Error {
     /// An unspecified other kind of error from the Package Manager proxy.
     case unspecified(_ message: String)
 }
+
+fileprivate extension PluginToHostMessage.BuildSubset {
+    init(_ subset: PackageManager.BuildSubset) {
+        switch subset {
+        case .all(let includingTests):
+            self = .all(includingTests: includingTests)
+        case .product(let name):
+            self = .product(name)
+        case .target(let name):
+            self = .target(name)
+        }
+    }
+}
+
+fileprivate extension PluginToHostMessage.BuildParameters {
+    init(_ parameters: PackageManager.BuildParameters) {
+        self.configuration = .init(parameters.configuration)
+        self.logging = .init(parameters.logging)
+        self.otherCFlags = parameters.otherCFlags
+        self.otherCxxFlags = parameters.otherCxxFlags
+        self.otherSwiftcFlags = parameters.otherSwiftcFlags
+        self.otherLinkerFlags = parameters.otherLinkerFlags
+    }
+}
+
+fileprivate extension PluginToHostMessage.BuildParameters.Configuration {
+    init(_ configuration: PackageManager.BuildConfiguration) {
+        switch configuration {
+        case .debug:
+            self = .debug
+        case .release:
+            self = .release
+        }
+    }
+}
+
+fileprivate extension PluginToHostMessage.BuildParameters.LogVerbosity {
+    init(_ verbosity: PackageManager.BuildLogVerbosity) {
+        switch verbosity {
+        case .concise:
+            self = .concise
+        case .verbose:
+            self = .verbose
+        case .debug:
+            self = .debug
+        }
+    }
+}
+
+fileprivate extension PackageManager.BuildResult {
+    init(_ result: HostToPluginMessage.BuildResult) {
+        self.succeeded = result.succeeded
+        self.logText = result.logText
+        self.builtArtifacts = result.builtArtifacts.map { .init($0) }
+    }
+}
+
+fileprivate extension PackageManager.BuildResult.BuiltArtifact {
+    init(_ artifact: HostToPluginMessage.BuildResult.BuiltArtifact) {
+        self.path = .init(artifact.path)
+        self.kind = .init(artifact.kind)
+    }
+}
+
+fileprivate extension PackageManager.BuildResult.BuiltArtifact.Kind {
+    init(_ kind: HostToPluginMessage.BuildResult.BuiltArtifact.Kind) {
+        switch kind {
+        case .executable:
+            self = .executable
+        case .dynamicLibrary:
+            self = .dynamicLibrary
+        case .staticLibrary:
+            self = .staticLibrary
+        }
+    }
+}
+
+fileprivate extension PluginToHostMessage.TestSubset {
+    init(_ subset: PackageManager.TestSubset) {
+        switch subset {
+        case .all:
+            self = .all
+        case .filtered(let regexes):
+            self = .filtered(regexes)
+        }
+    }
+}
+
+fileprivate extension PluginToHostMessage.TestParameters {
+    init(_ parameters: PackageManager.TestParameters) {
+        self.enableCodeCoverage = parameters.enableCodeCoverage
+    }
+}
+
+fileprivate extension PackageManager.TestResult {
+    init(_ result: HostToPluginMessage.TestResult) {
+        self.succeeded = result.succeeded
+        self.testTargets = result.testTargets.map{ .init($0) }
+        self.codeCoverageDataFile = result.codeCoverageDataFile.map{ .init($0) }
+    }
+}
+
+fileprivate extension PackageManager.TestResult.TestTarget {
+    init(_ testTarget: HostToPluginMessage.TestResult.TestTarget) {
+        self.name = testTarget.name
+        self.testCases = testTarget.testCases.map{ .init($0) }
+    }
+}
+
+fileprivate extension PackageManager.TestResult.TestTarget.TestCase {
+    init(_ testCase: HostToPluginMessage.TestResult.TestTarget.TestCase) {
+        self.name = testCase.name
+        self.tests = testCase.tests.map{ .init($0) }
+    }
+}
+
+fileprivate extension PackageManager.TestResult.TestTarget.TestCase.Test {
+    init(_ test: HostToPluginMessage.TestResult.TestTarget.TestCase.Test) {
+        self.name = test.name
+        self.result = .init(test.result)
+        self.duration = test.duration
+    }
+}
+
+fileprivate extension PackageManager.TestResult.TestTarget.TestCase.Test.Result {
+    init(_ result: HostToPluginMessage.TestResult.TestTarget.TestCase.Test.Result) {
+        switch result {
+        case .succeeded:
+            self = .succeeded
+        case .skipped:
+            self = .skipped
+        case .failed:
+            self = .failed
+        }
+    }
+}
+
+fileprivate extension PluginToHostMessage.SymbolGraphOptions {
+    init(_ options: PackageManager.SymbolGraphOptions) {
+        self.minimumAccessLevel = .init(options.minimumAccessLevel)
+        self.includeSynthesized = options.includeSynthesized
+        self.includeSPI = options.includeSPI
+    }
+}
+
+fileprivate extension PluginToHostMessage.SymbolGraphOptions.AccessLevel {
+    init(_ accessLevel: PackageManager.SymbolGraphOptions.AccessLevel) {
+        switch accessLevel {
+        case .private:
+            self = .private
+        case .fileprivate:
+            self = .fileprivate
+        case .internal:
+            self = .internal
+        case .public:
+            self = .public
+        case .open:
+            self = .open
+        }
+    }
+}
+
+fileprivate extension PackageManager.SymbolGraphResult {
+    init(_ result: HostToPluginMessage.SymbolGraphResult) {
+        self.directoryPath = .init(result.directoryPath)
+    }
+}