[Serialization] Properly skip invalid destructors when allowing errors

7856f2d only partially skipped writing
out destructors when they were invalid, ie. it skipped writing the decl
itself but not the records common to all decls. This would cause any
records on the destructor to be applied on the next serialized decl.

Make sure to skip serializing anything to do with the destructor when
it's invalid and does not have a class context.

Author: bnbarham

lib/Serialization/Serialization.cpp

@@ -3929,9 +3929,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];
@@ -3973,12 +3970,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.
@@ -3988,8 +4007,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}}