code review 182970043: cmd/go: avoid use of bufio.Scanner in generate

cmd/go: avoid use of bufio.Scanner in generate

Scanner can't handle stupid long lines and there are
reports of stupid long lines in production.

Note the issue isn't long "//go:generate" lines, but
any long line in any Go source file.

To be fair, if you're going to have a stupid long line
it's not a bad bet you'll want to run it through go
generate, because it's some embeddable asset that
has been machine generated. (One could ask why
that generation process didn't add a newline or two,
but we should cope anyway.)

Rewrite the file scanner in "go generate" so it can
handle arbitrarily long lines, and only stores in memory
those lines that start "//go:generate".

Also: Adjust the documentation to make clear that it
does not parse the file.

Fixes issue 9143.
Fixes issue 9196.

[r, 2014-12-02 04:55:20]

Hello rsc (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[rsc, 2014-12-02 05:30:13]

LGTM

what you have is fine, but it can be shortened with ReadSlice.

https://codereview.appspot.com/182970043/diff/1/src/cmd/go/generate.go
File src/cmd/go/generate.go (right):

https://codereview.appspot.com/182970043/diff/1/src/cmd/go/generate.go#newcod...
src/cmd/go/generate.go:193: for i := range marker { // Will be one loop per
byte, and that's fine.
Everyone maligns ReadSlice, but this is exactly what it's for.

func isGoGenerate(buf []byte) bool {
    return bytes.HasPrefix(buf, []byte("//go:generate ") || bytes.HasPrefix(buf,
[]byte("//go:generate\t"))
}

buf, err := input.ReadSlice('\n')
if err == bufio.ErrBufferFull {
    // line too long - consume and ignore
    if isGoGenerate(buf) {
        g.errorf("directive too long")
    }
    for err == bufio.ErrBufferFull {
        _, err = input.ReadSlice('\n')
    }
    if err != nil {
        break
    }
    continue
}

if err != nil {
    // Check for marker at EOF without final \n.
    if err == io.EOF && isGoGenerate(buf) {
        err = io.ErrUnexpectedEOF
    }
    break
}

if !isGoGenerate(buf) {
    continue
}

// Have full //go:generate line including \n in buf.
...

[r, 2014-12-02 05:49:09]

Hello rsc@golang.org (cc: golang-codereviews@googlegroups.com),

Please take another look.

[r, 2014-12-02 05:49:51]

Not sure I like yours much better, but I'll take it.

-rob


On Tue, Dec 2, 2014 at 2:30 PM, <rsc@golang.org> wrote:

> LGTM
>
> what you have is fine, but it can be shortened with ReadSlice.
>
>
>
> https://codereview.appspot.com/182970043/diff/1/src/cmd/go/generate.go
> File src/cmd/go/generate.go (right):
>
> https://codereview.appspot.com/182970043/diff/1/src/cmd/
> go/generate.go#newcode193
> src/cmd/go/generate.go:193: for i := range marker { // Will be one loop
> per byte, and that's fine.
> Everyone maligns ReadSlice, but this is exactly what it's for.
>
> func isGoGenerate(buf []byte) bool {
>     return bytes.HasPrefix(buf, []byte("//go:generate ") ||
> bytes.HasPrefix(buf, []byte("//go:generate\t"))
> }
>
> buf, err := input.ReadSlice('\n')
> if err == bufio.ErrBufferFull {
>     // line too long - consume and ignore
>     if isGoGenerate(buf) {
>         g.errorf("directive too long")
>     }
>     for err == bufio.ErrBufferFull {
>         _, err = input.ReadSlice('\n')
>     }
>     if err != nil {
>         break
>     }
>     continue
> }
>
> if err != nil {
>     // Check for marker at EOF without final \n.
>     if err == io.EOF && isGoGenerate(buf) {
>         err = io.ErrUnexpectedEOF
>     }
>     break
> }
>
> if !isGoGenerate(buf) {
>     continue
> }
>
> // Have full //go:generate line including \n in buf.
> ...
>
> https://codereview.appspot.com/182970043/
>

[rsc, 2014-12-02 06:13:41]

LGTM

https://codereview.appspot.com/182970043/diff/40001/src/cmd/go/generate.go
File src/cmd/go/generate.go (right):

https://codereview.appspot.com/182970043/diff/40001/src/cmd/go/generate.go#ne...
src/cmd/go/generate.go:185: buf := make([]byte, 2048)
Delete. Declare var buf []byte inside loop, after lineNum++.
ReadSlice returns its own bytes. You just need a decl to avoid the :=.

[r, 2014-12-03 02:26:35]

Hello rsc@golang.org (cc: golang-codereviews@googlegroups.com),

Please take another look.

[cespare, 2014-12-03 02:44:19]

Why is there an unrelated documentation change (concerning get -f) in this CL?

[r, 2014-12-03 02:47:43]

Should be gone now. I was behind tip and my doc.go was old. I synced and
uploaded.


On Wed, Dec 3, 2014 at 11:44 AM, <cespare@gmail.com> wrote:

> Why is there an unrelated documentation change (concerning get -f) in
> this CL?
>
> https://codereview.appspot.com/182970043/
>

[minux, 2014-12-03 02:55:08]

It seems you still need to run mkdoc.sh, doc.go in Patch Set 6 doesn't contain
the doc fix for issue 9196.

[r, 2014-12-03 03:26:55]

Done.

[r, 2014-12-03 03:36:07]

Hello rsc@golang.org, cespare@gmail.com, minux@golang.org (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[Dominik Honnef, 2014-12-03 09:47:34]

On 2014/12/03 03:36:07, r wrote:
> Hello mailto:rsc@golang.org, mailto:cespare@gmail.com, mailto:minux@golang.org
(cc:
> mailto:golang-codereviews@googlegroups.com),
> 
> Please take another look.

doc.go still a) undoes a typo fix from a previous CL b) doesn't include the
documentation changes from this CL

[r, 2014-12-03 11:38:53]

I have no idea what's going on. I am at tip, synced, built the command, ran
the script.
I'l reset and try again.

-rob


On Wed, Dec 3, 2014 at 6:47 PM, <dominik.honnef@gmail.com> wrote:

> On 2014/12/03 03:36:07, r wrote:
>
>> Hello mailto:rsc@golang.org, mailto:cespare@gmail.com,
>>
> mailto:minux@golang.org (cc:
>
>> mailto:golang-codereviews@googlegroups.com),
>>
>
>  Please take another look.
>>
>
> doc.go still a) undoes a typo fix from a previous CL b) doesn't include
> the documentation changes from this CL
>
> https://codereview.appspot.com/182970043/
>

[r, 2014-12-03 11:39:40]

Hello rsc@golang.org, cespare@gmail.com, minux@golang.org,
dominik.honnef@gmail.com (cc: golang-codereviews@googlegroups.com),

Please take another look.

[r, 2014-12-03 11:40:17]

All this for a CL that won't ever be checked in to this repo.

-rob


On Wed, Dec 3, 2014 at 8:38 PM, Rob Pike <r@golang.org> wrote:

> I have no idea what's going on. I am at tip, synced, built the command,
> ran the script.
> I'l reset and try again.
>
> -rob
>
>
> On Wed, Dec 3, 2014 at 6:47 PM, <dominik.honnef@gmail.com> wrote:
>
>> On 2014/12/03 03:36:07, r wrote:
>>
>>> Hello mailto:rsc@golang.org, mailto:cespare@gmail.com,
>>>
>> mailto:minux@golang.org (cc:
>>
>>> mailto:golang-codereviews@googlegroups.com),
>>>
>>
>>  Please take another look.
>>>
>>
>> doc.go still a) undoes a typo fix from a previous CL b) doesn't include
>> the documentation changes from this CL
>>
>> https://codereview.appspot.com/182970043/
>>
>
>

[Dominik Honnef, 2014-12-03 12:14:03]

LGTM

On 2014/12/03 11:40:17, r wrote:
> All this for a CL that won't ever be checked in to this repo.
> 
> -rob
> 
> 
> On Wed, Dec 3, 2014 at 8:38 PM, Rob Pike <mailto:r@golang.org> wrote:
> 
> > I have no idea what's going on. I am at tip, synced, built the command,
> > ran the script.
> > I'l reset and try again.
> >
> > -rob
> >
> >
> > On Wed, Dec 3, 2014 at 6:47 PM, <mailto:dominik.honnef@gmail.com> wrote:
> >
> >> On 2014/12/03 03:36:07, r wrote:
> >>
> >>> Hello mailto:rsc@golang.org, mailto:cespare@gmail.com,
> >>>
> >> mailto:minux@golang.org (cc:
> >>
> >>> mailto:golang-codereviews@googlegroups.com),
> >>>
> >>
> >>  Please take another look.
> >>>
> >>
> >> doc.go still a) undoes a typo fix from a previous CL b) doesn't include
> >> the documentation changes from this CL
> >>
> >> https://codereview.appspot.com/182970043/
> >>
> >
> >

[Dominik Honnef, 2014-12-03 12:14:10]

LGTM

On 2014/12/03 11:40:17, r wrote:
> All this for a CL that won't ever be checked in to this repo.
> 
> -rob
> 
> 
> On Wed, Dec 3, 2014 at 8:38 PM, Rob Pike <mailto:r@golang.org> wrote:
> 
> > I have no idea what's going on. I am at tip, synced, built the command,
> > ran the script.
> > I'l reset and try again.
> >
> > -rob
> >
> >
> > On Wed, Dec 3, 2014 at 6:47 PM, <mailto:dominik.honnef@gmail.com> wrote:
> >
> >> On 2014/12/03 03:36:07, r wrote:
> >>
> >>> Hello mailto:rsc@golang.org, mailto:cespare@gmail.com,
> >>>
> >> mailto:minux@golang.org (cc:
> >>
> >>> mailto:golang-codereviews@googlegroups.com),
> >>>
> >>
> >>  Please take another look.
> >>>
> >>
> >> doc.go still a) undoes a typo fix from a previous CL b) doesn't include
> >> the documentation changes from this CL
> >>
> >> https://codereview.appspot.com/182970043/
> >>
> >
> >

[rsc, 2014-12-04 16:29:13]

I think this should probably go into the release. //go:generate is new in 1.4,
might as well never distribute the version that can't handle very long lines.

[r, 2014-12-05 00:15:46]

*** Submitted as https://code.google.com/p/go/source/detail?r=573a7b5178c4 ***

cmd/go: avoid use of bufio.Scanner in generate

Scanner can't handle stupid long lines and there are
reports of stupid long lines in production.

Note the issue isn't long "//go:generate" lines, but
any long line in any Go source file.

To be fair, if you're going to have a stupid long line
it's not a bad bet you'll want to run it through go
generate, because it's some embeddable asset that
has been machine generated. (One could ask why
that generation process didn't add a newline or two,
but we should cope anyway.)

Rewrite the file scanner in "go generate" so it can
handle arbitrarily long lines, and only stores in memory
those lines that start "//go:generate".

Also: Adjust the documentation to make clear that it
does not parse the file.

Fixes issue 9143.
Fixes issue 9196.

LGTM=rsc, dominik.honnef
R=rsc, cespare, minux, dominik.honnef
CC=golang-codereviews
https://codereview.appspot.com/182970043

[gobot, 2014-12-05 00:17:29]

This CL appears to have broken the solaris-amd64-smartos builder.
See http://build.golang.org/log/9ffdfbe8dbc2594203d37ab4768cd75e49484732

[dave_cheney.net, 2014-12-05 00:19:29]

This is a real failure,

--- FAIL: TestGenerateCommandParse (0.00s)
panic: runtime error: slice bounds out of range [recovered]
panic: runtime error: slice bounds out of range

goroutine 8 [running]:
testing.func·006()
/usr/local/go/src/testing/testing.go:441 +0x14f
cmd/go.(*Generator).split(0x18734600, 0x83e1a78, 0xe, 0x0, 0x0, 0x0)
/linux-386-387-573a7b5178c4/go/src/cmd/go/generate.go:257 +0x7b6
cmd/go.TestGenerateCommandParse(0x18756900)
/linux-386-387-573a7b5178c4/go/src/cmd/go/generate_test.go:43 +0x1d6
testing.tRunner(0x18756900, 0x8597ec0)
/usr/local/go/src/testing/testing.go:447 +0xb1
created by testing.RunTests
/usr/local/go/src/testing/testing.go:555 +0x863

goroutine 1 [chan receive]:
testing.RunTests(0x8482318, 0x8597ec0, 0x8, 0x8, 0x1)
/usr/local/go/src/testing/testing.go:556 +0x89f
testing.(*M).Run(0x187344e0, 0x85a0da0)
/usr/local/go/src/testing/testing.go:485 +0x5e
main.main()
cmd/go/_test/_testmain.go:66 +0x177

goroutine 5 [syscall]:
os/signal.loop()
/usr/local/go/src/os/signal/signal_unix.go:21 +0x21
created by os/signal.init·1
/usr/local/go/src/os/signal/signal_unix.go:27 +0x34
FAIL cmd/go 0.064s


On Fri, Dec 5, 2014 at 11:17 AM,  <gobot@golang.org> wrote:
> This CL appears to have broken the solaris-amd64-smartos builder.
> See http://build.golang.org/log/9ffdfbe8dbc2594203d37ab4768cd75e49484732
>
>
> https://codereview.appspot.com/182970043/
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[minux, 2014-12-05 00:19:43]

Seems real:

--- FAIL: TestGenerateCommandParse (0.00s)
panic: runtime error: slice bounds out of range [recovered]
panic: runtime error: slice bounds out of range

goroutine 8 [running]:
testing.func·006()
/usr/local/go/src/testing/testing.go:441 +0x181
cmd/go.(*Generator).split(0xc208054600, 0x8a2350, 0xe, 0x0, 0x0, 0x0)
/root/work/solaris-amd64-smartos-573a7b5178c4/go/src/cmd/go/generate.go:257
+0xa14
cmd/go.TestGenerateCommandParse(0xc20800c5a0)
/root/work/solaris-amd64-smartos-573a7b5178c4/go/src/cmd/go/generate_test.go:43
+0x26c
testing.tRunner(0xc20800c5a0, 0xab5840)
/usr/local/go/src/testing/testing.go:447 +0xbf
created by testing.RunTests
/usr/local/go/src/testing/testing.go:555 +0xa8b

goroutine 1 [chan receive]:
testing.RunTests(0x968018, 0xab5840, 0x8, 0x8, 0xc20802af01)
/usr/local/go/src/testing/testing.go:556 +0xad6
testing.(*M).Run(0xc2080512c0, 0xac2a60)
/usr/local/go/src/testing/testing.go:485 +0x6c
main.main()
cmd/go/_test/_testmain.go:66 +0x1d5

goroutine 5 [syscall]:
os/signal.loop()
/usr/local/go/src/os/signal/signal_unix.go:21 +0x1f
created by os/signal.init·1
/usr/local/go/src/os/signal/signal_unix.go:27 +0x35
FAIL cmd/go 0.033s

On Thu, Dec 4, 2014 at 7:17 PM, <gobot@golang.org> wrote:

> This CL appears to have broken the solaris-amd64-smartos builder.
> See http://build.golang.org/log/9ffdfbe8dbc2594203d37ab4768cd75e49484732
>

[r, 2014-12-05 00:20:51]

I ran go test a hundred times yesterday but somehow I needed to run it once
more. Will fix.

-rob


On Fri, Dec 5, 2014 at 9:17 AM, <gobot@golang.org> wrote:

> This CL appears to have broken the solaris-amd64-smartos builder.
> See http://build.golang.org/log/9ffdfbe8dbc2594203d37ab4768cd75e49484732
>
> https://codereview.appspot.com/182970043/
>