swiftlang / swift

The Swift Programming Language
https://swift.org
Apache License 2.0
67.28k stars 10.33k forks source link

[SR-8779] UnsignedInteger constraint on RandomNumberGenerator is not ideal #51287

Open Azoy opened 5 years ago

Azoy commented 5 years ago
Previous ID SR-8779
Radar None
Original Reporter @Azoy
Type Bug
Additional Detail from JIRA | | | |------------------|-----------------| |Votes | 0 | |Component/s | Standard Library | |Labels | Bug | |Assignee | None | |Priority | Medium | md5: 7065369b6f42acb03b6e059f6af05a15

Issue Description:

Lack of a better bug title, these extension methods on RandomNumberGenerator:

public func next<T: FixedWidthInteger & UnsignedInteger>() -> T {}

public func next<T: FixedWidthInteger & UnsignedInteger>(upperBound: T) -> T {}

are somewhat difficult to work with. Here is the implementation of randomElement() on Collection:

  public func randomElement<T: RandomNumberGenerator>(
    using generator: inout T
  ) -> Element? {
    guard !isEmpty else { return nil }
    let random = generator.next(upperBound: UInt(count))
    let index = self.index(
      startIndex,
      offsetBy: numericCast(random)
    )
    return self[index]
  }

Granted the numericCast might be unnecessary and you can achieve the same using Int(random), but removing the UnsignedInteger constraint allows for an implementation like so:

  public func randomElement<T: RandomNumberGenerator>(
    using generator: inout T
  ) -> Element? {
    guard !isEmpty else { return nil }
    let random = generator.next(upperBound: count)
    let index = self.index(
      startIndex,
      offsetBy: random
    )
    return self[index]
  }

This is just one example of where removing this constraint is helpful.

Azoy commented 5 years ago

cc: @stephentyrone

belkadan commented 5 years ago

You're supposed to use Int.random(in: 0...count, using: &generator), not call next directly. See SE-0202 for more details.

stephentyrone commented 5 years ago

We have considered something like having .next( ) return any FixedWidthInteger, but basically "what Jordan said". Most use cases should be using the methods that take a range.