Must ranges support `r1 = r2;`?

[ag0aep6g]

On 03/24/2018 11:36 PM, Simen Kjærås wrote:
> void main()
> {
>      import std.range;
>      import std.stdio;
> 
>      string s = "foo";
>      auto r = refRange(&s);
> 
>      auto r2 = r;
>      r2 = r2.save;
>          /* Surprising: Effectively just does `s = s;` (i.e., nothing). */
> 
>      r2.popFront();
>      writeln(r); /* Surprising: Prints "oo". */
> }
> 
> None of this is surprising. When you call r2.popFront(), it's being 
> forwarded to s.popFront. That's what refRange does, it's what it's 
> supposed to do, and it's the only thing it could sensibly do.

I think it's surprising. It works as intended and as documented, but 
it's surprising. Note that the behavior is different when you change

     auto r2 = r;
     r2 = r2.save;

to

     auto r2 = r.save;

`.save` actually makes a new slice that can be popped independently from 
`s`. But the assignment throws that independent slice away.

> This is conceptually what refRange does:
> 
> struct RefRange(R) {
>      R* innerRange;
>      this(R* innerRange) {
>          this.innerRange = innerRange;
>      }
>      void popFront() {
>          innerRange.popFront();
>      }
>      @property auto front() {
>          return innerRange.front;
>      }
>      @property bool empty() {
>          return innerRange.empty;
>      }
> }

That's all fine. You're missing the interesting bit: opAssign. `save` 
might also be interesting, but the actual `save` is perfectly fine.

[...]
> So when you do this:
> 
>      string s = "foo";
>      auto r = refRange(&s).group;
>      writeln(r.save);
> 
> r.save is going to create a new Group range, which contains a RefRange, 
> which ultimately points to s. writeln() then repeatedly calls 
> popFront(). This mutates s, as specified above.
That's not how it should be. A saved range should be iterable 
independently from the original. That's the point of `.save`. If that's 
not possible, `.save` should not be there.

You're implying that a saved RefRange points to the same range as the 
original. That's not true. RefRange.save saves the underlying range and 
lets the new RefRange point to that. This is correct. The two ranges can 
be iterated independently.

It's inside `Group` that things get messed up. There you have this 
innocent looking implementation of `save` [1]:

     @property typeof(this) save() {
         typeof(this) ret = this;
         ret._input = this._input.save;
         ret._current = this._current;
         return ret;
     }

The problem is in the line `ret._input = this._input.save;`. That calls 
RefRange.opAssign. If you refactor the code to avoid opAssign, saving 
works properly. For example:

     private this(typeof(_input) inp, typeof(_current) cur)
     {
         this._input = inp; /* Initialization, not assigment. */
         this._current = cur; /* Ditto, but doesn't matter. */
     }
     @property typeof(this) save() {
         return typeof(this)(this._input.save, this._current);
     }

Note that it calls `_input.save` just the same as before. The code only 
avoids RefRange.opAssign.

[...]
> The 'chain', 'choose', and 'cycle' examples are an effect of strings not 
> being random access ranges. I'm uncertain if we should call this a bug, 
> but I agree the behavior is unexpected, so we probably should.

I don't think random access has anything to do with this.

> The splitter example is of course a bug. The crash, that is. The 
> expected behavior is that the first writeln prints [foo, ar], which it 
> does, and that the second print [].

Again, I don't agree. You're effectively saying that saving a RefRange 
isn't possible. If that were so, it simply shouldn't have a `save` 
method. But the `save` method works perfectly fine. It's Phobos 
(accidentally) calling RefRange.opAssign that breaks things.


[1] 
https://github.com/dlang/phobos/blob/3225b19c9bae4b7e8d7a93d2d8c22c94b2a1a6c5/std/algorithm/iteration.d#L1505-L1510