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

Jump to bottom

go/ast: CommentMap heuristic sensitive to parentheses around unnamed result type #23040

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

go/ast: CommentMap heuristic sensitive to parentheses around unnamed result type #23040

bcmills opened this issue Dec 7, 2017 · 6 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unplanned

Comments

@bcmills
Copy link
Contributor

bcmills 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
Copy link

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

@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Dec 11, 2017
@bradfitz bradfitz added this to the Unplanned milestone Dec 11, 2017
@bcmills bcmills mentioned this issue Oct 29, 2018
Open
@griesemer
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Contributor

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

@bcmills
Copy link
Contributor Author

bcmills 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
Assignees
No one assigned
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@bradfitz @agnivade @bcmills @griesemer @JicLotus