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: unexpected associations for comments in empty function/loop bodies #39753

Open
ec-m opened this issue Jun 22, 2020 · 8 comments
Open

go/ast: unexpected associations for comments in empty function/loop bodies #39753

ec-m opened this issue Jun 22, 2020 · 8 comments
Assignees
@griesemer
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.23

Comments

@ec-m
Copy link

ec-m commented Jun 22, 2020

Comments that are placed inside empty function or loop bodies are associated with statements after the body, hence statements outside of the scope the comments are in. I would expect them to be associated with the function or loop declaration. This snipped demonstrates the issue:

package main

func main() {
}

func foo() {
	// inside empty function, associated with function bar below
}

func bar() {
	i := 1
	for i < 2 {
		// inside empty for, associated with if i == 3
	}
	// after empty for loop, this, however, is associated with for loop above

	if i == 3 {
		// inside empty if, associated with i = 4
	}
	i = 4
}

And here you can find code to reproduce the issue: https://play.golang.org/p/m4j-OTbdi-L

As @griesemer mentioned in #20744, the comment association heuristic is not straight forward to implement. Thus, I am wondering whether the above shown associations are intentional or whether this is a bug. Thanks for the clarification!

$ go version
go version go1.14.2 darwin/amd64
go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/.../Library/Caches/go-build"
GOENV="/Users/.../Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/.../go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/.../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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/yb/hqncynqs0b5_3hyxcjpdsyr00000gr/T/go-build447564702=/tmp/go-build -gno-record-gcc-switches -fno-common"
@griesemer griesemer added NeedsFix The path to resolution is known, but the work has not been done. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 22, 2020
@gopherbot gopherbot removed the NeedsFix The path to resolution is known, but the work has not been done. label Jun 22, 2020
@griesemer
Copy link
Contributor

Not having looked at this in detail, from your description this sounds like a bug; at least I don't see why this would be intentional.

@griesemer griesemer self-assigned this Jun 22, 2020
@griesemer griesemer added this to the Go1.16 milestone Jun 22, 2020
@cagedmantis cagedmantis changed the title go/ast: Unexpected associations for comments in empty function/loop bodies go/ast: unexpected associations for comments in empty function/loop bodies Jun 22, 2020
@ec-m ec-m mentioned this issue Jun 23, 2020
Closed
@dave
Copy link
Contributor

dave commented Jun 23, 2020

In dst the algorithm to associate comments with decoration attachment points was by no means straightforward. If it finds nothing below the comment it bounces back and searches before the comment. In this example it would attach to the opening brace of the BlockStmt.

ec-m pushed a commit to ec-m/go that referenced this issue Jun 26, 2020
go/ast: improve comment associations in empty function/loop/if bodies
7b0f14c 
Fixes golang#39753

Signed-off-by: Eva Charlotte Mayer <eva.mayer@tum.de>
@ec-m
Copy link
Author

ec-m commented Jun 26, 2020 •
edited

As @dave pointed out, the question is now, with which nodes we want to associate the comments in empty bodies.

I changed the function NewCommentMap(...) in go/ast/commentmap.go by slightly improving the association heuristic to also cover empty body cases:

image

Please see here for the full changes that also add an additional unit test: ec-m@7b0f14c

With this fix, comments inside empty function/loop/if bodies are associated to the *ast.BlockStmt nodes that represent the empty bodies.

Do you think this would be an acceptable way to associate the comments, @griesemer? If so, I will open a pull request with Gerrit.
Or would you expect the associated node to be something else, e.g. the function declaration itself? Thanks for the feedback.

@odeke-em
Copy link
Member

Howdy @ec-m @dave @griesemer!

Thank you for filing, and investigating this bug, @ec-m. I'd say please go for it, please mail that change; @griesemer's plate is heavily stacked, but with your CL up for review, that provides more actionable feedback and brings in more reviewers. It would be awesome to have you as a Go contributor, @ec-m! Happy holidays!

@odeke-em odeke-em modified the milestones: Go1.16, Go1.17 Dec 28, 2020
@ec-m
Copy link
Author

ec-m commented Dec 28, 2020

@odeke-em Thanks for the feedback. I will set up Gerrit in the next few days and send a PR :)

@gopherbot
Copy link

Change https://golang.org/cl/281234 mentions this issue: go/ast: improve comment associations in empty function/loop/if bodies

@gopherbot
Copy link

Change https://golang.org/cl/315370 mentions this issue: go/ast: print CommentMap contents in source order

gopherbot pushed a commit that referenced this issue Apr 30, 2021
@griesemer
go/ast: print CommentMap contents in source order
0dfb6fb 
Sort the comment map entries before printing.
Makes it easier to use the output for debugging.

For #39753.

Change-Id: Ic8e7d27dd2df59173e2c3a04a6b71ae966703885
Reviewed-on: https://go-review.googlesource.com/c/go/+/315370
Trust: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
@mknyszek mknyszek modified the milestones: Go1.17, Go1.18 Aug 18, 2021
@griesemer
Copy link
Contributor

cc: @mvdan

@griesemer griesemer modified the milestones: Go1.18, Go1.19 Sep 24, 2021
@griesemer griesemer modified the milestones: Go1.19, Go1.20 Jun 23, 2022
@griesemer griesemer modified the milestones: Go1.20, Go1.21 Nov 21, 2022
@griesemer griesemer modified the milestones: Go1.21, Go1.22 Jun 1, 2023
@griesemer griesemer removed this from the Go1.22 milestone Nov 30, 2023
@griesemer griesemer added this to the Go1.23 milestone Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@griesemer griesemer

Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Release Status & Release Blockers
Status: No status
Milestone
Go1.23
Development

No branches or pull requests
6 participants
@dave @mknyszek @odeke-em @griesemer @gopherbot @ec-m