Add "No Fallthrough Only" Rule #2194
Conversation
|
Thanks for the contribution! I feel like this is a case where it'd be ok for this rule to be enabled by default rather than opt-in. It's reasonable for a style linter to be opinionated when there's no technical downside, such as is the case here. @dabelknap can you think of a reason why someone would not want this, other than personal stylistic preference? In other words, are there situations where code cannot be clearly expressed without resorting to a case statement comprising of only a single |
| """ | ||
| switch { | ||
| case 1: | ||
| fallthrough |
There was a problem hiding this comment.
can you add the violation marks (↓) to the examples?
There was a problem hiding this comment.
How are those marks used exactly? Do you place them at the leading or trailing edge of the violation?
There was a problem hiding this comment.
You should place them where the violation is reported. In this case, as an user, I'd expect the violation to be triggered on ↓fallthrough, but reading the code I believe it'll be triggered on ↓case 1:.
Adding the marker also makes the unit tests to verify that the violation is being reported on the expected offset.
| kind: .idiomatic, | ||
| nonTriggeringExamples: [ | ||
| """ | ||
| switch { |
There was a problem hiding this comment.
many of these examples have compiler warnings or errors. Can you refactor them to compile cleanly, as to not distract from what's actually relevant in this rule?
| name: "No Fallthrough Only", | ||
| description: "Fallthroughs can only be used if the `case` contains at least one other statement.", | ||
| kind: .idiomatic, | ||
| nonTriggeringExamples: [ |
There was a problem hiding this comment.
It'd be nice to have more complex test cases here, such as using pattern matching, or default or break statements.
| return [] | ||
| } | ||
|
|
||
| let pattern = "case[^:]+:\\s*" + // match start of case |
There was a problem hiding this comment.
This rule could be more robust if we avoided regular expressions altogether, and instead operated on the dictionary that's being passed as a parameter in this function.
There was a problem hiding this comment.
It isn't clear to me how to use the dictionary to accomplish this. I am able to get the length and offset for the entire case block, but it doesn't seem to provide any additional breakdown. Do you have a suggestion for how to approach this?
There was a problem hiding this comment.
Sorry I wasn't clear. You should take a look at how SwitchCaseAlignmentRule is implemented.
For example, see how it filters the dictionary for case statements in a type-safe & AST-safe manner:
There was a problem hiding this comment.
I'm not sure how this helps in my situation. The case statements exist as nodes on the dictionary, but it doesn't give me information about the individual tokens within the case statement. I tried grabbing the offset and length from the case node in the dictionary, and then used it to return the tokens in this range from the SyntaxMap. However, I can't think of an unambiguous way of filtering out the case keyword and any additional expressions before the colon.
Codecov Report
@@ Coverage Diff @@
## master #2194 +/- ##
==========================================
+ Coverage 92.08% 92.11% +0.02%
==========================================
Files 284 285 +1
Lines 14330 14381 +51
==========================================
+ Hits 13196 13247 +51
Misses 1134 1134
Continue to review full report at Codecov.
|
|
Maybe @marcelofabri can improve this even further, but here are some ideas of how we can rely less on regular expressions and more on the parsed tree from SourceKit to detect fallthrough-only case bodies: diff --git a/Source/SwiftLintFramework/Rules/NoFallthroughRuleOnlyRule.swift b/Source/SwiftLintFramework/Rules/NoFallthroughRuleOnlyRule.swift
index 1070702b..b555aa50 100644
--- a/Source/SwiftLintFramework/Rules/NoFallthroughRuleOnlyRule.swift
+++ b/Source/SwiftLintFramework/Rules/NoFallthroughRuleOnlyRule.swift
@@ -45,7 +45,7 @@ public struct NoFallthroughOnlyRule: ASTRule, OptInRule, ConfigurationProviderRu
"""
switch {
case 1:
- fallthrough
+ ↓fallthrough
case 2:
var a = 1
}
@@ -55,7 +55,7 @@ public struct NoFallthroughOnlyRule: ASTRule, OptInRule, ConfigurationProviderRu
case 1:
var a = 2
case 2:
- fallthrough
+ ↓fallthrough
case 3:
var a = 3
}
@@ -63,7 +63,7 @@ public struct NoFallthroughOnlyRule: ASTRule, OptInRule, ConfigurationProviderRu
"""
switch {
case 1: // comment
- fallthrough
+ ↓fallthrough
}
""",
"""
@@ -71,7 +71,7 @@ public struct NoFallthroughOnlyRule: ASTRule, OptInRule, ConfigurationProviderRu
case 1: /* multi
line
comment */
- fallthrough
+ ↓fallthrough
case 2:
var a = 2
}
@@ -83,27 +83,33 @@ public struct NoFallthroughOnlyRule: ASTRule, OptInRule, ConfigurationProviderRu
kind: StatementKind,
dictionary: [String: SourceKitRepresentable]) -> [StyleViolation] {
- guard kind == StatementKind.case else {
- return []
- }
-
- guard
+ guard kind == .case,
let length = dictionary.length,
- let offset = dictionary.offset
+ let offset = dictionary.offset,
+ case let nsstring = file.contents.bridge(),
+ let range = nsstring.byteRangeToNSRange(start: offset, length: length),
+ let colonLocation = file.match(pattern: ":", range: range).first?.0.location
else {
return []
}
- let pattern = "case[^:]+:\\s*" + // match start of case
- "((//.*\\n)|" + // match double-slash comments, or
- "(/\\*(.|\\n)*\\*/))*" + // match block comments (zero or more consecutive comments)
- "\\s*fallthrough" // look for fallthrough immediately following case and consecutive comments
- let range = file.contents.bridge().byteRangeToNSRange(start: offset, length: length)
+ let caseBodyRange = NSRange(location: colonLocation,
+ length: range.length + range.location - colonLocation)
+ let nonCommentCaseBody = file.match(pattern: "\\w+", range: caseBodyRange).filter { _, syntaxKinds in
+ return !Set(syntaxKinds).subtracting(SyntaxKind.commentKinds).isEmpty
+ }
+
+ guard nonCommentCaseBody.count == 1 else {
+ return []
+ }
- return file.match(pattern: pattern, range: range).map { nsRange, _ in
- StyleViolation(ruleDescription: type(of: self).description,
- severity: configuration.severity,
- location: Location(file: file, characterOffset: nsRange.location))
+ let nsRange = nonCommentCaseBody[0].0
+ if nsstring.substring(with: nsRange) == "fallthrough" && nonCommentCaseBody[0].1 == [.keyword] {
+ return [StyleViolation(ruleDescription: type(of: self).description,
+ severity: configuration.severity,
+ location: Location(file: file, characterOffset: nsRange.location))]
}
+
+ return []
}
}This also adds the violation markers ( |
|
Thanks for the code suggestion. I'm realizing that using the colon for delineating the case body might not work: My original solution would not have handled this properly either. |
|
Just checking in on this PR. Is there anything else that needs to be done to get this approved? |
|
@dabelknap Sorry for the delay. This looks good, but we need a changelog entry in the format described in CONTRIBUTING.md. |
|
@marcelofabri Do you have any idea why merging from |
|
We have a rule on Danger to avoid merge commits. Can you please rebase instead? |
dabelknap
force-pushed
the
no-fallthrough-only
branch
from
dc25cc3 to
e6517d9
Compare
|
@marcelofabri Yep, done. |
|
Thanks to both of you for driving this home! 💯 |
how about if a case should do the same as the default in which case there would only be a fallthrough statement as cases cannot be combined with the default? |
SwiftLint already has a "Fallthrough" rule, which can be used to prohibit the use of
fallthroughstatements in switches. At Google, we allow the use offallthroughstatements, but only if they are accompanied by other statements in thecase. In other words, acasemay not contain only afallthrough. The "No Fallthrough Only" rule enforces this usage offallthroughstatements.