golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.64k stars 17.61k forks source link

x/tools/cmd/bundle: does not recognize duplicate imports due to import aliases #37689

Open meling opened 4 years ago

meling commented 4 years ago

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

$ go version
go version go1.14 darwin/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
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/meling/Library/Caches/go-build"
GOENV="/Users/meling/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/meling/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.14/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.14/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/meling/work/gorumsv2/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/qf/stfl1sbn3p9939ylh7v1fm680000gn/T/go-build400092728=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Ran bundle (commit) on a package that contains both protobuf generated code and non-generated code. The generated code contains imports of the form:

    trace "golang.org/x/net/trace"
    grpc "google.golang.org/grpc"
    codes "google.golang.org/grpc/codes"
    status "google.golang.org/grpc/status"
    protoreflect "google.golang.org/protobuf/reflect/protoreflect"
    protoimpl "google.golang.org/protobuf/runtime/protoimpl"

Whereas the non-generated code have imports of the form:

    "golang.org/x/net/trace"
    "google.golang.org/grpc"

What did you expect to see?

I would expect the imports to be rewritten as follows:

    "golang.org/x/net/trace"
    "google.golang.org/grpc"

Or, alternatively:

    trace "golang.org/x/net/trace"
    grpc "google.golang.org/grpc"

What did you see instead?

Duplicate imports that cause compile error.

    "golang.org/x/net/trace"
    trace "golang.org/x/net/trace"
    "google.golang.org/grpc"
    grpc "google.golang.org/grpc"

I have reported a companion issue here.

dmitshur commented 4 years ago

bundle can handle this edge-case better by deduplicating identical imports.

When a given import path is loaded twice:

import (
    "same/import/path"
    a "same/import/path"
)

It would need to load the package with import path "same/import/path" and find out the package name. If the name the same, then one of those imports can be safely dropped.

Note that the package name is not always equal to the last path element of its import path. For example:

import (
    "github.com/micro/go-micro"
    micro "github.com/micro/go-micro"
)

Those two imports are identical despite "micro" not being the same as the last import path element, and should still be deduplicated.

However, it would be invalid to deduplicate two imports when the package name is different from the renamed one.

meling commented 4 years ago

I have a patch for this. What would be the best way to contribute this? I'm building on top of an existing CL. @dmitshur

dmitshur commented 4 years ago

@meling The best way would be to send a GitHub PR or Gerrit CL. Take a look at https://golang.org/doc/contribute.html#sending_a_change_github and https://golang.org/doc/contribute.html#sending_a_change_gerrit where the two options are described.

Building on top of an existing CL should be more achievable via Gerrit. You can just rebase your commit on top of the commit corresponding to the existing CL, and git codereview mail should work to create a new CL corresponding to your own commit. (Although I haven't tried to do this using someone else's existing CL, so I can't guarantee it'll work.)

gopherbot commented 4 years ago

Change https://golang.org/cl/222997 mentions this issue: cmd/bundle: de-duplicating identical imports