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
https://golang.org/cl/5305066

src/cmd/gofix/Makefile

@@ -6,6 +6,7 @@ include ../../Make.inc
 
 TARG=gofix
 GOFILES=\
+	error.go\
 	filepath.go\
 	fix.go\
 	httpfinalurl.go\

src/cmd/gofix/error_test.go

+// Copyright 2011 The Go Authors.  All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package main
+
+func init() {
+	addTestCases(errorTests, errorFn)
+}
+
+var errorTests = []testCase{
+	{
+		Name: "error.0",
+		In: `package main
+
+func error() {}
+
+var error int
+`,
+		Out: `package main
+
+func error() {}
+
+var error int
+`,
+	},
+	{
+		Name: "error.1",
+		In: `package main
+
+import "os"
+
+func f() os.Error {
+	return os.EOF
+}
+
+func error() {}
+
+var error int
+
+func g() {
+	error := 1
+	_ = error
+}
+`,
+		Out: `package main
+
+import "io"
+
+func f() error {
+	return io.EOF
+}
+
+func error_() {}
+
+var error_ int
+
+func g() {
+	error := 1
+	_ = error
+}
+`,
+	},
+	{
+		Name: "error.2",
+		In: `package main
+
+import "os"
+
+func f() os.Error {
+	return os.EOF
+}
+
+func g() string {
+	// these all convert because f is known
+	if err := f(); err != nil {
+		return err.String()
+	}
+	if err1 := f(); err1 != nil {
+		return err1.String()
+	}
+	if e := f(); e != nil {
+		return e.String()
+	}
+	if x := f(); x != nil {
+		return x.String()
+	}
+
+	// only the error names (err, err1, e) convert; u is not known
+	if err := u(); err != nil {
+		return err.String()
+	}
+	if err1 := u(); err1 != nil {
+		return err1.String()
+	}
+	if e := u(); e != nil {
+		return e.String()
+	}
+	if x := u(); x != nil {
+		return x.String()
+	}
+	return ""
+}
+
+type T int
+
+func (t T) String() string { return "t" }
+
+type PT int
+
+func (p *PT) String() string { return "pt" }
+
+type MyError int
+
+func (t MyError) String() string { return "myerror" }
+
+type PMyError int
+
+func (p *PMyError) String() string { return "pmyerror" }
+
+func error() {}
+
+var error int
+`,
+		Out: `package main
+
+import "io"
+
+func f() error {
+	return io.EOF
+}
+
+func g() string {
+	// these all convert because f is known
+	if err := f(); err != nil {
+		return err.Error()
+	}
+	if err1 := f(); err1 != nil {
+		return err1.Error()
+	}
+	if e := f(); e != nil {
+		return e.Error()
+	}
+	if x := f(); x != nil {
+		return x.Error()
+	}
+
+	// only the error names (err, err1, e) convert; u is not known
+	if err := u(); err != nil {
+		return err.Error()
+	}
+	if err1 := u(); err1 != nil {
+		return err1.Error()
+	}
+	if e := u(); e != nil {
+		return e.Error()
+	}
+	if x := u(); x != nil {
+		return x.String()
+	}
+	return ""
+}
+
+type T int
+
+func (t T) String() string { return "t" }
+
+type PT int
+
+func (p *PT) String() string { return "pt" }
+
+type MyError int
+
+func (t MyError) Error() string { return "myerror" }
+
+type PMyError int
+
+func (p *PMyError) Error() string { return "pmyerror" }
+
+func error_() {}
+
+var error_ int
+`,
+	},
+	{
+		Name: "error.3",
+		In: `package main
+
+import "os"
+
+func f() os.Error {
+	return os.EOF
+}
+
+type PathError struct {
+	Name  string
+	Error os.Error
+}
+
+func (p *PathError) String() string {
+	return p.Name + ": " + p.Error.String()
+}
+
+func (p *PathError) Error1() string {
+	p = &PathError{Error: nil}
+	return fmt.Sprint(p.Name, ": ", p.Error)
+}
+`,
+		Out: `package main
+
+import "io"
+
+func f() error {
+	return io.EOF
+}
+
+type PathError struct {
+	Name string
+	Err  error
+}
+
+func (p *PathError) Error() string {
+	return p.Name + ": " + p.Err.Error()
+}
+
+func (p *PathError) Error1() string {
+	p = &PathError{Err: nil}
+	return fmt.Sprint(p.Name, ": ", p.Err)
+}
+`,
+	},
+}

src/cmd/gofix/fix.go

@@ -4,11 +4,20 @@
 
 package main
 
+/*
+receiver named error
+function named error
+method on error
+exiterror
+slice of named type (go/scanner)
+*/
+
 import (
 	"fmt"
 	"go/ast"
 	"go/token"
 	"os"
+	"path"
 	"strconv"
 	"strings"
 )
@@ -97,6 +106,8 @@ func walkBeforeAfter(x interface{}, before, after func(interface{})) {
 		walkBeforeAfter(*n, before, after)
 	case **ast.Ident:
 		walkBeforeAfter(*n, before, after)
+	case **ast.BasicLit:
+		walkBeforeAfter(*n, before, after)
 
 	// pointers to slices
 	case *[]ast.Decl:
@@ -114,7 +125,9 @@ func walkBeforeAfter(x interface{}, before, after func(interface{})) {
 
 	// These are ordered and grouped to match ../../pkg/go/ast/ast.go
 	case *ast.Field:
+		walkBeforeAfter(&n.Names, before, after)
 		walkBeforeAfter(&n.Type, before, after)
+		walkBeforeAfter(&n.Tag, before, after)
 	case *ast.FieldList:
 		for _, field := range n.List {
 			walkBeforeAfter(field, before, after)
@@ -484,23 +497,101 @@ func newPkgDot(pos token.Pos, pkg, name string) ast.Expr {
 	}
 }
 
+// renameTop renames all references to the top-level name top.
+// It returns true if it makes any changes.
+func renameTop(f *ast.File, old, new string) bool {
+	var fixed bool
+
+	// Rename any conflicting imports
+	// (assuming package name is last element of path).
+	for _, s := range f.Imports {
+		if s.Name != nil {
+			if s.Name.Name == old {
+				s.Name.Name = new
+				fixed = true
+			}
+		} else {
+			_, thisName := path.Split(importPath(s))
+			if thisName == old {
+				s.Name = ast.NewIdent(new)
+				fixed = true
+			}
+		}
+	}
+
+	// Rename any top-level declarations.
+	for _, d := range f.Decls {
+		switch d := d.(type) {
+		case *ast.FuncDecl:
+			if d.Recv == nil && d.Name.Name == old {
+				d.Name.Name = new
+				d.Name.Obj.Name = new
+				fixed = true
+			}
+		case *ast.GenDecl:
+			for _, s := range d.Specs {
+				switch s := s.(type) {
+				case *ast.TypeSpec:
+					if s.Name.Name == old {
+						s.Name.Name = new
+						s.Name.Obj.Name = new
+						fixed = true
+					}
+				case *ast.ValueSpec:
+					for _, n := range s.Names {
+						if n.Name == old {
+							n.Name = new
+							n.Obj.Name = new
+							fixed = true
+						}
+					}
+				}
+			}
+		}
+	}
+
+	// Rename top-level old to new, both unresolved names
+	// (probably defined in another file) and names that resolve
+	// to a declaration we renamed.
+	walk(f, func(n interface{}) {
+		id, ok := n.(*ast.Ident)
+		if ok && isTopName(id, old) {
+			id.Name = new
+			fixed = true
+		}
+		if ok && id.Obj != nil && id.Name == old && id.Obj.Name == new {
+			id.Name = id.Obj.Name
+			fixed = true
+		}
+	})
+
+	return fixed
+}
+
 // addImport adds the import path to the file f, if absent.
-func addImport(f *ast.File, path string) {
-	if imports(f, path) {
+func addImport(f *ast.File, ipath string) {
+	if imports(f, ipath) {
 		return
 	}
 
+	// Determine name of import.
+	// Assume added imports follow convention of using last element.
+	_, name := path.Split(ipath)
+
+	// Rename any conflicting top-level references from name to name_.
+	renameTop(f, name, name+"_")
+
 	newImport := &ast.ImportSpec{
 		Path: &ast.BasicLit{
 			Kind:  token.STRING,
-			Value: strconv.Quote(path),
+			Value: strconv.Quote(ipath),
 		},
 	}
 
 	var impdecl *ast.GenDecl
 
 	// Find an import decl to add to.
-	var lastImport int = -1
+	var lastImport = -1
 	for i, decl := range f.Decls {
 		gen, ok := decl.(*ast.GenDecl)
 
@@ -535,7 +626,7 @@ func addImport(f *ast.File, path string) {
 	insertAt := len(impdecl.Specs) // default to end of specs
 	for i, spec := range impdecl.Specs {
 		impspec := spec.(*ast.ImportSpec)
-		if importPath(impspec) > path {
+		if importPath(impspec) > ipath {
 			insertAt = i
 			break
 		}

src/cmd/gofix/httpheaders.go

@@ -31,7 +31,7 @@ func httpheaders(f *ast.File) bool {
 	})
 
 	fixed := false
-	typeof := typecheck(headerTypeConfig, f)
+	typeof, _ := typecheck(headerTypeConfig, f)
 	walk(f, func(ni interface{}) {
 		switch n := ni.(type) {
 		case *ast.SelectorExpr:

src/cmd/gofix/imagecolor.go

@@ -47,8 +47,6 @@ func imagecolor(f *ast.File) (fixed bool) {
 		return
 	}
 
-	importColor := false
-
 	walk(f, func(n interface{}) {
 		s, ok := n.(*ast.SelectorExpr)
 
@@ -66,20 +64,17 @@ func imagecolor(f *ast.File) (fixed bool) {
 		default:
 			for _, rename := range colorRenames {
 				if sel == rename.in {
+					addImport(f, "image/color")
 					s.X.(*ast.Ident).Name = "color"
 					s.Sel.Name = rename.out
 					fixed = true
-					importColor = true
 				}
 			}
 		}
 	})
 
-	if importColor {
-		addImport(f, "image/color")
-		if !usesImport(f, "image") {
-			deleteImport(f, "image")
-		}
+	if fixed && !usesImport(f, "image") {
+		deleteImport(f, "image")
 	}
 	return
 }

src/cmd/gofix/main.go

@@ -28,14 +28,18 @@ var (
 var allowedRewrites = flag.String("r", "",
 	"restrict the rewrites to this comma-separated list")
 
-var allowed map[string]bool
+var forceRewrites = flag.String("force", "",
+	"force these fixes to run even if the code looks updated")
+
+var allowed, force map[string]bool
 
 var doDiff = flag.Bool("diff", false, "display diffs instead of rewriting files")
 
 func usage() {
-	fmt.Fprintf(os.Stderr, "usage: gofix [-diff] [-r fixname,...] [path ...]\n")
+	fmt.Fprintf(os.Stderr, "usage: gofix [-diff] [-r fixname,...] [-force fixname,...] [path ...]\n")
 	flag.PrintDefaults()
 	fmt.Fprintf(os.Stderr, "\nAvailable rewrites are:\n")
+	sort.Sort(fixes)
 	for _, f := range fixes {
 		fmt.Fprintf(os.Stderr, "\n%s\n", f.name)
 		desc := strings.TrimSpace(f.desc)
@@ -46,8 +50,6 @@ func usage() {
 }
 
 func main() {
-	sort.Sort(fixes)
-
 	flag.Usage = usage
 	flag.Parse()
 
@@ -58,6 +60,13 @@ func main() {
 		}
 	}
 
+	if *forceRewrites != "" {
+		force = make(map[string]bool)
+		for _, f := range strings.Split(*forceRewrites, ",") {
+			force[f] = true
+		}
+	}
+
 	if flag.NArg() == 0 {
 		if err := processFile("standard input", true); err != nil {
 			report(err)

src/cmd/gofix/math.go

@@ -4,14 +4,7 @@
 
 package main
 
-import (
-	"fmt"
-	"os"
-	"go/ast"
-)
-
-var _ fmt.Stringer
-var _ os.Error
+import "go/ast"
 
 var mathFix = fix{
 	"math",

src/cmd/gofix/reflect.go

@@ -95,7 +95,7 @@ func reflectFn(f *ast.File) bool {
 
 	// Rewrite names in method calls.
 	// Needs basic type information (see above).
-	typeof := typecheck(reflectTypeConfig, f)
+	typeof, _ := typecheck(reflectTypeConfig, f)
 	walk(f, func(n interface{}) {
 		switch n := n.(type) {
 		case *ast.SelectorExpr:

src/cmd/gofix/signal.go

@@ -32,16 +32,14 @@ func signal(f *ast.File) (fixed bool) {
 
 		sel := s.Sel.String()
 		if sel == "Signal" || sel == "UnixSignal" || strings.HasPrefix(sel, "SIG") {
+			addImport(f, "os")
 			s.X = &ast.Ident{Name: "os"}
 			fixed = true
 		}
 	})
 
-	if fixed {
-		addImport(f, "os")
-		if !usesImport(f, "os/signal") {
-			deleteImport(f, "os/signal")
-		}
+	if fixed && !usesImport(f, "os/signal") {
+		deleteImport(f, "os/signal")
 	}
 	return
 }

src/cmd/gofix/typecheck.go

@@ -99,6 +99,7 @@ type Type struct {
 	Field  map[string]string // map field name to type
 	Method map[string]string // map method name to comma-separated return types
 	Embed  []string          // list of types this type embeds (for extra methods)
+	Def    string            // definition of named type
 }
 
 // dot returns the type of "typ.name", making its decision
@@ -128,9 +129,15 @@ func (typ *Type) dot(cfg *TypeConfig, name string) string {
 }
 
 // typecheck type checks the AST f assuming the information in cfg.
-// It returns a map from AST nodes to type information in gofmt string form.
-func typecheck(cfg *TypeConfig, f *ast.File) map[interface{}]string {
-	typeof := make(map[interface{}]string)
+// It returns two maps with type information:
+// typeof maps AST nodes to type information in gofmt string form.
+// assign maps type strings to lists of expressions that were assigned
+// to values of another type that were assigned to that type.
+func typecheck(cfg *TypeConfig, f *ast.File) (typeof map[interface{}]string, assign map[string][]interface{}) {
+	typeof = make(map[interface{}]string)
+	assign = make(map[string][]interface{})
+	cfg1 := &TypeConfig{}
+	*cfg1 = *cfg // make copy so we can add locally
 
 	// gather function declarations
 	for _, decl := range f.Decls {
@@ -138,7 +145,7 @@ func typecheck(cfg *TypeConfig, f *ast.File) map[interface{}]string {
 		if !ok {
 			continue
 		}
-		typecheck1(cfg, fn.Type, typeof)
+		typecheck1(cfg, fn.Type, typeof, assign)
 		t := typeof[fn.Type]
 		if fn.Recv != nil {
 			// The receiver must be a type.
@@ -168,8 +175,42 @@ func typecheck(cfg *TypeConfig, f *ast.File) map[interface{}]string {
 		}
 	}
 
-	typecheck1(cfg, f, typeof)
-	return typeof
+	// gather struct declarations
+	for _, decl := range f.Decls {
+		d, ok := decl.(*ast.GenDecl)
+		if ok {
+			for _, s := range d.Specs {
+				switch s := s.(type) {
+				case *ast.TypeSpec:
+					if cfg1.Type[s.Name.Name] != nil {
+						break
+					}
+					if cfg1.Type == cfg.Type || cfg1.Type == nil {
+						// Copy map lazily: it's time.
+						cfg1.Type = make(map[string]*Type)
+						for k, v := range cfg.Type {
+							cfg1.Type[k] = v
+						}
+					}
+					t := &Type{Field: map[string]string{}}
+					cfg1.Type[s.Name.Name] = t
+					switch st := s.Type.(type) {
+					case *ast.StructType:
+						for _, f := range st.Fields.List {
+							for _, n := range f.Names {
+								t.Field[n.Name] = gofmt(f.Type)
+							}
+						}
+					case *ast.ArrayType, *ast.StarExpr, *ast.MapType:
+						t.Def = gofmt(st)
+					}
+				}
+			}
+		}
+	}
+
+	typecheck1(cfg1, f, typeof, assign)
+	return typeof, assign
 }
 
 func makeExprList(a []*ast.Ident) []ast.Expr {
@@ -183,11 +224,14 @@ func makeExprList(a []*ast.Ident) []ast.Expr {
 // Typecheck1 is the recursive form of typecheck.
 // It is like typecheck but adds to the information in typeof
 // instead of allocating a new map.
-func typecheck1(cfg *TypeConfig, f interface{}, typeof map[interface{}]string) {
+func typecheck1(cfg *TypeConfig, f interface{}, typeof map[interface{}]string, assign map[string][]interface{}) {
 	// set sets the type of n to typ.
 	// If isDecl is true, n is being declared.
 	set := func(n ast.Expr, typ string, isDecl bool) {
 		if typeof[n] != "" || typ == "" {
+			if typeof[n] != typ {
+				assign[typ] = append(assign[typ], n)
+			}
 			return
 		}
 		typeof[n] = typ
@@ -236,6 +280,14 @@ func typecheck1(cfg *TypeConfig, f interface{}, typeof map[interface{}]string) {
 		}
 	}
 
+	expand := func(s string) string {
+		typ := cfg.Type[s]
+		if typ != nil && typ.Def != "" {
+			return typ.Def
+		}
+		return s
+	}
+
 	// The main type check is a recursive algorithm implemented
 	// by walkBeforeAfter(n, before, after).
 	// Most of it is bottom-up, but in a few places we need
@@ -263,7 +315,7 @@ func typecheck1(cfg *TypeConfig, f interface{}, typeof map[interface{}]string) {
 			defer func() {
 				if t := typeof[n]; t != "" {
 					pos := fset.Position(n.(ast.Node).Pos())
-					fmt.Fprintf(os.Stderr, "%s: typeof[%s] = %s\n", pos.String(), gofmt(n), t)
+					fmt.Fprintf(os.Stderr, "%s: typeof[%s] = %s\n", pos, gofmt(n), t)
 				}
 			}()
 		}
@@ -405,6 +457,8 @@ func typecheck1(cfg *TypeConfig, f interface{}, typeof map[interface{}]string) {
 			// x.(T) has type T.
 			if t := typeof[n.Type]; isType(t) {
 				typeof[n] = getType(t)
+			} else {
+				typeof[n] = gofmt(n.Type)
 			}
 
 		case *ast.SliceExpr:
@@ -413,7 +467,7 @@ func typecheck1(cfg *TypeConfig, f interface{}, typeof map[interface{}]string) {
 
 		case *ast.IndexExpr:
 			// x[i] has key type of x's type.
-			t := typeof[n.X]
+			t := expand(typeof[n.X])
 			if strings.HasPrefix(t, "[") || strings.HasPrefix(t, "map[") {
 				// Lazy: assume there are no nested [] in the array
 				// length or map key type.
@@ -426,7 +480,7 @@ func typecheck1(cfg *TypeConfig, f interface{}, typeof map[interface{}]string) {
 			// *x for x of type *T has type T when x is an expr.
 			// We don't use the result when *x is a type, but
 			// compute it anyway.
-			t := typeof[n.X]
+			t := expand(typeof[n.X])
 			if isType(t) {
 				typeof[n] = "type *" + getType(t)
 			} else if strings.HasPrefix(t, "*") {
@@ -448,6 +502,39 @@ func typecheck1(cfg *TypeConfig, f interface{}, typeof map[interface{}]string) {
 			// (x) has type of x.
 			typeof[n] = typeof[n.X]
 
+		case *ast.RangeStmt:
+			t := expand(typeof[n.X])
+			if t == "" {
+				return
+			}
+			var key, value string
+			if t == "string" {
+				key, value = "int", "rune"
+			} else if strings.HasPrefix(t, "[") {
+				key = "int"
+				if i := strings.Index(t, "]"); i >= 0 {
+					value = t[i+1:]
+				}
+			} else if strings.HasPrefix(t, "map[") {
+				if i := strings.Index(t, "]"); i >= 0 {
+					key, value = t[4:i], t[i+1:]
+				}
+			}
+			changed := false
+			if n.Key != nil && key != "" {
+				changed = true
+				set(n.Key, key, n.Tok == token.DEFINE)
+			}
+			if n.Value != nil && value != "" {
+				changed = true
+				set(n.Value, value, n.Tok == token.DEFINE)
+			}
+			// Ugly failure of vision: already type-checked body.
+			// Do it again now that we have that type info.
+			if changed {
+				typecheck1(cfg, n.Body, typeof, assign)
+			}
+
 		case *ast.TypeSwitchStmt:
 			// Type of variable changes for each case in type switch,
 			// but go/parser generates just one variable.
@@ -471,7 +558,7 @@ func typecheck1(cfg *TypeConfig, f interface{}, typeof map[interface{}]string) {
 						tt = getType(tt)
 						typeof[varx] = tt
 						typeof[varx.Obj] = tt
-						typecheck1(cfg, cas.Body, typeof)
+						typecheck1(cfg, cas.Body, typeof, assign)
 					}
 				}
 			}

src/cmd/gofix/url.go

@@ -4,14 +4,7 @@
 
 package main
 
-import (
-	"fmt"
-	"os"
-	"go/ast"
-)
-
-var _ fmt.Stringer
-var _ os.Error
+import "go/ast"
 
 var urlFix = fix{
 	"url",
@@ -42,12 +35,7 @@ func url(f *ast.File) bool {
 	fixed := false
 
 	// Update URL code.
-	var skip interface{}
 	urlWalk := func(n interface{}) {
-		if n == skip {
-			skip = nil
-			return
-		}
 		// Is it an identifier?
 		if ident, ok := n.(*ast.Ident); ok && ident.Name == "url" {
 			ident.Name = "url_"
@@ -58,12 +46,6 @@ func url(f *ast.File) bool {
 			fixed = urlDoFields(fn.Params) || fixed
 			fixed = urlDoFields(fn.Results) || fixed
 		}
-		// U{url: ...} is likely a struct field.
-		if kv, ok := n.(*ast.KeyValueExpr); ok {
-			if ident, ok := kv.Key.(*ast.Ident); ok && ident.Name == "url" {
-				skip = ident
-			}
-		}
 	}
 
 	// Fix up URL code and add import, at most once.
@@ -71,8 +53,8 @@ func url(f *ast.File) bool {
 		if fixed {
 			return
 		}
-		walkBeforeAfter(f, urlWalk, nop)
 		addImport(f, "url")
+		walkBeforeAfter(f, urlWalk, nop)
 		fixed = true
 	}
 

src/cmd/gofix/url_test.go

@@ -103,14 +103,14 @@ func h() (url string) {
 
 import "url"
 
-type U struct{ url int }
+type U struct{ url_ int }
 type M map[int]int
 
 func f() {
 	url.Parse(a)
 	var url_ = 23
 	url_, x := 45, y
-	_ = U{url: url_}
+	_ = U{url_: url_}
 	_ = M{url_ + 1: url_}
 }