Enforce key uniqueness in strict mode

This is similar to PR go-yaml#285 but somewhat more efficient
because it does not require a new map to be created
for every struct unmarshal.

Fixes go-yaml#284.

.travis.yml

@@ -4,6 +4,9 @@ go:
     - 1.4
     - 1.5
     - 1.6
+    - 1.7
+    - 1.8
+    - 1.9
     - tip
 
 go_import_path: gopkg.in/yaml.v2

decode.go

@@ -580,14 +580,22 @@ func (d *decoder) mapping(n *node, out reflect.Value) (good bool) {
 			}
 			e := reflect.New(et).Elem()
 			if d.unmarshal(n.children[i+1], e) {
-				out.SetMapIndex(k, e)
+				d.setMapIndex(n.children[i+1], out, k, e)
 			}
 		}
 	}
 	d.mapType = mapType
 	return true
 }
 
+func (d *decoder) setMapIndex(n *node, out, k, v reflect.Value) {
+	if d.strict && out.MapIndex(k) != zeroValue {
+		d.terrors = append(d.terrors, fmt.Sprintf("line %d: key %#v already set in map", n.line+1, k.Interface()))
+		return
+	}
+	out.SetMapIndex(k, v)
+}
+
 func (d *decoder) mappingSlice(n *node, out reflect.Value) (good bool) {
 	outt := out.Type()
 	if outt.Elem() != mapItemType {
@@ -635,6 +643,10 @@ func (d *decoder) mappingStruct(n *node, out reflect.Value) (good bool) {
 		elemType = inlineMap.Type().Elem()
 	}
 
+	var doneFields []bool
+	if d.strict {
+		doneFields = make([]bool, len(sinfo.FieldsList))
+	}
 	for i := 0; i < l; i += 2 {
 		ni := n.children[i]
 		if isMerge(ni) {
@@ -645,6 +657,13 @@ func (d *decoder) mappingStruct(n *node, out reflect.Value) (good bool) {
 			continue
 		}
 		if info, ok := sinfo.FieldsMap[name.String()]; ok {
+			if d.strict {
+				if doneFields[info.Id] {
+					d.terrors = append(d.terrors, fmt.Sprintf("line %d: field %s already set in type %s", ni.line+1, name.String(), out.Type()))
+					continue
+				}
+				doneFields[info.Id] = true
+			}
 			var field reflect.Value
 			if info.Inline == nil {
 				field = out.Field(info.Num)
@@ -658,9 +677,9 @@ func (d *decoder) mappingStruct(n *node, out reflect.Value) (good bool) {
 			}
 			value := reflect.New(elemType).Elem()
 			d.unmarshal(n.children[i+1], value)
-			inlineMap.SetMapIndex(name, value)
+			d.setMapIndex(n.children[i+1], inlineMap, name, value)
 		} else if d.strict {
-			d.terrors = append(d.terrors, fmt.Sprintf("line %d: field %s not found in struct %s", ni.line+1, name.String(), out.Type()))
+			d.terrors = append(d.terrors, fmt.Sprintf("line %d: field %s not found in type %s", ni.line+1, name.String(), out.Type()))
 		}
 	}
 	return true

decode_test.go

@@ -1065,15 +1065,87 @@ func (s *S) TestUnmarshalSliceOnPreset(c *C) {
 	c.Assert(v.A, DeepEquals, []int{2})
 }
 
+var unmarshalStrictTests = []struct {
+	data  string
+	value interface{}
+	error string
+}{{
+	data:  "a: 1\nc: 2\n",
+	value: struct{ A, B int }{A: 1},
+	error: `yaml: unmarshal errors:\n  line 2: field c not found in type struct { A int; B int }`,
+}, {
+	data:  "a: 1\nb: 2\na: 3\n",
+	value: struct{ A, B int }{A: 3, B: 2},
+	error: `yaml: unmarshal errors:\n  line 3: field a already set in type struct { A int; B int }`,
+}, {
+	data: "c: 3\na: 1\nb: 2\nc: 4\n",
+	value: struct {
+		A       int
+		inlineB `yaml:",inline"`
+	}{
+		A: 1,
+		inlineB: inlineB{
+			B: 2,
+			inlineC: inlineC{
+				C: 4,
+			},
+		},
+	},
+	error: `yaml: unmarshal errors:\n  line 4: field c already set in type struct { A int; yaml_test.inlineB "yaml:\\",inline\\"" }`,
+}, {
+	data: "c: 0\na: 1\nb: 2\nc: 1\n",
+	value: struct {
+		A       int
+		inlineB `yaml:",inline"`
+	}{
+		A: 1,
+		inlineB: inlineB{
+			B: 2,
+			inlineC: inlineC{
+				C: 1,
+			},
+		},
+	},
+	error: `yaml: unmarshal errors:\n  line 4: field c already set in type struct { A int; yaml_test.inlineB "yaml:\\",inline\\"" }`,
+}, {
+	data: "c: 1\na: 1\nb: 2\nc: 3\n",
+	value: struct {
+		A int
+		M map[string]interface{} `yaml:",inline"`
+	}{
+		A: 1,
+		M: map[string]interface{}{
+			"b": 2,
+			"c": 3,
+		},
+	},
+	error: `yaml: unmarshal errors:\n  line 4: key "c" already set in map`,
+}, {
+	data: "a: 1\n9: 2\nnull: 3\n9: 4",
+	value: map[interface{}]interface{}{
+		"a": 1,
+		nil: 3,
+		9:   4,
+	},
+	error: `yaml: unmarshal errors:\n  line 4: key 9 already set in map`,
+}}
+
 func (s *S) TestUnmarshalStrict(c *C) {
-	v := struct{ A, B int }{}
-
-	err := yaml.UnmarshalStrict([]byte("a: 1\nb: 2"), &v)
-	c.Check(err, IsNil)
-	err = yaml.Unmarshal([]byte("a: 1\nb: 2\nc: 3"), &v)
-	c.Check(err, IsNil)
-	err = yaml.UnmarshalStrict([]byte("a: 1\nb: 2\nc: 3"), &v)
-	c.Check(err, ErrorMatches, "yaml: unmarshal errors:\n  line 3: field c not found in struct struct { A int; B int }")
+	for i, item := range unmarshalStrictTests {
+		c.Logf("test %d: %q", i, item.data)
+		// First test that normal Unmarshal unmarshals to the expected value.
+		t := reflect.ValueOf(item.value).Type()
+		value := reflect.New(t)
+		err := yaml.Unmarshal([]byte(item.data), value.Interface())
+		c.Assert(err, Equals, nil)
+		c.Assert(value.Elem().Interface(), DeepEquals, item.value)
+
+		// Then test that UnmarshalStrict fails on the same thing.
+		t = reflect.ValueOf(item.value).Type()
+		value = reflect.New(t)
+		err = yaml.UnmarshalStrict([]byte(item.data), value.Interface())
+		c.Assert(err, ErrorMatches, item.error)
+	}
 }
 
 //var data []byte

yaml.go

@@ -82,7 +82,8 @@ func Unmarshal(in []byte, out interface{}) (err error) {
 }
 
 // UnmarshalStrict is like Unmarshal except that any fields that are found
-// in the data that do not have corresponding struct members will result in
+// in the data that do not have corresponding struct members, or mapping
+// keys that are duplicates, will result in
 // an error.
 func UnmarshalStrict(in []byte, out interface{}) (err error) {
 	return unmarshal(in, out, true)
@@ -105,7 +106,7 @@ func NewDecoder(r io.Reader) *Decoder {
 }
 
 // SetStrict sets whether strict decoding behaviour is enabled when
-// decoding items in the data. By default, decoding is not strict.
+// decoding items in the data (see UnmarshalStrict). By default, decoding is not strict.
 func (dec *Decoder) SetStrict(strict bool) {
 	dec.strict = strict
 }
@@ -257,6 +258,9 @@ type fieldInfo struct {
 	Num       int
 	OmitEmpty bool
 	Flow      bool
+	// Id holds the unique field identifier, so we can cheaply
+	// check for field duplicates without maintaining an extra map.
+	Id int
 
 	// Inline holds the field index if the field is part of an inlined struct.
 	Inline []int
@@ -336,6 +340,7 @@ func getStructInfo(st reflect.Type) (*structInfo, error) {
 					} else {
 						finfo.Inline = append([]int{i}, finfo.Inline...)
 					}
+					finfo.Id = len(fieldsList)
 					fieldsMap[finfo.Key] = finfo
 					fieldsList = append(fieldsList, finfo)
 				}
@@ -357,11 +362,16 @@ func getStructInfo(st reflect.Type) (*structInfo, error) {
 			return nil, errors.New(msg)
 		}
 
+		info.Id = len(fieldsList)
 		fieldsList = append(fieldsList, info)
 		fieldsMap[info.Key] = info
 	}
 
-	sinfo = &structInfo{fieldsMap, fieldsList, inlineMap}
+	sinfo = &structInfo{
+		FieldsMap:  fieldsMap,
+		FieldsList: fieldsList,
+		InlineMap:  inlineMap,
+	}
 
 	fieldMapMutex.Lock()
 	structMap[st] = sinfo