From 9c15b5a9ce4a06c603da4a56ac4df9f32913c7b6 Mon Sep 17 00:00:00 2001 From: Aaron Gorenstein Date: Mon, 6 Nov 2023 15:48:18 +0000 Subject: [PATCH 01/10] Add new linter to require default case --- exhaustive.go | 5 +++++ switch.go | 25 +++++++++++++++++++++++-- 2 files changed, 28 insertions(+), 2 deletions(-) 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/switch.go b/switch.go index 000ef98..d4c5288 100644 --- a/switch.go +++ b/switch.go @@ -44,6 +44,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,6 +53,7 @@ const ( type switchConfig struct { explicit bool defaultSignifiesExhaustive bool + defaultCaseRequired bool checkGenerated bool ignoreConstant *regexp.Regexp // can be nil ignoreType *regexp.Regexp // can be nil @@ -114,13 +116,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 && cfg.defaultCaseRequired { + // 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 +177,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 in switch over type %s", + diagnosticEnumTypes(enumTypes), + ), + } +} From 15a3f065f353ba3949df129b7f767a4e198760a8 Mon Sep 17 00:00:00 2001 From: Aaron Gorenstein Date: Tue, 7 Nov 2023 15:13:47 +0000 Subject: [PATCH 02/10] Added escape comment. --- comment.go | 5 +++-- switch.go | 15 +++++++++------ 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/comment.go b/comment.go index cc84bea..020744d 100644 --- a/comment.go +++ b/comment.go @@ -40,8 +40,9 @@ func isGeneratedFile(file *ast.File) bool { } const ( - ignoreComment = "//exhaustive:ignore" - enforceComment = "//exhaustive:enforce" + ignoreComment = "//exhaustive:ignore" + enforceComment = "//exhaustive:enforce" + defaultRequireIgnoreComment = "//exhaustive:default-require-ignore" ) func hasCommentPrefix(comments []*ast.CommentGroup, comment string) bool { diff --git a/switch.go b/switch.go index d4c5288..8eaf8c7 100644 --- a/switch.go +++ b/switch.go @@ -118,12 +118,15 @@ func switchChecker(pass *analysis.Pass, cfg switchConfig, generated boolCache, c defaultCaseExists := analyzeSwitchClauses(sw, pass.TypesInfo, checkl.found) if !defaultCaseExists && cfg.defaultCaseRequired { - // 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 + // Don't enforce this if the user opted-out + if !hasCommentPrefix(switchComments, defaultRequireIgnoreComment) { + // 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. From bc6a1830f6622e9294c7b394c909aad066bf1c11 Mon Sep 17 00:00:00 2001 From: Aaron Gorenstein Date: Tue, 7 Nov 2023 15:29:32 +0000 Subject: [PATCH 03/10] Added test --- exhaustive_test.go | 3 +++ switch.go | 2 +- .../default-required/default_required.go | 19 +++++++++++++++++++ 3 files changed, 23 insertions(+), 1 deletion(-) create mode 100644 testdata/src/default-signifies-exhaustive/default-required/default_required.go diff --git a/exhaustive_test.go b/exhaustive_test.go index 4aac51d..7634d23 100644 --- a/exhaustive_test.go +++ b/exhaustive_test.go @@ -46,6 +46,9 @@ 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 }) + // This tests exercises the default-case-required flag and its escape comment + runTest(t, "default-signifies-exhaustive/default-required/...", func() { fDefaultCaseRequired = true }) + // 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 8eaf8c7..aaa2b7a 100644 --- a/switch.go +++ b/switch.go @@ -186,7 +186,7 @@ func makeMissingDefaultDiagnostic(sw *ast.SwitchStmt, enumTypes []enumType) anal Pos: sw.Pos(), End: sw.End(), Message: fmt.Sprintf( - "missing default in switch over type %s", + "missing default in switch of type %s", diagnosticEnumTypes(enumTypes), ), } diff --git a/testdata/src/default-signifies-exhaustive/default-required/default_required.go b/testdata/src/default-signifies-exhaustive/default-required/default_required.go new file mode 100644 index 0000000..efbd0cc --- /dev/null +++ b/testdata/src/default-signifies-exhaustive/default-required/default_required.go @@ -0,0 +1,19 @@ +package present + +import "default-signifies-exhaustive" + +func _a(t dse.T) { + // expect a diagnostic when fDefaultCaseRequired is true. + switch t { // want "^missing default in switch of type dse.T$" + case dse.A: + case dse.B: + } +} + +func _b(t dse.T) { + //exhaustive:default-require-ignore this is a comment showing that we can turn it off for select switches + switch t { + case dse.A: + case dse.B: + } +} From c728146ae960a48e5a20c02ab88bfe8b19a5a43e Mon Sep 17 00:00:00 2001 From: Aaron Gorenstein Date: Tue, 7 Nov 2023 15:43:41 +0000 Subject: [PATCH 04/10] Added the selective-enforce and tests --- comment.go | 7 +++-- exhaustive_test.go | 1 + switch.go | 23 ++++++++------- .../default_not_required.go | 28 +++++++++++++++++++ 4 files changed, 46 insertions(+), 13 deletions(-) create mode 100644 testdata/src/default-signifies-exhaustive/default-not-required/default_not_required.go diff --git a/comment.go b/comment.go index 020744d..0f641ee 100644 --- a/comment.go +++ b/comment.go @@ -40,9 +40,10 @@ func isGeneratedFile(file *ast.File) bool { } const ( - ignoreComment = "//exhaustive:ignore" - enforceComment = "//exhaustive:enforce" - defaultRequireIgnoreComment = "//exhaustive:default-require-ignore" + ignoreComment = "//exhaustive:ignore" + enforceComment = "//exhaustive:enforce" + defaultRequireIgnoreComment = "//exhaustive:default-require-ignore" + defaultRequireEnforceComment = "//exhaustive:default-require-enforce" ) func hasCommentPrefix(comments []*ast.CommentGroup, comment string) bool { diff --git a/exhaustive_test.go b/exhaustive_test.go index 7634d23..e917d42 100644 --- a/exhaustive_test.go +++ b/exhaustive_test.go @@ -48,6 +48,7 @@ func TestExhaustive(t *testing.T) { // This tests exercises the default-case-required flag and its escape comment runTest(t, "default-signifies-exhaustive/default-required/...", func() { fDefaultCaseRequired = true }) + runTest(t, "default-signifies-exhaustive/default-not-required/...", func() { fDefaultCaseRequired = false }) // Tests for -ignore-enum-members and -ignore-enum-types flags. runTest(t, "ignore-pattern/...", func() { diff --git a/switch.go b/switch.go index aaa2b7a..a037069 100644 --- a/switch.go +++ b/switch.go @@ -93,6 +93,12 @@ func switchChecker(pass *analysis.Pass, cfg switchConfig, generated boolCache, c // enforce comment. return true, resultNoEnforceComment } + requireDefaultCase := cfg.defaultCaseRequired + if hasCommentPrefix(switchComments, defaultRequireIgnoreComment) { + requireDefaultCase = false + } else if hasCommentPrefix(switchComments, defaultRequireEnforceComment) { + requireDefaultCase = true + } if sw.Tag == nil { return true, resultNoSwitchTag // never possible for valid Go program? @@ -117,16 +123,13 @@ func switchChecker(pass *analysis.Pass, cfg switchConfig, generated boolCache, c } defaultCaseExists := analyzeSwitchClauses(sw, pass.TypesInfo, checkl.found) - if !defaultCaseExists && cfg.defaultCaseRequired { - // Don't enforce this if the user opted-out - if !hasCommentPrefix(switchComments, defaultRequireIgnoreComment) { - // 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 !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. diff --git a/testdata/src/default-signifies-exhaustive/default-not-required/default_not_required.go b/testdata/src/default-signifies-exhaustive/default-not-required/default_not_required.go new file mode 100644 index 0000000..1154665 --- /dev/null +++ b/testdata/src/default-signifies-exhaustive/default-not-required/default_not_required.go @@ -0,0 +1,28 @@ +package present + +import "default-signifies-exhaustive" + +func _a(t dse.T) { + // No diagnostic because default-require is not set. + switch t { + case dse.A: + case dse.B: + } +} + +func _b(t dse.T) { + //exhaustive:default-require-enforce this is a comment showing that we can turn it on for select switches + switch t { // want "^missing default in switch of type dse.T$" + case dse.A: + case dse.B: + } +} + +func _c(t dse.T) { + //exhaustive:default-require-enforce this is happy because it has a default + switch t { + case dse.A: + case dse.B: + default: + } +} From 7c5568686dba6094b79595ce730843e411cc0126 Mon Sep 17 00:00:00 2001 From: Aaron Gorenstein <91498583+aaron-mongodb@users.noreply.github.com> Date: Fri, 10 Nov 2023 09:56:03 -0500 Subject: [PATCH 05/10] Update exhaustive_test.go Co-authored-by: Nishanth Shanmugham --- exhaustive_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/exhaustive_test.go b/exhaustive_test.go index e917d42..eb6ccf7 100644 --- a/exhaustive_test.go +++ b/exhaustive_test.go @@ -46,7 +46,7 @@ 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 }) - // This tests exercises the default-case-required flag and its escape comment + // These tests exercise the default-case-required flag and its escape comment runTest(t, "default-signifies-exhaustive/default-required/...", func() { fDefaultCaseRequired = true }) runTest(t, "default-signifies-exhaustive/default-not-required/...", func() { fDefaultCaseRequired = false }) From 63b22a72f027f7bd903ef094366182a74378248f Mon Sep 17 00:00:00 2001 From: Aaron Gorenstein <91498583+aaron-mongodb@users.noreply.github.com> Date: Fri, 10 Nov 2023 09:56:10 -0500 Subject: [PATCH 06/10] Update switch.go Co-authored-by: Nishanth Shanmugham --- switch.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/switch.go b/switch.go index a037069..8e9ffe9 100644 --- a/switch.go +++ b/switch.go @@ -189,7 +189,7 @@ func makeMissingDefaultDiagnostic(sw *ast.SwitchStmt, enumTypes []enumType) anal Pos: sw.Pos(), End: sw.End(), Message: fmt.Sprintf( - "missing default in switch of type %s", + "missing default case in switch of type %s", diagnosticEnumTypes(enumTypes), ), } From 90515b32bc26540010ef5bd237c402862913a0a2 Mon Sep 17 00:00:00 2001 From: Aaron Gorenstein Date: Fri, 10 Nov 2023 15:58:04 +0000 Subject: [PATCH 07/10] PR comment: improve tests, improve directive logic --- exhaustive_test.go | 4 +- switch.go | 5 +- .../default_not_required.go | 46 +++++++++++++++++++ .../default-required/default_required.go | 37 +++++++++++++++ .../default_case_required.go | 8 ++++ .../default_not_required.go | 28 ----------- .../default-required/default_required.go | 19 -------- 7 files changed, 97 insertions(+), 50 deletions(-) create mode 100644 testdata/src/default-case-required/default-not-required/default_not_required.go create mode 100644 testdata/src/default-case-required/default-required/default_required.go create mode 100644 testdata/src/default-case-required/default_case_required.go delete mode 100644 testdata/src/default-signifies-exhaustive/default-not-required/default_not_required.go delete mode 100644 testdata/src/default-signifies-exhaustive/default-required/default_required.go diff --git a/exhaustive_test.go b/exhaustive_test.go index e917d42..5943e84 100644 --- a/exhaustive_test.go +++ b/exhaustive_test.go @@ -47,8 +47,8 @@ func TestExhaustive(t *testing.T) { runTest(t, "default-signifies-exhaustive/default-present/...", func() { fDefaultSignifiesExhaustive = true }) // This tests exercises the default-case-required flag and its escape comment - runTest(t, "default-signifies-exhaustive/default-required/...", func() { fDefaultCaseRequired = true }) - runTest(t, "default-signifies-exhaustive/default-not-required/...", func() { fDefaultCaseRequired = false }) + 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() { diff --git a/switch.go b/switch.go index a037069..64f684a 100644 --- a/switch.go +++ b/switch.go @@ -96,7 +96,10 @@ func switchChecker(pass *analysis.Pass, cfg switchConfig, generated boolCache, c requireDefaultCase := cfg.defaultCaseRequired if hasCommentPrefix(switchComments, defaultRequireIgnoreComment) { requireDefaultCase = false - } else if hasCommentPrefix(switchComments, defaultRequireEnforceComment) { + } + if hasCommentPrefix(switchComments, defaultRequireEnforceComment) { + // 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 } 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..7ca9e5b --- /dev/null +++ b/testdata/src/default-case-required/default-not-required/default_not_required.go @@ -0,0 +1,46 @@ +package notrequired + +import "default-case-required" + +func _a(t dcr.T) { + // No diagnostic because default-require is not set. + switch t { + case dcr.A: + case dcr.B: + } +} + +func _b(t dcr.T) { + //exhaustive:default-require-enforce this is a comment showing that we can turn it on for select switches + switch t { // want "^missing default in switch of type dcr.T$" + case dcr.A: + case dcr.B: + } +} + +func _c(t dcr.T) { + //exhaustive:default-require-ignore this comment is discarded in facvor of the enforcement + //exhaustive:default-require-enforce this is a comment showing that we can turn it on for select switches + switch t { // want "^missing default in switch of type dcr.T$" + case dcr.A: + case dcr.B: + } +} + +func _d(t dcr.T) { + //exhaustive:default-require-enforce this is a comment showing that we can turn it on for select switches + //exhaustive:default-require-ignore this comment is discarded in facvor of the enforcement + switch t { // want "^missing default in switch of type dcr.T$" + case dcr.A: + case dcr.B: + } +} + +func _e(t dcr.T) { + //exhaustive:default-require-enforce 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..1d19860 --- /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 in switch of type dcr.T$" + case dcr.A: + case dcr.B: + } +} + +func _b(t dcr.T) { + //exhaustive:default-require-ignore 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:default-require-ignore this comment is discarded in facvor of the enforcement + //exhaustive:default-require-enforce this helps override the above + switch t { // want "^missing default 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 +) diff --git a/testdata/src/default-signifies-exhaustive/default-not-required/default_not_required.go b/testdata/src/default-signifies-exhaustive/default-not-required/default_not_required.go deleted file mode 100644 index 1154665..0000000 --- a/testdata/src/default-signifies-exhaustive/default-not-required/default_not_required.go +++ /dev/null @@ -1,28 +0,0 @@ -package present - -import "default-signifies-exhaustive" - -func _a(t dse.T) { - // No diagnostic because default-require is not set. - switch t { - case dse.A: - case dse.B: - } -} - -func _b(t dse.T) { - //exhaustive:default-require-enforce this is a comment showing that we can turn it on for select switches - switch t { // want "^missing default in switch of type dse.T$" - case dse.A: - case dse.B: - } -} - -func _c(t dse.T) { - //exhaustive:default-require-enforce this is happy because it has a default - switch t { - case dse.A: - case dse.B: - default: - } -} diff --git a/testdata/src/default-signifies-exhaustive/default-required/default_required.go b/testdata/src/default-signifies-exhaustive/default-required/default_required.go deleted file mode 100644 index efbd0cc..0000000 --- a/testdata/src/default-signifies-exhaustive/default-required/default_required.go +++ /dev/null @@ -1,19 +0,0 @@ -package present - -import "default-signifies-exhaustive" - -func _a(t dse.T) { - // expect a diagnostic when fDefaultCaseRequired is true. - switch t { // want "^missing default in switch of type dse.T$" - case dse.A: - case dse.B: - } -} - -func _b(t dse.T) { - //exhaustive:default-require-ignore this is a comment showing that we can turn it off for select switches - switch t { - case dse.A: - case dse.B: - } -} From 63e659b2fa0f87142c9371309fb464c5b408a5ed Mon Sep 17 00:00:00 2001 From: Aaron Gorenstein Date: Fri, 10 Nov 2023 16:21:53 +0000 Subject: [PATCH 08/10] Renamed flags --- comment.go | 8 ++-- switch.go | 45 +++++++++++++++++-- .../default_not_required.go | 12 ++--- .../default-required/default_required.go | 6 +-- 4 files changed, 54 insertions(+), 17 deletions(-) diff --git a/comment.go b/comment.go index 9df598c..123e018 100644 --- a/comment.go +++ b/comment.go @@ -7,10 +7,10 @@ import ( ) const ( - ignoreComment = "//exhaustive:ignore" - enforceComment = "//exhaustive:enforce" - defaultRequireIgnoreComment = "//exhaustive:default-require-ignore" - defaultRequireEnforceComment = "//exhaustive:default-require-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/switch.go b/switch.go index 64f684a..a553b99 100644 --- a/switch.go +++ b/switch.go @@ -5,6 +5,7 @@ import ( "go/ast" "go/types" "regexp" + "strings" "golang.org/x/tools/go/analysis" ) @@ -59,6 +60,41 @@ type switchConfig struct { 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. @@ -82,22 +118,23 @@ 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 hasCommentPrefix(switchComments, defaultRequireIgnoreComment) { + if directivesIncludes(uDirectives, ignoreDefaultCaseRequiredComment) { requireDefaultCase = false } - if hasCommentPrefix(switchComments, defaultRequireEnforceComment) { + 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 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 index 7ca9e5b..d0cfd4a 100644 --- 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 @@ -11,7 +11,7 @@ func _a(t dcr.T) { } func _b(t dcr.T) { - //exhaustive:default-require-enforce this is a comment showing that we can turn it on for select switches + //exhaustive:enforce-default-case-required this is a comment showing that we can turn it on for select switches switch t { // want "^missing default in switch of type dcr.T$" case dcr.A: case dcr.B: @@ -19,8 +19,8 @@ func _b(t dcr.T) { } func _c(t dcr.T) { - //exhaustive:default-require-ignore this comment is discarded in facvor of the enforcement - //exhaustive:default-require-enforce 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 + //exhaustive:enforce-default-case-required this is a comment showing that we can turn it on for select switches switch t { // want "^missing default in switch of type dcr.T$" case dcr.A: case dcr.B: @@ -28,8 +28,8 @@ func _c(t dcr.T) { } func _d(t dcr.T) { - //exhaustive:default-require-enforce this is a comment showing that we can turn it on for select switches - //exhaustive:default-require-ignore 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 + //exhaustive:ignore-default-case-required this comment is discarded in facvor of the enforcement switch t { // want "^missing default in switch of type dcr.T$" case dcr.A: case dcr.B: @@ -37,7 +37,7 @@ func _d(t dcr.T) { } func _e(t dcr.T) { - //exhaustive:default-require-enforce this is happy because it has a default + //exhaustive:enforce-default-case-required this is happy because it has a default switch t { case dcr.A: case dcr.B: diff --git a/testdata/src/default-case-required/default-required/default_required.go b/testdata/src/default-case-required/default-required/default_required.go index 1d19860..0f73b9e 100644 --- a/testdata/src/default-case-required/default-required/default_required.go +++ b/testdata/src/default-case-required/default-required/default_required.go @@ -11,7 +11,7 @@ func _a(t dcr.T) { } func _b(t dcr.T) { - //exhaustive:default-require-ignore this is a comment showing that we can turn it off for select switches + //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: @@ -19,8 +19,8 @@ func _b(t dcr.T) { } func _c(t dcr.T) { - //exhaustive:default-require-ignore this comment is discarded in facvor of the enforcement - //exhaustive:default-require-enforce this helps override the above + //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 in switch of type dcr.T$" case dcr.A: case dcr.B: From febf3bfac5bafdccaff689851747a53ad7af7a21 Mon Sep 17 00:00:00 2001 From: Nishanth Shanmugham Date: Fri, 10 Nov 2023 23:32:24 +0000 Subject: [PATCH 09/10] update testdata messages to match diagnostic change in 63b22a7 --- .../default-not-required/default_not_required.go | 6 +++--- .../default-required/default_required.go | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) 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 index d0cfd4a..df23580 100644 --- 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 @@ -12,7 +12,7 @@ func _a(t dcr.T) { 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 in switch of type dcr.T$" + switch t { // want "^missing default case in switch of type dcr.T$" case dcr.A: case dcr.B: } @@ -21,7 +21,7 @@ func _b(t dcr.T) { 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 in switch of type dcr.T$" + switch t { // want "^missing default case in switch of type dcr.T$" case dcr.A: case dcr.B: } @@ -30,7 +30,7 @@ func _c(t dcr.T) { 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 in switch of type dcr.T$" + switch t { // want "^missing default case in switch of type dcr.T$" case dcr.A: case dcr.B: } diff --git a/testdata/src/default-case-required/default-required/default_required.go b/testdata/src/default-case-required/default-required/default_required.go index 0f73b9e..e196f40 100644 --- a/testdata/src/default-case-required/default-required/default_required.go +++ b/testdata/src/default-case-required/default-required/default_required.go @@ -4,7 +4,7 @@ import "default-case-required" func _a(t dcr.T) { // expect a diagnostic when fDefaultCaseRequired is true. - switch t { // want "^missing default in switch of type dcr.T$" + switch t { // want "^missing default case in switch of type dcr.T$" case dcr.A: case dcr.B: } @@ -21,7 +21,7 @@ func _b(t dcr.T) { 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 in switch of type dcr.T$" + switch t { // want "^missing default case in switch of type dcr.T$" case dcr.A: case dcr.B: } From 21e383ac34855cbd88d0d7566a085c7cd8e72ee0 Mon Sep 17 00:00:00 2001 From: Nishanth Shanmugham Date: Fri, 10 Nov 2023 23:49:20 +0000 Subject: [PATCH 10/10] update outdated flag name in testdata --- .../default-not-required/default_not_required.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 index df23580..03cd18c 100644 --- 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 @@ -3,7 +3,8 @@ package notrequired import "default-case-required" func _a(t dcr.T) { - // No diagnostic because default-require is not set. + // No diagnostic because neither fDefaultCaseRequired is true + // nor the enforcement comment is present. switch t { case dcr.A: case dcr.B: