On Monday, 10 August 2020 at 15:36:35 UTC, Kagamin wrote:
> On Monday, 10 August 2020 at 13:44:17 UTC, Avrina wrote:
>> This really doesn't excuse why this bug is happening. Why is writeln() using front() and popFront() that modifies the range rather than simply using foreach()? Why is it modifying something it knows to be a class? This is definitely a bug that should be fixed. Not sure why people are trying to explain it away, looking that the writeln implementation it is completely convoluted. It's no wonder bugs like this are going to happen.
>
> When writeln receives a reference type range, are you sure it shouldn't be consumed? How do you decide that?

Because `writeln` should be a *view* (i.e read-only) function, it shouldn't *internally* call anything thing that change the data passed in.

Only the user can *explicitly* change the data, and let writeln print it: e.g.
```
  writeln(data.change_And_Print_The_State_After_The_Change());
```

On Monday, 10 August 2020 at 18:29:56 UTC, mw wrote:
> and where in this doc on range:
>
> https://tour.dlang.org/tour/en/basics/ranges
>
> it is mentioned that: the length property of the range will be changed after the range is "consumed" by foreach loop?
>
> ```
> foreach (element; range)
> {
>     // Loop body...
> }
> it's internally rewritten similar to the following:
>
> for (auto __rangeCopy = range;
>      !__rangeCopy.empty;
>      __rangeCopy.popFront())
>  {
>     auto element = __rangeCopy.front;
>     // Loop body...
> }
> ```

And wait, ... did I saw "__rangeCopy" here?  it should be a *copy*?!

On 8/10/20 2:36 PM, mw wrote:
> On Monday, 10 August 2020 at 18:29:56 UTC, mw wrote:
>> and where in this doc on range:
>>
>> https://tour.dlang.org/tour/en/basics/ranges
>>
>> it is mentioned that: the length property of the range will be changed after the range is "consumed" by foreach loop?
>>
>> ```
>> foreach (element; range)
>> {
>>     // Loop body...
>> }
>> it's internally rewritten similar to the following:
>>
>> for (auto __rangeCopy = range;
>>      !__rangeCopy.empty;
>>      __rangeCopy.popFront())
>>  {
>>     auto element = __rangeCopy.front;
>>     // Loop body...
>> }
>> ```
> 
> And wait, ... did I saw "__rangeCopy" here?  it should be a *copy*?!
> 
> 

In your code, the type of "range" is SharedArray!(string) which is a class or *reference type*.

So let's follow along what happens:

SharedArray!(string) fns;
for(auto __rangeCopy = fns;

// the above makes a copy of a *class reference*, which means it does not make a copy of the *array data* or even the array reference. It's like copying a pointer to the array.

    !__rangeCopy.empty;

// SharedArray(T) does not have empty member, so this forwards to the array, it's like saying:
// !__rangeCopy.array.empty

    __rangeCopy.popFront())

// This is equivalent to __rangeCopy.array.popFront, which alters the array inside the ONE SHARED class instance.

So now, what ends up happening is because fns and the __rangeCopy are actually just copies of a pointer, or class reference, when you iterate one, the other is iterated.

How to fix? extract the array before iterating:

writeln(s0.fns.array);

This will make a copy of the array reference itself, and iterate that.

This is precisely why classes should never be ranges.

-Steve

On Monday, 10 August 2020 at 19:30:18 UTC, Steven Schveighoffer wrote:
> On 8/10/20 2:36 PM, mw wrote:
>> On Monday, 10 August 2020 at 18:29:56 UTC, mw wrote:
>>> and where in this doc on range:
>>>
>>> https://tour.dlang.org/tour/en/basics/ranges
>>>
>>> it is mentioned that: the length property of the range will be changed after the range is "consumed" by foreach loop?
>>>
>>> ```
>>> foreach (element; range)
>>> {
>>>     // Loop body...
>>> }
>>> it's internally rewritten similar to the following:
>>>
>>> for (auto __rangeCopy = range;
>>>      !__rangeCopy.empty;
>>>      __rangeCopy.popFront())
>>>  {
>>>     auto element = __rangeCopy.front;
>>>     // Loop body...
>>> }
>>> ```
>> 
>> And wait, ... did I saw "__rangeCopy" here?  it should be a *copy*?!
>> 
>> 
>
> In your code, the type of "range" is SharedArray!(string) which is a class or *reference type*.
>
> So let's follow along what happens:
>
> SharedArray!(string) fns;
> for(auto __rangeCopy = fns;
>
> // the above makes a copy of a *class reference*, which means it does not make a copy of the *array data* or even the array reference. It's like copying a pointer to the array.
>
>     !__rangeCopy.empty;
>
> // SharedArray(T) does not have empty member, so this forwards to the array, it's like saying:
> // !__rangeCopy.array.empty
>
>     __rangeCopy.popFront())
>
> // This is equivalent to __rangeCopy.array.popFront, which alters the array inside the ONE SHARED class instance.


This still doesn't explain why the underlying array.length is modified after the range to consumed; too much black magic is happening here.

> How to fix? extract the array before iterating:
>
> writeln(s0.fns.array);

This defeats the purpose, i.e. the convenience that subtyping mechanism supposed to provide.

On Monday, 10 August 2020 at 19:30:18 UTC, Steven Schveighoffer wrote:
> This is precisely why classes should never be ranges.

And the user didn't code this range, the range is provided by the language standard library:

https://dlang.org/phobos/std_range_primitives.html

it's the std library that didn't take into account of the array-be-subtyped-as class loop-hole.

Ideally, to the user, subtyping should just perform simple forward to the underlying data member, i.e.

writeln(s0.fns);

should be translated by the *compiler* to:

writeln(s0.fns.array);

Obviously it failed to do so here.

On 8/10/20 3:39 PM, mw wrote:
> On Monday, 10 August 2020 at 19:30:18 UTC, Steven Schveighoffer wrote:
>> On 8/10/20 2:36 PM, mw wrote:
>>> On Monday, 10 August 2020 at 18:29:56 UTC, mw wrote:
>>>> and where in this doc on range:
>>>>
>>>> https://tour.dlang.org/tour/en/basics/ranges
>>>>
>>>> it is mentioned that: the length property of the range will be changed after the range is "consumed" by foreach loop?
>>>>
>>>> ```
>>>> foreach (element; range)
>>>> {
>>>>     // Loop body...
>>>> }
>>>> it's internally rewritten similar to the following:
>>>>
>>>> for (auto __rangeCopy = range;
>>>>      !__rangeCopy.empty;
>>>>      __rangeCopy.popFront())
>>>>  {
>>>>     auto element = __rangeCopy.front;
>>>>     // Loop body...
>>>> }
>>>> ```
>>>
>>> And wait, ... did I saw "__rangeCopy" here?  it should be a *copy*?!
>>>
>>>
>>
>> In your code, the type of "range" is SharedArray!(string) which is a class or *reference type*.
>>
>> So let's follow along what happens:
>>
>> SharedArray!(string) fns;
>> for(auto __rangeCopy = fns;
>>
>> // the above makes a copy of a *class reference*, which means it does not make a copy of the *array data* or even the array reference. It's like copying a pointer to the array.
>>
>>     !__rangeCopy.empty;
>>
>> // SharedArray(T) does not have empty member, so this forwards to the array, it's like saying:
>> // !__rangeCopy.array.empty
>>
>>     __rangeCopy.popFront())
>>
>> // This is equivalent to __rangeCopy.array.popFront, which alters the array inside the ONE SHARED class instance.
> 
> 
> This still doesn't explain why the underlying array.length is modified after the range to consumed; too much black magic is happening here.

Indeed there is a lot of magic. It's ordinary every-day D magic though ;)

string[] arr = ["abc"];
arr.popFront; // ufcs function defined in std.range.primitives
assert(arr.length == 0); // reduces the length

If we inlined this fully with the definition of std.range.primitives.popFront, the line:

__rangeCopy.popFront()

is really doing:

__rangeCopy.array = __rangeCopy.array[1 .. $];

This is how you iterate an array as a range.

>> How to fix? extract the array before iterating:
>>
>> writeln(s0.fns.array);
> 
> This defeats the purpose, i.e. the convenience that subtyping mechanism supposed to provide.

You are subtyping but inadvertently have turned a forward range (array) into an input range (iterate only once) by changing it into a class.

forward ranges aren't supposed to technically be valid if you don't *save* them, but in practice it's totally fine for arrays.

What might work (and I haven't tried this) is to @disable front, popFront, empty, and I think what will then happen is the compiler will try slicing instead (which should work) and iterate a copy of the array.

e.g.:

class SharedArray!T
{
   T[] array;
   alias array this;

   @disable:
      front();
      popFront();
      empty();
}

-Steve

On 8/10/20 3:49 PM, mw wrote:
> Ideally, to the user, subtyping should just perform simple forward to the underlying data member, i.e.
> 
> writeln(s0.fns);
> 
> should be translated by the *compiler* to:
> 
> writeln(s0.fns.array);
> 
> Obviously it failed to do so here.

It would never do that. Because classes are based on Object, and writeln is defined to accept an Object and print it that way (using Object.toString). So even if the range methods weren't available, it wouldn't instead pass the base type.

What's happening is that inadvertently, you have created a range type. writeln is going to PREFER using range mechanisms over Object.toString, so when it sees it can use range primitives, it uses those. This is the danger of duck-typing.

What you want is array functionality, not range functionality on the class. So you need to avoid those mechanisms. Try the disable idea, see if it works.

-Steve

>> This defeats the purpose, i.e. the convenience that subtyping mechanism supposed to provide.
>
> You are subtyping but inadvertently have turned a forward range (array) into an input range (iterate only once) by changing it into a class.

This subtyping loophole should be fixed by the compiler.


> What might work (and I haven't tried this) is to @disable front, popFront, empty, and I think what will then happen is the compiler will try slicing instead (which should work) and iterate a copy of the array.
>
> e.g.:
>
> class SharedArray!T
> {
>    T[] array;
>    alias array this;
>
>    @disable:
>       front();
>       popFront();
>       empty();
> }

This does not work:
```
 Error: no identifier for declarator front()
 Error: function declaration without return type. (Note that constructors are always named this)
 Error: no identifier for declarator popFront()
 Error: function declaration without return type. (Note that constructors are always named this)
 Error: no identifier for declarator empty()
```

looks like `@disable` expect the full function implementation with body:

https://dlang.org/spec/attribute.html#disable


BTW, this way of (thinking) fixing the problem is one step closer to Eiffel's mechanism: i.e. member level tailored inheritance.

Sigh, this is not done earlier into the language.

On Monday, 10 August 2020 at 20:13:42 UTC, mw wrote:
>> class SharedArray!T
>> {
>>    T[] array;
>>    alias array this;
>>
>>    @disable:
>>       front();
>>       popFront();
>>       empty();
>> }
>
> This does not work:

This does and prints the desired output:

class SharedArray(T)
{
  T[] array;
  alias array this;

  final @disable:
  T front();
  void popFront();
  bool empty();
}

https://run.dlang.io/is/5ciCue

On 8/10/20 4:13 PM, mw wrote:
>>> This defeats the purpose, i.e. the convenience that subtyping mechanism supposed to provide.
>>
>> You are subtyping but inadvertently have turned a forward range (array) into an input range (iterate only once) by changing it into a class.
> 
> This subtyping loophole should be fixed by the compiler.

No, it is doing exactly what you asked it to do -- turn a non-reference type into a full reference type.

The fault here is Phobos for accepting classes as ranges (range classes are IMO an abomination that should never be used).

> 
> 
>> What might work (and I haven't tried this) is to @disable front, popFront, empty, and I think what will then happen is the compiler will try slicing instead (which should work) and iterate a copy of the array.
>>
>> e.g.:
>>
>> class SharedArray!T
>> {
>>    T[] array;
>>    alias array this;
>>
>>    @disable:
>>       front();
>>       popFront();
>>       empty();
>> }
> 
> This does not work:
> ```
>   Error: no identifier for declarator front()
>   Error: function declaration without return type. (Note that constructors are always named this)
>   Error: no identifier for declarator popFront()
>   Error: function declaration without return type. (Note that constructors are always named this)
>   Error: no identifier for declarator empty()
> ```
> 
> looks like `@disable` expect the full function implementation with body:

Ugh, no, I just forgot to put in the types:

@disable:
   T front();
   void popFront();
   bool empty();

And I figured I'd try it out rather than use you as a REPL ;)

They also need final to avoid putting the non-existent functions in the vtable.

This works (and prints what you want):

class SharedArray(T)
{
   T[] array;
   alias array this;

   @disable final:
      T front();
      void popFront();
      bool empty();
}

-Steve