Repository: mgechev/revive
Pull request #1174 (2024-12-07 10:48:24)

fix: ast_utils.FuncSignatureIs panics with types names other than ast.Ident
Fixes panic of ast_utils.FuncSignatureIs with type names of types other than ast.Ident (ast.SelectorExpression, ast.StarExpression)

The bug makes revive panic if the code under analysis has a method named Less, Len, or Swap with parameters of types represented as ast.SelectorExpression or ast.StarExpression in the AST.


Patch:
diff --git a/internal/astutils/ast_utils.go b/internal/astutils/ast_utils.go
index 5a66f11..3ca5d7f 100644
--- a/internal/astutils/ast_utils.go
+++ b/internal/astutils/ast_utils.go
@@ -1,6 +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.
@@ -45,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
@@ -58,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/rule/confusing_results.go b/rule/confusing_results.go
index c1804d1..1f5c16b 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 21a7a70..e6b4dcf 100644
--- a/rule/datarace.go
+++ b/rule/datarace.go
@@ -11,50 +11,45 @@ import (
 type DataRaceRule struct{}
 
 // Apply applies the rule to given file.
-func (*DataRaceRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
+func (r *DataRaceRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
+	isGo122 := file.Pkg.IsAtLeastGo122()
 	var failures []lint.Failure
-	onFailure := func(failure lint.Failure) {
-		failures = append(failures, failure)
-	}
-	w := lintDataRaces{onFailure: onFailure, go122for: file.Pkg.IsAtLeastGo122()}
+	for _, decl := range file.AST.Decls {
+		funcDecl, ok := decl.(*ast.FuncDecl)
+		if !ok || funcDecl.Body == nil {
+			continue // not function declaration or empty function
+		}
 
-	ast.Walk(w, file.AST)
+		funcResults := funcDecl.Type.Results
 
-	return failures
-}
+		returnIDs := map[*ast.Object]struct{}{}
+		if funcResults != nil {
+			returnIDs = r.extractReturnIDs(funcResults.List)
+		}
 
-// Name returns the rule name.
-func (*DataRaceRule) Name() string {
-	return "datarace"
-}
+		onFailure := func(failure lint.Failure) {
+			failures = append(failures, failure)
+		}
 
-type lintDataRaces struct {
-	onFailure func(failure lint.Failure)
-	go122for  bool
-}
+		fl := &lintFunctionForDataRaces{
+			onFailure: onFailure,
+			returnIDs: returnIDs,
+			rangeIDs:  map[*ast.Object]struct{}{},
+			go122for:  isGo122,
+		}
 
-func (w lintDataRaces) Visit(n ast.Node) ast.Visitor {
-	node, ok := n.(*ast.FuncDecl)
-	if !ok {
-		return w // not function declaration
+		ast.Walk(fl, funcDecl.Body)
 	}
-	if node.Body == nil {
-		return nil // empty body
-	}
-
-	results := node.Type.Results
 
-	returnIDs := map[*ast.Object]struct{}{}
-	if results != nil {
-		returnIDs = w.ExtractReturnIDs(results.List)
-	}
-	fl := &lintFunctionForDataRaces{onFailure: w.onFailure, returnIDs: returnIDs, rangeIDs: map[*ast.Object]struct{}{}, go122for: w.go122for}
-	ast.Walk(fl, node.Body)
+	return failures
+}
 
-	return nil
+// Name returns the rule name.
+func (*DataRaceRule) Name() string {
+	return "datarace"
 }
 
-func (lintDataRaces) 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 {
diff --git a/rule/get_return.go b/rule/get_return.go
index 638c202..b16e271 100644
--- a/rule/get_return.go
+++ b/rule/get_return.go
@@ -15,12 +15,28 @@ type GetReturnRule struct{}
 func (*GetReturnRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
 	var failures []lint.Failure
 
-	onFailure := func(failure lint.Failure) {
-		failures = append(failures, failure)
+	for _, decl := range file.AST.Decls {
+		fd, ok := decl.(*ast.FuncDecl)
+		if !ok {
+			continue
+		}
+
+		if !isGetter(fd.Name.Name) {
+			continue
+		}
+
+		if hasResults(fd.Type.Results) {
+			continue
+		}
+
+		failures = append(failures, lint.Failure{
+			Confidence: 0.8,
+			Node:       fd,
+			Category:   "logic",
+			Failure:    fmt.Sprintf("function '%s' seems to be a getter but it does not return any result", fd.Name.Name),
+		})
 	}
 
-	w := lintReturnRule{onFailure}
-	ast.Walk(w, file.AST)
 	return failures
 }
 
@@ -29,10 +45,6 @@ func (*GetReturnRule) Name() string {
 	return "get-return"
 }
 
-type lintReturnRule struct {
-	onFailure func(lint.Failure)
-}
-
 const getterPrefix = "GET"
 
 var lenGetterPrefix = len(getterPrefix)
@@ -57,24 +69,3 @@ func isGetter(name string) bool {
 func hasResults(rs *ast.FieldList) bool {
 	return rs != nil && len(rs.List) > 0
 }
-
-func (w lintReturnRule) Visit(node ast.Node) ast.Visitor {
-	fd, ok := node.(*ast.FuncDecl)
-	if !ok {
-		return w
-	}
-
-	if !isGetter(fd.Name.Name) {
-		return w
-	}
-	if !hasResults(fd.Type.Results) {
-		w.onFailure(lint.Failure{
-			Confidence: 0.8,
-			Node:       fd,
-			Category:   "logic",
-			Failure:    fmt.Sprintf("function '%s' seems to be a getter but it does not return any result", fd.Name.Name),
-		})
-	}
-
-	return w
-}
diff --git a/rule/modifies_value_receiver.go b/rule/modifies_value_receiver.go
index d811c48..a33a79a 100644
--- a/rule/modifies_value_receiver.go
+++ b/rule/modifies_value_receiver.go
@@ -29,7 +29,7 @@ func (r *ModifiesValRecRule) Apply(file *lint.File, _ lint.Arguments) []lint.Fai
 		}
 
 		receiverName := receiver.Names[0].Name
-		assignmentsToReceiver := r.getAssignmentsToReceiver(receiverName, funcDecl.Body)
+		assignmentsToReceiver := r.getReceiverModifications(receiverName, funcDecl.Body)
 		if len(assignmentsToReceiver) == 0 {
 			continue // receiver is not modified
 		}
@@ -140,38 +140,44 @@ func (r *ModifiesValRecRule) mustSkip(receiver *ast.Field, pkg *lint.Package) bo
 	return false
 }
 
-func (r *ModifiesValRecRule) getAssignmentsToReceiver(receiverName string, funcBody *ast.BlockStmt) []ast.Node {
-	receiverAssignmentFinder := func(n ast.Node) bool {
-		// look for assignments with the receiver in the right hand
-		assignment, ok := n.(*ast.AssignStmt)
-		if !ok {
-			return false
-		}
+func (r *ModifiesValRecRule) getReceiverModifications(receiverName string, funcBody *ast.BlockStmt) []ast.Node {
+	receiverModificationFinder := func(n ast.Node) bool {
+		switch node := n.(type) {
+		case *ast.IncDecStmt:
+			se, ok := node.X.(*ast.SelectorExpr)
+			if !ok {
+				return false
+			}
 
-		for _, exp := range assignment.Lhs {
-			switch e := exp.(type) {
-			case *ast.IndexExpr: // receiver...[] = ...
-				continue
-			case *ast.StarExpr: // *receiver = ...
-				continue
-			case *ast.SelectorExpr: // receiver.field = ...
-				name := r.getNameFromExpr(e.X)
-				if name == "" || name != receiverName {
+			name := r.getNameFromExpr(se.X)
+			return name == receiverName
+		case *ast.AssignStmt:
+			// look for assignments with the receiver in the right hand
+			for _, exp := range node.Lhs {
+				switch e := exp.(type) {
+				case *ast.IndexExpr: // receiver...[] = ...
 					continue
-				}
-			case *ast.Ident: // receiver := ...
-				if e.Name != receiverName {
+				case *ast.StarExpr: // *receiver = ...
+					continue
+				case *ast.SelectorExpr: // receiver.field = ...
+					name := r.getNameFromExpr(e.X)
+					if name == "" || name != receiverName {
+						continue
+					}
+				case *ast.Ident: // receiver := ...
+					if e.Name != receiverName {
+						continue
+					}
+				default:
 					continue
 				}
-			default:
-				continue
-			}
 
-			return true
+				return true
+			}
 		}
 
 		return false
 	}
 
-	return pick(funcBody, receiverAssignmentFinder)
+	return pick(funcBody, receiverModificationFinder)
 }


Test patch:
diff --git a/testdata/golint/sort.go b/testdata/golint/sort.go
index 80901f4..0a101fe 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/
diff --git a/testdata/modifies_value_receiver.go b/testdata/modifies_value_receiver.go
index e6a7166..11757c9 100644
--- a/testdata/modifies_value_receiver.go
+++ b/testdata/modifies_value_receiver.go
@@ -28,3 +28,22 @@ func (b JailerCommandBuilder) WithBin(bin string) JailerCommandBuilder {
 	b.bin = bin
 	return b
 }
+
+func (this data) incrementDecrement() {
+	this.num++ // MATCH /suspicious assignment to a by-value method receiver/
+	this.num-- // MATCH /suspicious assignment to a by-value method receiver/
+	other++
+}
+
+func (this data) assignmentOperators() {
+	this.num += 1  // MATCH /suspicious assignment to a by-value method receiver/
+	this.num -= 1  // MATCH /suspicious assignment to a by-value method receiver/
+	this.num *= 1  // MATCH /suspicious assignment to a by-value method receiver/
+	this.num /= 1  // MATCH /suspicious assignment to a by-value method receiver/
+	this.num %= 1  // MATCH /suspicious assignment to a by-value method receiver/
+	this.num &= 1  // MATCH /suspicious assignment to a by-value method receiver/
+	this.num ^= 1  // MATCH /suspicious assignment to a by-value method receiver/
+	this.num |= 1  // MATCH /suspicious assignment to a by-value method receiver/
+	this.num >>= 1 // MATCH /suspicious assignment to a by-value method receiver/
+	this.num <<= 1 // MATCH /suspicious assignment to a by-value method receiver/
+}