x/tools/cmd/goimports: New feature, option to always use parentheses

[mcandre]

I would love an option to force goimports to always use parentheses, even for .go source files with only one import line.

By always using parentheses, the diff's between refactoring commits will be shorter. It's the same reason we use trailing commas on Go collections.

[seankhliao]

cc @heschi

[findleyr]

Related, one thing that I run into from time to time is the following asymmetry:

• if I have two imports and goimports removes one, it will preserve the parentheses
• if I have one (parenthesized!) import which goimports replaces with another, goimports removes the parentheses
• if I have no imports and goimports adds one, it chooses no parentheses

Not sure whether we want to change goimports behavior at this point, but I would prefer if goimports would choose a single canonical format for the case of one import, though I don't particularly care which format (I suppose I do mildly prefer parenthesized). This proposal would address that.

[josharian]

We made a decision some time back that goimports should standardize on always using parentheses, at all times, so that diffs are single-line. (On my phone, don't have an issue number handy.) If goimports removes parentheses or adds a parens-less import, that is a bug and should be fixed.

I'd be happy to review CLs that fix such bugs. Note that this code is particularly delicate and error-prone and has a long history of problems. CLs should include extensive, inventive tests. (There are many already to take inspiration from.)

[heschi]

@mcandre: at this point, we're not going to add new options to goimports without an extremely compelling requirement. This doesn't qualify. Anyone is welcome to make their own version that behaves the way you describe; that should be easy to do as a postprocessing step around imports.Process.

@findleyr: regarding fixing the asymmetry: I agree that the behavior seems odd at first glance. I suspect that this has to do with not destroying comments -- in general, most odd goimports formatting decisions do -- but I'm not sure without doing some investigation.

@josharian I would be very curious to see a citation for that decision if you can find it. Non-parenthesized single imports has been a tested behavior since before 2013, unless I'm missing something. And in general I'd be loath to make different versions of goimports fight over whether to parenthesize or not, and also to make people reformat their code bases to stay goimports-clean.

[heschi]

...all that said, I suppose it's safe to change the behavior of adding a single import be parenthesized if we want to, just not to have it change a single existing import.

[josharian]

I dug, and apparently the consensus I recalled was...my opinion. :P It is discussed in #18051. In any case, I agree that goimports shouldn't change any import blocks unless it needs to change the set of imports. But once we are modifying/creating a set of imports, I think we should always switch to the factored version.

[gopherbot]

Change https://golang.org/cl/355250 mentions this issue: cmd/compile: some fixes in type substituter for Instantiate