Skip to content

Commit 6cd9f0a

Browse files
jckarteraschwaighofer
jckarter
authored and
aschwaighofer
committed
Merge pull request #42490 from AnthonyLatsis/5.7-objc-optional-keypath-fix
🍒 [5.7-04182022] Use `AbstractStorageDecl::isSettableInSwift` to prohibit direct writes to `optional` requirements
1 parent 97d0487 commit 6cd9f0a

File tree

9 files changed

+143
-66
lines changed

9 files changed

+143
-66
lines changed

‎include/swift/AST/Decl.h‎

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4865,12 +4865,22 @@ class AbstractStorageDecl : public ValueDecl {
48654865
return getImplInfo().supportsMutation();
48664866
}
48674867

4868-
/// isSettable - Determine whether references to this decl may appear
4869-
/// on the left-hand side of an assignment or as the operand of a
4870-
/// `&` or 'inout' operator.
4868+
/// Determine whether references to this storage declaration may appear
4869+
/// on the left-hand side of an assignment, as the operand of a
4870+
/// `&` or 'inout' operator, or as a component in a writable key path.
48714871
bool isSettable(const DeclContext *UseDC,
48724872
const DeclRefExpr *base = nullptr) const;
48734873

4874+
/// Determine whether references to this storage declaration in Swift may
4875+
/// appear on the left-hand side of an assignment, as the operand of a
4876+
/// `&` or 'inout' operator, or as a component in a writable key path.
4877+
///
4878+
/// This method is equivalent to \c isSettable with the exception of
4879+
/// 'optional' storage requirements, which lack support for direct writes
4880+
/// in Swift.
4881+
bool isSettableInSwift(const DeclContext *UseDC,
4882+
const DeclRefExpr *base = nullptr) const;
4883+
48744884
/// Does this storage declaration have explicitly-defined accessors
48754885
/// written in the source?
48764886
bool hasParsedAccessors() const;
@@ -7976,6 +7986,17 @@ inline bool AbstractStorageDecl::isSettable(const DeclContext *UseDC,
79767986
return sd->supportsMutation();
79777987
}
79787988

7989+
inline bool
7990+
AbstractStorageDecl::isSettableInSwift(const DeclContext *UseDC,
7991+
const DeclRefExpr *base) const {
7992+
// TODO: Writing to an optional storage requirement is not supported in Swift.
7993+
if (getAttrs().hasAttribute<OptionalAttr>()) {
7994+
return false;
7995+
}
7996+
7997+
return isSettable(UseDC, base);
7998+
}
7999+
79798000
inline void
79808001
AbstractStorageDecl::overwriteSetterAccess(AccessLevel accessLevel) {
79818002
Accessors.setInt(accessLevel);

‎lib/SILGen/SILGenExpr.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3584,9 +3584,9 @@ SILGenModule::emitKeyPathComponentForDecl(SILLocation loc,
35843584
// supply the settability if needed. We only reference it here if the
35853585
// setter is public.
35863586
if (shouldUseExternalKeyPathComponent())
3587-
return storage->isSettable(useDC)
3588-
&& storage->isSetterAccessibleFrom(useDC);
3589-
return storage->isSettable(storage->getDeclContext());
3587+
return storage->isSettableInSwift(useDC) &&
3588+
storage->isSetterAccessibleFrom(useDC);
3589+
return storage->isSettableInSwift(storage->getDeclContext());
35903590
};
35913591

35923592
if (auto var = dyn_cast<VarDecl>(storage)) {

‎lib/Sema/ConstraintSystem.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1155,7 +1155,7 @@ doesStorageProduceLValue(AbstractStorageDecl *storage, Type baseType,
11551155
}
11561156

11571157
// Unsettable storage decls always produce rvalues.
1158-
if (!storage->isSettable(useDC, base))
1158+
if (!storage->isSettableInSwift(useDC, base))
11591159
return false;
11601160

11611161
if (!storage->isSetterAccessibleFrom(useDC))
@@ -6557,12 +6557,12 @@ bool ConstraintSystem::isReadOnlyKeyPathComponent(
65576557
// WritableKeyPaths to be formed in the same conditions we did
65586558
// in previous releases even if we should not be able to set
65596559
// the value in this context.
6560-
if (!storage->isSettable(DC)) {
6560+
if (!storage->isSettableInSwift(DC)) {
65616561
// A non-settable component makes the key path read-only, unless
65626562
// a reference-writable component shows up later.
65636563
return true;
65646564
}
6565-
} else if (!storage->isSettable(nullptr) ||
6565+
} else if (!storage->isSettableInSwift(nullptr) ||
65666566
!storage->isSetterAccessibleFrom(DC)) {
65676567
// A non-settable component makes the key path read-only, unless
65686568
// a reference-writable component shows up later.

‎lib/Sema/TypeCheckStorage.cpp‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3359,9 +3359,7 @@ StorageImplInfoRequest::evaluate(Evaluator &evaluator,
33593359
WriteImplKind writeImpl = WriteImplKind::Immutable;
33603360
ReadWriteImplKind readWriteImpl = ReadWriteImplKind::Immutable;
33613361

3362-
// TODO: Writing to optional storage requirements is not supported.
3363-
if (!storage->getAttrs().hasAttribute<OptionalAttr>() &&
3364-
storage->getParsedAccessor(AccessorKind::Set)) {
3362+
if (storage->getParsedAccessor(AccessorKind::Set)) {
33653363
readImpl = ReadImplKind::Get;
33663364
writeImpl = WriteImplKind::Set;
33673365
readWriteImpl = ReadWriteImplKind::MaterializeToTemporary;

‎test/Inputs/ObjCOptionalRequirements.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
@import Foundation;
2+
3+
@protocol ObjCProtocol
4+
@optional
5+
@property (nonatomic, assign) BOOL flag;
6+
@end

‎test/SILGen/keypaths_objc.swift‎

Lines changed: 0 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -157,44 +157,3 @@ func dynamicMemberLookupMixedKeypaths(foo: DynamicClass<Foo>) {
157157
// CHECK: keypath $KeyPath<NonObjC, NSObject>, (root
158158
_ = foo.bar.foo.nonobjc.y
159159
}
160-
161-
@objc class Object: NSObject {
162-
var name: String
163-
init(name: String) {
164-
self.name = name
165-
}
166-
}
167-
@objc protocol ObjCProtoOptional {
168-
@objc optional var object: Object { get set }
169-
170-
@objc optional subscript(_: Bool) -> Object { get set }
171-
}
172-
173-
// CHECK-LABEL: sil hidden [ossa] @{{.*}}0B28ProtocolOptionalRequirementsyyF
174-
// 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]+]]
175-
// 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]+]]
176-
// 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,
177-
// 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,
178-
// CHECK: } // end sil function '${{.*}}0B28ProtocolOptionalRequirementsyyF'
179-
//
180-
// CHECK: sil shared [thunk] [ossa] @$[[PROP_GETTER]] : $@convention(thin) (@in_guaranteed ObjCProtoOptional) -> @out Optional<Object> {
181-
// CHECK: [[BASE:%[0-9]+]] = open_existential_ref {{%[0-9]+}} : $ObjCProtoOptional to $[[OPENED_TY:@opened\("[-A-F0-9]+"\) ObjCProtoOptional]]
182-
// CHECK: dynamic_method_br [[BASE]] : $[[OPENED_TY]], #ObjCProtoOptional.object!getter.foreign, bb1
183-
// CHECK: bb1({{%[0-9]+}} : $@convention(objc_method) ([[OPENED_TY]]) -> @autoreleased Object)
184-
// CHECK: } // end sil function '$[[PROP_GETTER]]'
185-
//
186-
// CHECK: sil shared [thunk] [ossa] @$[[SUBSCR_GETTER]] : $@convention(thin) (@in_guaranteed ObjCProtoOptional, UnsafeRawPointer) -> @out Optional<Object> {
187-
// CHECK: [[BASE:%[0-9]+]] = open_existential_ref {{%[0-9]+}} : $ObjCProtoOptional to $[[OPENED_TY:@opened\("[-A-F0-9]+"\) ObjCProtoOptional]]
188-
// CHECK: [[INDEX:%[0-9]+]] = load [trivial] {{%[0-9]+}} : $*Bool
189-
// CHECK: dynamic_method_br [[BASE]] : $[[OPENED_TY]], #ObjCProtoOptional.subscript!getter.foreign, bb1, bb2
190-
// CHECK: bb1({{%[0-9]+}} : $@convention(objc_method) (ObjCBool, [[OPENED_TY]]) -> @autoreleased Object):
191-
// CHECK: %17 = apply {{%[0-9]+}}([[INDEX]]) : $@callee_guaranteed (Bool) -> @owned Object
192-
// CHECK: bb2:
193-
// CHECK: } // end sil function '$[[SUBSCR_GETTER]]'
194-
func objcProtocolOptionalRequirements() {
195-
_ = \ObjCProtoOptional.object
196-
_ = \ObjCProtoOptional.[true]
197-
198-
_ = \ObjCProtoOptional.object!.name
199-
_ = \ObjCProtoOptional.[true]!.name
200-
}

‎test/SILGen/keypaths_objc_optional.swift‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
// 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
2+
// RUN: %target-swift-emit-ir -import-objc-header %swift_src_root/test/Inputs/ObjCOptionalRequirements.h %s
3+
4+
// REQUIRES: objc_interop
5+
6+
import Foundation
7+
8+
@objc class Object: NSObject {
9+
var name: String
10+
init(name: String) {
11+
self.name = name
12+
}
13+
}
14+
@objc protocol SwiftProtocol {
15+
@objc optional var object: Object { get set }
16+
17+
@objc optional subscript(_: Bool) -> Object { get set }
18+
}
19+
20+
// CHECK-LABEL: sil hidden [ossa] @{{.*}}testKeyPathAccessorsForOptionalStorageComponentsyyF
21+
// 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]+]]
22+
// 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]+]]
23+
// 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,
24+
// 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,
25+
// 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]+]]
26+
// CHECK: } // end sil function '${{.*}}testKeyPathAccessorsForOptionalStorageComponentsyyF'
27+
//
28+
// CHECK: sil shared [thunk] [ossa] @$[[SWIFT_PROP_GETTER]] : $@convention(thin) (@in_guaranteed SwiftProtocol) -> @out Optional<Object> {
29+
// CHECK: [[BASE:%[0-9]+]] = open_existential_ref {{%[0-9]+}} : $SwiftProtocol to $[[OPENED_TY:@opened\("[-A-F0-9]+"\) SwiftProtocol]]
30+
// CHECK: dynamic_method_br [[BASE]] : $[[OPENED_TY]], #SwiftProtocol.object!getter.foreign, bb1
31+
// CHECK: bb1({{%[0-9]+}} : $@convention(objc_method) ([[OPENED_TY]]) -> @autoreleased Object)
32+
// CHECK: } // end sil function '$[[SWIFT_PROP_GETTER]]'
33+
//
34+
// CHECK: sil shared [thunk] [ossa] @$[[SWIFT_SUBSCR_GETTER]] : $@convention(thin) (@in_guaranteed SwiftProtocol, UnsafeRawPointer) -> @out Optional<Object> {
35+
// CHECK: [[BASE:%[0-9]+]] = open_existential_ref {{%[0-9]+}} : $SwiftProtocol to $[[OPENED_TY:@opened\("[-A-F0-9]+"\) SwiftProtocol]]
36+
// CHECK: [[INDEX:%[0-9]+]] = load [trivial] {{%[0-9]+}} : $*Bool
37+
// CHECK: dynamic_method_br [[BASE]] : $[[OPENED_TY]], #SwiftProtocol.subscript!getter.foreign, bb1, bb2
38+
// CHECK: bb1({{%[0-9]+}} : $@convention(objc_method) (ObjCBool, [[OPENED_TY]]) -> @autoreleased Object):
39+
// CHECK: %17 = apply {{%[0-9]+}}([[INDEX]]) : $@callee_guaranteed (Bool) -> @owned Object
40+
// CHECK: bb2:
41+
// CHECK: } // end sil function '$[[SWIFT_SUBSCR_GETTER]]'
42+
//
43+
// CHECK: sil shared [thunk] [ossa] @$[[OBJC_PROP_GETTER]] : $@convention(thin) (@in_guaranteed ObjCProtocol) -> @out Optional<Bool> {
44+
// CHECK: bb0([[OUT:%[0-9]+]] : $*Optional<Bool>,
45+
// CHECK: [[BASE:%[0-9]+]] = open_existential_ref {{%[0-9]+}} : $ObjCProtocol to $[[OPENED_TY:@opened\("[-A-F0-9]+"\) ObjCProtocol]]
46+
// CHECK: [[DEST:%[0-9]+]] = alloc_stack $Optional<Bool>
47+
// CHECK: dynamic_method_br [[BASE]] : $[[OPENED_TY]], #ObjCProtocol.flag!getter.foreign, bb1, bb2
48+
// CHECK: bb1({{%[0-9]+}} : $@convention(objc_method) ([[OPENED_TY]]) -> {{ObjCBool|Bool}}):
49+
// CHECK-macosx-x86_64: function_ref @${{.*}} : $@convention(thin) (@guaranteed @callee_guaranteed () -> ObjCBool) -> Bool
50+
// CHECK: inject_enum_addr [[DEST]] {{.*}} #Optional.some!enumelt
51+
// CHECK: br bb3
52+
// CHECK: bb2:
53+
// CHECK: inject_enum_addr [[DEST]] {{.*}} #Optional.none!enumelt
54+
// CHECK: br bb3
55+
// CHECK: bb3:
56+
// CHECK: [[TMP:%[0-9]+]] = load [trivial] [[DEST]]
57+
// CHECK: store [[TMP]] to [trivial] [[OUT]]
58+
// CHECK: } // end sil function '$[[OBJC_PROP_GETTER]]'
59+
func testKeyPathAccessorsForOptionalStorageComponents() {
60+
_ = \SwiftProtocol.object
61+
_ = \SwiftProtocol.[true]
62+
63+
_ = \SwiftProtocol.object!.name
64+
_ = \SwiftProtocol.[true]!.name
65+
66+
_ = \ObjCProtocol.flag
67+
}
68+
69+
// CHECK-macosx-x86_64: sil [transparent] [serialized] @$s10ObjectiveC22_convertObjCBoolToBoolySbAA0cD0VF : $@convention(thin) (ObjCBool) -> Bool

‎test/decl/protocol/conforms/near_miss_objc.swift‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,3 +153,14 @@ class C10Sub : C10Super, P8 {
153153
class C11 : P11 {
154154
func f(waggle: Int) { } // no warning
155155
}
156+
157+
@objc protocol P12 {
158+
@objc optional var prop: Bool { get set } // expected-note {{requirement 'prop' declared here}}
159+
}
160+
class C12: P12 {
161+
var prop: Bool { true }
162+
// expected-warning@-1 {{property 'prop' nearly matches optional requirement 'prop' of protocol 'P12'}}
163+
// expected-note@-2 {{candidate is not settable, but protocol requires it}}
164+
// expected-note@-3 {{move 'prop' to an extension to silence this warning}}
165+
// expected-note@-4 {{make 'prop' private to silence this warning}}
166+
}

‎test/expr/primary/keypath/swift-keypath-objc-optional-component.swift‎

Lines changed: 26 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
1-
// RUN: %target-typecheck-verify-swift -disable-objc-attr-requires-foundation-module -enable-objc-interop
1+
// RUN: %target-typecheck-verify-swift -disable-objc-attr-requires-foundation-module -import-objc-header %swift_src_root/test/Inputs/ObjCOptionalRequirements.h
2+
3+
// REQUIRES: objc_interop
24

35
@objc class Object {
46
var name: String
@@ -8,38 +10,49 @@
810
}
911
}
1012

11-
@objc protocol P {
13+
@objc protocol SwiftProtocol {
1214
@objc optional var object: Object { get set }
1315

14-
@objc optional subscript(_: Int) -> Object { get set }
16+
@objc optional subscript(_: Bool) -> Object { get set }
1517
}
1618

1719
func assertExactType<T>(of _: T, is _: T.Type) {}
1820

1921
// An optional storage component makes the key path read-only...
2022
do {
21-
let kp_property = \P.object
22-
let kp_subscript = \P.[0]
23+
let kp_property = \SwiftProtocol.object
24+
let kp_subscript = \SwiftProtocol.[false]
2325

24-
var p: P
26+
var p: SwiftProtocol
2527
// expected-error@+1 {{cannot assign through subscript: 'kp_property' is a read-only key path}}
2628
p[keyPath: kp_property] = Object(name: "nope")
2729
// expected-error@+1 {{cannot assign through subscript: 'kp_subscript' is a read-only key path}}
2830
p[keyPath: kp_subscript] = Object(name: "nope")
2931

30-
assertExactType(of: kp_property, is: KeyPath<P, Object?>.self)
31-
assertExactType(of: kp_subscript, is: KeyPath<P, Object?>.self)
32+
assertExactType(of: kp_property, is: KeyPath<SwiftProtocol, Object?>.self)
33+
assertExactType(of: kp_subscript, is: KeyPath<SwiftProtocol, Object?>.self)
34+
}
35+
do {
36+
let kp_property_objc = \ObjCProtocol.flag
37+
38+
var p: ObjCProtocol
39+
// expected-error@+1 {{cannot assign through subscript: 'kp_property_objc' is a read-only key path}}
40+
p[keyPath: kp_property_objc] = false
41+
42+
assertExactType(of: kp_property_objc, is: KeyPath<ObjCProtocol, Bool?>.self)
3243
}
3344

3445
// ...unless a reference-writable component shows up later.
3546
do {
36-
let kp_propertyForce_name = \P.object!.name
37-
let kp_subscriptForce_name = \P.[0]!.name
47+
let kp_propertyForce_name = \SwiftProtocol.object!.name
48+
let kp_subscriptForce_name = \SwiftProtocol.[true]!.name
3849

39-
let p: P
50+
let p: SwiftProtocol
4051
p[keyPath: kp_propertyForce_name] = "yes"
4152
p[keyPath: kp_subscriptForce_name] = "yes"
4253

43-
assertExactType(of: kp_propertyForce_name, is: ReferenceWritableKeyPath<P, String>.self)
44-
assertExactType(of: kp_subscriptForce_name, is: ReferenceWritableKeyPath<P, String>.self)
54+
assertExactType(of: kp_propertyForce_name,
55+
is: ReferenceWritableKeyPath<SwiftProtocol, String>.self)
56+
assertExactType(of: kp_subscriptForce_name,
57+
is: ReferenceWritableKeyPath<SwiftProtocol, String>.self)
4558
}

0 commit comments

Comments
 (0)