Merge pull request #66700 from hamishknight/interface-error-5.9

Author: hamishknight

include/swift/AST/DiagnosticEngine.h

@@ -1449,6 +1449,22 @@ namespace swift {
     }
   };
 
+  /// A RAII object that adds and removes a diagnostic consumer from an engine.
+  class DiagnosticConsumerRAII final {
+    DiagnosticEngine &Diags;
+    DiagnosticConsumer &Consumer;
+
+  public:
+    DiagnosticConsumerRAII(DiagnosticEngine &diags,
+                           DiagnosticConsumer &consumer)
+        : Diags(diags), Consumer(consumer) {
+      Diags.addConsumer(Consumer);
+    }
+    ~DiagnosticConsumerRAII() {
+      Diags.removeConsumer(Consumer);
+    }
+  };
+
   inline void
   DiagnosticEngine::diagnoseWithNotes(InFlightDiagnostic parentDiag,
                                       llvm::function_ref<void(void)> builder) {

test/SourceKit/InterfaceGen/error_clang_module.swift

@@ -0,0 +1,43 @@
+// RUN: %empty-directory(%t/Inputs)
+// RUN: %empty-directory(%t/Inputs/objcfail)
+// RUN: split-file %s %t/Inputs
+
+//--- objcfail/objcfail.h
+
+#ifdef FAIL
+#error some error from Clang module
+
+// We only record the first error emitted, so we ignore this one.
+#error another error from Clang module
+#endif
+
+void foo(void);
+
+//--- objcfail/module.modulemap
+
+module ObjCFail {
+  header "objcfail.h"
+  export *
+}
+
+//--- Library.swift
+
+import ObjCFail
+
+// First try printing the interface of the Clang module directly.
+
+// RUN: %sourcekitd-test -req=interface-gen -module ObjCFail -- -I %t/Inputs/objcfail -target %target-triple %s | %FileCheck --check-prefix DIRECT-SUCCESS %s
+// DIRECT-SUCCESS: public func foo()
+
+// RUN: not %sourcekitd-test -req=interface-gen -module ObjCFail -- -Xcc -DFAIL -I %t/Inputs/objcfail -target %target-triple %s 2>&1 | %FileCheck --check-prefix DIRECT-FAIL %s
+// DIRECT-FAIL: Could not load module: ObjCFail (could not build {{Objective-C|C}} module 'ObjCFail', some error from Clang module)
+
+// Now try doing it transitively
+
+// RUN: %target-swift-frontend -emit-module %t/Inputs/Library.swift -I %t/Inputs/objcfail -module-name Library -o %t
+
+// RUN: %sourcekitd-test -req=interface-gen -module Library -- -I %t -target %target-triple %s | %FileCheck --check-prefix TRANSITIVE-SUCCESS %s
+// TRANSITIVE-SUCCESS: import ObjCFail
+
+// RUN: not %sourcekitd-test -req=interface-gen -module Library -- -Xcc -DFAIL -I %t -target %target-triple %s 2>&1 | %FileCheck --check-prefix TRANSITIVE-FAIL %s
+// TRANSITIVE-FAIL: Could not load module: Library (could not build {{Objective-C|C}} module 'ObjCFail', some error from Clang module)

test/SourceKit/InterfaceGen/error_swift_module.swift

@@ -0,0 +1,58 @@
+// RUN: %empty-directory(%t/Inputs)
+// RUN: split-file %s %t/Inputs
+
+//--- Transitive.swift
+
+public func foo() {}
+
+//--- Library.swift
+
+import Transitive
+
+//--- LibraryWrong.swift
+
+import WrongName
+
+//--- LibraryNonExistant.swift
+
+import NonExistant
+
+// RUN: %target-swift-frontend -emit-module %t/Inputs/Transitive.swift -module-name Transitive -o %t/WrongName.swiftmodule
+// RUN: %target-swift-frontend -emit-module %t/Inputs/Transitive.swift -module-name Transitive -o %t/Transitive.swiftmodule
+
+// First try printing the interface of the Transitive module directly.
+
+// RUN: %sourcekitd-test -req=interface-gen -module Transitive -- -I %t -target %target-triple %s | %FileCheck --check-prefix DIRECT-SUCCESS %s
+// DIRECT-SUCCESS: public func foo()
+
+// RUN: not %sourcekitd-test -req=interface-gen -module WrongName -- -I %t -target %target-triple %s 2>&1 | %FileCheck --check-prefix DIRECT-FAIL %s
+// DIRECT-FAIL: Could not load module: WrongName (cannot load module 'Transitive' as 'WrongName')
+
+// Now try doing it transitively
+
+// First undo the WrongName module
+// RUN: %target-swift-frontend -emit-module %t/Inputs/Transitive.swift -module-name WrongName -o %t/WrongName.swiftmodule
+
+// RUN: %target-swift-frontend -emit-module %t/Inputs/Library.swift -I %t -module-name Library -o %t
+// RUN: %target-swift-frontend -emit-module %t/Inputs/LibraryWrong.swift -I %t -module-name LibraryWrong -o %t
+
+// Then redo the WrongName module
+// RUN: %target-swift-frontend -emit-module %t/Inputs/Transitive.swift -module-name Transitive -o %t/WrongName.swiftmodule
+
+// RUN: %sourcekitd-test -req=interface-gen -module Library -- -I %t -target %target-triple %s | %FileCheck --check-prefix TRANSITIVE-SUCCESS %s
+// TRANSITIVE-SUCCESS: import Transitive
+
+// RUN: not %sourcekitd-test -req=interface-gen -module LibraryWrong -- -I %t -target %target-triple %s 2>&1 | %FileCheck --check-prefix TRANSITIVE-FAIL %s
+// TRANSITIVE-FAIL: Could not load module: LibraryWrong (cannot load module 'Transitive' as 'WrongName')
+
+// Try a non-existant module
+
+// RUN: not %sourcekitd-test -req=interface-gen -module NonExistant -- -I %t -target %target-triple %s 2>&1 | %FileCheck --check-prefix DIRECT-NONEXISTANT %s
+// DIRECT-NONEXISTANT: Could not load module: NonExistant
+
+// RUN: %target-swift-frontend -emit-module %t/Inputs/Transitive.swift -module-name NonExistant -o %t
+// RUN: %target-swift-frontend -emit-module %t/Inputs/LibraryNonExistant.swift -module-name LibraryNonExistant -I %t -o %t
+// RUN: rm -rf %t/NonExistant.swiftmodule
+
+// RUN: not %sourcekitd-test -req=interface-gen -module LibraryNonExistant -- -I %t -target %target-triple %s 2>&1 | %FileCheck --check-prefix TRANSITIVE-NONEXISTANT %s
+// TRANSITIVE-NONEXISTANT: Could not load module: LibraryNonExistant (missing required module 'NonExistant')

tools/SourceKit/lib/SwiftLang/SwiftEditorInterfaceGen.cpp

@@ -265,6 +265,46 @@ static void reportSemanticAnnotations(const SourceTextInfo &IFaceInfo,
   }
 }
 
+namespace {
+/// A diagnostic consumer that picks up module loading errors.
+class ModuleLoadingErrorConsumer final : public DiagnosticConsumer {
+  llvm::SmallVector<std::string, 2> DiagMessages;
+
+  void handleDiagnostic(SourceManager &SM,
+                        const DiagnosticInfo &Info) override {
+    // Only record errors for now. In the future it might be useful to pick up
+    // some notes, but some notes are just noise.
+    if (Info.Kind != DiagnosticKind::Error)
+      return;
+
+    std::string Message;
+    {
+      llvm::raw_string_ostream Out(Message);
+      DiagnosticEngine::formatDiagnosticText(Out, Info.FormatString,
+                                             Info.FormatArgs);
+    }
+    // We're only interested in the first and last errors. For a clang module
+    // failure, the first error will be the reason why the module failed to
+    // load, and the last error will be a generic "could not build Obj-C module"
+    // error. For a Swift module, we'll typically only emit one error.
+    //
+    // NOTE: Currently when loading transitive dependencies for a Swift module,
+    // we'll only diagnose the root failure, and not record the error for the
+    // top-level module failure, as we stop emitting errors after a fatal error
+    // has been recorded. This is currently fine for our use case though, as
+    // we already include the top-level module name in the error we hand back.
+    if (DiagMessages.size() < 2) {
+      DiagMessages.emplace_back(std::move(Message));
+    } else {
+      DiagMessages.back() = std::move(Message);
+    }
+  }
+
+public:
+  ArrayRef<std::string> getDiagMessages() { return DiagMessages; }
+};
+} // end anonymous namespace
+
 static bool getModuleInterfaceInfo(ASTContext &Ctx,
                                    StringRef ModuleName,
                                    Optional<StringRef> Group,
@@ -280,21 +320,35 @@ static bool getModuleInterfaceInfo(ASTContext &Ctx,
     return true;
   }
 
-  // Get the (sub)module to generate.
-  Mod = Ctx.getModuleByName(ModuleName);
-  if (!Mod) {
-    ErrMsg = "Could not load module: ";
-    ErrMsg += ModuleName;
-    return true;
-  }
-  if (Mod->failedToLoad()) {
-    // We might fail to load the underlying Clang module
-    // for a Swift overlay module like 'CxxStdlib', or a mixed-language
-    // framework. Make sure an error is reported in this case, so that we can
-    // either retry to load with C++ interoperability enabled, and if that
-    // fails, we can report this to the user.
-    ErrMsg = "Could not load underlying module for: ";
-    ErrMsg += ModuleName;
+  // Get the (sub)module to generate, recording the errors emitted.
+  ModuleLoadingErrorConsumer DiagConsumer;
+  {
+    DiagnosticConsumerRAII R(Ctx.Diags, DiagConsumer);
+    Mod = Ctx.getModuleByName(ModuleName);
+  }
+
+  // Check to see if we either couldn't find the module, or we failed to load
+  // it, and report an error message back that includes the diagnostics we
+  // collected, which should help pinpoint what the issue was. Note we do this
+  // even if `Mod` is null, as the clang importer currently returns nullptr
+  // when a module fails to load, and there may be interesting errors to
+  // collect there.
+  // Note that us failing here also means the caller may retry with e.g C++
+  // interoperability enabled.
+  if (!Mod || Mod->failedToLoad()) {
+    llvm::raw_string_ostream OS(ErrMsg);
+
+    OS << "Could not load module: ";
+    OS << ModuleName;
+    auto ModuleErrs = DiagConsumer.getDiagMessages();
+    if (!ModuleErrs.empty()) {
+      // We print the errors in reverse, as they are typically emitted in
+      // a bottom-up manner by module loading, and a top-down presentation
+      // makes more sense.
+      OS << " (";
+      llvm::interleaveComma(llvm::reverse(ModuleErrs), OS);
+      OS << ")";
+    }
     return true;
   }