This subtle bug[1] and its memory leak are still here because my pull request[2] had no success.

It worths noting that since each() in std.algorithm uses a foreach+ref to iterate  structs[3], this memory leak probably is silently affecting not only me :)

I hope someone could fix this.

[1] https://issues.dlang.org/show_bug.cgi?id=11934
[2] https://github.com/dlang/dmd/pull/8437
[3] https://github.com/dlang/phobos/blob/master/std/algorithm/iteration.d#L983

On Wednesday, 20 February 2019 at 09:31:30 UTC, Andrea Fontana wrote:
> This subtle bug[1] and its memory leak are still here because my pull request[2] had no success.
>
> It worths noting that since each() in std.algorithm uses a foreach+ref to iterate  structs[3], this memory leak probably is silently affecting not only me :)
>
> I hope someone could fix this.
>
> [1] https://issues.dlang.org/show_bug.cgi?id=11934
> [2] https://github.com/dlang/dmd/pull/8437
> [3] https://github.com/dlang/phobos/blob/master/std/algorithm/iteration.d#L983

Does that patch ignore the ref when you specify it in a foreach loop?

If it does, then that sounds like it should be a compiler error and not ignored. A compiler should not change the meaning of my code silently just to make compilation work. We have javascript for that no?

Or did I misunderstand the patch?

On Wednesday, 20 February 2019 at 09:31:30 UTC, Andrea Fontana wrote:
> This subtle bug[1] and its memory leak are still here because my pull request[2] had no success.
>
> It worths noting that since each() in std.algorithm uses a foreach+ref to iterate  structs[3], this memory leak probably is silently affecting not only me :)
>
> I hope someone could fix this.
>
> [1] https://issues.dlang.org/show_bug.cgi?id=11934
> [2] https://github.com/dlang/dmd/pull/8437
> [3] https://github.com/dlang/phobos/blob/master/std/algorithm/iteration.d#L983

That's not a memory leak, you are creating a GC array, the GC might never free the array its not deterministic. Use @nogc and staticArray() if you want to destructors to be called deterministically.

On Wednesday, 20 February 2019 at 11:05:31 UTC, Dein wrote:
> That's not a memory leak, you are creating a GC array, the GC might never free the array its not deterministic. Use @nogc and staticArray() if you want to destructors to be called deterministically.

No: GC will not free those elements in this case. There's a bug. The only way to free those elements is to kill the application. GC doesn't track them at all.

On Wednesday, 20 February 2019 at 11:05:31 UTC, Dein wrote:
> On Wednesday, 20 February 2019 at 09:31:30 UTC, Andrea Fontana wrote:
>> This subtle bug[1] and its memory leak are still here because my pull request[2] had no success.
>>
>> It worths noting that since each() in std.algorithm uses a foreach+ref to iterate  structs[3], this memory leak probably is silently affecting not only me :)
>>
>> I hope someone could fix this.
>>
>> [1] https://issues.dlang.org/show_bug.cgi?id=11934
>> [2] https://github.com/dlang/dmd/pull/8437
>> [3] https://github.com/dlang/phobos/blob/master/std/algorithm/iteration.d#L983
>
> That's not a memory leak, you are creating a GC array, the GC might never free the array its not deterministic. Use @nogc and staticArray() if you want to destructors to be called deterministically.


If you check the example you will notice that for each element inside the array is created a copy by foreach. Those temporary copies are never freed (bug). So each time you are going to iterate through your array (using ref) you are filling  memory with additional copies of your elements.

On Wednesday, 20 February 2019 at 10:50:50 UTC, aliak wrote:
> On Wednesday, 20 February 2019 at 09:31:30 UTC, Andrea Fontana wrote:
>> This subtle bug[1] and its memory leak are still here because my pull request[2] had no success.
>>
>> It worths noting that since each() in std.algorithm uses a foreach+ref to iterate  structs[3], this memory leak probably is silently affecting not only me :)
>>
>> I hope someone could fix this.
>>
>> [1] https://issues.dlang.org/show_bug.cgi?id=11934
>> [2] https://github.com/dlang/dmd/pull/8437
>> [3] https://github.com/dlang/phobos/blob/master/std/algorithm/iteration.d#L983
>
> Does that patch ignore the ref when you specify it in a foreach loop?
>
> If it does, then that sounds like it should be a compiler error and not ignored. A compiler should not change the meaning of my code silently just to make compilation work. We have javascript for that no?
>
> Or did I misunderstand the patch?

Let me explain better.

Currently this is what is happening (if I'm right!):
- You have a range with struct elements
- You try to iterate this range using a foreach+ref.
- This range returns a copy of each element (a temporary value)
- You work on this copy by ref (it doesn't even make sense to use a ref to a  temporary value)
- Compiler doens't free this copy because it thinks it's a reference! <- leak

My idea: fix the current behaviour, freeing the elements created and then fix the language with a warning/error/deprecation/DIP/whatever

Please consider that throwing an error is going to brake all code using (for example) each() to iterate a range returning a struct, probably.

Should it throw an error? Don't know. Probably yes. But I feel it could add a lot of complexity to code: if you're writing generic code you have always to check if you're iterating a ref-returning range or not. In first case you can use foreach(ref) in the second case you have to use foreach().

Anyway I think it's still better to fix current behaviour and waiting for a decision than leaving this leak open for who knows how long...

On Wed, Feb 20, 2019 at 02:22:51PM +0000, Andrea Fontana via Digitalmars-d wrote: [...]
> Currently this is what is happening (if I'm right!):
> - You have a range with struct elements
> - You try to iterate this range using a foreach+ref.
> - This range returns a copy of each element (a temporary value)
> - You work on this copy by ref (it doesn't even make sense to use a ref to a
> temporary value)
> - Compiler doens't free this copy because it thinks it's a reference! <-
> leak

Confirmed: if I have a range of structs that have a dtor, then foreach over the range (without `ref`) correctly calls the dtor for each returned struct, but foreach with `ref` fails to invoke the dtor for the returned struct, even though .front is returning by value.

I think this is a bug.  It should be fixed, and you shouldn't need to do any deprecation cycle because not calling the dtor is wrong behaviour that fails to implement the language spec.


> My idea: fix the current behaviour, freeing the elements created and then fix the language with a warning/error/deprecation/DIP/whatever

The dtor should be called for structs that have a dtor. Classes are supposed to be managed by the GC and shouldn't need any change. No warning/deprecation needs to be done because this is a bug.

To be more precise, if .front returns by value a struct that has a dtor, then it needs to be destructed whether or not `foreach(ref...)` is used. If .front returns something by reference, then it should NOT be destructed regardless of whether you use `foreach` or `foreach(ref...)`.


> Please consider that throwing an error is going to brake all code
> using (for example) each() to iterate a range returning a struct,
> probably.

No, it should not throw anything.


> Should it throw an error? Don't know. Probably yes. But I feel it could add a lot of complexity to code: if you're writing generic code you have always to check if you're iterating a ref-returning range or not. In first case you can use foreach(ref) in the second case you have to use foreach().

No, it shouldn't throw an error.  All that's needed is to invoke the dtor when it needs to be invoked, that's all.  Generic code shouldn't have to care whether or not the dtor is invoked -- that's the whole point of the user type defining a dtor; callers shouldn't have to know how to destroy the type themselves, the type should handle it. Similarly, generic code shouldn't care whether there is a dtor; the language should invoke the dtor where necessary.


> Anyway I think it's still better to fix current behaviour and waiting for a decision than leaving this leak open for who knows how long...
[...]

The fix is just to invoke the dtor based on whether .front returns by ref or not, rather than whether `foreach(ref)` was used.

It's a different question of whether it should be a compile error to use `foreach(ref)` on a range whose .front doesn't return by ref.  That's a much more sticky issue that you may not want to get into. :-D  Fix the dtor bug first, then we can decide whether we want to pursue this question.


T

-- 
To provoke is to call someone stupid; to argue is to call each other stupid.

P.S. And BTW, this bug is not directly related to memory leaks. It relates to the dtor not being called when it should be.  Whether or not it's a memory leak depends on what the dtor is doing (e.g. it may be leaking other resources than memory).  In your PR / bugzilla issue you should probably phrase it in terms of dtors rather than memory leaks, which may cause misunderstandings.


T

-- 
What is Matter, what is Mind? Never Mind, it doesn't Matter.

On Wednesday, 20 February 2019 at 15:52:06 UTC, H. S. Teoh wrote:
> It's a different question of whether it should be a compile error to use `foreach(ref)` on a range whose .front doesn't return by ref.  That's a much more sticky issue that you may not want to get into. :-D  Fix the dtor bug first, then we can decide whether we want to pursue this question.

Eh, you summed up my idea perfectly.
Isn't exactly what my pull request is doing?

Andrea

On Wednesday, 20 February 2019 at 16:16:27 UTC, Andrea Fontana wrote:
> On Wednesday, 20 February 2019 at 15:52:06 UTC, H. S. Teoh wrote:
>> It's a different question of whether it should be a compile error to use `foreach(ref)` on a range whose .front doesn't return by ref.  That's a much more sticky issue that you may not want to get into. :-D  Fix the dtor bug first, then we can decide whether we want to pursue this question.
>
> Eh, you summed up my idea perfectly.
> Isn't exactly what my pull request is doing?
>
> Andrea

(My pr ignores ref for that case so dtor is called and memory freed)