[Deserialization] Output a diagnostic when deserializing an invalid decl

The previous change 47b068d was
partially reverted (when not allowing errors) as it output diagnostics
for valid code.

`Decl::isInvalid` had a special case for `AccessorDecl` where if its
interface type hadn't already been computed, it would fallback to
checking whether its storage (ie. its `VarDecl`) was valid. Since the
`VarDecl` isn't fully deserialized when deserializing the
`AccessorDecl`, this sometimes causes `isInvalid` to be true, even
though it isn't actually invalid.

The fallback introduced to `isInvalid` was to prevent a cycle during
typechecking caused by `SimpleDidSetRequest` - during `didSet`
typechecking, the body would be typechecked, which would then check
whether the declaration is valid, calling the typechecking for the
declaration again (and so on).

This change removes the special case from `isInvalid`. The result of
`isInvalid` is then correct for `AccessorDecl`, preventing spurious
diagnostics.

The `isInvalid` check in `performAbstractFuncDeclDiagnostics` has been
removed to prevent the cycle there - it works perfectly fine without it.
The unused variable diagnostics are skipped on seeing an error in the
body anyway. The opaque type matching diagnostics make sense to keep
when the return type is valid.

Removing that check exposes another cycle when computing captures in
body typechecking caused by `shouldMarkAsObjC` checking `isInvalid` as
well. It already checks for `didSet`, so have moved that check above the
`isInvalid` check.

Resolves rdar://74541834

Author: bnbarham

include/swift/AST/DiagnosticsSema.def

@@ -776,6 +776,9 @@ NOTE(serialization_compatibility_version_mismatch,none,
      "(this is supported but may expose additional compiler issues)",
      (StringRef, StringRef, StringRef))
 
+ERROR(serialization_invalid_decl,none,
+      "deserialization of invalid declaration %0 from module '%1'",
+      (DeclName, StringRef))
 ERROR(serialization_allowing_invalid_decl,none,
       "allowing deserialization of invalid declaration %0 from module '%1'",
       (DeclName, StringRef))

lib/AST/Decl.cpp

@@ -439,15 +439,9 @@ bool Decl::isInvalid() const {
   case DeclKind::Constructor:
   case DeclKind::Destructor:
   case DeclKind::Func:
+  case DeclKind::Accessor:
   case DeclKind::EnumElement:
     return cast<ValueDecl>(this)->getInterfaceType()->hasError();
-
-  case DeclKind::Accessor: {
-    auto *AD = cast<AccessorDecl>(this);
-    if (AD->hasInterfaceType() && AD->getInterfaceType()->hasError())
-      return true;
-    return AD->getStorage()->isInvalid();
-  }
   }
 
   llvm_unreachable("Unknown decl kind");

lib/Sema/MiscDiagnostics.cpp

@@ -3242,9 +3242,9 @@ performTopLevelDeclDiagnostics(TopLevelCodeDecl *TLCD) {
 /// Perform diagnostics for func/init/deinit declarations.
 void swift::performAbstractFuncDeclDiagnostics(AbstractFunctionDecl *AFD) {
   // Don't produce these diagnostics for implicitly generated code.
-  if (AFD->getLoc().isInvalid() || AFD->isImplicit() || AFD->isInvalid())
+  if (AFD->getLoc().isInvalid() || AFD->isImplicit())
     return;
-  
+
   // Check for unused variables, as well as variables that are could be
   // declared as constants. Skip local functions though, since they will
   // be checked as part of their parent function or TopLevelCodeDecl.

lib/Sema/TypeCheckDeclObjC.cpp

@@ -1248,18 +1248,6 @@ Optional<ObjCReason> shouldMarkAsObjC(const ValueDecl *VD, bool allowImplicit) {
 
   // Local function to determine whether we can implicitly infer @objc.
   auto canInferImplicitObjC = [&](bool allowAnyAccess) {
-    if (VD->isInvalid())
-      return false;
-    if (VD->isOperator())
-      return false;
-
-    // Implicitly generated declarations are not @objc, except for constructors.
-    if (!allowImplicit && VD->isImplicit())
-      return false;
-
-    if (!allowAnyAccess && VD->getFormalAccess() <= AccessLevel::FilePrivate)
-      return false;
-
     if (auto accessor = dyn_cast<AccessorDecl>(VD)) {
       switch (accessor->getAccessorKind()) {
       case AccessorKind::DidSet:
@@ -1275,6 +1263,19 @@ Optional<ObjCReason> shouldMarkAsObjC(const ValueDecl *VD, bool allowImplicit) {
         break;
       }
     }
+
+    if (VD->isInvalid())
+      return false;
+    if (VD->isOperator())
+      return false;
+
+    // Implicitly generated declarations are not @objc, except for constructors.
+    if (!allowImplicit && VD->isImplicit())
+      return false;
+
+    if (!allowAnyAccess && VD->getFormalAccess() <= AccessLevel::FilePrivate)
+      return false;
+
     return true;
   };
 

lib/Serialization/Deserialization.cpp

@@ -4085,15 +4085,23 @@ ModuleFile::getDeclChecked(
       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());
-        }
+    if (decl->isInvalid() && isa<ValueDecl>(decl)) {
+      auto *VD = cast<ValueDecl>(decl);
+      if (!isAllowModuleWithCompilerErrorsEnabled()) {
+        getContext().Diags.diagnose(
+            SourceLoc(), diag::serialization_invalid_decl,
+            VD->getName(), VD->getModuleContext()->getNameStr());
+      } else if (!isa<ParamDecl>(decl) && !decl->isImplicit()) {
+        // Grabbing the location of a parameter gets the location of its
+        // context, which isn't deserialized yet. Since the parent will be
+        // invalid if the parameter is, just skip it and wait for the
+        // parent to output the diagnostic.
+        //
+        // Any invalid implicit should have an invalid explicit, so ignore in
+        // preference for the explicit.
+        getContext().Diags.diagnose(
+            VD->getLoc(), diag::serialization_allowing_invalid_decl,
+            VD->getName(), VD->getModuleContext()->getNameStr());
       }
     }
   } else if (matchAttributes) {