From 462ce0584dfa7aecab8bfd8a5c52218039bdc19a Mon Sep 17 00:00:00 2001
From: Ben Barham <ben_barham@apple.com>
Date: Fri, 6 Aug 2021 13:00:47 +1000
Subject: [PATCH] [Serialization] Properly skip invalid destructors when
 allowing errors

7856f2d83da7fc213fb3c16676cd953b8e9ff369 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.
---
 lib/Serialization/Serialization.cpp           | 27 +++++++++++++++----
 .../AllowErrors/invalid-deinit.swift          | 21 +++++++++++++++
 .../AllowErrors/invalid-deinit.swift          |  8 ------
 3 files changed, 43 insertions(+), 13 deletions(-)
 create mode 100644 test/Serialization/AllowErrors/invalid-deinit.swift
 delete mode 100644 validation-test/Serialization/AllowErrors/invalid-deinit.swift

diff --git a/lib/Serialization/Serialization.cpp b/lib/Serialization/Serialization.cpp
index fcdabc69e0a..dc7729b4dc9 100644
--- a/lib/Serialization/Serialization.cpp
+++ b/lib/Serialization/Serialization.cpp
@@ -3957,9 +3957,6 @@ public:
     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 +3998,34 @@ public:
   }
 };
 
+/// 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 +4035,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;
diff --git a/test/Serialization/AllowErrors/invalid-deinit.swift b/test/Serialization/AllowErrors/invalid-deinit.swift
new file mode 100644
index 00000000000..f49ea87c909
--- /dev/null
+++ b/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}}
diff --git a/validation-test/Serialization/AllowErrors/invalid-deinit.swift b/validation-test/Serialization/AllowErrors/invalid-deinit.swift
deleted file mode 100644
index 17dc6e06f4b..00000000000
--- a/validation-test/Serialization/AllowErrors/invalid-deinit.swift
+++ /dev/null
@@ -1,8 +0,0 @@
-// RUN: %empty-directory(%t)
-
-// RUN: %target-swift-frontend -module-name errors -emit-module -o %t/errors.swiftmodule -experimental-allow-module-with-compiler-errors %s
-
-// deinit is only valid on a class, make sure we don't crash when allowing
-// errors
-
-deinit {}