Merge pull request swiftlang#36199 from bnbarham/add-error-flag

[Deserialization] Output a diagnostic for invalid decls or types

Author: bnbarham

include/swift/AST/Decl.h

@@ -678,6 +678,7 @@ class alignas(1 << DeclAlignInBits) Decl {
   friend class DeclIterator;
   friend class IterableDeclContext;
   friend class MemberLookupTable;
+  friend class DeclDeserializer;
 
 private:
   llvm::PointerUnion<DeclContext *, ASTContext *> Context;
@@ -694,6 +695,10 @@ class alignas(1 << DeclAlignInBits) Decl {
   };
   mutable CachedExternalSourceLocs const *CachedSerializedLocs = nullptr;
   const CachedExternalSourceLocs *getSerializedLocs() const;
+
+  /// Directly set the invalid bit
+  void setInvalidBit();
+
 protected:
 
   Decl(DeclKind kind, llvm::PointerUnion<DeclContext *, ASTContext *> context)

include/swift/AST/DiagnosticsSema.def

@@ -776,9 +776,18 @@ NOTE(serialization_compatibility_version_mismatch,none,
      "(this is supported but may expose additional compiler issues)",
      (StringRef, StringRef, StringRef))
 
+ERROR(serialization_invalid_decl,Fatal,
+      "deserialized invalid declaration %0 (%1) in module '%2'",
+      (DeclName, DescriptiveDeclKind, StringRef))
 ERROR(serialization_allowing_invalid_decl,none,
-      "allowing deserialization of invalid declaration %0 from module '%1'",
-      (DeclName, StringRef))
+      "allowing deserialization of invalid declaration %0 (%1) in module '%2'",
+      (DeclName, DescriptiveDeclKind, StringRef))
+ERROR(serialization_error_type,Fatal,
+      "deserialized error type '%0' in module '%1'",
+      (StringRef, StringRef))
+ERROR(serialization_allowing_error_type,none,
+      "allowing deserialization of error type '%0' in module '%1'",
+      (StringRef, StringRef))
 
 ERROR(reserved_member_name,none,
       "type member must not be named %0, since it would conflict with the"

lib/AST/Decl.cpp

@@ -453,6 +453,8 @@ bool Decl::isInvalid() const {
   llvm_unreachable("Unknown decl kind");
 }
 
+void Decl::setInvalidBit() { Bits.Decl.Invalid = true; }
+
 void Decl::setInvalid() {
   switch (getKind()) {
 #define VALUE_DECL(ID, PARENT)

lib/Serialization/DeclTypeRecordNodes.def

@@ -196,6 +196,8 @@ OTHER(CLANG_TYPE, 253)
 
 OTHER(DERIVATIVE_FUNCTION_CONFIGURATION, 254)
 
+OTHER(ERROR_FLAG, 255)
+
 #undef RECORD
 #undef DECLTYPERECORDNODES_HAS_RECORD_VAL
 #undef RECORD_VAL

lib/Serialization/Deserialization.cpp

@@ -2369,6 +2369,8 @@ class DeclDeserializer {
   ASTContext &ctx;
   Serialized<Decl *> &declOrOffset;
 
+  bool IsInvalid = false;
+
   DeclAttribute *DAttrs = nullptr;
   DeclAttribute **AttrsNext = &DAttrs;
 
@@ -2421,6 +2423,22 @@ class DeclDeserializer {
     if (!decl)
       return;
 
+    if (IsInvalid) {
+      decl->setInvalidBit();
+
+      DeclName name;
+      if (auto *VD = dyn_cast<ValueDecl>(decl)) {
+        name = VD->getName();
+      }
+
+      auto diagId = ctx.LangOpts.AllowModuleWithCompilerErrors
+                        ? diag::serialization_allowing_invalid_decl
+                        : diag::serialization_invalid_decl;
+      ctx.Diags.diagnose(SourceLoc(), diagId, name,
+                         decl->getDescriptiveKind(),
+                         MF.getAssociatedModule()->getNameStr());
+    }
+
     if (DAttrs)
       decl->getAttrs().setRawAttributeChain(DAttrs);
 
@@ -2436,10 +2454,9 @@ class DeclDeserializer {
     }
   }
 
-  /// Deserializes decl attribute and attribute-like records from
-  /// \c MF.DeclTypesCursor until a non-attribute record is found,
-  /// passing each one to AddAttribute.
-  llvm::Error deserializeDeclAttributes();
+  /// Deserializes records common to all decls from \c MF.DeclTypesCursor (ie.
+  /// the invalid flag, attributes, and discriminators)
+  llvm::Error deserializeDeclCommon();
 
   Expected<Decl *> getDeclCheckedImpl(
     llvm::function_ref<bool(DeclAttributes)> matchAttributes = nullptr);
@@ -4083,19 +4100,6 @@ ModuleFile::getDeclChecked(
         matchAttributes);
     if (!deserialized)
       return deserialized;
-
-    auto *decl = declOrOffset.get();
-    if (isAllowModuleWithCompilerErrorsEnabled() && decl->isInvalid()) {
-      if (!isa<ParamDecl>(decl) && !decl->isImplicit()) {
-        // The parent function will be invalid if the parameter is invalid,
-        // implicits should have an invalid explicit as well
-        if (auto *VD = dyn_cast<ValueDecl>(decl)) {
-          getContext().Diags.diagnose(
-              VD->getLoc(), diag::serialization_allowing_invalid_decl,
-              VD->getName(), VD->getModuleContext()->getNameStr());
-        }
-      }
-    }
   } else if (matchAttributes) {
     // Decl was cached but we may need to filter it
     if (!matchAttributes(declOrOffset.get()->getAttrs()))
@@ -4115,7 +4119,7 @@ ModuleFile::getDeclChecked(
   return declOrOffset;
 }
 
-llvm::Error DeclDeserializer::deserializeDeclAttributes() {
+llvm::Error DeclDeserializer::deserializeDeclCommon() {
   using namespace decls_block;
 
   SmallVector<uint64_t, 64> scratch;
@@ -4132,7 +4136,10 @@ llvm::Error DeclDeserializer::deserializeDeclAttributes() {
     unsigned recordID = MF.fatalIfUnexpected(
         MF.DeclTypeCursor.readRecord(entry.ID, scratch, &blobData));
 
-    if (isDeclAttrRecord(recordID)) {
+    if (recordID == ERROR_FLAG) {
+      assert(!IsInvalid && "Error flag written multiple times");
+      IsInvalid = true;
+    } else if (isDeclAttrRecord(recordID)) {
       DeclAttribute *Attr = nullptr;
       bool skipAttr = false;
       switch (recordID) {
@@ -4459,7 +4466,7 @@ llvm::Error DeclDeserializer::deserializeDeclAttributes() {
         // because it requires `DifferentiableAttr::setOriginalDeclaration` to
         // be called first. `DifferentiableAttr::setOriginalDeclaration` cannot
         // be called here because the original declaration is not accessible in
-        // this function (`DeclDeserializer::deserializeDeclAttributes`).
+        // this function (`DeclDeserializer::deserializeDeclCommon`).
         diffAttrParamIndicesMap[diffAttr] = indices;
         diffAttr->setDerivativeGenericSignature(derivativeGenSig);
         Attr = diffAttr;
@@ -4616,9 +4623,9 @@ Expected<Decl *>
 DeclDeserializer::getDeclCheckedImpl(
   llvm::function_ref<bool(DeclAttributes)> matchAttributes) {
 
-  auto attrError = deserializeDeclAttributes();
-  if (attrError)
-    return std::move(attrError);
+  auto commonError = deserializeDeclCommon();
+  if (commonError)
+    return std::move(commonError);
 
   if (matchAttributes) {
     // Deserialize the full decl only if matchAttributes finds a match.
@@ -5764,19 +5771,28 @@ class TypeDeserializer {
 
   Expected<Type> deserializeErrorType(ArrayRef<uint64_t> scratch,
                                       StringRef blobData) {
-    if (!MF.isAllowModuleWithCompilerErrorsEnabled())
-      MF.fatal();
-
     TypeID origID;
     decls_block::ErrorTypeLayout::readRecord(scratch, origID);
 
-    auto origTy = MF.getTypeChecked(origID);
-    if (!origTy)
-      return origTy.takeError();
+    auto origTyOrError = MF.getTypeChecked(origID);
+    if (!origTyOrError)
+      return origTyOrError.takeError();
+
+    auto origTy = *origTyOrError;
+    auto diagId = ctx.LangOpts.AllowModuleWithCompilerErrors
+                      ? diag::serialization_allowing_error_type
+                      : diag::serialization_error_type;
+    // Generally not a super useful diagnostic, so only output once if there
+    // hasn't been any other diagnostic yet to ensure nothing slips by and
+    // causes SILGen to crash.
+    if (!ctx.hadError()) {
+      ctx.Diags.diagnose(SourceLoc(), diagId, StringRef(origTy.getString()),
+                         MF.getAssociatedModule()->getNameStr());
+    }
 
-    if (!origTy.get())
+    if (!origTy)
       return ErrorType::get(ctx);
-    return ErrorType::get(origTy.get());
+    return ErrorType::get(origTy);
   }
 };
 }

lib/Serialization/ModuleFormat.h

@@ -56,7 +56,7 @@ const uint16_t SWIFTMODULE_VERSION_MAJOR = 0;
 /// describe what change you made. The content of this comment isn't important;
 /// it just ensures a conflict if two people change the module format.
 /// Don't worry about adhering to the 80-column limit for this line.
-const uint16_t SWIFTMODULE_VERSION_MINOR = 600; // custom attr (unsafe)
+const uint16_t SWIFTMODULE_VERSION_MINOR = 601; // diagnose invalid AST
 
 /// A standard hash seed used for all string hashes in a serialized module.
 ///
@@ -926,6 +926,11 @@ namespace decls_block {
     BCArray<BCVBR<6>>
   >;
 
+  /// A flag to mark a decl as being invalid
+  using ErrorFlagLayout = BCRecordLayout<
+    ERROR_FLAG
+  >;
+
   /// A placeholder for invalid types
   using ErrorTypeLayout = BCRecordLayout<
     ERROR_TYPE,

lib/Serialization/Serialization.cpp

@@ -3001,6 +3001,9 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> {
   }
 
   void visit(const Decl *D) {
+    if (D->isInvalid())
+      writeDeclErrorFlag();
+
     // Emit attributes (if any).
     for (auto Attr : D->getAttrs())
       writeDeclAttribute(D, Attr);
@@ -3015,6 +3018,12 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> {
     DeclVisitor<DeclSerializer>::visit(const_cast<Decl *>(D));
   }
 
+  void writeDeclErrorFlag() {
+    using namespace decls_block;
+    unsigned abbrCode = S.DeclTypeAbbrCodes[ErrorFlagLayout::Code];
+    ErrorFlagLayout::emitRecord(S.Out, S.ScratchRecord, abbrCode);
+  }
+
   void noteUseOfExportedPrespecialization(const AbstractFunctionDecl *afd) {
     for (auto *A : afd->getAttrs().getAttributes<SpecializeAttr>()) {
       auto *SA = cast<SpecializeAttr>(A);
@@ -4675,6 +4684,8 @@ void Serializer::writeAllDeclsAndTypes() {
   registerDeclTypeAbbr<UnboundGenericTypeLayout>();
   registerDeclTypeAbbr<OptionalTypeLayout>();
   registerDeclTypeAbbr<DynamicSelfTypeLayout>();
+
+  registerDeclTypeAbbr<ErrorFlagLayout>();
   registerDeclTypeAbbr<ErrorTypeLayout>();
 
   registerDeclTypeAbbr<ClangTypeLayout>();

test/Frontend/allow-errors.swift

@@ -68,6 +68,8 @@ func test(s: ValidStructInvalidMember) {
 // there were no errors)
 func other() -> Int {}
 // CHECK-VALID: allow-errors.swift:[[@LINE-1]]:22: error: missing return in a function expected to return 'Int'
+func other2() -> Bool {}
+// CHECK-VALID: allow-errors.swift:[[@LINE-1]]:24: error: missing return in a function expected to return 'Bool'
 #endif
 
 // All invalid uses should have no errors in the file itself, all referenced
@@ -82,7 +84,8 @@ func test() {
   invalidFunc()
 }
 // CHECK-INVALID-TOP-NOT: allow-errors.swift:{{.*}} error:
-// CHECK-INVALID-TOP: error: allowing deserialization of invalid declaration 'invalidFunc()' from module 'errors'
+// CHECK-INVALID-TOP: error: allowing deserialization of error type '<null>' in module 'errors'
+// CHECK-INVALID-TOP: error: allowing deserialization of invalid declaration 'invalidFunc()' (global function) in module 'errors'
 // CHECK-INVALID-TOP-NOT: allow-errors.swift:{{.*}} error:
 #endif
 
@@ -94,7 +97,9 @@ func test(s: ValidStructInvalidMember) {
   print(s.memberMissingType)
 }
 // CHECK-INVALID-MEMBER-NOT: allow-errors.swift:{{.*}} error:
-// CHECK-INVALID-MEMBER: error: allowing deserialization of invalid declaration 'memberMissingType' from module 'errors'
+// CHECK-INVALID-MEMBER: error: allowing deserialization of error type '<null>' in module 'errors'
+// CHECK-INVALID-MEMBER: error: allowing deserialization of invalid declaration '_' (getter) in module 'errors'
+// CHECK-INVALID-MEMBER: error: allowing deserialization of invalid declaration 'memberMissingType' (property) in module 'errors'
 // CHECK-INVALID-MEMBER-NOT: allow-errors.swift:{{.*}} error:
 #endif
 
@@ -106,6 +111,8 @@ func test(s: ValidStructInvalidMember) {
   s.funcBadArg()
 }
 // CHECK-INVALID-METHOD-NOT: allow-errors.swift:{{.*}} error:
-// CHECK-INVALID-METHOD: error: allowing deserialization of invalid declaration 'funcBadArg' from module 'errors'
+// CHECK-INVALID-METHOD: error: allowing deserialization of error type '<null>' in module 'errors'
+// CHECK-INVALID-METHOD: error: allowing deserialization of invalid declaration 'arg' (parameter) in module 'errors'
+// CHECK-INVALID-METHOD: error: allowing deserialization of invalid declaration 'funcBadArg' (instance method) in module 'errors'
 // CHECK-INVALID-METHOD-NOT: allow-errors.swift:{{.*}} error:
 #endif