Merge pull request #79851 from xymus/deser-recover-conformance-xref

Serialization: Diagnose broken conformances from stale swiftmodule files

Author: xymus

include/swift/AST/ASTContext.h

@@ -381,11 +381,10 @@ class ASTContext final {
   /// files?
   bool IgnoreAdjacentModules = false;
 
-  /// Override to accept reading errors from swiftmodules and being generally
-  /// more tolerant to inconsistencies. This is enabled for
-  /// index-while-building as it runs last and it can afford to read an AST
-  /// with inconsistencies.
-  bool ForceAllowModuleWithCompilerErrors = false;
+  /// Accept recovering from more issues at deserialization, even if it can
+  /// lead to an inconsistent state. This is enabled for index-while-building
+  /// as it runs last and it can afford to read an AST with inconsistencies.
+  bool ForceExtendedDeserializationRecovery = false;
 
   // Define the set of known identifiers.
 #define IDENTIFIER_WITH_NAME(Name, IdStr) Identifier Id_##Name;

include/swift/AST/DiagnosticsSema.def

@@ -1037,6 +1037,18 @@ NOTE(modularization_issue_worked_around,none,
      "-experimental-force-workaround-broken-modules",
      ())
 
+ERROR(modularization_issue_conformance_xref_error,none,
+      "Conformance of %0 to %1 not found in referenced module %2",
+      (DeclName, DeclName, DeclName))
+NOTE(modularization_issue_conformance_xref_note,none,
+     "Breaks conformances of '%0' to %1",
+     (StringRef, DeclName))
+ERROR(modularization_issue_conformance_error,none,
+      "Conformances of '%0' "
+      "do not match requirement signature of %1; "
+      "%2 conformances for %3 requirements",
+      (StringRef, DeclName, unsigned int, unsigned int))
+
 ERROR(reserved_member_name,none,
       "type member must not be named %0, since it would conflict with the"
       " 'foo.%1' expression", (const ValueDecl *, StringRef))

lib/Index/Index.cpp

@@ -2168,7 +2168,7 @@ void index::indexModule(ModuleDecl *module, IndexDataConsumer &consumer) {
     return;
   }
 
-  llvm::SaveAndRestore<bool> S(ctx.ForceAllowModuleWithCompilerErrors, true);
+  llvm::SaveAndRestore<bool> S(ctx.ForceExtendedDeserializationRecovery, true);
 
   IndexSwiftASTWalker walker(consumer, ctx);
   walker.visitModule(*module);

lib/Serialization/Deserialization.cpp

@@ -179,6 +179,8 @@ const char ModularizationError::ID = '\0';
 void ModularizationError::anchor() {}
 const char InvalidEnumValueError::ID = '\0';
 void InvalidEnumValueError::anchor() {}
+const char ConformanceXRefError::ID = '\0';
+void ConformanceXRefError::anchor() {}
 
 /// Skips a single record in the bitstream.
 ///
@@ -404,6 +406,16 @@ ModuleFile::diagnoseModularizationError(llvm::Error error,
   return outError;
 }
 
+void
+ConformanceXRefError::diagnose(const ModuleFile *MF,
+                               DiagnosticBehavior limit) const {
+  auto &diags = MF->getContext().Diags;
+  diags.diagnose(MF->getSourceLoc(),
+                 diag::modularization_issue_conformance_xref_error,
+                 name, protoName, expectedModule->getName())
+    .limitBehavior(limit);
+}
+
 llvm::Error ModuleFile::diagnoseFatal(llvm::Error error) const {
 
   auto &ctx = getContext();
@@ -860,7 +872,8 @@ ProtocolConformanceDeserializer::readSpecializedProtocolConformance(
     return subMapOrError.takeError();
   auto subMap = subMapOrError.get();
 
-  auto genericConformance = MF.getConformance(conformanceID);
+  ProtocolConformanceRef genericConformance;
+  UNWRAP(MF.getConformanceChecked(conformanceID), genericConformance);
 
   PrettyStackTraceDecl traceTo("... to", genericConformance.getRequirement());
   ++NumNormalProtocolConformancesLoaded;
@@ -892,8 +905,8 @@ ProtocolConformanceDeserializer::readInheritedProtocolConformance(
   PrettyStackTraceType trace(ctx, "reading inherited conformance for",
                              conformingType);
 
-  ProtocolConformanceRef inheritedConformance =
-    MF.getConformance(conformanceID);
+  ProtocolConformanceRef inheritedConformance;
+  UNWRAP(MF.getConformanceChecked(conformanceID), inheritedConformance);
   PrettyStackTraceDecl traceTo("... to",
                                inheritedConformance.getRequirement());
 
@@ -943,14 +956,16 @@ ProtocolConformanceDeserializer::readNormalProtocolConformanceXRef(
   ProtocolConformanceXrefLayout::readRecord(scratch, protoID, nominalID,
                                             moduleID);
 
-  auto maybeNominal = MF.getDeclChecked(nominalID);
-  if (!maybeNominal)
-    return maybeNominal.takeError();
-
-  auto nominal = cast<NominalTypeDecl>(maybeNominal.get());
+  Decl *maybeNominal;
+  UNWRAP(MF.getDeclChecked(nominalID), maybeNominal);
+  auto nominal = cast<NominalTypeDecl>(maybeNominal);
   PrettyStackTraceDecl trace("cross-referencing conformance for", nominal);
-  auto proto = cast<ProtocolDecl>(MF.getDecl(protoID));
+
+  Decl *maybeProto;
+  UNWRAP(MF.getDeclChecked(protoID), maybeProto);
+  auto proto = cast<ProtocolDecl>(maybeProto);
   PrettyStackTraceDecl traceTo("... to", proto);
+
   auto module = MF.getModule(moduleID);
 
   // FIXME: If the module hasn't been loaded, we probably don't want to fall
@@ -971,16 +986,26 @@ ProtocolConformanceDeserializer::readNormalProtocolConformanceXRef(
   // TODO: Sink Sendable derivation into the conformance lookup table
   if (proto->isSpecificProtocol(KnownProtocolKind::Sendable)) {
     auto conformanceRef = lookupConformance(nominal->getDeclaredInterfaceType(), proto);
-    if (!conformanceRef.isConcrete())
-      abort();
-    return conformanceRef.getConcrete();
+    if (conformanceRef.isConcrete())
+      return conformanceRef.getConcrete();
   } else {
     SmallVector<ProtocolConformance *, 2> conformances;
     nominal->lookupConformance(proto, conformances);
-    if (conformances.empty())
-      abort();
-    return conformances.front();
+    if (!conformances.empty())
+      return conformances.front();
   }
+
+  auto error = llvm::make_error<ConformanceXRefError>(
+                 nominal->getName(), proto->getName(), module);
+
+  if (!MF.enableExtendedDeserializationRecovery()) {
+    error = llvm::handleErrors(std::move(error),
+      [&](const ConformanceXRefError &error) -> llvm::Error {
+        error.diagnose(&MF);
+        return llvm::make_error<ConformanceXRefError>(std::move(error));
+      });
+  }
+  return error;
 }
 
 Expected<ProtocolConformance *>
@@ -7867,7 +7892,7 @@ Expected<Type> DESERIALIZE_TYPE(SIL_FUNCTION_TYPE)(
   ProtocolConformanceRef witnessMethodConformance;
   if (*representation == swift::SILFunctionTypeRepresentation::WitnessMethod) {
     auto conformanceID = variableData[nextVariableDataIndex++];
-    witnessMethodConformance = MF.getConformance(conformanceID);
+    UNWRAP(MF.getConformanceChecked(conformanceID), witnessMethodConformance);
   }
 
   GenericSignature invocationSig =
@@ -8647,21 +8672,37 @@ void ModuleFile::finishNormalConformance(NormalProtocolConformance *conformance,
       scratch, protoID, contextID, typeCount, valueCount, conformanceCount,
       rawOptions, rawIDs);
 
+  const ProtocolDecl *proto = conformance->getProtocol();
+
   // Read requirement signature conformances.
   SmallVector<ProtocolConformanceRef, 4> reqConformances;
   for (auto conformanceID : rawIDs.slice(0, conformanceCount)) {
     auto maybeConformance = getConformanceChecked(conformanceID);
     if (maybeConformance) {
       reqConformances.push_back(maybeConformance.get());
     } else if (getContext().LangOpts.EnableDeserializationRecovery) {
-      diagnoseAndConsumeError(maybeConformance.takeError());
+      llvm::Error error = maybeConformance.takeError();
+      if (error.isA<ConformanceXRefError>() &&
+          !enableExtendedDeserializationRecovery()) {
+
+        std::string typeStr = conformance->getType()->getString();
+        auto &diags = getContext().Diags;
+        diags.diagnose(getSourceLoc(),
+                       diag::modularization_issue_conformance_xref_note,
+                       typeStr, proto->getName());
+
+        consumeError(std::move(error));
+        conformance->setInvalid();
+        return;
+      }
+
+      diagnoseAndConsumeError(std::move(error));
       reqConformances.push_back(ProtocolConformanceRef::forInvalid());
     } else {
       fatal(maybeConformance.takeError());
     }
   }
 
-  const ProtocolDecl *proto = conformance->getProtocol();
   if (proto->isObjC() && getContext().LangOpts.EnableDeserializationRecovery) {
     // Don't crash if inherited protocols are added or removed.
     // This is limited to Objective-C protocols because we know their only
@@ -8702,12 +8743,39 @@ void ModuleFile::finishNormalConformance(NormalProtocolConformance *conformance,
       return req.getKind() == RequirementKind::Conformance;
     };
     auto requirements = proto->getRequirementSignature().getRequirements();
-    if (!allowCompilerErrors() &&
-        conformanceCount != llvm::count_if(requirements,
-                                           isConformanceReq)) {
-      fatal(llvm::make_error<llvm::StringError>(
-          "serialized conformances do not match requirement signature",
-          llvm::inconvertibleErrorCode()));
+    unsigned int conformanceRequirementCount =
+      llvm::count_if(requirements, isConformanceReq);
+    if (conformanceCount != conformanceRequirementCount) {
+      // Mismatch between the number of loaded conformances and the expected
+      // requirements. One or the other likely comes from a stale module.
+
+      if (!enableExtendedDeserializationRecovery()) {
+        // Error and print full context for visual inspection.
+        ASTContext &ctx = getContext();
+        std::string typeStr = conformance->getType()->getString();
+        ctx.Diags.diagnose(getSourceLoc(),
+                       diag::modularization_issue_conformance_error,
+                       typeStr, proto->getName(), conformanceCount,
+                       conformanceRequirementCount);
+        ctx.Diags.flushConsumers();
+
+        // Print context to stderr.
+        PrintOptions Opts;
+        llvm::errs() << "Requirements:\n";
+        for (auto req: requirements) {
+          req.print(llvm::errs(), Opts);
+          llvm::errs() << "\n";
+        }
+
+        llvm::errs() << "Conformances:\n";
+        for (auto req: reqConformances) {
+          req.print(llvm::errs());
+          llvm::errs() << "\n";
+        }
+      }
+
+      conformance->setInvalid();
+      return;
     }
   }
 

lib/Serialization/DeserializationErrors.h

@@ -677,6 +677,36 @@ class InvalidEnumValueError
   }
 };
 
+// Cross-reference to a conformance was not found where it was expected.
+class ConformanceXRefError : public llvm::ErrorInfo<ConformanceXRefError,
+                                                    DeclDeserializationError> {
+  friend ErrorInfo;
+  static const char ID;
+  void anchor() override;
+
+  DeclName protoName;
+  const ModuleDecl *expectedModule;
+
+public:
+  ConformanceXRefError(Identifier name, Identifier protoName,
+                       const ModuleDecl *expectedModule):
+                     protoName(protoName), expectedModule(expectedModule) {
+    this->name = name;
+  }
+
+  void diagnose(const ModuleFile *MF,
+                DiagnosticBehavior limit = DiagnosticBehavior::Fatal) const;
+
+  void log(raw_ostream &OS) const override {
+    OS << "Conformance of '" << name << "' to '" << protoName
+       << "' not found in module '" << expectedModule->getName() << "'";
+  }
+
+  std::error_code convertToErrorCode() const override {
+    return llvm::inconvertibleErrorCode();
+  }
+};
+
 class PrettyStackTraceModuleFile : public llvm::PrettyStackTraceEntry {
   const char *Action;
   const ModuleFile &MF;

lib/Serialization/ModuleFile.cpp

@@ -135,8 +135,15 @@ ModuleFile::ModuleFile(std::shared_ptr<const ModuleFileSharedCore> core)
 }
 
 bool ModuleFile::allowCompilerErrors() const {
-  return getContext().LangOpts.AllowModuleWithCompilerErrors ||
-         getContext().ForceAllowModuleWithCompilerErrors;
+  return getContext().LangOpts.AllowModuleWithCompilerErrors;
+}
+
+bool ModuleFile::enableExtendedDeserializationRecovery() const {
+  ASTContext &ctx = getContext();
+  return ctx.LangOpts.EnableDeserializationRecovery &&
+         (allowCompilerErrors() ||
+          ctx.LangOpts.DebuggerSupport ||
+          ctx.ForceExtendedDeserializationRecovery);
 }
 
 Status

lib/Serialization/ModuleFile.h

@@ -691,6 +691,10 @@ class ModuleFile
   /// '-experimental-allow-module-with-compiler-errors' is currently enabled).
   bool allowCompilerErrors() const;
 
+  /// Allow recovering from errors that could be unsafe when compiling
+  /// the binary. Useful for the debugger and IDE support tools.
+  bool enableExtendedDeserializationRecovery() const;
+
   /// \c true if this module has incremental dependency information.
   bool hasIncrementalInfo() const { return Core->hasIncrementalInfo(); }