Merge pull request swiftlang#74799 from tshortli/accessor-availability-6.0

[6.0] Sema: Fix accessor availability checking regressions caused by swiftlang#72412

Author: tshortli

lib/Sema/TypeCheckAvailability.cpp

@@ -3319,25 +3319,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;
 
@@ -3354,6 +3361,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();
 
@@ -3476,6 +3490,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);
   }
 
@@ -3532,20 +3551,6 @@ class ExprAvailabilityWalker : public ASTWalker {
     return call;
   }
 
-  /// Walks up to the first enclosing LoadExpr and returns it.
-  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)) {
-      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
@@ -3561,32 +3566,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.
@@ -3639,12 +3650,13 @@ class ExprAvailabilityWalker : public ASTWalker {
 
   /// Walk an inout expression, checking for availability.
   void walkInOutExpr(InOutExpr *E) {
-    // If there is a LoadExpr in the stack, then this InOutExpr is not actually
-    // indicative of any mutation so the access context should just be Getter.
-    auto accessContext = getEnclosingLoadExpr() ? MemberAccessContext::Getter
-                                                : MemberAccessContext::InOut;
-
-    walkInContext(E, E->getSubExpr(), accessContext);
+    // 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 {
@@ -3665,10 +3677,8 @@ class ExprAvailabilityWalker : public ASTWalker {
     return;
   }
 
-
   /// 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);
@@ -3695,17 +3705,18 @@ 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:
+    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);