fix 14478: support non-copyable elements in many range algorithms

[WebFreak001]

same as #8714

code not fully covered

[dlang-bot]

Thanks for your pull request, @WebFreak001!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	14478	enhancement	isInputRange should allow ranges of non-copyable elements

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#8721"

[RazvanN7]

@WebFreak001 the testers are failing. Are you willing to push this to the finish line?

[WebFreak001]

thanks!

[RazvanN7]

Thank you!

[JohanEngelen]

This PR results in issues at Weka with retro for a struct that has void opAssign()(auto ref const(typeof(this)) that) const defined, i.e. a struct that allows const assignment. The problem is that assignment is allowed, but move is not (because it would be a memcpy to const memory).

The cause of the problem: retro checks for hasAssignableElements, but then does a move rather than an assignment.
https://github.com/dlang/phobos/pull/8721/files#diff-705ed5d9f4ea4f9dadcb07d7116df8dc8397e851639a28538dc0b1d6fa446f56L312-R318

Testcase (https://d.godbolt.org/z/z6KMY6nYf):

import std.range;

struct Handle { 
    int[5] entries;
    void opAssign()(auto ref const(typeof(this)) that) const { }
}

const(Handle)[100] arr;
auto foo() {
    return arr[0..5].retro;
}

the problem is fixed by reverting the lines for retro.

[jmdavis]

This PR results in issues at Weka with retro for a struct that has void opAssign()(auto ref const(typeof(this)) that) const defined, i.e. a struct that allows const assignment. The problem is that assignment is allowed, but move is not (because it would be a memcpy to const memory).

Aside from anything to do with the specific changes here, what does a const opAssign even do? Conceptually, that seems like it would be illegal, because mutating const violates the type system (though without the compiler having extra checks to prevent it, it makes sense that opAssign would be treated like any other function in its ability to be const). Is this opAssign casting away const to mutate anyway? Or is it just mutating something outside the object instead of the object itself? Or is not actually doing any kind of assignment?

[JohanEngelen]

Aside from anything to do with the specific changes here, what does a const opAssign even do?

It's used to enable assigning to a const(Handle!Block) as if it is a Handle!(const(Block)), and it's indeed writing to something outside the object. There could be better solutions, but "it worked" before...

[jmdavis]

Aside from anything to do with the specific changes here, what does a const opAssign even do?

It's used to enable assigning to a const(Handle!Block) as if it is a Handle!(const(Block)), and it's indeed writing to something outside the object. There could be better solutions, but "it worked" before...

I'm not trying to argue against changing the Phobos code. I'm just trying to understand why anyone would be doing anything with a const opAssign, since at a glance, it seems like something that's violating the type system.

Regardless of the details of your code, you've clearly shown that retro's template constraint is wrong, because it checks for one operation and then does another instead.

[JohanEngelen]

Regardless of the details of your code, you've clearly shown that retro's template constraint is wrong, because it checks for one operation and then does another instead.

Then let's keep the discussion on topic and not derail it, thanks.

[WebFreak001]

The hasAssignableElements is used in most of the range operations I touched in this PR, should all of them be changed as well?

[jmdavis]

https://issues.dlang.org/show_bug.cgi?id=24481

Regardless of the details of your code, you've clearly shown that retro's template constraint is wrong, because it checks for one operation and then does another instead.

Then let's keep the discussion on topic and not derail it, thanks.

It's very relevant, because understanding why users are doing weird edge-case stuff can have a big impact on what we test for in Phobos and the way that algorithms are written. And in this case, the Weka code is doing something that plenty of folks are just going to assume is illegal.

The hasAssignableElements is used in most of the range operations I touched in this PR, should all of them be changed as well?

As far as correctness goes, none of those functions should be using a move without verifying that it's legal first. They should probably all be changed to use a static if that checks whether a move compiles and use that if it is and do assignment otherwise.

[jmdavis]

I'll have a PR for this in a bit.

[jmdavis]

#8969

[JohanEngelen]

#8969

thank you!