Issue 24827 - maxElement does not correctly handle types with opAssign
Summary: maxElement does not correctly handle types with opAssign
Status: RESOLVED FIXED
Alias: None
Product: D
Classification: Unclassified
Component: phobos (show other issues)
Version: D2
Hardware: All All
: P1 normal
Assignee: No Owner
URL:
Keywords: pull
Depends on:
Blocks:
 
Reported: 2024-10-23 05:49 UTC by Jonathan M Davis
Modified: 2024-11-17 01:03 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description Jonathan M Davis 2024-10-23 05:49:32 UTC
Strictly speaking, I'm pretty sure that the problem here is that Rebindable2 doesn't use emplaceCopy but instead uses =, but maxElement is the public symbol which exposes this issue, an example being

---
void main()
{
    import std.algorithm.searching : maxElement;
    auto arr = [S(19), S(2), S(145), S(7)];
    assert(maxElement(arr) == S(145));
}

struct S
{
    int i;
    bool destroyed;

    this(int i)
    {
        this.i = i;
    }

    ~this()
    {
        destroyed = true;
    }

    bool opEquals()(auto ref S rhs)
    {
        return this.i == rhs.i;
    }

    int opCmp()(auto ref S rhs)
    {
        if(this.i < rhs.i)
            return -1;

        return this.i == rhs.i ? 0 : 1;
    }

    invariant
    {
        assert(!destroyed);
    }
}
---

This results is the assertion in the invariant failing a bunch of times.
Comment 1 Dlang Bot 2024-10-23 15:05:00 UTC
@jmdavis created dlang/phobos pull request #9067 "Fix Bugzilla issue 24827: maxElement does not handle opAssign correctly." fixing this issue:

- Fix Bugzilla issue 24827: maxElement does not handle opAssign correctly.
  
  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.

https://github.com/dlang/phobos/pull/9067
Comment 2 Dlang Bot 2024-10-27 08:16:28 UTC
dlang/phobos pull request #9067 "Fix Bugzilla issue 24827: maxElement does not handle opAssign correctly." was merged into stable:

- 33fde0d1bbbd3585c574c52a7cfb0cafaa63a010 by Jonathan M Davis:
  Fix Bugzilla issue 24827: maxElement does not handle opAssign correctly.
  
  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.

https://github.com/dlang/phobos/pull/9067
Comment 3 Dlang Bot 2024-11-17 01:03:59 UTC
dlang/phobos pull request #9086 "Merge stable" was merged into master:

- f0c3e4a66b68d766c7601d9571aa1bf65a5c371e by Jonathan M Davis:
  Fix Bugzilla issue 24827: maxElement does not handle opAssign correctly. (#9067)
  
  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.

https://github.com/dlang/phobos/pull/9086