x/tools/cmd/gorename: allow renaming to builtins

rndz commented 6 years ago

What did you do?

While trying to rename a variable (func, etc..) to real the gorename tool panics.

Suppose you have a main.go like this:

package main

import "fmt"

func main() {
    world := "Hello world!"
    fmt.Println(world)
}

And you run gorename -from 'main.go::world' -to real

What did you expect to see?

Expected output: Renamed 2 occurrences in 1 file in 1 package.

and the main.go file containing:

package main

import "fmt"

func main() {
    real := "Hello world!"
    fmt.Println(real)
}

What did you see instead?

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x5eef75]

goroutine 1 [running]:
go/types.(*Package).Scope(...)
        /usr/local/go/src/go/types/package.go:41
golang.org/x/tools/refactor/rename.lexicalLookup(0xc4222d93b0, 0x7ffe2fd24c80, 0x4, 0x6a, 0xc42000f6d0, 0x79aea0, 0xc4222d92c0)
        /home/rndz/golang/src/golang.org/x/tools/refactor/rename/check.go:291 +0x95
golang.org/x/tools/refactor/rename.(*renamer).checkInLexicalScope.func2(0xc42000a840, 0xc4222d93b0, 0x6)
        /home/rndz/golang/src/golang.org/x/tools/refactor/rename/check.go:250 +0xb7
golang.org/x/tools/refactor/rename.forEachLexicalRef.func1(0x7977e0, 0xc42000a840, 0x639a00)
        /home/rndz/golang/src/golang.org/x/tools/refactor/rename/check.go:332 +0x56c
go/ast.inspector.Visit(0xc42247a000, 0x7977e0, 0xc42000a840, 0x0, 0x0)
        /usr/local/go/src/go/ast/walk.go:373 +0x3a
go/ast.Walk(0x7969e0, 0xc42247a000, 0x7977e0, 0xc42000a840)
        /usr/local/go/src/go/ast/walk.go:52 +0x66
go/ast.Walk(0x7969e0, 0xc42247a000, 0x7972a0, 0xc42004e700)
        /usr/local/go/src/go/ast/walk.go:136 +0x1041
go/ast.Walk(0x7969e0, 0xc42247a000, 0x797560, 0xc420044720)
        /usr/local/go/src/go/ast/walk.go:196 +0x1b1c
go/ast.walkStmtList(0x7969e0, 0xc42247a000, 0xc420044730, 0x1, 0x1)
        /usr/local/go/src/go/ast/walk.go:32 +0x81
go/ast.Walk(0x7969e0, 0xc42247a000, 0x797220, 0xc42005f350)
        /usr/local/go/src/go/ast/walk.go:224 +0x1b71
go/ast.Walk(0x7969e0, 0xc42247a000, 0x7976a0, 0xc42005f380)
        /usr/local/go/src/go/ast/walk.go:344 +0xd83
go/ast.walkDeclList(0x7969e0, 0xc42247a000, 0xc42004e740, 0x3, 0x4)
        /usr/local/go/src/go/ast/walk.go:38 +0x81
go/ast.Walk(0x7969e0, 0xc42247a000, 0x797620, 0xc42007c200)
        /usr/local/go/src/go/ast/walk.go:353 +0x266f
go/ast.Inspect(0x797620, 0xc42007c200, 0xc42247a000)
        /usr/local/go/src/go/ast/walk.go:385 +0x4b
golang.org/x/tools/refactor/rename.forEachLexicalRef(0xc4200b00b0, 0x79aea0, 0xc4222d92c0, 0xc4222e9fa0, 0xc42000e301)
        /home/rndz/golang/src/golang.org/x/tools/refactor/rename/check.go:365 +0x18f
golang.org/x/tools/refactor/rename.(*renamer).checkInLexicalScope(0xc42017ea10, 0x79aea0, 0xc4222d92c0, 0xc4200b00b0)
        /home/rndz/golang/src/golang.org/x/tools/refactor/rename/check.go:244 +0xd3
golang.org/x/tools/refactor/rename.(*renamer).checkInPackageBlock(0xc42017ea10, 0x79aea0, 0xc4222d92c0)
        /home/rndz/golang/src/golang.org/x/tools/refactor/rename/check.go:143 +0x5b3
golang.org/x/tools/refactor/rename.(*renamer).check(0xc42017ea10, 0x79aea0, 0xc4222d92c0)
        /home/rndz/golang/src/golang.org/x/tools/refactor/rename/check.go:38 +0x3c3
golang.org/x/tools/refactor/rename.Main(0x7b1b20, 0x0, 0x0, 0x7ffe2fd24c6e, 0xd, 0x7ffe2fd24c80, 0x4, 0x0, 0x0)
        /home/rndz/golang/src/golang.org/x/tools/refactor/rename/rename.go:347 +0x883
main.main()
        /home/rndz/golang/src/golang.org/x/tools/cmd/gorename/main.go:49 +0x157

System details

go version go1.9.2 linux/amd64
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/rndz/golang"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build855039640=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOROOT/bin/go version: go version go1.9.2 linux/amd64
GOROOT/bin/go tool compile -V: compile version go1.9.2
uname -sr: Linux 3.16.0-4-amd64
Distributor ID: Debian
Description:    Debian GNU/Linux 8.9 (jessie)
Release:    8.9
Codename:   jessie
/lib/x86_64-linux-gnu/libc.so.6: GNU C Library (Debian GLIBC 2.19-18+deb8u10) stable release version 2.19, by Roland McGrath et al.

tklauser commented 6 years ago

The crash also seems to occur whenever the new name is a builtin name, e.g. gorename -from 'main.go::world' -to len.

meirf commented 6 years ago

The crash also seems to occur whenever the new name is a builtin name

Actually, real is a builtin.

In contrast to renaming to builtins which cause crashes, renaming to keywords is handled gracefully:

$ gorename -from 'main.go:: world' -to else
gorename: -to "else": not a valid identifier

See isValidIdentifier -> token.Lookup

/cc @alandonovan

ysmolski commented 5 years ago

We can improve gorename by handling rename to builtins the same way as keywords are handled. It should return this error: gorename: -to "len": not a valid identifier

alandonovan commented 5 years ago

Renames to built-in names should not be handled specially. There are plenty of occasions when you want to re-use a predeclared name, such as a function called print or real, or a method called int or string.

The tool should not crash, of course.

seankhliao commented 3 weeks ago

closing as obsoleted by #69360

adonovan commented 3 weeks ago

Thanks @seankhliao. Do beware that many of these gorename issues are in principle applicable to gopls' Rename LSP RPC, which runs a fork of the same algorithm. That said, I just verified that this particular issue is fixed in gopls, and that the other ones just closed don't really apply to gopls because it has a notion of a workspace.

golang / go