Fix Bugzilla issue 24827: maxElement does not handle opAssign correctly.

[jmdavis]

Rebindable2 did not handle types with opAssign correctly, which affected both minElement and maxElement. Namely, Rebindable2 assigned to memory which was not properly initialized when the correct solution in such a situation is to use copyEmplace. Assignment works when assignment is just a memcpy, but in the general case, opAssign needs to have a properly initialized object in order to work correctly. copyEmplace instead copies the object and then places the copy into the unitialized memory, so it avoids assigning to uninitialized memory.

This commit also adds additional tests for types with destructors (which do get opAssign automatically) and types with postblit constructors or copy constructors to try to ensure that the code is doing the correct thing in those cases with regards to copying, assignment, and destruction.

https://issues.dlang.org/show_bug.cgi?id=24829 was found in the process, and this does not fix that. Namely, types which cannot be assigned to and which also have a postblit constructor or copy constructor do not get copied correctly. So, among the tests added here are commented out tests for that case, since they're an altered version of some of the enabled tests. However, fixing that issue would be involved enough that I'm not attempting to fix it at this time.

@FeepingCreature

[dlang-bot]

Thanks for your pull request, @jmdavis!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	24827	normal	maxElement does not correctly handle types with opAssign

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 "stable + phobos#9067"

[FeepingCreature]

Oh man I don't remember. I'll have a look during the week I guess, but don't let that stop this merge. So long as the existing tests pass I'm happy. (This case wasn't on my radar because we never use nontrivial assignments/copy constructors anywhere.) Definitely good on you for testcasing the weird things.

[FeepingCreature]

Rebindable2 assigned to memory which was not properly initialized when the correct solution in such a situation is to use copyEmplace. Assignment works when assignment is just a memcpy, but in the general case, opAssign needs to have a properly initialized object in order to work correctly.

Hang on, I don't understand this. The whole point of the ubyte[]/void[] Payload code is that it doesn't call opAssign (or copy constructors), so why would opAssign make problems here?

[jmdavis]

Rebindable2 assigned to memory which was not properly initialized when the correct solution in such a situation is to use copyEmplace. Assignment works when assignment is just a memcpy, but in the general case, opAssign needs to have a properly initialized object in order to work correctly.

Hang on, I don't understand this. The whole point of the ubyte[]/void[] Payload code is that it doesn't call opAssign (or copy constructors), so why would opAssign make problems here?

I don't know of any situation where not calling opAssign or copy constructors when they exist is the right thing to do when copying an object unless you're moving the object instead of copying it. Otherwise, you just break how the type works. If an object is assigned to or copied, it needs to be done properly. I honestly do not understand why the version where useQualifierCast is false even exists. I've tried to test for it to make sure that it works properly, but I don't know what problem it's trying to solve - especially since Rebindable2 has no tests whatsoever prior to these changes. The only testing it's had is the testing that's done for the code that uses it.

That being said, the main issue that is being fixed here is that the existing code assigns to memory that hasn't been properly initialized, and that only works properly if there is no opAssign. Without these changes, in the case where useQualifierCast is true, opAssign is being used even though the memory hasn't been initialized properly. This fixes it so that in such a case, copyEmplace is used instead so that a copy is created and then moved into the uninitialized memory. In the case where useQualiferCast is false, this also fits, since it's specifically not using assignment at all (and if it's also supposed to avoid copying, that should have been tested for along with assignment). It was previously copying without calling the copy constructor or postblit constructor, but that's just wrong behavior for any use case that I'm aware of, because any type that defines those is going to designed with the idea that they will be called for any copy.

What this commit does not fix is the issue where copying Rebindable2 does not trigger a copy constructor or postblit constructor if T has one, and useQualifierCast is false (since to fix such a case, Rebindable2 needs the appropriate postblit constructor or copy constructor, and the compiler does not generate one, because a static array of ubyte or void is used). In such a case, the object is not copied correctly and gets blitted instead, but that's the behavior that it's had since Rebindable2 was added.

[jmdavis]

Actually, looking at the code again, the copy constructor was being called before in the case where useQualifierCast was false. It's just that instead of using copyEmplace, it created a union to copy the object into and then didn't call the destructor on the union, presumably because you don't want to destroy the object, because the array holds its memory at that point. However, copyEmplace should make that trick unnecessary. So, the copy was being done correctly there, but the copy constructor or postblit constructor still isn't called when the Rebindable2 as a whole is copied and useQualifierCast is false.

Either way, the main issue that I was trying to fix here was that opAssign was being called on uninitialized memory when useQualifierCast is true. That needs to use copyEmplace instead, which it now does with these changes.

So, the useQualifierCast is true case should be fixed, and the useQualifierCast is false case should be effectively unchanged. It's just that it now uses copyEmplace to make the copy into the array instead of creating the extra union to do the copy without triggering the destructor.

[FeepingCreature]

I don't know of any situation where not calling opAssign or copy constructors when they exist is the right thing to do when copying an object unless you're moving the object instead of copying it.

The way that Rebindable2 is intended to model this situation is that the value is first destroyed and then recreated via a move. For an example of a type that must bypass opAssign, consider a humble immutable struct S { SysTime a; }. SysTime has opAssign, but it can obviously not be called in an immutable variable. The idea is that a Rebindable2 isn't "really" a variable, because you cannot access the contained value by reference (see my talk). So copy constructors make sense, but postblit and opAssign does not - we're basically taking the variable and moving it ~into the void~ (and back, later). Obviously, the number of copy and destructor calls must still add up correctly.