Merge pull request #16655 from davezarzycki/formalize_DeclIsBeingValidatedRAII

[AST] NFC: Formalize Decl validation tracking via RAII

Author: slavapestov

include/swift/AST/Decl.h

@@ -253,9 +253,11 @@ bool conflicting(ASTContext &ctx,
 
 /// Decl - Base class for all declarations in Swift.
 class alignas(1 << DeclAlignInBits) Decl {
+public:
   enum class ValidationState {
     Unchecked,
     Checking,
+    CheckingWithValidSignature,
     Checked,
   };
 
@@ -796,33 +798,51 @@ class alignas(1 << DeclAlignInBits) Decl {
     Bits.Decl.EarlyAttrValidation = validated;
   }
 
-  /// Whether the declaration has a valid interface type and
-  /// generic signature.
-  bool isBeingValidated() const {
-    return Bits.Decl.ValidationState == unsigned(ValidationState::Checking);
+  /// Get the validation state.
+  ValidationState getValidationState() const {
+    return ValidationState(Bits.Decl.ValidationState);
   }
 
-  /// Toggle whether or not the declaration is being validated.
-  void setIsBeingValidated(bool ibv = true) {
-    if (ibv) {
-      assert(Bits.Decl.ValidationState == unsigned(ValidationState::Unchecked));
-      Bits.Decl.ValidationState = unsigned(ValidationState::Checking);
-    } else {
-      assert(Bits.Decl.ValidationState == unsigned(ValidationState::Checking));
-      Bits.Decl.ValidationState = unsigned(ValidationState::Checked);
+private:
+  friend class DeclValidationRAII;
+
+  /// Set the validation state.
+  void setValidationState(ValidationState VS) {
+    assert(VS > getValidationState() && "Validation is unidirectional");
+    Bits.Decl.ValidationState = unsigned(VS);
+  }
+
+public:
+  /// Whether the declaration is in the middle of validation or not.
+  bool isBeingValidated() const {
+    switch (getValidationState()) {
+    case ValidationState::Unchecked:
+    case ValidationState::Checked:
+      return false;
+    default:
+      return true;
     }
   }
 
+  /// Update the validation state for the declaration to allow access to the
+  /// generic signature.
+  void setSignatureIsValidated() {
+    assert(getValidationState() == ValidationState::Checking);
+    setValidationState(ValidationState::CheckingWithValidSignature);
+  }
+
   bool hasValidationStarted() const {
-    return Bits.Decl.ValidationState >= unsigned(ValidationState::Checking);
+    return getValidationState() > ValidationState::Unchecked;
   }
 
-  /// Manually indicate that validation has started for the declaration.
+  /// Manually indicate that validation is complete for the declaration. For
+  /// example: during importing, code synthesis, or derived conformances.
   ///
-  /// This is implied by setIsBeingValidated(true) (i.e. starting validation)
-  /// and so rarely needs to be called directly.
-  void setValidationStarted() {
-    if (Bits.Decl.ValidationState != unsigned(ValidationState::Checking))
+  /// For normal code validation, please use DeclValidationRAII instead.
+  ///
+  /// FIXME -- Everything should use DeclValidationRAII instead of this.
+  void setValidationToChecked() {
+    if (!isBeingValidated())
       Bits.Decl.ValidationState = unsigned(ValidationState::Checked);
   }
 
@@ -924,6 +944,25 @@ class alignas(1 << DeclAlignInBits) Decl {
   }
 };
 
+/// \brief Use RAII to track Decl validation progress and non-reentrancy.
+class DeclValidationRAII {
+  Decl *D;
+
+public:
+  DeclValidationRAII(const DeclValidationRAII &) = delete;
+  DeclValidationRAII(DeclValidationRAII &&) = delete;
+  void operator =(const DeclValidationRAII &) = delete;
+  void operator =(DeclValidationRAII &&) = delete;
+
+  DeclValidationRAII(Decl *decl) : D(decl) {
+    D->setValidationState(Decl::ValidationState::Checking);
+  }
+
+  ~DeclValidationRAII() {
+    D->setValidationState(Decl::ValidationState::Checked);
+  }
+};
+
 /// \brief Allocates memory for a Decl with the given \p baseSize. If necessary,
 /// it includes additional space immediately preceding the Decl for a ClangNode.
 /// \note \p baseSize does not need to include space for a ClangNode if
@@ -1677,9 +1716,9 @@ class ExtensionDecl final : public GenericContext, public Decl,
   /// Retrieve one of the types listed in the "inherited" clause.
   Type getInheritedType(unsigned index) const;
 
-  /// Whether we have fully checked the extension.
+  /// Whether we have fully checked the extension signature.
   bool hasValidSignature() const {
-    return hasValidationStarted() && !isBeingValidated();
+    return getValidationState() > ValidationState::CheckingWithValidSignature;
   }
 
   bool hasDefaultAccessLevel() const {

lib/AST/Builtins.cpp

@@ -164,7 +164,7 @@ getBuiltinFunction(Identifier Id, ArrayRef<Type> argTypes, Type ResType,
         ParamDecl(VarDecl::Specifier::Default, SourceLoc(), SourceLoc(),
                   Identifier(), SourceLoc(), Identifier(), argType, DC);
     PD->setInterfaceType(argType);
-    PD->setValidationStarted();
+    PD->setValidationToChecked();
     PD->setImplicit();
     params.push_back(PD);
   }
@@ -181,7 +181,7 @@ getBuiltinFunction(Identifier Id, ArrayRef<Type> argTypes, Type ResType,
                              paramList,
                              TypeLoc::withoutLoc(ResType), DC);
   FD->setInterfaceType(FnType);
-  FD->setValidationStarted();
+  FD->setValidationToChecked();
   FD->setImplicit();
   FD->setAccess(AccessLevel::Public);
   return FD;
@@ -228,7 +228,7 @@ getBuiltinGenericFunction(Identifier Id,
                                       Identifier(),
                                       paramType->getInOutObjectType(), DC);
     PD->setInterfaceType(paramIfaceType->getInOutObjectType());
-    PD->setValidationStarted();
+    PD->setValidationToChecked();
     PD->setImplicit();
     params.push_back(PD);
   }
@@ -246,7 +246,7 @@ getBuiltinGenericFunction(Identifier Id,
                                TypeLoc::withoutLoc(ResBodyType), DC);
 
   func->setInterfaceType(InterfaceType);
-  func->setValidationStarted();
+  func->setValidationToChecked();
   func->setGenericEnvironment(Env);
   func->setImplicit();
   func->setAccess(AccessLevel::Public);

lib/AST/Decl.cpp

@@ -2123,7 +2123,11 @@ void ValueDecl::setInterfaceType(Type type) {
 }
 
 bool ValueDecl::hasValidSignature() const {
-  return hasInterfaceType() && !isBeingValidated();
+  if (!hasInterfaceType())
+    return false;
+  // FIXME -- The build blows up if the correct code is used:
+  // return getValidationState() > ValidationState::CheckingWithValidSignature;
+  return getValidationState() != ValidationState::Checking;
 }
 
 Optional<ObjCSelector> ValueDecl::getObjCRuntimeName() const {
@@ -2727,7 +2731,7 @@ SourceRange TypeAliasDecl::getSourceRange() const {
 }
 
 void TypeAliasDecl::setUnderlyingType(Type underlying) {
-  setValidationStarted();
+  setValidationToChecked();
 
   // lldb creates global typealiases containing archetypes
   // sometimes...
@@ -2945,7 +2949,7 @@ void ClassDecl::addImplicitDestructor() {
   auto *DD = new (ctx) DestructorDecl(getLoc(), selfDecl, this);
 
   DD->setImplicit();
-  DD->setValidationStarted();
+  DD->setValidationToChecked();
 
   // Create an empty body for the destructor.
   DD->setBody(BraceStmt::create(ctx, getLoc(), { }, getLoc(), true));
@@ -4527,7 +4531,7 @@ ParamDecl *ParamDecl::createSelf(SourceLoc loc, DeclContext *DC,
                                      Identifier(), loc, C.Id_self, Type(), DC);
   selfDecl->setImplicit();
   selfDecl->setInterfaceType(selfInterfaceType);
-  selfDecl->setValidationStarted();
+  selfDecl->setValidationToChecked();
   return selfDecl;
 }
 

lib/AST/Module.cpp

@@ -360,7 +360,7 @@ ModuleDecl::ModuleDecl(Identifier name, ASTContext &ctx)
   setInterfaceType(ModuleType::get(this));
 
   // validateDecl() should return immediately given a ModuleDecl.
-  setValidationStarted();
+  setValidationToChecked();
 
   setAccess(AccessLevel::Public);
 }