code review 6454089: go/ast: ast.Print must not crash with unexported fields (issue 6454089)

[g...@golang.org]

Reviewers: r,

Message:
Hello r...@golang.org (cc: golan...@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go


Description:
go/ast: ast.Print must not crash with unexported fields

Don't print unexported struct fields; their values are
not accessible via reflection.

Fixes issue 3898.

Also:
- added support for arrays
- print empty maps, arrays, slices, structs on one line
for a denser output
- added respective test cases

Please review this at http://codereview.appspot.com/6454089/

Affected files:
M src/pkg/go/ast/print.go
M src/pkg/go/ast/print_test.go


Index: src/pkg/go/ast/print.go
===================================================================
--- a/src/pkg/go/ast/print.go
+++ b/src/pkg/go/ast/print.go
@@ -34,7 +34,8 @@
//
// A non-nil FieldFilter f may be provided to control the output:
// struct fields for which f(fieldname, fieldvalue) is true are
-// are printed; all others are filtered from the output.
+// are printed; all others are filtered from the output. Unexported
+// struct fields are never printed.
//
func Fprint(w io.Writer, fset *token.FileSet, x interface{}, f
FieldFilter) (err error) {
// setup printer
@@ -145,15 +146,18 @@
p.print(x.Elem())

case reflect.Map:
- p.printf("%s (len = %d) {\n", x.Type(), x.Len())
- p.indent++
- for _, key := range x.MapKeys() {
- p.print(key)
- p.printf(": ")
- p.print(x.MapIndex(key))
+ p.printf("%s (len = %d) {", x.Type(), x.Len())
+ if x.Len() > 0 {
+ p.indent++
p.printf("\n")
+ for _, key := range x.MapKeys() {
+ p.print(key)
+ p.printf(": ")
+ p.print(x.MapIndex(key))
+ p.printf("\n")
+ }
+ p.indent--
}
- p.indent--
p.printf("}")

case reflect.Ptr:
@@ -169,32 +173,57 @@
p.print(x.Elem())
}

+ case reflect.Array:
+ p.printf("%s {", x.Type())
+ if x.Len() > 0 {
+ p.indent++
+ p.printf("\n")
+ for i, n := 0, x.Len(); i < n; i++ {
+ p.printf("%d: ", i)
+ p.print(x.Index(i))
+ p.printf("\n")
+ }
+ p.indent--
+ }
+ p.printf("}")
+
case reflect.Slice:
if s, ok := x.Interface().([]byte); ok {
p.printf("%#q", s)
return
}
- p.printf("%s (len = %d) {\n", x.Type(), x.Len())
- p.indent++
- for i, n := 0, x.Len(); i < n; i++ {
- p.printf("%d: ", i)
- p.print(x.Index(i))
+ p.printf("%s (len = %d) {", x.Type(), x.Len())
+ if x.Len() > 0 {
+ p.indent++
p.printf("\n")
+ for i, n := 0, x.Len(); i < n; i++ {
+ p.printf("%d: ", i)
+ p.print(x.Index(i))
+ p.printf("\n")
+ }
+ p.indent--
}
- p.indent--
p.printf("}")

case reflect.Struct:
- p.printf("%s {\n", x.Type())
+ t := x.Type()
+ p.printf("%s {", t)
p.indent++
- t := x.Type()
+ first := true
for i, n := 0, t.NumField(); i < n; i++ {
- name := t.Field(i).Name
- value := x.Field(i)
- if p.filter == nil || p.filter(name, value) {
- p.printf("%s: ", name)
- p.print(value)
- p.printf("\n")
+ // exclude non-exported fields because their
+ // values cannot be accessed via reflection
+ if name := t.Field(i).Name; IsExported(name) {
+ value := x.Field(i)
+ if p.filter == nil || p.filter(name, value) {
+ if first {
+ p.printf("\n")
+ first = false
+ }
+ p.printf("%s: ", name)
+ p.print(value)
+ p.printf("\n")
+ }
}
}
p.indent--
Index: src/pkg/go/ast/print_test.go
===================================================================
--- a/src/pkg/go/ast/print_test.go
+++ b/src/pkg/go/ast/print_test.go
@@ -23,6 +23,7 @@
{"foobar", "0 \"foobar\""},

// maps
+ {map[Expr]string{}, `0 map[ast.Expr]string (len = 0) {}`},
{map[string]int{"a": 1},
`0 map[string]int (len = 1) {
1 . "a": 1
@@ -31,7 +32,21 @@
// pointers
{new(int), "0 *0"},

+ // arrays
+ {[0]int{}, `0 [0]int {}`},
+ {[3]int{1, 2, 3},
+ `0 [3]int {
+ 1 . 0: 1
+ 2 . 1: 2
+ 3 . 2: 3
+ 4 }`},
+ {[...]int{42},
+ `0 [1]int {
+ 1 . 0: 42
+ 2 }`},
+
// slices
+ {[]int{}, `0 []int (len = 0) {}`},
{[]int{1, 2, 3},
`0 []int (len = 3) {
1 . 0: 1
@@ -40,6 +55,12 @@
4 }`},

// structs
+ {struct{}{}, `0 struct {} {}`},
+ {struct{ x int }{007}, `0 struct { x int } {}`},
+ {struct{ X, y int }{42, 991},
+ `0 struct { X int; y int } {
+ 1 . X: 42
+ 2 }`},
{struct{ X, Y int }{42, 991},
`0 struct { X int; Y int } {
1 . X: 42

[r...@golang.org]

LGTM


http://codereview.appspot.com/6454089/diff/4001/src/pkg/go/ast/print.go
File src/pkg/go/ast/print.go (right):

http://codereview.appspot.com/6454089/diff/4001/src/pkg/go/ast/print.go#newcode216
src/pkg/go/ast/print.go:216: if name := t.Field(i).Name;
IsExported(name) {
i'd use a continue here to reduce the indentation by 1 but it's fine
like this.

http://codereview.appspot.com/6454089/

[g...@golang.org]

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

go/ast: ast.Print must not crash with unexported fields

Don't print unexported struct fields; their values are
not accessible via reflection.

Fixes issue 3898.

Also:
- added support for arrays
- print empty maps, arrays, slices, structs on one line
for a denser output
- added respective test cases

R=r
CC=golang-dev
http://codereview.appspot.com/6454089


http://codereview.appspot.com/6454089/