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 <noreply+kokoro@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Alan Donovan <adonovan@google.com> Run-TryBot: Robert Findley <rfindley@google.com>
diff --git a/gopls/doc/analyzers.md b/gopls/doc/analyzers.md index 2717108..3e7301b 100644 --- a/gopls/doc/analyzers.md +++ b/gopls/doc/analyzers.md
@@ -217,22 +217,6 @@ **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** check references to loop variables from within nested functions @@ -755,6 +739,22 @@ **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** stub methods analyzer
diff --git a/gopls/internal/lsp/regtest/marker.go b/gopls/internal/lsp/regtest/marker.go index 059f7a6..b33d90a 100644 --- a/gopls/internal/lsp/regtest/marker.go +++ b/gopls/internal/lsp/regtest/marker.go
@@ -1335,41 +1335,51 @@ 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 @@ } 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. + 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) + _ = 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
diff --git a/gopls/internal/lsp/source/api_json.go b/gopls/internal/lsp/source/api_json.go index 88ff209..b564be1 100644 --- a/gopls/internal/lsp/source/api_json.go +++ b/gopls/internal/lsp/source/api_json.go
@@ -282,11 +282,6 @@ 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\n\tfunc f[T any](T) {}\n\t\n\tfunc _() {\n\t\tf[string](\"foo\") // string could be inferred\n\t}\n", - Default: "true", - }, - { Name: "\"loopclosure\"", Doc: "check references to loop variables from within nested functions\n\nThis analyzer reports places where a function literal references the\niteration variable of an enclosing loop, and the loop calls the function\nin such a way (e.g. with go or defer) that it may outlive the loop\niteration and possibly observe the wrong value of the variable.\n\nIn this example, all the deferred functions run after the loop has\ncompleted, so all observe the final value of v.\n\n\tfor _, v := range list {\n\t defer func() {\n\t use(v) // incorrect\n\t }()\n\t}\n\nOne fix is to create a new variable for each iteration of the loop:\n\n\tfor _, v := range list {\n\t v := v // new var per iteration\n\t defer func() {\n\t use(v) // ok\n\t }()\n\t}\n\nThe next example uses a go statement and has a similar problem.\nIn addition, it has a data race because the loop updates v\nconcurrent with the goroutines accessing it.\n\n\tfor _, v := range elem {\n\t go func() {\n\t use(v) // incorrect, and a data race\n\t }()\n\t}\n\nA fix is the same as before. The checker also reports problems\nin goroutines started by golang.org/x/sync/errgroup.Group.\nA hard-to-spot variant of this form is common in parallel tests:\n\n\tfunc Test(t *testing.T) {\n\t for _, test := range tests {\n\t t.Run(test.name, func(t *testing.T) {\n\t t.Parallel()\n\t use(test) // incorrect, and a data race\n\t })\n\t }\n\t}\n\nThe t.Parallel() call causes the rest of the function to execute\nconcurrent with the loop.\n\nThe analyzer reports references only in the last statement,\nas it is not deep enough to understand the effects of subsequent\nstatements that might render the reference benign.\n(\"Last statement\" is defined recursively in compound\nstatements such as if, switch, and select.)\n\nSee: https://golang.org/doc/go_faq.html#closures_and_goroutines", Default: "true", @@ -437,6 +432,11 @@ 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\n\tfunc f[T any](T) {}\n\t\n\tfunc _() {\n\t\tf[string](\"foo\") // string could be inferred\n\t}\n", + Default: "true", + }, + { Name: "\"stubmethods\"", Doc: "stub methods analyzer\n\nThis analyzer generates method stubs for concrete types\nin order to implement a target interface", Default: "true", @@ -952,11 +952,6 @@ 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\n\tfunc f[T any](T) {}\n\t\n\tfunc _() {\n\t\tf[string](\"foo\") // string could be inferred\n\t}\n", - Default: true, - }, - { Name: "loopclosure", Doc: "check references to loop variables from within nested functions\n\nThis analyzer reports places where a function literal references the\niteration variable of an enclosing loop, and the loop calls the function\nin such a way (e.g. with go or defer) that it may outlive the loop\niteration and possibly observe the wrong value of the variable.\n\nIn this example, all the deferred functions run after the loop has\ncompleted, so all observe the final value of v.\n\n\tfor _, v := range list {\n\t defer func() {\n\t use(v) // incorrect\n\t }()\n\t}\n\nOne fix is to create a new variable for each iteration of the loop:\n\n\tfor _, v := range list {\n\t v := v // new var per iteration\n\t defer func() {\n\t use(v) // ok\n\t }()\n\t}\n\nThe next example uses a go statement and has a similar problem.\nIn addition, it has a data race because the loop updates v\nconcurrent with the goroutines accessing it.\n\n\tfor _, v := range elem {\n\t go func() {\n\t use(v) // incorrect, and a data race\n\t }()\n\t}\n\nA fix is the same as before. The checker also reports problems\nin goroutines started by golang.org/x/sync/errgroup.Group.\nA hard-to-spot variant of this form is common in parallel tests:\n\n\tfunc Test(t *testing.T) {\n\t for _, test := range tests {\n\t t.Run(test.name, func(t *testing.T) {\n\t t.Parallel()\n\t use(test) // incorrect, and a data race\n\t })\n\t }\n\t}\n\nThe t.Parallel() call causes the rest of the function to execute\nconcurrent with the loop.\n\nThe analyzer reports references only in the last statement,\nas it is not deep enough to understand the effects of subsequent\nstatements that might render the reference benign.\n(\"Last statement\" is defined recursively in compound\nstatements such as if, switch, and select.)\n\nSee: https://golang.org/doc/go_faq.html#closures_and_goroutines", URL: "https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/loopclosure", @@ -1120,6 +1115,11 @@ 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\n\tfunc f[T any](T) {}\n\t\n\tfunc _() {\n\t\tf[string](\"foo\") // string could be inferred\n\t}\n", + Default: true, + }, + { Name: "stubmethods", Doc: "stub methods analyzer\n\nThis analyzer generates method stubs for concrete types\nin order to implement a target interface", Default: true,
diff --git a/gopls/internal/lsp/source/options.go b/gopls/internal/lsp/source/options.go index 3357495..20a2903 100644 --- a/gopls/internal/lsp/source/options.go +++ b/gopls/internal/lsp/source/options.go
@@ -1402,6 +1402,11 @@ Fix: StubMethods, Enabled: true, }, + infertypeargs.Analyzer.Name: { + Analyzer: infertypeargs.Analyzer, + Enabled: true, + ActionKind: []protocol.CodeActionKind{protocol.RefactorRewrite}, + }, } } @@ -1445,7 +1450,6 @@ 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},
diff --git a/gopls/internal/regtest/marker/testdata/codeaction/infertypeargs.txt b/gopls/internal/regtest/marker/testdata/codeaction/infertypeargs.txt new file mode 100644 index 0000000..8ee1b67 --- /dev/null +++ b/gopls/internal/regtest/marker/testdata/codeaction/infertypeargs.txt
@@ -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) +} +
diff --git a/gopls/internal/regtest/marker/testdata/hover/generics.txt b/gopls/internal/regtest/marker/testdata/hover/generics.txt index 673e860..d512f7f 100644 --- a/gopls/internal/regtest/marker/testdata/hover/generics.txt +++ b/gopls/internal/regtest/marker/testdata/hover/generics.txt
@@ -39,8 +39,7 @@ 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) }