Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix nested disable commands #5796

Closed
wants to merge 7 commits into from
Closed

Fix nested disable commands #5796

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cc6360b
Try remapping the regions
mildm8nnered Sep 15, 2024
5e9257b
Added more tests and fixes
mildm8nnered Sep 15, 2024
a54d56e
Added one more test
mildm8nnered Sep 15, 2024
07a86ec
Tidyup
mildm8nnered Sep 15, 2024
f31f85e
Cleanup
mildm8nnered Sep 15, 2024
76f4609
Added custom rule test
mildm8nnered Sep 15, 2024
57511ee
Added another test
mildm8nnered Sep 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions Source/SwiftLintCore/Extensions/SwiftLintFile+Regex.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,45 @@ extension SwiftLintFile {
return regions
}

public func remap(regions: [Region]) -> [Region] {
guard regions.isNotEmpty else {
return []
}

var remappedRegions = [Region]()
var startMap: [RuleIdentifier: Location] = [:]
var lastRegionEnd: Location?

for region in regions {
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
}
}
for ruleIdentifier in region.disabledRuleIdentifiers where startMap[ruleIdentifier] == nil {
startMap[ruleIdentifier] = region.start
}
if region.disabledRuleIdentifiers.isEmpty {
remappedRegions.append(region)
}
lastRegionEnd = region.end
}

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
}
}

return remappedRegions
}

public func commands(in range: NSRange? = nil) -> [Command] {
guard let range else {
return commands
Expand Down
5 changes: 3 additions & 2 deletions Source/SwiftLintCore/Models/Linter.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ private extension Rule {
guard regions.isNotEmpty, let superfluousDisableCommandRule else {
return []
}

let regionsDisablingSuperfluousDisableRule = regions.filter { region in
region.isRuleDisabled(superfluousDisableCommandRule)
}
Expand Down Expand Up @@ -121,8 +121,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
)
Expand Down
8 changes: 7 additions & 1 deletion Source/SwiftLintCore/Models/RuleIdentifier.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
}
}
48 changes: 48 additions & 0 deletions Tests/SwiftLintFrameworkTests/CustomRulesTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,54 @@ 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"))
}

// MARK: - Private

private func getCustomRules(_ extraConfig: [String: Any] = [:]) -> (Configuration, CustomRules) {
Expand Down
94 changes: 88 additions & 6 deletions Tests/SwiftLintFrameworkTests/RegionTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,56 +20,79 @@ 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)
}
}

func testRegionsFromMatchingPairCommands() {
// 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"]),
Region(start: Location(file: nil, line: 2, character: 28),
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: []),
Region(start: Location(file: nil, line: 2, character: 29),
end: Location(file: nil, line: .max, character: .max),
disabledRuleIdentifiers: ["rule_id"]),
])
XCTAssertEqual(file.remap(regions: fileRegions), fileRegions)
}
}

func testRegionsFromThreeCommandForSingleLine() {
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"]),
Region(start: Location(file: nil, line: 2, character: .max),
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() {
Expand All @@ -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"]),
Expand All @@ -99,5 +123,63 @@ 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: []),
])
}

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: []),
])
}
}
Loading