From c9d680cfc56a2639ec2540e7416b351716cea709 Mon Sep 17 00:00:00 2001 From: Rob Pike Date: Thu, 2 Oct 2014 14:16:58 -0700 Subject: fmt: make the %#v verb a special flag The %#v verb is special: it says all values below need to print as %#v. However, for some situations the # flag has other meanings and this causes some issues, particularly in how Formatters work. Since %#v dominates all formatting, translate it into actual state of the formatter and decouple it from the # flag itself within the calculations (although it must be restored when methods are doing the work.) The result is cleaner code and correct handling of # for Formatters. TODO: Apply the same thinking to the + flag in a followup CL. Also, the wasString return value in handleMethods is always false, so eliminate it. Update issue 8835 LGTM=iant R=golang-codereviews, iant CC=golang-codereviews https://codereview.appspot.com/146650043 --- src/fmt/fmt_test.go | 95 ++++++++++++++++++++++++++++- src/fmt/format.go | 12 +++- src/fmt/print.go | 173 ++++++++++++++++++++++++++++------------------------ 3 files changed, 196 insertions(+), 84 deletions(-) (limited to 'src/fmt') diff --git a/src/fmt/fmt_test.go b/src/fmt/fmt_test.go index 4586fcf93..f3b527d1f 100644 --- a/src/fmt/fmt_test.go +++ b/src/fmt/fmt_test.go @@ -1144,10 +1144,10 @@ var panictests = []struct { } func TestPanics(t *testing.T) { - for _, tt := range panictests { + for i, tt := range panictests { s := Sprintf(tt.fmt, tt.in) if s != tt.out { - t.Errorf("%q: got %q expected %q", tt.fmt, s, tt.out) + t.Errorf("%d: %q: got %q expected %q", i, tt.fmt, s, tt.out) } } } @@ -1207,3 +1207,94 @@ func TestNilDoesNotBecomeTyped(t *testing.T) { t.Errorf("expected:\n\t%q\ngot:\n\t%q", expect, got) } } + +// Formatters did not get delivered flags correctly in all cases. Issue 8835. +type fp struct{} + +func (fp) Format(f State, c rune) { + s := "%" + for i := 0; i < 128; i++ { + if f.Flag(i) { + s += string(i) + } + } + if w, ok := f.Width(); ok { + s += Sprintf("%d", w) + } + if p, ok := f.Precision(); ok { + s += Sprintf(".%d", p) + } + s += string(c) + io.WriteString(f, "["+s+"]") +} + +var formatterFlagTests = []struct { + in string + val interface{} + out string +}{ + // scalar values with the (unused by fmt) 'a' verb. + {"%a", fp{}, "[%a]"}, + {"%-a", fp{}, "[%-a]"}, + {"%+a", fp{}, "[%+a]"}, + {"%#a", fp{}, "[%#a]"}, + {"% a", fp{}, "[% a]"}, + {"%0a", fp{}, "[%0a]"}, + {"%1.2a", fp{}, "[%1.2a]"}, + {"%-1.2a", fp{}, "[%-1.2a]"}, + {"%+1.2a", fp{}, "[%+1.2a]"}, + {"%-+1.2a", fp{}, "[%+-1.2a]"}, + {"%-+1.2abc", fp{}, "[%+-1.2a]bc"}, + {"%-1.2abc", fp{}, "[%-1.2a]bc"}, + + // composite values with the 'a' verb + {"%a", [1]fp{}, "[[%a]]"}, + {"%-a", [1]fp{}, "[[%-a]]"}, + {"%+a", [1]fp{}, "[[%+a]]"}, + {"%#a", [1]fp{}, "[[%#a]]"}, + {"% a", [1]fp{}, "[[% a]]"}, + {"%0a", [1]fp{}, "[[%0a]]"}, + {"%1.2a", [1]fp{}, "[[%1.2a]]"}, + {"%-1.2a", [1]fp{}, "[[%-1.2a]]"}, + {"%+1.2a", [1]fp{}, "[[%+1.2a]]"}, + {"%-+1.2a", [1]fp{}, "[[%+-1.2a]]"}, + {"%-+1.2abc", [1]fp{}, "[[%+-1.2a]]bc"}, + {"%-1.2abc", [1]fp{}, "[[%-1.2a]]bc"}, + + // simple values with the 'v' verb + {"%v", fp{}, "[%v]"}, + {"%-v", fp{}, "[%-v]"}, + {"%+v", fp{}, "[%+v]"}, + {"%#v", fp{}, "[%#v]"}, + {"% v", fp{}, "[% v]"}, + {"%0v", fp{}, "[%0v]"}, + {"%1.2v", fp{}, "[%1.2v]"}, + {"%-1.2v", fp{}, "[%-1.2v]"}, + {"%+1.2v", fp{}, "[%+1.2v]"}, + {"%-+1.2v", fp{}, "[%+-1.2v]"}, + {"%-+1.2vbc", fp{}, "[%+-1.2v]bc"}, + {"%-1.2vbc", fp{}, "[%-1.2v]bc"}, + + // composite values with the 'v' verb. Some are still broken. + {"%v", [1]fp{}, "[[%v]]"}, + {"%-v", [1]fp{}, "[[%-v]]"}, + //{"%+v", [1]fp{}, "[[%+v]]"}, + {"%#v", [1]fp{}, "[1]fmt_test.fp{[%#v]}"}, + {"% v", [1]fp{}, "[[% v]]"}, + {"%0v", [1]fp{}, "[[%0v]]"}, + {"%1.2v", [1]fp{}, "[[%1.2v]]"}, + {"%-1.2v", [1]fp{}, "[[%-1.2v]]"}, + //{"%+1.2v", [1]fp{}, "[[%+1.2v]]"}, + //{"%-+1.2v", [1]fp{}, "[[%+-1.2v]]"}, + //{"%-+1.2vbc", [1]fp{}, "[[%+-1.2v]]bc"}, + {"%-1.2vbc", [1]fp{}, "[[%-1.2v]]bc"}, +} + +func TestFormatterFlags(t *testing.T) { + for _, tt := range formatterFlagTests { + s := Sprintf(tt.in, tt.val) + if s != tt.out { + t.Errorf("Sprintf(%q, %T) = %q, want %q", tt.in, tt.val, s, tt.out) + } + } +} diff --git a/src/fmt/format.go b/src/fmt/format.go index a92f3c2f8..355b73262 100644 --- a/src/fmt/format.go +++ b/src/fmt/format.go @@ -49,9 +49,14 @@ type fmt struct { plus bool sharp bool space bool - unicode bool - uniQuote bool // Use 'x'= prefix for %U if printable. - zero bool + // For the format %#v, we set this flag and + // clear the plus flag, since it is in effect + // a different, flagless format set at the top level. + // TODO: plusV could use this too. + sharpV bool + unicode bool + uniQuote bool // Use 'x'= prefix for %U if printable. + zero bool } func (f *fmt) clearflags() { @@ -63,6 +68,7 @@ func (f *fmt) clearflags() { f.plus = false f.sharp = false f.space = false + f.sharpV = false f.unicode = false f.uniQuote = false f.zero = false diff --git a/src/fmt/print.go b/src/fmt/print.go index de69e90fb..f141d39da 100644 --- a/src/fmt/print.go +++ b/src/fmt/print.go @@ -317,11 +317,11 @@ func (p *pp) badVerb(verb rune) { case p.arg != nil: p.buf.WriteString(reflect.TypeOf(p.arg).String()) p.add('=') - p.printArg(p.arg, 'v', false, false, 0) + p.printArg(p.arg, 'v', false, 0) case p.value.IsValid(): p.buf.WriteString(p.value.Type().String()) p.add('=') - p.printValue(p.value, 'v', false, false, 0) + p.printValue(p.value, 'v', false, 0) default: p.buf.Write(nilAngleBytes) } @@ -406,7 +406,7 @@ func (p *pp) fmtUnicode(v int64) { p.fmt.sharp = sharp } -func (p *pp) fmtUint64(v uint64, verb rune, goSyntax bool) { +func (p *pp) fmtUint64(v uint64, verb rune) { switch verb { case 'b': p.fmt.integer(int64(v), 2, unsigned, ldigits) @@ -415,7 +415,7 @@ func (p *pp) fmtUint64(v uint64, verb rune, goSyntax bool) { case 'd': p.fmt.integer(int64(v), 10, unsigned, ldigits) case 'v': - if goSyntax { + if p.fmt.sharpV { p.fmt0x64(v, true) } else { p.fmt.integer(int64(v), 10, unsigned, ldigits) @@ -499,10 +499,10 @@ func (p *pp) fmtComplex128(v complex128, verb rune) { } } -func (p *pp) fmtString(v string, verb rune, goSyntax bool) { +func (p *pp) fmtString(v string, verb rune) { switch verb { case 'v': - if goSyntax { + if p.fmt.sharpV { p.fmt.fmt_q(v) } else { p.fmt.fmt_s(v) @@ -520,9 +520,9 @@ func (p *pp) fmtString(v string, verb rune, goSyntax bool) { } } -func (p *pp) fmtBytes(v []byte, verb rune, goSyntax bool, typ reflect.Type, depth int) { +func (p *pp) fmtBytes(v []byte, verb rune, typ reflect.Type, depth int) { if verb == 'v' || verb == 'd' { - if goSyntax { + if p.fmt.sharpV { if v == nil { if typ == nil { p.buf.WriteString("[]byte(nil)") @@ -543,15 +543,15 @@ func (p *pp) fmtBytes(v []byte, verb rune, goSyntax bool, typ reflect.Type, dept } for i, c := range v { if i > 0 { - if goSyntax { + if p.fmt.sharpV { p.buf.Write(commaSpaceBytes) } else { p.buf.WriteByte(' ') } } - p.printArg(c, 'v', p.fmt.plus, goSyntax, depth+1) + p.printArg(c, 'v', p.fmt.plus, depth+1) } - if goSyntax { + if p.fmt.sharpV { p.buf.WriteByte('}') } else { p.buf.WriteByte(']') @@ -572,7 +572,7 @@ func (p *pp) fmtBytes(v []byte, verb rune, goSyntax bool, typ reflect.Type, dept } } -func (p *pp) fmtPointer(value reflect.Value, verb rune, goSyntax bool) { +func (p *pp) fmtPointer(value reflect.Value, verb rune) { use0x64 := true switch verb { case 'p', 'v': @@ -594,7 +594,7 @@ func (p *pp) fmtPointer(value reflect.Value, verb rune, goSyntax bool) { return } - if goSyntax { + if p.fmt.sharpV { p.add('(') p.buf.WriteString(value.Type().String()) p.add(')') @@ -611,7 +611,7 @@ func (p *pp) fmtPointer(value reflect.Value, verb rune, goSyntax bool) { if use0x64 { p.fmt0x64(uint64(u), !p.fmt.sharp) } else { - p.fmtUint64(uint64(u), verb, false) + p.fmtUint64(uint64(u), verb) } } } @@ -636,24 +636,44 @@ func (p *pp) catchPanic(arg interface{}, verb rune) { // Nested panics; the recursion in printArg cannot succeed. panic(err) } + p.fmt.clearflags() // We are done, and for this output we want default behavior. p.buf.Write(percentBangBytes) p.add(verb) p.buf.Write(panicBytes) p.panicking = true - p.printArg(err, 'v', false, false, 0) + p.printArg(err, 'v', false, 0) p.panicking = false p.buf.WriteByte(')') } } -func (p *pp) handleMethods(verb rune, plus, goSyntax bool, depth int) (wasString, handled bool) { +// clearSpecialFlags pushes %#v back into the regular flags and returns their old state. +func (p *pp) clearSpecialFlags() bool { + ret := p.fmt.sharpV + if ret { + p.fmt.sharp = true + p.fmt.sharpV = false + } + return ret +} + +// restoreSpecialFlags, whose argument should be a call to clearSpecialFlags, +// restores the setting of the sharpV flag. +func (p *pp) restoreSpecialFlags(sharpV bool) { + if sharpV { + p.fmt.sharp = false + p.fmt.sharpV = true + } +} + +func (p *pp) handleMethods(verb rune, plus bool, depth int) (handled bool) { if p.erroring { return } // Is it a Formatter? if formatter, ok := p.arg.(Formatter); ok { handled = true - wasString = false + defer p.restoreSpecialFlags(p.clearSpecialFlags()) defer p.catchPanic(p.arg, verb) formatter.Format(p, verb) return @@ -664,14 +684,13 @@ func (p *pp) handleMethods(verb rune, plus, goSyntax bool, depth int) (wasString } // If we're doing Go syntax and the argument knows how to supply it, take care of it now. - if goSyntax { - p.fmt.sharp = false + if p.fmt.sharpV { if stringer, ok := p.arg.(GoStringer); ok { - wasString = false handled = true + defer p.restoreSpecialFlags(p.clearSpecialFlags()) defer p.catchPanic(p.arg, verb) // Print the result of GoString unadorned. - p.fmtString(stringer.GoString(), 's', false) + p.fmtString(stringer.GoString(), 's') return } } else { @@ -682,30 +701,27 @@ func (p *pp) handleMethods(verb rune, plus, goSyntax bool, depth int) (wasString case 'v', 's', 'x', 'X', 'q': // Is it an error or Stringer? // The duplication in the bodies is necessary: - // setting wasString and handled, and deferring catchPanic, + // setting handled and deferring catchPanic // must happen before calling the method. switch v := p.arg.(type) { case error: - wasString = false handled = true defer p.catchPanic(p.arg, verb) - p.printArg(v.Error(), verb, plus, false, depth) + p.printArg(v.Error(), verb, plus, depth) return case Stringer: - wasString = false handled = true defer p.catchPanic(p.arg, verb) - p.printArg(v.String(), verb, plus, false, depth) + p.printArg(v.String(), verb, plus, depth) return } } } - handled = false - return + return false } -func (p *pp) printArg(arg interface{}, verb rune, plus, goSyntax bool, depth int) (wasString bool) { +func (p *pp) printArg(arg interface{}, verb rune, plus bool, depth int) (wasString bool) { p.arg = arg p.value = reflect.Value{} @@ -722,10 +738,10 @@ func (p *pp) printArg(arg interface{}, verb rune, plus, goSyntax bool, depth int // %T (the value's type) and %p (its address) are special; we always do them first. switch verb { case 'T': - p.printArg(reflect.TypeOf(arg).String(), 's', false, false, 0) + p.printArg(reflect.TypeOf(arg).String(), 's', false, 0) return false case 'p': - p.fmtPointer(reflect.ValueOf(arg), verb, goSyntax) + p.fmtPointer(reflect.ValueOf(arg), verb) return false } @@ -734,13 +750,9 @@ func (p *pp) printArg(arg interface{}, verb rune, plus, goSyntax bool, depth int // We could call handleMethods here and avoid this work, but // handleMethods is expensive enough to be worth delaying. oldPlus := p.fmt.plus - oldSharp := p.fmt.sharp if plus { p.fmt.plus = false } - if goSyntax { - p.fmt.sharp = false - } // Some types can be done without reflection. switch f := arg.(type) { @@ -765,40 +777,39 @@ func (p *pp) printArg(arg interface{}, verb rune, plus, goSyntax bool, depth int case int64: p.fmtInt64(f, verb) case uint: - p.fmtUint64(uint64(f), verb, goSyntax) + p.fmtUint64(uint64(f), verb) case uint8: - p.fmtUint64(uint64(f), verb, goSyntax) + p.fmtUint64(uint64(f), verb) case uint16: - p.fmtUint64(uint64(f), verb, goSyntax) + p.fmtUint64(uint64(f), verb) case uint32: - p.fmtUint64(uint64(f), verb, goSyntax) + p.fmtUint64(uint64(f), verb) case uint64: - p.fmtUint64(f, verb, goSyntax) + p.fmtUint64(f, verb) case uintptr: - p.fmtUint64(uint64(f), verb, goSyntax) + p.fmtUint64(uint64(f), verb) case string: - p.fmtString(f, verb, goSyntax) + p.fmtString(f, verb) wasString = verb == 's' || verb == 'v' case []byte: - p.fmtBytes(f, verb, goSyntax, nil, depth) + p.fmtBytes(f, verb, nil, depth) wasString = verb == 's' default: // Restore flags in case handleMethods finds a Formatter. p.fmt.plus = oldPlus - p.fmt.sharp = oldSharp // If the type is not simple, it might have methods. - if isString, handled := p.handleMethods(verb, plus, goSyntax, depth); handled { - return isString + if handled := p.handleMethods(verb, plus, depth); handled { + return false } // Need to use reflection - return p.printReflectValue(reflect.ValueOf(arg), verb, plus, goSyntax, depth) + return p.printReflectValue(reflect.ValueOf(arg), verb, plus, depth) } p.arg = nil return } // printValue is like printArg but starts with a reflect value, not an interface{} value. -func (p *pp) printValue(value reflect.Value, verb rune, plus, goSyntax bool, depth int) (wasString bool) { +func (p *pp) printValue(value reflect.Value, verb rune, plus bool, depth int) (wasString bool) { if !value.IsValid() { if verb == 'T' || verb == 'v' { p.buf.Write(nilAngleBytes) @@ -812,10 +823,10 @@ func (p *pp) printValue(value reflect.Value, verb rune, plus, goSyntax bool, dep // %T (the value's type) and %p (its address) are special; we always do them first. switch verb { case 'T': - p.printArg(value.Type().String(), 's', false, false, 0) + p.printArg(value.Type().String(), 's', false, 0) return false case 'p': - p.fmtPointer(value, verb, goSyntax) + p.fmtPointer(value, verb) return false } @@ -825,18 +836,18 @@ func (p *pp) printValue(value reflect.Value, verb rune, plus, goSyntax bool, dep if value.CanInterface() { p.arg = value.Interface() } - if isString, handled := p.handleMethods(verb, plus, goSyntax, depth); handled { - return isString + if handled := p.handleMethods(verb, plus, depth); handled { + return false } - return p.printReflectValue(value, verb, plus, goSyntax, depth) + return p.printReflectValue(value, verb, plus, depth) } var byteType = reflect.TypeOf(byte(0)) // printReflectValue is the fallback for both printArg and printValue. // It uses reflect to print the value. -func (p *pp) printReflectValue(value reflect.Value, verb rune, plus, goSyntax bool, depth int) (wasString bool) { +func (p *pp) printReflectValue(value reflect.Value, verb rune, plus bool, depth int) (wasString bool) { oldValue := p.value p.value = value BigSwitch: @@ -846,7 +857,7 @@ BigSwitch: case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: p.fmtInt64(f.Int(), verb) case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr: - p.fmtUint64(f.Uint(), verb, goSyntax) + p.fmtUint64(f.Uint(), verb) case reflect.Float32, reflect.Float64: if f.Type().Size() == 4 { p.fmtFloat32(float32(f.Float()), verb) @@ -860,9 +871,9 @@ BigSwitch: p.fmtComplex128(f.Complex(), verb) } case reflect.String: - p.fmtString(f.String(), verb, goSyntax) + p.fmtString(f.String(), verb) case reflect.Map: - if goSyntax { + if p.fmt.sharpV { p.buf.WriteString(f.Type().String()) if f.IsNil() { p.buf.WriteString("(nil)") @@ -875,23 +886,23 @@ BigSwitch: keys := f.MapKeys() for i, key := range keys { if i > 0 { - if goSyntax { + if p.fmt.sharpV { p.buf.Write(commaSpaceBytes) } else { p.buf.WriteByte(' ') } } - p.printValue(key, verb, plus, goSyntax, depth+1) + p.printValue(key, verb, plus, depth+1) p.buf.WriteByte(':') - p.printValue(f.MapIndex(key), verb, plus, goSyntax, depth+1) + p.printValue(f.MapIndex(key), verb, plus, depth+1) } - if goSyntax { + if p.fmt.sharpV { p.buf.WriteByte('}') } else { p.buf.WriteByte(']') } case reflect.Struct: - if goSyntax { + if p.fmt.sharpV { p.buf.WriteString(value.Type().String()) } p.add('{') @@ -899,32 +910,32 @@ BigSwitch: t := v.Type() for i := 0; i < v.NumField(); i++ { if i > 0 { - if goSyntax { + if p.fmt.sharpV { p.buf.Write(commaSpaceBytes) } else { p.buf.WriteByte(' ') } } - if plus || goSyntax { + if plus || p.fmt.sharpV { if f := t.Field(i); f.Name != "" { p.buf.WriteString(f.Name) p.buf.WriteByte(':') } } - p.printValue(getField(v, i), verb, plus, goSyntax, depth+1) + p.printValue(getField(v, i), verb, plus, depth+1) } p.buf.WriteByte('}') case reflect.Interface: value := f.Elem() if !value.IsValid() { - if goSyntax { + if p.fmt.sharpV { p.buf.WriteString(f.Type().String()) p.buf.Write(nilParenBytes) } else { p.buf.Write(nilAngleBytes) } } else { - wasString = p.printValue(value, verb, plus, goSyntax, depth+1) + wasString = p.printValue(value, verb, plus, depth+1) } case reflect.Array, reflect.Slice: // Byte slices are special: @@ -947,11 +958,11 @@ BigSwitch: bytes[i] = byte(f.Index(i).Uint()) } } - p.fmtBytes(bytes, verb, goSyntax, typ, depth) + p.fmtBytes(bytes, verb, typ, depth) wasString = verb == 's' break } - if goSyntax { + if p.fmt.sharpV { p.buf.WriteString(value.Type().String()) if f.Kind() == reflect.Slice && f.IsNil() { p.buf.WriteString("(nil)") @@ -963,15 +974,15 @@ BigSwitch: } for i := 0; i < f.Len(); i++ { if i > 0 { - if goSyntax { + if p.fmt.sharpV { p.buf.Write(commaSpaceBytes) } else { p.buf.WriteByte(' ') } } - p.printValue(f.Index(i), verb, plus, goSyntax, depth+1) + p.printValue(f.Index(i), verb, plus, depth+1) } - if goSyntax { + if p.fmt.sharpV { p.buf.WriteByte('}') } else { p.buf.WriteByte(']') @@ -984,17 +995,17 @@ BigSwitch: switch a := f.Elem(); a.Kind() { case reflect.Array, reflect.Slice: p.buf.WriteByte('&') - p.printValue(a, verb, plus, goSyntax, depth+1) + p.printValue(a, verb, plus, depth+1) break BigSwitch case reflect.Struct: p.buf.WriteByte('&') - p.printValue(a, verb, plus, goSyntax, depth+1) + p.printValue(a, verb, plus, depth+1) break BigSwitch } } fallthrough case reflect.Chan, reflect.Func, reflect.UnsafePointer: - p.fmtPointer(value, verb, goSyntax) + p.fmtPointer(value, verb) default: p.unknownType(f) } @@ -1160,9 +1171,13 @@ func (p *pp) doPrintf(format string, a []interface{}) { arg := a[argNum] argNum++ - goSyntax := c == 'v' && p.fmt.sharp + if c == 'v' && p.fmt.sharp { + // Go syntax. Set the flag in the fmt and clear the sharp flag. + p.fmt.sharp = false + p.fmt.sharpV = true + } plus := c == 'v' && p.fmt.plus - p.printArg(arg, c, plus, goSyntax, 0) + p.printArg(arg, c, plus, 0) } // Check for extra arguments unless the call accessed the arguments @@ -1176,7 +1191,7 @@ func (p *pp) doPrintf(format string, a []interface{}) { p.buf.WriteString(reflect.TypeOf(arg).String()) p.buf.WriteByte('=') } - p.printArg(arg, 'v', false, false, 0) + p.printArg(arg, 'v', false, 0) if argNum+1 < len(a) { p.buf.Write(commaSpaceBytes) } @@ -1197,7 +1212,7 @@ func (p *pp) doPrint(a []interface{}, addspace, addnewline bool) { p.buf.WriteByte(' ') } } - prevString = p.printArg(arg, 'v', false, false, 0) + prevString = p.printArg(arg, 'v', false, 0) } if addnewline { p.buf.WriteByte('\n') -- cgit v1.2.1