code review 1981044: go/ast, gofmt: facility for printing AST nodes

go/ast, gofmt: facility for printing AST nodes

go/ast: implement Fprint and print functions to
print AST nodes

gofmt: print AST nodes by setting -ast flag

[gri, 2010-08-18 22:54:37]

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

I'd like you to review this change.

[gri, 2010-08-18 23:23:56]

FYI. For:

package main
import "fmt"
func main() {
fmt.Println("Hello, World!")
}

the output is:

gofmt -ast -l ~/tmp/test.go
*ast.File {
.  Position: /Users/gri/tmp/test.go:1:1
.  Name: *ast.Ident {
.  .  Position: /Users/gri/tmp/test.go:1:9
.  .  Name: main
.  }
.  Decls: []ast.Decl (len = 2) {
.  .  0: *ast.GenDecl {
.  .  .  Position: /Users/gri/tmp/test.go:2:1
.  .  .  Tok: import
.  .  .  Lparen: -
.  .  .  Specs: []ast.Spec (len = 1) {
.  .  .  .  0: *ast.ImportSpec {
.  .  .  .  .  Path: *ast.BasicLit {
.  .  .  .  .  .  Position: /Users/gri/tmp/test.go:2:8
.  .  .  .  .  .  Kind: STRING
.  .  .  .  .  .  Value: `"fmt"`
.  .  .  .  .  }
.  .  .  .  }
.  .  .  }
.  .  .  Rparen: -
.  .  }
.  .  1: *ast.FuncDecl {
.  .  .  Name: *ast.Ident {
.  .  .  .  Position: /Users/gri/tmp/test.go:3:6
.  .  .  .  Name: main
.  .  .  }
.  .  .  Type: *ast.FuncType {
.  .  .  .  Position: /Users/gri/tmp/test.go:3:1
.  .  .  .  Params: *ast.FieldList {
.  .  .  .  .  Opening: /Users/gri/tmp/test.go:3:10
.  .  .  .  .  Closing: /Users/gri/tmp/test.go:3:11
.  .  .  .  }
.  .  .  }
.  .  .  Body: *ast.BlockStmt {
.  .  .  .  Position: /Users/gri/tmp/test.go:3:13
.  .  .  .  List: []ast.Stmt (len = 1) {
.  .  .  .  .  0: *ast.ExprStmt {
.  .  .  .  .  .  X: *ast.CallExpr {
.  .  .  .  .  .  .  Fun: *ast.SelectorExpr {
.  .  .  .  .  .  .  .  X: *ast.Ident {
.  .  .  .  .  .  .  .  .  Position: /Users/gri/tmp/test.go:4:2
.  .  .  .  .  .  .  .  .  Name: fmt
.  .  .  .  .  .  .  .  }
.  .  .  .  .  .  .  .  Sel: *ast.Ident {
.  .  .  .  .  .  .  .  .  Position: /Users/gri/tmp/test.go:4:6
.  .  .  .  .  .  .  .  .  Name: Println
.  .  .  .  .  .  .  .  }
.  .  .  .  .  .  .  }
.  .  .  .  .  .  .  Lparen: /Users/gri/tmp/test.go:4:13
.  .  .  .  .  .  .  Args: []ast.Expr (len = 1) {
.  .  .  .  .  .  .  .  0: *ast.BasicLit {
.  .  .  .  .  .  .  .  .  Position: /Users/gri/tmp/test.go:4:14
.  .  .  .  .  .  .  .  .  Kind: STRING
.  .  .  .  .  .  .  .  .  Value: `"Hello, World!"`
.  .  .  .  .  .  .  .  }
.  .  .  .  .  .  .  }
.  .  .  .  .  .  .  Rparen: /Users/gri/tmp/test.go:4:29
.  .  .  .  .  .  }
.  .  .  .  .  }
.  .  .  .  }
.  .  .  .  Rbrace: /Users/gri/tmp/test.go:5:1
.  .  .  }
.  .  }
.  }
.  Comments: []*ast.CommentGroup (len = 0) {
.  }
}


On Wed, Aug 18, 2010 at 3:54 PM, <gri@golang.org> wrote:

> Reviewers: rsc,
>
> Message:
> Hello rsc (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change.
>
>
> Description:
> go/ast, gofmt: facility for printing AST nodes
>
> go/ast: implement Fprint and print functions to
> print AST nodes
>
> gofmt: print AST nodes by setting -ast flag
>
> Please review this at http://codereview.appspot.com/1981044/
>
> Affected files:
>  M src/cmd/gofmt/doc.go
>  M src/cmd/gofmt/gofmt.go
>  M src/pkg/go/ast/Makefile
>  A src/pkg/go/ast/print.go
>  M src/pkg/go/token/token.go
>
>
>

[r, 2010-08-19 00:23:05]

http://codereview.appspot.com/1981044/diff/5001/1008
File src/pkg/go/ast/print.go (right):

http://codereview.appspot.com/1981044/diff/5001/1008#newcode43
src/pkg/go/ast/print.go:43: // not printed.
that's an odd design. i would have the boolean be true if it is to be printed.
but you can leave it like this if you prefer

http://codereview.appspot.com/1981044/diff/5001/1008#newcode91
src/pkg/go/ast/print.go:91: switch byte('\n') {
you don't need the conversion

http://codereview.appspot.com/1981044/diff/5001/1008#newcode108
src/pkg/go/ast/print.go:108: m, err = p.output.Write(data[n:])
how do you get here with n != len(data)? only if Write lied to you. i think you
just delete this line and the next

worse, you're not counting indent bytes, but you should be, so n shouldn't even
be used as the return value.

http://codereview.appspot.com/1981044/diff/5001/1008#newcode152
src/pkg/go/ast/print.go:152: }
you could do all the nil checks by calling NilValue

[rsc1, 2010-08-19 01:57:53]

LGTM
but wait for r

http://codereview.appspot.com/1981044/diff/5001/1008
File src/pkg/go/ast/print.go (right):

http://codereview.appspot.com/1981044/diff/5001/1008#newcode25
src/pkg/go/ast/print.go:25: switch v := value.(type) {
v, ok := value.(interface { IsNil() bool })
return ok && v.IsNil()

http://codereview.appspot.com/1981044/diff/5001/1008#newcode43
src/pkg/go/ast/print.go:43: // not printed.
On 2010/08/19 00:23:05, r wrote:
> that's an odd design. i would have the boolean be true if it is to be printed.
> but you can leave it like this if you prefer

I agree.  This is backward from go/parser's ParseDir, for example, and also
exp/iterable's Filter, though the latter
is almost irrelevant.

http://codereview.appspot.com/1981044/diff/5001/1008#newcode69
src/pkg/go/ast/print.go:69: // Print(x) is a convenience wrapper for
Fprint(os.Stdout, x, NilFilter);
It would be more helpful to the user to know
what it does directly:

// Print prints x to standard output, skipping nil fields.

http://codereview.appspot.com/1981044/diff/5001/1008#newcode91
src/pkg/go/ast/print.go:91: switch byte('\n') {
On 2010/08/19 00:23:05, r wrote:
> you don't need the conversion

You do; sorry.  '\n' is an int.

That aside, I find this really confusing to read.
It's like writing '\n' == b and '\n' == p.last.
It reminds me of gUnit.

What's really going on here is that when you get to a
newline, you do the write and then if there is another
byte, add the prefix.  Instead of p.last why not use 
a more descriptive flag like p.needPrefix?

for i, b := range data {
    if p.needPrefix {
        ...
    }
    if b == '\n' {
        ...
    }
}

[gri, 2010-08-19 06:10:10]

PTAL.

Looking for stronger arguments re: the filter design.

- gri

http://codereview.appspot.com/1981044/diff/5001/1008
File src/pkg/go/ast/print.go (right):

http://codereview.appspot.com/1981044/diff/5001/1008#newcode25
src/pkg/go/ast/print.go:25: switch v := value.(type) {
On 2010/08/19 01:57:53, rsc1 wrote:
> v, ok := value.(interface { IsNil() bool })
> return ok && v.IsNil()
> 

Neat!

http://codereview.appspot.com/1981044/diff/5001/1008#newcode43
src/pkg/go/ast/print.go:43: // not printed.
On 2010/08/19 01:57:53, rsc1 wrote:
> On 2010/08/19 00:23:05, r wrote:
> > that's an odd design. i would have the boolean be true if it is to be
printed.
> > but you can leave it like this if you prefer
> 
> I agree.  This is backward from go/parser's ParseDir, for example, and also
> exp/iterable's Filter, though the latter
> is almost irrelevant.

It's not that odd, really: filter returns true if the argument is filtered. The
use is: should x be filtered? if !filter(x) ... The function is called filter
not shouldPrint.

Not convinced anymore that the ParseDir use is the right way.

Looking for stronger arguments either way. Left alone for now.

http://codereview.appspot.com/1981044/diff/5001/1008#newcode69
src/pkg/go/ast/print.go:69: // Print(x) is a convenience wrapper for
Fprint(os.Stdout, x, NilFilter);
On 2010/08/19 01:57:53, rsc1 wrote:
> It would be more helpful to the user to know
> what it does directly:
> 
> // Print prints x to standard output, skipping nil fields.
> 

Done.

http://codereview.appspot.com/1981044/diff/5001/1008#newcode91
src/pkg/go/ast/print.go:91: switch byte('\n') {
On 2010/08/19 00:23:05, r wrote:
> you don't need the conversion

It's needed. See rsc's comment.

http://codereview.appspot.com/1981044/diff/5001/1008#newcode91
src/pkg/go/ast/print.go:91: switch byte('\n') {
On 2010/08/19 01:57:53, rsc1 wrote:
> On 2010/08/19 00:23:05, r wrote:
> > you don't need the conversion
> 
> You do; sorry.  '\n' is an int.
yes
> 
> That aside, I find this really confusing to read.
> It's like writing '\n' == b and '\n' == p.last.
> It reminds me of gUnit.
maybe
> 
> What's really going on here is that when you get to a
> newline, you do the write and then if there is another
> byte, add the prefix.  Instead of p.last why not use 
> a more descriptive flag like p.needPrefix?
> 
> for i, b := range data {
>     if p.needPrefix {
>         ...
>     }
>     if b == '\n' {
>         ...
>     }
> }
The prefix doesn't need to be written if the current char is a '\n', so the
tests need to be swapped. But then needPrefix has a more complex meaning. I was
there and decided that p.last has a simpler meaning, it's always the last char
processed. The complexity is then only in the logic.

Anyway, I rewrote it into an if-else-if which may be easier to read, it's also
one line shorter. Of course, the switch is simply the straight-forward
transformation of the if.

> 
>

http://codereview.appspot.com/1981044/diff/5001/1008#newcode108
src/pkg/go/ast/print.go:108: m, err = p.output.Write(data[n:])
On 2010/08/19 00:23:05, r wrote:
> how do you get here with n != len(data)? only if Write lied to you. i think
you
> just delete this line and the next

There may be only portions of data written each time. The last Write may have an
error and only write m bytes. The return value of this function will return that
error and the total number of input bytes processed. I believe this is correct.

> 
> worse, you're not counting indent bytes, but you should be, so n shouldn't
even
> be used as the return value.

indent bytes are not part of the input. io.Write returns the number of input
bytes processed, print routines the number of bytes printed.

http://codereview.appspot.com/1981044/diff/5001/1008#newcode152
src/pkg/go/ast/print.go:152: }
On 2010/08/19 00:23:05, r wrote:
> you could do all the nil checks by calling NilValue

There is no NilValue anywhere. What am I missing?

But I can use the NilFilter.

[r, 2010-08-19 07:57:37]

>
> There is no NilValue anywhere. What am I missing?
>
> But I can use the NilFilter.
>
> http://codereview.appspot.com/1981044/
>

yes, i meant NilFilter.

-rob

[r, 2010-08-19 07:58:03]

LGTM

leaving it up to you whether to flip the boolean on filter

http://codereview.appspot.com/1981044/diff/11001/12004
File src/pkg/go/ast/print.go (right):

http://codereview.appspot.com/1981044/diff/11001/12004#newcode28
src/pkg/go/ast/print.go:28: return ok && v.IsNil()
i knew there had to be a better way

http://codereview.appspot.com/1981044/diff/11001/12004#newcode38
src/pkg/go/ast/print.go:38: func Fprint(w io.Writer, x interface{}, f
FieldFilter) (n int, err os.Error) {
a yellow filter lets yellow through, it doesn't filter out the yellow. also the
sense we're proposing is the same as the funcs used in the sttrings and bytes
package.

but it's not important.

[rsc, 2010-08-19 15:50:21]

> Looking for stronger arguments re: the filter design.

Standard ML, OCaml, Haskell, JavaScript, Scheme,
Erlang, and Python all provide standard library functions
named filter that interpret the function as "true means
keep it".

Russ

[gri, 2010-08-19 16:39:34]

Inverted meaning of filter result and submitted.
- Robert

On Thu, Aug 19, 2010 at 12:58 AM, <r@golang.org> wrote:

> LGTM
>
> leaving it up to you whether to flip the boolean on filter
>
>
>
> http://codereview.appspot.com/1981044/diff/11001/12004
>
> File src/pkg/go/ast/print.go (right):
>
> http://codereview.appspot.com/1981044/diff/11001/12004#newcode28
> src/pkg/go/ast/print.go:28: return ok && v.IsNil()
> i knew there had to be a better way
>
> http://codereview.appspot.com/1981044/diff/11001/12004#newcode38
> src/pkg/go/ast/print.go:38: func Fprint(w io.Writer, x interface{}, f
> FieldFilter) (n int, err os.Error) {
> a yellow filter lets yellow through, it doesn't filter out the yellow.
> also the sense we're proposing is the same as the funcs used in the
> sttrings and bytes package.
>
> but it's not important.
>
>
> http://codereview.appspot.com/1981044/
>

[gri, 2010-08-19 16:39:40]

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

go/ast, gofmt: facility for printing AST nodes

go/ast: implement Fprint and print functions to
print AST nodes

gofmt: print AST nodes by setting -ast flag

R=rsc, r
CC=golang-dev
http://codereview.appspot.com/1981044

Committer: Robert Griesemer <gri@golang.org>