From 86ac9149668b2514f199ad5a0126a94c5176d0d8 Mon Sep 17 00:00:00 2001 From: moznion Date: Thu, 12 Dec 2019 23:44:38 +0900 Subject: [PATCH] lint: rename `*ast.Ident` value automatically when the ident violated name rule on fix mode When `*ast.Ident` value was fixed, it doesn't show a warning message. --- .gitignore | 2 +- golint/golint.go | 12 +- lint.go | 140 ++++++++++++---------- lint_test.go | 46 +++++++ misc/auto_fix_test/auto_fix_expected | 8 -- misc/auto_fix_test/auto_fix_given.go.orig | 8 -- misc/auto_fix_test/auto_fix_test.sh | 14 --- testdata/auto_fix/expected.go | 138 +++++++++++++++++++++ 8 files changed, 269 insertions(+), 99 deletions(-) delete mode 100644 misc/auto_fix_test/auto_fix_expected delete mode 100644 misc/auto_fix_test/auto_fix_given.go.orig delete mode 100755 misc/auto_fix_test/auto_fix_test.sh create mode 100644 testdata/auto_fix/expected.go diff --git a/.gitignore b/.gitignore index bcfae75b..7fc0f081 100644 --- a/.gitignore +++ b/.gitignore @@ -1 +1 @@ -misc/auto_fix_test/auto_fix_given.go +testdata/auto_fix/given.go diff --git a/golint/golint.go b/golint/golint.go index 0e125266..934bcec5 100644 --- a/golint/golint.go +++ b/golint/golint.go @@ -25,7 +25,7 @@ import ( var ( minConfidence = flag.Float64("min_confidence", 0.8, "minimum confidence of a problem to print it") setExitStatus = flag.Bool("set_exit_status", false, "set exit status to 1 if any issues are found") - shouldFix = flag.Bool("fix", false, "fix automatically if it is possible (this may overwrite warned files)") + isFixMode = flag.Bool("fix", false, "fix automatically if it is possible (this may overwrite warned files)") suggestions int ) @@ -120,7 +120,7 @@ func lintFiles(filenames ...string) { } l := new(lint.Linter) - ps, err := l.LintFiles(files) + ps, err := l.LintFiles(files, *isFixMode) if err != nil { fmt.Fprintf(os.Stderr, "%v\n", err) return @@ -131,7 +131,7 @@ func lintFiles(filenames ...string) { for _, p := range ps { if p.Confidence >= *minConfidence { autoFixedMarker := "" - if *shouldFix { + if *isFixMode { if fixer := p.Fixer; fixer != nil { if filename := p.Position.Filename; filename != "" { filenameToSrcAutoFix[filename] = &srcInfoForAutoFix{ @@ -142,6 +142,12 @@ func lintFiles(filenames ...string) { } } } + + if p.OnlyUsedToFix { + // don't show a warning message + continue + } + fmt.Printf("%v: %s%s\n", p.Position, autoFixedMarker, p.Text) suggestions++ } diff --git a/lint.go b/lint.go index 4001ec03..1417246d 100644 --- a/lint.go +++ b/lint.go @@ -52,6 +52,9 @@ type Problem struct { // FileSet is used to generate source code for fix mode. FileSet *token.FileSet + + // OnlyUsedToFix is a marker to indicate it should fix only, not show a warning message. + OnlyUsedToFix bool } func (p *Problem) String() string { @@ -83,13 +86,13 @@ func (p byPosition) Less(i, j int) bool { } // Lint lints src. -func (l *Linter) Lint(filename string, src []byte) ([]Problem, error) { - return l.LintFiles(map[string][]byte{filename: src}) +func (l *Linter) Lint(filename string, src []byte, isFixMode ...bool) ([]Problem, error) { + return l.LintFiles(map[string][]byte{filename: src}, isFixMode...) } // LintFiles lints a set of files of a single package. // The argument is a map of filename to source. -func (l *Linter) LintFiles(files map[string][]byte) ([]Problem, error) { +func (l *Linter) LintFiles(files map[string][]byte, isFixMode ...bool) ([]Problem, error) { pkg := &pkg{ fset: token.NewFileSet(), files: make(map[string]*file), @@ -109,11 +112,12 @@ func (l *Linter) LintFiles(files map[string][]byte) ([]Problem, error) { return nil, fmt.Errorf("%s is in package %s, not %s", filename, f.Name.Name, pkgName) } pkg.files[filename] = &file{ - pkg: pkg, - f: f, - fset: pkg.fset, - src: src, - filename: filename, + pkg: pkg, + f: f, + fset: pkg.fset, + src: src, + filename: filename, + isFixMode: len(isFixMode) > 0 && isFixMode[0], } } if len(pkg.files) == 0 { @@ -190,11 +194,12 @@ func (p *pkg) lint() []Problem { // file represents a file being linted. type file struct { - pkg *pkg - f *ast.File - fset *token.FileSet - src []byte - filename string + pkg *pkg + f *ast.File + fset *token.FileSet + src []byte + filename string + isFixMode bool } func (f *file) isTest() bool { return strings.HasSuffix(f.filename, "_test.go") } @@ -225,20 +230,21 @@ type category string // The variadic arguments may start with link and category types, // and must end with a format string and any arguments. // It returns the new Problem. -func (f *file) errorf(n ast.Node, confidence float64, fixer func() *ast.File, args ...interface{}) *Problem { +func (f *file) errorf(n ast.Node, confidence float64, fixer func() *ast.File, onlyUsedToFix bool, args ...interface{}) *Problem { pos := f.fset.Position(n.Pos()) if pos.Filename == "" { pos.Filename = f.filename } - return f.pkg.errorfAt(pos, confidence, fixer, args...) + return f.pkg.errorfAt(pos, confidence, fixer, onlyUsedToFix, args...) } -func (p *pkg) errorfAt(pos token.Position, confidence float64, fixer func() *ast.File, args ...interface{}) *Problem { +func (p *pkg) errorfAt(pos token.Position, confidence float64, fixer func() *ast.File, onlyUsedToFix bool, args ...interface{}) *Problem { problem := Problem{ - Position: pos, - Confidence: confidence, - Fixer: fixer, - FileSet: p.fset, + Position: pos, + Confidence: confidence, + Fixer: fixer, + FileSet: p.fset, + OnlyUsedToFix: onlyUsedToFix, } if pos.Filename != "" { // The file might not exist in our mapping if a //line directive was encountered. @@ -416,23 +422,23 @@ func (f *file) lintPackageComment() { Line: endPos.Line + 1, Column: 1, } - f.pkg.errorfAt(pos, 0.9, nil, link(ref), category("comments"), "package comment is detached; there should be no blank lines between it and the package statement") + f.pkg.errorfAt(pos, 0.9, nil, false, link(ref), category("comments"), "package comment is detached; there should be no blank lines between it and the package statement") return } } if f.f.Doc == nil { - f.errorf(f.f, 0.2, nil, link(ref), category("comments"), "should have a package comment, unless it's in another file for this package") + f.errorf(f.f, 0.2, nil, false, link(ref), category("comments"), "should have a package comment, unless it's in another file for this package") return } s := f.f.Doc.Text() if ts := strings.TrimLeft(s, " \t"); ts != s { - f.errorf(f.f.Doc, 1, nil, link(ref), category("comments"), "package comment should not have leading space") + f.errorf(f.f.Doc, 1, nil, false, link(ref), category("comments"), "package comment should not have leading space") s = ts } // Only non-main packages need to keep to this form. if !f.pkg.main && !strings.HasPrefix(s, prefix) { - f.errorf(f.f.Doc, 1, nil, link(ref), category("comments"), `package comment should be of the form "%s..."`, prefix) + f.errorf(f.f.Doc, 1, nil, false, link(ref), category("comments"), `package comment should be of the form "%s..."`, prefix) } } @@ -463,7 +469,7 @@ func (f *file) lintBlankImports() { // This is the first blank import of a group. if imp.Doc == nil && imp.Comment == nil { ref := "" - f.errorf(imp, 1, nil, link(ref), category("imports"), "a blank import should be only in a main or test package, or have a comment justifying it") + f.errorf(imp, 1, nil, false, link(ref), category("imports"), "a blank import should be only in a main or test package, or have a comment justifying it") } } } @@ -473,7 +479,7 @@ func (f *file) lintImports() { for i, is := range f.f.Imports { _ = i if is.Name != nil && is.Name.Name == "." && !f.isTest() { - f.errorf(is, 1, nil, link(styleGuideBase+"#import-dot"), category("imports"), "should not use dot imports") + f.errorf(is, 1, nil, false, link(styleGuideBase+"#import-dot"), category("imports"), "should not use dot imports") } } @@ -565,7 +571,7 @@ func isInTopLevel(f *ast.File, ident *ast.Ident) bool { func (f *file) lintNames() { // Package names need slightly different handling than other names. if strings.Contains(f.f.Name.Name, "_") && !strings.HasSuffix(f.f.Name.Name, "_test") { - f.errorf(f.f, 1, nil, link("http://golang.org/doc/effective_go.html#package-names"), category("naming"), "don't use an underscore in package name") + f.errorf(f.f, 1, nil, false, link("http://golang.org/doc/effective_go.html#package-names"), category("naming"), "don't use an underscore in package name") } if anyCapsRE.MatchString(f.f.Name.Name) { should := strings.ToLower(f.f.Name.Name) @@ -573,10 +579,10 @@ func (f *file) lintNames() { f.f.Name.Name = should return f.f } - f.errorf(f.f, 1, fixer, link("http://golang.org/doc/effective_go.html#package-names"), category("mixed-caps"), "don't use MixedCaps in package name; %s should be %s", f.f.Name.Name, should) + f.errorf(f.f, 1, fixer, false, link("http://golang.org/doc/effective_go.html#package-names"), category("mixed-caps"), "don't use MixedCaps in package name; %s should be %s", f.f.Name.Name, should) } - check := func(id *ast.Ident, thing string) { + check := func(id *ast.Ident, thing string, onlyUsedToFix bool) { if id.Name == "_" { return } @@ -594,18 +600,18 @@ func (f *file) lintNames() { } if capCount >= 2 { // TODO support auto fixing - f.errorf(id, 0.8, nil, link(styleGuideBase+"#mixed-caps"), category("naming"), "don't use ALL_CAPS in Go names; use CamelCase") + f.errorf(id, 0.8, nil, onlyUsedToFix, link(styleGuideBase+"#mixed-caps"), category("naming"), "don't use ALL_CAPS in Go names; use CamelCase") return } } - if thing == "const" || (thing == "var" && isInTopLevel(f.f, id)) { + if thing == "const" || (thing == "var" && isInTopLevel(f.f, id)) || thing == "ident" { if len(id.Name) > 2 && id.Name[0] == 'k' && id.Name[1] >= 'A' && id.Name[1] <= 'Z' { should := string(id.Name[1]+'a'-'A') + id.Name[2:] fixer := func() *ast.File { id.Name = should return f.f } - f.errorf(id, 0.8, fixer, link(styleGuideBase+"#mixed-caps"), category("naming"), "don't use leading k in Go names; %s %s should be %s", thing, id.Name, should) + f.errorf(id, 0.8, fixer, onlyUsedToFix, link(styleGuideBase+"#mixed-caps"), category("naming"), "don't use leading k in Go names; %s %s should be %s", thing, id.Name, should) } } @@ -620,10 +626,10 @@ func (f *file) lintNames() { } if len(id.Name) > 2 && strings.Contains(id.Name[1:], "_") { - f.errorf(id, 0.9, fixer, link("http://golang.org/doc/effective_go.html#mixed-caps"), category("naming"), "don't use underscores in Go names; %s %s should be %s", thing, id.Name, should) + f.errorf(id, 0.9, fixer, onlyUsedToFix, link("http://golang.org/doc/effective_go.html#mixed-caps"), category("naming"), "don't use underscores in Go names; %s %s should be %s", thing, id.Name, should) return } - f.errorf(id, 0.8, fixer, link(styleGuideBase+"#initialisms"), category("naming"), "%s %s should be %s", thing, id.Name, should) + f.errorf(id, 0.8, fixer, onlyUsedToFix, link(styleGuideBase+"#initialisms"), category("naming"), "%s %s should be %s", thing, id.Name, should) } checkList := func(fl *ast.FieldList, thing string) { if fl == nil { @@ -631,7 +637,7 @@ func (f *file) lintNames() { } for _, f := range fl.List { for _, id := range f.Names { - check(id, thing) + check(id, thing, false) } } } @@ -643,7 +649,7 @@ func (f *file) lintNames() { } for _, exp := range v.Lhs { if id, ok := exp.(*ast.Ident); ok { - check(id, "var") + check(id, "var", false) } } case *ast.FuncDecl: @@ -660,7 +666,7 @@ func (f *file) lintNames() { // not exported in the Go API. // See https://github.com/golang/lint/issues/144. if ast.IsExported(v.Name.Name) || !isCgoExported(v) { - check(v.Name, thing) + check(v.Name, thing, false) } checkList(v.Type.Params, thing+" parameter") @@ -681,10 +687,10 @@ func (f *file) lintNames() { for _, spec := range v.Specs { switch s := spec.(type) { case *ast.TypeSpec: - check(s.Name, thing) + check(s.Name, thing, false) case *ast.ValueSpec: for _, id := range s.Names { - check(id, thing) + check(id, thing, false) } } } @@ -704,17 +710,21 @@ func (f *file) lintNames() { return true } if id, ok := v.Key.(*ast.Ident); ok { - check(id, "range var") + check(id, "range var", false) } if id, ok := v.Value.(*ast.Ident); ok { - check(id, "range var") + check(id, "range var", false) } case *ast.StructType: for _, f := range v.Fields.List { for _, id := range f.Names { - check(id, "struct field") + check(id, "struct field", false) } } + case *ast.Ident: + if f.isFixMode { + check(v, "ident", true) + } } return true }) @@ -840,7 +850,7 @@ func (f *file) lintTypeDoc(t *ast.TypeSpec, doc *ast.CommentGroup) { return } if doc == nil { - f.errorf(t, 1, nil, link(docCommentsLink), category("comments"), "exported type %v should have comment or be unexported", t.Name) + f.errorf(t, 1, nil, false, link(docCommentsLink), category("comments"), "exported type %v should have comment or be unexported", t.Name) return } @@ -853,7 +863,7 @@ func (f *file) lintTypeDoc(t *ast.TypeSpec, doc *ast.CommentGroup) { } } if !strings.HasPrefix(s, t.Name.Name+" ") { - f.errorf(doc, 1, nil, link(docCommentsLink), category("comments"), `comment on exported type %v should be of the form "%v ..." (with optional leading article)`, t.Name, t.Name) + f.errorf(doc, 1, nil, false, link(docCommentsLink), category("comments"), `comment on exported type %v should be of the form "%v ..." (with optional leading article)`, t.Name, t.Name) } } @@ -895,13 +905,13 @@ func (f *file) lintFuncDoc(fn *ast.FuncDecl) { name = recv + "." + name } if fn.Doc == nil { - f.errorf(fn, 1, nil, link(docCommentsLink), category("comments"), "exported %s %s should have comment or be unexported", kind, name) + f.errorf(fn, 1, nil, false, link(docCommentsLink), category("comments"), "exported %s %s should have comment or be unexported", kind, name) return } s := fn.Doc.Text() prefix := fn.Name.Name + " " if !strings.HasPrefix(s, prefix) { - f.errorf(fn.Doc, 1, nil, link(docCommentsLink), category("comments"), `comment on exported %s %s should be of the form "%s..."`, kind, name, prefix) + f.errorf(fn.Doc, 1, nil, false, link(docCommentsLink), category("comments"), `comment on exported %s %s should be of the form "%s..."`, kind, name, prefix) } } @@ -918,7 +928,7 @@ func (f *file) lintValueSpecDoc(vs *ast.ValueSpec, gd *ast.GenDecl, genDeclMissi // Check that none are exported except for the first. for _, n := range vs.Names[1:] { if ast.IsExported(n.Name) { - f.errorf(vs, 1, nil, category("comments"), "exported %s %s should have its own declaration", kind, n.Name) + f.errorf(vs, 1, nil, false, category("comments"), "exported %s %s should have its own declaration", kind, n.Name) return } } @@ -938,7 +948,7 @@ func (f *file) lintValueSpecDoc(vs *ast.ValueSpec, gd *ast.GenDecl, genDeclMissi if kind == "const" && gd.Lparen.IsValid() { block = " (or a comment on this block)" } - f.errorf(vs, 1, nil, link(docCommentsLink), category("comments"), "exported %s %s should have comment%s or be unexported", kind, name, block) + f.errorf(vs, 1, nil, false, link(docCommentsLink), category("comments"), "exported %s %s should have comment%s or be unexported", kind, name, block) genDeclMissingComments[gd] = true return } @@ -954,7 +964,7 @@ func (f *file) lintValueSpecDoc(vs *ast.ValueSpec, gd *ast.GenDecl, genDeclMissi } prefix := name + " " if !strings.HasPrefix(doc.Text(), prefix) { - f.errorf(doc, 1, nil, link(docCommentsLink), category("comments"), `comment on exported %s %s should be of the form "%s..."`, kind, name, prefix) + f.errorf(doc, 1, nil, false, link(docCommentsLink), category("comments"), `comment on exported %s %s should be of the form "%s..."`, kind, name, prefix) } } @@ -979,7 +989,7 @@ func (f *file) checkStutter(id *ast.Ident, thing string) { // the it's starting a new word and thus this name stutters. rem := name[len(pkg):] if next, _ := utf8.DecodeRuneInString(rem); next == '_' || unicode.IsUpper(next) { - f.errorf(id, 0.8, nil, link(styleGuideBase+"#package-names"), category("naming"), "%s name will be used as %s.%s by other packages, and that stutters; consider calling this %s", thing, pkg, name, rem) + f.errorf(id, 0.8, nil, false, link(styleGuideBase+"#package-names"), category("naming"), "%s name will be used as %s.%s by other packages, and that stutters; consider calling this %s", thing, pkg, name, rem) } } @@ -1038,7 +1048,7 @@ func (f *file) lintElses() { if shortDecl { extra = " (move short variable declaration to its own line if necessary)" } - f.errorf(ifStmt.Else, 1, nil, link(styleGuideBase+"#indent-error-flow"), category("indent"), "if block ends with a return statement, so drop this else and outdent its block"+extra) + f.errorf(ifStmt.Else, 1, nil, false, link(styleGuideBase+"#indent-error-flow"), category("indent"), "if block ends with a return statement, so drop this else and outdent its block"+extra) } return true }) @@ -1053,7 +1063,7 @@ func (f *file) lintRanges() { } if isIdent(rs.Key, "_") && (rs.Value == nil || isIdent(rs.Value, "_")) { - p := f.errorf(rs.Key, 1, nil, category("range-loop"), "should omit values from range; this loop is equivalent to `for range ...`") + p := f.errorf(rs.Key, 1, nil, false, category("range-loop"), "should omit values from range; this loop is equivalent to `for range ...`") newRS := *rs // shallow copy newRS.Value = nil @@ -1064,7 +1074,7 @@ func (f *file) lintRanges() { } if isIdent(rs.Value, "_") { - p := f.errorf(rs.Value, 1, nil, category("range-loop"), "should omit 2nd value from range; this loop is equivalent to `for %s %s range ...`", f.render(rs.Key), rs.Tok) + p := f.errorf(rs.Value, 1, nil, false, category("range-loop"), "should omit 2nd value from range; this loop is equivalent to `for %s %s range ...`", f.render(rs.Key), rs.Tok) newRS := *rs // shallow copy newRS.Value = nil @@ -1105,7 +1115,7 @@ func (f *file) lintErrorf() { if isTestingError { errorfPrefix = f.render(se.X) } - p := f.errorf(node, 1, nil, category("errors"), "should replace %s(fmt.Sprintf(...)) with %s.Errorf(...)", f.render(se), errorfPrefix) + p := f.errorf(node, 1, nil, false, category("errors"), "should replace %s(fmt.Sprintf(...)) with %s.Errorf(...)", f.render(se), errorfPrefix) m := f.srcLineWithMatch(ce, `^(.*)`+f.render(se)+`\(fmt\.Sprintf\((.*)\)\)(.*)$`) if m != nil { @@ -1142,7 +1152,7 @@ func (f *file) lintErrors() { prefix = "Err" } if !strings.HasPrefix(id.Name, prefix) { - f.errorf(id, 0.9, nil, category("naming"), "error var %s should have name of the form %sFoo", id.Name, prefix) + f.errorf(id, 0.9, nil, false, category("naming"), "error var %s should have name of the form %sFoo", id.Name, prefix) } } } @@ -1197,7 +1207,7 @@ func (f *file) lintErrorStrings() { return true } - f.errorf(str, conf, nil, link(styleGuideBase+"#error-strings"), category("errors"), "error strings should not be capitalized or end with punctuation or a newline") + f.errorf(str, conf, nil, false, link(styleGuideBase+"#error-strings"), category("errors"), "error strings should not be capitalized or end with punctuation or a newline") return true }) } @@ -1218,16 +1228,16 @@ func (f *file) lintReceiverNames() { name := names[0].Name const ref = styleGuideBase + "#receiver-names" if name == "_" { - f.errorf(n, 1, nil, link(ref), category("naming"), `receiver name should not be an underscore, omit the name if it is unused`) + f.errorf(n, 1, nil, false, link(ref), category("naming"), `receiver name should not be an underscore, omit the name if it is unused`) return true } if name == "this" || name == "self" { - f.errorf(n, 1, nil, link(ref), category("naming"), `receiver name should be a reflection of its identity; don't use generic names such as "this" or "self"`) + f.errorf(n, 1, nil, false, link(ref), category("naming"), `receiver name should be a reflection of its identity; don't use generic names such as "this" or "self"`) return true } recv := receiverType(fn) if prev, ok := typeReceiver[recv]; ok && prev != name { - f.errorf(n, 1, nil, link(ref), category("naming"), "receiver name %s should be consistent with previous receiver name %s for %s", name, prev, recv) + f.errorf(n, 1, nil, false, link(ref), category("naming"), "receiver name %s should be consistent with previous receiver name %s for %s", name, prev, recv) return true } typeReceiver[recv] = name @@ -1258,7 +1268,7 @@ func (f *file) lintIncDec() { default: return true } - f.errorf(as, 0.8, nil, category("unary-op"), "should replace %s with %s%s", f.render(as), f.render(as.Lhs[0]), suffix) + f.errorf(as, 0.8, nil, false, category("unary-op"), "should replace %s with %s%s", f.render(as), f.render(as.Lhs[0]), suffix) return true }) } @@ -1282,7 +1292,7 @@ func (f *file) lintErrorReturn() { // Flag any error parameters found before the last. for _, r := range ret[:len(ret)-1] { if isIdent(r.Type, "error") { - f.errorf(fn, 0.9, nil, category("arg-order"), "error should be the last type when returning multiple items") + f.errorf(fn, 0.9, nil, false, category("arg-order"), "error should be the last type when returning multiple items") break // only flag one } } @@ -1318,7 +1328,7 @@ func (f *file) lintUnexportedReturn() { if exportedType(typ) { continue } - f.errorf(ret.Type, 0.8, nil, category("unexported-type-in-api"), "exported %s %s returns unexported type %s, which can be annoying to use", thing, fn.Name.Name, typ) + f.errorf(ret.Type, 0.8, nil, false, category("unexported-type-in-api"), "exported %s %s returns unexported type %s, which can be annoying to use", thing, fn.Name.Name, typ) break // only flag one } return false @@ -1381,7 +1391,7 @@ func (f *file) lintTimeNames() { if suffix == "" { continue } - f.errorf(v, 0.9, nil, category("time"), "var %s is of type %v; don't use unit-specific suffix %q", name.Name, origTyp, suffix) + f.errorf(v, 0.9, nil, false, category("time"), "var %s is of type %v; don't use unit-specific suffix %q", name.Name, origTyp, suffix) } return true }) @@ -1423,7 +1433,7 @@ func (f *file) checkContextKeyType(x *ast.CallExpr) { key := f.pkg.typesInfo.Types[x.Args[1]] if ktyp, ok := key.Type.(*types.Basic); ok && ktyp.Kind() != types.Invalid { - f.errorf(x, 1.0, nil, category("context"), fmt.Sprintf("should not use basic type %s as key in context.WithValue", key.Type)) + f.errorf(x, 1.0, nil, false, category("context"), fmt.Sprintf("should not use basic type %s as key in context.WithValue", key.Type)) } } @@ -1440,7 +1450,7 @@ func (f *file) lintContextArgs() { // Flag any that show up after the first. for _, arg := range fn.Type.Params.List[1:] { if isPkgDot(arg.Type, "context", "Context") { - f.errorf(fn, 0.9, nil, link("https://golang.org/pkg/context/"), category("arg-order"), "context.Context should be the first parameter of a function") + f.errorf(fn, 0.9, nil, false, link("https://golang.org/pkg/context/"), category("arg-order"), "context.Context should be the first parameter of a function") break // only flag one } } diff --git a/lint_test.go b/lint_test.go index 92db5968..0ab568d8 100644 --- a/lint_test.go +++ b/lint_test.go @@ -16,6 +16,8 @@ import ( "go/token" "go/types" "io/ioutil" + "os" + "os/exec" "path" "regexp" "strconv" @@ -44,6 +46,12 @@ func TestAll(t *testing.T) { if !rx.MatchString(fi.Name()) { continue } + + fInfo, _ := os.Stat(path.Join(baseDir, fi.Name())) + if fInfo.IsDir() { + continue + } + //t.Logf("Testing %s", fi.Name()) src, err := ioutil.ReadFile(path.Join(baseDir, fi.Name())) if err != nil { @@ -315,3 +323,41 @@ func TestIsGenerated(t *testing.T) { } } } + +func TestAutoFixOption(t *testing.T) { + contents, err := ioutil.ReadFile("testdata/names.go") + if err != nil { + t.Fatalf("ioutil.ReadFile to copy file: %v", err) + } + + err = ioutil.WriteFile("testdata/auto_fix/given.go", contents, 0644) + if err != nil { + t.Fatalf("ioutil.WriteFile to copy file: %v", err) + } + + cmd := exec.Command("go", "run", "./golint", "--fix", "--", "./testdata/auto_fix/given.go") + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + err = cmd.Run() + if err != nil { + t.Fatalf("run golint command with fix option: %v", err) + } + + given, err := ioutil.ReadFile("testdata/auto_fix/given.go") + if err != nil { + t.Fatalf("ioutil.ReadFile to read file: %v", err) + } + expected, err := ioutil.ReadFile("testdata/auto_fix/expected.go") + if err != nil { + t.Fatalf("ioutil.ReadFile to read file: %v", err) + } + + if string(given) != string(expected) { + t.Errorf(`given code and expected code were different +Given: +%s +Expected: +%s +`, given, expected) + } +} diff --git a/misc/auto_fix_test/auto_fix_expected b/misc/auto_fix_test/auto_fix_expected deleted file mode 100644 index b5b97730..00000000 --- a/misc/auto_fix_test/auto_fix_expected +++ /dev/null @@ -1,8 +0,0 @@ -package testtest - -const bar = "" - -func foo() { - const fooBar = "" - const isHTTP = false -} diff --git a/misc/auto_fix_test/auto_fix_given.go.orig b/misc/auto_fix_test/auto_fix_given.go.orig deleted file mode 100644 index a8444ce6..00000000 --- a/misc/auto_fix_test/auto_fix_given.go.orig +++ /dev/null @@ -1,8 +0,0 @@ -package testTest - -const kBar = "" - -func foo() { - const foo_bar = "" - const isHttp = false -} diff --git a/misc/auto_fix_test/auto_fix_test.sh b/misc/auto_fix_test/auto_fix_test.sh deleted file mode 100755 index edf3fb30..00000000 --- a/misc/auto_fix_test/auto_fix_test.sh +++ /dev/null @@ -1,14 +0,0 @@ -#!/bin/sh - -set -ue - -cp ./misc/auto_fix_test/auto_fix_given.go.orig ./misc/auto_fix_test/auto_fix_given.go -go run ./golint/* --fix -- ./misc/auto_fix_test/auto_fix_given.go - -if ! diff ./misc/auto_fix_test/auto_fix_given.go ./misc/auto_fix_test/auto_fix_expected; then - echo not ok - exit 1 -fi - -echo ok - diff --git a/testdata/auto_fix/expected.go b/testdata/auto_fix/expected.go new file mode 100644 index 00000000..92972af5 --- /dev/null +++ b/testdata/auto_fix/expected.go @@ -0,0 +1,138 @@ +// Test for name linting. + +// Package pkg_with_underscores ... +package pkgWithUnderscores // MATCH /underscore.*package name/ + +import ( + "io" + "net" + netHTTP "net/http" // renamed deliberately + "net/url" +) + +import "C" + +var varName int // MATCH /underscore.*var.*var_name/ + +type tWow struct { // MATCH /underscore.*type.*t_wow/ + xDamn int // MATCH /underscore.*field.*x_damn/ + URL *url.URL // MATCH /struct field.*Url.*URL/ +} + +const fooID = "blah" // MATCH /fooId.*fooID/ + +func fIt() { // MATCH /underscore.*func.*f_it/ + moreUnderscore := 4 // MATCH /underscore.*var.*more_underscore/ + _ = moreUnderscore + var err error + if isEOF := (err == io.EOF); isEOF { // MATCH /var.*isEof.*isEOF/ + moreUnderscore = 7 // should be okay + } + + x := netHTTP.Request{} // should be okay + _ = x + + var ips []net.IP + for _, theIP := range ips { // MATCH /range var.*theIp.*theIP/ + _ = theIP + } + + switch myJSON := g(); { // MATCH /var.*myJson.*myJSON/ + default: + _ = myJSON + } + var y netHTTP.ResponseWriter // an interface + switch tAPI := y.(type) { // MATCH /var.*tApi.*tAPI/ + default: + _ = tAPI + } + + var c chan int + select { + case qID := <-c: // MATCH /var.*qId.*qID/ + _ = qID + } +} + +// Common styles in other languages that don't belong in Go. +const ( + CPP_CONST = 1 // MATCH /ALL_CAPS.*CamelCase/ + leadingKay = 2 // MATCH /k.*leadingKay/ + + HTML = 3 // okay; no underscore + X509B = 4 // ditto + V1_10_5 = 5 // okay; fewer than two uppercase letters +) + +var varsAreSometimesUsedAsConstants = 0 // MATCH /k.*varsAreSometimesUsedAsConstants/ +var ( + varsAreSometimesUsedAsConstants2 = 0 // MATCH /k.*varsAreSometimesUsedAsConstants2/ +) + +var thisIsNotOkay = struct { // MATCH /k.*thisIsNotOkay/ + thisIsOkay bool +}{} + +func thisIsOkay() { // this is okay because this is a function name + var thisIsAlsoOkay = 1 // this is okay because this is a non-top-level variable + _ = thisIsAlsoOkay + const thisIsNotOkay = 2 // MATCH /k.*thisIsNotOkay/ +} + +var anotherFunctionScope = func() { + var thisIsOkay = 1 // this is okay because this is a non-top-level variable + _ = thisIsOkay + const thisIsNotOkay = 2 // MATCH /k.*thisIsNotOkay/} +} + +func f(badName int) {} // MATCH /underscore.*func parameter.*bad_name/ +func g() (noWay int) { return 0 } // MATCH /underscore.*func result.*no_way/ +func (t *tWow) f(moreUnder string) {} // MATCH /underscore.*method parameter.*more_under/ +func (t *tWow) g() (stillMore string) { return "" } // MATCH /underscore.*method result.*still_more/ + +type i interface { + CheckHTML() string // okay; interface method names are often constrained by the concrete types' method names + + F(fooBar int) // MATCH /foo_bar.*fooBar/ +} + +// All okay; underscore between digits +const case1_1 = 1 + +type case2_1 struct { + case2_2 int +} + +func case3_1(case3_2 int) (case3_3 string) { + case3_4 := 4 + _ = case3_4 + + return "" +} + +type t struct{} + +func (t) LastInsertId() (int64, error) { return 0, nil } // okay because it matches a known style violation + +//export exported_to_c +func exportedToC() {} // okay: https://github.com/golang/lint/issues/144 + +//export exported_to_c_with_arg +func exportedToCWithArg(butUseGoParamNames int) // MATCH /underscore.*func parameter.*but_use_go_param_names/ + +// This is an exported C function with a leading doc comment. +// +//export exported_to_c_with_comment +func exportedToCWithComment() {} // okay: https://github.com/golang/lint/issues/144 + +//export maybe_exported_to_CPlusPlusWithCamelCase +func maybeExportedToCPlusPlusWithCamelCase() {} // okay: https://github.com/golang/lint/issues/144 + +// WhyAreYouUsingCapitalLetters_InACFunctionName is a Go-exported function that +// is also exported to C as a name with underscores. +// +// Don't do that. If you want to use a C-style name for a C export, make it +// lower-case and leave it out of the Go-exported API. +// +//export WhyAreYouUsingCapitalLetters_InACFunctionName +func WhyAreYouUsingCapitalLettersInACFunctionName() {} // MATCH /underscore.*func.*Why.*CFunctionName/