cmd/gofmt: panic from simple rewrite rule

[skeeto]

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]

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

[skeeto]

Yeah, this is just stripped down example that reproduces the panic. I don't remember the original context (months ago), but it was a more complex rewrite rule on the elements of a composite literal. My rewrite rule was probably wrong there, too, but I never figured out why due to the panic and corresponding lack of feedback. I only saved off this example for future investigation/reporting.

[griesemer]

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.