[Macros] Improve macro plugin loading macro definition resolution diagnostics

include/swift/AST/DiagnosticsFrontend.def

@@ -536,9 +536,6 @@ REMARK(warning_in_access_notes_file,none,
        "ignored invalid content in access notes file: %0",
        (StringRef))
 
-WARNING(compiler_plugin_not_loaded,none,
-        "compiler plugin not loaded: %0; loader error: %1", (StringRef, StringRef))
-
 ERROR(dont_enable_interop_and_compat,none,
       "do not pass both '-enable-experimental-cxx-interop' and "
       "'-cxx-interoperability-mode'; remove '-enable-experimental-cxx-interop'", ())

include/swift/AST/DiagnosticsSema.def

@@ -7312,7 +7312,7 @@ ERROR(macro_undefined,PointsToFirstBadToken,
       "no macro named %0", (Identifier))
 ERROR(external_macro_not_found,none,
       "external macro implementation type '%0.%1' could not be found for "
-      "macro %2", (StringRef, StringRef, DeclName))
+      "macro %2; %3", (StringRef, StringRef, DeclName, StringRef))
 ERROR(macro_must_be_defined,none,
       "macro %0 requires a definition", (DeclName))
 ERROR(external_macro_outside_macro_definition,none,

include/swift/AST/MacroDefinition.h

@@ -18,6 +18,7 @@
 #ifndef SWIFT_AST_MACRO_DEFINITION_H
 #define SWIFT_AST_MACRO_DEFINITION_H
 
+#include "swift/Basic/StringExtras.h"
 #include "llvm/ADT/PointerUnion.h"
 
 namespace swift {
@@ -26,14 +27,25 @@ class ASTContext;
 
 /// A reference to an external macro definition that is understood by ASTGen.
 struct ExternalMacroDefinition {
-  enum class PluginKind {
+  enum class PluginKind : int8_t {
     InProcess = 0,
     Executable = 1,
+    Error = -1,
   };
   PluginKind kind;
   /// ASTGen's notion of an macro definition, which is opaque to the C++ part
-  /// of the compiler.
-  void *opaqueHandle = nullptr;
+  /// of the compiler. If 'kind' is 'PluginKind::Error', this is a C-string to
+  /// the error message
+  const void *opaqueHandle = nullptr;
+
+  static ExternalMacroDefinition error(NullTerminatedStringRef message) {
+    return ExternalMacroDefinition{PluginKind::Error,
+                                   static_cast<const void *>(message.data())};
+  }
+  bool isError() const { return kind == PluginKind::Error; }
+  NullTerminatedStringRef getErrorMessage() const {
+    return static_cast<const char *>(opaqueHandle);
+  }
 };
 
 /// A reference to an external macro.

include/swift/AST/PluginLoader.h

@@ -76,14 +76,15 @@ class PluginLoader {
   /// returns a nullptr.
   /// NOTE: This method is idempotent. If the plugin is already loaded, the same
   /// instance is simply returned.
-  LoadedLibraryPlugin *loadLibraryPlugin(llvm::StringRef path);
+  llvm::Expected<LoadedLibraryPlugin *> loadLibraryPlugin(llvm::StringRef path);
 
   /// Launch the specified executable plugin path resolving the path with the
   /// current VFS. If it fails to load the plugin, a diagnostic is emitted, and
   /// returns a nullptr.
   /// NOTE: This method is idempotent. If the plugin is already loaded, the same
   /// instance is simply returned.
-  LoadedExecutablePlugin *loadExecutablePlugin(llvm::StringRef path);
+  llvm::Expected<LoadedExecutablePlugin *>
+  loadExecutablePlugin(llvm::StringRef path);
 };
 
 } // namespace swift

include/swift/AST/PluginRegistry.h

@@ -33,11 +33,19 @@ class LoadedLibraryPlugin {
   /// Cache of loaded symbols.
   llvm::StringMap<void *> resolvedSymbols;
 
+  /// Path to the plugin library.
+  const std::string LibraryPath;
+
 public:
-  LoadedLibraryPlugin(void *handle) : handle(handle) {}
+  LoadedLibraryPlugin(void *handle, StringRef path)
+      : handle(handle), LibraryPath(path) {}
 
   /// Finds the address of the given symbol within the library.
   void *getAddressOfSymbol(const char *symbolName);
+
+  NullTerminatedStringRef getLibraryPath() {
+    return {LibraryPath.c_str(), LibraryPath.size()};
+  }
 };
 
 /// Represent a "resolved" executable plugin.

include/swift/AST/TypeCheckRequests.h

@@ -4238,34 +4238,57 @@ class ExpandSynthesizedMemberMacroRequest
 };
 
 /// Represent a loaded plugin either an in-process library or an executable.
-class LoadedCompilerPlugin {
-  llvm::PointerUnion<LoadedLibraryPlugin *, LoadedExecutablePlugin *> ptr;
+class CompilerPluginLoadResult {
+  enum class PluginKind : uint8_t {
+    Error = 0,
+    Library,
+    Executable,
+  };
+  PluginKind Kind;
+  void *opaqueHandle;
+
+  CompilerPluginLoadResult(PluginKind K, void *opaque)
+      : Kind(K), opaqueHandle(opaque) {}
 
 public:
-  LoadedCompilerPlugin(std::nullptr_t) : ptr(nullptr) {}
-  LoadedCompilerPlugin(LoadedLibraryPlugin *ptr) : ptr(ptr){};
-  LoadedCompilerPlugin(LoadedExecutablePlugin *ptr) : ptr(ptr){};
+  CompilerPluginLoadResult(LoadedLibraryPlugin *ptr)
+      : CompilerPluginLoadResult(PluginKind::Library, ptr){};
+  CompilerPluginLoadResult(LoadedExecutablePlugin *ptr)
+      : CompilerPluginLoadResult(PluginKind::Executable, ptr){};
+  static CompilerPluginLoadResult error(NullTerminatedStringRef message) {
+    return CompilerPluginLoadResult(PluginKind::Error,
+                                    const_cast<char *>(message.data()));
+  }
 
   LoadedLibraryPlugin *getAsLibraryPlugin() const {
-    return ptr.dyn_cast<LoadedLibraryPlugin *>();
+    if (Kind != PluginKind::Library)
+      return nullptr;
+    return static_cast<LoadedLibraryPlugin *>(opaqueHandle);
   }
   LoadedExecutablePlugin *getAsExecutablePlugin() const {
-    return ptr.dyn_cast<LoadedExecutablePlugin *>();
+    if (Kind != PluginKind::Executable)
+      return nullptr;
+    return static_cast<LoadedExecutablePlugin *>(opaqueHandle);
+  }
+  bool isError() const { return Kind == PluginKind::Error; }
+  NullTerminatedStringRef getErrorMessage() const {
+    assert(isError());
+    return static_cast<const char *>(opaqueHandle);
   }
 };
 
 class CompilerPluginLoadRequest
     : public SimpleRequest<CompilerPluginLoadRequest,
-                           LoadedCompilerPlugin(ASTContext *, Identifier),
+                           CompilerPluginLoadResult(ASTContext *, Identifier),
                            RequestFlags::Cached> {
 public:
   using SimpleRequest::SimpleRequest;
 
 private:
   friend SimpleRequest;
 
-  LoadedCompilerPlugin evaluate(Evaluator &evaluator, ASTContext *ctx,
-                                Identifier moduleName) const;
+  CompilerPluginLoadResult evaluate(Evaluator &evaluator, ASTContext *ctx,
+                                    Identifier moduleName) const;
 
 public:
   // Source location
@@ -4296,19 +4319,18 @@ class ExpandPeerMacroRequest
 /// Resolve an external macro given its module and type name.
 class ExternalMacroDefinitionRequest
     : public SimpleRequest<ExternalMacroDefinitionRequest,
-                           llvm::Optional<ExternalMacroDefinition>(
-                               ASTContext *, Identifier, Identifier),
+                           ExternalMacroDefinition(ASTContext *, Identifier,
+                                                   Identifier),
                            RequestFlags::Cached> {
 public:
   using SimpleRequest::SimpleRequest;
 
 private:
   friend SimpleRequest;
 
-  llvm::Optional<ExternalMacroDefinition> evaluate(
-      Evaluator &evaluator, ASTContext *ctx,
-      Identifier moduleName, Identifier typeName
-  ) const;
+  ExternalMacroDefinition evaluate(Evaluator &evaluator, ASTContext *ctx,
+                                   Identifier moduleName,
+                                   Identifier typeName) const;
 
 public:
   // Source location

include/swift/AST/TypeCheckerTypeIDZone.def

@@ -458,10 +458,10 @@ SWIFT_REQUEST(TypeChecker, MacroDefinitionRequest,
               MacroDefinition(MacroDecl *),
               Cached, NoLocationInfo)
 SWIFT_REQUEST(TypeChecker, CompilerPluginLoadRequest,
-              LoadedCompilerPlugin(ASTContext *, Identifier),
+              CompilerPluginLoadResult(ASTContext *, Identifier),
               Cached, NoLocationInfo)
 SWIFT_REQUEST(TypeChecker, ExternalMacroDefinitionRequest,
-              Optional<ExternalMacroDefinition>(ASTContext *, Identifier, Identifier),
+              ExternalMacroDefinition(ASTContext *, Identifier, Identifier),
               Cached, NoLocationInfo)
 SWIFT_REQUEST(TypeChecker, ExpandMacroExpansionDeclRequest,
               ArrayRef<Decl *>(MacroExpansionDecl *),

include/swift/Basic/StringExtras.h

@@ -21,9 +21,11 @@
 #include "swift/Basic/LLVM.h"
 #include "swift/Basic/OptionSet.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSet.h"
+#include "llvm/ADT/Twine.h"
 #include "llvm/Support/Allocator.h"
 #include <iterator>
 #include <string>
@@ -492,13 +494,21 @@ class NullTerminatedStringRef {
 
   /// Create a null-terminated string, copying \p Str into \p A .
   template <typename Allocator>
-  NullTerminatedStringRef(StringRef Str, Allocator &A) : Ref("") {
-    if (Str.empty())
+  NullTerminatedStringRef(llvm::Twine Str, Allocator &A) : Ref("") {
+    if (Str.isTriviallyEmpty())
       return;
+    if (Str.isSingleStringLiteral()) {
+      Ref = Str.getSingleStringRef();
+      return;
+    }
+    llvm::SmallString<0> stash;
+    auto _ref = Str.toStringRef(stash);
 
-    size_t size = Str.size();
-    char *memory = A.template Allocate<char>(size + 1);
-    memcpy(memory, Str.data(), size);
+    size_t size = _ref.size();
+    if (size == 0)
+      return;
+    char *memory = static_cast<char *>(A.Allocate(size + 1, alignof(char)));
+    memcpy(memory, _ref.data(), size);
     memory[size] = '\0';
     Ref = {memory, size};
   }

lib/AST/PluginLoader.cpp

@@ -158,13 +158,12 @@ PluginLoader::lookupPluginByModuleName(Identifier moduleName) {
   }
 }
 
-LoadedLibraryPlugin *PluginLoader::loadLibraryPlugin(StringRef path) {
+llvm::Expected<LoadedLibraryPlugin *>
+PluginLoader::loadLibraryPlugin(StringRef path) {
   auto fs = Ctx.SourceMgr.getFileSystem();
   SmallString<128> resolvedPath;
   if (auto err = fs->getRealPath(path, resolvedPath)) {
-    Ctx.Diags.diagnose(SourceLoc(), diag::compiler_plugin_not_loaded, path,
-                       err.message());
-    return nullptr;
+    return llvm::createStringError(err, err.message());
   }
 
   // Track the dependency.
@@ -174,21 +173,25 @@ LoadedLibraryPlugin *PluginLoader::loadLibraryPlugin(StringRef path) {
   // Load the plugin.
   auto plugin = getRegistry()->loadLibraryPlugin(resolvedPath);
   if (!plugin) {
-    Ctx.Diags.diagnose(SourceLoc(), diag::compiler_plugin_not_loaded, path,
-                       llvm::toString(plugin.takeError()));
-    return nullptr;
+    resolvedPath.push_back(0);
+    return llvm::handleErrors(
+        plugin.takeError(), [&](const llvm::ErrorInfoBase &err) {
+          return llvm::createStringError(
+              err.convertToErrorCode(),
+              "compiler plugin '%s' could not be loaded;  %s",
+              resolvedPath.data(), err.message().data());
+        });
   }
 
-  return plugin.get();
+  return plugin;
 }
 
-LoadedExecutablePlugin *PluginLoader::loadExecutablePlugin(StringRef path) {
+llvm::Expected<LoadedExecutablePlugin *>
+PluginLoader::loadExecutablePlugin(StringRef path) {
   auto fs = Ctx.SourceMgr.getFileSystem();
   SmallString<128> resolvedPath;
   if (auto err = fs->getRealPath(path, resolvedPath)) {
-    Ctx.Diags.diagnose(SourceLoc(), diag::compiler_plugin_not_loaded, path,
-                       err.message());
-    return nullptr;
+    return llvm::createStringError(err, err.message());
   }
 
   // Track the dependency.
@@ -198,10 +201,15 @@ LoadedExecutablePlugin *PluginLoader::loadExecutablePlugin(StringRef path) {
   // Load the plugin.
   auto plugin = getRegistry()->loadExecutablePlugin(resolvedPath);
   if (!plugin) {
-    Ctx.Diags.diagnose(SourceLoc(), diag::compiler_plugin_not_loaded, path,
-                       llvm::toString(plugin.takeError()));
-    return nullptr;
+    resolvedPath.push_back(0);
+    return llvm::handleErrors(
+        plugin.takeError(), [&](const llvm::ErrorInfoBase &err) {
+          return llvm::createStringError(
+              err.convertToErrorCode(),
+              "compiler plugin '%s' could not be loaded: %s",
+              resolvedPath.data(), err.message().data());
+        });
   }
 
-  return plugin.get();
+  return plugin;
 }

lib/AST/PluginRegistry.cpp

@@ -67,7 +67,7 @@ PluginRegistry::loadLibraryPlugin(StringRef path) {
   }
 #endif
 
-  storage = std::make_unique<LoadedLibraryPlugin>(lib);
+  storage = std::make_unique<LoadedLibraryPlugin>(lib, path);
   return storage.get();
 }
 

lib/ASTGen/Sources/ASTGen/PluginHost.swift

@@ -37,9 +37,8 @@ public func _initializePlugin(
     try plugin.initialize()
     return true
   } catch {
-    diagEngine?.diagnose(
-      message: "compiler plugin not loaded: '\(plugin.executableFilePath); failed to initialize",
-      severity: .warning)
+    // Don't care the actual error. Probably the plugin is completely broken.
+    // The failure is diagnosed in the caller.
     return false
   }
 }
@@ -59,16 +58,12 @@ func swift_ASTGen_pluginServerLoadLibraryPlugin(
   opaqueHandle: UnsafeMutableRawPointer,
   libraryPath: UnsafePointer<CChar>,
   moduleName: UnsafePointer<CChar>,
-  cxxDiagnosticEngine: UnsafeMutableRawPointer?
+  errorOut: UnsafeMutablePointer<BridgedString>?
 ) -> Bool {
   let plugin =  CompilerPlugin(opaqueHandle: opaqueHandle)
-  let diagEngine = PluginDiagnosticsEngine(cxxDiagnosticEngine: cxxDiagnosticEngine)
 
   if plugin.capability?.features.contains(.loadPluginLibrary) != true {
-    // This happens only if invalid plugin server was passed to `-external-plugin-path`.
-    diagEngine?.diagnose(
-      message: "compiler plugin not loaded: '\(libraryPath); invalid plugin server",
-      severity: .warning)
+    errorOut?.pointee = allocateBridgedString("compiler plugin not loaded: '\(libraryPath); invalid plugin server")
     return false
   }
   assert(plugin.capability?.features.contains(.loadPluginLibrary) == true)
@@ -82,12 +77,15 @@ func swift_ASTGen_pluginServerLoadLibraryPlugin(
     guard case .loadPluginLibraryResult(let loaded, let diagnostics) = result else {
       throw PluginError.invalidReponseKind
     }
-    diagEngine?.emit(diagnostics);
-    return loaded
+    if loaded {
+      assert(diagnostics.isEmpty)
+      return true
+    }
+    var errorMsgs = diagnostics.map({$0.message}).joined(separator: ", ");
+    errorOut?.pointee = allocateBridgedString(errorMsgs);
+    return false
   } catch {
-    diagEngine?.diagnose(
-      message: "compiler plugin not loaded: '\(libraryPath); \(error)",
-      severity: .warning)
+    errorOut?.pointee = allocateBridgedString("\(error)")
     return false
   }
 }

lib/Basic/Sandbox.cpp

@@ -30,7 +30,7 @@ static StringRef sandboxProfile(llvm::BumpPtrAllocator &Alloc) {
   // This is required to launch any processes (execve(2)).
   contents += "(allow process-exec*)\n";
 
-  return NullTerminatedStringRef(contents, Alloc);
+  return NullTerminatedStringRef(StringRef(contents), Alloc);
 }
 #endif