Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
stephentyrone opened this issue Jun 29, 2018 · 11 comments
Closed

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

stephentyrone opened this issue Jun 29, 2018 · 11 comments
Assignees
@stephentyrone
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. standard library Area: Standard library umbrella

Comments

@stephentyrone
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

CC @lorentey@airspeedswift@moiseev

@lorentey
Copy link
Member

lorentey commented Jul 3, 2018

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

lorentey commented Jul 3, 2018

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
Copy link
Mannequin

moiseev mannequin commented Jul 3, 2018

/cc @xwu and @natecook1000

@moiseev
Copy link
Mannequin

moiseev mannequin commented Jul 3, 2018

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
Copy link
Member Author

@swift-ci create

@stephentyrone
Copy link
Member Author

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
Copy link
Member Author

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

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@stephentyrone stephentyrone

Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. standard library Area: Standard library umbrella
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@lorentey @stephentyrone