From adcadef3aa804019b73463cda16d4b0df72babd7 Mon Sep 17 00:00:00 2001 From: Leon Klingele Date: Wed, 19 Jan 2022 22:41:27 +0100 Subject: [PATCH] pkg/analyzers: add 'var' analyzer --- README.md | 5 + pkg/analyzer/analyzer.go | 12 ++- pkg/analyzer/analyzer_test.go | 95 ++++++++++++++++++ pkg/analyzer/config.go | 2 + pkg/analyzer/flags.go | 6 ++ .../src/mixed-named-with-consts/testcase.go | 52 ++++++++++ .../testcase.go | 55 +++++++++++ .../src/mixed-require-grouping/testcase.go | 37 +++++++ .../src/mixed-require-single-var/testcase.go | 37 +++++++ .../var/src/multi-grouped/testcase.go | 15 +++ .../var/src/multi-ungrouped/testcase.go | 14 +++ .../var/src/single-grouped/testcase.go | 11 +++ .../var/src/single-ungrouped/testcase.go | 5 + pkg/analyzer/vars/analyzer.go | 98 +++++++++++++++++++ pkg/analyzer/vars/config.go | 6 ++ 15 files changed, 449 insertions(+), 1 deletion(-) create mode 100644 pkg/analyzer/testdata/var/src/mixed-named-with-consts/testcase.go create mode 100644 pkg/analyzer/testdata/var/src/mixed-named-with-var-shorthand/testcase.go create mode 100644 pkg/analyzer/testdata/var/src/mixed-require-grouping/testcase.go create mode 100644 pkg/analyzer/testdata/var/src/mixed-require-single-var/testcase.go create mode 100644 pkg/analyzer/testdata/var/src/multi-grouped/testcase.go create mode 100644 pkg/analyzer/testdata/var/src/multi-ungrouped/testcase.go create mode 100644 pkg/analyzer/testdata/var/src/single-grouped/testcase.go create mode 100644 pkg/analyzer/testdata/var/src/single-ungrouped/testcase.go create mode 100644 pkg/analyzer/vars/analyzer.go create mode 100644 pkg/analyzer/vars/config.go diff --git a/README.md b/README.md index bf67e45..7bc7e98 100644 --- a/README.md +++ b/README.md @@ -34,4 +34,9 @@ GOPATH/src/github.com/leonklingele/grouper/pkg/analyzer/flags.go:3:1: should onl require the use of grouped global 'type' declarations -type-require-single-type require the use of a single global 'type' declaration only + + -var-require-grouping + require the use of grouped global 'var' declarations + -var-require-single-var + require the use of a single global 'var' declaration only ``` diff --git a/pkg/analyzer/analyzer.go b/pkg/analyzer/analyzer.go index f531d06..9852c78 100644 --- a/pkg/analyzer/analyzer.go +++ b/pkg/analyzer/analyzer.go @@ -7,6 +7,7 @@ import ( "github.com/leonklingele/grouper/pkg/analyzer/consts" "github.com/leonklingele/grouper/pkg/analyzer/imports" "github.com/leonklingele/grouper/pkg/analyzer/types" + "github.com/leonklingele/grouper/pkg/analyzer/vars" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/inspect" @@ -14,7 +15,7 @@ import ( const ( Name = "grouper" - Doc = `expression group analyzer: require 'import', 'const' and/or 'var' declaration groups` + Doc = `expression group analyzer: require 'import', 'const', 'var' and/or 'type' declaration groups` ) func New() *analysis.Analyzer { @@ -47,6 +48,11 @@ func run(p *analysis.Pass) (interface{}, error) { RequireSingleType: flagLookupBool(FlagNameTypeRequireSingleType), RequireGrouping: flagLookupBool(FlagNameTypeRequireGrouping), }, + + VarsConfig: &vars.Config{ + RequireSingleVar: flagLookupBool(FlagNameVarRequireSingleVar), + RequireGrouping: flagLookupBool(FlagNameVarRequireGrouping), + }, } return nil, pass(c, p) @@ -75,5 +81,9 @@ func filepass(c *Config, p *analysis.Pass, f *ast.File) error { return fmt.Errorf("failed to types.Filepass: %w", err) } + if err := vars.Filepass(c.VarsConfig, p, f); err != nil { + return fmt.Errorf("failed to vars.Filepass: %w", err) + } + return nil } diff --git a/pkg/analyzer/analyzer_test.go b/pkg/analyzer/analyzer_test.go index 98d9718..1be36db 100644 --- a/pkg/analyzer/analyzer_test.go +++ b/pkg/analyzer/analyzer_test.go @@ -225,6 +225,85 @@ func TestType(t *testing.T) { } } +func TestVar(t *testing.T) { + t.Parallel() + + fixtures := []struct { + name string + flags flag.FlagSet + }{ + { + name: "single-grouped", + flags: flags(). + withVarRequireGrouping(). + build(), + }, + { + name: "single-ungrouped", + flags: flags(). + withVarRequireGrouping(). + build(), + }, + + { + name: "multi-grouped", + flags: flags(). + withVarRequireSingleVar(). + withVarRequireGrouping(). + build(), + }, + { + name: "multi-ungrouped", + flags: flags(). + withVarRequireSingleVar(). + withVarRequireGrouping(). + build(), + }, + + { + name: "mixed-require-single-var", + flags: flags(). + withVarRequireSingleVar(). + build(), + }, + { + name: "mixed-require-grouping", + flags: flags(). + withVarRequireGrouping(). + build(), + }, + + { + name: "mixed-named-with-consts", + flags: flags(). + withVarRequireSingleVar(). + withVarRequireGrouping(). + build(), + }, + { + name: "mixed-named-with-var-shorthand", + flags: flags(). + withVarRequireSingleVar(). + withVarRequireGrouping(). + build(), + }, + } + + for _, f := range fixtures { + f := f + + t.Run(f.name, func(t *testing.T) { + t.Parallel() + + a := analyzer.New() + a.Flags = f.flags + + testdata := filepath.Join(analysistest.TestData(), "var") + _ = analysistest.Run(t, testdata, a, f.name) + }) + } +} + type flagger struct { fs *flag.FlagSet } @@ -277,6 +356,22 @@ func (f *flagger) withTypeRequireGrouping() *flagger { return f } +func (f *flagger) withVarRequireSingleVar() *flagger { + if err := f.fs.Lookup(analyzer.FlagNameVarRequireSingleVar).Value.Set("true"); err != nil { + panic(err) + } + + return f +} + +func (f *flagger) withVarRequireGrouping() *flagger { + if err := f.fs.Lookup(analyzer.FlagNameVarRequireGrouping).Value.Set("true"); err != nil { + panic(err) + } + + return f +} + func (f *flagger) build() flag.FlagSet { return *f.fs } diff --git a/pkg/analyzer/config.go b/pkg/analyzer/config.go index 70ef82e..b00595f 100644 --- a/pkg/analyzer/config.go +++ b/pkg/analyzer/config.go @@ -4,10 +4,12 @@ import ( "github.com/leonklingele/grouper/pkg/analyzer/consts" "github.com/leonklingele/grouper/pkg/analyzer/imports" "github.com/leonklingele/grouper/pkg/analyzer/types" + "github.com/leonklingele/grouper/pkg/analyzer/vars" ) type Config struct { ConstsConfig *consts.Config ImportsConfig *imports.Config TypesConfig *types.Config + VarsConfig *vars.Config } diff --git a/pkg/analyzer/flags.go b/pkg/analyzer/flags.go index b353ccf..42447cb 100644 --- a/pkg/analyzer/flags.go +++ b/pkg/analyzer/flags.go @@ -13,6 +13,9 @@ const ( FlagNameTypeRequireSingleType = "type-require-single-type" FlagNameTypeRequireGrouping = "type-require-grouping" + + FlagNameVarRequireSingleVar = "var-require-single-var" + FlagNameVarRequireGrouping = "var-require-grouping" ) func Flags() flag.FlagSet { @@ -27,5 +30,8 @@ func Flags() flag.FlagSet { fs.Bool(FlagNameTypeRequireSingleType, false, "require the use of a single global 'type' declaration only") fs.Bool(FlagNameTypeRequireGrouping, false, "require the use of grouped global 'type' declarations") + fs.Bool(FlagNameVarRequireSingleVar, false, "require the use of a single global 'var' declaration only") + fs.Bool(FlagNameVarRequireGrouping, false, "require the use of grouped global 'var' declarations") + return *fs } diff --git a/pkg/analyzer/testdata/var/src/mixed-named-with-consts/testcase.go b/pkg/analyzer/testdata/var/src/mixed-named-with-consts/testcase.go new file mode 100644 index 0000000..8084af1 --- /dev/null +++ b/pkg/analyzer/testdata/var/src/mixed-named-with-consts/testcase.go @@ -0,0 +1,52 @@ +package testcase + +var ( + a = "a" + b = "b" + + _ = "underscore1" +) + +var ( // want "should only use a single global 'var' declaration, 5 found" + comment1 = "comment1" // some comment +) + +const ignoreconst1 = "ignoreconst1" +const ( + ignoreconst2 = "ignoreconst2" + ignoreconst3 = "ignoreconst3" +) + +func dummy() { + var ( + ignorea = "ignorea" + ignoreb = "ignoreb" + + _ = "ignoreunderscore1" + ) + + var ( + ignorecomment1 = "ignorecomment1" // some comment + ) + + var ignorec = "ignorec" + var _ = "ignoreunderscore2" + + var () + + _ = a + _ = b + _ = c + _ = comment1 + _ = comment2 + _ = ignorecomment1 + _ = ignorea + _ = ignoreb + _ = ignorec +} + +var c = "c" // want "should only use grouped global 'var' declarations" + +var comment2 = "comment2" // some comment + +var () diff --git a/pkg/analyzer/testdata/var/src/mixed-named-with-var-shorthand/testcase.go b/pkg/analyzer/testdata/var/src/mixed-named-with-var-shorthand/testcase.go new file mode 100644 index 0000000..fbae6b3 --- /dev/null +++ b/pkg/analyzer/testdata/var/src/mixed-named-with-var-shorthand/testcase.go @@ -0,0 +1,55 @@ +package testcase + +var ( + a = "a" + b = "b" + + _ = "underscore1" +) + +var ( // want "should only use a single global 'var' declaration, 5 found" + comment1 = "comment1" // some comment +) + +const ignoreconst1 = "ignoreconst1" +const ( + ignoreconst2 = "ignoreconst2" + ignoreconst3 = "ignoreconst3" +) + +func dummy() { + var ( + ignorea = "ignorea" + ignoreb = "ignoreb" + + _ = "ignoreunderscore1" + ) + + var ( + ignorecomment1 = "ignorecomment1" // some comment + ) + + var ignorec = "ignorec" + var _ = "ignoreunderscore2" + + var () + + _ = a + _ = b + _ = c + _ = comment1 + _ = comment2 + _ = ignorecomment1 + _ = ignorea + _ = ignoreb + _ = ignorec + + hello := "world" + println(hello) +} + +var c = "c" // want "should only use grouped global 'var' declarations" + +var comment2 = "comment2" // some comment + +var () diff --git a/pkg/analyzer/testdata/var/src/mixed-require-grouping/testcase.go b/pkg/analyzer/testdata/var/src/mixed-require-grouping/testcase.go new file mode 100644 index 0000000..19fffd6 --- /dev/null +++ b/pkg/analyzer/testdata/var/src/mixed-require-grouping/testcase.go @@ -0,0 +1,37 @@ +package testcase + +var ( + a = "a" + b = "b" + + _ = "underscore1" +) + +var ( + comment1 = "comment1" // some comment +) + +var c = "c" // want "should only use grouped global 'var' declarations" + +var comment2 = "comment2" // some comment + +var () + +func dummy() { + var ( + _ = "ignore1" + _ = "ignore2" + ) + + var ( + _ = "ignore3" + ) + + var d = "d" + var comment3 = "comment3" // some comment + + println(d) + println(comment3) + + var () +} diff --git a/pkg/analyzer/testdata/var/src/mixed-require-single-var/testcase.go b/pkg/analyzer/testdata/var/src/mixed-require-single-var/testcase.go new file mode 100644 index 0000000..fc3ff9b --- /dev/null +++ b/pkg/analyzer/testdata/var/src/mixed-require-single-var/testcase.go @@ -0,0 +1,37 @@ +package testcase + +var ( + a = "a" + b = "b" + + _ = "underscore1" +) + +var ( // want "should only use a single global 'var' declaration, 5 found" + comment1 = "comment1" // some comment +) + +var c = "c" + +var comment2 = "comment2" // some comment + +var () + +func dummy() { + var ( + _ = "ignore1" + _ = "ignore2" + ) + + var ( + _ = "ignore3" + ) + + var d = "d" + var comment3 = "comment3" // some comment + + println(d) + println(comment3) + + var () +} diff --git a/pkg/analyzer/testdata/var/src/multi-grouped/testcase.go b/pkg/analyzer/testdata/var/src/multi-grouped/testcase.go new file mode 100644 index 0000000..3cbdf03 --- /dev/null +++ b/pkg/analyzer/testdata/var/src/multi-grouped/testcase.go @@ -0,0 +1,15 @@ +package testcase + +var ( + a = "a" + b = "b" + + _ = "underscore1" +) + +func dummy() { + var ( + _ = "ignore1" + _ = "ignore2" + ) +} diff --git a/pkg/analyzer/testdata/var/src/multi-ungrouped/testcase.go b/pkg/analyzer/testdata/var/src/multi-ungrouped/testcase.go new file mode 100644 index 0000000..8920ef2 --- /dev/null +++ b/pkg/analyzer/testdata/var/src/multi-ungrouped/testcase.go @@ -0,0 +1,14 @@ +package testcase + +var a = "a" // want "should only use grouped global 'var' declarations" +var b = "b" // want "should only use a single global 'var' declaration, 3 found" + +var _ = "underscore1" + +func dummy() { + var _ = "ignore1" + var _ = "ignore2" + + println(a) + println(b) +} diff --git a/pkg/analyzer/testdata/var/src/single-grouped/testcase.go b/pkg/analyzer/testdata/var/src/single-grouped/testcase.go new file mode 100644 index 0000000..c0a14b4 --- /dev/null +++ b/pkg/analyzer/testdata/var/src/single-grouped/testcase.go @@ -0,0 +1,11 @@ +package testcase + +var ( + a = "a" +) + +func dummy() { + var ( + _ = "ignore1" + ) +} diff --git a/pkg/analyzer/testdata/var/src/single-ungrouped/testcase.go b/pkg/analyzer/testdata/var/src/single-ungrouped/testcase.go new file mode 100644 index 0000000..8afce07 --- /dev/null +++ b/pkg/analyzer/testdata/var/src/single-ungrouped/testcase.go @@ -0,0 +1,5 @@ +package testcase + +var a = "a" // want "should only use grouped global 'var' declarations" + +func dummy() { var _ = "ignore"; println(a) } diff --git a/pkg/analyzer/vars/analyzer.go b/pkg/analyzer/vars/analyzer.go new file mode 100644 index 0000000..d55737a --- /dev/null +++ b/pkg/analyzer/vars/analyzer.go @@ -0,0 +1,98 @@ +package vars + +import ( + "fmt" + "go/ast" + "go/token" + + "golang.org/x/tools/go/analysis" +) + +// https://go.dev/ref/spec#Variable_declarations + +type Var struct { + Decl *ast.GenDecl + IsGroup bool +} + +func Filepass(c *Config, p *analysis.Pass, f *ast.File) error { + var vars []*Var + for _, decl := range f.Decls { + genDecl, ok := decl.(*ast.GenDecl) + if !ok { + continue + } + + if genDecl.Tok == token.VAR { + vars = append(vars, &Var{ + Decl: genDecl, + IsGroup: genDecl.Lparen != 0, + }) + } + } + + numVars := len(vars) + if numVars == 0 { + // Bail out early + return nil + } + + if c.RequireSingleVar && numVars > 1 { + msg := fmt.Sprintf("should only use a single global 'var' declaration, %d found", numVars) + firstdup := vars[1] + decl := firstdup.Decl + + p.Report(analysis.Diagnostic{ //nolint:exhaustivestruct // we do not need all fields + Pos: decl.Pos(), + End: decl.End(), + Message: msg, + Related: toRelated(vars[1:]), + // TODO(leon): Suggest fix + }) + } + + if c.RequireGrouping { + var ungroupedVars []*Var + for _, c := range vars { + if !c.IsGroup { + ungroupedVars = append(ungroupedVars, c) + } + } + + if numUngroupedVars := len(ungroupedVars); numUngroupedVars != 0 { + msg := "should only use grouped global 'var' declarations" + firstmatch := ungroupedVars[0] + decl := firstmatch.Decl + + report := analysis.Diagnostic{ //nolint:exhaustivestruct // we do not need all fields + Pos: decl.Pos(), + End: decl.End(), + Message: msg, + // TODO(leon): Suggest fix + } + + if numUngroupedVars > 1 { + report.Related = toRelated(ungroupedVars[1:]) + } + + p.Report(report) + } + } + + return nil +} + +func toRelated(vars []*Var) []analysis.RelatedInformation { + related := make([]analysis.RelatedInformation, 0, len(vars)) + for _, c := range vars { + decl := c.Decl + + related = append(related, analysis.RelatedInformation{ + Pos: decl.Pos(), + End: decl.End(), + Message: "found here", + }) + } + + return related +} diff --git a/pkg/analyzer/vars/config.go b/pkg/analyzer/vars/config.go new file mode 100644 index 0000000..4c7c1d8 --- /dev/null +++ b/pkg/analyzer/vars/config.go @@ -0,0 +1,6 @@ +package vars + +type Config struct { + RequireSingleVar bool // Require the use of a single global 'var' declaration only + RequireGrouping bool // Require the use of grouped global 'var' declarations +}