Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add NSObjectPreferIsEqualRule #2663

Merged
merged 5 commits into from Mar 4, 2019

Conversation

Projects
None yet
3 participants
@matthew-healy
Copy link
Contributor

commented Mar 1, 2019

After accidentally mis-implementing == on an NSObject subclass one too many times, I decided to implement this rule. I also filled out the Rule Request info below, to explain why I think it's necessary.

Rule Request

  1. Why should this rule be added?

When subclassing NSObject in Swift, properly implementing Equatable can be a little tricky. Whereas with most Swift types the correct thing to do is implement ==, NSObject already exposes an implementation of == that calls out to isEqual. Adding a new implementation of == to an NSObject subclass can lead to odd situations where a == b is true, but (a as NSObject) == b (or, equivalently a.isEqual(b)) is false.

This rule is triggered whenever an @objc class or a direct NSObject subclass implements a static == function with exactly 2 arguments, both of which are of the same type as the class itself.

  1. Provide several examples of what would and wouldn't trigger violations.

See the triggering & non-triggering examples in NSObjectPreferIsEqualRuleExamples.

  1. Should the rule be configurable, if so what parameters should be configurable?

I included a SeverityConfiguration, because it seemed reasonable that folk might want to be aware of this violation without fixing it immediately.

  1. Should the rule be opt-in or enabled by default? Why?

I'm not entirely sure. I implemented it as an OptInRule because it seemed safer, but I also can't think of a particularly good reason why you wouldn't want this to run, given the relative risks of differing notions of equality existing on the same type depending on the context. Happy to either leave as-is or enable by default depending on what others think.

@SwiftLintBot

This comment has been minimized.

Copy link

commented Mar 1, 2019

12 Messages
📖 Linting Aerial with this PR took 2.01s vs 1.92s on master (4% slower)
📖 Linting Alamofire with this PR took 4.49s vs 3.95s on master (13% slower)
📖 Linting Firefox with this PR took 14.31s vs 12.47s on master (14% slower)
📖 Linting Kickstarter with this PR took 23.98s vs 20.17s on master (18% slower)
📖 Linting Moya with this PR took 2.23s vs 1.86s on master (19% slower)
📖 Linting Nimble with this PR took 2.17s vs 1.71s on master (26% slower)
📖 Linting Quick with this PR took 0.68s vs 0.55s on master (23% slower)
📖 Linting Realm with this PR took 3.74s vs 3.37s on master (10% slower)
📖 Linting SourceKitten with this PR took 1.22s vs 1.12s on master (8% slower)
📖 Linting Sourcery with this PR took 4.1s vs 4.01s on master (2% slower)
📖 Linting Swift with this PR took 27.24s vs 26.94s on master (1% slower)
📖 Linting WordPress with this PR took 21.21s vs 20.93s on master (1% slower)

Generated by 🚫 Danger


static let triggeringExamples: [String] = [
// NSObject subclass implementing ==
"""

This comment has been minimized.

Copy link
@marcelofabri

marcelofabri Mar 1, 2019

Collaborator

you should use the violation marker () to test where the violation location

* None.
* Add `nsobject_prefer_isequal` rule to warn against implementing `==` on an
`NSObject` subclass as calling `isEqual` (i.e. when using the class from
Objective-C) will will not use the defined `==` method.

This comment has been minimized.

Copy link
@marcelofabri

marcelofabri Mar 1, 2019

Collaborator

requires two trailing spaces as described in CONTRIBUTING.md: https://github.com/realm/SwiftLint/blob/master/CONTRIBUTING.md#tracking-changes

SwiftDeclarationKind(rawValue: kind) == .class
else { return false }
let isDirectNSObjectSubclass = dictionary.inheritedTypes.contains("NSObject")
let isMarkedObjc = dictionary.enclosedSwiftAttributes.contains(where: { $0 == .objc })

This comment has been minimized.

Copy link
@marcelofabri

marcelofabri Mar 1, 2019

Collaborator

nitpick: you can use .contains(.objc)

let kind = method.kind.flatMap(SwiftDeclarationKind.init),
let name = method.name,
kind == .functionMethodStatic,
name.hasPrefix("=="),

This comment has been minimized.

Copy link
@marcelofabri

marcelofabri Mar 1, 2019

Collaborator

maybe check if it's equal to ==(_:_:)? I'm a bit worried that using just == might cause some (very rare) false positives on custom operators.

private func areAllArguments(toMethod method: [String: SourceKitRepresentable],
ofType typeName: String) -> Bool {
return method.enclosedVarParameters.reduce(true) { soFar, param -> Bool in
soFar && (param.typeName == typeName)

This comment has been minimized.

Copy link
@marcelofabri

marcelofabri Mar 1, 2019

Collaborator

I was going to say that this might not work for generic types, but @objc classes can't be generic, so no problem here!

@@ -0,0 +1,68 @@
import SourceKittenFramework

public struct NSObjectPreferIsEqualRule: Rule, OptInRule, ConfigurationProviderRule, AutomaticTestableRule {

This comment has been minimized.

Copy link
@marcelofabri

marcelofabri Mar 1, 2019

Collaborator

I think we could promote this to a default rule, given how easy it's to make a mistake (I've definitely made this mistake several times)

@marcelofabri

This comment has been minimized.

Copy link
Collaborator

commented Mar 1, 2019

@matthew-healy Thanks so much for this! This is a great first contribution! 💯

I've added a few minor comments, but otherwise, it looks good to me 🚀

@matthew-healy

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

@marcelofabri Thanks! I think I've addressed all of your comments. 😄

@marcelofabri
Copy link
Collaborator

left a comment

Thanks again

@marcelofabri marcelofabri merged commit 6244d98 into realm:master Mar 4, 2019

11 checks passed

realm.SwiftLint Build #20190301.9 succeeded
Details
realm.SwiftLint (Analyze) Analyze succeeded
Details
realm.SwiftLint (CocoaPods) CocoaPods succeeded
Details
realm.SwiftLint (OSSCheck) OSSCheck succeeded
Details
realm.SwiftLint (SwiftPM xcode10) SwiftPM xcode10 succeeded
Details
realm.SwiftLint (SwiftPM xcode101) SwiftPM xcode101 succeeded
Details
realm.SwiftLint (TSan) TSan succeeded
Details
realm.SwiftLint (Xcode xcode10) Xcode xcode10 succeeded
Details
realm.SwiftLint (Xcode xcode101) Xcode xcode101 succeeded
Details
realm.SwiftLint (linux swift420) linux swift420 succeeded
Details
realm.SwiftLint (linux swift421) linux swift421 succeeded
Details

@matthew-healy matthew-healy deleted the matthew-healy:matthew-healy/equatable-nsobject branch Mar 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.