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

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

Closed
markusthoemmes opened this issue Dec 16, 2018 · 9 comments

Comments

@markusthoemmes
Copy link

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 bcmills added Documentation NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Dec 19, 2018
@bcmills bcmills added this to the Unreleased milestone Dec 19, 2018
@bcmills
Copy link
Contributor

bcmills commented Dec 19, 2018

CC @andybons

@markusthoemmes
Copy link
Author

Friendly poke 🙂

@bcmills bcmills modified the milestones: Unreleased, Proposal Jul 9, 2019
@andybons andybons changed the title wiki: CodeReviewComments change: Discourage duplicate imports with different names proposal: wiki: CodeReviewComments change: Discourage duplicate imports with different names Oct 7, 2019
@gopherbot gopherbot added Proposal and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Oct 7, 2019
@rsc rsc added this to Incoming in Proposals (old) Dec 4, 2019
@rsc
Copy link
Contributor

rsc commented Jan 15, 2020

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.

@rsc rsc moved this from Incoming to Active in Proposals (old) Jan 15, 2020
@magical
Copy link
Contributor

magical commented Jan 16, 2020

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
Copy link
Contributor

magical commented Jan 16, 2020

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
Copy link
Contributor

quasilyte commented Jan 17, 2020

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
Copy link
Contributor

rsc commented Jan 22, 2020

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.

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Jan 22, 2020
@ainar-g
Copy link
Contributor

ainar-g commented Jan 23, 2020

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

rsc commented Feb 5, 2020

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.)

@rsc rsc closed this as completed Feb 5, 2020
@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Feb 5, 2020
@golang golang locked and limited conversation to collaborators Feb 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

7 participants