[stdlib] Deprecate MutableCollection._withUnsafeMutableBufferPointerIfSupported (#36003)

lorentey · natecook1000 · web-flow · commit 083670730368 · 2021-02-26T11:32:27.000-08:00
* [stdlib] Deprecate MutableCollection._withUnsafeMutableBufferPointerIfSupported In Swift 5.0, [SE-0237] introduced the public `MutableCollection.withContiguousMutableStorageIfAvailable` method. It’s time we migrated off the old, underscored variant and deprecated it. The default `MutableCollection.sort` and `.partition(by:)` implementations are currently calling this hidden method rather than the documented interface, preventing custom Collection implementations from achieving good performance, even if they have contiguous storage. [SE-0237]: https://github.com/apple/swift-evolution/blob/master/proposals/0237-contiguous-collection.md * [test] Update tests for stdlib behavior changes * Update stdlib/private/StdlibCollectionUnittest/CheckMutableCollectionType.swift Co-authored-by: Nate Cook <natecook@apple.com> * Update stdlib/private/StdlibCollectionUnittest/CheckMutableCollectionType.swift Co-authored-by: Nate Cook <natecook@apple.com> * Apply suggestions from code review Co-authored-by: Nate Cook <natecook@apple.com> * [test] LoggingMutableCollection: Fix logging targets * [stdlib] Fix warning by restoring original workaround Co-authored-by: Nate Cook <natecook@apple.com>
diff --git a/stdlib/private/StdlibCollectionUnittest/CheckMutableCollectionType.swift b/stdlib/private/StdlibCollectionUnittest/CheckMutableCollectionType.swift
@@ -512,7 +512,7 @@ func checkSortedPredicateThrow(
   }
 }
 
-self.test("\(testNamePrefix).sorted/DispatchesThrough_withUnsafeMutableBufferPointerIfSupported/WhereElementIsComparable") {
+self.test("\(testNamePrefix).sorted/DispatchesThroughDirectStorageAccessors/WhereElementIsComparable") {
   let sequence = [ 5, 4, 3, 2, 1 ]
   let elements: [MinimalComparableValue] =
     zip(sequence, 0..<sequence.count).map {
@@ -525,13 +525,16 @@ self.test("\(testNamePrefix).sorted/DispatchesThrough_withUnsafeMutableBufferPoi
   let result = lc.sorted()
   let extractedResult = result.map(extractValueFromComparable)
 
+  let actualWUMBPIF = lc.log._withUnsafeMutableBufferPointerIfSupported[type(of: lc)]
+  let actualWCMSIA =  lc.log.withContiguousMutableStorageIfAvailable[type(of: lc)]
+
+  let actualWUMBPIFNonNil = lc.log._withUnsafeMutableBufferPointerIfSupportedNonNilReturns[type(of: lc)]
+  let actualWCMSIANonNil =  lc.log.withContiguousMutableStorageIfAvailableNonNilReturns[type(of: lc)]
+
   // This sort operation is not in-place.
   // The collection is copied into an array before sorting.
-  expectEqual(
-    0, lc.log._withUnsafeMutableBufferPointerIfSupported[type(of: lc)])
-  expectEqual(
-    0,
-    lc.log._withUnsafeMutableBufferPointerIfSupportedNonNilReturns[type(of: lc)])
+  expectEqual(0, actualWUMBPIF + actualWCMSIA)
+  expectEqual(0, actualWUMBPIFNonNil + actualWCMSIANonNil)
 
   expectEqualSequence([ 1, 2, 3, 4, 5 ], extractedResult.map { $0.value })
 }
@@ -631,7 +634,7 @@ self.test("\(testNamePrefix).sorted/ThrowingPredicateWithLargeNumberElements") {
 }
 
 
-self.test("\(testNamePrefix).sorted/DispatchesThrough_withUnsafeMutableBufferPointerIfSupported/Predicate") {
+self.test("\(testNamePrefix).sorted/DispatchesThroughDirectStorageAccessors/Predicate") {
   let sequence = [ 5, 4, 3, 2, 1 ]
   let elements: [OpaqueValue<Int>] =
     zip(sequence, 0..<sequence.count).map {
@@ -644,13 +647,16 @@ self.test("\(testNamePrefix).sorted/DispatchesThrough_withUnsafeMutableBufferPoi
   let result = lc.sorted { extractValue($0).value < extractValue($1).value }
   let extractedResult = result.map(extractValue)
 
+  let actualWUMBPIF = lc.log._withUnsafeMutableBufferPointerIfSupported[type(of: lc)]
+  let actualWCMSIA =  lc.log.withContiguousMutableStorageIfAvailable[type(of: lc)]
+
+  let actualWUMBPIFNonNil = lc.log._withUnsafeMutableBufferPointerIfSupportedNonNilReturns[type(of: lc)]
+  let actualWCMSIAIFNonNil =  lc.log.withContiguousMutableStorageIfAvailableNonNilReturns[type(of: lc)]
+
   // This sort operation is not in-place.
   // The collection is copied into an array before sorting.
-  expectEqual(
-    0, lc.log._withUnsafeMutableBufferPointerIfSupported[type(of: lc)])
-  expectEqual(
-    0,
-    lc.log._withUnsafeMutableBufferPointerIfSupportedNonNilReturns[type(of: lc)])
+  expectEqual(0, actualWUMBPIF + actualWCMSIA)
+  expectEqual(0, actualWUMBPIFNonNil + actualWCMSIAIFNonNil)
 
   expectEqualSequence([ 1, 2, 3, 4, 5 ], extractedResult.map { $0.value })
 }
@@ -949,7 +955,7 @@ self.test("\(testNamePrefix).reverse()") {
 // partition(by:)
 //===----------------------------------------------------------------------===//
 
-self.test("\(testNamePrefix).partition/DispatchesThrough_withUnsafeMutableBufferPointerIfSupported") {
+self.test("\(testNamePrefix).partition/DispatchesThroughDirectStorageAccessors") {
   let sequence = [ 5, 4, 3, 2, 1 ]
   let elements: [OpaqueValue<Int>] =
     zip(sequence, 0..<sequence.count).map {
@@ -965,11 +971,23 @@ self.test("\(testNamePrefix).partition/DispatchesThrough_withUnsafeMutableBuffer
     return !(extractValue(val).value < extractValue(first!).value)
   })
 
-  expectEqual(
-    1, lc.log._withUnsafeMutableBufferPointerIfSupported[type(of: lc)])
+  let actualWUMBPIF = lc.log._withUnsafeMutableBufferPointerIfSupported[type(of: lc)]
+  let actualWCMSIA =  lc.log.withContiguousMutableStorageIfAvailable[type(of: lc)]
+
+  let actualWUMBPIFNonNil = lc.log._withUnsafeMutableBufferPointerIfSupportedNonNilReturns[type(of: lc)]
+  let actualWCMSIAIFNonNil =  lc.log.withContiguousMutableStorageIfAvailableNonNilReturns[type(of: lc)]
+
+  expectEqual(1, actualWUMBPIF + actualWCMSIA)
   expectEqual(
     withUnsafeMutableBufferPointerIsSupported ? 1 : 0,
-    lc.log._withUnsafeMutableBufferPointerIfSupportedNonNilReturns[type(of: lc)])
+    actualWUMBPIFNonNil + actualWCMSIAIFNonNil)
+
+  if #available(macOS 9999, iOS 9999, watchOS 9999, tvOS 9999, *) {
+    // `partition(by:)` is expected to dispatch to the public API in releases
+    // that contain https://github.com/apple/swift/pull/36003.
+    expectEqual(0, actualWUMBPIF)
+    expectEqual(0, actualWUMBPIFNonNil)
+  }
 
   expectEqual(4, lc.distance(from: lc.startIndex, to: pivot))
   expectEqualsUnordered([1, 2, 3, 4], lc.prefix(upTo: pivot).map { extractValue($0).value })
@@ -1279,4 +1297,3 @@ self.test("\(testNamePrefix).sort/ThrowingPredicateWithLargeNumberElements") {
 
   } // addMutableRandomAccessCollectionTests
 }
-
diff --git a/stdlib/private/StdlibCollectionUnittest/LoggingWrappers.swift b/stdlib/private/StdlibCollectionUnittest/LoggingWrappers.swift
@@ -404,7 +404,7 @@ extension LoggingMutableCollection: MutableCollection {
     MutableCollectionLog.withContiguousMutableStorageIfAvailable[selfType] += 1
     let result = try base.withContiguousMutableStorageIfAvailable(body)
     if result != nil {
-      Log.withContiguousMutableStorageIfAvailable[selfType] += 1
+      Log.withContiguousMutableStorageIfAvailableNonNilReturns[selfType] += 1
     }
     return result
   }
@@ -618,7 +618,7 @@ extension BufferAccessLoggingMutableCollection: MutableCollection {
     Log.withContiguousMutableStorageIfAvailable[selfType] += 1
     let result = try base.withContiguousMutableStorageIfAvailable(body)
     if result != nil {
-      Log.withContiguousMutableStorageIfAvailable[selfType] += 1
+      Log.withContiguousMutableStorageIfAvailableNonNilReturns[selfType] += 1
     }
     return result
   }
diff --git a/stdlib/public/core/Array.swift b/stdlib/public/core/Array.swift
@@ -1356,6 +1356,7 @@ extension Array: RangeReplaceableCollection {
   //===--- algorithms -----------------------------------------------------===//
 
   @inlinable
+  @available(*, deprecated, renamed: "withContiguousMutableStorageIfAvailable")
   public mutating func _withUnsafeMutableBufferPointerIfSupported<R>(
     _ body: (inout UnsafeMutableBufferPointer<Element>) throws -> R
   ) rethrows -> R? {
diff --git a/stdlib/public/core/ArraySlice.swift b/stdlib/public/core/ArraySlice.swift
@@ -1079,6 +1079,7 @@ extension ArraySlice: RangeReplaceableCollection {
   //===--- algorithms -----------------------------------------------------===//
 
   @inlinable
+  @available(*, deprecated, renamed: "withContiguousMutableStorageIfAvailable")
   public mutating func _withUnsafeMutableBufferPointerIfSupported<R>(
     _ body: (inout UnsafeMutableBufferPointer<Element>) throws -> R
   ) rethrows -> R? {
diff --git a/stdlib/public/core/CollectionAlgorithms.swift b/stdlib/public/core/CollectionAlgorithms.swift
@@ -318,7 +318,7 @@ extension MutableCollection where Self: BidirectionalCollection {
   public mutating func partition(
     by belongsInSecondPartition: (Element) throws -> Bool
   ) rethrows -> Index {
-    let maybeOffset = try _withUnsafeMutableBufferPointerIfSupported {
+    let maybeOffset = try withContiguousMutableStorageIfAvailable {
       (bufferPointer) -> Int in
       let unsafeBufferPivot = try bufferPointer._partitionImpl(
         by: belongsInSecondPartition)
diff --git a/stdlib/public/core/ContiguousArray.swift b/stdlib/public/core/ContiguousArray.swift
@@ -980,6 +980,7 @@ extension ContiguousArray: RangeReplaceableCollection {
   //===--- algorithms -----------------------------------------------------===//
 
   @inlinable
+  @available(*, deprecated, renamed: "withContiguousMutableStorageIfAvailable")
   public mutating func _withUnsafeMutableBufferPointerIfSupported<R>(
     _ body: (inout UnsafeMutableBufferPointer<Element>) throws -> R
   ) rethrows -> R? {
diff --git a/stdlib/public/core/MutableCollection.swift b/stdlib/public/core/MutableCollection.swift
@@ -177,6 +177,7 @@ where SubSequence: MutableCollection
   /// within an algorithm, but when that fails, invoking the
   /// same algorithm on `body`\ 's argument lets you trade safety for
   /// speed.
+  @available(*, deprecated, renamed: "withContiguousMutableStorageIfAvailable")
   mutating func _withUnsafeMutableBufferPointerIfSupported<R>(
     _ body: (inout UnsafeMutableBufferPointer<Element>) throws -> R
   ) rethrows -> R?
@@ -199,6 +200,7 @@ where SubSequence: MutableCollection
 // TODO: swift-3-indexing-model - review the following
 extension MutableCollection {
   @inlinable
+  @available(*, deprecated, renamed: "withContiguousMutableStorageIfAvailable")
   public mutating func _withUnsafeMutableBufferPointerIfSupported<R>(
     _ body: (inout UnsafeMutableBufferPointer<Element>) throws -> R
   ) rethrows -> R? {
diff --git a/stdlib/public/core/Sort.swift b/stdlib/public/core/Sort.swift
@@ -246,10 +246,10 @@ extension MutableCollection where Self: RandomAccessCollection {
   public mutating func sort(
     by areInIncreasingOrder: (Element, Element) throws -> Bool
   ) rethrows {
-    let didSortUnsafeBuffer: Void? = try _withUnsafeMutableBufferPointerIfSupported {
-      buffer -> Void in
+    let didSortUnsafeBuffer: Void? =
+      try withContiguousMutableStorageIfAvailable { buffer in
         try buffer._stableSortImpl(by: areInIncreasingOrder)
-    }
+      }
     if didSortUnsafeBuffer == nil {
       // Fallback since we can't use an unsafe buffer: sort into an outside
       // array, then copy elements back in.
diff --git a/stdlib/public/core/UnsafeBufferPointer.swift.gyb b/stdlib/public/core/UnsafeBufferPointer.swift.gyb
@@ -446,6 +446,7 @@ extension Unsafe${Mutable}BufferPointer {
   }
 
   @inlinable
+  @available(*, deprecated, renamed: "withContiguousMutableStorageIfAvailable")
   public mutating func _withUnsafeMutableBufferPointerIfSupported<R>(
     _ body: (inout UnsafeMutableBufferPointer<Element>) throws -> R
   ) rethrows -> R? {

Original file line number	Diff line number	Diff line change
`@@ -404,7 +404,7 @@ extension LoggingMutableCollection: MutableCollection {`
`404`	`404`	`MutableCollectionLog.withContiguousMutableStorageIfAvailable[selfType] += 1`
`405`	`405`	`let result = try base.withContiguousMutableStorageIfAvailable(body)`
`406`	`406`	`if result != nil {`
`407`		`- Log.withContiguousMutableStorageIfAvailable[selfType] += 1`
	`407`	`+ Log.withContiguousMutableStorageIfAvailableNonNilReturns[selfType] += 1`
`408`	`408`	`}`
`409`	`409`	`return result`
`410`	`410`	`}`
`@@ -618,7 +618,7 @@ extension BufferAccessLoggingMutableCollection: MutableCollection {`
`618`	`618`	`Log.withContiguousMutableStorageIfAvailable[selfType] += 1`
`619`	`619`	`let result = try base.withContiguousMutableStorageIfAvailable(body)`
`620`	`620`	`if result != nil {`
`621`		`- Log.withContiguousMutableStorageIfAvailable[selfType] += 1`
	`621`	`+ Log.withContiguousMutableStorageIfAvailableNonNilReturns[selfType] += 1`
`622`	`622`	`}`
`623`	`623`	`return result`
`624`	`624`	`}`
Original file line number	Diff line number	Diff line change
`@@ -446,6 +446,7 @@ extension Unsafe${Mutable}BufferPointer {`
`446`	`446`	`}`
`447`	`447`
`448`	`448`	`@inlinable`
	`449`	`+ @available(*, deprecated, renamed: "withContiguousMutableStorageIfAvailable")`
`449`	`450`	`public mutating func _withUnsafeMutableBufferPointerIfSupported<R>(`
`450`	`451`	`_ body: (inout UnsafeMutableBufferPointer<Element>) throws -> R`
`451`	`452`	`) rethrows -> R? {`