[Typechecker] Implement SE-0268 Refine didSet Semantics

Author: theblixguy

include/swift/AST/Decl.h

@@ -428,16 +428,15 @@ class alignas(1 << DeclAlignInBits) Decl {
     HasTopLevelLocalContextCaptures : 1
   );
 
-  SWIFT_INLINE_BITFIELD(AccessorDecl, FuncDecl, 4+1+1,
-    /// The kind of accessor this is.
-    AccessorKind : 4,
+  SWIFT_INLINE_BITFIELD(AccessorDecl, FuncDecl, 4 + 1 + 1,
+                        /// The kind of accessor this is.
+                        AccessorKind : 4,
 
-    /// Whether the accessor is transparent.
-    IsTransparent : 1,
+                        /// Whether the accessor is transparent.
+                        IsTransparent : 1,
 
-    /// Whether we have computed the above.
-    IsTransparentComputed : 1
-  );
+                        /// Whether we have computed the above.
+                        IsTransparentComputed : 1);
 
   SWIFT_INLINE_BITFIELD(ConstructorDecl, AbstractFunctionDecl, 3+1+1,
     /// The body initialization kind (+1), or zero if not yet computed.
@@ -6502,6 +6501,11 @@ class AccessorDecl final : public FuncDecl {
     return isGetter() && getAccessorKeywordLoc().isInvalid();
   }
 
+  /// Is this accessor a "simple" didSet? A "simple" didSet does not
+  /// use the implicit oldValue parameter in its body or does not have
+  /// an explicit parameter in its parameter list.
+  bool isSimpleDidSet() const;
+
   void setIsTransparent(bool transparent) {
     Bits.AccessorDecl.IsTransparent = transparent;
     Bits.AccessorDecl.IsTransparentComputed = 1;

include/swift/AST/KnownIdentifiers.def

@@ -98,6 +98,7 @@ IDENTIFIER(objectAtIndexedSubscript)
 IDENTIFIER(objectForKeyedSubscript)
 IDENTIFIER(ObjectiveC)
 IDENTIFIER_(ObjectiveCType)
+IDENTIFIER(oldValue)
 IDENTIFIER(Optional)
 IDENTIFIER_(OptionalNilComparisonType)
 IDENTIFIER(parameter)

include/swift/AST/StorageImpl.h

@@ -222,6 +222,12 @@ enum class ReadWriteImplKind {
 
   /// There's a modify coroutine.
   Modify,
+
+  /// We have a didSet which doesn't use the oldValue
+  StoredWithSimpleDidSet,
+
+  /// We have a didSet which doesn't use the oldValue
+  InheritedWithSimpleDidSet,
 };
 enum { NumReadWriteImplKindBits = 4 };
 
@@ -266,12 +272,14 @@ class StorageImplInfo {
 
     case WriteImplKind::StoredWithObservers:
       assert(readImpl == ReadImplKind::Stored);
-      assert(readWriteImpl == ReadWriteImplKind::MaterializeToTemporary);
+      assert(readWriteImpl == ReadWriteImplKind::MaterializeToTemporary ||
+             readWriteImpl == ReadWriteImplKind::StoredWithSimpleDidSet);
       return;
 
     case WriteImplKind::InheritedWithObservers:
       assert(readImpl == ReadImplKind::Inherited);
-      assert(readWriteImpl == ReadWriteImplKind::MaterializeToTemporary);
+      assert(readWriteImpl == ReadWriteImplKind::MaterializeToTemporary ||
+             readWriteImpl == ReadWriteImplKind::InheritedWithSimpleDidSet);
       return;
 
     case WriteImplKind::Set:

include/swift/AST/TypeCheckRequests.h

@@ -2281,6 +2281,27 @@ class ResolveTypeEraserTypeRequest
   void cacheResult(Type value) const;
 };
 
+/// Determines whether this is a "simple" didSet i.e one that either does not
+/// use the implicit oldValue parameter in the body or does not take an explicit
+/// parameter (ex: 'didSet(oldValue)').
+class SimpleDidSetRequest
+    : public SimpleRequest<SimpleDidSetRequest, bool(AccessorDecl *),
+                           RequestFlags::Cached> {
+public:
+  using SimpleRequest::SimpleRequest;
+
+private:
+  friend SimpleRequest;
+
+  // Evaluation.
+  bool evaluate(Evaluator &evaluator, AccessorDecl *decl) const;
+
+public:
+  bool isCached() const {
+    return std::get<0>(getStorage())->getAccessorKind() == AccessorKind::DidSet;
+  }
+};
+
 // Allow AnyValue to compare two Type values, even though Type doesn't
 // support ==.
 template<>

include/swift/AST/TypeCheckerTypeIDZone.def

@@ -246,3 +246,5 @@ SWIFT_REQUEST(TypeChecker, ClosureHasExplicitResultRequest,
 SWIFT_REQUEST(TypeChecker, LookupAllConformancesInContextRequest,
               ProtocolConformanceLookupResult(const DeclContext *),
               Uncached, NoLocationInfo)
+SWIFT_REQUEST(TypeChecker, SimpleDidSetRequest, 
+              bool(AccessorDecl *), Cached, NoLocationInfo)

lib/AST/ASTDumper.cpp

@@ -264,6 +264,10 @@ StringRef swift::getReadWriteImplKindName(ReadWriteImplKind kind) {
     return "materialize_to_temporary";
   case ReadWriteImplKind::Modify:
     return "modify_coroutine";
+  case ReadWriteImplKind::StoredWithSimpleDidSet:
+    return "stored_simple_didset";
+  case ReadWriteImplKind::InheritedWithSimpleDidSet:
+    return "inherited_simple_didset";
   }
   llvm_unreachable("bad kind");
 }

lib/AST/Decl.cpp

@@ -2093,6 +2093,16 @@ getDirectReadWriteAccessStrategy(const AbstractStorageDecl *storage) {
   case ReadWriteImplKind::Modify:
     return AccessStrategy::getAccessor(AccessorKind::Modify,
                                        /*dispatch*/ false);
+  case ReadWriteImplKind::StoredWithSimpleDidSet:
+  case ReadWriteImplKind::InheritedWithSimpleDidSet:
+    if (storage->requiresOpaqueModifyCoroutine()) {
+      return AccessStrategy::getAccessor(AccessorKind::Modify,
+                                         /*dispatch*/ false);
+    } else {
+      return AccessStrategy::getMaterializeToTemporary(
+          getDirectReadAccessStrategy(storage),
+          getDirectWriteAccessStrategy(storage));
+    }
   case ReadWriteImplKind::MaterializeToTemporary:
     return AccessStrategy::getMaterializeToTemporary(
                                        getDirectReadAccessStrategy(storage),
@@ -7330,6 +7340,12 @@ bool AccessorDecl::isExplicitNonMutating() const {
     !getDeclContext()->getDeclaredInterfaceType()->hasReferenceSemantics();
 }
 
+bool AccessorDecl::isSimpleDidSet() const {
+  auto mutableThis = const_cast<AccessorDecl *>(this);
+  return evaluateOrDefault(getASTContext().evaluator,
+                           SimpleDidSetRequest{mutableThis}, false);
+}
+
 StaticSpellingKind FuncDecl::getCorrectStaticSpelling() const {
   assert(getDeclContext()->isTypeContext());
   if (!isStatic())

lib/AST/TypeCheckRequests.cpp

@@ -908,7 +908,7 @@ void EnumRawValuesRequest::diagnoseCycle(DiagnosticEngine &diags) const {
 }
 
 void EnumRawValuesRequest::noteCycleStep(DiagnosticEngine &diags) const {
-
+  
 }
 
 //----------------------------------------------------------------------------//

lib/SIL/IR/TypeLowering.cpp

@@ -2483,6 +2483,8 @@ TypeConverter::getLoweredLocalCaptures(SILDeclRef fn) {
             collectAccessorCaptures(AccessorKind::MutableAddress);
             break;
           case ReadWriteImplKind::Modify:
+          case ReadWriteImplKind::StoredWithSimpleDidSet:
+          case ReadWriteImplKind::InheritedWithSimpleDidSet:
             collectAccessorCaptures(AccessorKind::Modify);
             break;
           }

lib/Sema/TypeCheckDecl.cpp

@@ -1118,6 +1118,69 @@ EnumRawValuesRequest::evaluate(Evaluator &eval, EnumDecl *ED,
   return std::make_tuple<>();
 }
 
+bool SimpleDidSetRequest::evaluate(Evaluator &evaluator,
+                                   AccessorDecl *decl) const {
+
+  class OldValueFinder : public ASTWalker {
+    const ParamDecl *OldValueParam;
+    bool foundOldValueRef = false;
+
+  public:
+    OldValueFinder(const ParamDecl *param) : OldValueParam(param) {}
+
+    virtual std::pair<bool, Expr *> walkToExprPre(Expr *E) override {
+      if (!E)
+        return {true, E};
+      if (auto DRE = dyn_cast<DeclRefExpr>(E)) {
+        if (auto decl = DRE->getDecl()) {
+          if (decl == OldValueParam) {
+            foundOldValueRef = true;
+            return {false, nullptr};
+          }
+        }
+      }
+
+      return {true, E};
+    }
+
+    bool didFindOldValueRef() { return foundOldValueRef; }
+  };
+
+  // If this is not a didSet accessor, bail out.
+  if (decl->getAccessorKind() != AccessorKind::DidSet) {
+    return false;
+  }
+
+  // didSet must have a single parameter.
+  if (decl->getParameters()->size() != 1) {
+    return false;
+  }
+
+  auto param = decl->getParameters()->get(0);
+  // If this parameter is not implicit, then it means it has been explicitly
+  // provided by the user (i.e. 'didSet(oldValue)'). This means we cannot
+  // consider this a "simple" didSet because we have to fetch the oldValue
+  // regardless of whether it's referenced in the body or not.
+  if (!param->isImplicit()) {
+    return false;
+  }
+
+  // If we find a reference to the implicit 'oldValue' parameter, then it is
+  // not a "simple" didSet because we need to fetch it.
+  auto walker = OldValueFinder(param);
+  decl->getBody()->walk(walker);
+  auto hasOldValueRef = walker.didFindOldValueRef();
+  if (!hasOldValueRef) {
+    // If the body does not refer to implicit 'oldValue', it means we can
+    // consider this as a "simple" didSet. Let's also erase the implicit
+    // oldValue as it is never used.
+    auto &ctx = decl->getASTContext();
+    decl->setParameters(ParameterList::createEmpty(ctx));
+    return true;
+  }
+  return false;
+}
+
 const ConstructorDecl *
 swift::findNonImplicitRequiredInit(const ConstructorDecl *CD) {
   while (CD->isImplicit()) {
@@ -1577,6 +1640,12 @@ static ParamDecl *getOriginalParamFromAccessor(AbstractStorageDecl *storage,
   case AccessorKind::DidSet:
   case AccessorKind::WillSet:
   case AccessorKind::Set:
+    if (accessor->isSimpleDidSet()) {
+      // If this is a "simple" didSet, there won't be
+      // a parameter.
+      return nullptr;
+    }
+
     if (param == accessorParams->get(0)) {
       // This is the 'newValue' parameter.
       return nullptr;
@@ -2190,6 +2259,13 @@ InterfaceTypeRequest::evaluate(Evaluator &eval, ValueDecl *D) const {
   case DeclKind::Accessor:
   case DeclKind::Constructor:
   case DeclKind::Destructor: {
+    // If this is a didSet, then we need to check whether the body references
+    // the implicit 'oldValue' parameter or not, in order to correctly
+    // compute the interface type.
+    if (auto AD = dyn_cast<AccessorDecl>(D)) {
+      (void)AD->isSimpleDidSet();
+    }
+
     auto *AFD = cast<AbstractFunctionDecl>(D);
 
     auto sig = AFD->getGenericSignature();