[SR-8156] Difficult to use random integers in generic contexts

[stephentyrone]

Previous ID	SR-8156
Radar	rdar://problem/41789616
Original Reporter	@stephentyrone
Type	Bug
Status	Resolved
Resolution	Done

Additional Detail from JIRA

Votes	0
Component/s	Standard Library
Labels	Bug
Assignee	@stephentyrone
Priority	Medium

md5: cd2383b8df0c4be4e2e4a03e4f022a94

Issue Description:

One would like to be able to write code like the following:

(swift) func foo<T>( ) -> T where T: FixedWidthInteger {
          return T.random(in: 1 ... 6)
        }
<REPL Input>:2:10: error: ambiguous reference to member 'random(in:using:)'
  return T.random(in: 1 ... 6)
         ^
Swift.FixedWidthInteger:2:35: note: found this candidate
    @inlinable public static func random<T>(in range: Range<Self>, using generator: inout T) -> Self where T : RandomNumberGenerator
                                  ^
Swift.FixedWidthInteger:3:35: note: found this candidate
    @inlinable public static func random(in range: Range<Self>) -> Self
                                  ^
Swift.FixedWidthInteger:2:35: note: found this candidate
    @inlinable public static func random<T>(in range: ClosedRange<Self>, using generator: inout T) -> Self where T : RandomNumberGenerator
                                  ^
Swift.FixedWidthInteger:3:35: note: found this candidate
    @inlinable public static func random(in range: ClosedRange<Self>) -> Self

What's actually necessary here is:

(swift) func foo<T>( ) -> T where T: FixedWidthInteger, T.Stride: SignedInteger, T.Magnitude: UnsignedInteger {
          return T.random(in: 1 ... 6)
        }

But that's a lot to ask of people.

[stephentyrone]

It's seems likely that `FixedWidthInteger` should simply require `Stride: SignedInteger` and `Magnitude: UnsignedInteger` as a basic requirement. That would happily fix this bug too.

[stephentyrone]

CC @lorentey@airspeedswift@moiseev

[lorentey]

That would be ideal! I don't recall if we ever tried adding specifically these requirements, but I remember running into unacceptable compiler performance regressions when we wanted to add Collection conformance to FixedWidthInteger.Words. Recursive requirements like these slowed down compilation too much.

The conformance checker improved a great deal this year; it'd be worth a check to see if the issue is still there.

[stephentyrone]

I think Stride: SignedInteger is basically a no-brainer (assuming that conformance checking performance isn't an issue), but Magnitude: UnsignedInteger would proscribe implementations of FixedWidthInteger that are signed with a sign-magnitude representation, which probably want Magnitude: Self. I'm not sure if we care, however.

[stephentyrone]

Alternatively I think we could also drop the Magnitude: UnsignedInteger requirement by implementing next(upperBound: T) on FixedWidthInteger instead of FixedWidthInteger & UnsignedInteger, with precondition(upperBound > 0).

[lorentey]

I don't recall if swift-evolution had a discussion on the UnsignedInteger requirement; but if it was up to me, I'd allow both RandomNumberGenerator.next() and .next(upperBound🙂 to be used on signed integers, too!

[moiseev]

/cc @xwu and @natecook1000

[moiseev]

next(upperBound: ) turns out to already havea precondition, so we're not adding a precondition, merely changing the ...well, condition, so I agree it would make sense to relax the requirements and allow this API to be applied to signed integers as well.

As for the the using random in generic context, I wonder if we can change the random functions to be generic over the range bounds. Now the Range<Self> is used, but it can as well be Range<T> where T: Strideable, T.Stride: SignedInteger with a few bounds checks. This will allow to drop the stride requirement from the extension that introduces random APIs.

[stephentyrone]

@swift-ci create

[stephentyrone]

If it doesn't adversely effect performance, I think the right fix here is to add Stride: FixedWidthInteger & SignedInteger and Magnitude: FixedWidthInteger & UnsignedInteger. This is really just making the existing semantics explicit, and resolves the immediate issue entirely.

We may also want to make next( ) work with SignedInteger, but we can do that as a separate bug / enhancement, as it's not necessary to resolve the immediate ergonomics issue.

#17716

[stephentyrone]

Landed on master (#17716 and swift-4.2-branch (#17766