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

cmd/gofmt: remove duplicate imports #4414

Closed
Sajmani opened this issue Nov 20, 2012 · 21 comments
Closed

cmd/gofmt: remove duplicate imports #4414

Sajmani opened this issue Nov 20, 2012 · 21 comments

Comments

@Sajmani
Copy link
Contributor

@Sajmani Sajmani commented Nov 20, 2012

What steps will reproduce the problem?
1. When moving code from file A to file B, I copy file A's imports as well.
2. Run gofmt, which sorts the imports.
3. Compile the code, which detects unused imports, which I can remove.
4. I still need to remove duplicate unnamed imports manually.

What is the expected output? What do you see instead?
gofmt could safely remove the duplicate unnamed imports.

Please use labels and text to provide additional information.
@rsc
Copy link
Contributor

@rsc rsc commented Dec 9, 2012

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

@rsc
Copy link
Contributor

@rsc rsc commented Dec 10, 2012

Comment 2:

Labels changed: added size-m.

@rsc
Copy link
Contributor

@rsc rsc commented Dec 10, 2012

Comment 3:

Labels changed: added suggested.

@tebeka
Copy link
Contributor

@tebeka tebeka commented Jan 9, 2013

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

@davecheney davecheney commented Feb 17, 2013

Comment 5:

Miki, are you still interested in working on this issue ?
@tebeka
Copy link
Contributor

@tebeka tebeka commented Feb 17, 2013

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

@tebeka tebeka commented Feb 23, 2013

Comment 7:

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

@gopherbot gopherbot commented Feb 24, 2013

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

@tebeka tebeka commented Feb 24, 2013

Comment 9:

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

@rsc rsc commented Feb 25, 2013

Comment 10:

both comments that is, not both imports
@rsc
Copy link
Contributor

@rsc rsc commented Feb 25, 2013

Comment 11:

I think you should preserve both, possibly on separate lines.
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 7, 2013

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

@rsc rsc commented Mar 11, 2013

Comment 13:

Labels changed: removed go1.1.

@rsc
Copy link
Contributor

@rsc rsc commented Mar 11, 2013

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

@rsc rsc commented Jul 30, 2013

Comment 15:

Labels changed: added go1.2maybe.

@rsc
Copy link
Contributor

@rsc rsc commented Jul 30, 2013

Comment 16:

Labels changed: added feature.

@josharian
Copy link
Contributor

@josharian josharian commented Aug 6, 2013

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

@rsc rsc commented Aug 13, 2013

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

@josharian josharian commented Aug 14, 2013

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

@robpike robpike commented Aug 30, 2013

Comment 20:

Status changed to Started.

@rsc
Copy link
Contributor

@rsc rsc commented Sep 6, 2013

Comment 21:

This issue was closed by revision 08925ce.

Status changed to Fixed.

@rsc rsc added this to the Go1.2 milestone Apr 14, 2015
@rsc rsc removed the go1.2maybe label Apr 14, 2015
@golang golang locked and limited conversation to collaborators Jun 24, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.