code review 5305066: gofix: error fix

gofix: error fix

To make the error fix more useful, expand typecheck to gather
more information about struct fields, typecheck range statements,
typecheck indirect and index of named types, and collect information
about assignment conversions.

Also, change addImport to rename top-level uses of a to-be-imported
identifier to avoid conflicts.  This duplicated some of the code in 
the url fix, so that fix is now shorter.

[rsc, 2011-10-28 04:11:31]

Hello golang-dev@googlegroups.com,

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

[iant, 2011-10-30 04:39:28]

LGTM

http://codereview.appspot.com/5305066/diff/10001/src/cmd/gofix/error.go
File src/cmd/gofix/error.go (right):

http://codereview.appspot.com/5305066/diff/10001/src/cmd/gofix/error.go#newco...
src/cmd/gofix/error.go:35: Error field or method is renamed to Err.
Also any top level entity named "error" is renamed to "error_".  This should
probably also be mentioned below in the comment.

[r, 2011-10-31 19:10:35]

the DL desc is missing some information i need.

http://codereview.appspot.com/5305066/diff/10001/src/cmd/gofix/typecheck.go
File src/cmd/gofix/typecheck.go (right):

http://codereview.appspot.com/5305066/diff/10001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:132: // It returns two maps with type information:
what's all this about?

http://codereview.appspot.com/5305066/diff/10001/src/cmd/gofix/url.go
File src/cmd/gofix/url.go (left):

http://codereview.appspot.com/5305066/diff/10001/src/cmd/gofix/url.go#oldcode45
src/cmd/gofix/url.go:45: var skip interface{}
if you want to fix this test, fine, but please put it in a different CL and
explain why

http://codereview.appspot.com/5305066/diff/10001/src/cmd/gofix/url_test.go
File src/cmd/gofix/url_test.go (right):

http://codereview.appspot.com/5305066/diff/10001/src/cmd/gofix/url_test.go#ne...
src/cmd/gofix/url_test.go:106: type U struct{ url_ int }
what's this about?

[r2, 2011-10-31 19:11:37]

s/DL/CL/

[rsc, 2011-10-31 21:40:26]

http://codereview.appspot.com/5305066/diff/10001/src/cmd/gofix/typecheck.go
File src/cmd/gofix/typecheck.go (right):

http://codereview.appspot.com/5305066/diff/10001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:132: // It returns two maps with type information:
On 2011/10/31 19:10:35, r wrote:
> what's all this about?

This is the partial type checker used by gofix.  It only has
partial knowledge about the program (because import data
is not available/may be broken anyway) but it can do a good
job nonetheless.  It was written originally for the reflect fix,
but the error fix uses it too.  The error fix needs a little more
information (the assign map) and also needs more information
about types of struct fields, so I've added code below to gather that.

It also adds typechecking of range loops and index operations
and dereferences of named types.  All these helped extend the
reach of the error fix to the entire Go tree.

http://codereview.appspot.com/5305066/diff/10001/src/cmd/gofix/url.go
File src/cmd/gofix/url.go (left):

http://codereview.appspot.com/5305066/diff/10001/src/cmd/gofix/url.go#oldcode45
src/cmd/gofix/url.go:45: var skip interface{}
On 2011/10/31 19:10:35, r wrote:
> if you want to fix this test, fine, but please put it in a different CL and
> explain why

This fix was doing some work to make the top-level name "url"
available before adding the new import of "url".  That is a general
concern so I made addImport handle that and removed the
redundant code below.  The code below is still here because it 
does even more: it renames non-top-level instances of "url"
(local variables and such).

http://codereview.appspot.com/5305066/diff/10001/src/cmd/gofix/url_test.go
File src/cmd/gofix/url_test.go (right):

http://codereview.appspot.com/5305066/diff/10001/src/cmd/gofix/url_test.go#ne...
src/cmd/gofix/url_test.go:106: type U struct{ url_ int }
On 2011/10/31 19:10:35, r wrote:
> what's this about?

The fix is not smart enough to preserve 'url' here.
Well, it could preserve 'url' here but it's hard to 
preserve in composite literals, because in general
we don't know if this is a struct literal or a map 
literal, and the interpretation of "url:" changes in
the two cases.  I gave up trying to distinguish them.

The original url fix did not distinguish them either.
Gustavo added this not-renaming as a special case.
The special case is gone again.

[rsc, 2011-10-31 21:40:57]

> src/cmd/gofix/error.go:35: Error field or method is renamed to Err.
> Also any top level entity named "error" is renamed to "error_".  This should
> probably also be mentioned below in the comment.

done

[rsc, 2011-11-02 01:45:24]

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

gofix: error fix

To make the error fix more useful, expand typecheck to gather
more information about struct fields, typecheck range statements,
typecheck indirect and index of named types, and collect information
about assignment conversions.

Also, change addImport to rename top-level uses of a to-be-imported
identifier to avoid conflicts.  This duplicated some of the code in 
the url fix, so that fix is now shorter.

R=iant, r, r
CC=golang-dev
http://codereview.appspot.com/5305066