json: ImpliedType rejects duplicate property names of different types

CHANGELOG.md

@@ -1,5 +1,6 @@
 # 1.16.2 (Unreleased)
 
+* `json`: `ImpliedType` now returns an error if a JSON object contains two properties of the same name. As a compatibility concession it allows duplicates whose values have the same implied type, since it was unintentionally possible to combine `ImpliedType` and `Unmarshal` successfully in that case before, but this is not an endorsement of using duplicate property names since that makes the input ambiguous in any case. ([#199](https://github.com/zclconf/go-cty/issues/199))
 * `function/stdlib`: `ElementFunc` no longer crashes when asked for a negative index into a tuple. This fixes a miss in the negative index support added back in v1.15.0. ([#200](https://github.com/zclconf/go-cty/pull/200))
 
 # 1.16.1 (January 13, 2025)

cty/json/type_implied.go

@@ -127,6 +127,29 @@ func impliedObjectType(dec *json.Decoder) (cty.Type, error) {
 		if atys == nil {
 			atys = make(map[string]cty.Type)
 		}
+		if existing, exists := atys[key]; exists {
+			// We didn't originally have any special treatment for multiple properties
+			// of the same name, having the type of the last one "win". But that caused
+			// some confusing error messages when the same input was subsequently used
+			// with [Unmarshal] using the returned object type, since [Unmarshal] would
+			// try to fit all of the property values of that name to whatever type
+			// the last one had, and would likely fail in doing so if the earlier
+			// properties of the same name had different types.
+			//
+			// As a compromise to avoid breaking existing successful use of _consistently-typed_
+			// redundant properties, we return an error here only if the new type
+			// differs from the old type. The error message doesn't mention that subtlety
+			// because the equal type carveout is a compatibility concession rather than
+			// a feature folks should rely on in new code.
+			if !existing.Equals(aty) {
+				// This error message is low-quality because ImpliedType doesn't do
+				// path tracking while it traverses, unlike Unmarshal. However, this
+				// is a rare enough case that we don't want to pay the cost of allocating
+				// another path-tracking buffer that would in most cases be ignored,
+				// so we just accept a low-context error message. :(
+				return cty.NilType, fmt.Errorf("duplicate %q property in JSON object", key)
+			}
+		}
 		atys[key] = aty
 	}
 

cty/json/type_implied_test.go

@@ -91,6 +91,12 @@ func TestImpliedType(t *testing.T) {
 				}),
 			}),
 		},
+		{
+			`{"a": "hello", "a": "world"}`,
+			cty.Object(map[string]cty.Type{
+				"a": cty.String,
+			}),
+		},
 	}
 
 	for _, test := range tests {
@@ -110,3 +116,44 @@ func TestImpliedType(t *testing.T) {
 		})
 	}
 }
+
+func TestImpliedTypeErrors(t *testing.T) {
+	tests := []struct {
+		Input     string
+		WantError string
+	}{
+		{
+			`{"a": "hello", "a": true}`,
+			`duplicate "a" property in JSON object`,
+		},
+		{
+			`{}boop`,
+			`extraneous data after JSON object`,
+		},
+		{
+			`[!]`,
+			`invalid character '!' looking for beginning of value`,
+		},
+		{
+			`[}`,
+			`invalid character '}' looking for beginning of value`,
+		},
+		{
+			`{true: null}`,
+			`invalid character 't'`,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.Input, func(t *testing.T) {
+			_, err := ImpliedType([]byte(test.Input))
+			if err == nil {
+				t.Fatalf("unexpected success\nwant error: %s", err)
+			}
+
+			if got, want := err.Error(), test.WantError; got != want {
+				t.Errorf("wrong error\ngot:  %s\nwant: %s", got, want)
+			}
+		})
+	}
+}