Maybe it's just a big blunder, but I think there's something wrong in phobos.

All started a couple of days ago, when I was investigating on some not-freed resource on my code.

It turns out that on "each" function inside phobos it is used a "ref foreach". Something like:

foreach(ref element, myRange) {}

If myRange.front() doesn't return a ref, this syntax probably should be rejected, but it is accepted. So for each iterated element a ctor is called but not a dtor.

Discussing this with Rikki we discovered a similar bug is still opened [1] but was not related with missing dtor call, actually.

If "each" maybe is not so used, a similar problem popped out in another part of my code, using std.array : array. Apparently there is no "for-each-ref" code there.

That's fun because I was actually working on a workaround for another bug [2] I've found in the same code base.

Anyway code [3] (assumePure is a workaround for [2]) shows that 4 dtor call are missing (element constructed by array).

I wonder how diffused is this bug considering that "array" is a pretty used function. This leads to a leak on my code.

I hope I'm wrong. Maybe I'm missing something. Anyone can help?

Andrea

[1] https://issues.dlang.org/show_bug.cgi?id=11934
[2] https://forum.dlang.org/post/kfaakznkdnnoxwcrgtsg@forum.dlang.org
[3] https://run.dlang.io/is/IMqYzK

Andrea Fontana wrote:

> I hope I'm wrong. Maybe I'm missing something. Anyone can help?

you are not wrong, this is bug in DMDFE.

On 6/15/18 10:51 AM, ketmar wrote:
> Andrea Fontana wrote:
> 
>> I hope I'm wrong. Maybe I'm missing something. Anyone can help?
> 
> you are not wrong, this is bug in DMDFE.

Yep.

I tried using -vcg-ast, and see an interesting lowering:

for (; !__r115.empty(); __r115.popFront())
{
    ref A r = __r115.front();
...

I believe that what happens is r really is a ref to... a temporary return. Because the compiler at this point won't re-allocate that memory to something else, it works, but this is actually a memory bug too!

The destructor likely is not added, because the AST doesn't show any variable that needs it! r is a ref.

At the very least, the code should allocate a temporary and assign a ref to that, so lifetime management works properly. I agree the better path is to simply disallow ref, but this would be a non-code-breaking step that would fix the issue of lifetime management.

Note that in D1, only opApply was the way to use foreach, and it *requires* ref for the element, even if your foreach didn't use ref (and even if the actual thing you are iterating didn't make sense as ref). It was actually impossible to duplicate array foreach, which would not allow you to ref the index. You'd have to create a temporary index, send it into the delegate, and ignore any changes.

The lowering for ranges must have tried to simulate this ability (allowing you to use ref or non-ref despite what the actual range provides), but this kludge doesn't seem to be implemented properly.

-Steve

On Friday, 15 June 2018 at 15:10:54 UTC, Steven Schveighoffer wrote:
> I tried using -vcg-ast, and see an interesting lowering:
>
> for (; !__r115.empty(); __r115.popFront())
> {
>     ref A r = __r115.front();
> ...
>

Also for std.array:array?

I don't get where it is using foreach with refs.
Can be this [1] the problem instead?

[1] https://github.com/dlang/phobos/blob/master/std/array.d#L126

Andrea

On 6/15/18 11:18 AM, Andrea Fontana wrote:
> On Friday, 15 June 2018 at 15:10:54 UTC, Steven Schveighoffer wrote:
>> I tried using -vcg-ast, and see an interesting lowering:
>>
>> for (; !__r115.empty(); __r115.popFront())
>> {
>>     ref A r = __r115.front();
>> ...
>>
> 
> Also for std.array:array?
> 
> I don't get where it is using foreach with refs.
> Can be this [1] the problem instead?
> 
> [1] https://github.com/dlang/phobos/blob/master/std/array.d#L126
> 
> Andrea

That is a separate bug. The array function appears not to set the correct bits for the GC block. It should set the appendable bit, and the finalize and struct finalize bits. All it sets is the no-scan bit. It also needs to set up the allocated size properly. I think uninitializedArray is a bad idea for array to use when the type being inserted has a dtor.

So the code is executing all the proper postblits and destructors, it's just that the GC is not executing the destructors.

Can you file a bug or search for one on this? I will see if I can fix it.

-Steve

On Friday, 15 June 2018 at 15:49:01 UTC, Steven Schveighoffer wrote:
> Can you file a bug or search for one on this? I will see if I can fix it.
>
> -Steve

https://issues.dlang.org/show_bug.cgi?id=18995