Fix Issue 18631 - std.random.choice does not work with const arrays

[GrimMaple]

auto ref is not needed on Range param
An overload with explicit ref Range added to support ranges returning ref

[dlang-bot]

Thanks for your pull request and interest in making D better, @GrimMaple! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	18631	normal	std.random.choice does not work with const arrays

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#8495"

[dkorpel]

The documentation says:

A single random element drawn from the `range`. If it can, it will
    return a `ref` to the $(D range element), otherwise it will return
    a copy.

With this change, ranges that have a return ref front() will now return a copy instead of a ref.

[GrimMaple]

The documentation says:

A single random element drawn from the `range`. If it can, it will
    return a `ref` to the $(D range element), otherwise it will return
    a copy.

With this change, ranges that have a return ref front() will now return a copy instead of a ref.

Please, provide a concrete example

[dkorpel]

struct Range
{
    int x;
    ref int front() return {return x;}
    ref int back() return {return x;}
    void popFront() {}
    void popBack() {}
    bool empty = false;
    Range save() {return this;}
    size_t length = 10;
    alias opDollar = length;
    ref int opIndex(size_t i) return {return x;}
}

void main()
{
    import std.random, std.range;
    auto r = Range(10);
    int* x = &choice(r);
}

After this PR:

`choice(r)` is not an lvalue and cannot be modified

[GrimMaple]

This can be fixed if I add overloads that accept (EDIT: and return) ref Range explicitly, like this:

/// ditto
ref auto choice(Range, RandomGen = Random)(ref Range range, ref RandomGen urng)
{
    assert(range.length > 0,
           __PRETTY_FUNCTION__ ~ ": invalid Range supplied. Range cannot be empty");

    return range[uniform(size_t(0), $, urng)];
}

/// ditto
ref auto choice(Range)(ref Range range)
{
    return choice(range, rndGen);
}

Is that acceptable?

[dkorpel]

Is that acceptable?

Not sure. I'd wager most ranges have references to their elements, so it's a rare case to support. One range with elements inside it is only, but its opIndex doesn't return by ref, so currently you can't take the address of choice(onlyRange). Maybe it's best to adjust the documentation.

[GrimMaple]

Is that acceptable?

Not sure. I'd wager most ranges have references to their elements, so it's a rare case to support. One range with elements inside it is only, but its opIndex doesn't return by ref, so currently you can't take the address of choice(onlyRange). Maybe it's best to adjust the documentation.

The documentation already says If it can, so there is no need to adjust anything. It doesn't promise anything. I'm more worried about this change breaking existing code that relies on this behabior.

[GrimMaple]

Please note that provided solution for suspect range misses if constraints. But when added, the suspect range and provided example code also compiles and passes testing :)

[dkorpel]

The documentation already says If it can, so there is no need to adjust anything.

In my example, choice can return a ref, but it now makes a copy. I think you can remove that entire sentence, it's repeating what you can deduce from the auto ref in the signature.

[schveiguy]

Since I started this with a post in discord, I'll chime in.

1. The range you constructed is very suspect to me, but that is really not on choice. We have the API already available for such situations (though they require using dip1000 to avoid real problems), so we can't break it.
2. What we need specifically is choice(const(T[])) to work. To that end, instead of breaking the existing API (which IMO should never have included auto ref to begin with), we can add overloads that take a const(T[]) and be done:

ref choice(Range)(ref Range range)
    if (!isInputRange!Range && __traits(isDynamicArray, Range))
{
    return choice(range[]);
}

ref choice(Range, RandomGen)(ref Range range, ref RandomGen urng)
    if (!isInputRange!Range && __traits(isDynamicArray, Range))
{
    return choice(range[], urng);
}

This pattern is ugly, but I don't see another way to do it without breaking the example, or without splitting the ref and non-ref versions.

[GrimMaple]

I have updated the code to support the scenario provided by dkorpel. Provided scenario is also added as a unittest