gordonklaus / ineffassign

Detect ineffectual assignments in Go code.
MIT License
394 stars 22 forks source link

Panic #28

Closed alecthomas closed 6 years ago

alecthomas commented 6 years ago
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x10c3a5d]

goroutine 1 [running]:
main.(*branchStack).get(0xc420341258, 0x0, 0xc42034c2a0)
    /Users/aat/.go/src/github.com/alecthomas/gometalinter/_linters/src/github.com/gordonklaus/ineffassign/ineffassign.go:512 +0x6d
main.(*builder).Visit(0xc420341200, 0x111a2a0, 0xc4203ea560, 0x1119ce0, 0xc420341200)
    /Users/aat/.go/src/github.com/alecthomas/gometalinter/_linters/src/github.com/gordonklaus/ineffassign/ineffassign.go:226 +0x15c3
go/ast.Walk(0x1119ce0, 0xc420341200, 0x111a2a0, 0xc4203ea560)
    /usr/local/Cellar/go/1.10.2/libexec/src/go/ast/walk.go:52 +0x66
go/ast.walkStmtList(0x1119ce0, 0xc420341200, 0xc4203ea580, 0x2, 0x2)
    /usr/local/Cellar/go/1.10.2/libexec/src/go/ast/walk.go:32 +0x81
go/ast.Walk(0x1119ce0, 0xc420341200, 0x111a260, 0xc4203e65a0)
    /usr/local/Cellar/go/1.10.2/libexec/src/go/ast/walk.go:224 +0x1b61
main.(*builder).walk(0xc420341200, 0x111a260, 0xc4203e65a0)
    /Users/aat/.go/src/github.com/alecthomas/gometalinter/_linters/src/github.com/gordonklaus/ineffassign/ineffassign.go:122 +0x5a
main.(*builder).Visit(0xc420341200, 0x111a860, 0xc4203c1d80, 0x0, 0x0)
    /Users/aat/.go/src/github.com/alecthomas/gometalinter/_linters/src/github.com/gordonklaus/ineffassign/ineffassign.go:139 +0x236f
go/ast.Walk(0x1119ce0, 0xc420341200, 0x111a860, 0xc4203c1d80)
    /usr/local/Cellar/go/1.10.2/libexec/src/go/ast/walk.go:52 +0x66
go/ast.walkStmtList(0x1119ce0, 0xc420341200, 0xc42033b300, 0x9, 0x10)
    /usr/local/Cellar/go/1.10.2/libexec/src/go/ast/walk.go:32 +0x81
go/ast.Walk(0x1119ce0, 0xc420341200, 0x111a260, 0xc4203e65d0)
    /usr/local/Cellar/go/1.10.2/libexec/src/go/ast/walk.go:224 +0x1b61
main.(*builder).walk(0xc420341200, 0x111a260, 0xc4203e65d0)
    /Users/aat/.go/src/github.com/alecthomas/gometalinter/_linters/src/github.com/gordonklaus/ineffassign/ineffassign.go:122 +0x5a
main.(*builder).fun(0xc420341200, 0xc4203ea820, 0xc4203e65d0)
    /Users/aat/.go/src/github.com/alecthomas/gometalinter/_linters/src/github.com/gordonklaus/ineffassign/ineffassign.go:379 +0x1aa
main.(*builder).Visit(0xc420341200, 0x111a6e0, 0xc4203e6600, 0x0, 0x0)
    /Users/aat/.go/src/github.com/alecthomas/gometalinter/_linters/src/github.com/gordonklaus/ineffassign/ineffassign.go:130 +0xc84
go/ast.Walk(0x1119ce0, 0xc420341200, 0x111a6e0, 0xc4203e6600)
    /usr/local/Cellar/go/1.10.2/libexec/src/go/ast/walk.go:52 +0x66
go/ast.walkDeclList(0x1119ce0, 0xc420341200, 0xc4203b6580, 0x7, 0x8)
    /usr/local/Cellar/go/1.10.2/libexec/src/go/ast/walk.go:38 +0x81
go/ast.Walk(0x1119ce0, 0xc420341200, 0x111a660, 0xc4203b6780)
    /usr/local/Cellar/go/1.10.2/libexec/src/go/ast/walk.go:353 +0x2650
main.(*builder).walk(0xc420341200, 0x111a660, 0xc4203b6780)
    /Users/aat/.go/src/github.com/alecthomas/gometalinter/_linters/src/github.com/gordonklaus/ineffassign/ineffassign.go:122 +0x5a
main.checkPath(0xc42038eb60, 0x67, 0x3, 0xc42034db01, 0x1063946, 0xc42038ebd0, 0xc42038eb60, 0x67, 0xc42034db50)
    /Users/aat/.go/src/github.com/alecthomas/gometalinter/_linters/src/github.com/gordonklaus/ineffassign/ineffassign.go:79 +0xfa
main.walkPath.func1(0xc42038eb60, 0x67, 0x111bd60, 0xc42039fee0, 0x0, 0x0, 0xc, 0xc42034dc48)
    /Users/aat/.go/src/github.com/alecthomas/gometalinter/_linters/src/github.com/gordonklaus/ineffassign/ineffassign.go:61 +0x165
path/filepath.walk(0xc42038eb60, 0x67, 0x111bd60, 0xc42039fee0, 0xc4200a8060, 0x0, 0x0)
    /usr/local/Cellar/go/1.10.2/libexec/src/path/filepath/path.go:357 +0x402
path/filepath.walk(0xc42030df80, 0x5a, 0x111bd60, 0xc42039fe10, 0xc4200a8060, 0x0, 0x0)
    /usr/local/Cellar/go/1.10.2/libexec/src/path/filepath/path.go:381 +0x2c2
path/filepath.walk(0xc42008e120, 0x50, 0x111bd60, 0xc420096b60, 0xc4200a8060, 0x0, 0x20)
    /usr/local/Cellar/go/1.10.2/libexec/src/path/filepath/path.go:381 +0x2c2
path/filepath.Walk(0xc42008e120, 0x50, 0xc4200a8060, 0x50, 0x0)
    /usr/local/Cellar/go/1.10.2/libexec/src/path/filepath/path.go:403 +0x106
main.walkPath(0xc42008e120, 0x50, 0xc42008e120)
    /Users/aat/.go/src/github.com/alecthomas/gometalinter/_linters/src/github.com/gordonklaus/ineffassign/ineffassign.go:45 +0x9a
main.main()
    /Users/aat/.go/src/github.com/alecthomas/gometalinter/_linters/src/github.com/gordonklaus/ineffassign/ineffassign.go:34 +0x7a
gordonklaus commented 6 years ago

@alecthomas This seems to be the result of processing a continue statement outside a for statement. It would be nice if you could verify this by providing the code that was passed to ineffassign.

It's easy to fix this on my end. But ideally, code that doesn't compile shouldn't be passed to ineffassign, since it provides checks beyond basic Go validity and depends on it. Can you arrange to run ineffassign conditionally, or (less ideally) ignore it when it panics?

I suppose ineffassign could run its inputs through go/types and fail early. But that's a somewhat bigger change on my end, as ineffasign currently works on a file-by-file basis. This would also add some unnecessary overhead to the tool, as it doesn't need type information.

alecthomas commented 6 years ago

That's exactly what it is, yep. It was in this state temporarily due to some in-progress refactoring.

It's easy to fix this on my end. But ideally, code that doesn't compile shouldn't be passed to ineffassign, since it provides checks beyond basic Go validity and depends on it. Can you arrange to run ineffassign conditionally, or (less ideally) ignore it when it panics?

IMO it's not reasonable to panic when the code is ill-formed, but OTOH I agree it's not ideal having to use go/types. I'm not sure what the right course of action is :(

Note that this also occurs when ineffassign is run inside https://github.com/golangci/golangci-lint.

gordonklaus commented 6 years ago

You're right, panicking is not great. I'll fix this case and any others that pop up eventually.