Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
rndz opened this issue Nov 23, 2017 · 4 comments

Comments

@rndz
Copy link

commented Nov 23, 2017

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.

@gopherbot gopherbot added this to the Unreleased milestone Nov 23, 2017

@tklauser

This comment has been minimized.

Copy link
Member

commented Nov 23, 2017

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

@meirf

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2018

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

@ysmolsky ysmolsky changed the title x/tools/cmd/gorename: panic while trying to rename to `real` x/tools/cmd/gorename: renaming to keywords should result in "not a valid identifier" error Nov 21, 2018

@ysmolsky ysmolsky added the NeedsFix label Nov 21, 2018

@ysmolsky

This comment has been minimized.

Copy link
Member

commented Nov 21, 2018

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

@ysmolsky ysmolsky changed the title x/tools/cmd/gorename: renaming to keywords should result in "not a valid identifier" error x/tools/cmd/gorename: renaming to builtins should result in "not a valid identifier" error Nov 21, 2018

@alandonovan

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2018

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.

@ysmolsky ysmolsky changed the title x/tools/cmd/gorename: renaming to builtins should result in "not a valid identifier" error x/tools/cmd/gorename: allow renaming to builtins Nov 21, 2018

@gopherbot gopherbot added the Tools label Sep 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.