Skip to content

Commit 5f166be

Browse files
authored
Ensure the identifier of a "seen" object is only removed if the object's children were recursively reflected (#787)
This fixes a crash which can occur if an object has certain cyclic references and it's included in an expectation expression which fails. The bug is that the identifier of a "seen" object passed to `Expression.Value.init(_reflecting:label:seenObjects:)` should only be removed from the tracking dictionary if the object's children were recursively reflected. If recursion did not occur, that indicates that the object has been seen so it should be left in the tracking dictionary to prevent subsequent recursion. I added new unit tests to validate this. Fixes #785 ### Result: The scenarios described in #785 no longer crash. ### Checklist: - [x] Code and documentation should follow the style of the [Style Guide](https://github.com/apple/swift-testing/blob/main/Documentation/StyleGuide.md). - [x] If public symbols are renamed or modified, DocC references should be updated.
1 parent 405d8c9 commit 5f166be

File tree

2 files changed

+54
-7
lines changed

2 files changed

+54
-7
lines changed

Diff for: Sources/Testing/SourceAttribution/Expression.swift

+8-7
Original file line numberDiff line numberDiff line change
@@ -190,9 +190,9 @@ public struct __Expression: Sendable {
190190
/// This is used to halt further recursion if a previously-seen object
191191
/// is encountered again.
192192
private init(
193-
_reflecting subject: Any,
194-
label: String?,
195-
seenObjects: inout [ObjectIdentifier: AnyObject]
193+
_reflecting subject: Any,
194+
label: String?,
195+
seenObjects: inout [ObjectIdentifier: AnyObject]
196196
) {
197197
let mirror = Mirror(reflecting: subject)
198198

@@ -214,24 +214,25 @@ public struct __Expression: Sendable {
214214
// `type(of:)`, which is inexpensive. The object itself is stored as the
215215
// value in the dictionary to ensure it is retained for the duration of
216216
// the recursion.
217-
var objectIdentifierTeRemove: ObjectIdentifier?
217+
var objectIdentifierToRemove: ObjectIdentifier?
218218
var shouldIncludeChildren = true
219219
if mirror.displayStyle == .class, type(of: subject) is AnyObject.Type {
220220
let object = subject as AnyObject
221221
let objectIdentifier = ObjectIdentifier(object)
222222
let oldValue = seenObjects.updateValue(object, forKey: objectIdentifier)
223223
if oldValue != nil {
224224
shouldIncludeChildren = false
225+
} else {
226+
objectIdentifierToRemove = objectIdentifier
225227
}
226-
objectIdentifierTeRemove = objectIdentifier
227228
}
228229
defer {
229-
if let objectIdentifierTeRemove {
230+
if let objectIdentifierToRemove {
230231
// Remove the object from the set of previously-seen objects after
231232
// (potentially) recursing to reflect children. This is so that
232233
// repeated references to the same object are still included multiple
233234
// times; only _cyclic_ object references should be avoided.
234-
seenObjects[objectIdentifierTeRemove] = nil
235+
seenObjects[objectIdentifierToRemove] = nil
235236
}
236237
}
237238

Diff for: Tests/TestingTests/Expression.ValueTests.swift

+46
Original file line numberDiff line numberDiff line change
@@ -166,4 +166,50 @@ struct Expression_ValueTests {
166166
#expect(oneChildChildrenOptionalChild.children == nil)
167167
}
168168

169+
@Test("Value reflecting an object with two back-references to itself",
170+
.bug("https://github.com/swiftlang/swift-testing/issues/785#issuecomment-2440222995"))
171+
func multipleSelfReferences() {
172+
class A {
173+
weak var one: A?
174+
weak var two: A?
175+
}
176+
177+
let a = A()
178+
a.one = a
179+
a.two = a
180+
181+
let value = Expression.Value(reflecting: a)
182+
#expect(value.children?.count == 2)
183+
}
184+
185+
@Test("Value reflecting an object in a complex graph which includes back-references",
186+
.bug("https://github.com/swiftlang/swift-testing/issues/785"))
187+
func complexObjectGraphWithCyclicReferences() throws {
188+
class A {
189+
var c1: C!
190+
var c2: C!
191+
var b: B!
192+
}
193+
class B {
194+
weak var a: A!
195+
var c: C!
196+
}
197+
class C {
198+
weak var a: A!
199+
}
200+
201+
let a = A()
202+
let b = B()
203+
let c = C()
204+
a.c1 = c
205+
a.c2 = c
206+
a.b = b
207+
b.a = a
208+
b.c = c
209+
c.a = a
210+
211+
let value = Expression.Value(reflecting: a)
212+
#expect(value.children?.count == 3)
213+
}
214+
169215
}

0 commit comments

Comments
 (0)