Memory leak in foreach

[H. S. Teoh]

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.