Sema: Infer key path immutability during expression availability checking.

Accessor availability diagnostics for key path expressions were first
introduced by #83931. Those changes were
insufficient because sometimes key path expressions are generated in the AST
with more mutability than is needed by the context and so setter availability
could be diagnosed inappropriately. Key path expression binding was refined in
#84491 to make this less likely to
occur. However, there are still some circumstances in which a mutable key path
is generated in the AST and then immediately coerced into an immutable key path
to satisfy the contextual type. This change infers the immutability of these key
path expressions by looking through surrounding conversion expressions.

Author: tshortli

lib/Sema/TypeCheckAvailability.cpp

@@ -29,6 +29,7 @@
 #include "swift/AST/ClangModuleLoader.h"
 #include "swift/AST/DeclExportabilityVisitor.h"
 #include "swift/AST/DiagnosticsParse.h"
+#include "swift/AST/ExistentialLayout.h"
 #include "swift/AST/GenericEnvironment.h"
 #include "swift/AST/Initializer.h"
 #include "swift/AST/NameLookup.h"
@@ -2232,9 +2233,6 @@ class ExprAvailabilityWalker : public BaseDiagnosticWalker {
     /// The context does not involve a key path access.
     None,
 
-    /// The context is an expression that is coerced to a read-only key path.
-    ReadOnlyCoercion,
-
     /// The context is a key path application (`x[keyPath: \.member]`).
     Application,
   };
@@ -2361,15 +2359,6 @@ class ExprAvailabilityWalker : public BaseDiagnosticWalker {
                                           FCE->getLoc(),
                                           Where.getDeclContext());
     }
-    if (auto DTBE = dyn_cast<DerivedToBaseExpr>(E)) {
-      if (auto ty = DTBE->getType()) {
-        if (ty->isKeyPath()) {
-          walkInKeyPathAccessContext(DTBE->getSubExpr(),
-                                     KeyPathAccessContext::ReadOnlyCoercion);
-          return Action::SkipChildren(E);
-        }
-      }
-    }
     if (auto KP = dyn_cast<KeyPathExpr>(E)) {
       maybeDiagKeyPath(KP);
     }
@@ -2586,19 +2575,49 @@ class ExprAvailabilityWalker : public BaseDiagnosticWalker {
 
   StorageAccessKind getStorageAccessKindForKeyPath(KeyPathExpr *KP) const {
     switch (KeyPathAccess) {
-    case KeyPathAccessContext::None:
-      // Use the key path's type to determine the access kind.
-      if (!KP->isObjC())
-        if (auto keyPathType = KP->getKeyPathType())
-          if (keyPathType->isKeyPath())
+    case KeyPathAccessContext::None: {
+      // Obj-C key paths are always considered mutable.
+      if (KP->isObjC())
+        return StorageAccessKind::GetSet;
+
+      // Check whether key path type indicates immutability, in which case only
+      // the getter will be used.
+      if (auto keyPathType = KP->getKeyPathType())
+        if (keyPathType->isKnownImmutableKeyPathType())
+          return StorageAccessKind::Get;
+
+      // Check if there's a conversion expression containing this one directly
+      // above it in the stack. If so, and that conversion is to an immutable
+      // key path type, then we should infer that use of the key path only
+      // implies use of the getter.
+      ArrayRef<const Expr *> exprs = ExprStack;
+      // Must be called while visiting the given key path expression.
+      ASSERT(exprs.back() == KP);
+
+      for (auto *expr : llvm::reverse(exprs.drop_back())) {
+        // Only traverse through conversions on the stack.
+        if (!isa<ImplicitConversionExpr>(expr) &&
+            !isa<OpenExistentialExpr>(expr))
+          break;
+
+        if (auto ty = expr->getType()) {
+          if (ty->isKnownImmutableKeyPathType())
             return StorageAccessKind::Get;
 
-      return StorageAccessKind::GetSet;
+          if (auto existential = dyn_cast<ExistentialType>(ty)) {
+            if (auto superclass =
+                    existential->getExistentialLayout().getSuperclass()) {
+              if (superclass->isKnownImmutableKeyPathType())
+                return StorageAccessKind::Get;
+            }
+          }
+        }
+      }
 
-    case KeyPathAccessContext::ReadOnlyCoercion:
-      // The type of this key path is being coerced to the type KeyPath<_, _> so
-      // ignore the actual key path type.
-      return StorageAccessKind::Get;
+      // We failed to prove that the key path is only used immutably,
+      // so diagnose both get and set access.
+      return StorageAccessKind::GetSet;
+    }
 
     case KeyPathAccessContext::Application:
       // The key path is being applied directly to a base so treat it as if it

test/Availability/availability_accessors.swift

@@ -56,7 +56,7 @@ struct BaseStruct<T> {
 
   var unavailableGetter: T {
     @available(*, unavailable)
-    get { fatalError() } // expected-note 69 {{getter for 'unavailableGetter' has been explicitly marked unavailable here}}
+    get { fatalError() } // expected-note 73 {{getter for 'unavailableGetter' has been explicitly marked unavailable here}}
     set { }
   }
 
@@ -68,7 +68,7 @@ struct BaseStruct<T> {
 
   var unavailableGetterAndSetter: T {
     @available(*, unavailable)
-    get { fatalError() } // expected-note 68 {{getter for 'unavailableGetterAndSetter' has been explicitly marked unavailable here}}
+    get { fatalError() } // expected-note 71 {{getter for 'unavailableGetterAndSetter' has been explicitly marked unavailable here}}
     @available(*, unavailable)
     set { fatalError() } // expected-note 33 {{setter for 'unavailableGetterAndSetter' has been explicitly marked unavailable here}}
   }
@@ -116,11 +116,6 @@ struct SubscriptHelper {
   return t
 }
 
-func takesKeyPath<T, U>(_ t: T, _ keyPath: KeyPath<T, U>) -> () { }
-func takesWritableKeyPath<T, U>(_ t: inout T, _ keyPath: WritableKeyPath<T, U>) -> () {
-  // expected-note@-1 2 {{in call to function 'takesWritableKeyPath'}}
-}
-
 func testDiscardedRValueLoads_Struct() {
   var x = BaseStruct<StructValue>() // expected-warning {{variable 'x' was never mutated; consider changing to 'let' constant}}
 
@@ -548,13 +543,44 @@ func testDiscardedApplyOfFuncWithInOutParam_Class() {
 func testKeyPathArguments_Struct() {
   var x = BaseStruct<StructValue>()
 
+  func takesKeyPath<T, U>(_ t: T, _ keyPath: KeyPath<T, U>) -> () { }
+
   takesKeyPath(x, \.available)
   takesKeyPath(x, \.unavailableGetter) // expected-error {{getter for 'unavailableGetter' is unavailable}}
   takesKeyPath(x, \.unavailableSetter)
   takesKeyPath(x, \.unavailableGetterAndSetter) // expected-error {{getter for 'unavailableGetterAndSetter' is unavailable}}
   takesKeyPath(x, \.deprecatedGetter) // expected-warning {{getter for 'deprecatedGetter' is deprecated: reading not recommended}}
   takesKeyPath(x, \.deprecatedSetter)
 
+  func takesAnyKeyPath(_ keyPath: AnyKeyPath) -> () { }
+
+  takesAnyKeyPath(\BaseStruct<StructValue>.available)
+  takesAnyKeyPath(\BaseStruct<StructValue>.unavailableGetter) // expected-error {{getter for 'unavailableGetter' is unavailable}}
+  takesAnyKeyPath(\BaseStruct<StructValue>.unavailableSetter)
+  takesAnyKeyPath(\BaseStruct<StructValue>.unavailableGetterAndSetter) // expected-error {{getter for 'unavailableGetterAndSetter' is unavailable}}
+  takesAnyKeyPath(\BaseStruct<StructValue>.deprecatedGetter) // expected-warning {{getter for 'deprecatedGetter' is deprecated: reading not recommended}}
+  takesAnyKeyPath(\BaseStruct<StructValue>.deprecatedSetter)
+
+  func takesPartialKeyPath<T>(_ t: T, _ keyPath: PartialKeyPath<T>) -> () { }
+  takesPartialKeyPath(x, \.available)
+  takesPartialKeyPath(x, \.unavailableGetter) // expected-error {{getter for 'unavailableGetter' is unavailable}}
+  takesPartialKeyPath(x, \.unavailableSetter)
+  takesPartialKeyPath(x, \.unavailableGetterAndSetter) // expected-error {{getter for 'unavailableGetterAndSetter' is unavailable}}
+  takesPartialKeyPath(x, \.deprecatedGetter) // expected-warning {{getter for 'deprecatedGetter' is deprecated: reading not recommended}}
+  takesPartialKeyPath(x, \.deprecatedSetter)
+
+  func takesSendableKeyPath<T, U>(_ t: T, _ keyPath: KeyPath<T, U> & Sendable) -> () { }
+
+  takesSendableKeyPath(x, \.available)
+  takesSendableKeyPath(x, \.unavailableGetter) // expected-error {{getter for 'unavailableGetter' is unavailable}}
+  takesSendableKeyPath(x, \.unavailableSetter)
+  takesSendableKeyPath(x, \.unavailableGetterAndSetter) // expected-error {{getter for 'unavailableGetterAndSetter' is unavailable}}
+  takesSendableKeyPath(x, \.deprecatedGetter) // expected-warning {{getter for 'deprecatedGetter' is deprecated: reading not recommended}}
+  takesSendableKeyPath(x, \.deprecatedSetter)
+
+  func takesWritableKeyPath<T, U>(_ t: inout T, _ keyPath: WritableKeyPath<T, U>) -> () { }
+  // expected-note@-1 2 {{in call to function 'takesWritableKeyPath'}}
+
   takesWritableKeyPath(&x, \.available)
   takesWritableKeyPath(&x, \.unavailableGetter) // expected-error {{getter for 'unavailableGetter' is unavailable}}
   // FIXME: Ideally we would diagnose the unavailability of the setter instead
@@ -566,6 +592,14 @@ func testKeyPathArguments_Struct() {
   // expected-error@-1 {{generic parameter 'U' could not be inferred}}
   takesWritableKeyPath(&x, \.deprecatedGetter) // expected-warning {{getter for 'deprecatedGetter' is deprecated: reading not recommended}}
   takesWritableKeyPath(&x, \.deprecatedSetter) // expected-warning {{setter for 'deprecatedSetter' is deprecated: writing not recommended}}
+
+  func takesUnifiedTypes<T>(_: T, _: T) { }
+
+  takesUnifiedTypes(\BaseStruct<StructValue>.available, \BaseStruct<StructValue>.available)
+  takesUnifiedTypes(\BaseStruct<StructValue>.available, \BaseStruct<StructValue>.unavailableGetter) // expected-error {{getter for 'unavailableGetter' is unavailable}}
+  takesUnifiedTypes(\BaseStruct<StructValue>.available, \BaseStruct<StructValue>.unavailableSetter)
+  takesUnifiedTypes(\BaseStruct<StructValue>.available, \BaseStruct<StructValue>.deprecatedSetter) // expected-warning {{setter for 'deprecatedSetter' is deprecated: writing not recommended}}
+  takesUnifiedTypes(\BaseStruct<StructValue>.unavailableSetter, \BaseStruct<StructValue>.deprecatedSetter)
 }
 
 var global = BaseStruct<StructValue>()