Swiftlint: fallthrough false positive

[Sega-Zero]

New Issue Checklist

• [x] Updated SwiftLint to the latest version
• [x] I searched for existing GitHub issues

Bug Report

According to rule request, the primary goal of marking fallthrough statement is that it can be replaced by combining cases. In most cases.
However there are cases where that's not possible, reasonable or leading to code duplication:

1. when having optional statement to switch on
2. when having a default statement already

The mentioned linkedin styleguide also mentioned whenever possible, but not saying anything on avoidance.

Take a look at the sample below. I have a block of code that should run in any other cases rather than .needsRequest plus a small addition for .denied case.

To resolve the warning, I am forced to copy-paste the default section which is error prone (+ possibly may lead to function_body_length violation).
I think the rule should ignore falling through to default case. It's not that hard to read and a default case already marks the common code for a several case statements.

Environment

Swiftlint 0.23.0, installed through CocoaPods

identifier_name:
  excluded:
  - id
  - to
  - fic_UUID
  - fic_sourceImageUUID

excluded:
- Pods

• Are you using nested configurations?
  no
• Which Xcode version are you using (check xcode-select -p)?
  xcode 9
• Do you have a sample that shows the issue? Run echo "[string here]" | swiftlint lint --no-cache --use-stdin --enable-all-rules
  to quickly test if your example is really demonstrating the issue. If your example is more
  complex, you can use swiftlint lint --path [file here] --no-cache --enable-all-rules.

switch userLocation?.accessLevel {
case .needsRequest?:
    userLocation?.requestAccess()
case .denied?:
    if permissionChecksDone {
        UIApplication.shared.openURL(URL(string: UIApplicationOpenSettingsURLString)!)
    }
    // This triggers a violation:
    fallthrough
default:
    permissionChecksDone = true
    self.updateLocationPage()
}

[marcelofabri]

I'm not sure I'd consider this a false positive. You could avoid the fallthrough by creating a local closure for example. Anyway, you can always disable a rule locally with // swiftlint:disable.

[Sega-Zero]

Yeah, I know I can use a workaround with closure/function or disabling a rule. I can also make a several cases to remove the default case, but this removes code simplicity and readability IMO.
I'd like to use this rule, it's just one single case that probably should not produce a warning.

[marcelofabri]

I disagree: removing the default case prevents you from accidentally not considering a new case if it's ever introduced (see #131).

I still think the rule is behaving accordingly here. We could probably add a configuration for not warning when the next case is a default, but that would be disabled by default.

[Sega-Zero]

Exhaustive switch requirement is a very helpful feature, but very context-dependent IMHO. It's not always bad to have a default case, else apple wouldn't make it I think.

A configuration seems good alternative to me.

[Coeur]

Instead of a configuration for not warning when the next case is a default, I would prefer a configuration for not warning when the case isn't empty.

case .a:
    // warning in this config, as .a and .b factorable
    fallthrough
case .b:
    ...
case .c:
    doSomething
    // no warning in this config, as .c and .d not factorable
    fallthrough
case .d:
    ...

[Sega-Zero]

100% agree, that's a very sensible remark! This is the way that this rule should work, without any configuration

[weichsel]

+1

This rule is nice to detect combinable cases as described in the linked style guide - But this is a very narrow use case. It also triggers for this example from "The Swift Programming Language" where fallthrough is used to perform _additional_ work for a subset of (non-exhaustive) items:

let integerToDescribe = 5
var description = "The number \(integerToDescribe) is"
switch integerToDescribe {
case 2, 3, 5, 7, 11, 13, 17, 19:
    description += " a prime number, and also"
    fallthrough
default:
    description += " an integer."
}
print(description)
// Prints "The number 5 is a prime number, and also an integer."

The original Rule Request (#1834) suggested this rule as _opt-in_. Maybe this would be a better fit because currently it triggers for scenarios where fallthrough can be a helpful language feature.

[jpsim]

I agree with @weichsel.

[marcelofabri]

Closing this as the rule is now opt-in (#2024) and a new default rule was added (#2194)