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

x/tools/astutil: strange comment placement with AddNamedImport #30724

Open
dsnet opened this Issue Mar 11, 2019 · 3 comments

Comments

Projects
None yet
3 participants
@dsnet
Copy link
Member

dsnet commented Mar 11, 2019

Using 00c44ba9c14f88ffdd4fb5bfae57fe8dd6d6afb1

Consider the following snippet:

package main

import (
	"go/parser"
	"go/printer"
	"go/token"
	"os"

	"golang.org/x/tools/go/ast/astutil"
)

const source = `package comments//

// X comment
var X int
`

func main() {
	fset := token.NewFileSet()
	file, _ := parser.ParseFile(fset, "", source, parser.ParseComments)

	astutil.AddNamedImport(fset, file, "tar", "archive/tar")
	astutil.AddNamedImport(fset, file, "zip", "archive/zip")

	(&printer.Config{Mode: printer.TabIndent | printer.UseSpaces, Tabwidth: 8}).Fprint(os.Stdout, fset, file)
}

This currently outputs:

package comments //

import (
	tar "archive/tar"
	zip "archive/zip"
) // X comment
var X int

Notice how the next comment group X comment is attached to the import block? It seems that this only occurs if an inline comment occurs on the package statement.

I expect this to output:

package comments //

import (
	tar "archive/tar"
	zip "archive/zip"
)

// X comment
var X int

@gopherbot gopherbot added this to the Unreleased milestone Mar 11, 2019

@dsnet dsnet changed the title x/tools/astutil: strange comment placement x/tools/astutil: strange comment placement with AddNamedImport Mar 11, 2019

@dsnet

This comment has been minimized.

Copy link
Member Author

dsnet commented Mar 11, 2019

This seems to be a regression introduced by CL/122736 to address #26290. That CL always adds 2 to the end of the last comment position, which I do not think is quite correct.

\cc @Gnouc

@dsnet

This comment has been minimized.

Copy link
Member Author

dsnet commented Mar 11, 2019

I think CL/122736 should possibly be reverted and that this issue should be fixed in gofmt. See #30741 as this problem isn't really specific to astutil.AddNamedImport.

@Gnouc

This comment has been minimized.

Copy link
Contributor

Gnouc commented Mar 11, 2019

@dsnet SGTM 👍

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