From 759658656c36b2dd4cbbbac26618d98b0a29a688 Mon Sep 17 00:00:00 2001 From: Nishanth Shanmugham Date: Sun, 14 Nov 2021 19:45:00 +0530 Subject: [PATCH] clean up --- comment.go | 4 ++-- enum_test.go | 19 +++++++++---------- exhaustive.go | 8 ++++---- exhaustive_test.go | 2 +- fact.go | 3 +-- fact_test.go | 1 - switch.go | 31 ++++++++++++++++--------------- switch_test.go | 3 +-- 8 files changed, 34 insertions(+), 37 deletions(-) diff --git a/comment.go b/comment.go index ea184d8..6f44256 100644 --- a/comment.go +++ b/comment.go @@ -44,10 +44,10 @@ func isGeneratedFile(file *ast.File) bool { return false } -var generatedCodeRx = regexp.MustCompile(`^// Code generated .* DO NOT EDIT\.$`) +var generatedCodeRe = regexp.MustCompile(`^// Code generated .* DO NOT EDIT\.$`) func isGeneratedFileComment(s string) bool { - return generatedCodeRx.MatchString(s) + return generatedCodeRe.MatchString(s) } // ignoreDirective is used to exclude checking of specific switch statements. diff --git a/enum_test.go b/enum_test.go index b3cb68c..f80995b 100644 --- a/enum_test.go +++ b/enum_test.go @@ -61,15 +61,6 @@ func TestEnumMembers_add(t *testing.T) { // TODO(testing): add tests for iota, repeated values, ... } -var testdataEnumPkg = func() *packages.Package { - cfg := &packages.Config{Mode: packages.NeedTypesInfo | packages.NeedTypes | packages.NeedSyntax} - pkgs, err := packages.Load(cfg, "./testdata/src/enum") - if err != nil { - panic(err) - } - return pkgs[0] -}() - func TestFindEnums(t *testing.T) { transform := func(in map[enumType]enumMembers) []checkEnum { var out []checkEnum @@ -79,6 +70,14 @@ func TestFindEnums(t *testing.T) { return out } + testdataEnumPkg := func() *packages.Package { + cfg := &packages.Config{Mode: packages.NeedTypesInfo | packages.NeedTypes | packages.NeedSyntax} + pkgs, err := packages.Load(cfg, "./testdata/src/enum") + if err != nil { + panic(err) + } + return pkgs[0] + }() inspect := inspector.New(testdataEnumPkg.Syntax) for _, pkgOnly := range [...]bool{false, true} { @@ -89,7 +88,7 @@ func TestFindEnums(t *testing.T) { } } -// See checkEnums. +// See func checkEnums. type checkEnum struct { typeName string members enumMembers diff --git a/exhaustive.go b/exhaustive.go index 5e12a65..31aa42b 100644 --- a/exhaustive.go +++ b/exhaustive.go @@ -195,7 +195,7 @@ func (v *regexpFlag) Set(expr string) error { func (v *regexpFlag) value() *regexp.Regexp { return v.r } func init() { - Analyzer.Flags.BoolVar(&fCheckGeneratedFiles, CheckGeneratedFlag, false, "check switch statements in generated files") + 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") Analyzer.Flags.BoolVar(&fPackageScopeOnly, PackageScopeOnlyFlag, false, "consider enums only in package scopes, not in inner scopes") @@ -218,7 +218,7 @@ const ( ) var ( - fCheckGeneratedFiles bool + fCheckGenerated bool fDefaultSignifiesExhaustive bool fIgnoreEnumMembers regexpFlag fPackageScopeOnly bool @@ -227,7 +227,7 @@ var ( // resetFlags resets the flag variables to their default values. // Useful in tests. func resetFlags() { - fCheckGeneratedFiles = false + fCheckGenerated = false fDefaultSignifiesExhaustive = false fIgnoreEnumMembers = regexpFlag{} fPackageScopeOnly = false @@ -250,7 +250,7 @@ func run(pass *analysis.Pass) (interface{}, error) { cfg := config{ defaultSignifiesExhaustive: fDefaultSignifiesExhaustive, - checkGeneratedFiles: fCheckGeneratedFiles, + checkGeneratedFiles: fCheckGenerated, ignoreEnumMembers: fIgnoreEnumMembers.value(), } checkSwitchStatements(pass, inspect, cfg) diff --git a/exhaustive_test.go b/exhaustive_test.go index c0707f7..11265ab 100644 --- a/exhaustive_test.go +++ b/exhaustive_test.go @@ -88,7 +88,7 @@ func TestExhaustive(t *testing.T) { // Tests for the -check-generated flag. run(t, "generated-file/check-generated-off/...") - run(t, "generated-file/check-generated-on/...", func() { fCheckGeneratedFiles = true }) + run(t, "generated-file/check-generated-on/...", func() { fCheckGenerated = true }) // Tests for the -default-signifies-exhaustive flag. // (For tests with this flag off, see other testdata packages diff --git a/fact.go b/fact.go index 5fc09be..ecf0907 100644 --- a/fact.go +++ b/fact.go @@ -21,8 +21,7 @@ func exportFact(pass *analysis.Pass, enumTyp enumType, members enumMembers) { // An (_, false) return indicates that the enum type is not a known one. func importFact(pass *analysis.Pass, possibleEnumType enumType) (enumMembers, bool) { var f enumMembersFact - ok := pass.ImportObjectFact(possibleEnumType.factObject(), &f) - if !ok { + if !pass.ImportObjectFact(possibleEnumType.factObject(), &f) { return enumMembers{}, false } return f.Members, true diff --git a/fact_test.go b/fact_test.go index e890c07..8cbe844 100644 --- a/fact_test.go +++ b/fact_test.go @@ -112,7 +112,6 @@ func checkOneFactType(t *testing.T, fact analysis.Fact) { checkTypeEnumMembersFact(t, reflect.TypeOf(v).Elem()) default: t.Errorf("unhandled type %T", v) - return } }) } diff --git a/switch.go b/switch.go index 22d74a4..e8e6ad5 100644 --- a/switch.go +++ b/switch.go @@ -39,6 +39,7 @@ const ( // switchStmtChecker returns a node visitor that checks exhaustiveness // of enum switch statements for the supplied pass, and reports diagnostics for // switch statements that are non-exhaustive. +// It expects to only see *ast.SwitchStmt nodes. func switchStmtChecker(pass *analysis.Pass, cfg config) nodeVisitor { generated := make(map[*ast.File]bool) // cached results comments := make(map[*ast.File]ast.CommentMap) // cached results @@ -108,7 +109,7 @@ func switchStmtChecker(pass *analysis.Pass, cfg config) nodeVisitor { checkUnexported := samePkg // we want to include unexported members in the exhaustiveness check only if we're in the same package checklist := makeChecklist(members, tagPkg, checkUnexported, cfg.ignoreEnumMembers) - hasDefaultCase := analyzeSwitchClauses(sw, tagPkg, members.NameToValue, pass.TypesInfo, func(val constantValue) { + hasDefaultCase := analyzeSwitchClauses(sw, pass.TypesInfo, func(val constantValue) { checklist.found(val) }) @@ -163,14 +164,11 @@ func denotesPackage(ident *ast.Ident, info *types.Info) (*types.Package, bool) { } // analyzeSwitchClauses analyzes the clauses in the supplied switch statement. -// -// tagPkg is the package of the switch statement's tag value's type. // The info param should typically be pass.TypesInfo. The found function is // called for each enum member name found in the switch statement. -// // The hasDefaultCase return value indicates whether the switch statement has a // default clause. -func analyzeSwitchClauses(sw *ast.SwitchStmt, tagPkg *types.Package, members map[string]constantValue, info *types.Info, found func(val constantValue)) (hasDefaultCase bool) { +func analyzeSwitchClauses(sw *ast.SwitchStmt, info *types.Info, found func(val constantValue)) (hasDefaultCase bool) { for _, stmt := range sw.Body.List { caseCl := stmt.(*ast.CaseClause) if isDefaultCase(caseCl) { @@ -178,13 +176,13 @@ func analyzeSwitchClauses(sw *ast.SwitchStmt, tagPkg *types.Package, members map continue // nothing more to do if it's the default case } for _, expr := range caseCl.List { - analyzeCaseClauseExpr(expr, tagPkg, members, info, found) + analyzeCaseClauseExpr(expr, info, found) } } return hasDefaultCase } -func analyzeCaseClauseExpr(e ast.Expr, tagPkg *types.Package, members map[string]constantValue, info *types.Info, found func(val constantValue)) { +func analyzeCaseClauseExpr(e ast.Expr, info *types.Info, found func(val constantValue)) { handleIdent := func(ident *ast.Ident) { obj := info.Uses[ident] if obj == nil { @@ -282,6 +280,9 @@ func diagnosticEnumTypeName(enumType *types.TypeName, samePkg bool) string { return enumType.Pkg().Name() + "." + enumType.Name() } +// Makes a "missing cases in switch" diagnostic. +// samePkg should be true if the enum type and the switch statement are defined +// in the same package. func makeDiagnostic(sw *ast.SwitchStmt, samePkg bool, enumTyp enumType, allMembers enumMembers, missingMembers map[string]struct{}) analysis.Diagnostic { message := fmt.Sprintf("missing cases in switch of type %s: %s", diagnosticEnumTypeName(enumTyp.TypeName, samePkg), @@ -304,12 +305,12 @@ func makeDiagnostic(sw *ast.SwitchStmt, samePkg bool, enumTyp enumType, allMembe // The remaining method returns the member names not accounted for. // type checklist struct { - em enumMembers - names map[string]struct{} + em enumMembers + checkl map[string]struct{} } func makeChecklist(em enumMembers, enumPkg *types.Package, includeUnexported bool, ignore *regexp.Regexp) *checklist { - names := make(map[string]struct{}) + checkl := make(map[string]struct{}) add := func(memberName string) { if memberName == "_" { @@ -325,7 +326,7 @@ func makeChecklist(em enumMembers, enumPkg *types.Package, includeUnexported boo if ignore != nil && ignore.MatchString(enumPkg.Path()+"."+memberName) { return } - names[memberName] = struct{}{} + checkl[memberName] = struct{}{} } for _, name := range em.Names { @@ -333,18 +334,18 @@ func makeChecklist(em enumMembers, enumPkg *types.Package, includeUnexported boo } return &checklist{ - em: em, - names: names, + em: em, + checkl: checkl, } } func (c *checklist) found(val constantValue) { // Delete all of the same-valued names. for _, name := range c.em.ValueToNames[val] { - delete(c.names, name) + delete(c.checkl, name) } } func (c *checklist) remaining() map[string]struct{} { - return c.names + return c.checkl } diff --git a/switch_test.go b/switch_test.go index edb3bba..f2d3dbf 100644 --- a/switch_test.go +++ b/switch_test.go @@ -148,10 +148,9 @@ func TestAnalyzeSwitchClauses(t *testing.T) { assertFoundNames := func(t *testing.T, sw *ast.SwitchStmt, info *types.Info, want []constantValue, wantDefaultExists bool) { t.Helper() - tagType := info.Types[sw.Tag].Type.(*types.Named) var got []constantValue - gotDefaultExists := analyzeSwitchClauses(sw, tagType.Obj().Pkg(), m, info, func(val constantValue) { + gotDefaultExists := analyzeSwitchClauses(sw, info, func(val constantValue) { got = append(got, val) })