x/tools/gopls: inline multiple deprecated function calls at once

stapelberg commented 8 months ago

gopls version

golang.org/x/tools/gopls v0.15.2

go env

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/stapelberg/.cache/go-build'
GOENV='/home/stapelberg/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/stapelberg/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/stapelberg/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/lib/go-1.21'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/lib/go-1.21/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.6'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build1079342930=/tmp/go-build -gno-record-gcc-switches'

What did you do?

The situation was that I had a function that made many calls to a deprecated function — let’s say it was ioutil.ReadFile:

package main

import "io/ioutil"

func main() {
    _, _ = ioutil.ReadFile("/tmp/foo")
    _, _ = ioutil.ReadFile("/tmp/bar")
}

What did you see happen?

When I place my Emacs point on ioutil.ReadFile and run M-x eglot-code-actions, I do see the expected code action as the only completion:

But when I select multiple ioutil.ReadFile calls and run M-x eglot-code-actions, I only get “Extract function” as the sole completion option:

What did you expect to see?

Would you happen to know if inlining multiple functions at once just isn’t possible in LSP? Or is it possible in LSP, but not possible in Emacs’s eglot?

In my case, I resorted to string replacement, because it was quicker. Ideally, the inliner would be even quicker to reach for :)

Editor and settings

Emacs 29 with builtin eglot

Logs

No response

adonovan commented 8 months ago

Hi Michael, that's a good question.

LSP doesn't really have an opinion here: its "code actions" RPC requests the set of available commands for the selected region. If the client invokes one of these commands, it has the side effect of making the server send an "apply edits" downcall to the client. So there's nothing stopping the server from offering one "inline" code action per function call in the selected region, but it would be confusing to the user if the menu of code actions had potentially hundreds of alternative "inline" operations. That's why we only offer an inline code action if the selection is within a function call.

But I think you're asking for a way to "apply all" inline commands for all calls to a particular function within a region, which seems like a reasonable operation. It's just not obvious to me what the best UX would be for exposing it. We could detect that there are multiple ReadFile calls within the same source file and, if so, offer both "inline" and "inline all calls to ReadFile". But why stop at the file? What about the package, or module?

Or workspace: we have an open feature request for an "inline away" operation that would inline every call to ReadFile that can be found by a 'references' query. I wonder whether that is the real feature you are looking for.

There's a confounding factor, which is that complex inlinings don't always compose, but I suspect in practice simple cases like ReadFile, the edits are all non-conflicting.

stapelberg commented 8 months ago

Ah! Yes, indeed, inline all references would have worked just fine in my case. There’s still some value in inline-all-in-region (as it allows migrating piece by piece), but probably inline-all-references is the more important feature to focus on :)

Thank you

golang / go