Merge pull request #38779 from apple/serialization-crashes

Fix serialization crashes found from stress tester

Author: bnbarham

lib/Serialization/Serialization.cpp

@@ -2084,8 +2084,10 @@ void Serializer::writePatternBindingInitializer(PatternBindingDecl *binding,
   StringRef initStr;
   SmallString<128> scratch;
   auto varDecl = binding->getAnchoringVarDecl(bindingIndex);
+  assert((varDecl || allowCompilerErrors()) &&
+         "Serializing PDB without anchoring VarDecl");
   if (binding->hasInitStringRepresentation(bindingIndex) &&
-      varDecl->isInitExposedToClients()) {
+      varDecl && varDecl->isInitExposedToClients()) {
     initStr = binding->getInitStringRepresentation(bindingIndex, scratch);
   }
 
@@ -3957,9 +3959,6 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> {
     using namespace decls_block;
     verifyAttrSerializable(dtor);
 
-    if (S.allowCompilerErrors() && dtor->isInvalid())
-      return;
-
     auto contextID = S.addDeclContextRef(dtor->getDeclContext());
 
     unsigned abbrCode = S.DeclTypeAbbrCodes[DestructorLayout::Code];
@@ -4001,12 +4000,34 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> {
   }
 };
 
+/// When allowing modules with errors there may be cases where there's little
+/// point in serializing a declaration and doing so would create a maintenance
+/// burden on the deserialization side. Returns \c true if the given declaration
+/// should be skipped and \c false otherwise.
+static bool canSkipWhenInvalid(const Decl *D) {
+  // There's no point writing out the deinit when its context is not a class
+  // as nothing would be able to reference it
+  if (auto *deinit = dyn_cast<DestructorDecl>(D)) {
+    if (!isa<ClassDecl>(D->getDeclContext()))
+      return true;
+  }
+  return false;
+}
+
 void Serializer::writeASTBlockEntity(const Decl *D) {
   using namespace decls_block;
 
   PrettyStackTraceDecl trace("serializing", D);
   assert(DeclsToSerialize.hasRef(D));
 
+  if (D->isInvalid()) {
+    assert(allowCompilerErrors() &&
+           "cannot create a module with an invalid decl");
+
+    if (canSkipWhenInvalid(D))
+      return;
+  }
+
   BitOffset initialOffset = Out.GetCurrentBitNo();
   SWIFT_DEFER {
     // This is important enough to leave on in Release builds.
@@ -4016,8 +4037,6 @@ void Serializer::writeASTBlockEntity(const Decl *D) {
     }
   };
 
-  assert((allowCompilerErrors() || !D->isInvalid()) &&
-         "cannot create a module with an invalid decl");
   if (isDeclXRef(D)) {
     writeCrossReference(D);
     return;

test/Serialization/AllowErrors/invalid-deinit.swift

@@ -0,0 +1,21 @@
+// RUN: %empty-directory(%t)
+
+// Serialize and deserialize a deinit with SourceFile context to make sure we
+// don't crash
+// RUN: %target-swift-frontend -verify -module-name errors -emit-module -o %t/errors.swiftmodule -experimental-allow-module-with-compiler-errors %s
+// RUN: %target-swift-ide-test -print-module -module-to-print=errors -source-filename=x -I %t -allow-compiler-errors
+
+// Also check it wasn't serialized
+// RUN: llvm-bcanalyzer -dump %t/errors.swiftmodule | %FileCheck %s
+// CHECK-NOT: DESTRUCTOR_DECL
+
+struct Foo {}
+
+@discardableResult // expected-error{{'@discardableResult' attribute cannot be applied to this declaration}}
+deinit {} // expected-error{{deinitializers may only be declared within a class}}
+
+func foo() -> Foo { return Foo() }
+
+// Make sure @discardableResult isn't added to `foo`, which could be possible
+// if the deinit is partially serialized
+foo() // expected-warning{{result of call to 'foo()' is unused}}

validation-test/Serialization/AllowErrors/invalid-deinit.swift

@@ -0,0 +1,7 @@
+// RUN: %empty-directory(%t)
+
+// -parse-as-library added so that the PDB isn't added to a TopLevelCodeDecl,
+// which isn't serialized at all
+// RUN: %target-swift-frontend -emit-module -o %t/errors.swiftmodule -module-name errors -experimental-allow-module-with-compiler-errors -parse-as-library %s
+
+let self = 1