Guard code-quality recipes against emitting invalid Go

recipes/activate.go

@@ -120,6 +120,7 @@ func Activate(r *recipe.Registry) {
 	r.Register(&style.AvoidGlobalVariable{}, golang, codeQuality, styleCategory)
 	r.Register(&style.PreferRawStringForRegex{}, golang, codeQuality, styleCategory)
 	r.Register(&style.UseCryptoRand{}, golang, codeQuality, styleCategory)
+	r.Register(&style.UseStrongHash{}, golang, codeQuality, styleCategory)
 	r.Register(&style.AvoidDotImport{}, golang, codeQuality, styleCategory)
 	r.Register(&style.PreferHexEncoding{}, golang, codeQuality, styleCategory)
 	r.Register(&style.PreferStrconvQuote{}, golang, codeQuality, styleCategory)
@@ -190,6 +191,10 @@ func Activate(r *recipe.Registry) {
 	r.Register(&errorhandling.PreferErrorsIsContext{}, golang, codeQuality, errCategory)
 	r.Register(&errorhandling.PreferErrorsIsEOF{}, golang, codeQuality, errCategory)
 	r.Register(&errorhandling.PreferErrorsIsForFieldAccess{}, golang, codeQuality, errCategory)
+	r.Register(&errorhandling.PreferErrorsIsSqlNoRows{}, golang, codeQuality, errCategory)
+	r.Register(&errorhandling.PreferErrorsIsHttpServerClosed{}, golang, codeQuality, errCategory)
+	r.Register(&errorhandling.PreferErrorsIsNetClosed{}, golang, codeQuality, errCategory)
+	r.Register(&errorhandling.PreferErrorsIsOsInvalid{}, golang, codeQuality, errCategory)
 	r.Register(&errorhandling.UseErrorMethod{}, golang, codeQuality, errCategory)
 	r.Register(&errorhandling.CheckContextError{}, golang, codeQuality, errCategory)
 	r.Register(&errorhandling.AuditMustFunction{}, golang, codeQuality, errCategory)

recipes/errorhandling/check_close_error.go

@@ -6,7 +6,6 @@ package errorhandling
 
 import (
 	"github.com/openrewrite/rewrite/rewrite-go/pkg/recipe"
-	"github.com/openrewrite/rewrite/rewrite-go/pkg/tree/golang"
 	"github.com/openrewrite/rewrite/rewrite-go/pkg/tree/java"
 	"github.com/openrewrite/rewrite/rewrite-go/pkg/visitor"
 )
@@ -33,21 +32,18 @@ func (r *CheckCloseError) Editor() recipe.TreeVisitor {
 
 type checkCloseErrorVisitor struct {
 	visitor.GoVisitor
-	insideAssignment int
 }
 
-func (v *checkCloseErrorVisitor) VisitAssignment(assign *java.Assignment, p any) java.J {
-	v.insideAssignment++
-	assign = v.GoVisitor.VisitAssignment(assign, p).(*java.Assignment)
-	v.insideAssignment--
-	return assign
-}
-
-func (v *checkCloseErrorVisitor) VisitMultiAssignment(ma *golang.MultiAssignment, p any) java.J {
-	v.insideAssignment++
-	ma = v.GoVisitor.VisitMultiAssignment(ma, p).(*golang.MultiAssignment)
-	v.insideAssignment--
-	return ma
+// Reports whether mi's method returns exactly one value, the only case where
+// `_ = mi` compiles.
+func returnsSingleValue(mi *java.MethodInvocation) bool {
+	if mi.MethodType == nil || mi.MethodType.ReturnType == nil {
+		return false
+	}
+	if _, isTuple := mi.MethodType.ReturnType.(*java.JavaTypeParameterized); isTuple {
+		return false
+	}
+	return java.TypeSignature(mi.MethodType.ReturnType) != "void"
 }
 
 func (v *checkCloseErrorVisitor) VisitMethodInvocation(mi *java.MethodInvocation, p any) java.J {
@@ -58,9 +54,19 @@ func (v *checkCloseErrorVisitor) VisitMethodInvocation(mi *java.MethodInvocation
 		return mi
 	}
 
-	// Only transform bare statement calls. If this MethodInvocation is already
-	// the RHS of an assignment, skip it.
-	if v.insideAssignment > 0 {
+	// Only wraps a Close() that stands alone as a statement; a call whose result
+	// is consumed, such as `return x.Close()`, has a non-block parent.
+	parent := v.Cursor().Parent()
+	if parent == nil {
+		return mi
+	}
+	if _, ok := parent.Value().(*java.Block); !ok {
+		return mi
+	}
+
+	// `_ = x.Close()` only compiles when Close returns exactly one value; skip a
+	// void or multi-value Close.
+	if !returnsSingleValue(mi) {
 		return mi
 	}
 

recipes/errorhandling/errors_is_common.go

@@ -0,0 +1,74 @@
+/*
+ * Moderne Proprietary. Only for use by Moderne customers under the terms of a commercial contract.
+ */
+
+package errorhandling
+
+import (
+	recipegolang "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe/golang"
+	"github.com/openrewrite/rewrite/rewrite-go/pkg/tree/java"
+	"github.com/openrewrite/rewrite/rewrite-go/pkg/visitor"
+)
+
+// rewriteToErrorsIs builds `errors.Is(errExpr, sentinel)` (or its negation for a
+// `!=` binary) with bin's leading prefix, adding the errors import. It returns
+// bin unchanged when either operand is not an error value, since errors.Is
+// requires both arguments to be assignable to error.
+func rewriteToErrorsIs(v visitor.AfterVisitsProvider, bin *java.Binary, errExpr, sentinel java.Expression) java.J {
+	if !isErrorAssignable(errExpr) || !isErrorAssignable(sentinel) {
+		return bin
+	}
+
+	recipegolang.MaybeAddImport(v, "errors", nil, false)
+
+	// The leading whitespace lives on the outermost element, so carry the
+	// binary's prefix onto whichever node ends up outermost.
+	prefix := getLeadingPrefixExpr(bin)
+
+	sentinelArg := setExprPrefixLocal(stripExprPrefix(sentinel), java.Space{Whitespace: " "})
+	isCall := &java.MethodInvocation{
+		Select: &java.RightPadded[java.Expression]{Element: &java.Identifier{Name: "errors"}},
+		Name:   &java.Identifier{Name: "Is"},
+		Arguments: java.Container[java.Expression]{
+			Elements: []java.RightPadded[java.Expression]{
+				{Element: stripExprPrefix(errExpr)},
+				{Element: sentinelArg},
+			},
+		},
+	}
+
+	if bin.Operator.Element == java.NotEqual {
+		return &java.Unary{
+			Prefix:   prefix,
+			Operator: java.LeftPadded[java.UnaryOperator]{Element: java.Not},
+			Operand:  isCall,
+		}
+	}
+	return isCall.WithPrefix(prefix)
+}
+
+// matchSentinel returns (errExpr, sentinel, true) when one side of bin is the
+// package-qualified sentinel `pkg.name` (e.g. io.EOF), with the other side as
+// the error expression.
+func matchSentinel(bin *java.Binary, pkg, name string) (java.Expression, java.Expression, bool) {
+	if isSentinel(bin.Right, pkg, name) {
+		return bin.Left, bin.Right, true
+	}
+	if isSentinel(bin.Left, pkg, name) {
+		return bin.Right, bin.Left, true
+	}
+	return nil, nil, false
+}
+
+// isSentinel reports whether expr is the package-qualified value `pkg.name`.
+func isSentinel(expr java.Expression, pkg, name string) bool {
+	fa, ok := expr.(*java.FieldAccess)
+	if !ok {
+		return false
+	}
+	pkgIdent, ok := fa.Target.(*java.Identifier)
+	if !ok || pkgIdent.Name != pkg {
+		return false
+	}
+	return fa.Name.Element != nil && fa.Name.Element.Name == name
+}

recipes/errorhandling/handle_deferred_close_error.go

@@ -47,6 +47,12 @@ func (v *handleDeferredCloseErrorVisitor) VisitDefer(d *golang.Defer, p any) jav
 		return d
 	}
 
+	// `_ = x.Close()` only compiles when Close returns exactly one value; skip a
+	// void or multi-value Close.
+	if !returnsSingleValue(mi) {
+		return d
+	}
+
 	// Build: defer func() { _ = f.Close() }()
 	//
 	// Step 1: Move the original Close() call. The space after `=` lives on the

recipes/errorhandling/handle_error_return.go

@@ -5,16 +5,18 @@
 package errorhandling
 
 import (
+	"github.com/google/uuid"
 	"github.com/moderneinc/recipes-go/diagnostic"
+	"github.com/moderneinc/recipes-go/recipes/internal/lstutil"
 	"github.com/openrewrite/rewrite/rewrite-go/pkg/recipe"
 	"github.com/openrewrite/rewrite/rewrite-go/pkg/tree/golang"
 	"github.com/openrewrite/rewrite/rewrite-go/pkg/tree/java"
 	"github.com/openrewrite/rewrite/rewrite-go/pkg/visitor"
 )
 
-// HandleErrorReturn replaces the blank identifier `_` in the last position of
-// a multi-assignment with `err`, capturing the previously discarded error value.
-// `_, _ = f()` becomes `_, err = f()`.
+// Replaces a discarded trailing `_` in a `:=` capture with `err` and adds an
+// `if err != nil { return err }` guard, firing only in the top-level block of a
+// function that returns a single `error`.
 // golangci-lint: errcheck
 type HandleErrorReturn struct {
 	recipe.Base
@@ -43,22 +45,87 @@ type handleErrorReturnVisitor struct {
 	visitor.GoVisitor
 }
 
-func (v *handleErrorReturnVisitor) VisitMultiAssignment(ma *golang.MultiAssignment, p any) java.J {
-	ma = v.GoVisitor.VisitMultiAssignment(ma, p).(*golang.MultiAssignment)
+func (v *handleErrorReturnVisitor) VisitBlock(block *java.Block, p any) java.J {
+	block = v.GoVisitor.VisitBlock(block, p).(*java.Block)
 
-	if len(ma.Variables) == 0 {
-		return ma
+	// Only rewrite the function's top-level block, since a `return err` in a
+	// nested loop or if would change control flow.
+	if !lstutil.IsFunctionBodyBlock(v.Cursor()) {
+		return block
 	}
 
-	// Check if the last LHS variable is the blank identifier `_`.
-	lastVar := ma.Variables[len(ma.Variables)-1]
-	ident, ok := lastVar.Element.(*java.Identifier)
-	if !ok || ident.Name != "_" {
-		return ma
+	// Bail unless the enclosing function returns a single error, so `return err`
+	// compiles and the captured `err` is used.
+	if !enclosingReturnsSingleError(v.Cursor()) {
+		return block
+	}
+
+	changed := false
+	var newStmts []java.RightPadded[java.Statement]
+	for _, rp := range block.Statements {
+		ma, ok := rp.Element.(*golang.MultiAssignment)
+		if !ok || !capturesDiscardedError(ma) || !discardsAnError(ma) {
+			newStmts = append(newStmts, rp)
+			continue
+		}
+
+		newStmts = append(newStmts, java.RightPadded[java.Statement]{
+			Element: withErrCapture(ma),
+			After:   rp.After,
+			Markers: rp.Markers,
+		})
+		newStmts = append(newStmts, java.RightPadded[java.Statement]{
+			Element: buildReturnErrGuard(lstutil.BaseIndent(ma.Prefix)),
+		})
+		changed = true
+	}
+
+	if !changed {
+		return block
+	}
+	return block.WithStatements(newStmts)
+}
+
+// Reports whether ma is a `:=` declaration whose last variable is `_` and which
+// declares at least one non-blank variable.
+func capturesDiscardedError(ma *golang.MultiAssignment) bool {
+	if !java.HasMarker[golang.ShortVarDecl](ma.Markers) || len(ma.Variables) < 2 {
+		return false
+	}
+	last, ok := ma.Variables[len(ma.Variables)-1].Element.(*java.Identifier)
+	if !ok || last.Name != "_" {
+		return false
+	}
+	for _, v := range ma.Variables[:len(ma.Variables)-1] {
+		if id, ok := v.Element.(*java.Identifier); ok && id.Name != "_" {
+			return true
+		}
 	}
+	return false
+}
 
-	// Replace `_` with `err` to capture the error value.
-	replaced := ident.WithName("err")
+// Reports whether ma's value is a function call whose last result is of type
+// error, excluding comma-ok forms and non-error last results.
+func discardsAnError(ma *golang.MultiAssignment) bool {
+	if len(ma.Values) != 1 {
+		return false
+	}
+	mi, ok := ma.Values[0].Element.(*java.MethodInvocation)
+	if !ok || mi.MethodType == nil {
+		return false
+	}
+	pz, ok := mi.MethodType.ReturnType.(*java.JavaTypeParameterized)
+	if !ok || len(pz.TypeParameters) == 0 {
+		return false
+	}
+	last := pz.TypeParameters[len(pz.TypeParameters)-1]
+	return java.TypeSignature(last) == "error"
+}
+
+// Returns a copy of ma with the trailing blank identifier renamed to `err`.
+func withErrCapture(ma *golang.MultiAssignment) *golang.MultiAssignment {
+	lastVar := ma.Variables[len(ma.Variables)-1]
+	replaced := lastVar.Element.(*java.Identifier).WithName("err")
 	vars := make([]java.RightPadded[java.Expression], len(ma.Variables))
 	copy(vars, ma.Variables)
 	vars[len(vars)-1] = java.RightPadded[java.Expression]{
@@ -70,3 +137,37 @@ func (v *handleErrorReturnVisitor) VisitMultiAssignment(ma *golang.MultiAssignme
 	c.Variables = vars
 	return &c
 }
+
+// Constructs `if err != nil { return err }`, indented to sit at the same level
+// (base) as the assignment it follows.
+func buildReturnErrGuard(base string) *java.If {
+	cond := &java.ControlParentheses{
+		ID: uuid.New(),
+		Tree: java.RightPadded[java.Expression]{Element: &java.Binary{
+			ID:       uuid.New(),
+			Left:     &java.Identifier{ID: uuid.New(), Prefix: java.SingleSpace, Name: "err"},
+			Operator: java.LeftPadded[java.BinaryOperator]{Before: java.SingleSpace, Element: java.NotEqual},
+			Right:    &java.Identifier{ID: uuid.New(), Prefix: java.SingleSpace, Name: "nil"},
+		}},
+	}
+
+	ret := &java.Return{
+		ID:         uuid.New(),
+		Prefix:     java.Space{Whitespace: "\n" + base + "\t"},
+		Expression: &java.Identifier{ID: uuid.New(), Prefix: java.SingleSpace, Name: "err"},
+	}
+
+	guardBody := &java.Block{
+		ID:         uuid.New(),
+		Prefix:     java.SingleSpace,
+		Statements: []java.RightPadded[java.Statement]{{Element: ret}},
+		End:        java.Space{Whitespace: "\n" + base},
+	}
+
+	return &java.If{
+		ID:        uuid.New(),
+		Prefix:    java.Space{Whitespace: "\n" + base},
+		Condition: cond,
+		ThenPart:  java.RightPadded[java.Statement]{Element: guardBody},
+	}
+}

recipes/errorhandling/handle_swallowed_error.go

@@ -5,6 +5,7 @@
 package errorhandling
 
 import (
+	"github.com/moderneinc/recipes-go/recipes/internal/lstutil"
 	"github.com/openrewrite/rewrite/rewrite-go/pkg/recipe"
 	"github.com/openrewrite/rewrite/rewrite-go/pkg/tree/java"
 	"github.com/openrewrite/rewrite/rewrite-go/pkg/visitor"
@@ -36,7 +37,7 @@ type handleSwallowedErrorVisitor struct {
 func (v *handleSwallowedErrorVisitor) VisitIf(ifStmt *java.If, p any) java.J {
 	ifStmt = v.GoVisitor.VisitIf(ifStmt, p).(*java.If)
 
-	if ifStmt.Condition == nil || !isErrNotNil(ifStmt.Condition.Tree.Element) {
+	if ifStmt.Condition == nil || !lstutil.IsErrNotNil(ifStmt.Condition.Tree.Element) {
 		return ifStmt
 	}
 
@@ -82,20 +83,6 @@ func (v *handleSwallowedErrorVisitor) VisitIf(ifStmt *java.If, p any) java.J {
 	return ifStmt.WithThenPart(newThenPart)
 }
 
-// isErrNotNil checks whether an expression is `err != nil`.
-func isErrNotNil(expr java.Expression) bool {
-	bin, ok := expr.(*java.Binary)
-	if !ok || bin.Operator.Element != java.NotEqual {
-		return false
-	}
-	leftIdent, leftOk := bin.Left.(*java.Identifier)
-	rightIdent, rightOk := bin.Right.(*java.Identifier)
-	if !leftOk || !rightOk {
-		return false
-	}
-	return leftIdent.Name == "err" && rightIdent.Name == "nil"
-}
-
 // realStatements returns statements that are not *java.Empty.
 func realStatements(stmts []java.RightPadded[java.Statement]) []java.RightPadded[java.Statement] {
 	var out []java.RightPadded[java.Statement]