Hasahble's hashvalue is not promised to be unique and should not be used as a key in the registrations dictionary

[jrogccom]

Resolver's registration dictionary uses ObjectIdentifier's hashValue as its keys, but hash values shouldn't be assumed to be unique. There's a risk of overwriting a value in the registrations dictionary.

Note that for two arbitrary types SomeService and SomeOtherService,

ObjectIdentifier(SomeService.self) != ObjectIdentifier(SomeOtherService.self) // true

but

ObjectIdentifier(SomeService.self).hashValue != ObjectIdentifier(SomeOtherService.self).hashValue // may be false

The issue would manifest when registering services in the snippets below:

public final class Resolver {
    @discardableResult
    public final func register<Service>(_ type: Service.Type = Service.self, name: Resolver.Name? = nil,
                                        factory: @escaping ResolverFactory<Service>) -> ResolverOptions<Service> {
        lock.lock()
        defer { lock.unlock() }
        let key = ObjectIdentifier(Service.self).hashValue  **<- extracted key is another Hashable's hashvalue; not unique**
        let registration = ResolverRegistrationOnly(resolver: self, key: key, name: name, factory: factory)
        add(registration: registration, with: key, name: name)
        return registration
    }

    ...
    private var registrations = [Int : [String : Any]]() *<- subject to overwriting values where hash values from upstream ObjectIdentifiers are the same*
}

Since ObjectIdentifier itself is Hashable, I'd propose using ObjectIdentifier itself as the key to the registrations dictionary, and not hashValues extracted from ObjectIdentfier objects.

[jrogccom]

Conceding that a key-value overwrite is highly unlikely here, the main point I'm trying to illustrate is that ObjectIdentifier is already a fine key, and we don't have to derive a second key (hashValue) from it.

[jrogccom]

Talking through it, I became convinced this is not worth addressing and closed. Apple's documentation doesn't comment on the uniqueness of the hashvalue, and it must inevitably not be unique, but it doesn't seem likely you'll ever overwrite a key because you got the same hashValue from two different types. Using ObjectIdentifier itself as a key would make it unambiguously impossible, but it doesn't seem like a cause for concern.

[hmlongco]

Interesting. IIRC, at one point in time the hashValue for ObjectIdentifier was the wrapped raw pointer address itself and Resolver depended on that.

But it seems that Apple "improved" that when they created the new hashable / hash(into) scheme, and generating a hash value from an ObjectIdentifier's _value now generates a different number. Which, as you point out, is not guaranteed to be unique. Sigh.

While it's probably not a problem, I'm not a huge fan of probably. Most code would never experience threading problems either, but some do, and that's why there are recursive locks in place.

Looking through the implementation of ObjectIdentifier, however, I noticed that there are some overrides to Int and Uint that take an ObjectIdentifier and simply export the raw address as an Uint/Int, almost exactly what hashValue used to do.

As such, the next release of Resolver might switch from using hashValue to Int(bitPattern: ObjectIdentifier(Service.self)), which would be guaranteed to be an unique value.

If you look at the generated code, an Int value has a few advantages when passing it as a parameter over a struct, even though both equate to the same size of 8 bytes.

[hmlongco]

So... just did a performance measurement and using the raw Int is about 16% (!!!) faster than using ObjectIdentifier itself as a registration table key.

The Uint change is in the master branch preparatory to doing a 1.4.4 pod release.

[jrogccom]

Wow! A lot faster, then. I think I may have raised this issue prematurely. Strong case for not using ObjectIdentifier as a key, anyway.

[hmlongco]

If you trace the straight-line path through resolution of a registration, you'll find that there actually isn't that much code involved. As such, small discrepancies can really add up. That said, you have to cycle the tests about 10,000 times to get measurable results... ;)

[hmlongco]

1.4.4 released