From 9c2d0d5f86f29170ec5cc69235e3f1b5da6dac85 Mon Sep 17 00:00:00 2001 From: Martin Redington Date: Tue, 15 Oct 2024 18:56:39 +0100 Subject: [PATCH] Fix nested disable commands improved (#5797) --- CHANGELOG.md | 5 + Source/SwiftLintCore/Models/Linter.swift | 101 +++++++++++++----- .../SwiftLintCore/Models/RuleIdentifier.swift | 8 +- .../CustomRulesTests.swift | 77 +++++++++++++ 4 files changed, 166 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3596e31d18..e32680d436 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/Source/SwiftLintCore/Models/Linter.swift b/Source/SwiftLintCore/Models/Linter.swift index 23bfad7d54..c87065a557 100644 --- a/Source/SwiftLintCore/Models/Linter.swift +++ b/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. diff --git a/Source/SwiftLintCore/Models/RuleIdentifier.swift b/Source/SwiftLintCore/Models/RuleIdentifier.swift index c4ee97f5ba..8955c8badf 100644 --- a/Source/SwiftLintCore/Models/RuleIdentifier.swift +++ b/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 + } } diff --git a/Tests/SwiftLintFrameworkTests/CustomRulesTests.swift b/Tests/SwiftLintFrameworkTests/CustomRulesTests.swift index 3337822047..548fb98db2 100644 --- a/Tests/SwiftLintFrameworkTests/CustomRulesTests.swift +++ b/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) {