swiftlang / swift-corelibs-xctest

The XCTest Project, A Swift core library for providing unit test support
swift.org
Apache License 2.0
1.15k stars 267 forks source link

XCTAssertEqual and XCTAssertNotEqual don’t use the correct equality function for a subclass #477

Closed grantneufeld closed 8 months ago

grantneufeld commented 8 months ago

When comparing two instances of a subclass, if both the subclass and its parent class conform to Equatable with both having their own equality operator function (static func ==), XCTAssertEqual and XCTAssertNotEqual will use the superclass’ function rather than the subclass function.

Xcode: 15.2 macOS: 14.3.1

Sample test file showing the error:

import XCTest

// `XCTAssertEqual` and `XCTAssertNotEqual` ignore the `==` operator on the
// class of the objects being compared in favor of the `==` operator from the
// superclass.

final class BrokenEqualityAssertionTests: XCTestCase {
    private class SuperExperiment: Equatable {
        var superValue = ""

        static func == (lhs: SuperExperiment, rhs: SuperExperiment) -> Bool {
            lhs.superValue == rhs.superValue
        }
    }

    private class SubExperiment: SuperExperiment, Equatable {
        var value: Int = 0

        static func == (lhs: SubExperiment, rhs: SubExperiment) -> Bool {
            lhs.value == rhs.value
        }
    }

    // 👍 Correct results:
    func test_inheritedEquality_whenSameValues() throws {
        let one = SubExperiment()
        let two = SubExperiment()
        let same = one == two
        XCTAssertTrue(same) // 1: ✅
        XCTAssertEqual(one, two) // 2: ✅
        XCTAssertNotEqual(one, two) // 3: ❌ (👍) Expected
    }

    // 🚫 Wrong results:
    func test_inheritedEquality_whenNotEqual() throws {
        let one = SubExperiment()
        one.value = 1
        let two = SubExperiment()
        two.value = 2
        let same = one == two
        XCTAssertFalse(same) // 4: ✅
        XCTAssertEqual(one, two) // 5: ✅ (🚫) Should fail instead
        XCTAssertNotEqual(one, two) // 6: ❌ (🚫)  Should pass instead
    }

    // 🚫 Wrong results:
    func test_inheritedEquality_whenSameValuesButParentNotSame() throws {
        let one = SubExperiment()
        one.superValue = "one"
        one.value = 0
        let two = SubExperiment()
        two.superValue = "two"
        two.value = 0
        let same = one == two
        XCTAssertTrue(same) // 7: ✅
        XCTAssertNotEqual(one, two) // 8: ✅ (🚫)  Should fail instead
        XCTAssertEqual(one, two) // 9: ❌ (🚫)  Should pass instead
    }
}
grynspan commented 8 months ago

FYI your example code does not compile for me as I get the diagnostic:

🛑 Redundant conformance of 'BrokenEqualityAssertionTests.SubExperiment' to protocol 'Equatable'

XCTAssertEqual() simply calls == (well, !=, but that calls through to == for types conforming to Equatable.)

This is a general constraint of protocol conformances on classes in Swift: the superclass is the type that conforms, and subclasses are not able to override the superclass' conformance when it involves a reference to Self, because the override's type (Self) won't match.

What's happening here is that the == operator implemented in the subclass is valid, in isolation, independent of Equatable conformance, and when you explicitly write one == two the compiler type-checks the expression and says "==(lhs: SubExperiment, rhs: SubExperiment) is the best match among overloads of ==".

On the other hand, when you call XCTAssertEqual(), it is relying on Equatable conformance and, as a generic implementation, cannot see the overload of == in the subclass. It can only see the overload of == that satisfies the parent class' Equatable conformance, i.e. ==(lhs: SuperExperiment, rhs: SuperExperiment).

The solution depends on your real-world code: if these classes are actually subclasses of NSObject, then override isEqual(_:) instead of ==. If these classes are not subclasses of NSObject, then you can emulate what NSObject does by adding an open member to the base class and having == call it. However, be aware that when dealing with classes and inheritance, it can be difficult to ensure that the symmetric property of == is preserved if the base class is not aware of all possible subclasses.

grantneufeld commented 8 months ago

Oh, well. Thanks for looking into it. In the end, I gave up on using inheritance and reworked the whole thing with protocols instead. 🤷