www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.learn - auto ref function parameter causes that non copyable struct is copied?

reply tchaloupka <chalucha gmail.com> writes:
Lets have this code:

```D
import core.lifetime : forward;
import core.stdc.stdio;
import std.algorithm : move;

struct Value(T) {
     private T storage;

     this()(auto ref T val) {
         storage = forward!val;
     }

     ref inout(T) get() inout {
         return storage;
     }
}

Value!T value(T)(auto ref T val) {
     return Value!T(forward!val);
}

auto ref unwrap(EX)(auto ref EX res) {
     return res.get();
}

struct Foo {
     int n;
      disable this(this);
     ~this() { printf("~this(%d)\n", n); }
}

auto gen() {
     Foo f;
     f.n = 42;
     return value(f.move());
}

void main() {
     Foo f;
     f = gen().unwrap.move;
}
```

As I understand it unwrap in `f = gen().unwrap.move` can't be 
called by ref (as gen() returns rvalue) so it would force the 
`Value` to be copied but as it holds non copyable struct, it 
can't be and so it should end up with a compiler error.

But the code outputs:
```
~this(0)
~this(0)
~this(0)
~this(42) <- this is a copy (that shouldn't exist) being destroyed
~this(0)
~this(42)
```

This could cause unwanted resource cleanup on a seemingly non 
copyable structs.
Bug or feature? :)
Nov 08 2021
next sibling parent reply Stanislav Blinov <stanislav.blinov gmail.com> writes:
On Monday, 8 November 2021 at 23:26:39 UTC, tchaloupka wrote:


 ```
 auto gen() {
     Foo f;                   // <--- this one
     f.n = 42;
     return value(f.move());
 }

 void main() {
     Foo f;
     f = gen().unwrap.move;
 }
 ```
 ~this(0)
 ~this(0)
 ~this(0)
 ~this(42) <- this is a copy (that shouldn't exist) being 
 destroyed
 ~this(0)
 ~this(42)



Is it a copy? I think the first destructor call is one of `f` in 
`gen` (marked by the comment above)
Nov 08 2021
parent reply jfondren <julian.fondren gmail.com> writes:
On Tuesday, 9 November 2021 at 02:19:28 UTC, Stanislav Blinov 
wrote:

 On Monday, 8 November 2021 at 23:26:39 UTC, tchaloupka wrote:


 ```
 auto gen() {
     Foo f;                   // <--- this one
     f.n = 42;
     return value(f.move());
 }

 void main() {
     Foo f;
     f = gen().unwrap.move;
 }
 ```
 ~this(0)
 ~this(0)
 ~this(0)
 ~this(42) <- this is a copy (that shouldn't exist) being 
 destroyed
 ~this(0)
 ~this(42)



 Is it a copy? I think the first destructor call is one of `f` 
 in `gen` (marked by the comment above)


The expectation is probably that `f.move` set `f` to `Foo.init`, 
but the docs say:

"If T is a struct with a destructor or postblit defined, source 
is reset to its .init value after it is moved into target, 
otherwise it is left unchanged."

```d
struct A { int x; }
struct B { int x; this(this) { } }

unittest {
     import core.lifetime : move;
     auto a = A(5);
     auto b = B(5);
     a.move;
     b.move;
     assert(a == A(5));
     assert(b == B(0));
}
```
Nov 08 2021
parent reply jfondren <julian.fondren gmail.com> writes:
On Tuesday, 9 November 2021 at 02:41:18 UTC, jfondren wrote:

 The expectation is probably that `f.move` set `f` to 
 `Foo.init`, but the docs say:


Posted too fast. Foo qualifies with its  disable:

```d
struct A { int x; }
struct B { int x;  disable this(this); }

unittest {
     import core.lifetime : move;
     auto a = A(5);
     auto b = B(5);
     a.move;
     b.move;
     assert(a == A(5));
     assert(b == B(0));
}
```
Nov 08 2021
parent tchaloupka <chalucha gmail.com> writes:
On Tuesday, 9 November 2021 at 02:43:55 UTC, jfondren wrote:

 On Tuesday, 9 November 2021 at 02:41:18 UTC, jfondren wrote:

 The expectation is probably that `f.move` set `f` to 
 `Foo.init`, but the docs say:


 Posted too fast. Foo qualifies with its  disable:

 ```d
 struct A { int x; }
 struct B { int x;  disable this(this); }

 unittest {
     import core.lifetime : move;
     auto a = A(5);
     auto b = B(5);
     a.move;
     b.move;
     assert(a == A(5));
     assert(b == B(0));
 }
 ```


Yes it should qualify so it should be cleaned out when moved.
When I change:

```D
auto ref unwrap(EX)(auto ref EX res) {
     printf("unwrap()\n");
     return res.get();
}
```

I get:

```
~this(0)
~this(0)
~this(0)
unwrap()
~this(42)
~this(0)
~this(42)
```

So the destructor isn't called from `gen()` -> there is 
[NRVO](https://dlang.org/glossary.html#nrvo) in play.

Also `pragma(msg, __traits(isRef, res));` prints false in unwrap 
in this case (which is expected), but how it gets there as a 
value when it's not moved and instead copied even though it has 
disabled copy constructor?
`isCopyable!(Value!Foo)` returns false as expected too.
Nov 08 2021
prev sibling parent reply tchaloupka <chalucha gmail.com> writes:
On Monday, 8 November 2021 at 23:26:39 UTC, tchaloupka wrote:

 Bug or feature? :)


I've reported it in 
https://issues.dlang.org/show_bug.cgi?id=22498.
Nov 09 2021
parent Sebastiaan Koppe <mail skoppe.eu> writes:
On Tuesday, 9 November 2021 at 19:30:20 UTC, tchaloupka wrote:

 On Monday, 8 November 2021 at 23:26:39 UTC, tchaloupka wrote:

 Bug or feature? :)


 I've reported it in 
 https://issues.dlang.org/show_bug.cgi?id=22498.


It doesn't copy. It just destructs the same object twice... Ouch.

Change the destructor to `~this() { printf("~this(%d)\n", n); n = 
12; }` and you'll see.

I have been looking at LDC's IR of this. `unwrap` takes a lvalue, 
but the compiler puts the object from `gen` on main's stack and 
gives a pointer to unwrap instead. `unwrap` is now the sole owner 
of the object, and calls the destructor before returning a ref to 
one of its fields.

I haven't figured out the interplay of features and optimizations 
here, but it looks like a leak. I would have expected  safe to 
catch this.

As you said in the bug, NVRO solves it (not sure why). As well as 
not passing a lvalue into `unwrap` in the first place.
Nov 10 2021