Skip to content

Commit a72231e

Browse files
allevatoallevato
committed
Fix rule enabling/disabling, both in config and in source comments.
RuleMask previously only kept track of rule disablement, assuming that any range where a rule was not disabled should have it enabled. This isn't quite correct; it's actually a tri-state of enabled, disabled, and whatever-is-in-the-configuration. With these changes, rule enablement/disablement is now handled in the lint pipeline (format pipeline coming later), not in individual rules. (This will actually be refined later, because some individual rules do need finer control over that check.) Fixes SR-11114.
1 parent c96c9c0 commit a72231e

File tree

9 files changed

+134
-80
lines changed

9 files changed

+134
-80
lines changed

‎Sources/SwiftFormat/LintPipeline.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ struct LintPipeline: SyntaxVisitor {
3838
func visitIfEnabled<Rule: SyntaxLintRule, Node: Syntax>(
3939
_ visitor: (Rule) -> (Node) -> SyntaxVisitorContinueKind, in context: Context, for node: Node
4040
) {
41-
guard !context.isRuleDisabled(Rule.self.ruleName, node: node) else { return }
41+
guard context.isRuleEnabled(Rule.self.ruleName, node: node) else { return }
4242
let rule = Rule(context: context)
4343
_ = visitor(rule)(node)
4444
}
@@ -59,7 +59,7 @@ struct LintPipeline: SyntaxVisitor {
5959
// more importantly because the `visit` methods return protocol refinements of `Syntax` that
6060
// cannot currently be expressed as constraints without duplicating this function for each of
6161
// them individually.
62-
guard !context.isRuleDisabled(Rule.self.ruleName, node: node) else { return }
62+
guard context.isRuleEnabled(Rule.self.ruleName, node: node) else { return }
6363
let rule = Rule(context: context)
6464
_ = visitor(rule)(node)
6565
}

‎Sources/SwiftFormatCore/Context.swift

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,9 +73,16 @@ public class Context {
7373

7474
/// Given a rule's name and the node it is examining, determine if the rule is disabled at this
7575
/// location or not.
76-
public func isRuleDisabled(_ ruleName: String, node: Syntax) -> Bool {
76+
public func isRuleEnabled(_ ruleName: String, node: Syntax) -> Bool {
7777
let loc = node.startLocation(converter: self.sourceLocationConverter)
7878
guard let line = loc.line else { return false }
79-
return self.ruleMask.isDisabled(ruleName, line: line)
79+
switch ruleMask.ruleState(ruleName, atLine: line) {
80+
case .default:
81+
return configuration.rules[ruleName] ?? false
82+
case .disabled:
83+
return false
84+
case .enabled:
85+
return true
86+
}
8087
}
8188
}

‎Sources/SwiftFormatCore/RuleMask.swift

Lines changed: 61 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,19 @@ import SwiftSyntax
2424
/// RuleMask to see if it is disabled for the line it is currently examining.
2525
public class RuleMask {
2626

27-
/// Each rule has a list of ranges for which it is disabled.
28-
private var ruleMap: [String: [Range<Int>]] = [:]
27+
/// Information about whether a particular lint/format rule is enabled or disabled for a range of
28+
/// lines in the source file.
29+
private struct LineRangeState {
30+
31+
/// The line range where a rule is either disabled or enabled.
32+
var range: Range<Int>
33+
34+
/// Indicates whether the rule is enabled in this range.
35+
var isEnabled: Bool
36+
}
37+
38+
/// Each rule has a list of ranges for which it is explicitly enabled or disabled.
39+
private var ruleMap: [String: [LineRangeState]] = [:]
2940

3041
/// Regex to match the enable comments; rule name is in the first capture group.
3142
private let enablePattern = #"^\s*//\s*swift-format-enable:\s+(\S+)"#
@@ -58,8 +69,9 @@ public class RuleMask {
5869
return loc.line
5970
}
6071

61-
/// Check if a comment matches a disable/enable flag.
72+
/// Check if a comment matches a disable/enable flag, and if so, returns the name of the rule.
6273
private func getRule(regex: NSRegularExpression, text: String) -> String? {
74+
// TODO: Support multiple rules in the same comment; e.g., a comma-delimited list.
6375
let nsrange = NSRange(text.startIndex..<text.endIndex, in: text)
6476
if let match = regex.firstMatch(in: text, options: [], range: nsrange) {
6577
let matchRange = match.range(at: 1)
@@ -73,47 +85,73 @@ public class RuleMask {
7385
/// Generate the dictionary (ruleMap) by walking the syntax tokens.
7486
private func generateDictionary(_ node: Syntax) {
7587
var disableStart: [String: Int] = [:]
88+
var enableStart: [String: Int] = [:]
89+
7690
for token in node.tokens {
77-
guard let leadingtrivia = token.leadingTrivia else { continue }
91+
guard let leadingTrivia = token.leadingTrivia else { continue }
7892

7993
// Flags must be on lines by themselves: not at the end of an existing line.
8094
var firstPiece = true
8195

82-
for piece in leadingtrivia {
96+
for piece in leadingTrivia {
8397
guard case .lineComment(let text) = piece else {
8498
firstPiece = false
8599
continue
86100
}
87101
guard !firstPiece else { continue }
88102

89-
if let disableRule = getRule(regex: disableRegex, text: text) {
90-
guard !disableStart.keys.contains(disableRule) else { continue }
91-
guard let startLine = getLine(token) else { continue }
92-
disableStart[disableRule] = startLine
93-
}
103+
if let ruleName = getRule(regex: disableRegex, text: text) {
104+
guard !disableStart.keys.contains(ruleName) else { continue }
105+
guard let disableStartLine = getLine(token) else { continue }
94106

95-
if let enableRule = getRule(regex: enableRegex, text: text) {
96-
guard let startLine = disableStart.removeValue(forKey: enableRule) else { continue }
97-
guard let endLine = getLine(token) else { continue }
98-
let exclusionRange = startLine..<endLine
107+
if let enableStartLine = enableStart[ruleName] {
108+
// If we're processing an enable block for the rule, finalize it.
109+
ruleMap[ruleName, default: []].append(
110+
LineRangeState(range: enableStartLine..<disableStartLine, isEnabled: true))
111+
enableStart[ruleName] = nil
112+
}
99113

100-
if ruleMap.keys.contains(enableRule) {
101-
ruleMap[enableRule]?.append(exclusionRange)
102-
} else {
103-
ruleMap[enableRule] = [exclusionRange]
114+
disableStart[ruleName] = disableStartLine
115+
}
116+
else if let ruleName = getRule(regex: enableRegex, text: text) {
117+
guard !enableStart.keys.contains(ruleName) else { continue }
118+
guard let enableStartLine = getLine(token) else { continue }
119+
120+
if let disableStartLine = disableStart[ruleName] {
121+
// If we're processing a disable block for the rule, finalize it.
122+
ruleMap[ruleName, default: []].append(
123+
LineRangeState(range: disableStartLine..<enableStartLine, isEnabled: false))
124+
disableStart[ruleName] = nil
104125
}
126+
127+
enableStart[ruleName] = enableStartLine
105128
}
129+
106130
firstPiece = false
107131
}
108132
}
133+
134+
// Finalize any remaining blocks by closing them off at the last line number in the file.
135+
guard let lastToken = node.lastToken, let lastLine = getLine(lastToken) else { return }
136+
137+
for (ruleName, disableStartLine) in disableStart {
138+
ruleMap[ruleName, default: []].append(
139+
LineRangeState(range: disableStartLine..<lastLine + 1, isEnabled: false))
140+
}
141+
for (ruleName, enableStartLine) in enableStart {
142+
ruleMap[ruleName, default: []].append(
143+
LineRangeState(range: enableStartLine..<lastLine + 1, isEnabled: true))
144+
}
109145
}
110146

111147
/// Return if the given rule is disabled on the provided line.
112-
public func isDisabled(_ rule: String, line: Int) -> Bool {
113-
guard let ranges = ruleMap[rule] else { return false }
114-
for range in ranges {
115-
if range.contains(line) { return true }
148+
public func ruleState(_ rule: String, atLine line: Int) -> RuleState {
149+
guard let rangeStates = ruleMap[rule] else { return .default }
150+
for rangeState in rangeStates {
151+
if rangeState.range.contains(line) {
152+
return rangeState.isEnabled ? .enabled : .disabled
153+
}
116154
}
117-
return false
155+
return .default
118156
}
119157
}

‎Sources/SwiftFormatCore/RuleState.swift

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
//===----------------------------------------------------------------------===//
2+
//
3+
// This source file is part of the Swift.org open source project
4+
//
5+
// Copyright (c) 2014 - 2019 Apple Inc. and the Swift project authors
6+
// Licensed under Apache License v2.0 with Runtime Library Exception
7+
//
8+
// See https://swift.org/LICENSE.txt for license information
9+
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
10+
//
11+
//===----------------------------------------------------------------------===//
12+
13+
/// The enablement of a lint/format rule based on the presence or absence of comment directives in
14+
/// the source file.
15+
public enum RuleState {
16+
17+
/// There is no explicit information in the source file about whether the rule should be enabled
18+
/// or disabled at the requested location, so the configuration default should be used.
19+
case `default`
20+
21+
/// The rule is explicitly enabled at the requested location.
22+
case enabled
23+
24+
/// The rule is explicitly disabled at the requested location.
25+
case disabled
26+
}

‎Sources/SwiftFormatRules/OrderedImports.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ public final class OrderedImports: SyntaxFormatRule {
6464
var lineType = line.type
6565

6666
// Set the line type to codeBlock if the rule is disabled.
67-
if let codeblock = line.codeBlock, context.isRuleDisabled(Self.ruleName, node: codeblock) {
67+
if let codeblock = line.codeBlock, !context.isRuleEnabled(Self.ruleName, node: codeblock) {
6868
switch lineType {
6969
case .comment: ()
7070
default:
@@ -130,7 +130,7 @@ public final class OrderedImports: SyntaxFormatRule {
130130
var lineType = line.type
131131

132132
// Set the line type to codeBlock if the rule is disabled.
133-
if let codeblock = line.codeBlock, context.isRuleDisabled(Self.ruleName, node: codeblock) {
133+
if let codeblock = line.codeBlock, !context.isRuleEnabled(Self.ruleName, node: codeblock) {
134134
switch lineType {
135135
case .comment: ()
136136
default:

‎Tests/SwiftFormatCoreTests/RuleMaskTests.swift

Lines changed: 33 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,9 @@ public class RuleMaskTests: XCTestCase {
2424

2525
let mask = createMask(sourceText: text)
2626

27-
XCTAssertFalse(mask.isDisabled("rule1", line: 1))
28-
XCTAssertTrue(mask.isDisabled("rule1", line: 3))
29-
XCTAssertFalse(mask.isDisabled("rule1", line: 5))
27+
XCTAssertEqual(mask.ruleState("rule1", atLine: 1), .default)
28+
XCTAssertEqual(mask.ruleState("rule1", atLine: 3), .disabled)
29+
XCTAssertEqual(mask.ruleState("rule1", atLine: 5), .enabled)
3030
}
3131

3232
public func testTwoRules() {
@@ -45,20 +45,20 @@ public class RuleMaskTests: XCTestCase {
4545

4646
let mask = createMask(sourceText: text)
4747

48-
XCTAssertFalse(mask.isDisabled("rule1", line: 1))
49-
XCTAssertTrue(mask.isDisabled("rule1", line: 3))
50-
XCTAssertTrue(mask.isDisabled("rule1", line: 5))
51-
XCTAssertFalse(mask.isDisabled("rule1", line: 7))
52-
XCTAssertFalse(mask.isDisabled("rule1", line: 9))
53-
54-
XCTAssertFalse(mask.isDisabled("rule2", line: 1))
55-
XCTAssertFalse(mask.isDisabled("rule2", line: 3))
56-
XCTAssertTrue(mask.isDisabled("rule2", line: 5))
57-
XCTAssertTrue(mask.isDisabled("rule2", line: 7))
58-
XCTAssertFalse(mask.isDisabled("rule2", line: 9))
48+
XCTAssertEqual(mask.ruleState("rule1", atLine: 1), .default)
49+
XCTAssertEqual(mask.ruleState("rule1", atLine: 3), .disabled)
50+
XCTAssertEqual(mask.ruleState("rule1", atLine: 5), .disabled)
51+
XCTAssertEqual(mask.ruleState("rule1", atLine: 7), .enabled)
52+
XCTAssertEqual(mask.ruleState("rule1", atLine: 9), .enabled)
53+
54+
XCTAssertEqual(mask.ruleState("rule2", atLine: 1), .default)
55+
XCTAssertEqual(mask.ruleState("rule2", atLine: 3), .default)
56+
XCTAssertEqual(mask.ruleState("rule2", atLine: 5), .disabled)
57+
XCTAssertEqual(mask.ruleState("rule2", atLine: 7), .disabled)
58+
XCTAssertEqual(mask.ruleState("rule2", atLine: 9), .enabled)
5959
}
6060

61-
public func testWrongOrderFlags() {
61+
public func testEnableBeforeDisable() {
6262
let text =
6363
"""
6464
let a = 123
@@ -70,9 +70,9 @@ public class RuleMaskTests: XCTestCase {
7070

7171
let mask = createMask(sourceText: text)
7272

73-
XCTAssertFalse(mask.isDisabled("rule1", line: 1))
74-
XCTAssertFalse(mask.isDisabled("rule1", line: 3))
75-
XCTAssertFalse(mask.isDisabled("rule1", line: 5))
73+
XCTAssertEqual(mask.ruleState("rule1", atLine: 1), .default)
74+
XCTAssertEqual(mask.ruleState("rule1", atLine: 3), .enabled)
75+
XCTAssertEqual(mask.ruleState("rule1", atLine: 5), .disabled)
7676
}
7777

7878
public func testDuplicateNested() {
@@ -91,11 +91,11 @@ public class RuleMaskTests: XCTestCase {
9191

9292
let mask = createMask(sourceText: text)
9393

94-
XCTAssertFalse(mask.isDisabled("rule1", line: 1))
95-
XCTAssertTrue(mask.isDisabled("rule1", line: 3))
96-
XCTAssertTrue(mask.isDisabled("rule1", line: 5))
97-
XCTAssertFalse(mask.isDisabled("rule1", line: 7))
98-
XCTAssertFalse(mask.isDisabled("rule1", line: 9))
94+
XCTAssertEqual(mask.ruleState("rule1", atLine: 1), .default)
95+
XCTAssertEqual(mask.ruleState("rule1", atLine: 3), .disabled)
96+
XCTAssertEqual(mask.ruleState("rule1", atLine: 5), .disabled)
97+
XCTAssertEqual(mask.ruleState("rule1", atLine: 7), .enabled)
98+
XCTAssertEqual(mask.ruleState("rule1", atLine: 9), .enabled)
9999
}
100100

101101
public func testSingleFlags() {
@@ -110,8 +110,8 @@ public class RuleMaskTests: XCTestCase {
110110

111111
let mask1 = createMask(sourceText: text1)
112112

113-
XCTAssertFalse(mask1.isDisabled("rule1", line: 1))
114-
XCTAssertFalse(mask1.isDisabled("rule1", line: 6))
113+
XCTAssertEqual(mask1.ruleState("rule1", atLine: 1), .default)
114+
XCTAssertEqual(mask1.ruleState("rule1", atLine: 5), .disabled)
115115

116116
let text2 =
117117
"""
@@ -124,8 +124,8 @@ public class RuleMaskTests: XCTestCase {
124124

125125
let mask2 = createMask(sourceText: text2)
126126

127-
XCTAssertFalse(mask2.isDisabled("rule1", line: 1))
128-
XCTAssertFalse(mask2.isDisabled("rule1", line: 6))
127+
XCTAssertEqual(mask2.ruleState("rule1", atLine: 1), .default)
128+
XCTAssertEqual(mask2.ruleState("rule1", atLine: 5), .enabled)
129129
}
130130

131131
public func testSpuriousFlags() {
@@ -140,9 +140,9 @@ public class RuleMaskTests: XCTestCase {
140140

141141
let mask1 = createMask(sourceText: text1)
142142

143-
XCTAssertFalse(mask1.isDisabled("rule1", line: 1))
144-
XCTAssertFalse(mask1.isDisabled("rule1", line: 3))
145-
XCTAssertFalse(mask1.isDisabled("rule1", line: 5))
143+
XCTAssertEqual(mask1.ruleState("rule1", atLine: 1), .default)
144+
XCTAssertEqual(mask1.ruleState("rule1", atLine: 3), .default)
145+
XCTAssertEqual(mask1.ruleState("rule1", atLine: 5), .enabled)
146146

147147
let text2 =
148148
#"""
@@ -158,8 +158,8 @@ public class RuleMaskTests: XCTestCase {
158158

159159
let mask2 = createMask(sourceText: text2)
160160

161-
XCTAssertFalse(mask2.isDisabled("rule1", line: 1))
162-
XCTAssertFalse(mask2.isDisabled("rule1", line: 6))
163-
XCTAssertFalse(mask2.isDisabled("rule1", line: 8))
161+
XCTAssertEqual(mask2.ruleState("rule1", atLine: 1), .default)
162+
XCTAssertEqual(mask2.ruleState("rule1", atLine: 6), .default)
163+
XCTAssertEqual(mask2.ruleState("rule1", atLine: 8), .enabled)
164164
}
165165
}

‎Tests/SwiftFormatCoreTests/XCTestManifests.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,11 @@ extension RuleMaskTests {
77
// to regenerate.
88
static let __allTests__RuleMaskTests = [
99
("testDuplicateNested", testDuplicateNested),
10+
("testEnableBeforeDisable", testEnableBeforeDisable),
1011
("testSingleFlags", testSingleFlags),
1112
("testSingleRule", testSingleRule),
1213
("testSpuriousFlags", testSpuriousFlags),
1314
("testTwoRules", testTwoRules),
14-
("testWrongOrderFlags", testWrongOrderFlags),
1515
]
1616
}
1717

‎Tests/SwiftFormatRulesTests/NeverUseForceTryTests.swift

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -37,20 +37,4 @@ public class NeverUseForceTryTests: DiagnosingTestCase {
3737
performLint(NeverUseForceTry.self, input: input)
3838
XCTAssertNotDiagnosed(.doNotForceTry)
3939
}
40-
41-
public func testDisableForceTry() {
42-
let input =
43-
"""
44-
let a = 123
45-
46-
// swift-format-disable: NeverUseForceTry
47-
let document = try! Document(path: "important.data")
48-
let sheet = try! Paper()
49-
// swift-format-enable: NeverUseForceTry
50-
51-
let b = 456
52-
"""
53-
performLint(NeverUseForceTry.self, input: input)
54-
XCTAssertNotDiagnosed(.doNotForceTry)
55-
}
5640
}

‎Tests/SwiftFormatRulesTests/XCTestManifests.swift

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,6 @@ extension NeverUseForceTryTests {
137137
// to regenerate.
138138
static let __allTests__NeverUseForceTryTests = [
139139
("testAllowForceTryInTestCode", testAllowForceTryInTestCode),
140-
("testDisableForceTry", testDisableForceTry),
141140
("testInvalidTryExpression", testInvalidTryExpression),
142141
]
143142
}

0 commit comments

Comments
 (0)