I’ve been delinquent in reviewing this proposal, and I apologize for slipping this in under the wire here.
I know some of the points below have already been covered by other reviewers, but as the thread has grown unwieldy, I have to apologize that it’s hard to keep track of who said what and won’t attempt to cite individuals. Instead I’ll keep it brief—
What is your evaluation of the proposal?
This proposal addresses a pressing need. We’ve discussed the APIs at length during the pitch phase, and @Alejandro deserves a lot of credit for working so diligently to synthesize the discussion. In its final form, the API largely reflects what was consensus during the pitch phase as to the proper breadth for a standard library implementation.
I will set aside issues with implementation for commentary at a later time and in a different forum. Instead, I will bring up a few concerns here about the design that I think would merit addressing:
Some PRNG algorithms only generate 32-bits of randomness at a time. I think it’d be fair to allow the result of
next() to be of any associated type that conforms to
UnsignedInteger, not just
UInt64. This does not impair the ability to write a performant generic default implementation of
Visibility of the default RNG
As mentioned during the pitch phase, I agree with the decision that the default RNG should not be seedable; since it’s not seedable, and since it’s a thread-safe singleton that should never need to be instantiated, I don’t think the default RNG needs to be a publicly visible entity. That is to say, I don’t think we need to have a public type named
Random. This may decrease the API surface area of the proposed additions.
Choice of default RNG
As others have mentioned, it isn’t necessary to specify the exact implementation as part of the proposal. But the principles behind choosing to use one implementation or another does go to the intended design.
Here, IIUC, the enunciated principle is that the default RNG should be as secure as pragmatically feasible, which I think is the right choice.
However, I am concerned about the philosophy behind using a cryptographically-secure
arc4random for newer versions of macOS and
SecCopyRandomBytes for earlier versions. They reflect different philosophical approaches to “pragmatic feasibility.” In my understanding, the latter API is a potentially slower one that is meant to be suitable for users to seed their own PRNGs with extremely good random bits, while the former API is a cryptographically-secure PRNG seeded and re-seeded periodically using extremely good random bits.
Now, in practice, there may be little distinction between the two (as to performance or security), but the question remains: is Swift’s default RNG a slower one intended to provide extremely good random bits even at a performance cost, or a cryptographically-secure PRNG seeded using those random bits? I think the answer should be the latter. But more importantly, I think we ought to decide on an answer as part of the design.
Static methods named
As mentioned during the pitch phase, I think
Bool.random() doesn’t pull its own weight:
@Alejandro has said that
Bool.random(using:) isn’t intended to allow different distributions (i.e., an unfair coin toss); in that case, a 50:50 choice between heads or tails doesn’t seem to be worth two methods in the standard library when it is trivial to implement in other ways.
I agree with @Chris_Lattner3 that
randomElement might be a more suitable name. In general, having multiple distinct methods named
random that have different semantics seems like an inferior choice when we have the option of distinguishing them without impairing readability.
I agree with others that a
shuffle API seems separable from the rest of the proposal; like
Bool.toggle, it may be worth considering separately.
Moreover, I’d urge study as to whether a more general API could take the place of
shuffle: specifically, a method that allows the user to choose n elements randomly without replacement. When n is equal to
count, the method is equivalent to
shuffle. This design incidentally has the nice benefit that it (a method to choose n elements randomly without replacement) would be to
prefix is to
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
Yes, with the caveats discussed above.
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
I have used similar features in many other languages; I think the proposed design generally compares favorably and very smartly avoids certain pitfalls (such as unwittingly encouraging modulo bias).
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?