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

go/parser: non-contiguous comments grouped together #32944

Closed
myitcv opened this issue Jul 4, 2019 · 10 comments

Comments

Projects
None yet
5 participants
@myitcv
Copy link
Member

commented Jul 4, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2019

Tentatively marking as a release blocker therefore.

@myitcv myitcv added this to the Go1.13 milestone Jul 4, 2019

@dmitshur

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

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

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2019

cc @andybons as an FYI from a release perspective

@myitcv

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2019

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

This comment has been minimized.

Copy link

commented Jul 8, 2019

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

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

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.

@gopherbot gopherbot closed this in a19c0ce Jul 8, 2019

@dmitshur

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.