Errors thrown at top level of plugin should be reported as diagnostics (#3957)

When a plugin throws an uncaught error, it should be reported as a diagnostic rather than causing the standard Swift output.

As part of making this work well, we also simplify the way the result code is received from the plugin.  It was a bit awkward to have two result codes (the one from the last message as well as the exit code from the process) so now we just use the exit code.  This is also most robust to any plugin that decides to do "exit(1)" on its own from inside the code.  If/when we have long-running multi-action plugins we can revisit this.

We also make sure that at least one error message is emitted from any plugin that exits with an error code but that does not emit an error diagnostic.

This also reworks the unit test for command invocations to be more scalable.

Author: abertelrud

Sources/PackagePlugin/Plugin.swift

@@ -92,7 +92,15 @@ extension Plugin {
         // Handle messages from the host until the input stream is closed,
         // indicating that we're done.
         while let message = try pluginHostConnection.waitForNextMessage() {
-            try handleMessage(message)
+            do {
+                try handleMessage(message)
+            }
+            catch {
+                // Emit a diagnostic and indicate failure to the plugin host,
+                // and exit with an error code.
+                Diagnostics.error(String(describing: error))
+                exit(1)
+            }
         }
     }
 
@@ -185,8 +193,8 @@ extension Plugin {
                 try plugin.performCommand(context: context, targets: targets, arguments: arguments)
             }
 
-            // Send back a message to the host indicating that we're done.
-            try pluginHostConnection.sendMessage(.actionComplete(success: true))
+            // Exit with a zero exit code to indicate success.
+            exit(0)
 
         default:
             internalError("unexpected top-level message \(message)")
@@ -258,9 +266,6 @@ enum PluginToHostMessage: Encodable {
 
     /// The plugin is requesting symbol graph information for a given target and set of options.
     case symbolGraphRequest(targetName: String, options: PackageManager.SymbolGraphOptions)
-    
-    /// The plugin has finished the requested action.
-    case actionComplete(success: Bool)
 }
 
 typealias PluginHostConnection = MessageConnection<PluginToHostMessage, HostToPluginMessage>

Sources/Workspace/DefaultPluginScriptRunner.swift

@@ -375,7 +375,7 @@ public struct DefaultPluginScriptRunner: PluginScriptRunner {
         process.standardInput = stdinPipe
 
         // Private message handler method. Always invoked on the callback queue.
-        var result: Bool? = .none
+        var emittedAtLeastOneError = false
         func handle(message: PluginToHostMessage) throws {
             dispatchPrecondition(condition: .onQueue(callbackQueue))
             switch message {
@@ -390,6 +390,7 @@ public struct DefaultPluginScriptRunner: PluginScriptRunner {
                 let diagnostic: Basics.Diagnostic
                 switch severity {
                 case .error:
+                    emittedAtLeastOneError = true
                     diagnostic = .error(message, metadata: metadata)
                 case .warning:
                     diagnostic = .warning(message, metadata: metadata)
@@ -447,13 +448,6 @@ public struct DefaultPluginScriptRunner: PluginScriptRunner {
                         outputQueue.async { try? outputHandle.writePluginMessage(.errorResponse(error: String(describing: error))) }
                     }
                 }
-
-            case .actionComplete(let success):
-                // The plugin has indicated that it's finished the requested action.
-                result = success
-                outputQueue.async {
-                    try? outputHandle.close()
-                }
             }
         }
 
@@ -463,7 +457,7 @@ public struct DefaultPluginScriptRunner: PluginScriptRunner {
         stdoutPipe.fileHandleForReading.readabilityHandler = { fileHandle in
             // Parse the next message and pass it on to the delegate.
             stdoutLock.withLock {
-                if let message = try? fileHandle.readPluginMessage() {
+                while let message = try? fileHandle.readPluginMessage() {
                     // FIXME: We should handle errors here.
                     callbackQueue.async { try? handle(message: message) }
                 }
@@ -488,31 +482,35 @@ public struct DefaultPluginScriptRunner: PluginScriptRunner {
 
         // Set up a handler to deal with the exit of the plugin process.
         process.terminationHandler = { process in
+            // Close the output handle through which we talked to the plugin.
+            try? outputHandle.close()
+
+            // Read and pass on any remaining messages from the plugin.
+            stdoutPipe.fileHandleForReading.readabilityHandler?(stdoutPipe.fileHandleForReading)
+
             // Read and pass on any remaining free-form text output from the plugin.
             stderrPipe.fileHandleForReading.readabilityHandler?(stderrPipe.fileHandleForReading)
-            
+
             // Call the completion block with a result that depends on how the process ended.
             callbackQueue.async {
                 completion(Result {
+                    // We throw an error if the plugin ended with a signal.
                     if process.terminationReason == .uncaughtSignal {
                         throw DefaultPluginScriptRunnerError.invocationEndedBySignal(
                             signal: process.terminationStatus,
                             command: command,
                             output: String(decoding: stderrData, as: UTF8.self))
                     }
-                    if process.terminationStatus != 0 {
-                        throw DefaultPluginScriptRunnerError.invocationEndedWithNonZeroExitCode(
-                            exitCode: process.terminationStatus,
-                            command: command,
-                            output: String(decoding: stderrData, as: UTF8.self))
+                    // Otherwise we return a result based on its exit code. If
+                    // the plugin exits with an error but hasn't already emitted
+                    // an error, we do so for it.
+                    let success = (process.terminationStatus == 0)
+                    if !success && !emittedAtLeastOneError {
+                        delegate.pluginEmittedDiagnostic(
+                            .error("Plugin ended with exit code \(process.terminationStatus)")
+                        )
                     }
-                    guard let result = result else {
-                        throw DefaultPluginScriptRunnerError.pluginCommunicationError(
-                            message: "didn’t receive output result from plugin",
-                            command: command,
-                            output: String(decoding: stderrData, as: UTF8.self))
-                    }
-                    return result
+                    return success
                 })
             }
         }
@@ -660,9 +658,6 @@ enum PluginToHostMessage: Decodable {
 
     /// The plugin is requesting symbol graph information for a given target and set of options.
     case symbolGraphRequest(targetName: String, options: PluginInvocationSymbolGraphOptions)
-    
-    /// The plugin has finished the requested action.
-    case actionComplete(success: Bool)
 }
 
 fileprivate extension FileHandle {

Tests/FunctionalTests/PluginTests.swift

@@ -187,6 +187,7 @@ class PluginTests: XCTestCase {
     }
 
     func testCommandPluginInvocation() throws {
+        // FIXME: This test is getting quite long — we should add some support functionality for creating synthetic plugin tests and factor this out into separate tests.
         try testWithTemporaryDirectory { tmpPath in
             // Create a sample package with a library target and a plugin. It depends on a sample package.
             let packageDir = tmpPath.appending(components: "MyPackage")
@@ -207,10 +208,22 @@ class PluginTests: XCTestCase {
                             ]
                         ),
                         .plugin(
-                            name: "MyPlugin",
+                            name: "PluginPrintingInfo",
                             capability: .command(
-                                intent: .custom(verb: "mycmd", description: "What is mycmd anyway?"),
-                                permissions: [.writeToPackageDirectory(reason: "YOLO")]
+                                intent: .custom(verb: "print-info", description: "Description of the command"),
+                                permissions: [.writeToPackageDirectory(reason: "Reason for wanting to write to package directory")]
+                            )
+                        ),
+                        .plugin(
+                            name: "PluginFailingWithError",
+                            capability: .command(
+                                intent: .custom(verb: "fail-with-error", description: "Sample plugin that throws an error")
+                            )
+                        ),
+                        .plugin(
+                            name: "PluginFailingWithoutError",
+                            capability: .command(
+                                intent: .custom(verb: "fail-without-error", description: "Sample plugin that exits without error")
                             )
                         ),
                     ]
@@ -222,7 +235,7 @@ class PluginTests: XCTestCase {
                     public func Foo() { }
                 """
             }
-            try localFileSystem.writeFileContents(packageDir.appending(components: "Plugins", "MyPlugin", "plugin.swift")) {
+            try localFileSystem.writeFileContents(packageDir.appending(components: "Plugins", "PluginPrintingInfo", "plugin.swift")) {
                 $0 <<< """
                     import PackagePlugin
                     
@@ -243,6 +256,49 @@ class PluginTests: XCTestCase {
                     }
                 """
             }
+            try localFileSystem.writeFileContents(packageDir.appending(components: "Plugins", "PluginFailingWithError", "plugin.swift")) {
+                $0 <<< """
+                    import PackagePlugin
+
+                    @main
+                    struct MyCommandPlugin: CommandPlugin {
+                        func performCommand(
+                            context: PluginContext,
+                            targets: [Target],
+                            arguments: [String]
+                        ) throws {
+                            // Print some output that should appear before the error diagnostic.
+                            print("This text should appear before the error.")
+
+                            // Throw an uncaught error that should be reported as a diagnostics.
+                            throw "Houston, we have a problem."
+                        }
+                    }
+                    extension String: Error { }
+                """
+            }
+            try localFileSystem.writeFileContents(packageDir.appending(components: "Plugins", "PluginFailingWithoutError", "plugin.swift")) {
+                $0 <<< """
+                    import PackagePlugin
+                    import Foundation
+
+                    @main
+                    struct MyCommandPlugin: CommandPlugin {
+                        func performCommand(
+                            context: PluginContext,
+                            targets: [Target],
+                            arguments: [String]
+                        ) throws {
+                            // Print some output that should appear before we exit.
+                            print("This text should appear before we exit.")
+
+                            // Just exit with an error code without an emitting error.
+                            exit(1)
+                        }
+                    }
+                    extension String: Error { }
+                """
+            }
 
             // Create the sample vendored dependency package.
             try localFileSystem.writeFileContents(packageDir.appending(components: "VendoredDependencies", "HelperPackage", "Package.swift")) {
@@ -302,62 +358,110 @@ class PluginTests: XCTestCase {
             let libraryTarget = try XCTUnwrap(package.targets.map(\.underlyingTarget).first{ $0.name == "MyLibrary" } as? SwiftTarget)
             XCTAssertEqual(libraryTarget.type, .library)
 
-            // Find the command plugin in our test package.
-            let pluginTarget = try XCTUnwrap(package.targets.map(\.underlyingTarget).first{ $0.name == "MyPlugin" } as? PluginTarget)
-            XCTAssertEqual(pluginTarget.type, .plugin)
-            
             // Set up a delegate to handle callbacks from the command plugin.
             let delegateQueue = DispatchQueue(label: "plugin-invocation")
             class PluginDelegate: PluginInvocationDelegate {
                 let delegateQueue: DispatchQueue
-                var outputData = Data()
+                var diagnostics: [Basics.Diagnostic] = []
 
                 init(delegateQueue: DispatchQueue) {
                     self.delegateQueue = delegateQueue
                 }
 
                 func pluginEmittedOutput(_ data: Data) {
+                    // Add each line of emitted output as a `.info` diagnostic.
                     dispatchPrecondition(condition: .onQueue(delegateQueue))
-                    outputData.append(contentsOf: data)
-                    print(String(decoding: data, as: UTF8.self)
-                        .split(separator: "\n")
-                        .map{ "🧩 \($0)" }
-                        .joined(separator: "\n")
-                    )
+                    let textlines = String(decoding: data, as: UTF8.self).split(separator: "\n")
+                    print(textlines.map{ "🧩 \($0)" }.joined(separator: "\n"))
+                    diagnostics.append(contentsOf: textlines.map{
+                        Basics.Diagnostic(severity: .info, message: String($0), metadata: .none)
+                    })
                 }
 
                 func pluginEmittedDiagnostic(_ diagnostic: Basics.Diagnostic) {
+                    // Add the diagnostic as-is.
                     dispatchPrecondition(condition: .onQueue(delegateQueue))
+                    diagnostics.append(diagnostic)
                 }
             }
-            let pluginDelegate = PluginDelegate(delegateQueue: delegateQueue)
 
-            // Invoke the command plugin.
-            let pluginCacheDir = tmpPath.appending(component: "plugin-cache")
-            let pluginOutputDir = tmpPath.appending(component: "plugin-output")
-            let pluginScriptRunner = DefaultPluginScriptRunner(cacheDir: pluginCacheDir, toolchain: ToolchainConfiguration.default)
-            let target = try XCTUnwrap(package.targets.first{ $0.underlyingTarget == libraryTarget })
-            let invocationSucceeded = try tsc_await { pluginTarget.invoke(
-                action: .performCommand(
-                    targets: [ target ],
-                    arguments: ["veni", "vidi", "vici"]),
-                package: package,
-                buildEnvironment: BuildEnvironment(platform: .macOS, configuration: .debug),
-                scriptRunner: pluginScriptRunner,
-                outputDirectory: pluginOutputDir,
-                toolSearchDirectories: [UserToolchain.default.swiftCompilerPath.parentDirectory],
-                toolNamesToPaths: [:],
-                fileSystem: localFileSystem,
-                observabilityScope: observability.topScope,
-                callbackQueue: delegateQueue,
-                delegate: pluginDelegate,
-                completion: $0) }
-            
-            // Check the results.
-            XCTAssertTrue(invocationSucceeded)
-            let outputText = String(decoding: pluginDelegate.outputData, as: UTF8.self)
-            XCTAssertTrue(outputText.contains("Root package is MyPackage."), outputText)
-            XCTAssertTrue(outputText.contains("Found the swiftc tool"), outputText)
+            // Helper function to invoke a plugin with given input and to check its outputs.
+            func testCommand(
+                package: ResolvedPackage,
+                plugin pluginName: String,
+                targets targetNames: [String],
+                arguments: [String],
+                toolSearchDirectories: [AbsolutePath] = [UserToolchain.default.swiftCompilerPath.parentDirectory],
+                toolNamesToPaths: [String: AbsolutePath] = [:],
+                file: StaticString = #file,
+                line: UInt = #line,
+                expectFailure: Bool = false,
+                diagnosticsChecker: (DiagnosticsTestResult) throws -> Void
+            ) {
+                // Find the named plugin.
+                let plugins = package.targets.compactMap{ $0.underlyingTarget as? PluginTarget }
+                guard let plugin = plugins.first(where: { $0.name == pluginName }) else {
+                    return XCTFail("There is no plugin target named ‘\(pluginName)’")
+                }
+                XCTAssertTrue(plugin.type == .plugin, "Target \(plugin) isn’t a plugin")
+
+                // Find the named input targets to the plugin.
+                var targets: [ResolvedTarget] = []
+                for targetName in targetNames {
+                    guard let target = package.targets.first(where: { $0.underlyingTarget.name == targetName }) else {
+                        return XCTFail("There is no target named ‘\(targetName)’")
+                    }
+                    XCTAssertTrue(target.type != .plugin, "Target \(target) is a plugin")
+                    targets.append(target)
+                }
+
+                let pluginDir = tmpPath.appending(components: package.identity.description, plugin.name)
+                let scriptRunner = DefaultPluginScriptRunner(cacheDir: pluginDir.appending(component: "cache"), toolchain: ToolchainConfiguration.default)
+                let delegate = PluginDelegate(delegateQueue: delegateQueue)
+                do {
+                    let success = try tsc_await { plugin.invoke(
+                        action: .performCommand(targets: targets, arguments: arguments),
+                        package: package,
+                        buildEnvironment: BuildEnvironment(platform: .macOS, configuration: .debug),
+                        scriptRunner: scriptRunner,
+                        outputDirectory: pluginDir.appending(component: "output"),
+                        toolSearchDirectories: [UserToolchain.default.swiftCompilerPath.parentDirectory],
+                        toolNamesToPaths: [:],
+                        fileSystem: localFileSystem,
+                        observabilityScope: observability.topScope,
+                        callbackQueue: delegateQueue,
+                        delegate: delegate,
+                        completion: $0) }
+                    if expectFailure {
+                        XCTAssertFalse(success, "expected command to fail, but it succeeded", file: file, line: line)
+                    }
+                    else {
+                        XCTAssertTrue(success, "expected command to succeed, but it failed", file: file, line: line)
+                    }
+                }
+                catch {
+                    XCTFail("error \(String(describing: error))", file: file, line: line)
+                }
+                testDiagnostics(delegate.diagnostics, problemsOnly: false, file: file, line: line, handler: diagnosticsChecker)
+            }
+
+            // Invoke the command plugin that prints out various things it was given, and check them.
+            testCommand(package: package, plugin: "PluginPrintingInfo", targets: ["MyLibrary"], arguments: ["veni", "vidi", "vici"]) { output in
+                output.check(diagnostic: .equal("Root package is MyPackage."), severity: .info)
+                output.check(diagnostic: .and(.prefix("Found the swiftc tool"), .suffix(".")), severity: .info)
+            }
+
+            // Invoke the command plugin that throws an unhandled error at the top level.
+            testCommand(package: package, plugin: "PluginFailingWithError", targets: [], arguments: [], expectFailure: true) { output in
+                output.check(diagnostic: .equal("This text should appear before the error."), severity: .info)
+                output.check(diagnostic: .equal("Houston, we have a problem."), severity: .error)
+
+            }
+            // Invoke the command plugin that exits with code 1 without returning an error.
+            testCommand(package: package, plugin: "PluginFailingWithoutError", targets: [], arguments: [], expectFailure: true) { output in
+                output.check(diagnostic: .equal("This text should appear before we exit."), severity: .info)
+                output.check(diagnostic: .equal("Plugin ended with exit code 1"), severity: .error)
+            }
         }
     }
 }