Fix nested disable commands improved (#5797)

CHANGELOG.md

@@ -33,6 +33,11 @@
   [Martin Redington](https://github.com/mildm8nnered)
   [#5802](https://github.com/realm/SwiftLint/issues/5802)
 
+* Fixes an issue where the `superfluous_disable_command` rule could generate
+  false positives for nested disable commands for custom rules.  
+  [Martin Redington](https://github.com/mildm8nnered)
+  [#5788](https://github.com/realm/SwiftLint/issues/5788)
+
 ## 0.57.0: Squeaky Clean Cycle
 
 #### Breaking

Source/SwiftLintCore/Models/Linter.swift

@@ -1,6 +1,8 @@
 import Foundation
 import SourceKittenFramework
 
+// swiftlint:disable file_length
+
 private let warnSourceKitFailedOnceImpl: Void = {
     Issue.genericWarning("SourceKit-based rules will be skipped because sourcekitd has failed.").print()
 }()
@@ -23,6 +25,8 @@ private extension Rule {
             return []
         }
 
+        let regions = regions.perIdentifierRegions
+
         let regionsDisablingSuperfluousDisableRule = regions.filter { region in
             region.isRuleDisabled(superfluousDisableCommandRule)
         }
@@ -32,33 +36,31 @@ private extension Rule {
             if regionsDisablingSuperfluousDisableRule.contains(where: { $0.contains(region.start) }) {
                 continue
             }
-            let sortedDisabledIdentifiers = region.disabledRuleIdentifiers.sorted {
-                $0.stringRepresentation < $1.stringRepresentation
+            guard let disabledRuleIdentifier = region.disabledRuleIdentifiers.first else {
+                continue
             }
-            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
-                    }
+            guard !isEnabled(in: region, for: disabledRuleIdentifier.stringRepresentation) else {
+                continue
+            }
+            var disableCommandValid = false
+            for violation in allViolations where region.contains(violation.location) {
+                if canBeDisabled(violation: violation, by: disabledRuleIdentifier) {
+                    disableCommandValid = true
+                    break
                 }
-                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
-                        )
+            }
+            if !disableCommandValid {
+                let reason = superfluousDisableCommandRule.reason(
+                    forRuleIdentifier: disabledRuleIdentifier.stringRepresentation
+                )
+                superfluousDisableCommandViolations.append(
+                    StyleViolation(
+                        ruleDescription: type(of: superfluousDisableCommandRule).description,
+                        severity: superfluousDisableCommandRule.configuration.severity,
+                        location: region.start,
+                        reason: reason
                     )
-                }
+                )
             }
         }
         return superfluousDisableCommandViolations
@@ -147,6 +149,57 @@ private extension Rule {
     }
 }
 
+private extension [Region] {
+    // Normally regions correspond to changes in the set of enabled rules. To detect superfluous disable command
+    // rule violations effectively, we need individual regions for each disabled rule identifier.
+    var perIdentifierRegions: [Region] {
+        guard isNotEmpty else {
+            return []
+        }
+
+        var convertedRegions = [Region]()
+        var startMap: [RuleIdentifier: Location] = [:]
+        var lastRegionEnd: Location?
+
+        for region in self {
+            let ruleIdentifiers = startMap.keys.sorted()
+            for ruleIdentifier in ruleIdentifiers where !region.disabledRuleIdentifiers.contains(ruleIdentifier) {
+                if let lastRegionEnd, let start = startMap[ruleIdentifier] {
+                    let newRegion = Region(start: start, end: lastRegionEnd, disabledRuleIdentifiers: [ruleIdentifier])
+                    convertedRegions.append(newRegion)
+                    startMap[ruleIdentifier] = nil
+                }
+            }
+            for ruleIdentifier in region.disabledRuleIdentifiers where startMap[ruleIdentifier] == nil {
+                startMap[ruleIdentifier] = region.start
+            }
+            if region.disabledRuleIdentifiers.isEmpty {
+                convertedRegions.append(region)
+            }
+            lastRegionEnd = region.end
+        }
+
+        let end = Location(file: first?.start.file, line: .max, character: .max)
+        for ruleIdentifier in startMap.keys.sorted() {
+            if let start = startMap[ruleIdentifier] {
+                let newRegion = Region(start: start, end: end, disabledRuleIdentifiers: [ruleIdentifier])
+                convertedRegions.append(newRegion)
+                startMap[ruleIdentifier] = nil
+            }
+        }
+
+        return convertedRegions.sorted {
+            if $0.start == $1.start {
+                if let lhsDisabledRuleIdentifier = $0.disabledRuleIdentifiers.first,
+                   let rhsDisabledRuleIdentifier = $1.disabledRuleIdentifiers.first {
+                    return lhsDisabledRuleIdentifier < rhsDisabledRuleIdentifier
+                }
+            }
+            return $0.start < $1.start
+        }
+    }
+}
+
 /// Represents a file that can be linted for style violations and corrections after being collected.
 public struct Linter {
     /// The file to lint with this linter.

Source/SwiftLintCore/Models/RuleIdentifier.swift

@@ -1,5 +1,5 @@
 /// An identifier representing a SwiftLint rule, or all rules.
-public enum RuleIdentifier: Hashable, ExpressibleByStringLiteral {
+public enum RuleIdentifier: Hashable, ExpressibleByStringLiteral, Comparable {
     // MARK: - Values
 
     /// Special identifier that should be treated as referring to 'all' SwiftLint rules. One helpful usecase is in
@@ -39,4 +39,10 @@ public enum RuleIdentifier: Hashable, ExpressibleByStringLiteral {
     public init(stringLiteral value: String) {
         self = Self(value)
     }
+
+    // MARK: - Comparable Conformance
+
+    public static func < (lhs: Self, rhs: Self) -> Bool {
+        lhs.stringRepresentation < rhs.stringRepresentation
+    }
 }

Tests/SwiftLintFrameworkTests/CustomRulesTests.swift

@@ -427,6 +427,83 @@ final class CustomRulesTests: SwiftLintTestCase {
         XCTAssertTrue(try violations(forExample: example, customRules: customRules).isEmpty)
     }
 
+    func testNestedCustomRuleDisablesDoNotTriggerSuperfluousDisableCommand() throws {
+        let customRules: [String: Any] = [
+            "rule1": [
+                "regex": "pattern1"
+            ],
+            "rule2": [
+                "regex": "pattern2"
+            ],
+        ]
+        let example = Example("""
+                               // swiftlint:disable rule1
+                               // swiftlint:disable rule2
+                               let pattern2 = ""
+                               // swiftlint:enable rule2
+                               let pattern1 = ""
+                               // swiftlint:enable rule1
+                               """)
+        XCTAssertTrue(try violations(forExample: example, customRules: customRules).isEmpty)
+    }
+
+    func testNestedAndOverlappingCustomRuleDisables() throws {
+        let customRules: [String: Any] = [
+            "rule1": [
+                "regex": "pattern1"
+            ],
+            "rule2": [
+                "regex": "pattern2"
+            ],
+            "rule3": [
+                "regex": "pattern3"
+            ],
+        ]
+        let example = Example("""
+                              // swiftlint:disable rule1
+                              // swiftlint:disable rule2
+                              // swiftlint:disable rule3
+                              let pattern2 = ""
+                              // swiftlint:enable rule2
+                              // swiftlint:enable rule3
+                              let pattern1 = ""
+                              // swiftlint:enable rule1
+                              """)
+        let violations = try violations(forExample: example, customRules: customRules)
+
+        XCTAssertEqual(violations.count, 1)
+        XCTAssertTrue(violations[0].isSuperfluousDisableCommandViolation(for: "rule3"))
+    }
+
+    func testSuperfluousDisableRuleOrder() throws {
+        let customRules: [String: Any] = [
+            "rule1": [
+                "regex": "pattern1"
+            ],
+            "rule2": [
+                "regex": "pattern2"
+            ],
+            "rule3": [
+                "regex": "pattern3"
+            ],
+        ]
+        let example = Example("""
+                              // swiftlint:disable rule1
+                              // swiftlint:disable rule2 rule3
+                              // swiftlint:enable rule3 rule2
+                              // swiftlint:disable rule2
+                              // swiftlint:enable rule1
+                              // swiftlint:enable rule2
+                              """)
+        let violations = try violations(forExample: example, customRules: customRules)
+
+        XCTAssertEqual(violations.count, 4)
+        XCTAssertTrue(violations[0].isSuperfluousDisableCommandViolation(for: "rule1"))
+        XCTAssertTrue(violations[1].isSuperfluousDisableCommandViolation(for: "rule2"))
+        XCTAssertTrue(violations[2].isSuperfluousDisableCommandViolation(for: "rule3"))
+        XCTAssertTrue(violations[3].isSuperfluousDisableCommandViolation(for: "rule2"))
+    }
+
     // MARK: - Private
 
     private func getCustomRules(_ extraConfig: [String: Any] = [:]) -> (Configuration, CustomRules) {