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/ast: CommentMap mishandles loose, trailing file comments #21755

Open
mvdan opened this issue Sep 4, 2017 · 5 comments

Comments

@mvdan
Copy link
Member

commented Sep 4, 2017

go version devel +812b34efae Mon Sep 4 00:07:33 2017 +0000 linux/amd64

Construct a comment map for:

package p

func f() {
        return
}

// loose trailing comment

The loose trailing comment is associated in the CommentMap with the
return statement inside f. It should probably be associated with f
instead, and the NewCommentMap docs updated with new rules.

Live proof: https://play.golang.org/p/XCktcSxYl8

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2017

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2017

I also see that there even is a test that includes this:

"50: *ast.Ident":      "the very last comment\n",

In that case, the trailing comment in the file is attached to an ast.Ident. I don't see a TODO nor any open issue about this, so I'm wondering if this is actually on purpose.

Another option would be to attach it to the whole ast.File. Either sounds better than the current behavior. The docs don't seem to cover this case.

@josharian josharian closed this Sep 5, 2017

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2017

Closed by mistake in one of @josharian's private repos, I assume.

@mvdan mvdan reopened this Sep 5, 2017

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2017

@josharian

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2018

Closed by mistake in one of @josharian's private repos, I assume.

Indeed. FWIW, here's the diff I used, which fixed it the way I wanted it fixed for my private purposes. I haven't thought about whether it is an appropriate fix for the go/ast package.

diff --git a/astutil/commentmap.go b/astutil/commentmap.go
index c6f0d62..7c8170f 100644
--- a/astutil/commentmap.go
+++ b/astutil/commentmap.go
@@ -206,7 +206,8 @@ func NewCommentMap(fset *token.FileSet, node ast.Node, comments []*ast.CommentGr
 			switch {
 			case pg != nil &&
 				(pgend.Line == r.pos.Line ||
-					pgend.Line+1 == r.pos.Line && r.end.Line+1 < qpos.Line):
+					pgend.Line+1 == r.pos.Line && r.end.Line+1 < qpos.Line ||
+					q == nil):
 				// 1) comment starts on same line as previous node group ends, or
 				// 2) comment starts on the line immediately after the
 				//    previous node group and there is an empty line before
diff --git a/astutil/commentmap_test.go b/astutil/commentmap_test.go
index 3df0e65..a645032 100644
--- a/astutil/commentmap_test.go
+++ b/astutil/commentmap_test.go
@@ -92,9 +92,9 @@ var res = map[string]string{
 	"31: *ast.ExprStmt":   " associated with s1\nalso associated with s1\n",
 	"37: *ast.ExprStmt":   "associated with s2\nalso associated with s2\nline comment for s2\n",
 	"45: *ast.FuncDecl":   "associated with f2\nf2\n",
+	"48: *ast.FuncDecl":   "the very last comment\n",
 	"49: *ast.AssignStmt": "addition\n",
 	"49: *ast.BasicLit":   " 1\n",
-	"50: *ast.Ident":      "the very last comment\n",
 }
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.