Comment 1:

(Two imports are duplicated only if both the path and the import symbol are the same.)

Labels changed: added priority-later, removed priority-triage.

Owner changed to ---.

Comment 2:

Labels changed: added size-m.

Comment 3:

Labels changed: added suggested.

Comment 4:

I started working on this (patch attached). The tests are failing, once I figure out the
tests I'll post a CR.

Attachments:

1. 4414.patch (1880 bytes)

Comment 5:

Miki, are you still interested in working on this issue ?

Comment 6:

Yeah I still am. It's been a busy month at work but hopefully next week will be calmer.
I *think* I have the code right, but I'm missing something about the way the test suite
is working. Need to dig deeper.

Comment 7:

Dave, I don't seem to find the time. Feel free to use my patch :)

Comment 8 by jonathan.lukens:

I'd like to pick this one up. One question: what is the desired outcome if two duplicate
import path+symbols have unique comments?

Comment 9:

Good question, IMO pick one :) Maybe @rcs has a better answer.

Comment 10:

both comments that is, not both imports

Comment 11:

I think you should preserve both, possibly on separate lines.

Comment 12:

I modified ast/import.go so that it removes duplicate ImportSpecs from the syntax tree
(see attached patch).
However, removing ImportSpecs can create holes (empty lines) in the file. If the input
consists from 1 run of ImportSpecs then there can be 2 or more runs on the output. It
appears this makes it impossible for:
  cd $GOROOT/src/cmd/gofmt
  go test -v -run=".*Rewrite" .
to succeed because of an incosistency between "import.golden", "import.golden.gofmt" and
"import.input.gofmt". It seems impossible to overcome this limitation without changing
more code either in gofmt or in go/ast.
The patch puts the logic into go/ast/import.go, but the same problem would arise if the
logic is put into cmd/gofmt/gofmt.go.
Maybe we should simply ignore the possibility of such inconsistencies, and devise a test
case that succeeds (while knowing that a more complicated test case would make gofmt
fail).
So the question is whether it acceptable to put a simpler (although incomplete) test in
src/cmd/gofmt/import.input.

Attachments:

1. 4414-atom.patch (2280 bytes)

Comment 13:

Labels changed: removed go1.1.

Comment 14:

Here is yet another attempt: https://golang.org/cl/7231070/.
I abandoned it but I don't remember why. I think I decided it wasn't good
enough.
I think this might have to wait until after Go 1.1. There are too many
corner cases.
Russ

Comment 15:

Labels changed: added go1.2maybe.

Comment 16:

Labels changed: added feature.

Comment 17:

I was thinking about taking a stab at this. I have a proposal; feedback would be most
welcome.
(0) Following @rsc's patch, do the following per set of consecutive imports.
(1) Sort by import path, then import name, then comment. (Currently, gofmt sorts only by
import path.)
(2) For imports A, B, if they have the same import path and import name, and A's comment
is a prefix of B's comment, then remove import A. This handles: (a) exact duplicates,
(b) B has a comment and A doesn't, and (c) A's comment has had extra info added to it.
For example:
http://play.golang.org/p/1tVBooLtPy
would become:
http://play.golang.org/p/2NV45hJpPY
Note that no attempt is made to combine significantly different comments. Reasoning:
* I believe that this case will be uncommon (admittedly based only on anecdotal
evidence).
* I was unable to find a proposal that looked obviously right to me, and as @rsc points
out, there are lots of corner cases. Under those circumstances, requiring human
intervention seems reasonable.
* Such combining could yet be added in the future if the right approach became clear.

Comment 18:

This sounds good. I would strengthen the constraint even further: if one of
A or B has no comment, delete that one and leave the other; otherwise leave
both.

Comment 19:

Ok, I took a first crack at this, following @rsc's suggestion (thanks!); see
https://golang.org/cl/12837044.
It's not quite done -- there's one test failure still, a pesky newline that I haven't
figured out how to safely dispose of. And I don't feel a lot of confidence yet in my
changes; I've learned the hard way that this stuff is delicate business. I thought I'd
go ahead and share now, though, in case anyone is inspired to provide hints or early
feedback or wanted to give it a try locally.

Comment 20:

Status changed to Started.

Comment 21:

This issue was closed by revision 08925ce.

Status changed to Fixed.