From 65afac4a51c5aa737ee55a9b982b119f41009771 Mon Sep 17 00:00:00 2001 From: Navneeth Jayendran Date: Sat, 30 Dec 2023 14:49:02 -0500 Subject: [PATCH 1/8] Resolves #70: use stricter comment directive parsing --- comment.go | 51 +++++++++++++++++++++------ comment_test.go | 93 +++++++++++++++++++++++++++++++++++++++++++++++++ map.go | 6 ++-- switch.go | 21 ++++------- 4 files changed, 144 insertions(+), 27 deletions(-) diff --git a/comment.go b/comment.go index 123e018..a7b07c1 100644 --- a/comment.go +++ b/comment.go @@ -7,21 +7,52 @@ import ( ) const ( - ignoreComment = "//exhaustive:ignore" - enforceComment = "//exhaustive:enforce" - ignoreDefaultCaseRequiredComment = "//exhaustive:ignore-default-case-required" - enforceDefaultCaseRequiredComment = "//exhaustive:enforce-default-case-required" + exhaustiveComment = "//exhaustive:" + ignoreComment = "ignore" + enforceComment = "enforce" + ignoreDefaultCaseRequiredComment = "ignore-default-case-required" + enforceDefaultCaseRequiredComment = "enforce-default-case-required" ) -func hasCommentPrefix(comments []*ast.CommentGroup, comment string) bool { - for _, c := range comments { - for _, cc := range c.List { - if strings.HasPrefix(cc.Text, comment) { - return true +type directive int64 + +const ( + ignoreDirective = 1 << iota + enforceDirective + ignoreDefaultCaseRequiredDirective + enforceDefaultCaseRequiredDirective +) + +type directiveSet int64 + +func parseDirectiveSet(commentGroups []*ast.CommentGroup) (out directiveSet) { + for _, commentGroup := range commentGroups { + for _, comment := range commentGroup.List { + commentLine := comment.Text + if !strings.HasPrefix(commentLine, exhaustiveComment) { + continue + } + directive := commentLine[len(exhaustiveComment):] + if whiteSpaceIndex := strings.IndexAny(directive, " \t"); whiteSpaceIndex != -1 { + directive = directive[:whiteSpaceIndex] + } + switch directive { + case ignoreComment: + out |= ignoreDirective + case enforceComment: + out |= enforceDirective + case ignoreDefaultCaseRequiredComment: + out |= ignoreDefaultCaseRequiredDirective + case enforceDefaultCaseRequiredComment: + out |= enforceDefaultCaseRequiredDirective } } } - return false + return +} + +func (d directiveSet) hasDirective(directive directive) bool { + return int64(d)&int64(directive) != 0 } func fileCommentMap(fset *token.FileSet, file *ast.File) ast.CommentMap { diff --git a/comment_test.go b/comment_test.go index 8c39c28..3a007d9 100644 --- a/comment_test.go +++ b/comment_test.go @@ -1,6 +1,7 @@ package exhaustive import ( + "go/ast" "go/parser" "go/token" "testing" @@ -32,3 +33,95 @@ func f() { } }) } + +func TestDirectives(t *testing.T) { + t.Run("no directives", func(t *testing.T) { + commentGroups := []*ast.CommentGroup{ + { + List: []*ast.Comment{ + { + Text: "//exhaustive:foo", + }, + }, + }, + { + List: []*ast.Comment{ + { + Text: "//bar:value", + }, + { + Text: "Another comment", + }, + }, + }, + } + directives := parseDirectiveSet(commentGroups) + if directives != 0 { + t.Errorf("unexpected directives: %d", directives) + } + }) + + t.Run("several conflicting directives", func(t *testing.T) { + commentGroups := []*ast.CommentGroup{ + { + List: []*ast.Comment{ + { + Text: "//exhaustive:ignore", + }, + { + Text: "//exhaustive:enforce", + }, + }, + }, + { + List: []*ast.Comment{ + { + Text: "//exhaustive:ignore-default-case-required", + }, + { + Text: "//exhaustive:enforce-default-case-required", + }, + { + Text: "//exhaustive:ignore-default-case-required", + }, + }, + }, + } + directives := parseDirectiveSet(commentGroups) + expected := ignoreDirective | enforceDirective | + ignoreDefaultCaseRequiredDirective | enforceDefaultCaseRequiredDirective + + if directives != directiveSet(expected) { + t.Errorf("unexpected directives: %d", directives) + } + }) + + t.Run("directives with trailing text and surrounding non-directives", func(t *testing.T) { + commentGroups := []*ast.CommentGroup{ + { + List: []*ast.Comment{ + { + Text: "Nevermind this comment", + }, + { + Text: "//exhaustive:ignore foo", + }, + { + Text: "//exhaustive:enforce-default-case-required\tbar", + }, + { + Text: "//exhaustive:ignore-default-case-garbage", + }, + { + Text: "This comment doesn't matter", + }, + }, + }, + } + + directives := parseDirectiveSet(commentGroups) + if directives != ignoreDirective|enforceDefaultCaseRequiredDirective { + t.Errorf("unexpected directives: %d", directives) + } + }) +} diff --git a/map.go b/map.go index 23b4bef..53918a5 100644 --- a/map.go +++ b/map.go @@ -77,13 +77,15 @@ func mapChecker(pass *analysis.Pass, cfg mapConfig, generated boolCache, comment } } - if !cfg.explicit && hasCommentPrefix(relatedComments, ignoreComment) { + directives := parseDirectiveSet(relatedComments) + + if !cfg.explicit && directives.hasDirective(ignoreDirective) { // Skip checking of this map literal due to ignore // comment. Still return true because there may be nested // map literals that are not to be ignored. return true, resultIgnoreComment } - if cfg.explicit && !hasCommentPrefix(relatedComments, enforceComment) { + if cfg.explicit && !directives.hasDirective(enforceDirective) { return true, resultNoEnforceComment } diff --git a/switch.go b/switch.go index d235fbe..c12b693 100644 --- a/switch.go +++ b/switch.go @@ -85,16 +85,6 @@ func userDirectives(comments []*ast.CommentGroup) []string { 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. @@ -118,23 +108,24 @@ func switchChecker(pass *analysis.Pass, cfg switchConfig, generated boolCache, c sw := n.(*ast.SwitchStmt) switchComments := comments.get(pass.Fset, file)[sw] - uDirectives := userDirectives(switchComments) - if !cfg.explicit && directivesIncludes(uDirectives, ignoreComment) { + uDirectives := parseDirectiveSet(switchComments) + + if !cfg.explicit && uDirectives.hasDirective(ignoreDirective) { // 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 && !directivesIncludes(uDirectives, enforceComment) { + if cfg.explicit && !uDirectives.hasDirective(enforceDirective) { // Skip checking of this switch statement due to missing // enforce comment. return true, resultNoEnforceComment } requireDefaultCase := cfg.defaultCaseRequired - if directivesIncludes(uDirectives, ignoreDefaultCaseRequiredComment) { + if uDirectives.hasDirective(ignoreDefaultCaseRequiredDirective) { requireDefaultCase = false } - if directivesIncludes(uDirectives, enforceDefaultCaseRequiredComment) { + if uDirectives.hasDirective(enforceDefaultCaseRequiredDirective) { // 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 From 8733b84db6eb8712b62969d185f34b8beaef6a5d Mon Sep 17 00:00:00 2001 From: Navneeth Jayendran Date: Sat, 30 Dec 2023 14:51:42 -0500 Subject: [PATCH 2/8] Add E2E test cases --- .../default-not-required/default_not_required.go | 3 ++- .../default-required/default_required.go | 4 +++- 2 files changed, 5 insertions(+), 2 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 63e22b4..95512c7 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 @@ -1,10 +1,11 @@ package notrequired -import "default-case-required" +import dcr "default-case-required" func _a(t dcr.T) { // No diagnostic because neither fDefaultCaseRequired is true // nor the enforcement comment is present. + //exhaustive:enforce-default-case-required-typo this invalid directive should be ignored 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 1368eaa..41c8bea 100644 --- a/testdata/src/default-case-required/default-required/default_required.go +++ b/testdata/src/default-case-required/default-required/default_required.go @@ -1,9 +1,10 @@ package required -import "default-case-required" +import dcr "default-case-required" func _a(t dcr.T) { // expect a diagnostic when fDefaultCaseRequired is true. + //exhaustive:ignore-default-case-required-typo This invalid directive should be ignored switch t { // want "^missing default case in switch of type dcr.T$" case dcr.A: case dcr.B: @@ -12,6 +13,7 @@ func _a(t dcr.T) { func _b(t dcr.T) { //exhaustive:ignore-default-case-required this is a comment showing that we can turn it off for select switches + //exhaustive:enforce-default-case-required-typo This invalid directive should be ignored switch t { case dcr.A: case dcr.B: From cbba3b8254f26887b16fa77794f27fce3256de76 Mon Sep 17 00:00:00 2001 From: Navneeth Jayendran Date: Sat, 30 Dec 2023 15:19:45 -0500 Subject: [PATCH 3/8] Remove unused code --- switch.go | 26 -------------------------- 1 file changed, 26 deletions(-) diff --git a/switch.go b/switch.go index c12b693..96c69df 100644 --- a/switch.go +++ b/switch.go @@ -5,7 +5,6 @@ import ( "go/ast" "go/types" "regexp" - "strings" "golang.org/x/tools/go/analysis" ) @@ -60,31 +59,6 @@ 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 -} - // 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. From 7539055f9d8a88899d73a8434f727a5902671a25 Mon Sep 17 00:00:00 2001 From: Navneeth Jayendran Date: Sat, 30 Dec 2023 15:21:02 -0500 Subject: [PATCH 4/8] Remove incorrect comment --- switch.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/switch.go b/switch.go index 96c69df..ca2eb60 100644 --- a/switch.go +++ b/switch.go @@ -106,7 +106,7 @@ func switchChecker(pass *analysis.Pass, cfg switchConfig, generated boolCache, c } if sw.Tag == nil { - return true, resultNoSwitchTag // never possible for valid Go program? + return true, resultNoSwitchTag } t := pass.TypesInfo.Types[sw.Tag] From 1995b7967f6499593b06ccf3fc099cbdbba1fe61 Mon Sep 17 00:00:00 2001 From: Navneeth Jayendran Date: Wed, 3 Jan 2024 20:03:49 -0500 Subject: [PATCH 5/8] Address minor feedback --- comment.go | 7 ++++--- comment_test.go | 6 +++--- map.go | 6 +++--- switch.go | 10 +++++----- 4 files changed, 15 insertions(+), 14 deletions(-) diff --git a/comment.go b/comment.go index a7b07c1..776f004 100644 --- a/comment.go +++ b/comment.go @@ -25,7 +25,8 @@ const ( type directiveSet int64 -func parseDirectiveSet(commentGroups []*ast.CommentGroup) (out directiveSet) { +func parseDirectives(commentGroups []*ast.CommentGroup) directiveSet { + var out directiveSet for _, commentGroup := range commentGroups { for _, comment := range commentGroup.List { commentLine := comment.Text @@ -48,10 +49,10 @@ func parseDirectiveSet(commentGroups []*ast.CommentGroup) (out directiveSet) { } } } - return + return out } -func (d directiveSet) hasDirective(directive directive) bool { +func (d directiveSet) has(directive directive) bool { return int64(d)&int64(directive) != 0 } diff --git a/comment_test.go b/comment_test.go index 3a007d9..d1f7fab 100644 --- a/comment_test.go +++ b/comment_test.go @@ -55,7 +55,7 @@ func TestDirectives(t *testing.T) { }, }, } - directives := parseDirectiveSet(commentGroups) + directives := parseDirectives(commentGroups) if directives != 0 { t.Errorf("unexpected directives: %d", directives) } @@ -87,7 +87,7 @@ func TestDirectives(t *testing.T) { }, }, } - directives := parseDirectiveSet(commentGroups) + directives := parseDirectives(commentGroups) expected := ignoreDirective | enforceDirective | ignoreDefaultCaseRequiredDirective | enforceDefaultCaseRequiredDirective @@ -119,7 +119,7 @@ func TestDirectives(t *testing.T) { }, } - directives := parseDirectiveSet(commentGroups) + directives := parseDirectives(commentGroups) if directives != ignoreDirective|enforceDefaultCaseRequiredDirective { t.Errorf("unexpected directives: %d", directives) } diff --git a/map.go b/map.go index 53918a5..3b39620 100644 --- a/map.go +++ b/map.go @@ -77,15 +77,15 @@ func mapChecker(pass *analysis.Pass, cfg mapConfig, generated boolCache, comment } } - directives := parseDirectiveSet(relatedComments) + directives := parseDirectives(relatedComments) - if !cfg.explicit && directives.hasDirective(ignoreDirective) { + if !cfg.explicit && directives.has(ignoreDirective) { // Skip checking of this map literal due to ignore // comment. Still return true because there may be nested // map literals that are not to be ignored. return true, resultIgnoreComment } - if cfg.explicit && !directives.hasDirective(enforceDirective) { + if cfg.explicit && !directives.has(enforceDirective) { return true, resultNoEnforceComment } diff --git a/switch.go b/switch.go index ca2eb60..531c05d 100644 --- a/switch.go +++ b/switch.go @@ -82,24 +82,24 @@ func switchChecker(pass *analysis.Pass, cfg switchConfig, generated boolCache, c sw := n.(*ast.SwitchStmt) switchComments := comments.get(pass.Fset, file)[sw] - uDirectives := parseDirectiveSet(switchComments) + uDirectives := parseDirectives(switchComments) - if !cfg.explicit && uDirectives.hasDirective(ignoreDirective) { + if !cfg.explicit && uDirectives.has(ignoreDirective) { // 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 && !uDirectives.hasDirective(enforceDirective) { + if cfg.explicit && !uDirectives.has(enforceDirective) { // Skip checking of this switch statement due to missing // enforce comment. return true, resultNoEnforceComment } requireDefaultCase := cfg.defaultCaseRequired - if uDirectives.hasDirective(ignoreDefaultCaseRequiredDirective) { + if uDirectives.has(ignoreDefaultCaseRequiredDirective) { requireDefaultCase = false } - if uDirectives.hasDirective(enforceDefaultCaseRequiredDirective) { + if uDirectives.has(enforceDefaultCaseRequiredDirective) { // 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 From 9cfb9072462ad3c121ee862d94af78ec0fa8e1bd Mon Sep 17 00:00:00 2001 From: Navneeth Jayendran Date: Wed, 3 Jan 2024 22:17:49 -0500 Subject: [PATCH 6/8] Address error reporting feedback --- comment.go | 28 ++++++- comment_test.go | 82 ++++++++++++++++--- map.go | 5 +- switch.go | 17 +++- .../default_not_required.go | 13 ++- .../default-required/default_required.go | 13 ++- .../default-absent/default_absent.go | 11 ++- .../default-present/default_present.go | 11 ++- .../enforce-comment/enforce_comment_map.go | 7 ++ .../enforce-comment/enforce_comment_switch.go | 10 +++ 10 files changed, 172 insertions(+), 25 deletions(-) diff --git a/comment.go b/comment.go index 776f004..1f1852f 100644 --- a/comment.go +++ b/comment.go @@ -1,6 +1,8 @@ package exhaustive import ( + "errors" + "fmt" "go/ast" "go/token" "strings" @@ -14,6 +16,11 @@ const ( enforceDefaultCaseRequiredComment = "enforce-default-case-required" ) +var ( + errConflictingDirectives = errors.New("conflicting directives") + errInvalidDirective = errors.New("invalid directive") +) + type directive int64 const ( @@ -25,7 +32,7 @@ const ( type directiveSet int64 -func parseDirectives(commentGroups []*ast.CommentGroup) directiveSet { +func parseDirectives(commentGroups []*ast.CommentGroup) (directiveSet, error) { var out directiveSet for _, commentGroup := range commentGroups { for _, comment := range commentGroup.List { @@ -46,16 +53,33 @@ func parseDirectives(commentGroups []*ast.CommentGroup) directiveSet { out |= ignoreDefaultCaseRequiredDirective case enforceDefaultCaseRequiredComment: out |= enforceDefaultCaseRequiredDirective + default: + return out, fmt.Errorf("%w %q", errInvalidDirective, directive) } } } - return out + return out, out.validate() } func (d directiveSet) has(directive directive) bool { return int64(d)&int64(directive) != 0 } +func (d directiveSet) validate() error { + enforceConflict := ignoreDirective | enforceDirective + if d&(directiveSet(enforceConflict)) == directiveSet(enforceConflict) { + return fmt.Errorf("%w %q and %q", errConflictingDirectives, ignoreComment, enforceComment) + } + defaultCaseRequiredConflict := ignoreDefaultCaseRequiredDirective | enforceDefaultCaseRequiredDirective + if d&(directiveSet(defaultCaseRequiredConflict)) == directiveSet(defaultCaseRequiredConflict) { + return fmt.Errorf( + "%w %q and %q", errConflictingDirectives, + ignoreDefaultCaseRequiredComment, enforceDefaultCaseRequiredComment, + ) + } + return nil +} + func fileCommentMap(fset *token.FileSet, file *ast.File) ast.CommentMap { return ast.NewCommentMap(fset, file, file.Comments) } diff --git a/comment_test.go b/comment_test.go index d1f7fab..ecad023 100644 --- a/comment_test.go +++ b/comment_test.go @@ -40,7 +40,7 @@ func TestDirectives(t *testing.T) { { List: []*ast.Comment{ { - Text: "//exhaustive:foo", + Text: "//exhauster:foo", }, }, }, @@ -55,13 +55,43 @@ func TestDirectives(t *testing.T) { }, }, } - directives := parseDirectives(commentGroups) + directives, err := parseDirectives(commentGroups) + if err != nil { + t.Errorf("unexpected error: %s", err) + } if directives != 0 { t.Errorf("unexpected directives: %d", directives) } }) - t.Run("several conflicting directives", func(t *testing.T) { + t.Run("invalid directive", func(t *testing.T) { + commentGroups := []*ast.CommentGroup{ + { + List: []*ast.Comment{ + { + Text: "//exhauster:foo", + }, + }, + }, + { + List: []*ast.Comment{ + { + Text: "//exhaustive:enfocre", + }, + }, + }, + } + _, err := parseDirectives(commentGroups) + if err == nil { + t.Errorf("expectedly no error on invalid directive") + return + } + if err.Error() != `invalid directive "enfocre"` { + t.Errorf("unexpected error: %s", err) + } + }) + + t.Run("conflicting enforcement directives", func(t *testing.T) { commentGroups := []*ast.CommentGroup{ { List: []*ast.Comment{ @@ -73,6 +103,33 @@ func TestDirectives(t *testing.T) { }, }, }, + { + List: []*ast.Comment{ + { + Text: "//exhaustive:ignore-default-case-required", + }, + }, + }, + } + _, err := parseDirectives(commentGroups) + if err == nil { + t.Errorf("unexpectedly no error on conflicting directives") + return + } + if err.Error() != `conflicting directives "ignore" and "enforce"` { + t.Errorf("unexpected error: %s", err) + } + }) + + t.Run("conflicting default case enforcement directives", func(t *testing.T) { + commentGroups := []*ast.CommentGroup{ + { + List: []*ast.Comment{ + { + Text: "//exhaustive:enforce", + }, + }, + }, { List: []*ast.Comment{ { @@ -87,13 +144,12 @@ func TestDirectives(t *testing.T) { }, }, } - directives := parseDirectives(commentGroups) - expected := ignoreDirective | enforceDirective | - ignoreDefaultCaseRequiredDirective | enforceDefaultCaseRequiredDirective - - if directives != directiveSet(expected) { - t.Errorf("unexpected directives: %d", directives) + _, err := parseDirectives(commentGroups) + if err == nil { + t.Errorf("unexpectedly no error on conflicting directives") + return } + }) t.Run("directives with trailing text and surrounding non-directives", func(t *testing.T) { @@ -109,9 +165,6 @@ func TestDirectives(t *testing.T) { { Text: "//exhaustive:enforce-default-case-required\tbar", }, - { - Text: "//exhaustive:ignore-default-case-garbage", - }, { Text: "This comment doesn't matter", }, @@ -119,7 +172,10 @@ func TestDirectives(t *testing.T) { }, } - directives := parseDirectives(commentGroups) + directives, err := parseDirectives(commentGroups) + if err != nil { + t.Errorf("unexpected error: %s", err) + } if directives != ignoreDirective|enforceDefaultCaseRequiredDirective { t.Errorf("unexpected directives: %d", directives) } diff --git a/map.go b/map.go index 3b39620..aa5e9bd 100644 --- a/map.go +++ b/map.go @@ -77,7 +77,10 @@ func mapChecker(pass *analysis.Pass, cfg mapConfig, generated boolCache, comment } } - directives := parseDirectives(relatedComments) + directives, err := parseDirectives(relatedComments) + if err != nil { + pass.Report(makeInvalidDirectiveDiagnostic(lit, err)) + } if !cfg.explicit && directives.has(ignoreDirective) { // Skip checking of this map literal due to ignore diff --git a/switch.go b/switch.go index 531c05d..e9d6966 100644 --- a/switch.go +++ b/switch.go @@ -82,7 +82,10 @@ func switchChecker(pass *analysis.Pass, cfg switchConfig, generated boolCache, c sw := n.(*ast.SwitchStmt) switchComments := comments.get(pass.Fset, file)[sw] - uDirectives := parseDirectives(switchComments) + uDirectives, err := parseDirectives(switchComments) + if err != nil { + pass.Report(makeInvalidDirectiveDiagnostic(sw, err)) + } if !cfg.explicit && uDirectives.has(ignoreDirective) { // Skip checking of this switch statement due to ignore @@ -134,6 +137,7 @@ func switchChecker(pass *analysis.Pass, cfg switchConfig, generated boolCache, c // 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 { @@ -199,3 +203,14 @@ func makeMissingDefaultDiagnostic(sw *ast.SwitchStmt, enumTypes []enumType) anal ), } } + +func makeInvalidDirectiveDiagnostic(node ast.Node, err error) analysis.Diagnostic { + return analysis.Diagnostic{ + Pos: node.Pos(), + End: node.End(), + Message: fmt.Sprintf( + "failed to parse directives: %s", + err.Error(), + ), + } +} 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 95512c7..8cb63b1 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 @@ -5,7 +5,6 @@ import dcr "default-case-required" func _a(t dcr.T) { // No diagnostic because neither fDefaultCaseRequired is true // nor the enforcement comment is present. - //exhaustive:enforce-default-case-required-typo this invalid directive should be ignored switch t { case dcr.A: case dcr.B: @@ -21,7 +20,6 @@ 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 case in switch of type dcr.T$" case dcr.A: @@ -31,7 +29,6 @@ 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 case in switch of type dcr.T$" case dcr.A: case dcr.B: @@ -61,3 +58,13 @@ func _f() { case x == 0: } } + +func _g(t dcr.T) { + //exhaustive:ignore-default-case-required + //exhaustive:enforce-default-case-required + switch t { // want "^failed to parse directives: conflicting directives \"ignore-default-case-required\" and \"enforce-default-case-required\"$" + 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 index 41c8bea..246a5f0 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,6 @@ import dcr "default-case-required" func _a(t dcr.T) { // expect a diagnostic when fDefaultCaseRequired is true. - //exhaustive:ignore-default-case-required-typo This invalid directive should be ignored switch t { // want "^missing default case in switch of type dcr.T$" case dcr.A: case dcr.B: @@ -13,7 +12,6 @@ func _a(t dcr.T) { func _b(t dcr.T) { //exhaustive:ignore-default-case-required this is a comment showing that we can turn it off for select switches - //exhaustive:enforce-default-case-required-typo This invalid directive should be ignored switch t { case dcr.A: case dcr.B: @@ -21,7 +19,6 @@ 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 case in switch of type dcr.T$" case dcr.A: @@ -50,3 +47,13 @@ func _e() { case x == 0: } } + +func _f(t dcr.T) { + //exhaustive:enforce-default-case-required + //exhaustive:ignore-default-case-required + switch t { // want "^failed to parse directives: conflicting directives \"ignore-default-case-required\" and \"enforce-default-case-required\"$" + case dcr.A: + case dcr.B: + default: + } +} diff --git a/testdata/src/default-signifies-exhaustive/default-absent/default_absent.go b/testdata/src/default-signifies-exhaustive/default-absent/default_absent.go index 8884d7d..238a3ed 100644 --- a/testdata/src/default-signifies-exhaustive/default-absent/default_absent.go +++ b/testdata/src/default-signifies-exhaustive/default-absent/default_absent.go @@ -1,9 +1,18 @@ package absent -import "default-signifies-exhaustive" +import dse "default-signifies-exhaustive" func _a(t dse.T) { switch t { // want "^missing cases in switch of type dse.T: dse.B$" case dse.A: } } + +func _b(t dse.T) { + //exhaustive:ignore + //exhaustive:enforce + switch t { // want "^failed to parse directives: conflicting directives \"ignore\" and \"enforce\"$" + case dse.A: + case dse.B: + } +} diff --git a/testdata/src/default-signifies-exhaustive/default-present/default_present.go b/testdata/src/default-signifies-exhaustive/default-present/default_present.go index 749f377..7866249 100644 --- a/testdata/src/default-signifies-exhaustive/default-present/default_present.go +++ b/testdata/src/default-signifies-exhaustive/default-present/default_present.go @@ -1,6 +1,6 @@ package present -import "default-signifies-exhaustive" +import dse "default-signifies-exhaustive" func _a(t dse.T) { // expect no diagnostics, since default case is present, @@ -11,3 +11,12 @@ func _a(t dse.T) { default: } } + +func _b(t dse.T) { + //exhaustive:enforce + //exhaustive:ignore + switch t { // want "^failed to parse directives: conflicting directives \"ignore\" and \"enforce\"$" + case dse.A: + case dse.B: + } +} diff --git a/testdata/src/enforce-comment/enforce_comment_map.go b/testdata/src/enforce-comment/enforce_comment_map.go index 6acd44e..2442d33 100644 --- a/testdata/src/enforce-comment/enforce_comment_map.go +++ b/testdata/src/enforce-comment/enforce_comment_map.go @@ -323,3 +323,10 @@ func localVarDeclaration() { }) ) } + +func invalidDirectiveMap() { + //exhaustive:enfocre + var _ = map[Direction]int{ // want "^failed to parse directives: invalid directive \"enfocre\"$" + N: 1, + } +} diff --git a/testdata/src/enforce-comment/enforce_comment_switch.go b/testdata/src/enforce-comment/enforce_comment_switch.go index 5360c7f..e3672b6 100644 --- a/testdata/src/enforce-comment/enforce_comment_switch.go +++ b/testdata/src/enforce-comment/enforce_comment_switch.go @@ -82,3 +82,13 @@ func _reverse_nested() { } } } + +func invalidDirectiveSwitch() { + var d Direction + + //exhaustive:ingore + switch d { // want "^failed to parse directives: invalid directive \"ingore\"$" + case N: + } + +} From 499a89c77865de0c96e8d06e6e4690d8a5f2677d Mon Sep 17 00:00:00 2001 From: Nishanth Shanmugham Date: Tue, 27 Feb 2024 21:19:43 +0000 Subject: [PATCH 7/8] add testdata for no-op directive comment --- testdata/src/general/x/directive.go | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 testdata/src/general/x/directive.go diff --git a/testdata/src/general/x/directive.go b/testdata/src/general/x/directive.go new file mode 100644 index 0000000..df660ca --- /dev/null +++ b/testdata/src/general/x/directive.go @@ -0,0 +1,28 @@ +package x + +func _() { + var d Direction + + // This comment should not produce an diagnostic (note unknown prefix "exhauster:" + //instead of "exhaustive:". + //exhauster:foo + switch d { + case N: + case S: + case W: + case E: + case directionInvalid: + default: + } + + // This comment should not produce an diagnostic (note unknown prefix "exhauster:" + //instead of "exhaustive:". + //exhauster:foo + _ = map[Direction]int{ + N: 1, + S: 2, + W: 3, + E: 4, + directionInvalid: 0, + } +} From 74821eac10c04583939b729880e7fb40a0d9469b Mon Sep 17 00:00:00 2001 From: Nishanth Shanmugham Date: Tue, 27 Feb 2024 21:30:58 +0000 Subject: [PATCH 8/8] remove error vars --- comment.go | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/comment.go b/comment.go index 1f1852f..caf7649 100644 --- a/comment.go +++ b/comment.go @@ -1,7 +1,6 @@ package exhaustive import ( - "errors" "fmt" "go/ast" "go/token" @@ -16,11 +15,6 @@ const ( enforceDefaultCaseRequiredComment = "enforce-default-case-required" ) -var ( - errConflictingDirectives = errors.New("conflicting directives") - errInvalidDirective = errors.New("invalid directive") -) - type directive int64 const ( @@ -54,7 +48,7 @@ func parseDirectives(commentGroups []*ast.CommentGroup) (directiveSet, error) { case enforceDefaultCaseRequiredComment: out |= enforceDefaultCaseRequiredDirective default: - return out, fmt.Errorf("%w %q", errInvalidDirective, directive) + return out, fmt.Errorf("invalid directive %q", directive) } } } @@ -68,14 +62,11 @@ func (d directiveSet) has(directive directive) bool { func (d directiveSet) validate() error { enforceConflict := ignoreDirective | enforceDirective if d&(directiveSet(enforceConflict)) == directiveSet(enforceConflict) { - return fmt.Errorf("%w %q and %q", errConflictingDirectives, ignoreComment, enforceComment) + return fmt.Errorf("conflicting directives %q and %q", ignoreComment, enforceComment) } defaultCaseRequiredConflict := ignoreDefaultCaseRequiredDirective | enforceDefaultCaseRequiredDirective if d&(directiveSet(defaultCaseRequiredConflict)) == directiveSet(defaultCaseRequiredConflict) { - return fmt.Errorf( - "%w %q and %q", errConflictingDirectives, - ignoreDefaultCaseRequiredComment, enforceDefaultCaseRequiredComment, - ) + return fmt.Errorf("conflicting directives %q and %q", ignoreDefaultCaseRequiredComment, enforceDefaultCaseRequiredComment) } return nil }