Skip to content

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

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.

Sign up for GitHub

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
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
Labels
FrozenDueToAge help wanted
Milestone
Unreleased

Comments

@rhysh
Copy link
Contributor

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

You want to fix?

@bradfitz bradfitz added this to the Unreleased milestone Sep 23, 2016
@rhysh rhysh mentioned this issue Sep 23, 2016
Closed
@rhysh
Copy link
Contributor Author

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

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

I think @griesemer likes those CLs. :-)

@griesemer
Copy link
Contributor

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

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.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@bradfitz @rhysh @griesemer @gopherbot