Fix superfluous_disable_command for custom_rules (#5670)

Co-authored-by: Danny Mösch <danny.moesch@icloud.com>

CHANGELOG.md

@@ -34,7 +34,12 @@
 
 #### Bug Fixes
 
-* None.
+* `superfluous_disable_command` violations are now triggered for
+  custom rules.  
+  [Marcelo Fabri](https://github.com/marcelofabri)
+  [Martin Redington](https://github.com/mildm8nnered)
+  [SimplyDanny](https://github.com/SimplyDanny)
+  [#4754](https://github.com/realm/SwiftLint/issues/4754)
 
 ## 0.56.2: Heat Pump Dryer
 

Source/SwiftLintCore/Extensions/Configuration+RulesMode.swift

@@ -112,8 +112,7 @@ public extension Configuration {
             switch self {
             case let .only(onlyRules) where onlyRules.contains { $0 == CustomRules.description.identifier }:
                 let customRulesRule = (allRulesWrapped.first { $0.rule is CustomRules })?.rule as? CustomRules
-                let customRuleIdentifiers = customRulesRule?.configuration.customRuleConfigurations.map(\.identifier)
-                return .only(onlyRules.union(Set(customRuleIdentifiers ?? [])))
+                return .only(onlyRules.union(Set(customRulesRule?.customRuleIdentifiers ?? [])))
 
             default:
                 return self

Source/SwiftLintCore/Extensions/Configuration+RulesWrapper.swift

@@ -13,8 +13,7 @@ internal extension Configuration {
         private var validRuleIdentifiers: Set<String> {
             let regularRuleIdentifiers = allRulesWrapped.map { type(of: $0.rule).description.identifier }
             let configurationCustomRulesIdentifiers =
-                (allRulesWrapped.first { $0.rule is CustomRules }?.rule as? CustomRules)?
-                    .configuration.customRuleConfigurations.map(\.identifier) ?? []
+                (allRulesWrapped.first { $0.rule is CustomRules }?.rule as? CustomRules)?.customRuleIdentifiers ?? []
             return Set(regularRuleIdentifiers + configurationCustomRulesIdentifiers)
         }
 
@@ -247,7 +246,7 @@ internal extension Configuration {
                         as? CustomRules {
                         onlyRules = onlyRules.union(
                             Set(
-                                childCustomRulesRule.configuration.customRuleConfigurations.map(\.identifier)
+                                childCustomRulesRule.customRuleIdentifiers
                             )
                         )
                     }

Source/SwiftLintCore/Models/Linter.swift

@@ -16,47 +16,56 @@ private struct LintResult {
 }
 
 private extension Rule {
-    static func superfluousDisableCommandViolations(regions: [Region],
-                                                    superfluousDisableCommandRule: SuperfluousDisableCommandRule?,
-                                                    allViolations: [StyleViolation]) -> [StyleViolation] {
+    func superfluousDisableCommandViolations(regions: [Region],
+                                             superfluousDisableCommandRule: SuperfluousDisableCommandRule?,
+                                             allViolations: [StyleViolation]) -> [StyleViolation] {
         guard regions.isNotEmpty, let superfluousDisableCommandRule else {
             return []
         }
 
-        let regionsDisablingCurrentRule = regions.filter { region in
-            region.isRuleDisabled(self.init())
-        }
         let regionsDisablingSuperfluousDisableRule = regions.filter { region in
             region.isRuleDisabled(superfluousDisableCommandRule)
         }
 
-        return regionsDisablingCurrentRule.compactMap { region -> StyleViolation? in
-            let isSuperfluousRuleDisabled = regionsDisablingSuperfluousDisableRule.contains {
-                $0.contains(region.start)
-            }
-
-            guard !isSuperfluousRuleDisabled else {
-                return nil
+        var superfluousDisableCommandViolations = [StyleViolation]()
+        for region in regions {
+            if regionsDisablingSuperfluousDisableRule.contains(where: { $0.contains(region.start) }) {
+                continue
             }
-
-            let noViolationsInDisabledRegion = !allViolations.contains { violation in
-                region.contains(violation.location)
+            let sortedDisabledIdentifiers = region.disabledRuleIdentifiers.sorted {
+                $0.stringRepresentation < $1.stringRepresentation
             }
-            guard noViolationsInDisabledRegion else {
-                return nil
+            commandIDsLoop: for disabledIdentifier in sortedDisabledIdentifiers {
+                guard !isEnabled(in: region, for: disabledIdentifier.stringRepresentation) else {
+                    continue
+                }
+                var disableCommandValid = false
+                for violation in allViolations where region.contains(violation.location) {
+                    if canBeDisabled(violation: violation, by: disabledIdentifier) {
+                        disableCommandValid = true
+                        continue commandIDsLoop
+                    }
+                }
+                if !disableCommandValid {
+                    let reason = superfluousDisableCommandRule.reason(
+                        forRuleIdentifier: disabledIdentifier.stringRepresentation
+                    )
+                    superfluousDisableCommandViolations.append(
+                        StyleViolation(
+                            ruleDescription: type(of: superfluousDisableCommandRule).description,
+                            severity: superfluousDisableCommandRule.configuration.severity,
+                            location: region.start,
+                            reason: reason
+                        )
+                    )
+                }
             }
-
-            return StyleViolation(
-                ruleDescription: type(of: superfluousDisableCommandRule).description,
-                severity: superfluousDisableCommandRule.configuration.severity,
-                location: region.start,
-                reason: superfluousDisableCommandRule.reason(for: self)
-            )
         }
+        return superfluousDisableCommandViolations
     }
 
     // As we need the configuration to get custom identifiers.
-    // swiftlint:disable:next function_parameter_count
+    // swiftlint:disable:next function_parameter_count function_body_length
     func lint(file: SwiftLintFile,
               regions: [Region],
               benchmark: Bool,
@@ -93,16 +102,26 @@ private extension Rule {
 
         let (disabledViolationsAndRegions, enabledViolationsAndRegions) = violations.map { violation in
             (violation, regions.first { $0.contains(violation.location) })
-        }.partitioned { _, region in
-            region?.isRuleEnabled(self) ?? true
+        }.partitioned { violation, region in
+            if let region {
+                return isEnabled(in: region, for: violation.ruleIdentifier)
+            }
+            return true
         }
 
+        let customRulesIDs: [String] = {
+             guard let customRules = self as? CustomRules else {
+                 return []
+             }
+             return customRules.customRuleIdentifiers
+         }()
         let ruleIDs = Self.description.allIdentifiers +
+            customRulesIDs +
             (superfluousDisableCommandRule.map({ type(of: $0) })?.description.allIdentifiers ?? []) +
             [RuleIdentifier.all.stringRepresentation]
         let ruleIdentifiers = Set(ruleIDs.map { RuleIdentifier($0) })
 
-        let superfluousDisableCommandViolations = Self.superfluousDisableCommandViolations(
+        let superfluousDisableCommandViolations = superfluousDisableCommandViolations(
             regions: regions.count > 1 ? file.regions(restrictingRuleIdentifiers: ruleIdentifiers) : regions,
             superfluousDisableCommandRule: superfluousDisableCommandRule,
             allViolations: violations

Source/SwiftLintCore/Models/Region.swift

@@ -44,6 +44,10 @@ public struct Region: Equatable {
     ///
     /// - parameter rule: The rule whose status should be determined.
     ///
+    /// - note: For CustomRules, this will only return true if the `custom_rules` identifier
+    ///         is used with the `swiftlint` disable command, but this method is never
+    ///         called for CustomRules.
+    ///
     /// - returns: True if the specified rule is disabled in this region.
     public func isRuleDisabled(_ rule: some Rule) -> Bool {
         areRulesDisabled(ruleIDs: type(of: rule).description.allIdentifiers)

Source/SwiftLintCore/Protocols/Rule.swift

@@ -70,6 +70,26 @@ public protocol Rule {
     ///
     /// - returns: All style violations to the rule's expectations.
     func validate(file: SwiftLintFile, using storage: RuleStorage, compilerArguments: [String]) -> [StyleViolation]
+
+    /// Checks if a style violation can be disabled by a command specifying a rule ID. Only the rule can claim that for
+    /// sure since it knows all the possible identifiers.
+    ///
+    /// - Parameters:
+    ///   - violation: A style violation.
+    ///   - ruleID: The name of a rule as used in a disable command.
+    ///
+    /// - Returns: A boolean value indicating whether the violation can be disabled by the given ID.
+   func canBeDisabled(violation: StyleViolation, by ruleID: RuleIdentifier) -> Bool
+
+   /// Checks if a the rule is enabled in a given region. A specific rule ID can be provided in case a rule supports
+   /// more than one identifier.
+   ///
+   /// - Parameters:
+   ///   - region: The region to check.
+   ///   - ruleID: Rule identifier deviating from the default rule's name.
+   ///
+   /// - Returns: A boolean value indicating whether the rule is enabled in the given region.
+   func isEnabled(in region: Region, for ruleID: String) -> Bool
 }
 
 public extension Rule {
@@ -110,6 +130,20 @@ public extension Rule {
     func createConfigurationDescription(exclusiveOptions: Set<String> = []) -> RuleConfigurationDescription {
         RuleConfigurationDescription.from(configuration: configuration, exclusiveOptions: exclusiveOptions)
     }
+
+    func canBeDisabled(violation: StyleViolation, by ruleID: RuleIdentifier) -> Bool {
+        switch ruleID {
+        case .all:
+            true
+        case let .single(identifier: id):
+               Self.description.allIdentifiers.contains(id)
+            && Self.description.allIdentifiers.contains(violation.ruleIdentifier)
+        }
+    }
+
+    func isEnabled(in region: Region, for ruleID: String) -> Bool {
+        !Self.description.allIdentifiers.contains(ruleID) || region.isRuleEnabled(self)
+    }
 }
 
 public extension Rule {

Source/SwiftLintCore/Rules/CustomRules.swift

@@ -41,6 +41,10 @@ struct CustomRules: Rule, CacheDescriptionProvider {
         configuration.cacheDescription
     }
 
+    var customRuleIdentifiers: [String] {
+        configuration.customRuleConfigurations.map(\.identifier)
+    }
+
     static let description = RuleDescription(
         identifier: "custom_rules",
         name: "Custom Rules",
@@ -79,19 +83,29 @@ struct CustomRules: Rule, CacheDescriptionProvider {
                                severity: configuration.severity,
                                location: Location(file: file, characterOffset: $0.location),
                                reason: configuration.message)
-            }).filter { violation in
-                guard let region = file.regions().first(where: { $0.contains(violation.location) }) else {
-                    return true
-                }
+            })
+        }
+    }
 
-                return !region.isRuleDisabled(customRuleIdentifier: configuration.identifier)
-            }
+    func canBeDisabled(violation: StyleViolation, by ruleID: RuleIdentifier) -> Bool {
+        switch ruleID {
+        case let .single(identifier: id):
+            id == Self.description.identifier
+                ? customRuleIdentifiers.contains(violation.ruleIdentifier)
+                : customRuleIdentifiers.contains(id) && violation.ruleIdentifier == id
+        default:
+            (self as any Rule).canBeDisabled(violation: violation, by: ruleID)
         }
     }
-}
 
-private extension Region {
-    func isRuleDisabled(customRuleIdentifier: String) -> Bool {
-        disabledRuleIdentifiers.contains(RuleIdentifier(customRuleIdentifier))
+    func isEnabled(in region: Region, for ruleID: String) -> Bool {
+        if !Self.description.allIdentifiers.contains(ruleID),
+           !customRuleIdentifiers.contains(ruleID),
+           Self.description.identifier != ruleID {
+            return true
+        }
+        return !region.disabledRuleIdentifiers.contains(RuleIdentifier(Self.description.identifier))
+            && !region.disabledRuleIdentifiers.contains(RuleIdentifier(ruleID))
+            && !region.disabledRuleIdentifiers.contains(.all)
     }
 }

Source/SwiftLintCore/Rules/SuperfluousDisableCommandRule.swift

@@ -34,9 +34,9 @@ package struct SuperfluousDisableCommandRule: SourceKitFreeRule {
         []
     }
 
-    func reason(for rule: (some Rule).Type) -> String {
+    func reason(forRuleIdentifier ruleIdentifier: String) -> String {
         """
-        SwiftLint rule '\(rule.description.identifier)' did not trigger a violation in the disabled region; \
+        SwiftLint rule '\(ruleIdentifier)' did not trigger a violation in the disabled region; \
         remove the disable command
         """
     }