diff --git a/comment.go b/comment.go index 5da2dd0..123e018 100644 --- a/comment.go +++ b/comment.go @@ -7,8 +7,10 @@ import ( ) const ( - ignoreComment = "//exhaustive:ignore" - enforceComment = "//exhaustive:enforce" + ignoreComment = "//exhaustive:ignore" + enforceComment = "//exhaustive:enforce" + ignoreDefaultCaseRequiredComment = "//exhaustive:ignore-default-case-required" + enforceDefaultCaseRequiredComment = "//exhaustive:enforce-default-case-required" ) func hasCommentPrefix(comments []*ast.CommentGroup, comment string) bool { diff --git a/exhaustive.go b/exhaustive.go index d67a60c..013ac47 100644 --- a/exhaustive.go +++ b/exhaustive.go @@ -19,6 +19,7 @@ func registerFlags() { Analyzer.Flags.BoolVar(&fExplicitExhaustiveMap, ExplicitExhaustiveMapFlag, false, `check map literal only if associated with "//exhaustive:enforce" comment`) Analyzer.Flags.BoolVar(&fCheckGenerated, CheckGeneratedFlag, false, "check generated files") Analyzer.Flags.BoolVar(&fDefaultSignifiesExhaustive, DefaultSignifiesExhaustiveFlag, false, "switch statement is unconditionally exhaustive if it has a default case") + Analyzer.Flags.BoolVar(&fDefaultCaseRequired, DefaultCaseRequiredFlag, false, "switch statement requires default case even if exhaustive") Analyzer.Flags.Var(&fIgnoreEnumMembers, IgnoreEnumMembersFlag, "ignore constants matching `regexp`") Analyzer.Flags.Var(&fIgnoreEnumTypes, IgnoreEnumTypesFlag, "ignore types matching `regexp`") Analyzer.Flags.BoolVar(&fPackageScopeOnly, PackageScopeOnlyFlag, false, "only discover enums declared in file-level blocks") @@ -36,6 +37,7 @@ const ( ExplicitExhaustiveMapFlag = "explicit-exhaustive-map" CheckGeneratedFlag = "check-generated" DefaultSignifiesExhaustiveFlag = "default-signifies-exhaustive" + DefaultCaseRequiredFlag = "default-case-required" IgnoreEnumMembersFlag = "ignore-enum-members" IgnoreEnumTypesFlag = "ignore-enum-types" PackageScopeOnlyFlag = "package-scope-only" @@ -52,6 +54,7 @@ var ( fExplicitExhaustiveMap bool fCheckGenerated bool fDefaultSignifiesExhaustive bool + fDefaultCaseRequired bool fIgnoreEnumMembers regexpFlag fIgnoreEnumTypes regexpFlag fPackageScopeOnly bool @@ -65,6 +68,7 @@ func resetFlags() { fExplicitExhaustiveMap = false fCheckGenerated = false fDefaultSignifiesExhaustive = false + fDefaultCaseRequired = false fIgnoreEnumMembers = regexpFlag{} fIgnoreEnumTypes = regexpFlag{} fPackageScopeOnly = false @@ -121,6 +125,7 @@ func run(pass *analysis.Pass) (interface{}, error) { conf := switchConfig{ explicit: fExplicitExhaustiveSwitch, defaultSignifiesExhaustive: fDefaultSignifiesExhaustive, + defaultCaseRequired: fDefaultCaseRequired, checkGenerated: fCheckGenerated, ignoreConstant: fIgnoreEnumMembers.re, ignoreType: fIgnoreEnumTypes.re, diff --git a/exhaustive_test.go b/exhaustive_test.go index 4aac51d..5fc343c 100644 --- a/exhaustive_test.go +++ b/exhaustive_test.go @@ -46,6 +46,10 @@ func TestExhaustive(t *testing.T) { runTest(t, "default-signifies-exhaustive/default-absent/...", func() { fDefaultSignifiesExhaustive = true }) runTest(t, "default-signifies-exhaustive/default-present/...", func() { fDefaultSignifiesExhaustive = true }) + // These tests exercise the default-case-required flag and its escape comment + runTest(t, "default-case-required/default-required/...", func() { fDefaultCaseRequired = true }) + runTest(t, "default-case-required/default-not-required/...", func() { fDefaultCaseRequired = false }) + // Tests for -ignore-enum-members and -ignore-enum-types flags. runTest(t, "ignore-pattern/...", func() { fIgnoreEnumMembers = regexpFlag{ diff --git a/switch.go b/switch.go index 000ef98..d235fbe 100644 --- a/switch.go +++ b/switch.go @@ -5,6 +5,7 @@ import ( "go/ast" "go/types" "regexp" + "strings" "golang.org/x/tools/go/analysis" ) @@ -44,6 +45,7 @@ const ( resultNoEnforceComment = "has no enforce comment" resultEnumMembersAccounted = "required enum members accounted for" resultDefaultCaseSuffices = "default case satisfies exhaustiveness" + resultMissingDefaultCase = "missing required default case" resultReportedDiagnostic = "reported diagnostic" resultEnumTypes = "invalid or empty composing enum types" ) @@ -52,11 +54,47 @@ const ( type switchConfig struct { explicit bool defaultSignifiesExhaustive bool + defaultCaseRequired bool checkGenerated bool ignoreConstant *regexp.Regexp // can be nil ignoreType *regexp.Regexp // can be nil } +// There are few possibilities, and often none, so we use a possibly-nil slice +func userDirectives(comments []*ast.CommentGroup) []string { + var directives []string + for _, c := range comments { + for _, cc := range c.List { + // The order matters here: we always want to check the longest first. + for _, d := range []string{ + enforceDefaultCaseRequiredComment, + ignoreDefaultCaseRequiredComment, + enforceComment, + ignoreComment, + } { + if strings.HasPrefix(cc.Text, d) { + directives = append(directives, d) + // The break here is important: once we associate a comment + // with a particular (longest-possible) directive, we don't want + // to map to another! + break + } + } + } + } + return directives +} + +// Can be replaced with slices.Contains with go1.21 +func directivesIncludes(directives []string, d string) bool { + for _, ud := range directives { + if ud == d { + return true + } + } + return false +} + // switchChecker returns a node visitor that checks exhaustiveness of // enum switch statements for the supplied pass, and reports // diagnostics. The node visitor expects only *ast.SwitchStmt nodes. @@ -80,17 +118,27 @@ func switchChecker(pass *analysis.Pass, cfg switchConfig, generated boolCache, c sw := n.(*ast.SwitchStmt) switchComments := comments.get(pass.Fset, file)[sw] - if !cfg.explicit && hasCommentPrefix(switchComments, ignoreComment) { + uDirectives := userDirectives(switchComments) + if !cfg.explicit && directivesIncludes(uDirectives, ignoreComment) { // Skip checking of this switch statement due to ignore // comment. Still return true because there may be nested // switch statements that are not to be ignored. return true, resultIgnoreComment } - if cfg.explicit && !hasCommentPrefix(switchComments, enforceComment) { + if cfg.explicit && !directivesIncludes(uDirectives, enforceComment) { // Skip checking of this switch statement due to missing // enforce comment. return true, resultNoEnforceComment } + requireDefaultCase := cfg.defaultCaseRequired + if directivesIncludes(uDirectives, ignoreDefaultCaseRequiredComment) { + requireDefaultCase = false + } + if directivesIncludes(uDirectives, enforceDefaultCaseRequiredComment) { + // We have "if" instead of "else if" here in case of conflicting ignore/enforce directives. + // In that case, because this is second, we will default to enforcing. + requireDefaultCase = true + } if sw.Tag == nil { return true, resultNoSwitchTag // never possible for valid Go program? @@ -114,13 +162,21 @@ func switchChecker(pass *analysis.Pass, cfg switchConfig, generated boolCache, c checkl.add(e.typ, e.members, pass.Pkg == e.typ.Pkg()) } - def := analyzeSwitchClauses(sw, pass.TypesInfo, checkl.found) + defaultCaseExists := analyzeSwitchClauses(sw, pass.TypesInfo, checkl.found) + if !defaultCaseExists && requireDefaultCase { + // Even if the switch explicitly enumerates all the + // enum values, the user has still required all switches + // to have a default case. We check this first to avoid + // early-outs + pass.Report(makeMissingDefaultDiagnostic(sw, dedupEnumTypes(toEnumTypes(es)))) + return true, resultMissingDefaultCase + } if len(checkl.remaining()) == 0 { // All enum members accounted for. // Nothing to report. return true, resultEnumMembersAccounted } - if def && cfg.defaultSignifiesExhaustive { + if defaultCaseExists && cfg.defaultSignifiesExhaustive { // Though enum members are not accounted for, the // existence of the default case signifies // exhaustiveness. So don't report. @@ -167,3 +223,14 @@ func makeSwitchDiagnostic(sw *ast.SwitchStmt, enumTypes []enumType, missing map[ ), } } + +func makeMissingDefaultDiagnostic(sw *ast.SwitchStmt, enumTypes []enumType) analysis.Diagnostic { + return analysis.Diagnostic{ + Pos: sw.Pos(), + End: sw.End(), + Message: fmt.Sprintf( + "missing default case in switch of type %s", + diagnosticEnumTypes(enumTypes), + ), + } +} diff --git a/testdata/src/default-case-required/default-not-required/default_not_required.go b/testdata/src/default-case-required/default-not-required/default_not_required.go new file mode 100644 index 0000000..03cd18c --- /dev/null +++ b/testdata/src/default-case-required/default-not-required/default_not_required.go @@ -0,0 +1,47 @@ +package notrequired + +import "default-case-required" + +func _a(t dcr.T) { + // No diagnostic because neither fDefaultCaseRequired is true + // nor the enforcement comment is present. + switch t { + case dcr.A: + case dcr.B: + } +} + +func _b(t dcr.T) { + //exhaustive:enforce-default-case-required this is a comment showing that we can turn it on for select switches + switch t { // want "^missing default case in switch of type dcr.T$" + case dcr.A: + case dcr.B: + } +} + +func _c(t dcr.T) { + //exhaustive:ignore-default-case-required this comment is discarded in facvor of the enforcement + //exhaustive:enforce-default-case-required this is a comment showing that we can turn it on for select switches + switch t { // want "^missing default case in switch of type dcr.T$" + case dcr.A: + case dcr.B: + } +} + +func _d(t dcr.T) { + //exhaustive:enforce-default-case-required this is a comment showing that we can turn it on for select switches + //exhaustive:ignore-default-case-required this comment is discarded in facvor of the enforcement + switch t { // want "^missing default case in switch of type dcr.T$" + case dcr.A: + case dcr.B: + } +} + +func _e(t dcr.T) { + //exhaustive:enforce-default-case-required this is happy because it has a default + switch t { + case dcr.A: + case dcr.B: + default: + } +} diff --git a/testdata/src/default-case-required/default-required/default_required.go b/testdata/src/default-case-required/default-required/default_required.go new file mode 100644 index 0000000..e196f40 --- /dev/null +++ b/testdata/src/default-case-required/default-required/default_required.go @@ -0,0 +1,37 @@ +package required + +import "default-case-required" + +func _a(t dcr.T) { + // expect a diagnostic when fDefaultCaseRequired is true. + switch t { // want "^missing default case in switch of type dcr.T$" + case dcr.A: + case dcr.B: + } +} + +func _b(t dcr.T) { + //exhaustive:ignore-default-case-required this is a comment showing that we can turn it off for select switches + switch t { + case dcr.A: + case dcr.B: + } +} + +func _c(t dcr.T) { + //exhaustive:ignore-default-case-required this comment is discarded in facvor of the enforcement + //exhaustive:enforce-default-case-required this helps override the above + switch t { // want "^missing default case in switch of type dcr.T$" + case dcr.A: + case dcr.B: + } +} + +func _d(t dcr.T) { + // this is happy even with enforcement because we have a default + switch t { + case dcr.A: + case dcr.B: + default: + } +} diff --git a/testdata/src/default-case-required/default_case_required.go b/testdata/src/default-case-required/default_case_required.go new file mode 100644 index 0000000..afae9c6 --- /dev/null +++ b/testdata/src/default-case-required/default_case_required.go @@ -0,0 +1,8 @@ +package dcr + +type T int + +const ( + A T = iota + B +)