Merge pull request #36431 from xymus/notes-on-xrefs

[Serialization] Reword xref errors and show notes for common issues

Author: xymus

lib/Serialization/Deserialization.cpp

@@ -1469,8 +1469,91 @@ ModuleFile::resolveCrossReference(ModuleID MID, uint32_t pathLen) {
   };
 
   if (values.empty()) {
+    // Couldn't resolve the reference. Try to explain the problem and leave it
+    // up to the caller to recover if possible.
+
+    // Look for types and value decls in other modules. This extra information
+    // is mostly for compiler engineers to understand a likely solution at a
+    // quick glance.
+    SmallVector<char> strScratch;
+    SmallVector<std::string, 2> notes;
+    auto declName = getXRefDeclNameForError();
+    if (recordID == XREF_TYPE_PATH_PIECE ||
+        recordID == XREF_VALUE_PATH_PIECE) {
+      auto &ctx = getContext();
+      for (auto nameAndModule : ctx.getLoadedModules()) {
+        auto baseModule = nameAndModule.second;
+
+        IdentifierID IID;
+        IdentifierID privateDiscriminator = 0;
+        TypeID TID = 0;
+        bool isType = (recordID == XREF_TYPE_PATH_PIECE);
+        bool inProtocolExt = false;
+        bool importedFromClang = false;
+        bool isStatic = false;
+        if (isType) {
+          XRefTypePathPieceLayout::readRecord(scratch, IID, privateDiscriminator,
+                                              inProtocolExt, importedFromClang);
+        } else {
+          XRefValuePathPieceLayout::readRecord(scratch, TID, IID, inProtocolExt,
+                                               importedFromClang, isStatic);
+        }
+
+        DeclBaseName name = getDeclBaseName(IID);
+        Type filterTy;
+        if (!isType) {
+          auto maybeType = getTypeChecked(TID);
+          // Any error here would have been handled previously.
+          if (maybeType) {
+            filterTy = maybeType.get();
+          }
+        }
+
+        values.clear();
+        if (privateDiscriminator) {
+          baseModule->lookupMember(values, baseModule, name,
+                                   getIdentifier(privateDiscriminator));
+        } else {
+          baseModule->lookupQualified(baseModule, DeclNameRef(name),
+                                      NL_QualifiedDefault,
+                                      values);
+        }
+
+        bool hadAMatchBeforeFiltering = !values.empty();
+        filterValues(filterTy, nullptr, nullptr, isType, inProtocolExt,
+                     importedFromClang, isStatic, None, values);
+
+        strScratch.clear();
+        if (!values.empty()) {
+          // 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 = "'" +
+                      declName.getString(strScratch).str() +
+                      "' was not found in module '" +
+                      std::string(baseModule->getName().str()) +
+                      "', but there is one 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);
+        } 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(baseModule->getName().str()) +
+                      "' was filtered out.";
+          notes.emplace_back(line);
+        }
+      }
+    }
+
     return llvm::make_error<XRefError>("top-level value not found", pathTrace,
-                                       getXRefDeclNameForError());
+                                       declName, notes);
   }
 
   // Filters for values discovered in the remaining path pieces.

lib/Serialization/DeserializationErrors.h

@@ -222,12 +222,11 @@ class XRefTracePath {
   }
 
   void print(raw_ostream &os, StringRef leading = "") const {
-    os << "Cross-reference to module '" << baseM.getName() << "'\n";
-    for (auto &piece : path) {
-      os << leading << "... ";
-      piece.print(os);
-      os << "\n";
-    }
+    os << "Cross-reference to '";
+    interleave(path,
+               [&](auto &piece) { piece.print(os); },
+               [&] { os << '.'; });
+    os << "' in module '" << baseM.getName() << "'\n";
   }
 };
 
@@ -276,16 +275,25 @@ class XRefError : public llvm::ErrorInfo<XRefError, DeclDeserializationError> {
 
   XRefTracePath path;
   const char *message;
+  SmallVector<std::string, 2> notes;
 public:
   template <size_t N>
-  XRefError(const char (&message)[N], XRefTracePath path, DeclName name)
-      : path(path), message(message) {
+  XRefError(const char (&message)[N], XRefTracePath path, DeclName name,
+            SmallVector<std::string, 2> notes = {})
+      : path(path), message(message), notes(notes) {
     this->name = name;
   }
 
   void log(raw_ostream &OS) const override {
     OS << message << "\n";
     path.print(OS);
+
+    if (!notes.empty()) {
+      OS << "Notes:\n";
+      for (auto &line : notes) {
+        OS << "* " << line << "\n";
+      }
+    }
   }
 
   std::error_code convertToErrorCode() const override {
@@ -360,9 +368,9 @@ class TypeError : public llvm::ErrorInfo<TypeError, DeclDeserializationError> {
   }
 
   void log(raw_ostream &OS) const override {
-    OS << "could not deserialize type for '" << name << "'";
+    OS << "Could not deserialize type for '" << name << "'";
     if (underlyingReason) {
-      OS << ": ";
+      OS << "\nCaused by: ";
       underlyingReason->log(OS);
     }
   }

test/Serialization/Recovery/crash-xref.swift

@@ -0,0 +1,35 @@
+/// Test xref error description by removing a type from a module after building
+/// a client.
+// RUN: %empty-directory(%t)
+
+/// Compile module A with a type and an empty module B.
+// RUN: %target-swift-frontend %s -emit-module-path %t/A.swiftmodule -module-name A -D LIB
+// RUN: %target-swift-frontend %s -emit-module-path %t/B.swiftmodule -module-name B
+#if LIB
+public struct SomeType {
+    public init() {}
+}
+
+/// Compile a client using the type from A.
+// RUN: %target-swift-frontend %s -emit-module-path %t/Client.swiftmodule -module-name Client -D CLIENT -I %t
+#elseif CLIENT
+import A
+import B
+
+public func foo() -> A.SomeType { fatalError() }
+
+/// Swap A and B around! A is now empty and B defines the type.
+// RUN: %target-swift-frontend %s -emit-module-path %t/A.swiftmodule -module-name A
+// RUN: %target-swift-frontend %s -emit-module-path %t/B.swiftmodule -module-name B -D LIB
+#endif // Empty
+
+/// Read from the client to get an xref error to the type missing from A.
+// RUN: not --crash %target-swift-frontend -emit-sil %t/Client.swiftmodule -module-name Client -I %t -disable-deserialization-recovery 2> %t/stderr
+
+// RUN: cat %t/stderr | %FileCheck %s
+// CHECK: *** DESERIALIZATION FAILURE (please include this section in any bug report) ***
+// CHECK-NEXT: Could not deserialize type for 'foo()'
+// CHECK-NEXT: Caused by: top-level value not found
+// CHECK-NEXT: Cross-reference to 'SomeType' in module 'A'
+// CHECK-NEXT: Notes:
+// CHECK-NEXT: * 'SomeType' was not found in module 'B', but there is one in module 'B'. If this is imported from clang, please make sure the header is part of a single clang module.