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.

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

fallthrough false positive #1892

Closed
2 tasks done
Sega-Zero opened this issue Oct 7, 2017 · 9 comments
Closed
2 tasks done

fallthrough false positive #1892

Sega-Zero opened this issue Oct 7, 2017 · 9 comments
Labels
question Question or doubts that needs discussion and clarification. Can become a bug or proposal.

Comments

@Sega-Zero
Copy link
Contributor

New Issue Checklist

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
Copy link
Collaborator

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.

@marcelofabri marcelofabri added the question Question or doubts that needs discussion and clarification. Can become a bug or proposal. label Oct 7, 2017
@Sega-Zero
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link

Coeur commented Oct 13, 2017

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
Copy link
Contributor Author

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

@weichsel
Copy link

+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
Copy link
Collaborator

jpsim commented Dec 29, 2017

I agree with @weichsel.

@marcelofabri
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Question or doubts that needs discussion and clarification. Can become a bug or proposal.
Projects
None yet
Development

No branches or pull requests

5 participants