From cc6360b1aee17705482250291436ae10dada6041 Mon Sep 17 00:00:00 2001 From: Martin Redington Date: Sun, 15 Sep 2024 11:16:24 +0100 Subject: [PATCH 1/7] Try remapping the regions --- .../Extensions/SwiftLintFile+Regex.swift | 45 +++++++++++++++++++ Source/SwiftLintCore/Models/Linter.swift | 9 +++- 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/Source/SwiftLintCore/Extensions/SwiftLintFile+Regex.swift b/Source/SwiftLintCore/Extensions/SwiftLintFile+Regex.swift index c8fe43d36d..54540f1203 100644 --- a/Source/SwiftLintCore/Extensions/SwiftLintFile+Regex.swift +++ b/Source/SwiftLintCore/Extensions/SwiftLintFile+Regex.swift @@ -57,6 +57,51 @@ extension SwiftLintFile { return regions } + public func remap(regions: [Region]) -> [Region] { + var remappedRegions = [Region]() + var startMap: [RuleIdentifier: Location] = [:] + var lastRegionEnd: Location? + + guard regions.isNotEmpty else { + return [] + } + + for region in regions { + let disabledRuleIdentifiers = startMap.keys + for ruleIdentifier in region.disabledRuleIdentifiers where startMap[ruleIdentifier] == nil { + startMap[ruleIdentifier] = region.start + } + // We've started all of our new regions - can we finish any? + // swiftlint:disable:next line_length + for ruleIdentifier in disabledRuleIdentifiers where !region.disabledRuleIdentifiers.contains(ruleIdentifier) { + if let lastRegionEnd, let start = startMap[ruleIdentifier] { + let newRegion = Region(start: start, end: lastRegionEnd, disabledRuleIdentifiers: [ruleIdentifier]) + remappedRegions.append(newRegion) + startMap[ruleIdentifier] = nil + } else { + print(">>>> this should not happen") + } + } + lastRegionEnd = region.end + if region.disabledRuleIdentifiers.isEmpty { + remappedRegions.append(region) + } + } + // We're at the end now, so we need to finish up any still disabled rules + let end = Location(file: path, line: .max, character: .max) + for ruleIdentifier in startMap.keys { + if let start = startMap[ruleIdentifier] { + let newRegion = Region(start: start, end: end, disabledRuleIdentifiers: [ruleIdentifier]) + remappedRegions.append(newRegion) + startMap[ruleIdentifier] = nil + } else { + print(">>>> this also should not happen") + } + } + + return remappedRegions + } + public func commands(in range: NSRange? = nil) -> [Command] { guard let range else { return commands diff --git a/Source/SwiftLintCore/Models/Linter.swift b/Source/SwiftLintCore/Models/Linter.swift index 23bfad7d54..1c3ecd62a7 100644 --- a/Source/SwiftLintCore/Models/Linter.swift +++ b/Source/SwiftLintCore/Models/Linter.swift @@ -19,7 +19,11 @@ private extension Rule { func superfluousDisableCommandViolations(regions: [Region], superfluousDisableCommandRule: SuperfluousDisableCommandRule?, allViolations: [StyleViolation]) -> [StyleViolation] { - guard regions.isNotEmpty, let superfluousDisableCommandRule else { + guard let superfluousDisableCommandRule else { + return [] + } + guard regions.isNotEmpty, allViolations.isNotEmpty + else { return [] } @@ -121,8 +125,9 @@ private extension Rule { [RuleIdentifier.all.stringRepresentation] let ruleIdentifiers = Set(ruleIDs.map { RuleIdentifier($0) }) + let regions = regions.count > 1 ? file.regions(restrictingRuleIdentifiers: ruleIdentifiers) : regions let superfluousDisableCommandViolations = superfluousDisableCommandViolations( - regions: regions.count > 1 ? file.regions(restrictingRuleIdentifiers: ruleIdentifiers) : regions, + regions: file.remap(regions: regions), superfluousDisableCommandRule: superfluousDisableCommandRule, allViolations: violations ) From 5e9257b9bcbf8f0674c06f479795e6bd41f21b06 Mon Sep 17 00:00:00 2001 From: Martin Redington Date: Sun, 15 Sep 2024 18:53:12 +0100 Subject: [PATCH 2/7] Added more tests and fixes --- .../Extensions/SwiftLintFile+Regex.swift | 4 +- Source/SwiftLintCore/Models/Linter.swift | 3 +- .../SwiftLintCore/Models/RuleIdentifier.swift | 8 ++- .../SwiftLintFrameworkTests/RegionTests.swift | 50 ++++++++++++++++--- 4 files changed, 54 insertions(+), 11 deletions(-) diff --git a/Source/SwiftLintCore/Extensions/SwiftLintFile+Regex.swift b/Source/SwiftLintCore/Extensions/SwiftLintFile+Regex.swift index 54540f1203..1030f73d38 100644 --- a/Source/SwiftLintCore/Extensions/SwiftLintFile+Regex.swift +++ b/Source/SwiftLintCore/Extensions/SwiftLintFile+Regex.swift @@ -73,7 +73,7 @@ extension SwiftLintFile { } // We've started all of our new regions - can we finish any? // swiftlint:disable:next line_length - for ruleIdentifier in disabledRuleIdentifiers where !region.disabledRuleIdentifiers.contains(ruleIdentifier) { + for ruleIdentifier in disabledRuleIdentifiers.sorted() where !region.disabledRuleIdentifiers.contains(ruleIdentifier) { if let lastRegionEnd, let start = startMap[ruleIdentifier] { let newRegion = Region(start: start, end: lastRegionEnd, disabledRuleIdentifiers: [ruleIdentifier]) remappedRegions.append(newRegion) @@ -89,7 +89,7 @@ extension SwiftLintFile { } // We're at the end now, so we need to finish up any still disabled rules let end = Location(file: path, line: .max, character: .max) - for ruleIdentifier in startMap.keys { + for ruleIdentifier in startMap.keys.sorted() { if let start = startMap[ruleIdentifier] { let newRegion = Region(start: start, end: end, disabledRuleIdentifiers: [ruleIdentifier]) remappedRegions.append(newRegion) diff --git a/Source/SwiftLintCore/Models/Linter.swift b/Source/SwiftLintCore/Models/Linter.swift index 1c3ecd62a7..0a7e2f7237 100644 --- a/Source/SwiftLintCore/Models/Linter.swift +++ b/Source/SwiftLintCore/Models/Linter.swift @@ -22,8 +22,7 @@ private extension Rule { guard let superfluousDisableCommandRule else { return [] } - guard regions.isNotEmpty, allViolations.isNotEmpty - else { + guard regions.isNotEmpty else { return [] } 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/RegionTests.swift b/Tests/SwiftLintFrameworkTests/RegionTests.swift index 3935f583c7..e27ed3f09e 100644 --- a/Tests/SwiftLintFrameworkTests/RegionTests.swift +++ b/Tests/SwiftLintFrameworkTests/RegionTests.swift @@ -20,14 +20,18 @@ final class RegionTests: SwiftLintTestCase { let file = SwiftLintFile(contents: "// swiftlint:disable rule_id\n") let start = Location(file: nil, line: 1, character: 29) let end = Location(file: nil, line: .max, character: .max) - XCTAssertEqual(file.regions(), [Region(start: start, end: end, disabledRuleIdentifiers: ["rule_id"])]) + let fileRegions = file.regions() + XCTAssertEqual(fileRegions, [Region(start: start, end: end, disabledRuleIdentifiers: ["rule_id"])]) + XCTAssertEqual(file.remap(regions: fileRegions), fileRegions) } // enable do { let file = SwiftLintFile(contents: "// swiftlint:enable rule_id\n") let start = Location(file: nil, line: 1, character: 28) let end = Location(file: nil, line: .max, character: .max) - XCTAssertEqual(file.regions(), [Region(start: start, end: end, disabledRuleIdentifiers: [])]) + let fileRegions = file.regions() + XCTAssertEqual(fileRegions, [Region(start: start, end: end, disabledRuleIdentifiers: [])]) + XCTAssertEqual(file.remap(regions: fileRegions), fileRegions) } } @@ -35,7 +39,8 @@ final class RegionTests: SwiftLintTestCase { // disable/enable do { let file = SwiftLintFile(contents: "// swiftlint:disable rule_id\n// swiftlint:enable rule_id\n") - XCTAssertEqual(file.regions(), [ + let fileRegions = file.regions() + XCTAssertEqual(fileRegions, [ Region(start: Location(file: nil, line: 1, character: 29), end: Location(file: nil, line: 2, character: 27), disabledRuleIdentifiers: ["rule_id"]), @@ -43,11 +48,13 @@ final class RegionTests: SwiftLintTestCase { end: Location(file: nil, line: .max, character: .max), disabledRuleIdentifiers: []), ]) + XCTAssertEqual(file.remap(regions: fileRegions), fileRegions) } // enable/disable do { let file = SwiftLintFile(contents: "// swiftlint:enable rule_id\n// swiftlint:disable rule_id\n") - XCTAssertEqual(file.regions(), [ + let fileRegions = file.regions() + XCTAssertEqual(fileRegions, [ Region(start: Location(file: nil, line: 1, character: 28), end: Location(file: nil, line: 2, character: 28), disabledRuleIdentifiers: []), @@ -55,6 +62,7 @@ final class RegionTests: SwiftLintTestCase { end: Location(file: nil, line: .max, character: .max), disabledRuleIdentifiers: ["rule_id"]), ]) + XCTAssertEqual(file.remap(regions: fileRegions), fileRegions) } } @@ -62,7 +70,8 @@ final class RegionTests: SwiftLintTestCase { let file = SwiftLintFile(contents: "// swiftlint:disable:next 1\n" + "// swiftlint:disable:this 2\n" + "// swiftlint:disable:previous 3\n") - XCTAssertEqual(file.regions(), [ + let fileRegions = file.regions() + XCTAssertEqual(fileRegions, [ Region(start: Location(file: nil, line: 2, character: nil), end: Location(file: nil, line: 2, character: .max - 1), disabledRuleIdentifiers: ["1", "2", "3"]), @@ -70,6 +79,20 @@ final class RegionTests: SwiftLintTestCase { end: Location(file: nil, line: .max, character: .max), disabledRuleIdentifiers: []), ]) + XCTAssertEqual(file.remap(regions: fileRegions), [ + Region(start: Location(file: nil, line: 2, character: nil), + end: Location(file: nil, line: 2, character: .max - 1), + disabledRuleIdentifiers: ["1"]), + Region(start: Location(file: nil, line: 2, character: nil), + end: Location(file: nil, line: 2, character: .max - 1), + disabledRuleIdentifiers: ["2"]), + Region(start: Location(file: nil, line: 2, character: nil), + end: Location(file: nil, line: 2, character: .max - 1), + disabledRuleIdentifiers: ["3"]), + Region(start: Location(file: nil, line: 2, character: .max), + end: Location(file: nil, line: .max, character: .max), + disabledRuleIdentifiers: []), + ]) } func testSeveralRegionsFromSeveralCommands() { @@ -79,7 +102,8 @@ final class RegionTests: SwiftLintTestCase { "// swiftlint:enable 1\n" + "// swiftlint:enable 2\n" + "// swiftlint:enable 3\n") - XCTAssertEqual(file.regions(), [ + let fileRegions = file.regions() + XCTAssertEqual(fileRegions, [ Region(start: Location(file: nil, line: 1, character: 23), end: Location(file: nil, line: 2, character: 22), disabledRuleIdentifiers: ["1"]), @@ -99,5 +123,19 @@ final class RegionTests: SwiftLintTestCase { end: Location(file: nil, line: .max, character: .max), disabledRuleIdentifiers: []), ]) + XCTAssertEqual(file.remap(regions: fileRegions), [ + Region(start: Location(file: nil, line: 1, character: 23), + end: Location(file: nil, line: 4, character: 21), + disabledRuleIdentifiers: ["1"]), + Region(start: Location(file: nil, line: 2, character: 23), + end: Location(file: nil, line: 5, character: 21), + disabledRuleIdentifiers: ["2"]), + Region(start: Location(file: nil, line: 3, character: 23), + end: Location(file: nil, line: 6, character: 21), + disabledRuleIdentifiers: ["3"]), + Region(start: Location(file: nil, line: 6, character: 22), + end: Location(file: nil, line: .max, character: .max), + disabledRuleIdentifiers: []), + ]) } } From a54d56e69a0345be3b5f0b57fd0e1c76a7998168 Mon Sep 17 00:00:00 2001 From: Martin Redington Date: Sun, 15 Sep 2024 20:56:20 +0100 Subject: [PATCH 3/7] Added one more test --- .../SwiftLintFrameworkTests/RegionTests.swift | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/Tests/SwiftLintFrameworkTests/RegionTests.swift b/Tests/SwiftLintFrameworkTests/RegionTests.swift index e27ed3f09e..5345334770 100644 --- a/Tests/SwiftLintFrameworkTests/RegionTests.swift +++ b/Tests/SwiftLintFrameworkTests/RegionTests.swift @@ -138,4 +138,48 @@ final class RegionTests: SwiftLintTestCase { disabledRuleIdentifiers: []), ]) } + + func testOverlappingAndNestedRegions() { + let file = SwiftLintFile(contents: "// swiftlint:disable 1\n" + + "// swiftlint:disable 2\n" + + "// swiftlint:disable 3\n" + + "// swiftlint:enable 2\n" + + "// swiftlint:enable 3\n" + + "// swiftlint:enable 1\n") + let fileRegions = file.regions() + XCTAssertEqual(fileRegions, [ + Region(start: Location(file: nil, line: 1, character: 23), + end: Location(file: nil, line: 2, character: 22), + disabledRuleIdentifiers: ["1"]), + Region(start: Location(file: nil, line: 2, character: 23), + end: Location(file: nil, line: 3, character: 22), + disabledRuleIdentifiers: ["1", "2"]), + Region(start: Location(file: nil, line: 3, character: 23), + end: Location(file: nil, line: 4, character: 21), + disabledRuleIdentifiers: ["1", "2", "3"]), + Region(start: Location(file: nil, line: 4, character: 22), + end: Location(file: nil, line: 5, character: 21), + disabledRuleIdentifiers: ["1", "3"]), + Region(start: Location(file: nil, line: 5, character: 22), + end: Location(file: nil, line: 6, character: 21), + disabledRuleIdentifiers: ["1"]), + Region(start: Location(file: nil, line: 6, character: 22), + end: Location(file: nil, line: .max, character: .max), + disabledRuleIdentifiers: []), + ]) + XCTAssertEqual(file.remap(regions: fileRegions), [ + Region(start: Location(file: nil, line: 2, character: 23), + end: Location(file: nil, line: 4, character: 21), + disabledRuleIdentifiers: ["2"]), + Region(start: Location(file: nil, line: 3, character: 23), + end: Location(file: nil, line: 5, character: 21), + disabledRuleIdentifiers: ["3"]), + Region(start: Location(file: nil, line: 1, character: 23), + end: Location(file: nil, line: 6, character: 21), + disabledRuleIdentifiers: ["1"]), + Region(start: Location(file: nil, line: 6, character: 22), + end: Location(file: nil, line: .max, character: .max), + disabledRuleIdentifiers: []), + ]) + } } From 07a86ec52c85d027ad26273776f1fe165091bbce Mon Sep 17 00:00:00 2001 From: Martin Redington Date: Sun, 15 Sep 2024 21:05:34 +0100 Subject: [PATCH 4/7] Tidyup --- .../Extensions/SwiftLintFile+Regex.swift | 28 ++++++++----------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/Source/SwiftLintCore/Extensions/SwiftLintFile+Regex.swift b/Source/SwiftLintCore/Extensions/SwiftLintFile+Regex.swift index 1030f73d38..68a9955355 100644 --- a/Source/SwiftLintCore/Extensions/SwiftLintFile+Regex.swift +++ b/Source/SwiftLintCore/Extensions/SwiftLintFile+Regex.swift @@ -58,44 +58,38 @@ extension SwiftLintFile { } public func remap(regions: [Region]) -> [Region] { - var remappedRegions = [Region]() - var startMap: [RuleIdentifier: Location] = [:] - var lastRegionEnd: Location? - guard regions.isNotEmpty else { return [] } + var remappedRegions = [Region]() + var startMap: [RuleIdentifier: Location] = [:] + var lastRegionEnd: Location? + for region in regions { - let disabledRuleIdentifiers = startMap.keys - for ruleIdentifier in region.disabledRuleIdentifiers where startMap[ruleIdentifier] == nil { - startMap[ruleIdentifier] = region.start - } - // We've started all of our new regions - can we finish any? - // swiftlint:disable:next line_length - for ruleIdentifier in disabledRuleIdentifiers.sorted() where !region.disabledRuleIdentifiers.contains(ruleIdentifier) { + 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]) remappedRegions.append(newRegion) startMap[ruleIdentifier] = nil - } else { - print(">>>> this should not happen") } } - lastRegionEnd = region.end + for ruleIdentifier in region.disabledRuleIdentifiers where startMap[ruleIdentifier] == nil { + startMap[ruleIdentifier] = region.start + } if region.disabledRuleIdentifiers.isEmpty { remappedRegions.append(region) } + lastRegionEnd = region.end } - // We're at the end now, so we need to finish up any still disabled rules + let end = Location(file: path, line: .max, character: .max) for ruleIdentifier in startMap.keys.sorted() { if let start = startMap[ruleIdentifier] { let newRegion = Region(start: start, end: end, disabledRuleIdentifiers: [ruleIdentifier]) remappedRegions.append(newRegion) startMap[ruleIdentifier] = nil - } else { - print(">>>> this also should not happen") } } From f31f85eee2771a3e2aba07fe42c74d46f5257440 Mon Sep 17 00:00:00 2001 From: Martin Redington Date: Sun, 15 Sep 2024 21:29:30 +0100 Subject: [PATCH 5/7] Cleanup --- Source/SwiftLintCore/Models/Linter.swift | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/Source/SwiftLintCore/Models/Linter.swift b/Source/SwiftLintCore/Models/Linter.swift index 0a7e2f7237..ad7db97bee 100644 --- a/Source/SwiftLintCore/Models/Linter.swift +++ b/Source/SwiftLintCore/Models/Linter.swift @@ -19,13 +19,10 @@ private extension Rule { func superfluousDisableCommandViolations(regions: [Region], superfluousDisableCommandRule: SuperfluousDisableCommandRule?, allViolations: [StyleViolation]) -> [StyleViolation] { - guard let superfluousDisableCommandRule else { - return [] - } - guard regions.isNotEmpty else { + guard regions.isNotEmpty, let superfluousDisableCommandRule else { return [] } - + let regionsDisablingSuperfluousDisableRule = regions.filter { region in region.isRuleDisabled(superfluousDisableCommandRule) } From 76f4609d0bc485d250fff64c394d4ed6acc4469b Mon Sep 17 00:00:00 2001 From: Martin Redington Date: Sun, 15 Sep 2024 21:56:16 +0100 Subject: [PATCH 6/7] Added custom rule test --- .../CustomRulesTests.swift | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/Tests/SwiftLintFrameworkTests/CustomRulesTests.swift b/Tests/SwiftLintFrameworkTests/CustomRulesTests.swift index 3337822047..a8adf00dca 100644 --- a/Tests/SwiftLintFrameworkTests/CustomRulesTests.swift +++ b/Tests/SwiftLintFrameworkTests/CustomRulesTests.swift @@ -427,6 +427,26 @@ final class CustomRulesTests: SwiftLintTestCase { XCTAssertTrue(try violations(forExample: example, customRules: customRules).isEmpty) } + func testNestedCustomRuleDoNotTriggerSuperfluousDisableCommand() 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) + } + // MARK: - Private private func getCustomRules(_ extraConfig: [String: Any] = [:]) -> (Configuration, CustomRules) { From 57511eecfbbf27ff95b941bb6432d4ed3b909068 Mon Sep 17 00:00:00 2001 From: Martin Redington Date: Sun, 15 Sep 2024 22:36:41 +0100 Subject: [PATCH 7/7] Added another test --- .../CustomRulesTests.swift | 30 ++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/Tests/SwiftLintFrameworkTests/CustomRulesTests.swift b/Tests/SwiftLintFrameworkTests/CustomRulesTests.swift index a8adf00dca..4fc5c142d8 100644 --- a/Tests/SwiftLintFrameworkTests/CustomRulesTests.swift +++ b/Tests/SwiftLintFrameworkTests/CustomRulesTests.swift @@ -427,7 +427,7 @@ final class CustomRulesTests: SwiftLintTestCase { XCTAssertTrue(try violations(forExample: example, customRules: customRules).isEmpty) } - func testNestedCustomRuleDoNotTriggerSuperfluousDisableCommand() throws { + func testNestedCustomRuleDisablesDoNotTriggerSuperfluousDisableCommand() throws { let customRules: [String: Any] = [ "rule1": [ "regex": "pattern1" @@ -447,6 +447,34 @@ final class CustomRulesTests: SwiftLintTestCase { 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")) + } + // MARK: - Private private func getCustomRules(_ extraConfig: [String: Any] = [:]) -> (Configuration, CustomRules) {