Sema: Fix accessor availability checking in argument lists.

tshortli · tshortli · commit 0dbbb6817138 · 2024-06-27T11:13:52.000-07:00
The changes in #72410 caused a regression when checking availability in the following example: ``` // warning: Setter for 'hasDeprecatedSetter' is deprecated: ... x[y.hasDeprecatedSetter] = ... ``` The result of `y.hasDeprecatedSetter` is being passed as an argument to the subscript and its setter will not be called. To fix this, `ExprAvailabilityWalker` now consistently creates a new default `MemberAccessContext` when descending into any `Argument`, since the access context for the expressions surrounding the call should not affect the arguments to the call. Additionally, `MemberAccessContext` has been refactored to better model context state transitions. Instead of only modeling which accessors will be called, the enumeration's members now reflect the possible states that `ExprAvailabilityWalker` can be in during its traversal. This should hopefully make it easier to follow the logic for traversal of `LoadExpr`s and arguments. Resolves rdar://130487998.
diff --git a/lib/Sema/TypeCheckAvailability.cpp b/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;
     }
   }
diff --git a/test/Sema/availability_accessors.swift b/test/Sema/availability_accessors.swift
@@ -56,21 +56,21 @@ struct BaseStruct<T> {
 
   var unavailableGetter: T {
     @available(*, unavailable)
-    get { fatalError() } // expected-note 65 {{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 { fatalError() }
     @available(*, unavailable)
-    set { fatalError() } // expected-note 39 {{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 65 {{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 39 {{setter for 'unavailableGetterAndSetter' has been explicitly marked unavailable here}}
+    set { fatalError() } // expected-note 38 {{setter for 'unavailableGetterAndSetter' has been explicitly marked unavailable here}}
   }
 }
 
@@ -187,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}}
@@ -197,8 +197,7 @@ 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}}
 }
 
@@ -207,11 +206,9 @@ func testSubscripts(_ s: BaseStruct<StructValue>) {
 
   // Available subscript, available member, varying argument availability
   x.available[available: s.available] = ()
-  x.available[available: s.unavailableGetter] = () // FIXME: missing diagnostic for getter
-  // FIXME: spurious diagnostic for setter
-  x.available[available: s.unavailableSetter] = () // expected-error {{setter for 'unavailableSetter' is unavailable}}
-  // FIXME: spurious diagnostic for setter
-  x.available[available: s.unavailableGetterAndSetter] = () // expected-error {{setter for 'unavailableGetterAndSetter' is unavailable}}
+  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}}
