proposal: wiki: CodeReviewComments change: Discourage duplicate imports with different names

[markusthoemmes]

I've seen it happen a couple of times (especially in Kubernetes related projects with lots of naming collisions) that the same packages have been imported multiple times with different aliases. My gut feeling was, that golint should warn me about this, so I can take the appropriate steps without having to manually check for that condition.

Turns out there was a proposed PR to do that, but it's been closed because it was out of scope. The CodeReviewComments indeed do not contain a mention of this condition. Should we add it?

I feel like a simple "Do not import the same package twice under a different alias." or even a softer "Avoid importing the same package under a different alias." would be reasonable and thus pulling such a change to golint in scope.

[bcmills]

CC @andybons

[markusthoemmes]

Friendly poke 🙂

[rsc]

In general, code should strive to be consistent. Having multiple names for something is in general discouraged. If you import and rename the same package as xpb and ypb, that's a bit odd.

But there may be cases where it does make sense to import a package once under its normal name and once under an alias. For example, net/http/request.go imports "net/url" as itself (url) but also as urlpkg. There is some code with variables named url that can still access urlpkg. But we still have the original name available for use in exported type definitions, to avoid having godoc that says "urlpkg.URL".

The situation seems nuanced enough that maybe it's not worth a CodeReviewComments change. Certainly we wouldn't want a golint that rejected net/http/request.go.

[magical]

This can also come up in generated code. For example, goyacc adds import __yyfmt__ "fmt" to the generated code. The alias is there specifically so that user code can still import "fmt" without getting an error.

This seems like a valid use case for duplicate imports.

[magical]

For example, net/http/request.go imports "net/url" as itself (url) but also as urlpkg [...] to avoid having godoc that says "urlpkg.URL".

If godoc generates confusing (or ugly) documentation in the presence of aliased imports, that seems like a problem with godoc, not the code. A user reading the documentation for a package should not have to dive into the code to figure out which package an identifier refers to. The author of the package should not have to go through contortions to render the documentation readable.

[quasilyte]

Note that go-critic dupImport diagnostic can catch this. (revive also does it via duplicated-imports check.)

If you're using golangci-lint, you can use it from that.

[rsc]

It seems like there is enough disagreement here that we should not be attempting to amend this wiki page, which too many people treat as "the law". There is also a good role for code reviewers here, to make sure changes match local conventions and are readable in general.

Based on the discussion above and the lack of consensus, this seems like a likely decline.

[ainar-g]

@rsc I've had this “fun” bug recently. I intended to write:

import (
	htemplate "html/template"
	ttemplate "text/template"
)

But for some reason (goimports?) wrote:

import (
	htemplate "text/template"
	ttemplate "text/template"
)

Which in turn meant that the HTML templates in my programme didn't receive the escaping it should have received. If there was a vet check against double imports, this wouldn't have happened. My personal vote is for this proposal, and for inclusion of a linter for this into go vet.

[rsc]

No change in consensus, so declined.

(If goimports really did change renamed imports that you'd written by hand, that's a bug and please file that separately. But I don't think it does that.)