code review 5436051: gofix: trivial rewrite for template.ParseFiles

gofix: trivial rewrite for template.ParseFiles
Also warn about uses of Set.

[r, 2011-11-22 19:52:52]

Hello golang-dev@googlegroups.com,

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

[rsc, 2011-11-22 20:05:43]

http://codereview.appspot.com/5436051/diff/2002/src/cmd/gofix/template.go
File src/cmd/gofix/template.go (right):

http://codereview.appspot.com/5436051/diff/2002/src/cmd/gofix/template.go#new...
src/cmd/gofix/template.go:52: if sel.Sel.Name == "ParseFile" {
This test is not safe, because there might be some other
type with a ParseFile method.  It would be fine to say that if
fixed is already true (there was a reference to template.ParseFile
elsewhere) then this rewrite can proceed, if you want to do two
passes.  Other alternatives: add type checking, delete the rewrite.

http://codereview.appspot.com/5436051/diff/2002/src/cmd/gofix/template.go#new...
src/cmd/gofix/template.go:59: if sel.Sel.Name == name {
if isPkgDot(sel, "template", name) {

[r, 2011-11-22 20:10:34]

http://codereview.appspot.com/5436051/diff/2002/src/cmd/gofix/template.go
File src/cmd/gofix/template.go (right):

http://codereview.appspot.com/5436051/diff/2002/src/cmd/gofix/template.go#new...
src/cmd/gofix/template.go:52: if sel.Sel.Name == "ParseFile" {

a typical file will have a single call to ParseFile, either a function or a
method, so a separate reference won't fix this in most cases.

so i suppose the best i can offer is to warn that there appears to be a
ParseFile call i can't fix, unless you can explain how to type check with the
current ast.

http://codereview.appspot.com/5436051/diff/2002/src/cmd/gofix/template.go#new...
src/cmd/gofix/template.go:59: if sel.Sel.Name == name {
On 2011/11/22 20:05:43, rsc wrote:
> if isPkgDot(sel, "template", name) {

that won't get method calls.

[rsc, 2011-11-22 20:19:22]

To use the type checker, the first step is to write a TypeConfig
that describes the types of interest, the return types of their
method calls, the return types of any functions of interest,
and the types of any variables of interest (probably none here):


var templateTypeConfig = &TypeConfig{
	Type: map[string]*Type{
		"template.Template": &Type{
			Method: map[string]string{
				"Delims": "*template.Template",
				"Funcs": "*template.Template",
				...
			},
		},
		"template.Set": &Type{
			Method: map[string]string{
				...
			},
		},
	},
	
	Func: map[string]string{
		"template.Must": "*template.Template,error",
		"template.SetMust": "*template.Set,error",
		...
	},
	
	Var: map[string]string{
		// probably empty
	},
}

The next step is to invoke the type checker at the top of the fix:

    typeof, _ := typecheck(templateTypeConfig, f)

Then in the type checker, when you are looking at a selector
expression, you should be able to say

    if typeof[sel.X] == "*template.Template" && sel.Name.Name == "ParseFiles" {
        ... do rewrite
    }

to constrain the rewrite to an appropriate context.

I will not be surprised if you hit small bugs.  Feel free to
ping me before getting too confused.

Russ

[r, 2011-11-22 21:32:33]

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

Please take another look.

[rsc, 2011-11-22 21:38:37]

LGTM

http://codereview.appspot.com/5436051/diff/4001/src/cmd/gofix/template.go
File src/cmd/gofix/template.go (right):

http://codereview.appspot.com/5436051/diff/4001/src/cmd/gofix/template.go#new...
src/cmd/gofix/template.go:45: "ParseFile": "*template.Template,error",
If you add the chaining methods and an entry in Funcs for template.New, you'll
be able to catch things like template.New("x").Funcs(m).ParseFile.

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

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

Please take another look.

[rsc, 2011-11-22 22:21:20]

LGTM

[r, 2011-11-24 04:17:41]

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

gofix: trivial rewrite for template.ParseFiles
Also warn about uses of Set.

R=rsc
CC=golang-dev
http://codereview.appspot.com/5436051