Use AbstractStorageDecl::isSettableInSwift to prohibit direct writes to optional requirements

AnthonyLatsis · AnthonyLatsis · commit 934964d49d08 · 2022-04-20T14:27:15.000+03:00
Stop pretending that an optional requirement is immutable via the `StorageImplInfo` request.
This approach has lead astray the conformance checker and may have had a negative impact
on other code paths, and it doesn't work for imported declarations because they bypass the
request. Instead, use a forwarding `AbstractStorageDecl::isSettableInSwift` method
that special-cases optional requirements.
diff --git a/include/swift/AST/Decl.h b/include/swift/AST/Decl.h
@@ -4875,12 +4875,22 @@ class AbstractStorageDecl : public ValueDecl {
     return getImplInfo().supportsMutation();
   }
 
-  /// isSettable - Determine whether references to this decl may appear
-  /// on the left-hand side of an assignment or as the operand of a
-  /// `&` or 'inout' operator.
+  /// Determine whether references to this storage declaration may appear
+  /// on the left-hand side of an assignment, as the operand of a
+  /// `&` or 'inout' operator, or as a component in a writable key path.
   bool isSettable(const DeclContext *UseDC,
                   const DeclRefExpr *base = nullptr) const;
 
+  /// Determine whether references to this storage declaration in Swift may
+  /// appear on the left-hand side of an assignment, as the operand of a
+  /// `&` or 'inout' operator, or as a component in a writable key path.
+  ///
+  /// This method is equivalent to \c isSettable with the exception of
+  /// 'optional' storage requirements, which lack support for direct writes
+  /// in Swift.
+  bool isSettableInSwift(const DeclContext *UseDC,
+                         const DeclRefExpr *base = nullptr) const;
+
   /// Does this storage declaration have explicitly-defined accessors
   /// written in the source?
   bool hasParsedAccessors() const;
@@ -7989,6 +7999,17 @@ inline bool AbstractStorageDecl::isSettable(const DeclContext *UseDC,
   return sd->supportsMutation();
 }
 
+inline bool
+AbstractStorageDecl::isSettableInSwift(const DeclContext *UseDC,
+                                       const DeclRefExpr *base) const {
+  // TODO: Writing to an optional storage requirement is not supported in Swift.
+  if (getAttrs().hasAttribute<OptionalAttr>()) {
+    return false;
+  }
+
+  return isSettable(UseDC, base);
+}
+
 inline void
 AbstractStorageDecl::overwriteSetterAccess(AccessLevel accessLevel) {
   Accessors.setInt(accessLevel);
diff --git a/lib/SILGen/SILGenExpr.cpp b/lib/SILGen/SILGenExpr.cpp
@@ -3584,9 +3584,9 @@ SILGenModule::emitKeyPathComponentForDecl(SILLocation loc,
     // supply the settability if needed. We only reference it here if the
     // setter is public.
     if (shouldUseExternalKeyPathComponent())
-      return storage->isSettable(useDC)
-        && storage->isSetterAccessibleFrom(useDC);
-    return storage->isSettable(storage->getDeclContext());
+      return storage->isSettableInSwift(useDC) &&
+             storage->isSetterAccessibleFrom(useDC);
+    return storage->isSettableInSwift(storage->getDeclContext());
   };
 
   if (auto var = dyn_cast<VarDecl>(storage)) {
diff --git a/lib/Sema/ConstraintSystem.cpp b/lib/Sema/ConstraintSystem.cpp
@@ -1155,7 +1155,7 @@ doesStorageProduceLValue(AbstractStorageDecl *storage, Type baseType,
   }
 
   // Unsettable storage decls always produce rvalues.
-  if (!storage->isSettable(useDC, base))
+  if (!storage->isSettableInSwift(useDC, base))
     return false;
 
   if (!storage->isSetterAccessibleFrom(useDC))
@@ -6561,12 +6561,12 @@ bool ConstraintSystem::isReadOnlyKeyPathComponent(
     // WritableKeyPaths to be formed in the same conditions we did
     // in previous releases even if we should not be able to set
     // the value in this context.
-    if (!storage->isSettable(DC)) {
+    if (!storage->isSettableInSwift(DC)) {
       // A non-settable component makes the key path read-only, unless
       // a reference-writable component shows up later.
       return true;
     }
-  } else if (!storage->isSettable(nullptr) ||
+  } else if (!storage->isSettableInSwift(nullptr) ||
              !storage->isSetterAccessibleFrom(DC)) {
     // A non-settable component makes the key path read-only, unless
     // a reference-writable component shows up later.
diff --git a/lib/Sema/TypeCheckStorage.cpp b/lib/Sema/TypeCheckStorage.cpp
@@ -3309,9 +3309,7 @@ StorageImplInfoRequest::evaluate(Evaluator &evaluator,
     WriteImplKind writeImpl = WriteImplKind::Immutable;
     ReadWriteImplKind readWriteImpl = ReadWriteImplKind::Immutable;
 
-    // TODO: Writing to optional storage requirements is not supported.
-    if (!storage->getAttrs().hasAttribute<OptionalAttr>() &&
-        storage->getParsedAccessor(AccessorKind::Set)) {
+    if (storage->getParsedAccessor(AccessorKind::Set)) {
       readImpl = ReadImplKind::Get;
       writeImpl = WriteImplKind::Set;
       readWriteImpl = ReadWriteImplKind::MaterializeToTemporary;
diff --git a/test/Inputs/ObjCOptionalRequirements.h b/test/Inputs/ObjCOptionalRequirements.h
@@ -0,0 +1,6 @@
+@import Foundation;
+
+@protocol ObjCProtocol
+@optional
+@property (nonatomic, assign) BOOL flag;
+@end
diff --git a/test/SILGen/keypaths_objc.swift b/test/SILGen/keypaths_objc.swift
@@ -157,44 +157,3 @@ func dynamicMemberLookupMixedKeypaths(foo: DynamicClass<Foo>) {
   // CHECK: keypath $KeyPath<NonObjC, NSObject>, (root
   _ = foo.bar.foo.nonobjc.y 
 }
-
-@objc class Object: NSObject {
-  var name: String
-  init(name: String) {
-    self.name = name
-  }
-}
-@objc protocol ObjCProtoOptional {
-  @objc optional var object: Object { get set }
-
-  @objc optional subscript(_: Bool) -> Object { get set }
-}
-
-// CHECK-LABEL: sil hidden [ossa] @{{.*}}0B28ProtocolOptionalRequirementsyyF
-// CHECK: keypath $KeyPath<ObjCProtoOptional, Optional<Object>>, (objc "object"; root $ObjCProtoOptional; gettable_property $Optional<Object>,  id #ObjCProtoOptional.object!getter.foreign : <Self where Self : ObjCProtoOptional> (Self) -> () -> Object, getter @$[[PROP_GETTER:[_a-zA-Z0-9]+]]
-// CHECK: keypath $KeyPath<ObjCProtoOptional, Optional<Object>>, (root $ObjCProtoOptional; gettable_property $Optional<Object>,  id #ObjCProtoOptional.subscript!getter.foreign : <Self where Self : ObjCProtoOptional> (Self) -> (Bool) -> Object, getter @$[[SUBSCR_GETTER:[_a-zA-Z0-9]+]]
-// CHECK: keypath $ReferenceWritableKeyPath<ObjCProtoOptional, String>, (root $ObjCProtoOptional; gettable_property $Optional<Object>,  id #ObjCProtoOptional.object!getter.foreign : <Self where Self : ObjCProtoOptional> (Self) -> () -> Object, getter @$[[PROP_GETTER]] : {{.*}}; optional_force : $Object; settable_property $String,
-// CHECK: keypath $ReferenceWritableKeyPath<ObjCProtoOptional, String>, (root $ObjCProtoOptional; gettable_property $Optional<Object>,  id #ObjCProtoOptional.subscript!getter.foreign : <Self where Self : ObjCProtoOptional> (Self) -> (Bool) -> Object, getter @$[[SUBSCR_GETTER]] : {{.*}}; optional_force : $Object; settable_property $String,
-// CHECK: } // end sil function '${{.*}}0B28ProtocolOptionalRequirementsyyF'
-//
-// CHECK: sil shared [thunk] [ossa] @$[[PROP_GETTER]] : $@convention(thin) (@in_guaranteed ObjCProtoOptional) -> @out Optional<Object> {
-// CHECK:   [[BASE:%[0-9]+]] = open_existential_ref {{%[0-9]+}} : $ObjCProtoOptional to $[[OPENED_TY:@opened\("[-A-F0-9]+"\) ObjCProtoOptional]]
-// CHECK:   dynamic_method_br [[BASE]] : $[[OPENED_TY]], #ObjCProtoOptional.object!getter.foreign, bb1
-// CHECK: bb1({{%[0-9]+}} : $@convention(objc_method) ([[OPENED_TY]]) -> @autoreleased Object)
-// CHECK: } // end sil function '$[[PROP_GETTER]]'
-//
-// CHECK: sil shared [thunk] [ossa] @$[[SUBSCR_GETTER]] : $@convention(thin) (@in_guaranteed ObjCProtoOptional, UnsafeRawPointer) -> @out Optional<Object> {
-// CHECK:   [[BASE:%[0-9]+]] = open_existential_ref {{%[0-9]+}} : $ObjCProtoOptional to $[[OPENED_TY:@opened\("[-A-F0-9]+"\) ObjCProtoOptional]]
-// CHECK:   [[INDEX:%[0-9]+]] = load [trivial] {{%[0-9]+}} : $*Bool
-// CHECK:   dynamic_method_br [[BASE]] : $[[OPENED_TY]], #ObjCProtoOptional.subscript!getter.foreign, bb1, bb2
-// CHECK: bb1({{%[0-9]+}} : $@convention(objc_method) (ObjCBool, [[OPENED_TY]]) -> @autoreleased Object):
-// CHECK:   %17 = apply {{%[0-9]+}}([[INDEX]]) : $@callee_guaranteed (Bool) -> @owned Object
-// CHECK: bb2:
-// CHECK: } // end sil function '$[[SUBSCR_GETTER]]'
-func objcProtocolOptionalRequirements() {
-  _ = \ObjCProtoOptional.object
-  _ = \ObjCProtoOptional.[true]
-
-  _ = \ObjCProtoOptional.object!.name
-  _ = \ObjCProtoOptional.[true]!.name
-}
diff --git a/test/SILGen/keypaths_objc_optional.swift b/test/SILGen/keypaths_objc_optional.swift
@@ -0,0 +1,69 @@
+// RUN: %target-swift-emit-silgen -import-objc-header %swift_src_root/test/Inputs/ObjCOptionalRequirements.h %s | %FileCheck  %s --check-prefix=CHECK --check-prefix=CHECK-%target-os-%target-cpu
+// RUN: %target-swift-emit-ir -import-objc-header %swift_src_root/test/Inputs/ObjCOptionalRequirements.h %s
+
+// REQUIRES: objc_interop
+
+import Foundation
+
+@objc class Object: NSObject {
+  var name: String
+  init(name: String) {
+    self.name = name
+  }
+}
+@objc protocol SwiftProtocol {
+  @objc optional var object: Object { get set }
+
+  @objc optional subscript(_: Bool) -> Object { get set }
+}
+
+// CHECK-LABEL: sil hidden [ossa] @{{.*}}testKeyPathAccessorsForOptionalStorageComponentsyyF
+// CHECK: keypath $KeyPath<SwiftProtocol, Optional<Object>>, (objc "object"; root $SwiftProtocol; gettable_property $Optional<Object>,  id #SwiftProtocol.object!getter.foreign : <Self where Self : SwiftProtocol> (Self) -> () -> Object, getter @$[[SWIFT_PROP_GETTER:[_a-zA-Z0-9]+]]
+// CHECK: keypath $KeyPath<SwiftProtocol, Optional<Object>>, (root $SwiftProtocol; gettable_property $Optional<Object>,  id #SwiftProtocol.subscript!getter.foreign : <Self where Self : SwiftProtocol> (Self) -> (Bool) -> Object, getter @$[[SWIFT_SUBSCR_GETTER:[_a-zA-Z0-9]+]]
+// CHECK: keypath $ReferenceWritableKeyPath<SwiftProtocol, String>, (root $SwiftProtocol; gettable_property $Optional<Object>,  id #SwiftProtocol.object!getter.foreign : <Self where Self : SwiftProtocol> (Self) -> () -> Object, getter @$[[SWIFT_PROP_GETTER]] : {{.*}}; optional_force : $Object; settable_property $String,
+// CHECK: keypath $ReferenceWritableKeyPath<SwiftProtocol, String>, (root $SwiftProtocol; gettable_property $Optional<Object>,  id #SwiftProtocol.subscript!getter.foreign : <Self where Self : SwiftProtocol> (Self) -> (Bool) -> Object, getter @$[[SWIFT_SUBSCR_GETTER]] : {{.*}}; optional_force : $Object; settable_property $String,
+// CHECK: keypath $KeyPath<ObjCProtocol, Optional<Bool>>, (objc "flag"; root $ObjCProtocol; gettable_property $Optional<Bool>,  id #ObjCProtocol.flag!getter.foreign : <Self where Self : ObjCProtocol> (Self) -> () -> Bool, getter @$[[OBJC_PROP_GETTER:[_a-zA-Z0-9]+]]
+// CHECK: } // end sil function '${{.*}}testKeyPathAccessorsForOptionalStorageComponentsyyF'
+//
+// CHECK: sil shared [thunk] [ossa] @$[[SWIFT_PROP_GETTER]] : $@convention(thin) (@in_guaranteed SwiftProtocol) -> @out Optional<Object> {
+// CHECK:   [[BASE:%[0-9]+]] = open_existential_ref {{%[0-9]+}} : $SwiftProtocol to $[[OPENED_TY:@opened\("[-A-F0-9]+"\) SwiftProtocol]]
+// CHECK:   dynamic_method_br [[BASE]] : $[[OPENED_TY]], #SwiftProtocol.object!getter.foreign, bb1
+// CHECK: bb1({{%[0-9]+}} : $@convention(objc_method) ([[OPENED_TY]]) -> @autoreleased Object)
+// CHECK: } // end sil function '$[[SWIFT_PROP_GETTER]]'
+//
+// CHECK: sil shared [thunk] [ossa] @$[[SWIFT_SUBSCR_GETTER]] : $@convention(thin) (@in_guaranteed SwiftProtocol, UnsafeRawPointer) -> @out Optional<Object> {
+// CHECK:   [[BASE:%[0-9]+]] = open_existential_ref {{%[0-9]+}} : $SwiftProtocol to $[[OPENED_TY:@opened\("[-A-F0-9]+"\) SwiftProtocol]]
+// CHECK:   [[INDEX:%[0-9]+]] = load [trivial] {{%[0-9]+}} : $*Bool
+// CHECK:   dynamic_method_br [[BASE]] : $[[OPENED_TY]], #SwiftProtocol.subscript!getter.foreign, bb1, bb2
+// CHECK: bb1({{%[0-9]+}} : $@convention(objc_method) (ObjCBool, [[OPENED_TY]]) -> @autoreleased Object):
+// CHECK:   %17 = apply {{%[0-9]+}}([[INDEX]]) : $@callee_guaranteed (Bool) -> @owned Object
+// CHECK: bb2:
+// CHECK: } // end sil function '$[[SWIFT_SUBSCR_GETTER]]'
+//
+// CHECK: sil shared [thunk] [ossa] @$[[OBJC_PROP_GETTER]] : $@convention(thin) (@in_guaranteed ObjCProtocol) -> @out Optional<Bool> {
+// CHECK: bb0([[OUT:%[0-9]+]] : $*Optional<Bool>,
+// CHECK:   [[BASE:%[0-9]+]] = open_existential_ref {{%[0-9]+}} : $ObjCProtocol to $[[OPENED_TY:@opened\("[-A-F0-9]+"\) ObjCProtocol]]
+// CHECK:   [[DEST:%[0-9]+]] = alloc_stack $Optional<Bool>
+// CHECK:   dynamic_method_br [[BASE]] : $[[OPENED_TY]], #ObjCProtocol.flag!getter.foreign, bb1, bb2
+// CHECK: bb1({{%[0-9]+}} : $@convention(objc_method) ([[OPENED_TY]]) -> {{ObjCBool|Bool}}):
+// CHECK-macosx-x86_64: function_ref @${{.*}} : $@convention(thin) (@guaranteed @callee_guaranteed () -> ObjCBool) -> Bool
+// CHECK:   inject_enum_addr [[DEST]] {{.*}} #Optional.some!enumelt
+// CHECK:   br bb3
+// CHECK: bb2:
+// CHECK:   inject_enum_addr [[DEST]] {{.*}} #Optional.none!enumelt
+// CHECK:   br bb3
+// CHECK: bb3:
+// CHECK:   [[TMP:%[0-9]+]] = load [trivial] [[DEST]]
+// CHECK:   store [[TMP]] to [trivial] [[OUT]]
+// CHECK: } // end sil function '$[[OBJC_PROP_GETTER]]'
+func testKeyPathAccessorsForOptionalStorageComponents() {
+  _ = \SwiftProtocol.object
+  _ = \SwiftProtocol.[true]
+
+  _ = \SwiftProtocol.object!.name
+  _ = \SwiftProtocol.[true]!.name
+
+  _ = \ObjCProtocol.flag
+}
+
+// CHECK-macosx-x86_64: sil [transparent] [serialized] @$s10ObjectiveC22_convertObjCBoolToBoolySbAA0cD0VF : $@convention(thin) (ObjCBool) -> Bool
diff --git a/test/decl/protocol/conforms/near_miss_objc.swift b/test/decl/protocol/conforms/near_miss_objc.swift
@@ -153,3 +153,14 @@ class C10Sub : C10Super, P8 {
 class C11 : P11 {
   func f(waggle: Int) { } // no warning
 }
+
+@objc protocol P12 {
+  @objc optional var prop: Bool { get set } // expected-note {{requirement 'prop' declared here}}
+}
+class C12: P12 {
+  var prop: Bool { true }
+  // expected-warning@-1 {{property 'prop' nearly matches optional requirement 'prop' of protocol 'P12'}}
+  // expected-note@-2 {{candidate is not settable, but protocol requires it}}
+  // expected-note@-3 {{move 'prop' to an extension to silence this warning}}
+  // expected-note@-4 {{make 'prop' private to silence this warning}}
+}
diff --git a/test/expr/primary/keypath/swift-keypath-objc-optional-component.swift b/test/expr/primary/keypath/swift-keypath-objc-optional-component.swift
@@ -1,4 +1,6 @@
-// RUN: %target-typecheck-verify-swift -disable-objc-attr-requires-foundation-module -enable-objc-interop
+// RUN: %target-typecheck-verify-swift -disable-objc-attr-requires-foundation-module -import-objc-header %swift_src_root/test/Inputs/ObjCOptionalRequirements.h
+
+// REQUIRES: objc_interop
 
 @objc class Object {
   var name: String
@@ -8,38 +10,49 @@
   }
 }
 
-@objc protocol P {
+@objc protocol SwiftProtocol {
   @objc optional var object: Object { get set }
 
-  @objc optional subscript(_: Int) -> Object { get set }
+  @objc optional subscript(_: Bool) -> Object { get set }
 }
 
 func assertExactType<T>(of _: T, is _: T.Type) {}
 
 // An optional storage component makes the key path read-only...
 do {
-  let kp_property = \P.object
-  let kp_subscript = \P.[0]
+  let kp_property = \SwiftProtocol.object
+  let kp_subscript = \SwiftProtocol.[false]
 
-  var p: P
+  var p: SwiftProtocol
   // expected-error@+1 {{cannot assign through subscript: 'kp_property' is a read-only key path}}
   p[keyPath: kp_property] = Object(name: "nope")
   // expected-error@+1 {{cannot assign through subscript: 'kp_subscript' is a read-only key path}}
   p[keyPath: kp_subscript] = Object(name: "nope")
 
-  assertExactType(of: kp_property, is: KeyPath<P, Object?>.self)
-  assertExactType(of: kp_subscript, is: KeyPath<P, Object?>.self)
+  assertExactType(of: kp_property, is: KeyPath<SwiftProtocol, Object?>.self)
+  assertExactType(of: kp_subscript, is: KeyPath<SwiftProtocol, Object?>.self)
+}
+do {
+  let kp_property_objc = \ObjCProtocol.flag
+
+  var p: ObjCProtocol
+  // expected-error@+1 {{cannot assign through subscript: 'kp_property_objc' is a read-only key path}}
+  p[keyPath: kp_property_objc] = false
+
+  assertExactType(of: kp_property_objc, is: KeyPath<ObjCProtocol, Bool?>.self)
 }
 
 // ...unless a reference-writable component shows up later.
 do {
-  let kp_propertyForce_name = \P.object!.name
-  let kp_subscriptForce_name = \P.[0]!.name
+  let kp_propertyForce_name = \SwiftProtocol.object!.name
+  let kp_subscriptForce_name = \SwiftProtocol.[true]!.name
 
-  let p: P
+  let p: SwiftProtocol
   p[keyPath: kp_propertyForce_name] = "yes"
   p[keyPath: kp_subscriptForce_name] = "yes"
 
-  assertExactType(of: kp_propertyForce_name, is: ReferenceWritableKeyPath<P, String>.self)
-  assertExactType(of: kp_subscriptForce_name, is: ReferenceWritableKeyPath<P, String>.self)
+  assertExactType(of: kp_propertyForce_name,
+                  is: ReferenceWritableKeyPath<SwiftProtocol, String>.self)
+  assertExactType(of: kp_subscriptForce_name,
+                  is: ReferenceWritableKeyPath<SwiftProtocol, String>.self)
 }
