Quick / Nimble

A Matcher Framework for Swift and Objective-C
https://quick.github.io/Nimble/documentation/nimble/
Apache License 2.0
4.8k stars 601 forks source link

`beIdenticalTo` / `===`: disallow comparing non-objects #955

Closed NachoSoto closed 2 years ago

NachoSoto commented 2 years ago

Equality comparisons don't make sense for anything other than objects, and they will always fail. Using === with structs for example is likely a programmer error, so to avoid it, a compile-time error is better.

This is potentially a breaking change.

NachoSoto commented 2 years ago

Any thoughts on this @ikesyo? I was just bitten by it again comparing 2 structs with === instead of ==:

Screen Shot 2022-01-29 at 15 47 53
acecilia commented 2 years ago

Hi 👋 After this change I am experiencing that below code that previously worked fine, now it does not compile:

protocol MyProtocol { }

final class MyClass: MyProtocol {
    init() { }
}

final class Spec: QuickSpec {
    override func spec() {
        it("success") {
            let object1 = MyClass()
            let object2: MyProtocol = object1
            expect(object2).to(be(object1))
        }

        it("fails") {
            let object1 = MyClass()
            let object2: MyProtocol = MyClass()
            expect(object2).to(be(object1))
        }
    }
}

I understand that there are multiple ways to make these tests compile:

But I am not sure any of above solutions are correct:

I would consider above code snippet correct: it should build and run as expected without further modifications. Wdyt?

I believe the issue in this case boils down to this: the compiler is not able to tell at compile time that a type (in this case a protocol) is a class

NachoSoto commented 2 years ago

I believe the issue in this case boils down to this: the compiler is not able to tell at compile time that a type (in this case a protocol) is a class

That's exactly right. And without that, === can't really work.

acecilia commented 2 years ago

I believe the issue in this case boils down to this: the compiler is not able to tell at compile time that a type (in this case a protocol) is a class

That's exactly right. And without that, === can't really work.

=== used to work by casting and performing the check at runtime.

The change in this PR is based on an assumption that is false, no? The change modified the code so instead of making a run-time check, it performs a compile-time check, based on the assumption that all valid comparisons can be checked at compile-time

so to avoid it, a compile-time error is better

But we are saying that for some valid cases (the one above) the compiler does not know at compile time if a type is a class or not, so the current code does not allow valid comparisons that previously (with the run-time check) were allowed

acecilia commented 2 years ago

I would consider above code snippet correct: it should build and run as expected without further modifications. Wdyt?

@NachoSoto could you please reconsider this. I am considering opening a PR reverting the change here, but would like to hear if you have any alternative for the stated problem before doing so

NachoSoto commented 2 years ago

If you prefer to do this at runtime you could always add an overload for Any that casts it to AnyObject.

acecilia commented 2 years ago

If you prefer to do this at runtime

I do not prefer it, but protocol comparisons I believe cannot happen at compile time, as I shown above

you could always add an overload for Any that casts it to AnyObject.

@NachoSoto I cannot imagine how. Can you elaborate?

NachoSoto commented 2 years ago
extension Expectation where T == Any {
    public static func === (lhs: Expectation, rhs: Any?) {
        lhs.to(beIdenticalTo(rhs as? AnyObject))
    }
}
acecilia commented 2 years ago

Thanks for that. But above code does not build, throws the error Cannot convert value of type 'Predicate<AnyObject>' to expected argument type 'Predicate<Any>'