Iglistkit: NSObject equality naming conflict

[iwasrobbed]

Issue

Since this project extends NSObject to make it conform to IGListDiffable and that protocol has a method with a similar isEqual signature as the standard NSObject one, it is creating naming conflicts with other, completely unrelated, objects in the project that use the vanilla isEqual comparison.

You should consider uniquely naming the IGListDiffable method, e.g.:

- (BOOL)isEqualToDiffableObject:(nullable id<IGListDiffable>)object;

I looked into opening a PR for this, but honestly there are too many vague isEqual calls in the codebase so I'm not sure which should be the diffable equality check and which should be the vanilla NSObject ones.

New issue checklist

• [x] I have reviewed the README and documentation

• [x] I have searched existing issues and this is not a duplicate

  General information

• Library version(s): 1.0.0

• iOS version(s):
• Devices/Simulators affected:
• Reproducible in the demo project? (Yes/No): N/A
• Related issues:

[jessesquires]

Hey @iwasrobbed ! 😄

It looks like your error is with override. Removing this should fix that. However, isEqual: changed in Swift 3. It should be isEqual(object: Any?) -> Bool. (Note: not AnyObject.) Are you using Swift 2.3 or 3.0? (We're only supporting 3.0)

[iwasrobbed]

This is Swift 2.3 code, but the same issue would possibly exist in a Swift 3.0 project unless they made compiler fixes for these sorts of ambiguity.

This is actually existing code, not something I added after adding IGListKit, and the class it's on inherits from NSObject so it's technically an override. The class in question has nothing to do with IGListKit whatsoever which strikes me as side effects which you wouldn't want to have in people's projects.

Removing that just gives another error Method 'isEqual' with Objective-C selector 'isEqual:' conflicts with method 'isEqual' from superclass 'NSObject' with the same Objective-C selector

[jessesquires]

Removing that just gives another error Method 'isEqual' with Objective-C selector 'isEqual:' conflicts with method 'isEqual' from superclass 'NSObject' with the same Objective-C selector

Can you annotate the func with @nonobjc ?

[jessesquires]

Oh — and if you can fork and reproduce this in the demo project, that would be great! 👍

[iwasrobbed]

re: @nonobjc: Ultimately, the compiler is not able to find the vanilla isEqual method since it's being overwritten by the diffable version, so the annotation doesn't help unfortunately (i.e. the vanilla isEqual never gets called w/ the annotation).

Looks to be fixed in Swift 3.0 since it disambiguates the two equality functions.

Feel free to close, but I imagine a lot of people will be stuck on Swift 2.3 for a bit longer. At the very least, may be worth specifying this via a .swift-version file and README.

[rnystrom]

This was something that @ryanolsonk and I discussed a while back. We landed on overriding isEqual: for simplicity's sake. Also we had a bunch of models that had custom equality methods, so the change was easy.

The other problem that we have with -isEqual: is that it basically requires implementing -hash. We could:

• Change the protocol to the method -isEqualToDiffableObject: or something like that
• Have the NSObject+IGListDiffable category just return [super isEqual:object]; so everything "just works"

This would obviously be a breaking change, but its early so we can afford it.

[chrismcs]

It looks like you closed this in favor of just saying that it requires Swift 3. However, this creates problems for us in Objective-C. We useisEqual: to test what you call diffIdentifier. This made it handy for us to use common NSArray methods (like containsObject:, removeObject:, indexOfObject:, etc) - such that when we got a new array of updated objects, we could simply call removeObject: on the old array which meant "remove the thing that has the same uniqueID as this object". Arguably, this was perhaps a misuse of isEqual: on our part. But given how pervasively isEqual: is used by collection objects and such, I can't imagine we're the only ones who will bump into an issue like this.

Would it be a simple enough solution to add an optional method to the IGListDiffable protocol along the lines of isEqualToDiffableObject: and then add a 3rd option to IGListDiffOption (perhaps called something like IGListDiffCustomEquality) and when you use that option, you use isEqualToDiffableObject: instead of isEqual:?

[Sherlouk]

Across the same lines I'm currently using it in a few Swift 2.3 projects, and I'm constantly getting bit in the ass by the fact CoreData refuses to let you override isEqual: and based on the way my processes work (and have done for ages) that one will return false even if they're the same object.

This issue may still be in Swift 3 as well, not been able to test it.

[rnystrom]

@chrismcs this isn't closed yet (just ref'd from a closed PR).

Ya there are definitely issues w/ this when used w/ ObjC, esp if you aren't careful (see my note above about -hash). It's pretty easy to break things and the compiler wont have your back.

I think I'm set on changing the protocol to -isEqualToDiffableObject:. Been meaning to grab @ryanolsonk's opinion on this b/c he was influential in shaping IGListDiffable.

[jessesquires]

@rnystrom +1 for isEqualToDiffableObject:

[rnystrom]

Working on this, got a question for everyone. I want to keep the default NSObject<IGListDiffable> category.

We also include this category in the umbrella header. When I changed the protocol method to isEqualToDiffableObject: everything works, but things will _silently break_ if you had custom isEqual: and #import <IGListKit/IGListKit.h> (in Swift bridging header or ObjC). Kind of a bummer.

Chalk this up to growing pains?

One idea is to build a "bridging" function that checks if objects conform to IGListDiffable and _also_ have custom isEqual: implementations? Sort of saying "hey you probably didn't migrate".

Not sure its worth the effort.

[jessesquires]

Chalk this up to growing pains?

Yeah, I would. This is why we have a changelog + release notes. 😄

Not sure its worth the effort.

Definitely not, IMO.

---

Another option: don't include that category header in the umbrella header. Force clients to import that manually if they want it.