gopls/internal/analysis/modernize: invalid code produced by fmtappendf modernizer

The fmtappendf produces invalid code when the byte[] conversion contains a comma like
T(x,). The fix involves adding an additional text edit to remove the comma (or any whitespace
before the right parenthesis) in this case.

There still exists some strange formatting from white space left over, but it's hard
to remove this without also removing comments.

Fixes golang/go#74709

Change-Id: I80c5a88c590897fa40809718beeb1d3d295da86b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/697342
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Peter Weinberger <[email protected]>

Author: madelinekalil

gopls/internal/analysis/modernize/fmtappendf.go

@@ -45,34 +45,49 @@ func fmtappendf(pass *analysis.Pass) {
 					}
 
 					old, new := fn.Name(), strings.Replace(fn.Name(), "Sprint", "Append", 1)
+					edits := []analysis.TextEdit{
+						{
+							// delete "[]byte("
+							Pos: conv.Pos(),
+							End: conv.Lparen + 1,
+						},
+						{
+							// remove ")"
+							Pos: conv.Rparen,
+							End: conv.Rparen + 1,
+						},
+						{
+							Pos:     id.Pos(),
+							End:     id.End(),
+							NewText: []byte(new),
+						},
+						{
+							Pos:     call.Lparen + 1,
+							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,
+							})
+						}
+					}
 					pass.Report(analysis.Diagnostic{
 						Pos:      conv.Pos(),
 						End:      conv.End(),
 						Category: "fmtappendf",
 						Message:  fmt.Sprintf("Replace []byte(fmt.%s...) with fmt.%s", old, new),
 						SuggestedFixes: []analysis.SuggestedFix{{
-							Message: fmt.Sprintf("Replace []byte(fmt.%s...) with fmt.%s", old, new),
-							TextEdits: []analysis.TextEdit{
-								{
-									// delete "[]byte("
-									Pos: conv.Pos(),
-									End: conv.Lparen + 1,
-								},
-								{
-									// remove ")"
-									Pos: conv.Rparen,
-									End: conv.Rparen + 1,
-								},
-								{
-									Pos:     id.Pos(),
-									End:     id.End(),
-									NewText: []byte(new),
-								},
-								{
-									Pos:     call.Lparen + 1,
-									NewText: []byte("nil, "),
-								},
-							},
+							Message:   fmt.Sprintf("Replace []byte(fmt.%s...) with fmt.%s", old, new),
+							TextEdits: edits,
 						}},
 					})
 				}

gopls/internal/analysis/modernize/testdata/src/fmtappendf/fmtappendf.go

@@ -9,28 +9,36 @@ func two() string {
 }
 
 func bye() {
-	bye := []byte(fmt.Sprintf("bye %d", 1)) // want "Replace .*Sprintf.* with fmt.Appendf"
-	print(bye)
+	_ = []byte(fmt.Sprintf("bye %d", 1)) // want "Replace .*Sprintf.* with fmt.Appendf"
 }
 
 func funcsandvars() {
 	one := "one"
-	bye := []byte(fmt.Sprintf("bye %d %s %s", 1, two(), one)) // want "Replace .*Sprintf.* with fmt.Appendf"
-	print(bye)
+	_ = []byte(fmt.Sprintf("bye %d %s %s", 1, two(), one)) // want "Replace .*Sprintf.* with fmt.Appendf"
 }
 
 func typealias() {
 	type b = byte
 	type bt = []byte
-	bye := []b(fmt.Sprintf("bye %d", 1)) // want "Replace .*Sprintf.* with fmt.Appendf"
-	print(bye)
-	bye = bt(fmt.Sprintf("bye %d", 1)) // want "Replace .*Sprintf.* with fmt.Appendf"
-	print(bye)
+	_ = []b(fmt.Sprintf("bye %d", 1)) // want "Replace .*Sprintf.* with fmt.Appendf"
+	_ = bt(fmt.Sprintf("bye %d", 1))  // want "Replace .*Sprintf.* with fmt.Appendf"
 }
 
 func otherprints() {
-	sprint := []byte(fmt.Sprint("bye %d", 1)) // want "Replace .*Sprint.* with fmt.Append"
-	print(sprint)
-	sprintln := []byte(fmt.Sprintln("bye %d", 1)) // want "Replace .*Sprintln.* with fmt.Appendln"
-	print(sprintln)
+	_ = []byte(fmt.Sprint("bye %d", 1))   // want "Replace .*Sprint.* with fmt.Append"
+	_ = []byte(fmt.Sprintln("bye %d", 1)) // want "Replace .*Sprintln.* with fmt.Appendln"
+}
+
+func comma() {
+	type S struct{ Bytes []byte }
+	var _ = struct{ A S }{
+		A: S{
+			Bytes: []byte( // want "Replace .*Sprint.* with fmt.Appendf"
+				fmt.Sprintf("%d", 0),
+			),
+		},
+	}
+	_ = []byte( // want "Replace .*Sprint.* with fmt.Appendf"
+		fmt.Sprintf("%d", 0),
+	)
 }

gopls/internal/analysis/modernize/testdata/src/fmtappendf/fmtappendf.go.golden

@@ -9,28 +9,34 @@ func two() string {
 }
 
 func bye() {
-	bye := fmt.Appendf(nil, "bye %d", 1) // want "Replace .*Sprintf.* with fmt.Appendf"
-	print(bye)
+	_ = fmt.Appendf(nil, "bye %d", 1) // want "Replace .*Sprintf.* with fmt.Appendf"
 }
 
 func funcsandvars() {
 	one := "one"
-	bye := fmt.Appendf(nil, "bye %d %s %s", 1, two(), one) // want "Replace .*Sprintf.* with fmt.Appendf"
-	print(bye)
+	_ = fmt.Appendf(nil, "bye %d %s %s", 1, two(), one) // want "Replace .*Sprintf.* with fmt.Appendf"
 }
 
 func typealias() {
 	type b = byte
 	type bt = []byte
-	bye := fmt.Appendf(nil, "bye %d", 1) // want "Replace .*Sprintf.* with fmt.Appendf"
-	print(bye)
-	bye = fmt.Appendf(nil, "bye %d", 1) // want "Replace .*Sprintf.* with fmt.Appendf"
-	print(bye)
+	_ = fmt.Appendf(nil, "bye %d", 1) // want "Replace .*Sprintf.* with fmt.Appendf"
+	_ = fmt.Appendf(nil, "bye %d", 1) // want "Replace .*Sprintf.* with fmt.Appendf"
 }
 
 func otherprints() {
-	sprint := fmt.Append(nil, "bye %d", 1) // want "Replace .*Sprint.* with fmt.Append"
-	print(sprint)
-	sprintln := fmt.Appendln(nil, "bye %d", 1) // want "Replace .*Sprintln.* with fmt.Appendln"
-	print(sprintln)
+	_ = fmt.Append(nil, "bye %d", 1) // want "Replace .*Sprint.* with fmt.Append"
+	_ = fmt.Appendln(nil, "bye %d", 1) // want "Replace .*Sprintln.* with fmt.Appendln"
+}
+
+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),
+		},
+	}
+	_ = // want "Replace .*Sprint.* with fmt.Appendf" 
+		fmt.Appendf(nil, "%d", 0)          
 }