-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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: can fail when a .go file has a line of 65536+ length #18201
Comments
Any such code is probably auto-generated. Who runs goimports on auto-generated code? |
I ran into this issue on code I wrote by hand (most of it was "written" by using Cmd+V). It was a quick experiment/prototype of a thought. It was not production code and not meant to be committed anywhere, just throwaway code to be run once and give me an answer to something. My editor runs goimports on save. I expected it to add "golang.org/x/net/html" import, but it didn't because of this issue. |
Maybe I should mention, I consider this issue very low priority, and don't care about it being fixed anytime soon. I'm mainly reporting it because I discovered a bug, and my personal policy is to always try to report bugs in Go in pursuit of contributing to making it better, instead of just ignoring them and moving on. I have no immediate plans to work on this issue right now, but I might in future, unless it happens to be resolved by then. For now my goal was just to file an issue, and do a bit of investigation to find the cause. :) |
We have run into this issue. We want to run goimports against our entire source tree. There is one large generated file that exhibits the issue. I have tried to use the exclude file to exclude the directory the large file is in but it appears that the exclude functionality presently is not called when executing goimports. I tried to figure out why that is and that problem seems significantly more complicated. The problem reported here can be resolved by adding a single line of code that increases the max token size allowed by Scanner. There is also the related issue of a scanner error not being reported. I have changes for both of these (one line each) and tests that cover them. I've not submitted a contribution before. I assume the process in https://golang.org/doc/contribute.html must be followed even for a simple change like this. |
If somebody wants to fix this, feel free. And if the easiest "fix" is to simply detect this case (a ridiculously long line) and do nothing on that file and treat it as already import-happy and gofmt-happy, I'm also cool with that. |
Change https://golang.org/cl/83800 mentions this issue: |
Change https://golang.org/cl/84155 mentions this issue: |
Change https://golang.org/cl/93439 mentions this issue: |
Previous work to resolve golang/go#18201 increased the maximum line length that goimports could handle to 1MiB (CL83800), but generated code can result in Go files with longer lines. Use a bufio.Reader instead of a bufio.Scanner to support arbitrarily long lines, as permitted by the Go spec. Change-Id: If719e531859804304d60a8c00db6304ab3d5fe5e Reviewed-on: https://go-review.googlesource.com/93439 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Foreword
I ran into this a while ago, but only getting around to investigating and submitting an issue report with a minimal reproduce case just now. I've also looked at the code and I can see exactly where the issue is.
What version of Go are you using (
go version
)?What operating system and processor architecture are you using (
go env
)?What did you do?
I wrote this minimal reproduce case, a file
main.go
. It contains one line that has length equal tobufio.MaxScanTokenSize
(i.e., 65536) or greater.Then I ran
goimports -d main.go
.What did you expect to see?
What did you see instead?
If you make the long line 1 byte shorter, the problem goes away.
Depending on the exact position of the long line,
goimports
can fail in a different way, by running with 0 exit code and producing incorrect output.Cause
This bug was easy to track down, it was my first guess that
bufio.Scanner
is being used and that's exactly the case. 65536 is the value ofbufio.MaxScanTokenSize
.goimports
will try to add "fmt" package. Since "fmt" is in GOROOT, and x/net/html isn't, it causesaddImportSpaces
to be executed in order to add a blank line between different import groups. The problem is insideaddImportSpaces
, and it's quite visible:The problem is that
bufio.NewScanner(r)
is being used to try to process and reconstruct the .go file contents. That fails when a single line is 65536 bytes or more.Solution
To support arbitrary files that may contain lines of unrestricted length (since the Go specification does not restrict the length of any line inside a .go file), the fix is to rewrite
addImportSpaces
to not usebufio.Scanner
because of its inherent limitation on token size.The text was updated successfully, but these errors were encountered: