[stdlib][SR-13883] Avoid advancing past representable bounds when striding (#34860)

* [stdlib][SR-13883] Avoid advancing past representable bounds when striding.

* [stdlib] Expand a test and add a comment to ensure correct floating-point stride bounds checking.

* [stdlib][NFC] Clarify a comment in a test.

* [stdlib][NFC] Adjust copyright notices, clarify comments, delete '-swift-version=3' for tests.

* [stdlib] Add implementations for fixed-width integer strides for performance.

* [stdlib] Document `Strideable._step` and modify overflow checking behavior of `Stride*Iterator`.

* [stdlib] Address reviewer comments, postpone documentation changes

* [stdlib][NFC] Update documentation for '_step(after:from:by:)'

* [stdlib][NFC] Use 'nil' instead of an arbitrary value for integer striding '_step' index

Author: xwu

stdlib/public/core/Stride.swift

@@ -2,7 +2,7 @@
 //
 // This source file is part of the Swift.org open source project
 //
-// Copyright (c) 2014 - 2017 Apple Inc. and the Swift project authors
+// Copyright (c) 2014 - 2021 Apple Inc. and the Swift project authors
 // Licensed under Apache License v2.0 with Runtime Library Exception
 //
 // See https://swift.org/LICENSE.txt for license information
@@ -134,15 +134,68 @@ public protocol Strideable: Comparable {
   /// If this type's `Stride` type conforms to `BinaryInteger`, then for a
   /// value `x`, a distance `n`, and a value `y = x.advanced(by: n)`,
   /// `x.distance(to: y) == n`. Using this method with types that have a
-  /// noninteger `Stride` may result in an approximation.
+  /// noninteger `Stride` may result in an approximation. If the result of
+  /// advancing by `n` is not representable as a value of this type, then a
+  /// runtime error may occur.
   ///
   /// - Parameter n: The distance to advance this value.
   /// - Returns: A value that is offset from this value by `n`.
   ///
   /// - Complexity: O(1)
   func advanced(by n: Stride) -> Self
 
-  /// `_step` is an implementation detail of Strideable; do not use it directly.
+  /// Returns the next result of striding by a specified distance.
+  ///
+  /// This method is an implementation detail of `Strideable`; do not call it
+  /// directly.
+  ///
+  /// While striding, `_step(after:from:by:)` is called at each step to
+  /// determine the next result. At the first step, the value of `current` is
+  /// `(index: 0, value: start)`. At each subsequent step, the value of
+  /// `current` is the result returned by this method in the immediately
+  /// preceding step.
+  ///
+  /// If the result of advancing by a given `distance` is not representable as a
+  /// value of this type, then a runtime error may occur.
+  ///
+  /// Implementing `_step(after:from:by:)` to Customize Striding Behavior
+  /// ===================================================================
+  ///
+  /// The default implementation of this method calls `advanced(by:)` to offset
+  /// `current.value` by a specified `distance`. No attempt is made to count the
+  /// number of prior steps, and the result's `index` is always `nil`.
+  ///
+  /// To avoid incurring runtime errors that arise from advancing past
+  /// representable bounds, a conforming type can signal that the result of
+  /// advancing by a given `distance` is not representable by using `Int.min` as
+  /// a sentinel value for the result's `index`. In that case, the result's
+  /// `value` must be either the minimum representable value of this type if
+  /// `distance` is less than zero or the maximum representable value of this
+  /// type otherwise. Fixed-width integer types make use of arithmetic
+  /// operations reporting overflow to implement this customization.
+  ///
+  /// A conforming type may use any positive value for the result's `index` as
+  /// an opaque state that is private to that type. For example, floating-point
+  /// types increment `index` with each step so that the corresponding `value`
+  /// can be computed by multiplying the number of steps by the specified
+  /// `distance`. Serially calling `advanced(by:)` would accumulate
+  /// floating-point rounding error at each step, which is avoided by this
+  /// customization.
+  ///
+  /// - Parameters:
+  ///   - current: The result returned by this method in the immediately
+  ///     preceding step while striding, or `(index: 0, value: start)` if there
+  ///     have been no preceding steps.
+  ///   - start: The starting value used for the striding sequence.
+  ///   - distance: The amount to step by with each iteration of the striding
+  ///     sequence.
+  /// - Returns: A tuple of `index` and `value`; `index` may be `nil`, any
+  ///   positive value as an opaque state private to the conforming type, or
+  ///   `Int.min` to signal that the notional result of advancing by `distance`
+  ///   is unrepresentable, and `value` is the next result after `current.value`
+  ///   while striding from `start` by `distance`.
+  ///
+  /// - Complexity: O(1)
   static func _step(
     after current: (index: Int?, value: Self),
     from start: Self, by distance: Self.Stride
@@ -171,6 +224,39 @@ extension Strideable {
   }
 }
 
+extension Strideable where Self: FixedWidthInteger & SignedInteger {
+  @_alwaysEmitIntoClient
+  public static func _step(
+    after current: (index: Int?, value: Self),
+    from start: Self, by distance: Self.Stride
+  ) -> (index: Int?, value: Self) {
+    let value = current.value
+    let (partialValue, overflow) =
+      Self.bitWidth >= Self.Stride.bitWidth ||
+        (value < (0 as Self)) == (distance < (0 as Self.Stride))
+          ? value.addingReportingOverflow(Self(distance))
+          : (Self(Self.Stride(value) + distance), false)
+    return overflow
+      ? (.min, distance < (0 as Self.Stride) ? .min : .max)
+      : (nil, partialValue)
+  }
+}
+
+extension Strideable where Self: FixedWidthInteger & UnsignedInteger {
+  @_alwaysEmitIntoClient
+  public static func _step(
+    after current: (index: Int?, value: Self),
+    from start: Self, by distance: Self.Stride
+  ) -> (index: Int?, value: Self) {
+    let (partialValue, overflow) = distance < (0 as Self.Stride)
+      ? current.value.subtractingReportingOverflow(Self(-distance))
+      : current.value.addingReportingOverflow(Self(distance))
+    return overflow
+      ? (.min, distance < (0 as Self.Stride) ? .min : .max)
+      : (nil, partialValue)
+  }
+}
+
 extension Strideable where Stride: FloatingPoint {
   @inlinable // protocol-only
   public static func _step(
@@ -439,10 +525,13 @@ extension StrideThroughIterator: IteratorProtocol {
   public mutating func next() -> Element? {
     let result = _current.value
     if _stride > 0 ? result >= _end : result <= _end {
-      // This check is needed because if we just changed the above operators
-      // to > and <, respectively, we might advance current past the end
-      // and throw it out of bounds (e.g. above Int.max) unnecessarily.
-      if result == _end && !_didReturnEnd {
+      // Note the `>=` and `<=` operators above. When `result == _end`, the
+      // following check is needed to prevent advancing `_current` past the
+      // representable bounds of the `Strideable` type unnecessarily.
+      //
+      // If the `Strideable` type is a fixed-width integer, overflowed results
+      // are represented using a sentinel value for `_current.index`, `Int.min`.
+      if result == _end && !_didReturnEnd && _current.index != .min {
         _didReturnEnd = true
         return result
       }

test/stdlib/Strideable.swift

@@ -2,14 +2,14 @@
 //
 // This source file is part of the Swift.org open source project
 //
-// Copyright (c) 2014 - 2017 Apple Inc. and the Swift project authors
+// Copyright (c) 2014 - 2020 Apple Inc. and the Swift project authors
 // Licensed under Apache License v2.0 with Runtime Library Exception
 //
 // See https://swift.org/LICENSE.txt for license information
 // See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
 //
 //===----------------------------------------------------------------------===//
-// RUN: %target-run-simple-swift -swift-version=3
+// RUN: %target-run-simple-swift
 // REQUIRES: executable_test
 //
 
@@ -202,6 +202,27 @@ StrideTestSuite.test("FloatingPointStride/rounding error") {
     expectEqual(7, c.count)
     expectEqual(1 as Double, c.last)
   }
+  
+  if (1 as Float).addingProduct(0.9, 6) == 6.3999996 {
+    let d = Array(stride(from: 1 as Float, through: 6.3999996, by: 0.9))
+    expectEqual(7, d.count)
+    // The reason that `d` has seven elements and not six is that the fused
+    // multiply-add operation `(1 as Float).addingProduct(0.9, 6)` gives the
+    // result `6.3999996`. This is nonetheless the desired behavior because
+    // avoiding error accumulation and intermediate rounding error wherever
+    // possible will produce better results more often than not (see SR-6377).
+    //
+    // If checking of end bounds has been inadvertently modified such that we're
+    // computing the distance from the penultimate element to the end (in this
+    // case, `6.3999996 - (1 as Float).addingProduct(0.9, 5)`), then the last
+    // element will be omitted here.
+    //
+    // Therefore, if the test has failed, there may have been a regression in
+    // the bounds-checking logic of `Stride*Iterator`. Restore the expected
+    // behavior here by ensuring that floating-point strides are opted out of
+    // any bounds checking that performs arithmetic with values other than the
+    // bounds themselves and the stride.
+  }
 }
 
 func strideIteratorTest<

validation-test/stdlib/Stride.swift

@@ -9,11 +9,15 @@ var StrideTestSuite = TestSuite("Stride")
 StrideTestSuite.test("to") {
   checkSequence(Array(0...4), stride(from: 0, to: 5, by: 1))
   checkSequence(Array(1...5).reversed(), stride(from: 5, to: 0, by: -1))
+  checkSequence(stride(from: 0, to: 127, by: 3).map { Int8($0) },
+                stride(from: 0, to: 127 as Int8, by: 3))
 }
 
 StrideTestSuite.test("through") {
   checkSequence(Array(0...5), stride(from: 0, through: 5, by: 1))
   checkSequence(Array(0...5).reversed(), stride(from: 5, through: 0, by: -1))
+  checkSequence(stride(from: 0, through: 127, by: 3).map { Int8($0) },
+                stride(from: 0, through: 127 as Int8, by: 3))
 }
 
 runAllTests()