Fix Issue 17832 - std.random.choice cannot be used with other random generators

[dunkyp]

Random choice default argument was deciding the type of the
template. I'm not sure if this is a deeper bug, but it can easily be
fixed with this patch which simply overloads the function.
Also added a unittest which would have failed under the old code.

[dlang-bot]

Thanks for your pull request, @dunkyp! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

• smaller, focused PRs are easier to review than big ones

• try not to mix up refactoring or style changes with bug fixes or feature enhancements

• provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Auto-close	Bugzilla	Description
✓	17832	std.random.choice cannot be used with other random generators

[MetaLang]

The fix looks good, but there's something deeper going on here that needs to be investigated further. It looks like there may be a bug here with template type deduction. It's not clear to me how things should work in this situation.

[wilzbach]

Thanks a lot for your PR!
I am not sure whether this is the right way to move forward as Phobos should be a prime example of D code and not a collection of workarounds against compiler bugs. Hence, I would prefer that this gets reduced and filled as compiler bug.
If there is a strong opinion to merge this now, we should at the very least mark the workaround and mention it on Bugzilla so that it can be removed in the future.

[quickfur]

Found underlying cause: https://issues.dlang.org/show_bug.cgi?id=16467

Basically, the problem is that a template function like void func(T)(T t = 123) is just shorthand for:

template func(T)
{
    void func(T t = 123) { ... }
}

The problem is that during IFTI of a call like func("abc"), T is found to be string, which makes the declaration equivalent to void func(string t = 123), which is invalid. I'm not 100% sure what happens after that in the compiler, but I suspect something along the lines of a compilation error in a template being gagged and the compiler moving on to the next possible overload / instantiation or whatever, which eventually ends with it trying to shoehorn a string into a func!int overload, causing it to abort with a compile error.

One possible solution is to change the declaration to:

template func(T)
{
    static if (is(T == typeof(123)))
        void func(T t = 123) { ... }
    else
        void func(T t) { ... }
}

Since this is a lot of ugly boilerplate, I'm tempted to say the compiler should perform this lowering automatically. However, that unfortunately breaks the valid case where you do want the default parameter to apply across different types, for example:

void func(T)(T t = 123) if (isIntegral!T) { ... }

In this case, the default argument could arguably(!) be construed to impose a constraint that T must be a type for which 123 is a valid value.

So, unfortunately, it seems that the workaround in this PR is probably a necessary one.

[MetaLang]

Thanks for the explanation. When you put it that way, this is definitely a fairly subtle bug in the code and needs to be fixed. It's basically the same as doing:

void test(T)() { T t = 123; }

Obviously this is wrong and should be corrected.

[quickfur]

ping @wilzbach

Does the above address your concerns? Should we merge this? It's been rotting here for far too long already.

[quickfur]

ping @wilzbach I'm in favor of merging this, because the bug is real, and needs to be addressed somehow. The fact that Phobos looks a little uglier to me seems secondary, and something that we can work on afterwards. I've thought of an idea that might work, but it will probably involve a DIP, so I'd much rather get this fix in now rather than later. What do you think?

[wilzbach]

Should be /// ditto - this tells the Ddoc engine to visually group the overloads.

[wilzbach]

Sorry getting too many GH lately. Feel free to ping me on Slack for something that requires immediate attention.

1. I agree that the bug is real and needs to be fixed. All other methods in std.random are probably affected by this too 😱

2. I'm still not a huge friend of uglifying Phobos to workaround compiler bugs / implementation problems, but that sadly seems to be a common motive this month :/

3. Let's look closer at the compiler bug / limitation

Here's a reduction:

struct Foo {}
struct Bar {}
Foo foo;
void myFun(D)(D d = foo){}

void bar()
{
    myFun(); // works
    myFun(Bar()); // error
}

> dmd -c foo.d
foo.d(6): Error: cannot implicitly convert expression foo of type Foo to Bar
foo.d(11): Error: template instance foo.myFun!(Bar) error instantiating

As you might have guessed, it's a known bug:
https://issues.dlang.org/show_bug.cgi?id=17036

There's also the pre-approved https://issues.dlang.org/show_bug.cgi?id=17186 which even had a PR (dlang/dmd#2270).
It could fix this problem by allowing auto d = foo.

4. I've thought of an idea that might work, but it will probably involve a DIP

Without knowing your idea, I can say the following: "you might be luckily and it could be considered as a bug fix". Remember that 17186 is pre-approved.

5. I'm in favor of merging this, because the bug is real

Yeah, fair enough, but let's fix entire std.random then (doesn't have to be this PR)?
Thanks for pushing this!

We won't be able to fix the compiler bug in the near future :/

[wilzbach]

Should be /// ditto - this tells the Ddoc engine to visually group the overloads.

I quickly fixed this myself.

[quickfur]

Is the CircleCI failure relevant? All I can glean from the output is that some built-in timeout was exceeded while running tests. Doesn't seem related to this PR directly?

[wilzbach]

It has to do with something else, but so far I haven't had time to dig deeper into it.