1151567 - Make optional arrays and dictionaries Equatable

Status: RESOLVED FIXED

(Firefox for iOS :: General, defect)

Description

[Richard Newman [:rnewman]]

Swift makes *some* optional fields Equatable:

------
struct Y: Equatable {
    var foo: String?
}

func ==(lhs: Y, rhs: Y) -> Bool {
    return lhs.foo == rhs.foo
}

Y(foo: nil) == Y(foo: nil)       // -> true
Y(foo: nil) == Y(foo: "Bar")     // -> false
------

but not others:

------
struct X: Equatable {
    var foo: [String]?
}

func ==(lhs: X, rhs: X) -> Bool {
    return lhs.foo == rhs.foo      // Value of optional type '[String]}' not unwrapped…
}
------


I propose adding:

------
public func ==<T>(lhs: T?, rhs: T?) -> Bool {
    switch (lhs, rhs) {
    case (.None, .None):
        return true
    case (.None, _):
        return false
    case (_, .None):
        return false
    default:
        return lhs! == rhs!
    }
}
------


which simply makes nil == nil, nil != _, _ != nil, then delegates to the non-optional operator.

You might ask "why isn't that <T: Equatable>?". Good question! Apparently random other code breaks if I do so.

Comment 1

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I don't think this is a good idea because it changes something fundamental of how the language/stdlib works. Voting against this.

Comment 2

[Richard Newman [:rnewman]]

So what about:

func optArrayEq<T: Equatable>(lhs: [T]?, rhs: [T]?) -> Bool {
    switch (lhs, rhs) {
    case (.None, .None):
        return true
    case (.None, _):
        return false
    case (_, .None):
        return false
    default:
        return lhs! == rhs!
    }
}

This uses Swift's own array == by unwrapping the optional, but doesn't interact with Equatable.

Comment 3

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

This blows up the playground, looks like it is being called recursively:

// Causes the == below to be called recursively

let a: [Int]? = [1,2]
let b: [Int]? = [1,2]

public func ==<T>(lhs: T?, rhs: T?) -> Bool {
    println("Calling!")
    switch (lhs, rhs) {
    case (.None, .None):
        return true
    case (.None, _):
        return false
    case (_, .None):
        return false
    default:
        return lhs! == rhs!
    }
}

a == b

Comment 4

[Richard Newman [:rnewman]]

I accidentally cherry-picked this onto master:

https://github.com/mozilla/firefox-ios/commit/3f6d07c6ace8065c8955845c85c6ed1d36e6e0e1

I think this doesn't diverge from overall consensus on IRC, but scream if you disagree.

Comment 5

[Richard Newman [:rnewman]]

Wes mentioned he was fine with it, but I'll leave the ni for Stefan just in case.

Comment 6

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Looks fine do it.