code review 4285053: gofix: add -diff, various fixes and helpers

gofix: add -diff, various fixes and helpers

  * add -diff command line option
  * use scoping information in refersTo, isPkgDot, isPtrPkgDot.
  * add new scoping-based helpers countUses, rewriteUses, assignsTo, isTopName.
  * rename rewrite to walk, add walkBeforeAfter.
  * add toy typechecker, a placeholder for go/types

[rsc, 2011-04-04 20:21:25]

Hello gri (cc: golang-dev@googlegroups.com),

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

[gri, 2011-04-04 22:18:38]

I still need to read the typecheck.go file in detail. Some initial feedback.

Some sections could use a few more comments.

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

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/fix.go#newcode33
src/cmd/gofix/fix.go:33: // rewrite walks the AST x, calling visit(y) for each
node y in the tree but
s/rewrite/walk/

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/fix.go#newcode258
src/cmd/gofix/fix.go:258: for _, decl := range f.Decls {
for _, spec := range f.Imports {
    // TODO(rsc): better handling of quoting
    if string(s.Path.Value) == `"`+path+`"` {
        return true
    }
}
return false

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/fix.go#newcode284
src/cmd/gofix/fix.go:284: return isTopName(sel.X, pkg) && sel.Sel.String() ==
name
return ok && ...

and you don't need the if before

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/fix.go#newcode294
src/cmd/gofix/fix.go:294: return isPkgDot(ptr.X, pkg, name)
return ok && ...

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/fix.go#newcode297
src/cmd/gofix/fix.go:297: // isTopName returns true if n is a top-level
(unresolved) identifier with the given name.
s/(unresolved)/unresolved/

seems important

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/fix.go#newcode303
src/cmd/gofix/fix.go:303: return id.Name == name && id.Obj == nil
return ok && id.Name == name && id.Obj == nil

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/fix.go#newcode312
src/cmd/gofix/fix.go:312: return id.String() == name
return ok && ...

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/fix.go#newcode327
src/cmd/gofix/fix.go:327: return id.Obj == x.Obj && id.Name == x.Name
return ok && id.Obj == x.Obj

(If they refer to the same object, they must have the same name)

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/fix.go#newcode345
src/cmd/gofix/fix.go:345: return s == `""` || s == "``"
return ok && lit.Kind == token.STRING && len(lit.Value) == 2

and get rid of the rest

(note also that lit.Value is now of type string - I wish T(x) would be an error
if the type of x is already a T)

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/fix.go#newcode371
src/cmd/gofix/fix.go:371: // rewriteUses replaces all uses of the identifier x
with f(x.Pos()) in scope.
comment doesn't mention fnot

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/fix.go#newcode400
src/cmd/gofix/fix.go:400: return nil
just:

id, _ := n.(*ast.Ident)
return id

(id should be the zero value for *ast.Ident (nil), if the assertion fails)

Also: perhaps this function should be moved up and grouped with all the other
isX predicates?

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/fix.go#newcode410
src/cmd/gofix/fix.go:410: if u, ok := n.(*ast.UnaryExpr); ok && u.Op ==
token.AND && refersTo(u.X, x) {
example for this case?

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/main.go
File src/cmd/gofix/main.go (right):

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/main.go#newcode99
src/cmd/gofix/main.go:99: var fixedlist bytes.Buffer
s/fixedlist/log/ ?

fixedlist seems not a very good name

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/main.go#newcode122
src/cmd/gofix/main.go:122: newFile := file
could use a section comment here:

// apply all fixes to file

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/main.go#newcode168
src/cmd/gofix/main.go:168: return fmt.Errorf("computing dfif: %s", err)
s/dfif/diff/

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/main.go#newcode170
src/cmd/gofix/main.go:170: fmt.Printf("diff %s fixed/%s\n", filename, filename)
filename, filename?

I don't understand this print

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/main.go#newcode185
src/cmd/gofix/main.go:185: func gofmt(n interface{}) string {
I guess this is used elsewhere? (not in this file)

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

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/typecheck.go#ne...
src/cmd/gofix/typecheck.go:47: func (cfg *TypeConfig) typeof(name string) string
{
comment would make this easier to read

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/typecheck.go#ne...
src/cmd/gofix/typecheck.go:70: func (typ *Type) dot(cfg *TypeConfig, name
string) string {
comment please

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/typecheck.go#ne...
src/cmd/gofix/typecheck.go:94: func typecheck(cfg *TypeConfig, f *ast.File)
map[interface{}]string {
comment please (what is the result?)

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/typecheck.go#ne...
src/cmd/gofix/typecheck.go:119: //println(rcvr+"."+fn.Name.Name, t)
should these prints remain?

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/typecheck.go#ne...
src/cmd/gofix/typecheck.go:121: //println(gofmt(fn.Name), t)
same here?

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/typecheck.go#ne...
src/cmd/gofix/typecheck.go:136: func namesToExprs(a []*ast.Ident) []ast.Expr {
s/namesToExprs/makeExprList/ ?

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/typecheck.go#ne...
src/cmd/gofix/typecheck.go:397: // type information.
Roger Peppe remarked on this, too. I'm wondering if I should introduce an object
per case to bind a type.

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/typecheck.go#ne...
src/cmd/gofix/typecheck.go:407: for _, cas := range n.Body.List {
s/cas/clause/?

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/typecheck.go#ne...
src/cmd/gofix/typecheck.go:438: // Convert between function strings and lists of
types.
s/function strings/function type strings/ ?

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/typecheck.go#ne...
src/cmd/gofix/typecheck.go:448: i := 5
5?

i := len("func ") I assume? There should be at least a comment

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/typecheck.go#ne...
src/cmd/gofix/typecheck.go:459: if len(out) >= 2 && out[0] == '(' &&
out[len(out)-1] == ')' {
may function result types should always be in ()'s - simpler internal invariant
and then one could use one function that just splits (...) type strings

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/typecheck.go#ne...
src/cmd/gofix/typecheck.go:481: i := 0
comment for i would make this easier to read

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/typecheck.go#ne...
src/cmd/gofix/typecheck.go:493: if nparen < 0 {
shouldn't this be an internal error? (can this happen, and if so, why?) -
deserves comment

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/typecheck.go#ne...
src/cmd/gofix/typecheck.go:506: return nil
can this happen?

[rsc, 2011-04-05 05:07:26]

Hello gri (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-04-05 05:07:50]

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

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/fix.go#newcode33
src/cmd/gofix/fix.go:33: // rewrite walks the AST x, calling visit(y) for each
node y in the tree but
On 2011/04/04 22:18:38, gri wrote:
> s/rewrite/walk/

Done.

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/fix.go#newcode258
src/cmd/gofix/fix.go:258: for _, decl := range f.Decls {
On 2011/04/04 22:18:38, gri wrote:
> for _, spec := range f.Imports {
>     // TODO(rsc): better handling of quoting
>     if string(s.Path.Value) == `"`+path+`"` {
>         return true
>     }
> }
> return false

Done.

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/fix.go#newcode284
src/cmd/gofix/fix.go:284: return isTopName(sel.X, pkg) && sel.Sel.String() ==
name
On 2011/04/04 22:18:38, gri wrote:
> return ok && ...
> 
> and you don't need the if before

Done.

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/fix.go#newcode294
src/cmd/gofix/fix.go:294: return isPkgDot(ptr.X, pkg, name)
On 2011/04/04 22:18:38, gri wrote:
> return ok && ...

Done.

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/fix.go#newcode297
src/cmd/gofix/fix.go:297: // isTopName returns true if n is a top-level
(unresolved) identifier with the given name.
On 2011/04/04 22:18:38, gri wrote:
> s/(unresolved)/unresolved/
> 
> seems important

Done.

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/fix.go#newcode303
src/cmd/gofix/fix.go:303: return id.Name == name && id.Obj == nil
On 2011/04/04 22:18:38, gri wrote:
> return ok && id.Name == name && id.Obj == nil

Done.

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/fix.go#newcode312
src/cmd/gofix/fix.go:312: return id.String() == name
On 2011/04/04 22:18:38, gri wrote:
> return ok && ...

Done.

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/fix.go#newcode327
src/cmd/gofix/fix.go:327: return id.Obj == x.Obj && id.Name == x.Name
On 2011/04/04 22:18:38, gri wrote:
> return ok && id.Obj == x.Obj
> 
> (If they refer to the same object, they must have the same name)

Added comment.  The test of Name makes refersTo
work even for the unresolved identifiers.

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/fix.go#newcode345
src/cmd/gofix/fix.go:345: return s == `""` || s == "``"
On 2011/04/04 22:18:38, gri wrote:
> return ok && lit.Kind == token.STRING && len(lit.Value) == 2
> 
> and get rid of the rest
> 
> (note also that lit.Value is now of type string - I wish T(x) would be an
error
> if the type of x is already a T)

Done.

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/fix.go#newcode371
src/cmd/gofix/fix.go:371: // rewriteUses replaces all uses of the identifier x
with f(x.Pos()) in scope.
On 2011/04/04 22:18:38, gri wrote:
> comment doesn't mention fnot

Done.

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/fix.go#newcode400
src/cmd/gofix/fix.go:400: return nil
On 2011/04/04 22:18:38, gri wrote:
> just:
> 
> id, _ := n.(*ast.Ident)
> return id
> 
> (id should be the zero value for *ast.Ident (nil), if the assertion fails)
> 
> Also: perhaps this function should be moved up and grouped with all the other
> isX predicates?

Done.

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/fix.go#newcode410
src/cmd/gofix/fix.go:410: if u, ok := n.(*ast.UnaryExpr); ok && u.Op ==
token.AND && refersTo(u.X, x) {
On 2011/04/04 22:18:38, gri wrote:
> example for this case?

Done.

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/main.go
File src/cmd/gofix/main.go (right):

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/main.go#newcode99
src/cmd/gofix/main.go:99: var fixedlist bytes.Buffer
On 2011/04/04 22:18:38, gri wrote:
> s/fixedlist/log/ ?
> 
> fixedlist seems not a very good name

Done.

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/main.go#newcode122
src/cmd/gofix/main.go:122: newFile := file
On 2011/04/04 22:18:38, gri wrote:
> could use a section comment here:
> 
> // apply all fixes to file

Done.

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/main.go#newcode168
src/cmd/gofix/main.go:168: return fmt.Errorf("computing dfif: %s", err)
On 2011/04/04 22:18:38, gri wrote:
> s/dfif/diff/

Done.

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/main.go#newcode170
src/cmd/gofix/main.go:170: fmt.Printf("diff %s fixed/%s\n", filename, filename)
On 2011/04/04 22:18:38, gri wrote:
> filename, filename?
> 
> I don't understand this print

It makes the output say 

diff x.go fixed/x.go
[output of diff command]

It makes clear what the left and right side of
the diff are.

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/main.go#newcode185
src/cmd/gofix/main.go:185: func gofmt(n interface{}) string {
On 2011/04/04 22:18:38, gri wrote:
> I guess this is used elsewhere? (not in this file)

Yes, it gets used in debugging prints but also
in the type checker.

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

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/typecheck.go#ne...
src/cmd/gofix/typecheck.go:47: func (cfg *TypeConfig) typeof(name string) string
{
On 2011/04/04 22:18:38, gri wrote:
> comment would make this easier to read

Done.

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/typecheck.go#ne...
src/cmd/gofix/typecheck.go:94: func typecheck(cfg *TypeConfig, f *ast.File)
map[interface{}]string {
On 2011/04/04 22:18:38, gri wrote:
> comment please (what is the result?)

Done.

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/typecheck.go#ne...
src/cmd/gofix/typecheck.go:119: //println(rcvr+"."+fn.Name.Name, t)
On 2011/04/04 22:18:38, gri wrote:
> should these prints remain?

Done.

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/typecheck.go#ne...
src/cmd/gofix/typecheck.go:121: //println(gofmt(fn.Name), t)
On 2011/04/04 22:18:38, gri wrote:
> same here?

Done.

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/typecheck.go#ne...
src/cmd/gofix/typecheck.go:136: func namesToExprs(a []*ast.Ident) []ast.Expr {
On 2011/04/04 22:18:38, gri wrote:
> s/namesToExprs/makeExprList/ ?

Done.

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/typecheck.go#ne...
src/cmd/gofix/typecheck.go:407: for _, cas := range n.Body.List {
On 2011/04/04 22:18:38, gri wrote:
> s/cas/clause/?

Left as is.  For me it's more important that it is
a case.  The word "clause" has no meaning to me.

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/typecheck.go#ne...
src/cmd/gofix/typecheck.go:438: // Convert between function strings and lists of
types.
On 2011/04/04 22:18:38, gri wrote:
> s/function strings/function type strings/ ?

Done.

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/typecheck.go#ne...
src/cmd/gofix/typecheck.go:448: i := 5
On 2011/04/04 22:18:38, gri wrote:
> 5?
> 
> i := len("func ") I assume? There should be at least a comment

Done.

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/typecheck.go#ne...
src/cmd/gofix/typecheck.go:459: if len(out) >= 2 && out[0] == '(' &&
out[len(out)-1] == ')' {
On 2011/04/04 22:18:38, gri wrote:
> may function result types should always be in ()'s - simpler internal
invariant
> and then one could use one function that just splits (...) type strings

I didn't do this only because when the type checker
gets confused it falls back to using gofmt(t) as
the type, and the gofmt form has ( ).  It's not too 
hard to handle here, and of course all this should
go away eventually.

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/typecheck.go#ne...
src/cmd/gofix/typecheck.go:481: i := 0
On 2011/04/04 22:18:38, gri wrote:
> comment for i would make this easier to read

Done.

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/typecheck.go#ne...
src/cmd/gofix/typecheck.go:493: if nparen < 0 {
On 2011/04/04 22:18:38, gri wrote:
> shouldn't this be an internal error? (can this happen, and if so, why?) -
> deserves comment

Commented.  I try not to panic in this code, 
because it's all so approximate, and because
if you run gofix on a big directory tree it's
kind of annoying if it panics halfway through.

http://codereview.appspot.com/4285053/diff/7001/src/cmd/gofix/typecheck.go#ne...
src/cmd/gofix/typecheck.go:506: return nil
On 2011/04/04 22:18:38, gri wrote:
> can this happen?

Probably not.  Commented.

[gri, 2011-04-06 06:07:06]

some more comments. I think this looks pretty good but I still need to wrap my
head around the type checking code. final comments tomorrow.

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

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/fix.go#newcode361
src/cmd/gofix/fix.go:361: if ok && not.Op == token.NOT && not.X == lastF {
This only works because we have bottom-up traversal in walk, i.e., the children
are visited first, correct? Could use a comment here, otherwise this is pretty
hard to understand.

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/fix.go#newcode383
src/cmd/gofix/fix.go:383: if u, ok := n.(*ast.UnaryExpr); ok && u.Op ==
token.AND && refersTo(u.X, x) {
a type switch might make this shorter

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

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:452: i := len("func(") // index of beginning of in
type
s/of in/of parameter list in/

[gri, 2011-04-06 21:41:44]

Overall: Please more comments - there's a lot of implicit assumptions that are
not obvious. Also, there's some code that could be factored, especially with the
type string representation (which seems rather fragile as it is, and even
dependent on gofmt).

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

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:41: type TypeConfig struct {
Somewhere it would be extremely useful to have a description of the typical type
string, together with examples for each possible type. With an explanation of
the "type " prefix (which sometimes is there and sometimes doesn't appear to be
there). As it is, one has to guess from the code. Seems like a major invariant
worth documenting. (it's there partially in the code, e.g. for Fields and
FieldLists, but not in general).

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:113: if !strings.HasPrefix(rcvr, "type ") {
comments please

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:114: if fn.Recv == nil || len(fn.Recv.List) != 1 {
fn.Recv is not nil above I thought

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:126: if strings.HasPrefix(t, "type ") {
a comment on the "type " prefix would be useful - perhaps not herebut somewhere
global?

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:131: typeof[fn.Name.Obj] = t
comment why t is entered for both the Name and the Obj

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:148: func typecheck1(cfg *TypeConfig, f interface{},
typeof map[interface{}]string) {
function comment please
(I guess it's like typecheck but it takes the typeof map and augments is)

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:149: var curfn []*ast.FuncType
comment on the purpose of curfn please

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:151: set := func(n ast.Expr, typ string, isDecl bool)
{
function comment please

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:175: for i := 0; i < len(lhs) && i < len(t); i++ {
comment please that in correct programs len(lhs) == len(t) - extra tests only
for fault tolerance

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:199: 
A general high-level comment giving a general outline of the algorithm would
make this easier to read as the actual walkBeforeAfter is at the very end.

Plus function comments for before and after please.

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:217: v := fset.Position(n.(ast.Node).Pos())
s/v/pos/

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:229: typeof[n] = "type " +
joinFunc(split(typeof[n.Params]), split(typeof[n.Results]))
there should be a utility function doing the "type " prepending - it's all over
the place

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:271: if !strings.HasPrefix(t, "type ") {
This operation happens in other places as well. Should be factored.

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:275: t = t[len("type "):]
including this.

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:353: if t := typeof[n.Type]; strings.HasPrefix(t,
"type ") {
it may make sense to have

type typestr string

and methods on typestr that deal with the "type " prefix, etc.

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:374: // We don't use the result when *x is a type. 
XXX
XXX?

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:413: if len(cas.List) == 1 {
comment about len = 0, len > 1

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:422: typeof[varx] = t
restore t?

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:427: return
can this happen? if not, comment please

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:473: func joinFunc(in, out []string) string {
function comment please

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:483: func split(s string) []string {
function comment please

[rsc, 2011-04-06 22:11:12]

PTAL

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

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/fix.go#newcode361
src/cmd/gofix/fix.go:361: if ok && not.Op == token.NOT && not.X == lastF {
On 2011/04/06 06:07:06, gri wrote:
> This only works because we have bottom-up traversal in walk, i.e., the
children
> are visited first, correct? Could use a comment here, otherwise this is pretty
> hard to understand.

Done.

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/fix.go#newcode383
src/cmd/gofix/fix.go:383: if u, ok := n.(*ast.UnaryExpr); ok && u.Op ==
token.AND && refersTo(u.X, x) {
On 2011/04/06 06:07:06, gri wrote:
> a type switch might make this shorter 

Done.

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

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:113: if !strings.HasPrefix(rcvr, "type ") {
On 2011/04/06 21:41:44, gri wrote:
> comments please

Done.

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:114: if fn.Recv == nil || len(fn.Recv.List) != 1 {
On 2011/04/06 21:41:44, gri wrote:
> fn.Recv is not nil above I thought

Done.

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:126: if strings.HasPrefix(t, "type ") {
On 2011/04/06 21:41:44, gri wrote:
> a comment on the "type " prefix would be useful - perhaps not herebut
somewhere
> global?

Done.

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:131: typeof[fn.Name.Obj] = t
On 2011/04/06 21:41:44, gri wrote:
> comment why t is entered for both the Name and the Obj

Done.

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:199: 
On 2011/04/06 21:41:44, gri wrote:
> A general high-level comment giving a general outline of the algorithm would
> make this easier to read as the actual walkBeforeAfter is at the very end.
> 
> Plus function comments for before and after please.

Done.

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:217: v := fset.Position(n.(ast.Node).Pos())
On 2011/04/06 21:41:44, gri wrote:
> s/v/pos/

Done.

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:229: typeof[n] = "type " +
joinFunc(split(typeof[n.Params]), split(typeof[n.Results]))
On 2011/04/06 21:41:44, gri wrote:
> there should be a utility function doing the "type " prepending - it's all
over
> the place

Done.

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:271: if !strings.HasPrefix(t, "type ") {
On 2011/04/06 21:41:44, gri wrote:
> This operation happens in other places as well. Should be factored.

Done.

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:275: t = t[len("type "):]
On 2011/04/06 21:41:44, gri wrote:
> including this.

Done.

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:353: if t := typeof[n.Type]; strings.HasPrefix(t,
"type ") {
On 2011/04/06 21:41:44, gri wrote:
> it may make sense to have
> 
> type typestr string
> 
> and methods on typestr that deal with the "type " prefix, etc.

This doesn't work unless I make the map be map[interface{}]interface{}
and use type switches everywhere.  I'd prefer not to.
But I did introduce the helper functions.

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:374: // We don't use the result when *x is a type. 
XXX
On 2011/04/06 21:41:44, gri wrote:
> XXX?

Done.

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:413: if len(cas.List) == 1 {
On 2011/04/06 21:41:44, gri wrote:
> comment about len = 0, len > 1

Done.

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:422: typeof[varx] = t
On 2011/04/06 21:41:44, gri wrote:
> restore t?

Done.

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:427: return
On 2011/04/06 21:41:44, gri wrote:
> can this happen? if not, comment please

Probably not.  Done.

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:452: i := len("func(") // index of beginning of in
type
On 2011/04/06 06:07:06, gri wrote:
> s/of in/of parameter list in/

Changed to
 // index of beginning of 'in' arguments

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:473: func joinFunc(in, out []string) string {
On 2011/04/06 21:41:44, gri wrote:
> function comment please

Done.

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:483: func split(s string) []string {
On 2011/04/06 21:41:44, gri wrote:
> function comment please

Done.

[gri, 2011-04-06 22:26:09]

LGTM

but please address the few outstanding issues

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

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:149: var curfn []*ast.FuncType
On 2011/04/06 21:41:44, gri wrote:
> comment on the purpose of curfn please

still no comment

http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:151: set := func(n ast.Expr, typ string, isDecl bool)
{
On 2011/04/06 21:41:44, gri wrote:
> function comment please

same here

http://codereview.appspot.com/4285053/diff/32001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:44: Func map[string]string
ah! much better.

http://codereview.appspot.com/4285053/diff/32001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:146: }
use isType here

http://codereview.appspot.com/4285053/diff/32001/src/cmd/gofix/typecheck.go#n...
src/cmd/gofix/typecheck.go:159: // The !isDecl case is a cheat here, but it
makes
use isType/getType

[rsc, 2011-04-06 22:47:35]

>
http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
> src/cmd/gofix/typecheck.go:149: var curfn []*ast.FuncType
> On 2011/04/06 21:41:44, gri wrote:
>>
>> comment on the purpose of curfn please

moved curfn down to be near the comment
i added in the last round.

>
http://codereview.appspot.com/4285053/diff/18001/src/cmd/gofix/typecheck.go#n...
> src/cmd/gofix/typecheck.go:151: set := func(n ast.Expr, typ string,
> isDecl bool) {
> On 2011/04/06 21:41:44, gri wrote:
>>
>> function comment please

done, sorry.

>
http://codereview.appspot.com/4285053/diff/32001/src/cmd/gofix/typecheck.go#n...
> src/cmd/gofix/typecheck.go:146: }
> use isType here

done

>
http://codereview.appspot.com/4285053/diff/32001/src/cmd/gofix/typecheck.go#n...
> src/cmd/gofix/typecheck.go:159: // The !isDecl case is a cheat here, but
> it makes
> use isType/getType

done

will wait to submit until 4343047 is ready,
just in case they depend on each other in
some way i didn't realize.

russ

[gri, 2011-04-06 22:49:41]

LGTM

[rsc, 2011-04-08 16:27:10]

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

gofix: add -diff, various fixes and helpers

  * add -diff command line option
  * use scoping information in refersTo, isPkgDot, isPtrPkgDot.
  * add new scoping-based helpers countUses, rewriteUses, assignsTo, isTopName.
  * rename rewrite to walk, add walkBeforeAfter.
  * add toy typechecker, a placeholder for go/types

R=gri
CC=golang-dev
http://codereview.appspot.com/4285053