Merge pull request #74793 from tshortli/subscript-setter-availability

Sema: Fix accessor availability checking in argument lists

Author: tshortli

lib/Sema/TypeCheckAvailability.cpp

@@ -3311,25 +3311,32 @@ bool swift::diagnoseExplicitUnavailability(
 
 namespace {
 class ExprAvailabilityWalker : public ASTWalker {
-  /// Describes how the next member reference will be treated as we traverse
-  /// the AST.
+  /// Models how member references will translate to accessor usage. This is
+  /// used to diagnose the availability of individual accessors that may be
+  /// called by the expression being checked.
   enum class MemberAccessContext : unsigned {
-    /// The member reference is in a context where an access will call
-    /// the getter.
-    Getter,
-
-    /// The member reference is in a context where an access will call
-    /// the setter.
-    Setter,
-
-    /// The member reference is in a context where it will be turned into
-    /// an inout argument. (Once this happens, we have to conservatively assume
-    /// that both the getter and setter could be called.)
-    InOut
+    /// The starting access context for the root of any expression tree. In this
+    /// context, a member access will call the get accessor only.
+    Default,
+
+    /// The access context for expressions rooted in a LoadExpr. A LoadExpr
+    /// coerces l-values to r-values and thus member access inside of a LoadExpr
+    /// will only invoke get accessors.
+    Load,
+
+    /// The access context for the outermost member accessed in the expression
+    /// tree on the left-hand side of an assignment. Only the set accessor will
+    /// be invoked on this member.
+    Assignment,
+
+    /// The access context for expressions in which member is being read and
+    /// then written back to. For example, a writeback will occur inside of an
+    /// InOutExpr. Both the get and set accessors may be called in this context.
+    Writeback
   };
 
   ASTContext &Context;
-  MemberAccessContext AccessContext = MemberAccessContext::Getter;
+  MemberAccessContext AccessContext = MemberAccessContext::Default;
   SmallVector<const Expr *, 16> ExprStack;
   const ExportContext &Where;
 
@@ -3346,6 +3353,13 @@ class ExprAvailabilityWalker : public ASTWalker {
     return MacroWalking::Arguments;
   }
 
+  PreWalkAction walkToArgumentPre(const Argument &Arg) override {
+    // Arguments should be walked in their own member access context which
+    // starts out read-only by default.
+    walkInContext(Arg.getExpr(), MemberAccessContext::Default);
+    return Action::SkipChildren();
+  }
+
   PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
     auto *DC = Where.getDeclContext();
 
@@ -3468,6 +3482,11 @@ class ExprAvailabilityWalker : public ASTWalker {
           ME->getMacroRef(), ME->getMacroNameLoc().getSourceRange());
     }
 
+    if (auto LE = dyn_cast<LoadExpr>(E)) {
+      walkLoadExpr(LE);
+      return Action::SkipChildren(E);
+    }
+
     return Action::Continue(E);
   }
 
@@ -3524,26 +3543,6 @@ class ExprAvailabilityWalker : public ASTWalker {
     return call;
   }
 
-  /// Walks up from a potential member reference to the first LoadExpr that would
-  /// make the member reference an r-value instead of an l-value.
-  const LoadExpr *getEnclosingLoadExpr() const {
-    assert(!ExprStack.empty() && "must be called while visiting an expression");
-    ArrayRef<const Expr *> stack = ExprStack;
-    stack = stack.drop_back();
-
-    for (auto expr : llvm::reverse(stack)) {
-      // Do not search past the first enclosing ApplyExpr. Any enclosing
-      // LoadExpr from this point only applies to the result of the call.
-      if (auto applyExpr = dyn_cast<ApplyExpr>(expr))
-        return nullptr;
-
-      if (auto loadExpr = dyn_cast<LoadExpr>(expr))
-        return loadExpr;
-    }
-
-    return nullptr;
-  }
-
   /// Walk an assignment expression, checking for availability.
   void walkAssignExpr(AssignExpr *E) {
     // We take over recursive walking of assignment expressions in order to
@@ -3559,32 +3558,38 @@ class ExprAvailabilityWalker : public ASTWalker {
     // encountered walking (pre-order) is the Dest is the destination of the
     // write. For the moment this is fine -- but future syntax might violate
     // this assumption.
-    walkInContext(E, Dest, MemberAccessContext::Setter);
+    walkInContext(Dest, MemberAccessContext::Assignment);
 
     // Check RHS in getter context
     Expr *Source = E->getSrc();
     if (!Source) {
       return;
     }
-    walkInContext(E, Source, MemberAccessContext::Getter);
+    walkInContext(Source, MemberAccessContext::Default);
   }
-  
+
+  /// Walk a load expression, checking for availability.
+  void walkLoadExpr(LoadExpr *E) {
+    walkInContext(E->getSubExpr(), MemberAccessContext::Load);
+  }
+
   /// Walk a member reference expression, checking for availability.
   void walkMemberRef(MemberRefExpr *E) {
-    // Walk the base. If the access context is currently `Setter`, then we must
-    // be diagnosing the destination of an assignment. When recursing, diagnose
-    // any remaining member refs as if they were in an InOutExpr, since there is
-    // a writeback occurring through them as a result of the assignment.
+    // Walk the base. If the access context is currently `Assignment`, then we
+    // must be diagnosing the destination of an assignment. When recursing,
+    // diagnose any remaining member refs in a `Writeback` context, since
+    // there is a writeback occurring through them as a result of the
+    // assignment.
     //
     //   someVar.x.y = 1
-    //           │ ╰─ MemberAccessContext::Setter
-    //           ╰─── MemberAccessContext::InOut
+    //           │ ╰─ MemberAccessContext::Assignment
+    //           ╰─── MemberAccessContext::Writeback
     //
     MemberAccessContext accessContext =
-        (AccessContext == MemberAccessContext::Setter)
-            ? MemberAccessContext::InOut
+        (AccessContext == MemberAccessContext::Assignment)
+            ? MemberAccessContext::Writeback
             : AccessContext;
-    walkInContext(E, E->getBase(), accessContext);
+    walkInContext(E->getBase(), accessContext);
 
     ConcreteDeclRef DR = E->getMember();
     // Diagnose for the member declaration itself.
@@ -3637,7 +3642,13 @@ class ExprAvailabilityWalker : public ASTWalker {
 
   /// Walk an inout expression, checking for availability.
   void walkInOutExpr(InOutExpr *E) {
-    walkInContext(E, E->getSubExpr(), MemberAccessContext::InOut);
+    // Typically an InOutExpr should begin a `Writeback` context. However,
+    // inside a LoadExpr this transition is suppressed since the entire
+    // expression is being coerced to an r-value.
+    auto accessContext = AccessContext != MemberAccessContext::Load
+                             ? MemberAccessContext::Writeback
+                             : AccessContext;
+    walkInContext(E->getSubExpr(), accessContext);
   }
 
   bool shouldWalkIntoClosure(AbstractClosureExpr *closure) const {
@@ -3659,8 +3670,7 @@ class ExprAvailabilityWalker : public ASTWalker {
   }
 
   /// Walk the given expression in the member access context.
-  void walkInContext(Expr *baseExpr, Expr *E,
-                     MemberAccessContext AccessContext) {
+  void walkInContext(Expr *E, MemberAccessContext AccessContext) {
     llvm::SaveAndRestore<MemberAccessContext>
       C(this->AccessContext, AccessContext);
     E->walk(*this);
@@ -3687,28 +3697,25 @@ class ExprAvailabilityWalker : public ASTWalker {
     // this probably needs to be refined to not assume that the accesses are
     // specifically using the getter/setter.
     switch (AccessContext) {
-    case MemberAccessContext::Getter:
+    case MemberAccessContext::Default:
+    case MemberAccessContext::Load:
       diagAccessorAvailability(D->getOpaqueAccessor(AccessorKind::Get),
                                ReferenceRange, ReferenceDC, std::nullopt);
       break;
 
-    case MemberAccessContext::Setter:
-      if (!getEnclosingLoadExpr()) {
-        diagAccessorAvailability(D->getOpaqueAccessor(AccessorKind::Set),
-                                 ReferenceRange, ReferenceDC, std::nullopt);
-      }
+    case MemberAccessContext::Assignment:
+      diagAccessorAvailability(D->getOpaqueAccessor(AccessorKind::Set),
+                               ReferenceRange, ReferenceDC, std::nullopt);
       break;
 
-    case MemberAccessContext::InOut:
+    case MemberAccessContext::Writeback:
       diagAccessorAvailability(D->getOpaqueAccessor(AccessorKind::Get),
                                ReferenceRange, ReferenceDC,
                                DeclAvailabilityFlag::ForInout);
 
-      if (!getEnclosingLoadExpr()) {
-        diagAccessorAvailability(D->getOpaqueAccessor(AccessorKind::Set),
-                                 ReferenceRange, ReferenceDC,
-                                 DeclAvailabilityFlag::ForInout);
-      }
+      diagAccessorAvailability(D->getOpaqueAccessor(AccessorKind::Set),
+                               ReferenceRange, ReferenceDC,
+                               DeclAvailabilityFlag::ForInout);
       break;
     }
   }

test/Sema/availability_accessors.swift

@@ -11,17 +11,7 @@ struct Nested {
   }
 }
 
-protocol ValueProto {
-  static var defaultValue: Self { get }
-
-  var a: Nested { get set }
-
-  subscript(_ i: Int) -> Nested { get set }
-
-  @discardableResult mutating func setToZero() -> Int
-}
-
-struct StructValue: ValueProto {
+struct StructValue {
   static var defaultValue: Self { .init(a: Nested(b: 1)) }
 
   var a: Nested
@@ -39,9 +29,7 @@ struct StructValue: ValueProto {
   }
 }
 
-class ClassValue: ValueProto {
-  static var defaultValue: Self { .init(a: Nested(b: 1)) }
-
+class ClassValue {
   var a: Nested
 
   required init(a: Nested) {
@@ -60,29 +48,59 @@ class ClassValue: ValueProto {
   }
 }
 
-struct BaseStruct<T: ValueProto> {
-  var available: T = .defaultValue
+struct BaseStruct<T> {
+  var available: T {
+    get { fatalError() }
+    set { }
+  }
 
   var unavailableGetter: T {
     @available(*, unavailable)
-    get { fatalError() } // expected-note 62 {{getter for 'unavailableGetter' has been explicitly marked unavailable here}}
+    get { fatalError() } // expected-note 67 {{getter for 'unavailableGetter' has been explicitly marked unavailable here}}
     set {}
   }
 
   var unavailableSetter: T {
-    get { .defaultValue }
+    get { fatalError() }
     @available(*, unavailable)
-    set { fatalError() } // expected-note 37 {{setter for 'unavailableSetter' has been explicitly marked unavailable here}}
+    set { fatalError() } // expected-note 38 {{setter for 'unavailableSetter' has been explicitly marked unavailable here}}
   }
 
   var unavailableGetterAndSetter: T {
     @available(*, unavailable)
-    get { fatalError() } // expected-note 62 {{getter for 'unavailableGetterAndSetter' has been explicitly marked unavailable here}}
+    get { fatalError() } // expected-note 67 {{getter for 'unavailableGetterAndSetter' has been explicitly marked unavailable here}}
     @available(*, unavailable)
-    set { fatalError() } // expected-note 37 {{setter for 'unavailableGetterAndSetter' has been explicitly marked unavailable here}}
+    set { fatalError() } // expected-note 38 {{setter for 'unavailableGetterAndSetter' has been explicitly marked unavailable here}}
   }
 }
 
+struct SubscriptHelper {
+  subscript<T>(available t: T) -> () {
+    get { }
+    set { }
+  }
+
+  subscript<T>(unavailableGetter t: T) -> () {
+    @available(*, unavailable)
+    get { } // expected-note {{getter for 'subscript(unavailableGetter:)' has been explicitly marked unavailable here}}
+    set { }
+  }
+
+  subscript<T>(unavailableSetter t: T) -> () {
+    get { }
+    @available(*, unavailable)
+    set { } // expected-note {{setter for 'subscript(unavailableSetter:)' has been explicitly marked unavailable here}}
+  }
+
+  subscript<T>(unavailableGetterAndSetter t: T) -> () {
+    @available(*, unavailable)
+    get { } // expected-note {{getter for 'subscript(unavailableGetterAndSetter:)' has been explicitly marked unavailable here}}
+    @available(*, unavailable)
+    set { } // expected-note {{setter for 'subscript(unavailableGetterAndSetter:)' has been explicitly marked unavailable here}}
+  }
+
+}
+
 @discardableResult func takesInOut<T>(_ t: inout T) -> T {
   return t
 }
@@ -169,7 +187,7 @@ func testLValueAssignments_Class(_ someValue: ClassValue) {
 
   x.unavailableGetter = someValue
   x.unavailableGetter.a = someValue.a // expected-error {{getter for 'unavailableGetter' is unavailable}}
-  x.unavailableGetter[0] = someValue.a // FIXME: missing diagnostic for getter
+  x.unavailableGetter[0] = someValue.a // expected-error {{getter for 'unavailableGetter' is unavailable}}
   x.unavailableGetter[0].b = 1 // expected-error {{getter for 'unavailableGetter' is unavailable}}
 
   x.unavailableSetter = someValue // expected-error {{setter for 'unavailableSetter' is unavailable}}
@@ -179,11 +197,43 @@ func testLValueAssignments_Class(_ someValue: ClassValue) {
 
   x.unavailableGetterAndSetter = someValue // expected-error {{setter for 'unavailableGetterAndSetter' is unavailable}}
   x.unavailableGetterAndSetter.a = someValue.a // expected-error {{getter for 'unavailableGetterAndSetter' is unavailable}}
-  // FIXME: missing diagnostic for getter
-  x.unavailableGetterAndSetter[0] = someValue.a
+  x.unavailableGetterAndSetter[0] = someValue.a // expected-error {{getter for 'unavailableGetterAndSetter' is unavailable}}
   x.unavailableGetterAndSetter[0].b = 1 // expected-error {{getter for 'unavailableGetterAndSetter' is unavailable}}
 }
 
+func testSubscripts(_ s: BaseStruct<StructValue>) {
+  var x = BaseStruct<SubscriptHelper>()
+
+  // Available subscript, available member, varying argument availability
+  x.available[available: s.available] = ()
+  x.available[available: s.unavailableGetter] = () // expected-error {{getter for 'unavailableGetter' is unavailable}}
+  x.available[available: s.unavailableSetter] = ()
+  x.available[available: s.unavailableGetterAndSetter] = () // expected-error {{getter for 'unavailableGetterAndSetter' is unavailable}}
+
+  _ = x.available[available: s.available]
+  _ = x.available[available: s.unavailableGetter] // expected-error {{getter for 'unavailableGetter' is unavailable}}
+  _ = x.available[available: s.unavailableSetter]
+  _ = x.available[available: s.unavailableGetterAndSetter] // expected-error {{getter for 'unavailableGetterAndSetter' is unavailable}}
+
+  // Varying subscript availability, available member, available argument
+  x.available[unavailableGetter: s.available] = ()
+  x.available[unavailableSetter: s.available] = () // expected-error {{setter for 'subscript(unavailableSetter:)' is unavailable}}
+  x.available[unavailableGetterAndSetter: s.available] = () // expected-error {{setter for 'subscript(unavailableGetterAndSetter:)' is unavailable}}
+
+  _ = x.available[unavailableGetter: s.available] // expected-error {{getter for 'subscript(unavailableGetter:)' is unavailable}}
+  _ = x.available[unavailableSetter: s.available]
+  _ = x.available[unavailableGetterAndSetter: s.available] // expected-error {{getter for 'subscript(unavailableGetterAndSetter:)' is unavailable}}
+
+  // Available subscript, varying member availability, available argument
+  x.unavailableGetter[available: s.available] = () // expected-error {{getter for 'unavailableGetter' is unavailable}}
+  x.unavailableSetter[available: s.available] = () // expected-error {{setter for 'unavailableSetter' is unavailable}}
+  x.unavailableGetterAndSetter[available: s.available] = () // expected-error {{getter for 'unavailableGetterAndSetter' is unavailable}} expected-error {{setter for 'unavailableGetterAndSetter' is unavailable}}
+
+  _ = x.unavailableGetter[available: s.available] // expected-error {{getter for 'unavailableGetter' is unavailable}}
+  _ = x.unavailableSetter[available: s.available]
+  _ = x.unavailableGetterAndSetter[available: s.available] // expected-error {{getter for 'unavailableGetterAndSetter' is unavailable}}
+}
+
 func testDiscardedKeyPathLoads_Struct() {
   let a = [0]
   var x = BaseStruct<StructValue>()