Compiler panic at tip Go due to assumption about nil basic types

[findleyr]

Gopherjs breaks at CL 284052 CL 289715 in the Go repository, due to an assumption about nil basic types here:

gopherjs/compiler/expressions.go

Line 738 in bed99a8

if t.Kind() != types.UnsafePointer {

(t.Kind() is now types.UntypedNil)

This is reproducible as follows:

> GO111MODULE=off go test -v -run=TestGoRepositoryCompilerTests ./tests                                                                                         
=== RUN   TestGoRepositoryCompilerTests                                                                             
goos: "linux", goarch: "js"                                                                                         
ok      fixedbugs/bug000.go     0.254s                                                                              
ok      fixedbugs/bug002.go     0.190s                                                                                                                                                                                                  
ok      fixedbugs/bug003.go     0.186s                                                                              
ok      fixedbugs/bug004.go     0.176s                                                                                                                                                                                                  
ok      fixedbugs/bug005.go     0.178s                                                                              
# go run run.go -- fixedbugs/bug006.go                                                                                                                                                                                                  
exit status 2                                                                                                       
panic: unexpected basic type [recovered]                                                                            
        panic: unexpected basic type                                                                                
                                                                                                                    
goroutine 1 [running]:                                                                                              
go/types.(*Checker).handleBailout(0xc00024a000, 0xc0000eed00)                                                       
        /usr/local/google/home/rfindley/src/go2/src/go/types/check.go:245 +0x9f                                   
panic(0x918020, 0xa87510)                                                                                       
        /usr/local/google/home/rfindley/src/go2/src/runtime/panic.go:965 +0x1c7
github.com/gopherjs/gopherjs/compiler.(*funcContext).translateExpr(0xc00052c2c0, 0xa9a310, 0xc00071c1c0, 0xa9a310)
        /usr/local/google/home/rfindley/dev/go/src/github.com/gopherjs/gopherjs/compiler/expressions.go:739 +0xf0d3

This appears to be a case of Hyrum's law -- I'm not aware of documentation stating that a basic nil must be types.UnsafePointer, though please let me know if I've missed something.

Unless there is evidence that this implicit assumption about nil basic types is widespread, we'd like to land this change in go/types for go1.17. It seems like a trivial fix, but I'm not familiar with the gopherjs compiler. Could someone take a look? Thanks.

CC @dmitshur

[nevkontakte]

Admittedly I don't know why GopherJS makes this assumption, but I'll go ahead and guess maybe at the time this code was written UnsafePointer was the only BasicKind that could plausibly be nil? Although it seems like UntypedNil existed even back in Go 1.5.1: https://pkg.go.dev/go/types@go1.5.1#BasicKind ¯\_(ツ)_/¯ CC @neelance who authored 0f211c5.

I'd like to better understand what changes with https://golang.org/cl/284052. Is the below correct?

Before:
var _ unsafe.Pointer = nil
    ^- unsafe.Pointer   ^-unsafe.Pointer

After:
var _ unsafe.Pointer = nil
    ^- unsafe.Pointer   ^-unsafe.UntypedNil

I have a suspicion that to support this change we'll have to do more than just remove the panic, We will probably have new cases of implicit conversions to handle, unless we already do. It's also a bit puzzling to me that fixedbugs/bug006.go of all things blew up, it doesn't seem to involve nil at all.

[findleyr]

Sorry, I linked the wrong CL above (will amend): https://golang.org/cl/284052 is the compiler change, https://golang.org/cl/289715 is the go/types port.

Yes, the example in #1011 (comment) is one example of what is changing. Note that 'nil' in the following was already reported as untyped nil:

var _ *int = nil

See more at https://golang.org/issue/13061. Here are playground examples illustrating the change (I'm using go2goplay only because it has the new logic; none of this pertains directly to generics):
https://play.golang.org/p/LroExeoXjg-
https://go2goplay.golang.org/p/LroExeoXjg-

The actual source that causes the panic is here:

https://github.com/golang/go/blob/46cb016190389b7e37b21f04e5343a628ca1f662/src/io/multi.go#L72

Here's an error message with more information:

# go run run.go -- fixedbugs/bug006.go                                                                              
exit status 2                                                                                                       
panic: unexpected basic type 25@/usr/local/google/home/rfindley/sdk/go1.12.17/src/io/multi.go:72:37 [recovered]

You're right, it's not completely trivial. I assumed it was straightforward because there must have been pre-existing handling for assignments, for example:

gopherjs/compiler/expressions.go

Line 1120 in bed99a8

if isBasicExpr && basicExprType.Kind() == types.UntypedNil {

Here is more of the panicking stack:

# go run run.go -- fixedbugs/bug006.go                                                                              
exit status 2                                                                                                       
panic: unexpected basic type 25@/usr/local/google/home/rfindley/sdk/go1.12.17/src/io/multi.go:72:37 [recovered]
        panic: unexpected basic type 25@/usr/local/google/home/rfindley/sdk/go1.12.17/src/io/multi.go:72:37         
                                                                                                                    
goroutine 1 [running]:                                                                                              
go/types.(*Checker).handleBailout(0xc0002ca000, 0xc0008e4d00)                      
        /usr/local/google/home/rfindley/src/go2/src/go/types/check.go:245 +0x9f                                     
panic(0x918020, 0xc000487950)                                                                                       
        /usr/local/google/home/rfindley/src/go2/src/runtime/panic.go:965 +0x1c7                                   
github.com/gopherjs/gopherjs/compiler.(*funcContext).translateExpr(0xc0004322c0, 0xa9a4f0, 0xc000280700, 0xa9a4f0) 
        /usr/local/google/home/rfindley/dev/go/src/github.com/gopherjs/gopherjs/compiler/expressions.go:739 +0xf289 
github.com/gopherjs/gopherjs/compiler.(*funcContext).formatExprInternal.func3.1(0x0, 0x0)                         
        /usr/local/google/home/rfindley/dev/go/src/github.com/gopherjs/gopherjs/compiler/expressions.go:1293 +0xce  
github.com/gopherjs/gopherjs/compiler.(*funcContext).formatExprInternal.func3(0x6500, 0x0)                         
        /usr/local/google/home/rfindley/dev/go/src/github.com/gopherjs/gopherjs/compiler/expressions.go:1314 +0x28c
github.com/gopherjs/gopherjs/compiler.(*funcContext).formatExprInternal.func1(0xc0008e00c0)                       
        /usr/local/google/home/rfindley/dev/go/src/github.com/gopherjs/gopherjs/compiler/expressions.go:1248 +0xc2  
github.com/gopherjs/gopherjs/compiler.(*funcContext).formatExprInternal(0xc0004322c0, 0x9d2c59, 0x22, 0xc0008e0358, 0x2, 0x2, 0x0, 0x438800)
        /usr/local/google/home/rfindley/dev/go/src/github.com/gopherjs/gopherjs/compiler/expressions.go:1287 +0x2ad
github.com/gopherjs/gopherjs/compiler.(*funcContext).formatExpr(...)                                        
        /usr/local/google/home/rfindley/dev/go/src/github.com/gopherjs/gopherjs/compiler/expressions.go:1228        
github.com/gopherjs/gopherjs/compiler.(*funcContext).translateConversion(0xc0004322c0, 0xa9a4f0, 0xc000280700, 0xa95cf0, 0xc0003761c0, 0x0)
        /usr/local/google/home/rfindley/dev/go/src/github.com/gopherjs/gopherjs/compiler/expressions.go:1075 +0x13b4
github.com/gopherjs/gopherjs/compiler.(*funcContext).translateExpr(0xc0004322c0, 0xa9a1f0, 0xc000332440, 0xc0000c2910)
        /usr/local/google/home/rfindley/dev/go/src/github.com/gopherjs/gopherjs/compiler/expressions.go:542 +0x32b
github.com/gopherjs/gopherjs/compiler.(*funcContext).translateStmt(0xc0004322c0, 0xa9a070, 0xc000352d00, 0x0)    

[nevkontakte]

Thanks, this is very helpful. I'll try to give this a close look as soon as I can, but unfortunately can't promise this before the weekend.

Addendum: my concern over how non-trivial a change this would be was mostly expression of caution. I only recently started working on GopherJS, so its certain corners are still unfamiliar to me.

[findleyr]

Thanks! Really appreciate your help looking into this.

CC @griesemer

[nevkontakte]

Sigh, I feel like supporting Go 1.17 and generics is going to be an adventure…

When I test against https://github.com/nevkontakte/gopherjs/tree/go1.16-stdlib-wip (which is slightly ahead of https://github.com/gopherjs/gopherjs/tree/go1.16-stdlib, which is the about-to-land Go 1.16 support) and https://github.com/golang/go master I get a different panic:

$ cd ~/git
$ git clone https://go.googlesource.com/go go-tip
$ cd go-tip/src
$ ./make.bash
$ alias go-tip=~/git/go-tip/bin/go
$ export GOROOT="/home/aleks/git/go-tip"
$ cd ~/go/src/github.com/gopherjs/gopherjs
$ go-tip install -tags=gopherjsdev . && GOPHERJS_NO_VERSION_CHECK=true go-tip run tests/run.go -v -- fixedbugs/bug006.go                                                                          [44/1953]goos: "linux", goarch: "js"                                                                                                                                                                                      # go run run.go -- fixedbugs/bug006.go
exit status 2                                                                                                                                                                                                    panic: interface conversion: types.Type is *types.Basic, not *types.Pointer [recovered]
        panic: interface conversion: types.Type is *types.Basic, not *types.Pointer

goroutine 1 [running]:                                                                                                                                                                                           go/types.(*Checker).handleBailout(0xc000230000, 0xc00108ed00)
        /home/aleks/git/go-tip/src/go/types/check.go:249 +0xa5
panic(0x932960, 0xc00105f230)
        /home/aleks/git/go-tip/src/runtime/panic.go:1031 +0x25b
github.com/gopherjs/gopherjs/compiler.(*funcContext).translateConversion(0xc000eb6c60, 0xa861f0, 0xc000f7f0a0, 0xa818a8, 0xc000f079c0, 0xc000edcd68)
        /home/aleks/go/src/github.com/gopherjs/gopherjs/compiler/expressions.go:1085 +0x2cfc
github.com/gopherjs/gopherjs/compiler.(*funcContext).translateExpr(0xc000eb6c60, 0xa85ef0, 0xc001064400, 0xa81830)
        /home/aleks/go/src/github.com/gopherjs/gopherjs/compiler/expressions.go:542 +0xc50d
github.com/gopherjs/gopherjs/compiler.(*funcContext).translateImplicitConversion(0xc000eb6c60, 0xa85ef0, 0xc001064400, 0xa81830, 0xc0002d6e80, 0x0)
        /home/aleks/go/src/github.com/gopherjs/gopherjs/compiler/expressions.go:1147 +0x6d7
github.com/gopherjs/gopherjs/compiler.(*funcContext).translateImplicitConversionWithCloning(0xc000eb6c60, 0xa85ef0, 0xc001064400, 0xa81830, 0xc0002d6e80, 0x1)
        /home/aleks/go/src/github.com/gopherjs/gopherjs/compiler/expressions.go:1112 +0x1ed
github.com/gopherjs/gopherjs/compiler.(*funcContext).translateArgs(0xc000eb6c60, 0xc000364600, 0xc00106e040, 0x1, 0x1, 0xc001088900, 0x411591, 0xc0010889d8, 0xc0010680d8)
        /home/aleks/go/src/github.com/gopherjs/gopherjs/compiler/utils.go:122 +0x553                                                                                                                             github.com/gopherjs/gopherjs/compiler.(*funcContext).translateCall(0xc000eb6c60, 0xc001064440, 0xc000364600, 0xc000f7b728, 0xc000364600)
        /home/aleks/go/src/github.com/gopherjs/gopherjs/compiler/expressions.go:769 +0x7d                                                                                                                        github.com/gopherjs/gopherjs/compiler.(*funcContext).translateExpr(0xc000eb6c60, 0xa85ef0, 0xc001064440, 0xa81880)
        /home/aleks/go/src/github.com/gopherjs/gopherjs/compiler/expressions.go:571 +0xf225
github.com/gopherjs/gopherjs/compiler.(*funcContext).translateImplicitConversion(0xc000eb6c60, 0xa85ef0, 0xc001064440, 0xa81880, 0xc0002d6480, 0x4)
        /home/aleks/go/src/github.com/gopherjs/gopherjs/compiler/expressions.go:1122 +0x71f                                                                                                                      github.com/gopherjs/gopherjs/compiler.(*funcContext).translateImplicitConversionWithCloning(0xc000eb6c60, 0xa85ef0, 0xc001064440, 0xa81880, 0xc0002d6480, 0x0)
        /home/aleks/go/src/github.com/gopherjs/gopherjs/compiler/expressions.go:1112 +0x1ed
github.com/gopherjs/gopherjs/compiler.(*funcContext).makeReceiver(0xc000eb6c60, 0xc000f7b608, 0xc000f7b608)
        /home/aleks/go/src/github.com/gopherjs/gopherjs/compiler/expressions.go:824 +0x6bb
github.com/gopherjs/gopherjs/compiler.(*funcContext).translateExpr(0xc000eb6c60, 0xa85ef0, 0xc001064480, 0xa81880)
...

@findleyr can you please confirm which branches of GopherJS and Go you've used it your tests? Also, am I even setting it up correctly?

[nevkontakte]

The panic happens at this line:

https://github.com/golang/go/blob/9e3328e7407ab7cd24f48d27ba69d265c57d5805/src/errors/wrap.go#L103

So I assume it is caused by the same change, just a different instance of it.

[findleyr]

It looks like that panic would have been caused by the same underlying assumption which was invalidated by CL 289715: the type of nil in that expression is types.Typ[UntypedNil].

I suspect that the different panic is an artifact of the changes in go1.16-stdlib-wip, but FTR I tested with Go at exactly CL 289715 (618e3c15bdb5c031ac037e7ad5c1b3791a913226).

[nevkontakte]

Could you also share which GopherJS commit you were using?

[findleyr]

Oh, sorry, I was at bed99a8 (1.12.3+go1.12)

[nevkontakte]

Having spent a few hours on this today, this is what I have so far:

1. I think I understand the meaning of the changes in https://golang.org/cl/284052 reasonably well.
2. A naive fix of just allowing types.UntypedNil wherever a panic happens doesn't work: the compiler ends up generating broken code and the program crashes at runtime.
3. Unfortunately the relevant part of the compiler code is complicated enough that I still don't know if this is going to be an easy fix.
4. As a next step, I need to create minimal reproduction examples and carefully stare at compiler behavior before and after https://golang.org/cl/284052 to understand what are the right code paths we should be triggering. Unfortunately, this will take some time…

[nevkontakte]

I believe the problem is fixed now, but please let us know if you run into more corner cases. Thanks!

[findleyr]

Thank you! I really appreciate your working on this.

Will let you know if we discovery anything else.