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

Resolve #36: add -explicit-exhaustive-switch flag #39

Merged
merged 4 commits into from
Mar 7, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,23 @@ func isGeneratedFileComment(s string) bool {

// ignoreDirective is used to exclude checking of specific switch statements.
const ignoreDirective = "//exhaustive:ignore"
const enforceDirective = "//exhaustive:enforce"

func containsIgnoreDirective(comments []*ast.CommentGroup) bool {
func containsDirective(comments []*ast.CommentGroup, directive string) bool {
for _, c := range comments {
for _, cc := range c.List {
if strings.HasPrefix(cc.Text, ignoreDirective) {
if strings.HasPrefix(cc.Text, directive) {
return true
}
}
}
return false
}

func containsEnforceDirective(comments []*ast.CommentGroup) bool {
return containsDirective(comments, enforceDirective)
}

func containsIgnoreDirective(comments []*ast.CommentGroup) bool {
return containsDirective(comments, ignoreDirective)
}
26 changes: 23 additions & 3 deletions exhaustive.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,11 +114,18 @@ All of these flags are optional.

flag type default value

-explicit-exhaustive-switch bool false
-check-generated bool false
-default-signifies-exhaustive bool false
-ignore-enum-members string (none)
-package-scope-only bool false


If the -explicit-exhaustive-switch flag is enabled, the analyzer only runs on
switch statements explicitly marked with the comment text
("exhaustive:enforce"). Otherwise, it runs on every enum switch statement not
marked with the comment text ("exhaustive:ignore").

If the -check-generated flag is enabled, switch statements in generated Go
source files are also checked. Otherwise, by default, switch statements in
generated files are not checked. See https://golang.org/s/generatedcode for the
Expand All @@ -145,13 +152,21 @@ switch on all these enums.

Skip analysis

To skip checking of a specific switch statement, associate the comment shown in
the example below with the switch statement. Note the lack of whitespace between
the comment marker ("//") and the comment text ("exhaustive:ignore").
In implicitly exhaustive switch mode (-explicit-exhaustive-switch=false), skip
checking of a specific switch statement by associating the comment shown in
the example below with the switch statement. Note the lack of whitespace
between the comment marker ("//") and the comment text ("exhaustive:ignore").

//exhaustive:ignore
switch v { ... }

In explicitly exhaustive switch mode (-explicit-exhaustive-switch=true), run
exhaustiveness checks on a specific switch statement by associating the
comment shown in the example below with the switch statement.

//exhaustive:enforce
switch v { ... }

To ignore specific enum members, see the -ignore-enum-members flag.

Switch statements in generated Go source files are not checked by default.
Expand Down Expand Up @@ -199,6 +214,7 @@ func (v *regexpFlag) Set(expr string) error {
func (v *regexpFlag) value() *regexp.Regexp { return v.r }

func init() {
Analyzer.Flags.BoolVar(&fExplicitExhaustiveSwitch, ExplicitExhaustiveSwitchFlag, false, "only run exhaustive check on switches with \"//exhaustive:enforce\" comment")
Analyzer.Flags.BoolVar(&fCheckGenerated, CheckGeneratedFlag, false, "check switch statements in generated files")
Analyzer.Flags.BoolVar(&fDefaultSignifiesExhaustive, DefaultSignifiesExhaustiveFlag, false, "presence of \"default\" case in switch statements satisfies exhaustiveness, even if all enum members are not listed")
Analyzer.Flags.Var(&fIgnoreEnumMembers, IgnoreEnumMembersFlag, "enum members matching `regex` do not have to be listed in switch statements to satisfy exhaustiveness")
Expand All @@ -212,6 +228,7 @@ func init() {
// Flag names used by the analyzer. They are exported for use by analyzer
// driver programs.
const (
ExplicitExhaustiveSwitchFlag = "explicit-exhaustive-switch"
Copy link
Owner

@nishanths nishanths Mar 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of using a shorter name: explicit, and changing the exported constant name to ExplicitFlag?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would work. Although it's maybe useful to clarify here between looking at switches as explicitly exhaustive vs e.g. looking at enums as explicitly exhaustive.

// only run exhaustive checks on defined types marked for enforcement
//exhaustive:enforce
type MyEnum int

//exhaustive:enforce
type MyExternalEnumAlias = someexternalpkg.Enum

It's not something currently supported by this analyzer, but maybe it will be one day.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this makes a lot of sense.

CheckGeneratedFlag = "check-generated"
DefaultSignifiesExhaustiveFlag = "default-signifies-exhaustive"
IgnoreEnumMembersFlag = "ignore-enum-members"
Expand All @@ -222,6 +239,7 @@ const (
)

var (
fExplicitExhaustiveSwitch bool
fCheckGenerated bool
fDefaultSignifiesExhaustive bool
fIgnoreEnumMembers regexpFlag
Expand All @@ -231,6 +249,7 @@ var (
// resetFlags resets the flag variables to their default values.
// Useful in tests.
func resetFlags() {
fExplicitExhaustiveSwitch = false
fCheckGenerated = false
fDefaultSignifiesExhaustive = false
fIgnoreEnumMembers = regexpFlag{}
Expand All @@ -253,6 +272,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
}

checkSwitchStatements(pass, inspect, config{
explicitExhaustiveSwitch: fExplicitExhaustiveSwitch,
defaultSignifiesExhaustive: fDefaultSignifiesExhaustive,
checkGeneratedFiles: fCheckGenerated,
ignoreEnumMembers: fIgnoreEnumMembers.value(),
Expand Down
7 changes: 6 additions & 1 deletion exhaustive_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,14 @@ func TestExhaustive(t *testing.T) {
run(t, "scope/allscope/...")
run(t, "scope/pkgscope/...", func() { fPackageScopeOnly = true })

// Switch statements with ignore directive comment should not be checked.
// Switch statements with ignore directive comment should not be checked during implicitly exhaustive switch
// mode
run(t, "ignore-comment/...")

// Switch statements without enforce directive comment should not be checked during explicitly exhaustive
// switch mode
run(t, "enforce-comment/...", func() { fExplicitExhaustiveSwitch = true })

// To satisfy exhaustiveness, it is sufficient for each unique constant
// value of the members to be listed, not each member by name.
run(t, "duplicate-enum-value/...")
Expand Down
31 changes: 19 additions & 12 deletions switch.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,17 +23,18 @@ type nodeVisitor func(n ast.Node, push bool, stack []ast.Node) (proceed bool, re

// Result values returned by a node visitor constructed via switchStmtChecker.
const (
resultNotPush = "not push"
resultGeneratedFile = "generated file"
resultNoSwitchTag = "no switch tag"
resultTagNotValue = "switch tag not value type"
resultTagNotNamed = "switch tag not named type"
resultTagNoPkg = "switch tag does not belong to regular package"
resultTagNotEnum = "switch tag not known enum type"
resultSwitchIgnoreComment = "switch statement has ignore comment"
resultEnumMembersAccounted = "requisite enum members accounted for"
resultDefaultCaseSuffices = "default case presence satisfies exhaustiveness"
resultReportedDiagnostic = "reported diagnostic"
resultNotPush = "not push"
resultGeneratedFile = "generated file"
resultNoSwitchTag = "no switch tag"
resultTagNotValue = "switch tag not value type"
resultTagNotNamed = "switch tag not named type"
resultTagNoPkg = "switch tag does not belong to regular package"
resultTagNotEnum = "switch tag not known enum type"
resultSwitchIgnoreComment = "switch statement has ignore comment"
resultSwitchNoEnforceComment = "switch statement has no enforce comment"
resultEnumMembersAccounted = "requisite enum members accounted for"
resultDefaultCaseSuffices = "default case presence satisfies exhaustiveness"
resultReportedDiagnostic = "reported diagnostic"
)

// switchStmtChecker returns a node visitor that checks exhaustiveness
Expand Down Expand Up @@ -70,12 +71,17 @@ func switchStmtChecker(pass *analysis.Pass, cfg config) nodeVisitor {
if _, ok := comments[file]; !ok {
comments[file] = ast.NewCommentMap(pass.Fset, file, file.Comments)
}
if containsIgnoreDirective(comments[file][sw]) {
switchComments := comments[file][sw]
if !cfg.explicitExhaustiveSwitch && containsIgnoreDirective(switchComments) {
// Skip checking of this switch statement due to ignore directive comment.
// Still return true because there may be nested switch statements
// that are not to be ignored.
return true, resultSwitchIgnoreComment
}
if cfg.explicitExhaustiveSwitch && !containsEnforceDirective(switchComments) {
// Skip checking of this switch statement due to missing enforce directive comment.
return true, resultSwitchNoEnforceComment
}

if sw.Tag == nil {
return true, resultNoSwitchTag
Expand Down Expand Up @@ -131,6 +137,7 @@ func switchStmtChecker(pass *analysis.Pass, cfg config) nodeVisitor {

// config is configuration for checkSwitchStatements.
type config struct {
explicitExhaustiveSwitch bool
defaultSignifiesExhaustive bool
checkGeneratedFiles bool
ignoreEnumMembers *regexp.Regexp // can be nil
Expand Down
Empty file added testdata/src/direction.go
Empty file.
11 changes: 11 additions & 0 deletions testdata/src/enforce-comment/direction.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package enforcecomment

type Direction int // want Direction:"^N,E,S,W,directionInvalid$"

const (
N Direction = 1
E Direction = 2
S Direction = 3
W Direction = 4
directionInvalid Direction = 5
)
84 changes: 84 additions & 0 deletions testdata/src/enforce-comment/enforce_comment.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
package enforcecomment

func _a() {
var d Direction

switch d {
case N:
case S:
case W:
default:
}

// this should report.
// some other comment
//exhaustive:enforce
// some other comment
switch d { // want "^missing cases in switch of type Direction: E, directionInvalid$"
case N:
case S:
case W:
default:
}
}

func _b() {
var d Direction

// this should not report.
switch d {
case N:
case S:
case W:
default:
}

// this should report
//exhaustive:enforce
switch d { // want "^missing cases in switch of type Direction: E, directionInvalid$"
case N:
case S:
case W:
default:
}
}

func _nested() {
var d Direction

// this should not report.
switch d {
case N:
case S:
case W:
default:
// this should report.
//exhaustive:enforce
switch d { // want "^missing cases in switch of type Direction: E, directionInvalid$"
case N:
case S:
case W:
default:
}
}
}

func _reverse_nested() {
var d Direction

// this should report.
//exhaustive:enforce
switch d { // want "^missing cases in switch of type Direction: E, directionInvalid$"
case N:
case S:
case W:
default:
// this should not report.
switch d {
case N:
case S:
case W:
default:
}
}
}
32 changes: 16 additions & 16 deletions testdata/src/ignore-comment/ignore_comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,21 +65,21 @@ func _nested() {
}

func _reverse_nested() {
var d Direction
var d Direction

// this should report.
switch d { // want "^missing cases in switch of type Direction: E, directionInvalid$"
case N:
case S:
case W:
default:
// this should not report.
//exhaustive:ignore
switch d {
case N:
case S:
case W:
default:
}
}
// this should report.
switch d { // want "^missing cases in switch of type Direction: E, directionInvalid$"
case N:
case S:
case W:
default:
// this should not report.
//exhaustive:ignore
switch d {
case N:
case S:
case W:
default:
}
}
}