gopls/internal/lsp/source: make infertypeargs a convenience analyzer

Fixes golang/go#57930

Change-Id: I49fee92e92f29912a1f83f68dd3c1bf14f5dcfe2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/472181
gopls-CI: kokoro <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
Run-TryBot: Robert Findley <[email protected]>

Author: findleyr

gopls/doc/analyzers.md

@@ -215,22 +215,6 @@ name but different signatures. Example:
 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**
@@ -753,6 +737,22 @@ expensive to compute, callers should compute it separately, using the
 SuggestedFix function below.
 
 
+**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.**
 
 ## **stubmethods**

gopls/internal/lsp/regtest/marker.go

@@ -1335,41 +1335,51 @@ func rename(env *Env, loc protocol.Location, newName string) (map[string][]byte,
 		return nil, err
 	}
 
-	return applyDocumentChanges(env, editMap.DocumentChanges)
+	fileChanges := make(map[string][]byte)
+	if err := applyDocumentChanges(env, editMap.DocumentChanges, fileChanges); err != nil {
+		return nil, fmt.Errorf("applying document changes: %v", err)
+	}
+	return fileChanges, nil
 }
 
-// applyDocumentChanges returns the effect of applying the document
-// changes to the contents of the Editor buffers. The actual editor
-// buffers are unchanged.
-func applyDocumentChanges(env *Env, changes []protocol.DocumentChanges) (map[string][]byte, error) {
-	result := make(map[string][]byte)
+// applyDocumentChanges applies the given document changes to the editor buffer
+// content, recording the resulting contents in the fileChanges map. It is an
+// error for a change to an edit a file that is already present in the
+// fileChanges map.
+func applyDocumentChanges(env *Env, changes []protocol.DocumentChanges, fileChanges map[string][]byte) error {
+	getMapper := func(path string) (*protocol.Mapper, error) {
+		if _, ok := fileChanges[path]; ok {
+			return nil, fmt.Errorf("internal error: %s is already edited", path)
+		}
+		return env.Editor.Mapper(path)
+	}
+
 	for _, change := range changes {
 		if change.RenameFile != nil {
 			// rename
 			oldFile := env.Sandbox.Workdir.URIToPath(change.RenameFile.OldURI)
-			newFile := env.Sandbox.Workdir.URIToPath(change.RenameFile.NewURI)
-			mapper, err := env.Editor.Mapper(oldFile)
+			mapper, err := getMapper(oldFile)
 			if err != nil {
-				return nil, err
+				return err
 			}
-			result[newFile] = mapper.Content
-
+			newFile := env.Sandbox.Workdir.URIToPath(change.RenameFile.NewURI)
+			fileChanges[newFile] = mapper.Content
 		} else {
 			// edit
 			filename := env.Sandbox.Workdir.URIToPath(change.TextDocumentEdit.TextDocument.URI)
-			mapper, err := env.Editor.Mapper(filename)
+			mapper, err := getMapper(filename)
 			if err != nil {
-				return nil, err
+				return err
 			}
 			patched, _, err := source.ApplyProtocolEdits(mapper, change.TextDocumentEdit.Edits)
 			if err != nil {
-				return nil, err
+				return err
 			}
-			result[filename] = patched
+			fileChanges[filename] = patched
 		}
 	}
 
-	return result, nil
+	return nil
 }
 
 func codeActionMarker(mark marker, actionKind string, start, end protocol.Location, golden *Golden) {
@@ -1453,40 +1463,56 @@ func codeAction(env *Env, uri protocol.DocumentURI, rng protocol.Range, actionKi
 	}
 	action := candidates[0]
 
+	// Apply the codeAction.
+	//
+	// Spec:
+	//  "If a code action provides an edit and a command, first the edit is
+	//  executed and then the command."
+	fileChanges := make(map[string][]byte)
 	// An action may specify an edit and/or a command, to be
 	// applied in that order. But since applyDocumentChanges(env,
 	// action.Edit.DocumentChanges) doesn't compose, for now we
 	// assert that all commands used in the @suggestedfix tests
 	// return only a command.
-	if action.Edit != nil && action.Edit.DocumentChanges != nil {
-		env.T.Errorf("internal error: discarding unexpected CodeAction{Kind=%s, Title=%q}.Edit.DocumentChanges", action.Kind, action.Title)
-	}
-	if action.Command == nil {
-		return nil, fmt.Errorf("missing CodeAction{Kind=%s, Title=%q}.Command", action.Kind, action.Title)
+	if action.Edit != nil {
+		if action.Edit.Changes != nil {
+			env.T.Errorf("internal error: discarding unexpected CodeAction{Kind=%s, Title=%q}.Edit.Changes", action.Kind, action.Title)
+		}
+		if action.Edit.DocumentChanges != nil {
+			if err := applyDocumentChanges(env, action.Edit.DocumentChanges, fileChanges); err != nil {
+				return nil, fmt.Errorf("applying document changes: %v", err)
+			}
+		}
 	}
 
-	// This is a typical CodeAction command:
-	//
-	//   Title:     "Implement error"
-	//   Command:   gopls.apply_fix
-	//   Arguments: [{"Fix":"stub_methods","URI":".../a.go","Range":...}}]
-	//
-	// The client makes an ExecuteCommand RPC to the server,
-	// which dispatches it to the ApplyFix handler.
-	// ApplyFix dispatches to the "stub_methods" suggestedfix hook (the meat).
-	// The server then makes an ApplyEdit RPC to the client,
-	// whose Awaiter hook gathers the edits instead of applying them.
-
-	_ = env.Awaiter.takeDocumentChanges() // reset (assuming Env is confined to this thread)
+	if action.Command != nil {
+		// This is a typical CodeAction command:
+		//
+		//   Title:     "Implement error"
+		//   Command:   gopls.apply_fix
+		//   Arguments: [{"Fix":"stub_methods","URI":".../a.go","Range":...}}]
+		//
+		// The client makes an ExecuteCommand RPC to the server,
+		// which dispatches it to the ApplyFix handler.
+		// ApplyFix dispatches to the "stub_methods" suggestedfix hook (the meat).
+		// The server then makes an ApplyEdit RPC to the client,
+		// whose Awaiter hook gathers the edits instead of applying them.
+
+		_ = env.Awaiter.takeDocumentChanges() // reset (assuming Env is confined to this thread)
+
+		if _, err := env.Editor.Server.ExecuteCommand(env.Ctx, &protocol.ExecuteCommandParams{
+			Command:   action.Command.Command,
+			Arguments: action.Command.Arguments,
+		}); err != nil {
+			env.T.Fatalf("error converting command %q to edits: %v", action.Command.Command, err)
+		}
 
-	if _, err := env.Editor.Server.ExecuteCommand(env.Ctx, &protocol.ExecuteCommandParams{
-		Command:   action.Command.Command,
-		Arguments: action.Command.Arguments,
-	}); err != nil {
-		env.T.Fatalf("error converting command %q to edits: %v", action.Command.Command, err)
+		if err := applyDocumentChanges(env, env.Awaiter.takeDocumentChanges(), fileChanges); err != nil {
+			return nil, fmt.Errorf("applying document changes from command: %v", err)
+		}
 	}
 
-	return applyDocumentChanges(env, env.Awaiter.takeDocumentChanges())
+	return fileChanges, nil
 }
 
 // TODO(adonovan): suggestedfixerr

gopls/internal/lsp/source/api_json.go

@@ -1402,6 +1402,11 @@ func convenienceAnalyzers() map[string]*Analyzer {
 			Fix:        StubMethods,
 			Enabled:    true,
 		},
+		infertypeargs.Analyzer.Name: {
+			Analyzer:   infertypeargs.Analyzer,
+			Enabled:    true,
+			ActionKind: []protocol.CodeActionKind{protocol.RefactorRewrite},
+		},
 	}
 }
 
@@ -1445,7 +1450,6 @@ 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: false},
-		infertypeargs.Analyzer.Name:    {Analyzer: infertypeargs.Analyzer, Enabled: true},
 		embeddirective.Analyzer.Name:   {Analyzer: embeddirective.Analyzer, Enabled: true},
 		timeformat.Analyzer.Name:       {Analyzer: timeformat.Analyzer, Enabled: true},
 

gopls/internal/lsp/source/options.go

@@ -0,0 +1,38 @@
+This test verifies the infertypeargs refactoring.
+
+-- flags --
+-min_go=go1.18
+
+-- go.mod --
+module mod.test/infertypeargs
+
+go 1.18
+
+-- p.go --
+package infertypeargs
+
+func app[S interface{ ~[]E }, E interface{}](s S, e E) S {
+	return append(s, e)
+}
+
+func _() {
+	_ = app[[]int]
+	_ = app[[]int, int]
+	_ = app[[]int]([]int{}, 0) //@codeaction("refactor.rewrite", "app", ")", infer)
+	_ = app([]int{}, 0)
+}
+
+-- @infer/p.go --
+package infertypeargs
+
+func app[S interface{ ~[]E }, E interface{}](s S, e E) S {
+	return append(s, e)
+}
+
+func _() {
+	_ = app[[]int]
+	_ = app[[]int, int]
+	_ = app([]int{}, 0) //@codeaction("refactor.rewrite", "app", ")", infer)
+	_ = app([]int{}, 0)
+}
+

gopls/internal/regtest/marker/testdata/codeaction/infertypeargs.txt

@@ -39,8 +39,7 @@ func app[S interface{ ~[]E }, E interface{}](s S, e E) S {
 func _() {
 	_ = app[[]int]             //@hover("app", "app", appint)
 	_ = app[[]int, int]        //@hover("app", "app", appint)
-	// TODO(rfindley): eliminate this diagnostic.
-	_ = app[[]int]([]int{}, 0) //@hover("app", "app", appint),diag("[[]int]", re"unnecessary type arguments")
+	_ = app[[]int]([]int{}, 0) //@hover("app", "app", appint)
 	_ = app([]int{}, 0)        //@hover("app", "app", appint)
 }