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

Jump to bottom

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

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

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

rndz opened this issue Nov 23, 2017 · 4 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done. Refactoring Issues related to refactoring tools Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@rndz
Copy link

rndz 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
Copy link
Member

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

@meirf
Copy link
Contributor

meirf commented Jul 17, 2018 •
edited

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 ysmolski 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
@ysmolski ysmolski added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 21, 2018
@ysmolski
Copy link
Member

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

@ysmolski ysmolski 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
Copy link
Contributor

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.

@ysmolski ysmolski 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 This label describes issues relating to any tools in the x/tools repository. label Sep 12, 2019
@adonovan adonovan added the Refactoring Issues related to refactoring tools label Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsFix The path to resolution is known, but the work has not been done. Refactoring Issues related to refactoring tools Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
7 participants
@ysmolski @tklauser @meirf @rndz @adonovan @alandonovan @gopherbot