[stdlib] Dictionary.updateValue(_:,forKey:): Don’t overwrite the existing key

Replacing the old key with the new is unnecessary and somewhat surprising. It is also harmful to some usecases.

rdar://problem/32144087

Author: lorentey

stdlib/public/core/NativeDictionary.swift

@@ -410,10 +410,6 @@ extension _NativeDictionary { // Insertions
     if found {
       let oldValue = (_values + index.bucket).move()
       (_values + index.bucket).initialize(to: value)
-      // FIXME: Replacing the old key with the new is unnecessary, unintuitive,
-      // and actively harmful to some usecases. We shouldn't do it.
-      // rdar://problem/32144087
-      (_keys + index.bucket).pointee = key
       return oldValue
     }
     _insert(at: index, key: key, value: value)
@@ -429,10 +425,6 @@ extension _NativeDictionary { // Insertions
     let (index, found) = mutatingFind(key, isUnique: isUnique)
     if found {
       (_values + index.bucket).pointee = value
-      // FIXME: Replacing the old key with the new is unnecessary, unintuitive,
-      // and actively harmful to some usecases. We shouldn't do it.
-      // rdar://problem/32144087
-      (_keys + index.bucket).pointee = key
     } else {
       _insert(at: index, key: key, value: value)
     }

validation-test/stdlib/Dictionary.swift

@@ -411,7 +411,7 @@ DictionaryTestSuite.test("COW.Fast.UpdateValueForKeyDoesNotReallocate") {
   }
 }
 
-DictionaryTestSuite.test("COW.Slow.AddDoesNotReallocate") {
+DictionaryTestSuite.test("COW.Slow.UpdateValueForKeyDoesNotReallocate") {
   do {
     var d1 = getCOWSlowDictionary()
     let identity1 = d1._rawIdentifier()
@@ -4642,6 +4642,32 @@ DictionaryTestSuite.test("removeAt") {
   }
 }
 
+DictionaryTestSuite.test("updateValue") {
+  let key1 = TestKeyTy(42)
+  let key2 = TestKeyTy(42)
+  let value1 = TestValueTy(1)
+  let value2 = TestValueTy(2)
+
+  var d: [TestKeyTy: TestValueTy] = [:]
+
+  expectNil(d.updateValue(value1, forKey: key1))
+
+  expectEqual(d.count, 1)
+  let index1 = d.index(forKey: key2)
+  expectNotNil(index1)
+  expectTrue(d[index1!].key === key1)
+  expectTrue(d[index1!].value === value1)
+
+  expectTrue(d.updateValue(value2, forKey: key2) === value1)
+
+  expectEqual(d.count, 1)
+  let index2 = d.index(forKey: key2)
+  expectEqual(index1, index2)
+  // We expect updateValue to keep the original key in place.
+  expectTrue(d[index2!].key === key1) // Not key2
+  expectTrue(d[index2!].value === value2)
+}
+
 DictionaryTestSuite.test("localHashSeeds") {
   // With global hashing, copying elements in hash order between hash tables
   // can become quadratic. (See https://bugs.swift.org/browse/SR-3268)