From a9f9bd13946f40dd3469fd5bf6ca3d26d3536373 Mon Sep 17 00:00:00 2001 From: Hamish Knight Date: Thu, 15 Jun 2023 23:55:07 +0100 Subject: [PATCH] [SourceKit] Record module loading errors when generating interfaces Record up to two errors emitted when we fail to load a module for interface generation, and include these errors in the message we pass back to the editor. This should help us better pin down the reason why interface generation failed. rdar://109511099 --- include/swift/AST/DiagnosticEngine.h | 16 ++++ .../InterfaceGen/error_clang_module.swift | 43 ++++++++++ .../InterfaceGen/error_swift_module.swift | 58 +++++++++++++ .../lib/SwiftLang/SwiftEditorInterfaceGen.cpp | 84 +++++++++++++++---- 4 files changed, 186 insertions(+), 15 deletions(-) create mode 100644 test/SourceKit/InterfaceGen/error_clang_module.swift create mode 100644 test/SourceKit/InterfaceGen/error_swift_module.swift diff --git a/include/swift/AST/DiagnosticEngine.h b/include/swift/AST/DiagnosticEngine.h index df9130b971694..083713d10af43 100644 --- a/include/swift/AST/DiagnosticEngine.h +++ b/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 builder) { diff --git a/test/SourceKit/InterfaceGen/error_clang_module.swift b/test/SourceKit/InterfaceGen/error_clang_module.swift new file mode 100644 index 0000000000000..b31501ab1a6c7 --- /dev/null +++ b/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) diff --git a/test/SourceKit/InterfaceGen/error_swift_module.swift b/test/SourceKit/InterfaceGen/error_swift_module.swift new file mode 100644 index 0000000000000..61ab865b16d58 --- /dev/null +++ b/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') diff --git a/tools/SourceKit/lib/SwiftLang/SwiftEditorInterfaceGen.cpp b/tools/SourceKit/lib/SwiftLang/SwiftEditorInterfaceGen.cpp index 993506166af39..80f7f1b17b556 100644 --- a/tools/SourceKit/lib/SwiftLang/SwiftEditorInterfaceGen.cpp +++ b/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 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 getDiagMessages() { return DiagMessages; } +}; +} // end anonymous namespace + static bool getModuleInterfaceInfo(ASTContext &Ctx, StringRef ModuleName, Optional 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; }