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

cmd/gofmt: panic from simple rewrite rule #52145

Open
skeeto opened this issue Apr 4, 2022 · 3 comments
Open

cmd/gofmt: panic from simple rewrite rule #52145

skeeto opened this issue Apr 4, 2022 · 3 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unplanned

Comments

@skeeto
Copy link

skeeto commented Apr 4, 2022

What version of Go are you using (go version)?

$ go version
go version go1.18 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="off"
GOENV=""
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH=""
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/bugreport/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/bugreport/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.18"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="0"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build859309737=/tmp/go-build -gno-record-gcc-switches"
GOROOT/bin/go version: go version go1.18 linux/amd64
GOROOT/bin/go tool compile -V: compile version go1.18
uname -sr: Linux 5.10.0-7-amd64
Distributor ID:	Debian
Description:	Debian GNU/Linux 11 (bullseye)
Release:	11
Codename:	bullseye
/lib/x86_64-linux-gnu/libc.so.6: GNU C Library (Debian GLIBC 2.31-13+deb11u3) stable release version 2.31.
lldb --version: lldb version 11.0.1
gdb --version: GNU gdb (Debian 10.1-1.7) 10.1.90.20210103-git

What did you do?

package main

var _ int
gofmt -r 'a -> p.a' <example.go

What did you expect to see?

No panic.

What did you see instead?

panic: reflect.Set: value of type ast.Expr is not assignable to type *ast.Ident

goroutine 6 [running]:
reflect.Value.assignTo({0x5259e0?, 0xc0000125c0?, 0x61e2c8?}, {0x53fa76, 0xb}, 0x52af60, 0x0)
        /bugreport/go/src/reflect/value.go:3062 +0x2ac
reflect.Value.Set({0x52af60?, 0xc00000c160?, 0xc00000c088?}, {0x5259e0?, 0xc0000125c0?, 0x61e068?})
        /bugreport/go/src/reflect/value.go:2088 +0xeb
main.subst(0xc000049bf8, {0x528940?, 0xc00000c078?, 0xc00000e0e8?}, {0x51eee0?, 0x61e068?, 0x549048?})
        /bugreport/go/src/cmd/gofmt/rewrite.go:291 +0x633
main.subst(0xc000049bf8, {0x5246e0?, 0xc00000c078?, 0x0?}, {0x51eee0?, 0x61e068?, 0x519840?})
        /bugreport/go/src/cmd/gofmt/rewrite.go:298 +0x3c9
main.rewriteFile.func1({0x5259e0?, 0xc000064110?, 0xc000049370?})
        /bugreport/go/src/cmd/gofmt/rewrite.go:76 +0x1e7
main.apply(0xc000049c28, {0x524be0?, 0xc0000640f0?, 0xc00000e0a0?})
        /bugreport/go/src/cmd/gofmt/rewrite.go:145 +0x22a
main.rewriteFile.func1({0x524be0?, 0xc0000640f0?, 0x409df5?})
        /bugreport/go/src/cmd/gofmt/rewrite.go:71 +0x9c
main.apply(0xc000049c28, {0x525ae0?, 0xc000012490?, 0x5259e0?})
        /bugreport/go/src/cmd/gofmt/rewrite.go:149 +0x156
main.rewriteFile.func1({0x525ae0?, 0xc000012490?, 0x182?})
        /bugreport/go/src/cmd/gofmt/rewrite.go:71 +0x9c
main.apply(0xc000049c28, {0x51a080?, 0xc000016320?, 0x0?})
        /bugreport/go/src/cmd/gofmt/rewrite.go:140 +0x2b5
main.rewriteFile.func1({0x51a080?, 0xc000016320?, 0x0?})
        /bugreport/go/src/cmd/gofmt/rewrite.go:71 +0x9c
main.apply(0xc000049c28, {0x523ee0?, 0xc000016300?, 0x1000000000000?})
        /bugreport/go/src/cmd/gofmt/rewrite.go:145 +0x22a
main.rewriteFile.func1({0x523ee0?, 0xc000016300?, 0xc0000498a0?})
        /bugreport/go/src/cmd/gofmt/rewrite.go:71 +0x9c
main.apply(0xc000049c28, {0x5258e0?, 0xc0000124a0?, 0xc000064190?})
        /bugreport/go/src/cmd/gofmt/rewrite.go:149 +0x156
main.rewriteFile.func1({0x5258e0?, 0xc0000124a0?, 0x196?})
        /bugreport/go/src/cmd/gofmt/rewrite.go:71 +0x9c
main.apply(0xc000049c28, {0x519fc0?, 0xc000114018?, 0x0?})
        /bugreport/go/src/cmd/gofmt/rewrite.go:140 +0x2b5
main.rewriteFile.func1({0x519fc0?, 0xc000114018?, 0x522ce0?})
        /bugreport/go/src/cmd/gofmt/rewrite.go:71 +0x9c
main.apply(0xc000049c28, {0x521300?, 0xc000114000?, 0x0?})
        /bugreport/go/src/cmd/gofmt/rewrite.go:145 +0x22a
main.rewriteFile(0x0?, {0x56e7b0?, 0xc000058040}, {0x56ea50?, 0xc00000c078}, 0xc000114000)
        /bugreport/go/src/cmd/gofmt/rewrite.go:81 +0x385
main.initRewrite.func1(0x540828?, 0x540828?)
        /bugreport/go/src/cmd/gofmt/rewrite.go:32 +0x2f
main.processFile({0x540828, 0x10}, {0x0, 0x0}, {0x56d618?, 0xc00000e010?}, 0xc000012450)
        /bugreport/go/src/cmd/gofmt/gofmt.go:245 +0x1ad
main.gofmtMain.func2(0x0?)
        /bugreport/go/src/cmd/gofmt/gofmt.go:410 +0x3a
main.(*sequencer).Add.func2()
        /bugreport/go/src/cmd/gofmt/gofmt.go:146 +0x45
created by main.(*sequencer).Add
        /bugreport/go/src/cmd/gofmt/gofmt.go:145 +0x1b3
@mvdan
Copy link
Member

mvdan commented Apr 5, 2022

What kind of rewrite were you actually trying to make here? Encountering a panic is a bug for sure, but as far as I can tell, this rewrite should generally result in an error anyway.

Naively, your code could become something like:

package p.main

var p._ p.int

That's not valid syntax, and it's what's causing the panic: names can't be arbitrary expressions like selectors. Perhaps we could teach gofmt's rewrite about that, so that it would error in those cases.

An alternative is to teach it to not apply the rewrite in nodes where it would not be possible, resulting in:

package main

var _ p.int

That wouldn't lead to invalid syntax, and could arguably be useful for some cases, but it could also make the rewrite feature less consistent. I'm not sure which of the two approaches gofmt -r is designed for. cc @griesemer

@mvdan mvdan changed the title gofmt panic from simple rewrite rule cmd/gofmt: panic from simple rewrite rule Apr 5, 2022
@skeeto
Copy link
Author

skeeto commented Apr 5, 2022 via email

@griesemer
Copy link
Contributor

The -r rewrite feature of gofmt is here for historic reasons; now we would probably rather have a stand-alone tool for rewriting. For the record, @rsc had the idea and added the code of the rewriter to gofmt.

In this case I'd argue that if the rewrite is not possible because an *ast.Ident cannot be replaced with an ast.Expr, it probably should not crash or error, but just ignore those cases. Clearly a crash is not desirable, and an error is not helpful because there wouldn't be a way to avoid it (at least in this case, though I have not looked into the code).

Doesn't seem urgent to have fixed.

@cherrymui cherrymui added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 5, 2022
@cherrymui cherrymui added this to the Unplanned milestone Apr 5, 2022
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.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
4 participants
@skeeto @mvdan @griesemer @cherrymui