Repository: mgechev/revive
Pull request #1163 (2024-12-04 07:31:43)

 refactor (rule/error-return): replace AST walker by iteration over declarations
Simplifies error-return rule by replacing AST walker by an iteration over global declarations



Patch:
diff --git a/rule/confusing_results.go b/rule/confusing_results.go
index 1b79ada..c1804d1 100644
--- a/rule/confusing_results.go
+++ b/rule/confusing_results.go
@@ -13,54 +13,43 @@ type ConfusingResultsRule struct{}
 func (*ConfusingResultsRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
 	var failures []lint.Failure
 
-	fileAst := file.AST
-	walker := lintConfusingResults{
-		onFailure: func(failure lint.Failure) {
-			failures = append(failures, failure)
-		},
-	}
+	for _, decl := range file.AST.Decls {
+		funcDecl, ok := decl.(*ast.FuncDecl)
 
-	ast.Walk(walker, fileAst)
+		isFunctionWithMoreThanOneResult := ok && funcDecl.Type.Results != nil && len(funcDecl.Type.Results.List) > 1
+		if !isFunctionWithMoreThanOneResult {
+			continue
+		}
 
-	return failures
-}
+		resultsAreNamed := len(funcDecl.Type.Results.List[0].Names) > 0
+		if resultsAreNamed {
+			continue
+		}
 
-// Name returns the rule name.
-func (*ConfusingResultsRule) Name() string {
-	return "confusing-results"
-}
+		lastType := ""
+		for _, result := range funcDecl.Type.Results.List {
 
-type lintConfusingResults struct {
-	onFailure func(lint.Failure)
-}
+			resultTypeName := gofmt(result.Type)
 
-func (w lintConfusingResults) Visit(n ast.Node) ast.Visitor {
-	fn, ok := n.(*ast.FuncDecl)
-	if !ok || fn.Type.Results == nil || len(fn.Type.Results.List) < 2 {
-		return w
-	}
-	lastType := ""
-	for _, result := range fn.Type.Results.List {
-		if len(result.Names) > 0 {
-			return w
-		}
+			if resultTypeName == lastType {
+				failures = append(failures, lint.Failure{
+					Node:       result,
+					Confidence: 1,
+					Category:   "naming",
+					Failure:    "unnamed results of the same type may be confusing, consider using named results",
+				})
 
-		t, ok := result.Type.(*ast.Ident)
-		if !ok {
-			return w
-		}
+				break
+			}
 
-		if t.Name == lastType {
-			w.onFailure(lint.Failure{
-				Node:       n,
-				Confidence: 1,
-				Category:   "naming",
-				Failure:    "unnamed results of the same type may be confusing, consider using named results",
-			})
-			break
+			lastType = resultTypeName
 		}
-		lastType = t.Name
 	}
 
-	return w
+	return failures
+}
+
+// Name returns the rule name.
+func (*ConfusingResultsRule) Name() string {
+	return "confusing-results"
 }
diff --git a/rule/cyclomatic.go b/rule/cyclomatic.go
index 60f8441..7f42a1c 100644
--- a/rule/cyclomatic.go
+++ b/rule/cyclomatic.go
@@ -38,53 +38,30 @@ func (r *CyclomaticRule) Apply(file *lint.File, arguments lint.Arguments) []lint
 	r.configureOnce.Do(func() { r.configure(arguments) })
 
 	var failures []lint.Failure
-	fileAst := file.AST
-
-	walker := lintCyclomatic{
-		file:       file,
-		complexity: r.maxComplexity,
-		onFailure: func(failure lint.Failure) {
-			failures = append(failures, failure)
-		},
-	}
-
-	ast.Walk(walker, fileAst)
-
-	return failures
-}
-
-// Name returns the rule name.
-func (*CyclomaticRule) Name() string {
-	return "cyclomatic"
-}
-
-type lintCyclomatic struct {
-	file       *lint.File
-	complexity int
-	onFailure  func(lint.Failure)
-}
-
-func (w lintCyclomatic) Visit(_ ast.Node) ast.Visitor {
-	f := w.file
-	for _, decl := range f.AST.Decls {
+	for _, decl := range file.AST.Decls {
 		fn, ok := decl.(*ast.FuncDecl)
 		if !ok {
 			continue
 		}
 
 		c := complexity(fn)
-		if c > w.complexity {
-			w.onFailure(lint.Failure{
+		if c > r.maxComplexity {
+			failures = append(failures, lint.Failure{
 				Confidence: 1,
 				Category:   "maintenance",
 				Failure: fmt.Sprintf("function %s has cyclomatic complexity %d (> max enabled %d)",
-					funcName(fn), c, w.complexity),
+					funcName(fn), c, r.maxComplexity),
 				Node: fn,
 			})
 		}
 	}
 
-	return nil
+	return failures
+}
+
+// Name returns the rule name.
+func (*CyclomaticRule) Name() string {
+	return "cyclomatic"
 }
 
 // funcName returns the name representation of a function or method:
diff --git a/rule/error_return.go b/rule/error_return.go
index f26d807..c5d3f46 100644
--- a/rule/error_return.go
+++ b/rule/error_return.go
@@ -13,16 +13,34 @@ type ErrorReturnRule struct{}
 func (*ErrorReturnRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
 	var failures []lint.Failure
 
-	fileAst := file.AST
-	walker := lintErrorReturn{
-		file:    file,
-		fileAst: fileAst,
-		onFailure: func(failure lint.Failure) {
-			failures = append(failures, failure)
-		},
-	}
+	for _, decl := range file.AST.Decls {
+		funcDecl, ok := decl.(*ast.FuncDecl)
+		isFunctionWithMoreThanOneResult := ok && funcDecl.Type.Results != nil && len(funcDecl.Type.Results.List) > 1
+		if !isFunctionWithMoreThanOneResult {
+			continue
+		}
+
+		funcResults := funcDecl.Type.Results.List
+		isLastResultError := isIdent(funcResults[len(funcResults)-1].Type, "error")
+		if isLastResultError {
+			continue
+		}
 
-	ast.Walk(walker, fileAst)
+		// An error return parameter should be the last parameter.
+		// Flag any error parameters found before the last.
+		for _, r := range funcResults[:len(funcResults)-1] {
+			if isIdent(r.Type, "error") {
+				failures = append(failures, lint.Failure{
+					Category:   "style",
+					Confidence: 0.9,
+					Node:       funcDecl,
+					Failure:    "error should be the last type when returning multiple items",
+				})
+
+				break // only flag one
+			}
+		}
+	}
 
 	return failures
 }
@@ -31,37 +49,3 @@ func (*ErrorReturnRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure
 func (*ErrorReturnRule) Name() string {
 	return "error-return"
 }
-
-type lintErrorReturn struct {
-	file      *lint.File
-	fileAst   *ast.File
-	onFailure func(lint.Failure)
-}
-
-func (w lintErrorReturn) Visit(n ast.Node) ast.Visitor {
-	fn, ok := n.(*ast.FuncDecl)
-	if !ok || fn.Type.Results == nil {
-		return w
-	}
-	ret := fn.Type.Results.List
-	if len(ret) <= 1 {
-		return w
-	}
-	if isIdent(ret[len(ret)-1].Type, "error") {
-		return nil
-	}
-	// An error return parameter should be the last parameter.
-	// Flag any error parameters found before the last.
-	for _, r := range ret[:len(ret)-1] {
-		if isIdent(r.Type, "error") {
-			w.onFailure(lint.Failure{
-				Category:   "arg-order",
-				Confidence: 0.9,
-				Node:       fn,
-				Failure:    "error should be the last type when returning multiple items",
-			})
-			break // only flag one
-		}
-	}
-	return w
-}


Test patch:
diff --git a/testdata/confusing_results.go b/testdata/confusing_results.go
index fc47abe..6f45414 100644
--- a/testdata/confusing_results.go
+++ b/testdata/confusing_results.go
@@ -15,6 +15,14 @@ func GetTaz(a string, b int) string {
 
 }
 
-func (t *t) GetTaz(a int, b int)  {
+func (t *t) GetTaz(a int, b int) {
 
 }
+
+func namedResults() (a string, b string) {
+	return "nil", "nil"
+}
+
+func pointerResults() (*string, *string) { // MATCH /unnamed results of the same type may be confusing, consider using named results/
+	return nil, nil
+}