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

go/types: gotype can easily run into panics #25438

Closed
mvdan opened this issue May 17, 2018 · 8 comments
Closed

go/types: gotype can easily run into panics #25438

mvdan opened this issue May 17, 2018 · 8 comments

Comments

@mvdan
Copy link
Member

@mvdan mvdan commented May 17, 2018

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

go version devel +c3d10e64b9 Wed May 16 21:15:03 2018 +0000 linux/amd64

Does this issue reproduce with the latest release?

No.

What did you do?

$ cat ~/f.go
package p

func _(b bool) {
        if b; {
        }
}
$ ./gotype ~/f.go
/home/mvdan/f.go:4:6: missing condition in if statement
/home/mvdan/f.go:4:5: b (variable of type bool) is not used
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x57cf7c]

goroutine 1 [running]:
main.checkPkgFiles.func2()
        /home/mvdan/tip/src/go/types/gotype.go:310 +0x67
panic(0x62d860, 0x7b4d50)
        /home/mvdan/tip/src/runtime/panic.go:494 +0x1b9
go/types.(*Checker).handleBailout(0xc0000d8000, 0xc0000e1dd8)
        /home/mvdan/tip/src/go/types/check.go:215 +0x98
panic(0x62d860, 0x7b4d50)
        /home/mvdan/tip/src/runtime/panic.go:494 +0x1b9
go/types.(*Checker).exprInternal(0xc0000d8000, 0xc00004c800, 0x0, 0x0, 0x0, 0x0, 0x0)
        /home/mvdan/tip/src/go/types/expr.go:1498 +0x180c
[...]

What did you expect to see?

Just the syntax error.

What did you see instead?

An internal go/types panic.

I realise that gotype is a debugging tool, but I imagine that we still want it to be robust. The way it currently ignores syntax errors may need some work, as go/types clearly doesn't like semi-broken syntax trees.

Another option is to make go/types error instead of panic when given these semi-broken syntax trees. I don't know whether that's something the package wants to support.

/cc @griesemer @myitcv

@griesemer
Copy link
Contributor

@griesemer griesemer commented May 17, 2018

I can't reproduce this, at least for the example here. I just get

x.go:4:13: missing condition in if statement

What am I missing?

Loading

@mvdan
Copy link
Member Author

@mvdan mvdan commented May 17, 2018

Just to clarify, these are the full steps from tip:

cd tip/src/go/types
go build gotype.go
./gotype ~/f.go

I still see the crash shown above. How are you running the tool?

Also, we are getting slightly different position information in the error messages. I wonder why that is.

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented May 17, 2018

Ok, reproducing now. I suspect that a more recent causes this problem; my older gotype worked. Will investigate (probably next week - I'm out on an internal event this week).

Loading

@mvdan
Copy link
Member Author

@mvdan mvdan commented May 17, 2018

Actually, I wrote the original post wrong - gotype from 1.10.2 built with Go 1.10.2 doesn't crash. And gotype from tip built with Go 1.10.2 doesn't crash either. So this seems like a regression in 1.11's go/types.

Loading

@dominikh
Copy link
Member

@dominikh dominikh commented May 17, 2018

git bisect points at this commit, which sounds plausible:

commit 70a04f6880a2082a76f6282361b607f859db950f
Author: Robert Griesemer <gri@golang.org>
Date:   Tue Feb 13 16:37:51 2018 -0800

    go/types: make gotype continue after syntax errors
    
    This avoids odd behavior where sometimes a lot of useful
    errors are not reported simply because of a small syntax
    error.
    
    Tested manually with non-existing files. (We cannot easily
    add an automatic test because this is a stand-alone binary
    in this directory that must be built manually.)
    
    Fixes #23593.
    
    Change-Id: Iff90f95413bed7d1023fa0a5c9eb0414144428a9
    Reviewed-on: https://go-review.googlesource.com/93815
    Reviewed-by: Alan Donovan <adonovan@google.com>

Loading

@mvdan
Copy link
Member Author

@mvdan mvdan commented May 17, 2018

That doesn't make a lot of sense to me, though. If I build gotype.go with Go 1.10.2, I cannot reproduce the crash. Perhaps a change was made to go/types between Go 1.10 and February 13th, making it less stable in the face of partially broken syntax trees.

Loading

@dominikh
Copy link
Member

@dominikh dominikh commented May 17, 2018

commit c5f3a8b10797258cf527601a44bfdfa63d5ef1a7
Author: Robert Griesemer <gri@golang.org>
Date:   Thu Jan 11 15:09:38 2018 -0800

    go/parser: more robust error handling for 'if' headers
    
    R=go1.11
    
    To fix this, this CL borrows code from the new syntax
    package which has a better tuned parser at this point.
    
    Fixes #11377.
    
    Change-Id: Ib9212c945903d6f62abcc59ef5a5767d4ef36981
    Reviewed-on: https://go-review.googlesource.com/87495
    Reviewed-by: Matthew Dempsky <mdempsky@google.com>

causes this specific crash.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented May 18, 2018

Change https://golang.org/cl/113735 mentions this issue: go/parser: make sure we have a valid AST when 'if' condition is missing

Loading

@gopherbot gopherbot closed this in db37e16 May 18, 2018
@golang golang locked and limited conversation to collaborators May 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants