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

Allow explicit allowlist mode for switch-case checking #36

Closed
FastNav opened this issue Mar 6, 2022 · 3 comments
Closed

Allow explicit allowlist mode for switch-case checking #36

FastNav opened this issue Mar 6, 2022 · 3 comments

Comments

@FastNav
Copy link
Contributor

FastNav commented Mar 6, 2022

Hello there. This package seems very useful for many use cases in our team's projects. However, in a significant number of cases, switch-case is used as a shorthand for something like enum == X || enum == Y || enum == Z, and in most of those cases I don't think exhaustiveness checking is warranted. On the other hand, it would be very useful for situations like enum to enum mappers.

Do you think it would be possible to define a separate mode of enforcement which only looks at explicitly labeled "exhaustive switch" blocks? e.g. a //exhaustive:enforce comment above switches + -enforcement-mode=explicit?

@nishanths
Copy link
Owner

Hi, thanks for the report. I think the proposal is a good one. But also a question:

Did you consider using the existing //exhasutive:ignore comment directive as an opt-out mechanism on the if-statement-like switch statements? I generally think it would be safer to use a mechanism (such as the //exhasutive:ignore comment) that requires opting out of checking individual switch statements, instead of using a mechanism that requires opting in to checking each switch statement. But perhaps this approach is not suitable in some codebases (because of the significant number of if-statement-like switch statements? could you share more insights, if any, on this topic?)

--

Besides that, if we want to go ahead with the proposal, please feel welcome to send a PR! And feel free to use the new comment syntax and the new flag name that you described in the issue.

If I'm going to work on it myself, I'm afraid I won't find time in the next 3 weeks. But I can definitely help review a change sooner.

@FastNav
Copy link
Contributor Author

FastNav commented Mar 7, 2022

Yeah, I considered that option. However, after looking at our codebase for a bit, I realize that the prevailing use case for switch statements on enums was to select one or two members. I would really like to adopt this tooling for certain areas of interest like enum mappers which are meant to be highly exhaustive, but it seems harder to convince its use when the opt-out becomes the heavy majority of cases. We also use a Bazel monorepo so it's not something that can be easily decided at the team level.

I think there's sort of a "know it when you see it" moment when it comes to uses that are meant to be exhaustive vs those that are not. Usually the not cases where exhaustiveness makes less sense look like boolean expressions like

switch enumVal {
    case ConditionFailed, TransactionCancelled:
        return true // is transaction error
}
return false // is something else

whereas cases where exhaustiveness makes sense are more like cases similar to implementing a sealed interface in other languages, e.g. Stringer implementation for an enum. I think in those cases, developers recognize a strong need for being able to cover every new enum added and would want a specific syntactic construct like Java's switch expressions.

I think the setting to allow default to allow exhaustiveness is almost good enough. The problem is that since Go doesn't have true enums, developers may still want to insert default in areas where they want to guard against random values. e.g.

type Enum int
const (
    EnumUnspecified Enum = iota
    EnumBad Enum
    EnumGood Enum
    EnumTerrible Enum
)
// ...
    switch enumValue {
        case EnumTerrible, EnumBad:
            isBad = true
        case EnumGood:
            isBad = false
        case EnumUnspecified:
            fallthrough
        default: // removes exhaustiveness check even though we want it but also want to handle
                 // some random value like Enum(30)
            return fmt.Errorf("Invalid enum value %d", enumValue")
    }

@FastNav
Copy link
Contributor Author

FastNav commented Mar 7, 2022

Also I'm happy to take this one on. It seems like a relatively simple ask.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants