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/internal/imports: redundant import name is not removed #33294

Open
nd opened this issue Jul 26, 2019 · 6 comments
Open

x/tools/internal/imports: redundant import name is not removed #33294

nd opened this issue Jul 26, 2019 · 6 comments

Comments

@nd
Copy link
Contributor

@nd nd commented Jul 26, 2019

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

$ go version
go version go1.12.6 linux/amd64

Does this issue reproduce with the latest release?

yes

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

go env Output
$ go env

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/nd/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/nd/go"
GOPROXY=""
GORACE=""
GOROOT="/home/nd/go/go1.12.6"
GOTMPDIR=""
GOTOOLDIR="/home/nd/go/go1.12.6/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/nd/p/golang-tools/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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build074254564=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I've ran goimports built on revision 2e34cfcb95cb3d24b197d58fe6d25046b8f25c86 on the following file:

package main

import opentracing "github.com/opentracing/opentracing-go"

func main() {
	println(opentracing.Binary)
}

What did you expect to see?

The redundant import alias is removed.

What did you see instead?

Nothing is changed.

Looks like introduced by 9bea2ecb9504b47b0689751119be8b92a371a771: the imp.name != "" check prevents redundant import cleanup.

@gopherbot gopherbot added this to the Unreleased milestone Jul 26, 2019
@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Jul 26, 2019

cc @suzmue, but I think that commit just moved around existing code.

I'm not sure goimports has ever been able to do this, so it's more of a feature request than a bug, but I think it's a reasonable one. Just to confirm: if you remove the import name manually, it stays gone?

@nd

This comment has been minimized.

Copy link
Contributor Author

@nd nd commented Jul 26, 2019

Hmm, I was under impression that goimports removed redundant aliases, but it seems I was wrogn. Yes, if I remove the alias it is not added back.

@shawnshuang

This comment was marked as off-topic.

Copy link

@shawnshuang shawnshuang commented Jul 30, 2019

I am also experiencing this. Previously, redundant names would not be added, but now they are.

How I tested this:

(1) I use VSCode with the Go extension (https://github.com/Microsoft/vscode-go, v0.11.4), which provides the ability to change the formatting tool (i.e. gofmt, goimports, goreturns).

If I have the formatting tool set to gofmt or goreturns, the redundant aliases are not added.

If I have the formatting tool set to goimports, the redundant aliases are added. The redundant aliases are even added back when I remove them.

(2) Some of our team members use VSCode and others GoLand. GoLand doesn’t have extensions, but they have file watches, for which you can set one up with goimports.

When I enable the gofmt file watcher and disable the goimports one, the redundant aliases are not added.

Likewise with VSCode, when I disable the gofmt file watcher and enable the goimports one, the redundant aliases are added, and removing them has them added back.

(3) I used goimports on a file on the command line, and doing so also adds the redundant alias.

@suzmue

This comment was marked as off-topic.

Copy link
Contributor

@suzmue suzmue commented Jul 30, 2019

@shawnshuang could you provide an example that will have redundant aliases added by goimports? Thanks!

@shawnshuang

This comment was marked as off-topic.

Copy link

@shawnshuang shawnshuang commented Jul 30, 2019

@suzmue github.com/go-ozzo/ozzo-dbx is one of the libraries that our team uses. If you:

(1) Run go get github.com/go-ozzo/ozzo-dbx
(2) Create a file with the following somewhere in your GOPATH:

package main

import (
	"fmt"

         // Intentionally leaving out "github.com/go-ozzo/ozzo-dbx"
)

func main() {
        // Does nothing, just code to access something from the dbx package
	params := dbx.Params{}

	fmt.Println(params)
}

(3) Run goimports -w path/to/the/above/file
(4) You should see that dbx "github.com/go-ozzo/ozzo-dbx" is added to the file, which is redundant since the package name is already dbx.

@suzmue

This comment was marked as off-topic.

Copy link
Contributor

@suzmue suzmue commented Jul 30, 2019

@shawnshuang Thanks for the repro!

This is intended behavior from goimports, which makes sure that an existing import with mismatched path/name has its name added (See #28428).

If you disagree with this behavior, please file a separate issue.

@gopherbot gopherbot added the Tools label Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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