Skip to content

Commit 6fd3974

Browse files
tbkkatbkka
committed
Fix a number of issues (review feedback)
* Fix how we were looking for common parent class * Use a constant hashValue for types that are Equatable but not Hashable
1 parent a840c69 commit 6fd3974

File tree

3 files changed

+182
-58
lines changed

3 files changed

+182
-58
lines changed

Diff for: ‎stdlib/public/runtime/SwiftObject.mm

+30-22
Original file line numberDiff line numberDiff line change
@@ -380,7 +380,12 @@ - (NSUInteger)hash {
380380
swift_conformsToProtocolCommon(
381381
selfMetadata, &hashable_support::HashableProtocolDescriptor));
382382
if (hashableConformance == NULL) {
383-
return (NSUInteger)self;
383+
// If a type is Equatable (but not Hashable), we
384+
// have to return something here that is compatible
385+
// with the `isEqual:` below. NSObject's default
386+
// of `(NSUInteger)self` won't work, so we instead
387+
// return a constant fallback value:
388+
return (NSUInteger)1;
384389
}
385390
return _swift_stdlib_Hashable_hashValue_indirect(
386391
&self, selfMetadata, hashableConformance);
@@ -390,36 +395,39 @@ - (BOOL)isEqual:(id)other {
390395
if (self == other) {
391396
return YES;
392397
}
393-
// Both objects must be `SwiftObject` in ObjC
394-
if (object_getClass(other) != object_getClass((id)self)) {
395-
return NO;
396-
}
397-
398-
// TODO: Bincompat check -- return NO if this is an old executable
399398

400399
// Get Swift type for self and other
401400
auto selfMetadata = _swift_getClassOfAllocated(self);
402401
auto otherMetadata = _swift_getClassOfAllocated(other);
403402

404-
// Find common parent Swift class
403+
// Find common parent class
405404
const ClassMetadata * parentMetadata = NULL;
406-
// Collect parents of self
407-
std::unordered_set<const ClassMetadata *> selfParents;
408-
for (auto m = selfMetadata; m != NULL; m = m->Superclass) {
409-
selfParents.emplace(m);
410-
}
411-
// Find first parent of other that is also a parent of self
412-
for (auto m = otherMetadata; m != NULL; m = m->Superclass) {
413-
if (selfParents.find(m) != selfParents.end()) {
414-
parentMetadata = m;
415-
break;
405+
if (selfMetadata == otherMetadata) {
406+
parentMetadata = selfMetadata;
407+
} else {
408+
// Collect parents of self
409+
std::unordered_set<const ClassMetadata *> selfAncestors;
410+
for (auto m = selfMetadata; m != NULL; m = m->Superclass) {
411+
selfAncestors.emplace(m);
412+
}
413+
// Find first parent of other that is also a parent of self
414+
for (auto m = otherMetadata; m != NULL; m = m->Superclass) {
415+
if (selfAncestors.find(m) != selfAncestors.end()) {
416+
parentMetadata = m;
417+
break;
418+
}
419+
}
420+
// If there's no common parent class, we can't compare them
421+
if (parentMetadata == NULL) {
422+
return NO;
416423
}
417424
}
418-
// If there's no common Swift parent class, we can't compare them
419-
if (parentMetadata == NULL) {
420-
return NO;
421-
}
425+
// Note: Because `self` subclasses SwiftObject, we know parentMetadata
426+
// must be a Swift type or SwiftObject itself.
422427

428+
// This will work for types that implement Hashable or only Equatable.
429+
// In the latter case, there is a risk that `-hash` and `-isEqual:` might
430+
// be incompatible. See notes above for `-hash`
423431
auto equatableConformance =
424432
reinterpret_cast<const equatable_support::EquatableWitnessTable *>(
425433
swift_conformsToProtocolCommon(

Diff for: ‎test/stdlib/Inputs/SwiftObjectNSObject/SwiftObjectNSObject.m

+37-23
Original file line numberDiff line numberDiff line change
@@ -80,10 +80,43 @@ void HackSwiftObject()
8080
class_addMethod(cls, @selector(perform2::), (IMP)Perform2, "@@:@@");
8181
}
8282

83-
void TestSwiftObjectNSObject(id c, id d,
84-
id e1a, id e1b, id e2,
85-
id h1a, id h1b, id h2,
86-
NSUInteger hash1, NSUInteger hash2)
83+
void TestSwiftObjectNSObjectAssertNoErrors(void)
84+
{
85+
printf("\nTotal: %d error%s\n",
86+
Errors, Errors == 1 ? "" : "s");
87+
if (Errors > 0) {
88+
exit(1);
89+
}
90+
}
91+
92+
93+
void TestSwiftObjectNSObjectEquals(id e1, id e2)
94+
{
95+
printf("NSObjectProtocol.isEqual: Expect %s == %s\n",
96+
[[e1 description] UTF8String],
97+
[[e2 description] UTF8String]);
98+
expectTrue([e1 isEqual:e2]);
99+
expectTrue([e2 isEqual:e1]);
100+
}
101+
102+
void TestSwiftObjectNSObjectNotEquals(id e1, id e2)
103+
{
104+
printf("NSObjectProtocol.isEqual: Expect %s != %s\n",
105+
[[e1 description] UTF8String],
106+
[[e2 description] UTF8String]);
107+
expectFalse([e1 isEqual:e2]);
108+
expectFalse([e2 isEqual:e1]);
109+
}
110+
111+
void TestSwiftObjectNSObjectHashValue(id e, NSUInteger hashValue)
112+
{
113+
printf("NSObjectProtocol.hash: Expect [%s hashValue] == %lu\n",
114+
[[e description] UTF8String],
115+
(unsigned long)hashValue);
116+
expectTrue([e hash] == hashValue);
117+
}
118+
119+
void TestSwiftObjectNSObject(id c, id d)
87120
{
88121
printf("TestSwiftObjectNSObject\n");
89122

@@ -162,25 +195,11 @@ void TestSwiftObjectNSObject(id c, id d,
162195
expectFalse([C_meta isEqual:D_meta]);
163196
expectFalse([S_meta isEqual:C_meta]);
164197

165-
// Check that ObjC isEqual delegates to Equatable
166-
expectTrue([e1a isEqual: e1b]);
167-
expectFalse([e1a isEqual: e2]);
168-
expectFalse([e1b isEqual: e2]);
169-
170-
// Check that ObjC isEqual delegates to Hashable
171-
expectTrue([h1a isEqual: h1b]);
172-
expectFalse([h1a isEqual: h2]);
173-
expectFalse([h1b isEqual: h2]);
174-
175198
printf("NSObjectProtocol.hash\n");
176199

177200
expectTrue ([d hash] + [c hash] + [D hash] + [C hash] + [S hash] +
178201
[D_meta hash] + [C_meta hash] + [S_meta hash] != 0);
179202

180-
expectTrue([h1a hash] == hash1);
181-
expectTrue([h1b hash] == hash1);
182-
expectTrue([h2 hash] == hash2);
183-
184203
printf("NSObjectProtocol.self\n");
185204

186205
expectTrue ([d self] == d);
@@ -813,9 +832,4 @@ void TestSwiftObjectNSObject(id c, id d,
813832
expectTrue ([S_meta instanceMethodForSelector:@selector(DESSLOK)] == fwd);
814833
expectTrue ([C_meta instanceMethodForSelector:@selector(DESSLOK)] == fwd);
815834
expectTrue ([D_meta instanceMethodForSelector:@selector(DESSLOK)] == fwd);
816-
817-
818-
printf("TestSwiftObjectNSObject: %d error%s\n",
819-
Errors, Errors == 1 ? "" : "s");
820-
exit(Errors ? 1 : 0);
821835
}

Diff for: ‎test/stdlib/SwiftObjectNSObject.swift

+115-13
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
// RUN: %empty-directory(%t)
1414
//
1515
// RUN: %target-clang %S/Inputs/SwiftObjectNSObject/SwiftObjectNSObject.m -c -o %t/SwiftObjectNSObject.o -g
16-
// RUN: %target-build-swift %s -I %S/Inputs/SwiftObjectNSObject/ -Xlinker %t/SwiftObjectNSObject.o -o %t/SwiftObjectNSObject
16+
// RUN: %target-build-swift %s -g -I %S/Inputs/SwiftObjectNSObject/ -Xlinker %t/SwiftObjectNSObject.o -o %t/SwiftObjectNSObject
1717
// RUN: %target-codesign %t/SwiftObjectNSObject
1818
// RUN: %target-run %t/SwiftObjectNSObject 2> %t/log.txt
1919
// RUN: %FileCheck %s < %t/log.txt
@@ -39,24 +39,75 @@ class D : C {
3939
@objc override class func cClassOverride() -> Int { return 8 }
4040
}
4141

42-
class E : Equatable {
42+
class E : Equatable, CustomStringConvertible {
4343
var i : Int
4444
static func ==(lhs: E, rhs: E) -> Bool { lhs.i == rhs.i }
4545
init(i: Int) { self.i = i }
46+
var description: String { "\(type(of:self))(i:\(self.i))" }
4647
}
4748

48-
class H : Hashable {
49+
class E1: E {
50+
}
51+
52+
class E2: E {
53+
}
54+
55+
class F : CustomStringConvertible {
4956
var i : Int
57+
init(i: Int) { self.i = i }
58+
var description: String { "\(type(of:self))(i:\(self.i))" }
59+
}
60+
61+
class F1: F, Equatable {
62+
static func ==(lhs: F1, rhs: F1) -> Bool { lhs.i == rhs.i }
63+
}
64+
65+
class F2: F, Equatable {
66+
static func ==(lhs: F2, rhs: F2) -> Bool { lhs.i == rhs.i }
67+
}
68+
69+
class H : E, Hashable {
5070
static func ==(lhs: H, rhs: H) -> Bool { lhs.i == rhs.i }
5171
func hash(into hasher: inout Hasher) { hasher.combine(i + 17) }
52-
init(i: Int) { self.i = i }
5372
}
5473

55-
@_silgen_name("TestSwiftObjectNSObject")
56-
func TestSwiftObjectNSObject(
57-
_ c: C, _ d: D,
58-
_ e1a: E, _ e1b: E, _ e2: E,
59-
_ h1a: H, _ h1b: H, _ h2: H, _ hash1: UInt, _ hash2: UInt)
74+
@_silgen_name("TestSwiftObjectNSObject")
75+
func TestSwiftObjectNSObject(_ c: C, _ d: D)
76+
@_silgen_name("TestSwiftObjectNSObjectEquals")
77+
func TestSwiftObjectNSObjectEquals(_: AnyObject, _: AnyObject)
78+
@_silgen_name("TestSwiftObjectNSObjectNotEquals")
79+
func TestSwiftObjectNSObjectNotEquals(_: AnyObject, _: AnyObject)
80+
@_silgen_name("TestSwiftObjectNSObjectHashValue")
81+
func TestSwiftObjectNSObjectHashValue(_: AnyObject, _: Int)
82+
@_silgen_name("TestSwiftObjectNSObjectAssertNoErrors")
83+
func TestSwiftObjectNSObjectAssertNoErrors()
84+
85+
// Verify that Obj-C isEqual: provides same answer as Swift ==
86+
func TestEquatableEquals<T: Equatable & AnyObject>(_ e1: T, _ e2: T) {
87+
if e1 == e2 {
88+
TestSwiftObjectNSObjectEquals(e1, e2)
89+
} else {
90+
TestSwiftObjectNSObjectNotEquals(e1, e2)
91+
}
92+
}
93+
94+
func TestNonEquatableEquals(_ e1: AnyObject, _ e2: AnyObject) {
95+
TestSwiftObjectNSObjectNotEquals(e1, e2)
96+
}
97+
98+
// Verify that Obj-C hashValue matches Swift hashValue for Hashable types
99+
func TestHashable(_ h: H)
100+
{
101+
TestSwiftObjectNSObjectHashValue(h, h.hashValue)
102+
}
103+
104+
// Test Obj-C hashValue for Swift types that are not Hashable
105+
func TestNonHashableHash(_ e: AnyObject)
106+
{
107+
// Non-Hashable type should always have the
108+
// same hash value in Obj-C
109+
TestSwiftObjectNSObjectHashValue(e, 1)
110+
}
60111
61112
// This check is for NSLog() output from TestSwiftObjectNSObject().
62113
// CHECK: c ##SwiftObjectNSObject.C##
@@ -66,10 +117,61 @@ func TestSwiftObjectNSObject(
66117
// Temporarily disable this test on older OSes until we have time to
67118
// look into why it's failing there. rdar://problem/47870743
68119
if #available(OSX 10.12, iOS 10.0, *) {
69-
TestSwiftObjectNSObject(C(), D(),
70-
E(i:1), E(i:1), E(i:2),
71-
H(i:1), H(i:1), H(i:2), UInt(H(i:1).hashValue), UInt(H(i:2).hashValue))
72-
// does not return
120+
// Test a large number of Obj-C APIs
121+
TestSwiftObjectNSObject(C(), D())
122+
123+
// ** Equatable types with an Equatable parent class
124+
// Same type and class
125+
TestEquatableEquals(E(i: 1), E(i: 1))
126+
TestEquatableEquals(E(i: 790), E(i: 790))
127+
TestEquatableEquals(E1(i: 1), E1(i: 1))
128+
TestEquatableEquals(E1(i: 18), E1(i: 18))
129+
TestEquatableEquals(E2(i: 1), E2(i: 1))
130+
TestEquatableEquals(E2(i: 2), E2(i: 2))
131+
// Different class
132+
TestEquatableEquals(E1(i: 1), E2(i: 1))
133+
TestEquatableEquals(E1(i: 1), E(i: 1))
134+
TestEquatableEquals(E2(i: 1), E(i: 1))
135+
// Different value
136+
TestEquatableEquals(E(i: 1), E(i: 2))
137+
TestEquatableEquals(E1(i: 1), E1(i: 2))
138+
TestEquatableEquals(E2(i: 1), E2(i: 2))
139+
140+
// ** Non-Equatable parent class
141+
// Same class and value
142+
TestEquatableEquals(F1(i: 1), F1(i: 1))
143+
TestEquatableEquals(F1(i: 1), F1(i: 2))
144+
TestEquatableEquals(F2(i: 1), F2(i: 1))
145+
TestEquatableEquals(F2(i: 1), F2(i: 2))
146+
147+
// Different class and/or value
148+
TestNonEquatableEquals(F(i: 1), F(i: 2))
149+
TestNonEquatableEquals(F(i: 1), F(i: 1))
150+
TestNonEquatableEquals(F1(i: 1), F2(i: 1))
151+
TestNonEquatableEquals(F1(i: 1), F(i: 1))
152+
153+
// Two equatable types with no common parent class
154+
TestNonEquatableEquals(F1(i: 1), E(i: 1))
155+
TestEquatableEquals(H(i:1), E(i:1))
156+
157+
// Non-Hashable: alway have the same Obj-C hashValue
158+
TestNonHashableHash(E(i: 1))
159+
TestNonHashableHash(E1(i: 3))
160+
TestNonHashableHash(E2(i: 8))
161+
TestNonHashableHash(C())
162+
TestNonHashableHash(D())
163+
164+
// Hashable types are also Equatable
165+
TestEquatableEquals(H(i:1), H(i:1))
166+
TestEquatableEquals(H(i:1), H(i:2))
167+
TestEquatableEquals(H(i:2), H(i:1))
168+
169+
// Verify Obj-C hash value agrees with Swift
170+
TestHashable(H(i:1))
171+
TestHashable(H(i:2))
172+
TestHashable(H(i:18))
173+
174+
TestSwiftObjectNSObjectAssertNoErrors()
73175
} else {
74176
// Horrible hack to satisfy FileCheck
75177
fputs("c ##SwiftObjectNSObject.C##\n", stderr)

0 commit comments

Comments
 (0)