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

x/tools/cmd/goimports: MergeLine panic on weird input #20229

Closed
josharian opened this issue May 3, 2017 · 3 comments
Closed

x/tools/cmd/goimports: MergeLine panic on weird input #20229

josharian opened this issue May 3, 2017 · 3 comments

Comments

@josharian
Copy link
Contributor

@josharian josharian commented May 3, 2017

package m
import("t"
"g"
"_"
"s")

Run goimports on that. Result:

panic: illegal line number

goroutine 1 [running]:
go/token.(*File).MergeLine(0xc420054ba0, 0x5)
	/Users/josh/go/tip/src/go/token/position.go:149 +0x145
golang.org/x/tools/go/ast/astutil.DeleteNamedImport(0xc4200132c0, 0xc42001ad80, 0x0, 0x0, 0xc420011241, 0x1, 0x100ea01)
	/Users/josh/src/golang.org/x/tools/go/ast/astutil/imports.go:267 +0xc3e
golang.org/x/tools/imports.fixImports(0xc4200132c0, 0xc42001ad80, 0x7fff5fbff9ea, 0x4, 0x21, 0x600, 0x1321770, 0xc42001ad80, 0x0)
	/Users/josh/src/golang.org/x/tools/imports/fix.go:207 +0x5d3
golang.org/x/tools/imports.Process(0x7fff5fbff9ea, 0x4, 0xc420168000, 0x21, 0x600, 0x1321770, 0x0, 0x4, 0xc420041da8, 0x0, ...)
	/Users/josh/src/golang.org/x/tools/imports/imports.go:58 +0x718
main.processFile(0x7fff5fbff9ea, 0x4, 0x0, 0x0, 0x1322660, 0xc42000c018, 0x1, 0x0, 0x0)
	/Users/josh/src/golang.org/x/tools/cmd/goimports/goimports.go:136 +0x139
main.gofmtMain()
	/Users/josh/src/golang.org/x/tools/cmd/goimports/goimports.go:273 +0x1fa
main.main()
	/Users/josh/src/golang.org/x/tools/cmd/goimports/goimports.go:189 +0x32

Discovered (accidentally) by go-fuzz.

Low priority, but probably an easy fix.

cc @bradfitz

@josharian josharian added this to the Unreleased milestone May 3, 2017
@fatih
Copy link
Member

@fatih fatih commented May 28, 2017

The problem is inside astutil pkg (the DeleteNamedImport function) and can be reproduced by adding a case to tests. It's because it tries to merge a line by assuming the last line of an import declaration is always in form of:

import (
"foo"
)

However in this case it's:

import (
"foo")

After removing foo, we need to check that it's safe to merge. I have a fix for this, should I open a CL directly or do I need to claim this issue before I can open the CL (according to the Contribution Guide)?

@josharian
Copy link
Contributor Author

@josharian josharian commented May 28, 2017

Just mail a CL. I'm happy to review.

@gopherbot
Copy link

@gopherbot gopherbot commented May 28, 2017

CL https://golang.org/cl/44372 mentions this issue.

@golang golang locked and limited conversation to collaborators May 29, 2018
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
You can’t perform that action at this time.