From 0af6cacb28e4cddcefd6d87d036ec1016c94eace Mon Sep 17 00:00:00 2001 From: golangci Date: Mon, 7 May 2018 12:43:52 +0300 Subject: [PATCH] support goconst --- Gopkg.lock | 8 +- internal/commands/run.go | 10 +- pkg/config/config.go | 4 + pkg/enabled_linters.go | 11 +- pkg/golinters/goconst.go | 44 +++++ pkg/golinters/linters_test.go | 2 +- pkg/golinters/testdata/goconst.go | 30 +++ vendor/github.com/golangci/goconst/LICENSE | 21 ++ vendor/github.com/golangci/goconst/README.md | 49 +++++ vendor/github.com/golangci/goconst/parser.go | 187 ++++++++++++++++++ vendor/github.com/golangci/goconst/visitor.go | 143 ++++++++++++++ 11 files changed, 500 insertions(+), 9 deletions(-) create mode 100644 pkg/golinters/goconst.go create mode 100644 pkg/golinters/testdata/goconst.go create mode 100644 vendor/github.com/golangci/goconst/LICENSE create mode 100644 vendor/github.com/golangci/goconst/README.md create mode 100644 vendor/github.com/golangci/goconst/parser.go create mode 100644 vendor/github.com/golangci/goconst/visitor.go diff --git a/Gopkg.lock b/Gopkg.lock index 6ddc13ce..04fa087c 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -74,6 +74,12 @@ ] revision = "f557f368b7f3d00a54ed44eb57b9eca59e85cee1" +[[projects]] + name = "github.com/golangci/goconst" + packages = ["."] + revision = "b67b9035d29a7561902d87eb3757dd490b31c1b1" + branch = "master" + [[projects]] branch = "master" name = "github.com/golangci/gocyclo" @@ -300,6 +306,6 @@ [solve-meta] analyzer-name = "dep" analyzer-version = 1 - inputs-digest = "3c773b17692c1bc6ce7d565b959908aa95160e5d4f2486721deaff48608c864e" + inputs-digest = "33466d605f73dcfcecf89a0d3a46aaa719366433cdd6080d00f8476fe77e747f" solver-name = "gps-cdcl" solver-version = 1 diff --git a/internal/commands/run.go b/internal/commands/run.go index 77408071..f1f07aa2 100644 --- a/internal/commands/run.go +++ b/internal/commands/run.go @@ -52,7 +52,7 @@ func (e *Executor) initRun() { runCmd.Flags().BoolVar(&rc.Gofmt.Simplify, "gofmt.simplify", true, "Gofmt: simplify code") runCmd.Flags().IntVar(&rc.Gocyclo.MinComplexity, "gocyclo.min-complexity", - 20, "Minimal complexity of function to report it") + 50, "Minimal complexity of function to report it") runCmd.Flags().BoolVar(&rc.Structcheck.CheckExportedFields, "structcheck.exported-fields", false, "Structcheck: report about unused exported struct fields") runCmd.Flags().BoolVar(&rc.Varcheck.CheckExportedFields, "varcheck.exported-fields", false, "Varcheck: report about unused exported variables") @@ -62,8 +62,14 @@ func (e *Executor) initRun() { runCmd.Flags().BoolVar(&rc.Megacheck.EnableStaticcheck, "megacheck.staticcheck", true, "Megacheck: run Staticcheck sub-linter: staticcheck is go vet on steroids, applying a ton of static analysis checks") runCmd.Flags().BoolVar(&rc.Megacheck.EnableGosimple, "megacheck.gosimple", true, "Megacheck: run Gosimple sub-linter: gosimple is a linter for Go source code that specialises on simplifying code") runCmd.Flags().BoolVar(&rc.Megacheck.EnableUnused, "megacheck.unused", true, "Megacheck: run Unused sub-linter: unused checks Go code for unused constants, variables, functions and types") + runCmd.Flags().IntVar(&rc.Dupl.Threshold, "dupl.threshold", - 20, "Minimal threshold to detect copy-paste") + 20, "Dupl: Minimal threshold to detect copy-paste") + + runCmd.Flags().IntVar(&rc.Goconst.MinStringLen, "goconst.min-len", + 3, "Goconst: minimum constant string length") + runCmd.Flags().IntVar(&rc.Goconst.MinOccurrencesCount, "goconst.min-occurrences", + 3, "Goconst: minimum occurences of constant string count to trigger issue") runCmd.Flags().StringSliceVarP(&rc.EnabledLinters, "enable", "E", []string{}, "Enable specific linter") runCmd.Flags().StringSliceVarP(&rc.DisabledLinters, "disable", "D", []string{}, "Disable specific linter") diff --git a/pkg/config/config.go b/pkg/config/config.go index 70736278..1178cbde 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -64,6 +64,10 @@ type Run struct { Dupl struct { Threshold int } + Goconst struct { + MinStringLen int + MinOccurrencesCount int + } EnabledLinters []string DisabledLinters []string diff --git a/pkg/enabled_linters.go b/pkg/enabled_linters.go index a994f227..b2a78977 100644 --- a/pkg/enabled_linters.go +++ b/pkg/enabled_linters.go @@ -62,15 +62,16 @@ func GetAllSupportedLinterConfigs() []LinterConfig { enabledByDefault(golinters.Govet{}, "Vet examines Go source code and reports suspicious constructs, such as Printf calls whose arguments do not align with the format string", false, false), enabledByDefault(golinters.Errcheck{}, "Errcheck is a program for checking for unchecked errors in go programs. These unchecked errors can be critical bugs in some cases", true, false), enabledByDefault(golinters.Golint{}, "Golint differs from gofmt. Gofmt reformats Go source code, whereas golint prints out style mistakes", false, false), - enabledByDefault(golinters.Deadcode{}, "Finds unused code", true, false), - enabledByDefault(golinters.Gocyclo{}, "Computes and checks the cyclomatic complexity of functions", false, false), + enabledByDefault(golinters.Megacheck{}, "Megacheck: 3 sub-linters in one: staticcheck, gosimple and unused", true, true), enabledByDefault(golinters.Structcheck{}, "Finds unused struct fields", true, false), enabledByDefault(golinters.Varcheck{}, "Finds unused global variables and constants", true, false), - enabledByDefault(golinters.Megacheck{}, "Megacheck: 3 sub-linters in one: staticcheck, gosimple and unused", true, true), - enabledByDefault(golinters.Dupl{}, "Tool for code clone detection", false, false), - enabledByDefault(golinters.Ineffassign{}, "Detects when assignments to existing variables are not used", false, false), enabledByDefault(golinters.Interfacer{}, "Linter that suggests narrower interface types", true, true), enabledByDefault(golinters.Unconvert{}, "Remove unnecessary type conversions", true, false), + enabledByDefault(golinters.Ineffassign{}, "Detects when assignments to existing variables are not used", false, false), + enabledByDefault(golinters.Dupl{}, "Tool for code clone detection", false, false), + enabledByDefault(golinters.Goconst{}, "Finds repeated strings that could be replaced by a constant", false, false), + enabledByDefault(golinters.Deadcode{}, "Finds unused code", true, false), + enabledByDefault(golinters.Gocyclo{}, "Computes and checks the cyclomatic complexity of functions", false, false), disabledByDefault(golinters.Gofmt{}, "Gofmt checks whether code was gofmt-ed. By default this tool runs with -s option to check for code simplification", false, false), disabledByDefault(golinters.Gofmt{UseGoimports: true}, "Goimports does everything that gofmt does. Additionally it checks unused imports", false, false), diff --git a/pkg/golinters/goconst.go b/pkg/golinters/goconst.go new file mode 100644 index 00000000..22ecced3 --- /dev/null +++ b/pkg/golinters/goconst.go @@ -0,0 +1,44 @@ +package golinters + +import ( + "context" + "fmt" + + goconstAPI "github.com/golangci/goconst" + "github.com/golangci/golangci-lint/pkg/result" +) + +type Goconst struct{} + +func (Goconst) Name() string { + return "goconst" +} + +func (lint Goconst) Run(ctx context.Context, lintCtx *Context) (*result.Result, error) { + issues, err := goconstAPI.Run(lintCtx.Paths.Files, true, + lintCtx.RunCfg().Goconst.MinStringLen, + lintCtx.RunCfg().Goconst.MinOccurrencesCount, + ) + if err != nil { + return nil, err + } + + res := &result.Result{} + for _, i := range issues { + textBegin := fmt.Sprintf("string %s has %d occurrences", formatCode(i.Str, lintCtx.RunCfg()), i.OccurencesCount) + var textEnd string + if i.MatchingConst == "" { + textEnd = ", make it a constant" + } else { + textEnd = fmt.Sprintf(", but such constant %s already exists", formatCode(i.MatchingConst, lintCtx.RunCfg())) + } + res.Issues = append(res.Issues, result.Issue{ + File: i.Pos.Filename, + LineNumber: i.Pos.Line, + Text: textBegin + textEnd, + FromLinter: lint.Name(), + }) + } + + return res, nil +} diff --git a/pkg/golinters/linters_test.go b/pkg/golinters/linters_test.go index 6aad9c19..94898bff 100644 --- a/pkg/golinters/linters_test.go +++ b/pkg/golinters/linters_test.go @@ -45,6 +45,6 @@ func installBinary(t *testing.T) { func testOneSource(t *testing.T, sourcePath string) { goErrchkBin := filepath.Join(runtime.GOROOT(), "test", "errchk") - cmd := exec.Command(goErrchkBin, binName, "run", "--enable-all", sourcePath) + cmd := exec.Command(goErrchkBin, binName, "run", "--enable-all", "--gocyclo.min-complexity", "20", sourcePath) runGoErrchk(cmd, t) } diff --git a/pkg/golinters/testdata/goconst.go b/pkg/golinters/testdata/goconst.go new file mode 100644 index 00000000..6d881321 --- /dev/null +++ b/pkg/golinters/testdata/goconst.go @@ -0,0 +1,30 @@ +package testdata + +import "fmt" + +func GoconstA() { // nolint:dupl + a := "needconst" // ERROR "string `needconst` has 5 occurrences, make it a constant" + fmt.Print(a) + b := "needconst" + fmt.Print(b) + c := "needconst" + fmt.Print(c) +} + +func GoconstB() { + a := "needconst" + fmt.Print(a) + b := "needconst" + fmt.Print(b) +} + +const AlreadyHasConst = "alreadyhasconst" + +func GoconstC() { // nolint:dupl + a := "alreadyhasconst" // ERROR "string `alreadyhasconst` has 3 occurrences, but such constant `AlreadyHasConst` already exists" + fmt.Print(a) + b := "alreadyhasconst" + fmt.Print(b) + c := "alreadyhasconst" + fmt.Print(c) +} diff --git a/vendor/github.com/golangci/goconst/LICENSE b/vendor/github.com/golangci/goconst/LICENSE new file mode 100644 index 00000000..e9264954 --- /dev/null +++ b/vendor/github.com/golangci/goconst/LICENSE @@ -0,0 +1,21 @@ +The MIT License (MIT) + +Copyright (c) 2015 Jonathan Gautheron + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in all +copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +SOFTWARE. diff --git a/vendor/github.com/golangci/goconst/README.md b/vendor/github.com/golangci/goconst/README.md new file mode 100644 index 00000000..04fc9b01 --- /dev/null +++ b/vendor/github.com/golangci/goconst/README.md @@ -0,0 +1,49 @@ +# goconst + +Find repeated strings that could be replaced by a constant. + +### Motivation + +There are obvious benefits to using constants instead of repeating strings, mostly to ease maintenance. Cannot argue against changing a single constant versus many strings. + +While this could be considered a beginner mistake, across time, multiple packages and large codebases, some repetition could have slipped in. + +### Get Started + + $ go get github.com/jgautheron/goconst/cmd/goconst + $ goconst ./... + +### Usage + +``` +Usage: + + goconst ARGS + +Flags: + + -ignore exclude files matching the given regular expression + -ignore-tests exclude tests from the search (default: true) + -min-occurrences report from how many occurrences (default: 2) + -min-length only report strings with the minimum given length (default: 3) + -match-constant look for existing constants matching the values + -numbers search also for duplicated numbers + -min minimum value, only works with -numbers + -max maximum value, only works with -numbers + -output output formatting (text or json) + +Examples: + + goconst ./... + goconst -ignore "yacc|\.pb\." $GOPATH/src/github.com/cockroachdb/cockroach/... + goconst -min-occurrences 3 -output json $GOPATH/src/github.com/cockroachdb/cockroach + goconst -numbers -min 60 -max 512 . +``` + +### Other static analysis tools + +- [gogetimports](https://github.com/jgautheron/gogetimports): Get a JSON-formatted list of imports. +- [usedexports](https://github.com/jgautheron/usedexports): Find exported variables that could be unexported. + +### License +MIT diff --git a/vendor/github.com/golangci/goconst/parser.go b/vendor/github.com/golangci/goconst/parser.go new file mode 100644 index 00000000..f1bc4b28 --- /dev/null +++ b/vendor/github.com/golangci/goconst/parser.go @@ -0,0 +1,187 @@ +// Package goconst finds repeated strings that could be replaced by a constant. +// +// There are obvious benefits to using constants instead of repeating strings, +// mostly to ease maintenance. Cannot argue against changing a single constant versus many strings. +// While this could be considered a beginner mistake, across time, +// multiple packages and large codebases, some repetition could have slipped in. +package goconst + +import ( + "go/ast" + "go/parser" + "go/token" + "log" + "os" + "path/filepath" + "regexp" + "strings" +) + +const ( + testSuffix = "_test.go" +) + +type Parser struct { + // Meant to be passed via New() + path, ignore string + ignoreTests, matchConstant bool + minLength int + + supportedTokens []token.Token + + // Internals + strs Strings + consts Constants +} + +// New creates a new instance of the parser. +// This is your entry point if you'd like to use goconst as an API. +func New(path, ignore string, ignoreTests, matchConstant, numbers bool, minLength int) *Parser { + supportedTokens := []token.Token{token.STRING} + if numbers { + supportedTokens = append(supportedTokens, token.INT, token.FLOAT) + } + + return &Parser{ + path: path, + ignore: ignore, + ignoreTests: ignoreTests, + matchConstant: matchConstant, + minLength: minLength, + supportedTokens: supportedTokens, + + // Initialize the maps + strs: Strings{}, + consts: Constants{}, + } +} + +// ParseTree will search the given path for occurrences that could be moved into constants. +// If "..." is appended, the search will be recursive. +func (p *Parser) ParseTree() (Strings, Constants, error) { + pathLen := len(p.path) + // Parse recursively the given path if the recursive notation is found + if pathLen >= 5 && p.path[pathLen-3:] == "..." { + filepath.Walk(p.path[:pathLen-3], func(path string, f os.FileInfo, err error) error { + if err != nil { + log.Println(err) + // resume walking + return nil + } + + if f.IsDir() { + p.parseDir(path) + } + return nil + }) + } else { + p.parseDir(p.path) + } + return p.strs, p.consts, nil +} + +func (p *Parser) parseDir(dir string) error { + fset := token.NewFileSet() + pkgs, err := parser.ParseDir(fset, dir, func(info os.FileInfo) bool { + valid, name := true, info.Name() + + if p.ignoreTests { + if strings.HasSuffix(name, testSuffix) { + valid = false + } + } + + if len(p.ignore) != 0 { + match, err := regexp.MatchString(p.ignore, dir+name) + if err != nil { + log.Fatal(err) + return true + } + if match { + valid = false + } + } + + return valid + }, 0) + if err != nil { + return err + } + + for _, pkg := range pkgs { + for fn, f := range pkg.Files { + ast.Walk(&treeVisitor{ + fileSet: fset, + packageName: pkg.Name, + fileName: fn, + p: p, + }, f) + } + } + + return nil +} + +type Strings map[string][]ExtendedPos +type Constants map[string]ConstType + +type ConstType struct { + token.Position + Name, packageName string +} + +type ExtendedPos struct { + token.Position + packageName string +} + +type Issue struct { + Pos token.Position + OccurencesCount int + Str string + MatchingConst string +} + +func Run(files []string, matchWithConstants bool, minStringLength, minOccurrences int) ([]Issue, error) { + p := New("", "", false, matchWithConstants, false, minStringLength) + var issues []Issue + fset := token.NewFileSet() + for _, path := range files { + f, err := parser.ParseFile(fset, path, nil, 0) + if err != nil { + return nil, err + } + ast.Walk(&treeVisitor{ + fileSet: fset, + packageName: "", + fileName: path, + p: p, + }, f) + } + + for str, item := range p.strs { + // Filter out items whose occurrences don't match the min value + if len(item) < minOccurrences { + delete(p.strs, str) + } + } + + for str, item := range p.strs { + fi := item[0] + i := Issue{ + Pos: fi.Position, + OccurencesCount: len(item), + Str: str, + } + + if len(p.consts) != 0 { + if cst, ok := p.consts[str]; ok { + // const should be in the same package and exported + i.MatchingConst = cst.Name + } + } + issues = append(issues, i) + } + + return issues, nil +} diff --git a/vendor/github.com/golangci/goconst/visitor.go b/vendor/github.com/golangci/goconst/visitor.go new file mode 100644 index 00000000..a86421f6 --- /dev/null +++ b/vendor/github.com/golangci/goconst/visitor.go @@ -0,0 +1,143 @@ +package goconst + +import ( + "go/ast" + "go/token" + "strings" +) + +// treeVisitor carries the package name and file name +// for passing it to the imports map, and the fileSet for +// retrieving the token.Position. +type treeVisitor struct { + p *Parser + fileSet *token.FileSet + packageName, fileName string +} + +// Visit browses the AST tree for strings that could be potentially +// replaced by constants. +// A map of existing constants is built as well (-match-constant). +func (v *treeVisitor) Visit(node ast.Node) ast.Visitor { + if node == nil { + return v + } + + // A single case with "ast.BasicLit" would be much easier + // but then we wouldn't be able to tell in which context + // the string is defined (could be a constant definition). + switch t := node.(type) { + // Scan for constants in an attempt to match strings with existing constants + case *ast.GenDecl: + if !v.p.matchConstant { + return v + } + if t.Tok != token.CONST { + return v + } + + for _, spec := range t.Specs { + val := spec.(*ast.ValueSpec) + for i, str := range val.Values { + lit, ok := str.(*ast.BasicLit) + if !ok || !v.isSupported(lit.Kind) { + continue + } + + v.addConst(val.Names[i].Name, lit.Value, val.Names[i].Pos()) + } + } + + // foo := "moo" + case *ast.AssignStmt: + for _, rhs := range t.Rhs { + lit, ok := rhs.(*ast.BasicLit) + if !ok || !v.isSupported(lit.Kind) { + continue + } + + v.addString(lit.Value, rhs.(*ast.BasicLit).Pos()) + } + + // if foo == "moo" + case *ast.BinaryExpr: + if t.Op != token.EQL && t.Op != token.NEQ { + return v + } + + var lit *ast.BasicLit + var ok bool + + lit, ok = t.X.(*ast.BasicLit) + if ok && v.isSupported(lit.Kind) { + v.addString(lit.Value, lit.Pos()) + } + + lit, ok = t.Y.(*ast.BasicLit) + if ok && v.isSupported(lit.Kind) { + v.addString(lit.Value, lit.Pos()) + } + + // case "foo": + case *ast.CaseClause: + for _, item := range t.List { + lit, ok := item.(*ast.BasicLit) + if ok && v.isSupported(lit.Kind) { + v.addString(lit.Value, lit.Pos()) + } + } + + // return "boo" + case *ast.ReturnStmt: + for _, item := range t.Results { + lit, ok := item.(*ast.BasicLit) + if ok && v.isSupported(lit.Kind) { + v.addString(lit.Value, lit.Pos()) + } + } + } + + return v +} + +// addString adds a string in the map along with its position in the tree. +func (v *treeVisitor) addString(str string, pos token.Pos) { + str = strings.Replace(str, `"`, "", 2) + + // Ignore empty strings + if len(str) == 0 { + return + } + + if len(str) < v.p.minLength { + return + } + + _, ok := v.p.strs[str] + if !ok { + v.p.strs[str] = make([]ExtendedPos, 0) + } + v.p.strs[str] = append(v.p.strs[str], ExtendedPos{ + packageName: v.packageName, + Position: v.fileSet.Position(pos), + }) +} + +// addConst adds a const in the map along with its position in the tree. +func (v *treeVisitor) addConst(name string, val string, pos token.Pos) { + val = strings.Replace(val, `"`, "", 2) + v.p.consts[val] = ConstType{ + Name: name, + packageName: v.packageName, + Position: v.fileSet.Position(pos), + } +} + +func (v *treeVisitor) isSupported(tk token.Token) bool { + for _, s := range v.p.supportedTokens { + if tk == s { + return true + } + } + return false +}