internal/lsp/analysis: quick-fix to remove unnecessary type arguments

This CL adds a new infertypeargs analyzer, which finds call exprs where
type arguments could be inferred, and suggests a quick fix to simplify
them.

Along the way, may two changes to the supporting frameworks:
 - Initialized types.Info.Instances in go/packages
 - Fail analysis tests run with suggested fixes if formatting the
   resulting source fails.

Change-Id: Ib15e5bd7c26aa293c5fc18a4cff6bc047e9e31d2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/351552
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

go/analysis/analysistest/analysistest.go

@@ -196,12 +196,13 @@ func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns
 							want := string(bytes.TrimRight(vf.Data, "\n")) + "\n"
 							formatted, err := format.Source([]byte(out))
 							if err != nil {
+								t.Errorf("%s: error formatting edited source: %v\n%s", file.Name(), err, out)
 								continue
 							}
 							if want != string(formatted) {
 								d, err := myers.ComputeEdits("", want, string(formatted))
 								if err != nil {
-									t.Errorf("failed to compute suggested fixes: %v", err)
+									t.Errorf("failed to compute suggested fix diff: %v", err)
 								}
 								t.Errorf("suggested fixes failed for %s:\n%s", file.Name(), diff.ToUnified(fmt.Sprintf("%s.golden [%s]", file.Name(), sf), "actual", want, d))
 							}
@@ -225,12 +226,13 @@ func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns
 
 				formatted, err := format.Source([]byte(out))
 				if err != nil {
+					t.Errorf("%s: error formatting resulting source: %v\n%s", file.Name(), err, out)
 					continue
 				}
 				if want != string(formatted) {
 					d, err := myers.ComputeEdits("", want, string(formatted))
 					if err != nil {
-						t.Errorf("failed to compute edits: %s", err)
+						t.Errorf("%s: failed to compute suggested fix diff: %s", file.Name(), err)
 					}
 					t.Errorf("suggested fixes failed for %s:\n%s", file.Name(), diff.ToUnified(file.Name()+".golden", "actual", want, d))
 				}

go/packages/packages.go

@@ -26,6 +26,7 @@ import (
 	"golang.org/x/tools/go/gcexportdata"
 	"golang.org/x/tools/internal/gocommand"
 	"golang.org/x/tools/internal/packagesinternal"
+	"golang.org/x/tools/internal/typeparams"
 	"golang.org/x/tools/internal/typesinternal"
 )
 
@@ -910,6 +911,7 @@ func (ld *loader) loadPackage(lpkg *loaderPackage) {
 		Scopes:     make(map[ast.Node]*types.Scope),
 		Selections: make(map[*ast.SelectorExpr]*types.Selection),
 	}
+	typeparams.InitInstanceInfo(lpkg.TypesInfo)
 	lpkg.TypesSizes = ld.sizes
 
 	importer := importerFunc(func(path string) (*types.Package, error) {

gopls/doc/analyzers.md

@@ -182,6 +182,22 @@ The Read method in v has a different signature than the Read method in
 io.Reader, so this assertion cannot succeed.
 
 
+**Enabled by default.**
+
+## **infertypeargs**
+
+check for unnecessary type arguments in call expressions
+
+Explicit type arguments may be omitted from call expressions if they can be
+inferred from function arguments, or from other type arguments:
+
+func f[T any](T) {}
+
+func _() {
+	f[string]("foo") // string could be inferred
+}
+
+
 **Enabled by default.**
 
 ## **loopclosure**

internal/lsp/analysis/infertypeargs/infertypeargs.go

@@ -0,0 +1,31 @@
+// 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 infertypeargs defines an analyzer that checks for explicit function
+// arguments that could be inferred.
+package infertypeargs
+
+import (
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+)
+
+const Doc = `check for unnecessary type arguments in call expressions
+
+Explicit type arguments may be omitted from call expressions if they can be
+inferred from function arguments, or from other type arguments:
+
+func f[T any](T) {}
+
+func _() {
+	f[string]("foo") // string could be inferred
+}
+`
+
+var Analyzer = &analysis.Analyzer{
+	Name:     "infertypeargs",
+	Doc:      Doc,
+	Requires: []*analysis.Analyzer{inspect.Analyzer},
+	Run:      run,
+}

internal/lsp/analysis/infertypeargs/infertypeargs_test.go

@@ -0,0 +1,23 @@
+// 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 infertypeargs_test
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+	"golang.org/x/tools/internal/lsp/analysis/infertypeargs"
+	"golang.org/x/tools/internal/testenv"
+	"golang.org/x/tools/internal/typeparams"
+)
+
+func Test(t *testing.T) {
+	testenv.NeedsGo1Point(t, 13)
+	if !typeparams.Enabled {
+		t.Skip("type params are not enabled")
+	}
+	testdata := analysistest.TestData()
+	analysistest.RunWithSuggestedFixes(t, testdata, infertypeargs.Analyzer, "a")
+}

internal/lsp/analysis/infertypeargs/run_go117.go

@@ -0,0 +1,16 @@
+// 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.
+
+//go:build !go1.18
+// +build !go1.18
+
+package infertypeargs
+
+import "golang.org/x/tools/go/analysis"
+
+// This analyzer only relates to go1.18+, and uses the types.CheckExpr API that
+// was added in Go 1.13.
+func run(pass *analysis.Pass) (interface{}, error) {
+	return nil, nil
+}

internal/lsp/analysis/infertypeargs/run_go118.go

@@ -0,0 +1,115 @@
+// 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.
+
+//go:build go1.18
+// +build go1.18
+
+package infertypeargs
+
+import (
+	"go/ast"
+	"go/token"
+	"go/types"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/go/ast/inspector"
+	"golang.org/x/tools/internal/typeparams"
+)
+
+func run(pass *analysis.Pass) (interface{}, error) {
+	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+
+	nodeFilter := []ast.Node{
+		(*ast.CallExpr)(nil),
+	}
+
+	inspect.Preorder(nodeFilter, func(node ast.Node) {
+		call := node.(*ast.CallExpr)
+		ident, ix := instanceData(call)
+		if ix == nil || len(ix.Indices) == 0 {
+			return // no explicit args, nothing to do
+		}
+
+		// Confirm that instantiation actually occurred at this ident.
+		_, instance := typeparams.GetInstance(pass.TypesInfo, ident)
+		if instance == nil {
+			return // something went wrong, but fail open
+		}
+
+		// Start removing argument expressions from the right, and check if we can
+		// still infer the call expression.
+		required := len(ix.Indices) // number of type expressions that are required
+		for i := len(ix.Indices) - 1; i >= 0; i-- {
+			var fun ast.Expr
+			if i == 0 {
+				// No longer an index expression: just use the parameterized operand.
+				fun = ix.X
+			} else {
+				fun = typeparams.PackIndexExpr(ix.X, ix.Lbrack, ix.Indices[:i], ix.Indices[i-1].End())
+			}
+			newCall := &ast.CallExpr{
+				Fun:      fun,
+				Lparen:   call.Lparen,
+				Args:     call.Args,
+				Ellipsis: call.Ellipsis,
+				Rparen:   call.Rparen,
+			}
+			info := new(types.Info)
+			typeparams.InitInstanceInfo(info)
+			if err := types.CheckExpr(pass.Fset, pass.Pkg, call.Pos(), newCall, info); err != nil {
+				// Most likely inference failed.
+				break
+			}
+			_, newInstance := typeparams.GetInstance(info, ident)
+			if !types.Identical(instance, newInstance) {
+				// The inferred result type does not match the original result type, so
+				// this simplification is not valid.
+				break
+			}
+			required = i
+		}
+		if required < len(ix.Indices) {
+			var start, end token.Pos
+			if required == 0 {
+				start, end = ix.Lbrack, ix.Rbrack+1 // erase the entire index
+			} else {
+				start = ix.Indices[required-1].End()
+				end = ix.Rbrack
+			}
+			pass.Report(analysis.Diagnostic{
+				Pos:     start,
+				End:     end,
+				Message: "unnecessary type arguments",
+				SuggestedFixes: []analysis.SuggestedFix{{
+					Message: "simplify type arguments",
+					TextEdits: []analysis.TextEdit{{
+						Pos: start,
+						End: end,
+					}},
+				}},
+			})
+		}
+	})
+
+	return nil, nil
+}
+
+// instanceData returns the instantiated identifier and index data.
+func instanceData(call *ast.CallExpr) (*ast.Ident, *typeparams.IndexExprData) {
+	ix := typeparams.GetIndexExprData(call.Fun)
+	if ix == nil {
+		return nil, nil
+	}
+	var id *ast.Ident
+	switch x := ix.X.(type) {
+	case *ast.SelectorExpr:
+		id = x.Sel
+	case *ast.Ident:
+		id = x
+	default:
+		return nil, nil
+	}
+	return id, ix
+}

internal/lsp/analysis/infertypeargs/testdata/src/a/basic.go

@@ -0,0 +1,20 @@
+// 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.
+
+// This file contains tests for the infertyepargs checker.
+
+package a
+
+func f[T any](T) {}
+
+func g[T any]() T { var x T; return x }
+
+func h[P interface{ ~*T }, T any]() {}
+
+func _() {
+	f[string]("hello") // want "unnecessary type arguments"
+	f[int](2)          // want "unnecessary type arguments"
+	_ = g[int]()
+	h[*int, int]() // want "unnecessary type arguments"
+}

internal/lsp/analysis/infertypeargs/testdata/src/a/basic.go.golden

@@ -0,0 +1,20 @@
+// 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.
+
+// This file contains tests for the infertyepargs checker.
+
+package a
+
+func f[T any](T) {}
+
+func g[T any]() T { var x T; return x }
+
+func h[P interface{ ~*T }, T any]() {}
+
+func _() {
+	f("hello") // want "unnecessary type arguments"
+	f(2)       // want "unnecessary type arguments"
+	_ = g[int]()
+	h[*int]() // want "unnecessary type arguments"
+}

internal/lsp/analysis/infertypeargs/testdata/src/a/imported.go

@@ -0,0 +1,12 @@
+// 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 a
+
+import "a/imported"
+
+func _() {
+	var x int
+	imported.F[int](x) // want "unnecessary type arguments"
+}

internal/lsp/analysis/infertypeargs/testdata/src/a/imported.go.golden

@@ -0,0 +1,12 @@
+// 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 a
+
+import "a/imported"
+
+func _() {
+	var x int
+	imported.F(x) // want "unnecessary type arguments"
+}

internal/lsp/analysis/infertypeargs/testdata/src/a/imported/imported.go

@@ -0,0 +1,7 @@
+// 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 imported
+
+func F[T any](T) {}

internal/lsp/analysis/infertypeargs/testdata/src/a/notypechange.go

@@ -0,0 +1,26 @@
+// 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.
+
+// We should not suggest removing type arguments if doing so would change the
+// resulting type.
+
+package a
+
+func id[T any](t T) T { return t }
+
+var _ = id[int](1)        // want "unnecessary type arguments"
+var _ = id[string]("foo") // want "unnecessary type arguments"
+var _ = id[int64](2)
+
+func pair[T any](t T) (T, T) { return t, t }
+
+var _, _ = pair[int](3) // want "unnecessary type arguments"
+var _, _ = pair[int64](3)
+
+func noreturn[T any](t T) {}
+
+func _() {
+	noreturn[int64](4)
+	noreturn[int](4) // want "unnecessary type arguments"
+}

internal/lsp/analysis/infertypeargs/testdata/src/a/notypechange.go.golden

@@ -0,0 +1,26 @@
+// 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.
+
+// We should not suggest removing type arguments if doing so would change the
+// resulting type.
+
+package a
+
+func id[T any](t T) T { return t }
+
+var _ = id(1)     // want "unnecessary type arguments"
+var _ = id("foo") // want "unnecessary type arguments"
+var _ = id[int64](2)
+
+func pair[T any](t T) (T, T) { return t, t }
+
+var _, _ = pair(3) // want "unnecessary type arguments"
+var _, _ = pair[int64](3)
+
+func noreturn[T any](t T) {}
+
+func _() {
+	noreturn[int64](4)
+	noreturn(4) // want "unnecessary type arguments"
+}

internal/lsp/source/api_json.go

@@ -441,6 +441,11 @@ var GeneratedAPIJSON = &APIJSON{
 							Doc:     "detect impossible interface-to-interface type assertions\n\nThis checker flags type assertions v.(T) and corresponding type-switch cases\nin which the static type V of v is an interface that cannot possibly implement\nthe target interface T. This occurs when V and T contain methods with the same\nname but different signatures. Example:\n\n\tvar v interface {\n\t\tRead()\n\t}\n\t_ = v.(io.Reader)\n\nThe Read method in v has a different signature than the Read method in\nio.Reader, so this assertion cannot succeed.\n",
 							Default: "true",
 						},
+						{
+							Name:    "\"infertypeargs\"",
+							Doc:     "check for unnecessary type arguments in call expressions\n\nExplicit type arguments may be omitted from call expressions if they can be\ninferred from function arguments, or from other type arguments:\n\nfunc f[T any](T) {}\n\nfunc _() {\n\tf[string](\"foo\") // string could be inferred\n}\n",
+							Default: "true",
+						},
 						{
 							Name:    "\"loopclosure\"",
 							Doc:     "check references to loop variables from within nested functions\n\nThis analyzer checks for references to loop variables from within a\nfunction literal inside the loop body. It checks only instances where\nthe function literal is called in a defer or go statement that is the\nlast statement in the loop body, as otherwise we would need whole\nprogram analysis.\n\nFor example:\n\n\tfor i, v := range s {\n\t\tgo func() {\n\t\t\tprintln(i, v) // not what you might expect\n\t\t}()\n\t}\n\nSee: https://golang.org/doc/go_faq.html#closures_and_goroutines",
@@ -1014,6 +1019,11 @@ var GeneratedAPIJSON = &APIJSON{
 			Doc:     "detect impossible interface-to-interface type assertions\n\nThis checker flags type assertions v.(T) and corresponding type-switch cases\nin which the static type V of v is an interface that cannot possibly implement\nthe target interface T. This occurs when V and T contain methods with the same\nname but different signatures. Example:\n\n\tvar v interface {\n\t\tRead()\n\t}\n\t_ = v.(io.Reader)\n\nThe Read method in v has a different signature than the Read method in\nio.Reader, so this assertion cannot succeed.\n",
 			Default: true,
 		},
+		{
+			Name:    "infertypeargs",
+			Doc:     "check for unnecessary type arguments in call expressions\n\nExplicit type arguments may be omitted from call expressions if they can be\ninferred from function arguments, or from other type arguments:\n\nfunc f[T any](T) {}\n\nfunc _() {\n\tf[string](\"foo\") // string could be inferred\n}\n",
+			Default: true,
+		},
 		{
 			Name:    "loopclosure",
 			Doc:     "check references to loop variables from within nested functions\n\nThis analyzer checks for references to loop variables from within a\nfunction literal inside the loop body. It checks only instances where\nthe function literal is called in a defer or go statement that is the\nlast statement in the loop body, as otherwise we would need whole\nprogram analysis.\n\nFor example:\n\n\tfor i, v := range s {\n\t\tgo func() {\n\t\t\tprintln(i, v) // not what you might expect\n\t\t}()\n\t}\n\nSee: https://golang.org/doc/go_faq.html#closures_and_goroutines",

internal/lsp/source/options.go

@@ -49,6 +49,7 @@ import (
 	"golang.org/x/tools/go/analysis/passes/unusedwrite"
 	"golang.org/x/tools/internal/lsp/analysis/fillreturns"
 	"golang.org/x/tools/internal/lsp/analysis/fillstruct"
+	"golang.org/x/tools/internal/lsp/analysis/infertypeargs"
 	"golang.org/x/tools/internal/lsp/analysis/nonewvars"
 	"golang.org/x/tools/internal/lsp/analysis/noresultvalues"
 	"golang.org/x/tools/internal/lsp/analysis/simplifycompositelit"
@@ -1238,6 +1239,7 @@ func defaultAnalyzers() map[string]*Analyzer {
 		unusedparams.Analyzer.Name:     {Analyzer: unusedparams.Analyzer, Enabled: false},
 		unusedwrite.Analyzer.Name:      {Analyzer: unusedwrite.Analyzer, Enabled: false},
 		useany.Analyzer.Name:           {Analyzer: useany.Analyzer, Enabled: true},
+		infertypeargs.Analyzer.Name:    {Analyzer: infertypeargs.Analyzer, Enabled: true},
 
 		// gofmt -s suite:
 		simplifycompositelit.Analyzer.Name: {

internal/typeparams/typeparams_go117.go

@@ -9,6 +9,7 @@ package typeparams
 
 import (
 	"go/ast"
+	"go/token"
 	"go/types"
 )
 
@@ -30,6 +31,24 @@ func GetIndexExprData(n ast.Node) *IndexExprData {
 	return nil
 }
 
+// PackIndexExpr returns an *ast.IndexExpr with the given index.
+// Calling PackIndexExpr with len(indices) != 1 will panic.
+func PackIndexExpr(x ast.Expr, lbrack token.Pos, indices []ast.Expr, rbrack token.Pos) ast.Expr {
+	switch len(indices) {
+	case 0:
+		panic("empty indices")
+	case 1:
+		return &ast.IndexExpr{
+			X:      x,
+			Lbrack: lbrack,
+			Index:  indices[0],
+			Rbrack: rbrack,
+		}
+	default:
+		panic("cannot pack multiple indices at this go version")
+	}
+}
+
 // ForTypeSpec returns an empty field list, as type parameters on not supported
 // at this Go version.
 func ForTypeSpec(*ast.TypeSpec) *ast.FieldList {

internal/typeparams/typeparams_go118.go

@@ -9,6 +9,7 @@ package typeparams
 
 import (
 	"go/ast"
+	"go/token"
 	"go/types"
 )
 
@@ -36,6 +37,30 @@ func GetIndexExprData(n ast.Node) *IndexExprData {
 	return nil
 }
 
+// PackIndexExpr returns an *ast.IndexExpr or *ast.IndexListExpr, depending on
+// the cardinality of indices. Calling PackIndexExpr with len(indices) == 0
+// will panic.
+func PackIndexExpr(x ast.Expr, lbrack token.Pos, indices []ast.Expr, rbrack token.Pos) ast.Expr {
+	switch len(indices) {
+	case 0:
+		panic("empty indices")
+	case 1:
+		return &ast.IndexExpr{
+			X:      x,
+			Lbrack: lbrack,
+			Index:  indices[0],
+			Rbrack: rbrack,
+		}
+	default:
+		return &ast.IndexListExpr{
+			X:       x,
+			Lbrack:  lbrack,
+			Indices: indices,
+			Rbrack:  rbrack,
+		}
+	}
+}
+
 // ForTypeSpec returns n.TypeParams.
 func ForTypeSpec(n *ast.TypeSpec) *ast.FieldList {
 	if n == nil {