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
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@josharian
Copy link
Contributor

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
@bradfitz bradfitz added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels May 19, 2017
@fatih
Copy link
Member

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

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

@gopherbot
Copy link

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.
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants