Skip to content

Ensure the identifier of a "seen" object is only removed if the object's children were recursively reflected #787

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions Sources/Testing/SourceAttribution/Expression.swift
Original file line number Diff line number Diff line change
Expand Up @@ -190,9 +190,9 @@ public struct __Expression: Sendable {
/// This is used to halt further recursion if a previously-seen object
/// is encountered again.
private init(
_reflecting subject: Any,
label: String?,
seenObjects: inout [ObjectIdentifier: AnyObject]
_reflecting subject: Any,
label: String?,
seenObjects: inout [ObjectIdentifier: AnyObject]
) {
let mirror = Mirror(reflecting: subject)

Expand All @@ -214,24 +214,25 @@ public struct __Expression: Sendable {
// `type(of:)`, which is inexpensive. The object itself is stored as the
// value in the dictionary to ensure it is retained for the duration of
// the recursion.
var objectIdentifierTeRemove: ObjectIdentifier?
var objectIdentifierToRemove: ObjectIdentifier?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive by typo and style fixes here and above

var shouldIncludeChildren = true
if mirror.displayStyle == .class, type(of: subject) is AnyObject.Type {
let object = subject as AnyObject
let objectIdentifier = ObjectIdentifier(object)
let oldValue = seenObjects.updateValue(object, forKey: objectIdentifier)
if oldValue != nil {
shouldIncludeChildren = false
} else {
objectIdentifierToRemove = objectIdentifier
}
objectIdentifierTeRemove = objectIdentifier
}
defer {
if let objectIdentifierTeRemove {
if let objectIdentifierToRemove {
// Remove the object from the set of previously-seen objects after
// (potentially) recursing to reflect children. This is so that
// repeated references to the same object are still included multiple
// times; only _cyclic_ object references should be avoided.
seenObjects[objectIdentifierTeRemove] = nil
seenObjects[objectIdentifierToRemove] = nil
}
}

Expand Down
46 changes: 46 additions & 0 deletions Tests/TestingTests/Expression.ValueTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -166,4 +166,50 @@ struct Expression_ValueTests {
#expect(oneChildChildrenOptionalChild.children == nil)
}

@Test("Value reflecting an object with two back-references to itself",
.bug("https://github.com/swiftlang/swift-testing/issues/785#issuecomment-2440222995"))
func multipleSelfReferences() {
class A {
weak var one: A?
weak var two: A?
}

let a = A()
a.one = a
a.two = a

let value = Expression.Value(reflecting: a)
#expect(value.children?.count == 2)
}

@Test("Value reflecting an object in a complex graph which includes back-references",
.bug("https://github.com/swiftlang/swift-testing/issues/785"))
func complexObjectGraphWithCyclicReferences() throws {
class A {
var c1: C!
var c2: C!
var b: B!
}
class B {
weak var a: A!
var c: C!
}
class C {
weak var a: A!
}

let a = A()
let b = B()
let c = C()
a.c1 = c
a.c2 = c
a.b = b
b.a = a
b.c = c
c.a = a

let value = Expression.Value(reflecting: a)
#expect(value.children?.count == 3)
}

}