DIP 1022--foreach auto ref--Community Review Round 1

[Manu]

On Sat, Aug 10, 2019 at 1:11 AM Dukc via Digitalmars-d
<digitalmars-d at puremagic.com> wrote:
>
> On Friday, 9 August 2019 at 21:55:45 UTC, Manu wrote:
> >
> > Sorry if that sounded blunt.
>
> No, not at all. I was just surprised that you questioned my core
> resoning, yet wanted the dip to move forward.
>
> >
> > Let me try and be more clear; by comparison, we can consider
> > [Snip]
> >
> > I suspect from reading that you are trying to solve the second
> > case
> > with this DIP, but it's already solved by Andrei's work (which
> > he
> > presented at dconf).
> > What this DIP should actually solve though, in the 3rd case. We
> > need that.
>
> Hmm, you want to retain the current behaviour of `ref`? This
> confuses me, because `auto ref` I'm proposing has exactly the
> same semantics as `ref` does now, yet you also want it to be
> included.
>
> My guess is that you're thinking the benefit of `auto ref` to be
> allowing to handle rvalue ranges without becoming a pointer
> internally, thus being more efficient in at least some cases. The
> rest of my answer assumes that was your point.
>
> I disagree. I am not trying to target efficiency issues a all. If
> my problem was that, I'd simply propose foreach `ref` to use
> rvalue copies directly, not via internal pointers. Unlike in C++,
> refs and the values they point to are indistinguishable from D
> user perspective, so that is just an implementation detail that
> would probably get through even without a DIP. Unless it's
> already done.
>
> The problem I'm targeting is that currently the compiler will not
> complain, if the user, when expecting to mutate range elements,
> iterates by `ref` over foreach. But since I know there's need for
> the notion I'm deprecating, I also suggest `auto ref` for the
> behaviour.
>
> I know this is a kind of a jerk change, as it forces to change
> code. But I thought that the migration path is so simple (just
> changing `ref` to `auto ref` where you get deprecation messages)
> that avoiding it (see alternative 1 of the DIP) is not worth
> having a language inconsistency.

I'm thought about it some more, and I think you're right about the deprecation.
I think I can agree that the probability of confusion + mistake where
a ref to temporary exists is higher weighted than the uniformity with
function calling.
I tend to hate special-case rules (in this case, loop counters
behaving differently than function arguments), but I think I'm
persuaded here.

I support your text unamended ;) .. we've needed `auto ref` in foreach
for a long time.

I'm curious about this line: "It should be allowed in static foreach,
but with no effect, as elements of compile-time aggregates can never
be lvalues."

Is that true? I don't think there's any reason they can't be lvalues.
But what I'm trying to get my head around is if the loop counter for
`static foreach` is a variable at all, or if it's an alias?

I think there's inherent complexity here, because there's a lot of
cases... and some should work with auto ref, but not all.

Okay, here's some experiment cases:

  int x = 1;
  static foreach (i; AliasSeq!(10, x, 20))
      pragma(msg, __traits(isRef, i));

> false false false

In this case, `i` is an alias. Ref-ness doesn't mean anything here.

This can't work:

  int x = 1;
  static foreach (int i; AliasSeq!(10, x, 20))
      pragma(msg, __traits(isRef, i));

> Error: can't initialise `i` because `x` is a runtime value

Because it tries to initialise an int from elements of tuple, and `x`
is a runtime variable.

This could work if ref was treated the same way as parameter arguments:

  int x = 1;
  static foreach (ref int i; AliasSeq!(10, x, 20))
      pragma(msg, __traits(isRef, i));

> true true true

In that case, rvalues (which can be determined at compile time) would
populate temporaries, the same way as function parameters.
But according to the rules in your dip, we don't allow that, and I
conceded above.

This should work though:

  int x = 1;
  static foreach (ref int i; AliasSeq!(x, x, x))
      pragma(msg, __traits(isRef, i));

> true true true

All elements are lvalues, so we accept `ref`.

Finally, this should work as expected:

  int x = 1;
  static foreach (auto ref int i; AliasSeq!(10, x, 20))
      pragma(msg, __traits(isRef, i));

> false true false

Functionally identical to the first case, except the interesting
distinction that `i` contains information which can be used in
introspection logic, which is almost always present in static foreach
bodies.
I believe this should be allowed AND behave as expected; that is; `i`
is a ref when encountering an lvalue.
If it does not behave in this way, it should not be allowed.


Now, there's a different set of cases, where you call static foreach
over a not-a-tuple:

  static foreach (i; [10, 20, 30])
      pragma(msg, __traits(isRef, i));

> false false false

In this case, `i` is not an alias like the previous experiment, it is
inferred to be `int`.
Should be the same as `static foreach (int i; ...)`, right?

  static foreach (ref i; [10, 20, 30])
      pragma(msg, __traits(isRef, i));

> true true true

As above, we could allow this by creating temporaries the same as
function arguments... but we've decided not to allow ref iterators
from rvalues.

  static foreach (auto ref i; [10, 20, 30])
      pragma(msg, __traits(isRef, i));

> false false false ??

I guess the whole array is an rvalue, so then the loop counter would
take copies of the elements?

It gets interesting when you do this:

  int[3] x = [10, 20, 30];
  static foreach (ref i; x)
      pragma(msg, __traits(isRef, i));

> true true true

And:

  int[3] x = [10, 20, 30];
  static foreach (auto ref i; x)
      pragma(msg, __traits(isRef, i));

> true true true

These should work as expected.


TL;DR, your sentence "It should be allowed in static foreach, but with
no effect" should be removed, and if you want to detail the expected
semantics with `static foreach`, that might be a good idea.