code review 5316078: gofix: add go1pkgrename

gofix: add go1pkgrename
This will do the package import renamings for Go 1.

[r, 2011-11-03 22:20:54]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2011-11-03 22:38:14]

http://codereview.appspot.com/5316078/diff/8001/src/cmd/gofix/fix.go
File src/cmd/gofix/fix.go (right):

http://codereview.appspot.com/5316078/diff/8001/src/cmd/gofix/fix.go#newcode66
src/cmd/gofix/fix.go:66: go1renameFix,
Sync. David changed this around and this array is gone.

http://codereview.appspot.com/5316078/diff/8001/src/cmd/gofix/go1rename.go
File src/cmd/gofix/go1rename.go (right):

http://codereview.appspot.com/5316078/diff/8001/src/cmd/gofix/go1rename.go#ne...
src/cmd/gofix/go1rename.go:63: 
if !fixed {
   return false
}

Otherwise, the renaming will happen even on code
that was already using the new imports.

http://codereview.appspot.com/5316078/diff/8001/src/cmd/gofix/go1rename.go#ne...
src/cmd/gofix/go1rename.go:71: if imp.Path.Value == rename.newPath {
I don't expect this to fire.  Path.Value is a quoted string.
I think you want if importPath(imp) == rename.newPath.

The test is passing because isPkgRef does not handling
renamed imports correctly.  If an import is renamed,
the parser knows that name, so the name can be resolved.
isTopName only recognizes unresolved names.

Another way to write this would be to drop this loop
and look for n.(*ast.SelectorExpr) in the walk,
and then check isTopName(sel.X) && sel.X.(*ast.Ident).Name == rename.old. 
That's justified by something like

// This is old.Foo for an unresolved identifier old.
// Assume old refers to the package we renamed.

And then we could drop isPkgRef.  We could keep it,
but implementing it to handle renames is a little harder.

It might also be good to move this up into the loop above.
If you just rewrote the import from old to new, then
that's the best time to do the rename.  Could look
at the final element (path.Split) of the paths involved
instead of a separate table.

[r2, 2011-11-04 00:13:49]

On Nov 3, 2011, at 3:38 PM, rsc@golang.org wrote:
> 
> Another way to write this would be to drop this loop
> and look for n.(*ast.SelectorExpr) in the walk,
> and then check isTopName(sel.X) && sel.X.(*ast.Ident).Name ==
> rename.old.

you don't mean isTopName(sel.X) but I'm not sure what you do mean.

-rob

[rsc, 2011-11-04 00:17:35]

On Thu, Nov 3, 2011 at 20:13, Rob 'Commander' Pike <r@google.com> wrote:
> Another way to write this would be to drop this loop
> and look for n.(*ast.SelectorExpr) in the walk,
> and then check isTopName(sel.X) && sel.X.(*ast.Ident).Name ==
> rename.old.
>
> you don't mean isTopName(sel.X) but I'm not sure what you do mean.

sorry, looks like i meant isTopName(sel.X, rename.old)

[r2, 2011-11-04 00:19:28]

PTAL

i think i wrote what you were asking, but you had a much more complex
expression, which means either you misspoke or i misunderstood. the test passes
with this code.

i can refactor once i understand your approach.

-rob

[r2, 2011-11-04 00:20:00]

On Nov 3, 2011, at 5:17 PM, Russ Cox wrote:

> On Thu, Nov 3, 2011 at 20:13, Rob 'Commander' Pike <r@google.com> wrote:
>> Another way to write this would be to drop this loop
>> and look for n.(*ast.SelectorExpr) in the walk,
>> and then check isTopName(sel.X) && sel.X.(*ast.Ident).Name ==
>> rename.old.
>> 
>> you don't mean isTopName(sel.X) but I'm not sure what you do mean.
> 
> sorry, looks like i meant isTopName(sel.X, rename.old)

right, but that's mostly the same as the rest of that line. see my CL.

[rsc, 2011-11-04 00:23:02]

LGTM

http://codereview.appspot.com/5316078/diff/18001/src/cmd/gofix/go1rename.go
File src/cmd/gofix/go1rename.go (right):

http://codereview.appspot.com/5316078/diff/18001/src/cmd/gofix/go1rename.go#n...
src/cmd/gofix/go1rename.go:80: fixed = true
fixed is already true

[dsymonds, 2011-11-04 00:30:44]

I can't say I'm enamoured by the name, though. It seems awfully generic. What
about "go1pkgrename"?

[r2, 2011-11-04 00:31:50]

sure

[r, 2011-11-04 20:18:51]

Hello rsc@golang.org, r@google.com, dsymonds@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[r2, 2011-11-04 20:19:44]

Now called go1pkgrename, and asn1 is on the list - missed it last time.
With this version I've built a working renamed tree.

-rob

[r, 2011-11-08 23:34:29]

*** Submitted as http://code.google.com/p/go/source/detail?r=a0b71f0eb12f ***

gofix: add go1pkgrename
This will do the package import renamings for Go 1.

R=rsc, r, dsymonds
CC=golang-dev
http://codereview.appspot.com/5316078