Merge pull request #1045 from ahoppen/ahoppen/address-reducer-review-comments

Address review comments from #1037

Author: ahoppen

Package.swift

@@ -54,17 +54,20 @@ let package = Package(
       dependencies: []
     ),
 
-    // MARK: Csourcekitd:
+    // MARK: Csourcekitd
     // C modules wrapper for sourcekitd.
     .target(
       name: "Csourcekitd",
       dependencies: [],
       exclude: ["CMakeLists.txt"]
     ),
 
+    // MARK: Diagnose
+
     .target(
       name: "Diagnose",
       dependencies: [
+        "LSPLogging",
         "SourceKitD",
         "SKCore",
         .product(name: "ArgumentParser", package: "swift-argument-parser"),
@@ -75,6 +78,19 @@ let package = Package(
       exclude: ["CMakeLists.txt"]
     ),
 
+    .testTarget(
+      name: "DiagnoseTests",
+      dependencies: [
+        "Diagnose",
+        "LSPLogging",
+        "LSPTestSupport",
+        "SourceKitD",
+        "SKCore",
+        "SKTestSupport",
+        .product(name: "SwiftToolsSupport-auto", package: "swift-tools-support-core"),
+      ]
+    ),
+
     // MARK: LanguageServerProtocol
     // The core LSP types, suitable for any LSP implementation.
     .target(

Sources/Diagnose/CMakeLists.txt

@@ -1,14 +1,15 @@
 add_library(Diagnose STATIC
   CommandLineArgumentsReducer.swift
   DiagnoseCommand.swift
-  FileReducer.swift
   FixItApplier.swift
   OSLogScraper.swift
   ReductionError.swift
   ReproducerBundle.swift
   RequestInfo.swift
+  SourceKitD+RunWithYaml.swift
   SourceKitDRequestExecutor.swift
-  SourcekitdRequestCommand.swift)
+  SourcekitdRequestCommand.swift
+  SourceReducer.swift)
 
 set_target_properties(Diagnose PROPERTIES
   INTERFACE_INCLUDE_DIRECTORIES ${CMAKE_Swift_MODULE_DIRECTORY})

Sources/Diagnose/CommandLineArgumentsReducer.swift

@@ -12,8 +12,19 @@
 
 import Foundation
 
+// MARK: - Entry point
+
+extension RequestInfo {
+  func reduceCommandLineArguments(using executor: SourceKitRequestExecutor) async throws -> RequestInfo {
+    let reducer = CommandLineArgumentReducer(sourcekitdExecutor: executor)
+    return try await reducer.run(initialRequestInfo: self)
+  }
+}
+
+// MARK: - FileProducer
+
 /// Reduces the compiler arguments needed to reproduce a sourcekitd crash.
-class CommandLineArgumentReducer {
+fileprivate class CommandLineArgumentReducer {
   /// The executor that is used to run a sourcekitd request and check whether it
   /// still crashes.
   private let sourcekitdExecutor: SourceKitRequestExecutor
@@ -23,7 +34,11 @@ class CommandLineArgumentReducer {
 
   init(sourcekitdExecutor: SourceKitRequestExecutor) {
     self.sourcekitdExecutor = sourcekitdExecutor
-    temporarySourceFile = FileManager.default.temporaryDirectory.appendingPathComponent("reduce.swift")
+    temporarySourceFile = FileManager.default.temporaryDirectory.appendingPathComponent("reduce-\(UUID()).swift")
+  }
+
+  deinit {
+    try? FileManager.default.removeItem(at: temporarySourceFile)
   }
 
   func logSuccessfulReduction(_ requestInfo: RequestInfo) {

Sources/Diagnose/DiagnoseCommand.swift

@@ -26,13 +26,13 @@ public struct DiagnoseCommand: AsyncParsableCommand {
   @Option(
     name: .customLong("request-file"),
     help:
-      "Path to a sourcekitd request. If not specified, the command will look for crashed sourcekitd requests and have been logged to OSLog"
+      "Path to a sourcekitd request. If not specified, the command will look for crashed sourcekitd requests that have been logged to OSLog"
   )
   var sourcekitdRequestPath: String?
 
   @Option(
     name: .customLong("os-log-history"),
-    help: "If now request file is passed, how many minutes of OS Log history should be scraped for a crash."
+    help: "If no request file is passed, how many minutes of OS Log history should be scraped for a crash."
   )
   var osLogScrapeDuration: Int = 60
 
@@ -88,45 +88,53 @@ public struct DiagnoseCommand: AsyncParsableCommand {
       throw ReductionError("Unable to find sourcekitd.framework")
     }
 
+    var reproducerBundle: URL?
     for (name, requestInfo) in try requestInfos() {
       print("-- Diagnosing \(name)")
       do {
-        var requestInfo = requestInfo
-        var nspredicate: NSPredicate? = nil
-        #if canImport(Darwin)
-        if let predicate {
-          nspredicate = NSPredicate(format: predicate)
-        }
-        #endif
-        let executor = SourceKitRequestExecutor(
-          sourcekitd: URL(fileURLWithPath: sourcekitd),
-          reproducerPredicate: nspredicate
-        )
-        let fileReducer = FileReducer(sourcekitdExecutor: executor)
-        requestInfo = try await fileReducer.run(initialRequestInfo: requestInfo)
-
-        let commandLineReducer = CommandLineArgumentReducer(sourcekitdExecutor: executor)
-        requestInfo = try await commandLineReducer.run(initialRequestInfo: requestInfo)
-
-        let reproducerBundle = try makeReproducerBundle(for: requestInfo)
-
-        print("----------------------------------------")
-        print(
-          "Reduced SourceKit crash and created a bundle that contains information to reproduce the issue at the following path."
-        )
-        print("Please file an issue at https://github.com/apple/sourcekit-lsp/issues/new and attach this bundle")
-        print()
-        print(reproducerBundle.path)
-
-        // We have found a reproducer. Stop. Looking further probably won't help because other crashes are likely the same cause.
-        return
+        reproducerBundle = try await reduce(requestInfo: requestInfo, sourcekitd: sourcekitd)
+        // If reduce didn't throw, we have found a reproducer. Stop.
+        // Looking further probably won't help because other crashes are likely the same cause.
+        break
       } catch {
         // Reducing this request failed. Continue reducing the next one, maybe that one succeeds.
         print(error)
       }
     }
 
-    print("No reducible crashes found")
-    throw ExitCode(1)
+    guard let reproducerBundle else {
+      print("No reducible crashes found")
+      throw ExitCode(1)
+    }
+    print(
+      """
+        ----------------------------------------
+        Reduced SourceKit issue and created a bundle that contains a reduced sourcekitd request exhibiting the issue
+        and all the files referenced from the request.
+        The information in this bundle should be sufficient to reproduce the issue.
+
+        Please file an issue at https://github.com/apple/sourcekit-lsp/issues/new and attach the bundle located at
+        \(reproducerBundle.path)
+      """
+    )
+
+  }
+
+  private func reduce(requestInfo: RequestInfo, sourcekitd: String) async throws -> URL {
+    var requestInfo = requestInfo
+    var nspredicate: NSPredicate? = nil
+    #if canImport(Darwin)
+    if let predicate {
+      nspredicate = NSPredicate(format: predicate)
+    }
+    #endif
+    let executor = OutOfProcessSourceKitRequestExecutor(
+      sourcekitd: URL(fileURLWithPath: sourcekitd),
+      reproducerPredicate: nspredicate
+    )
+    requestInfo = try await requestInfo.reduceInputFile(using: executor)
+    requestInfo = try await requestInfo.reduceCommandLineArguments(using: executor)
+
+    return try makeReproducerBundle(for: requestInfo)
   }
 }

Sources/Diagnose/RequestInfo.swift

@@ -14,22 +14,25 @@ import Foundation
 import RegexBuilder
 
 /// All the information necessary to replay a sourcektid request.
-struct RequestInfo {
+@_spi(Testing)
+public struct RequestInfo {
   /// The JSON request object. Contains the following dynamic placeholders:
   ///  - `$OFFSET`: To be replaced by `offset` before running the request
   ///  - `$FILE`: Will be replaced with a path to the file that contains the reduced source code.
-  ///  - `$COMPILERARGS`: Will be replaced by the compiler arguments of the request
+  ///  - `$COMPILER_ARGS`: Will be replaced by the compiler arguments of the request
   var requestTemplate: String
 
   /// The offset at which the sourcekitd request should be run. Replaces the
   /// `$OFFSET` placeholder in the request template.
   var offset: Int
 
-  /// The compiler arguments of the request. Replaces the `$COMPILERARGS`placeholder in the request template.
-  var compilerArgs: [String]
+  /// The compiler arguments of the request. Replaces the `$COMPILER_ARGS`placeholder in the request template.
+  @_spi(Testing)
+  public var compilerArgs: [String]
 
   /// The contents of the file that the sourcekitd request operates on.
-  var fileContents: String
+  @_spi(Testing)
+  public var fileContents: String
 
   func request(for file: URL) throws -> String {
     let encoder = JSONEncoder()
@@ -42,12 +45,13 @@ struct RequestInfo {
     return
       requestTemplate
       .replacingOccurrences(of: "$OFFSET", with: String(offset))
-      .replacingOccurrences(of: "$COMPILERARGS", with: compilerArgs)
+      .replacingOccurrences(of: "$COMPILER_ARGS", with: compilerArgs)
       .replacingOccurrences(of: "$FILE", with: file.path)
 
   }
 
-  init(requestTemplate: String, offset: Int, compilerArgs: [String], fileContents: String) {
+  @_spi(Testing)
+  public init(requestTemplate: String, offset: Int, compilerArgs: [String], fileContents: String) {
     self.requestTemplate = requestTemplate
     self.offset = offset
     self.compilerArgs = compilerArgs
@@ -57,7 +61,8 @@ struct RequestInfo {
   /// Creates `RequestInfo` from the contents of the JSON sourcekitd request at `requestPath`.
   ///
   /// The contents of the source file are read from disk.
-  init(request: String) throws {
+  @_spi(Testing)
+  public init(request: String) throws {
     var requestTemplate = request
 
     // Extract offset
@@ -106,7 +111,7 @@ private func extractCompilerArguments(
   else {
     return (requestTemplate, [])
   }
-  let template = lines[...compilerArgsStartIndex] + ["$COMPILERARGS"] + lines[compilerArgsEndIndex...]
+  let template = lines[...compilerArgsStartIndex] + ["$COMPILER_ARGS"] + lines[compilerArgsEndIndex...]
   let compilerArgsJson = "[" + lines[(compilerArgsStartIndex + 1)..<compilerArgsEndIndex].joined(separator: "\n") + "]"
   let compilerArgs = try JSONDecoder().decode([String].self, from: compilerArgsJson)
   return (template.joined(separator: "\n"), compilerArgs)

Sources/Diagnose/SourceKitD+RunWithYaml.swift

@@ -0,0 +1,35 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2024 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+
+import SourceKitD
+
+extension SourceKitD {
+  /// Parse the request from YAML and execute it.
+  @_spi(Testing)
+  public func run(requestYaml: String) async throws -> SKDResponse {
+    let request = try requestYaml.cString(using: .utf8)?.withUnsafeBufferPointer { buffer in
+      var error: UnsafeMutablePointer<CChar>?
+      let req = api.request_create_from_yaml(buffer.baseAddress, &error)
+      if let error {
+        throw ReductionError("Failed to parse sourcekitd request from YAML: \(String(cString: error))")
+      }
+      precondition(req != nil)
+      return req
+    }
+    return await withCheckedContinuation { continuation in
+      var handle: sourcekitd_request_handle_t? = nil
+      api.send_request(request, &handle) { resp in
+        continuation.resume(returning: SKDResponse(resp, sourcekitd: self))
+      }
+    }
+  }
+}

Sources/Diagnose/SourceKitDRequestExecutor.swift

@@ -16,8 +16,9 @@ import SourceKitD
 import struct TSCBasic.AbsolutePath
 import class TSCBasic.Process
 
-/// The different states in which a sourcektid request can finish.
-enum SourceKitDRequestResult {
+/// The different states in which a sourcekitd request can finish.
+@_spi(Testing)
+public enum SourceKitDRequestResult {
   /// The request succeeded.
   case success(response: String)
 
@@ -41,8 +42,14 @@ fileprivate extension String {
   }
 }
 
+/// An executor that can run a sourcekitd request and indicate whether the request reprodes a specified issue.
+@_spi(Testing)
+public protocol SourceKitRequestExecutor {
+  func run(request requestString: String) async throws -> SourceKitDRequestResult
+}
+
 /// Runs `sourcekit-lsp run-sourcekitd-request` to check if a sourcekit-request crashes.
-struct SourceKitRequestExecutor {
+struct OutOfProcessSourceKitRequestExecutor: SourceKitRequestExecutor {
   /// The path to `sourcekitd.framework/sourcekitd`.
   private let sourcekitd: URL