[Serialization] Report detected modularization breaks

The Swift compiler expects the context against which a binary
swiftmodule was built to remain stable when imported by a client.
Usually the build system would rebuild a module if a dependency changes,
or the compiler would regenerate it from a swiftinterface on a context
change. However, such changes are not always detected and in such a case
the compiler can crash on a inconsistency in the context. We often see
that when a clang module is poorly modularized which makes it be import
differently depending on the precise compiler invocation.

These are project issues, having them make the compiler crash provides a
bad experience and doesn't encourage the developer to fix them by
themselves. Let's keep track of modularization issues encountered during
deserialization and report them as errors when they trigger a fatal
failure preventing compilation.

Author: xymus

include/swift/AST/DiagnosticsSema.def

@@ -872,6 +872,31 @@ ERROR(need_hermetic_seal_to_import_module,none,
       "current compilation does not have -experimental-hermetic-seal-at-link",
       (Identifier))
 
+ERROR(modularization_issue_moved,Fatal,
+      "module reference to top-level %0 broken by a context change, "
+      "was found in %1 when building '%2', "
+      "but now a candidate is found only in %3. "
+      "Audit related headers or uninstall SDK roots.",
+      (DeclName, Identifier, StringRef, Identifier))
+ERROR(modularization_issue_type_changed,Fatal,
+      "module reference to top-level %0 broken by a context change, "
+      "its type changed since building '%2', "
+      "it was in %1 now found in %3. "
+      "Ensure Swift language version match between modules or uninstall SDK roots.",
+      (DeclName, Identifier, StringRef, Identifier))
+ERROR(modularization_issue_not_found,Fatal,
+      "module reference to top-level %0 broken by a context change, "
+      "module '%2' references %0 in %1, but it cannot be found in this build. "
+      "Audit related headers or uninstall SDK roots.",
+      (DeclName, Identifier, StringRef, Identifier))
+
+NOTE(modularization_issue_effect_extension_error,none,
+     "could not deserialize extension",
+     ())
+NOTE(modularization_issue_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() {}
 
 /// Skips a single record in the bitstream.
 ///
@@ -177,18 +179,92 @@ void ModuleFile::fatal(llvm::Error error) const {
   Core->fatal(diagnoseFatal(std::move(error)));
 }
 
+void
+ModularizationError::diagnose(ASTContext &ctx,
+                              DiagnosticBehavior limit) const {
+  auto kindToDiagId = [&](Kind kind) {
+    switch (kind) {
+      case Kind::Moved:
+        return diag::modularization_issue_moved;
+      case Kind::TypeChanged:
+        return diag::modularization_issue_type_changed;
+      case Kind::NotFound:
+        return diag::modularization_issue_not_found;
+    }
+    llvm_unreachable("Unhandled ModularizationError::Kind in switch.");
+  };
+
+  // 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.
+  auto inFlight =
+    ctx.Diags.diagnose(SourceLoc(), kindToDiagId(kind), name,
+                       expectedModuleName, referencedFromModuleName,
+                       foundModuleName);
+  inFlight.limitBehavior(limit);
+}
+
+void TypeError::diagnose(ASTContext &ctx) const {
+  ctx.Diags.diagnose(SourceLoc(),
+                     diag::modularization_issue_effect_type_error,
+                     name);
+}
+
+void ExtensionError::diagnose(ASTContext &ctx) const {
+  ctx.Diags.diagnose(SourceLoc(),
+                     diag::modularization_issue_effect_extension_error);
+}
+
 llvm::Error ModuleFile::diagnoseFatal(llvm::Error error) const {
-  if (FileContext)
-    getContext().Diags.diagnose(SourceLoc(), diag::serialization_fatal,
+
+  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(ctx);
+          return llvm::Error::success();
+        };
+      error = llvm::handleErrors(std::move(error),
+            handleModularizationError,
+            [&](TypeError &typeError) -> llvm::Error {
+              if (typeError.diagnoseUnderlyingReason(handleModularizationError)) {
+                typeError.diagnose(ctx);
+                return llvm::Error::success();
+              }
+              return llvm::make_error<TypeError>(std::move(typeError));
+            },
+            [&](ExtensionError &extError) -> llvm::Error {
+              if (extError.diagnoseUnderlyingReason(handleModularizationError)) {
+                extError.diagnose(ctx);
+                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(SourceLoc(), 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);
@@ -1778,13 +1854,15 @@ 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::NotFound;
+    Identifier foundIn;
+
     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;
@@ -1813,10 +1891,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);
         }
@@ -1830,30 +1908,30 @@ 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::Moved;
+          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::TypeChanged;
+          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,
+                                                 errorKind,
+                                                 expectedIn,
+                                                 referencedFrom,
+                                                 foundIn,
+                                                 pathTrace);
   }
 
   // Filters for values discovered in the remaining path pieces.
@@ -7257,8 +7335,9 @@ llvm::Error ModuleFile::consumeExpectedError(llvm::Error &&error) {
     // implementation-only import hiding types and decls.
     // rdar://problem/60291019
     if (error.isA<XRefNonLoadedModuleError>() ||
-        error.isA<UnsafeDeserializationError>()) {
-      consumeError(std::move(error));
+        error.isA<UnsafeDeserializationError>() ||
+        error.isA<ModularizationError>()) {
+      diagnoseAndConsumeError(std::move(error));
       return llvm::Error::success();
     }
 
@@ -7270,8 +7349,9 @@ llvm::Error ModuleFile::consumeExpectedError(llvm::Error &&error) {
       auto *TE = static_cast<TypeError*>(errorInfo.get());
 
       if (TE->underlyingReasonIsA<XRefNonLoadedModuleError>() ||
-          TE->underlyingReasonIsA<UnsafeDeserializationError>()) {
-        consumeError(std::move(errorInfo));
+          TE->underlyingReasonIsA<UnsafeDeserializationError>() ||
+          TE->underlyingReasonIsA<ModularizationError>()) {
+        diagnoseAndConsumeError(std::move(errorInfo));
         return llvm::Error::success();
       }