Rule Request: Futile NotificationCenter Observer Removal

[Amzd]

New Issue Checklist

• Updated SwiftLint to the latest version
• I searched for existing GitHub issues

New rule request

Since observers are automatically unregistered on dealloc now you should't call removeObserver(self) in the deinit.

1. Why should this rule be added? Share links to existing discussion about what the community thinks about this.

Developer docs:

If your app targets iOS 9.0 and later or macOS 10.11 and later, you don't need to unregister an observer in its dealloc method.
https://developer.apple.com/documentation/foundation/notificationcenter/1413994-removeobserver

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

Would trigger:

class Example {
    deinit {
        NotificationCenter.default.removeObserver(self)
    }
}

class Example {
    deinit {
        NotificationCenter.default.removeObserver(self, name: UITextView.textDidChangeNotification, object: nil)
    }
}

class Example {
    deinit {
        let center = NotificationCenter.default
        center.removeObserver(self)
    }
}

Would not trigger:

class Example {
    deinit {
        NotificationCenter.default.removeObserver(someOtherObserver)
    }
}

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

Can't think of any parameters you'd need.

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

See README.md for guidelines on when to mark a rule as opt-in.

I'm actually not sure if SwiftLint can check Deployment Target but if it doesn't then this should be opt-in.

[Amzd]

I've tried to make this with the custom_rules regex but even the simple regex that doesn't cover all cases doesn't work:

# Doesn't work
futile_observer_removal:
    name: "Futile Observer Removal"
    message: "Don't remove NC observer in deinit."
    regex: 'deinit {([^{}]*(removeObserver\(self)+[^{}]*)*}'

In atom it does show up:

[Amzd]

I started to write this but I can't get it to trigger at the expected location when it's spread over multiple lines:

error: -[SwiftLintFrameworkTests.FutileObserverRemovalRuleTests testWithDefaultConfiguration] : failed - triggeringExample did not violate at expected location:

class Foo {
   deinit {
       let center = NotificationCenter.default
       center.removeObserver(↓self)
   }
}

All the other examples work. If someone could pitch in on the multi line example that would be great.

import Foundation
import SourceKittenFramework

public struct FutileObserverRemovalRule: ASTRule, ConfigurationProviderRule, AutomaticTestableRule {
    public var configuration = SeverityConfiguration(.warning)
    
    public init() {}
    
    public static let description = RuleDescription(
        identifier: "futile_observer_removal",
        name: "Futile Observer Removal",
        description: "Observers are automatically unregistered on dealloc (iOS 9 / macOS 10.11) so you should't call `removeObserver(self)` in the deinit.",
        kind: .idiomatic,
        nonTriggeringExamples: [
            """
            class Example {
                deinit {
                    NotificationCenter.default.removeObserver(someOtherObserver)
                }
            }
            """
            ,
            """
            class Example {
                func removeObservers() {
                    NotificationCenter.default.removeObserver(self)
                }
            }
            """
        ],
        triggeringExamples: [
            """
            class Foo {
                deinit {
                    NotificationCenter.default.removeObserver(↓self)
                }
            }
            """
            ,
            """
            class Foo {
                deinit {
                    NotificationCenter.default.removeObserver(↓self, name: UITextView.textDidChangeNotification, object: nil)
                }
            }
            """
            ,
            """
            class Foo {
                deinit {
                    let center = NotificationCenter.default
                    center.removeObserver(↓self)
                }
            }
            """
        ]
    )
    
    public func validate(file: File,
                         kind: SwiftDeclarationKind,
                         dictionary: [String: SourceKitRepresentable]) -> [StyleViolation] {
        guard kind == .class else { return [] }
        
        let methodCollector = NamespaceCollector(dictionary: dictionary)
        let methods = methodCollector.findAllElements(of: [.functionMethodInstance])
        let deinitMethod = methods.first(where: { $0.name == "deinit" })
        
        return deinitMethod?.dictionary.substructure.flatMap { subDict -> [StyleViolation] in
            guard subDict.kind == SwiftExpressionKind.call.rawValue else { return [] }
            
            return violationRanges(in: file, dictionary: subDict).map {
                StyleViolation(ruleDescription: type(of: self).description,
                               severity: configuration.severity,
                               location: Location(file: file, characterOffset: $0.location))
            }
        } ?? []
    }
    
    public func violationRanges(in file: File, dictionary: [String: SourceKitRepresentable]) -> [NSRange] {
        guard
            dictionary.name == "NotificationCenter.default.removeObserver",
            let observer = firstArgumentBody(in: dictionary),
            let observerName = file.contents.bridge().substringWithByteRange(start: observer.offset, length: observer.length),
            observerName == "self"
            else { return [] }
        
        guard
            let range = file.contents.bridge().byteRangeToNSRange(start: observer.offset, length: observer.length)
            else { return [] }
        
        return [range]
    }
    
    /// observer parameter range
    private func firstArgumentBody(in dictionary: [String: SourceKitRepresentable]) -> (offset: Int, length: Int)? {
        if dictionary.enclosedArguments.names == [nil, "name", "object"],
            let bodyOffset = dictionary.enclosedArguments.first?.bodyOffset,
            let bodyLength = dictionary.enclosedArguments.first?.bodyLength {
            return (bodyOffset, bodyLength)
        } else if dictionary.enclosedArguments.isEmpty,
            let bodyOffset = dictionary.bodyOffset,
            let bodyLength = dictionary.bodyLength {
            return (bodyOffset, bodyLength)
        } else {
            return nil
        }
    }
}

private extension Array where Element == [String: SourceKitRepresentable] {
    var names: [String?] {
        return map { $0.name }
    }
}

[marcelofabri]

@Amzd I think this is valuable already even if it doesn't catch the NotificationCenter as variable scenario. One nitpick: I'd move the triggering location to be the method call and not self.

[Amzd]

@marcelofabri The issue is with removing self in deinit, not necessarily removeObserver. That's why I put it on self. If you think it's better to put it on removeObserver maybe I can add a configuration. Haven't dabbled with that yet though.