Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Assigning valueless that leaks @union #776

Closed
JohelEGP opened this issue Oct 23, 2023 · 3 comments
Closed

[BUG] Assigning valueless that leaks @union #776

JohelEGP opened this issue Oct 23, 2023 · 3 comments
Labels
bug Something isn't working

Comments

@JohelEGP
Copy link
Contributor

JohelEGP commented Oct 23, 2023

Title: Assigning valueless that leaks @union.

Description:

A @printed assignment looks like this.

    operator=:(
        out this, 
        in that
    ) = 
    {
        if that.is_l()
        {
            set_l(that.l());
        }
        _discriminator = that._discriminator;
    }

The object held by this is not destroyed if that is valueless.

Minimal reproducer (https://cpp2.godbolt.org/z/hnTKMaWMx):

#include <iostream>
logger: @value type = {
  operator=: (move this) = std::cout << "Destroy logger.\n";
}
t: @union type = {
  l: logger;
}
main: () = {
  (copy a := t()) a.set_l(); // OK, destroys the logger.
  (copy a := t()) {
    a.set_l();
    a = t();
  } // Not OK, logger leaked.
}
Commands:
cppfront main.cpp2
clang++18 -std=c++23 -stdlib=libc++ -lc++abi -pedantic-errors -Wall -Wextra -Wconversion -Werror=unused-result -I . main.cpp

Expected result:

Destroy logger.
Destroy logger.

Actual result and error:

Destroy logger.
Cpp2 lowered to Cpp1:
//=== Cpp2 type declarations ====================================================


#include "cpp2util.h"


#line 2 "x.cpp2"
class logger;
  

class t;
  

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

#include <iostream>
#line 2 "x.cpp2"
class logger {
  public: ~logger() noexcept;

public: [[nodiscard]] auto operator<=>([[maybe_unused]] logger const& that) const& -> std::strong_ordering = default;
public: logger([[maybe_unused]] logger const& that);
public: auto operator=([[maybe_unused]] logger const& that) -> logger& ;
public: logger([[maybe_unused]] logger&& that) noexcept;
public: auto operator=([[maybe_unused]] logger&& that) noexcept -> logger& ;
public: explicit logger();

#line 4 "x.cpp2"
};
class t {
private: std::aligned_storage_t<cpp2::max(sizeof(logger))> _storage {}; private: cpp2::i8 _discriminator {-1}; public: [[nodiscard]] auto is_l() const& -> bool;
public: [[nodiscard]] auto l() const& -> logger const&;
public: [[nodiscard]] auto l() & -> logger&;
public: auto set_l(cpp2::in<logger> _value) & -> void;
public: auto set_l(auto&& ..._args) & -> void;
private: auto _destroy() & -> void;
public: ~t() noexcept;
public: explicit t();
public: t(t const& that);

public: auto operator=(t const& that) -> t& ;
public: t(t&& that) noexcept;
public: auto operator=(t&& that) noexcept -> t& ;

#line 7 "x.cpp2"
};
auto main() -> int;
  

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


#line 3 "x.cpp2"
  logger::~logger() noexcept { std::cout << "Destroy logger.\n";  }


  logger::logger([[maybe_unused]] logger const& that){}

auto logger::operator=([[maybe_unused]] logger const& that) -> logger& {
                                return *this;}
logger::logger([[maybe_unused]] logger&& that) noexcept{}
auto logger::operator=([[maybe_unused]] logger&& that) noexcept -> logger& {
                                return *this;}
logger::logger(){}
[[nodiscard]] auto t::is_l() const& -> bool { return _discriminator == 0; }
[[nodiscard]] auto t::l() const& -> logger const& { 
                                                  cpp2::Default.expects(is_l(), "");return *cpp2::assert_not_null(reinterpret_cast<logger const*>(&_storage)); }
[[nodiscard]] auto t::l() & -> logger& { 
                                                        cpp2::Default.expects(is_l(), "");return *cpp2::assert_not_null(reinterpret_cast<logger*>(&_storage)); }
auto t::set_l(cpp2::in<logger> _value) & -> void{if (!(is_l())) {_destroy();std::construct_at(reinterpret_cast<logger*>(&_storage), _value);}else {*cpp2::assert_not_null(reinterpret_cast<logger*>(&_storage)) = _value;}_discriminator = 0;}
auto t::set_l(auto&& ..._args) & -> void{if (!(is_l())) {_destroy();std::construct_at(reinterpret_cast<logger*>(&_storage), _args...);}else {*cpp2::assert_not_null(reinterpret_cast<logger*>(&_storage)) = logger{_args...};}_discriminator = 0;}
auto t::_destroy() & -> void{
  if (_discriminator == 0) {std::destroy_at(reinterpret_cast<logger*>(&_storage));}
  _discriminator = -1;
  }

  t::~t() noexcept{_destroy();}
t::t()
                              : _discriminator{ -1 }{}
t::t(t const& that)
        : _storage{ that._storage }
        , _discriminator{ that._discriminator }{
  if (CPP2_UFCS_0(is_l, that)) {set_l(CPP2_UFCS_0(l, that));}
  _discriminator = that._discriminator;
  }

  auto t::operator=(t const& that) -> t& {
        _storage = that._storage;
        _discriminator = that._discriminator;
  if (CPP2_UFCS_0(is_l, that)) {set_l(CPP2_UFCS_0(l, that));}
  _discriminator = that._discriminator;
        return *this;
  }

  t::t(t&& that) noexcept
        : _storage{ std::move(that)._storage }
        , _discriminator{ std::move(that)._discriminator }{
  if (CPP2_UFCS_0(is_l, std::move(that))) {set_l(CPP2_UFCS_0(l, std::move(that)));}
  _discriminator = std::move(that)._discriminator;
  }

  auto t::operator=(t&& that) noexcept -> t& {
        _storage = std::move(that)._storage;
        _discriminator = std::move(that)._discriminator;
  if (CPP2_UFCS_0(is_l, std::move(that))) {set_l(CPP2_UFCS_0(l, std::move(that)));}
  _discriminator = std::move(that)._discriminator;
        return *this;
  }
#line 8 "x.cpp2"
auto main() -> int{
{
auto a = t();
#line 9 "x.cpp2"
  CPP2_UFCS_0(set_l, a);
}
{
auto a = t();     // OK, destroys the logger.
#line 10 "x.cpp2"
  {
    CPP2_UFCS_0(set_l, a);
    a = t();
  }
} // Not OK, logger leaked.
#line 14 "x.cpp2"
}

See also:

@JohelEGP

This comment was marked as resolved.

@JohelEGP
Copy link
Contributor Author

This is how the body of operator= should change to fix this issue:

         if that.is_l()
         {
             set_l(that.l());
         }
-        if that.is_a()
+        else if that.is_a()
         {
             set_a(that.a());
         }
+        else
+        {
+            destroy();
+        }
         _discriminator = that._discriminator;

@JohelEGP
Copy link
Contributor Author

JohelEGP commented Nov 8, 2023

Resolved by commit cdf71bd.

@JohelEGP JohelEGP closed this as completed Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant