Merge pull request #8580 from jrose-apple/enable-experimental-deserialization-recovery

[Serialization] Proof-of-concept: drop overriding methods if the base is missing

include/swift/AST/DiagnosticsSema.def

@@ -549,6 +549,11 @@ ERROR(serialization_target_too_new_repl,none,
       "module file's minimum deployment target is %0 v%1.%2%select{|.%3}3: %4",
       (StringRef, unsigned, unsigned, unsigned, StringRef))
 
+ERROR(serialization_fatal,Fatal,
+      "fatal error encountered while reading from module '%0'; "
+      "please file a bug report with your project and the crash log",
+      (StringRef))
+
 ERROR(reserved_member_name,none,
       "type member may not be named %0, since it would conflict with the"
       " 'foo.%1' expression", (DeclName, StringRef))

include/swift/Basic/LangOptions.h

@@ -178,6 +178,12 @@ namespace swift {
     /// new enough?
     bool EnableTargetOSChecking = true;
 
+    /// Whether to attempt to recover from missing cross-references and other
+    /// errors when deserializing from a Swift module.
+    ///
+    /// This is a staging flag; eventually it will be on by default.
+    bool EnableDeserializationRecovery = false;
+
     /// Should we use \c ASTScope-based resolution for unqualified name lookup?
     bool EnableASTScopeLookup = false;
 

include/swift/Option/FrontendOptions.td

@@ -263,6 +263,10 @@ def enable_experimental_property_behaviors :
   Flag<["-"], "enable-experimental-property-behaviors">,
   HelpText<"Enable experimental property behaviors">;
 
+def enable_experimental_deserialization_recovery :
+  Flag<["-"], "enable-experimental-deserialization-recovery">,
+  HelpText<"Attempt to recover from missing xrefs (etc) in swiftmodules">;
+
 def enable_cow_existentials : Flag<["-"], "enable-cow-existentials">,
   HelpText<"Enable the copy-on-write existential implementation">;
 

include/swift/Serialization/ModuleFile.h

@@ -26,6 +26,7 @@
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/TinyPtrVector.h"
 #include "llvm/Bitcode/BitstreamReader.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
 
 namespace llvm {
@@ -435,6 +436,10 @@ public:
     return getStatus();
   }
 
+  /// Emits one last diagnostic, logs the error, and then aborts for the stack
+  /// trace.
+  void fatal(llvm::Error error) LLVM_ATTRIBUTE_NORETURN;
+
   ASTContext &getContext() const {
     assert(FileContext && "no associated context yet");
     return FileContext->getParentModule()->getASTContext();
@@ -542,12 +547,14 @@ private:
   /// because it reads from the cursor, it is not possible to reset the cursor
   /// after reading. Nothing should ever follow an XREF record except
   /// XREF_PATH_PIECE records.
-  Decl *resolveCrossReference(ModuleDecl *M, uint32_t pathLen);
+  llvm::Expected<Decl *> resolveCrossReference(ModuleDecl *M, uint32_t pathLen);
 
   /// Populates TopLevelIDs for name lookup.
   void buildTopLevelDeclMap();
 
-  void configureStorage(AbstractStorageDecl *storage, unsigned rawStorageKind,
+  /// Sets the accessors for \p storage based on \p rawStorageKind.
+  void configureStorage(AbstractStorageDecl *storage,
+                        unsigned rawStorageKind,
                         serialization::DeclID getter,
                         serialization::DeclID setter,
                         serialization::DeclID materializeForSet,
@@ -743,8 +750,13 @@ public:
   }
 
   /// Returns the type with the given ID, deserializing it if needed.
+  ///
+  /// \sa getTypeChecked
   Type getType(serialization::TypeID TID);
 
+  /// Returns the type with the given ID, deserializing it if needed.
+  llvm::Expected<Type> getTypeChecked(serialization::TypeID TID);
+
   /// Returns the identifier with the given ID, deserializing it if needed.
   Identifier getIdentifier(serialization::IdentifierID IID);
 
@@ -754,9 +766,21 @@ public:
   /// \param ForcedContext Optional override for the decl context of certain
   ///                      kinds of decls, used to avoid re-entrant
   ///                      deserialization.
+  ///
+  /// \sa getDeclChecked
   Decl *getDecl(serialization::DeclID DID,
                 Optional<DeclContext *> ForcedContext = None);
 
+  /// Returns the decl with the given ID, deserializing it if needed.
+  ///
+  /// \param DID The ID for the decl within this module.
+  /// \param ForcedContext Optional override for the decl context of certain
+  ///                      kinds of decls, used to avoid re-entrant
+  ///                      deserialization.
+  llvm::Expected<Decl *>
+  getDeclChecked(serialization::DeclID DID,
+                 Optional<DeclContext *> ForcedContext = None);
+
   /// Returns the decl context with the given ID, deserializing it if needed.
   DeclContext *getDeclContext(serialization::DeclContextID DID);
 

lib/Frontend/CompilerInvocation.cpp

@@ -874,6 +874,9 @@ static bool ParseLangArgs(LangOptions &Opts, ArgList &Args,
   Opts.EnableClassResilience |=
     Args.hasArg(OPT_enable_class_resilience);
 
+  Opts.EnableDeserializationRecovery |=
+    Args.hasArg(OPT_enable_experimental_deserialization_recovery);
+
   Opts.DisableAvailabilityChecking |=
       Args.hasArg(OPT_disable_availability_checking);
 

lib/Serialization/Deserialization.cpp

@@ -13,6 +13,7 @@
 #include "swift/Serialization/ModuleFile.h"
 #include "swift/Serialization/ModuleFormat.h"
 #include "swift/AST/ASTContext.h"
+#include "swift/AST/DiagnosticsSema.h"
 #include "swift/AST/ForeignErrorConvention.h"
 #include "swift/AST/GenericEnvironment.h"
 #include "swift/AST/Initializer.h"
@@ -37,6 +38,7 @@ STATISTIC(NumNestedTypeShortcuts,
 
 using namespace swift;
 using namespace swift::serialization;
+using llvm::Expected;
 
 StringRef swift::getNameOfModule(const ModuleFile *MF) {
   return MF->Name;
@@ -94,7 +96,7 @@ namespace {
     }
   };
 
-  class PrettyXRefTrace : public llvm::PrettyStackTraceEntry {
+  class XRefTracePath {
     class PathPiece {
     public:
       enum class Kind {
@@ -195,12 +197,11 @@ namespace {
       }
     };
 
-  private:
     ModuleDecl &baseM;
     SmallVector<PathPiece, 8> path;
 
   public:
-    PrettyXRefTrace(ModuleDecl &M) : baseM(M) {}
+    explicit XRefTracePath(ModuleDecl &M) : baseM(M) {}
 
     void addValue(Identifier name) {
       path.push_back({ PathPiece::Kind::Value, name });
@@ -240,16 +241,27 @@ namespace {
       path.pop_back();
     }
 
-    void print(raw_ostream &os) const override {
+    void print(raw_ostream &os, StringRef leading = "") const {
       os << "Cross-reference to module '" << baseM.getName() << "'\n";
       for (auto &piece : path) {
-        os << "\t... ";
+        os << leading << "... ";
         piece.print(os);
         os << "\n";
       }
     }
   };
 
+  class PrettyXRefTrace :
+      public llvm::PrettyStackTraceEntry,
+      public XRefTracePath {
+  public:
+    explicit PrettyXRefTrace(ModuleDecl &M) : XRefTracePath(M) {}
+
+    void print(raw_ostream &os) const override {
+      XRefTracePath::print(os, "\t");
+    }
+  };
+
   class PrettyStackTraceModuleFile : public llvm::PrettyStackTraceEntry {
     const char *Action;
     const ModuleFile *MF;
@@ -261,6 +273,50 @@ namespace {
       os << Action << " \'" << getNameOfModule(MF) << "'\n";
     }
   };
+
+  class XRefError : public llvm::ErrorInfo<XRefError> {
+    friend ErrorInfo;
+    static const char ID;
+
+    XRefTracePath path;
+    const char *message;
+  public:
+    template <size_t N>
+    XRefError(const char (&message)[N], XRefTracePath path)
+        : path(path), message(message) {}
+
+    void log(raw_ostream &OS) const override {
+      OS << message << "\n";
+      path.print(OS);
+    }
+
+    std::error_code convertToErrorCode() const override {
+      // This is a deprecated part of llvm::Error, so we just return a very
+      // generic value.
+      return {EINVAL, std::generic_category()};
+    }
+  };
+  const char XRefError::ID = '\0';
+
+  class OverrideError : public llvm::ErrorInfo<OverrideError> {
+    friend ErrorInfo;
+    static const char ID;
+
+    DeclName name;
+  public:
+    explicit OverrideError(DeclName name) : name(name) {}
+
+    void log(raw_ostream &OS) const override {
+      OS << "could not find '" << name << "' in parent class";
+    }
+
+    std::error_code convertToErrorCode() const override {
+      // This is a deprecated part of llvm::Error, so we just return a very
+      // generic value.
+      return {EINVAL, std::generic_category()};
+    }
+  };
+  const char OverrideError::ID = '\0';
 } // end anonymous namespace
 
 
@@ -285,6 +341,17 @@ static bool skipRecord(llvm::BitstreamCursor &cursor, unsigned recordKind) {
 #endif
 }
 
+void ModuleFile::fatal(llvm::Error error) {
+  if (FileContext) {
+    getContext().Diags.diagnose(SourceLoc(), diag::serialization_fatal, Name);
+  }
+
+  logAllUnhandledErrors(std::move(error), llvm::errs(),
+                        "\n*** DESERIALIZATION FAILURE (please include this "
+                        "section in any bug report) ***\n");
+  abort();
+}
+
 ModuleFile &ModuleFile::getModuleFileForDelayedActions() {
   assert(FileContext && "cannot delay actions before associating with a file");
   ModuleDecl *associatedModule = getAssociatedModule();
@@ -1159,9 +1226,23 @@ bool ModuleFile::readMembers(SmallVectorImpl<Decl *> &Members) {
 
   Members.reserve(rawMemberIDs.size());
   for (DeclID rawID : rawMemberIDs) {
-    Decl *D = getDecl(rawID);
+    Expected<Decl *> D = getDeclChecked(rawID);
-    assert(D && "unable to deserialize next member");
+    if (!D) {
-    Members.push_back(D);
+      if (!getContext().LangOpts.EnableDeserializationRecovery)
+        fatal(D.takeError());
+
+      // Silently drop the member if there was a problem.
+      // FIXME: This isn't sound for protocols; we need to at least record that
+      // it happened.
+      llvm::handleAllErrors(D.takeError(),
+                            [](const OverrideError &) { /* expected */ },
+                            [&](std::unique_ptr<llvm::ErrorInfoBase> unhandled){
+        fatal(std::move(unhandled));
+      });
+      continue;
+    }
+    assert(D.get() && "unchecked error deserializing next member");
+    Members.push_back(D.get());
   }
 
   return false;
@@ -1284,8 +1365,8 @@ static void filterValues(Type expectedTy, ModuleDecl *expectedModule,
   values.erase(newEnd, values.end());
 }
 
-Decl *ModuleFile::resolveCrossReference(ModuleDecl *baseModule,
+Expected<Decl *>
-                                        uint32_t pathLen) {
+ModuleFile::resolveCrossReference(ModuleDecl *baseModule, uint32_t pathLen) {
   using namespace decls_block;
   assert(baseModule && "missing dependency");
   PrettyXRefTrace pathTrace(*baseModule);
@@ -1406,8 +1487,7 @@ Decl *ModuleFile::resolveCrossReference(ModuleDecl *baseModule,
   }
 
   if (values.empty()) {
-    error();
+    return llvm::make_error<XRefError>("top-level value not found", pathTrace);
-    return nullptr;
   }
 
   // Filters for values discovered in the remaining path pieces.
@@ -1526,16 +1606,16 @@ Decl *ModuleFile::resolveCrossReference(ModuleDecl *baseModule,
         pathTrace.addType(filterTy);
 
       if (values.size() != 1) {
-        error();
+        return llvm::make_error<XRefError>("multiple matching base values",
-        return nullptr;
+                                           pathTrace);
       }
 
       auto nominal = dyn_cast<NominalTypeDecl>(values.front());
       values.clear();
 
       if (!nominal) {
-        error();
+        return llvm::make_error<XRefError>("base is not a nominal type",
-        return nullptr;
+                                           pathTrace);
       }
 
       auto members = nominal->lookupDirect(memberName);
@@ -1624,8 +1704,8 @@ Decl *ModuleFile::resolveCrossReference(ModuleDecl *baseModule,
 
     case XREF_GENERIC_PARAM_PATH_PIECE: {
       if (values.size() != 1) {
-        error();
+        return llvm::make_error<XRefError>("multiple matching base values",
-        return nullptr;
+                                           pathTrace);
       }
 
       uint32_t paramIndex;
@@ -1658,9 +1738,15 @@ Decl *ModuleFile::resolveCrossReference(ModuleDecl *baseModule,
       } else if (auto fn = dyn_cast<AbstractFunctionDecl>(base))
         paramList = fn->getGenericParams();
 
-      if (!paramList || paramIndex >= paramList->size()) {
+      if (!paramList) {
-        error();
+        return llvm::make_error<XRefError>(
-        return nullptr;
+            "cross-reference to generic param for non-generic type",
+            pathTrace);
+      }
+      if (paramIndex >= paramList->size()) {
+        return llvm::make_error<XRefError>(
+            "generic argument index out of bounds",
+            pathTrace);
       }
 
       values.clear();
@@ -1684,8 +1770,7 @@ Decl *ModuleFile::resolveCrossReference(ModuleDecl *baseModule,
     }
 
     if (values.empty()) {
-      error();
+      return llvm::make_error<XRefError>("result not found", pathTrace);
-      return nullptr;
     }
 
     // Reset the module filter.
@@ -1704,8 +1789,9 @@ Decl *ModuleFile::resolveCrossReference(ModuleDecl *baseModule,
 
   // When all is said and done, we should have a single value here to return.
   if (values.size() != 1) {
-    error();
+    return llvm::make_error<llvm::StringError>(
-    return nullptr;
+        "result is ambiguous",
+        std::error_code(EINVAL, std::generic_category()));
   }
 
   return values.front();
@@ -2127,6 +2213,15 @@ static uint64_t encodeLazyConformanceContextData(uint64_t numProtocols,
 }
 
 Decl *ModuleFile::getDecl(DeclID DID, Optional<DeclContext *> ForcedContext) {
+  Expected<Decl *> deserialized = getDeclChecked(DID, ForcedContext);
+  if (!deserialized) {
+    fatal(deserialized.takeError());
+  }
+  return deserialized.get();
+}
+
+Expected<Decl *>
+ModuleFile::getDeclChecked(DeclID DID, Optional<DeclContext *> ForcedContext) {
   if (DID == 0)
     return nullptr;
 
@@ -2841,12 +2936,31 @@ Decl *ModuleFile::getDecl(DeclID DID, Optional<DeclContext *> ForcedContext) {
                                         overriddenID, accessorStorageDeclID,
                                         hasCompoundName, rawAddressorKind,
                                         rawAccessLevel, nameIDs);
-    
+
     // Resolve the name ids.
     SmallVector<Identifier, 2> names;
     for (auto nameID : nameIDs)
       names.push_back(getIdentifier(nameID));
 
+    DeclName name;
+    if (!names.empty()) {
+      if (hasCompoundName)
+        name = DeclName(ctx, names[0],
+                        llvm::makeArrayRef(names.begin() + 1, names.end()));
+      else
+        name = DeclName(names[0]);
+    }
+
+    Expected<Decl *> overridden = getDeclChecked(overriddenID);
+    if (!overridden) {
+      llvm::handleAllErrors(overridden.takeError(),
+                            [](const XRefError &) { /* expected */ },
+                            [&](std::unique_ptr<llvm::ErrorInfoBase> unhandled){
+        fatal(std::move(unhandled));
+      });
+      return llvm::make_error<OverrideError>(name);
+    }
+
     auto DC = getDeclContext(contextID);
     if (declOrOffset.isComplete())
       return declOrOffset;
@@ -2865,14 +2979,6 @@ Decl *ModuleFile::getDecl(DeclID DID, Optional<DeclContext *> ForcedContext) {
     if (declOrOffset.isComplete())
       return declOrOffset;
 
-    DeclName name;
-    if (!names.empty()) {
-      if (hasCompoundName)
-        name = DeclName(ctx, names[0],
-                        llvm::makeArrayRef(names.begin() + 1, names.end()));
-      else
-        name = DeclName(names[0]);
-    }
     auto fn = FuncDecl::createDeserialized(
         ctx, /*StaticLoc=*/SourceLoc(), staticSpelling.getValue(),
         /*FuncLoc=*/SourceLoc(), name, /*NameLoc=*/SourceLoc(),
@@ -2929,8 +3035,8 @@ Decl *ModuleFile::getDecl(DeclID DID, Optional<DeclContext *> ForcedContext) {
     if (auto errorConvention = maybeReadForeignErrorConvention())
       fn->setForeignErrorConvention(*errorConvention);
 
-    if (auto overridden = cast_or_null<FuncDecl>(getDecl(overriddenID))) {
+    if (auto overriddenFunc = cast_or_null<FuncDecl>(overridden.get())) {
-      fn->setOverriddenDecl(overridden);
+      fn->setOverriddenDecl(overriddenFunc);
       AddAttribute(new (ctx) OverrideAttr(SourceLoc()));
     }
 
@@ -3528,7 +3634,10 @@ Decl *ModuleFile::getDecl(DeclID DID, Optional<DeclContext *> ForcedContext) {
     ModuleID baseModuleID;
     uint32_t pathLen;
     decls_block::XRefLayout::readRecord(scratch, baseModuleID, pathLen);
-    declOrOffset = resolveCrossReference(getModule(baseModuleID), pathLen);
+    auto resolved = resolveCrossReference(getModule(baseModuleID), pathLen);
+    if (!resolved)
+      return resolved;
+    declOrOffset = resolved.get();
     break;
   }
   
@@ -3643,6 +3752,14 @@ Optional<swift::ResultConvention> getActualResultConvention(uint8_t raw) {
 }
 
 Type ModuleFile::getType(TypeID TID) {
+  Expected<Type> deserialized = getTypeChecked(TID);
+  if (!deserialized) {
+    fatal(deserialized.takeError());
+  }
+  return deserialized.get();
+}
+
+Expected<Type> ModuleFile::getTypeChecked(TypeID TID) {
   if (TID == 0)
     return Type();
 

test/Serialization/Recovery/Inputs/custom-modules/Overrides.h

@@ -0,0 +1,5 @@
+@interface Base
+#ifndef BAD
+- (void)method;
+#endif
+@end

test/Serialization/Recovery/Inputs/custom-modules/module.modulemap

@@ -0,0 +1 @@
+module Overrides { header "Overrides.h" }

test/Serialization/Recovery/overrides.swift

@@ -0,0 +1,27 @@
+// RUN: rm -rf %t && mkdir -p %t
+// RUN: %target-swift-frontend -emit-module -o %t -module-name Lib -I %S/Inputs/custom-modules %s
+
+// RUN: %target-swift-ide-test -source-filename=x -print-module -module-to-print Lib -I %t -I %S/Inputs/custom-modules | %FileCheck %s
+
+// RUN: not --crash %target-swift-ide-test -source-filename=x -print-module -module-to-print Lib -I %t -I %S/Inputs/custom-modules -Xcc -DBAD 2>&1 | %FileCheck -check-prefix CHECK-CRASH %s
+// RUN: %target-swift-ide-test -source-filename=x -print-module -module-to-print Lib -I %t -I %S/Inputs/custom-modules -Xcc -DBAD -enable-experimental-deserialization-recovery | %FileCheck -check-prefix CHECK-RECOVERY %s
+
+// REQUIRES: objc_interop
+
+import Overrides
+
+public class Sub: Base {
+  public override func method() {}
+}
+
+// CHECK-LABEL: class Sub : Base {
+// CHECK-NEXT: func method()
+// CHECK-NEXT: {{^}$}}
+
+// CHECK-CRASH: error: fatal error encountered while reading from module 'Lib'; please file a bug report with your project and the crash log
+// CHECK-CRASH-LABEL: *** DESERIALIZATION FAILURE (please include this section in any bug report) ***
+// CHECK-CRASH: could not find 'method()' in parent class
+// CHECK-CRASH: While loading members for 'Sub' in module 'Lib'
+
+// CHECK-RECOVERY-LABEL: class Sub : Base {
+// CHECK-RECOVERY-NEXT: {{^}$}}

tools/swift-ide-test/swift-ide-test.cpp

@@ -322,6 +322,12 @@ EnableSwift3ObjCInference(
                   llvm::cl::desc("Enable Swift 3's @objc inference rules"),
                   llvm::cl::init(false));
 
+static llvm::cl::opt<bool>
+EnableDeserializationRecovery(
+    "enable-experimental-deserialization-recovery",
+    llvm::cl::desc("Attempt to recover from missing xrefs (etc) in swiftmodules"),
+    llvm::cl::init(false));
+
 static llvm::cl::opt<bool>
 DisableObjCAttrRequiresFoundationModule(
     "disable-objc-attr-requires-foundation-module",
@@ -3004,6 +3010,8 @@ int main(int argc, char *argv[]) {
   InitInvok.getLangOptions().EnableSwift3ObjCInference =
     options::EnableSwift3ObjCInference ||
     InitInvok.getLangOptions().isSwiftVersion3();
+  InitInvok.getLangOptions().EnableDeserializationRecovery |=
+    options::EnableDeserializationRecovery;
   InitInvok.getClangImporterOptions().ImportForwardDeclarations |=
     options::ObjCForwardDeclarations;
   InitInvok.getClangImporterOptions().InferImportAsMember |=