[Serialization] Report detected modularization breaks

The Swift compiler expects the context to remain stable between when a
module is built and loaded by a client. Usually the build system would
rebuild a module if a dependency changes, or the compiler would rebuilt
the module from a swiftinterface on a context change. However, such
changes are not always detected and in that case the compiler may crash
on an inconsistency in the context. We often see this when a clang
module is poorly modularized, the headers are modified in the SDK, or
some clang define change its API.

These are project issues that used to make the compiler crash, it
provided a poor experience and doesn't encourage the developer to fix
them by themselves. Instead, let's keep track of modularization issues
encountered during deserialization and report them as proper errors when
they trigger a fatal failure preventing compilation.

Author: xymus

include/swift/AST/DiagnosticsSema.def

@@ -872,6 +872,27 @@ ERROR(need_hermetic_seal_to_import_module,none,
       "current compilation does not have -experimental-hermetic-seal-at-link",
       (Identifier))
 
+ERROR(modularization_issue_decl_moved,Fatal,
+      "reference to %select{top-level|type}0 %1 broken by a context change; "
+      "%1 was expected to be in %2, but now a candidate is found only in %3",
+      (bool, DeclName, Identifier, Identifier))
+ERROR(modularization_issue_decl_type_changed,Fatal,
+      "reference to %select{top-level|type}0 %1 broken by a context change; "
+      "the details of %1 %select{from %2|}5 changed since building '%3'"
+      "%select{|, it was in %2 and is now found in %4}5",
+      (bool, DeclName, Identifier, StringRef, Identifier, bool))
+ERROR(modularization_issue_decl_not_found,Fatal,
+      "reference to %select{top-level|type}0 %1 broken by a context change; "
+      "%1 is not found, it was expected to be in %2",
+      (bool, DeclName, Identifier))
+
+NOTE(modularization_issue_side_effect_extension_error,none,
+     "could not deserialize extension",
+     ())
+NOTE(modularization_issue_side_effect_type_error,none,
+     "could not deserialize type for %0",
+     (DeclName))
+
 ERROR(reserved_member_name,none,
       "type member must not be named %0, since it would conflict with the"
       " 'foo.%1' expression", (DeclName, StringRef))

lib/Serialization/Deserialization.cpp

@@ -157,6 +157,8 @@ const char InvalidRecordKindError::ID = '\0';
 void InvalidRecordKindError::anchor() {}
 const char UnsafeDeserializationError::ID = '\0';
 void UnsafeDeserializationError::anchor() {}
+const char ModularizationError::ID = '\0';
+void ModularizationError::anchor() {}
 
 static llvm::Error consumeErrorIfXRefNonLoadedModule(llvm::Error &&error);
 
@@ -188,18 +190,99 @@ SourceLoc ModuleFile::getSourceLoc() const {
   return SourceMgr.getLocForBufferStart(*bufferID);
 }
 
+void
+ModularizationError::diagnose(const ModuleFile *MF,
+                              DiagnosticBehavior limit) const {
+  auto &ctx = MF->getContext();
+
+  auto diagnoseError = [&](Kind errorKind) {
+    switch (errorKind) {
+    case Kind::DeclMoved:
+      return ctx.Diags.diagnose(MF->getSourceLoc(), diag::modularization_issue_decl_moved,
+                                declIsType, name, expectedModuleName,
+                                foundModuleName);
+    case Kind::DeclKindChanged:
+      return
+        ctx.Diags.diagnose(MF->getSourceLoc(), diag::modularization_issue_decl_type_changed,
+                           declIsType, name, expectedModuleName,
+                           referencedFromModuleName, foundModuleName,
+                           foundModuleName != expectedModuleName);
+    case Kind::DeclNotFound:
+      return ctx.Diags.diagnose(MF->getSourceLoc(), diag::modularization_issue_decl_not_found,
+                                declIsType, name, expectedModuleName);
+    }
+    llvm_unreachable("Unhandled ModularizationError::Kind in switch.");
+  };
+
+  auto inFlight = diagnoseError(errorKind);
+  inFlight.limitBehavior(limit);
+  inFlight.flush();
+
+  // We could pass along the `path` information through notes.
+  // However, for a top-level decl a path would just duplicate the
+  // expected module name and the decl name from the diagnostic.
+}
+
+void TypeError::diagnose(const ModuleFile *MF) const {
+  MF->getContext().Diags.diagnose(MF->getSourceLoc(),
+                                  diag::modularization_issue_side_effect_type_error,
+                                  name);
+}
+
+void ExtensionError::diagnose(const ModuleFile *MF) const {
+  MF->getContext().Diags.diagnose(MF->getSourceLoc(),
+                       diag::modularization_issue_side_effect_extension_error);
+}
+
 llvm::Error ModuleFile::diagnoseFatal(llvm::Error error) const {
-  if (FileContext)
-    getContext().Diags.diagnose(SourceLoc(), diag::serialization_fatal,
-                                Core->Name);
+
+  auto &ctx = getContext();
+  if (FileContext) {
+    if (ctx.LangOpts.EnableDeserializationRecovery) {
+      // Attempt to report relevant errors as diagnostics.
+      // At this time, only ModularizationErrors are reported directly. They
+      // can get here either directly or as underlying causes to a TypeError or
+      // and ExtensionError.
+      auto handleModularizationError =
+        [&](const ModularizationError &modularError) -> llvm::Error {
+          modularError.diagnose(this);
+          return llvm::Error::success();
+        };
+      error = llvm::handleErrors(std::move(error),
+        handleModularizationError,
+        [&](TypeError &typeError) -> llvm::Error {
+          if (typeError.diagnoseUnderlyingReason(handleModularizationError)) {
+            typeError.diagnose(this);
+            return llvm::Error::success();
+          }
+          return llvm::make_error<TypeError>(std::move(typeError));
+        },
+        [&](ExtensionError &extError) -> llvm::Error {
+          if (extError.diagnoseUnderlyingReason(handleModularizationError)) {
+            extError.diagnose(this);
+            return llvm::Error::success();
+          }
+          return llvm::make_error<ExtensionError>(std::move(extError));
+        });
+
+      // If no error is left, it was reported as a diagnostic. There's no
+      // need to crash.
+      if (!error)
+        return llvm::Error::success();
+    }
+
+    // General deserialization failure message.
+    ctx.Diags.diagnose(getSourceLoc(), diag::serialization_fatal, Core->Name);
+  }
   // Unless in the debugger, crash. ModuleFileSharedCore::fatal() calls abort().
   // This allows aggregation of crash logs for compiler development, but in a
   // long-running process like LLDB this is undesirable. Only abort() if not in
   // the debugger.
-  if (!getContext().LangOpts.DebuggerSupport)
+  if (!ctx.LangOpts.DebuggerSupport)
     Core->fatal(std::move(error));
 
-  // Otherwise, augment the error with contextual information and pass it back.
+  // Otherwise, augment the error with contextual information at this point
+  // of failure and pass it back to be reported later.
   std::string msg;
   {
     llvm::raw_string_ostream os(msg);
@@ -1789,18 +1872,21 @@ ModuleFile::resolveCrossReference(ModuleID MID, uint32_t pathLen) {
     // is mostly for compiler engineers to understand a likely solution at a
     // quick glance.
     SmallVector<char, 64> strScratch;
-    SmallVector<std::string, 2> notes;
-    auto declName = getXRefDeclNameForError();
+
+    auto errorKind = ModularizationError::Kind::DeclNotFound;
+    Identifier foundIn;
+    bool isType = false;
+
     if (recordID == XREF_TYPE_PATH_PIECE ||
         recordID == XREF_VALUE_PATH_PIECE) {
       auto &ctx = getContext();
       for (auto nameAndModule : ctx.getLoadedModules()) {
-        auto baseModule = nameAndModule.second;
+        auto otherModule = nameAndModule.second;
 
         IdentifierID IID;
         IdentifierID privateDiscriminator = 0;
         TypeID TID = 0;
-        bool isType = (recordID == XREF_TYPE_PATH_PIECE);
+        isType = (recordID == XREF_TYPE_PATH_PIECE);
         bool inProtocolExt = false;
         bool importedFromClang = false;
         bool isStatic = false;
@@ -1824,10 +1910,10 @@ ModuleFile::resolveCrossReference(ModuleID MID, uint32_t pathLen) {
 
         values.clear();
         if (privateDiscriminator) {
-          baseModule->lookupMember(values, baseModule, name,
+          otherModule->lookupMember(values, otherModule, name,
                                    getIdentifier(privateDiscriminator));
         } else {
-          baseModule->lookupQualified(baseModule, DeclNameRef(name),
+          otherModule->lookupQualified(otherModule, DeclNameRef(name),
                                       NL_QualifiedDefault,
                                       values);
         }
@@ -1841,30 +1927,31 @@ ModuleFile::resolveCrossReference(ModuleID MID, uint32_t pathLen) {
           // Found a full match in a different module. It should be a different
           // one because otherwise it would have succeeded on the first search.
           // This is usually caused by the use of poorly modularized headers.
-          auto line = "There is a matching '" +
-                      declName.getString(strScratch).str() +
-                      "' in module '" +
-                      std::string(nameAndModule.first.str()) +
-                      "'. If this is imported from clang, please make sure " +
-                      "the header is part of a single clang module.";
-          notes.emplace_back(line);
+          errorKind = ModularizationError::Kind::DeclMoved;
+          foundIn = otherModule->getName();
+          break;
         } else if (hadAMatchBeforeFiltering) {
           // Found a match that was filtered out. This may be from the same
           // expected module if there's a type difference. This can be caused
           // by the use of different Swift language versions between a library
           // with serialized SIL and a client.
-          auto line = "'" +
-                      declName.getString(strScratch).str() +
-                      "' in module '" +
-                      std::string(nameAndModule.first.str()) +
-                      "' was filtered out.";
-          notes.emplace_back(line);
+          errorKind = ModularizationError::Kind::DeclKindChanged;
+          foundIn = otherModule->getName();
+          break;
         }
       }
     }
 
-    return llvm::make_error<XRefError>("top-level value not found", pathTrace,
-                                       declName, notes);
+    auto declName = getXRefDeclNameForError();
+    auto expectedIn = baseModule->getName();
+    auto referencedFrom = getName();
+    return llvm::make_error<ModularizationError>(declName,
+                                                 isType,
+                                                 errorKind,
+                                                 expectedIn,
+                                                 referencedFrom,
+                                                 foundIn,
+                                                 pathTrace);
   }
 
   // Filters for values discovered in the remaining path pieces.
@@ -7265,7 +7352,8 @@ static llvm::Error consumeErrorIfXRefNonLoadedModule(llvm::Error &&error) {
     // implementation-only import hiding types and decls.
     // rdar://problem/60291019
     if (error.isA<XRefNonLoadedModuleError>() ||
-        error.isA<UnsafeDeserializationError>()) {
+        error.isA<UnsafeDeserializationError>() ||
+        error.isA<ModularizationError>()) {
       consumeError(std::move(error));
       return llvm::Error::success();
     }
@@ -7278,7 +7366,8 @@ static llvm::Error consumeErrorIfXRefNonLoadedModule(llvm::Error &&error) {
       auto *TE = static_cast<TypeError*>(errorInfo.get());
 
       if (TE->underlyingReasonIsA<XRefNonLoadedModuleError>() ||
-          TE->underlyingReasonIsA<UnsafeDeserializationError>()) {
+          TE->underlyingReasonIsA<UnsafeDeserializationError>() ||
+          TE->underlyingReasonIsA<ModularizationError>()) {
         consumeError(std::move(errorInfo));
         return llvm::Error::success();
       }