fix issue 18631 - Allow choice to work with const arrays

[ghost]

No description provided.

[dlang-bot]

Thanks for your pull request and interest in making D better, @bbasile! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
• 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

[JackStouffer]

Shouldn't this be inout auto ref?

Because I don't think this will compile anymore choice([1, 2, 3, 4]);

[ghost]

inout implies also auto in this case. I've added a test for a call by value since your comment.

[jmdavis]

Honestly, this seems like a compiler bug. I've never heard of inout ref doing anything similar to auto ref, and that behavior does not match what happens with const or immutable. Also, the way inout is designed, it's always supposed to be used on the return value in addition to one of the parameters. So, even if it's true that inout ref is supposed to act like auto ref with regards to rvalues, it would then be a bug that inout isn't used on the return type.

[JackStouffer]

Oh, just saw your comment @jmdavis. @bbasile Can you add inout to the return type?

[ghost]

Okay done.

[JackStouffer]

Geez inout is so ugly.

[jmdavis]

Geez inout is so ugly.

In most cases, with templated functions, you can just not bother with const or inout or whatever, and everything works, but as soon as ref gets involved, things get messier. :(

[wilzbach]

(manually merging as Darwin_64_32 is currently broken)

[jmdavis]

This PR is a breaking change, because it assumes that length and opSlice work with const, and they frequently do not.

[quickfur]

Any suggestions as to how to fix it, short of reverting the whole thing? Preferably, we would not have to undo everything done here.

[jmdavis]

This is trying to fix a corner cases in ranges. Either it will work to just slice the array before passing it in, or a separate overload for dynamic arrays will be required. Regardless, marking a range as inout or const is almost never appropriate. I didn't look over this thoroughly enough when I glanced over it before, so I didn't notice the problem like I should have. This should never have been merged.

#6331

[schveiguy]

Geez inout is so ugly.

The inout(ElementType!Range) was not required, you could have just put inout. It's a storage class, just like all the others, and so allows for type inference.

Edit: actually, all you needed to do is put inout on the parameter. Type inference was already happening. In fact, inout on the function would have done nothing, I forget that it only matters if it's a member function.

[schveiguy]

Yes, inout (and const) needs to have buy-in from all the types you are trying to wrap, or you need to introspect and forward them separately. In the case where it is supported, unfortunately, you can't have it inferred.

The downside of not doing this, is that you lose the "temporary const" guarantee that it provides.

[schveiguy]

Since it's pretty accepted that you can slice an object and get a range from it, Another way to fix this may be (not a full implementation, needs some tweaking):

auto ref choice(R)(auto ref R r) if (!isRandomAccessRange!R && isRandomAccessRange!(typeof(r[])))
{
    return choice(r[]);
}

It should work for containers as well, such as Array!T