diff --git a/fixtures/names.go b/fixtures/names.go new file mode 100644 index 0000000..8c34a8b --- /dev/null +++ b/fixtures/names.go @@ -0,0 +1,116 @@ +// Test for name linting. + +// Package pkg_with_underscores ... +package pkg_with_underscores // MATCH /don't use an underscore in package name/ + +import ( + "io" + "net" + net_http "net/http" // renamed deliberately + "net/url" +) + +import "C" + +var var_name int // MATCH /don't use underscores in Go names; var var_name should be varName/ + +type t_wow struct { // MATCH /don't use underscores in Go names; type t_wow should be tWow/ + x_damn int // MATCH /don't use underscores in Go names; struct field x_damn should be xDamn/ + Url *url.URL // MATCH /struct field Url should be URL/ +} + +const fooId = "blah" // MATCH /const fooId should be fooID/ + +func f_it() { // MATCH /don't use underscores in Go names; func f_it should be fIt/ + more_underscore := 4 // MATCH /don't use underscores in Go names; var more_underscore should be moreUnderscore/ + _ = more_underscore + var err error + if isEof := (err == io.EOF); isEof { // MATCH /var isEof should be isEOF/ + more_underscore = 7 // should be okay + } + + x := net_http.Request{} // should be okay + _ = x + + var ips []net.IP + for _, theIp := range ips { // MATCH /range var theIp should be theIP/ + _ = theIp + } + + switch myJson := g(); { // MATCH /var myJson should be myJSON/ + default: + _ = myJson + } + var y net_http.ResponseWriter // an interface + switch tApi := y.(type) { // MATCH /var tApi should be tAPI/ + default: + _ = tApi + } + + var c chan int + select { + case qId := <-c: // MATCH /var qId should be qID/ + _ = qId + } +} + +// Common styles in other languages that don't belong in Go. +const ( + CPP_CONST = 1 // MATCH /don't use ALL_CAPS in Go names; use CamelCase/ + kLeadingKay = 2 // MATCH /don't use leading k in Go names; const kLeadingKay should be leadingKay/ + + HTML = 3 // okay; no underscore + X509B = 4 // ditto +) + +func f(bad_name int) {} // MATCH /don't use underscores in Go names; func parameter bad_name should be badName/ +func g() (no_way int) { return 0 } // MATCH /don't use underscores in Go names; func result no_way should be noWay/ +func (t *t_wow) f(more_under string) {} // MATCH /don't use underscores in Go names; method parameter more_under should be moreUnder/ +func (t *t_wow) g() (still_more string) { return "" } // MATCH /don't use underscores in Go names; method result still_more should be stillMore/ + +type i interface { + CheckHtml() string // okay; interface method names are often constrained by the concrete types' method names + + F(foo_bar int) // MATCH /don't use underscores in Go names; interface method parameter foo_bar should be 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 exported_to_c() {} // okay: https://github.com/golang/lint/issues/144 + +//export exported_to_c_with_arg +func exported_to_c_with_arg(but_use_go_param_names int) // MATCH /don't use underscores in Go names; func parameter but_use_go_param_names should be butUseGoParamNames/ + +// This is an exported C function with a leading doc comment. +// +//export exported_to_c_with_comment +func exported_to_c_with_comment() {} // okay: https://github.com/golang/lint/issues/144 + +//export maybe_exported_to_CPlusPlusWithCamelCase +func maybe_exported_to_CPlusPlusWithCamelCase() {} // 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 WhyAreYouUsingCapitalLetters_InACFunctionName() {} // MATCH /don't use underscores in Go names; func WhyAreYouUsingCapitalLetters_InACFunctionName should be WhyAreYouUsingCapitalLettersInACFunctionName/ diff --git a/linter/utils.go b/linter/utils.go new file mode 100644 index 0000000..3beebb5 --- /dev/null +++ b/linter/utils.go @@ -0,0 +1,118 @@ +package linter + +import ( + "strings" + "unicode" +) + +// LintName returns a different name if it should be different. +func LintName(name string) (should string) { + // Fast path for simple cases: "_" and all lowercase. + if name == "_" { + return name + } + allLower := true + for _, r := range name { + if !unicode.IsLower(r) { + allLower = false + break + } + } + if allLower { + return name + } + + // Split camelCase at any lower->upper transition, and split on underscores. + // Check each word for common initialisms. + runes := []rune(name) + w, i := 0, 0 // index of start of word, scan + for i+1 <= len(runes) { + eow := false // whether we hit the end of a word + if i+1 == len(runes) { + eow = true + } else if runes[i+1] == '_' { + // underscore; shift the remainder forward over any run of underscores + eow = true + n := 1 + for i+n+1 < len(runes) && runes[i+n+1] == '_' { + n++ + } + + // Leave at most one underscore if the underscore is between two digits + if i+n+1 < len(runes) && unicode.IsDigit(runes[i]) && unicode.IsDigit(runes[i+n+1]) { + n-- + } + + copy(runes[i+1:], runes[i+n+1:]) + runes = runes[:len(runes)-n] + } else if unicode.IsLower(runes[i]) && !unicode.IsLower(runes[i+1]) { + // lower->non-lower + eow = true + } + i++ + if !eow { + continue + } + + // [w,i) is a word. + word := string(runes[w:i]) + if u := strings.ToUpper(word); commonInitialisms[u] { + // Keep consistent case, which is lowercase only at the start. + if w == 0 && unicode.IsLower(runes[w]) { + u = strings.ToLower(u) + } + // All the common initialisms are ASCII, + // so we can replace the bytes exactly. + copy(runes[w:], []rune(u)) + } else if w > 0 && strings.ToLower(word) == word { + // already all lowercase, and not the first word, so uppercase the first character. + runes[w] = unicode.ToUpper(runes[w]) + } + w = i + } + return string(runes) +} + +// commonInitialisms is a set of common initialisms. +// Only add entries that are highly unlikely to be non-initialisms. +// For instance, "ID" is fine (Freudian code is rare), but "AND" is not. +var commonInitialisms = map[string]bool{ + "ACL": true, + "API": true, + "ASCII": true, + "CPU": true, + "CSS": true, + "DNS": true, + "EOF": true, + "GUID": true, + "HTML": true, + "HTTP": true, + "HTTPS": true, + "ID": true, + "IP": true, + "JSON": true, + "LHS": true, + "QPS": true, + "RAM": true, + "RHS": true, + "RPC": true, + "SLA": true, + "SMTP": true, + "SQL": true, + "SSH": true, + "TCP": true, + "TLS": true, + "TTL": true, + "UDP": true, + "UI": true, + "UID": true, + "UUID": true, + "URI": true, + "URL": true, + "UTF8": true, + "VM": true, + "XML": true, + "XMPP": true, + "XSRF": true, + "XSS": true, +} diff --git a/rule/argument-limit_test.go b/rule/argument-limit_test.go deleted file mode 100644 index e0b980e..0000000 --- a/rule/argument-limit_test.go +++ /dev/null @@ -1,34 +0,0 @@ -package rule - -import ( - "testing" - - "github.com/mgechev/revive/rule" - "github.com/mgechev/revive/testutil" -) - -func TestArgumentLimit(t *testing.T) { - t.Parallel() - - program := ` - package foo - - [@1]func foo(a int, b int, c int)[/@1] { - return a + b + c; - } - ` - testutil.AssertFailures(t, program, &ArgumentsLimitRule{}, rule.Arguments{"2"}) -} - -func TestArgumentLimit2(t *testing.T) { - t.Parallel() - - program := ` - package foo - - func foo(a int, b int, c int) { - return a + b + c; - } - ` - testutil.AssertSuccess(t, program, &ArgumentsLimitRule{}, rule.Arguments{"3"}) -} diff --git a/rule/names.go b/rule/names.go index 5e56a03..b15d7cb 100644 --- a/rule/names.go +++ b/rule/names.go @@ -5,7 +5,6 @@ import ( "go/ast" "go/token" "strings" - "unicode" "github.com/mgechev/revive/linter" ) @@ -30,6 +29,17 @@ func (r *NamesRule) Apply(file *linter.File, arguments linter.Arguments) []linte }, } + // Package names need slightly different handling than other names. + if strings.Contains(walker.fileAst.Name.Name, "_") && !strings.HasSuffix(walker.fileAst.Name.Name, "_test") { + walker.onFailure(linter.Failure{ + Failure: "don't use an underscore in package name", + Confidence: 1, + Node: walker.fileAst, + Category: "naming", + URL: "http://golang.org/doc/effective_go.html#package-names", + }) + } + ast.Walk(&walker, fileAst) return failures @@ -40,6 +50,71 @@ func (r *NamesRule) Name() string { return "imports" } +func checkList(fl *ast.FieldList, thing string, w *lintNames) { + if fl == nil { + return + } + for _, f := range fl.List { + for _, id := range f.Names { + check(id, thing, w) + } + } +} + +func check(id *ast.Ident, thing string, w *lintNames) { + if id.Name == "_" { + return + } + if knownNameExceptions[id.Name] { + return + } + + // Handle two common styles from other languages that don't belong in Go. + if len(id.Name) >= 5 && allCapsRE.MatchString(id.Name) && strings.Contains(id.Name, "_") { + w.onFailure(linter.Failure{ + Failure: "don't use ALL_CAPS in Go names; use CamelCase", + Confidence: 0.8, + Node: id, + Category: "naming", + URL: "#mixed-caps", + }) + return + } + 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:] + w.onFailure(linter.Failure{ + Failure: fmt.Sprintf("don't use leading k in Go names; %s %s should be %s", thing, id.Name, should), + Confidence: 0.8, + Node: id, + Category: "naming", + URL: "#mixed-caps", + }) + } + + should := linter.LintName(id.Name) + if id.Name == should { + return + } + + if len(id.Name) > 2 && strings.Contains(id.Name[1:], "_") { + w.onFailure(linter.Failure{ + Failure: fmt.Sprintf("don't use underscores in Go names; %s %s should be %s", thing, id.Name, should), + Confidence: 0.9, + Node: id, + Category: "naming", + URL: "http://golang.org/doc/effective_go.html#mixed-caps", + }) + return + } + w.onFailure(linter.Failure{ + Failure: fmt.Sprintf("%s %s should be %s", thing, id.Name, should), + Confidence: 0.8, + Node: id, + Category: "naming", + URL: "#initialisms", + }) +} + type lintNames struct { file *linter.File fileAst *ast.File @@ -49,12 +124,6 @@ type lintNames struct { } func (w *lintNames) Visit(n ast.Node) ast.Visitor { - - // 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, link("http://golang.org/doc/effective_go.html#package-names"), category("naming"), "don't use an underscore in package name") - // } - switch v := n.(type) { case *ast.AssignStmt: if v.Tok == token.ASSIGN { @@ -62,11 +131,11 @@ func (w *lintNames) Visit(n ast.Node) ast.Visitor { } for _, exp := range v.Lhs { if id, ok := exp.(*ast.Ident); ok { - w.check(id, "var") + check(id, "var", w) } } case *ast.FuncDecl: - if isTest(w.file) && (strings.HasPrefix(v.Name.Name, "Example") || strings.HasPrefix(v.Name.Name, "Test") || strings.HasPrefix(v.Name.Name, "Benchmark")) { + if w.file.IsTest() && (strings.HasPrefix(v.Name.Name, "Example") || strings.HasPrefix(v.Name.Name, "Test") || strings.HasPrefix(v.Name.Name, "Benchmark")) { return w } @@ -79,11 +148,11 @@ func (w *lintNames) Visit(n ast.Node) ast.Visitor { // not exported in the Go API. // See https://github.com/golang/lint/issues/144. if ast.IsExported(v.Name.Name) || !isCgoExported(v) { - w.check(v.Name, thing) + check(v.Name, thing, w) } - w.checkList(v.Type.Params, thing+" parameter") - w.checkList(v.Type.Results, thing+" result") + checkList(v.Type.Params, thing+" parameter", w) + checkList(v.Type.Results, thing+" result", w) case *ast.GenDecl: if v.Tok == token.IMPORT { return w @@ -100,10 +169,10 @@ func (w *lintNames) Visit(n ast.Node) ast.Visitor { for _, spec := range v.Specs { switch s := spec.(type) { case *ast.TypeSpec: - w.check(s.Name, thing) + check(s.Name, thing, w) case *ast.ValueSpec: for _, id := range s.Names { - w.check(id, thing) + check(id, thing, w) } } } @@ -115,150 +184,25 @@ func (w *lintNames) Visit(n ast.Node) ast.Visitor { if !ok { // might be an embedded interface name continue } - w.checkList(ft.Params, "interface method parameter") - w.checkList(ft.Results, "interface method result") + checkList(ft.Params, "interface method parameter", w) + checkList(ft.Results, "interface method result", w) } case *ast.RangeStmt: if v.Tok == token.ASSIGN { return w } if id, ok := v.Key.(*ast.Ident); ok { - w.check(id, "range var") + check(id, "range var", w) } if id, ok := v.Value.(*ast.Ident); ok { - w.check(id, "range var") + check(id, "range var", w) } case *ast.StructType: for _, f := range v.Fields.List { for _, id := range f.Names { - w.check(id, "struct field") + check(id, "struct field", w) } } } return w } - -func (w *lintNames) check(id *ast.Ident, thing string) { - if id.Name == "_" { - return - } - if knownNameExceptions[id.Name] { - return - } - - // Handle two common styles from other languages that don't belong in Go. - if len(id.Name) >= 5 && allCapsRE.MatchString(id.Name) && strings.Contains(id.Name, "_") { - w.onFailure(linter.Failure{ - Failure: fmt.Sprintf("don't use ALL_CAPS in Go names; use CamelCase"), - Confidence: 0.8, - Node: id, - }) - return - } - 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:] - w.onFailure(linter.Failure{ - Failure: fmt.Sprintf("don't use leading k in Go names; %s %s should be %s", thing, id.Name, should), - Confidence: 0.8, - Node: id, - }) - } - - should := lintName(id.Name) - if id.Name == should { - return - } - - if len(id.Name) > 2 && strings.Contains(id.Name[1:], "_") { - w.onFailure(linter.Failure{ - Failure: fmt.Sprintf("don't use underscores in Go names; %s %s should be %s", thing, id.Name, should), - Confidence: 0.9, - Node: id, - }) - return - } - w.onFailure(linter.Failure{ - Failure: fmt.Sprintf("%s %s should be %s", thing, id.Name, should), - Confidence: 0.8, - Node: id, - }) -} - -func (w *lintNames) checkList(fl *ast.FieldList, thing string) { - if fl == nil { - return - } - for _, f := range fl.List { - for _, id := range f.Names { - w.check(id, thing) - } - } -} - -// lintName returns a different name if it should be different. -func lintName(name string) (should string) { - // Fast path for simple cases: "_" and all lowercase. - if name == "_" { - return name - } - allLower := true - for _, r := range name { - if !unicode.IsLower(r) { - allLower = false - break - } - } - if allLower { - return name - } - - // Split camelCase at any lower->upper transition, and split on underscores. - // Check each word for common initialisms. - runes := []rune(name) - w, i := 0, 0 // index of start of word, scan - for i+1 <= len(runes) { - eow := false // whether we hit the end of a word - if i+1 == len(runes) { - eow = true - } else if runes[i+1] == '_' { - // underscore; shift the remainder forward over any run of underscores - eow = true - n := 1 - for i+n+1 < len(runes) && runes[i+n+1] == '_' { - n++ - } - - // Leave at most one underscore if the underscore is between two digits - if i+n+1 < len(runes) && unicode.IsDigit(runes[i]) && unicode.IsDigit(runes[i+n+1]) { - n-- - } - - copy(runes[i+1:], runes[i+n+1:]) - runes = runes[:len(runes)-n] - } else if unicode.IsLower(runes[i]) && !unicode.IsLower(runes[i+1]) { - // lower->non-lower - eow = true - } - i++ - if !eow { - continue - } - - // [w,i) is a word. - word := string(runes[w:i]) - if u := strings.ToUpper(word); commonInitialisms[u] { - // Keep consistent case, which is lowercase only at the start. - if w == 0 && unicode.IsLower(runes[w]) { - u = strings.ToLower(u) - } - // All the common initialisms are ASCII, - // so we can replace the bytes exactly. - copy(runes[w:], []rune(u)) - } else if w > 0 && strings.ToLower(word) == word { - // already all lowercase, and not the first word, so uppercase the first character. - runes[w] = unicode.ToUpper(runes[w]) - } - w = i - } - return string(runes) -} diff --git a/rule/no-else-return-rule.go b/rule/no-else-return.go similarity index 100% rename from rule/no-else-return-rule.go rename to rule/no-else-return.go diff --git a/testutil/lint_test.go b/testutil/lint_test.go index 024fe13..a3c715a 100644 --- a/testutil/lint_test.go +++ b/testutil/lint_test.go @@ -24,7 +24,6 @@ import ( "strconv" "strings" "testing" - "unicode" "github.com/mgechev/revive/rule" @@ -39,6 +38,7 @@ var rules = []linter.Rule{ &rule.DotImportsRule{}, &rule.BlankImportsRule{}, &rule.ExportedRule{}, + &rule.NamesRule{}, } func TestAll(t *testing.T) { @@ -242,118 +242,6 @@ func TestLine(t *testing.T) { } } -// commonInitialisms is a set of common initialisms. -// Only add entries that are highly unlikely to be non-initialisms. -// For instance, "ID" is fine (Freudian code is rare), but "AND" is not. -var commonInitialisms = map[string]bool{ - "ACL": true, - "API": true, - "ASCII": true, - "CPU": true, - "CSS": true, - "DNS": true, - "EOF": true, - "GUID": true, - "HTML": true, - "HTTP": true, - "HTTPS": true, - "ID": true, - "IP": true, - "JSON": true, - "LHS": true, - "QPS": true, - "RAM": true, - "RHS": true, - "RPC": true, - "SLA": true, - "SMTP": true, - "SQL": true, - "SSH": true, - "TCP": true, - "TLS": true, - "TTL": true, - "UDP": true, - "UI": true, - "UID": true, - "UUID": true, - "URI": true, - "URL": true, - "UTF8": true, - "VM": true, - "XML": true, - "XMPP": true, - "XSRF": true, - "XSS": true, -} - -// lintName returns a different name if it should be different. -func lintName(name string) (should string) { - // Fast path for simple cases: "_" and all lowercase. - if name == "_" { - return name - } - allLower := true - for _, r := range name { - if !unicode.IsLower(r) { - allLower = false - break - } - } - if allLower { - return name - } - - // Split camelCase at any lower->upper transition, and split on underscores. - // Check each word for common initialisms. - runes := []rune(name) - w, i := 0, 0 // index of start of word, scan - for i+1 <= len(runes) { - eow := false // whether we hit the end of a word - if i+1 == len(runes) { - eow = true - } else if runes[i+1] == '_' { - // underscore; shift the remainder forward over any run of underscores - eow = true - n := 1 - for i+n+1 < len(runes) && runes[i+n+1] == '_' { - n++ - } - - // Leave at most one underscore if the underscore is between two digits - if i+n+1 < len(runes) && unicode.IsDigit(runes[i]) && unicode.IsDigit(runes[i+n+1]) { - n-- - } - - copy(runes[i+1:], runes[i+n+1:]) - runes = runes[:len(runes)-n] - } else if unicode.IsLower(runes[i]) && !unicode.IsLower(runes[i+1]) { - // lower->non-lower - eow = true - } - i++ - if !eow { - continue - } - - // [w,i) is a word. - word := string(runes[w:i]) - if u := strings.ToUpper(word); commonInitialisms[u] { - // Keep consistent case, which is lowercase only at the start. - if w == 0 && unicode.IsLower(runes[w]) { - u = strings.ToLower(u) - } - // All the common initialisms are ASCII, - // so we can replace the bytes exactly. - copy(runes[w:], []rune(u)) - } else if w > 0 && strings.ToLower(word) == word { - // already all lowercase, and not the first word, so uppercase the first character. - runes[w] = unicode.ToUpper(runes[w]) - } - w = i - } - return string(runes) -} - func TestLintName(t *testing.T) { tests := []struct { name, want string @@ -387,7 +275,7 @@ func TestLintName(t *testing.T) { {"IEEE802_16Bit", "IEEE802_16Bit"}, } for _, test := range tests { - got := lintName(test.name) + got := linter.LintName(test.name) if got != test.want { t.Errorf("lintName(%q) = %q, want %q", test.name, got, test.want) }