Fix issue 15853 - std.random save methods must be const

[PetarKirov]

I removed the other attributes, because they are inferred.

[dlang-bot]

Fix	Bugzilla	Description
✓	15853	std.random save methods must be const

[PetarKirov]

There are also two other save() methods [1] [2] where adding const will introduce a breaking change, because the ranges their structs wrap may have non-const save() methods.

[quickfur]

Please add a unittest that verifies that the .save method of a const RNG can be assigned to a non-const RNG variable.

[quickfur]

Also, why are pure @safe nothrow removed from the existing code? Sorry, didn't see your explanation. Nevermind what I said.

[jmdavis]

Why are you removing @safe, pure, and nothrow? In none of the cases that you've changed here does whether the function can be @safe, pure, or nothrow depend on template arguments. So, there's no reason to remove them and every reason to have them.

[PetarKirov]

At first I started also adding @nogc, in addition to the other attributes, but then I thought that I should leave it to attribute inference, because the UintType could be a BigInteger with non-@nogc non-nothrow, etc. postblit)

[jmdavis]

It cannot be BigInt. isUnsigned is only true for built-in types which are unsigned integer types, and that's what's used on UintType in the template constraints. So, please put back the attributes that you removed. And feel free to add @nogc if it works. None of those attributes are going to depend on whether the template argument is a ulong, uint, etc. If BigInt and other user-defined types were allowed, then yes, attribute inference would be needed, but they're not, so it's not.

[PetarKirov]

Ok, I'll add them back. I think I will make a pass over the whole module to add all possible attributes.

[andralex]

@ZombineDev ping

[jmdavis]

Yes. Please add a test verifying that a the result of save can be assigned to a mutable variable - though honestly, given that save does not work with const in general, I'm inclined to argue that we shouldn't be slapping const on save. Rather, what we need is a more general solution that would deal with tail-const properly. And as it stands, the sooner that folks realize that ranges and const don't mix, the less trouble they'll have down the line. Supporting const with the rare ranges where it can be easily done might be a good idea, but it also results in oddball ranges that are doing stuff that you can't do with most ranges, which doesn't seem like a good idea to me.

[PetarKirov]

@quickfur @jmdavis Added tests.

[PetarKirov]

I had to add some attributes, because otherwise I was getting the following errors:

std/random.d(837): Error: @nogc function 'std.random.__unittestL831_18' cannot call non-@nogc constructor 'std.random.MersenneTwisterEngine!(uint, 32LU, 624LU, 397LU, 31LU, 2567483615u, 11LU, 7LU, 2636928640u, 15LU, 4022730752u, 18LU).MersenneTwisterEngine.this'
std/random.d(840): Error: @nogc function 'std.random.__unittestL831_18' cannot call non-@nogc function 'std.algorithm.comparison.equal!().equal!(Take!(const(MersenneTwisterEngine!(uint, 32LU, 624LU, 397LU, 31LU, 2567483615u, 11LU, 7LU, 2636928640u, 15LU, 4022730752u, 18LU))), Take!(MersenneTwisterEngine!(uint, 32LU, 624LU, 397LU, 31LU, 2567483615u, 11LU, 7LU, 2636928640u, 15LU, 4022730752u, 18LU))).equal'
posix.mak:357: recipe for target 'std/random.test' failed
make: *** [std/random.test] Error 1

and:

std/random.d(585): Error: @nogc constructor 'std.random.MersenneTwisterEngine!(uint, 32LU, 624LU, 397LU, 31LU, 2567483615u, 11LU, 7LU, 2636928640u, 15LU, 4022730752u, 18LU).MersenneTwisterEngine.this' cannot call non-@nogc function 'std.random.MersenneTwisterEngine!(uint, 32LU, 624LU, 397LU, 31LU, 2567483615u, 11LU, 7LU, 2636928640u, 15LU, 4022730752u, 18LU).MersenneTwisterEngine.seed!().seed'

and

std/random.d(617): Error: @nogc function 'std.random.MersenneTwisterEngine!(uint, 32LU, 624LU, 397LU, 31LU, 2567483615u, 11LU, 7LU, 2636928640u, 15LU, 4022730752u, 18LU).MersenneTwisterEngine.seed!().seed' cannot call non-@nogc function 'std.random.MersenneTwisterEngine!(uint, 32LU, 624LU, 397LU, 31LU, 2567483615u, 11LU, 7LU, 2636928640u, 15LU, 4022730752u, 18LU).MersenneTwisterEngine.popFront'

[PetarKirov]

I changed this to assert, because it is a logic bug.

[jmdavis]

Okay, but why are you ||ing instead of using assert(x0, ...)? That's not how assertions are normally used.

And since you changed it to an assertion, you can make the function nothrow.

[JakobOvrum]

Remember that assert(0, …) (and assert(false, …) etc) are a special case of assert.

[jmdavis]

Yes. They're a special case. They're used when they need to fail even in release mode, which is not the normal thing to do with assertions. It's for stuff like the default case of switch statement when it should never be hit or the catch statement that catches an exception that should never be thrown, but you have to catch it to make the function nothrow. It's not for validating function arguments.

[andralex]

I'm okay with changing enforce with assert here although pedantically it's a breaking change. But no assert(0).

[JakobOvrum]

@quickfur, @jmdavis, the implicit conversion you're asking for is enshrined in the signature of save. typeof(this) does not change based on the type qualifier applied to the this-parameter. If mutable indirections are later added to the type, save will cease to compile. The correct fix will then be to use inout(typeof(this)) save() inout @property; (which I don't recommend now, as it is a breaking change).

---

LGTM. @ZombineDev, is that a failure of attribute inference? :O

[PetarKirov]

@JakobOvrum

The correct fix will then be to use inout(typeof(this)) save() inout @Property; (which I don't recommend now, as it is a breaking change).

I would argue that const(Range) and immutable(Range) are both useless to the caller of range.save() and I see no reason why any range implementation would return anything different than typeof(this) (without mutability qualifiers).

is that a failure of attribute inference? :O

It looks like so. I will try to find a reduce test case.

[JakobOvrum]

I agree that non-mutable range types are of very limited use, but by the same logic, surely this patch is also "useless". On the other hand, it's trivial and harmless to support const so I'm completely fine with any effort to that end.

[jmdavis]

I agree that non-mutable range types are of very limited use, but by the same logic, surely this patch is also "useless".

Which is why that I argued that this PR is not necessarily a good idea. I'm not sure that it's a bad idea, but I seriously question that it makes sense to try and support const piecemeal like this. I tend to think that we should either find a solution for getting const and ranges to interact properly, or we shouldn't be mucking around with const and ranges. As long as const is pretty much useless with ranges, I'd argue that it's better for folks to realize that they shouldn't be used together rather than trying to get it to work in the corner cases where we happen to be able to get it to work at the moment. Add making save const means that we can't do anything to this range later which doesn't work with const. Now, in theory, std.random is going to get a major overhaul, in which case, what we do to this stuff now is probably moot, but that may or may not ever happen, and this PR is trying to shoehorn in a behavior that is not supported by ranges in general. And that just seems like a bad idea to me. It's not so bad that I'll fight tooth and nail for this not to get merged, but if it were purely up to me, it wouldn't be.

[DmitryOlshansky]

Soo are we pulling this?

[andralex]

OK, can we move forward with this?

[PetarKirov]

Thanks for the ping, I'll find some time later today or tomorrow to address comments.

[PetarKirov]

Updated. I went into adding attributes to unpredictableSeed, but since it used Thread.getThis, I went ahead and made a PR to druntime to add attributes to core.thread. So dlang/druntime#1726 needs to be pulled first.

[wilzbach]

I went ahead and made a PR to druntime to add attributes to core.thread. So dlang/druntime#1726 needs to be pulled first.

@ZombineDev your druntime PR has been merged, so we could move forward with this. Would you be so kind to rebase your work?

[PetarKirov]

Sure will do later this week

[JackStouffer]

@ZombineDev Can you rebase and then ping me?

[quickfur]

ping @ZombineDev Rebase?

[quickfur]

ping @ZombineDev
I managed to rebase this for you, but apparently you disabled write access to this PR so I couldn't push it. Did you want to to the rebase yourself, or should I push my merge to a new PR and close this one?

[JackStouffer]

Ping @ZombineDev

[quickfur]

ping @ZombineDev @wilzbach

Are we still moving forward with this PR, or should we close it?