Merge branch 'master' into rules-avoid-using-panic

# Conflicts: # rule/datarace.go # rule/get_return.go
mgechev · Dec 7, 2024 · 92bcf9e · 92bcf9e
2 parents 9c9e05e + a106e63
commit 92bcf9e
Show file tree

Hide file tree

Showing 4 changed files with 98 additions and 87 deletions.
diff --git 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, error) {
+func (r *DataRaceRule) Apply(file *lint.File, _ lint.Arguments) ([]lint.Failure, error) {
+	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, nil
-}
+		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, nil
+}
 
-	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
@@ -15,12 +15,28 @@ type GetReturnRule struct{}
 func (*GetReturnRule) Apply(file *lint.File, _ lint.Arguments) ([]lint.Failure, error) {
 	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, nil
 }
 
@@ -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
@@ -29,7 +29,7 @@ func (r *ModifiesValRecRule) Apply(file *lint.File, _ lint.Arguments) ([]lint.Fa
 		}
 
 		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)
 }
diff --git 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/
+}