go/analysis/passes/stringintconv: offer fix of fmt.Sprint(x)

Previously, the offered fix was string(x) -> string(rune(x)),
which is not the most commonly desired change. Now, the analyzer
offers both fixes, with informative descriptions.

The Sprint fix must take care to preserve the type when it's
not exactly string. Also, it may require adding an import
of "fmt", using the new AddImport refactoring helper.

The "did you mean?" hint in the diagnostic (added by CL 235797
for golang/go#39151) has been removed, since the descriptions
of the offered fixes capture the same information, and there
was a recent editorial decision made against such hints.

Also:
- split the tests into tests of diagnostics and tests of fixes,
  since the latter now need to use the clumsy golden-file-is-a-txtar
  mechanism. Reduce the regexp patterns to just the minimum.
- clarify RunWithSuggestedFixes' explanation of txtar mechanism.
- clarify SuggestedFix.Message and provide an example of the
  proper form.

Googlers: see b/346560254

Change-Id: I493cf675a4c71d4ee40632fc9ad47a8071eafa76
Reviewed-on: https://go-review.googlesource.com/c/tools/+/592155
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Lasse Folger <lassefolger@google.com>
Reviewed-by: Tim King <taking@google.com>

go/analysis/analysistest/analysistest.go

@@ -114,9 +114,9 @@ type Testing interface {
 // into nonconflicting parts.
 //
 // Conflicts of the second kind can be avoided by giving the
-// alternative fixes different names (SuggestedFix.Message) and using
-// a multi-section .txtar file with a named section for each
-// alternative fix.
+// alternative fixes different names (SuggestedFix.Message) and
+// defining the .golden file as a multi-section txtar file with a
+// named section for each alternative fix, as shown above.
 //
 // Analyzers that compute fixes from a textual diff of the
 // before/after file contents (instead of directly from syntax tree

go/analysis/diagnostic.go

@@ -58,8 +58,10 @@ type RelatedInformation struct {
 //
 // The TextEdits must not overlap, nor contain edits for other packages.
 type SuggestedFix struct {
-	// A description for this suggested fix to be shown to a user deciding
-	// whether to accept it.
+	// A verb phrase describing the fix, to be shown to
+	// a user trying to decide whether to accept it.
+	//
+	// Example: "Remove the surplus argument"
 	Message   string
 	TextEdits []TextEdit
 }

go/analysis/passes/stringintconv/string.go

@@ -16,6 +16,7 @@ import (
 	"golang.org/x/tools/go/analysis/passes/internal/analysisutil"
 	"golang.org/x/tools/go/ast/inspector"
 	"golang.org/x/tools/internal/aliases"
+	"golang.org/x/tools/internal/analysisinternal"
 	"golang.org/x/tools/internal/typeparams"
 )
 
@@ -73,9 +74,15 @@ func typeName(t types.Type) string {
 func run(pass *analysis.Pass) (interface{}, error) {
 	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
 	nodeFilter := []ast.Node{
+		(*ast.File)(nil),
 		(*ast.CallExpr)(nil),
 	}
+	var file *ast.File
 	inspect.Preorder(nodeFilter, func(n ast.Node) {
+		if n, ok := n.(*ast.File); ok {
+			file = n
+			return
+		}
 		call := n.(*ast.CallExpr)
 
 		if len(call.Args) != 1 {
@@ -167,28 +174,75 @@ func run(pass *analysis.Pass) (interface{}, error) {
 
 		diag := analysis.Diagnostic{
 			Pos:     n.Pos(),
-			Message: fmt.Sprintf("conversion from %s to %s yields a string of one rune, not a string of digits (did you mean fmt.Sprint(x)?)", source, target),
+			Message: fmt.Sprintf("conversion from %s to %s yields a string of one rune, not a string of digits", source, target),
+		}
+		addFix := func(message string, edits []analysis.TextEdit) {
+			diag.SuggestedFixes = append(diag.SuggestedFixes, analysis.SuggestedFix{
+				Message:   message,
+				TextEdits: edits,
+			})
 		}
 
-		if convertibleToRune {
-			diag.SuggestedFixes = []analysis.SuggestedFix{
-				{
-					Message: "Did you mean to convert a rune to a string?",
-					TextEdits: []analysis.TextEdit{
-						{
-							Pos:     arg.Pos(),
-							End:     arg.Pos(),
-							NewText: []byte("rune("),
-						},
-						{
-							Pos:     arg.End(),
-							End:     arg.End(),
-							NewText: []byte(")"),
-						},
+		// Fix 1: use fmt.Sprint(x)
+		//
+		// Prefer fmt.Sprint over strconv.Itoa, FormatInt,
+		// or FormatUint, as it works for any type.
+		// Add an import of "fmt" as needed.
+		//
+		// Unless the type is exactly string, we must retain the conversion.
+		//
+		// Do not offer this fix if type parameters are involved,
+		// as there are too many combinations and subtleties.
+		// Consider x = rune | int16 | []byte: in all cases,
+		// string(x) is legal, but the appropriate diagnostic
+		// and fix differs. Similarly, don't offer the fix if
+		// the type has methods, as some {String,GoString,Format}
+		// may change the behavior of fmt.Sprint.
+		if len(ttypes) == 1 && len(vtypes) == 1 && types.NewMethodSet(V0).Len() == 0 {
+			fmtName, importEdit := analysisinternal.AddImport(pass.TypesInfo, file, arg.Pos(), "fmt", "fmt")
+			if types.Identical(T0, types.Typ[types.String]) {
+				// string(x) -> fmt.Sprint(x)
+				addFix("Format the number as a decimal", []analysis.TextEdit{
+					importEdit,
+					{
+						Pos:     call.Fun.Pos(),
+						End:     call.Fun.End(),
+						NewText: []byte(fmtName + ".Sprint"),
 					},
-				},
+				})
+			} else {
+				// mystring(x) -> mystring(fmt.Sprint(x))
+				addFix("Format the number as a decimal", []analysis.TextEdit{
+					importEdit,
+					{
+						Pos:     call.Lparen + 1,
+						End:     call.Lparen + 1,
+						NewText: []byte(fmtName + ".Sprint("),
+					},
+					{
+						Pos:     call.Rparen,
+						End:     call.Rparen,
+						NewText: []byte(")"),
+					},
+				})
 			}
 		}
+
+		// Fix 2: use string(rune(x))
+		if convertibleToRune {
+			addFix("Convert a single rune to a string", []analysis.TextEdit{
+				{
+					Pos:     arg.Pos(),
+					End:     arg.Pos(),
+					NewText: []byte("rune("),
+				},
+				{
+					Pos:     arg.End(),
+					End:     arg.End(),
+					NewText: []byte(")"),
+				},
+			})
+		}
 		pass.Report(diag)
 	})
 	return nil, nil

go/analysis/passes/stringintconv/string_test.go

@@ -13,5 +13,6 @@ import (
 
 func Test(t *testing.T) {
 	testdata := analysistest.TestData()
-	analysistest.RunWithSuggestedFixes(t, testdata, stringintconv.Analyzer, "a", "typeparams")
+	analysistest.Run(t, testdata, stringintconv.Analyzer, "a", "typeparams")
+	analysistest.RunWithSuggestedFixes(t, testdata, stringintconv.Analyzer, "fix")
 }

go/analysis/passes/stringintconv/testdata/src/a/a.go

@@ -25,12 +25,13 @@ func StringTest() {
 		o struct{ x int }
 	)
 	const p = 0
-	_ = string(i) // want `^conversion from int to string yields a string of one rune, not a string of digits \(did you mean fmt\.Sprint\(x\)\?\)$`
+	// First time only, assert the complete message:
+	_ = string(i) // want `^conversion from int to string yields a string of one rune, not a string of digits$`
 	_ = string(j)
 	_ = string(k)
-	_ = string(p)    // want `^conversion from untyped int to string yields a string of one rune, not a string of digits \(did you mean fmt\.Sprint\(x\)\?\)$`
-	_ = A(l)         // want `^conversion from C \(int\) to A \(string\) yields a string of one rune, not a string of digits \(did you mean fmt\.Sprint\(x\)\?\)$`
-	_ = B(m)         // want `^conversion from (uintptr|D \(uintptr\)) to B \(string\) yields a string of one rune, not a string of digits \(did you mean fmt\.Sprint\(x\)\?\)$`
-	_ = string(n[1]) // want `^conversion from int to string yields a string of one rune, not a string of digits \(did you mean fmt\.Sprint\(x\)\?\)$`
-	_ = string(o.x)  // want `^conversion from int to string yields a string of one rune, not a string of digits \(did you mean fmt\.Sprint\(x\)\?\)$`
+	_ = string(p)    // want `...from untyped int to string...`
+	_ = A(l)         // want `...from C \(int\) to A \(string\)...`
+	_ = B(m)         // want `...from (uintptr|D \(uintptr\)) to B \(string\)...`
+	_ = string(n[1]) // want `...from int to string...`
+	_ = string(o.x)  // want `...from int to string...`
 }

go/analysis/passes/stringintconv/testdata/src/a/a.go.golden

@@ -1,36 +0,0 @@
-// Copyright 2020 The Go Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style
-// license that can be found in the LICENSE file.
-
-// This file contains tests for the stringintconv checker.
-
-package a
-
-type A string
-
-type B = string
-
-type C int
-
-type D = uintptr
-
-func StringTest() {
-	var (
-		i int
-		j rune
-		k byte
-		l C
-		m D
-		n = []int{0, 1, 2}
-		o struct{ x int }
-	)
-	const p = 0
-	_ = string(rune(i)) // want `^conversion from int to string yields a string of one rune, not a string of digits \(did you mean fmt\.Sprint\(x\)\?\)$`
-	_ = string(j)
-	_ = string(k)
-	_ = string(rune(p))    // want `^conversion from untyped int to string yields a string of one rune, not a string of digits \(did you mean fmt\.Sprint\(x\)\?\)$`
-	_ = A(rune(l))         // want `^conversion from C \(int\) to A \(string\) yields a string of one rune, not a string of digits \(did you mean fmt\.Sprint\(x\)\?\)$`
-	_ = B(rune(m))         // want `^conversion from (uintptr|D \(uintptr\)) to B \(string\) yields a string of one rune, not a string of digits \(did you mean fmt\.Sprint\(x\)\?\)$`
-	_ = string(rune(n[1])) // want `^conversion from int to string yields a string of one rune, not a string of digits \(did you mean fmt\.Sprint\(x\)\?\)$`
-	_ = string(rune(o.x))  // want `^conversion from int to string yields a string of one rune, not a string of digits \(did you mean fmt\.Sprint\(x\)\?\)$`
-}

go/analysis/passes/stringintconv/testdata/src/fix/fix.go

@@ -0,0 +1,5 @@
+package fix
+
+func _(x uint64) {
+	println(string(x)) // want `conversion from uint64 to string yields...`
+}

go/analysis/passes/stringintconv/testdata/src/fix/fix.go.golden

@@ -0,0 +1,16 @@
+-- Format the number as a decimal --
+package fix
+
+import "fmt"
+
+func _(x uint64) {
+     println(fmt.Sprint(x)) // want `conversion from uint64 to string yields...`
+}
+
+-- Convert a single rune to a string --
+package fix
+
+func _(x uint64) {
+     println(string(rune(x))) // want `conversion from uint64 to string yields...`
+}
+

go/analysis/passes/stringintconv/testdata/src/fix/fixnamed.go

@@ -0,0 +1,7 @@
+package fix
+
+type mystring string
+
+func _(x int16) mystring {
+	return mystring(x) // want `conversion from int16 to mystring \(string\)...`
+}

go/analysis/passes/stringintconv/testdata/src/fix/fixnamed.go.golden

@@ -0,0 +1,19 @@
+-- Format the number as a decimal --
+package fix
+
+import "fmt"
+
+type mystring string
+
+func _(x int16) mystring {
+	return mystring(fmt.Sprint(x)) // want `conversion from int16 to mystring \(string\)...`
+}
+
+-- Convert a single rune to a string --
+package fix
+
+type mystring string
+
+func _(x int16) mystring {
+	return mystring(rune(x)) // want `conversion from int16 to mystring \(string\)...`
+}

go/analysis/passes/stringintconv/testdata/src/typeparams/typeparams.go

@@ -22,16 +22,16 @@ func _[AllString ~string, MaybeString ~string | ~int, NotString ~int | byte, Nam
 	)
 	const p = 0
 
-	_ = MaybeString(i) // want `conversion from int to string .in MaybeString. yields a string of one rune, not a string of digits .did you mean fmt\.Sprint.x.\?.`
+	_ = MaybeString(i) // want `conversion from int to string .in MaybeString. yields a string of one rune, not a string of digits`
 	_ = MaybeString(r)
 	_ = MaybeString(b)
-	_ = MaybeString(I) // want `conversion from Int .int. to string .in MaybeString. yields a string of one rune, not a string of digits .did you mean fmt\.Sprint.x.\?.`
-	_ = MaybeString(U) // want `conversion from uintptr to string .in MaybeString. yields a string of one rune, not a string of digits .did you mean fmt\.Sprint.x.\?.`
+	_ = MaybeString(I) // want `conversion from Int .int. to string .in MaybeString. yields a string of one rune, not a string of digits`
+	_ = MaybeString(U) // want `conversion from uintptr to string .in MaybeString. yields a string of one rune, not a string of digits`
 	// Type parameters are never constant types, so arguments are always
 	// converted to their default type (int versus untyped int, in this case)
-	_ = MaybeString(p) // want `conversion from int to string .in MaybeString. yields a string of one rune, not a string of digits .did you mean fmt\.Sprint.x.\?.`
+	_ = MaybeString(p) // want `conversion from int to string .in MaybeString. yields a string of one rune, not a string of digits`
 	// ...even if the type parameter is only strings.
-	_ = AllString(p) // want `conversion from int to string .in AllString. yields a string of one rune, not a string of digits .did you mean fmt\.Sprint.x.\?.`
+	_ = AllString(p) // want `conversion from int to string .in AllString. yields a string of one rune, not a string of digits`
 
 	_ = NotString(i)
 	_ = NotString(r)
@@ -40,10 +40,10 @@ func _[AllString ~string, MaybeString ~string | ~int, NotString ~int | byte, Nam
 	_ = NotString(U)
 	_ = NotString(p)
 
-	_ = NamedString(i) // want `conversion from int to String .string, in NamedString. yields a string of one rune, not a string of digits .did you mean fmt\.Sprint.x.\?.`
-	_ = string(M)      // want `conversion from int .in MaybeString. to string yields a string of one rune, not a string of digits .did you mean fmt\.Sprint.x.\?.`
+	_ = NamedString(i) // want `conversion from int to String .string, in NamedString. yields a string of one rune, not a string of digits`
+	_ = string(M)      // want `conversion from int .in MaybeString. to string yields a string of one rune, not a string of digits`
 
 	// Note that M is not convertible to rune.
-	_ = MaybeString(M) // want `conversion from int .in MaybeString. to string .in MaybeString. yields a string of one rune, not a string of digits .did you mean fmt\.Sprint.x.\?.`
+	_ = MaybeString(M) // want `conversion from int .in MaybeString. to string .in MaybeString. yields a string of one rune, not a string of digits`
 	_ = NotString(N)   // ok
 }

go/analysis/passes/stringintconv/testdata/src/typeparams/typeparams.go.golden

@@ -1,49 +0,0 @@
-// Copyright 2021 The Go Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style
-// license that can be found in the LICENSE file.
-
-package typeparams
-
-type (
-	Int     int
-	Uintptr = uintptr
-	String  string
-)
-
-func _[AllString ~string, MaybeString ~string | ~int, NotString ~int | byte, NamedString String | Int]() {
-	var (
-		i int
-		r rune
-		b byte
-		I Int
-		U uintptr
-		M MaybeString
-		N NotString
-	)
-	const p = 0
-
-	_ = MaybeString(rune(i)) // want `conversion from int to string .in MaybeString. yields a string of one rune, not a string of digits .did you mean fmt\.Sprint.x.\?.`
-	_ = MaybeString(r)
-	_ = MaybeString(b)
-	_ = MaybeString(rune(I)) // want `conversion from Int .int. to string .in MaybeString. yields a string of one rune, not a string of digits .did you mean fmt\.Sprint.x.\?.`
-	_ = MaybeString(rune(U)) // want `conversion from uintptr to string .in MaybeString. yields a string of one rune, not a string of digits .did you mean fmt\.Sprint.x.\?.`
-	// Type parameters are never constant types, so arguments are always
-	// converted to their default type (int versus untyped int, in this case)
-	_ = MaybeString(rune(p)) // want `conversion from int to string .in MaybeString. yields a string of one rune, not a string of digits .did you mean fmt\.Sprint.x.\?.`
-	// ...even if the type parameter is only strings.
-	_ = AllString(rune(p)) // want `conversion from int to string .in AllString. yields a string of one rune, not a string of digits .did you mean fmt\.Sprint.x.\?.`
-
-	_ = NotString(i)
-	_ = NotString(r)
-	_ = NotString(b)
-	_ = NotString(I)
-	_ = NotString(U)
-	_ = NotString(p)
-
-	_ = NamedString(rune(i)) // want `conversion from int to String .string, in NamedString. yields a string of one rune, not a string of digits .did you mean fmt\.Sprint.x.\?.`
-	_ = string(M)            // want `conversion from int .in MaybeString. to string yields a string of one rune, not a string of digits .did you mean fmt\.Sprint.x.\?.`
-
-	// Note that M is not convertible to rune.
-	_ = MaybeString(M) // want `conversion from int .in MaybeString. to string .in MaybeString. yields a string of one rune, not a string of digits .did you mean fmt\.Sprint.x.\?.`
-	_ = NotString(N)   // ok
-}