implement typechecking for effects specifiers on 'get' accessors

The basic rule is that the protocol requirement
describes the maximal set of effects that the
property is allowed to have. Thus, witnesses
must have the same or fewer effects specifiers
on the getter.

For class inheritance overrides, you can remove
effects freely, as long as you stay within
the bounds of the normal override restrictions.
But, you cannot override with more effects than
the base member has. Same goes for protocol
member overrides.

Furthermore, we disallow key paths to effectful
properties/subscripts, which also cannot be @objc.

Author: kavon

include/swift/AST/Decl.h

@@ -4521,6 +4521,34 @@ class AbstractStorageDecl : public ValueDecl {
     return {};
   }
 
+  /// This is the primary mechanism by which we can easily determine whether
+  /// this storage decl has any effects.
+  ///
+  /// \returns the getter decl iff this decl has only one accessor that is
+  ///          a 'get' with an effect (i.e., 'async', 'throws', or both).
+  ///          Otherwise returns nullptr.
+  AccessorDecl *getEffectfulGetAccessor() const;
+
+  /// Performs a "limit check" on an effect possibly exhibited by this storage
+  /// decl with respect to some other storage decl that serves as the "limit."
+  /// This check says that \c this is less effectful than \c other if
+  /// \c this either does not exhibit the effect, or if it does, then \c other
+  /// also exhibits the effect. Thus, it is conceptually equivalent to
+  /// a less-than-or-equal (≤) check like so:
+  ///
+  /// \verbatim
+  ///
+  ///           this->hasEffect(E) ≤ other->hasEffect(E)
+  ///
+  /// \endverbatim
+  ///
+  /// \param kind the single effect we are performing a check for.
+  ///
+  /// \returns true iff \c this decl either does not exhibit the effect,
+  ///          or \c other also exhibits the effect.
+  bool isLessEffectfulThan(AbstractStorageDecl const* other,
+                           EffectKind kind) const;
+
   /// Return an accessor that this storage is expected to have, synthesizing
   /// one if necessary. Note that will always synthesize one, even if the
   /// accessor is not part of the expected opaque set for the storage, so use

include/swift/AST/DiagnosticsSema.def

@@ -1584,6 +1584,10 @@ WARNING(enum_frozen_nonpublic,none,
 ERROR(getset_init,none,
       "variable with getter/setter cannot have an initial value", ())
 
+ERROR(effectful_not_representable_objc,none,
+      "%0 with 'throws' or 'async' is not representable in Objective-C",
+      (DescriptiveDeclKind))
+
 ERROR(unimplemented_static_var,none,
       "%select{ERROR|static|class}1 stored properties not supported"
       "%select{ in this context| in generic types| in classes| in protocol extensions}0"
@@ -2600,6 +2604,10 @@ ERROR(override_class_declaration_in_extension,none,
 ERROR(override_throws,none,
       "cannot override non-throwing %select{method|initializer}0 with "
       "throwing %select{method|initializer}0", (bool))
+// TODO: this really could be merged with the above.
+ERROR(override_with_more_effects,none,
+      "cannot override non-'%1' %0 with '%1' %0",
+      (DescriptiveDeclKind, StringRef))
 ERROR(override_throws_objc,none,
       "overriding a throwing @objc %select{method|initializer}0 with "
       "a non-throwing %select{method|initializer}0 is not supported", (bool))
@@ -4150,21 +4158,21 @@ ERROR(isa_collection_downcast_pattern_value_unimplemented,none,
 ERROR(try_unhandled,none,
       "errors thrown from here are not handled", ())
 ERROR(throwing_call_unhandled,none,
-      "call can throw, but the error is not handled", ())
+      "%0 can throw, but the error is not handled", (StringRef))
 ERROR(tryless_throwing_call_unhandled,none,
-      "call can throw, but it is not marked with 'try' and "
-      "the error is not handled", ())
+      "%0 can throw, but it is not marked with 'try' and "
+      "the error is not handled", (StringRef))
 ERROR(throw_in_nonthrowing_function,none,
       "error is not handled because the enclosing function "
       "is not declared 'throws'", ())
 
 ERROR(throwing_call_in_rethrows_function,none,
-      "call can throw, but the error is not handled; a function declared "
-      "'rethrows' may only throw if its parameter does", ())
+      "%0 can throw, but the error is not handled; a function declared "
+      "'rethrows' may only throw if its parameter does", (StringRef))
 ERROR(tryless_throwing_call_in_rethrows_function,none,
-      "call can throw, but it is not marked with 'try' and "
+      "%0 can throw, but it is not marked with 'try' and "
       "the error is not handled; a function declared "
-      "'rethrows' may only throw if its parameter does", ())
+      "'rethrows' may only throw if its parameter does", (StringRef))
 ERROR(throw_in_rethrows_function,none,
       "a function declared 'rethrows' may only throw if its parameter does", ())
 NOTE(because_rethrows_argument_throws,none,
@@ -4176,11 +4184,11 @@ NOTE(because_rethrows_conformance_throws,none,
      "call is to 'rethrows' function, but a conformance has a throwing witness", ())
 
 ERROR(throwing_call_in_nonthrowing_autoclosure,none,
-      "call can throw, but it is executed in a non-throwing "
-      "autoclosure",())
+      "%0 can throw, but it is executed in a non-throwing "
+      "autoclosure",(StringRef))
 ERROR(tryless_throwing_call_in_nonthrowing_autoclosure,none,
-      "call can throw, but it is not marked with 'try' and "
-      "it is executed in a non-throwing autoclosure",())
+      "%0 can throw, but it is not marked with 'try' and "
+      "it is executed in a non-throwing autoclosure",(StringRef))
 ERROR(throw_in_nonthrowing_autoclosure,none,
       "error is not handled because it is thrown in a non-throwing "
       "autoclosure", ())
@@ -4189,17 +4197,17 @@ ERROR(try_unhandled_in_nonexhaustive_catch,none,
       "errors thrown from here are not handled because the "
       "enclosing catch is not exhaustive", ())
 ERROR(throwing_call_in_nonexhaustive_catch,none,
-      "call can throw, but the enclosing catch is not exhaustive", ())
+      "%0 can throw, but the enclosing catch is not exhaustive", (StringRef))
 ERROR(tryless_throwing_call_in_nonexhaustive_catch,none,
-      "call can throw, but it is not marked with 'try' and "
-      "the enclosing catch is not exhaustive", ())
+      "%0 can throw, but it is not marked with 'try' and "
+      "the enclosing catch is not exhaustive", (StringRef))
 ERROR(throw_in_nonexhaustive_catch,none,
       "error is not handled because the enclosing catch is not exhaustive", ())
 
-ERROR(throwing_call_in_illegal_context,none,
-      "call can throw, but errors cannot be thrown out of "
+ERROR(throwing_op_in_illegal_context,none,
+      "%1 can throw, but errors cannot be thrown out of "
       "%select{<<ERROR>>|a default argument|a property wrapper initializer|a property initializer|a global variable initializer|an enum case raw value|a catch pattern|a catch guard expression|a defer body}0",
-      (unsigned))
+      (unsigned, StringRef))
 ERROR(throw_in_illegal_context,none,
       "errors cannot be thrown out of "
       "%select{<<ERROR>>|a default argument|a property wrapper initializer|a property initializer|a global variable initializer|an enum case raw value|a catch pattern|a catch guard expression|a defer body}0",
@@ -4213,6 +4221,10 @@ ERROR(throwing_call_without_try,none,
       "call can throw but is not marked with 'try'", ())
 ERROR(throwing_async_let_without_try,none,
       "reading 'async let' can throw but is not marked with 'try'", ())
+ERROR(throwing_prop_access_without_try,none,
+      "property access can throw but is not marked with 'try'", ())
+ERROR(throwing_subscript_access_without_try,none,
+      "subscript access can throw but is not marked with 'try'", ())
 NOTE(note_forgot_try,none,
       "did you mean to use 'try'?", ())
 NOTE(note_error_to_optional,none,
@@ -4382,6 +4394,9 @@ ERROR(actor_isolated_from_escaping_closure,none,
 ERROR(actor_isolated_keypath_component,none,
       "cannot form key path to actor-isolated %0 %1",
       (DescriptiveDeclKind, DeclName))
+ERROR(effectful_keypath_component,none,
+      "cannot form key path to %0 with 'throws' or 'async'",
+      (DescriptiveDeclKind))
 ERROR(local_function_executed_concurrently,none,
       "concurrently-executed %0 %1 must be marked as '@Sendable'",
       (DescriptiveDeclKind, DeclName))
@@ -5615,6 +5630,9 @@ ERROR(property_wrapper_let, none,
 ERROR(property_wrapper_computed, none,
       "property wrapper cannot be applied to a computed property",
       ())
+ERROR(property_wrapper_effectful,none,
+    "property wrappers currently cannot define an 'async' or 'throws' accessor",
+    ())
 
 ERROR(property_with_wrapper_conflict_attribute,none,
       "property %0 with a wrapper cannot also be "

lib/AST/Decl.cpp

@@ -2122,6 +2122,30 @@ AccessorDecl *AbstractStorageDecl::getSynthesizedAccessor(AccessorKind kind) con
     nullptr);
 }
 
+AccessorDecl *AbstractStorageDecl::getEffectfulGetAccessor() const {
+    if (getAllAccessors().size() != 1)
+      return nullptr;
+
+    if (auto accessor = getAccessor(AccessorKind::Get))
+      if (accessor->hasAsync() || accessor->hasThrows())
+        return accessor;
+
+  return nullptr;
+}
+
+bool AbstractStorageDecl::isLessEffectfulThan(AbstractStorageDecl const* other,
+                                              EffectKind kind) const {
+  bool allowedByOther = false;
+  if (auto otherGetter = other->getEffectfulGetAccessor())
+    allowedByOther = otherGetter->hasEffect(kind);
+
+  if (auto getter = getEffectfulGetAccessor())
+    if (getter->hasEffect(kind) && !allowedByOther)
+      return false; // has the effect when other does not; it's more effectful!
+
+  return true; // OK
+}
+
 AccessorDecl *AbstractStorageDecl::getOpaqueAccessor(AccessorKind kind) const {
   auto *accessor = getAccessor(kind);
   if (accessor && !accessor->isImplicit())

lib/Sema/MiscDiagnostics.cpp

@@ -68,6 +68,7 @@ static Expr *isImplicitPromotionToOptional(Expr *E) {
 ///   - Error about collection literals that default to Any collections in
 ///     invalid positions.
 ///   - Marker protocols cannot occur as the type of an as? or is expression.
+///   - KeyPath expressions cannot refer to effectful properties / subscripts
 ///
 static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
                                          bool isExprStmt) {
@@ -150,6 +151,9 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
         CallArgs.insert(DSE->getIndex());
 
       if (auto *KPE = dyn_cast<KeyPathExpr>(E)) {
+        // raise an error if this KeyPath contains an effectful member.
+        checkForEffectfulKeyPath(KPE);
+
         for (auto Comp : KPE->getComponents()) {
           if (auto *Arg = Comp.getIndexExpr())
             CallArgs.insert(Arg);
@@ -322,6 +326,25 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
       return { true, E };
     }
 
+    /// Visit each component of the keypath and emit a diganostic if they
+    /// refer to a member that has effects.
+    void checkForEffectfulKeyPath(KeyPathExpr *keyPath) {
+      for (const auto &component : keyPath->getComponents()) {
+        if (component.hasDeclRef()) {
+          auto decl = component.getDeclRef().getDecl();
+          if (auto asd = dyn_cast<AbstractStorageDecl>(decl)) {
+            if (auto getter = asd->getEffectfulGetAccessor()) {
+              Ctx.Diags.diagnose(component.getLoc(),
+                                 diag::effectful_keypath_component,
+                                 asd->getDescriptiveKind());
+              Ctx.Diags.diagnose(asd->getLoc(), diag::kind_declared_here,
+                                 asd->getDescriptiveKind());
+            }
+          }
+        }
+      }
+    }
+
     void checkCheckedCastExpr(CheckedCastExpr *cast) {
       if (!isa<ConditionalCheckedCastExpr>(cast) && !isa<IsExpr>(cast))
         return;

lib/Sema/TypeCheckConcurrency.cpp

@@ -650,6 +650,14 @@ ActorIsolationRestriction ActorIsolationRestriction::forDeclaration(
         isAccessibleAcrossActors = true;
     }
 
+    // Similarly, a computed property or subscript that has an 'async' getter
+    // provides an asynchronous context, and has no restrictions.
+    if (auto storageDecl = dyn_cast<AbstractStorageDecl>(decl)) {
+      if (auto effectfulGetter = storageDecl->getEffectfulGetAccessor())
+        if (effectfulGetter->hasAsync())
+          isAccessibleAcrossActors = true;
+    }
+
     // Determine the actor isolation of the given declaration.
     switch (auto isolation = getActorIsolation(cast<ValueDecl>(decl))) {
     case ActorIsolation::ActorInstance:

lib/Sema/TypeCheckConstraints.cpp

@@ -603,7 +603,7 @@ bool TypeChecker::typeCheckForEachBinding(DeclContext *dc, ForEachStmt *stmt) {
     if (conformanceRef.hasEffect(EffectKind::Throws) &&
         stmt->getTryLoc().isInvalid()) {
       auto &diags = dc->getASTContext().Diags;
-      diags.diagnose(stmt->getAwaitLoc(), diag::throwing_call_unhandled)
+      diags.diagnose(stmt->getAwaitLoc(), diag::throwing_call_unhandled, "call")
         .fixItInsert(stmt->getAwaitLoc(), "try");
 
       return failed();

lib/Sema/TypeCheckDeclObjC.cpp

@@ -997,6 +997,14 @@ bool swift::isRepresentableInObjC(const VarDecl *VD, ObjCReason Reason) {
   if (checkObjCActorIsolation(VD, Reason))
     return false;
 
+  // effectful computed properties cannot be represented, and its an error
+  // to mark them as @objc
+  if (VD->getEffectfulGetAccessor()) {
+    VD->diagnose(diag::effectful_not_representable_objc,
+                 VD->getDescriptiveKind());
+    return false;
+  }
+
   if (!Result) {
     SourceRange TypeRange = VD->getTypeSourceRangeForDiagnostics();
     // TypeRange can be invalid; e.g. '@objc let foo = SwiftType()'
@@ -1028,6 +1036,14 @@ bool swift::isRepresentableInObjC(const SubscriptDecl *SD, ObjCReason Reason) {
   if (checkObjCActorIsolation(SD, Reason))
     return false;
 
+  // effectful subscripts cannot be represented, and its an error
+  // to mark them as @objc
+  if (SD->getEffectfulGetAccessor()) {
+    SD->diagnose(diag::effectful_not_representable_objc,
+                 SD->getDescriptiveKind());
+    return false;
+  }
+
   // ObjC doesn't support class subscripts.
   if (!SD->isInstanceMember()) {
     SD->diagnose(diag::objc_invalid_on_static_subscript,

lib/Sema/TypeCheckDeclOverride.cpp

@@ -1818,6 +1818,18 @@ static bool checkSingleOverride(ValueDecl *override, ValueDecl *base) {
         return true;
       }
     }
+
+    // Make sure an effectful storage decl is only overridden by a storage
+    // decl with the same or fewer effect kinds.
+    if (!overrideASD->isLessEffectfulThan(baseASD, EffectKind::Async)) {
+      diags.diagnose(overrideASD, diag::override_with_more_effects,
+                     overrideASD->getDescriptiveKind(), "async");
+      return true;
+    } else if (!overrideASD->isLessEffectfulThan(baseASD, EffectKind::Throws)) {
+      diags.diagnose(overrideASD, diag::override_with_more_effects,
+                     overrideASD->getDescriptiveKind(), "throws");
+      return true;
+    }
   }
 
   // Various properties are only checked for the storage declarations