Merge pull request swiftlang#66482 from DougGregor/requestify-has-storage

DougGregor · web-flow · commit bcb01ce659c4 · 2023-06-11T22:06:51.000-07:00
Requestify `AbstractStorageDecl::hasStorage()`.
diff --git a/include/swift/AST/Decl.h b/include/swift/AST/Decl.h
@@ -5293,10 +5293,10 @@ class AbstractStorageDecl : public ValueDecl {
 
   /// Overwrite the registered implementation-info.  This should be
   /// used carefully.
-  void setImplInfo(StorageImplInfo implInfo) {
-    LazySemanticInfo.ImplInfoComputed = 1;
-    ImplInfo = implInfo;
-  }
+  void setImplInfo(StorageImplInfo implInfo);
+
+  /// Cache the implementation-info, for use by the request-evaluator.
+  void cacheImplInfo(StorageImplInfo implInfo);
 
   ReadImplKind getReadImpl() const {
     return getImplInfo().getReadImpl();
@@ -5311,9 +5311,7 @@ class AbstractStorageDecl : public ValueDecl {
 
   /// Return true if this is a VarDecl that has storage associated with
   /// it.
-  bool hasStorage() const {
-    return getImplInfo().hasStorage();
-  }
+  bool hasStorage() const;
 
   /// Return true if this storage has the basic accessors/capability
   /// to be mutated.  This is generally constant after the accessors are
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -7181,6 +7181,13 @@ ERROR(invalid_macro_role_for_macro_syntax,none,
       (unsigned))
 ERROR(macro_cannot_introduce_names,none,
       "'%0' macros are not allowed to introduce names", (StringRef))
+ERROR(macro_accessor_missing_from_expansion,none,
+      "expansion of macro %0 did not produce a %select{non-|}1observing "
+      "accessor",
+      (DeclName, bool))
+ERROR(macro_init_accessor_not_documented,none,
+      "expansion of macro %0 produced an unexpected 'init' accessor",
+      (DeclName))
 
 ERROR(macro_resolve_circular_reference, none,
       "circular reference resolving %select{freestanding|attached}0 macro %1",
diff --git a/include/swift/AST/Evaluator.h b/include/swift/AST/Evaluator.h
@@ -316,6 +316,12 @@ class Evaluator {
     cache.insert<Request>(request, std::move(output));
   }
 
+  template<typename Request,
+           typename std::enable_if<!Request::hasExternalCache>::type* = nullptr>
+  bool hasCachedResult(const Request &request) {
+    return cache.find_as(request) != cache.end<Request>();
+  }
+
   /// Do not introduce new callers of this function.
   template<typename Request,
            typename std::enable_if<!Request::hasExternalCache>::type* = nullptr>
diff --git a/include/swift/AST/NameLookup.h b/include/swift/AST/NameLookup.h
@@ -556,6 +556,13 @@ void forEachPotentialResolvedMacro(
     llvm::function_ref<void(MacroDecl *, const MacroRoleAttr *)> body
 );
 
+/// For each macro with the given role that might be attached to the given
+/// declaration, call the body.
+void forEachPotentialAttachedMacro(
+    Decl *decl, MacroRole role,
+    llvm::function_ref<void(MacroDecl *macro, const MacroRoleAttr *)> body
+);
+
 } // end namespace namelookup
 
 /// Describes an inherited nominal entry.
diff --git a/include/swift/AST/TypeCheckRequests.h b/include/swift/AST/TypeCheckRequests.h
@@ -1686,6 +1686,26 @@ class InitAccessorPropertiesRequest :
   ArrayRef<VarDecl *>
   evaluate(Evaluator &evaluator, NominalTypeDecl *decl) const;
 
+  // Evaluation.
+  bool evaluate(Evaluator &evaluator, AbstractStorageDecl *decl) const;
+
+public:
+  bool isCached() const { return true; }
+};
+
+class HasStorageRequest :
+    public SimpleRequest<HasStorageRequest,
+                         bool(AbstractStorageDecl *),
+                         RequestFlags::Cached> {
+public:
+  using SimpleRequest::SimpleRequest;
+
+private:
+  friend SimpleRequest;
+
+  // Evaluation.
+  bool evaluate(Evaluator &evaluator, AbstractStorageDecl *decl) const;
+
 public:
   bool isCached() const { return true; }
 };
diff --git a/include/swift/AST/TypeCheckerTypeIDZone.def b/include/swift/AST/TypeCheckerTypeIDZone.def
@@ -302,6 +302,9 @@ SWIFT_REQUEST(TypeChecker, SelfAccessKindRequest, SelfAccessKind(FuncDecl *),
 SWIFT_REQUEST(TypeChecker, StorageImplInfoRequest,
               StorageImplInfo(AbstractStorageDecl *), SeparatelyCached,
               NoLocationInfo)
+SWIFT_REQUEST(TypeChecker, HasStorageRequest,
+              bool(AbstractStorageDecl *), Cached,
+              NoLocationInfo)
 SWIFT_REQUEST(TypeChecker, StoredPropertiesAndMissingMembersRequest,
               ArrayRef<Decl *>(NominalTypeDecl *), Cached, NoLocationInfo)
 SWIFT_REQUEST(TypeChecker, StoredPropertiesRequest,
diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp
@@ -2525,7 +2525,7 @@ AbstractStorageDecl::getAccessStrategy(AccessSemantics semantics,
                                        ResilienceExpansion expansion) const {
   switch (semantics) {
   case AccessSemantics::DirectToStorage:
-    assert(hasStorage());
+    assert(hasStorage() || getASTContext().Diags.hadAnyError());
     return AccessStrategy::getStorage();
 
   case AccessSemantics::DistributedThunk:
@@ -6393,13 +6393,40 @@ bool ProtocolDecl::hasCircularInheritedProtocols() const {
       ctx.evaluator, HasCircularInheritedProtocolsRequest{mutableThis}, true);
 }
 
+bool AbstractStorageDecl::hasStorage() const {
+  ASTContext &ctx = getASTContext();
+  return evaluateOrDefault(ctx.evaluator,
+    HasStorageRequest{const_cast<AbstractStorageDecl *>(this)},
+    false);
+}
+
 StorageImplInfo AbstractStorageDecl::getImplInfo() const {
   ASTContext &ctx = getASTContext();
   return evaluateOrDefault(ctx.evaluator,
     StorageImplInfoRequest{const_cast<AbstractStorageDecl *>(this)},
     StorageImplInfo::getSimpleStored(StorageIsMutable));
 }
 
+void AbstractStorageDecl::cacheImplInfo(StorageImplInfo implInfo) {
+  LazySemanticInfo.ImplInfoComputed = 1;
+  ImplInfo = implInfo;
+}
+
+void AbstractStorageDecl::setImplInfo(StorageImplInfo implInfo) {
+  cacheImplInfo(implInfo);
+
+  if (isImplicit()) {
+    auto &evaluator = getASTContext().evaluator;
+    HasStorageRequest request{this};
+    if (!evaluator.hasCachedResult(request))
+      evaluator.cacheOutput(request, implInfo.hasStorage());
+    else {
+      assert(
+        evaluateOrDefault(evaluator, request, false) == implInfo.hasStorage());
+    }
+  }
+}
+
 bool AbstractStorageDecl::hasPrivateAccessor() const {
   for (auto accessor : getAllAccessors()) {
     if (hasPrivateOrFilePrivateFormalAccess(accessor))
diff --git a/lib/AST/NameLookup.cpp b/lib/AST/NameLookup.cpp
@@ -1627,7 +1627,7 @@ void namelookup::forEachPotentialResolvedMacro(
 
 /// For each macro with the given role that might be attached to the given
 /// declaration, call the body.
-static void forEachPotentialAttachedMacro(
+void namelookup::forEachPotentialAttachedMacro(
     Decl *decl, MacroRole role,
     llvm::function_ref<void(MacroDecl *macro, const MacroRoleAttr *)> body
 ) {
diff --git a/lib/AST/TypeCheckRequests.cpp b/lib/AST/TypeCheckRequests.cpp
@@ -687,7 +687,7 @@ StorageImplInfoRequest::getCachedResult() const {
 
 void StorageImplInfoRequest::cacheResult(StorageImplInfo value) const {
   auto *storage = std::get<0>(getStorage());
-  storage->setImplInfo(value);
+  storage->cacheImplInfo(value);
 }
 
 //----------------------------------------------------------------------------//
diff --git a/lib/ClangImporter/ClangImporter.cpp b/lib/ClangImporter/ClangImporter.cpp
@@ -34,6 +34,7 @@
 #include "swift/AST/NameLookupRequests.h"
 #include "swift/AST/PrettyStackTrace.h"
 #include "swift/AST/SourceFile.h"
+#include "swift/AST/TypeCheckRequests.h"
 #include "swift/AST/Types.h"
 #include "swift/Basic/Defer.h"
 #include "swift/Basic/Platform.h"
@@ -5056,6 +5057,7 @@ cloneBaseMemberDecl(ValueDecl *decl, DeclContext *newContext) {
     out->setIsObjC(var->isObjC());
     out->setIsDynamic(var->isDynamic());
     out->copyFormalAccessFrom(var);
+    out->getASTContext().evaluator.cacheOutput(HasStorageRequest{out}, false);
     out->setAccessors(SourceLoc(),
                       makeBaseClassMemberAccessors(newContext, out, var),
                       SourceLoc());
diff --git a/lib/ClangImporter/ImportDecl.cpp b/lib/ClangImporter/ImportDecl.cpp
@@ -126,6 +126,7 @@ void ClangImporter::Implementation::makeComputed(AbstractStorageDecl *storage,
                                                  AccessorDecl *getter,
                                                  AccessorDecl *setter) {
   assert(getter);
+  storage->getASTContext().evaluator.cacheOutput(HasStorageRequest{storage}, false);
   if (setter) {
     storage->setImplInfo(StorageImplInfo::getMutableComputed());
     storage->setAccessors(SourceLoc(), {getter, setter}, SourceLoc());
diff --git a/lib/Sema/DerivedConformanceEquatableHashable.cpp b/lib/Sema/DerivedConformanceEquatableHashable.cpp
@@ -886,6 +886,10 @@ static ValueDecl *deriveHashable_hashValue(DerivedConformance &derived) {
                     SourceLoc(), C.Id_hashValue, parentDC);
   hashValueDecl->setInterfaceType(intType);
   hashValueDecl->setSynthesized();
+  hashValueDecl->setImplicit();
+  hashValueDecl->setImplInfo(StorageImplInfo::getImmutableComputed());
+  hashValueDecl->copyFormalAccessFrom(derived.Nominal,
+                                      /*sourceIsParentContext*/ true);
 
   ParameterList *params = ParameterList::createEmpty(C);
 
@@ -904,12 +908,7 @@ static ValueDecl *deriveHashable_hashValue(DerivedConformance &derived) {
                                    /*sourceIsParentContext*/ true);
 
   // Finish creating the property.
-  hashValueDecl->setImplicit();
-  hashValueDecl->setInterfaceType(intType);
-  hashValueDecl->setImplInfo(StorageImplInfo::getImmutableComputed());
   hashValueDecl->setAccessors(SourceLoc(), {getterDecl}, SourceLoc());
-  hashValueDecl->copyFormalAccessFrom(derived.Nominal,
-                                      /*sourceIsParentContext*/ true);
 
   // The derived hashValue of an actor must be nonisolated.
   if (!addNonIsolatedToSynthesized(derived.Nominal, hashValueDecl) &&
diff --git a/lib/Sema/TypeCheckMacros.cpp b/lib/Sema/TypeCheckMacros.cpp
@@ -1240,6 +1240,50 @@ static SourceFile *evaluateAttachedMacro(MacroDecl *macro, Decl *attachedTo,
   return macroSourceFile;
 }
 
+bool swift::accessorMacroOnlyIntroducesObservers(
+    MacroDecl *macro,
+    const MacroRoleAttr *attr
+) {
+  // Will this macro introduce observers?
+  bool foundObserver = false;
+  for (auto name : attr->getNames()) {
+    if (name.getKind() == MacroIntroducedDeclNameKind::Named &&
+        (name.getName().getBaseName().userFacingName() == "willSet" ||
+         name.getName().getBaseName().userFacingName() == "didSet")) {
+      foundObserver = true;
+    } else {
+      // Introduces something other than an observer.
+      return false;
+    }
+  }
+
+  if (foundObserver)
+    return true;
+
+  // WORKAROUND: Older versions of the Observation library make
+  // `ObservationIgnored` an accessor macro that implies that it makes a
+  // stored property computed. Override that, because we know it produces
+  // nothing.
+  if (macro->getName().getBaseName().userFacingName() == "ObservationIgnored") {
+    return true;
+  }
+
+  return false;
+}
+
+bool swift::accessorMacroIntroducesInitAccessor(
+    MacroDecl *macro, const MacroRoleAttr *attr
+) {
+  for (auto name : attr->getNames()) {
+    if (name.getKind() == MacroIntroducedDeclNameKind::Named &&
+        (name.getName().getBaseName().getKind() ==
+           DeclBaseName::Kind::Constructor))
+      return true;
+  }
+
+  return false;
+}
+
 Optional<unsigned> swift::expandAccessors(
     AbstractStorageDecl *storage, CustomAttr *attr, MacroDecl *macro
 ) {
@@ -1251,30 +1295,54 @@ Optional<unsigned> swift::expandAccessors(
     return None;
 
   PrettyStackTraceDecl debugStack(
-      "type checking expanded declaration macro", storage);
+      "type checking expanded accessor macro", storage);
 
   // Trigger parsing of the sequence of accessor declarations. This has the
   // side effect of registering those accessor declarations with the storage
   // declaration, so there is nothing further to do.
+  bool foundNonObservingAccessor = false;
+  bool foundInitAccessor = false;
   for (auto decl : macroSourceFile->getTopLevelItems()) {
     auto accessor = dyn_cast_or_null<AccessorDecl>(decl.dyn_cast<Decl *>());
     if (!accessor)
       continue;
 
-    if (accessor->isObservingAccessor())
-      continue;
+    if (accessor->isInitAccessor())
+      foundInitAccessor = true;
 
+    if (!accessor->isObservingAccessor())
+      foundNonObservingAccessor = true;
+  }
+
+  auto roleAttr = macro->getMacroRoleAttr(MacroRole::Accessor);
+  bool expectedNonObservingAccessor =
+    !accessorMacroOnlyIntroducesObservers(macro, roleAttr);
+  if (foundNonObservingAccessor) {
     // If any non-observing accessor was added, mark the initializer as
     // subsumed.
     if (auto var = dyn_cast<VarDecl>(storage)) {
       if (auto binding = var->getParentPatternBinding()) {
         unsigned index = binding->getPatternEntryIndexForVarDecl(var);
         binding->setInitializerSubsumed(index);
-        break;
       }
     }
   }
 
+  // Make sure we got non-observing accessors exactly where we expected to.
+  if (foundNonObservingAccessor != expectedNonObservingAccessor) {
+    storage->diagnose(
+        diag::macro_accessor_missing_from_expansion, macro->getName(),
+        !expectedNonObservingAccessor);
+  }
+
+  // 'init' accessors must be documented in the macro role attribute.
+  if (foundInitAccessor &&
+      !accessorMacroIntroducesInitAccessor(macro, roleAttr)) {
+    storage->diagnose(
+        diag::macro_init_accessor_not_documented, macro->getName());
+    // FIXME: Add the appropriate "names: named(init)".
+  }
+
   return macroSourceFile->getBufferID();
 }
 
diff --git a/lib/Sema/TypeCheckMacros.h b/lib/Sema/TypeCheckMacros.h
@@ -77,6 +77,16 @@ Optional<unsigned> expandPeers(CustomAttr *attr, MacroDecl *macro, Decl *decl);
 Optional<unsigned> expandConformances(CustomAttr *attr, MacroDecl *macro,
                                       NominalTypeDecl *nominal);
 
+/// Determine whether an accessor macro with the given attribute only
+/// introduces observers like willSet and didSet.
+bool accessorMacroOnlyIntroducesObservers(
+    MacroDecl *macro, const MacroRoleAttr *attr);
+
+/// Determine whether an accessor macro (defined with the given role attribute)
+/// introduces an init accessor.
+bool accessorMacroIntroducesInitAccessor(
+    MacroDecl *macro, const MacroRoleAttr *attr);
+
 } // end namespace swift
 
 #endif /* SWIFT_SEMA_TYPECHECKMACROS_H */
diff --git a/lib/Sema/TypeCheckStorage.cpp b/lib/Sema/TypeCheckStorage.cpp
diff --git a/lib/Serialization/Deserialization.cpp b/lib/Serialization/Deserialization.cpp
diff --git a/stdlib/public/Observation/Sources/Observation/Observable.swift b/stdlib/public/Observation/Sources/Observation/Observable.swift
diff --git a/test/Macros/accessor_has_storage_cycle.swift b/test/Macros/accessor_has_storage_cycle.swift
diff --git a/test/ModuleInterface/Observable.swift b/test/ModuleInterface/Observable.swift
diff --git a/test/Serialization/macros.swift b/test/Serialization/macros.swift
diff --git a/test/stdlib/Observation/Observable.swift b/test/stdlib/Observation/Observable.swift

Original file line number	Diff line number	Diff line change
`@@ -687,7 +687,7 @@ StorageImplInfoRequest::getCachedResult() const {`
`687`	`687`
`688`	`688`	`void StorageImplInfoRequest::cacheResult(StorageImplInfo value) const {`
`689`	`689`	`auto *storage = std::get<0>(getStorage());`
`690`		`- storage->setImplInfo(value);`
	`690`	`+ storage->cacheImplInfo(value);`
`691`	`691`	`}`
`692`	`692`
`693`	`693`	`//----------------------------------------------------------------------------//`