From 06192a559997d540988315ced58a70bdf6a316d3 Mon Sep 17 00:00:00 2001 From: chavacava Date: Sat, 7 Dec 2024 14:43:25 +0100 Subject: [PATCH 1/3] refactor: remove lint issues (#1176) --- internal/astutils/ast_utils.go | 1 + rule/confusing_results.go | 1 - rule/datarace.go | 4 ++-- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/astutils/ast_utils.go b/internal/astutils/ast_utils.go index 5a66f11fa..876d9a7a5 100644 --- a/internal/astutils/ast_utils.go +++ b/internal/astutils/ast_utils.go @@ -1,3 +1,4 @@ +// Package astutils provides utility functions for working with AST nodes package astutils import "go/ast" diff --git a/rule/confusing_results.go b/rule/confusing_results.go index c1804d124..1f5c16ba4 100644 --- a/rule/confusing_results.go +++ b/rule/confusing_results.go @@ -28,7 +28,6 @@ func (*ConfusingResultsRule) Apply(file *lint.File, _ lint.Arguments) []lint.Fai lastType := "" for _, result := range funcDecl.Type.Results.List { - resultTypeName := gofmt(result.Type) if resultTypeName == lastType { diff --git a/rule/datarace.go b/rule/datarace.go index d95775d91..e6b4dcf46 100644 --- a/rule/datarace.go +++ b/rule/datarace.go @@ -24,7 +24,7 @@ func (r *DataRaceRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { returnIDs := map[*ast.Object]struct{}{} if funcResults != nil { - returnIDs = r.ExtractReturnIDs(funcResults.List) + returnIDs = r.extractReturnIDs(funcResults.List) } onFailure := func(failure lint.Failure) { @@ -49,7 +49,7 @@ func (*DataRaceRule) Name() string { return "datarace" } -func (*DataRaceRule) ExtractReturnIDs(fields []*ast.Field) map[*ast.Object]struct{} { +func (*DataRaceRule) extractReturnIDs(fields []*ast.Field) map[*ast.Object]struct{} { r := map[*ast.Object]struct{}{} for _, f := range fields { for _, id := range f.Names { From 24a9325b5a66a5100382ceef3cd82283c6419919 Mon Sep 17 00:00:00 2001 From: chavacava Date: Sat, 7 Dec 2024 15:40:39 +0100 Subject: [PATCH 2/3] fix: ast_utils.FuncSignatureIs panics with types names other than ast.Ident (#1174) --- internal/astutils/ast_utils.go | 20 ++++++++++++++++++-- testdata/golint/sort.go | 5 +++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/internal/astutils/ast_utils.go b/internal/astutils/ast_utils.go index 876d9a7a5..3ca5d7f67 100644 --- a/internal/astutils/ast_utils.go +++ b/internal/astutils/ast_utils.go @@ -1,7 +1,10 @@ // Package astutils provides utility functions for working with AST nodes package astutils -import "go/ast" +import ( + "fmt" + "go/ast" +) // FuncSignatureIs returns true if the given func decl satisfies a signature characterized // by the given name, parameters types and return types; false otherwise. @@ -46,7 +49,7 @@ func getTypeNames(fields *ast.FieldList) []string { } for _, field := range fields.List { - typeName := field.Type.(*ast.Ident).Name + typeName := getFieldTypeName(field.Type) if field.Names == nil { // unnamed field result = append(result, typeName) continue @@ -59,3 +62,16 @@ func getTypeNames(fields *ast.FieldList) []string { return result } + +func getFieldTypeName(typ ast.Expr) string { + switch f := typ.(type) { + case *ast.Ident: + return f.Name + case *ast.SelectorExpr: + return f.Sel.Name + "." + getFieldTypeName(f.X) + case *ast.StarExpr: + return "*" + getFieldTypeName(f.X) + default: + panic(fmt.Sprintf("not supported type %T", typ)) + } +} diff --git a/testdata/golint/sort.go b/testdata/golint/sort.go index 80901f48b..0a101feb8 100644 --- a/testdata/golint/sort.go +++ b/testdata/golint/sort.go @@ -36,3 +36,8 @@ type Vv []int func (vv Vv) Len() (result int) { return len(w) } // MATCH /exported method Vv.Len should have comment or be unexported/ func (vv Vv) Less(i int, j int) (result bool) { return w[i] < w[j] } // MATCH /exported method Vv.Less should have comment or be unexported/ + +// X is ... +type X []int + +func (x X) Less(i *pkg.tip) (result bool) { return len(x) } // MATCH /exported method X.Less should have comment or be unexported/ From 5c2aadfa917faac0461e4dcda1cd5f8886bc7457 Mon Sep 17 00:00:00 2001 From: chavacava Date: Sat, 7 Dec 2024 23:04:31 +0100 Subject: [PATCH 3/3] refactor (rule/receiver-naming): replace AST walker by iteration over declarations (#1169) Co-authored-by: chavacava --- rule/receiver_naming.go | 122 ++++++++++++++++++---------------------- 1 file changed, 54 insertions(+), 68 deletions(-) diff --git a/rule/receiver_naming.go b/rule/receiver_naming.go index e1060a527..1d66875c2 100644 --- a/rule/receiver_naming.go +++ b/rule/receiver_naming.go @@ -47,18 +47,63 @@ func (r *ReceiverNamingRule) configure(arguments lint.Arguments) { func (r *ReceiverNamingRule) Apply(file *lint.File, args lint.Arguments) []lint.Failure { r.configureOnce.Do(func() { r.configure(args) }) + typeReceiver := map[string]string{} var failures []lint.Failure + for _, decl := range file.AST.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Recv == nil || len(fn.Recv.List) == 0 { + continue + } - fileAst := file.AST - walker := lintReceiverName{ - onFailure: func(failure lint.Failure) { - failures = append(failures, failure) - }, - typeReceiver: map[string]string{}, - receiverNameMaxLength: r.receiverNameMaxLength, - } + names := fn.Recv.List[0].Names + if len(names) < 1 { + continue + } + name := names[0].Name + + if name == "_" { + failures = append(failures, lint.Failure{ + Node: decl, + Confidence: 1, + Category: "naming", + Failure: "receiver name should not be an underscore, omit the name if it is unused", + }) + continue + } + + if name == "this" || name == "self" { + failures = append(failures, lint.Failure{ + Node: decl, + Confidence: 1, + Category: "naming", + Failure: `receiver name should be a reflection of its identity; don't use generic names such as "this" or "self"`, + }) + continue + } - ast.Walk(walker, fileAst) + if r.receiverNameMaxLength > 0 && len([]rune(name)) > r.receiverNameMaxLength { + failures = append(failures, lint.Failure{ + Node: decl, + Confidence: 1, + Category: "naming", + Failure: fmt.Sprintf("receiver name %s is longer than %d characters", name, r.receiverNameMaxLength), + }) + continue + } + + recv := typeparams.ReceiverType(fn) + if prev, ok := typeReceiver[recv]; ok && prev != name { + failures = append(failures, lint.Failure{ + Node: decl, + Confidence: 1, + Category: "naming", + Failure: fmt.Sprintf("receiver name %s should be consistent with previous receiver name %s for %s", name, prev, recv), + }) + continue + } + + typeReceiver[recv] = name + } return failures } @@ -67,62 +112,3 @@ func (r *ReceiverNamingRule) Apply(file *lint.File, args lint.Arguments) []lint. func (*ReceiverNamingRule) Name() string { return "receiver-naming" } - -type lintReceiverName struct { - onFailure func(lint.Failure) - typeReceiver map[string]string - receiverNameMaxLength int -} - -func (w lintReceiverName) Visit(n ast.Node) ast.Visitor { - fn, ok := n.(*ast.FuncDecl) - if !ok || fn.Recv == nil || len(fn.Recv.List) == 0 { - return w - } - names := fn.Recv.List[0].Names - if len(names) < 1 { - return w - } - name := names[0].Name - if name == "_" { - w.onFailure(lint.Failure{ - Node: n, - Confidence: 1, - Category: "naming", - Failure: "receiver name should not be an underscore, omit the name if it is unused", - }) - return w - } - if name == "this" || name == "self" { - w.onFailure(lint.Failure{ - Node: n, - Confidence: 1, - Category: "naming", - Failure: `receiver name should be a reflection of its identity; don't use generic names such as "this" or "self"`, - }) - return w - } - - if w.receiverNameMaxLength > 0 && len([]rune(name)) > w.receiverNameMaxLength { - w.onFailure(lint.Failure{ - Node: n, - Confidence: 1, - Category: "naming", - Failure: fmt.Sprintf("receiver name %s is longer than %d characters", name, w.receiverNameMaxLength), - }) - return w - } - - recv := typeparams.ReceiverType(fn) - if prev, ok := w.typeReceiver[recv]; ok && prev != name { - w.onFailure(lint.Failure{ - Node: n, - Confidence: 1, - Category: "naming", - Failure: fmt.Sprintf("receiver name %s should be consistent with previous receiver name %s for %s", name, prev, recv), - }) - return w - } - w.typeReceiver[recv] = name - return w -}