go/ast: trailing func comments mishandled in CommentMap

[mvdan]

Working example: https://play.golang.org/p/TlsDxVbGow

Prints: g: "trailing comment in f\n"

Seems like the correct output should either be nothing (if it is associated with _ = x), or f: "trailing comment in f\n".

What's currently happening is that the "_ = x" in func f is the most recent "important" node when we encounter the comment. Since the comment isn't on the line immediately following "_ = x", it doesn't
get associated with it. We don't look further up the "important nodes" stack to notice that it's within the bounds of f. Instead, we just give up and associate it with g.

/cc @griesemer

[griesemer]

Feel free to take a shot at it.

[alindeman]

Is there a way to have it associated with _ = x or f while still following the three current rules? Or would a new rule need to be added?

- g starts on the same line as n ends

Not applicable in this circumstance. This is the foo := bar // baz case, as I understand it.

- g starts on the line immediately following n, and there is
  at least one empty line after g and before the next node

The line break between _ = x and the comment prevents this one from applying.

- g starts before n and is not associated to the node before n
  via the previous rules

At this point, g can only be attached to a node after its appearance.

[soreavis]

Still reproduces at tip (go1.27-devel): the example's comment maps to *ast.FuncDecl g. The mechanism matches the original description, at today's line numbers: when the comment is processed, stack.pop(r.comment.Pos()) (commentmap.go:169) pops _ = x but leaves f open on the stack (its closing brace is after the comment) — and then neither the two line-adjacency rules nor the default branch ever consult the still-open stack, so assoc = q crosses f's boundary and lands on g.

Since the enclosing node is sitting right there on the stack, the fix can be small. I prototyped this, between the p rule and the default:

+			case len(stack) > 0 && q != nil && stack[len(stack)-1].End() <= q.Pos():
+				// the comment lies within a node still open on the stack,
+				// and the current node starts beyond that node's end;
+				// associate the comment with the enclosing node rather
+				// than crossing its boundary, preferring the largest
+				// enclosing node ending at the same position (e.g. the
+				// declaration rather than its body)
+				i := len(stack) - 1
+				for i > 0 && stack[i-1].End() == stack[i].End() {
+					i--
+				}
+				assoc = stack[i]
 			default:

The guard only fires when associating with q would cross the innermost open node's end, and since the stack only ever holds File/Field/Decl/Spec/Stmt nodes the association stays on a node of importance. With the prototype, the example above yields f: "trailing comment in f" (one of the two outcomes the report suggested); a standalone comment between f and g in doc position still maps to g; same-line and doc-comment rules are unchanged; and go test go/ast go/printer go/format go/doc cmd/gofmt all pass. To be clear about the breadth: this reassociates the whole class of trailing-before-closing-brace comments, not just the example — a trailing comment in a struct's braces moves to its type declaration, in a switch's last case to the SwitchStmt, in a var (...) block to the GenDecl, and (the most surprising one) in a function literal's body the same-End walk climbs to the enclosing statement. In my probing each of these is an improvement over crossing into the following node, and the walk can never reach a non-ancestor, but a CL would pin all these shapes with table tests.

Two things a CL needs to settle beyond the prototype. First, the same shape as the last declaration in the file isn't covered by this guard: with no following node the existing q == nil rule associates the comment with the last leaf node (an *ast.Ident), and that behavior is pinned by an existing expectation in commentmap_test.go ("the very last comment") — handling both variants consistently means deciding that case too and updating that expectation. Second, the compatibility question: NewCommentMap is a documented heuristic, and in-tree consumers exist — gofmt -r preserves comments across rewrites via cmap.Filter(r).Comments(), so association changes determine which comments survive for any node-filtering tool — plus whatever third-party tools rely on the current association. Nothing in-tree pins the boundary-crossing behavior itself, but the doc's three rules would need a sentence about enclosing-node boundaries.

If the direction looks right, I'm happy to send the CL with tests.