code review 156103: gofmt: add -r flag to rewrite source code according to ...

gofmt: add -r flag to rewrite source code according to pattern
a little slow, but usable (speed unchanged when not using -r)

tweak go/printer to handle nodes without line numbers
more gracefully in a couple cases.

[rsc, 2009-11-20 02:05:54]

Hello gri,

I'd like you to review the following change.

[rsc, 2009-11-20 02:16:53]

Added rewrite.go to this CL.

[gri, 2009-11-20 18:22:51]

Pretty cool!

http://codereview.appspot.com/156103/diff/8/1010
File src/cmd/gofmt/gofmt.go (right):

http://codereview.appspot.com/156103/diff/8/1010#newcode26
src/cmd/gofmt/gofmt.go:26: rewriteString	= flag.String("r", "", "rewrite pattern
(e.g., 'α[β:len(α)] -> α[β:]')");
What about rewriting code that contains Greek letters (I think big does, but
perhaps it's just comments)? Also, on some systems it may be cumbersome to get
to those letters.

How about declaring the "free" variables? For instance:

α β: α[β:len(α)] -> α[β:]

where the identifiers between the : are the free variables. If none are needed,
one may have to write the : anyway to avoid ambiguities, but that shouldn't be a
problem.

http://codereview.appspot.com/156103/diff/8/1010#newcode26
src/cmd/gofmt/gofmt.go:26: rewriteString	= flag.String("r", "", "rewrite pattern
(e.g., 'α[β:len(α)] -> α[β:]')");
s/rewriteString/rewritePattern/ or perhaps just: pattern ?

http://codereview.appspot.com/156103/diff/8/1011
File src/cmd/gofmt/rewrite.go (right):

http://codereview.appspot.com/156103/diff/8/1011#newcode1
src/cmd/gofmt/rewrite.go:1: package main
Missing copyright notice.

http://codereview.appspot.com/156103/diff/8/1011#newcode22
src/cmd/gofmt/rewrite.go:22: fmt.Fprintf(os.Stderr, "rewrite string should be of
the form 'pattern -> replacement'\n");
s/should/must/

http://codereview.appspot.com/156103/diff/8/1011#newcode25
src/cmd/gofmt/rewrite.go:25: s1, err := parser.ParseStmtList("input", f[0]);
Could factor this out by having a parse(s string) []ast.Stmt helper function.
Same code appears twice.

http://codereview.appspot.com/156103/diff/8/1011#newcode41
src/cmd/gofmt/rewrite.go:41: var pattern, replace interface{}
s/interface/ast.Node/

http://codereview.appspot.com/156103/diff/8/1011#newcode56
src/cmd/gofmt/rewrite.go:56: func rewriteFile(pattern, replace interface{}, p
*ast.File) *ast.File {
s/interface{}/ast.Node/

http://codereview.appspot.com/156103/diff/8/1011#newcode61
src/cmd/gofmt/rewrite.go:61: f = func(val reflect.Value) reflect.Value {
s/=/:=/ and get rid of the var f decl above

http://codereview.appspot.com/156103/diff/8/1011#newcode72
src/cmd/gofmt/rewrite.go:72: 
please add an extra empty line for consistency

http://codereview.appspot.com/156103/diff/8/1011#newcode106
src/cmd/gofmt/rewrite.go:106: 
please add extra blank line for consistency.

(Gofmt should do this eventually by looking at the prevalent style).

http://codereview.appspot.com/156103/diff/8/1011#newcode110
src/cmd/gofmt/rewrite.go:110: func match(m map[string]reflect.Value, pattern,
val reflect.Value) bool {
It might almost be good to have this (generalized) function instead of DeepEqual
in reflect. It could be interesting for a whole range of uses.

http://codereview.appspot.com/156103/diff/8/1011#newcode172
src/cmd/gofmt/rewrite.go:172: // Either way, the returned value has no line
number information.
s/no line/no valid line/

http://codereview.appspot.com/156103/diff/8/1011#newcode189
src/cmd/gofmt/rewrite.go:189: return zeroPosition
wouldn't it be better to keep the old position?

http://codereview.appspot.com/156103/diff/8/1012
File src/pkg/go/printer/nodes.go (right):

http://codereview.appspot.com/156103/diff/8/1012#newcode49
src/pkg/go/printer/nodes.go:49: if line == 0 {
This should not be necessary. It's always 0 <= min <= max (perhaps a comment is
needed) and p.pos.Line >= 0 and line >= 0. If line == 0, then n <= 0 <= min, and
the switch does the right thing.

http://codereview.appspot.com/156103/diff/8/1012#newcode58
src/pkg/go/printer/nodes.go:58: p.pos.Line += n;
Please don't do this. Code in nodes.go shouldn't mess with p.pos, it should only
read it; otherwise it will become incomprehensible. If this is necessary, there
is a bug elsewhere.

http://codereview.appspot.com/156103/diff/8/1012#newcode572
src/pkg/go/printer/nodes.go:572: //println(x.OpPos.String());
remove this println

[rsc, 2009-11-20 21:43:25]

http://codereview.appspot.com/156103/diff/8/1011
File src/cmd/gofmt/rewrite.go (right):

http://codereview.appspot.com/156103/diff/8/1011#newcode1
src/cmd/gofmt/rewrite.go:1: package main
On 2009/11/20 18:22:51, gri wrote:
> Missing copyright notice.

Done.

http://codereview.appspot.com/156103/diff/8/1011#newcode22
src/cmd/gofmt/rewrite.go:22: fmt.Fprintf(os.Stderr, "rewrite string should be of
the form 'pattern -> replacement'\n");
On 2009/11/20 18:22:51, gri wrote:
> s/should/must/ 

Done.

http://codereview.appspot.com/156103/diff/8/1011#newcode25
src/cmd/gofmt/rewrite.go:25: s1, err := parser.ParseStmtList("input", f[0]);
On 2009/11/20 18:22:51, gri wrote:
> Could factor this out by having a parse(s string) []ast.Stmt helper function.
> Same code appears twice.

Done.

http://codereview.appspot.com/156103/diff/8/1011#newcode41
src/cmd/gofmt/rewrite.go:41: var pattern, replace interface{}
On 2009/11/20 18:22:51, gri wrote:
> s/interface/ast.Node/

Done as ast.Expr for now.

http://codereview.appspot.com/156103/diff/8/1011#newcode56
src/cmd/gofmt/rewrite.go:56: func rewriteFile(pattern, replace interface{}, p
*ast.File) *ast.File {
On 2009/11/20 18:22:51, gri wrote:
> s/interface{}/ast.Node/

Done as ast.Expr for now.

http://codereview.appspot.com/156103/diff/8/1011#newcode61
src/cmd/gofmt/rewrite.go:61: f = func(val reflect.Value) reflect.Value {
On 2009/11/20 18:22:51, gri wrote:
> s/=/:=/ and get rid of the var f decl above

Can't - f is recursive.  Move the decl down and added a comment.

http://codereview.appspot.com/156103/diff/8/1011#newcode72
src/cmd/gofmt/rewrite.go:72: 
On 2009/11/20 18:22:51, gri wrote:
> please add an extra empty line for consistency

Done.

http://codereview.appspot.com/156103/diff/8/1011#newcode106
src/cmd/gofmt/rewrite.go:106: 
On 2009/11/20 18:22:51, gri wrote:
> please add extra blank line for consistency.
> 
> (Gofmt should do this eventually by looking at the prevalent style).

Done.

http://codereview.appspot.com/156103/diff/8/1011#newcode172
src/cmd/gofmt/rewrite.go:172: // Either way, the returned value has no line
number information.
On 2009/11/20 18:22:51, gri wrote:
> s/no line/no valid line/

Done.

http://codereview.appspot.com/156103/diff/8/1011#newcode189
src/cmd/gofmt/rewrite.go:189: return zeroPosition
On 2009/11/20 18:22:51, gri wrote:
> wouldn't it be better to keep the old position?

No, because the line number here (if m != nil)
is file "input" line 1.

http://codereview.appspot.com/156103/diff/8/1012
File src/pkg/go/printer/nodes.go (right):

http://codereview.appspot.com/156103/diff/8/1012#newcode49
src/pkg/go/printer/nodes.go:49: if line == 0 {
On 2009/11/20 18:22:51, gri wrote:
> This should not be necessary. It's always 0 <= min <= max (perhaps a comment
is
> needed) and p.pos.Line >= 0 and line >= 0. If line == 0, then n <= 0 <= min,
and
> the switch does the right thing.

Done.

http://codereview.appspot.com/156103/diff/8/1012#newcode58
src/pkg/go/printer/nodes.go:58: p.pos.Line += n;
On 2009/11/20 18:22:51, gri wrote:
> Please don't do this. Code in nodes.go shouldn't mess with p.pos, it should
only
> read it; otherwise it will become incomprehensible. If this is necessary,
there
> is a bug elsewhere.

The code is about the emit n newlines.
Shouldn't it update the line number?

The problem I am trying to solve is this:
linebreak gets called with a min of 1 printing
an expression statement where the expression
has no line number information.  p.pos.Line == 1,
because that's the line we're on.  Since there's no
line number information, the caller can't update
p.pos.Line, but linebreak is going to print a \n.
The next expression has line number information
and is on line 2.  The output is also on line 2
but p.pos.Line still says 1, so you get a spurious
newline.  

I figured if the caller is going to update p.pos.Line,
it will do so with an assignment (not a +=), so no
harm done here.  On the other hand, if the caller
isn't going to update p.pos.Line, this gets closer 
to the actual answer.

http://codereview.appspot.com/156103/diff/8/1012#newcode572
src/pkg/go/printer/nodes.go:572: //println(x.OpPos.String());
On 2009/11/20 18:22:51, gri wrote:
> remove this println

Done.

[gri, 2009-11-20 23:01:51]

LGTM

http://codereview.appspot.com/156103/diff/2002/2005
File src/cmd/gofmt/rewrite.go (right):

http://codereview.appspot.com/156103/diff/2002/2005#newcode63
src/cmd/gofmt/rewrite.go:63: var f func(val reflect.Value) reflect.Value;	// f
is recursive
Ah! I guess this is less uncommon than one might think.

http://codereview.appspot.com/156103/diff/2002/2006
File src/pkg/go/printer/nodes.go (right):

http://codereview.appspot.com/156103/diff/2002/2006#newcode55
src/pkg/go/printer/nodes.go:55: p.pos.Line += n;
Put a todo here:
TODO(gri): try to avoid direct manipulation of p.pos

and i'll figure out what's going on. If you have test case, please send it to
me.

[rsc, 2009-11-20 23:09:56]

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

gofmt: add -r flag to rewrite source code according to pattern
a little slow, but usable (speed unchanged when not using -r)

tweak go/printer to handle nodes without line numbers
more gracefully in a couple cases.

R=gri
http://codereview.appspot.com/156103