[BUG] Dangers of move-from-last-use

[YehezkelShB]

Describe the bug
Move from last use is dangerous when there are dependencies between objects, e.g. for synchronization or using shared/weak_ptr.

To Reproduce
cpp code:

#include <mutex>

class Obj
{
private:
    std::mutex m_mutex;
    std::string m_name;

public:
    std::unique_lock<decltype(m_mutex)> lock();
    // some setters

    void waitForEvent(std::unique_lock<decltype(m_mutex)>& lock);

    // To allow calling waitForEvent(obj.lock());
    void waitForEvent(std::unique_lock<decltype(m_mutex)>&& lock);
    void setName(std::string name) { m_name = name; }
};

void f(Obj& obj)
{
    auto lock = obj.lock();
    obj.waitForEvent(lock); // <-- last usage of lock, as much as the compiler sees
    // use obj setters; if implicit move happened, this is a data race
    obj.setName("new name");
}

Rewritten as cpp2:

Obj: type =
{
    lock: (this) -> _ = { return std::unique_lock(m_mutex); }
    // some setters

    waitForEvent: (this, inout l: std::unique_lock<std::mutex>) = {}

    // To allow calling waitForEvent(obj.lock());
    waitForEvent: (this, move _: std::unique_lock<std::mutex>) = {}

    setName: (this, name: std::string) = { m_name = name; }

    private m_mutex: std::mutex;
    private m_name: std::string;
}

f: (inout obj: Obj) = {
    lock := obj.lock();
    obj.waitForEvent(lock); // <-- last usage of lock, as much as the compiler sees
    // use obj setters; if implicit move happened, this is a data race
    obj.setName("new name");
}

Resulted with:

//=== Cpp2 type declarations ====================================================


#include "cpp2util.h"

#line 1 "test_implicit_move.cpp2"
class Obj;
#line 2 "test_implicit_move.cpp2"


//=== Cpp2 type definitions and function declarations ===========================

#line 1 "test_implicit_move.cpp2"
class Obj
#line 2 "test_implicit_move.cpp2"
 {
    public: [[nodiscard]] auto lock() const& -> auto;
    // some setters

    public: auto waitForEvent(std::unique_lock<std::mutex>& l) const& -> void;

    // To allow calling waitForEvent(obj.lock());
    public: auto waitForEvent([[maybe_unused]] std::unique_lock<std::mutex>&& unnamed_param_2) const& -> void;

    public: auto setName(cpp2::in<std::string> name) const& -> void;

    private: std::mutex m_mutex; 
    private: std::string m_name; 
    public: Obj() = default;
    public: Obj(Obj const&) = delete; /* No 'that' constructor, suppress copy */
    public: auto operator=(Obj const&) -> void = delete;

#line 15 "test_implicit_move.cpp2"
};

auto f(Obj& obj) -> void;

//=== Cpp2 function definitions =================================================

#line 1 "test_implicit_move.cpp2"

#line 3 "test_implicit_move.cpp2"
    [[nodiscard]] auto Obj::lock() const& -> auto{return std::unique_lock(m_mutex); }

#line 6 "test_implicit_move.cpp2"
    auto Obj::waitForEvent(std::unique_lock<std::mutex>& l) const& -> void{}

#line 9 "test_implicit_move.cpp2"
    auto Obj::waitForEvent([[maybe_unused]] std::unique_lock<std::mutex>&& unnamed_param_2) const& -> void{}

    auto Obj::setName(cpp2::in<std::string> name) const& -> void{m_name = name; }

#line 17 "test_implicit_move.cpp2"
auto f(Obj& obj) -> void{
    auto lock {CPP2_UFCS_0(lock, obj)}; 
    CPP2_UFCS(waitForEvent, obj, std::move(lock));// <-- last usage of lock, as much as the compiler sees
    // use obj setters; if implicit move happened, this is a data race
    CPP2_UFCS(setName, obj, "new name");
}

[gregmarr]

Yeah, scope guards and such need to be exempt from move from last use. Not sure how to differentiate between "raii object" and vector. Both have non-trivial destructors, but the "raii object" is there for its side effects, and vector for its data. Move from last use is fine for vector but not "raii object".

[YehezkelShB]

Even vector might hold elements that have similar effects.
As mentioned, this applies not only for locks etc. but also for shared_ptr, where moving from it invalidates the matching weak_ptrs (and of cours raw pointers/references used to observe the object or any sub-objects inside it).
My guess is that there is no way the compiler can understand all the dependecies between objects in arbitrary code

[tomFlowee]

lets continue talking on #901

[DyXel]

relates to #1002 (comment) as well

[hsutter]

Thanks! See #1030, this should now be fixed in the above commit.

The following (same as the top code with a couple of small unrelated fixes) used to compile and generate the move you're concerned about, and will now be a compile error:

Obj: type =
{
    lock: (inout this) -> _ = { return std::unique_lock(m_mutex); }
    // some setters

    waitForEvent: (this, inout _: std::unique_lock<std::mutex>) = {}

    // To allow calling waitForEvent(obj.lock());
    waitForEvent: (this, move _: std::unique_lock<std::mutex>) = {}

    setName: (inout this, name: std::string) = { m_name = name; }

    private m_mutex: std::mutex;
    private m_name: std::string;
}

f: (inout obj: Obj) = {
    lock := obj.lock();
    obj.waitForEvent(lock); // <-- last usage of lock, as much as the compiler sees
    // use obj setters; if implicit move happened, this is a data race
    obj.setName("new name");
}

The last use of lock will not be a move.