go/analysis/passes/modernize: fmtappendf: remove whitespace

The text edits that remove "[]byte" from multi-line conversions like

return []byte(
   fmt.Sprintf(...),
)

do not delete the space after the first "(" and before the last ")", which can result in invalid code like:

return
   fmt.Appendf(...)

This CL corrects the range to delete to include spaces or other non-argument characters. Unfortunately, this may result in deleting comments.

Also, add fmt.Sprint to the list of formatters that have different nil-semantics from their Append counterparts.
(See CL 745540)

Fixes golang/go#77557, golang/go#77821

Change-Id: Ia9e9325f7e77acabc52e8e084ce3c465b3a6b9d7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/749521
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Madeline Kalil <mkalil@google.com>

Author: madelinekalil

go/analysis/passes/modernize/fmtappendf.go

@@ -54,12 +54,12 @@ func fmtappendf(pass *analysis.Pass) (any, error) {
 					if len(call.Args) == 0 {
 						continue
 					}
-					// fmt.Sprintf and fmt.Appendf have different nil semantics
+					// fmt.Sprint(f) and fmt.Append(f) have different nil semantics
 					// when the format produces an empty string:
-					// ([]byte(fmt.Sprintf("")) returns an empty but non-nil
+					// []byte(fmt.Sprintf("")) returns an empty but non-nil
 					// []byte{}, while fmt.Appendf(nil, "") returns nil) so we
 					// should skip these cases.
-					if fn.Name() == "Sprintf" {
+					if fn.Name() == "Sprint" || fn.Name() == "Sprintf" {
 						format := info.Types[call.Args[0]].Value
 						if format != nil && mayFormatEmpty(constant.StringVal(format)) {
 							continue
@@ -78,13 +78,18 @@ func fmtappendf(pass *analysis.Pass) (any, error) {
 					old, new := fn.Name(), strings.Replace(fn.Name(), "Sprint", "Append", 1)
 					edits := []analysis.TextEdit{
 						{
-							// delete "[]byte("
+							// Delete "[]byte(", including any spaces before the first argument.
 							Pos: conv.Pos(),
-							End: conv.Lparen + 1,
+							End: conv.Args[0].Pos(), // always exactly one argument in a valid byte slice conversion
 						},
 						{
-							// remove ")"
-							Pos: conv.Rparen,
+							// Delete ")", including any non-args (space or
+							// commas) that come before the right parenthesis.
+							// Leaving an extra comma here produces invalid
+							// code. (See golang/go#74709)
+							// Unfortunately, this and the edit above may result
+							// in deleting some comments.
+							Pos: conv.Args[0].End(),
 							End: conv.Rparen + 1,
 						},
 						{
@@ -97,20 +102,6 @@ func fmtappendf(pass *analysis.Pass) (any, error) {
 							NewText: []byte("nil, "),
 						},
 					}
-					if len(conv.Args) == 1 {
-						arg := conv.Args[0]
-						// Determine if we have T(fmt.SprintX(...)<non-args,
-						// like a space or a comma>). If so, delete the non-args
-						// that come before the right parenthesis. Leaving an
-						// extra comma here produces invalid code. (See
-						// golang/go#74709)
-						if arg.End() < conv.Rparen {
-							edits = append(edits, analysis.TextEdit{
-								Pos: arg.End(),
-								End: conv.Rparen,
-							})
-						}
-					}
 					if !analyzerutil.FileUsesGoVersion(pass, astutil.EnclosingFile(curCall), versions.Go1_19) {
 						continue
 					}

go/analysis/passes/modernize/testdata/src/fmtappendf/fmtappendf.go

@@ -47,7 +47,7 @@ func emptystring() {
 	// empty string edge case only applies to Sprintf
 	_ = []byte(fmt.Sprintln("")) // want "Replace .*Sprintln.* with fmt.Appendln"
 	// nope - these return []byte{}, while the fmt.Append version returns nil
-	_ = []byte(fmt.Sprintf(""))
+	_ = []byte(fmt.Sprint(""))
 	_ = []byte(fmt.Sprintf("%s", ""))
 	_ = []byte(fmt.Sprintf("%#s", ""))
 	_ = []byte(fmt.Sprintf("%s%v", "", getString()))
@@ -58,6 +58,15 @@ func emptystring() {
 	_ = []byte(fmt.Sprintf("%vother", "")) // want "Replace .*Sprint.* with fmt.Appendf"
 }
 
+func multiline() []byte {
+	_ = []byte( // want "Replace .*Sprintf.* with fmt.Appendf"
+		fmt.Sprintf("str %d", 1))
+
+	return []byte( // want "Replace .*Sprintf.* with fmt.Appendf"
+		fmt.Sprintf("str %d", 1),
+	)
+}
+
 func getString() string {
 	return ""
 }

go/analysis/passes/modernize/testdata/src/fmtappendf/fmtappendf.go.golden

@@ -33,19 +33,17 @@ func comma() {
 	type S struct{ Bytes []byte }
 	var _ = struct{ A S }{
 		A: S{
-			Bytes: // want "Replace .*Sprint.* with fmt.Appendf"
-				fmt.Appendf(nil, "%d", 0),
+			Bytes: fmt.Appendf(nil, "%d", 0),
 		},
 	}
-	_ = // want "Replace .*Sprint.* with fmt.Appendf" 
-		fmt.Appendf(nil, "%d", 0)          
+	_ = fmt.Appendf(nil, "%d", 0)          
 }
 
 func emptystring() {
 	// empty string edge case only applies to Sprintf
 	_ = fmt.Appendln(nil, "")  // want "Replace .*Sprintln.* with fmt.Appendln"
 	// nope - these return []byte{}, while the fmt.Append version returns nil
-	_ = []byte(fmt.Sprintf(""))
+	_ = []byte(fmt.Sprint(""))
 	_ = []byte(fmt.Sprintf("%s", ""))
 	_ = []byte(fmt.Sprintf("%#s", ""))
 	_ = []byte(fmt.Sprintf("%s%v", "", getString()))
@@ -56,6 +54,12 @@ func emptystring() {
 	_ = fmt.Appendf(nil, "%vother", "") // want "Replace .*Sprint.* with fmt.Appendf"
 }
 
+func multiline() []byte {
+	_ = fmt.Appendf(nil, "str %d", 1)
+
+	return fmt.Appendf(nil, "str %d", 1)
+}
+
 func getString() string {
 	return ""
 }