From 08865884b673ea7255f0afdf759ec24cdbfe327e Mon Sep 17 00:00:00 2001 From: Benjamin Muschko Date: Thu, 6 Aug 2026 11:34:48 -0600 Subject: [PATCH 1/5] Guard code-quality recipes against emitting invalid Go --- recipes/errorhandling/check_close_error.go | 18 +++ recipes/errorhandling/errors_is_common.go | 74 ++++++++++ .../handle_deferred_close_error.go | 6 + recipes/errorhandling/handle_error_return.go | 129 ++++++++++++++++-- .../errorhandling/handle_swallowed_error.go | 17 +-- recipes/errorhandling/prefer_errors_is.go | 13 ++ .../errorhandling/prefer_errors_is_context.go | 70 ++++------ recipes/errorhandling/prefer_errors_is_eof.go | 48 +++---- .../prefer_errors_is_for_field_access.go | 36 +---- .../errorhandling/prefer_errors_is_http.go | 49 ++++--- recipes/errorhandling/prefer_errors_is_net.go | 48 +++---- .../errorhandling/prefer_errors_is_os_path.go | 39 +++--- recipes/errorhandling/prefer_errors_is_sql.go | 48 +++---- recipes/errorhandling/prefer_errors_join.go | 41 ++++-- recipes/errorhandling/use_error_method.go | 5 + recipes/errorhandling/use_errors_as.go | 15 +- .../errorhandling/wrap_error_with_context.go | 6 + recipes/internal/lstutil/lstutil.go | 63 +++++++++ .../performance/prefer_strconv_format_bool.go | 54 ++++++-- recipes/performance/prefer_strconv_itoa.go | 8 +- .../use_strings_builder_in_loop.go | 6 + recipes/redundancy/if_init.go | 21 --- .../redundancy/remove_redundant_sprintf.go | 71 ++++++++-- .../redundancy/simplify_goroutine_closure.go | 16 +++ .../simplify_nil_check_before_close.go | 3 +- .../simplify_redundant_len_before_range.go | 3 +- recipes/simplification/if_init.go | 21 --- .../simplification/merge_collapsible_if.go | 3 +- .../prefer_empty_string_check.go | 62 ++++++--- .../simplification/prefer_io_writestring.go | 51 +++++-- recipes/simplification/prefer_os_readdir.go | 50 +++++-- recipes/simplification/prefer_strconv_atoi.go | 102 ++++++++++++-- .../prefer_strings_builder_writestring.go | 52 +++++-- .../prefer_strings_newreader.go | 59 ++++++-- .../simplification/prefer_strings_repeat.go | 54 ++++++-- recipes/simplification/type_context.go | 86 ++++++++++++ .../simplification/use_structured_logging.go | 83 ++++++----- recipes/style/check_template_execute_error.go | 9 +- recipes/style/prefer_hex_encoding.go | 60 ++++++-- recipes/style/prefer_raw_string_regex.go | 26 ++++ recipes/style/prefer_strconv_quote.go | 51 +++++-- recipes/style/reduce_error_check_nesting.go | 36 ++--- recipes/style/reduce_nesting_depth.go | 88 ++++++++++-- recipes/style/use_strong_hash.go | 31 +---- tests/errorhandling/check_close_error_test.go | 18 +++ .../handle_deferred_close_error_test.go | 18 +++ .../errorhandling/handle_error_return_test.go | 81 ++++++++++- .../prefer_errors_is_context_test.go | 16 +++ .../prefer_errors_is_eof_test.go | 16 +++ .../prefer_errors_is_for_field_access_test.go | 16 +++ .../prefer_errors_is_http_test.go | 16 +++ .../prefer_errors_is_net_test.go | 16 +++ .../prefer_errors_is_os_path_test.go | 16 +++ .../prefer_errors_is_sql_test.go | 16 +++ tests/errorhandling/prefer_errors_is_test.go | 16 +++ .../errorhandling/prefer_errors_join_test.go | 17 +++ tests/errorhandling/use_error_method_test.go | 16 +++ tests/errorhandling/use_errors_as_test.go | 20 +++ .../wrap_error_with_context_test.go | 20 +++ .../prefer_strconv_format_bool_test.go | 16 +++ .../use_strings_builder_in_loop_test.go | 18 +++ .../remove_redundant_sprintf_test.go | 16 +++ .../simplify_goroutine_closure_test.go | 19 +++ .../prefer_empty_string_check_test.go | 14 ++ .../prefer_io_writestring_test.go | 19 +++ .../simplification/prefer_os_readdir_test.go | 29 +++- .../prefer_strconv_atoi_test.go | 78 ++++++++++- ...prefer_strings_builder_writestring_test.go | 21 +++ .../prefer_strings_newreader_test.go | 123 ++++++++++++++++- .../prefer_strings_repeat_test.go | 16 +++ .../use_structured_logging_test.go | 25 ++++ .../check_template_execute_error_test.go | 21 +++ tests/style/prefer_hex_encoding_test.go | 16 +++ tests/style/prefer_raw_string_regex_test.go | 31 ++++- tests/style/prefer_strconv_quote_test.go | 16 +++ .../style/reduce_error_check_nesting_test.go | 31 ++++- tests/style/reduce_nesting_depth_test.go | 71 +++++++++- tests/style/use_strong_hash_test.go | 31 +---- 78 files changed, 2197 insertions(+), 563 deletions(-) create mode 100644 recipes/errorhandling/errors_is_common.go create mode 100644 recipes/internal/lstutil/lstutil.go delete mode 100644 recipes/redundancy/if_init.go delete mode 100644 recipes/simplification/if_init.go create mode 100644 recipes/simplification/type_context.go diff --git a/recipes/errorhandling/check_close_error.go b/recipes/errorhandling/check_close_error.go index f77ca06..30a145c 100644 --- a/recipes/errorhandling/check_close_error.go +++ b/recipes/errorhandling/check_close_error.go @@ -50,6 +50,18 @@ func (v *checkCloseErrorVisitor) VisitMultiAssignment(ma *golang.MultiAssignment 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 { mi = v.GoVisitor.VisitMethodInvocation(mi, p).(*java.MethodInvocation) @@ -64,6 +76,12 @@ func (v *checkCloseErrorVisitor) VisitMethodInvocation(mi *java.MethodInvocation return mi } + // `_ = x.Close()` only compiles when Close returns exactly one value; skip a + // void or multi-value Close. + if !returnsSingleValue(mi) { + return mi + } + // Wrap: f.Close() → _ = f.Close() // The leading whitespace lives on the outermost element, so carry the // invocation's prefix onto the new assignment. The space after `=` lives on diff --git a/recipes/errorhandling/errors_is_common.go b/recipes/errorhandling/errors_is_common.go new file mode 100644 index 0000000..eceee15 --- /dev/null +++ b/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 +} diff --git a/recipes/errorhandling/handle_deferred_close_error.go b/recipes/errorhandling/handle_deferred_close_error.go index 6733607..991c0d1 100644 --- a/recipes/errorhandling/handle_deferred_close_error.go +++ b/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 diff --git a/recipes/errorhandling/handle_error_return.go b/recipes/errorhandling/handle_error_return.go index 5be76d1..dcfcdcc 100644 --- a/recipes/errorhandling/handle_error_return.go +++ b/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}, + } +} diff --git a/recipes/errorhandling/handle_swallowed_error.go b/recipes/errorhandling/handle_swallowed_error.go index d36da90..47dc97e 100644 --- a/recipes/errorhandling/handle_swallowed_error.go +++ b/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] diff --git a/recipes/errorhandling/prefer_errors_is.go b/recipes/errorhandling/prefer_errors_is.go index 5999710..f4d1df2 100644 --- a/recipes/errorhandling/prefer_errors_is.go +++ b/recipes/errorhandling/prefer_errors_is.go @@ -5,6 +5,7 @@ package errorhandling import ( + "github.com/openrewrite/rewrite/rewrite-go/pkg/matcher" "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe" recipegolang "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe/golang" "github.com/openrewrite/rewrite/rewrite-go/pkg/tree/java" @@ -68,6 +69,12 @@ func (v *preferErrorsIsVisitor) VisitBinary(bin *java.Binary, p any) java.J { return bin } + // errors.Is requires both operands to be error values; skip a comparison that + // only matched by the Err* name, such as an int constant named ErrLevel. + if !isErrorAssignable(errExpr) || !isErrorAssignable(sentinel) { + return bin + } + // The rewrite introduces a reference to the `errors` package; ensure it is imported. recipegolang.MaybeAddImport(v, "errors", nil, false) @@ -105,6 +112,12 @@ func (v *preferErrorsIsVisitor) VisitBinary(bin *java.Binary, p any) java.J { return isCall.WithPrefix(prefix) } +// Reports whether expr is a value assignable to error, which errors.Is requires +// of both of its arguments and err.Error() requires of its receiver. +func isErrorAssignable(expr java.Expression) bool { + return matcher.IsAssignableTo(matcher.TypeOfExpression(expr), "error") +} + func isErrorSentinel(expr java.Expression) bool { switch n := expr.(type) { case *java.Identifier: diff --git a/recipes/errorhandling/prefer_errors_is_context.go b/recipes/errorhandling/prefer_errors_is_context.go index 1f75522..74474d6 100644 --- a/recipes/errorhandling/prefer_errors_is_context.go +++ b/recipes/errorhandling/prefer_errors_is_context.go @@ -5,50 +5,13 @@ package errorhandling import ( - "fmt" - "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe" - "github.com/openrewrite/rewrite/rewrite-go/pkg/template" -) - -var ctxErr = template.Expr("ctxErr") - -var preferErrorsIsContextCanceledEqualImpl = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.PreferErrorsIsContext$CanceledEqual"), - template.WithDisplayName("err == context.Canceled -> errors.Is(err, context.Canceled)"), - template.WithBefore(fmt.Sprintf(`%s == context.Canceled`, ctxErr), template.Imports("context")), - template.WithAfter(fmt.Sprintf(`errors.Is(%s, context.Canceled)`, ctxErr), template.Imports("errors", "context"), template.SourceImports("errors", "context")), - template.WithCaptures(ctxErr), -) - -var preferErrorsIsContextCanceledNotEqualImpl = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.PreferErrorsIsContext$CanceledNotEqual"), - template.WithDisplayName("err != context.Canceled -> !errors.Is(err, context.Canceled)"), - template.WithBefore(fmt.Sprintf(`%s != context.Canceled`, ctxErr), template.Imports("context")), - template.WithAfter(fmt.Sprintf(`!errors.Is(%s, context.Canceled)`, ctxErr), template.Imports("errors", "context"), template.SourceImports("errors", "context")), - template.WithCaptures(ctxErr), -) - -var preferErrorsIsContextDeadlineEqualImpl = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.PreferErrorsIsContext$DeadlineEqual"), - template.WithDisplayName("err == context.DeadlineExceeded -> errors.Is(err, context.DeadlineExceeded)"), - template.WithBefore(fmt.Sprintf(`%s == context.DeadlineExceeded`, ctxErr), template.Imports("context")), - template.WithAfter(fmt.Sprintf(`errors.Is(%s, context.DeadlineExceeded)`, ctxErr), template.Imports("errors", "context"), template.SourceImports("errors", "context")), - template.WithCaptures(ctxErr), + "github.com/openrewrite/rewrite/rewrite-go/pkg/tree/java" + "github.com/openrewrite/rewrite/rewrite-go/pkg/visitor" ) -var preferErrorsIsContextDeadlineNotEqualImpl = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.PreferErrorsIsContext$DeadlineNotEqual"), - template.WithDisplayName("err != context.DeadlineExceeded -> !errors.Is(err, context.DeadlineExceeded)"), - template.WithBefore(fmt.Sprintf(`%s != context.DeadlineExceeded`, ctxErr), template.Imports("context")), - template.WithAfter(fmt.Sprintf(`!errors.Is(%s, context.DeadlineExceeded)`, ctxErr), template.Imports("errors", "context"), template.SourceImports("errors", "context")), - template.WithCaptures(ctxErr), -) - -// PreferErrorsIsContext replaces `err == context.Canceled` with -// `errors.Is(err, context.Canceled)` and `err == context.DeadlineExceeded` with -// `errors.Is(err, context.DeadlineExceeded)`, plus their != variants. -// Using errors.Is handles wrapped errors correctly. +// Replaces `err == context.Canceled` and `err == context.DeadlineExceeded` with +// `errors.Is` (and the negated forms) for correct wrapped error handling. type PreferErrorsIsContext struct { recipe.Base } @@ -64,11 +27,24 @@ func (r *PreferErrorsIsContext) Description() string { } func (r *PreferErrorsIsContext) Tags() []string { return []string{"error-handling"} } -func (r *PreferErrorsIsContext) RecipeList() []recipe.Recipe { - return []recipe.Recipe{ - preferErrorsIsContextCanceledEqualImpl, - preferErrorsIsContextCanceledNotEqualImpl, - preferErrorsIsContextDeadlineEqualImpl, - preferErrorsIsContextDeadlineNotEqualImpl, +func (r *PreferErrorsIsContext) Editor() recipe.TreeVisitor { + return visitor.Init(&preferErrorsIsContextVisitor{}) +} + +type preferErrorsIsContextVisitor struct { + visitor.GoVisitor +} + +func (v *preferErrorsIsContextVisitor) VisitBinary(bin *java.Binary, p any) java.J { + bin = v.GoVisitor.VisitBinary(bin, p).(*java.Binary) + + if bin.Operator.Element != java.Equal && bin.Operator.Element != java.NotEqual { + return bin + } + for _, name := range []string{"Canceled", "DeadlineExceeded"} { + if errExpr, sentinel, ok := matchSentinel(bin, "context", name); ok { + return rewriteToErrorsIs(v, bin, errExpr, sentinel) + } } + return bin } diff --git a/recipes/errorhandling/prefer_errors_is_eof.go b/recipes/errorhandling/prefer_errors_is_eof.go index 38038ec..d3e5387 100644 --- a/recipes/errorhandling/prefer_errors_is_eof.go +++ b/recipes/errorhandling/prefer_errors_is_eof.go @@ -5,33 +5,13 @@ package errorhandling import ( - "fmt" - "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe" - "github.com/openrewrite/rewrite/rewrite-go/pkg/template" -) - -var eofErr = template.Expr("eofErr") - -var preferErrorsIsEOFEqualImpl = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.PreferErrorsIsEOF$Equal"), - template.WithDisplayName("err == io.EOF -> errors.Is(err, io.EOF)"), - template.WithBefore(fmt.Sprintf(`%s == io.EOF`, eofErr), template.Imports("io")), - template.WithAfter(fmt.Sprintf(`errors.Is(%s, io.EOF)`, eofErr), template.Imports("errors", "io"), template.SourceImports("errors", "io")), - template.WithCaptures(eofErr), -) - -var preferErrorsIsEOFNotEqualImpl = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.PreferErrorsIsEOF$NotEqual"), - template.WithDisplayName("err != io.EOF -> !errors.Is(err, io.EOF)"), - template.WithBefore(fmt.Sprintf(`%s != io.EOF`, eofErr), template.Imports("io")), - template.WithAfter(fmt.Sprintf(`!errors.Is(%s, io.EOF)`, eofErr), template.Imports("errors", "io"), template.SourceImports("errors", "io")), - template.WithCaptures(eofErr), + "github.com/openrewrite/rewrite/rewrite-go/pkg/tree/java" + "github.com/openrewrite/rewrite/rewrite-go/pkg/visitor" ) -// PreferErrorsIsEOF replaces `err == io.EOF` with `errors.Is(err, io.EOF)` and -// `err != io.EOF` with `!errors.Is(err, io.EOF)`. The io.EOF sentinel is the -// most common error value compared by ==; using errors.Is handles wrapped errors. +// Replaces `err == io.EOF` with `errors.Is(err, io.EOF)` (and the negated form) +// for correct wrapped error handling. type PreferErrorsIsEOF struct { recipe.Base } @@ -47,6 +27,22 @@ func (r *PreferErrorsIsEOF) Description() string { } func (r *PreferErrorsIsEOF) Tags() []string { return []string{"error-handling"} } -func (r *PreferErrorsIsEOF) RecipeList() []recipe.Recipe { - return []recipe.Recipe{preferErrorsIsEOFEqualImpl, preferErrorsIsEOFNotEqualImpl} +func (r *PreferErrorsIsEOF) Editor() recipe.TreeVisitor { + return visitor.Init(&preferErrorsIsEOFVisitor{}) +} + +type preferErrorsIsEOFVisitor struct { + visitor.GoVisitor +} + +func (v *preferErrorsIsEOFVisitor) VisitBinary(bin *java.Binary, p any) java.J { + bin = v.GoVisitor.VisitBinary(bin, p).(*java.Binary) + + if bin.Operator.Element != java.Equal && bin.Operator.Element != java.NotEqual { + return bin + } + if errExpr, sentinel, ok := matchSentinel(bin, "io", "EOF"); ok { + return rewriteToErrorsIs(v, bin, errExpr, sentinel) + } + return bin } diff --git a/recipes/errorhandling/prefer_errors_is_for_field_access.go b/recipes/errorhandling/prefer_errors_is_for_field_access.go index ec57d28..2522d98 100644 --- a/recipes/errorhandling/prefer_errors_is_for_field_access.go +++ b/recipes/errorhandling/prefer_errors_is_for_field_access.go @@ -6,7 +6,6 @@ package errorhandling import ( "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe" - 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" ) @@ -66,40 +65,7 @@ func (v *preferErrorsIsForFieldAccessVisitor) VisitBinary(bin *java.Binary, p an return bin } - // The rewrite introduces a reference to the `errors` package; ensure it is imported. - recipegolang.MaybeAddImport(v, "errors", nil, false) - - // Build errors.Is(errExpr, sentinel) or !errors.Is(errExpr, sentinel). The - // leading whitespace lives on the outermost element, so carry the binary's - // prefix onto whichever node ends up outermost. - prefix := getLeadingPrefixExpr(bin) - - errorsIdent := &java.Identifier{Name: "errors"} - isIdent := &java.Identifier{Name: "Is"} - - errArg := stripExprPrefix(errExpr) - sentinelArg := stripExprPrefix(sentinel) - sentinelArgWithSpace := setExprPrefixLocal(sentinelArg, java.Space{Whitespace: " "}) - - isCall := &java.MethodInvocation{ - Select: &java.RightPadded[java.Expression]{Element: errorsIdent}, - Name: isIdent, - Arguments: java.Container[java.Expression]{ - Elements: []java.RightPadded[java.Expression]{ - {Element: errArg}, - {Element: sentinelArgWithSpace}, - }, - }, - } - - 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) + return rewriteToErrorsIs(v, bin, errExpr, sentinel) } // isPackageQualifiedSentinel checks if the expression is a FieldAccess diff --git a/recipes/errorhandling/prefer_errors_is_http.go b/recipes/errorhandling/prefer_errors_is_http.go index efdc11f..c881019 100644 --- a/recipes/errorhandling/prefer_errors_is_http.go +++ b/recipes/errorhandling/prefer_errors_is_http.go @@ -5,33 +5,14 @@ package errorhandling import ( - "fmt" - "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe" - "github.com/openrewrite/rewrite/rewrite-go/pkg/template" -) - -var htErr = template.Expr("htErr") - -var preferErrorsIsHttpServerClosedEqualImpl = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.PreferErrorsIsHttpServerClosed$Equal"), - template.WithDisplayName("err == http.ErrServerClosed -> errors.Is(err, http.ErrServerClosed)"), - template.WithBefore(fmt.Sprintf(`%s == http.ErrServerClosed`, htErr), template.Imports("net/http")), - template.WithAfter(fmt.Sprintf(`errors.Is(%s, http.ErrServerClosed)`, htErr), template.Imports("errors", "net/http"), template.SourceImports("errors", "net/http")), - template.WithCaptures(htErr), -) - -var preferErrorsIsHttpServerClosedNotEqualImpl = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.PreferErrorsIsHttpServerClosed$NotEqual"), - template.WithDisplayName("err != http.ErrServerClosed -> !errors.Is(err, http.ErrServerClosed)"), - template.WithBefore(fmt.Sprintf(`%s != http.ErrServerClosed`, htErr), template.Imports("net/http")), - template.WithAfter(fmt.Sprintf(`!errors.Is(%s, http.ErrServerClosed)`, htErr), template.Imports("errors", "net/http"), template.SourceImports("errors", "net/http")), - template.WithCaptures(htErr), + "github.com/openrewrite/rewrite/rewrite-go/pkg/tree/java" + "github.com/openrewrite/rewrite/rewrite-go/pkg/visitor" ) -// PreferErrorsIsHttpServerClosed replaces `err == http.ErrServerClosed` with -// `errors.Is(err, http.ErrServerClosed)` and `err != http.ErrServerClosed` with -// `!errors.Is(err, http.ErrServerClosed)`. Using errors.Is handles wrapped errors. +// Replaces `err == http.ErrServerClosed` with +// `errors.Is(err, http.ErrServerClosed)` (and the negated form) for correct +// wrapped error handling. type PreferErrorsIsHttpServerClosed struct { recipe.Base } @@ -47,6 +28,22 @@ func (r *PreferErrorsIsHttpServerClosed) Description() string { } func (r *PreferErrorsIsHttpServerClosed) Tags() []string { return []string{"error-handling"} } -func (r *PreferErrorsIsHttpServerClosed) RecipeList() []recipe.Recipe { - return []recipe.Recipe{preferErrorsIsHttpServerClosedEqualImpl, preferErrorsIsHttpServerClosedNotEqualImpl} +func (r *PreferErrorsIsHttpServerClosed) Editor() recipe.TreeVisitor { + return visitor.Init(&preferErrorsIsHttpVisitor{}) +} + +type preferErrorsIsHttpVisitor struct { + visitor.GoVisitor +} + +func (v *preferErrorsIsHttpVisitor) VisitBinary(bin *java.Binary, p any) java.J { + bin = v.GoVisitor.VisitBinary(bin, p).(*java.Binary) + + if bin.Operator.Element != java.Equal && bin.Operator.Element != java.NotEqual { + return bin + } + if errExpr, sentinel, ok := matchSentinel(bin, "http", "ErrServerClosed"); ok { + return rewriteToErrorsIs(v, bin, errExpr, sentinel) + } + return bin } diff --git a/recipes/errorhandling/prefer_errors_is_net.go b/recipes/errorhandling/prefer_errors_is_net.go index 929ad71..f86e836 100644 --- a/recipes/errorhandling/prefer_errors_is_net.go +++ b/recipes/errorhandling/prefer_errors_is_net.go @@ -5,33 +5,13 @@ package errorhandling import ( - "fmt" - "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe" - "github.com/openrewrite/rewrite/rewrite-go/pkg/template" -) - -var netErr = template.Expr("netErr") - -var preferErrorsIsNetClosedEqualImpl = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.PreferErrorsIsNetClosed$Equal"), - template.WithDisplayName("err == net.ErrClosed -> errors.Is(err, net.ErrClosed)"), - template.WithBefore(fmt.Sprintf(`%s == net.ErrClosed`, netErr), template.Imports("net")), - template.WithAfter(fmt.Sprintf(`errors.Is(%s, net.ErrClosed)`, netErr), template.Imports("errors", "net"), template.SourceImports("errors", "net")), - template.WithCaptures(netErr), -) - -var preferErrorsIsNetClosedNotEqualImpl = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.PreferErrorsIsNetClosed$NotEqual"), - template.WithDisplayName("err != net.ErrClosed -> !errors.Is(err, net.ErrClosed)"), - template.WithBefore(fmt.Sprintf(`%s != net.ErrClosed`, netErr), template.Imports("net")), - template.WithAfter(fmt.Sprintf(`!errors.Is(%s, net.ErrClosed)`, netErr), template.Imports("errors", "net"), template.SourceImports("errors", "net")), - template.WithCaptures(netErr), + "github.com/openrewrite/rewrite/rewrite-go/pkg/tree/java" + "github.com/openrewrite/rewrite/rewrite-go/pkg/visitor" ) -// PreferErrorsIsNetClosed replaces `err == net.ErrClosed` with -// `errors.Is(err, net.ErrClosed)` and `err != net.ErrClosed` with -// `!errors.Is(err, net.ErrClosed)`. Using errors.Is handles wrapped errors. +// Replaces `err == net.ErrClosed` with `errors.Is(err, net.ErrClosed)` (and the +// negated form) for correct wrapped error handling. type PreferErrorsIsNetClosed struct { recipe.Base } @@ -47,6 +27,22 @@ func (r *PreferErrorsIsNetClosed) Description() string { } func (r *PreferErrorsIsNetClosed) Tags() []string { return []string{"error-handling"} } -func (r *PreferErrorsIsNetClosed) RecipeList() []recipe.Recipe { - return []recipe.Recipe{preferErrorsIsNetClosedEqualImpl, preferErrorsIsNetClosedNotEqualImpl} +func (r *PreferErrorsIsNetClosed) Editor() recipe.TreeVisitor { + return visitor.Init(&preferErrorsIsNetVisitor{}) +} + +type preferErrorsIsNetVisitor struct { + visitor.GoVisitor +} + +func (v *preferErrorsIsNetVisitor) VisitBinary(bin *java.Binary, p any) java.J { + bin = v.GoVisitor.VisitBinary(bin, p).(*java.Binary) + + if bin.Operator.Element != java.Equal && bin.Operator.Element != java.NotEqual { + return bin + } + if errExpr, sentinel, ok := matchSentinel(bin, "net", "ErrClosed"); ok { + return rewriteToErrorsIs(v, bin, errExpr, sentinel) + } + return bin } diff --git a/recipes/errorhandling/prefer_errors_is_os_path.go b/recipes/errorhandling/prefer_errors_is_os_path.go index c8e9e72..54354a0 100644 --- a/recipes/errorhandling/prefer_errors_is_os_path.go +++ b/recipes/errorhandling/prefer_errors_is_os_path.go @@ -5,24 +5,13 @@ package errorhandling import ( - "fmt" - "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe" - "github.com/openrewrite/rewrite/rewrite-go/pkg/template" + "github.com/openrewrite/rewrite/rewrite-go/pkg/tree/java" + "github.com/openrewrite/rewrite/rewrite-go/pkg/visitor" ) -var opiErr = template.Expr("opiErr") - -var preferErrorsIsOsInvalidEqualImpl = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.PreferErrorsIsOsInvalid$Equal"), - template.WithDisplayName("err == os.ErrInvalid -> errors.Is(err, os.ErrInvalid)"), - template.WithBefore(fmt.Sprintf(`%s == os.ErrInvalid`, opiErr), template.Imports("os")), - template.WithAfter(fmt.Sprintf(`errors.Is(%s, os.ErrInvalid)`, opiErr), template.Imports("errors", "os"), template.SourceImports("errors", "os")), - template.WithCaptures(opiErr), -) - -// PreferErrorsIsOsInvalid replaces `err == os.ErrInvalid` with -// `errors.Is(err, os.ErrInvalid)`. Using errors.Is handles wrapped errors. +// Replaces `err == os.ErrInvalid` with `errors.Is(err, os.ErrInvalid)` for +// correct wrapped error handling. type PreferErrorsIsOsInvalid struct { recipe.Base } @@ -38,6 +27,22 @@ func (r *PreferErrorsIsOsInvalid) Description() string { } func (r *PreferErrorsIsOsInvalid) Tags() []string { return []string{"error-handling"} } -func (r *PreferErrorsIsOsInvalid) RecipeList() []recipe.Recipe { - return []recipe.Recipe{preferErrorsIsOsInvalidEqualImpl} +func (r *PreferErrorsIsOsInvalid) Editor() recipe.TreeVisitor { + return visitor.Init(&preferErrorsIsOsVisitor{}) +} + +type preferErrorsIsOsVisitor struct { + visitor.GoVisitor +} + +func (v *preferErrorsIsOsVisitor) VisitBinary(bin *java.Binary, p any) java.J { + bin = v.GoVisitor.VisitBinary(bin, p).(*java.Binary) + + if bin.Operator.Element != java.Equal && bin.Operator.Element != java.NotEqual { + return bin + } + if errExpr, sentinel, ok := matchSentinel(bin, "os", "ErrInvalid"); ok { + return rewriteToErrorsIs(v, bin, errExpr, sentinel) + } + return bin } diff --git a/recipes/errorhandling/prefer_errors_is_sql.go b/recipes/errorhandling/prefer_errors_is_sql.go index 6a33937..38fd53d 100644 --- a/recipes/errorhandling/prefer_errors_is_sql.go +++ b/recipes/errorhandling/prefer_errors_is_sql.go @@ -5,33 +5,13 @@ package errorhandling import ( - "fmt" - "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe" - "github.com/openrewrite/rewrite/rewrite-go/pkg/template" -) - -var sqlErr = template.Expr("sqlErr") - -var preferErrorsIsSqlNoRowsEqualImpl = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.PreferErrorsIsSqlNoRows$Equal"), - template.WithDisplayName("err == sql.ErrNoRows -> errors.Is(err, sql.ErrNoRows)"), - template.WithBefore(fmt.Sprintf(`%s == sql.ErrNoRows`, sqlErr), template.Imports("database/sql")), - template.WithAfter(fmt.Sprintf(`errors.Is(%s, sql.ErrNoRows)`, sqlErr), template.Imports("errors", "database/sql"), template.SourceImports("errors", "database/sql")), - template.WithCaptures(sqlErr), -) - -var preferErrorsIsSqlNoRowsNotEqualImpl = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.PreferErrorsIsSqlNoRows$NotEqual"), - template.WithDisplayName("err != sql.ErrNoRows -> !errors.Is(err, sql.ErrNoRows)"), - template.WithBefore(fmt.Sprintf(`%s != sql.ErrNoRows`, sqlErr), template.Imports("database/sql")), - template.WithAfter(fmt.Sprintf(`!errors.Is(%s, sql.ErrNoRows)`, sqlErr), template.Imports("errors", "database/sql"), template.SourceImports("errors", "database/sql")), - template.WithCaptures(sqlErr), + "github.com/openrewrite/rewrite/rewrite-go/pkg/tree/java" + "github.com/openrewrite/rewrite/rewrite-go/pkg/visitor" ) -// PreferErrorsIsSqlNoRows replaces `err == sql.ErrNoRows` with `errors.Is(err, sql.ErrNoRows)` and -// `err != sql.ErrNoRows` with `!errors.Is(err, sql.ErrNoRows)`. The sql.ErrNoRows sentinel is a -// common error value compared by ==; using errors.Is handles wrapped errors. +// Replaces `err == sql.ErrNoRows` with `errors.Is(err, sql.ErrNoRows)` (and the +// negated form) for correct wrapped error handling. type PreferErrorsIsSqlNoRows struct { recipe.Base } @@ -47,6 +27,22 @@ func (r *PreferErrorsIsSqlNoRows) Description() string { } func (r *PreferErrorsIsSqlNoRows) Tags() []string { return []string{"error-handling"} } -func (r *PreferErrorsIsSqlNoRows) RecipeList() []recipe.Recipe { - return []recipe.Recipe{preferErrorsIsSqlNoRowsEqualImpl, preferErrorsIsSqlNoRowsNotEqualImpl} +func (r *PreferErrorsIsSqlNoRows) Editor() recipe.TreeVisitor { + return visitor.Init(&preferErrorsIsSqlVisitor{}) +} + +type preferErrorsIsSqlVisitor struct { + visitor.GoVisitor +} + +func (v *preferErrorsIsSqlVisitor) VisitBinary(bin *java.Binary, p any) java.J { + bin = v.GoVisitor.VisitBinary(bin, p).(*java.Binary) + + if bin.Operator.Element != java.Equal && bin.Operator.Element != java.NotEqual { + return bin + } + if errExpr, sentinel, ok := matchSentinel(bin, "sql", "ErrNoRows"); ok { + return rewriteToErrorsIs(v, bin, errExpr, sentinel) + } + return bin } diff --git a/recipes/errorhandling/prefer_errors_join.go b/recipes/errorhandling/prefer_errors_join.go index 1c72808..5de31a9 100644 --- a/recipes/errorhandling/prefer_errors_join.go +++ b/recipes/errorhandling/prefer_errors_join.go @@ -10,20 +10,16 @@ import ( "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe" recipegolang "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe/golang" "github.com/openrewrite/rewrite/rewrite-go/pkg/template" + "github.com/openrewrite/rewrite/rewrite-go/pkg/tree/java" + "github.com/openrewrite/rewrite/rewrite-go/pkg/visitor" ) var ejErr = template.Expr("ejErr") -var simplifyRedundantErrorWrapImpl = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.SimplifyRedundantErrorWrap$Impl"), - template.WithDisplayName("Simplify redundant error wrap"), - template.WithBefore(fmt.Sprintf(`fmt.Errorf("%%w", %s)`, ejErr), template.Imports("fmt")), - template.WithAfter(fmt.Sprintf(`%s`, ejErr)), - template.WithCaptures(ejErr), -) +var errorWrapPattern = template.Expression(fmt.Sprintf(`fmt.Errorf("%%w", %s)`, ejErr)). + Captures(ejErr).Imports("fmt").Build() -// SimplifyRedundantErrorWrap replaces `fmt.Errorf("%w", err)` with just `err`. -// Wrapping an error with no additional context is redundant. +// Replaces `fmt.Errorf("%w", err)` with `err` when wrapping adds no context. type SimplifyRedundantErrorWrap struct { recipe.Base } @@ -37,6 +33,29 @@ func (r *SimplifyRedundantErrorWrap) Description() string { } func (r *SimplifyRedundantErrorWrap) Tags() []string { return []string{"error-handling"} } -func (r *SimplifyRedundantErrorWrap) RecipeList() []recipe.Recipe { - return []recipe.Recipe{simplifyRedundantErrorWrapImpl, &recipegolang.RemoveUnusedImports{}} +func (r *SimplifyRedundantErrorWrap) Editor() recipe.TreeVisitor { + return visitor.Init(&simplifyRedundantErrorWrapVisitor{}) +} + +type simplifyRedundantErrorWrapVisitor struct { + visitor.GoVisitor +} + +func (v *simplifyRedundantErrorWrapVisitor) VisitMethodInvocation(mi *java.MethodInvocation, p any) java.J { + mi = v.GoVisitor.VisitMethodInvocation(mi, p).(*java.MethodInvocation) + + match := errorWrapPattern.Match(mi, nil) + if match == nil { + return mi + } + + // Skip unless the wrapped value is an error, since replacing fmt.Errorf (which + // returns error) with the bare value would otherwise not compile. + arg, ok := match.GetCapture(ejErr).(java.Expression) + if !ok || !isErrorAssignable(arg) { + return mi + } + + v.DoAfterVisit(recipe.Service[*recipegolang.ImportService](nil).RemoveUnusedImportsVisitor()) + return setExprPrefixLocal(stripExprPrefix(arg), mi.GetPrefix()) } diff --git a/recipes/errorhandling/use_error_method.go b/recipes/errorhandling/use_error_method.go index 9eb083e..f3a2259 100644 --- a/recipes/errorhandling/use_error_method.go +++ b/recipes/errorhandling/use_error_method.go @@ -72,6 +72,11 @@ func (v *useErrorMethodVisitor) VisitMethodInvocation(mi *java.MethodInvocation, return mi } + // err.Error() only compiles when the value implements error. + if !isErrorAssignable(argIdent) { + return mi + } + // Build err.Error() as a replacement, preserving the original leading prefix. v.changed = true errIdent := argIdent.WithPrefix(ident.Prefix) diff --git a/recipes/errorhandling/use_errors_as.go b/recipes/errorhandling/use_errors_as.go index ca98295..ae14b92 100644 --- a/recipes/errorhandling/use_errors_as.go +++ b/recipes/errorhandling/use_errors_as.go @@ -6,6 +6,7 @@ package errorhandling import ( "github.com/google/uuid" + "github.com/openrewrite/rewrite/rewrite-go/pkg/matcher" "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe" recipegolang "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe/golang" "github.com/openrewrite/rewrite/rewrite-go/pkg/tree/golang" @@ -158,21 +159,19 @@ func matchCommaOkTypeAssert(swi *golang.StatementWithInit) (string, java.Express return targetIdent.Name, typeExpr, tc.Expr } -// looksLikeError returns true if the expression is likely an error value. -// It checks type information first; if unavailable, falls back to the -// common convention that error variables are named "err". +// Reports whether the expression is assignable to error, deciding from the +// resolved type when present and otherwise from the "err" naming convention. func looksLikeError(expr java.Expression) bool { ident, ok := expr.(*java.Identifier) if !ok { return false } - // Check type info if available. + // A resolved type is decisive: an `any`/`interface{}` value is not assignable + // to error, so errors.As would not compile. if ident.Type != nil { - if fq, ok := ident.Type.(java.FullyQualified); ok { - return fq.GetFullyQualifiedName() == "error" - } + return matcher.IsAssignableTo(ident.Type, "error") } - // Fall back to name convention. + // Fall back to name convention only when the type is unresolved. return ident.Name == "err" } diff --git a/recipes/errorhandling/wrap_error_with_context.go b/recipes/errorhandling/wrap_error_with_context.go index 7d18e34..d6eb6dd 100644 --- a/recipes/errorhandling/wrap_error_with_context.go +++ b/recipes/errorhandling/wrap_error_with_context.go @@ -64,6 +64,12 @@ func (v *wrapErrorWithContextVisitor) VisitReturn(ret *java.Return, p any) java. return ret } + // fmt.Errorf returns error, so only wrap when the function returns a single + // error result; a concrete error type such as *MyErr would not accept it. + if !enclosingReturnsSingleError(v.Cursor()) { + return ret + } + // The rewrite introduces a reference to the `fmt` package; ensure it is imported. recipegolang.MaybeAddImport(v, "fmt", nil, false) diff --git a/recipes/internal/lstutil/lstutil.go b/recipes/internal/lstutil/lstutil.go new file mode 100644 index 0000000..ce636e8 --- /dev/null +++ b/recipes/internal/lstutil/lstutil.go @@ -0,0 +1,63 @@ +/* + * Moderne Proprietary. Only for use by Moderne customers under the terms of a commercial contract. + */ + +// Package lstutil holds small helpers shared across recipe packages for +// working with the LST and the visitor cursor. +package lstutil + +import ( + "strings" + + "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" +) + +// Reports whether the block at the cursor is a function's body (its parent is a +// function declaration) rather than a nested block such as a loop or if body. +func IsFunctionBodyBlock(c *visitor.Cursor) bool { + parent := c.Parent() + if parent == nil { + return false + } + switch parent.Value().(type) { + case *java.MethodDeclaration, *golang.MethodDeclaration: + return true + } + return false +} + +// Returns the indentation (text after the last newline) of a Space. +func BaseIndent(space java.Space) string { + ws := space.Whitespace + if idx := strings.LastIndex(ws, "\n"); idx >= 0 { + return ws[idx+1:] + } + return ws +} + +// Reports whether the If at the cursor is the inner statement of a +// golang.StatementWithInit, i.e. it carried an `if init; cond` init clause. +func IsInitWrappedIf(c *visitor.Cursor) bool { + parent := c.Parent() + if parent == nil { + return false + } + _, ok := parent.Value().(*golang.StatementWithInit) + return ok +} + +// Reports 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" +} diff --git a/recipes/performance/prefer_strconv_format_bool.go b/recipes/performance/prefer_strconv_format_bool.go index edd6297..5deedff 100644 --- a/recipes/performance/prefer_strconv_format_bool.go +++ b/recipes/performance/prefer_strconv_format_bool.go @@ -7,24 +7,25 @@ package performance import ( "fmt" + "github.com/openrewrite/rewrite/rewrite-go/pkg/matcher" "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe" + recipegolang "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe/golang" "github.com/openrewrite/rewrite/rewrite-go/pkg/template" + "github.com/openrewrite/rewrite/rewrite-go/pkg/tree/java" + "github.com/openrewrite/rewrite/rewrite-go/pkg/visitor" ) +var fbB = template.Expr("fbB") + var ( - fbB = template.Expr("fbB") - - preferStrconvFormatBoolImpl = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.PreferStrconvFormatBool$Impl"), - template.WithDisplayName("Prefer strconv.FormatBool over fmt.Sprintf"), - template.WithBefore(fmt.Sprintf(`fmt.Sprintf("%%t", %s)`, fbB), template.Imports("fmt")), - template.WithAfter(fmt.Sprintf(`strconv.FormatBool(%s)`, fbB), template.Imports("strconv"), template.SourceImports("strconv")), - template.WithCaptures(fbB), - ) + formatBoolPattern = template.Expression(fmt.Sprintf(`fmt.Sprintf("%%t", %s)`, fbB)). + Captures(fbB).Imports("fmt").Build() + formatBoolTemplate = template.ExpressionTemplate(fmt.Sprintf(`strconv.FormatBool(%s)`, fbB)). + Captures(fbB).Imports("strconv").Build() ) -// PreferStrconvFormatBool replaces `fmt.Sprintf("%t", b)` with -// `strconv.FormatBool(b)` for better performance on bool-to-string conversion. +// Replaces `fmt.Sprintf("%t", b)` with `strconv.FormatBool(b)` for better +// performance on bool-to-string conversion. type PreferStrconvFormatBool struct { recipe.Base } @@ -40,6 +41,33 @@ func (r *PreferStrconvFormatBool) Description() string { } func (r *PreferStrconvFormatBool) Tags() []string { return []string{"performance"} } -func (r *PreferStrconvFormatBool) RecipeList() []recipe.Recipe { - return []recipe.Recipe{preferStrconvFormatBoolImpl} +func (r *PreferStrconvFormatBool) Editor() recipe.TreeVisitor { + return visitor.Init(&preferStrconvFormatBoolVisitor{}) +} + +type preferStrconvFormatBoolVisitor struct { + visitor.GoVisitor +} + +func (v *preferStrconvFormatBoolVisitor) VisitMethodInvocation(mi *java.MethodInvocation, p any) java.J { + mi = v.GoVisitor.VisitMethodInvocation(mi, p).(*java.MethodInvocation) + + match := formatBoolPattern.Match(mi, nil) + if match == nil { + return mi + } + + // Skip unless the argument is a bool, since strconv.FormatBool takes a bool. + arg, ok := match.GetCapture(fbB).(java.Expression) + if !ok || !matcher.IsBool(matcher.TypeOfExpression(arg)) { + return mi + } + + replaced, ok := formatBoolTemplate.Apply(nil, match).(*java.MethodInvocation) + if !ok { + return mi + } + recipegolang.MaybeAddImport(v, "strconv", nil, false) + v.DoAfterVisit(recipe.Service[*recipegolang.ImportService](nil).RemoveUnusedImportsVisitor()) + return replaced.WithPrefix(mi.GetPrefix()) } diff --git a/recipes/performance/prefer_strconv_itoa.go b/recipes/performance/prefer_strconv_itoa.go index 695715c..8de7d01 100644 --- a/recipes/performance/prefer_strconv_itoa.go +++ b/recipes/performance/prefer_strconv_itoa.go @@ -23,8 +23,12 @@ var ( ) ) -// PreferStrconvItoa replaces `fmt.Sprintf("%d", n)` with `strconv.Itoa(n)` -// for better performance on int-to-string conversion. +// Replaces `fmt.Sprintf("%d", n)` with `strconv.Itoa(n)` for better performance +// on int-to-string conversion. +// +// It rewrites int64/uint arguments into non-compiling code because the LST +// resolves all integer widths to a single type, so it cannot restrict itself to +// int. type PreferStrconvItoa struct { recipe.Base } diff --git a/recipes/performance/use_strings_builder_in_loop.go b/recipes/performance/use_strings_builder_in_loop.go index fff4105..f8ae874 100644 --- a/recipes/performance/use_strings_builder_in_loop.go +++ b/recipes/performance/use_strings_builder_in_loop.go @@ -6,6 +6,7 @@ package performance import ( "github.com/google/uuid" + "github.com/openrewrite/rewrite/rewrite-go/pkg/matcher" "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" @@ -167,6 +168,11 @@ func findStringConcats(body *java.Block) []stringConcatInfo { if ao.Operator.Element != java.AddAssign { continue } + // Only string accumulators concatenate; `+=` on a numeric variable would + // become builder.WriteString(number), which does not compile. + if !matcher.IsString(matcher.TypeOfExpression(ao.Variable)) { + continue + } results = append(results, stringConcatInfo{ stmtIdx: i, variable: ao.Variable, diff --git a/recipes/redundancy/if_init.go b/recipes/redundancy/if_init.go deleted file mode 100644 index d5bcd7a..0000000 --- a/recipes/redundancy/if_init.go +++ /dev/null @@ -1,21 +0,0 @@ -/* - * Moderne Proprietary. Only for use by Moderne customers under the terms of a commercial contract. - */ - -package redundancy - -import ( - "github.com/openrewrite/rewrite/rewrite-go/pkg/tree/golang" - "github.com/openrewrite/rewrite/rewrite-go/pkg/visitor" -) - -// isInitWrappedIf reports whether the If at the cursor is the inner statement of -// a golang.StatementWithInit — i.e. it carried an `if init; cond` init clause. -func isInitWrappedIf(c *visitor.Cursor) bool { - parent := c.Parent() - if parent == nil { - return false - } - _, ok := parent.Value().(*golang.StatementWithInit) - return ok -} diff --git a/recipes/redundancy/remove_redundant_sprintf.go b/recipes/redundancy/remove_redundant_sprintf.go index dfade4d..026f288 100644 --- a/recipes/redundancy/remove_redundant_sprintf.go +++ b/recipes/redundancy/remove_redundant_sprintf.go @@ -8,25 +8,21 @@ import ( "fmt" "github.com/moderneinc/recipes-go/diagnostic" + "github.com/openrewrite/rewrite/rewrite-go/pkg/matcher" "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe" recipegolang "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe/golang" "github.com/openrewrite/rewrite/rewrite-go/pkg/template" + "github.com/openrewrite/rewrite/rewrite-go/pkg/tree/java" + "github.com/openrewrite/rewrite/rewrite-go/pkg/visitor" ) -var ( - sprintfArg = template.Expr("s") +var sprintfArg = template.Expr("s") - removeRedundantSprintfImpl = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.RemoveRedundantSprintf"), - template.WithDisplayName("Remove redundant fmt.Sprintf"), - template.WithBefore(fmt.Sprintf(`fmt.Sprintf("%%s", %s)`, sprintfArg), template.Imports("fmt")), - template.WithAfter(fmt.Sprintf(`%s`, sprintfArg)), - template.WithCaptures(sprintfArg), - ) -) +var redundantSprintfPattern = template.Expression(fmt.Sprintf(`fmt.Sprintf("%%s", %s)`, sprintfArg)). + Captures(sprintfArg).Imports("fmt").Build() -// RemoveRedundantSprintf replaces `fmt.Sprintf("%s", s)` with just `s` -// when the format string is a single %s and the argument is a string. +// Replaces `fmt.Sprintf("%s", s)` with `s` when the format string is a single +// %s and the argument is a string. // Staticcheck: S1025 type RemoveRedundantSprintf struct { recipe.Base @@ -47,6 +43,53 @@ func (r *RemoveRedundantSprintf) DiagnosticMappings() []diagnostic.Mapping { } } -func (r *RemoveRedundantSprintf) RecipeList() []recipe.Recipe { - return []recipe.Recipe{removeRedundantSprintfImpl, &recipegolang.RemoveUnusedImports{}} +func (r *RemoveRedundantSprintf) Editor() recipe.TreeVisitor { + return visitor.Init(&removeRedundantSprintfVisitor{}) +} + +type removeRedundantSprintfVisitor struct { + visitor.GoVisitor +} + +func (v *removeRedundantSprintfVisitor) VisitMethodInvocation(mi *java.MethodInvocation, p any) java.J { + mi = v.GoVisitor.VisitMethodInvocation(mi, p).(*java.MethodInvocation) + + match := redundantSprintfPattern.Match(mi, nil) + if match == nil { + return mi + } + + // Skip unless the argument is a string, since %s also formats []byte, a + // fmt.Stringer, or a named string type, none of which are a plain string. + arg, ok := match.GetCapture(sprintfArg).(java.Expression) + if !ok || !matcher.IsString(matcher.TypeOfExpression(arg)) { + return mi + } + + v.DoAfterVisit(recipe.Service[*recipegolang.ImportService](nil).RemoveUnusedImportsVisitor()) + return withLeadingPrefix(arg, mi.GetPrefix()) +} + +// Returns e with its leading prefix set to p, covering the expression kinds that +// appear as a fmt.Sprintf string argument. +func withLeadingPrefix(e java.Expression, p java.Space) java.Expression { + switch n := e.(type) { + case *java.Identifier: + return n.WithPrefix(p) + case *java.Literal: + return n.WithPrefix(p) + case *java.FieldAccess: + return n.WithPrefix(p) + case *java.MethodInvocation: + return n.WithPrefix(p) + case *java.Parentheses: + return n.WithPrefix(p) + case *java.Binary: + return n.WithPrefix(p) + case *java.ArrayAccess: + return n.WithPrefix(p) + case *java.TypeCast: + return n.WithPrefix(p) + } + return e } diff --git a/recipes/redundancy/simplify_goroutine_closure.go b/recipes/redundancy/simplify_goroutine_closure.go index 0f986c3..c445147 100644 --- a/recipes/redundancy/simplify_goroutine_closure.go +++ b/recipes/redundancy/simplify_goroutine_closure.go @@ -39,6 +39,16 @@ type simplifyGoroutineClosureVisitor struct { visitor.GoVisitor } +// Reports whether the function literal declares any parameters. +func closureHasParams(md *java.MethodDeclaration) bool { + for _, e := range md.Parameters.Elements { + if _, isEmpty := e.Element.(*java.Empty); !isEmpty { + return true + } + } + return false +} + func (v *simplifyGoroutineClosureVisitor) VisitGoStmt(g *golang.GoStmt, p any) java.J { g = v.GoVisitor.VisitGoStmt(g, p).(*golang.GoStmt) @@ -66,6 +76,12 @@ func (v *simplifyGoroutineClosureVisitor) VisitGoStmt(g *golang.GoStmt, p any) j return g } + // Only simplify a parameterless closure; dropping its parameters and the + // call's arguments would leave the inner call referencing out-of-scope names. + if closureHasParams(funcLit) { + return g + } + // The function literal must have a body with exactly 1 real statement. if funcLit.Body == nil { return g diff --git a/recipes/redundancy/simplify_nil_check_before_close.go b/recipes/redundancy/simplify_nil_check_before_close.go index 8eb2bea..28dda4f 100644 --- a/recipes/redundancy/simplify_nil_check_before_close.go +++ b/recipes/redundancy/simplify_nil_check_before_close.go @@ -5,6 +5,7 @@ package redundancy 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" @@ -47,7 +48,7 @@ func (v *simplifyNilCheckBeforeCloseVisitor) VisitIf(ifStmt *java.If, p any) jav // Must not have an init statement: `if x := ...; cond` is wrapped in a // golang.StatementWithInit, so skip Ifs that are its inner statement. - if isInitWrappedIf(v.Cursor()) { + if lstutil.IsInitWrappedIf(v.Cursor()) { return ifStmt } diff --git a/recipes/redundancy/simplify_redundant_len_before_range.go b/recipes/redundancy/simplify_redundant_len_before_range.go index b95f954..51e2082 100644 --- a/recipes/redundancy/simplify_redundant_len_before_range.go +++ b/recipes/redundancy/simplify_redundant_len_before_range.go @@ -7,6 +7,7 @@ package redundancy import ( "strings" + "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" @@ -50,7 +51,7 @@ func (v *simplifyRedundantLenBeforeRangeVisitor) VisitIf(ifStmt *java.If, p any) // Must not have an init statement: `if init; cond` is wrapped in a // golang.StatementWithInit, so skip Ifs that are its inner statement. - if isInitWrappedIf(v.Cursor()) { + if lstutil.IsInitWrappedIf(v.Cursor()) { return ifStmt } diff --git a/recipes/simplification/if_init.go b/recipes/simplification/if_init.go deleted file mode 100644 index 56503c2..0000000 --- a/recipes/simplification/if_init.go +++ /dev/null @@ -1,21 +0,0 @@ -/* - * Moderne Proprietary. Only for use by Moderne customers under the terms of a commercial contract. - */ - -package simplification - -import ( - "github.com/openrewrite/rewrite/rewrite-go/pkg/tree/golang" - "github.com/openrewrite/rewrite/rewrite-go/pkg/visitor" -) - -// isInitWrappedIf reports whether the If at the cursor is the inner statement of -// a golang.StatementWithInit — i.e. it carried an `if init; cond` init clause. -func isInitWrappedIf(c *visitor.Cursor) bool { - parent := c.Parent() - if parent == nil { - return false - } - _, ok := parent.Value().(*golang.StatementWithInit) - return ok -} diff --git a/recipes/simplification/merge_collapsible_if.go b/recipes/simplification/merge_collapsible_if.go index 98b2464..b1e05e7 100644 --- a/recipes/simplification/merge_collapsible_if.go +++ b/recipes/simplification/merge_collapsible_if.go @@ -7,6 +7,7 @@ package simplification import ( "strings" + "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" @@ -51,7 +52,7 @@ func (v *mergeCollapsibleIfVisitor) VisitIf(ifStmt *java.If, p any) java.J { // Outer if must not have an else clause or init statement. An `if init; cond` // is wrapped in a golang.StatementWithInit, so skip wrapped Ifs. - if ifStmt.ElsePart != nil || isInitWrappedIf(v.Cursor()) { + if ifStmt.ElsePart != nil || lstutil.IsInitWrappedIf(v.Cursor()) { return ifStmt } diff --git a/recipes/simplification/prefer_empty_string_check.go b/recipes/simplification/prefer_empty_string_check.go index cd9fe6e..d2d8568 100644 --- a/recipes/simplification/prefer_empty_string_check.go +++ b/recipes/simplification/prefer_empty_string_check.go @@ -8,14 +8,23 @@ import ( "fmt" "github.com/moderneinc/recipes-go/diagnostic" + "github.com/openrewrite/rewrite/rewrite-go/pkg/matcher" "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe" "github.com/openrewrite/rewrite/rewrite-go/pkg/template" + "github.com/openrewrite/rewrite/rewrite-go/pkg/tree/java" + "github.com/openrewrite/rewrite/rewrite-go/pkg/visitor" ) var esS = template.Expr("esS") -// PreferEmptyStringCheck replaces `len(s) == 0` with `s == ""` and -// `len(s) != 0` with `s != ""`. +var ( + emptyStrEqPattern = template.Expression(fmt.Sprintf(`len(%s) == 0`, esS)).Captures(esS).Build() + emptyStrEqTemplate = template.ExpressionTemplate(fmt.Sprintf(`%s == ""`, esS)).Captures(esS).Build() + emptyStrNePattern = template.Expression(fmt.Sprintf(`len(%s) != 0`, esS)).Captures(esS).Build() + emptyStrNeTemplate = template.ExpressionTemplate(fmt.Sprintf(`%s != ""`, esS)).Captures(esS).Build() +) + +// Replaces `len(s) == 0` with `s == ""` and `len(s) != 0` with `s != ""`. type PreferEmptyStringCheck struct { recipe.Base } @@ -35,22 +44,39 @@ func (r *PreferEmptyStringCheck) DiagnosticMappings() []diagnostic.Mapping { return []diagnostic.Mapping{} } -var preferEmptyStringCheckEqual = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.PreferEmptyStringCheck$Equal"), - template.WithDisplayName("len(s) == 0 → s == \"\""), - template.WithBefore(fmt.Sprintf(`len(%s) == 0`, esS)), - template.WithAfter(fmt.Sprintf(`%s == ""`, esS)), - template.WithCaptures(esS), -) +func (r *PreferEmptyStringCheck) Editor() recipe.TreeVisitor { + return visitor.Init(&preferEmptyStringCheckVisitor{}) +} -var preferEmptyStringCheckNotEqual = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.PreferEmptyStringCheck$NotEqual"), - template.WithDisplayName("len(s) != 0 → s != \"\""), - template.WithBefore(fmt.Sprintf(`len(%s) != 0`, esS)), - template.WithAfter(fmt.Sprintf(`%s != ""`, esS)), - template.WithCaptures(esS), -) +type preferEmptyStringCheckVisitor struct { + visitor.GoVisitor +} + +func (v *preferEmptyStringCheckVisitor) VisitBinary(bin *java.Binary, p any) java.J { + bin = v.GoVisitor.VisitBinary(bin, p).(*java.Binary) -func (r *PreferEmptyStringCheck) RecipeList() []recipe.Recipe { - return []recipe.Recipe{preferEmptyStringCheckEqual, preferEmptyStringCheckNotEqual} + for _, pt := range []struct { + pat *template.GoPattern + tmpl *template.GoTemplate + }{ + {emptyStrEqPattern, emptyStrEqTemplate}, + {emptyStrNePattern, emptyStrNeTemplate}, + } { + match := pt.pat.Match(bin, nil) + if match == nil { + continue + } + // Skip unless the len() argument is a string, since `== ""` requires a + // string while len also accepts slices, maps, arrays, and channels. + arg, ok := match.GetCapture(esS).(java.Expression) + if !ok || !matcher.IsString(matcher.TypeOfExpression(arg)) { + return bin + } + replaced, ok := pt.tmpl.Apply(nil, match).(*java.Binary) + if !ok { + return bin + } + return replaced.WithPrefix(bin.GetPrefix()) + } + return bin } diff --git a/recipes/simplification/prefer_io_writestring.go b/recipes/simplification/prefer_io_writestring.go index 11dc005..ff6cd2a 100644 --- a/recipes/simplification/prefer_io_writestring.go +++ b/recipes/simplification/prefer_io_writestring.go @@ -8,8 +8,12 @@ import ( "fmt" "github.com/moderneinc/recipes-go/diagnostic" + "github.com/openrewrite/rewrite/rewrite-go/pkg/matcher" "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe" + recipegolang "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe/golang" "github.com/openrewrite/rewrite/rewrite-go/pkg/template" + "github.com/openrewrite/rewrite/rewrite-go/pkg/tree/java" + "github.com/openrewrite/rewrite/rewrite-go/pkg/visitor" ) var ( @@ -17,7 +21,14 @@ var ( wsStr = template.Expr("wsStr") ) -// PreferIoWriteString replaces `fmt.Fprintf(w, "%s", s)` with `io.WriteString(w, s)`. +var ( + ioWriteStringPattern = template.Expression(fmt.Sprintf(`fmt.Fprintf(%s, "%%s", %s)`, wsW, wsStr)). + Captures(wsW, wsStr).Imports("fmt").Build() + ioWriteStringTemplate = template.ExpressionTemplate(fmt.Sprintf(`io.WriteString(%s, %s)`, wsW, wsStr)). + Captures(wsW, wsStr).Imports("io").Build() +) + +// Replaces `fmt.Fprintf(w, "%s", s)` with `io.WriteString(w, s)`. // Staticcheck: S1025 type PreferIoWriteString struct { recipe.Base @@ -38,14 +49,34 @@ func (r *PreferIoWriteString) DiagnosticMappings() []diagnostic.Mapping { } } -var preferIoWriteStringImpl = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.PreferIoWriteString$Impl"), - template.WithDisplayName(`fmt.Fprintf(w, "%s", s) → io.WriteString(w, s)`), - template.WithBefore(fmt.Sprintf(`fmt.Fprintf(%s, "%%s", %s)`, wsW, wsStr), template.Imports("fmt")), - template.WithAfter(fmt.Sprintf(`io.WriteString(%s, %s)`, wsW, wsStr), template.Imports("io"), template.SourceImports("io")), - template.WithCaptures(wsW, wsStr), -) +func (r *PreferIoWriteString) Editor() recipe.TreeVisitor { + return visitor.Init(&preferIoWriteStringVisitor{}) +} + +type preferIoWriteStringVisitor struct { + visitor.GoVisitor +} -func (r *PreferIoWriteString) RecipeList() []recipe.Recipe { - return []recipe.Recipe{preferIoWriteStringImpl} +func (v *preferIoWriteStringVisitor) VisitMethodInvocation(mi *java.MethodInvocation, p any) java.J { + mi = v.GoVisitor.VisitMethodInvocation(mi, p).(*java.MethodInvocation) + + match := ioWriteStringPattern.Match(mi, nil) + if match == nil { + return mi + } + + // Skip unless the formatted value is a string, since io.WriteString takes a + // string while %s also accepts []byte or a fmt.Stringer. + arg, ok := match.GetCapture(wsStr).(java.Expression) + if !ok || !matcher.IsString(matcher.TypeOfExpression(arg)) { + return mi + } + + replaced, ok := ioWriteStringTemplate.Apply(nil, match).(*java.MethodInvocation) + if !ok { + return mi + } + recipegolang.MaybeAddImport(v, "io", nil, false) + v.DoAfterVisit(recipe.Service[*recipegolang.ImportService](nil).RemoveUnusedImportsVisitor()) + return replaced.WithPrefix(mi.GetPrefix()) } diff --git a/recipes/simplification/prefer_os_readdir.go b/recipes/simplification/prefer_os_readdir.go index 1bae819..81d79ed 100644 --- a/recipes/simplification/prefer_os_readdir.go +++ b/recipes/simplification/prefer_os_readdir.go @@ -9,7 +9,10 @@ import ( "github.com/moderneinc/recipes-go/diagnostic" "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe" + recipegolang "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe/golang" "github.com/openrewrite/rewrite/rewrite-go/pkg/template" + "github.com/openrewrite/rewrite/rewrite-go/pkg/tree/java" + "github.com/openrewrite/rewrite/rewrite-go/pkg/visitor" ) var rdirName = template.Expr("rdirName") @@ -37,14 +40,45 @@ func (r *PreferOsReadDir) DiagnosticMappings() []diagnostic.Mapping { } } -var preferOsReadDirImpl = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.PreferOsReadDir$Impl"), - template.WithDisplayName("ioutil.ReadDir \u2192 os.ReadDir"), - template.WithBefore(fmt.Sprintf(`ioutil.ReadDir(%s)`, rdirName), template.Imports("io/ioutil")), - template.WithAfter(fmt.Sprintf(`os.ReadDir(%s)`, rdirName), template.Imports("os"), template.SourceImports("os")), - template.WithCaptures(rdirName), +var ( + readDirPattern = template.Expression(fmt.Sprintf(`ioutil.ReadDir(%s)`, rdirName)). + Captures(rdirName).Imports("io/ioutil").Build() + readDirTemplate = template.ExpressionTemplate(fmt.Sprintf(`os.ReadDir(%s)`, rdirName)). + Captures(rdirName).Imports("os").Build() ) -func (r *PreferOsReadDir) RecipeList() []recipe.Recipe { - return []recipe.Recipe{preferOsReadDirImpl} +func (r *PreferOsReadDir) Editor() recipe.TreeVisitor { + return visitor.Init(&preferOsReadDirVisitor{}) +} + +type preferOsReadDirVisitor struct { + visitor.GoVisitor +} + +func (v *preferOsReadDirVisitor) VisitMethodInvocation(mi *java.MethodInvocation, p any) java.J { + mi = v.GoVisitor.VisitMethodInvocation(mi, p).(*java.MethodInvocation) + + match := readDirPattern.Match(mi, nil) + if match == nil { + return mi + } + + // Skip when the result is required as []os.FileInfo by a return or a typed + // variable declaration, where os.ReadDir's []os.DirEntry would not compile. + if t, ok := requiredResultType(v.Cursor()); ok && t == "[]os.FileInfo" { + return mi + } + + replaced := readDirTemplate.Apply(nil, match) + if replaced == nil { + return mi + } + newCall, ok := replaced.(*java.MethodInvocation) + if !ok { + return mi + } + + recipegolang.MaybeAddImport(v, "os", nil, false) + v.DoAfterVisit(recipe.Service[*recipegolang.ImportService](nil).RemoveUnusedImportsVisitor()) + return newCall.WithPrefix(mi.GetPrefix()) } diff --git a/recipes/simplification/prefer_strconv_atoi.go b/recipes/simplification/prefer_strconv_atoi.go index b916822..ebd28e4 100644 --- a/recipes/simplification/prefer_strconv_atoi.go +++ b/recipes/simplification/prefer_strconv_atoi.go @@ -10,6 +10,9 @@ import ( "github.com/moderneinc/recipes-go/diagnostic" "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe" "github.com/openrewrite/rewrite/rewrite-go/pkg/template" + "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" ) var atoiS = template.Expr("atoiS") @@ -35,14 +38,97 @@ func (r *PreferStrconvAtoi) DiagnosticMappings() []diagnostic.Mapping { } } -var preferStrconvAtoiImpl = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.PreferStrconvAtoi$Impl"), - template.WithDisplayName("strconv.ParseInt(s, 10, 0) → strconv.Atoi(s)"), - template.WithBefore(fmt.Sprintf(`strconv.ParseInt(%s, 10, 0)`, atoiS), template.Imports("strconv")), - template.WithAfter(fmt.Sprintf(`strconv.Atoi(%s)`, atoiS), template.Imports("strconv")), - template.WithCaptures(atoiS), +var ( + atoiPattern = template.Expression(fmt.Sprintf(`strconv.ParseInt(%s, 10, 0)`, atoiS)). + Captures(atoiS).Imports("strconv").Build() + atoiTemplate = template.ExpressionTemplate(fmt.Sprintf(`strconv.Atoi(%s)`, atoiS)). + Captures(atoiS).Imports("strconv").Build() ) -func (r *PreferStrconvAtoi) RecipeList() []recipe.Recipe { - return []recipe.Recipe{preferStrconvAtoiImpl} +func (r *PreferStrconvAtoi) Editor() recipe.TreeVisitor { + return visitor.Init(&preferStrconvAtoiVisitor{}) +} + +type preferStrconvAtoiVisitor struct { + visitor.GoVisitor +} + +func (v *preferStrconvAtoiVisitor) VisitMethodInvocation(mi *java.MethodInvocation, p any) java.J { + mi = v.GoVisitor.VisitMethodInvocation(mi, p).(*java.MethodInvocation) + + match := atoiPattern.Match(mi, nil) + if match == nil { + return mi + } + + // Skip when the int64 result is required as int64 by a return or a typed + // variable declaration, where strconv.Atoi's int would not compile. + if t, ok := requiredResultType(v.Cursor()); ok && t == "int64" { + return mi + } + + // Skip when a `n, err :=` capture of the int64 result is later returned as int64. + if capturedValueReturnedAsInt64(v.Cursor()) { + return mi + } + + replaced := atoiTemplate.Apply(nil, match) + if replaced == nil { + return mi + } + newCall, ok := replaced.(*java.MethodInvocation) + if !ok { + return mi + } + return newCall.WithPrefix(mi.GetPrefix()) +} + +// Reports whether a `x, err :=` capture of the ParseInt result has x later +// returned at an int64 result position, a best-effort check that misses int64 +// parameters, arithmetic, and closures. +func capturedValueReturnedAsInt64(c *visitor.Cursor) bool { + ma, ok := c.Parent().Value().(*golang.MultiAssignment) + if !ok || len(ma.Variables) == 0 { + return false + } + // ParseInt's int64 result is captured by the first variable. + id, ok := ma.Variables[0].Element.(*java.Identifier) + if !ok || id.Name == "_" { + return false + } + md, ok := visitor.FirstEnclosing[*java.MethodDeclaration](c) + if !ok { + return false + } + + scan := &int64ReturnScanner{varName: id.Name, resultTypes: functionResultTypes(md)} + scan.Self = scan + scan.Visit(md, nil) + return scan.found +} + +// Sets found when a `return` yields varName at an int64 result position. +type int64ReturnScanner struct { + visitor.GoVisitor + varName string + resultTypes []string + found bool +} + +func (s *int64ReturnScanner) VisitGoReturn(ret *golang.Return, p any) java.J { + for i, e := range ret.Expressions { + if id, ok := e.Element.(*java.Identifier); ok && id.Name == s.varName && + i < len(s.resultTypes) && s.resultTypes[i] == "int64" { + s.found = true + } + } + return s.GoVisitor.VisitGoReturn(ret, p) +} + +func (s *int64ReturnScanner) VisitReturn(ret *java.Return, p any) java.J { + if id, ok := ret.Expression.(*java.Identifier); ok && id.Name == s.varName && + len(s.resultTypes) >= 1 && s.resultTypes[0] == "int64" { + s.found = true + } + return s.GoVisitor.VisitReturn(ret, p) } diff --git a/recipes/simplification/prefer_strings_builder_writestring.go b/recipes/simplification/prefer_strings_builder_writestring.go index 3e1d192..4ac180d 100644 --- a/recipes/simplification/prefer_strings_builder_writestring.go +++ b/recipes/simplification/prefer_strings_builder_writestring.go @@ -7,9 +7,12 @@ package simplification import ( "fmt" + "github.com/openrewrite/rewrite/rewrite-go/pkg/matcher" "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe" recipegolang "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe/golang" "github.com/openrewrite/rewrite/rewrite-go/pkg/template" + "github.com/openrewrite/rewrite/rewrite-go/pkg/tree/java" + "github.com/openrewrite/rewrite/rewrite-go/pkg/visitor" ) var ( @@ -17,8 +20,16 @@ var ( sbwS = template.Expr("sbwS") ) -// PreferStringsBuilderWriteString replaces `fmt.Fprintf(&b, "%s", s)` with -// `b.WriteString(s)` when writing to a strings.Builder. Staticcheck: S1038 +var ( + sbWriteStringPattern = template.Expression(fmt.Sprintf(`fmt.Fprintf(&%s, "%%s", %s)`, sbwB, sbwS)). + Captures(sbwB, sbwS).Imports("fmt").Build() + sbWriteStringTemplate = template.ExpressionTemplate(fmt.Sprintf(`%s.WriteString(%s)`, sbwB, sbwS)). + Captures(sbwB, sbwS).Build() +) + +// Replaces `fmt.Fprintf(&b, "%s", s)` with `b.WriteString(s)` when writing to a +// strings.Builder. +// Staticcheck: S1038 type PreferStringsBuilderWriteString struct { recipe.Base } @@ -36,14 +47,33 @@ func (r *PreferStringsBuilderWriteString) Tags() []string { return []string{"cleanup", "simplification"} } -var preferStringsBuilderWriteStringImpl = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.PreferStringsBuilderWriteString$Impl"), - template.WithDisplayName(`fmt.Fprintf(&b, "%s", s) -> b.WriteString(s)`), - template.WithBefore(fmt.Sprintf(`fmt.Fprintf(&%s, "%%s", %s)`, sbwB, sbwS), template.Imports("fmt")), - template.WithAfter(fmt.Sprintf(`%s.WriteString(%s)`, sbwB, sbwS)), - template.WithCaptures(sbwB, sbwS), -) +func (r *PreferStringsBuilderWriteString) Editor() recipe.TreeVisitor { + return visitor.Init(&preferStringsBuilderWriteStringVisitor{}) +} + +type preferStringsBuilderWriteStringVisitor struct { + visitor.GoVisitor +} + +func (v *preferStringsBuilderWriteStringVisitor) VisitMethodInvocation(mi *java.MethodInvocation, p any) java.J { + mi = v.GoVisitor.VisitMethodInvocation(mi, p).(*java.MethodInvocation) + + match := sbWriteStringPattern.Match(mi, nil) + if match == nil { + return mi + } + + // Skip unless the formatted value is a string, since Builder.WriteString takes + // a string while %s also accepts []byte or a fmt.Stringer. + arg, ok := match.GetCapture(sbwS).(java.Expression) + if !ok || !matcher.IsString(matcher.TypeOfExpression(arg)) { + return mi + } -func (r *PreferStringsBuilderWriteString) RecipeList() []recipe.Recipe { - return []recipe.Recipe{preferStringsBuilderWriteStringImpl, &recipegolang.RemoveUnusedImports{}} + replaced, ok := sbWriteStringTemplate.Apply(nil, match).(*java.MethodInvocation) + if !ok { + return mi + } + v.DoAfterVisit(recipe.Service[*recipegolang.ImportService](nil).RemoveUnusedImportsVisitor()) + return replaced.WithPrefix(mi.GetPrefix()) } diff --git a/recipes/simplification/prefer_strings_newreader.go b/recipes/simplification/prefer_strings_newreader.go index 1a2ea50..9844770 100644 --- a/recipes/simplification/prefer_strings_newreader.go +++ b/recipes/simplification/prefer_strings_newreader.go @@ -8,8 +8,12 @@ import ( "fmt" "github.com/moderneinc/recipes-go/diagnostic" + "github.com/openrewrite/rewrite/rewrite-go/pkg/matcher" "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe" + recipegolang "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe/golang" "github.com/openrewrite/rewrite/rewrite-go/pkg/template" + "github.com/openrewrite/rewrite/rewrite-go/pkg/tree/java" + "github.com/openrewrite/rewrite/rewrite-go/pkg/visitor" ) var snrS = template.Expr("snrS") @@ -38,14 +42,53 @@ func (r *PreferStringsNewReader) DiagnosticMappings() []diagnostic.Mapping { } } -var preferStringsNewReaderImpl = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.PreferStringsNewReader$Impl"), - template.WithDisplayName("bytes.NewReader([]byte) → strings.NewReader"), - template.WithBefore(fmt.Sprintf(`bytes.NewReader([]byte(%s))`, snrS), template.Imports("bytes")), - template.WithAfter(fmt.Sprintf(`strings.NewReader(%s)`, snrS), template.Imports("strings"), template.SourceImports("strings")), - template.WithCaptures(snrS), +var ( + newReaderPattern = template.Expression(fmt.Sprintf(`bytes.NewReader([]byte(%s))`, snrS)). + Captures(snrS).Imports("bytes").Build() + newReaderTemplate = template.ExpressionTemplate(fmt.Sprintf(`strings.NewReader(%s)`, snrS)). + Captures(snrS).Imports("strings").Build() ) -func (r *PreferStringsNewReader) RecipeList() []recipe.Recipe { - return []recipe.Recipe{preferStringsNewReaderImpl} +func (r *PreferStringsNewReader) Editor() recipe.TreeVisitor { + return visitor.Init(&preferStringsNewReaderVisitor{}) +} + +type preferStringsNewReaderVisitor struct { + visitor.GoVisitor +} + +func (v *preferStringsNewReaderVisitor) VisitMethodInvocation(mi *java.MethodInvocation, p any) java.J { + mi = v.GoVisitor.VisitMethodInvocation(mi, p).(*java.MethodInvocation) + + match := newReaderPattern.Match(mi, nil) + if match == nil { + return mi + } + + // Skip unless the converted value is a string, since strings.NewReader takes + // a string and would not compile on a []byte argument. + inner, ok := match.GetCapture(snrS).(java.Expression) + if !ok || !matcher.IsString(matcher.TypeOfExpression(inner)) { + return mi + } + + // Skip when the result is required as *bytes.Reader by a return or a typed + // variable declaration, where strings.NewReader's *strings.Reader would not + // compile, but leave an interface target such as io.Reader to rewrite. + if t, ok := requiredResultType(v.Cursor()); ok && t == "*bytes.Reader" { + return mi + } + + replaced := newReaderTemplate.Apply(nil, match) + if replaced == nil { + return mi + } + newCall, ok := replaced.(*java.MethodInvocation) + if !ok { + return mi + } + + recipegolang.MaybeAddImport(v, "strings", nil, false) + v.DoAfterVisit(recipe.Service[*recipegolang.ImportService](nil).RemoveUnusedImportsVisitor()) + return newCall.WithPrefix(mi.GetPrefix()) } diff --git a/recipes/simplification/prefer_strings_repeat.go b/recipes/simplification/prefer_strings_repeat.go index d97f689..871d104 100644 --- a/recipes/simplification/prefer_strings_repeat.go +++ b/recipes/simplification/prefer_strings_repeat.go @@ -8,9 +8,12 @@ import ( "fmt" "github.com/moderneinc/recipes-go/diagnostic" + "github.com/openrewrite/rewrite/rewrite-go/pkg/matcher" "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe" recipegolang "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe/golang" "github.com/openrewrite/rewrite/rewrite-go/pkg/template" + "github.com/openrewrite/rewrite/rewrite-go/pkg/tree/java" + "github.com/openrewrite/rewrite/rewrite-go/pkg/visitor" ) var ( @@ -18,8 +21,15 @@ var ( scB = template.Expr("scB") ) -// SimplifySprintfConcat replaces `fmt.Sprintf("%s%s", a, b)` with `a + b`. -// Using fmt.Sprintf for simple string concatenation is unnecessary overhead. +var ( + sprintfConcatPattern = template.Expression(fmt.Sprintf(`fmt.Sprintf("%%s%%s", %s, %s)`, scA, scB)). + Captures(scA, scB).Imports("fmt").Build() + sprintfConcatTemplate = template.ExpressionTemplate(fmt.Sprintf(`%s + %s`, scA, scB)). + Captures(scA, scB).Build() +) + +// Replaces `fmt.Sprintf("%s%s", a, b)` with `a + b` to avoid unnecessary +// formatting overhead for simple string concatenation. type SimplifySprintfConcat struct { recipe.Base } @@ -37,14 +47,36 @@ func (r *SimplifySprintfConcat) DiagnosticMappings() []diagnostic.Mapping { return []diagnostic.Mapping{} } -var simplifySprintfConcatImpl = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.SimplifySprintfConcat$Impl"), - template.WithDisplayName("fmt.Sprintf(\"%s%s\", a, b) → a + b"), - template.WithBefore(fmt.Sprintf(`fmt.Sprintf("%%s%%s", %s, %s)`, scA, scB), template.Imports("fmt")), - template.WithAfter(fmt.Sprintf(`%s + %s`, scA, scB)), - template.WithCaptures(scA, scB), -) +func (r *SimplifySprintfConcat) Editor() recipe.TreeVisitor { + return visitor.Init(&simplifySprintfConcatVisitor{}) +} + +type simplifySprintfConcatVisitor struct { + visitor.GoVisitor +} + +func (v *simplifySprintfConcatVisitor) VisitMethodInvocation(mi *java.MethodInvocation, p any) java.J { + mi = v.GoVisitor.VisitMethodInvocation(mi, p).(*java.MethodInvocation) + + match := sprintfConcatPattern.Match(mi, nil) + if match == nil { + return mi + } + + // Skip unless both arguments are strings, since `+` concatenates strings but + // %s also accepts []byte and fmt.Stringer, which cannot be added. + a, aok := match.GetCapture(scA).(java.Expression) + b, bok := match.GetCapture(scB).(java.Expression) + if !aok || !bok || + !matcher.IsString(matcher.TypeOfExpression(a)) || + !matcher.IsString(matcher.TypeOfExpression(b)) { + return mi + } -func (r *SimplifySprintfConcat) RecipeList() []recipe.Recipe { - return []recipe.Recipe{simplifySprintfConcatImpl, &recipegolang.RemoveUnusedImports{}} + replaced, ok := sprintfConcatTemplate.Apply(nil, match).(*java.Binary) + if !ok { + return mi + } + v.DoAfterVisit(recipe.Service[*recipegolang.ImportService](nil).RemoveUnusedImportsVisitor()) + return replaced.WithPrefix(mi.GetPrefix()) } diff --git a/recipes/simplification/type_context.go b/recipes/simplification/type_context.go new file mode 100644 index 0000000..0128de6 --- /dev/null +++ b/recipes/simplification/type_context.go @@ -0,0 +1,86 @@ +/* + * Moderne Proprietary. Only for use by Moderne customers under the terms of a commercial contract. + */ + +package simplification + +import ( + "strings" + + "github.com/openrewrite/rewrite/rewrite-go/pkg/printer" + "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" +) + +// Reports the source text of the type the value at the cursor must locally +// satisfy from a direct `return` or a `var x T = call` declaration, or +// ("", false) when the type is inferred or otherwise not locally knowable. +func requiredResultType(c *visitor.Cursor) (string, bool) { + parent := c.Parent() + if parent == nil { + return "", false + } + switch parent.Value().(type) { + case *java.Return, *golang.Return: + return enclosingFirstResultType(c) + case *java.VariableDeclarator: + return explicitDeclType(parent) + } + return "", false +} + +// Returns the source text of the first result type of the function enclosing +// the cursor. +func enclosingFirstResultType(c *visitor.Cursor) (string, bool) { + md, ok := visitor.FirstEnclosing[*java.MethodDeclaration](c) + if !ok { + return "", false + } + types := functionResultTypes(md) + if len(types) == 0 { + return "", false + } + return types[0], true +} + +// Returns the source text of each of a function's result types in order, read +// from the signature rather than the LST's resolved scalar types which do not +// reliably distinguish int from int64. +func functionResultTypes(md *java.MethodDeclaration) []string { + if md == nil || md.ReturnType == nil { + return nil + } + tl, ok := md.ReturnType.(*golang.TypeList) + if !ok { + // Single unnamed result, e.g. func f() error. + return []string{strings.TrimSpace(printer.Print(md.ReturnType))} + } + out := make([]string, 0, len(tl.Types.Elements)) + for _, e := range tl.Types.Elements { + var typeExpr java.J = e.Element + if vd, ok := e.Element.(*java.VariableDeclarations); ok { + typeExpr = vd.TypeExpr + } + if typeExpr == nil { + out = append(out, "") + continue + } + out = append(out, strings.TrimSpace(printer.Print(typeExpr))) + } + return out +} + +// Returns the declared type of a `var x T = ...` declaration, or ("", false) +// for inferred declarations that carry no explicit type. +func explicitDeclType(declarator *visitor.Cursor) (string, bool) { + gp := declarator.Parent() + if gp == nil { + return "", false + } + vd, ok := gp.Value().(*java.VariableDeclarations) + if !ok || vd.TypeExpr == nil { + return "", false + } + return strings.TrimSpace(printer.Print(vd.TypeExpr)), true +} diff --git a/recipes/simplification/use_structured_logging.go b/recipes/simplification/use_structured_logging.go index e5b8ffc..70e7827 100644 --- a/recipes/simplification/use_structured_logging.go +++ b/recipes/simplification/use_structured_logging.go @@ -8,15 +8,16 @@ import ( "fmt" "strings" + "github.com/openrewrite/rewrite/rewrite-go/pkg/matcher" "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe" + recipegolang "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe/golang" "github.com/openrewrite/rewrite/rewrite-go/pkg/template" "github.com/openrewrite/rewrite/rewrite-go/pkg/tree/java" "github.com/openrewrite/rewrite/rewrite-go/pkg/visitor" ) -// UseStructuredLogging finds calls to the standard `log` package such as `log.Println`, -// `log.Printf`, `log.Fatal`, and `log.Fatalf`. In Go 1.21+ consider migrating -// to `log/slog` for structured logging. +// Finds calls to the standard `log` package such as `log.Println`, `log.Printf`, +// `log.Fatal`, and `log.Fatalf` and suggests migrating to `log/slog` (Go 1.21+). type UseStructuredLogging struct { recipe.Base } @@ -32,26 +33,15 @@ func (r *UseStructuredLogging) Tags() []string { return []string{"simplification var slogMsg = template.Expr("slogMsg") -var useStructuredLoggingPrintln = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.UseStructuredLogging$Println"), - template.WithDisplayName("log.Println → slog.Info"), - template.WithBefore(fmt.Sprintf(`log.Println(%s)`, slogMsg), template.Imports("log")), - template.WithAfter(fmt.Sprintf(`slog.Info(%s)`, slogMsg), template.Imports("log/slog"), template.SourceImports("log/slog")), - template.WithCaptures(slogMsg), +var ( + logPrintlnPattern = template.Expression(fmt.Sprintf(`log.Println(%s)`, slogMsg)). + Captures(slogMsg).Imports("log").Build() + logPrintPattern = template.Expression(fmt.Sprintf(`log.Print(%s)`, slogMsg)). + Captures(slogMsg).Imports("log").Build() + slogInfoTemplate = template.ExpressionTemplate(fmt.Sprintf(`slog.Info(%s)`, slogMsg)). + Captures(slogMsg).Imports("log/slog").Build() ) -var useStructuredLoggingPrint = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.UseStructuredLogging$Print"), - template.WithDisplayName("log.Print → slog.Info"), - template.WithBefore(fmt.Sprintf(`log.Print(%s)`, slogMsg), template.Imports("log")), - template.WithAfter(fmt.Sprintf(`slog.Info(%s)`, slogMsg), template.Imports("log/slog"), template.SourceImports("log/slog")), - template.WithCaptures(slogMsg), -) - -func (r *UseStructuredLogging) RecipeList() []recipe.Recipe { - return []recipe.Recipe{useStructuredLoggingPrintln, useStructuredLoggingPrint} -} - func (r *UseStructuredLogging) Editor() recipe.TreeVisitor { return visitor.Init(&findStdLogVisitor{}) } @@ -64,34 +54,61 @@ type findStdLogVisitor struct { // that should be flagged. var stdLogPrefixes = []string{"Print", "Fatal"} -// stdLogAutoFixed lists method names that are auto-converted by the template -// sub-recipes (single-argument calls only). -var stdLogAutoFixed = map[string]bool{"Print": true, "Println": true} - func (v *findStdLogVisitor) VisitMethodInvocation(mi *java.MethodInvocation, p any) java.J { mi = v.GoVisitor.VisitMethodInvocation(mi, p).(*java.MethodInvocation) if mi.Select == nil { return mi } - ident, ok := mi.Select.Element.(*java.Identifier) if !ok || ident.Name != "log" { return mi } + // Auto-convert a single-argument log.Print/Println to slog.Info only when the + // argument is a string, since slog.Info takes a string message; a non-string + // argument falls through to the markup hint below. + if mi.Name.Name == "Print" || mi.Name.Name == "Println" { + if replaced := v.toSlogInfo(mi); replaced != nil { + return replaced + } + } + for _, prefix := range stdLogPrefixes { if strings.HasPrefix(mi.Name.Name, prefix) { - // Skip single-argument Print/Println — handled by template sub-recipes. - if stdLogAutoFixed[mi.Name.Name] && len(mi.Arguments.Elements) == 1 { - return mi - } - mi = mi.WithMarkers( + return mi.WithMarkers( java.MarkupInfo(mi.Markers, "consider migrating to log/slog for structured logging (Go 1.21+)"), ) - return mi } } - return mi } + +// toSlogInfo returns the slog.Info replacement for a single-argument log.Print / +// log.Println whose argument is a string, or nil when it does not apply. +func (v *findStdLogVisitor) toSlogInfo(mi *java.MethodInvocation) java.J { + var pat *template.GoPattern + switch mi.Name.Name { + case "Println": + pat = logPrintlnPattern + case "Print": + pat = logPrintPattern + default: + return nil + } + match := pat.Match(mi, nil) + if match == nil { + return nil + } + arg, ok := match.GetCapture(slogMsg).(java.Expression) + if !ok || !matcher.IsString(matcher.TypeOfExpression(arg)) { + return nil + } + replaced, ok := slogInfoTemplate.Apply(nil, match).(*java.MethodInvocation) + if !ok { + return nil + } + recipegolang.MaybeAddImport(v, "log/slog", nil, false) + v.DoAfterVisit(recipe.Service[*recipegolang.ImportService](nil).RemoveUnusedImportsVisitor()) + return replaced.WithPrefix(mi.GetPrefix()) +} diff --git a/recipes/style/check_template_execute_error.go b/recipes/style/check_template_execute_error.go index ed5ea37..283ba07 100644 --- a/recipes/style/check_template_execute_error.go +++ b/recipes/style/check_template_execute_error.go @@ -92,8 +92,8 @@ func isTemplateExecuteCall(mi *java.MethodInvocation) bool { return mi.Name.Name == "Execute" || mi.Name.Name == "ExecuteTemplate" } -// funcReturnsError returns true when the last return type of md is the -// identifier "error". +// Reports whether md returns a single error result, the only case where the +// synthesized `return err` compiles. func funcReturnsError(md *java.MethodDeclaration) bool { if md.ReturnType == nil { return false @@ -103,11 +103,10 @@ func funcReturnsError(md *java.MethodDeclaration) bool { return rt.Name == "error" case *golang.TypeList: types := rt.Types.Elements - if len(types) == 0 { + if len(types) != 1 { return false } - last := types[len(types)-1].Element - if vd, ok := last.(*java.VariableDeclarations); ok { + if vd, ok := types[0].Element.(*java.VariableDeclarations); ok { if ident, ok2 := vd.TypeExpr.(*java.Identifier); ok2 { return ident.Name == "error" } diff --git a/recipes/style/prefer_hex_encoding.go b/recipes/style/prefer_hex_encoding.go index c3732bd..176e03a 100644 --- a/recipes/style/prefer_hex_encoding.go +++ b/recipes/style/prefer_hex_encoding.go @@ -7,22 +7,25 @@ package style import ( "fmt" + "github.com/openrewrite/rewrite/rewrite-go/pkg/matcher" "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe" + recipegolang "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe/golang" "github.com/openrewrite/rewrite/rewrite-go/pkg/template" + "github.com/openrewrite/rewrite/rewrite-go/pkg/tree/java" + "github.com/openrewrite/rewrite/rewrite-go/pkg/visitor" ) var heData = template.Expr("heData") -var preferHexEncodingImpl = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.PreferHexEncoding$Impl"), - template.WithDisplayName("fmt.Sprintf(\"%x\", d) → hex.EncodeToString(d)"), - template.WithBefore(fmt.Sprintf(`fmt.Sprintf("%%x", %s)`, heData), template.Imports("fmt")), - template.WithAfter(fmt.Sprintf(`hex.EncodeToString(%s)`, heData), template.Imports("encoding/hex"), template.SourceImports("encoding/hex")), - template.WithCaptures(heData), +var ( + hexPattern = template.Expression(fmt.Sprintf(`fmt.Sprintf("%%x", %s)`, heData)). + Captures(heData).Imports("fmt").Build() + hexTemplate = template.ExpressionTemplate(fmt.Sprintf(`hex.EncodeToString(%s)`, heData)). + Captures(heData).Imports("encoding/hex").Build() ) -// PreferHexEncoding replaces `fmt.Sprintf("%x", data)` with -// `hex.EncodeToString(data)` for clearer intent and better performance. +// Replaces `fmt.Sprintf("%x", data)` with `hex.EncodeToString(data)` for +// clearer intent and better performance. type PreferHexEncoding struct { recipe.Base } @@ -38,6 +41,43 @@ func (r *PreferHexEncoding) Description() string { } func (r *PreferHexEncoding) Tags() []string { return []string{"style", "cleanup"} } -func (r *PreferHexEncoding) RecipeList() []recipe.Recipe { - return []recipe.Recipe{preferHexEncodingImpl} +func (r *PreferHexEncoding) Editor() recipe.TreeVisitor { + return visitor.Init(&preferHexEncodingVisitor{}) +} + +type preferHexEncodingVisitor struct { + visitor.GoVisitor +} + +func (v *preferHexEncodingVisitor) VisitMethodInvocation(mi *java.MethodInvocation, p any) java.J { + mi = v.GoVisitor.VisitMethodInvocation(mi, p).(*java.MethodInvocation) + + match := hexPattern.Match(mi, nil) + if match == nil { + return mi + } + + // Skip unless the argument is a []byte, since hex.EncodeToString takes a + // []byte while %x also accepts a string or an integer. + arg, ok := match.GetCapture(heData).(java.Expression) + if !ok || !isByteSlice(matcher.TypeOfExpression(arg)) { + return mi + } + + replaced, ok := hexTemplate.Apply(nil, match).(*java.MethodInvocation) + if !ok { + return mi + } + recipegolang.MaybeAddImport(v, "encoding/hex", nil, false) + v.DoAfterVisit(recipe.Service[*recipegolang.ImportService](nil).RemoveUnusedImportsVisitor()) + return replaced.WithPrefix(mi.GetPrefix()) +} + +// Reports whether t is a []byte (equivalently []uint8). +func isByteSlice(t java.JavaType) bool { + switch java.TypeSignature(t) { + case "byte[]", "uint8[]": + return true + } + return false } diff --git a/recipes/style/prefer_raw_string_regex.go b/recipes/style/prefer_raw_string_regex.go index 18064fd..14adf64 100644 --- a/recipes/style/prefer_raw_string_regex.go +++ b/recipes/style/prefer_raw_string_regex.go @@ -46,6 +46,20 @@ type preferRawStringRegexVisitor struct { visitor.GoVisitor } +// Reports whether the node at the cursor is the sole value of a single variable +// declaration or single-target assignment. +func inSingleValueContext(c *visitor.Cursor) bool { + parent := c.Parent() + if parent == nil { + return false + } + switch parent.Value().(type) { + case *java.VariableDeclarator, *java.Assignment: + return true + } + return false +} + func (v *preferRawStringRegexVisitor) VisitMethodInvocation(mi *java.MethodInvocation, p any) java.J { mi = v.GoVisitor.VisitMethodInvocation(mi, p).(*java.MethodInvocation) @@ -53,6 +67,12 @@ func (v *preferRawStringRegexVisitor) VisitMethodInvocation(mi *java.MethodInvoc return mi } + // Skip regexp.Compile forced into a single-value slot, where the surrounding + // two-value-in-single-value code already fails to compile. + if regexpCompileMatcher.Matches(mi) && inSingleValueContext(v.Cursor()) { + return mi + } + // Get the first argument. args := mi.Arguments.Elements if len(args) == 0 { @@ -88,6 +108,12 @@ func (v *preferRawStringRegexVisitor) VisitMethodInvocation(mi *java.MethodInvoc return mi } + // Bail on control characters, since a raw string would embed a literal + // newline or tab instead of the readable interpreted escape. + if strings.ContainsFunc(unquoted, func(r rune) bool { return r < 0x20 || r == 0x7f }) { + return mi + } + newSource := "`" + unquoted + "`" newLit := *lit newLit.Source = newSource diff --git a/recipes/style/prefer_strconv_quote.go b/recipes/style/prefer_strconv_quote.go index c355c5d..9d78af2 100644 --- a/recipes/style/prefer_strconv_quote.go +++ b/recipes/style/prefer_strconv_quote.go @@ -7,22 +7,25 @@ package style import ( "fmt" + "github.com/openrewrite/rewrite/rewrite-go/pkg/matcher" "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe" + recipegolang "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe/golang" "github.com/openrewrite/rewrite/rewrite-go/pkg/template" + "github.com/openrewrite/rewrite/rewrite-go/pkg/tree/java" + "github.com/openrewrite/rewrite/rewrite-go/pkg/visitor" ) var sqS = template.Expr("sqS") -var preferStrconvQuoteImpl = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.PreferStrconvQuote$Impl"), - template.WithDisplayName("fmt.Sprintf(\"%q\", s) → strconv.Quote(s)"), - template.WithBefore(fmt.Sprintf(`fmt.Sprintf("%%q", %s)`, sqS), template.Imports("fmt")), - template.WithAfter(fmt.Sprintf(`strconv.Quote(%s)`, sqS), template.Imports("strconv"), template.SourceImports("strconv")), - template.WithCaptures(sqS), +var ( + quotePattern = template.Expression(fmt.Sprintf(`fmt.Sprintf("%%q", %s)`, sqS)). + Captures(sqS).Imports("fmt").Build() + quoteTemplate = template.ExpressionTemplate(fmt.Sprintf(`strconv.Quote(%s)`, sqS)). + Captures(sqS).Imports("strconv").Build() ) -// PreferStrconvQuote replaces `fmt.Sprintf("%q", s)` with `strconv.Quote(s)` -// for clearer intent when quoting strings. +// Replaces `fmt.Sprintf("%q", s)` with `strconv.Quote(s)` for clearer intent +// when quoting strings. type PreferStrconvQuote struct { recipe.Base } @@ -38,6 +41,34 @@ func (r *PreferStrconvQuote) Description() string { } func (r *PreferStrconvQuote) Tags() []string { return []string{"style", "cleanup"} } -func (r *PreferStrconvQuote) RecipeList() []recipe.Recipe { - return []recipe.Recipe{preferStrconvQuoteImpl} +func (r *PreferStrconvQuote) Editor() recipe.TreeVisitor { + return visitor.Init(&preferStrconvQuoteVisitor{}) +} + +type preferStrconvQuoteVisitor struct { + visitor.GoVisitor +} + +func (v *preferStrconvQuoteVisitor) VisitMethodInvocation(mi *java.MethodInvocation, p any) java.J { + mi = v.GoVisitor.VisitMethodInvocation(mi, p).(*java.MethodInvocation) + + match := quotePattern.Match(mi, nil) + if match == nil { + return mi + } + + // Skip unless the argument is a string, since strconv.Quote takes a string + // while %q also accepts a rune or []byte. + arg, ok := match.GetCapture(sqS).(java.Expression) + if !ok || !matcher.IsString(matcher.TypeOfExpression(arg)) { + return mi + } + + replaced, ok := quoteTemplate.Apply(nil, match).(*java.MethodInvocation) + if !ok { + return mi + } + recipegolang.MaybeAddImport(v, "strconv", nil, false) + v.DoAfterVisit(recipe.Service[*recipegolang.ImportService](nil).RemoveUnusedImportsVisitor()) + return replaced.WithPrefix(mi.GetPrefix()) } diff --git a/recipes/style/reduce_error_check_nesting.go b/recipes/style/reduce_error_check_nesting.go index ccb3822..614c58e 100644 --- a/recipes/style/reduce_error_check_nesting.go +++ b/recipes/style/reduce_error_check_nesting.go @@ -5,6 +5,7 @@ package style 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" @@ -40,12 +41,23 @@ type reduceErrorCheckNestingVisitor struct { func (v *reduceErrorCheckNestingVisitor) VisitBlock(block *java.Block, p any) java.J { block = v.GoVisitor.VisitBlock(block, p).(*java.Block) + // Only rewrite the function's top-level body, and only when it returns a + // single error, so the synthesized `return err` compiles and an early return + // from a nested block does not change control flow. + if !lstutil.IsFunctionBodyBlock(v.Cursor()) { + return block + } + md, ok := visitor.FirstEnclosing[*java.MethodDeclaration](v.Cursor()) + if !ok || !funcReturnsError(md) { + return block + } + changed := false var newStmts []java.RightPadded[java.Statement] dedent := visitor.Init(&nestingDedentVisitor{}) - for _, rp := range block.Statements { + for i, rp := range block.Statements { // An `if init; cond` is a golang.StatementWithInit, not a *java.If, so the // assertion already excludes init-bearing ifs. ifStmt, ok := rp.Element.(*java.If) @@ -64,6 +76,13 @@ func (v *reduceErrorCheckNestingVisitor) VisitBlock(block *java.Block, p any) ja continue } + // Inverting to an early return is behaviour-preserving only when the `if` + // is the block's last statement. + if !isLastRealStatement(block.Statements, i) { + newStmts = append(newStmts, rp) + continue + } + changed = true // Build `if err != nil { return err }` @@ -87,18 +106,3 @@ func (v *reduceErrorCheckNestingVisitor) VisitBlock(block *java.Block, p any) ja } return block.WithStatements(newStmts) } - -// isErrNotNil returns true if the 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" -} diff --git a/recipes/style/reduce_nesting_depth.go b/recipes/style/reduce_nesting_depth.go index 8df2906..a78880a 100644 --- a/recipes/style/reduce_nesting_depth.go +++ b/recipes/style/reduce_nesting_depth.go @@ -7,7 +7,9 @@ package style import ( "strings" + "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" ) @@ -41,12 +43,18 @@ type reduceNestingDepthVisitor struct { func (v *reduceNestingDepthVisitor) VisitBlock(block *java.Block, p any) java.J { block = v.GoVisitor.VisitBlock(block, p).(*java.Block) + // Only rewrite the function's top-level body, where falling through the `if` + // reaches the function end so the synthesised `return` preserves behaviour. + if !lstutil.IsFunctionBodyBlock(v.Cursor()) { + return block + } + changed := false var newStmts []java.RightPadded[java.Statement] dedent := visitor.Init(&nestingDedentVisitor{}) - for _, rp := range block.Statements { + for i, rp := range block.Statements { // An `if init; cond` is a golang.StatementWithInit, not a *java.If, so the // assertion already excludes init-bearing ifs. ifStmt, ok := rp.Element.(*java.If) @@ -54,6 +62,13 @@ func (v *reduceNestingDepthVisitor) VisitBlock(block *java.Block, p any) java.J newStmts = append(newStmts, rp) continue } + + // Only invert when the `if` is the block's last statement, so the guard's + // `return` does not skip statements that ran after it on the err != nil path. + if !isLastRealStatement(block.Statements, i) { + newStmts = append(newStmts, rp) + continue + } thenBlock, ok := ifStmt.ThenPart.Element.(*java.Block) if !ok { newStmts = append(newStmts, rp) @@ -65,6 +80,13 @@ func (v *reduceNestingDepthVisitor) VisitBlock(block *java.Block, p any) java.J continue } + // Skip unless a bare `return` is legal here, since a function with unnamed + // results would need return values ("not enough return values"). + if !bareReturnLegal(v.Cursor()) { + newStmts = append(newStmts, rp) + continue + } + changed = true // Build `if err != nil { return }` @@ -88,6 +110,56 @@ func (v *reduceNestingDepthVisitor) VisitBlock(block *java.Block, p any) java.J return block.WithStatements(newStmts) } +// Reports whether a bare `return` is legal in the enclosing function, true for +// no results or all-named results and false when the function cannot be resolved. +func bareReturnLegal(c *visitor.Cursor) bool { + md, ok := visitor.FirstEnclosing[*java.MethodDeclaration](c) + if !ok { + return false + } + if md.ReturnType == nil { + return true + } + // A single unnamed result is not a TypeList, so a bare return is illegal. + tl, ok := md.ReturnType.(*golang.TypeList) + if !ok { + return false + } + for _, e := range tl.Types.Elements { + vd, ok := e.Element.(*java.VariableDeclarations) + if !ok || !allNamed(vd) { + return false + } + } + return true +} + +// Reports whether a result-list entry declares a name (`err error`) rather than +// being a bare type (`error`). +func allNamed(vd *java.VariableDeclarations) bool { + if len(vd.Variables) == 0 { + return false + } + for _, dv := range vd.Variables { + d := dv.Element + if d == nil || d.Name == nil || d.Name.Name == "" { + return false + } + } + return true +} + +// Reports whether index i is the last non-empty statement in stmts (trailing +// *java.Empty entries, e.g. stray semicolons, are ignored). +func isLastRealStatement(stmts []java.RightPadded[java.Statement], i int) bool { + for _, rp := range stmts[i+1:] { + if _, isEmpty := rp.Element.(*java.Empty); !isEmpty { + return false + } + } + return true +} + // isErrEqualNil returns true if the expression is `err == nil`. func isErrEqualNil(expr java.Expression) bool { bin, ok := expr.(*java.Binary) @@ -128,7 +200,7 @@ func buildErrGuard(ifStmt *java.If, returnExpr java.Expression) *java.If { Statements: []java.RightPadded[java.Statement]{ {Element: ret}, }, - End: java.Space{Whitespace: "\n" + baseIndent(ifStmt.Prefix)}, + End: java.Space{Whitespace: "\n" + lstutil.BaseIndent(ifStmt.Prefix)}, } return &java.If{ @@ -138,19 +210,9 @@ func buildErrGuard(ifStmt *java.If, returnExpr java.Expression) *java.If { } } -// baseIndent extracts the indentation (everything after the last newline) -// from a Space's Whitespace field. -func baseIndent(space java.Space) string { - ws := space.Whitespace - if idx := strings.LastIndex(ws, "\n"); idx >= 0 { - return ws[idx+1:] - } - return ws -} - // guardIndent returns one extra tab level of indentation for the guard body. func guardIndent(space java.Space) string { - return baseIndent(space) + "\t" + return lstutil.BaseIndent(space) + "\t" } // nestingDedentVisitor removes one tab from every whitespace in a subtree, diff --git a/recipes/style/use_strong_hash.go b/recipes/style/use_strong_hash.go index 21b17b6..9f2b09b 100644 --- a/recipes/style/use_strong_hash.go +++ b/recipes/style/use_strong_hash.go @@ -5,17 +5,14 @@ package style import ( - "fmt" - "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe" "github.com/openrewrite/rewrite/rewrite-go/pkg/template" ) -var shData = template.Expr("shData") - -// UseStrongHash replaces weak hash functions (md5, sha1) with sha256 equivalents. -// `md5.New()` and `sha1.New()` become `sha256.New()`. -// `md5.Sum(d)` and `sha1.Sum(d)` become `sha256.Sum256(d)`. +// Replaces the weak hash constructors md5.New() and sha1.New() with +// sha256.New(), leaving md5.Sum/sha1.Sum alone since their [16]byte/[20]byte +// results differ from sha256.Sum256's [32]byte and would need a whole-usage +// migration. type UseStrongHash struct { recipe.Base } @@ -25,7 +22,7 @@ func (r *UseStrongHash) Name() string { } func (r *UseStrongHash) DisplayName() string { return "Use strong hash functions" } func (r *UseStrongHash) Description() string { - return "Replace weak hash functions (md5, sha1) with SHA-256 equivalents." + return "Replace weak hash constructors (md5.New, sha1.New) with sha256.New." } func (r *UseStrongHash) Tags() []string { return []string{"style", "security"} } @@ -36,14 +33,6 @@ var useStrongHashMd5New = template.NewRecipe( template.WithAfter(`sha256.New()`, template.Imports("crypto/sha256"), template.SourceImports("crypto/sha256")), ) -var useStrongHashMd5Sum = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.UseStrongHash$Md5Sum"), - template.WithDisplayName("md5.Sum(d) -> sha256.Sum256(d)"), - template.WithBefore(fmt.Sprintf(`md5.Sum(%s)`, shData), template.Imports("crypto/md5")), - template.WithAfter(fmt.Sprintf(`sha256.Sum256(%s)`, shData), template.Imports("crypto/sha256"), template.SourceImports("crypto/sha256")), - template.WithCaptures(shData), -) - var useStrongHashSha1New = template.NewRecipe( template.RecipeName("org.openrewrite.golang.codequality.UseStrongHash$Sha1New"), template.WithDisplayName("sha1.New() -> sha256.New()"), @@ -51,19 +40,9 @@ var useStrongHashSha1New = template.NewRecipe( template.WithAfter(`sha256.New()`, template.Imports("crypto/sha256"), template.SourceImports("crypto/sha256")), ) -var useStrongHashSha1Sum = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.UseStrongHash$Sha1Sum"), - template.WithDisplayName("sha1.Sum(d) -> sha256.Sum256(d)"), - template.WithBefore(fmt.Sprintf(`sha1.Sum(%s)`, shData), template.Imports("crypto/sha1")), - template.WithAfter(fmt.Sprintf(`sha256.Sum256(%s)`, shData), template.Imports("crypto/sha256"), template.SourceImports("crypto/sha256")), - template.WithCaptures(shData), -) - func (r *UseStrongHash) RecipeList() []recipe.Recipe { return []recipe.Recipe{ useStrongHashMd5New, - useStrongHashMd5Sum, useStrongHashSha1New, - useStrongHashSha1Sum, } } diff --git a/tests/errorhandling/check_close_error_test.go b/tests/errorhandling/check_close_error_test.go index aba06a0..938a0ec 100644 --- a/tests/errorhandling/check_close_error_test.go +++ b/tests/errorhandling/check_close_error_test.go @@ -75,3 +75,21 @@ func TestCheckCloseErrorNoChangeRead(t *testing.T) { `), ) } + +// Skips a void Close(), where `_ = t.Close()` would not compile. +func TestCheckCloseErrorNoChangeVoidClose(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&errorhandling.CheckCloseError{}) + spec.RewriteRun(t, + test.Golang(` + package main + + type T struct{} + + func (T) Close() {} + + func f(t T) { + t.Close() + } + `), + ) +} diff --git a/tests/errorhandling/handle_deferred_close_error_test.go b/tests/errorhandling/handle_deferred_close_error_test.go index 94df8ff..41f8f6d 100644 --- a/tests/errorhandling/handle_deferred_close_error_test.go +++ b/tests/errorhandling/handle_deferred_close_error_test.go @@ -52,3 +52,21 @@ func TestHandleDeferredCloseErrorNoChangeDone(t *testing.T) { `), ) } + +// Skips a void Close(), where `_ = t.Close()` inside the closure would not compile. +func TestHandleDeferredCloseErrorNoChangeVoidClose(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&errorhandling.HandleDeferredCloseError{}) + spec.RewriteRun(t, + test.Golang(` + package main + + type T struct{} + + func (T) Close() {} + + func f(t T) { + defer t.Close() + } + `), + ) +} diff --git a/tests/errorhandling/handle_error_return_test.go b/tests/errorhandling/handle_error_return_test.go index 4e7a6fc..f27e165 100644 --- a/tests/errorhandling/handle_error_return_test.go +++ b/tests/errorhandling/handle_error_return_test.go @@ -19,16 +19,91 @@ func TestHandleErrorReturnDiscarded(t *testing.T) { import "os" - func main() { - _, _ = os.Open("file") + func f() error { + file, _ := os.Open("file") + _ = file + return nil } `, ` package main import "os" + func f() error { + file, err := os.Open("file") + if err != nil { + return err + } + _ = file + return nil + } + `), + ) +} + +// Skips a plain `=` assignment in main(), where `err` is undeclared and no error return exists. +func TestHandleErrorReturnNoChangeUndeclaredErr(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&errorhandling.HandleErrorReturn{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import "os" + func main() { - _, err = os.Open("file") + _, _ = os.Open("file") + } + `), + ) +} + +// Skips the comma-ok map access, where the discarded value is a bool rather than an error. +func TestHandleErrorReturnNoChangeCommaOkMap(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&errorhandling.HandleErrorReturn{}) + spec.RewriteRun(t, + test.Golang(` + package main + + func f(m map[string]int) error { + v, _ := m["k"] + _ = v + return nil + } + `), + ) +} + +// Skips the comma-ok type assertion, where the discarded value is a bool. +func TestHandleErrorReturnNoChangeCommaOkTypeAssert(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&errorhandling.HandleErrorReturn{}) + spec.RewriteRun(t, + test.Golang(` + package main + + func f(i interface{}) error { + s, _ := i.(string) + _ = s + return nil + } + `), + ) +} + +// Skips a capture in a loop body, where the inserted `return err` would change control flow. +func TestHandleErrorReturnNoChangeInsideLoop(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&errorhandling.HandleErrorReturn{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import "os" + + func f(names []string) error { + for _, n := range names { + file, _ := os.Open(n) + _ = file + } + return nil } `), ) diff --git a/tests/errorhandling/prefer_errors_is_context_test.go b/tests/errorhandling/prefer_errors_is_context_test.go index e3054a7..d56245b 100644 --- a/tests/errorhandling/prefer_errors_is_context_test.go +++ b/tests/errorhandling/prefer_errors_is_context_test.go @@ -127,3 +127,19 @@ func TestPreferErrorsIsContextNoChangeNilCheck(t *testing.T) { `), ) } + +// Skips a non-error (any) operand, where errors.Is would not compile. +func TestPreferErrorsIsContextNoChangeNonError(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&errorhandling.PreferErrorsIsContext{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import "context" + + func f(x any) bool { + return x == context.Canceled + } + `), + ) +} diff --git a/tests/errorhandling/prefer_errors_is_eof_test.go b/tests/errorhandling/prefer_errors_is_eof_test.go index 14f34dc..db19277 100644 --- a/tests/errorhandling/prefer_errors_is_eof_test.go +++ b/tests/errorhandling/prefer_errors_is_eof_test.go @@ -75,3 +75,19 @@ func TestPreferErrorsIsEOFNoChangeNil(t *testing.T) { `), ) } + +// Skips a non-error (any) operand, where errors.Is would not compile. +func TestPreferErrorsIsEOFNoChangeNonError(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&errorhandling.PreferErrorsIsEOF{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import "io" + + func f(x any) bool { + return x == io.EOF + } + `), + ) +} diff --git a/tests/errorhandling/prefer_errors_is_for_field_access_test.go b/tests/errorhandling/prefer_errors_is_for_field_access_test.go index d9dade1..d8363fc 100644 --- a/tests/errorhandling/prefer_errors_is_for_field_access_test.go +++ b/tests/errorhandling/prefer_errors_is_for_field_access_test.go @@ -114,3 +114,19 @@ func TestPreferErrorsIsForFieldAccessNoChangeNonSentinel(t *testing.T) { `), ) } + +// Skips a non-error field comparison that only matched by an Err* field name. +func TestPreferErrorsIsForFieldAccessNoChangeNonError(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&errorhandling.PreferErrorsIsForFieldAccess{}) + spec.RewriteRun(t, + test.Golang(` + package main + + type C struct{ ErrThreshold int } + + func f(c C, n int) bool { + return n == c.ErrThreshold + } + `), + ) +} diff --git a/tests/errorhandling/prefer_errors_is_http_test.go b/tests/errorhandling/prefer_errors_is_http_test.go index ff026ee..4afb742 100644 --- a/tests/errorhandling/prefer_errors_is_http_test.go +++ b/tests/errorhandling/prefer_errors_is_http_test.go @@ -75,3 +75,19 @@ func TestPreferErrorsIsHttpServerClosedNoChangeNil(t *testing.T) { `), ) } + +// Skips a non-error (any) operand, where errors.Is would not compile. +func TestPreferErrorsIsHttpServerClosedNoChangeNonError(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&errorhandling.PreferErrorsIsHttpServerClosed{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import "net/http" + + func f(x any) bool { + return x == http.ErrServerClosed + } + `), + ) +} diff --git a/tests/errorhandling/prefer_errors_is_net_test.go b/tests/errorhandling/prefer_errors_is_net_test.go index 25d4775..ce39183 100644 --- a/tests/errorhandling/prefer_errors_is_net_test.go +++ b/tests/errorhandling/prefer_errors_is_net_test.go @@ -75,3 +75,19 @@ func TestPreferErrorsIsNetClosedNoChangeNil(t *testing.T) { `), ) } + +// Skips a non-error (any) operand, where errors.Is would not compile. +func TestPreferErrorsIsNetClosedNoChangeNonError(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&errorhandling.PreferErrorsIsNetClosed{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import "net" + + func f(x any) bool { + return x == net.ErrClosed + } + `), + ) +} diff --git a/tests/errorhandling/prefer_errors_is_os_path_test.go b/tests/errorhandling/prefer_errors_is_os_path_test.go index f9527db..647311f 100644 --- a/tests/errorhandling/prefer_errors_is_os_path_test.go +++ b/tests/errorhandling/prefer_errors_is_os_path_test.go @@ -49,3 +49,19 @@ func TestPreferErrorsIsOsInvalidNoChangeNil(t *testing.T) { `), ) } + +// Skips a non-error (any) operand, where errors.Is would not compile. +func TestPreferErrorsIsOsInvalidNoChangeNonError(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&errorhandling.PreferErrorsIsOsInvalid{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import "os" + + func f(x any) bool { + return x == os.ErrInvalid + } + `), + ) +} diff --git a/tests/errorhandling/prefer_errors_is_sql_test.go b/tests/errorhandling/prefer_errors_is_sql_test.go index 97065f6..4518381 100644 --- a/tests/errorhandling/prefer_errors_is_sql_test.go +++ b/tests/errorhandling/prefer_errors_is_sql_test.go @@ -75,3 +75,19 @@ func TestPreferErrorsIsSqlNoRowsNoChangeNil(t *testing.T) { `), ) } + +// Skips a non-error (any) operand, where errors.Is would not compile. +func TestPreferErrorsIsSqlNoRowsNoChangeNonError(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&errorhandling.PreferErrorsIsSqlNoRows{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import "database/sql" + + func f(x any) bool { + return x == sql.ErrNoRows + } + `), + ) +} diff --git a/tests/errorhandling/prefer_errors_is_test.go b/tests/errorhandling/prefer_errors_is_test.go index 206a0e5..090d14a 100644 --- a/tests/errorhandling/prefer_errors_is_test.go +++ b/tests/errorhandling/prefer_errors_is_test.go @@ -116,3 +116,19 @@ func TestPreferErrorsIsNoChangeNonError(t *testing.T) { `), ) } + +// Skips a comparison that only matched by an Err* name but is an int constant. +func TestPreferErrorsIsNoChangeIntErrConst(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&errorhandling.PreferErrorsIsOverEquality{}) + spec.RewriteRun(t, + test.Golang(` + package main + + const ErrLevel = 3 + + func f(x int) bool { + return x == ErrLevel + } + `), + ) +} diff --git a/tests/errorhandling/prefer_errors_join_test.go b/tests/errorhandling/prefer_errors_join_test.go index 84aa3e1..668d6ce 100644 --- a/tests/errorhandling/prefer_errors_join_test.go +++ b/tests/errorhandling/prefer_errors_join_test.go @@ -46,3 +46,20 @@ func TestSimplifyRedundantErrorWrapNoChangeWithContext(t *testing.T) { `), ) } + +// Skips a non-error (any) argument, where replacing fmt.Errorf with the bare +// value would not compile. +func TestSimplifyRedundantErrorWrapNoChangeNonError(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&errorhandling.SimplifyRedundantErrorWrap{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import "fmt" + + func f(x any) error { + return fmt.Errorf("%w", x) + } + `), + ) +} diff --git a/tests/errorhandling/use_error_method_test.go b/tests/errorhandling/use_error_method_test.go index 1298f8e..da6e49d 100644 --- a/tests/errorhandling/use_error_method_test.go +++ b/tests/errorhandling/use_error_method_test.go @@ -46,3 +46,19 @@ func TestUseErrorMethodNoChangeInt(t *testing.T) { `), ) } + +// Skips a non-error value named err, since err.Error() requires the error interface. +func TestUseErrorMethodNoChangeNonError(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&errorhandling.UseErrorMethod{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import "fmt" + + func f(err any) string { + return fmt.Sprint(err) + } + `), + ) +} diff --git a/tests/errorhandling/use_errors_as_test.go b/tests/errorhandling/use_errors_as_test.go index dc83951..c040c61 100644 --- a/tests/errorhandling/use_errors_as_test.go +++ b/tests/errorhandling/use_errors_as_test.go @@ -74,3 +74,23 @@ func TestUseErrorsAsNoChangeNoInit(t *testing.T) { `), ) } + +// Skips an assertion on an any-typed value, since errors.As needs an error argument. +func TestUseErrorsAsNoChangeNonError(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&errorhandling.UseErrorsAs{}) + spec.RewriteRun(t, + test.Golang(` + package main + + type MyError struct{} + + func (e *MyError) Error() string { return "x" } + + func f(err any) { + if e, ok := err.(*MyError); ok { + _ = e + } + } + `), + ) +} diff --git a/tests/errorhandling/wrap_error_with_context_test.go b/tests/errorhandling/wrap_error_with_context_test.go index 2f06c83..d195946 100644 --- a/tests/errorhandling/wrap_error_with_context_test.go +++ b/tests/errorhandling/wrap_error_with_context_test.go @@ -83,3 +83,23 @@ func TestWrapErrorWithContextNoChangeMultiReturn(t *testing.T) { `), ) } + +// Skips a function returning a concrete error type, where `return fmt.Errorf(...)` +// (which yields error) would not compile. +func TestWrapErrorWithContextNoChangeConcreteReturn(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&errorhandling.WrapErrorWithContext{}) + spec.RewriteRun(t, + test.Golang(` + package main + + type MyErr struct{} + + func (*MyErr) Error() string { return "" } + + func g() *MyErr { + err := &MyErr{} + return err + } + `), + ) +} diff --git a/tests/performance/prefer_strconv_format_bool_test.go b/tests/performance/prefer_strconv_format_bool_test.go index 059b1aa..a37ced4 100644 --- a/tests/performance/prefer_strconv_format_bool_test.go +++ b/tests/performance/prefer_strconv_format_bool_test.go @@ -65,3 +65,19 @@ func TestPreferStrconvFormatBoolNoChangeMultipleArgs(t *testing.T) { `), ) } + +// Skips a non-bool argument, since strconv.FormatBool needs a bool. +func TestPreferStrconvFormatBoolNoChangeNonBool(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&performance.PreferStrconvFormatBool{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import "fmt" + + func f(x int) string { + return fmt.Sprintf("%t", x) + } + `), + ) +} diff --git a/tests/performance/use_strings_builder_in_loop_test.go b/tests/performance/use_strings_builder_in_loop_test.go index 0a11bec..7e67c96 100644 --- a/tests/performance/use_strings_builder_in_loop_test.go +++ b/tests/performance/use_strings_builder_in_loop_test.go @@ -87,3 +87,21 @@ func TestStringConcatNoChangeOutsideLoop(t *testing.T) { `), ) } + +// Skips a numeric accumulator, where builder.WriteString(number) would not compile. +func TestStringConcatNoChangeNumeric(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&performance.UseStringsBuilderInLoop{}) + spec.RewriteRun(t, + test.Golang(` + package main + + func total(xs []int) int { + s := 0 + for _, x := range xs { + s += x + } + return s + } + `), + ) +} diff --git a/tests/redundancy/remove_redundant_sprintf_test.go b/tests/redundancy/remove_redundant_sprintf_test.go index a0b5303..3d711d0 100644 --- a/tests/redundancy/remove_redundant_sprintf_test.go +++ b/tests/redundancy/remove_redundant_sprintf_test.go @@ -61,3 +61,19 @@ func TestRemoveRedundantSprintfNoChangeFormatD(t *testing.T) { `), ) } + +// Skips a []byte argument, since %s accepts it but the bare value is not a string. +func TestRemoveRedundantSprintfNoChangeBytes(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&redundancy.RemoveRedundantSprintf{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import "fmt" + + func f(b []byte) string { + return fmt.Sprintf("%s", b) + } + `), + ) +} diff --git a/tests/redundancy/simplify_goroutine_closure_test.go b/tests/redundancy/simplify_goroutine_closure_test.go index d7096ea..d0967aa 100644 --- a/tests/redundancy/simplify_goroutine_closure_test.go +++ b/tests/redundancy/simplify_goroutine_closure_test.go @@ -67,3 +67,22 @@ func TestSimplifyGoroutineClosureNoChangeDirectCall(t *testing.T) { `), ) } + +// Skips a closure with parameters, where dropping them would leave the inner +// call referencing an out-of-scope name. +func TestSimplifyGoroutineClosureNoChangeWithParams(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&redundancy.SimplifyGoroutineClosure{}) + spec.RewriteRun(t, + test.Golang(` + package main + + func process(x int) {} + + func f() { + go func(n int) { + process(n) + }(5) + } + `), + ) +} diff --git a/tests/simplification/prefer_empty_string_check_test.go b/tests/simplification/prefer_empty_string_check_test.go index 9436435..fd779d3 100644 --- a/tests/simplification/prefer_empty_string_check_test.go +++ b/tests/simplification/prefer_empty_string_check_test.go @@ -48,3 +48,17 @@ func TestPreferEmptyStringCheckNotEqual(t *testing.T) { `), ) } + +// Skips a []byte argument, since `== ""` requires a string while len accepts slices. +func TestPreferEmptyStringCheckNoChangeBytes(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&simplification.PreferEmptyStringCheck{}) + spec.RewriteRun(t, + test.Golang(` + package main + + func f(b []byte) bool { + return len(b) == 0 + } + `), + ) +} diff --git a/tests/simplification/prefer_io_writestring_test.go b/tests/simplification/prefer_io_writestring_test.go index 4686184..c37930a 100644 --- a/tests/simplification/prefer_io_writestring_test.go +++ b/tests/simplification/prefer_io_writestring_test.go @@ -57,3 +57,22 @@ func TestPreferIoWriteStringNoChange(t *testing.T) { `), ) } + +// Skips a []byte argument, since io.WriteString takes a string. +func TestPreferIoWriteStringNoChangeBytes(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&simplification.PreferIoWriteString{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import ( + "fmt" + "io" + ) + + func f(w io.Writer, b []byte) { + fmt.Fprintf(w, "%s", b) + } + `), + ) +} diff --git a/tests/simplification/prefer_os_readdir_test.go b/tests/simplification/prefer_os_readdir_test.go index 3ac668a..61c4514 100644 --- a/tests/simplification/prefer_os_readdir_test.go +++ b/tests/simplification/prefer_os_readdir_test.go @@ -19,8 +19,9 @@ func TestPreferOsReadDir(t *testing.T) { import "io/ioutil" - func f(name string) ([]os.FileInfo, error) { - return ioutil.ReadDir(name) + func f(name string) { + entries, _ := ioutil.ReadDir(name) + _ = entries } `, ` package main @@ -29,8 +30,9 @@ func TestPreferOsReadDir(t *testing.T) { "os" ) - func f(name string) ([]os.FileInfo, error) { - return os.ReadDir(name) + func f(name string) { + entries, _ := os.ReadDir(name) + _ = entries } `), ) @@ -51,3 +53,22 @@ func TestPreferOsReadDirNoChange(t *testing.T) { `), ) } + +// Skips a direct return of []os.FileInfo, where os.ReadDir's []os.DirEntry would not compile. +func TestPreferOsReadDirNoChangeFileInfoContext(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&simplification.PreferOsReadDir{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import ( + "io/ioutil" + "os" + ) + + func f(name string) ([]os.FileInfo, error) { + return ioutil.ReadDir(name) + } + `), + ) +} diff --git a/tests/simplification/prefer_strconv_atoi_test.go b/tests/simplification/prefer_strconv_atoi_test.go index 76ce137..9e9b5c0 100644 --- a/tests/simplification/prefer_strconv_atoi_test.go +++ b/tests/simplification/prefer_strconv_atoi_test.go @@ -19,16 +19,70 @@ func TestPreferStrconvAtoi(t *testing.T) { import "strconv" - func f(s string) (int64, error) { - return strconv.ParseInt(s, 10, 0) + func f(s string) int { + n, _ := strconv.ParseInt(s, 10, 0) + return int(n) } `, ` package main import "strconv" - func f(s string) (int64, error) { - return strconv.Atoi(s) + func f(s string) int { + n, _ := strconv.Atoi(s) + return int(n) + } + `), + ) +} + +// Skips the capture-then-return pattern where n is returned as int64. +func TestPreferStrconvAtoiNoChangeCapturedReturnedInt64(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&simplification.PreferStrconvAtoi{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import "strconv" + + func parse(s string) (int64, error) { + n, err := strconv.ParseInt(s, 10, 0) + if err != nil { + return 0, err + } + return n, nil + } + `), + ) +} + +// Rewrites when the captured value is only used through an int() conversion. +func TestPreferStrconvAtoiCapturedConvertedToInt(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&simplification.PreferStrconvAtoi{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import "strconv" + + func parse(s string) (int, error) { + n, err := strconv.ParseInt(s, 10, 0) + if err != nil { + return 0, err + } + return int(n), nil + } + `, ` + package main + + import "strconv" + + func parse(s string) (int, error) { + n, err := strconv.Atoi(s) + if err != nil { + return 0, err + } + return int(n), nil } `), ) @@ -63,3 +117,19 @@ func TestPreferStrconvAtoiNoChangeBitSize(t *testing.T) { `), ) } + +// Skips a direct return of the int64 result, where strconv.Atoi's int would not compile. +func TestPreferStrconvAtoiNoChangeInt64Context(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&simplification.PreferStrconvAtoi{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import "strconv" + + func f(s string) (int64, error) { + return strconv.ParseInt(s, 10, 0) + } + `), + ) +} diff --git a/tests/simplification/prefer_strings_builder_writestring_test.go b/tests/simplification/prefer_strings_builder_writestring_test.go index e9c1370..f4a83be 100644 --- a/tests/simplification/prefer_strings_builder_writestring_test.go +++ b/tests/simplification/prefer_strings_builder_writestring_test.go @@ -62,3 +62,24 @@ func TestPreferStringsBuilderWriteStringNoChangeFormat(t *testing.T) { `), ) } + +// Skips a []byte argument, since Builder.WriteString takes a string. +func TestPreferStringsBuilderWriteStringNoChangeBytes(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&simplification.PreferStringsBuilderWriteString{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import ( + "fmt" + "strings" + ) + + func f(b []byte) { + var sb strings.Builder + fmt.Fprintf(&sb, "%s", b) + _ = sb + } + `), + ) +} diff --git a/tests/simplification/prefer_strings_newreader_test.go b/tests/simplification/prefer_strings_newreader_test.go index a67b88d..777c38a 100644 --- a/tests/simplification/prefer_strings_newreader_test.go +++ b/tests/simplification/prefer_strings_newreader_test.go @@ -17,26 +17,125 @@ func TestPreferStringsNewReader(t *testing.T) { test.Golang(` package main - import "bytes" + import ( + "bytes" + "io" + ) - func f(s string) *bytes.Reader { + func f(s string) io.Reader { return bytes.NewReader([]byte(s)) } `, ` package main import ( - "bytes" + "io" "strings" ) - func f(s string) *bytes.Reader { + func f(s string) io.Reader { return strings.NewReader(s) } `), ) } +// Skips a []byte argument, where strings.NewReader's string parameter would not compile. +func TestPreferStringsNewReaderNoChangeByteSliceArg(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&simplification.PreferStringsNewReader{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import "bytes" + + func f(b []byte) { + r := bytes.NewReader([]byte(b)) + _ = r + } + `), + ) +} + +// A string literal is a string, so the rewrite still proceeds. +func TestPreferStringsNewReaderStringLiteralArg(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&simplification.PreferStringsNewReader{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import ( + "bytes" + "io" + ) + + func f() io.Reader { + return bytes.NewReader([]byte("hello")) + } + `, ` + package main + + import ( + "io" + "strings" + ) + + func f() io.Reader { + return strings.NewReader("hello") + } + `), + ) +} + +// Skips a *bytes.Reader variable declaration, which *strings.Reader would not satisfy. +func TestPreferStringsNewReaderNoChangeTypedVarDecl(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&simplification.PreferStringsNewReader{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import "bytes" + + func f(s string) { + var r *bytes.Reader = bytes.NewReader([]byte(s)) + _ = r + } + `), + ) +} + +// An interface-typed declaration accepts both readers, so the rewrite proceeds. +func TestPreferStringsNewReaderInterfaceVarDecl(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&simplification.PreferStringsNewReader{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import ( + "bytes" + "io" + ) + + func f(s string) { + var r io.Reader = bytes.NewReader([]byte(s)) + _ = r + } + `, ` + package main + + import ( + "io" + "strings" + ) + + func f(s string) { + var r io.Reader = strings.NewReader(s) + _ = r + } + `), + ) +} + func TestPreferStringsNewReaderNoChange(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&simplification.PreferStringsNewReader{}) spec.RewriteRun(t, @@ -51,3 +150,19 @@ func TestPreferStringsNewReaderNoChange(t *testing.T) { `), ) } + +// Skips a direct return of *bytes.Reader, where *strings.Reader would not compile. +func TestPreferStringsNewReaderNoChangeBytesReaderContext(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&simplification.PreferStringsNewReader{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import "bytes" + + func f(s string) *bytes.Reader { + return bytes.NewReader([]byte(s)) + } + `), + ) +} diff --git a/tests/simplification/prefer_strings_repeat_test.go b/tests/simplification/prefer_strings_repeat_test.go index 1ea749a..afc5ae9 100644 --- a/tests/simplification/prefer_strings_repeat_test.go +++ b/tests/simplification/prefer_strings_repeat_test.go @@ -46,3 +46,19 @@ func TestSimplifySprintfConcatNoChangeFormat(t *testing.T) { `), ) } + +// Skips []byte arguments, since %s accepts them but []byte values cannot be added. +func TestSimplifySprintfConcatNoChangeBytes(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&simplification.SimplifySprintfConcat{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import "fmt" + + func f(a, b []byte) string { + return fmt.Sprintf("%s%s", a, b) + } + `), + ) +} diff --git a/tests/simplification/use_structured_logging_test.go b/tests/simplification/use_structured_logging_test.go index 7aa9514..3b10457 100644 --- a/tests/simplification/use_structured_logging_test.go +++ b/tests/simplification/use_structured_logging_test.go @@ -96,3 +96,28 @@ func TestUseStructuredLoggingNoChangeFmt(t *testing.T) { `), ) } + +// A non-string argument is not auto-converted to slog.Info (which needs a string +// message); it is flagged for manual migration instead. +func TestUseStructuredLoggingNoAutoConvertNonString(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&simplification.UseStructuredLogging{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import "log" + + func f(err error) { + log.Println(err) + } + `, ` + package main + + import "log" + + func f(err error) { + /*~~(consider migrating to log/slog for structured logging (Go 1.21+))~~>*/log.Println(err) + } + `), + ) +} diff --git a/tests/style/check_template_execute_error_test.go b/tests/style/check_template_execute_error_test.go index a55d236..5da8d6c 100644 --- a/tests/style/check_template_execute_error_test.go +++ b/tests/style/check_template_execute_error_test.go @@ -109,3 +109,24 @@ func TestCheckTemplateExecuteErrorNoChangeNoErrorReturn(t *testing.T) { `), ) } + +// Skips a multi-value result like (int, error), where the synthesized bare +// `return err` would not compile. +func TestCheckTemplateExecuteErrorNoChangeMultiReturn(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&style.CheckTemplateExecuteError{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import ( + "html/template" + "io" + ) + + func f(tmpl *template.Template, w io.Writer) (int, error) { + tmpl.Execute(w, nil) + return 0, nil + } + `), + ) +} diff --git a/tests/style/prefer_hex_encoding_test.go b/tests/style/prefer_hex_encoding_test.go index c114c0b..1f7325c 100644 --- a/tests/style/prefer_hex_encoding_test.go +++ b/tests/style/prefer_hex_encoding_test.go @@ -50,3 +50,19 @@ func TestPreferHexEncodingNoChangeOtherVerb(t *testing.T) { `), ) } + +// Skips a string argument, since %x accepts it but hex.EncodeToString needs []byte. +func TestPreferHexEncodingNoChangeString(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&style.PreferHexEncoding{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import "fmt" + + func f(s string) string { + return fmt.Sprintf("%x", s) + } + `), + ) +} diff --git a/tests/style/prefer_raw_string_regex_test.go b/tests/style/prefer_raw_string_regex_test.go index 76633c4..c721377 100644 --- a/tests/style/prefer_raw_string_regex_test.go +++ b/tests/style/prefer_raw_string_regex_test.go @@ -15,8 +15,8 @@ func TestPreferRawStringRegexCompile(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&style.PreferRawStringForRegex{}) spec.RewriteRun(t, test.Golang( - "package main\n\nimport \"regexp\"\n\nvar r = regexp.Compile(\"\\\\d+\")\n", - "package main\n\nimport \"regexp\"\n\nvar r = regexp.Compile(`\\d+`)\n", + "package main\n\nimport \"regexp\"\n\nfunc f() *regexp.Regexp {\n\tr, _ := regexp.Compile(\"\\\\d+\")\n\treturn r\n}\n", + "package main\n\nimport \"regexp\"\n\nfunc f() *regexp.Regexp {\n\tr, _ := regexp.Compile(`\\d+`)\n\treturn r\n}\n", ), ) } @@ -38,6 +38,33 @@ func TestPreferRawStringRegexNoChangeRawString(t *testing.T) { ) } +// Skips regexp.Compile in a single-value context, where the two-value call does not compile. +func TestPreferRawStringRegexNoChangeSingleValueContext(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&style.PreferRawStringForRegex{}) + spec.RewriteRun(t, + test.Golang("package main\n\nimport \"regexp\"\n\nvar r = regexp.Compile(\"\\\\d+\")\n"), + ) +} + +// Skips a real newline escape, which a raw string would embed as a literal line break. +func TestPreferRawStringRegexNoChangeControlChar(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&style.PreferRawStringForRegex{}) + spec.RewriteRun(t, + test.Golang("package main\n\nimport \"regexp\"\n\nvar re = regexp.MustCompile(\"a\\nb\")\n"), + ) +} + +// Rewrites a `\\t` metacharacter escape, which is a backslash rather than a control character. +func TestPreferRawStringRegexMetacharTab(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&style.PreferRawStringForRegex{}) + spec.RewriteRun(t, + test.Golang( + "package main\n\nimport \"regexp\"\n\nvar re = regexp.MustCompile(\"\\\\t+\")\n", + "package main\n\nimport \"regexp\"\n\nvar re = regexp.MustCompile(`\\t+`)\n", + ), + ) +} + func TestPreferRawStringRegexNoChangeNoBackslash(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&style.PreferRawStringForRegex{}) spec.RewriteRun(t, diff --git a/tests/style/prefer_strconv_quote_test.go b/tests/style/prefer_strconv_quote_test.go index 553e935..e9ce80f 100644 --- a/tests/style/prefer_strconv_quote_test.go +++ b/tests/style/prefer_strconv_quote_test.go @@ -50,3 +50,19 @@ func TestPreferStrconvQuoteNoChangeOtherVerb(t *testing.T) { `), ) } + +// Skips a rune argument, since %q accepts it but strconv.Quote needs a string. +func TestPreferStrconvQuoteNoChangeRune(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&style.PreferStrconvQuote{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import "fmt" + + func f(r rune) string { + return fmt.Sprintf("%q", r) + } + `), + ) +} diff --git a/tests/style/reduce_error_check_nesting_test.go b/tests/style/reduce_error_check_nesting_test.go index 4c6302e..01c0f18 100644 --- a/tests/style/reduce_error_check_nesting_test.go +++ b/tests/style/reduce_error_check_nesting_test.go @@ -17,12 +17,11 @@ func TestReduceErrorCheckNesting(t *testing.T) { test.Golang(` package main - func f() error { - err := doSomething() + func f() (err error) { + err = doSomething() if err == nil { process() } - return nil } func doSomething() error { return nil } @@ -30,13 +29,12 @@ func TestReduceErrorCheckNesting(t *testing.T) { `, ` package main - func f() error { - err := doSomething() + func f() (err error) { + err = doSomething() if err != nil { return err } process() - return nil } func doSomething() error { return nil } @@ -45,6 +43,27 @@ func TestReduceErrorCheckNesting(t *testing.T) { ) } +// Skips a function that does not return a single error, where the synthesized +// `return err` would not compile. +func TestReduceErrorCheckNestingNoChangeNonErrorReturn(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&style.ReduceErrorCheckNesting{}) + spec.RewriteRun(t, + test.Golang(` + package main + + func load() int { + err := doSomething() + if err == nil { + return 42 + } + return 0 + } + + func doSomething() error { return nil } + `), + ) +} + func TestReduceErrorCheckNestingNoChangeErrNotNil(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&style.ReduceErrorCheckNesting{}) spec.RewriteRun(t, diff --git a/tests/style/reduce_nesting_depth_test.go b/tests/style/reduce_nesting_depth_test.go index 8967e50..1ccab7b 100644 --- a/tests/style/reduce_nesting_depth_test.go +++ b/tests/style/reduce_nesting_depth_test.go @@ -17,12 +17,11 @@ func TestReduceNestingDepthGuardClause(t *testing.T) { test.Golang(` package main - func f() error { + func f() { err := doSomething() if err == nil { process() } - return nil } func doSomething() error { return nil } @@ -30,13 +29,12 @@ func TestReduceNestingDepthGuardClause(t *testing.T) { `, ` package main - func f() error { + func f() { err := doSomething() if err != nil { return } process() - return nil } func doSomething() error { return nil } @@ -61,6 +59,71 @@ func TestReduceNestingDepthNoChangeNotErrEqualNil(t *testing.T) { ) } +// Skips a value-returning function, where the bare `return` guard would not compile. +func TestReduceNestingDepthNoChangeValueReturningFunc(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&style.ReduceNestingDepth{}) + spec.RewriteRun(t, + test.Golang(` + package main + + func f() error { + err := doSomething() + if err == nil { + process() + } + return nil + } + + func doSomething() error { return nil } + func process() {} + `), + ) +} + +// Skips a non-terminal `if err == nil`, where the early return would drop the following cleanup(). +func TestReduceNestingDepthNoChangeNotLastStatement(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&style.ReduceNestingDepth{}) + spec.RewriteRun(t, + test.Golang(` + package main + + func f() { + err := doSomething() + if err == nil { + process() + } + cleanup() + } + + func doSomething() error { return nil } + func process() {} + func cleanup() {} + `), + ) +} + +// Skips an `if err == nil` in a loop body, where the early return would exit the whole function. +func TestReduceNestingDepthNoChangeInsideLoop(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&style.ReduceNestingDepth{}) + spec.RewriteRun(t, + test.Golang(` + package main + + func f(xs []int) { + for _, x := range xs { + err := check(x) + if err == nil { + process(x) + } + } + } + + func check(int) error { return nil } + func process(int) {} + `), + ) +} + func TestReduceNestingDepthNoChangeHasElse(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&style.ReduceNestingDepth{}) spec.RewriteRun(t, diff --git a/tests/style/use_strong_hash_test.go b/tests/style/use_strong_hash_test.go index 1149b23..47dc7e6 100644 --- a/tests/style/use_strong_hash_test.go +++ b/tests/style/use_strong_hash_test.go @@ -38,7 +38,10 @@ func TestUseStrongHashMd5New(t *testing.T) { ) } -func TestUseStrongHashMd5Sum(t *testing.T) { +// md5.Sum is intentionally not rewritten: it returns [16]byte while +// sha256.Sum256 returns [32]byte, so a local swap does not compile in typed +// contexts and would need a whole-usage migration. +func TestUseStrongHashNoChangeMd5Sum(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&style.UseStrongHash{}) spec.RewriteRun(t, test.Golang(` @@ -50,17 +53,6 @@ func TestUseStrongHashMd5Sum(t *testing.T) { h := md5.Sum(data) _ = h } - `, ` - package main - - import ( - "crypto/sha256" - ) - - func f(data []byte) { - h := sha256.Sum256(data) - _ = h - } `), ) } @@ -92,7 +84,9 @@ func TestUseStrongHashSha1New(t *testing.T) { ) } -func TestUseStrongHashSha1Sum(t *testing.T) { +// sha1.Sum is intentionally not rewritten for the same [20]byte vs [32]byte +// reason as md5.Sum. +func TestUseStrongHashNoChangeSha1Sum(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&style.UseStrongHash{}) spec.RewriteRun(t, test.Golang(` @@ -104,17 +98,6 @@ func TestUseStrongHashSha1Sum(t *testing.T) { h := sha1.Sum(data) _ = h } - `, ` - package main - - import ( - "crypto/sha256" - ) - - func f(data []byte) { - h := sha256.Sum256(data) - _ = h - } `), ) } From 272220cdeae15af7955bad3a5afd1517af132936 Mon Sep 17 00:00:00 2001 From: Benjamin Muschko Date: Fri, 7 Aug 2026 14:38:50 -0600 Subject: [PATCH 2/5] Only mark discarded Close() errors in statement position --- recipes/errorhandling/check_close_error.go | 26 +++------- tests/errorhandling/check_close_error_test.go | 50 +++++++++++++++++++ 2 files changed, 57 insertions(+), 19 deletions(-) diff --git a/recipes/errorhandling/check_close_error.go b/recipes/errorhandling/check_close_error.go index 30a145c..ce98311 100644 --- a/recipes/errorhandling/check_close_error.go +++ b/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,6 @@ 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 @@ -70,9 +54,13 @@ 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 } diff --git a/tests/errorhandling/check_close_error_test.go b/tests/errorhandling/check_close_error_test.go index 938a0ec..7db42d5 100644 --- a/tests/errorhandling/check_close_error_test.go +++ b/tests/errorhandling/check_close_error_test.go @@ -93,3 +93,53 @@ func TestCheckCloseErrorNoChangeVoidClose(t *testing.T) { `), ) } + +// Skips a returned Close(), where `return _ = r.Close()` would not compile. +func TestCheckCloseErrorNoChangeReturn(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&errorhandling.CheckCloseError{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import "os" + + func f(r *os.File) error { + return r.Close() + } + `), + ) +} + +// Skips a Close() whose error is already inspected in a condition. +func TestCheckCloseErrorNoChangeInCondition(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&errorhandling.CheckCloseError{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import "os" + + func f(r *os.File) { + if r.Close() != nil { + return + } + } + `), + ) +} + +// Skips a deferred Close(), where `defer _ = r.Close()` would not compile. +func TestCheckCloseErrorNoChangeDefer(t *testing.T) { + spec := test.NewRecipeSpec().WithRecipe(&errorhandling.CheckCloseError{}) + spec.RewriteRun(t, + test.Golang(` + package main + + import "os" + + func f(r *os.File) { + defer r.Close() + } + `), + ) +} From 9daa8eb65d1cc39ffa716ecd1f8d61b19a1b2764 Mon Sep 17 00:00:00 2001 From: Benjamin Muschko Date: Fri, 7 Aug 2026 14:56:14 -0600 Subject: [PATCH 3/5] Fix UseStrongHash to add sha256 and drop weak-hash imports --- recipes/style/use_strong_hash.go | 47 ++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 15 deletions(-) diff --git a/recipes/style/use_strong_hash.go b/recipes/style/use_strong_hash.go index 9f2b09b..fa81c45 100644 --- a/recipes/style/use_strong_hash.go +++ b/recipes/style/use_strong_hash.go @@ -6,7 +6,10 @@ package style import ( "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe" + recipegolang "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe/golang" "github.com/openrewrite/rewrite/rewrite-go/pkg/template" + "github.com/openrewrite/rewrite/rewrite-go/pkg/tree/java" + "github.com/openrewrite/rewrite/rewrite-go/pkg/visitor" ) // Replaces the weak hash constructors md5.New() and sha1.New() with @@ -26,23 +29,37 @@ func (r *UseStrongHash) Description() string { } func (r *UseStrongHash) Tags() []string { return []string{"style", "security"} } -var useStrongHashMd5New = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.UseStrongHash$Md5New"), - template.WithDisplayName("md5.New() -> sha256.New()"), - template.WithBefore(`md5.New()`, template.Imports("crypto/md5")), - template.WithAfter(`sha256.New()`, template.Imports("crypto/sha256"), template.SourceImports("crypto/sha256")), -) +func (r *UseStrongHash) Editor() recipe.TreeVisitor { + return visitor.Init(&useStrongHashVisitor{}) +} -var useStrongHashSha1New = template.NewRecipe( - template.RecipeName("org.openrewrite.golang.codequality.UseStrongHash$Sha1New"), - template.WithDisplayName("sha1.New() -> sha256.New()"), - template.WithBefore(`sha1.New()`, template.Imports("crypto/sha1")), - template.WithAfter(`sha256.New()`, template.Imports("crypto/sha256"), template.SourceImports("crypto/sha256")), +var ( + md5NewPattern = template.Expression(`md5.New()`).Imports("crypto/md5").Build() + sha1NewPattern = template.Expression(`sha1.New()`).Imports("crypto/sha1").Build() + sha256NewTemplate = template.ExpressionTemplate(`sha256.New()`).Imports("crypto/sha256").Build() ) -func (r *UseStrongHash) RecipeList() []recipe.Recipe { - return []recipe.Recipe{ - useStrongHashMd5New, - useStrongHashSha1New, +type useStrongHashVisitor struct { + visitor.GoVisitor +} + +func (v *useStrongHashVisitor) VisitMethodInvocation(mi *java.MethodInvocation, p any) java.J { + mi = v.GoVisitor.VisitMethodInvocation(mi, p).(*java.MethodInvocation) + + match := md5NewPattern.Match(mi, nil) + if match == nil { + match = sha1NewPattern.Match(mi, nil) } + if match == nil { + return mi + } + + replaced, ok := sha256NewTemplate.Apply(nil, match).(*java.MethodInvocation) + if !ok { + return mi + } + + recipegolang.MaybeAddImport(v, "crypto/sha256", nil, false) + v.DoAfterVisit(recipe.Service[*recipegolang.ImportService](nil).RemoveUnusedImportsVisitor()) + return replaced.WithPrefix(mi.GetPrefix()) } From bc219abe5adeea675b86e9baa1570a5d70f137ed Mon Sep 17 00:00:00 2001 From: Benjamin Muschko Date: Fri, 7 Aug 2026 14:56:14 -0600 Subject: [PATCH 4/5] Register five recipes missing from the marketplace --- recipes/activate.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/recipes/activate.go b/recipes/activate.go index 3d2900b..95ec069 100644 --- a/recipes/activate.go +++ b/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) From 7785774e024e2f215e62f4487494b05fe48956a7 Mon Sep 17 00:00:00 2001 From: Benjamin Muschko Date: Tue, 11 Aug 2026 09:52:02 -0600 Subject: [PATCH 5/5] Shorten verbose test summary comments --- tests/errorhandling/check_close_error_test.go | 6 +++--- .../errorhandling/handle_deferred_close_error_test.go | 2 +- tests/errorhandling/handle_error_return_test.go | 8 ++++---- tests/errorhandling/prefer_errors_is_context_test.go | 2 +- tests/errorhandling/prefer_errors_is_eof_test.go | 2 +- tests/errorhandling/prefer_errors_is_http_test.go | 2 +- tests/errorhandling/prefer_errors_is_net_test.go | 2 +- tests/errorhandling/prefer_errors_is_os_path_test.go | 2 +- tests/errorhandling/prefer_errors_is_sql_test.go | 2 +- tests/errorhandling/prefer_errors_join_test.go | 3 +-- tests/errorhandling/use_error_method_test.go | 2 +- tests/errorhandling/use_errors_as_test.go | 2 +- tests/errorhandling/wrap_error_with_context_test.go | 3 +-- tests/performance/prefer_strconv_format_bool_test.go | 2 +- tests/performance/use_strings_builder_in_loop_test.go | 2 +- tests/redundancy/remove_redundant_sprintf_test.go | 2 +- tests/redundancy/simplify_goroutine_closure_test.go | 3 +-- tests/simplification/prefer_empty_string_check_test.go | 2 +- tests/simplification/prefer_io_writestring_test.go | 2 +- tests/simplification/prefer_os_readdir_test.go | 2 +- tests/simplification/prefer_strconv_atoi_test.go | 2 +- .../prefer_strings_builder_writestring_test.go | 2 +- tests/simplification/prefer_strings_newreader_test.go | 10 +++++----- tests/simplification/prefer_strings_repeat_test.go | 2 +- tests/style/check_template_execute_error_test.go | 3 +-- tests/style/prefer_hex_encoding_test.go | 2 +- tests/style/prefer_raw_string_regex_test.go | 6 +++--- tests/style/prefer_strconv_quote_test.go | 2 +- tests/style/reduce_error_check_nesting_test.go | 3 +-- tests/style/reduce_nesting_depth_test.go | 6 +++--- tests/style/use_strong_hash_test.go | 7 ++----- 31 files changed, 45 insertions(+), 53 deletions(-) diff --git a/tests/errorhandling/check_close_error_test.go b/tests/errorhandling/check_close_error_test.go index 7db42d5..b30c7d7 100644 --- a/tests/errorhandling/check_close_error_test.go +++ b/tests/errorhandling/check_close_error_test.go @@ -76,7 +76,7 @@ func TestCheckCloseErrorNoChangeRead(t *testing.T) { ) } -// Skips a void Close(), where `_ = t.Close()` would not compile. +// Skips a void Close(). func TestCheckCloseErrorNoChangeVoidClose(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&errorhandling.CheckCloseError{}) spec.RewriteRun(t, @@ -94,7 +94,7 @@ func TestCheckCloseErrorNoChangeVoidClose(t *testing.T) { ) } -// Skips a returned Close(), where `return _ = r.Close()` would not compile. +// Skips a returned Close(). func TestCheckCloseErrorNoChangeReturn(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&errorhandling.CheckCloseError{}) spec.RewriteRun(t, @@ -128,7 +128,7 @@ func TestCheckCloseErrorNoChangeInCondition(t *testing.T) { ) } -// Skips a deferred Close(), where `defer _ = r.Close()` would not compile. +// Skips a deferred Close(). func TestCheckCloseErrorNoChangeDefer(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&errorhandling.CheckCloseError{}) spec.RewriteRun(t, diff --git a/tests/errorhandling/handle_deferred_close_error_test.go b/tests/errorhandling/handle_deferred_close_error_test.go index 41f8f6d..8be9462 100644 --- a/tests/errorhandling/handle_deferred_close_error_test.go +++ b/tests/errorhandling/handle_deferred_close_error_test.go @@ -53,7 +53,7 @@ func TestHandleDeferredCloseErrorNoChangeDone(t *testing.T) { ) } -// Skips a void Close(), where `_ = t.Close()` inside the closure would not compile. +// Skips a void Close(). func TestHandleDeferredCloseErrorNoChangeVoidClose(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&errorhandling.HandleDeferredCloseError{}) spec.RewriteRun(t, diff --git a/tests/errorhandling/handle_error_return_test.go b/tests/errorhandling/handle_error_return_test.go index f27e165..5f9c72a 100644 --- a/tests/errorhandling/handle_error_return_test.go +++ b/tests/errorhandling/handle_error_return_test.go @@ -41,7 +41,7 @@ func TestHandleErrorReturnDiscarded(t *testing.T) { ) } -// Skips a plain `=` assignment in main(), where `err` is undeclared and no error return exists. +// Skips a plain `=` assignment in main(). func TestHandleErrorReturnNoChangeUndeclaredErr(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&errorhandling.HandleErrorReturn{}) spec.RewriteRun(t, @@ -57,7 +57,7 @@ func TestHandleErrorReturnNoChangeUndeclaredErr(t *testing.T) { ) } -// Skips the comma-ok map access, where the discarded value is a bool rather than an error. +// Skips the comma-ok map access. func TestHandleErrorReturnNoChangeCommaOkMap(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&errorhandling.HandleErrorReturn{}) spec.RewriteRun(t, @@ -73,7 +73,7 @@ func TestHandleErrorReturnNoChangeCommaOkMap(t *testing.T) { ) } -// Skips the comma-ok type assertion, where the discarded value is a bool. +// Skips the comma-ok type assertion. func TestHandleErrorReturnNoChangeCommaOkTypeAssert(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&errorhandling.HandleErrorReturn{}) spec.RewriteRun(t, @@ -89,7 +89,7 @@ func TestHandleErrorReturnNoChangeCommaOkTypeAssert(t *testing.T) { ) } -// Skips a capture in a loop body, where the inserted `return err` would change control flow. +// Skips a capture in a loop body. func TestHandleErrorReturnNoChangeInsideLoop(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&errorhandling.HandleErrorReturn{}) spec.RewriteRun(t, diff --git a/tests/errorhandling/prefer_errors_is_context_test.go b/tests/errorhandling/prefer_errors_is_context_test.go index d56245b..8b29a59 100644 --- a/tests/errorhandling/prefer_errors_is_context_test.go +++ b/tests/errorhandling/prefer_errors_is_context_test.go @@ -128,7 +128,7 @@ func TestPreferErrorsIsContextNoChangeNilCheck(t *testing.T) { ) } -// Skips a non-error (any) operand, where errors.Is would not compile. +// Skips a non-error (any) operand. func TestPreferErrorsIsContextNoChangeNonError(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&errorhandling.PreferErrorsIsContext{}) spec.RewriteRun(t, diff --git a/tests/errorhandling/prefer_errors_is_eof_test.go b/tests/errorhandling/prefer_errors_is_eof_test.go index db19277..8063b0d 100644 --- a/tests/errorhandling/prefer_errors_is_eof_test.go +++ b/tests/errorhandling/prefer_errors_is_eof_test.go @@ -76,7 +76,7 @@ func TestPreferErrorsIsEOFNoChangeNil(t *testing.T) { ) } -// Skips a non-error (any) operand, where errors.Is would not compile. +// Skips a non-error (any) operand. func TestPreferErrorsIsEOFNoChangeNonError(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&errorhandling.PreferErrorsIsEOF{}) spec.RewriteRun(t, diff --git a/tests/errorhandling/prefer_errors_is_http_test.go b/tests/errorhandling/prefer_errors_is_http_test.go index 4afb742..4386838 100644 --- a/tests/errorhandling/prefer_errors_is_http_test.go +++ b/tests/errorhandling/prefer_errors_is_http_test.go @@ -76,7 +76,7 @@ func TestPreferErrorsIsHttpServerClosedNoChangeNil(t *testing.T) { ) } -// Skips a non-error (any) operand, where errors.Is would not compile. +// Skips a non-error (any) operand. func TestPreferErrorsIsHttpServerClosedNoChangeNonError(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&errorhandling.PreferErrorsIsHttpServerClosed{}) spec.RewriteRun(t, diff --git a/tests/errorhandling/prefer_errors_is_net_test.go b/tests/errorhandling/prefer_errors_is_net_test.go index ce39183..b39ca4b 100644 --- a/tests/errorhandling/prefer_errors_is_net_test.go +++ b/tests/errorhandling/prefer_errors_is_net_test.go @@ -76,7 +76,7 @@ func TestPreferErrorsIsNetClosedNoChangeNil(t *testing.T) { ) } -// Skips a non-error (any) operand, where errors.Is would not compile. +// Skips a non-error (any) operand. func TestPreferErrorsIsNetClosedNoChangeNonError(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&errorhandling.PreferErrorsIsNetClosed{}) spec.RewriteRun(t, diff --git a/tests/errorhandling/prefer_errors_is_os_path_test.go b/tests/errorhandling/prefer_errors_is_os_path_test.go index 647311f..c31caca 100644 --- a/tests/errorhandling/prefer_errors_is_os_path_test.go +++ b/tests/errorhandling/prefer_errors_is_os_path_test.go @@ -50,7 +50,7 @@ func TestPreferErrorsIsOsInvalidNoChangeNil(t *testing.T) { ) } -// Skips a non-error (any) operand, where errors.Is would not compile. +// Skips a non-error (any) operand. func TestPreferErrorsIsOsInvalidNoChangeNonError(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&errorhandling.PreferErrorsIsOsInvalid{}) spec.RewriteRun(t, diff --git a/tests/errorhandling/prefer_errors_is_sql_test.go b/tests/errorhandling/prefer_errors_is_sql_test.go index 4518381..3e4d166 100644 --- a/tests/errorhandling/prefer_errors_is_sql_test.go +++ b/tests/errorhandling/prefer_errors_is_sql_test.go @@ -76,7 +76,7 @@ func TestPreferErrorsIsSqlNoRowsNoChangeNil(t *testing.T) { ) } -// Skips a non-error (any) operand, where errors.Is would not compile. +// Skips a non-error (any) operand. func TestPreferErrorsIsSqlNoRowsNoChangeNonError(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&errorhandling.PreferErrorsIsSqlNoRows{}) spec.RewriteRun(t, diff --git a/tests/errorhandling/prefer_errors_join_test.go b/tests/errorhandling/prefer_errors_join_test.go index 668d6ce..b8b1210 100644 --- a/tests/errorhandling/prefer_errors_join_test.go +++ b/tests/errorhandling/prefer_errors_join_test.go @@ -47,8 +47,7 @@ func TestSimplifyRedundantErrorWrapNoChangeWithContext(t *testing.T) { ) } -// Skips a non-error (any) argument, where replacing fmt.Errorf with the bare -// value would not compile. +// Skips a non-error (any) argument. func TestSimplifyRedundantErrorWrapNoChangeNonError(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&errorhandling.SimplifyRedundantErrorWrap{}) spec.RewriteRun(t, diff --git a/tests/errorhandling/use_error_method_test.go b/tests/errorhandling/use_error_method_test.go index da6e49d..8341226 100644 --- a/tests/errorhandling/use_error_method_test.go +++ b/tests/errorhandling/use_error_method_test.go @@ -47,7 +47,7 @@ func TestUseErrorMethodNoChangeInt(t *testing.T) { ) } -// Skips a non-error value named err, since err.Error() requires the error interface. +// Skips a non-error value named err. func TestUseErrorMethodNoChangeNonError(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&errorhandling.UseErrorMethod{}) spec.RewriteRun(t, diff --git a/tests/errorhandling/use_errors_as_test.go b/tests/errorhandling/use_errors_as_test.go index c040c61..edd8b61 100644 --- a/tests/errorhandling/use_errors_as_test.go +++ b/tests/errorhandling/use_errors_as_test.go @@ -75,7 +75,7 @@ func TestUseErrorsAsNoChangeNoInit(t *testing.T) { ) } -// Skips an assertion on an any-typed value, since errors.As needs an error argument. +// Skips an assertion on an any-typed value. func TestUseErrorsAsNoChangeNonError(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&errorhandling.UseErrorsAs{}) spec.RewriteRun(t, diff --git a/tests/errorhandling/wrap_error_with_context_test.go b/tests/errorhandling/wrap_error_with_context_test.go index d195946..41f1860 100644 --- a/tests/errorhandling/wrap_error_with_context_test.go +++ b/tests/errorhandling/wrap_error_with_context_test.go @@ -84,8 +84,7 @@ func TestWrapErrorWithContextNoChangeMultiReturn(t *testing.T) { ) } -// Skips a function returning a concrete error type, where `return fmt.Errorf(...)` -// (which yields error) would not compile. +// Skips a function returning a concrete error type. func TestWrapErrorWithContextNoChangeConcreteReturn(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&errorhandling.WrapErrorWithContext{}) spec.RewriteRun(t, diff --git a/tests/performance/prefer_strconv_format_bool_test.go b/tests/performance/prefer_strconv_format_bool_test.go index a37ced4..6b1299a 100644 --- a/tests/performance/prefer_strconv_format_bool_test.go +++ b/tests/performance/prefer_strconv_format_bool_test.go @@ -66,7 +66,7 @@ func TestPreferStrconvFormatBoolNoChangeMultipleArgs(t *testing.T) { ) } -// Skips a non-bool argument, since strconv.FormatBool needs a bool. +// Skips a non-bool argument. func TestPreferStrconvFormatBoolNoChangeNonBool(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&performance.PreferStrconvFormatBool{}) spec.RewriteRun(t, diff --git a/tests/performance/use_strings_builder_in_loop_test.go b/tests/performance/use_strings_builder_in_loop_test.go index 7e67c96..898b62e 100644 --- a/tests/performance/use_strings_builder_in_loop_test.go +++ b/tests/performance/use_strings_builder_in_loop_test.go @@ -88,7 +88,7 @@ func TestStringConcatNoChangeOutsideLoop(t *testing.T) { ) } -// Skips a numeric accumulator, where builder.WriteString(number) would not compile. +// Skips a numeric accumulator. func TestStringConcatNoChangeNumeric(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&performance.UseStringsBuilderInLoop{}) spec.RewriteRun(t, diff --git a/tests/redundancy/remove_redundant_sprintf_test.go b/tests/redundancy/remove_redundant_sprintf_test.go index 3d711d0..91051bb 100644 --- a/tests/redundancy/remove_redundant_sprintf_test.go +++ b/tests/redundancy/remove_redundant_sprintf_test.go @@ -62,7 +62,7 @@ func TestRemoveRedundantSprintfNoChangeFormatD(t *testing.T) { ) } -// Skips a []byte argument, since %s accepts it but the bare value is not a string. +// Skips a []byte argument. func TestRemoveRedundantSprintfNoChangeBytes(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&redundancy.RemoveRedundantSprintf{}) spec.RewriteRun(t, diff --git a/tests/redundancy/simplify_goroutine_closure_test.go b/tests/redundancy/simplify_goroutine_closure_test.go index d0967aa..e34f379 100644 --- a/tests/redundancy/simplify_goroutine_closure_test.go +++ b/tests/redundancy/simplify_goroutine_closure_test.go @@ -68,8 +68,7 @@ func TestSimplifyGoroutineClosureNoChangeDirectCall(t *testing.T) { ) } -// Skips a closure with parameters, where dropping them would leave the inner -// call referencing an out-of-scope name. +// Skips a closure with parameters. func TestSimplifyGoroutineClosureNoChangeWithParams(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&redundancy.SimplifyGoroutineClosure{}) spec.RewriteRun(t, diff --git a/tests/simplification/prefer_empty_string_check_test.go b/tests/simplification/prefer_empty_string_check_test.go index fd779d3..4523185 100644 --- a/tests/simplification/prefer_empty_string_check_test.go +++ b/tests/simplification/prefer_empty_string_check_test.go @@ -49,7 +49,7 @@ func TestPreferEmptyStringCheckNotEqual(t *testing.T) { ) } -// Skips a []byte argument, since `== ""` requires a string while len accepts slices. +// Skips a []byte argument. func TestPreferEmptyStringCheckNoChangeBytes(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&simplification.PreferEmptyStringCheck{}) spec.RewriteRun(t, diff --git a/tests/simplification/prefer_io_writestring_test.go b/tests/simplification/prefer_io_writestring_test.go index c37930a..703a8a1 100644 --- a/tests/simplification/prefer_io_writestring_test.go +++ b/tests/simplification/prefer_io_writestring_test.go @@ -58,7 +58,7 @@ func TestPreferIoWriteStringNoChange(t *testing.T) { ) } -// Skips a []byte argument, since io.WriteString takes a string. +// Skips a []byte argument. func TestPreferIoWriteStringNoChangeBytes(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&simplification.PreferIoWriteString{}) spec.RewriteRun(t, diff --git a/tests/simplification/prefer_os_readdir_test.go b/tests/simplification/prefer_os_readdir_test.go index 61c4514..192103c 100644 --- a/tests/simplification/prefer_os_readdir_test.go +++ b/tests/simplification/prefer_os_readdir_test.go @@ -54,7 +54,7 @@ func TestPreferOsReadDirNoChange(t *testing.T) { ) } -// Skips a direct return of []os.FileInfo, where os.ReadDir's []os.DirEntry would not compile. +// Skips a direct return of []os.FileInfo. func TestPreferOsReadDirNoChangeFileInfoContext(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&simplification.PreferOsReadDir{}) spec.RewriteRun(t, diff --git a/tests/simplification/prefer_strconv_atoi_test.go b/tests/simplification/prefer_strconv_atoi_test.go index 9e9b5c0..cd27c7f 100644 --- a/tests/simplification/prefer_strconv_atoi_test.go +++ b/tests/simplification/prefer_strconv_atoi_test.go @@ -118,7 +118,7 @@ func TestPreferStrconvAtoiNoChangeBitSize(t *testing.T) { ) } -// Skips a direct return of the int64 result, where strconv.Atoi's int would not compile. +// Skips a direct return of the int64 result. func TestPreferStrconvAtoiNoChangeInt64Context(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&simplification.PreferStrconvAtoi{}) spec.RewriteRun(t, diff --git a/tests/simplification/prefer_strings_builder_writestring_test.go b/tests/simplification/prefer_strings_builder_writestring_test.go index f4a83be..788a143 100644 --- a/tests/simplification/prefer_strings_builder_writestring_test.go +++ b/tests/simplification/prefer_strings_builder_writestring_test.go @@ -63,7 +63,7 @@ func TestPreferStringsBuilderWriteStringNoChangeFormat(t *testing.T) { ) } -// Skips a []byte argument, since Builder.WriteString takes a string. +// Skips a []byte argument. func TestPreferStringsBuilderWriteStringNoChangeBytes(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&simplification.PreferStringsBuilderWriteString{}) spec.RewriteRun(t, diff --git a/tests/simplification/prefer_strings_newreader_test.go b/tests/simplification/prefer_strings_newreader_test.go index 777c38a..73a8229 100644 --- a/tests/simplification/prefer_strings_newreader_test.go +++ b/tests/simplification/prefer_strings_newreader_test.go @@ -40,7 +40,7 @@ func TestPreferStringsNewReader(t *testing.T) { ) } -// Skips a []byte argument, where strings.NewReader's string parameter would not compile. +// Skips a []byte argument. func TestPreferStringsNewReaderNoChangeByteSliceArg(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&simplification.PreferStringsNewReader{}) spec.RewriteRun(t, @@ -57,7 +57,7 @@ func TestPreferStringsNewReaderNoChangeByteSliceArg(t *testing.T) { ) } -// A string literal is a string, so the rewrite still proceeds. +// A string literal is a string. func TestPreferStringsNewReaderStringLiteralArg(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&simplification.PreferStringsNewReader{}) spec.RewriteRun(t, @@ -87,7 +87,7 @@ func TestPreferStringsNewReaderStringLiteralArg(t *testing.T) { ) } -// Skips a *bytes.Reader variable declaration, which *strings.Reader would not satisfy. +// Skips a *bytes.Reader variable declaration. func TestPreferStringsNewReaderNoChangeTypedVarDecl(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&simplification.PreferStringsNewReader{}) spec.RewriteRun(t, @@ -104,7 +104,7 @@ func TestPreferStringsNewReaderNoChangeTypedVarDecl(t *testing.T) { ) } -// An interface-typed declaration accepts both readers, so the rewrite proceeds. +// An interface-typed declaration accepts both readers. func TestPreferStringsNewReaderInterfaceVarDecl(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&simplification.PreferStringsNewReader{}) spec.RewriteRun(t, @@ -151,7 +151,7 @@ func TestPreferStringsNewReaderNoChange(t *testing.T) { ) } -// Skips a direct return of *bytes.Reader, where *strings.Reader would not compile. +// Skips a direct return of *bytes.Reader. func TestPreferStringsNewReaderNoChangeBytesReaderContext(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&simplification.PreferStringsNewReader{}) spec.RewriteRun(t, diff --git a/tests/simplification/prefer_strings_repeat_test.go b/tests/simplification/prefer_strings_repeat_test.go index afc5ae9..be13acf 100644 --- a/tests/simplification/prefer_strings_repeat_test.go +++ b/tests/simplification/prefer_strings_repeat_test.go @@ -47,7 +47,7 @@ func TestSimplifySprintfConcatNoChangeFormat(t *testing.T) { ) } -// Skips []byte arguments, since %s accepts them but []byte values cannot be added. +// Skips []byte arguments. func TestSimplifySprintfConcatNoChangeBytes(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&simplification.SimplifySprintfConcat{}) spec.RewriteRun(t, diff --git a/tests/style/check_template_execute_error_test.go b/tests/style/check_template_execute_error_test.go index 5da8d6c..4290a86 100644 --- a/tests/style/check_template_execute_error_test.go +++ b/tests/style/check_template_execute_error_test.go @@ -110,8 +110,7 @@ func TestCheckTemplateExecuteErrorNoChangeNoErrorReturn(t *testing.T) { ) } -// Skips a multi-value result like (int, error), where the synthesized bare -// `return err` would not compile. +// Skips a multi-value result like (int, error). func TestCheckTemplateExecuteErrorNoChangeMultiReturn(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&style.CheckTemplateExecuteError{}) spec.RewriteRun(t, diff --git a/tests/style/prefer_hex_encoding_test.go b/tests/style/prefer_hex_encoding_test.go index 1f7325c..cfe84a3 100644 --- a/tests/style/prefer_hex_encoding_test.go +++ b/tests/style/prefer_hex_encoding_test.go @@ -51,7 +51,7 @@ func TestPreferHexEncodingNoChangeOtherVerb(t *testing.T) { ) } -// Skips a string argument, since %x accepts it but hex.EncodeToString needs []byte. +// Skips a string argument. func TestPreferHexEncodingNoChangeString(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&style.PreferHexEncoding{}) spec.RewriteRun(t, diff --git a/tests/style/prefer_raw_string_regex_test.go b/tests/style/prefer_raw_string_regex_test.go index c721377..cca7c67 100644 --- a/tests/style/prefer_raw_string_regex_test.go +++ b/tests/style/prefer_raw_string_regex_test.go @@ -38,7 +38,7 @@ func TestPreferRawStringRegexNoChangeRawString(t *testing.T) { ) } -// Skips regexp.Compile in a single-value context, where the two-value call does not compile. +// Skips regexp.Compile in a single-value context. func TestPreferRawStringRegexNoChangeSingleValueContext(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&style.PreferRawStringForRegex{}) spec.RewriteRun(t, @@ -46,7 +46,7 @@ func TestPreferRawStringRegexNoChangeSingleValueContext(t *testing.T) { ) } -// Skips a real newline escape, which a raw string would embed as a literal line break. +// Skips a real newline escape. func TestPreferRawStringRegexNoChangeControlChar(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&style.PreferRawStringForRegex{}) spec.RewriteRun(t, @@ -54,7 +54,7 @@ func TestPreferRawStringRegexNoChangeControlChar(t *testing.T) { ) } -// Rewrites a `\\t` metacharacter escape, which is a backslash rather than a control character. +// Rewrites a `\\t` metacharacter escape. func TestPreferRawStringRegexMetacharTab(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&style.PreferRawStringForRegex{}) spec.RewriteRun(t, diff --git a/tests/style/prefer_strconv_quote_test.go b/tests/style/prefer_strconv_quote_test.go index e9ce80f..a7cf65c 100644 --- a/tests/style/prefer_strconv_quote_test.go +++ b/tests/style/prefer_strconv_quote_test.go @@ -51,7 +51,7 @@ func TestPreferStrconvQuoteNoChangeOtherVerb(t *testing.T) { ) } -// Skips a rune argument, since %q accepts it but strconv.Quote needs a string. +// Skips a rune argument. func TestPreferStrconvQuoteNoChangeRune(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&style.PreferStrconvQuote{}) spec.RewriteRun(t, diff --git a/tests/style/reduce_error_check_nesting_test.go b/tests/style/reduce_error_check_nesting_test.go index 01c0f18..cb7f9e6 100644 --- a/tests/style/reduce_error_check_nesting_test.go +++ b/tests/style/reduce_error_check_nesting_test.go @@ -43,8 +43,7 @@ func TestReduceErrorCheckNesting(t *testing.T) { ) } -// Skips a function that does not return a single error, where the synthesized -// `return err` would not compile. +// Skips a function that does not return a single error. func TestReduceErrorCheckNestingNoChangeNonErrorReturn(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&style.ReduceErrorCheckNesting{}) spec.RewriteRun(t, diff --git a/tests/style/reduce_nesting_depth_test.go b/tests/style/reduce_nesting_depth_test.go index 1ccab7b..9cc63a1 100644 --- a/tests/style/reduce_nesting_depth_test.go +++ b/tests/style/reduce_nesting_depth_test.go @@ -59,7 +59,7 @@ func TestReduceNestingDepthNoChangeNotErrEqualNil(t *testing.T) { ) } -// Skips a value-returning function, where the bare `return` guard would not compile. +// Skips a value-returning function. func TestReduceNestingDepthNoChangeValueReturningFunc(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&style.ReduceNestingDepth{}) spec.RewriteRun(t, @@ -80,7 +80,7 @@ func TestReduceNestingDepthNoChangeValueReturningFunc(t *testing.T) { ) } -// Skips a non-terminal `if err == nil`, where the early return would drop the following cleanup(). +// Skips a non-terminal `if err == nil`. func TestReduceNestingDepthNoChangeNotLastStatement(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&style.ReduceNestingDepth{}) spec.RewriteRun(t, @@ -102,7 +102,7 @@ func TestReduceNestingDepthNoChangeNotLastStatement(t *testing.T) { ) } -// Skips an `if err == nil` in a loop body, where the early return would exit the whole function. +// Skips an `if err == nil` in a loop body. func TestReduceNestingDepthNoChangeInsideLoop(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&style.ReduceNestingDepth{}) spec.RewriteRun(t, diff --git a/tests/style/use_strong_hash_test.go b/tests/style/use_strong_hash_test.go index 47dc7e6..508ebbf 100644 --- a/tests/style/use_strong_hash_test.go +++ b/tests/style/use_strong_hash_test.go @@ -38,9 +38,7 @@ func TestUseStrongHashMd5New(t *testing.T) { ) } -// md5.Sum is intentionally not rewritten: it returns [16]byte while -// sha256.Sum256 returns [32]byte, so a local swap does not compile in typed -// contexts and would need a whole-usage migration. +// md5.Sum is intentionally left alone: its result type differs from sha256.Sum256. func TestUseStrongHashNoChangeMd5Sum(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&style.UseStrongHash{}) spec.RewriteRun(t, @@ -84,8 +82,7 @@ func TestUseStrongHashSha1New(t *testing.T) { ) } -// sha1.Sum is intentionally not rewritten for the same [20]byte vs [32]byte -// reason as md5.Sum. +// sha1.Sum is intentionally left alone: its result type differs from sha256.Sum256. func TestUseStrongHashNoChangeSha1Sum(t *testing.T) { spec := test.NewRecipeSpec().WithRecipe(&style.UseStrongHash{}) spec.RewriteRun(t,