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/go/ast/astutil: AddImport removes imports #17212

Closed
rhysh opened this issue Sep 23, 2016 · 6 comments
Closed

x/tools/go/ast/astutil: AddImport removes imports #17212

rhysh opened this issue Sep 23, 2016 · 6 comments

Comments

@rhysh
Copy link
Contributor

@rhysh rhysh commented Sep 23, 2016

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

$ go version
go version go1.7.1 darwin/amd64

golang/tools@3f4088e

What operating system and processor architecture are you using (go env)?

$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/rhys/work"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/49/zmds5zsn75z1283vtzxyfr5hj7yjq4/T/go-build054967312=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"

What did you do?

I added a test case to golang.org/x/tools/go/ast/astutil's imports_test.go for adding an import of "net/http" the following file:

package main

import "bufio"
import "net/url"

What did you expect to see?

I expected the output file to include all three imports, probably in a single import block.

package main

import (
    "bufio"
    "net/http"
    "net/url"
)

What did you see instead?

package main

import "bufio"

It seems that this doesn't happen when the new package doesn't share a prefix with an existing package, or when the packages are all listed in a single block-style import.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Sep 23, 2016

You want to fix?

@rhysh
Copy link
Contributor Author

@rhysh rhysh commented Sep 23, 2016

I'll take a stab at it, though I'm not yet familiar with manipulating the AST.

Do you want to review?

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Sep 23, 2016

I'm scarred by and now scared of the AST package.

I think @griesemer likes those CLs. :-)

@griesemer
Copy link
Contributor

@griesemer griesemer commented Sep 23, 2016

@bradfitz "likes" is a strong word. I prefer "unfortunately spent too much time with"... - I'll review it, but I cannot promise a quick turn-around as I am under water at the moment with other stuff.

@rhysh
Copy link
Contributor Author

@rhysh rhysh commented Sep 24, 2016

I've got a fix (https://golang.org/cl/29688), though I may have messed up the multi-commit workflow (it requires the change for #17213). I'd be happy to do them as a single commit, if that ends up being easier.

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 24, 2016

CL https://golang.org/cl/29688 mentions this issue.

@golang golang locked and limited conversation to collaborators Oct 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.