go/parser: non-contiguous comments grouped together

[myitcv]

What version of Go are you using (go version)?

$ go version
go version devel +adcb2b1e7a Wed Jul 3 23:08:27 2019 +0000 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output

$ go env

GO111MODULE="on"

GOARCH="amd64"

GOBIN=""

GOCACHE="/home/myitcv/.cache/go-build"

GOENV="/home/myitcv/.config/go/env"

GOEXE=""

GOFLAGS=""

GOHOSTARCH="amd64"

GOHOSTOS="linux"

GONOPROXY=""

GONOSUMDB=""

GOOS="linux"

GOPATH="/home/myitcv/gostuff"

GOPRIVATE=""

GOPROXY="https://proxy.golang.org,direct"

GOROOT="/home/myitcv/gos"

GOSUMDB="sum.golang.org"

GOTMPDIR=""

GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"

GCCGO="gccgo"

AR="ar"

CC="gcc"

CXX="g++"

CGO_ENABLED="1"

GOMOD="/home/myitcv/gostuff/src/github.com/myitcv/playground/go.mod"

CGO_CFLAGS="-g -O2"

CGO_CPPFLAGS=""

CGO_CXXFLAGS="-g -O2"

CGO_FFLAGS="-g -O2"

CGO_LDFLAGS="-g -O2"

PKG_CONFIG="pkg-config"

GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build876576551=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Consider the following:

package main

import (
    "go/ast"
    "go/parser"
    "go/token"
)

func main() {
    src := `
package p

type S struct {
    // First comment

    // Second comment

    Name string
}
`
    fset := token.NewFileSet()
    f, err := parser.ParseFile(fset, "", src, parser.ParseComments)
    if err != nil {
        panic(err)
    }
    ast.Print(fset, f.Comments)
}

Under go1.12.6 this prints:

     0  []*ast.CommentGroup (len = 2) {
     1  .  0: *ast.CommentGroup {
     2  .  .  List: []*ast.Comment (len = 1) {
     3  .  .  .  0: *ast.Comment {
     4  .  .  .  .  Slash: 5:2
     5  .  .  .  .  Text: "// First comment"
     6  .  .  .  }
     7  .  .  }
     8  .  }
     9  .  1: *ast.CommentGroup {
    10  .  .  List: []*ast.Comment (len = 1) {
    11  .  .  .  0: *ast.Comment {
    12  .  .  .  .  Slash: 7:2
    13  .  .  .  .  Text: "// Second comment"
    14  .  .  .  }
    15  .  .  }
    16  .  }
    17  }

On tip this shows:

     0  []*ast.CommentGroup (len = 1) {
     1  .  0: *ast.CommentGroup {
     2  .  .  List: []*ast.Comment (len = 2) {
     3  .  .  .  0: *ast.Comment {
     4  .  .  .  .  Slash: 5:2
     5  .  .  .  .  Text: "// First comment"
     6  .  .  .  }
     7  .  .  .  1: *ast.Comment {
     8  .  .  .  .  Slash: 7:2
     9  .  .  .  .  Text: "// Second comment"
    10  .  .  .  }
    11  .  .  }
    12  .  }
    13  }

What did you expect to see?

Non-contiguous comments not to be grouped.

What did you see instead?

Non-contiguous comments grouped.

Is this an intended side effect of https://go-review.googlesource.com/c/go/+/161177?

cc @griesemer @agnivade

[myitcv]

Note this also affects things like go doc:

package p

type S struct {
    // Hello

    // Name is a test
    Name string
}

With Go 1.12.6:

$ go doc -all
package p // import "playground.com"


TYPES

type S struct {

        // Name is a test
        Name string
}

With tip:

$ go doc -all

package p // import "playground.com"


TYPES

type S struct {
        // Hello

        // Name is a test
        Name string
}

[myitcv]

Tentatively marking as a release blocker therefore.

[dmitshur]

Thanks for reporting this.

This seems to be a direct consequence of CL 161177 to fix issue #10858.

However, issue #10858 was about godoc not displaying the entire documentation of structs, and I think it should be fixed inside godoc, not by changing go/parser to treat a comment separated by a blank line as part of a comment group.

go/ast.CommentGroup is clearly documented (emphasis mine):

A CommentGroup represents a sequence of comments with no other tokens and no empty lines between.

Unless I'm mistaken, this is too big of a change to low-level primitives like go/parser and go/ast to fix a minor godoc issue that I imagine can be fixed in godoc itself (but I haven't looked into this yet). I suspect many tools may depend on the current meaning of ast.CommentGroup and we should not (and cannot due to Go 1 compatibility promise) change it. But I will defer to @griesemer on this.

Based on my current understanding, I think we should roll back CL 161177 and try to fix #10858 in a less disruptive way.

[agnivade]

I see. The CommentGroup thing is indeed an issue. However, the go doc output seems to be more accurate to me, since that was the whole point of #10858. Would you _not_ expect // Hello to be printed ?

In any case, up to @griesemer to take a call.

[myitcv]

cc @andybons as an FYI from a release perspective

[myitcv]

I'm conscious of the fact that we're likely rapidly approaching beta2.

@agnivade - I think it makes sense to create a CL to revert this change in anticipation of @griesemer's decision. That way, at least, we're ready to hit "submit" (try bots having run etc) and not hold things up any more. Would you agree?

cc @mvdan

[gopherbot]

Change https://golang.org/cl/185040 mentions this issue: Revert "go/parser: include more comments in a struct or interface"

[griesemer]

Looking at this again, I believe @dmitshur is correct; this should perhaps be fixed in godoc. In retrospective, we should not have changed the parser for this. My apologies, I should not have missed this.

@agnivade Thanks for providing the rollback CL.

[griesemer]

A bit more on this subject:

1) Changing the parser - while it fixed the godoc issue with a relatively minor change - broke the more fundamental assumption about comment groups documented explicitly. We don't know what other tools/packages make the assumptions that comment groups have no empty lines.

2) It may be harder to fix issue #10858 in go doc, but that is probably the right place. Generally, it's not always 100% clear which comments belong together; so we should make that decision in a place with fewer dependencies (i.e., godoc rather than go/parser), so we can adjust it more easily as needed.

[dmitshur]

Thank you everyone for helping resolve this issue. I considered sending a CL to add a test so this ast.CommentGroup behavior doesn't regress without us noticing, but then I realized it would have to be a very specific test (comments separated by a blank line within a type declaration) and decided it wouldn't be as helpful as I originally thought. I can still send it if people think it'd be helpful.

I've added issue #10858 to a list of godoc-related issues I'm tracking and will be on the lookout for a way to resolve it in the future. Please feel free to /cc me on any new developments in that area.