From be2cf952a87366fc85f9c6f3e06d1e9126bb11c6 Mon Sep 17 00:00:00 2001 From: Rob Pike Date: Thu, 16 Jul 2009 23:01:10 -0700 Subject: [PATCH] clean up the code, flow errors out to decoder. R=rsc DELTA=99 (32 added, 22 deleted, 45 changed) OCL=31759 CL=31759 --- src/pkg/gob/decode.go | 96 +++++++++++++++++++++++-------------- src/pkg/gob/decoder.go | 24 ++-------- src/pkg/gob/encode.go | 4 +- src/pkg/gob/encoder_test.go | 8 ++-- 4 files changed, 71 insertions(+), 61 deletions(-) diff --git a/src/pkg/gob/decode.go b/src/pkg/gob/decode.go index 2fbcf68b3e..659ab68d24 100644 --- a/src/pkg/gob/decode.go +++ b/src/pkg/gob/decode.go @@ -17,6 +17,11 @@ import ( "unsafe"; ) +var ( + ErrRange = os.ErrorString("gob: internal error: field numbers out of bounds"); + ErrNotStruct = os.ErrorString("gob: TODO: can only handle structs") +) + // The global execution state of an instance of the decoder. type decodeState struct { b *bytes.Buffer; @@ -342,7 +347,8 @@ func decodeStruct(engine *decEngine, rtyp *reflect.StructType, b *bytes.Buffer, } fieldnum := state.fieldnum + delta; if fieldnum >= len(engine.instr) { - panicln("TODO(r): field number out of range", fieldnum, len(engine.instr)); + state.err = ErrRange; + break; } instr := &engine.instr[fieldnum]; p := unsafe.Pointer(basep+instr.offset); @@ -452,11 +458,11 @@ var decIgnoreOpMap = map[TypeId] decOp { tString: ignoreUint8Array, } -func getDecEnginePtr(wireId TypeId, rt reflect.Type) **decEngine +func getDecEnginePtr(wireId TypeId, rt reflect.Type) (enginePtr **decEngine, err os.Error) // Return the decoding op for the base type under rt and // the indirection count to reach it. -func decOpFor(wireId TypeId, rt reflect.Type) (decOp, int) { +func decOpFor(wireId TypeId, rt reflect.Type) (decOp, int, os.Error) { typ, indir := indirect(rt); op, ok := decOpMap[reflect.Typeof(typ)]; if !ok { @@ -468,21 +474,30 @@ func decOpFor(wireId TypeId, rt reflect.Type) (decOp, int) { break; } elemId := wireId.gobType().(*sliceType).Elem; - elemOp, elemIndir := decOpFor(elemId, t.Elem()); + elemOp, elemIndir, err := decOpFor(elemId, t.Elem()); + if err != nil { + return nil, 0, err + } op = func(i *decInstr, state *decodeState, p unsafe.Pointer) { state.err = decodeSlice(t, state, uintptr(p), elemOp, t.Elem().Size(), i.indir, elemIndir); }; case *reflect.ArrayType: elemId := wireId.gobType().(*arrayType).Elem; - elemOp, elemIndir := decOpFor(elemId, t.Elem()); + elemOp, elemIndir, err := decOpFor(elemId, t.Elem()); + if err != nil { + return nil, 0, err + } op = func(i *decInstr, state *decodeState, p unsafe.Pointer) { state.err = decodeArray(t, state, uintptr(p), elemOp, t.Elem().Size(), t.Len(), i.indir, elemIndir); }; case *reflect.StructType: // Generate a closure that calls out to the engine for the nested type. - enginePtr := getDecEnginePtr(wireId, typ); + enginePtr, err := getDecEnginePtr(wireId, typ); + if err != nil { + return nil, 0, err + } op = func(i *decInstr, state *decodeState, p unsafe.Pointer) { // indirect through info to delay evaluation for recursive structs state.err = decodeStruct(*enginePtr, t, state.b, uintptr(p), i.indir) @@ -490,27 +505,33 @@ func decOpFor(wireId TypeId, rt reflect.Type) (decOp, int) { } } if op == nil { - panicln("decode can't handle type", rt.String()); + return nil, 0, os.ErrorString("gob: decode can't handle type " + rt.String()); } - return op, indir + return op, indir, nil } // Return the decoding op for a field that has no destination. -func decIgnoreOpFor(wireId TypeId) decOp { +func decIgnoreOpFor(wireId TypeId) (decOp, os.Error) { op, ok := decIgnoreOpMap[wireId]; if !ok { // Special cases switch t := wireId.gobType().(type) { case *sliceType: elemId := wireId.gobType().(*sliceType).Elem; - elemOp := decIgnoreOpFor(elemId); + elemOp, err := decIgnoreOpFor(elemId); + if err != nil { + return nil, err + } op = func(i *decInstr, state *decodeState, p unsafe.Pointer) { state.err = ignoreSlice(state, elemOp); }; case *arrayType: elemId := wireId.gobType().(*arrayType).Elem; - elemOp := decIgnoreOpFor(elemId); + elemOp, err := decIgnoreOpFor(elemId); + if err != nil { + return nil, err + } op = func(i *decInstr, state *decodeState, p unsafe.Pointer) { state.err = ignoreArray(state, elemOp, t.Len); }; @@ -520,9 +541,9 @@ func decIgnoreOpFor(wireId TypeId) decOp { } } if op == nil { - panicln("decode can't handle type", wireId.gobType().String()); + return nil, os.ErrorString("ignore can't handle type " + wireId.String()); } - return op; + return op, nil; } // Are these two gob Types compatible? @@ -588,48 +609,46 @@ func compatibleType(fr reflect.Type, fw TypeId) bool { return true; } -func compileDec(wireId TypeId, rt reflect.Type) *decEngine { +func compileDec(wireId TypeId, rt reflect.Type) (engine *decEngine, err os.Error) { srt, ok1 := rt.(*reflect.StructType); wireStruct, ok2 := wireId.gobType().(*structType); if !ok1 || !ok2 { - panicln("gob: TODO: can't handle non-structs"); + return nil, ErrNotStruct } - engine := new(decEngine); + engine = new(decEngine); engine.instr = make([]decInstr, len(wireStruct.field)); // Loop over the fields of the wire type. for fieldnum := 0; fieldnum < len(wireStruct.field); fieldnum++ { wireField := wireStruct.field[fieldnum]; // Find the field of the local type with the same name. - // TODO: put this as a method in reflect - var localField reflect.StructField; - for lfn := 0; lfn < srt.NumField(); lfn++ { - if srt.Field(lfn).Name == wireField.name { - localField = srt.Field(lfn); - break; - } - } + localField, present := srt.FieldByName(wireField.name); // TODO(r): anonymous names - if localField.Anonymous || localField.Name == "" { + if !present || localField.Anonymous { println("no matching field", wireField.name, "in type", wireId.String()); - op := decIgnoreOpFor(wireField.typeId); + op, err := decIgnoreOpFor(wireField.typeId); + if err != nil { + return nil, err + } engine.instr[fieldnum] = decInstr{op, fieldnum, 0, 0}; continue; } if !compatibleType(localField.Type, wireField.typeId) { - panicln("TODO: wrong type for field", wireField.name, "in type", wireId.String()); + return nil, os.ErrorString("gob: TODO: wrong type for field " + wireField.name + " in type " + wireId.String()); + } + op, indir, err := decOpFor(wireField.typeId, localField.Type); + if err != nil { + return nil, err } - op, indir := decOpFor(wireField.typeId, localField.Type); engine.instr[fieldnum] = decInstr{op, fieldnum, indir, uintptr(localField.Offset)}; engine.numInstr++; } - return engine; + return; } // typeLock must be held. -func getDecEnginePtr(wireId TypeId, rt reflect.Type) **decEngine { +func getDecEnginePtr(wireId TypeId, rt reflect.Type) (enginePtr **decEngine, err os.Error) { info := getTypeInfo(rt); // TODO: eliminate this; creates a gobType you don't need. - var enginePtr **decEngine; var ok bool; if enginePtr, ok = info.decoderPtr[wireId]; !ok { if info.typeId.gobType() == nil { @@ -639,9 +658,12 @@ func getDecEnginePtr(wireId TypeId, rt reflect.Type) **decEngine { // mark this engine as underway before compiling to handle recursive types. enginePtr = new(*decEngine); info.decoderPtr[wireId] = enginePtr; - *enginePtr = compileDec(wireId, rt); + *enginePtr, err = compileDec(wireId, rt); + if err != nil { + info.decoderPtr[wireId] = nil, false; + } } - return enginePtr + return } func decode(b *bytes.Buffer, wireId TypeId, e interface{}) os.Error { @@ -657,11 +679,15 @@ func decode(b *bytes.Buffer, wireId TypeId, e interface{}) os.Error { return os.ErrorString("gob: decode can't handle " + rt.String()) } typeLock.Lock(); - engine := *getDecEnginePtr(wireId, rt); + enginePtr, err := getDecEnginePtr(wireId, rt); typeLock.Unlock(); + if err != nil { + return err + } + engine := *enginePtr; if engine.numInstr == 0 && st.NumField() > 0 { path, name := rt.Name(); - return os.ErrorString("no fields matched compiling decoder for " + name) + return os.ErrorString("type mismatch: no fields matched compiling decoder for " + name) } return decodeStruct(engine, rt.(*reflect.StructType), b, uintptr(v.Addr()), 0); } diff --git a/src/pkg/gob/decoder.go b/src/pkg/gob/decoder.go index 9c5a755542..609a20484c 100644 --- a/src/pkg/gob/decoder.go +++ b/src/pkg/gob/decoder.go @@ -83,7 +83,7 @@ func (dec *Decoder) Decode(e interface{}) os.Error { // Receive a type id. id := TypeId(decodeInt(dec.state)); if dec.state.err != nil { - return dec.state.err + break; } // Is it a new type? @@ -91,28 +91,14 @@ func (dec *Decoder) Decode(e interface{}) os.Error { // If the id is negative, we have a type. dec.recvType(-id); if dec.state.err != nil { - return dec.state.err + break; } continue; } // No, it's a value. - typeLock.Lock(); - info := getTypeInfo(rt); - typeLock.Unlock(); - - // Check type compatibility. - // TODO(r): need to make the decoder work correctly if the wire type is compatible - // but not equal to the local type (e.g, extra fields). - if info.wire.name() != dec.seen[id].name() { - dec.state.err = os.ErrorString("gob decode: incorrect type for wire value: want " + info.wire.name() + "; received " + dec.seen[id].name()); - return dec.state.err - } - - // Receive a value. - decode(dec.state.b, id, e); - - return dec.state.err + dec.state.err = decode(dec.state.b, id, e); + break; } - return nil // silence compiler + return dec.state.err } diff --git a/src/pkg/gob/encode.go b/src/pkg/gob/encode.go index 7f12658145..bfa2d69050 100644 --- a/src/pkg/gob/encode.go +++ b/src/pkg/gob/encode.go @@ -372,7 +372,7 @@ func encOpFor(rt reflect.Type) (encOp, int) { } } if op == nil { - panicln("encode can't handle type", rt.String()); + panicln("can't happen: encode type", rt.String()); } return op, indir } @@ -381,7 +381,7 @@ func encOpFor(rt reflect.Type) (encOp, int) { func compileEnc(rt reflect.Type) *encEngine { srt, ok := rt.(*reflect.StructType); if !ok { - panicln("TODO: can't handle non-structs"); + panicln("can't happen: non-struct"); } engine := new(encEngine); engine.instr = make([]encInstr, srt.NumField()+1); // +1 for terminator diff --git a/src/pkg/gob/encoder_test.go b/src/pkg/gob/encoder_test.go index 5acb310c75..c261376ef1 100644 --- a/src/pkg/gob/encoder_test.go +++ b/src/pkg/gob/encoder_test.go @@ -38,11 +38,9 @@ type ET4 struct { next *ET2; } -// Like ET1 but with a different type for a self-referencing field +// Has different type for a self-referencing field compared to ET1 type ET5 struct { - a int; - et2 *ET2; - next *ET1; + next *ET2; } func TestBasicEncoder(t *testing.T) { @@ -227,7 +225,7 @@ func badTypeCheck(e interface{}, msg string, t *testing.T) { // Test that we recognize a bad type the first time. func TestWrongTypeDecoder(t *testing.T) { - badTypeCheck(new(ET2), "different number of fields", t); + badTypeCheck(new(ET2), "no fields in common", t); badTypeCheck(new(ET3), "different name of field", t); badTypeCheck(new(ET4), "different type of field", t); badTypeCheck(new(ET5), "different type of self-reference field", t);