refactor and unit test switch.go; various other changes

.travis.yml

@@ -1,6 +1,7 @@
 language: go
 
 go:
+  - 1.14
   - 1.x
   - master
 

CONTRIBUTING

@@ -0,0 +1,4 @@
+Contributing
+
+Pull requests are welcome. But please create a new issue (or comment on an
+existing issue) before working on a substantial change.

README.md

@@ -1,39 +1,39 @@
-## exhaustive [![Godoc][godoc-status]][godoc] [![Build Status][build-status]][build]
+# exhaustive [![Godoc][godoc-status]][godoc] [![Build Status][build-status]][build]
 
 The `exhaustive` package and the related command line program
 (`cmd/exhaustive`) can be used to check exhaustiveness of enum switch
 statements in Go code.
 
 An enum switch statement is exhaustive if it has cases for each of the
-enum type's members. See godoc for the definition of enum used by this
+enum's members. See Godoc for the definition of enum used by this
 package.
 
-### Install
+## Docs
 
-Install the command line program (with Go 1.16 or higher):
+https://godoc.org/github.com/nishanths/exhaustive
+
+## Known issues
+
+The package may not correctly handle enums that are [type
+aliases][4]. See issue [#13][5].
+
+## Install
+
+Install latest tagged release:
 
 ```
 go install github.com/nishanths/exhaustive/cmd/exhaustive@latest
 ```
 
-Install the package:
+Install latest `master`:
 
 ```
-go get github.com/nishanths/exhaustive
+go install github.com/nishanths/exhaustive/cmd/exhaustive@master
 ```
 
-### Known issues
-
-The package may not correctly handle enum types that are [type
-aliases][4]. See issue [#13][5].
-
-### Docs
-
-https://godoc.org/github.com/nishanths/exhaustive
+## Example
 
-### Example
-
-Given this enum type:
+Given this enum:
 
 ```diff
 package token
@@ -68,27 +68,22 @@ func processToken(t token.Token) {
 }
 ```
 
-Running the `exhaustive` command will print:
+Running the `exhaustive` command on the `calc` package will print:
 
 ```
 calc.go:6:2: missing cases in switch of type token.Token: Quotient, Remainder
 ```
 
 Enums can also be defined using explicit constant values instead of `iota`.
 
-### Integrate with analyzer driver programs
+## Integrate with analyzer driver programs
 
-The `exhaustive` package provides an `Analyzer` type that follows the
+The `exhaustive` package provides an `Analyzer` that follows the
 guidelines described in the [go/analysis][3] package; this should make
 it possible to integrate `exhaustive` into analysis driver
 programs.
 
-### Contributing
-
-Pull requests are welcome! But please create a new issue or comment on
-an existing issue before making a pull request for a new feature.
-
-### License
+## License
 
 BSD 2-Clause
 

cmd/exhaustive/exhaustive.go

@@ -1,6 +1,6 @@
-// Command exhaustive can find enum switch statements that are non-exhaustive.
+// Command exhaustive checks exhaustiveness of enum switch statements.
 //
-// For documentation, see https://godoc.org/github.com/nishanths/exhaustive.
+// For documentation see https://godoc.org/github.com/nishanths/exhaustive.
 package main
 
 import (

doc.go

@@ -1,10 +1,6 @@
 /*
 Package exhaustive provides an analyzer that checks exhaustiveness of enum
-switch statements.
-
-The analyzer also provides fixes to make the offending
-switch statements exhaustive (see "Fixes" section).
-See "cmd/exhaustive" subpackage for the related command line program.
+switch statements in Go code.
 
 Definition of enum
 
@@ -15,8 +11,10 @@ a string type. An enum type must have associated with it one or more
 package-level variables of the named type in the package. These variables
 constitute the enum's members.
 
-In the code snippet below, Biome is an enum type with 3 members. (You may
-also use iota instead of explicitly specifying values.)
+In the code snippet below, Biome is an enum type with 3 members. Enum values may
+be specified using iota (they don't have to be explicit values, like in the
+snippet), and enum members don't necessarily have to all be defined in the same
+var or const block.
 
   type Biome int
 
@@ -32,65 +30,80 @@ An enum switch statement is exhaustive if it has cases for each of the enum's
 members.
 
 For an enum type defined in the same package as the switch statement, both
-exported and unexported enum members must be present in order to consider
-the switch exhaustive. On the other hand, for an enum type defined
-in an external package it is sufficient for just exported enum members
-to be present in order to consider the switch exhaustive.
+exported and unexported enum members must be present in order to consider the
+switch exhaustive. For an enum type defined in an external package it is
+sufficient for just the exported enum members to be present in order to consider
+the switch exhaustive.
 
-Flags
+There are two strategies for checking exhaustiveness: the "value" strategy
+(which is the default) and the "name" strategy. The "value" strategy requires
+that each independent enum value is listed in a switch statement to satisfy
+exhaustiveness. The "name" strategy requires that each independent enum member
+name is listed in a switch statement to satisfy exhaustiveness.
 
-The analyzer accepts the following flags.(The analysis package provides
-additional flags, such as -fix.)
+To illustrate the difference between the strategies, consider the following enum
+type and switch statement. The switch statement is not exhaustive when using the
+"name" strategy (because the name AccessDefault is not listed). But it is
+exhaustive when using the "value" strategy (because AccessDefault and AccessAll
+have the same value).
 
-The -default-signifies-exhaustive boolean flag indicates to the analyzer
-whether switch statements are to be considered exhaustive as long as a
-'default' case is present (even if all enum members aren't listed in the
-switch statements cases). The default value is false.
+  type AccessControl string
 
-The -check-generated boolean flag indicates whether to check switch
-statements in generated Go source files. The default value is false.
+  const (
+      AccessAll     AccessControl = "all"
+      AccessAny     AccessControl = "any"
+      AccessDefault AccessControl = All
+  )
 
-The -ignore-pattern flag specifies a regular expression. Member names
-in enum definitions that match the regular expression do not require a case
-clause to satisfy exhaustiveness. The regular expression is matched against
-enum member names inclusive of the import path, e.g. of the
-form "github.com/foo/bar.Tundra", where "github.com/foo/bar" is the import path
-and "Tundra" is the enum member name.
+  func example(v AccessControl) {
+      switch v {
+          case AccessAll:
+          case AccessAny:
+      }
+  }
 
-Fixes
+The exhaustiveness checking strategy can be controlled by the
+"-checking-strategy" flag described in the next section.
 
-The analyzer suggests fixes for a switch statement if it is not exhaustive.
-The suggested fix always adds a single case clause for the missing enum member
-values.
+Notable flags
 
-  case MissingA, MissingB, MissingC:
-      panic(fmt.Sprintf("unhandled value: %v", v))
+The "-default-signifies-exhaustive" boolean flag indicates to the analyzer
+whether switch statements are to be considered exhaustive—even if all enum
+members aren't listed in the switch statements cases—as long as a 'default' case
+is present. The default value for the flag is false.
 
-where v is the expression in the switch statement's tag (in other words, the
-value being switched upon). If the switch statement's tag is a function or a
-method call the analyzer does not suggest a fix, as reusing the call expression
-in the panic/fmt.Sprintf call could be mutative.
+The "-check-generated" boolean flag indicates whether to check switch statements
+in generated Go source files. The default value for the flag is false.
 
-The rationale for the fix using panic is that it might be better to fail loudly
-on existing unhandled or impossible cases than to let them slip by quietly
-unnoticed. An even better fix may, of course, be to manually inspect the sites
-reported by the package and handle the missing cases if necessary.
+The "-ignore-enum-members" flag specifies a regular expression. Enum members
+that match the regular expression do need to be listed in switch statements in
+order for switch statements to be considered exhaustive. The supplied
+regular expression is matched against the enum package's import path and the
+enum member name combined in the following format: <import path>.<enum member
+name>. For example: "github.com/foo/bar.Tundra", where the enum package's import
+path is "github.com/foo/bar" and the enum member name is "Tundra".
 
-Imports will be adjusted automatically to account for the "fmt" dependency.
+The "-checking-strategy" flag specifies the checking strategy to use. The flag
+value must be either "value" (which is the default) or "name". See discussion
+in the "Defintion of exhaustiveness" section for more details.
 
 Skipping analysis
 
 If the following directive comment:
 
   //exhaustive:ignore
 
-is associated with a switch statement, the analyzer skips
-checking of the switch statement and no diagnostics are reported.
+is associated with a switch statement, the analyzer skips checking of the switch
+statement and no diagnostics are reported. Note the lack of whitespace between
+the comment marker ("//") and the comment text.
 
-No diagnostics are reported for switch statements in
-generated files (see https://golang.org/s/generatedcode for definition of
-generated file), unless the -check-generated flag is enabled.
+Additionally, no diagnostics are reported for switch statements in generated
+files unless the "-check-generated" flag is enabled. (See
+https://golang.org/s/generatedcode for definition of generated file).
 
-Additionally, see the -ignore-pattern flag.
+Additionally, see the "-ignore-enum-members" flag.
 */
 package exhaustive
+
+// TODO: add docs for upcoming -by-name flag.
+// TODO: add docs to "Definition of exhaustiveness" section for by value vs. by name checking.

enum.go

@@ -22,7 +22,7 @@ type enumMembers struct {
 	NameToValue map[string]string
 
 	// ValueToNames maps (constant.Value).ExactString() -> member names.
-	// Note the use of []string for the value type of the map: Multiple
+	// Note the use of []string for the element type of the map: Multiple
 	// names can have the same value.
 	// Names that don't have a constant.Value defined in the AST (e.g. some
 	// iota constants) will not have a corresponding entry in this map.
@@ -49,10 +49,6 @@ func (em *enumMembers) add(name string, constVal *string) {
 	}
 }
 
-func (em *enumMembers) numMembers() int {
-	return len(em.Names)
-}
-
 // Find the enums for the files in a package. The files is typically obtained from
 // pass.Files and typesInfo is obtained from pass.TypesInfo.
 func findEnums(files []*ast.File, typesInfo *types.Info) enums {

enum_test.go

@@ -95,7 +95,7 @@ func TestFindEnums(t *testing.T) {
 	checkEnums(t, result)
 }
 
-// shared by TestFindEnumMembers and TestFindEnums.
+// shared utility for TestFindEnumMembers and TestFindEnums.
 func checkEnums(t *testing.T, got map[string]*enumMembers) {
 	t.Helper()
 

exhaustive.go

@@ -1,6 +1,9 @@
 package exhaustive
 
 import (
+	"fmt"
+	"regexp"
+
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/passes/inspect"
 	"golang.org/x/tools/go/ast/inspector"
@@ -11,27 +14,35 @@ import (
 const (
 	DefaultSignifiesExhaustiveFlag = "default-signifies-exhaustive"
 	CheckGeneratedFlag             = "check-generated"
-	IgnorePatternFlag              = "ignore-pattern"
+	IgnorePatternFlag              = "ignore-pattern" // Deprecated. See IgnoreEnumMembersFlag instead.
+	IgnoreEnumMembersFlag          = "ignore-enum-members"
+	CheckingStrategyFlag           = "checking-strategy"
 )
 
 var (
 	fDefaultSignifiesExhaustive bool
 	fCheckGeneratedFiles        bool
-	fIgnorePattern              regexpFlag
+	fDeprecatedIgnorePattern    string // Deprecated.
+	fIgnoreEnumMembers          regexpFlag
+	fCheckingStrategy           string
 )
 
 func init() {
-	Analyzer.Flags.BoolVar(&fDefaultSignifiesExhaustive, DefaultSignifiesExhaustiveFlag, false, "indicates that switch statements are to be considered exhaustive if a 'default' case is present, even if all enum members aren't listed in the switch")
-	Analyzer.Flags.BoolVar(&fCheckGeneratedFiles, CheckGeneratedFlag, false, "check switch statements in generated files also")
-	Analyzer.Flags.Var(&fIgnorePattern, IgnorePatternFlag, "do not require a case clause to satisfy exhaustiveness for enum member names that match the provided regular expression pattern")
+	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.BoolVar(&fCheckGeneratedFiles, CheckGeneratedFlag, false, "check switch statements in generated files")
+	Analyzer.Flags.StringVar(&fDeprecatedIgnorePattern, IgnorePatternFlag, "", "no effect (deprecated); see -"+IgnoreEnumMembersFlag+" instead")
+	Analyzer.Flags.Var(&fIgnoreEnumMembers, IgnoreEnumMembersFlag, "enum members matching `regex` do not have to be listed in switch statements to satisfy exhaustiveness")
+	Analyzer.Flags.StringVar(&fCheckingStrategy, CheckingStrategyFlag, "value", "`strategy` to use when checking exhaustiveness of switch statements; one of: \"value\", \"name\"")
 }
 
 // resetFlags resets the flag variables to their default values.
 // Useful in tests.
 func resetFlags() {
 	fDefaultSignifiesExhaustive = false
 	fCheckGeneratedFiles = false
-	fIgnorePattern = regexpFlag{}
+	fDeprecatedIgnorePattern = ""
+	fIgnoreEnumMembers = regexpFlag{}
+	fCheckingStrategy = "value"
 }
 
 var Analyzer = &analysis.Analyzer{
@@ -48,7 +59,31 @@ func run(pass *analysis.Pass) (interface{}, error) {
 		pass.ExportPackageFact(&enumsFact{Enums: e})
 	}
 
+	strategy, err := determineCheckingStrategy()
+	if err != nil {
+		return nil, err
+	}
+
 	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
-	err := checkSwitchStatements(pass, inspect, byValue) // TODO: support other hitlist strategies via a user-specified flag
+	cfg := config{
+		defaultSignifiesExhaustive: fDefaultSignifiesExhaustive,
+		checkGeneratedFiles:        fCheckGeneratedFiles,
+		ignoreEnumMembers:          fIgnoreEnumMembers.Get().(*regexp.Regexp),
+		checkingStrategy:           strategy,
+	}
+
+	err = checkSwitchStatements(pass, inspect, cfg)
 	return nil, err
 }
+
+// Determine the checkingStrategy from flags.
+func determineCheckingStrategy() (checkingStrategy, error) {
+	switch fCheckingStrategy {
+	case "value":
+		return strategyValue, nil
+	case "name":
+		return strategyName, nil
+	default:
+		return 0, fmt.Errorf("bad value %q for flag -%s", fCheckingStrategy, CheckingStrategyFlag)
+	}
+}