Update the compiler arguments used for background AST builds

This fixes two issues:
1. The SwiftPM build system was setup without passing through whether it
   should prepare or not. This meant that we lost eg. the argument to
   allow compiler errors when building the AST (even though it was set
   when building the modules)
2. The compiler argument adjustment to remove harmful and unnecessary
   flags only applied to indexing arguments, not those passed to the AST
   builds

Resolves rdar://141508656.

Author: bnbarham

Contributor Documentation/Implementing a BSP server.md

@@ -29,7 +29,7 @@ If the build system loads the entire build graph during initialization, it may i
 
 ## Supporting background indexing
 
-To support background indexing, the build system must set `data.prepareProvider: true` in the `build/initialize` response and implement the `buildTarget/prepare` method.
+To support background indexing, the build system must set `data.prepareProvider: true` in the `build/initialize` response and implement the `buildTarget/prepare` method. The compiler options used to prepare a target should match those sent for `textDocument/sourceKitOptions` in order to avoid mismatches when loading modules.
 
 ## Optional methods
 

Sources/BuildSystemIntegration/BuildSystemManager.swift

@@ -325,7 +325,7 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
     }
   }
 
-  private var cachedSourceKitOptions = RequestCache<TextDocumentSourceKitOptionsRequest>()
+  private var cachedAdjustedSourceKitOptions = RequestCache<TextDocumentSourceKitOptionsRequest>()
 
   private var cachedBuildTargets = Cache<WorkspaceBuildTargetsRequest, [BuildTargetIdentifier: BuildTargetInfo]>()
 
@@ -529,7 +529,7 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
       } else {
         nil
       }
-    self.cachedSourceKitOptions.clear(isolation: self) { cacheKey in
+    self.cachedAdjustedSourceKitOptions.clear(isolation: self) { cacheKey in
       guard let updatedTargets else {
         // All targets might have changed
         return true
@@ -758,13 +758,22 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
       target: target,
       language: language
     )
-
-    let response = try await cachedSourceKitOptions.get(request, isolation: self) { request in
-      try await buildSystemAdapter.send(request)
+    let response = try await cachedAdjustedSourceKitOptions.get(request, isolation: self) { request in
+      let options = try await buildSystemAdapter.send(request)
+      switch language.semanticKind {
+      case .swift:
+        return options?.adjustArgsForSemanticSwiftFunctionality(fileToIndex: document)
+      case .clang:
+        return options?.adjustingArgsForSemanticClangFunctionality()
+      default:
+        return options
+      }
     }
+
     guard let response else {
       return nil
     }
+
     return FileBuildSettings(
       compilerArguments: response.compilerArguments,
       workingDirectory: response.workingDirectory,
@@ -1239,3 +1248,153 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
 private func isDescendant(_ selfPathComponents: [String], of otherPathComponents: [String]) -> Bool {
   return selfPathComponents.dropLast().starts(with: otherPathComponents)
 }
+
+fileprivate extension TextDocumentSourceKitOptionsResponse {
+  /// Adjust compiler arguments that were created for building to compiler arguments that should be used for indexing
+  /// or background AST builds.
+  ///
+  /// This removes compiler arguments that produce output files and adds arguments to eg. allow errors and index the
+  /// file.
+  func adjustArgsForSemanticSwiftFunctionality(fileToIndex: DocumentURI) -> TextDocumentSourceKitOptionsResponse {
+    let optionsToRemove: [CompilerCommandLineOption] = [
+      .flag("c", [.singleDash]),
+      .flag("disable-cmo", [.singleDash]),
+      .flag("emit-dependencies", [.singleDash]),
+      .flag("emit-module-interface", [.singleDash]),
+      .flag("emit-module", [.singleDash]),
+      .flag("emit-objc-header", [.singleDash]),
+      .flag("incremental", [.singleDash]),
+      .flag("no-color-diagnostics", [.singleDash]),
+      .flag("parseable-output", [.singleDash]),
+      .flag("save-temps", [.singleDash]),
+      .flag("serialize-diagnostics", [.singleDash]),
+      .flag("use-frontend-parseable-output", [.singleDash]),
+      .flag("validate-clang-modules-once", [.singleDash]),
+      .flag("whole-module-optimization", [.singleDash]),
+      .flag("experimental-skip-all-function-bodies", frontendName: "Xfrontend", [.singleDash]),
+      .flag("experimental-skip-non-inlinable-function-bodies", frontendName: "Xfrontend", [.singleDash]),
+      .flag("experimental-skip-non-exportable-decls", frontendName: "Xfrontend", [.singleDash]),
+      .flag("experimental-lazy-typecheck", frontendName: "Xfrontend", [.singleDash]),
+
+      .option("clang-build-session-file", [.singleDash], [.separatedBySpace]),
+      .option("emit-module-interface-path", [.singleDash], [.separatedBySpace]),
+      .option("emit-module-path", [.singleDash], [.separatedBySpace]),
+      .option("emit-objc-header-path", [.singleDash], [.separatedBySpace]),
+      .option("emit-package-module-interface-path", [.singleDash], [.separatedBySpace]),
+      .option("emit-private-module-interface-path", [.singleDash], [.separatedBySpace]),
+      .option("num-threads", [.singleDash], [.separatedBySpace]),
+      // Technically, `-o` and the output file don't need to be separated by a space. Eg. `swiftc -oa file.swift` is
+      // valid and will write to an output file named `a`.
+      // We can't support that because the only way to know that `-output-file-map` is a different flag and not an option
+      // to write to an output file named `utput-file-map` is to know all compiler arguments of `swiftc`, which we don't.
+      .option("o", [.singleDash], [.separatedBySpace]),
+      .option("output-file-map", [.singleDash], [.separatedBySpace, .separatedByEqualSign]),
+    ]
+
+    var result: [String] = []
+    result.reserveCapacity(compilerArguments.count)
+    var iterator = compilerArguments.makeIterator()
+    while let argument = iterator.next() {
+      switch optionsToRemove.firstMatch(for: argument) {
+      case .removeOption:
+        continue
+      case .removeOptionAndNextArgument:
+        _ = iterator.next()
+        continue
+      case .removeOptionAndPreviousArgument(let name):
+        if let previousArg = result.last, previousArg.hasSuffix("-\(name)") {
+          _ = result.popLast()
+        }
+        continue
+      case nil:
+        break
+      }
+      result.append(argument)
+    }
+
+    result += [
+      // Avoid emitting the ABI descriptor, we don't need it
+      "-Xfrontend", "-empty-abi-descriptor",
+    ]
+
+    result += supplementalClangIndexingArgs.flatMap { ["-Xcc", $0] }
+
+    return TextDocumentSourceKitOptionsResponse(compilerArguments: result, workingDirectory: workingDirectory)
+  }
+
+  /// Adjust compiler arguments that were created for building to compiler arguments that should be used for indexing
+  /// or background AST builds.
+  ///
+  /// This removes compiler arguments that produce output files and adds arguments to eg. typecheck only.
+  func adjustingArgsForSemanticClangFunctionality() -> TextDocumentSourceKitOptionsResponse {
+    let optionsToRemove: [CompilerCommandLineOption] = [
+      // Disable writing of a depfile
+      .flag("M", [.singleDash]),
+      .flag("MD", [.singleDash]),
+      .flag("MMD", [.singleDash]),
+      .flag("MG", [.singleDash]),
+      .flag("MM", [.singleDash]),
+      .flag("MV", [.singleDash]),
+      // Don't create phony targets
+      .flag("MP", [.singleDash]),
+      // Don't write out compilation databases
+      .flag("MJ", [.singleDash]),
+      // Don't compile
+      .flag("c", [.singleDash]),
+
+      .flag("fmodules-validate-once-per-build-session", [.singleDash]),
+
+      // Disable writing of a depfile
+      .option("MT", [.singleDash], [.noSpace, .separatedBySpace]),
+      .option("MF", [.singleDash], [.noSpace, .separatedBySpace]),
+      .option("MQ", [.singleDash], [.noSpace, .separatedBySpace]),
+
+      // Don't write serialized diagnostic files
+      .option("serialize-diagnostics", [.singleDash, .doubleDash], [.separatedBySpace]),
+
+      .option("fbuild-session-file", [.singleDash], [.separatedByEqualSign]),
+    ]
+
+    var result: [String] = []
+    result.reserveCapacity(compilerArguments.count)
+    var iterator = compilerArguments.makeIterator()
+    while let argument = iterator.next() {
+      switch optionsToRemove.firstMatch(for: argument) {
+      case .removeOption:
+        continue
+      case .removeOptionAndNextArgument:
+        _ = iterator.next()
+        continue
+      case .removeOptionAndPreviousArgument(let name):
+        if let previousArg = result.last, previousArg.hasSuffix("-\(name)") {
+          _ = result.popLast()
+        }
+        continue
+      case nil:
+        break
+      }
+      result.append(argument)
+    }
+    result += supplementalClangIndexingArgs
+    result.append(
+      "-fsyntax-only"
+    )
+    return TextDocumentSourceKitOptionsResponse(compilerArguments: result, workingDirectory: workingDirectory)
+  }
+}
+
+fileprivate let supplementalClangIndexingArgs: [String] = [
+  // Retain extra information for indexing
+  "-fretain-comments-from-system-headers",
+  // Pick up macro definitions during indexing
+  "-Xclang", "-detailed-preprocessing-record",
+
+  // libclang uses 'raw' module-format. Match it so we can reuse the module cache and PCHs that libclang uses.
+  "-Xclang", "-fmodule-format=raw",
+
+  // Be less strict - we want to continue and typecheck/index as much as possible
+  "-Xclang", "-fallow-pch-with-compiler-errors",
+  "-Xclang", "-fallow-pcm-with-compiler-errors",
+  "-Wno-non-modular-include-in-framework-module",
+  "-Wno-incomplete-umbrella",
+]

Sources/BuildSystemIntegration/CMakeLists.txt

@@ -9,6 +9,7 @@ add_library(BuildSystemIntegration STATIC
   BuiltInBuildSystem.swift
   BuiltInBuildSystemAdapter.swift
   CompilationDatabase.swift
+  CompilerCommandLineOption.swift
   DetermineBuildSystem.swift
   ExternalBuildSystemAdapter.swift
   FallbackBuildSettings.swift

Sources/SemanticIndex/CompilerCommandLineOption.swift renamed to Sources/BuildSystemIntegration/CompilerCommandLineOption.swift

@@ -12,14 +12,18 @@
 
 package struct CompilerCommandLineOption {
   /// Return value of `matches(argument:)`.
-  package enum Match {
+  package enum Match: Equatable {
     /// The `CompilerCommandLineOption` matched the command line argument. The next element in the command line is a
     /// separate argument and should not be removed.
     case removeOption
 
     /// The `CompilerCommandLineOption` matched the command line argument. The next element in the command line is an
     /// argument to this option and should be removed as well.
     case removeOptionAndNextArgument
+
+    /// The `CompilerCommandLineOption` matched the command line argument. The previous element in the command line is
+    /// a prefix to this argument and should be removed if it matches `name`.
+    case removeOptionAndPreviousArgument(name: String)
   }
 
   package enum DashSpelling {
@@ -39,16 +43,28 @@ package struct CompilerCommandLineOption {
   /// The name of the option, without any preceeding `-` or `--`.
   private let name: String
 
+  /// The name of the argument that prefixes this flag, without any preceeding `-` or `--` (eg. `Xfrontend`/`Xclang`).
+  private let frontendName: String?
+
   /// Whether the option can be spelled with one or two dashes.
   private let dashSpellings: [DashSpelling]
 
   /// The ways that arguments can specified after the option. Empty if the option is a flag that doesn't take any
   /// argument.
   private let argumentStyles: [ArgumentStyles]
 
-  package static func flag(_ name: String, _ dashSpellings: [DashSpelling]) -> CompilerCommandLineOption {
+  package static func flag(
+    _ name: String,
+    frontendName: String? = nil,
+    _ dashSpellings: [DashSpelling]
+  ) -> CompilerCommandLineOption {
     precondition(!dashSpellings.isEmpty)
-    return CompilerCommandLineOption(name: name, dashSpellings: dashSpellings, argumentStyles: [])
+    return CompilerCommandLineOption(
+      name: name,
+      frontendName: frontendName,
+      dashSpellings: dashSpellings,
+      argumentStyles: []
+    )
   }
 
   package static func option(
@@ -58,10 +74,29 @@ package struct CompilerCommandLineOption {
   ) -> CompilerCommandLineOption {
     precondition(!dashSpellings.isEmpty)
     precondition(!argumentStyles.isEmpty)
-    return CompilerCommandLineOption(name: name, dashSpellings: dashSpellings, argumentStyles: argumentStyles)
+    return CompilerCommandLineOption(
+      name: name,
+      frontendName: nil,
+      dashSpellings: dashSpellings,
+      argumentStyles: argumentStyles
+    )
   }
 
   package func matches(argument: String) -> Match? {
+    let match = matchesIgnoringFrontend(argument: argument)
+    guard let match, let frontendName else {
+      return match
+    }
+
+    switch match {
+    case .removeOption:
+      return .removeOptionAndPreviousArgument(name: frontendName)
+    default:
+      return match
+    }
+  }
+
+  private func matchesIgnoringFrontend(argument: String) -> Match? {
     let argumentName: Substring
     if argument.hasPrefix("--") {
       if dashSpellings.contains(.doubleDash) {

Sources/BuildSystemIntegration/SwiftPMBuildSystem.swift

@@ -287,7 +287,8 @@ package actor SwiftPMBuildSystem: BuiltInBuildSystem {
       configuration: buildConfiguration,
       toolchain: destinationSwiftPMToolchain,
       triple: destinationSDK.targetTriple,
-      flags: buildFlags
+      flags: buildFlags,
+      prepareForIndexing: options.backgroundPreparationModeOrDefault.toSwiftPMPreparation
     )
 
     packageLoadingQueue.async {
@@ -709,4 +710,17 @@ fileprivate extension URL {
   }
 }
 
+fileprivate extension SourceKitLSPOptions.BackgroundPreparationMode {
+  var toSwiftPMPreparation: BuildParameters.PrepareForIndexingMode {
+    switch self {
+    case .build:
+      return .off
+    case .noLazy:
+      return .noLazy
+    case .enabled:
+      return .on
+    }
+  }
+}
+
 #endif

Sources/LanguageServerProtocolExtensions/CMakeLists.txt

@@ -3,7 +3,7 @@ add_library(LanguageServerProtocolExtensions STATIC
   Connection+Send.swift
   DocumentURI+CustomLogStringConvertible.swift
   DocumentURI+symlinkTarget.swift
-  Language+InferredFromFileExtension.swift
+  Language+Inference.swift
   LocalConnection.swift
   QueueBasedMessageHandler.swift
   RequestAndReply.swift

Sources/LanguageServerProtocolExtensions/Language+InferredFromFileExtension.swift renamed to Sources/LanguageServerProtocolExtensions/Language+Inference.swift

@@ -19,6 +19,22 @@ import LanguageServerProtocol
 #endif
 
 extension Language {
+  package enum SemanticKind {
+    case clang
+    case swift
+  }
+
+  package var semanticKind: SemanticKind? {
+    switch self {
+    case .swift:
+      return .swift
+    case .c, .cpp, .objective_c, .objective_cpp:
+      return .clang
+    default:
+      return nil
+    }
+  }
+
   package init?(inferredFromFileExtension uri: DocumentURI) {
     // URL.pathExtension is only set for file URLs but we want to also infer a file extension for non-file URLs like
     // untitled:file.cpp

Sources/SemanticIndex/CMakeLists.txt

@@ -1,7 +1,6 @@
 
 add_library(SemanticIndex STATIC
   CheckedIndex.swift
-  CompilerCommandLineOption.swift
   IndexTaskDescription.swift
   IndexHooks.swift
   PreparationTaskDescription.swift