[Serialization] Show contextual notes on deserialization errors

This PR makes diagnostics on deserialization errors caused by project
configuration more helpful by providing contextual information on the
issue:

- Path to the modules involved (up to 4 modules): the loaded swiftmodule
with the broken outgoing reference, the path to the module where the
decl was expected, the path to the underlying clang module, and the path
to the module where the decl was found. This information should prevent
us from having to ask for a different log with `-Rmodule-loading`.
- Hint to delete the swiftmodule files when the module is
library-evolution enabled.
- Hint that clang settings affect clang modules involved in this
scenario.
- Pointing out when a decl moved between two modules with a similar name
(where one name is a prefix of the other). This is a common issue when
headers are shared between a clang framework's public and private
modules.
- Pointing out layering issues when an SDK module imports a local
module.
- Pointing out Swift language version mismatch which may lead to the
compiler viewing the same clang decls differently when they are modified
by APINotes files.

Author: xymus

include/swift/AST/DiagnosticsSema.def

@@ -873,19 +873,55 @@ ERROR(need_hermetic_seal_to_import_module,none,
       (Identifier))
 
 ERROR(modularization_issue_decl_moved,Fatal,
-      "reference to %select{top-level|type}0 %1 broken by a context change; "
+      "reference to %select{top-level declaration|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, const ModuleDecl*, const ModuleDecl*))
 ERROR(modularization_issue_decl_type_changed,Fatal,
-      "reference to %select{top-level|type}0 %1 broken by a context change; "
+      "reference to %select{top-level declaration|type}0 %1 broken by a context change; "
       "the declaration kind of %1 %select{from %2|}5 changed since building '%3'"
       "%select{|, it was in %2 and is now a candidate is found only in %4}5",
       (bool, DeclName, const ModuleDecl*, StringRef, const ModuleDecl*, bool))
 ERROR(modularization_issue_decl_not_found,Fatal,
-      "reference to %select{top-level|type}0 %1 broken by a context change; "
+      "reference to %select{top-level declaration|type}0 %1 broken by a context change; "
       "%1 is not found, it was expected to be in %2",
       (bool, DeclName, const ModuleDecl*))
 
+NOTE(modularization_issue_note_expected,none,
+     "the %select{declaration|type}0 was expected to be found in module %1 at '%2'",
+     (bool, const ModuleDecl*, StringRef))
+NOTE(modularization_issue_note_expected_underlying,none,
+     "or expected to be found in the underlying module '%0' defined at '%1'",
+     (StringRef, StringRef))
+NOTE(modularization_issue_note_found,none,
+     "the %select{declaration|type}0 was actually found in module %1 at '%2'",
+     (bool, const ModuleDecl*, StringRef))
+
+NOTE(modularization_issue_stale_module,none,
+     "the module %0 has enabled library-evolution; "
+     "the following file may need to be deleted if the SDK was modified: '%1'",
+     (const ModuleDecl *, StringRef))
+NOTE(modularization_issue_swift_version,none,
+     "the module %0 was built with a Swift language version set to %1 "
+     "while the current invocation uses %2; "
+     "APINotes may change how clang declarations are imported",
+     (const ModuleDecl *, llvm::VersionTuple, llvm::VersionTuple))
+NOTE(modularization_issue_audit_headers,none,
+     "declarations in the %select{underlying|}0 clang module %1 "
+     "may be hidden by clang project settings",
+     (bool, const ModuleDecl*))
+NOTE(modularization_issue_layering_expected_local,none,
+     "the distributed module %0 refers to the local module %1; "
+     "this may be caused by clang project settings",
+     (const ModuleDecl*, const ModuleDecl*))
+NOTE(modularization_issue_layering_found_local,none,
+     "the reference may break layering; the candidate was found in "
+     "the local module %1 for a reference from the distributed module %0",
+     (const ModuleDecl*, const ModuleDecl*))
+NOTE(modularization_issue_related_modules,none,
+     "the %select{declaration|type}0 %1 moved between related modules; "
+     "clang project settings may affect headers shared between these modules",
+     (bool, DeclName))
+
 NOTE(modularization_issue_side_effect_extension_error,none,
      "could not deserialize extension",
      ())

lib/Serialization/Deserialization.cpp

@@ -40,6 +40,7 @@
 #include "swift/ClangImporter/SwiftAbstractBasicReader.h"
 #include "swift/Serialization/SerializedModuleLoader.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/Basic/SourceManager.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/Debug.h"
@@ -185,7 +186,7 @@ SourceLoc ModuleFile::getSourceLoc() const {
   auto filename = getModuleFilename();
   auto bufferID = SourceMgr.getIDForBufferIdentifier(filename);
   if (!bufferID)
-    bufferID = SourceMgr.addMemBufferCopy(StringRef(), filename);
+    bufferID = SourceMgr.addMemBufferCopy("<binary format>", filename);
   return SourceMgr.getLocForBufferStart(*bufferID);
 }
 
@@ -238,6 +239,103 @@ ModularizationError::diagnose(const ModuleFile *MF,
   // 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.
+
+  // Show context with relevant file paths.
+  ctx.Diags.diagnose(SourceLoc(),
+                     diag::modularization_issue_note_expected,
+                     declIsType, expectedModule,
+                     expectedModule->getModuleSourceFilename());
+
+  const clang::Module *expectedUnderlying =
+                                   expectedModule->findUnderlyingClangModule();
+  if (!expectedModule->isNonSwiftModule() &&
+      expectedUnderlying) {
+    auto CML = ctx.getClangModuleLoader();
+    auto &CSM = CML->getClangASTContext().getSourceManager();
+    StringRef filename = CSM.getFilename(expectedUnderlying->DefinitionLoc);
+    ctx.Diags.diagnose(SourceLoc(),
+                       diag::modularization_issue_note_expected_underlying,
+                       expectedUnderlying->Name,
+                       filename);
+  }
+
+  if (foundModule)
+    ctx.Diags.diagnose(SourceLoc(),
+                       diag::modularization_issue_note_found,
+                       declIsType, foundModule,
+                       foundModule->getModuleSourceFilename());
+
+  // A Swift language version mismatch could lead to a different set of rules
+  // from APINotes files being applied when building the module vs when reading
+  // from it.
+  version::Version
+    moduleLangVersion = referenceModule->getCompatibilityVersion(),
+    clientLangVersion = MF->getContext().LangOpts.EffectiveLanguageVersion;
+  ModuleDecl *referenceModuleDecl = referenceModule->getAssociatedModule();
+  if (clientLangVersion != moduleLangVersion) {
+    ctx.Diags.diagnose(SourceLoc(),
+                       diag::modularization_issue_swift_version,
+                       referenceModuleDecl, moduleLangVersion,
+                       clientLangVersion);
+  }
+
+  // If the error is in a resilient swiftmodule adjacent to a swiftinterface,
+  // deleting the module to rebuild from the swiftinterface may fix the issue.
+  // Limit this suggestion to distributed Swift modules to not hint at
+  // deleting local caches and such.
+  bool referenceModuleIsDistributed = referenceModuleDecl &&
+                                      referenceModuleDecl->isNonUserModule();
+  if (referenceModule->getResilienceStrategy() ==
+                                               ResilienceStrategy::Resilient &&
+      referenceModuleIsDistributed) {
+    ctx.Diags.diagnose(SourceLoc(),
+                       diag::modularization_issue_stale_module,
+                       referenceModuleDecl,
+                       referenceModule->getModuleFilename());
+  }
+
+  // If the missing decl was expected to be in a clang module,
+  // it may be hidden by some clang defined passed via `-Xcc` affecting how
+  // headers are seen.
+  if (expectedUnderlying) {
+    ctx.Diags.diagnose(SourceLoc(),
+                       diag::modularization_issue_audit_headers,
+                       expectedModule->isNonSwiftModule(), expectedModule);
+  }
+
+  // If the reference goes from a distributed module to a local module,
+  // the compiler may have picked up an undesired module. We usually expect
+  // distributed modules to only reference other distributed modules.
+  // Local modules can reference both local modules and distributed modules.
+  if (referenceModuleIsDistributed) {
+    if (!expectedModule->isNonUserModule()) {
+      ctx.Diags.diagnose(SourceLoc(),
+                         diag::modularization_issue_layering_expected_local,
+                         referenceModuleDecl, expectedModule);
+    } else if (foundModule && !foundModule->isNonUserModule()) {
+      ctx.Diags.diagnose(SourceLoc(),
+                         diag::modularization_issue_layering_found_local,
+                         referenceModuleDecl, foundModule);
+    }
+  }
+
+  // If a type moved between MyModule and MyModule_Private, it can be caused
+  // by the use of `-Xcc -D` to change the API of the modules, leading to
+  // decls moving between both modules.
+  if (errorKind == Kind::DeclMoved ||
+      errorKind == Kind::DeclKindChanged) {
+    StringRef foundModuleName = foundModule->getName().str();
+    StringRef expectedModuleName = expectedModule->getName().str();
+    if (foundModuleName != expectedModuleName &&
+        (foundModuleName.startswith(expectedModuleName) ||
+         expectedModuleName.startswith(foundModuleName)) &&
+        (expectedUnderlying ||
+         expectedModule->findUnderlyingClangModule())) {
+      ctx.Diags.diagnose(SourceLoc(),
+                         diag::modularization_issue_related_modules,
+                         declIsType, name);
+    }
+  }
 }
 
 void TypeError::diagnose(const ModuleFile *MF) const {
@@ -2064,7 +2162,7 @@ ModuleFile::resolveCrossReference(ModuleID MID, uint32_t pathLen) {
         [&](const ModularizationError &modularError) {
           modularError.diagnose(this, DiagnosticBehavior::Warning);
         });
-      getContext().Diags.diagnose(getSourceLoc(),
+      getContext().Diags.diagnose(SourceLoc(),
                                   diag::modularization_issue_worked_around);
     } else {
       return std::move(error);

test/Serialization/Recovery/module-recovery-remarks.swift

@@ -15,6 +15,43 @@
 
 /// Main error downgraded to a remark.
 // CHECK-MOVED: LibWithXRef.swiftmodule:1:1: remark: reference to type 'MyType' broken by a context change; 'MyType' was expected to be in 'A', but now a candidate is found only in 'A_related'
+
+/// Contextual notes about the modules involved.
+// CHECK-MOVED: <unknown>:0: note: the type was expected to be found in module 'A' at '
+// CHECK-MOVED-SAME: A.swiftmodule'
+// CHECK-MOVED: <unknown>:0: note: or expected to be found in the underlying module 'A' defined at '
+// CHECK-MOVED-SAME: module.modulemap'
+// CHECK-MOVED: <unknown>:0: note: the type was actually found in module 'A_related' at '
+// CHECK-MOVED-SAME: A_related.swiftmodule'
+
+/// More notes depending on the context
+// CHECK-MOVED: <unknown>:0: note: the module 'LibWithXRef' was built with a Swift language version set to 5
+// CHECK-MOVED-SAME: while the current invocation uses 4
+
+// CHECK-MOVED: <unknown>:0: note: the module 'LibWithXRef' has enabled library-evolution; the following file may need to be deleted if the SDK was modified: '
+// CHECK-MOVED-SAME: LibWithXRef.swiftmodule'
+// CHECK-MOVED: <unknown>:0: note: declarations in the underlying clang module 'A' may be hidden by clang project settings
+// CHECK-MOVED: <unknown>:0: note: the distributed module 'LibWithXRef' refers to the local module 'A'; this may be caused by clang project settings
+// CHECK-MOVED: <unknown>:0: note: the type 'MyType' moved between related modules; clang project settings may affect headers shared between these modules
+// CHECK-MOVED: LibWithXRef.swiftmodule:1:1: note: could not deserialize type for 'foo()'
+// CHECK-MOVED: error: cannot find 'foo' in scope
+
+/// Move A to the SDK, triggering a different note about layering.
+// RUN: mv %t/A.swiftmodule %t/sdk/A.swiftmodule
+// RUN: not %target-swift-frontend -c -O %t/Client.swift -I %t -I %t/sdk -Rmodule-recovery -sdk %t/sdk 2>&1 \
+// RUN:   | %FileCheck --check-prefixes CHECK-LAYERING-FOUND %s
+// CHECK-LAYERING-FOUND: <unknown>:0: note: the reference may break layering; the candidate was found in the local module 'A_related' for a reference from the distributed module 'LibWithXRef'
+// CHECK-LAYERING-FOUND: error: cannot find 'foo' in scope
+
+/// Delete A, keep only the underlying clangmodule for notes about clang modules.
+// RUN: rm %t/sdk/A.swiftmodule
+// RUN: %target-swift-frontend %t/Empty.swift -emit-module-path %t/A_related.swiftmodule -module-name A_related
+// RUN: not %target-swift-frontend -c -O %t/Client.swift -I %t -I %t/sdk -Rmodule-recovery -sdk %t/sdk 2>&1 \
+// RUN:   | %FileCheck --check-prefixes CHECK-CLANG %s
+// CHECK-CLANG: <unknown>:0: note: declarations in the clang module 'A' may be hidden by clang project settings
+// CHECK-CLANG: error: cannot find 'foo' in scope
+
+
 //--- module.modulemap
 module A {
     header "A.h"

test/Serialization/modularization-error.swift

@@ -19,7 +19,13 @@
 // RUN:   -experimental-force-workaround-broken-modules 2>&1 \
 // RUN:   | %FileCheck --check-prefixes CHECK-WORKAROUND %s
 // CHECK-WORKAROUND: LibWithXRef.swiftmodule:1:1: warning: reference to type 'MyType' broken by a context change; 'MyType' was expected to be in 'A', but now a candidate is found only in 'B'
-// CHECK-WORKAROUND: LibWithXRef.swiftmodule:1:1: note: attempting forced recovery enabled by -experimental-force-workaround-broken-modules
+// CHECK-WORKAROUND-NEXT: A.MyType
+// CHECK-WORKAROUND-NEXT: ^
+// CHECK-WORKAROUND: <unknown>:0: note: the type was expected to be found in module 'A' at '
+// CHECK-WORKAROUND-SAME: A.swiftmodule'
+// CHECK-WORKAROUND: <unknown>:0: note: the type was actually found in module 'B' at '
+// CHECK-WORKAROUND-SAME: B.swiftmodule'
+// CHECK-WORKAROUND: <unknown>:0: note: attempting forced recovery enabled by -experimental-force-workaround-broken-modules
 // CHECK-WORKAROUND: func foo() -> some Proto
 
 /// Change MyType into a function.