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 heuristic sensitive to parentheses around unnamed result type #23040

Open
bcmills opened this issue Dec 7, 2017 · 6 comments

Comments

@bcmills
Copy link
Member

commented Dec 7, 2017

Background

I want to use structured comments to associate metadata with function parameter types in Go source code, with the function bodies automatically generated by an external tool.

Problem

There is a bad interaction between (*printer.Config).Fprint and ast.NewCommentMap for comments on function results.

If a function returns only one result, has an associated trailing comment, and lacks a body (e.g., because it is implemented in assembly), NewCommentMap will only associate the comment with the result if both are enclosed in parentheses. (Otherwise, the algorithm of NewCommentMap instead associates the comment with the entire file.)

Unfortunately, (*printer.Config).Fprint omits those parentheses, so running gofmt on such a file breaks the association between the metadata and the function result.

Example: https://play.golang.org/p/ovMOY6e5pB

package foo

func Foo() (error /*metadata*/)

formats to

package foo

func Foo() error /*metadata*/

Proposal

The problematic check is here:

go/src/go/printer/nodes.go

Lines 344 to 348 in 7c46b62

if n == 1 && result.List[0].Names == nil {
// single anonymous result; no ()'s
p.expr(stripParensAlways(result.List[0].Type))
return
}

I suspect it would be fixed by weakening that condition to:

if n == 1 && result.List[0].Names == nil &&
   (!result.Closing.IsValid() || !p.commentBefore(p.posFor(result.Closing)) {
@JicLotus

This comment has been minimized.

Copy link

commented Dec 10, 2017

Hi there. Do you have any problem if i take this issue?
regards

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Oct 29, 2018

It seems to me that this is a problem with CommentMap - it should associate the comment with the result field, independent on whether the ()'s are present or not. It's a heuristic that needs tuning.

@griesemer griesemer changed the title go/{ast,printer}: printing breaks comment association with single function result go/ast: CommentMap heuristic sensitive to parentheses around unnamed result type Oct 29, 2018

@griesemer griesemer self-assigned this Oct 29, 2018

@agnivade

This comment has been minimized.

Copy link
Member

commented Feb 16, 2019

@griesemer - I added an error field with a block comment in f1 in commentmap_test.go, and ran the test. The comment seems to be attached to the error field.

...
// f1
func f1() error /* associated with error */{
	/* associated with s1 */
	s1()
...
$gotip test -v -run=CommentMap
=== RUN   TestCommentMap
...
	"29: *ast.Field":	" associated with error\n",
	"29: *ast.FuncDecl":	"f1\nassociated with f1\nalso associated with f1\n",
...

Thoughts ?

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

@bcmills While one could adjust the nodes.go printing part of results as you suggest, I am not sure this is the "right" approach. Is there a good reason for those parentheses to remain if there is a comment, unrelated to the problem with comment maps? I.e., assuming comment maps work as expected, why should we keep the parentheses in this case?

@agnivade Per @bcmills, the problem occurs if the function does not have a function body. f1() does have a function body. If I add an additional test func f4() error /* foo */ to the src, the comment does get associated to the file, which looks incorrect.

@agnivade

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

@griesemer - Ahh, that's right. That slipped my mind while running the commentmap test.

@bcmills

This comment has been minimized.

Copy link
Member Author

commented Feb 22, 2019

assuming comment maps work as expected, why should we keep the parentheses in this case?

For the example

func Foo() error /*metadata*/

I would find it ambiguous whether the comment is intended to be a line-comment for the function or an inline comment for the error.

In contrast, in

func f1() error /* associated with error */ {

and

func f1() error {} /* associated with function? */

the position of the comment relative to the braces seems to make the intent of the comment fairly explicit.

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